Conversation
There was a problem hiding this comment.
🟡 Changes recommended
prompt=none can still render verification UI and send mail when return_on_error=false.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Prevents silent OAuth requests from triggering verification UI or email and adds coverage for heuristic unverified sessions.
Changes:
- Returns
interaction_requiredfor relevantprompt=noneflows. - Adds configurable heuristic-session test accounts.
- Expands unit and functional coverage across OAuth, Settings, and Payments Next.
File summaries
| File | Description |
|---|---|
packages/fxa-settings/src/pages/Signin/utils.ts |
Updates verification routing and OTP sending. |
packages/fxa-settings/src/pages/Signin/utils.test.ts |
Tests OAuth navigation outcomes. |
packages/fxa-settings/src/pages/Authorization/container.test.tsx |
Tests RP error relaying. |
packages/fxa-auth-server/lib/routes/account.ts |
Adds heuristic verification forcing. |
packages/fxa-auth-server/lib/routes/account.spec.ts |
Tests session-token properties. |
packages/fxa-auth-server/config/index.ts |
Defines verification regex configuration. |
packages/functional-tests/tests/signin/unverifiedSession.spec.ts |
Adds end-to-end session verification coverage. |
packages/functional-tests/tests/signin/signIn.spec.ts |
Corrects test setup and PKCE parameters. |
packages/functional-tests/tests/oauth/syncSignIn.spec.ts |
Clarifies Sync verification behavior. |
packages/functional-tests/tests-payments-next/checkout.spec.ts |
Tests Payments Next verification. |
packages/functional-tests/lib/testAccountTracker.ts |
Adds an unverified-session account fixture. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
A prompt-none request can still navigate to TOTP enrollment after an insufficient-ACR response.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 4
- Review effort level: Balanced
| navigationOptions.canRelayPromptNoneError && | ||
| error.errno === AuthUiErrors.INSUFFICIENT_ACR_VALUES.errno | ||
| ) { | ||
| return { error: new OAuthError('UNMET_AUTHENTICATION_REQUIREMENTS') }; |
There was a problem hiding this comment.
It looks like this is what we want? We have a unit test, does not fail the request when the RP opted out of error redirects added in https://mozilla-hub.atlassian.net/browse/FXA-12860. @StaberindeZA does that seem right?
There was a problem hiding this comment.
Yup that looks correct to me. Thank you for checking 👍
Because: * prompt=none must not render UI or send mail, but a verified email with an unverified session reached /signin_token_code and emailed an OTP * The RP was never told its request failed, and the mail was reachable repeatedly by an RP retrying in the background * The heuristic unverified session had no functional coverage, so the pass-through the fix preserves was never exercised end to end This commit: * Fails prompt=none with interaction_required when an unverified session would reach a verification page, whatever the RP asked for on error, and skips the code resend on that path * Keeps servicesWithEmailVerification authoritative: RPs outside it still bypass session verification and continue on to the grant * Covers the silent grant against the heuristic fixture, plus a listed-RP checkout test that reaches stage * Corrects two signin skip reasons and drops PKCE params that 400 on a confidential client * Records why both listed-RP tests exist, in each Closes #FXA-14408 Closes #FXA-13740
| // to the RP. The pass-through is what makes the silent grant possible, so | ||
| // both halves are exercised against the same session state. | ||
| test.describe('prompt=none', () => { | ||
| test('grants silently for an RP outside servicesWithEmailVerification', async ({ |
There was a problem hiding this comment.
I like having this as a functional test to assert the one email code across two grants plus the Settings sign-in.
@StaberindeZA since you reviewed, I've removed 3 other functional tests after Nick's review on #21235. I personally want to have some functional tests around this behavior due to bugs we've seen with it, but I agree we should "shift left" where we can and use unit tests.
| // .spec.ts. This one runs the real embedded sign-in, and is the only one | ||
| // that reaches stage; that one is the only one that runs on PRs. Neither | ||
| // reaches production — the describe above skips checkout there. | ||
| test('checkout challenges a heuristic unverified session and sends one code', async ({ |
There was a problem hiding this comment.
@StaberindeZA could I ask you to run this locally to verify it since this only runs in your Nightly and won't run in this PR? I'm sure it's something to do with my Stripe key but I'd like you to take a peek anyway if you can 🤞
There was a problem hiding this comment.
OK great, I ran this locally and everything worked as expected.
Because:
an unverified session reached /signin_token_code and emailed an OTP
repeatedly by an RP retrying in the background
the refusal nor the pass-through it preserves was exercised end to end
This commit:
would reach a verification page, whatever the RP asked for on error,
and skips the code resend on that path
bypass session verification and continue on to the grant
listed-RP checkout test that reaches stage
confidential client
Closes #FXA-14408
Closes #FXA-13740
This is part 2 of FXA-13740, the first half which was covered in #21235.
At the time of writing this branch contains both commits. To review just the test additions + fix for 14408 (~500 lines), check out the 2nd commit.
The payments-next checkout test does not run on PRs —
playwright-payments-testsis only in thesubplat-v*tag and nightly workflows — and it is skipped in production by the suite-levelbeforeEach. Its first execution will be nightly.