Skip to content

feat(auth): replace WAF "unexpected error" with more meaningful copy#20677

Merged
toufali merged 1 commit into
mainfrom
waf-error-messages
Jun 3, 2026
Merged

feat(auth): replace WAF "unexpected error" with more meaningful copy#20677
toufali merged 1 commit into
mainfrom
waf-error-messages

Conversation

@toufali
Copy link
Copy Markdown
Member

@toufali toufali commented Jun 2, 2026

Because

  • The WAF (Fastly) returns 406/429 directly with non-JSON bodies, bypassing the auth server. The auth client's JSON.parse fails and the UI falls back to "Unexpected error", giving devs and false-positive users no guidance.

This pull request

  • Catches the JSON parse failure on 406/429 and throws an AuthClientError with errno: 125 (REQUEST_BLOCKED) for 406 and errno: 114 (THROTTLED) for 429.
  • Reuses existing localized strings (auth-error-125, auth-error-114-generic)
  • Adds unit tests covering both status codes, the Sentry tag shape, the non-WAF passthrough, and that auth-server JSON-bodied 429s are not overridden.

Issue that this pull request solves

Closes: FXA-13612

Checklist

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).
  • I have manually reviewed all AI generated code.

How to review (Optional)

  • Key file: packages/fxa-auth-client/lib/client.ts — the parse-failure branch in AuthClient.request().
  • The frontend path is unchanged; verify by reading getLocalizedErrorMessage in packages/fxa-settings/src/lib/error-utils.ts — it looks up by errno, so 114/125 already work.

Other information (Optional)

  • Sentry signal change: 406/429-with-non-JSON-body events previously surfaced as "unexpected error" with a SyntaxError extra. They now surface as "WAF-blocked response" with status and errno tags. Intentional, but worth knowing for any saved searches.
Screenshot 2026-06-03 at 11 05 21 AM

Copilot AI review requested due to automatic review settings June 2, 2026 22:06
@toufali toufali requested a review from a team as a code owner June 2, 2026 22:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates fxa-auth-client request error handling to detect WAF-blocked responses that return non-JSON bodies (notably 406/429) and map them to known FxA errnos so downstream UIs can render localized, actionable error messages instead of a generic “Unexpected error”.

Changes:

  • Add special-case handling for non-JSON 406/429 responses to synthesize known errnos (114/125) and emit a targeted Sentry message.
  • Extend the auth-client test suite with coverage for WAF-blocked non-JSON bodies and for preserving existing behavior in other non-JSON failure cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
packages/fxa-auth-client/lib/client.ts Adds WAF detection on JSON parse failure for 406/429 and maps to known errnos with Sentry tagging.
packages/fxa-auth-client/test/client.ts Adds/updates tests to validate synthesized errno behavior and Sentry capture for WAF-blocked responses.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/fxa-auth-client/lib/client.ts
Copy link
Copy Markdown
Contributor

@LZoog LZoog left a comment

Choose a reason for hiding this comment

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

I didn't test this locally by trying to throw one of these errors, but code LGTM.

Comment thread packages/fxa-auth-client/lib/client.ts Outdated
throw parseError;
}
errorCaptured = true;
Sentry.captureMessage(`Auth client encountered WAF-blocked response`, {
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.

Is this something we want in Sentry? Seems like it'd be nice, but also don't want it eating our tokens for something we won't ever resolve.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fair point @LZoog ! Removed.

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.

Maybe we want this in statsd though? Not sure 🙂

@toufali toufali force-pushed the waf-error-messages branch from 8079949 to 2bc60e5 Compare June 2, 2026 23:35
Copy link
Copy Markdown
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

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

@toufali Thanks!

}
errorCaptured = true;
throw new AuthClientError(
'WAF blocked',
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.

I don't think we need to surface "WAF'? Don't think many folks will know/care what that is. Maybe just "Request blocked"

Copy link
Copy Markdown
Member Author

@toufali toufali Jun 3, 2026

Choose a reason for hiding this comment

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

@vbudhram -- I believe this is internal only, for dev reviewing logs, sentry, etc. Users should see translated strings instead.

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.

@toufali you could add a Storybook state for this error too if it's easy to ask Claude to do it. 🤷‍♀️

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@toufali toufali force-pushed the waf-error-messages branch 2 times, most recently from a9e927e to ca25f73 Compare June 3, 2026 17:44
@toufali toufali force-pushed the waf-error-messages branch from ca25f73 to ac9dba4 Compare June 3, 2026 18:29
@toufali toufali merged commit 5a3c189 into main Jun 3, 2026
21 checks passed
@toufali toufali deleted the waf-error-messages branch June 3, 2026 18:56
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.

4 participants