π‘οΈ Sentinel: [HIGH] JWT crit ν€λ κ²μ¦ κΈ°λ₯ μΆκ° - #851
π‘οΈ Sentinel: [HIGH] JWT crit ν€λ κ²μ¦ κΈ°λ₯ μΆκ°#851seonghobae wants to merge 1 commit into
Conversation
JWTμ `crit` (critical) ν€λκ° κ²μ¦λμ§ μμ RFC 7515λ₯Ό μλ°νλ 보μ μ·¨μ½μ μ μμ νμ΅λλ€. μ ν리μΌμ΄μ μ΄ μΈμν μ μλ critical νμ₯ κΈ°λ₯μ ν¬ν¨ν ν ν°μ΄ μ‘°μ©ν ν΅κ³Όλλ λ¬Έμ λ₯Ό λ°©μ§νκΈ° μν΄ `_validate_jwt_header`μ κ²μ¦ λ‘μ§μ μΆκ°νμ΅λλ€. νμ¬ μ ν리μΌμ΄μ μ νμ₯ κΈ°λ₯μ μ§μνμ§ μμΌλ―λ‘ `crit` ν€λκ° μ‘΄μ¬νλ©΄ 401 μ€λ₯λ₯Ό λ°νν©λλ€.
|
π 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. |
π WalkthroughWalkthroughJWT μΈμ¦ κ²½λ‘μ ChangesJWT
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
π₯ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
π§Ή Nitpick comments (1)
backend/tests/test_auth_security.py (1)
570-571: π Maintainability & Code Quality | π΅ Trivial | β‘ Quick winμ μ€μ²© mock ν¨μμ λͺ μμ νμ μ μΆκ°νμΈμ.
backend/**/*.pyλ μ격ν νμ μ μꡬν©λλ€. μΈ mock ν¨μμ_token맀κ°λ³μμ λ°ν νμ μ΄ μμμ μ λλ€. μ μ₯μμ header νμ λλdict[str, Any]λ₯Ό μ¬μ©νμ¬ κ° μκ·Έλμ²λ₯Ό λͺ μνμΈμ.Also applies to: 579-580, 588-589
π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_auth_security.py` around lines 570 - 571, Update all three nested mock functions around mock_get_unverified_header_bad_crit to add explicit types for the _token parameter and return value, using the repositoryβs header type or dict[str, Any].Source: Coding guidelines
π€ Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/auth.py`:
- Around line 188-196: Update _validate_jwt_headerβs crit validation to
distinguish an absent key from crit: null, require crit to be a non-empty list
of at most 10 items, and reject invalid values with HTTP 401 before iterating.
Add regression coverage for null, an empty array, and 11-item crit arrays.
In `@backend/tests/test_auth_security.py`:
- Around line 566-595: Extend test_oidc_decode_rejects_jwt_crit_header to cover
crit=[] , crit=None, and a list containing 11 entries. For each mocked header,
call auth._decode_verified_oidc_token and assert HTTPException status_code is
401 with detail "invalid crit header", preserving the existing
unsupported-extension assertion.
---
Nitpick comments:
In `@backend/tests/test_auth_security.py`:
- Around line 570-571: Update all three nested mock functions around
mock_get_unverified_header_bad_crit to add explicit types for the _token
parameter and return value, using the repositoryβs header type or dict[str,
Any].
πͺ Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
βΉοΈ Review info
βοΈ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6837b815-06af-42fe-b778-269c97d13207
π Files selected for processing (4)
.jules/sentinel.mdbackend/app/auth.pybackend/tests/test_auth_security.pypr_body.txt
| crit = header.get("crit") | ||
| if crit is not None: | ||
| if not isinstance(crit, list) or len(crit) > 10: | ||
| raise HTTPException(status_code=401, detail="invalid crit header") | ||
| for item in crit: | ||
| if not isinstance(item, str): | ||
| raise HTTPException(status_code=401, detail="invalid crit header") | ||
| # We do not support any critical extensions. | ||
| raise HTTPException(status_code=401, detail="unsupported critical extension") |
There was a problem hiding this comment.
π Security & Privacy | π Major | β‘ Quick win
critμ μ‘΄μ¬ μ¬λΆμ λΉμ΄ μμ§ μμ λ°°μ΄ μ‘°κ±΄μ κ²μ¦νμΈμ.
νμ¬ header.get("crit")λ ν€κ° μλ κ²½μ°μ crit: nullμ λμΌνκ² μ²λ¦¬ν©λλ€. crit: []λ κΈΈμ΄ κ²μ¬μ λ°λ³΅λ¬Έμ ν΅κ³Όν©λλ€. λ°λΌμ μλͺ»λ crit ν€λκ° _validate_jwt_headerλ₯Ό ν΅κ³Όν©λλ€. if "crit" in headerλ‘ μ‘΄μ¬ μ¬λΆλ₯Ό νμΈνκ³ , 리μ€νΈκ° μλκ±°λ λΉμ΄ μκ±°λ 10κ°λ₯Ό μ΄κ³Όνλ©΄ 401λ‘ κ±°λΆνμΈμ. null, λΉ λ°°μ΄, 11κ° νλͺ©μ λν νκ· ν
μ€νΈλ μΆκ°νμΈμ.
μμ μμ
- crit = header.get("crit")
- if crit is not None:
- if not isinstance(crit, list) or len(crit) > 10:
+ if "crit" in header:
+ crit = header["crit"]
+ if not isinstance(crit, list) or not crit or len(crit) > 10:
raise HTTPException(status_code=401, detail="invalid crit header")π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| crit = header.get("crit") | |
| if crit is not None: | |
| if not isinstance(crit, list) or len(crit) > 10: | |
| raise HTTPException(status_code=401, detail="invalid crit header") | |
| for item in crit: | |
| if not isinstance(item, str): | |
| raise HTTPException(status_code=401, detail="invalid crit header") | |
| # We do not support any critical extensions. | |
| raise HTTPException(status_code=401, detail="unsupported critical extension") | |
| if "crit" in header: | |
| crit = header["crit"] | |
| if not isinstance(crit, list) or not crit or len(crit) > 10: | |
| raise HTTPException(status_code=401, detail="invalid crit header") | |
| for item in crit: | |
| if not isinstance(item, str): | |
| raise HTTPException(status_code=401, detail="invalid crit header") | |
| # We do not support any critical extensions. | |
| raise HTTPException(status_code=401, detail="unsupported critical extension") |
π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/auth.py` around lines 188 - 196, Update _validate_jwt_headerβs
crit validation to distinguish an absent key from crit: null, require crit to be
a non-empty list of at most 10 items, and reject invalid values with HTTP 401
before iterating. Add regression coverage for null, an empty array, and 11-item
crit arrays.
Source: Coding guidelines
| @pytest.mark.asyncio | ||
| async def test_oidc_decode_rejects_jwt_crit_header( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| def mock_get_unverified_header_bad_crit(_token): | ||
| return {"kid": "key-1", "alg": "RS256", "crit": "not-a-list"} | ||
|
|
||
| monkeypatch.setattr(auth.jwt, "get_unverified_header", mock_get_unverified_header_bad_crit) | ||
| with pytest.raises(HTTPException) as excinfo: | ||
| await auth._decode_verified_oidc_token("invalid_token") | ||
| assert excinfo.value.status_code == 401 | ||
| assert excinfo.value.detail == "invalid crit header" | ||
|
|
||
| def mock_get_unverified_header_bad_crit_item(_token): | ||
| return {"kid": "key-1", "alg": "RS256", "crit": [123]} | ||
|
|
||
| monkeypatch.setattr(auth.jwt, "get_unverified_header", mock_get_unverified_header_bad_crit_item) | ||
| with pytest.raises(HTTPException) as excinfo: | ||
| await auth._decode_verified_oidc_token("invalid_token") | ||
| assert excinfo.value.status_code == 401 | ||
| assert excinfo.value.detail == "invalid crit header" | ||
|
|
||
| def mock_get_unverified_header_unsupported_crit(_token): | ||
| return {"kid": "key-1", "alg": "RS256", "crit": ["b32"]} | ||
|
|
||
| monkeypatch.setattr(auth.jwt, "get_unverified_header", mock_get_unverified_header_unsupported_crit) | ||
| with pytest.raises(HTTPException) as excinfo: | ||
| await auth._decode_verified_oidc_token("invalid_token") | ||
| assert excinfo.value.status_code == 401 | ||
| assert excinfo.value.detail == "unsupported critical extension" |
There was a problem hiding this comment.
π Security & Privacy | π‘ Minor | β‘ Quick win
crit κ²½κ³κ° νκ· ν
μ€νΈλ₯Ό μΆκ°νμΈμ.
νμ¬ ν
μ€νΈλ λΉλ¦¬μ€νΈ κ°, λΉλ¬Έμμ΄ νλͺ©, μ§μλμ§ μλ νμ₯λ§ νμΈν©λλ€. crit=[], crit=None, 11κ° νλͺ©μ νμΈνμ§ μμ΅λλ€. μ΄ λλ½μΌλ‘ Line 188μ νν 쑰건과 λͺ
μμ null μ²λ¦¬λ₯Ό 보νΈνμ§ λͺ»ν©λλ€. κ° μ
λ ₯μμ 401κ³Ό "invalid crit header"λ₯Ό νμΈνμΈμ.
π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_auth_security.py` around lines 566 - 595, Extend
test_oidc_decode_rejects_jwt_crit_header to cover crit=[] , crit=None, and a
list containing 11 entries. For each mocked header, call
auth._decode_verified_oidc_token and assert HTTPException status_code is 401
with detail "invalid crit header", preserving the existing unsupported-extension
assertion.
Source: Coding guidelines
1 participant
π¨ Severity: HIGH
π‘ Vulnerability: JWT
crit(critical) ν€λκ° κ²μ¦λμ§ μμ RFC 7515λ₯Ό μλ°νμ΅λλ€. 곡격μκ° μ ν리μΌμ΄μ μ΄ μ§μνμ§ μλ μ€μν νμ₯μ ν¬ν¨μμΌ λ³΄μ μ μ΄λ₯Ό μ°νν μ μμ΅λλ€.π― Impact: 무μλ critical νλΌλ―Έν°λ‘ μΈν λμ 보μ μ°ν μν.
π§ Fix:
backend/app/auth.pyμ_validate_jwt_headerμ λͺ μμ μΈ κ²μ¦μ μΆκ°νμ¬ μ ν리μΌμ΄μ μ΄ μ»€μ€ν νμ₯μ μ§μνμ§ μμΌλ―λ‘critν€λκ° ν¬ν¨λ λͺ¨λ JWTλ₯Ό κ±°λΆνλλ‘ λ³κ²½νμ΅λλ€.critκΈΈμ΄μ μ νμ λν λ°©μ΄ λ‘μ§λ μΆκ°νμ΅λλ€.β Verification: μ 체 pytest ν μ€νΈλ₯Ό μ€ννμ¬ λͺ¨λ ν΅κ³ΌνμΌλ©°,
backend/tests/test_auth_security.pyμ μΆκ°λ μλ‘μ΄ ν μ€νΈ μΌμ΄μ€λ₯Ό ν΅ν΄critν€λκ° ν¬ν¨λ ν ν°μ΄ 401 μλ΅κ³Ό ν¨κ» κ±°λΆλλ κ²μ νμΈνμ΅λλ€.PR created automatically by Jules for task 17978932081618502390 started by @seonghobae
Summary by CodeRabbit
λ²κ·Έ μμ
critκ° νμκ³Ό νλͺ© μλ₯Ό κ²μ¦ν©λλ€.ν μ€νΈ