Apply security hardening across authentication, access control, and observability

[rujealfon]

Critical/High fixes:

• Protect /metrics and /health/details with fastify.authenticate
• Add per-route rate limits (5/15min) on login, register, and mobile login
• Remove dev bypass from rate limiting; keep test-only skip
• Fix XFF key generator to parse first IP from forwarded chain
• Add product RBAC: product:create/update/delete:any permissions required for mutations
• Use crypto.timingSafeEqual for mobile API key comparison (prevents timing attack)
• Validate x-request-id header against safe pattern before echoing (prevents header injection)
• Replace CORS placeholder origin with CORS_ORIGIN env var; restrict non-production to localhost

Medium/Low fixes:

• Remove email PII from all audit log metadata; log changedFields keys only
• Generalize registration conflict message to prevent account enumeration
• Normalize bcrypt timing in registerUser to prevent soft-delete oracle
• Add clearCookie security flags (httpOnly, secure, sameSite) on logout
• Enforce COOKIE_SECRET in production; fail fast if not set
• Fix plugin registration order: db before rate-limit, metrics after auth decorator
• Make admin role a system role (isSystemRole: true) to prevent deletion
• Explicit field list in updateRole to prevent latent mass-assignment
• Add password complexity requirement (uppercase, lowercase, digit)
• Add index on audit_logs.user_id + migration 0008

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01S3NT5TALAVtn916JREYXwQ

[rujealfon]

@claude review

[claude]

Claude finished @rujealfon's task in 3m 7s —— View job

---

PR Review: Security Hardening

Tasks

• Gather context and understand request
• Read changed files and analyze security fixes
• Provide comprehensive review feedback

---

This PR delivers a solid set of security hardening changes across authentication, access control, observability, and data protection. The overall direction is correct and most fixes are well-implemented. Below are the key findings — grouped by severity.

---

Critical / Must-Fix

1. /metrics has auth but no authorization — any authenticated user can read Prometheus data

src/plugins/metrics.ts:20 adds preValidation: [fastify.authenticate], but any valid JWT holder can now read Prometheus metrics (heap stats, request rates, event-loop lag, custom counters). Metrics endpoints are a common recon vector.

Recommendation: Add a permission check (e.g. fastify.requirePermission('metrics:read:any')) and assign it only to super-admin in the seed, or use a separate bearer token outside the user JWT system.

Fix this →

2. loginUser has a timing oracle — email existence can be inferred by response time

src/modules/auth/services/auth.service.ts:73-82 — the PR correctly adds DUMMY_HASH timing normalization to registerUser's dead-account path, but loginUser still takes the fast path when an email is not found:

const user = await db.query.users.findFirst(...)
if (!user)
  throw new UnauthorizedError('Invalid email or password')  // fast — no bcrypt
const valid = await bcrypt.compare(body.password, user.passwordHash)  // slow

An attacker can enumerate valid emails by measuring response time differences across the login endpoint (which is the much higher-value target vs. register).

Recommendation:

const user = await db.query.users.findFirst(...)
const valid = user ? await bcrypt.compare(body.password, user.passwordHash) : (await bcrypt.compare(body.password, DUMMY_HASH), false)
if (!user || !valid)
  throw new UnauthorizedError('Invalid email or password')

Fix this →

3. Rate limit allowList: ['127.0.0.1'] can be bypassed by forging X-Forwarded-For

src/plugins/rate-limit.ts:14 — after the XFF fix, the key generator now returns the first IP from the X-Forwarded-For chain. The @fastify/rate-limit allowList is checked against the generated key. This means any client can send X-Forwarded-For: 127.0.0.1 and completely bypass rate limiting — including the tight 5/15min auth limits.

The XFF key-generation fix (parsing first IP) is correct, but the allowList cannot safely reference 127.0.0.1 when the key comes from a client-controlled header.

Recommendation: Remove allowList entirely (the auth routes already have per-route limits; the global 100/15min limit on 127.0.0.1 is low value), or only allow the actual TCP connection IP to bypass limits rather than the derived key.

Fix this →

---

High / Should-Fix

4. Empty CORS_ORIGIN in production silently blocks all CORS requests

src/plugins/cors.ts:10-12 — in production, if CORS_ORIGIN is not set, configuredOrigins becomes [], which with @fastify/cors means all cross-origin requests are rejected. Unlike COOKIE_SECRET, there's no startup-time validation. A misconfigured deploy would silently break the frontend with no clear error.

Recommendation: Add a fast-fail check mirroring the cookie plugin:

if (fastify.config.NODE_ENV === 'production' && configuredOrigins.length === 0) {
  throw new Error('CORS_ORIGIN must be set in production')
}

Fix this →

---

Approved Fixes — Well Implemented

The following changes are correct and well-reasoned:

• timingSafeEqual for mobile API key (src/modules/auth/routes/index.ts:33-36) — proper length check before compare prevents the panic on mismatched buffer lengths; handles missing header gracefully via Buffer.from('').
• REQUEST_ID_PATTERN validation (src/common/hooks/request-id.ts:5,10) — [\w\-]{1,64} correctly rejects header injection payloads while accepting UUIDs and common ID formats.
• Bcrypt timing normalization in registerUser (src/modules/auth/services/auth.service.ts:40) — DUMMY_HASH at cost 12 correctly normalizes the no-dead-account path vs. the dead-account bcrypt path.
• Generalized conflict messages — conflict error no longer echoes the email back, closing the account enumeration oracle on register.
• clearCookie security flags — matching httpOnly, secure, sameSite on logout correctly prevents a browser from persisting the cookie.
• COOKIE_SECRET enforcement in production (src/plugins/cookie.ts:6-8) — correct fast-fail pattern.
• Product RBAC (src/contract/schemas/products.ts:38,50,64) — switching from auth: true to permission: 'product:*:any' is the right approach; 403 responses are now declared.
• Explicit field list in updateRole (src/modules/roles/services/role.service.ts:54-57) — prevents latent mass-assignment if new columns are added to the schema.
• Admin as system role (src/db/seed.ts:9) — prevents admin from being deleted via the normal role-deletion endpoint.
• allowList: ['127.0.0.1'] → test skip (src/plugins/rate-limit.ts:8-9) — dev environments now get rate-limited, which is the correct posture.
• Audit log PII removal — email removed from metadata; changedFields: Object.keys(body) logs field names not values.
• audit_logs_user_id_idx migration — index on user_id is correct for the access patterns.
• Password complexity regex (src/modules/auth/schemas/index.ts:5-6) — enforces uppercase, lowercase, and digit; applied only on register (login correctly skips it).
• Plugin registration order (src/app.ts:83-84) — metricsPlugin after authDecorator ensures fastify.authenticate is available.

---

Minor Observations

• updateRole TypeScript return type — the function's catch branch calls throwIfRoleNameConflict (which returns never), but TypeScript still infers the function can return undefined. This is pre-existing and not introduced here, but it's worth a non-null assertion or explicit never cast in the catch block.
• /health/details has no 403 declared — since only fastify.authenticate is used (no permissions check), a 403 can't occur in practice. Clean.
• CORS_ORIGIN documented in .env.example — the comment correctly marks it as required for production. Pair with the startup validation above to close the loop.

---

Summary: 3 security issues to address before merge (metrics authz, login timing, XFF allowList bypass) and 1 high-confidence deployment footgun (CORS validation). The remaining 17 fixes are correct and well-implemented. The PR represents a materially stronger security posture — these four items are the gaps.
|

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ff5164c7b7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Backfill admin system-role status

On existing databases this change never takes effect: seedRoles() inserts SEED_ROLES with onConflictDoNothing(), and the only new migration is the audit-log index, so an already-seeded admin row keeps is_system_role = false even after deploy or a seed rerun. In those deployments the admin role remains deletable/modifiable despite the hardening intent; add a data migration or upsert update for the built-in role.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Reuse the password policy for admin-created users

Adding complexity only to /auth/register leaves the other account-creation path unchanged: /api/v1/users still uses createUserBodySchema with only min/max password validation, so an admin can create users with password123 even though self-registration now rejects it. Move this regex into a shared password schema and use it for both creation schemas.

Useful? React with 👍 / 👎.

[rujealfon]

@claude review

[claude]

Claude finished @rujealfon's task in 2m 18s —— View job

---

PR Review: Security Hardening

Tasks

• Gather context and understand request
• Read changed files and analyze security fixes
• Provide comprehensive review feedback

---

This PR delivers a strong set of security hardening changes. Most fixes are well-implemented and the overall direction is correct. I found 3 critical issues and 1 high issue that should be resolved before merge.

---

Critical / Must-Fix

1. loginUser timing oracle — email enumeration via login endpoint

src/modules/auth/services/auth.service.ts:75-76 — The PR correctly adds DUMMY_HASH timing normalization to registerUser, but loginUser still exits early when the email is not found, without running bcrypt.compare:

const user = await db.query.users.findFirst(...)
if (!user)
  throw new UnauthorizedError('Invalid email or password')  // fast — no bcrypt
const valid = await bcrypt.compare(body.password, user.passwordHash)  // slow

The login endpoint is the higher-value enumeration target vs. register. An attacker can reliably distinguish valid from invalid emails by measuring response time.

Fix:

const user = await db.query.users.findFirst({ where: and(eq(users.email, body.email), isNull(users.deletedAt)) })
const valid = user
  ? await bcrypt.compare(body.password, user.passwordHash)
  : (await bcrypt.compare(body.password, DUMMY_HASH), false)
if (!user || !valid)
  throw new UnauthorizedError('Invalid email or password')

Fix this →

---

2. /metrics has authentication but no authorization

src/plugins/metrics.ts:20 adds preValidation: [fastify.authenticate], but any valid JWT holder (a regular user role) can read Prometheus metrics — heap stats, request rates, event-loop lag, error counters. Metrics endpoints are a common recon vector that can reveal internal behavior and be used to time attacks.

Fix: Add a permission gate:

preValidation: [fastify.authenticate, (req, reply) => fastify.requirePermission(req, reply, 'metrics:read:any')],

And assign metrics:read:any only to super-admin in the seed. Alternatively, use a separate bearer token completely outside the user JWT system.

Fix this →

---

3. allowList: ['127.0.0.1'] can be bypassed by forging X-Forwarded-For

src/plugins/rate-limit.ts:14 — The XFF key-generator fix correctly parses the first IP from the forwarded chain. However, @fastify/rate-limit checks allowList against the generated key (not the raw TCP connection IP). Since the key comes from a client-controlled header, any client can send X-Forwarded-For: 127.0.0.1 and completely bypass rate limiting — including the tight 5/15min auth limits added on login and register.

The dev-only skip (lines 8-9) is the correct fix for that concern; the remaining allowList entry is now a vulnerability.

Fix: Remove allowList: ['127.0.0.1'] from the plugin config entirely.

Fix this →

---

High / Should-Fix

4. Empty CORS_ORIGIN silently breaks all CORS in production with no error

src/plugins/cors.ts:5-8 — In production, if CORS_ORIGIN is not configured, configuredOrigins becomes [] and @fastify/cors rejects all cross-origin requests. Unlike COOKIE_SECRET, there's no startup-time validation — a misconfigured deploy silently breaks the frontend with CORS errors, not a clear server-side message.

Fix: Add a fast-fail check mirroring the cookie plugin:

if (fastify.config.NODE_ENV === 'production' && configuredOrigins.length === 0) {
  throw new Error('CORS_ORIGIN must be set in production')
}

Fix this →

---

Clarifications on Codex Review Comments

Codex P2: "Backfill admin system-role status" — This is already handled. Migration 0009_backfill_admin_system_role.sql runs UPDATE "roles" SET "is_system_role" = true WHERE "name" = 'admin', which correctly updates the existing row on all deployed databases. The Codex concern is addressed.

Codex P2: "Reuse the password policy for admin-created users" — This is also already fixed. src/modules/users/schemas/index.ts:29 uses passwordSchema (not a bare z.string()) in createUserBodySchema, so admin-created users are subject to the same complexity requirement as self-registered users. No action needed.

---

Well-Implemented Fixes

• timingSafeEqual for mobile API key (src/modules/auth/routes/index.ts:35) — length check before compare is correct; Buffer.from('') for missing header handles the null case cleanly.
• REQUEST_ID_PATTERN (src/common/hooks/request-id.ts) — [\w\-]{1,64} rejects injection payloads, accepts UUIDs.
• DUMMY_HASH in registerUser (src/modules/auth/services/auth.service.ts:40) — correct bcrypt cost-12 normalization for the no-dead-account path.
• Generalized conflict messages — no longer echoes the email, closing the register enumeration oracle.
• clearCookie security flags — matching httpOnly, secure, sameSite on logout prevents cookie persistence.
• COOKIE_SECRET production enforcement (src/plugins/cookie.ts:6-8) — fast-fail is the right pattern.
• Product RBAC (src/contract/schemas/products.ts) — permission: 'product:*:any' on mutations; 403 responses declared in contract.
• Explicit field list in updateRole (src/modules/roles/services/role.service.ts:54-57) — prevents mass-assignment if new columns are added.
• Admin as system role + migration 0009 — seed sets isSystemRole: true, migration backfills existing rows, deleteRole / updateRole both check the flag.
• Audit log PII removal — email removed from metadata; changedFields: Object.keys(body) logs field names only.
• Password complexity regex (src/common/schemas/index.ts:3-9) — shared passwordSchema correctly enforced on both register and admin createUser.
• Plugin registration order (src/app.ts) — metricsPlugin after authDecorator ensures fastify.authenticate is available.
• audit_logs_user_id_idx migration — correct index for the primary access pattern.
• test-only rate-limit skip — dev environments now get rate-limited; the explicit comment explains why tests skip it.

---

Minor Observations

• updateRole return type — TypeScript still infers the function can return undefined because the catch branch's call to throwIfRoleNameConflict (which is typed : never) isn't always recognized as exhaustive by the compiler. Pre-existing; not introduced here.
• /health/details has no 403 declared — since preValidation: [fastify.authenticate] is used with no permission check, 403 can't occur. The schema is clean.

---

Summary: 3 critical security issues to address before merge (login timing oracle, metrics authorization gap, XFF allowList bypass) and 1 high-confidence deployment footgun (CORS startup validation). The remaining ~18 fixes are correct and well-implemented. Both Codex P2 comments are already addressed in the current PR state.

[rujealfon]

@claude code review and fix remaining issues

[claude]

Claude finished @rujealfon's task in 2m 13s —— View job

---

Fixed: 4 Remaining Security Issues

Tasks

• Gather context and understand request
• Read relevant files
• Fix: loginUser timing oracle — always run bcrypt even when user not found
• Fix: /metrics authorization gap — add metrics:read:any permission gate
• Fix: allowList: ['127.0.0.1'] XFF bypass — remove the allowList entry
• Fix: Empty CORS_ORIGIN silent failure — add startup validation
• Commit and push changes

---

All 4 issues from the prior review have been addressed in commit f64a882.

Changes Made

1. loginUser timing oracle — src/modules/auth/services/auth.service.ts

The early-exit on unknown email let attackers enumerate valid addresses by measuring response time. Now always runs bcrypt.compare — against DUMMY_HASH when no user is found — mirroring the existing fix already applied to registerUser:

const valid = user
  ? await bcrypt.compare(body.password, user.passwordHash)
  : (await bcrypt.compare(body.password, DUMMY_HASH), false)

if (!user || !valid)
  throw new UnauthorizedError('Invalid email or password')

2. /metrics authorization gap — src/plugins/metrics.ts + src/common/constants/index.ts + src/db/seed.ts

Added PERMISSIONS.METRICS.READ_ANY = 'metrics:read:any' to the constants, seeded the permission (super-admin automatically inherits it via the SEED_PERMISSIONS.map(...) aggregate), and gated the route:

preValidation: [fastify.authenticate, fastify.requirePermission(PERMISSIONS.METRICS.READ_ANY)],

Regular admin and user roles are not assigned this permission, so Prometheus data is restricted to super-admin only.

3. allowList: ['127.0.0.1'] XFF bypass — src/plugins/rate-limit.ts

Removed the allowList entry. Because the key generator reads from the client-controlled X-Forwarded-For header, any client could send X-Forwarded-For: 127.0.0.1 to skip all rate limits including the tight 5/15min auth endpoint limits. The test-only skip on lines 8–9 already handles the legitimate dev concern.

4. Empty CORS_ORIGIN production footgun — src/plugins/cors.ts

Added a fast-fail startup check matching the COOKIE_SECRET pattern in cookie.ts:

if (fastify.config.NODE_ENV === 'production' && configuredOrigins.length === 0) {
  throw new Error('CORS_ORIGIN must be set in production')
}

A misconfigured deploy now fails loudly at startup rather than silently rejecting all CORS requests.

---

|

[rujealfon]

@claude code review

[claude]

Claude finished @rujealfon's task in 3m 38s —— View job

---

PR Code Review

Tasks

• Gather context and understand request
• Read changed files and analyze PR diff
• Provide comprehensive review feedback

---

This is the third review pass on this PR. All four issues from the prior reviews have been addressed. I found one new security issue introduced by the fixes, plus one minor observation.

---

Critical / Must-Fix

Double bcrypt in the new-user registration path creates an enumeration timing oracle

src/modules/auth/services/auth.service.ts:43-46 — The fix to registerUser adds a DUMMY_HASH compare for the active-conflict path (line 23), which is correct. But lines 43–44 then add a second dummy compare immediately before the real bcrypt.hash for the new-user path:

// New-user path (lines 43–46):
await bcrypt.compare(body.password, DUMMY_HASH)  // ← unnecessary: adds ~100ms
const passwordHash = await bcrypt.hash(body.password, 12)  // real hash: ~100ms

This makes the timing for each path:

Path	Time
Active conflict → 409	1 bcrypt ≈ 100ms
Dead account conflict → 409	1 bcrypt ≈ 100ms
Dead account reactivation → 201	1 bcrypt ≈ 100ms
New user → 201	2 bcrypt ≈ 200ms

An attacker who probes the register endpoint can distinguish "this email is new to the system" (200ms) from "this email is already registered" (100ms) — which is exactly the email-enumeration oracle the fix was designed to prevent.

The bcrypt.hash at cost 12 takes approximately the same time as bcrypt.compare at cost 12, so it already normalizes the new-user path against the conflict paths. The dummy compare on lines 43–44 is not needed and should be removed:

// Remove these two lines:
// Normalize timing vs the dead-account path above.
await bcrypt.compare(body.password, DUMMY_HASH)

const passwordHash = await bcrypt.hash(body.password, 12)  // keep this

Fix this →

---

Minor Observation

Migration 0009 has a misleading name

migrations/0009_backfill_admin_system_role.sql contains only SELECT 1 with a comment explaining the change was intentionally not made. The PR description still says "Make admin role a system role" which contradicts the current code (isSystemRole: false for admin in seed.ts:9). The design decision to not make admin a system role is fine, but the migration name will create confusion in migration history. Renaming the migration or updating the PR description would clarify intent.

---

Approved Changes

All previously reviewed fixes are correct in the current state:

• loginUser timing normalization — Always runs bcrypt.compare against DUMMY_HASH when no user is found. The comma-expression (await bcrypt.compare(...), false) correctly evaluates and discards the compare result. ✓
• timingSafeEqual for mobile API key — SHA-256 digests of both values before compare produces fixed-length buffers; handles missing header via empty string. ✓
• Request-ID injection prevention — /^[\w\-]{1,64}$/ rejects injection payloads, accepts UUIDs. ✓
• Rate limit allowList removed — XFF-derived key can no longer be spoofed to bypass limits. ✓
• /metrics auth + metrics:read:any permission gate — super-admin inherits all SEED_PERMISSIONS including the new entry; regular users cannot access Prometheus data. ✓
• CORS_ORIGIN startup validation — Fails fast in production if unset, mirroring the COOKIE_SECRET pattern. ✓
• clearCookie security flags — httpOnly, secure, sameSite on logout match the login cookie settings. ✓
• Product RBAC — product:create/update/delete:any permissions on contract, 403 responses declared, seeded to admin and super-admin, read permission seeded to user role. ✓
• updateRole explicit field list — Prevents mass-assignment from future schema additions. ✓
• Audit log PII removal — Email stripped from all log metadata; changedFields: Object.keys(body) logs field names only. ✓
• Shared passwordSchema — Applied to both registerBodySchema and createUserBodySchema (admin-created users); login schema correctly left with plain min/max. ✓
• Plugin registration order — metricsPlugin after authDecorator; dbPlugin before rateLimitPlugin. ✓
• audit_logs_user_id_idx migration — Correct index for the primary audit-log access pattern. ✓
• COOKIE_SECRET production enforcement — Fast-fail at startup. ✓
• Bcrypt timing in registerUser active-conflict path — 1 dummy compare normalizes active-conflict timing against dead-account-conflict timing. ✓

---

Summary: One new timing bug was introduced by the registerUser fix — remove the dummy bcrypt.compare on lines 43–44 before the real bcrypt.hash. Everything else is in good shape.

[rujealfon]

@claude code review

[claude]

Claude finished @rujealfon's task in 2m 54s —— View job

---

PR Code Review

Tasks

• Gather context and understand request
• Read changed files and analyze current state
• Provide comprehensive review feedback

---

This is the fourth and final review pass. All issues from all prior review passes have been resolved. No new security issues were introduced. The PR is ready to merge.

---

Status of Previously Flagged Issues

Issue	Status
loginUser timing oracle (email enumeration via login)	✅ Fixed — DUMMY_HASH compare always runs
/metrics authorization gap (any JWT holder could read Prometheus data)	✅ Fixed — requirePermission(PERMISSIONS.METRICS.READ_ANY) added
allowList: ['127.0.0.1'] XFF bypass	✅ Fixed — allowList removed entirely
CORS_ORIGIN empty → silent CORS failure in production	✅ Fixed — startup validation throws on empty origin
Double bcrypt in new-user registration path (last review)	✅ Fixed — unnecessary bcrypt.compare(DUMMY_HASH) before bcrypt.hash removed

---

Timing Analysis — registerUser (verified correct)

Path	bcrypt ops	Result
Active email conflict	1 compare (DUMMY_HASH)	409
Dead account, correct password	1 compare (dead hash)	201 reactivation
Dead account, wrong password	1 compare (dead hash)	409
New user	1 hash at cost 12	201

All four paths involve exactly one cost-12 bcrypt operation, so response times are normalized. ✓

---

Minor Observations (non-blocking)

Comma expression in loginUser (src/modules/auth/services/auth.service.ts:81)

: (await bcrypt.compare(body.password, DUMMY_HASH), false)

This is a JavaScript comma expression — it runs the bcrypt.compare for its timing side-effect and discards the result in favor of false. It is semantically correct and the existing comment explains the intent, but the comma-expression syntax is obscure enough that a future developer might not recognize it. An equivalent that's slightly more readable:

: await bcrypt.compare(body.password, DUMMY_HASH).then(() => false)

Either form is fine; this is a style preference, not a correctness issue.

RouteSchema interface is missing rateLimit (src/contract/types.ts)

The RouteSchema generic interface (lines 5–20) doesn't include a rateLimit field, but the RouteMap inline type (line 22–34) does. Since all contract schemas use satisfies RouteMap rather than implementing RouteSchema directly, the per-route rate limits are correctly wired through rpc.ts:45. The RouteSchema interface appears to be legacy that isn't enforced by current consumers — not a bug.

---

Confirmed Correct Across All Changed Files

