Skip to content

fix(oauth): redact secrets in structured event values - #2226

Merged
lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:agent/oauth-log-structured-redaction-current
Aug 21, 2026
Merged

lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:agent/oauth-log-structured-redaction-current

Conversation

@luvs01

@luvs01 luvs01 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • apply the shared secret redactor to the final structured OAuth log line so token-shaped values in otherwise safe fields, including cleanup error text, cannot reach console.info
  • suppress code_verifier by normalized field name before serialization
  • preserve existing provider output, masked account identifiers, safe diagnostic fields, and event wording

Exact base: 03735eca62398c55056d4595145561aecc444e91
Exact head: 1435ec5f4fdb259f0d2a449a41407954bed96137

Verification

  • Bun 1.4 OAuth logger, shared redactor, PKCE/manual-code, and refresh flows — 92/92 passed (433 assertions)
  • Bun 1.4 typecheck — passed
  • privacy scan and git diff --check — passed
  • independent current-diff security/correctness review — no findings

The full repository suite was not duplicated locally; maintained cross-platform CI remains the merge gate. No OAuth exchange, credential storage, GUI, or configuration behavior is changed.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed; this internal log-sink hardening adds no user-facing workflow or configuration.
  • Security-sensitive changes were reviewed for secret exposure, account masking, false-positive behavior, and unsafe logging defaults. Maintainer security review and sponsorship remain required for the OAuth credential surface.

Review readiness checklist

  • All CI tests are green on my local testing.
  • I pushed my PR to the latest dev commit.
  • I resolved all correct Codex and CodeRabbit findings.
  • My PR is ready for review.

@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Aug 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/oauth/log.ts.

@github-actions github-actions Bot added the bug Something isn't working label Aug 20, 2026
@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bd0963ff-3f9d-44b0-b5c3-eed8d85145c1

📥 Commits

Reviewing files that changed from the base of the PR and between 03735ec and 1435ec5.

📒 Files selected for processing (2)
  • src/oauth/log.ts
  • tests/oauth-log.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

OAuth logging now sanitizes assembled messages with redactSecretString and excludes code_verifier as a forbidden field key. Tests cover both key omission and redaction of token-shaped values in safe fields.

Changes

OAuth Log Redaction

Layer / File(s) Summary
Apply OAuth log sanitization
src/oauth/log.ts:3, src/oauth/log.ts:24, src/oauth/log.ts:49
The logger imports redactSecretString, adds code_verifier to forbidden keys, and sanitizes the assembled message before calling console.info.
Validate redaction behavior
tests/oauth-log.test.ts:44, tests/oauth-log.test.ts:68-94
Tests verify that code_verifier is omitted and that token-shaped values in cause and request are replaced with [REDACTED].

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 1435e

The change narrowly strengthens secret redaction in OAuth structured logs without changing OAuth exchanges or configuration behavior; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: wibias

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: redacting secrets in OAuth structured event values.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (3/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 3/4).

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ⬜ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

3/4 boxes ticked.

Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked.

@github-actions
github-actions Bot marked this pull request as draft August 20, 2026 20:19
@luvs01

luvs01 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

@lidge-jun @Ingwannu — exact-head author review is complete: 92/92 focused tests, typecheck, privacy/diff checks, and CodeRabbit with no actionable findings. The only remaining blocker is the repository-required security review and maintainer-sponsored label for src/oauth/log.ts; no further author code changes are pending.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 60 / 80

키 필터만 있고 값 필터가 없음. 지금 dev src/oauth/log.ts logOAuthEventisForbiddenFieldKey로 필드 이름만 봄. access_token / client_secret은 스킵. cause / request / cleanup 에러 텍스트는 통과해서 console.info로 감. Bearer나 sk-가 그 문자열에 있으면 그대로 찍힘. code_verifier는 셋에 없고 endsWith("_code")도 안 맞음. PKCE 검증자가 키 이름으로 통과함.

이 PR이 code_verifierFORBIDDEN_NORMALIZED에 넣고, 직렬화 끝난 한 줄에 redactSecretString을 씌움. src/lib/redact.ts 공유 레드액터임. 키 거절 + 값 패턴 이중임. 프로바이더/마스킹된 account/이벤트 문구는 유지. 교환/저장/GUI는 안 만짐. 방향 맞음.

테스트가 금지 키 목록에 verifier를 넣고, cleanup 필드에 Bearer credentialvalue123456 / sk-fixturecredential을 넣음. 기대값이 Bearer [REDACTED]request=[REDACTED]. 공유 레드액터 계약을 로그 싱크가 우회하지 않게 잠근 거임.

draft고 intake: hygiene-blocked. unsponsored_surfacesrc/oauth/log.ts. 메인터가 maintainer-sponsored 달기 전엔 머지 금지. OAuth 교환을 안 바꿔도 로그 싱크가 크레덴셜 표면임. 프로세스 게이트가 안 열렸음.

types.ts/config.ts 스플릿 안 씹힘. #2188 사이드카, #2190 x_search랑 섞지 말 것. #2222 메인 리프레시랑도 다른 선임. 로그 하드닝만. base가 HEAD 03735eca6. 2.28 블로커 아님. 시크릿 유출이라 점수는 있음.

해결방안: hygiene 통과 + maintainer-sponsored 받은 뒤에 draft 해제. 라인 단위 redactSecretString 유지. 필드 키만 믿고 값 레드액션을 빼지 말 것. PKCE verifier를 다시 통과시키지 말 것.

이 댓글은 grok-bot이 작성했습니다

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved exact head 1435ec5 after explicit credential-log review. The change is narrowly scoped: forbidden key filtering now includes PKCE code_verifier, and the complete structured log line passes through the shared redactor before the console sink. I reproduced the stated 92/92 OAuth logger, redactor, manual-code, and refresh tests; typecheck, privacy scan, and diff checks also pass. Jun owner review independently confirmed the same boundary. Applying maintainer-sponsored; merge remains gated on ready state and exact-head required CI.

@Ingwannu
Ingwannu marked this pull request as ready for review August 21, 2026 02:34
@github-actions
github-actions Bot marked this pull request as draft August 21, 2026 02:34
@Ingwannu Ingwannu added the maintainer-sponsored Maintainer sponsors this change to an auth, workflow, release, or dependency surface label Aug 21, 2026
@github-actions github-actions Bot removed the intake: hygiene-blocked Deterministic PR hygiene checks failed label Aug 21, 2026
@Ingwannu
Ingwannu marked this pull request as ready for review August 21, 2026 02:40
@github-actions
github-actions Bot marked this pull request as draft August 21, 2026 02:40
@lidge-jun
lidge-jun marked this pull request as ready for review August 21, 2026 04:01
@lidge-jun
lidge-jun merged commit 0a120da into lidge-jun:dev Aug 21, 2026
14 of 19 checks passed
agentHits pushed a commit to agentHits/opencodex that referenced this pull request Sep 17, 2026
Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working maintainer-sponsored Maintainer sponsors this change to an auth, workflow, release, or dependency surface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants