security(auth): reject unsupported JWT critical headers - #895
security(auth): reject unsupported JWT critical headers#895seonghobae wants to merge 16 commits into
Conversation
|
👋 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 New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
Next review available in: 49 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughJWT 검증을 PyJWT와 ChangesJWT 검증 및
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟠 High · up to The authentication path may accept a token under an algorithm that conflicts with the signing key’s declared constraint, weakening signature-validation policy. This is a concrete security issue, so the PR is not merge-ready until the mismatch is rejected before decoding and covered by a regression test. Sequence Diagram(s)sequenceDiagram
participant OIDCToken as OIDC JWT
participant Decode as _decode_verified_oidc_token
participant Header as _validate_jwt_header
participant JWKS as _get_jwks
participant PyJWT as jwt.decode
OIDCToken->>Decode: JOSE 헤더 추출
Decode->>Header: crit 및 알고리즘 검증
Header-->>Decode: 정규화된 알고리즘 또는 401 오류
Decode->>JWKS: 유효한 헤더의 JWKS 조회
Decode->>PyJWT: PyJWK 키와 클레임 검증 옵션 전달
PyJWT-->>Decode: 검증된 JWT 클레임 반환
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
seonghobae
changed the title
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head137d943fac612a1b20023fea4e34a4d8d0898328. -
Head SHA:
137d943fac612a1b20023fea4e34a4d8d0898328 -
Workflow run: 32075757301
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Backend (3 files)"]
S2 --> I2["API and service runtime"]
I2 --> R2["Review risk: Backend (3 files)"]
R2 --> V2["backend tests"]
Evidence --> S3["Docs: jwt-critical-header-boundary.md"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs: jwt-critical-header-boundary.md"]
R3 --> V3["docs review"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Backend (3 files)"]
S2 --> I2["API and service runtime"]
I2 --> R2["Review risk: Backend (3 files)"]
R2 --> V2["backend tests"]
Evidence --> S3["Docs: jwt-critical-header-boundary.md"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs: jwt-critical-header-boundary.md"]
R3 --> V3["docs review"]
|
2157a64 to
af5cd48
Compare
|
@OpenCode review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
의존성 검사 과정에서 이를 해결하기 위해 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
JWT JOSE 헤더의 crit 파라미터가 포함된 경우를 안전하게 처리하기 위해 검증 로직을 추가했습니다. STRIX 보안 요건 및 RFC 7515에 따라 지원되지 않는 critical 확장을 명시적으로 거부하도록 합니다.
JWT JOSE 헤더의 crit 검증 로직을 상단으로 이동하여 RFC 7515 요구 사항에 맞게 처리되도록 수정했습니다.
- JWT JOSE 헤더의 `crit` (critical) 파라미터가 포함된 경우를 안전하게 처리하기 위해 검증 로직을 추가했습니다. STRIX 보안 요건 및 RFC 7515에 따라 지원되지 않는 critical 확장을 명시적으로 거부하도록 합니다. - RFC 7515 스펙에 따라 지원하지 않는 critical 파라미터가 최우선으로 거부되도록 `_validate_jwt_header`의 상단에서 `crit` 검증을 수행하도록 했습니다. - `crit` 헤더 검증에 관한 상세 테스트를 추가하고, mock 객체를 실제 동작에 맞게 수정했습니다. - 보안 취약점이 발견된 `python-jose` 패키지 종속성(ecdsa) 문제를 해결하기 위해 `PyJWT` 라이브러리로 마이그레이션했습니다.
8436cb4 to
f0e474e
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Current HEAD validationUpdated current HEAD to
Local evidence: backend Please evaluate reviews and required Checks against this exact HEAD; the push starts fresh PR workflows. |
Removed the unsupported `"verify_jti": True` option from the `jwt.decode` configuration in `backend/app/auth.py` and `test_auth_security.py`. PyJWT does not support this option and strict validation of the options dictionary causes authentication to fail entirely with a generic 401 error. The JTI requirement is already correctly enforced via the `"require": ["jti", ...]` list. Reverted out-of-scope frontend `Dockerfile` and `package.json` dependency bumps.
|
현재 HEAD |
확인했습니다. 알림 감사합니다. 추가 작업이 필요하면 알려주세요. |
Removed the unsupported `"verify_jti": True` option from the `jwt.decode` configuration in `backend/app/auth.py` and `test_auth_security.py`. PyJWT does not support this option and strict validation of the options dictionary causes authentication to fail entirely with a generic 401 error. The JTI requirement is already correctly enforced via the `"require": ["jti", ...]` list. Reverted out-of-scope frontend `Dockerfile` and `package.json` dependency bumps.
|
Current-head revalidation: |
확인했습니다. CI 파이프라인(exact-head, Security Scan, SAST Semgrep)이 큐에 있고 승인이 필요한 상태임을 인지했습니다. |
1 participant
Security outcome
OIDC bearer tokens now fail closed when their protected JOSE header declares a critical extension that this deployment profile does not understand and process. Rejection occurs before JWKS retrieval, so an unsupported extension cannot proceed by selecting another signing key or algorithm.
Exact current scope
73d64e095b1144a1bef82cd18d11864921fac7c2466e616e96fc3069a5022351db35fa378b342d64CHANGELOG.mdImplemented contract
A present
critmember is invalid when it is:nullor any non-array value;alg,typ,cty, orcrit.A structurally valid declaration still receives a fixed non-reflecting
401 unsupported critical parameter, because this application currently supports no critical JOSE extensions. Tokens that omitcritretain the existing token-type, content-type, algorithm allowlist, key-family, issuer, audience, expiry, and revocation boundaries.Verification design
Focused tests cover the supported no-
critpath, every malformed shape above, registered-name rejection, absent extension members, an RFC Appendix E-style unknown extension, and the production decode path proving no JWKS I/O occurs before rejection. Exact-head repository CI, coverage, SAST, Strix, OpenCode, and independent review remain authoritative.Standards
The implementation follows RFC 7515 Section 4.1.11 and Appendix E. The doctoring record also traces the deployment boundary to RFC 8725 (BCP 225). Adding support for any critical extension requires a separate ADR, extension-specific processing, negative tests, and security review; merely allowlisting a name is prohibited.
Merge boundary
Do not merge until this unchanged head has every required check terminal-success, zero valid unresolved findings, and any policy-required qualifying independent non-author approval. Queued, stale, absent, skipped-required, predecessor, or status-only evidence is not passing.
Summary by CodeRabbit
버그 수정
crit헤더를 인증 단계에서 거부합니다.401오류로 거부합니다.crit헤더가 없는 기존 JWT 검증 동작은 유지됩니다.문서