• loginUser — comma expression is subtle but correct; always runs bcrypt regardless of user existence ✓
• registerUser — timing correctly normalized across all four paths with exactly one bcrypt operation each ✓
• timingSafeEqual for mobile API key — SHA-256 digests ensure fixed-length buffers; empty-string fallback for missing header ✓
• Request-ID injection prevention — /^[\w\-]{1,64}$/ rejects injection payloads; matches UUIDs ✓
• /metrics route — authenticate + requirePermission(METRICS.READ_ANY) in sequence; metrics:read:any seeded only to super-admin ✓
• Rate limit plugin — no allowList, test-only skip with explanatory comment, XFF key generator reads first IP ✓
• Per-route rate limits — 5/15min on register, login, and mobileLogin via config: { rateLimit: route.rateLimit } in rpc.ts:45 ✓
• CORS plugin — production fast-fail, localhost fallback for non-production ✓
• Cookie plugin — COOKIE_SECRET required in production, httpOnly/secure/sameSite on parseOptions ✓
• clearCookie flags — httpOnly, secure, sameSite match the login cookie settings ✓
• Product RBAC — permission: 'product:*:any' on mutations; 403 declared in contract; read routes remain auth: true ✓
• updateRole explicit field list — prevents mass-assignment from future schema additions ✓
• System role guard — deleteRole and updateRole both check isSystemRole; super-admin flag threads correctly ✓
• Password complexity — shared passwordSchema applied to both registerBodySchema and createUserBodySchema (admin-created users); loginBodySchema correctly left with plain min(1) ✓
• Audit log PII — email removed from all metadata; changedFields: Object.keys(body) logs field names only ✓
• Conflict messages — generalized to prevent account enumeration on register ✓
• Plugin registration order — db → rateLimitPlugin; metricsPlugin after authDecorator ✓
• audit_logs_user_id_idx migration — correct index for the primary audit-log access pattern ✓
• Migration 0009 — intentional no-op; admin is not a system role by design (admin role lacks role:delete:any permission, so admins can't delete roles regardless) ✓
• Codex P2: backfill admin system-role — moot; admin is intentionally not a system role ✓
• Codex P2: password policy for admin-created users — already fixed; src/modules/users/schemas/index.ts:29 uses passwordSchema ✓

---

Verdict: LGTM — no blocking issues. Ready to merge.
|

[rujealfon]

@claude code review again comprehensively

[claude]

Claude finished @rujealfon's task in 5m 8s —— View job

---

Comprehensive Code Review (Pass 5)

Tasks

• Gather context and understand request
• Read all changed files and analyze PR diff
• Review security fixes for correctness and completeness
• Review new functionality and test coverage
• Provide comprehensive review feedback

---

This is the fifth and most thorough review pass. All four issues from pass 4 remain fixed. I found one new medium-severity regression introduced by the timingSafeEqual fix, plus three missing test gaps for the security changes.

---

Medium / Should-Fix

mobileLogin timing-safe fix introduces a bypass when MOBILE_API_KEY is empty

src/modules/auth/routes/index.ts:35-37

The original string comparison caught the case of an absent header vs. an empty key:

// OLD — correctly rejects: undefined !== '' → throws 403
if (request.headers['x-mobile-api-key'] !== request.server.config.MOBILE_API_KEY)
  throw new ForbiddenError(...)

The new timingSafeEqual implementation hashes both sides to SHA-256 digests before comparing. This is correct when both values are non-empty. But when MOBILE_API_KEY is '' (e.g. MOBILE_API_KEY= in the env file) and the header is absent:

const provided = createHash('sha256').update(undefined ?? '').digest()  // SHA256('')
const expected = createHash('sha256').update('').digest()               // SHA256('')
timingSafeEqual(provided, expected)  // → true! — 200 returned

The app won't start without MOBILE_API_KEY (it's in the JSON schema required array) but an empty-string value passes schema validation. The original comparison caught this (undefined !== ''); the new one doesn't.

Fix: Add a guard before the comparison — mirror the COOKIE_SECRET pattern:

mobileLogin: async ({ body, request, reply }) => {
  if (!request.server.config.MOBILE_API_KEY)
    throw new ForbiddenError('Mobile login is restricted to mobile clients')
  const provided = createHash('sha256').update(request.headers['x-mobile-api-key'] as string ?? '').digest()
  const expected = createHash('sha256').update(request.server.config.MOBILE_API_KEY).digest()
  if (!timingSafeEqual(provided, expected))
    throw new ForbiddenError('Mobile login is restricted to mobile clients')
  ...
}

Or equivalently, add a startup fast-fail in app.ts or config/schema.ts that rejects an empty MOBILE_API_KEY.

Fix this →

---

Low / Test Coverage Gaps

1. No test for password complexity requirement

src/tests/modules/auth.test.ts — the register tests check for min-length (short) but not for the new regex requirement. A password like password123 (meets 8-char minimum, no uppercase) should return 400. As-is, the test suite wouldn't catch a regression if the passwordSchema regex were accidentally removed.

Missing test:

it('returns 400 for password without uppercase', async () => {
  const res = await app.inject({
    method: 'POST', url: '/api/v1/auth/register',
    payload: { email: 'alice@example.com', password: 'password123' },
  })
  expect(res.statusCode).toBe(400)
})

Fix this →

2. No tests for /metrics RBAC

src/plugins/metrics.ts:21 — the metrics endpoint gained authentication and a metrics:read:any permission gate, but there are no tests covering this. Missing coverage:

• 401 without a JWT
• 403 with a regular user JWT (user role only has user:read:own, user:update:own, product:read:any)
• 403 with an admin JWT (admin role has no metrics:read:any)
• 200 with a super-admin JWT (gets all permissions including metrics:read:any)

Fix this →

3. Product mutations lack a 403 test for the user role

src/tests/modules/products.test.ts — all product mutation tests use registerAdminAndLogin. After the product RBAC changes, a regular user role hitting POST /api/v1/products should receive 403 (user has product:read:any but not product:create:any). No test verifies this enforcement.

Fix this →

---

All Prior Issues: Confirmed Fixed ✓

Issue	Status
loginUser timing oracle (email enumeration via bcrypt skip)	✅ DUMMY_HASH compare always runs via .then(() => false)
/metrics authorization gap (any JWT holder reads Prometheus data)	✅ requirePermission(PERMISSIONS.METRICS.READ_ANY) added
allowList: ['127.0.0.1'] XFF bypass	✅ allowList removed entirely
CORS_ORIGIN empty → silent CORS failure in production	✅ Startup validation throws on empty origin
Double bcrypt in new-user registration path	✅ Unnecessary bcrypt.compare(DUMMY_HASH) before bcrypt.hash removed

---

Timing Verification (all paths confirmed)

registerUser — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
Active email conflict	1 compare (DUMMY_HASH)	409
Dead account, correct password	1 compare (dead hash) → reactivate	201
Dead account, wrong password	1 compare (dead hash)	409
New user	1 hash at cost 12	201

loginUser — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
User found, correct password	1 compare (user hash)	200
User found, wrong password	1 compare (user hash)	401
No user found	1 compare (DUMMY_HASH) → .then(() => false)	401

---

Well-Implemented Fixes (confirmed correct across all changed files)

• loginUser comma expression replaced with .then(() => false) — semantically equivalent but more readable ✓
• timingSafeEqual mobile API key — SHA-256 digests produce fixed-length buffers; ?? '' handles absent header ✓ (modulo the empty MOBILE_API_KEY edge case above)
• REQUEST_ID_PATTERN = /^[\w\-]{1,64}$/ — rejects injection payloads, accepts UUIDs ✓
• Rate limit allowList removed — XFF key is client-controlled and cannot safely appear in allowList ✓
• Per-route rate limits 5/15min — wired through config: { rateLimit: route.rateLimit } in rpc.ts:45 ✓
• CORS_ORIGIN startup validation — fails fast in production, localhost fallback in dev/test ✓
• clearCookie security flags — httpOnly, secure, sameSite match login setCookie options ✓
• COOKIE_SECRET production enforcement — fast-fail pattern matches CORS_ORIGIN guard ✓
• Product RBAC — permission: 'product:*:any' on mutations, 403 declared in contract, seeded to admin/super-admin; user has read-only ✓
• Explicit field list in updateRole — prevents mass-assignment from future schema columns ✓
• System role guard — super-admin is isSystemRole: true; admin is isSystemRole: false by intentional design ✓
• Audit log PII removal — email stripped from all logAudit calls; changedFields: Object.keys(body) logs field names only ✓
• Shared passwordSchema — applied to both registerBodySchema and createUserBodySchema; login schema correctly uses z.string().min(1) ✓
• Plugin registration order — metricsPlugin after authDecorator; dbPlugin before rateLimitPlugin ✓
• audit_logs_user_id_idx migration — correct index for the primary audit-log query pattern ✓
• Migration 0009_noop_admin_role_not_system.sql — intentional no-op with explanatory comment; name accurately reflects its content ✓
• RouteSchema interface now includes rateLimit — src/contract/types.ts:17 aligns the interface with the RouteMap type ✓

---

Minor Observations (non-blocking)

• development environments are now rate-limited — The skip moved from development to test only. Developers hitting the auth endpoints (login/register at 5/15min) will be rate-limited in dev. Intentional per PR description, but worth noting in the README or .env.example for local setup guidance.
• updateRole TypeScript return type — pre-existing issue; catch branch's throwIfRoleNameConflict is typed : never but TypeScript still infers the function may return undefined. Not introduced here.

---

Summary: One medium regression to fix (MOBILE_API_KEY empty-string bypass in timingSafeEqual path), three test coverage gaps (password complexity, /metrics RBAC, product 403 for user role). The 20+ security fixes from prior passes are all confirmed correct in the current state.

[rujealfon]

@claude code review again comprehensively

[claude]

Claude finished @rujealfon's task in 3m 37s —— View job

---

Comprehensive Code Review (Pass 6)

Tasks

• Gather context and understand request
• Read current state of all changed files
• Check status of Pass 5 findings (MOBILE_API_KEY bypass, 3 test gaps)
• Comprehensive review of all security fixes
• Provide final review feedback

---

Pass 5 Findings — All Resolved ✓

Issue	Status
MOBILE_API_KEY empty-string bypass in timingSafeEqual path	✅ Fixed — if (!request.server.config.MOBILE_API_KEY) throw ForbiddenError guard added (auth/routes/index.ts:33-34)
No test for password complexity requirement	✅ Fixed — auth.test.ts:62-69 tests that password123 (meets length, no uppercase) returns 400
No tests for /metrics RBAC	✅ Fixed — metrics.test.ts covers 401 (no token), 403 (user), 403 (admin), 200 (super-admin)
Product mutations lack 403 test for user role	✅ Fixed — products.test.ts:90-141 covers POST/PATCH/DELETE returning 403 for user role

---

Current State Analysis

Timing normalization — fully correct across all paths

registerUser — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
Active email conflict	1 compare (DUMMY_HASH)	409
Dead account + correct password	1 compare (dead.passwordHash)	201 reactivation
Dead account + wrong password	1 compare (dead.passwordHash)	409
New user	1 hash at cost 12	201

loginUser — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
User found, correct password	1 compare (user.passwordHash)	200
User found, wrong password	1 compare (user.passwordHash)	401
No user found	1 compare (DUMMY_HASH) → .then(() => false)	401

---

New Findings (Minor / Non-Blocking)

1. MOBILE_API_KEY empty string — no startup fast-fail in production

src/config/schema.ts:40 — JSON Schema required means the key must be present but not non-empty. MOBILE_API_KEY= (empty string) passes schema validation and the app starts. The handler guard at auth/routes/index.ts:33 correctly throws 403 for every mobile login attempt in that state, so there's no security bypass. However, COOKIE_SECRET and CORS_ORIGIN both have loud startup fast-fails in production (cookie.ts:6-8, cors.ts:10-12). A misconfigured MOBILE_API_KEY= would silently disable mobile login without a clear server-side message.

Observation: Consider adding startup validation mirroring the other plugins:

if (fastify.config.NODE_ENV === 'production' && !fastify.config.MOBILE_API_KEY) {
  throw new Error('MOBILE_API_KEY must be set in production')
}

This is a low-severity deployment footgun, not a security bypass.

2. /health/details accessible to any authenticated user — information disclosure

src/modules/health/routes/index.ts:49 — preValidation: [fastify.authenticate] with no permission gate means any JWT holder (regular user role) can read: heapUsed, rssBytes, eventLoopDelay, eventLoopUtilized, and underPressure. This reveals internal system state that could help an attacker time load-based attacks or understand memory pressure. The PR correctly added a metrics:read:any permission gate to the Prometheus endpoint for the same reason.

Observation: The data exposed here is less actionable than Prometheus metrics, but it is real system internals. Consider either:

• Adding a permission gate (e.g., health:read:details assigned to admin+), or
• Stripping memory detail fields from the 200 response for non-admin callers

This is non-blocking but the asymmetry (Prometheus gated, health details not gated) is worth noting.

---

All Prior Issues — Confirmed Fixed ✓

• loginUser timing oracle — .then(() => false) pattern always runs bcrypt regardless of whether user exists (auth.service.ts:81) ✓
• registerUser double bcrypt — unnecessary bcrypt.compare(DUMMY_HASH) before bcrypt.hash removed; each path has exactly one bcrypt op ✓
• /metrics authorization gap — authenticate + requirePermission(PERMISSIONS.METRICS.READ_ANY); permission seeded only to super-admin (metrics.ts:21, seed.ts:33,37) ✓
• allowList: ['127.0.0.1'] XFF bypass — allowList removed entirely from rate-limit.ts ✓
• CORS_ORIGIN startup validation — fast-fail throws in production if empty (cors.ts:10-12) ✓
• MOBILE_API_KEY empty-string bypass — !request.server.config.MOBILE_API_KEY guard before timingSafeEqual (auth/routes/index.ts:33-34) ✓
• timingSafeEqual for mobile API key — SHA-256 digests ensure fixed-length buffers; ?? '' handles absent header; length-independent ✓
• REQUEST_ID_PATTERN — /^[\w\-]{1,64}$/ rejects injection payloads, accepts UUIDs (request-id.ts:5) ✓
• COOKIE_SECRET production enforcement — fast-fail at cookie.ts:6-8 ✓
• clearCookie security flags — httpOnly, secure, sameSite on logout match login setCookie options ✓
• Product RBAC — permission: 'product:*:any' on mutations; 403 declared in contract; seeded to admin/super-admin; user has read-only ✓
• Explicit field list in updateRole — prevents mass-assignment from future schema columns (role.service.ts:54-57) ✓
• System role guard — super-admin is isSystemRole: true; admin is isSystemRole: false by intentional design; both deleteRole and updateRole check the flag ✓
• Audit log PII removal — email stripped from all logAudit calls; changedFields: Object.keys(body) logs field names only (users/routes/index.ts:41) ✓
• Shared passwordSchema — applied to registerBodySchema (auth/schemas/index.ts:6) and createUserBodySchema (users/schemas/index.ts:29); login schema correctly uses z.string().min(1) ✓
• Plugin registration order — metricsPlugin after authDecorator; dbPlugin before rateLimitPlugin; correct in app.ts:67-84 ✓
• audit_logs_user_id_idx migration — correct index for the primary audit-log access pattern ✓
• Migration 0009 — intentional no-op; name accurately reflects content; admin is not a system role by design (admin role lacks role:delete:any permission so admins can't delete roles regardless) ✓
• Per-route rate limits — 5/15min on register, login, and mobileLogin via config: { rateLimit: route.rateLimit } in rpc.ts:45 ✓
• RouteSchema interface includes rateLimit — contract/types.ts:17 aligned with RouteMap ✓
• Conflict messages generalized — no longer echoes email, preventing account enumeration on register ✓
• Password complexity regex — ^(?=.*[a-z])(?=.*[A-Z])(?=.*\d) with min(8), max(72) ✓

---

Verdict: LGTM — no blocking issues. Two minor observations worth tracking (startup validation for MOBILE_API_KEY, health endpoint info disclosure) but neither prevents merge. This is a materially stronger security posture across all 36 changed files.

[rujealfon]

@claude code review

[claude]

Claude finished @rujealfon's task in 3m 2s —— View job

---

PR Code Review (Pass 7)

Tasks

• Gather context and understand request
• Read current state of all changed files
• Check if Pass 6 minor observations were addressed
• Comprehensive review of all security fixes
• Provide final review feedback

---

This is the seventh review pass. All issues from all prior passes have been resolved, including both "minor observations" flagged in Pass 6. No new blocking issues found.

---

Pass 6 Minor Observations — Both Resolved ✓

/health/details permission gate (Pass 6 flagged: info disclosure to any JWT holder)

src/modules/health/routes/index.ts:51 now uses:

preValidation: [fastify.authenticate, fastify.requirePermission(PERMISSIONS.HEALTH.READ_DETAILS)],

health:read:details is seeded to admin and super-admin only (seed.ts:40), and health.test.ts now verifies all four cases: 401 (no token), 403 (user role), 200 (admin), 200 (super-admin). The 403 response is declared in the contract schema. ✓

MOBILE_API_KEY startup fast-fail (Pass 6 flagged: no production startup validation)

src/plugins/mobile-auth.ts now throws at startup if MOBILE_API_KEY is falsy in production:

if (fastify.config.NODE_ENV === 'production' && !fastify.config.MOBILE_API_KEY)
  throw new Error('MOBILE_API_KEY must be set in production')

Consistent with the COOKIE_SECRET and CORS_ORIGIN patterns. ✓

---

Status of All Prior Issues

Issue	Pass	Status
loginUser timing oracle (email enumeration via bcrypt skip)	1	✅ .then(() => false) always runs bcrypt
/metrics authorization gap (any JWT holder reads Prometheus data)	1	✅ requirePermission(PERMISSIONS.METRICS.READ_ANY)
allowList: ['127.0.0.1'] XFF bypass	1	✅ allowList removed entirely
CORS_ORIGIN empty → silent CORS failure in production	1	✅ Startup validation added
Double bcrypt in new-user registration path	3	✅ Unnecessary compare before bcrypt.hash removed
MOBILE_API_KEY empty-string bypass in timingSafeEqual	5	✅ Guard added before hash comparison
No test for password complexity	5	✅ auth.test.ts:62-69 covers password123 → 400
No /metrics RBAC tests	5	✅ metrics.test.ts covers 401/403/403/200
Product mutations no 403 for user role	5	✅ products.test.ts:90-141 covers all three
/health/details info disclosure (any JWT holder)	6	✅ requirePermission(PERMISSIONS.HEALTH.READ_DETAILS)
MOBILE_API_KEY no production startup validation	6	✅ mobile-auth.ts fast-fail added

---

Timing Verification (current state, confirmed correct)

registerUser — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
Active email conflict	1 compare (DUMMY_HASH)	409
Dead account + correct password	1 compare (dead.passwordHash)	201 reactivation
Dead account + wrong password	1 compare (dead.passwordHash)	409
New user	1 hash at cost 12	201

loginUser — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
User found, correct password	1 compare (user.passwordHash)	200
User found, wrong password	1 compare (user.passwordHash)	401
No user found	1 compare (DUMMY_HASH) .then(() => false)	401

---

All Security Fixes Confirmed Correct

• loginUser — .then(() => false) always runs bcrypt regardless of user existence ✓
• registerUser — four paths each have exactly one bcrypt op; no double-compare regression ✓
• timingSafeEqual for mobile API key — SHA-256 digests ensure fixed-length buffers; ?? '' fallback for missing header; !MOBILE_API_KEY guard prevents empty-key match ✓
• REQUEST_ID_PATTERN = /^[\w\-]{1,64}$/ — rejects injection payloads, accepts UUIDs ✓
• /metrics route — authenticate + requirePermission(METRICS.READ_ANY); metrics:read:any seeded only to super-admin ✓
• /health/details route — authenticate + requirePermission(HEALTH.READ_DETAILS); health:read:details seeded to admin + super-admin; 403 declared in contract ✓
• Rate limit plugin — no allowList, test-only skip with comment, XFF key generator reads first IP ✓
• Per-route rate limits — 5/15min on register, login, mobileLogin via config: { rateLimit } in rpc.ts:45 ✓
• CORS plugin — production fast-fail on empty CORS_ORIGIN; localhost fallback in dev/test ✓
• Cookie plugin — COOKIE_SECRET required in production; httpOnly/secure/sameSite on parseOptions ✓
• Mobile auth plugin — MOBILE_API_KEY required in production; registered correctly after config ✓
• clearCookie flags — httpOnly, secure, sameSite on logout match login setCookie settings ✓
• Product RBAC — permission: 'product:*:any' on mutations; 403 declared in contract; seeded to admin/super-admin; user role has read-only ✓
• updateRole explicit field list — prevents mass-assignment from future schema additions ✓
• System role guard — super-admin is isSystemRole: true; both deleteRole and updateRole check the flag ✓
• Audit log PII removal — email stripped from all logAudit calls; changedFields: Object.keys(body) logs field names only ✓
• Shared passwordSchema — applied to registerBodySchema and createUserBodySchema; loginBodySchema correctly uses z.string().min(1) ✓
• Plugin registration order — metricsPlugin after authDecorator; mobileAuthPlugin before authDecorator; dbPlugin before rateLimitPlugin ✓
• audit_logs_user_id_idx migration — correct index for primary audit-log access pattern ✓
• Migration 0009 — intentional no-op; name accurately reflects content ✓
• RouteSchema interface includes rateLimit — contract/types.ts aligned with RouteMap ✓

---

Verdict: LGTM — no blocking issues. All 11 previously flagged issues resolved across 7 passes. Ready to merge.

[rujealfon]

@claude code review

[claude]

Claude finished @rujealfon's task in 4m 24s —— View job

---

PR Code Review (Pass 8)

Tasks

• Gather context and understand request
• Read current state of all changed files
• Comprehensive review of security fixes and new code
• Provide final review feedback

---

This is the eighth review pass. All eleven issues flagged across passes 1–7 remain fixed. No new blocking issues found. Full pass below.

---

Status: All Prior Issues Resolved ✓

Issue	Pass	Status
loginUser timing oracle (email enumeration via bcrypt skip)	1	✅ .then(() => false) always runs bcrypt
/metrics authorization gap (any JWT holder reads Prometheus data)	1	✅ requirePermission(PERMISSIONS.METRICS.READ_ANY)
allowList: ['127.0.0.1'] XFF bypass	1	✅ allowList removed entirely
CORS_ORIGIN empty → silent CORS failure in production	1	✅ Startup validation throws on empty origin
Double bcrypt in new-user registration path	3	✅ Unnecessary bcrypt.compare(DUMMY_HASH) before bcrypt.hash removed
MOBILE_API_KEY empty-string bypass in timingSafeEqual	5	✅ Guard added (!MOBILE_API_KEY → ForbiddenError)
No test for password complexity	5	✅ auth.test.ts:62-69 covers password123 → 400
No /metrics RBAC tests	5	✅ metrics.test.ts covers 401/403 (user)/403 (admin)/200 (super-admin)
Product mutations missing 403 test for user role	5	✅ products.test.ts:90-141 covers POST/PATCH/DELETE → 403
/health/details info disclosure to any JWT holder	6	✅ requirePermission(PERMISSIONS.HEALTH.READ_DETAILS) added
MOBILE_API_KEY no production startup validation	6	✅ mobile-auth.ts fast-fail on empty key in production

---

Timing Verification — registerUser (confirmed correct)

Path	bcrypt ops	Result
Active email conflict	1 compare (DUMMY_HASH)	409
Dead account + correct password	1 compare (dead.passwordHash)	201 reactivation
Dead account + wrong password	1 compare (dead.passwordHash)	409
New user (+ concurrent race → PG 23505)	1 hash at cost 12	201 (or 409 on race)

Timing Verification — loginUser (confirmed correct)

Path	bcrypt ops	Result
User found, correct password	1 compare (user.passwordHash)	200
User found, wrong password	1 compare (user.passwordHash)	401
No user found	1 compare (DUMMY_HASH) .then(() => false)	401

---

Comprehensive File-by-File Verification

Auth service (src/modules/auth/services/auth.service.ts)

• DUMMY_HASH at $2b$12$... — valid bcrypt cost-12 hash; bcrypt.compare against it takes ~100ms matching a normal cost-12 compare ✓
• registerUser four paths each execute exactly one cost-12 bcrypt operation ✓
• loginUser .then(() => false) pattern: bcrypt.compare runs for its timing side-effect regardless of result; false is returned when no user exists ✓
• Conflict messages generalized; email never echoed back ✓

Mobile login (src/modules/auth/routes/index.ts:33-40)

• !MOBILE_API_KEY guard at top prevents SHA256('') === SHA256('') bypass ✓
• createHash('sha256').update(header ?? '').digest() produces a fixed 32-byte buffer regardless of header length — no timingSafeEqual panic on length mismatch ✓
• logAudit no longer includes email in metadata ✓

Request-ID hook (src/common/hooks/request-id.ts)

• /^[\w\-]{1,64}$/ rejects newline, angle-bracket, and other injection payloads; accepts UUIDs and common ID formats ✓
• Silently falls back to randomUUID() on mismatch — no error leaked to client ✓

Rate limit plugin (src/plugins/rate-limit.ts)

• allowList removed — XFF-derived key can no longer be spoofed ✓
• Skip moved from development to test — dev environments are now rate-limited ✓
• XFF key generator: Array.isArray(xff) ? xff[0] : xff handles both single and multi-header forms; .split(',')[0].trim() extracts leftmost IP ✓

CORS plugin (src/plugins/cors.ts)

• Production fast-fail on empty configuredOrigins mirrors the COOKIE_SECRET pattern ✓
• Non-production fallback to ['http://localhost:3000', 'http://localhost:5173'] is safe — app.inject() in tests bypasses CORS entirely ✓
• Test environment: CORS_ORIGIN defaults to '', configuredOrigins is [], startup check skips (non-production), origin = localhost fallback ✓

Cookie plugin (src/plugins/cookie.ts)

• Production fast-fail on empty COOKIE_SECRET ✓
• Falls back to JWT_SECRET in non-production; warning comment removed (was misleading)

Logout clearCookie (src/modules/auth/routes/index.ts:53-59)

• httpOnly: true, secure: NODE_ENV === 'production', sameSite: 'strict' matches setCookie options exactly — browsers clear the right cookie ✓

Metrics plugin (src/plugins/metrics.ts)

• preValidation: [fastify.authenticate, fastify.requirePermission(PERMISSIONS.METRICS.READ_ANY)] ✓
• metrics:read:any in SEED_PERMISSIONS and assigned only to super-admin via ROLE_PERMISSIONS ✓
• Plugin registered after authDecorator in app.ts so fastify.authenticate is available ✓

Health details route (src/modules/health/routes/index.ts:44-51)

• preValidation: [fastify.authenticate, fastify.requirePermission(PERMISSIONS.HEALTH.READ_DETAILS)] ✓
• health:read:details seeded to admin and super-admin; user role excluded ✓
• 401 and 403 declared in contract responses ✓

Health controller (src/modules/health/controllers/health.controller.ts)

• Optional chaining on pressure.memoryUsage?.() and pressure.isUnderPressure?.() with process-level fallbacks — correct defensive pattern ✓
• processMemory.rss → rssBytes mapping is correct ✓

Mobile auth plugin (src/plugins/mobile-auth.ts)

• Registered after jwtPlugin but before authDecorator — correct since it performs only a startup check, no decorators ✓

Product RBAC (src/contract/schemas/products.ts, src/db/seed.ts)

• permission: 'product:create/update/delete:any' on mutation routes ✓
• 403 declared in all mutation contracts ✓
• product:read:any seeded to user role; create/update/delete to admin + super-admin ✓
• Read route retains auth: true with no 403 (correct — auth but no permission gate) ✓

Role service (src/modules/roles/services/role.service.ts:54-57)

• Explicit { name, description } field list prevents mass-assignment if new columns are added to the schema ✓
• isSystemRole guard checked before update and delete ✓

Password complexity (src/common/schemas/index.ts, src/modules/auth/schemas/index.ts, src/modules/users/schemas/index.ts)

• Shared passwordSchema applied to both registerBodySchema and createUserBodySchema ✓
• loginBodySchema correctly uses z.string().min(1) to allow existing users with legacy passwords to log in ✓

Audit log PII (src/modules/auth/routes/index.ts, src/modules/users/routes/index.ts)

• email removed from all logAudit metadata ✓
• changedFields: Object.keys(body) logs field names only, not values ✓

Per-route rate limits (src/contract/schemas/auth.ts, src/plugins/rpc.ts:45)

• rateLimit: { max: 5, timeWindow: '15 minutes' } on register, login, mobileLogin ✓
• config: { rateLimit: route.rateLimit } in rpc.ts wires the config to @fastify/rate-limit ✓

Plugin registration order (src/app.ts)

• redisPlugin → dbPlugin → rateLimitPlugin (rate limit can now use db decorators if needed) ✓
• metricsPlugin after authDecorator ✓

Schema and types (src/contract/types.ts)

• rateLimit and tags added to RouteSchema interface, aligning it with RouteMap ✓

Migrations

• 0008_audit_logs_user_id_idx.sql: correct index on user_id for audit log access patterns ✓
• 0009_noop_admin_role_not_system.sql: intentional no-op with explanatory comment; name accurately reflects content ✓

Permissions test (src/tests/modules/permissions.test.ts)

• Replaced hardcoded 15 with SEED_PERMISSIONS.length — auto-adjusts as permissions are added ✓
• Expected keys derived from SEED_PERMISSIONS.map(...) — no more manual list to maintain ✓

Test coverage — all new security paths are exercised:

• auth.test.ts: password complexity (password123 → 400), all test passwords updated to Password123 ✓
• metrics.test.ts: 401/403 (user)/403 (admin)/200 (super-admin) ✓
• health.test.ts: 401/403 (user)/200 (admin)/200 (super-admin) ✓
• products.test.ts: user-role 403 on POST/PATCH/DELETE; admin token for existing mutation tests ✓
• audit-logs.test.ts: product mutation tests now use adminToken; email removed from metadata assertion ✓
• fixtures/index.ts: all default passwords updated to Password123 ✓

---

Minor Observations (non-blocking, genuinely new)

Rate limiting now applies in development — The skip moved from development to test, so developers using NODE_ENV=development (the default) will hit the 5/15min auth endpoint limits when testing their own flows with Postman or similar tools. This is intentional per the PR description, but a note in .env.example or the README would prevent surprises. Non-blocking — the behavior is documented in the PR.

DUMMY_HASH bcrypt cost and legacy user hashes — If an old user was registered when bcrypt was running at cost 10, their dead.passwordHash compares in ~10ms vs DUMMY_HASH's ~100ms. This creates a minor timing signal distinguishing "email was registered at a lower cost setting" from "email was never registered." Not actionable without a rehash-on-login strategy, and not introduced by this PR — noting for completeness.

---

Verdict: LGTM — no blocking issues. All 11 previously flagged issues confirmed resolved. Ready to merge.

[rujealfon]

@claude code review

[claude]

Claude finished @rujealfon's task in 3m 26s —— View job

---

PR Code Review (Pass 9)

Tasks

• Gather context and understand request
• Read current state of all changed files
• Comprehensive review of security fixes and new code
• Provide final review feedback

---

This is the ninth review pass. All eleven issues flagged across passes 1–8 remain fixed. No new blocking issues found.

---

Status: All Prior Issues Resolved ✓

Issue	Pass	Status
loginUser timing oracle (email enumeration via bcrypt skip)	1	✅ .then(() => false) always runs bcrypt
/metrics authorization gap (any JWT holder reads Prometheus data)	1	✅ requirePermission(PERMISSIONS.METRICS.READ_ANY)
allowList: ['127.0.0.1'] XFF bypass	1	✅ allowList removed entirely
CORS_ORIGIN empty → silent CORS failure in production	1	✅ Startup validation throws on empty origin
Double bcrypt in new-user registration path	3	✅ Unnecessary bcrypt.compare(DUMMY_HASH) before bcrypt.hash removed
MOBILE_API_KEY empty-string bypass in timingSafeEqual	5	✅ !MOBILE_API_KEY guard before hash comparison
No test for password complexity	5	✅ auth.test.ts:62-69 covers password123 → 400
No /metrics RBAC tests	5	✅ metrics.test.ts covers 401/403 (user)/403 (admin)/200 (super-admin)
Product mutations missing 403 test for user role	5	✅ products.test.ts:90-141 covers POST/PATCH/DELETE → 403
/health/details info disclosure to any JWT holder	6	✅ requirePermission(PERMISSIONS.HEALTH.READ_DETAILS) added
MOBILE_API_KEY no production startup validation	6	✅ mobile-auth.ts fast-fail on empty key in production

---

Timing Verification — All Paths Correct

registerUser (auth.service.ts:15-71) — each path executes exactly one cost-12 bcrypt operation:

Path	DB queries	bcrypt ops	Result
Active email conflict	1 (active found)	1 compare (DUMMY_HASH)	409
Dead account + correct password	2 (active miss, dead found)	1 compare (dead.passwordHash)	201 reactivation
Dead account + wrong password	2 (active miss, dead found)	1 compare (dead.passwordHash)	409
New user	2 (active miss, dead miss)	1 hash at cost 12	201

Note: path 1 has one fewer DB query than paths 2-4 due to early exit. This is a minor pre-existing timing artifact (DB round-trips are ~1ms vs bcrypt's ~100ms), not the primary oracle concern addressed by this PR.

loginUser (auth.service.ts:74-87) — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
User found, correct password	1 compare (user.passwordHash)	200
User found, wrong password	1 compare (user.passwordHash)	401
No user found	1 compare (DUMMY_HASH) .then(() => false)	401

---

Minor Observations (non-blocking)

Rate limiting skips ALL non-production environments, not just test

src/plugins/rate-limit.ts:8-9:

if (fastify.config.NODE_ENV !== 'production')
  return

The code comment ("Keep local development and integration tests unthrottled") correctly reflects this. Prior review passes (5 and 6) described this as "skip moved from development to test only" which was inaccurate — both development and test remain unrate-limited. The behavior itself is correct for the use case: local dev and CI both need to work without hitting limits. No action needed, but the prior pass commentary should not be relied on.

XFF key generator remains susceptible to rate-limit evasion via IP rotation

src/plugins/rate-limit.ts:18-22: The leftmost XFF IP is used as the rate-limit key. Without a trusted-proxy list, a client can rotate through different forged X-Forwarded-For IPs to evade per-IP limits. This is a known trade-off — the rightmost approach (trusting the proxy's appended IP) requires knowing the proxy's IP. The allowList bypass (a hard fix) was correctly removed in pass 1. What remains is the softer spoofing risk inherent to XFF-based keying. Worth documenting in the deployment guide if there's a known proxy IP; not a blocker.

Dead account reactivation writes outside a transaction

src/modules/auth/services/auth.service.ts:33-38: The UPDATE users and INSERT user_roles for account reactivation are two separate writes without a wrapping transaction. If the role insert fails after the user is un-deleted, the user would be reactivated with no roles. Pre-existing; not introduced by this PR.

---

All Security Fixes Confirmed Correct

• loginUser — .then(() => false) pattern always runs bcrypt; false is returned when no user exists (auth.service.ts:79-81) ✓
• registerUser — four paths each have exactly one cost-12 bcrypt operation; no double-compare regression ✓
• timingSafeEqual for mobile API key — !MOBILE_API_KEY guard prevents SHA256('') === SHA256(''); SHA-256 digests ensure fixed-length buffers regardless of header length; ?? '' handles absent header (auth/routes/index.ts:33-40) ✓
• REQUEST_ID_PATTERN = /^[\w\-]{1,64}$/ — rejects injection payloads; accepts UUIDs; silently falls back to randomUUID() (request-id.ts:5,10) ✓
• /metrics route — authenticate + requirePermission(METRICS.READ_ANY); metrics:read:any seeded only to super-admin (metrics.ts:21, seed.ts:38) ✓
• /health/details route — authenticate + requirePermission(HEALTH.READ_DETAILS); seeded to admin + super-admin; 401 and 403 declared in contract schema (health/routes/index.ts:51, seed.ts:40-43) ✓
• Rate limit plugin — no allowList, non-production skip with accurate comment, XFF key generator reads leftmost IP (rate-limit.ts) ✓
• Per-route rate limits — 5/15min on register, login, mobileLogin via config: { rateLimit: route.rateLimit } in rpc.ts:45 ✓
• CORS plugin — production fast-fail on empty CORS_ORIGIN; localhost fallback for non-production; app.inject() bypasses CORS in tests (cors.ts) ✓
• Cookie plugin — COOKIE_SECRET required in production; httpOnly/secure/sameSite on parseOptions (cookie.ts:6-8,12-15) ✓
• Mobile auth plugin — MOBILE_API_KEY required in production (mobile-auth.ts:5-6) ✓
• clearCookie flags — httpOnly, secure, sameSite on logout match login setCookie options (auth/routes/index.ts:50-55) ✓
• Product RBAC — permission: 'product:*:any' on mutations; 403 declared in all mutation contracts; seeded to admin/super-admin; user role has read-only (products.ts, seed.ts:39-55) ✓
• updateRole explicit field list — only name and description updated; prevents mass-assignment from future schema additions (role.service.ts:54-57) ✓
• System role guard — super-admin is isSystemRole: true; admin is isSystemRole: false by design (admin lacks role:delete:any); both deleteRole and updateRole check the flag (role.service.ts:49-52,71-72) ✓
• Audit log PII removal — email stripped from all logAudit calls; changedFields: Object.keys(body) logs field names only (users/routes/index.ts:41) ✓
• Shared passwordSchema — applied to both registerBodySchema (auth/schemas/index.ts:5-7) and createUserBodySchema (users/schemas/index.ts:29); loginBodySchema correctly uses z.string().min(1) to allow pre-existing users with legacy passwords ✓
• Plugin registration order — metricsPlugin after authDecorator; mobileAuthPlugin before authDecorator; redisPlugin → dbPlugin → rateLimitPlugin (app.ts:59-86) ✓
• audit_logs_user_id_idx migration — correct index for the primary audit-log access pattern ✓
• Migration 0009 — intentional no-op; name reflects content; admin is not a system role by design ✓
• RouteSchema interface includes rateLimit and tags — contract/types.ts:17-18 aligned with RouteMap ✓
• Conflict messages generalized — register conflict no longer echoes email, preventing account enumeration ✓
• Test coverage — auth complexity, metrics RBAC (401/403/403/200), health details (401/403/200/200), product RBAC (user 403 on POST/PATCH/DELETE), permissions count derived from SEED_PERMISSIONS.length ✓

---

Verdict: LGTM — no blocking issues. Three minor observations documented above (non-production rate-limit scope, XFF spoofing, dead-account reactivation transaction) are all pre-existing and not introduced by this PR. Ready to merge.

[rujealfon]

@claude code review

[claude]

Claude finished @rujealfon's task in 3m 17s —— View job

---

PR Code Review (Pass 10)

Tasks

• Gather context and understand request
• Read current state of all changed files
• Check for any new issues since Pass 9
• Provide comprehensive review feedback

---

All eleven issues flagged across passes 1–9 remain fixed. This pass found one new positive fix (dead-account reactivation transaction, previously a pre-existing finding) and no new blocking issues.

---

New Since Pass 9 — Net Improvements

Dead account reactivation now wrapped in a transaction ✓

src/modules/auth/services/auth.service.ts:33-38 — Pass 9 flagged this as a pre-existing issue: the UPDATE to un-delete the user and the INSERT to re-assign the user role were two separate writes, so a failure between them would leave the user reactivated with no roles. This has been fixed:

await db.transaction(async (tx) => {
  await tx.update(users).set({ deletedAt: null, deletedBy: null }).where(eq(users.id, dead.id))
  const [userRole] = await tx.select({ id: roles.id }).from(roles).where(eq(roles.name, ROLES.USER)).limit(1)
  if (userRole)
    await tx.insert(userRoles).values({ userId: dead.id, roleId: userRole.id }).onConflictDoNothing()
})

Both writes are now atomic. ✓

ROLES.USER constant used instead of hardcoded 'user'

Both the dead-account reactivation path and the new-user registration path now use ROLES.USER from constants rather than the string literal 'user'. Consistent with the codebase convention. ✓

---

Status of All Prior Issues

Issue	Pass	Status
loginUser timing oracle (email enumeration via bcrypt skip)	1	✅ .then(() => false) always runs bcrypt
/metrics authorization gap (any JWT holder reads Prometheus data)	1	✅ requirePermission(PERMISSIONS.METRICS.READ_ANY)
allowList: ['127.0.0.1'] XFF bypass	1	✅ allowList removed entirely
CORS_ORIGIN empty → silent CORS failure in production	1	✅ Startup validation throws on empty origin
Double bcrypt in new-user registration path	3	✅ Unnecessary bcrypt.compare(DUMMY_HASH) before bcrypt.hash removed
MOBILE_API_KEY empty-string bypass in timingSafeEqual	5	✅ !MOBILE_API_KEY guard before hash comparison
No test for password complexity	5	✅ auth.test.ts:62-69 covers password123 → 400
No /metrics RBAC tests	5	✅ metrics.test.ts covers 401/403 (user)/403 (admin)/200 (super-admin)
Product mutations missing 403 test for user role	5	✅ products.test.ts:90-141 covers POST/PATCH/DELETE → 403
/health/details info disclosure to any JWT holder	6	✅ requirePermission(PERMISSIONS.HEALTH.READ_DETAILS) added
MOBILE_API_KEY no production startup validation	6	✅ mobile-auth.ts fast-fail on empty key in production
Dead account reactivation writes outside a transaction	9	✅ Wrapped in db.transaction(async (tx) => {...})

---

Timing Verification — Current State

registerUser — each path executes exactly one cost-12 bcrypt operation:

Path	DB queries	bcrypt ops	Result
Active email conflict	1 (active found)	1 compare (DUMMY_HASH)	409
Dead account + correct password	2 (active miss, dead found)	1 compare (dead.passwordHash)	201 reactivation
Dead account + wrong password	2 (active miss, dead found)	1 compare (dead.passwordHash)	409
New user	2 (active miss, dead miss)	1 hash at cost 12	201

loginUser — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
User found, correct password	1 compare (user.passwordHash)	200
User found, wrong password	1 compare (user.passwordHash)	401
No user found	1 compare (DUMMY_HASH) .then(() => false)	401

Note: the active-conflict path in registerUser has one fewer DB round-trip than the dead-account and new-user paths. At ~1-2ms per query vs ~100ms for bcrypt, this is a ~1-2% timing signal — not actionable without restructuring the lookup logic, and not introduced by this PR.

---

Minor Observations (non-blocking)

XFF empty-string edge case — src/plugins/rate-limit.ts:20: If a client sends X-Forwarded-For: (header present but empty), the expression ''.split(',')[0].trim() returns ''. Since ?? only triggers on null/undefined, the rate-limit key becomes the empty string '' rather than request.ip. Multiple clients sending this header would share one rate-limit bucket. Unlikely in practice — well-behaved proxies don't forward empty XFF headers — and not a security bypass. Not actionable without a regex validation on the extracted IP.

Rate limiting scope — src/plugins/rate-limit.ts:8: The skip applies to all NODE_ENV !== 'production' environments (both test and development). The comment correctly describes this ("Keep local development and integration tests unthrottled"). Intentional, per PR description.

---

All Security Fixes Confirmed Correct

• loginUser — .then(() => false) always runs bcrypt; valid = false when no user exists (auth.service.ts:81-83) ✓
• registerUser — four paths each have exactly one cost-12 bcrypt operation; dead-account reactivation now atomic in transaction ✓
• timingSafeEqual for mobile API key — SHA-256 digests ensure fixed-length buffers; ?? '' handles absent header; !MOBILE_API_KEY guard prevents SHA256('') === SHA256('') bypass (auth/routes/index.ts:33-40) ✓
• REQUEST_ID_PATTERN = /^[\w\-]{1,64}$/ — rejects injection payloads; accepts UUIDs; silently falls back to randomUUID() (request-id.ts:4,9) ✓
• /metrics route — authenticate + requirePermission(METRICS.READ_ANY); seeded only to super-admin (metrics.ts:19, seed.ts:38) ✓
• /health/details route — authenticate + requirePermission(HEALTH.READ_DETAILS); seeded to admin + super-admin; 401 and 403 declared in contract (health/routes/index.ts:47, seed.ts:40) ✓
• Rate limit plugin — no allowList; non-production skip; XFF key reads leftmost IP ✓
• Per-route rate limits — 5/15min on register, login, mobileLogin via config: { rateLimit } in rpc.ts:43 ✓
• CORS plugin — production fast-fail on empty origin; localhost fallback for non-production (cors.ts:5-15) ✓
• Cookie plugin — COOKIE_SECRET required in production; httpOnly/secure/sameSite on parseOptions (cookie.ts:3-5) ✓
• Mobile auth plugin — MOBILE_API_KEY required in production at startup (mobile-auth.ts:4-5) ✓
• clearCookie flags — httpOnly, secure, sameSite on logout match login setCookie options (auth/routes/index.ts:50-55) ✓
• Product RBAC — permission: 'product:*:any' on mutations; 403 declared in contract; seeded to admin/super-admin; user role has read-only (products.ts, seed.ts:51-55) ✓
• updateRole explicit field list — only name and description updated; prevents mass-assignment (role.service.ts:54-57) ✓
• System role guard — super-admin is isSystemRole: true; both deleteRole and updateRole check the flag ✓
• Audit log PII removal — email stripped from all logAudit calls; changedFields: Object.keys(body) logs field names only (users/routes/index.ts:38) ✓
• Shared passwordSchema — applied to both registerBodySchema and createUserBodySchema; loginBodySchema correctly uses z.string().min(1) ✓
• Plugin registration order — metricsPlugin after authDecorator; mobileAuthPlugin before authDecorator; dbPlugin before rateLimitPlugin (app.ts:76-88) ✓
• audit_logs_user_id_idx migration — correct index for the primary audit-log access pattern ✓
• Migration 0009 — intentional no-op; name reflects content; admin is not a system role by design ✓
• RouteSchema interface includes rateLimit and tags — contract/types.ts:17-18 aligned with RouteMap ✓
• Conflict messages generalized — register conflict no longer echoes email ✓
• Test coverage — auth complexity, metrics RBAC, health details, product RBAC (user 403), permissions count derived from SEED_PERMISSIONS.length ✓

---

Verdict: LGTM — no blocking issues. The dead-account reactivation transaction fix (flagged in Pass 9 as pre-existing) has been added, making this a marginally stronger PR than the last pass. Ready to merge.

[rujealfon]

@claude run /security-review

[claude]

Claude finished @rujealfon's task in 4m 12s —— View job

---

Security Review (Pass 11)

Tasks

• Gather context and understand request
• Read all changed files
• Analyze security findings
• Post comprehensive security review

---

All 12 issues flagged across passes 1–10 remain fixed. This pass found one genuine security improvement (rate limit key) introduced since Pass 10, plus two minor observations. No new blocking issues.

---

Key Security Change Since Pass 10

Rate limit key upgraded from XFF header to request.ip with TRUST_PROXY support

src/plugins/rate-limit.ts:18 + src/app.ts:60

This is a significant improvement that was not covered in prior review passes. The previous implementation (even after the allowList bypass was fixed in Pass 1) still used:

// BEFORE (all prior passes)
keyGenerator: request => request.headers['x-forwarded-for'] as string ?? request.ip

This meant any client could rotate X-Forwarded-For values (X-Forwarded-For: 1.2.3.4, then X-Forwarded-For: 5.6.7.8, etc.) to evade per-IP rate limits, including the tight 5/15min auth endpoint limits. IP rotation is a standard brute-force evasion technique.

The new implementation:

// AFTER
keyGenerator: request => request.ip

With the new TRUST_PROXY environment variable, request.ip is derived by Fastify's built-in proxy-addr logic, which:

• Without TRUST_PROXY: returns the TCP connection IP (cannot be spoofed from the network)
• With TRUST_PROXY=<ip>: trusts XFF only from that upstream proxy IP, then walks the XFF chain correctly

This closes the IP-rotation evasion vector entirely when TRUST_PROXY is configured to match the actual reverse proxy. ✓

The allowList correctly uses a path predicate (not an IP allowList) so health checks bypass rate limiting by URL, not a forgeable header:

allowList: request => request.url === '/health/live' || request.url === '/health/ready'

---

Minor Observations (non-blocking)

1. readDotEnvValue does not strip inline comments

src/app.ts:48-57 — The custom .env parser strips surrounding quotes but not inline comments:

// .env line: TRUST_PROXY=127.0.0.1 # my proxy
// readDotEnvValue returns: '127.0.0.1 # my proxy'
// parseTrustProxy returns: '127.0.0.1 # my proxy'
// Fastify trustProxy: '127.0.0.1 # my proxy'  ← invalid

The @fastify/env plugin (which uses dotenv) correctly strips inline comments for every other env var, so this is an inconsistency specific to TRUST_PROXY. In practice, .env files rarely use inline comments on value lines, so this is unlikely to cause problems. Non-blocking.

2. registerBodySchema uses a deliberately loose password constraint

src/modules/auth/schemas/index.ts:5 — registerBodySchema uses z.string().min(1).max(72) rather than the shared passwordSchema (which requires uppercase + lowercase + digit). This is intentional: it enables dead-account re-registration with a legacy weak password to flow through HTTP validation and reach the service layer, which intentionally bypasses the complexity check for that path.

The reactivates a legacy account with a correct weak password test (auth.test.ts) documents and validates this behavior. The downside is that the OpenAPI spec for POST /api/v1/auth/register doesn't surface the real password constraints to API consumers — they only see minLength: 1, not the actual complexity requirement. A .meta({ description: 'Must contain uppercase, lowercase, and digit for new accounts' }) on the field would close the documentation gap without changing behavior.

---

All Prior Issues — Confirmed Fixed ✓

Issue	Pass	Status
loginUser timing oracle (email enumeration via bcrypt skip)	1	✅ .then(() => false) always runs bcrypt
/metrics authorization gap (any JWT holder reads Prometheus data)	1	✅ requirePermission(PERMISSIONS.METRICS.READ_ANY)
allowList: ['127.0.0.1'] XFF bypass	1	✅ allowList removed; now path-based function
CORS_ORIGIN empty → silent CORS failure in production	1	✅ Startup validation throws on empty origin
Double bcrypt in new-user registration path	3	✅ Unnecessary bcrypt.compare before bcrypt.hash removed
MOBILE_API_KEY empty-string bypass in timingSafeEqual	5	✅ !MOBILE_API_KEY guard before hash comparison
No test for password complexity	5	✅ auth.test.ts:62-69 covers password123 → 400
No /metrics RBAC tests	5	✅ metrics.test.ts covers 401/403 (user)/403 (admin)/200 (super-admin)
Product mutations missing 403 test for user role	5	✅ products.test.ts covers POST/PATCH/DELETE → 403
/health/details info disclosure to any JWT holder	6	✅ requirePermission(PERMISSIONS.HEALTH.READ_DETAILS) added
MOBILE_API_KEY no production startup validation	6	✅ mobile-auth.ts fast-fail on empty key in production
Dead account reactivation writes outside a transaction	9	✅ Wrapped in db.transaction()

---

Timing Verification — Current State ✓

registerUser — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
Active email conflict	1 compare (DUMMY_HASH)	409
Dead account + correct password	1 compare (dead.passwordHash)	201 reactivation
Dead account + wrong password	1 compare (dead.passwordHash)	409
New user	1 hash at cost 12	201

loginUser — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
User found, correct password	1 compare (user.passwordHash)	200
User found, wrong password	1 compare (user.passwordHash)	401
No user found	1 compare (DUMMY_HASH) .then(() => false)	401

---

All Security Fixes Confirmed Correct

• loginUser — .then(() => false) always runs bcrypt; false returned when no user exists ✓
• registerUser — four paths each have exactly one bcrypt op; dead-account reactivation is atomic in a transaction ✓
• timingSafeEqual for mobile API key — SHA-256 digests ensure fixed-length buffers; ?? '' handles absent header; !MOBILE_API_KEY guard prevents SHA256('') === SHA256('') bypass ✓
• REQUEST_ID_PATTERN = /^[\w\-]{1,64}$/ — rejects injection payloads; accepts UUIDs ✓
• /metrics route — authenticate + requirePermission(METRICS.READ_ANY); seeded only to super-admin ✓
• /health/details route — authenticate + requirePermission(HEALTH.READ_DETAILS); seeded to admin + super-admin; 401/403 declared in contract ✓
• Rate limit plugin — keyGenerator: request.ip (respects TRUST_PROXY); path-based allowList for health checks; non-production skip ✓
• Per-route rate limits — 5/15min on register, login, mobileLogin via config: { rateLimit } in rpc.ts:43 ✓
• CORS plugin — production fast-fail on empty CORS_ORIGIN; localhost fallback for non-production ✓
• Cookie plugin — COOKIE_SECRET required in production; httpOnly/secure/sameSite on parseOptions ✓
• Mobile auth plugin — MOBILE_API_KEY required in production at startup ✓
• clearCookie flags — httpOnly, secure, sameSite on logout match login setCookie options ✓
• Product RBAC — permission: 'product:*:any' on mutations; 403 declared in contract; seeded to admin/super-admin; user role has read-only ✓
• updateRole explicit field list — prevents mass-assignment from future schema additions ✓
• System role guard — super-admin is isSystemRole: true; both deleteRole and updateRole check the flag ✓
• Audit log PII removal — email stripped from all logAudit calls; changedFields: Object.keys(body) logs field names only ✓
• Shared passwordSchema — applied to createUserBodySchema; registerBodySchema intentionally uses loose schema to allow legacy dead-account re-registration ✓
• Plugin registration order — metricsPlugin after authDecorator; mobileAuthPlugin before authDecorator; dbPlugin before rateLimitPlugin ✓
• audit_logs_user_id_idx migration — correct index for the primary audit-log access pattern ✓
• TRUST_PROXY + parseTrustProxy — handles true/false/numeric/IP string forms; .env fallback for local dev; process.env takes precedence ✓
• Proxy test (proxy.test.ts) — validates that TRUST_PROXY=127.0.0.1 in .env causes request.ip to return the correct XFF IP in audit logs ✓
• RouteSchema interface — rateLimit and tags fields added; aligned with RouteMap ✓
• Test coverage — auth complexity, metrics RBAC, health details RBAC, product RBAC (user 403), legacy dead-account weak-password reactivation, proxy trust ✓

---

Verdict: LGTM — no blocking issues. The rate-limit key change to request.ip + TRUST_PROXY is a genuine additional security improvement over Pass 10. Two minor observations (inline comment in .env, loose HTTP password schema for legacy re-registration) are both non-blocking and well-reasoned design decisions. Ready to merge.
| Branch

[rujealfon]

@claude

[claude]

Claude finished @rujealfon's task in 4m 25s —— View job

---

PR Code Review (Pass 12)

Tasks

• Gather context and understand request
• Read current state of all changed files
• Check for any new changes since Pass 11
• Provide comprehensive review feedback

---

All 12 issues flagged across passes 1–11 remain fixed. This pass found two positive fixes introduced since Pass 11 (both previously flagged as minor observations), plus one minor ordering note. No new blocking security issues.

---

Changes Since Pass 11 — All Positive

1. readDotEnvValue inline comment stripping — Pass 11 observation resolved ✓

src/app.ts:63 — The function now strips inline comments before returning:

return value.replace(/\s+#.*$/, '').trim()

Pass 11 flagged that TRUST_PROXY=127.0.0.1 # my proxy would produce '127.0.0.1 # my proxy' (an invalid IP string for Fastify's trustProxy). This is now correctly stripped to '127.0.0.1'. The proxy test verifies this end-to-end by writing TRUST_PROXY=127.0.0.1 # local reverse proxy to a temp .env and asserting that the resulting request.ip resolves to the XFF-provided address. ✓

2. Rate-limit error shape normalized — new 429 handler in app.ts:147-155 ✓

if (err.statusCode === 429) {
  return reply.status(429).send({
    success: false,
    error: { code: err.code ?? 'HTTP_ERROR', message: err.message },
  })
}

@fastify/rate-limit throws its own error object (not an AppError) when the limit is exceeded. Without this branch the global error handler would fall through to the unhandled-error path and return 500. This fix ensures rate-limit responses return 429 with the same { success, error } envelope as all other API errors. The 429: apiErrorSchema responses are already declared in all auth and health contracts. ✓

3. global: true added to rate-limit config ✓

src/plugins/rate-limit.ts:13 — Makes the global scope of the rate limit explicit rather than relying on the default. Correct. ✓

---

Minor Observation (non-blocking)

Plugin registration order deviates from AGENTS.md

src/app.ts:89-110 — AGENTS.md documents the required order as env → db → valkey → rate-limit → helmet → cors → cookie. The current code registers:

helmet → cors → cookie → valkey → rate-limit → db

db is after rate-limit (which needs only fastify.valkey) and helmet/cors/cookie are before valkey (which they don't need). There is no functional breakage — rateLimitPlugin accesses only fastify.config and fastify.valkey, both of which are registered before it. But the AGENTS.md documented order is not maintained. This is likely an artifact of the dd9b33b commit that fixed a duplicate registration.

---

Pass 11 Pass-Through Observations — Status

Observation	Status
readDotEnvValue doesn't strip inline comments	✅ Fixed — value.replace(/\s+#.*$/, '').trim()
MOBILE_API_KEY no production startup validation	✅ Fixed (Pass 6) — mobile-auth.ts fast-fail present

---

All Prior Issues — Confirmed Fixed ✓

Issue	Pass	Status
loginUser timing oracle (email enumeration via bcrypt skip)	1	✅ .then(() => false) always runs bcrypt
/metrics authorization gap (any JWT holder reads Prometheus data)	1	✅ requirePermission(PERMISSIONS.METRICS.READ_ANY)
allowList: ['127.0.0.1'] XFF bypass	1	✅ allowList removed; now path-based function
CORS_ORIGIN empty → silent CORS failure in production	1	✅ Startup validation throws on empty origin
Double bcrypt in new-user registration path	3	✅ Unnecessary bcrypt.compare before bcrypt.hash removed
MOBILE_API_KEY empty-string bypass in timingSafeEqual	5	✅ !MOBILE_API_KEY guard before hash comparison
No test for password complexity	5	✅ auth.test.ts:64-71 covers password123 → 400
No /metrics RBAC tests	5	✅ metrics.test.ts covers 401/403 (user)/403 (admin)/200 (super-admin)
Product mutations missing 403 test for user role	5	✅ products.test.ts covers POST/PATCH/DELETE → 403
/health/details info disclosure to any JWT holder	6	✅ requirePermission(PERMISSIONS.HEALTH.READ_DETAILS) added
MOBILE_API_KEY no production startup validation	6	✅ mobile-auth.ts fast-fail on empty key in production
Dead account reactivation writes outside a transaction	9	✅ Wrapped in db.transaction()

---

Timing Verification — Current State ✓

registerUser — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
Active email conflict	1 compare (DUMMY_HASH)	409
Dead account + correct password	1 compare (dead.passwordHash)	201 reactivation
Dead account + wrong password	1 compare (dead.passwordHash)	409
New user	1 hash at cost 12	201

loginUser — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
User found, correct password	1 compare (user.passwordHash)	200
User found, wrong password	1 compare (user.passwordHash)	401
No user found	1 compare (DUMMY_HASH) .then(() => false)	401

---

All Security Fixes Confirmed Correct

• loginUser — .then(() => false) always runs bcrypt; false returned when no user exists (auth.service.ts:86-88) ✓
• registerUser — four paths each have exactly one bcrypt op; dead-account reactivation is atomic in a transaction (auth.service.ts:34-39) ✓
• timingSafeEqual for mobile API key — SHA-256 digests ensure fixed-length buffers; array-header handled (mobileApiKey[0] ?? ''); !MOBILE_API_KEY guard prevents SHA256('') === SHA256('') bypass (auth/routes/index.ts:33-41) ✓
• REQUEST_ID_PATTERN = /^[\w\-]{1,64}$/ — rejects injection payloads; accepts UUIDs ✓
• /metrics route — authenticate + requirePermission(METRICS.READ_ANY); seeded only to super-admin ✓
• /health/details route — authenticate + requirePermission(HEALTH.READ_DETAILS); seeded to admin + super-admin; 401/403/429 declared in contract (health/routes/index.ts:47-52) ✓
• Rate limit plugin — keyGenerator: request.ip respects TRUST_PROXY; path-based allowList for health checks (unforgeable); non-production skip; global: true explicit ✓
• Per-route rate limits — 5/15min on register, login, mobileLogin; 429 declared in all contracts; 429 error handler returns correct JSON shape ✓
• TRUST_PROXY + parseTrustProxy + readDotEnvValue — handles true/false/integer/IP string; strips inline comments; process.env takes precedence over .env file ✓
• Proxy test — writes TRUST_PROXY=127.0.0.1 # local reverse proxy to temp .env, verifies request.ip reflects XFF header in audit log ✓
• CORS plugin — production fast-fail on empty CORS_ORIGIN; localhost fallback for non-production ✓
• Cookie plugin — COOKIE_SECRET required in production; httpOnly/secure/sameSite on parseOptions ✓
• Mobile auth plugin — MOBILE_API_KEY required in production at startup ✓
• clearCookie flags — httpOnly, secure, sameSite match login setCookie options ✓
• Product RBAC — permission: 'product:*:any' on mutations; 403 declared; seeded to admin/super-admin; user role has read-only ✓
• updateRole explicit field list — prevents mass-assignment from future schema additions ✓
• System role guard — super-admin is isSystemRole: true; both deleteRole and updateRole check the flag ✓
• Audit log PII removal — email stripped from all logAudit calls; changedFields: Object.keys(body) logs field names only ✓
• Shared passwordSchema — applied in auth.service.ts:46-48 for new-user path (service-layer enforcement); createUserBodySchema uses shared schema at schema layer; login correctly uses z.string().min(1) ✓
• Plugin registration order — metricsPlugin after authDecorator; mobileAuthPlugin before authDecorator; valkeyPlugin before rateLimitPlugin (functional dependency satisfied) ✓
• audit_logs_user_id_idx migration — correct index for primary audit-log access pattern ✓
• Migration 0009 — intentional no-op; name reflects content; admin not a system role by design ✓
• RouteSchema interface — rateLimit and tags fields aligned with RouteMap ✓
• Test coverage — auth complexity, metrics RBAC (401/403/403/200), health details RBAC (401/403/200/200), product RBAC (user 403), proxy TRUST_PROXY + inline comment ✓

---

Verdict: LGTM — no blocking issues. Both Pass 11 minor observations are now resolved. One minor note on plugin registration order deviating from AGENTS.md (no functional impact). Ready to merge.
| Branch

[rujealfon]

@claude

[claude]

Claude finished @rujealfon's task in 4m 22s —— View job

---

PR Code Review (Pass 13)

Tasks

• Gather context and understand request
• Read current state of all changed files
• Comprehensive review of security fixes and new code
• Provide final review feedback

---

All 12 issues flagged across passes 1–12 remain fixed. Pass 12's "minor observation" about plugin registration order (deviating from AGENTS.md) has been resolved in commit e08d535. No new blocking security issues found.

---

What Changed Since Pass 12

Plugin registration order now matches AGENTS.md exactly ✓

src/app.ts (commit e08d535) — Pass 12 flagged that the plugin order helmet → cors → cookie → valkey → rate-limit → db deviated from the documented order. The current code now matches the AGENTS.md specification:

env → db → valkey → rate-limit → helmet → cors → cookie → jwt
→ request-context → mobile-auth → authDecorator → requestIdHook
→ ... → metricsPlugin (after auth)

The functional dependency that matters (valkeyPlugin before rateLimitPlugin, authDecorator before metricsPlugin) was always satisfied, but the order is now also documentation-correct. ✓

Duplicate rateLimitPlugin registration removed ✓

src/app.ts (commit dd9b33b) — Removed the duplicate rateLimitPlugin registration that was present in a prior state. ✓

---

Status of All Prior Issues

Issue	Pass	Status
loginUser timing oracle (email enumeration via bcrypt skip)	1	✅ .then(() => false) always runs bcrypt
/metrics authorization gap (any JWT holder reads Prometheus data)	1	✅ requirePermission(PERMISSIONS.METRICS.READ_ANY)
allowList: ['127.0.0.1'] XFF bypass	1	✅ allowList removed; now unforgeable path-based function
CORS_ORIGIN empty → silent CORS failure in production	1	✅ Startup validation throws on empty origin
Double bcrypt in new-user registration path	3	✅ Unnecessary bcrypt.compare before bcrypt.hash removed
MOBILE_API_KEY empty-string bypass in timingSafeEqual	5	✅ !MOBILE_API_KEY guard before hash comparison
No test for password complexity	5	✅ auth.test.ts:64-71 covers password123 → 400
No /metrics RBAC tests	5	✅ metrics.test.ts covers 401/403 (user)/403 (admin)/200 (super-admin)
Product mutations missing 403 test for user role	5	✅ products.test.ts covers POST/PATCH/DELETE → 403
/health/details info disclosure to any JWT holder	6	✅ requirePermission(PERMISSIONS.HEALTH.READ_DETAILS) added
MOBILE_API_KEY no production startup validation	6	✅ mobile-auth.ts fast-fail on empty key in production
Dead account reactivation writes outside a transaction	9	✅ Wrapped in db.transaction()
Plugin registration order deviating from AGENTS.md	12	✅ Fixed in e08d535

---

Timing Verification — Current State ✓

registerUser (auth.service.ts:16-79) — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
Active email conflict	1 compare (DUMMY_HASH) at line 24	409
Dead account + correct password	1 compare (dead.passwordHash) at line 33	201 reactivation (atomic tx)
Dead account + wrong password	1 compare (dead.passwordHash) at line 33	409
New user	1 hash at cost 12 at line 50	201

loginUser (auth.service.ts:81-93) — each path executes exactly one cost-12 bcrypt operation:

Path	bcrypt ops	Result
User found, correct password	1 compare (user.passwordHash) line 87	200
User found, wrong password	1 compare (user.passwordHash) line 87	401
No user found	1 compare (DUMMY_HASH) .then(() => false) line 88	401

---

All Security Fixes Confirmed Correct

• loginUser — .then(() => false) always runs bcrypt; false returned when no user exists (auth.service.ts:86-88) ✓
• registerUser — four paths each have exactly one bcrypt op; dead-account reactivation is atomic in db.transaction() (auth.service.ts:34-39) ✓
• timingSafeEqual for mobile API key — !MOBILE_API_KEY guard prevents SHA256('') === SHA256(''); SHA-256 digests produce fixed 32-byte buffers regardless of header length; array-header handled (auth/routes/index.ts:33-41) ✓
• REQUEST_ID_PATTERN = /^[\w\-]{1,64}$/ — rejects injection payloads; accepts UUIDs; silently falls back to randomUUID() (request-id.ts:5,10) ✓
• /metrics route — authenticate + requirePermission(METRICS.READ_ANY) in preValidation; metrics:read:any seeded only to super-admin (metrics.ts:19-21, seed.ts:33,38) ✓
• /health/details route — authenticate + requirePermission(HEALTH.READ_DETAILS); 401/403/429 declared in contract; seeded to admin + super-admin (health/routes/index.ts:47-52, seed.ts:34,40-43) ✓
• Rate limit plugin — global: true; keyGenerator: request => request.ip (respects TRUST_PROXY); path-based allowList for /health/live and /health/ready (unforgeable); non-production skip (rate-limit.ts:9-19) ✓
• Per-route rate limits — 5/15min on register, login, mobileLogin via rateLimit in contract (auth.ts:16,29,41); wired through config: { rateLimit: route.rateLimit } in rpc.ts:45 ✓
• 429 error shape — Normalized via err.statusCode === 429 branch in global error handler (app.ts:147-155); returns { success: false, error: { code, message } } consistent with all other API errors ✓
• TRUST_PROXY + parseTrustProxy + readDotEnvValue — handles true/false/integer/IP string forms; strips inline comments (value.replace(/\s+#.*$/, '').trim()); process.env takes precedence over .env file (app.ts:35-67) ✓
• CORS plugin — production fast-fail on empty CORS_ORIGIN; localhost fallback for non-production (cors.ts:10-16) ✓
• Cookie plugin — COOKIE_SECRET required in production; httpOnly/secure/sameSite on parseOptions (cookie.ts:6-8,12-15) ✓
• Mobile auth plugin — MOBILE_API_KEY required in production at startup (mobile-auth.ts:5-6) ✓
• clearCookie flags — httpOnly, secure, sameSite on logout match login setCookie options (auth/routes/index.ts:51-56) ✓
• Product RBAC — permission: 'product:*:any' on all mutation routes; 403 declared in contract; seeded to admin/super-admin; user role has read-only only (products.ts, seed.ts:48-55) ✓
• updateRole explicit field list — only name and description updated; prevents mass-assignment from future schema additions (role.service.ts:54-57) ✓
• System role guard — super-admin is isSystemRole: true; admin is isSystemRole: false by design; both deleteRole and updateRole enforce the guard (role.service.ts:49-52,69-72) ✓
• Audit log PII removal — email stripped from all logAudit calls; changedFields: Object.keys(body) logs field names only (users/routes/index.ts:41) ✓
• Shared passwordSchema — applied to createUserBodySchema (users/schemas/index.ts:29); registerBodySchema intentionally loose (auth/schemas/index.ts:5) with service-layer enforcement for new-user path; loginBodySchema correctly uses z.string().min(1) ✓
• Plugin registration order — env → db → valkey → rate-limit → helmet → cors → cookie → jwt → request-context → mobile-auth → authDecorator → metricsPlugin (after auth) (app.ts:82-120) ✓
• audit_logs_user_id_idx migration — correct index for the primary audit-log access pattern ✓
• Migration 0009 — intentional no-op; name reflects content; admin not a system role by design (admin lacks role:delete:any permission anyway) ✓
• Proxy test — proxy.test.ts writes TRUST_PROXY=127.0.0.1 # local reverse proxy to a temp .env and verifies request.ip reflects the XFF header in audit metadata ✓
• Test coverage — auth complexity, metrics RBAC (401/403/403/200), health details RBAC (401/403/200/200), product mutations (user → 403), legacy dead-account weak-password reactivation, proxy TRUST_PROXY with inline comment ✓

---

Minor Observations (non-blocking, unchanged from prior passes)

• registerBodySchema HTTP validation is intentionally loose — z.string().min(1).max(72) passes HTTP validation for both new and dead-account re-registration. Service enforces passwordSchema complexity for new users only. The .meta({ description: ... }) on the field documents this intent. The test at auth.test.ts:383-393 verifies legacy weak-password reactivation.
• DB query timing delta in registerUser — the active-conflict path runs one fewer DB query than paths 2-4. At ~1ms per query vs ~100ms bcrypt, this is a negligible ~1% timing signal; not actionable without restructuring the query logic.
• XFF IP-rotation evasion — request.ip is correct with TRUST_PROXY configured to the actual proxy; without it, the TCP IP is used (unforgeable). IP rotation remains possible if an attacker controls the upstream network. Mitigated by TRUST_PROXY configuration.

---

Verdict: LGTM — no blocking issues. All 13 previously flagged issues (including Pass 12's plugin order observation) are confirmed resolved. This PR represents a materially stronger security posture. Ready to merge.
| Branch