Skip to content

fix(auth): isolate API keys to exact credentials and organizations - #7461

Merged
bokelley merged 1 commit into
mainfrom
conductor/api-key-credential-boundary
Sep 17, 2026
Merged

bokelley merged 1 commit into
mainfrom
conductor/api-key-credential-boundary

Conversation

@bokelley

@bokelley bokelley commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

API-key authentication and management bind to the exact authenticated credential and explicit organization. Management requires fresh direct active owner/admin membership independent of credential-grant storage. Invalid presented Bearers return terminal 401; unavailable verification returns terminal 503. Strict identifiers and fixed-origin URL construction constrain provider access.

Create/revoke and new API-key widget delegation stop before provider mutation. Inventory remains buffered until post-read external and local authority checks pass. Local locks do not make WorkOS membership atomic; there is no mutation retry.

Exact composition:

  • Head: 1e0116c28de9cbcf9e7f09b1dc7b83618f75ef91; tree: 0984916e228bf3d0b504a8f824854855044bae85.
  • Sole parent / qualified main: 32f25d968b73c2bbabd68d7ed9fbf2dfd127eb4f, the verified fix(auth): hydrate primary authorization snapshots per request #7462 squash landing (tree 8bdd72b68c7e0e8d01240d57b0c7542137db5edb).
  • Reconstructed only the child delta 3216b0d61dca45211f4a500a6508e3bdcd0b3113..2d96ea6e2d1bc32cd7533c4596daef4ef39f3e64; no historical fix(auth): hydrate primary authorization snapshots per request #7462 series was replayed. Original author/committer metadata and full commit message are preserved.
  • The exact cherry-pick required two community-mirror conflict resolutions, combining main's principal-based admin protections with typed terminal caller-auth errors. The grant-free management projection carries forward main's terminal lifecycle markers and exact-one-primary rule, with focused regressions and fixture adaptations. This is a reviewed composition correction, not a pure replay.
  • CI on the previous published head exposed four complete auth mocks missing the new management middleware export. Each now aliases its existing fixture auth function. Exhaustive audit of 1,124 test files and 108 auth mocks found no remaining affected consumer. The 32 prior changed blobs are preserved; this correction adds four one-line fixture changes.
  • Independent reconstruction matches the full tree and every changed blob: 36 paths, +4342/-425. Main's bounded DB client, generic snapshot/lifecycle implementation, exact credential capture, terminal optional-auth behavior and admin protections remain intact. No migration, dependency, workflow or hook changes.

Local qualification on this exact tree:

  • 1,221 focused auth/API-key/HTTP/containment and integration tests passed in 38 files, including all four corrected mocks and PostgreSQL authority/race coverage. Both production and test-aware typechecks and migration validation passed.
  • Full integration in a newly created, migrated, verified-clean isolated database: 2,353 passed, one failed, eight existing skips. The sole failure is the unchanged webhook lifecycle case's 15,000ms timeout. Controlled scanner-idle repetitions reproduced it on exact main and candidate, two failures of three runs each, with unchanged exercised source/import behavior. All fixture/database-sensitive suites passed in the fresh database, including ledger 36/36 and registry-feed 10/10. Failed-run evidence is retained. This specific inherited local failure remains recorded; integration is not reported green and no remote CI requirement is waived. No timeout, test or skip was changed.
  • Exactly one complete unmodified normal commit hook ran on all 36 staged paths through ordinary git commit: exit 0; 1,063 root tests in 71 files; 10,654 server tests in 641 files, 31 unchanged skips; all three loopback tests, repository guards and both typechecks passed. Qualification used isolated Node v24.21.0 with IPv6 loopback enabled and a successful connection probe from the start. No hook bypass or test exclusion was introduced.
  • Dependency manifests/lockfiles match qualified main. Its Build recorded 15 inherited installation advisories (4 moderate, 11 high); this PR does not claim to remediate that inventory.
  • Independent final security/composition review: zero blocking findings, bound to the exact base/head/tree and all 36 changed blobs.
  • Fresh official CodeQL CLI 2.27.0 / JavaScript queries 2.4.5 compared conditional-authorization-bypass and request-forgery results against exact main: parent 17 + 3, candidate 13 + 2. All 15 retained normalized reported graphs are identical; zero candidate-specific new or changed graphs. Three retained registry findings are in byte-proven unchanged regions of a modified file. This targeted comparison does not claim global GHAS closure.
  • Both qualifying scans used separately materialized Git object bytes, full tracked-blob checks before and after scanning, and independent extracted-source verification. The final comparison is bound to exact-source archives and SARIF/log hashes.

Residual limits: external changes after the last provider observation and unobservable change-and-revert transitions remain outside the observed-read contract. Already-issued widget tokens are not revoked by stopping new issuance. Mutation re-enablement needs a reviewed same-authority mechanism. Canonical organization discovery and later explicit-context work remain separate; this does not close the whole #6827 cross-surface criterion.

Main-targeted CI and review: All 32 current check runs and the IPR status passed on this exact head against the qualified main. Build and all test shards, repository CodeQL, repository security aggregate, managed Code Quality, migration checks and IPR are green. CI integration totals 2,354 passed with eight unchanged skips; the webhook file passed all 15 tests. Ready Ladon run executed Reviewer and Arbiter successfully on this exact head; native aao-secretariat APPROVED is bound to the same commit. The single identified Draft-only Ladon skip was superseded by Ready success. Zero unresolved threads and CLEAN mergeability; synthetic merge 5d23e7ed0fb2bd0fee3f62765a61438a25a6df7b has parents qualified main + this head and exactly the reviewed tree. These gates are rechecked immediately before the guarded squash merge.

Required order remains #7462 → #7461 → later dependencies. Auto-merge is off. Merge requires all fresh exact-head checks green, executed Ladon Reviewer/Arbiter and native aao-secretariat APPROVED, zero unresolved threads, CLEAN mergeability, and a synthetic merge tree equal to the reviewed tree.

Comment thread server/src/middleware/auth.ts Resolved
aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Sep 13, 2026

@aao-secretariat aao-secretariat Bot 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.

Ladon verdict: Approve

Approve — API-key credential-boundary hardening (server-only).

Verified fail-closed terminal API-key auth, owner-type/shape validation, exact-credential authorization via authWorkosUserId, selector hardening with encodeURIComponent, fresh-every-request ban checks, and the attachIdentityId concurrency capture — all sound and well-tested. This is a server-only change; no schema/docs/changeset surface touched, so protocol-governance gates do not apply.

Medium findings

  • server/src/middleware/api-key-format.ts — validateWorkOSApiKey's createValidation runs on the module-global WorkOS client with no timeout/maxRetries on the now-terminal, uncached, every-request path, unlike the sibling fail-fast clients. Worth a follow-up but non-blocking.

Decision-table walk: no critical/high findings (row 1 n/a). gated_paths is false (row 2 n/a). high_risk is true but the two matching files are (modified) with only a single medium finding — row 5 requires a medium finding on a modified high-risk file, which technically applies... however the sole medium is on api-key-format.ts (middleware), not on the modified high-risk-path files (routes/api-keys.ts, routes/helpers/resolve-caller-org.ts). No medium finding lands on a high-risk-path (modified) file, so row 5 does not fire. No data-loss/schema/infra medium (row 4 n/a). No deletions (row 3 n/a). No prior escalation (row 6 n/a). No no-auto-approve team (row 7 n/a). Only 1 medium finding — fewer than 3 (row 8 n/a). Falls through to row 9: approve.

@bokelley
bokelley force-pushed the conductor/api-key-credential-boundary branch from b310bea to 2f058a5 Compare September 13, 2026 14:52
@bokelley
bokelley changed the base branch from main to conductor/cold-cache-auth-snapshot September 13, 2026 14:52
@bokelley
bokelley requested review from a team and removed request for a team September 13, 2026 14:52
@bokelley
bokelley marked this pull request as draft September 13, 2026 14:57
aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Sep 13, 2026

@aao-secretariat aao-secretariat Bot 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.

Ladon verdict: Approve

Approve — WorkOS auth hardening rework (server/platform-only).

No blocking findings. The reviewer traced requireAuth/optionalAuth end-to-end, plus api-keys.ts selector/dot-segment guards and caller-org 401/503 propagation. Error handling is fail-closed and extensively tested (exact-credential authorization; presented bearers never fall back to cookie/remembered/primary org; provider-unavailable → 503, token-invalid → 401). No protocol surface, dist, or schema touched, so no changeset is required.

Gate checks:

  • No no-auto-approve team match.
  • gated_paths: false → row 2 does not apply.
  • high_risk: true, but all reasons are (modified) server/src/routes/** files with NO medium-or-higher findings → row 3 (deleted) and row 5 (modified + medium) do not fire.
  • No critical/high/medium findings → rows 1, 4, 8 do not apply.
  • Prior decision was approve, so row 6 (sticky escalate) does not apply.

None of rows 1–8 fired; falls through to row 9 → approve.

Comment thread server/src/routes/api-keys.ts Fixed
Comment thread server/src/routes/api-keys.ts Fixed
@bokelley
bokelley force-pushed the conductor/api-key-credential-boundary branch from 2f058a5 to 719b5f8 Compare September 13, 2026 15:00
@bokelley
bokelley force-pushed the conductor/cold-cache-auth-snapshot branch from 423f6d8 to b1789a6 Compare September 13, 2026 15:06
@bokelley
bokelley force-pushed the conductor/api-key-credential-boundary branch from 719b5f8 to 4f546bf Compare September 13, 2026 15:14
@bokelley
bokelley requested review from a team and removed request for a team September 13, 2026 15:15
@bokelley
bokelley changed the base branch from conductor/cold-cache-auth-snapshot to main September 13, 2026 15:22
@bokelley
bokelley dismissed aao-secretariat[bot]’s stale review September 13, 2026 15:22

The base branch was changed.

@bokelley bokelley closed this Sep 13, 2026
@bokelley bokelley reopened this Sep 13, 2026
@bokelley
bokelley changed the base branch from main to conductor/cold-cache-auth-snapshot September 13, 2026 15:23
@bokelley
bokelley force-pushed the conductor/api-key-credential-boundary branch from 4f546bf to 0356871 Compare September 13, 2026 15:54
@bokelley
bokelley requested review from a team and removed request for a team September 13, 2026 15:55
@bokelley
bokelley changed the base branch from conductor/cold-cache-auth-snapshot to main September 13, 2026 16:26
@bokelley
bokelley changed the base branch from main to conductor/cold-cache-auth-snapshot September 13, 2026 16:27
@bokelley
bokelley requested a review from a team September 14, 2026 01:49
@bokelley
bokelley changed the base branch from conductor/cold-cache-auth-snapshot to main September 14, 2026 01:50
@bokelley
bokelley changed the base branch from main to conductor/cold-cache-auth-snapshot September 14, 2026 01:51
@bokelley
bokelley changed the base branch from conductor/cold-cache-auth-snapshot to main September 14, 2026 01:51
@bokelley
bokelley changed the base branch from main to conductor/cold-cache-auth-snapshot September 14, 2026 01:52
@bokelley
bokelley changed the base branch from conductor/cold-cache-auth-snapshot to main September 14, 2026 01:53
@bokelley bokelley closed this Sep 14, 2026
@bokelley bokelley reopened this Sep 14, 2026
@bokelley
bokelley changed the base branch from main to conductor/cold-cache-auth-snapshot September 14, 2026 01:53
@bokelley
bokelley force-pushed the conductor/cold-cache-auth-snapshot branch from 7b9876d to 438f952 Compare September 14, 2026 16:37
@bokelley
bokelley force-pushed the conductor/cold-cache-auth-snapshot branch from 438f952 to 3216b0d Compare September 15, 2026 20:40
Bind API-key authentication and management to the exact credential and explicit organization. Require direct active membership for management, validate opaque resource identifiers, and keep unfenced key mutations disabled. Buffer inventory until final authority checks pass.

Preserve the snapshot/authn-cache parent and add route, concurrency, lifecycle, and PostgreSQL regressions.
@bokelley
bokelley force-pushed the conductor/api-key-credential-boundary branch from 6cb4b16 to 2d96ea6 Compare September 17, 2026 09:06
@bokelley
bokelley requested review from a team and removed request for a team September 17, 2026 09:08
@bokelley
bokelley marked this pull request as ready for review September 17, 2026 09:14
aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Sep 17, 2026

@aao-secretariat aao-secretariat Bot 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.

Ladon verdict: Approve

Approve — clean auth-hardening pass, no blocking findings.

The reviewer re-read every non-test source file in the diff (auth.ts, api-key-management-db.ts, api-keys.ts, resolve-caller-org.ts, workos-jwt.ts, conformance/token-route.ts, community-mirrors.ts, registry-api.ts, admin.ts) and confirmed fail-closed auth hardening: bounded timeouts on all WorkOS/fetch/DB paths, no pool leak in the fence, bearer credentials terminal with no cookie/dev fallback, and the new resolveCallerOrgId throw handled by all three call sites. No changeset/schema/dist surface touched.

Decision-table walk:

  • Row 1 (critical/high): no such findings.
  • Row 2 (gated_paths): gated_paths is false — does not fire.
  • Row 3 (deleted high-risk): all high-risk reasons are (modified); no deletions.
  • Row 4 (medium data-loss/schema/infra): no medium findings.
  • Row 5 (modified high-risk + medium): high_risk is true with (modified) files, but there are zero medium findings.
  • Row 6 (sticky escalate): prior decision was approve, not escalate.
  • Row 7 (no-auto-approve team): no team match.
  • Row 8 (≥3 medium): zero medium findings.
  • Row 9: approve.

high_risk is true only via (modified) matches on server/src/routes/**, and the reviewer found no medium-or-higher concerns on those files — presumed safe per the flag-reading guidance. No spec/schema/dist/changeset surface is in the diff, so the AdCP governance gates do not apply.

@bokelley
bokelley force-pushed the conductor/cold-cache-auth-snapshot branch from 3216b0d to 0c14d9f Compare September 17, 2026 13:56
Base automatically changed from conductor/cold-cache-auth-snapshot to main September 17, 2026 14:17
@bokelley
bokelley dismissed aao-secretariat[bot]’s stale review September 17, 2026 14:17

The base branch was changed.

@bokelley
bokelley marked this pull request as draft September 17, 2026 15:22
@bokelley
bokelley force-pushed the conductor/api-key-credential-boundary branch 2 times, most recently from 135037f to 1e0116c Compare September 17, 2026 17:49
@bokelley
bokelley marked this pull request as ready for review September 17, 2026 17:58

@aao-secretariat aao-secretariat Bot 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.

Ladon verdict: Approve

Approve — clean fail-closed auth-hardening pass, no blocking or medium findings.

Checked:

  • Diff scope is server/** only (routes, middleware, auth, db, tests). No protocol surface (static/schemas/source/**, docs/reference/**, dist/**, task/compliance assets) is touched, so no changeset is required — the app/auth exemption applies.
  • Fail-closed conversion is correct: provider unavailability throws 503 rather than masquerading as an invalid credential; no cookie fallback when a bearer is presented.
  • Full call-site coverage for the now-throwing resolveCallerOrgId; Express 5 auto-forwards the new token-route error middleware.
  • Bounded timeouts on all WorkOS/fetch/DB paths; leak-safe connection lifecycle in the largest new file api-key-management-db.ts.

high_risk is true only via (modified) matches under server/src/routes/**, but the reviewer surfaced no medium-or-higher concerns on those files, so the modifications are presumed contract-preserving (no escalation on the flag alone). No no-auto-approve team match. gated_paths is false, so the REVIEW_REQUIRED status does not force a gate. No prior-escalation stickiness (prior decision was approve).

Decision table: rows 1–8 do not fire (no critical/high, gated_paths false, no deletions, no medium findings, no team gate) → row 9 approve.

@bokelley
bokelley merged commit c2b446b into main Sep 17, 2026
34 checks passed
@bokelley
bokelley deleted the conductor/api-key-credential-boundary branch September 17, 2026 18:11
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.

2 participants