π‘οΈ Sentinel: [MEDIUM] hmac.compare_digestμμμ μ²λ¦¬λμ§ μμ μμΈλ‘ μΈν DoS μ·¨μ½μ μμ - #386
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. |
π WalkthroughWalkthroughAPI ν€ κ²μ¦μμ ChangesAPI ν€ κ²μ¦
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
π₯ Pre-merge checks | β 4 | β 1β Failed checks (1 warning)
β Passed checks (4 passed)
β¨ Finishing Touches π‘ 1π 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
π€ 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 `@saas_web.py`:
- Line 117: Update saas_web.py lines 117-117 in get_configured_api_keys() to
load runtime API keys from the credential registry/KV instead of directly
reading CODEC_CARVER_API_KEYS from the environment, while preserving the
existing key comparison behavior. Update tests/test_saas_web.py lines 642-652 to
configure the test API key in the test registry/KV rather than patching the
environment variable.
In `@test_hmac_direct2.py`:
- Around line 1-44: Remove the import-time reproduction scripts from
test_hmac_direct2.py (1-44), test_encode.py (1-10), test_hmac_encode.py (1-6),
test_hmac_exception.py (1-7), test_hmac_header.py (1-27), test_hmac_latin1.py
(1-27), test_hmac_direct.py (1-33), and test_direct.py (1-11); delete each
fileβs executable comparison, FastAPI/Uvicorn server, raw socket, or localhost
request code so these files cannot run during test collection, relying on
tests/test_saas_web.py for regression coverage.
πͺ 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: 9edf0d4c-c99a-4651-bb83-d5ece31cf3d5
π Files selected for processing (12)
.jules/sentinel.mdCHANGELOG.mdsaas_web.pytest_direct.pytest_encode.pytest_hmac_direct.pytest_hmac_direct2.pytest_hmac_encode.pytest_hmac_exception.pytest_hmac_header.pytest_hmac_latin1.pytests/test_saas_web.py
| provided_key = request.headers.get("x-api-key", "") | ||
| if not any( | ||
| hmac.compare_digest(provided_key, key) for key in configured_keys | ||
| hmac.compare_digest(provided_key.encode("utf-8"), key.encode("utf-8")) for key in configured_keys |
There was a problem hiding this comment.
π Security & Privacy | π Major | ποΈ Heavy lift
λ°νμ API ν€ μμ€λ₯Ό credential registry/KVλ‘ μ΄μ νμΈμ.
saas_web.pyμ get_configured_api_keys()λ Line 97μμ CODEC_CARVER_API_KEYSλ₯Ό μ§μ μ½μ΅λλ€. μ μΈμ¦ νλ¦λ μ΄ κ°μ μ¬μ©ν©λλ€. νκ²½ λ³μμμ μ§μ μ½λ λ°©μμ ν€ νμ , μ κ·Ό μ μ΄, κ°μ¬ κ²½λ‘λ₯Ό μ°νν©λλ€.
saas_web.py#L117-L117:get_configured_api_keys()κ° credential registry/KVμμ λ°νμ ν€λ₯Ό μ½λλ‘ λ³κ²½νμΈμ.tests/test_saas_web.py#L642-L652: νκ²½ λ³μλ₯Ό ν¨μΉνμ§ λ§κ³ , ν μ€νΈμ© registry/KVμ ν€λ₯Ό μ€μ νμΈμ.
As per coding guidelines, βsaas_web.py must source runtime API keys, database credentials, endpoints, and other secrets from the credential registry/KV rather than directly from environment variables.β
π Affects 2 files
saas_web.py#L117-L117(this comment)tests/test_saas_web.py#L642-L652
π€ 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 `@saas_web.py` at line 117, Update saas_web.py lines 117-117 in
get_configured_api_keys() to load runtime API keys from the credential
registry/KV instead of directly reading CODEC_CARVER_API_KEYS from the
environment, while preserving the existing key comparison behavior. Update
tests/test_saas_web.py lines 642-652 to configure the test API key in the test
registry/KV rather than patching the environment variable.
Source: Coding guidelines
| from fastapi import FastAPI, Request | ||
| from fastapi.responses import JSONResponse | ||
| import hmac | ||
| import os | ||
| import uvicorn | ||
| import threading | ||
| import time | ||
| import socket | ||
|
|
||
| app = FastAPI() | ||
|
|
||
| @app.middleware("http") | ||
| async def require_api_key(request: Request, call_next): | ||
| configured_keys = ["secret-key"] | ||
| provided_key = request.headers.get("x-api-key", "") | ||
| try: | ||
| if not any( | ||
| hmac.compare_digest(provided_key, key) for key in configured_keys | ||
| ): | ||
| return JSONResponse(status_code=401, content={"error": "Invalid"}) | ||
| except Exception as e: | ||
| print(f"Server caught exception: {repr(e)}") | ||
| return JSONResponse(status_code=500, content={"error": repr(e)}) | ||
| return await call_next(request) | ||
|
|
||
| @app.get("/") | ||
| def read_root(): | ||
| return {"Hello": "World"} | ||
|
|
||
| def run_server(): | ||
| uvicorn.run(app, host="127.0.0.1", port=8000, log_level="error") | ||
|
|
||
| t = threading.Thread(target=run_server, daemon=True) | ||
| t.start() | ||
| time.sleep(1) | ||
|
|
||
| # Manually send a raw HTTP request with non-ASCII header | ||
| req = b"GET / HTTP/1.1\r\nHost: localhost:8000\r\nx-api-key: \xff\r\n\r\n" | ||
| s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| s.connect(("127.0.0.1", 8000)) | ||
| s.sendall(req) | ||
| resp = s.recv(4096) | ||
| print(resp.decode("latin-1")) | ||
| s.close() |
There was a problem hiding this comment.
π©Ί Stability & Availability | π Major | β‘ Quick win
μ€ν μ¬ν νμΌμ ν μ€νΈ μμ§ κ²½λ‘μμ μ κ±°νμΈμ.
μ΄ νμΌλ€μ test_*.py μ΄λ¦μ μ¬μ©νκ³ λͺ¨λ import μ μ¦μ μ€νλ©λλ€. ν
μ€νΈ μμ§ μ€μ μμΈ, λ€νΈμν¬ λκΈ°, ν¬νΈ μΆ©λ, Uvicorn λ°±κ·ΈλΌμ΄λ μλ²κ° λ°μν μ μμ΅λλ€. κ²μ¦μ μ΄λ―Έ tests/test_saas_web.pyμ νκ· ν
μ€νΈλ‘ μνν©λλ€.
test_hmac_direct2.py#L1-L44: Uvicorn μλ²μ raw socket μ¬ν μ½λλ₯Ό μμ νμΈμ.test_encode.py#L1-L10: import μ μ€νλλ λΉκ΅ μ½λλ₯Ό μμ νμΈμ.test_hmac_encode.py#L1-L6: import μ μ€νλλ λΉκ΅ μ½λλ₯Ό μμ νμΈμ.test_hmac_exception.py#L1-L7: import μ μ€νλλ μ·¨μ½ λΉκ΅ μ½λλ₯Ό μμ νμΈμ.test_hmac_header.py#L1-L27: import μ μ€νλλ FastAPI μ¬ν μ½λλ₯Ό μμ νμΈμ.test_hmac_latin1.py#L1-L27: import μ μ€νλλ FastAPI μ¬ν μ½λλ₯Ό μμ νμΈμ.test_hmac_direct.py#L1-L33: import μ μ€νλλ localhost μμ² μ½λλ₯Ό μμ νμΈμ.test_direct.py#L1-L11: import μ μ€νλλ μμ² μ½λλ₯Ό μμ νμΈμ.
π§° Tools
πͺ Ruff (0.16.1)
[warning] 21-21: Do not catch blind exception: Exception
(BLE001)
[warning] 22-22: Use explicit conversion flag
Replace with conversion flag
(RUF010)
π Affects 8 files
test_hmac_direct2.py#L1-L44(this comment)test_encode.py#L1-L10test_hmac_encode.py#L1-L6test_hmac_exception.py#L1-L7test_hmac_header.py#L1-L27test_hmac_latin1.py#L1-L27test_hmac_direct.py#L1-L33test_direct.py#L1-L11
π€ 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 `@test_hmac_direct2.py` around lines 1 - 44, Remove the import-time
reproduction scripts from test_hmac_direct2.py (1-44), test_encode.py (1-10),
test_hmac_encode.py (1-6), test_hmac_exception.py (1-7), test_hmac_header.py
(1-27), test_hmac_latin1.py (1-27), test_hmac_direct.py (1-33), and
test_direct.py (1-11); delete each fileβs executable comparison, FastAPI/Uvicorn
server, raw socket, or localhost request code so these files cannot run during
test collection, relying on tests/test_saas_web.py for regression coverage.
|
Closing as superseded by the narrower canonical fix in #390 for the same |
Understood. Acknowledging that this work is superseded by #390 and stopping work on this task. |
1 participant
π¨ Severity: MEDIUM
π‘ Vulnerability:
hmac.compare_digestλ ASCII μ μ© λ¬Έμμ΄μ΄λ λ°μ΄νΈλ§ μ²λ¦¬ν μ μμ΅λλ€. μ μμ μΈ μ¬μ©μκ°x-api-keyHTTP ν€λμ ASCIIκ° μλ λ¬Έμλ₯Ό ν¬ν¨μν€λ©΄ μ²λ¦¬λμ§ μμTypeErrorκ° λ°μνμ¬ μ΄ν리μΌμ΄μ μ΄ μΆ©λνκ³ 500 Internal Server Errorλ₯Ό λ°νν©λλ€.π― Impact: μ μ ν 401 μν μ½λ λ°νμ μ°ννμ¬ μ€ν νΈλ μ΄μ€ λ ΈμΆ λλ μ΄ν리μΌμ΄μ DoS 곡격μ μ λ°ν μ μμ΅λλ€.
π§ Fix: λΉκ΅νκΈ° μ μ λ λ¬Έμμ΄μ λͺ¨λ λ°μ΄νΈ(UTF-8)λ‘ μμ νκ² μΈμ½λ©νμ¬ λͺ¨λ λ¬Έμ μ λ ₯μ μ€λ₯ μμ΄ μ²λ¦¬νλλ‘ μμ νμ΅λλ€.
β Verification: ν μ€νΈλ₯Ό μ€ννμ¬ μ΄ν리μΌμ΄μ μ΄ ASCIIκ° μλ λ¬Έμμ λν΄ λ μ΄μ μΆ©λνμ§ μκ³ 401 μλ΅μ μ¬λ°λ₯΄κ² λ°ννλμ§ νμΈνμ΅λλ€.
PR created automatically by Jules for task 2895693522345377856 started by @seonghobae
Summary by CodeRabbit
λ²κ·Έ μμ
ν μ€νΈ
λ¬Έμ