Endurecimiento de seguridad del starter (S-1: 22 tickets)#17
Merged
Conversation
… behind flag [CRITICAL] T-1: the schema seeded [email protected] with a public bcrypt('123456') ADMIN via docker-entrypoint-initdb.d, and the password was documented in the READMEs. POSTGRESDB_PASSWORD also silently defaulted to 'password'. - Drop the hardcoded ADMIN INSERT/setval from db/10.user.sql (schema seeds no users). - Add fail-safe-OFF dev seed db/zz-dev-seed.sh: no-op unless DEV_SEED_ADMIN=true with an explicit BOOTSTRAP_ADMIN_EMAIL + externally-generated bcrypt hash. - Add scripts/gen-admin-hash.sh helper to produce the hash (cost 12). - Make POSTGRESDB_PASSWORD mandatory in docker-compose.db.yml (no :-password default). - Rotate documented credentials in README.md / docs/README_eng.md to seed instructions. - Document the no-secrets-in-schema rule in db/AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[ALTO] T-2: validateCredentials/getUser queried User without deleted:false, so a
soft-deleted account (offboarded admin, compromised user) could still log in and
receive its original permissions.
- validateCredentials: where { email, deleted: false }.
- getUser: switch findByPk -> findOne with { id, deleted: false } so the filter
is actually enforced (findByPk ignores extra where on the PK).
- Update/extend auth.service.spec.ts.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[ALTO] T-3: AbstractCrudService passed req.body verbatim to model.create/update,
letting a caller over-post permissions, deleted, deletedAt or id (privilege
escalation / tampering). This is the base CRUD for every future entity, so the
pattern would propagate.
- AbstractCrudService: optional writableFields allow-list, applied as Sequelize's
{ fields } option on post/put so unlisted keys are dropped.
- UserCrudService: declare USER_WRITABLE_FIELDS (email,name,lastName,permissions,
password) — excludes id/deleted/deletedAt/audit columns even for admins.
- Update specs to assert the fields allow-list is enforced.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[ALTO] T-4: the API had no input validation — req.body/query/params flowed untyped into Sequelize, enabling mass-assignment, NaN pagination and inconsistent errors. This establishes the validation pattern for every entity. - libs/rest-dto: add Zod schemas (userCreateSchema, userUpdateSchema, paginationQuerySchema, idParamSchema) as the single source of truth; input schemas are .strict() so unknown keys are rejected. - apps/api: add validate(schema, source) middleware (Express 5-safe: body/params replaced in place, query exposed on res.locals.query); wire body validation onto user POST/PUT. - Tests for the middleware; document the layer in api & rest-dto AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[ALTO] T-5: refresh rotation copied permissions from the OLD refresh token, and remember=true minted a 365-day token, so a downgraded/revoked user kept elevated access for up to a year. remember was also read truthy from req.body. - API /internal/refresh/rotate re-reads the user (authService.getUser, which filters deleted:false) and returns current email+permissions; revokes the family and 401s if the user is gone/deactivated. - Gateway uses the API-returned claims on rotation, not the stale token claims. - Bound remember lifetime via JWT_REFRESH_REMEMBER_DAYS (default 30); JWT expiry and cookie maxAge share one source (token.service.rememberRefresh*). - Coerce remember to a strict boolean (=== true) in the login controller. - Update gateway specs + AGENTS.md; add env to .env.example/compose.yaml. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[MEDIO] T-6: /login and the auth surface had no throttling/lockout, enabling credential stuffing and password brute force. - Add express-rate-limit limiters: loginRateLimiter keyed by IP+email counting only failed attempts (skipSuccessfulRequests), and a coarse per-IP authRateLimiter over the whole /v1/auth router. - Configurable via LOGIN_RATE_*/AUTH_RATE_* env (sane defaults); relies on the existing trust proxy:1 to see the real client IP behind Nginx. - Add supertest-based tests; document env in .env.example. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[MEDIO] T-7: the security job ran 'npm audit ... || true' with continue-on-error, so vulnerable deps never blocked a merge; and notify needs [lint,test,build,security,docker] referenced build/docker which are commented out, making the graph invalid. - Gate on production deps: npm audit --omit=dev --audit-level=high (blocking), full report still generated+uploaded for visibility. Dev/build-tool advisories churn and aren't shipped, so they're reported, not gated. - Apply non-breaking npm audit fix: clears the production axios high advisory (remaining prod issues are moderate, below the gate). - Fix notify needs -> [lint, test, security]. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[MEDIO] T-8: release.yml ran on every push to main with contents:write and pushed back to main using a token left in the local git config (persist-credentials default true). Any pre-push lifecycle script/dependency could reuse that credential to push to main / cut releases. - Trigger via workflow_dispatch with a bump choice (deliberate, maintainer-gated) instead of automatic push-to-main. - Top-level permissions: contents:read; elevate to write only in the job. - Checkout with persist-credentials:false; inject GITHUB_TOKEN only at the push step via an x-access-token remote URL. - npm version --ignore-scripts so no project/dep scripts run on the privileged runner before the push. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[MEDIO] T-9: docker-compose.db.yml published 5432 on 0.0.0.0, exposing the dev
database to the whole network. Combined with the (now-removed, see T-1) default
password it was a direct DB compromise on an untrusted network.
- Publish on 127.0.0.1:${POSTGRESDB_PORT:-5432}:5432 so only the local host can
reach it. Password was already made mandatory in T-1.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[MEDIO] T-10: app.routes.ts declared no canActivate, so access control relied solely on @if(auth.isLoggedIn()) in templates, and the guards redirected to a non-existent 'unauthorized' route (404 inside the router). - Add a real 'unauthorized' route + page (the guards' redirect target). - Add a guarded example 'profile' route (canActivateFn) demonstrating the protected-route pattern; link it from the logged-in home card. - Add a '**' wildcard redirect to the landing page. - i18n (en/es/ca) for the new pages; document the guard convention in apps/front/AGENTS.md (guards are UX only — backend enforces authz). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[MEDIO] T-11: login.component took redirectUrl from navigation state and passed it to navigateByUrl without validation, allowing redirects to arbitrary internal routes and mishandled '//evil.com'-style values. - Add sanitizeRedirect() util: accepts only same-origin relative paths (single leading '/', no '//', no backslashes, no scheme); returns '' otherwise. - Use it when reading the redirect from navigation state. - Unit tests covering the open-redirect payloads. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[MEDIO] T-12: the auth interceptor added Authorization + withCredentials to EVERY request regardless of target. The moment the app calls a third-party absolute URL (CDN, analytics, avatar), the in-memory access token would leak in the Authorization header. - Attach token/withCredentials only when the request resolves to the app origin or the explicitly-configured API origin (AuthConfig.idpServer). - Don't capture an Authorization response header from an untrusted origin, and don't trigger logout on a third-party 401. - Tests for the cross-origin cases. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…trust headers [MEDIO] T-13: the gateway read the client's x-request-id and used it verbatim as the requestId embedded in the signed internal JWT and forwarded to the API, enabling correlation forgery/collision and log poisoning. Inbound internal headers were also not stripped defensively. - Always mint requestId with randomUUID() (never from the client header). - Strip inbound x-internal-auth / x-request-id at the start of the proxy, and removeHeader them on proxyReq before setting the gateway-minted values. - (API already derives requestId from the verified token claims, not the header — defense in depth already in place there.) Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[MEDIO] T-14: neither the gateway nor the API used helmet, and the front's nginx set no CSP/X-Frame-Options/nosniff. Since the access token is JS-visible by design, a strong CSP is a key XSS mitigation. - Gateway & API: helmet() (HSTS, nosniff, X-Frame-Options: DENY, …); CSP off there since they return JSON only — the document CSP belongs to the front. - Front nginx (nginx/default.conf): CSP (script-src 'self', object-src 'none', frame-ancestors 'none', style-src 'unsafe-inline' for Angular styles), X-Frame-Options: DENY, X-Content-Type-Options: nosniff, Referrer-Policy, HSTS; re-declared in the /assets and /index.html locations (nginx add_header inheritance caveat). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[MEDIO] T-15: /v1/health/detailed and /v1/health/db exposed Node version, platform, pid, memory/cpu and DB host/port/name with no auth, aiding CVE targeting and lateral movement if the network were misconfigured. - Keep /v1/health (liveness) public but minimal (health + timestamp). - Gate /db and /detailed behind requireInternalAuth (gateway-signed token). - Strip fingerprinting: drop nodeVersion/pid/platform and DB host/port/name/ dialect; expose only connection booleans/counters + operational metrics. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[BAJO] T-16: hashPassword passed process.env.HASH_SALT_ROUNDS (a string) to bcrypt.hash, which treats a string as a pre-generated salt; cost was also only 10 with no floor. - parseInt the env and enforce a minimum cost of 12 (modern baseline). - Default HASH_SALT_ROUNDS to 12 in .env.example and compose.yaml. - Tests for numeric coercion and the minimum-cost clamp. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[BAJO] T-17: after a put the base controller re-read with getById({id}) without
deleted:false, allowing read/edit of soft-deleted rows; and page/limit came
straight from req.query (NaN/negative offset, unbounded limit -> memory/DoS).
- put: pre-check the row with deleted:false (404 if gone), and read back with
deleted:false so soft-deleted records stay invisible.
- getAllPaged: parse+clamp via paginationQuerySchema (page>=1, 1<=limit<=100),
falling back to defaults on invalid input; reads res.locals.query when a
validate('query') middleware ran.
- Add abstract-crud.controller spec.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[BAJO] T-18:
- dbLoggingMiddleware logged the response body + client IP + User-Agent on any
5xx (PII / internals leakage) -> log only method/path/status/timestamp.
- sequelizeErrorMiddleware logged the full error object (query fragments/values)
-> log only { name, code }.
- Remove orphan console.log(error) in AbstractCrudController.put.
- pg.connector gated SQL logging on NODE_PRODUCTION only; now fail-safe on
NODE_ENV==='production' || NODE_PRODUCTION==='true' so SQL+params are never
logged in prod if one var is unset.
- Update db-error middleware specs.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[BAJO] T-19: - apps/front/Dockerfile used FROM nginx (=> nginx:latest): non-reproducible builds. Pin to nginx:1.27 (Debian, keeps the groupadd/useradd block working; -alpine would break it). - verifyInternalAuth had no clockTolerance while the internal token TTL is 60s, so clock drift between containers could cause intermittent 401s on legit gateway->API calls. Add clockTolerance: '5s'. - Tests: expired-beyond-tolerance still rejected; small drift now tolerated. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[BAJO] T-20, several: - CORS fail-closed: drop the dead '?? true' fallback; empty CORS_ORIGIN allows no cross-origin (logged as misconfig) instead of risking reflect-any. - Logout revokes the whole refresh family: embed familyId in the refresh JWT and revoke by familyId (fallback to jti), so logout ends the session everywhere. - Public tokens now carry issuer/audience and are verified (issuer+audience+ clockTolerance), so a token minted elsewhere can't be replayed. - express.json with an explicit 100kb limit, applied only to /api/v1/auth; proxy traffic is streamed, never parsed here. - Login always returns a generic 401 instead of forwarding the upstream message (prevents account enumeration). - Tests for iss/aud binding; keep typ-enforcement test meaningful. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[BAJO] T-21:
- auth.service.tokenDecoded tested the signal reference (this.token, always
truthy) instead of its value, so an empty token led to atob/JSON.parse on
undefined and threw. Read this.token(), guard empty, and try/catch -> {}.
- Document the CSRF posture in apps/front/AGENTS.md: state-changing requests
require the Authorization header (non-cookie proof) + SameSite=strict refresh
cookie in prod; no cookie XSRF token, so no withXsrfConfiguration.
- Tests for empty and malformed tokens.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
[BAJO] T-22: the internal EdDSA token carries a requestId but the verifier does not enforce jti/nonce uniqueness, so within its 60s TTL a captured X-Internal-Auth could be replayed. This is acceptable for the starter given the network design — document the assumption explicitly in docs/SECURITY.md and describe how to harden (short-lived jti cache) if a less-trusted service ever crosses the internal boundary. Also refresh SECURITY.md for prior fixes: bounded remember lifetime (JWT_REFRESH_REMEMBER_DAYS), iss/aud on client tokens, authoritative permission re-read on rotation, server-minted requestId + header stripping, and logout-by-family. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Endurecimiento de seguridad del starter (spec S-1)
Implementa los 22 tickets del spec S-1, un commit por ticket. Decisiones tomadas con criterio de seguridad y encaje con la arquitectura del starter (gateway EdDSA + API privada + SPA Angular).
Críticos / Altos
bcrypt('123456')del esquema; seed dev-only fail-safe-off (DEV_SEED_ADMIN, hash externo),POSTGRESDB_PASSWORDobligatorio, credenciales rotadas en los READMEs.getUserfiltrandeleted:false→ las cuentas soft-deleted ya no autentican.fieldsde Sequelize) contra mass-assignment / escalada de privilegios.@dto+ middlewarevalidate()(.strict()), patrón para toda entidad.rememberacotado (JWT_REFRESH_REMEMBER_DAYS, 30d) y booleano estricto.Medios
npm auditbloqueante (prod deps) + grafo de jobsnotifyarreglado.release.ymlporworkflow_dispatch,persist-credentials:false, token sólo en el push,--ignore-scripts.127.0.0.1).unauthorized+ ejemploprofile.sanitizeRedirect, same-origin).requestIdinterno generado en servidor + strip de cabeceras de confianza.helmet(gateway/API) + CSP/HSTS/X-Frame-Options en nginx./v1/health/{db,detailed}tras auth interna y sin fingerprinting (node/pid/DB).Bajos
putrespetadeleted:false+ paginación validada/acotada.NODE_ENVpara SQL log).clockToleranceen verificación interna.familyId,iss/auden tokens, body limit, 401 genérico en login.tokenDecoded+ postura CSRF documentada.docs/SECURITY.md.Validación
npm run lintlimpionpm audit --omit=dev --audit-level=highsin issues en producciónAGENTS.mdpor capa actualizados (db, api, gateway, front, rest-dto)Pasos manuales tras el merge
npm install(nuevas deps:zod,helmet,express-rate-limit,supertestdev)..envdesde.env.example(ahoraPOSTGRESDB_PASSWORDes obligatorio).bash scripts/gen-admin-hash.sh '<pass>'+DEV_SEED_ADMIN=true.npm run dev:db:clean && npm run dev:db) para aplicar el esquema sin el seed antiguo.🤖 Generated with Claude Code