[dawsontoth]

⊙ Problem

When two enabled OIDC trust policies matched one identity token, the exchange took the first by id, so which user a CI run acted as was up to the ids. enable_previews refuses to write such a pair, but only on its own node.

Fixes #3608. Part of #3042.

🤔 Refuse at the exchange, or prevent the pair from being written?

Refuse at the exchange. No write path can exclude a pair written on another node, and the exchange is the one place every policy passes through, whoever wrote it.

💡 Solution

The exchange checks every enabled policy, and refuses a token two of them give a different user or different operations, before the token is spent. Policies that agree still answer with the first by id. list_oidc_trust names each such pair in invalid_reason, and harper deploy setup refuses to write one.

🤔 A cluster that relied on id order starts refusing those runs

Two harper deploy setup runs sharing a workflow, environment and branch already wrote such a pair, and id order made one work. After upgrade both refuse until one policy is dropped; list_oidc_trust names them, and this needs a release note. The guarantee covers the policies the exchanging node sees.

🤔 Known limits of the listing

list_oidc_trust can flag a pair a provider veto keeps apart, such as a policy with no event_name beside one pinned to pull_request_target, and setup refuses that pair too. It can also miss a conflict only a token with two audiences hits, though the exchange still refuses that token. Making each provider say whether two policies can co-match would close the first.

Docs this needs

• Operations reference: list_oidc_trust gains exchangeable, and invalid_reason now covers conflicting pairs.
• Release note: run list_oidc_trust before upgrading, and fix any pair it names; upgrade every node, since an older node still takes the first by id.

⚖️ Alternatives

• A create-only write across nodes: replication is last-writer-wins per id with no consensus, and overlap is between different ids, so no key constraint could exclude it.
• Refusing any second match: smaller, but it would break existing pairs that agree on user and scope, which are unambiguous.

🔧 Changes

• The exchange. security/authn/oidc/tokenExchange.ts: findMatchingPolicy scans past the first match and returns a conflict, and exchangeOidcToken refuses it before the replay record, with both ids in the audit.
• The comparison. security/ciIdentity.ts: sameAuthority, used by the exchange, the listing and policyDifferences.
• Finding a pair. security/authn/oidc/trustPolicyOperations.ts: list_oidc_trust marks each row exchangeable and names a rival in invalid_reason. bin/deploySetupGithubActions.ts: planReconcile refuses a rival before writing.
• Wording. components/previewOperations.ts and security/DESIGN.md say what the exchange now does.

✅ Verification

• unitTests/security/authn/oidc/tokenExchange.test.js: conflicting users and operations in either id order, a conflict behind two that agree, no token spent, a retry after the conflict is removed, and a token with two audiences. Six of these fail on main's code.
• unitTests/security/ciIdentity.test.js and unitTests/bin/deploySetupGithubActions.test.js cover the comparison and the setup refusal, and unitTests/components/previewOperations.test.js the rows enable_previews ignores.
• unitTests/security passes except two jsLoader tests that fail on main; lint:required, typecheck and prettier pass.
• End to end: integrationTests/security/deploy-setup-github-actions.test.ts runs the setup refusal in CI, and each other component there now has its own environment. The OIDC exchange tests use real signing and JWKS verification with the tables mocked; a cross-node pair was not exercised.
• PR QA: skipped @ HEAD — no dispatch in this session.

🤖 Generated with Claude Code

Release target: v5.4 — closes a gap in unreleased 5.4 preview work from #3617

Related PRs: #3617 overlaps (merged; this closes a window it documented), #3807 overlaps (open; both edit bin/deploySetupGithubActions.ts and components/previewOperations.ts), #3809 overlaps (open; both edit bin/deploySetupGithubActions.ts), #3812 overlaps (open; both edit components/previewOperations.ts)
Complexity: simple

Late Grok findings

6 surviving (highest minor) — Grok reviewed e56bc3a

• minor — security/DESIGN.md:31 — Item 5 sits between item 4 and item 4's indented continuation paragraphs (enforcement surface, sql scope). Those paragraphs now read as part of "Policy ids never choose authority"
• minor — security/authn/oidc/tokenExchange.ts:156 — The scan no longer stops at the first match. Every successful exchange now verifies the remaining audiences for the issuer, and rejectToken (security/authn/oidc/identityToken.ts:59) plus the catch emit "Rejecting identity token" / "Verification failed" WARNs alongside a granted token
• minor — security/authn/oidc/tokenExchange.ts:257 — Overlapping enabled policies that worked before this change now 401. Only preview setup checks for this (components/previewOperations.ts:194-206); harper deploy setup and list_oidc_trust don't, so operators only find out from the server log
• nit — security/authn/oidc/tokenExchange.ts:121 — Added comments restate the matching logic: the first-match/conflict walkthrough in the findMatchingPolicy docblock, and the exchange narration at security/ciIdentity.ts:142-145
• nit — unitTests/security/authn/oidc/tokenExchange.test.js:439 — The operations-conflict cases never check useTable.mock.rows.size. The shared conflict branch runs before recordTokenUse (tokenExchange.ts:257 vs :274), and the users case at :436 already proves it
• nit — security/ciIdentity.ts:38 — Setup's sameSet and the exchange's sameAuthority compare operations by different rules, so the two can drift (deferred, scope-predicate-divergence)

Not standing: 1 separate-issue. Grok ran 13m (13m from launch to report); the fold adjudication (cursor-grok@e56bc3a1f92b-1922aaca, claude) took 1m.

Late Grok findings

8 surviving (highest minor) — Grok reviewed 35514c1

• minor — security/authn/oidc/trustPolicyOperations.ts:337 — The list_oidc_trust rival check does not match the exchange's matching rules, so it gets conflicts wrong in both directions:
• minor — bin/deploySetupGithubActions.ts:753 — Setup's preflight (planIdentityReconcile) never checks for overlapping policies. Setup writes the new policy at :760 and only finds the conflict in the verify step at :767. By then a sibling policy with the same repo and workflow, but a different user, has already lost its CI exchange, and the new
• nit — components/previewOperations.ts:197 — rivalPolicies returns 409 for any enabled overlapping policy, including one with the same user and operations, which the exchange accepts (tokenExchange.ts:162-163). The message gives the different-authority refusal as the reason, which is wrong for that case. The suggested fix (workflow=) doe
• nit — security/authn/oidc/tokenExchange.ts:121 — findMatchingPolicy decodes the token a second time, though :227 already decoded it. Exchange runs once per CI run, so the cost is small. Scope: in-scope — Convergence-Class: previously-adjudicated
• nit — security/ciIdentity.ts:42 — sameOperationScope is exported, but nothing outside ciIdentity.ts uses it (only :51 and :118). Scope: in-scope — Convergence-Class: previously-adjudicated
• nit — security/ciIdentity.ts:49 — Several added comments narrate the code: the sameAuthority JSDoc, the pair-scan comment at trustPolicyOperations.ts:336, and the comment at tokenExchange.ts:120. The invariant comment at ciIdentity.ts:41 is worth keeping. Scope: in-scope — Convergence-Class: previously-adjudicated
• nit — unitTests/security/authn/oidc/tokenExchange.test.js:439 — The operations-conflict cases don't check that the token was left unspent. Disputed: :436 already covers the shared conflict branch, which runs before recordTokenUse. Scope: in-scope — Convergence-Class: previously-adjudicated
• nit — unitTests/security/ciIdentity.test.js:4 — sameAuthority and sameOperationScope have no direct unit tests. Disputed: tokenExchange.test.js:452-466 already covers set comparison, duplicates, and empty-versus-absent scope. Scope: in-scope — Convergence-Class: previously-adjudicated

Not standing: none. Grok ran 11m (11m from launch to report); the fold adjudication (cursor-grok@35514c1fc2af-564cec1e, claude) took 2m.

Deferred findings

• 737c2735e1ed9b58 nit concurrency — bin/deploySetupGithubActions.ts — pre-write rival recheck cannot exclude a conflicting policy written on another node — NOT FILED

_{Review-Coverage: authored=claude; ran=gemini,cursor-composer,codex,cursor-grok; late=cursor-grok:adjudicated; adjudicated=domain; declined=muse; rounds=5; full=3 @ ae4f404}

_{Review-Attention: study ~15m (sensitive: tokenExchange.ts, trustPolicyOperations.ts +1; decisions: refuse-on-ambiguity, authority-is-user-identity, dead-user-policies-still-rival, exchangeable-field, static-overlap-not-exchange-parity, do-less-alternative) @ ae4f404}

[gemini-code-assist]

Code Review

This pull request updates the OIDC token exchange mechanism to refuse token exchanges when multiple matching trust policies resolve to different users or operations (different authorities), rather than simply selecting the first matching policy by ID. It introduces sameAuthority and sameOperationScope helpers to compare policies, updates reconciliation logic in GitHub Actions setup and preview operations, updates design documentation, and adds comprehensive unit tests. The review feedback highlights three key areas for improvement: downgrading verification warnings to debug level once a match is found to prevent log pollution, updating rivalPolicies in previewOperations.ts to ignore overlapping policies that share the same authority, and capturing caught error messages as invalid_reason in listOidcTrust to improve diagnostic visibility for administrators.

[gemini-code-assist]

While skipping non-matching audiences when matched is set is a great optimization, any subsequent policies that do have matching audiences will still trigger verifyIdentityToken and log warnings if they fail verification. Since a successful match has already been found, these verification failures on other policies are expected (as they simply don't match) and should not pollute the server logs with warnings.

Consider suppressing or downgrading these warnings to debug level when matched is already set.

[dawsontoth]

Not changed: the audience skip above means verification after a match only runs for an audience the token names, so a failure here is operational (a JWKS outage, say) and leaves that policy's conflict unchecked. That should stay a warning.

🤖 Addressed by Claude Code

[gemini-code-assist]

rivalPolicies flags a conflict for any overlapping policy, even if they have the same authority (same user and operations). However, the exchange actually accepts overlapping policies that agree on user and operations (taking the first by ID). The error message itself claims that the exchange refuses them because they give different users or operations, which is incorrect when they actually agree.

To fix this, you should update the policies.find call in rivalPolicies (around line 197) to include !sameAuthority(candidate, policy) and import sameAuthority from ../security/ciIdentity.ts:

const rival = policies.find(
	candidate => !ours.has(candidate?.id) && candidate?.enabled !== false && policiesOverlap(candidate, policy) && !sameAuthority(candidate, policy)
);

[dawsontoth]

Fixed in 8939e5d: rivalPolicies now also requires !sameAuthority, so a policy that agrees on user and operations no longer blocks enable. A test covers it.

🤖 Addressed by Claude Code

[gemini-code-assist]

In listOidcTrust, when a policy is not exchangeable because of specificity or profile errors (e.g., missing workflow/ref gate or unknown issuer), the try-catch block in the honored filter catches the error and returns false, but it does not set policy.invalid_reason. This leaves the policy marked as exchangeable: false in the listing but with no explanation to the administrator as to why it is invalid.

Setting policy.invalid_reason to the caught error's message will provide crucial diagnostic visibility.

		} catch (error) {
			policy.invalid_reason = (error as Error).message;
			return false;
		}

[dawsontoth]

Fixed in 8939e5d: the caught specificity error now lands in invalid_reason (unless one is already set). A test covers a too-broad policy.

🤖 Addressed by Claude Code