|
| 1 | +# Fix Deferred Security Findings from Codex Token Refresh Proxy (PR #600) |
| 2 | + |
| 3 | +## Problem Statement |
| 4 | + |
| 5 | +PR #600 (codex-token-refresh-proxy) shipped with deferred security findings from specialist reviewers. CRITICAL/HIGH actionable items were fixed pre-merge, but these items were explicitly deferred and need follow-up. |
| 6 | + |
| 7 | +## Research Findings |
| 8 | + |
| 9 | +### 1. Token-in-URL (HIGH) |
| 10 | +- The endpoint at `apps/api/src/routes/codex-refresh.ts:47` extracts the callback token from `?token=` query param |
| 11 | +- This is a hard constraint — Codex CLI's built-in refresh logic cannot set custom HTTP headers |
| 12 | +- The VM agent injects `CODEX_REFRESH_TOKEN_URL_OVERRIDE` with the token in the URL at `packages/vm-agent/internal/acp/session_host.go:884-908` |
| 13 | +- Current callback token lifetime: **24 hours** (configurable via `CALLBACK_TOKEN_EXPIRY_MS`) |
| 14 | +- Tokens are RS256-signed JWTs with workspace-scoped claims |
| 15 | +- Mitigations needed: formal risk documentation, access logging already present via `log.info('codex_refresh.request_received')` |
| 16 | + |
| 17 | +### 2. Scope Validation on Refreshed Tokens (HIGH) |
| 18 | +- `apps/api/src/durable-objects/codex-refresh-lock.ts:210` parses new tokens from OpenAI but does not validate scopes |
| 19 | +- OpenAI's token response may include a `scope` field |
| 20 | +- Should warn-log (not hard block) when unexpected scopes appear, for backward compatibility with legacy tokens |
| 21 | +- Implementation: after parsing `newTokens` at line 210, check for a `scope` field and log a warning if it contains unexpected values |
| 22 | + |
| 23 | +### 3. Rate Limiting (MEDIUM) |
| 24 | +- Existing rate limiting infrastructure at `apps/api/src/middleware/rate-limit.ts` — KV-based, per-user or per-IP, configurable |
| 25 | +- `DEFAULT_RATE_LIMITS` object and factory functions for creating rate limit middleware |
| 26 | +- The codex-refresh endpoint currently has NO rate limiting (relies on per-user DO serialization for correctness, but not abuse prevention) |
| 27 | +- The endpoint uses workspace callback token auth (not session auth), so we need a custom approach: rate limit per workspaceId (extracted from the verified JWT) |
| 28 | +- Env type at `apps/api/src/index.ts:124-192` already has `RATE_LIMIT_*` pattern |
| 29 | +- Need to add `RATE_LIMIT_CODEX_REFRESH` to Env and `DEFAULT_RATE_LIMITS` |
| 30 | + |
| 31 | +### 4. JWT Lifetime Review (MEDIUM) |
| 32 | +- Callback token lifetime: 24h default (`apps/api/src/services/jwt.ts:36-39`, `CALLBACK_TOKEN_EXPIRY_MS`) |
| 33 | +- Terminal token lifetime: 1h default (`TERMINAL_TOKEN_EXPIRY_MS`) |
| 34 | +- MCP token TTL: 4h default (`MCP_TOKEN_TTL_SECONDS`) |
| 35 | +- 24h is appropriate for workspace callback tokens since workspaces may run for extended periods |
| 36 | +- The auto-refresh mechanism at 50% of lifetime (`CALLBACK_TOKEN_REFRESH_THRESHOLD_RATIO`) ensures tokens are renewed during heartbeats |
| 37 | +- Document this design decision in secrets taxonomy |
| 38 | + |
| 39 | +### Key Files |
| 40 | +- `apps/api/src/routes/codex-refresh.ts` — Refresh endpoint (139 lines) |
| 41 | +- `apps/api/src/durable-objects/codex-refresh-lock.ts` — Per-user lock DO (265 lines) |
| 42 | +- `apps/api/src/middleware/rate-limit.ts` — Rate limiting middleware (229 lines) |
| 43 | +- `apps/api/src/services/jwt.ts` — JWT signing/verification |
| 44 | +- `apps/api/src/index.ts` — Env type, route registration |
| 45 | +- `docs/architecture/secrets-taxonomy.md` — Secrets documentation |
| 46 | +- `apps/api/tests/unit/routes/codex-refresh.test.ts` — Existing tests |
| 47 | + |
| 48 | +## Implementation Checklist |
| 49 | + |
| 50 | +### Token-in-URL Documentation |
| 51 | +- [ ] Add "Accepted Risks" section to `docs/architecture/secrets-taxonomy.md` |
| 52 | +- [ ] Document token-in-URL constraint, mitigations (short-lived JWT, scope enforcement, access logging), and accepted risk |
| 53 | + |
| 54 | +### Scope Validation |
| 55 | +- [ ] Add scope validation in `codex-refresh-lock.ts` after parsing upstream response (line ~210) |
| 56 | +- [ ] Define expected scopes constant (configurable via `CODEX_EXPECTED_SCOPES`) |
| 57 | +- [ ] Log warning (not error) when scope field present with unexpected values |
| 58 | +- [ ] Pass through tokens regardless (warning only, not blocking) |
| 59 | +- [ ] Add unit tests for scope validation (expected scopes, unexpected scopes, missing scope field) |
| 60 | + |
| 61 | +### Rate Limiting |
| 62 | +- [ ] Add `CODEX_REFRESH` to `DEFAULT_RATE_LIMITS` (default: 30 per hour per workspace) |
| 63 | +- [ ] Add `RATE_LIMIT_CODEX_REFRESH` to Env type |
| 64 | +- [ ] Create `rateLimitCodexRefresh()` factory function in rate-limit.ts |
| 65 | +- [ ] Apply rate limiting in `codex-refresh.ts` after token verification (use workspaceId as identifier) |
| 66 | +- [ ] Add unit tests for rate limiting behavior (allowed, exceeded) |
| 67 | + |
| 68 | +### JWT Lifetime Documentation |
| 69 | +- [ ] Add JWT lifetime reference table to secrets taxonomy |
| 70 | +- [ ] Document callback token 24h lifetime rationale and auto-refresh mechanism |
| 71 | + |
| 72 | +### Documentation Sync |
| 73 | +- [ ] Update CLAUDE.md recent changes if needed |
| 74 | + |
| 75 | +## Acceptance Criteria |
| 76 | +- [ ] Token-in-URL risk formally documented in secrets taxonomy with mitigations |
| 77 | +- [ ] Scope validation added to token refresh response (warning log, not hard block) |
| 78 | +- [ ] Rate limiting added to `/api/auth/codex-refresh` endpoint |
| 79 | +- [ ] JWT lifetime for workspace callback tokens reviewed and documented |
| 80 | +- [ ] Tests cover rate limiting behavior and scope validation |
0 commit comments