Skip to content

Fix public password reset proxy - #54

Open
vishu-bh wants to merge 1 commit into
mainfrom
fix/public-password-reset-bff
Open

Fix public password reset proxy#54
vishu-bh wants to merge 1 commit into
mainfrom
fix/public-password-reset-bff

Conversation

@vishu-bh

Copy link
Copy Markdown
Contributor

Summary

  • add narrowly allowlisted anonymous BFF routes for requesting, validating, and completing password resets
  • keep all other /api/* and email-auth administration routes behind session authentication
  • enforce same-origin checks for mutations and strip browser cookies/authorization before upstream forwarding
  • prevent password-reset tokens from entering BFF request/error logs
  • add a configurable password-reset timeout above the upstream SMTP default

Testing

  • npm --prefix server test -- --run — 72 tests passed
  • npm run lint
  • Prettier check and git diff --check

Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
@vishu-bh
vishu-bh marked this pull request as ready for review August 20, 2026 14:13

@marekdano marekdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. Log redaction bypass (case-sensitive URL match) — Medium-High, fix before merge

server/src/lib/request-logging.ts:14

isSensitivePasswordResetUrl() does a case-sensitive prefix match, so a case-varied request path bypasses the log-redaction it exists to enforce.

Failure scenario: A request to /App/Reset-Password/<token> or /API/Auth/Email/Reset-Password/<token> (e.g. via a WAF/proxy that canonicalizes path casing, or a mistyped/forwarded link) fails to case-match the lowercase route in find-my-way (caseSensitive: true by default) and also fails to case-match the lowercase prefixes here, so disableRequestLogging returns false and Fastify's automatic request/response logger writes the raw URL — including the plaintext reset token — into application logs, exactly the leak this file's own docstring says it must prevent.

Why it matters: password-reset flow specifically; a leaked token grants account takeover during its validity window; logs are typically lower-trust than the primary datastore. Fastify's logging hooks fire before routing resolves, so the mismatched-case request doesn't need to hit the real handler to get logged.


2. Missing empty-body JSON parser — Low, non-blocking

server/src/routes/proxy/public-password-reset.ts:102

The new POST routes (forgot-password, reset-password complete) don't register the empty-body-tolerant JSON content-type parser that catch-all.ts explicitly added for this same Fastify quirk.

Failure scenario: A POST to /api/auth/email/forgot-password or /api/auth/email/reset-password/:token with Content-Type: application/json and an empty body (e.g. a health-checker, a misbehaving client, or a future frontend change) hits Fastify's default JSON parser, which throws FST_ERR_CTP_EMPTY_JSON_BODY during body-parsing — before rejectCrossOriginMutation or forwardPublicRequest ever run — producing an uncontrolled framework 400 instead of the route's own consistent {error: ...} JSON envelope, unlike every other error path in this file.

Why it matters: inconsistent error shape for an edge case unlikely in normal browser usage. Papercut, safe to land as a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants