feat(oauth): add an untrusted-client consent screen and restore 321done by vbudhram · Pull Request #21219 · mozilla/fxa

vbudhram · 2026-09-15T17:49:48Z

Because

321done could not complete a sign-in: the shared secrets file overrode the per-instance config, so the untrusted app sent the wrong client secret.
An untrusted client's sign-in redirected straight back to the relying party, telling the user nothing about the profile information it reads.
No functional test drove the untrusted client, so both gaps were invisible.

This pull request

Loads the per-instance 123done config after the shared secrets file, so each client resolves its own secret.
Adds a React consent screen at /signin_permissions that lists the email and display name an untrusted client can read.
Gates the screen on a new isUntrusted(), so an unresolved client lookup never treats a trusted client as untrusted.
Returns a consent_required error instead of the screen when the relying party sent prompt=none.
Badges the untrusted demo app and reports the flag on /api/auth_status.
Adds the untrusted port to clean-start and lets the pm2 readiness banner report two apps for one project.
Adds untrusted relier and permissions page objects, seven Playwright tests, and unit tests for the scope and consent rules.

Issue that this pull request solves

Closes: FXA-14505

Checklist

Put an x in the boxes that apply

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 files/areas to focus on: packages/fxa-settings/src/pages/Permissions/, src/lib/oauth/permissions.ts, and the gate in src/pages/Signin/utils.ts.
Suggested review order: the consent rules in permissions.ts, then the gate, then the screen and its container.
Risky or complex parts: the gate runs for untrusted clients only, so a trusted client keeps its key material and never routes through a page.

Screenshots (Optional)

The consent screen an untrusted client now shows:

Recording, sign-in at 321done through the consent screen and back to the relying party:

video.mp4

Other information (Optional)

Copilot

🟡 Changes recommended

Sign-up bypasses the consent gate, and permission persistence plus the new OAuth errno conflict with existing conventions.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds an OAuth permissions screen for untrusted clients and restores the local 321done flow.

Changes:

Adds consent gating, persistence, routing, localization, and error handling.
Configures and badges the untrusted 321done instance.
Adds unit and Playwright coverage plus local startup support.

File summaries

File	Description
`packages/fxa-settings/src/pages/Signin/utils.ts`	Gates OAuth completion on permissions.
`packages/fxa-settings/src/pages/Signin/utils.test.ts`	Tests consent navigation and prompt-none errors.
`packages/fxa-settings/src/pages/Signin/mocks.tsx`	Adds trust-state mocks.
`packages/fxa-settings/src/pages/Signin/interfaces.ts`	Exposes `isUntrusted`.
`packages/fxa-settings/src/pages/Permissions/index.tsx`	Implements the permissions UI.
`packages/fxa-settings/src/pages/Permissions/index.test.tsx`	Tests permissions rendering and actions.
`packages/fxa-settings/src/pages/Permissions/index.stories.tsx`	Adds UI scenarios.
`packages/fxa-settings/src/pages/Permissions/en.ftl`	Adds localized copy.
`packages/fxa-settings/src/pages/Permissions/container.tsx`	Connects consent state and OAuth completion.
`packages/fxa-settings/src/models/integrations/oauth-web-integration.ts`	Adds explicit untrusted-client detection.
`packages/fxa-settings/src/models/integrations/oauth-web-integration.test.ts`	Tests trust classification.
`packages/fxa-settings/src/models/integrations/integration.ts`	Adds the default trust API.
`packages/fxa-settings/src/lib/storage-utils.ts`	Adds persisted permission history.
`packages/fxa-settings/src/lib/oauth/permissions.ts`	Implements permission filtering and persistence.
`packages/fxa-settings/src/lib/oauth/permissions.test.ts`	Tests permission rules.
`packages/fxa-settings/src/lib/oauth/oauth-errors.ts`	Adds `consent_required`.
`packages/fxa-settings/src/components/App/index.tsx`	Registers the React route.
`packages/fxa-content-server/server/lib/routes/react-app/index.js`	Serves the new React route.
`packages/functional-tests/tests/oauth/untrustedRelierSignin.spec.ts`	Exercises trusted and untrusted flows.
`packages/functional-tests/pages/untrustedRelier.ts`	Adds the 321done page object.
`packages/functional-tests/pages/signup.ts`	Removes obsolete permission locators.
`packages/functional-tests/pages/signin.ts`	Removes obsolete permission locators.
`packages/functional-tests/pages/relier.ts`	Supports alternate relier origins.
`packages/functional-tests/pages/permissions.ts`	Adds the permissions page object.
`packages/functional-tests/pages/index.ts`	Registers new page objects.
`packages/functional-tests/lib/targets/stage.ts`	Defines stage 321done settings.
`packages/functional-tests/lib/targets/production.ts`	Defines production 321done settings.
`packages/functional-tests/lib/targets/local.ts`	Defines local 321done settings.
`packages/functional-tests/lib/targets/base.ts`	Extends the target contract.
`packages/123done/static/js/123done.js`	Toggles the untrusted badge.
`packages/123done/static/index.html`	Adds the badge markup.
`packages/123done/static/css/main.css`	Styles the badge.
`packages/123done/server.js`	Reports untrusted status.
`packages/123done/README.md`	Documents dual-instance configuration.
`packages/123done/config.js`	Corrects configuration precedence.
`packages/123done/config-stage-untrusted.json`	Marks stage 321done untrusted.
`packages/123done/config-local-untrusted.json`	Configures local 321done credentials.
`_scripts/pm2-all.sh`	Checks both demo-app endpoints.
`_scripts/clean-start.sh`	Cleans the 321done port.

Review details

Files reviewed: 39/39 changed files
Comments generated: 3
Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

nshirley · 2026-09-22T16:42:14Z

+  if (
+    isOAuthWebIntegration(navigationOptions.integration) &&
+    needsPermissions({
+      untrusted: navigationOptions.integration.isUntrusted(),
+      scopes: Array.from(


There look to be several routes that call finishOAuthFlowHandler, and can navigate back to the RP, so maybe we can hoist the guard somewhere all of them go through instead of adding it to each? If I'm following things right, it looks like useFinishOAuthFlowHandler might be that common place?

    response_error_code: 'interaction_required',
  },
+  PROMPT_NONE_CONSENT_REQUIRED: {
+    errno: 1014,


+  /** Profile scopes the user has been shown, keyed by OAuth client id. */
+  grantedPermissions?: Record<string, string[]>;


nshirley · 2026-09-22T16:23:20Z

+  test.describe('OAuth untrusted relier signin', () => {
+    test.beforeEach(async ({}, { project }) => {
+      test.skip(
+        project.name !== 'local',


These are sev-1 but skipped if not local, is that intended?

I'll remove the severity tag. I don't think we have anything defined on what it means or how it is defined. I'll also make this run in stage and prod.

We have some stuff on severity, but it's not a lot. Thanks for checking though! Just wanted to make sure where these are intended to run

nshirley · 2026-09-22T16:26:43Z

+      expect(page.url()).toContain(target.untrustedRelierUrl);
+    });
+
+    test('lists only the profile information the relier asked for', async ({


It looks like this test and the next two are re-testing what permissions.test.ts is doing in unit. If keeping the functional test for this is necessary, could we at least fold the other two test assertions into this one since the test is already on the page?

nshirley · 2026-09-22T16:31:16Z

+    setBannerErrorMessage('');
+    const clientId = integration.getClientId();
+    if (clientId) {
+      recordSeenPermissions(signinState.uid, clientId, scopes);


Since this is before the navigation, if that navigation fails then they'll skip this screen on the next attempt, right? Just wanted to see if that's intentional

nshirley · 2026-09-22T16:32:18Z

+    }
+  };
+
+  const onCancel = () => navigateWithQuery('/signin');


Is it expected that we would just redirect to signin here? If an RP is waiting for a response then this would leave them hanging right?

nshirley · 2026-09-22T16:33:11Z

  readonly relierUrl = `https://${RELIER_DOMAIN}`;
  readonly relierClientID = RELIER_CLIENT_ID;
+  readonly untrustedRelierUrl = `https://${UNTRUSTED_RELIER_DOMAIN}`;
+  readonly untrustedRelierClientID = UNTRUSTED_RELIER_CLIENT_ID;


I see this is set in a few of the targets but not read anywhere, is that intentional?

nshirley · 2026-09-22T16:42:14Z

+  if (
+    isOAuthWebIntegration(navigationOptions.integration) &&
+    needsPermissions({
+      untrusted: navigationOptions.integration.isUntrusted(),
+      scopes: Array.from(


There look to be several routes that call finishOAuthFlowHandler, and can navigate back to the RP, so maybe we can hoist the guard somewhere all of them go through instead of adding it to each? If I'm following things right, it looks like useFinishOAuthFlowHandler might be that common place?

Because: - 321done could not complete a sign-in: one environment variable served both demo clients, and the shared secrets file overrode the per-instance config, so the untrusted app sent the wrong secret. - An untrusted client's sign-in redirected straight back to the relying party, telling the user nothing about the profile information it reads. - No functional test drove the untrusted client, so both gaps were invisible. This commit: - Loads the per-instance 123done config after the shared secrets file, and keeps CI's CLIENT_SECRET_123DONE out of 321done, so each client resolves its own secret. - Adds a React consent screen at /signin_permissions that lists the email and display name an untrusted client can read, recording what it showed in the `permissions` field Backbone also writes. - Gates useFinishOAuthFlowHandler on a new isUntrusted(), so every flow that completes OAuth shows the screen, not only sign-in. - Returns access_denied to the relying party when the user cancels. - Badges the untrusted demo app, and adds page objects, Playwright tests, and unit tests for the consent rules.

vbudhram marked this pull request as ready for review September 17, 2026 17:11

vbudhram requested review from a team as code owners September 17, 2026 17:11

Copilot AI balanced review requested due to automatic review settings September 17, 2026 17:11

Copilot started reviewing on behalf of vbudhram September 17, 2026 17:11 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

bcolsson approved these changes Sep 18, 2026

View reviewed changes

vbudhram force-pushed the fxa-14505 branch from b3ee0cb to de2f302 Compare September 22, 2026 16:10

nshirley reviewed Sep 22, 2026

View reviewed changes

vbudhram force-pushed the fxa-14505 branch from de2f302 to 6c66e78 Compare September 23, 2026 16:49

vbudhram force-pushed the fxa-14505 branch from 6c66e78 to 0ae3ae1 Compare September 23, 2026 16:53

nshirley approved these changes Sep 23, 2026

View reviewed changes

vbudhram merged commit 05491e7 into main Sep 23, 2026
22 checks passed

vbudhram deleted the fxa-14505 branch September 23, 2026 18:16

Navigation Menu

Repository navigation

feat(oauth): add an untrusted-client consent screen and restore 321done - #21219

feat(oauth): add an untrusted-client consent screen and restore 321done#21219
vbudhram merged 1 commit into
mainfrom
fxa-14505

vbudhram commented Sep 15, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

nshirley Sep 22, 2026

Uh oh!

nshirley Sep 22, 2026

Uh oh!

vbudhram Sep 22, 2026

Uh oh!

nshirley Sep 22, 2026

Uh oh!

nshirley Sep 22, 2026

Uh oh!

nshirley Sep 22, 2026

Uh oh!

nshirley Sep 22, 2026

Uh oh!

nshirley Sep 22, 2026

Uh oh!

nshirley Sep 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		/** Profile scopes the user has been shown, keyed by OAuth client id. */
		grantedPermissions?: Record<string, string[]>;

Conversation

vbudhram commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Because

This pull request

Issue that this pull request solves

Checklist

How to review (Optional)

Screenshots (Optional)

Other information (Optional)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vbudhram commented Sep 15, 2026 •

edited

Loading