Skip to content

fix(auth): compare API keys at the raw ASGI boundary - #520

Draft
seonghobae wants to merge 13 commits into
mainfrom
sentinel/fix-hmac-non-ascii-1412282215803553404
Draft

fix(auth): compare API keys at the raw ASGI boundary#520
seonghobae wants to merge 13 commits into
mainfrom
sentinel/fix-hmac-non-ascii-1412282215803553404

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Current repair boundary

The weak fix for hmac.compare_digest(str, str) is to re-encode Starlette's decoded header string. That can corrupt the credential boundary and does not resolve duplicate-header ambiguity. This canonical lane instead authenticates from the raw ASGI header bytes: exactly one X-API-Key is accepted and compared in constant time with each configured UTF-8 key.

Current protected main: 47c6fd27de13b0da37a7db64697b869941909351.
Current exact head: 08d1c3ca20e3e41a703ac9e5c5aea6f702962d2d.
Fresh compare: ahead 13 / behind 0, merge base exactly current protected main.
Lifecycle: Draft.

Effective protected-base delta is now exactly five paths; the earlier generated .jules/sentinel.md change has been removed from the current diff:

  • CHANGELOG.md;
  • saas_web.py;
  • tests/test_api_key_header_multiplicity.py;
  • tests/test_api_key_unicode_contract.py;
  • tests/test_saas_web.py.

RED → GREEN

  • configured Unicode key 안녕 with identical raw UTF-8 header bytes must reach the protected handler;
  • a different Unicode/raw high-bit credential must fail closed with 401 rather than raising through compare_digest;
  • duplicate X-API-Key headers must fail closed whether their values match or differ;
  • missing/invalid credentials must not be echoed or reach the protected handler.

_raw_api_key_header() reads request.scope['headers'] directly and authorizes only one raw value. The middleware compares that byte sequence with configured UTF-8 key bytes using hmac.compare_digest. The focused regressions cover valid Unicode, wrong Unicode/high-bit input, and duplicate authority. The CHANGELOG records the public credential-boundary behavior.

Sibling #527's only valid non-ASCII 500→401 intent/regression is fully superseded here; #527 was closed only after that succession was verified. Its blanket “always encode user-controlled strings before compare_digest” doctrine and unsupported CRITICAL/process-crash framing were not inherited.

Remaining governance finding

CODEC_CARVER_API_KEYS is still an environment-variable credential registry. That existing governance finding is independent of the transport-encoding repair and remains a promotion blocker until it is repaired here or completely inherited by a canonical successor. Do not dilute the raw-header fix by reverting to framework-decoded credential authority.

Exact-head hosted evidence and foreign-owner RCA

On exact 08d1c3ca20e3e41a703ac9e5c5aea6f702962d2d:

  • CI 33905349122: success;
  • fuzz 33905349153: success;
  • Security Scan 33905349127: success;
  • SAST Semgrep 33905348978: success;
  • CodeQL PR 33905348976: failure.

The CodeQL failure is central control-plane evidence, not a local source failure. Detect job 101128928869 succeeded on the exact head. Both compatibility jobs successfully requested current-head dispatch, then failed at Fail closed without a current-head CodeQL dispatch verdict: python job 101188835268, actions job 101188835300. The owner-path evidence has been handed to dedicated .github repair lane #1902. Leaf no-op commits, synthetic status, gate weakening, or broad timeout changes are not authorized substitutes.

Keep Draft until the unchanged exact head receives authenticated terminal CodeQL verdicts after the central repair, the credential-registry governance finding is resolved or canonically inherited, all valid review threads are resolved, and live protection has qualifying independent approval.

No self-approval, administrator bypass, force push, destructive rebase, or predecessor-evidence transfer.

@google-labs-jules

Copy link
Copy Markdown

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: bae2c2b8-d69c-4d92-ac41-422fd772cff1

📥 Commits

Reviewing files that changed from the base of the PR and between a67bc12 and 4710e64.

📒 Files selected for processing (1)
  • tests/test_saas_web.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/test_saas_web.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

API 키 검증이 문자열 비교에서 UTF-8 바이트 비교로 변경되었습니다. 비ASCII API 키 헤더의 401 응답을 검증하는 회귀 테스트와 보안 학습 문서가 추가되었습니다.

Changes

API 키 검증

Layer / File(s) Summary
UTF-8 바이트 비교 및 회귀 검증
saas_web.py, tests/test_saas_web.py, .jules/sentinel.md
hmac.compare_digest가 UTF-8 바이트를 비교하도록 변경되었습니다. 비ASCII API 키 헤더는 401Invalid or missing API key 오류를 반환합니다. 동일한 취약점과 예방 조치가 보안 문서에 기록되었습니다.

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

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 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 제목은 API 키 인증 비교 변경을 정확히 설명합니다. raw ASGI boundary에서 API 키를 비교한다는 내용은 Non-ASCII 입력을 바이트로 처리하는 주요 변경과 관련됩니다.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sentinel/fix-hmac-non-ascii-1412282215803553404

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.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 3 potential issues.

Devin Review

Comment thread saas_web.py

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔍 API-key storage still violates governance

get_configured_api_keys still reads secrets from the environment. AGENTS.md explicitly requires migrating this known deviation to a credential registry.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread saas_web.py Outdated
Comment thread tests/test_saas_web.py Outdated
coderabbitai[bot]

This comment was marked as resolved.

@seonghobae
seonghobae marked this pull request as draft September 3, 2026 11:15
@seonghobae seonghobae changed the title 🛡️ Sentinel: [MEDIUM] hmac.compare_digest의 Non-ASCII 예외 처리 수정 fix(auth): compare API keys at the raw ASGI boundary Sep 3, 2026
seonghobae and others added 2 commits September 4, 2026 12:46
…- Fixes a 500 error in `hmac.compare_digest` where non-ASCII API keys\n (e.g., Unicode sequences) raised a `TypeError` by explicitly extracting\n and decoding raw ASGI byte strings.\n- Prevents HTTP header injection / ambiguity bypasses by strictly failing\n closed if multiple `X-API-Key` headers are provided.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: medium Normal-priority or P2 work type: bug Defect or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant