Skip to content

fix: block unexpected Codex refresh scopes#1412

Draft
simple-agent-manager[bot] wants to merge 4 commits into
mainfrom
sam/codex-refresh-scope-block-01kvzg
Draft

fix: block unexpected Codex refresh scopes#1412
simple-agent-manager[bot] wants to merge 4 commits into
mainfrom
sam/codex-refresh-scope-block-01kvzg

Conversation

@simple-agent-manager

Copy link
Copy Markdown
Contributor

Summary

  • Change Codex OAuth refresh scope validation to fail closed on unexpected upstream scopes.
  • Remove the stale CODEX_SCOPE_VALIDATION_MODE env surface so warn-and-persist cannot be restored accidentally.
  • Update refresh-lock tests to assert rejected rotations do not encrypt or update stored credentials, while expected scopes and CODEX_EXPECTED_SCOPES="" still succeed.

Validation

  • pnpm lint
  • pnpm typecheck
  • pnpm test
  • Additional validation run: pnpm build
  • Additional validation run: pnpm --filter @simple-agent-manager/api test -- tests/unit/durable-objects/codex-refresh-lock.test.ts
  • Additional validation run: pnpm --filter @simple-agent-manager/api typecheck
  • Additional validation run: pnpm --filter @simple-agent-manager/api lint

Note: first full pnpm test attempt hit an unrelated @simple-agent-manager/infra dns.test.ts beforeAll timeout. Direct retry of pnpm --filter @simple-agent-manager/infra test passed, and full pnpm test then passed.

Staging Verification (REQUIRED for all code changes — merge-blocking)

  • Staging deployment green — Not run. User explicitly instructed: DO NOT DEPLOY TO STAGING.
  • Live app verified via Playwright — Not run. User explicitly instructed human owns staging verification.
  • Existing workflows confirmed working — Not run on staging.
  • New feature/fix verified on staging — Not run on staging.
  • Infrastructure verification completed — N/A: no infra changes.
  • Mobile and desktop verification notes added for UI changes — N/A: no UI changes.

Staging Verification Evidence

Not performed by explicit task constraint. This PR is draft and labeled needs-human-review; human reviewer owns staging verification and merge.

UI Compliance Checklist (Required for UI changes)

  • N/A: no UI changes.

End-to-End Verification (Required for multi-component changes)

  • Data flow traced from user input to final outcome with code path citations.
  • Capability test exercises the complete happy path across system boundaries.
  • All spec/doc assumptions about existing behavior verified against code.
  • If any gap exists between automated test coverage and full E2E, manual verification steps documented below.

Data Flow Trace

Codex refresh request enters CodexRefreshLock.fetch() and calls the upstream token endpoint. After successful upstream response parsing, apps/api/src/durable-objects/codex-refresh-lock.ts validates newTokens.scope before recordRotatedToken(), encrypt(), and UPDATE credentials. On validation failure it returns 502 upstream_unexpected_scope, leaving the prior credential row unchanged.

Untested Gaps

No staging/live-provider verification was run by explicit instruction. Local unit coverage uses mocked upstream refresh responses and asserts both response behavior and D1/encryption side effects.

Post-Mortem (Required for bug fix PRs)

What broke

Codex token refresh accepted rotated upstream tokens with unexpected scopes by default and persisted them after warning.

Root cause

The refresh lock had a validation mode fallback of ?? 'warn', causing unexpected scopes to log and proceed unless an explicit block mode was configured.

Class of bug

Credential rotation validation failed open due to warn-only fallback behavior.

Why it wasn't caught

Existing tests encoded warn-and-persist as the expected default behavior.

Process fix included in this PR

Tests now assert fail-closed behavior, non-persistence on rejected rotations, default allowlist enforcement when CODEX_EXPECTED_SCOPES is absent, and explicit opt-out only via CODEX_EXPECTED_SCOPES="".

Post-mortem file

tasks/archive/2026-06-25-codex-refresh-scope-block.md

Specialist Review Evidence (Required for agent-authored PRs)

  • All local reviewers completed and findings addressed before merge
  • If any reviewer did NOT complete: needs-human-review label added and merge deferred to human
Reviewer Status Outcome
security-auditor PASS No critical/high findings. Unexpected scopes now fail before rotated-token grace recording, encryption, and D1 credential update.
cloudflare-specialist PASS No D1/DO concern found; rejection occurs before D1 mutation and keeps existing Durable Object flow intact.
test-engineer PASS Targeted tests cover unexpected-scope rejection, no persistence, expected-scope success, unset default allowlist, empty allowlist opt-out, and stale warn-mode inertness.
constitution-validator PASS No new hardcoded URL/timeout/limit issue; default scope allowlist remains an explicit configurable default with CODEX_EXPECTED_SCOPES override.
env-validator PASS Removed stale CODEX_SCOPE_VALIDATION_MODE from runtime Env interface; no runtime/docs references remain outside the regression test.
task-completion-validator PASS Research findings, checklist items, and acceptance criteria are represented in the diff and local tests.

Exceptions (If any)

  • Scope: Staging verification and merge.
  • Rationale: User explicitly instructed DO NOT DEPLOY TO STAGING and DO NOT MERGE; human owns staging verification and merge.
  • Expiration: Until human review completes staging verification.

Agent Preflight (Required)

  • Preflight completed before code changes

Classification

  • external-api-change
  • cross-component-change
  • business-logic-change
  • public-surface-change
  • docs-sync-change
  • security-sensitive-change
  • ui-change
  • infra-change

External References

N/A: no external API contract change; work was based on local task context, rule 28, and existing code/tests.

Codebase Impact Analysis

Affected components: Codex refresh Durable Object, API Env interface, and unit tests for codex-refresh-lock.

Documentation & Specs

No user-facing docs changed. The stale env interface entry was removed because CODEX_SCOPE_VALIDATION_MODE is no longer a supported runtime control. Task archived at tasks/archive/2026-06-25-codex-refresh-scope-block.md.

Constitution & Risk Check

Checked fail-closed credential rotation behavior, no hardcoded-value regression, and env consistency. Main behavior tradeoff: unexpected provider scope changes now block refresh unless explicitly opted out with CODEX_EXPECTED_SCOPES="", matching Rule 28.

@simple-agent-manager simple-agent-manager Bot added the needs-human-review Agent could not complete all review gates — human must approve before merge label Jun 25, 2026
@sonarqubecloud

Copy link
Copy Markdown

@codspeed-hq

codspeed-hq Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 6 untouched benchmarks


Comparing sam/codex-refresh-scope-block-01kvzg (2b2eedc) with main (bc159a6)

Open in CodSpeed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human-review Agent could not complete all review gates — human must approve before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant