๐ก๏ธ Sentinel: [MEDIUM] API ํค ๊ฒ์ฆ ์ ์ฒ๋ฆฌ๋์ง ์์ ์์ธ ์์ - #490
๐ก๏ธ Sentinel: [MEDIUM] API ํค ๊ฒ์ฆ ์ ์ฒ๋ฆฌ๋์ง ์์ ์์ธ ์์ #490seonghobae wants to merge 2 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 reachedNext included review available in 12 minutes. View limit detailsLimit 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. Review configuration: โ๏ธ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ๐ Files selected for processing (5)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
๐ Info: API keys still sourced from environment
get_configured_api_keys reads CODEC_CARVER_API_KEYS from os.environ, the anti-pattern AGENTS.md marks for migration to the credential registry. Unchanged by this PR and outside the diff, but noted since adjacent auth code is being edited.
(Refers to this code)
Was this helpful? React with ๐ or ๐ to provide feedback.
| from fastapi import FastAPI, Request | ||
| from fastapi.testclient import TestClient | ||
| from fastapi.responses import JSONResponse | ||
| import hmac | ||
| import traceback | ||
| import uvicorn | ||
| import httpx | ||
| import asyncio | ||
|
|
||
| app = FastAPI() | ||
|
|
||
| configured_keys = ["validkey"] | ||
|
|
||
| @app.middleware("http") | ||
| async def require_api_key(request: Request, call_next): | ||
| 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 or missing API key"}) | ||
| except Exception as e: | ||
| print("Exception:", e) | ||
| traceback.print_exc() | ||
| return JSONResponse(status_code=500, content={"error": "Server error"}) | ||
| return await call_next(request) | ||
|
|
||
| @app.get("/test") | ||
| def test(): | ||
| return {"status": "ok"} | ||
|
|
||
| async def run_test(): | ||
| config = uvicorn.Config(app, port=8888, log_level="info") | ||
| server = uvicorn.Server(config) | ||
| task = asyncio.create_task(server.serve()) | ||
| await asyncio.sleep(1) # wait for server to start | ||
|
|
||
| # Use raw socket to bypass httpx ascii check | ||
| import socket | ||
| s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| s.connect(("127.0.0.1", 8888)) | ||
|
|
||
| # send raw bytes | ||
| req = b"GET /test HTTP/1.1\r\nHost: 127.0.0.1:8888\r\nx-api-key: invalid\xc3\xb1\r\n\r\n" | ||
| s.sendall(req) | ||
|
|
||
| resp = s.recv(4096) | ||
| print("Response:\n", resp.decode('latin1')) | ||
| s.close() | ||
|
|
||
| server.should_exit = True | ||
| await task | ||
|
|
||
| if __name__ == "__main__": | ||
| asyncio.run(run_test()) |
There was a problem hiding this comment.
๐ก Debug scratch files break the docstring-coverage gate
Three new root-level scripts (run_test even boots a live uvicorn server) carry no module or function docstrings. The repo mandates 100% interrogate coverage and excludes only scripts/tests/fuzz, so these root files fail the gate.
Prompt for agents
test_hmac.py, test_hmac_2.py, and test_hmac_3.py are manual debugging scripts left over from developing the hmac fix (test_hmac.py starts a real uvicorn server on port 8888 and uses raw sockets). They should not be committed: they add no automated test value (CI only runs unittest discover under tests/), and they violate the repo's 100% docstring-coverage rule since interrogate scans root-level files and only excludes scripts/tests/fuzz. Remove all three files. If a regression test for the non-ASCII API-key case is desired, add it as a proper unittest under tests/ with docstrings, using FastAPI TestClient rather than a live server.
Was this helpful? React with ๐ or ๐ to provide feedback.
| 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.
๐ Info: UTF-8 encoding fix resolves the TypeError
require_api_key encodes both operands to bytes before comparison. Starlette decodes headers as latin-1, so the provided key can hold non-ASCII code points; str.encode('utf-8') never raises, removing the prior TypeError. ASCII comparison semantics are unchanged.
Was this helpful? React with ๐ or ๐ to provide feedback.
1 participant
Verified succession
Exact predecessor
50e35b52f9e669fa163f325e77413b66e3fb10ac์ ์ ํจ ์๊ตฌ์ฌํญ์ non-ASCIIX-API-Key๊ฐhmac.compare_digest(str, str)์์ธ๋ฅผ ์ผ์ผํค์ง ์๊ณ ์ธ์ฆ ์คํจ๋ก ์ฒ๋ฆฌ๋์ด์ผ ํ๋ค๋ ๊ฒ์ ๋๋ค. Canonical #520 exactcf730d007543ee828b7b8e77c9473288924047d4๊ฐ raw ASGI header bytes๋ฅผ ์ง์ ์ฝ์ด ์ด ๊ณ์ฝ์ ๋ณด์กดํ๋ฉด์ configured Unicode credential success์ duplicate-header fail-closed๊น์ง ๋ ๊ฐํ๊ฒ ๊ฒ์ฆํฉ๋๋ค.#490์ decoded framework string UTF-8 ์ฌ์ธ์ฝ๋ฉ์ Unicode credential์ raw byte identity๋ฅผ ํผ์ํ ์ ์์ผ๋ฏ๋ก ๋ณ๋ ์ ํจ source delta๊ฐ ์๋๋๋ค. ์
test_hmac.py,test_hmac_2.py,test_hmac_3.py๋ repository test harness์ ํตํฉ๋์ง ์์ ad-hoc server/socket/print repro scripts์ด๋ฉฐ, canonical #520์ executable unit/integration contracts๋ณด๋ค ์ฝํ๋ฏ๋ก ์น๊ณํ์ง ์์ต๋๋ค. Blanket.julesencoding ์ง์นจ๋ raw-ASGI boundary์ exactly-one header ์ ์ฑ ์ผ๋ก ๊ต์ ๋ canonical guidance๊ฐ ๋์ฒดํฉ๋๋ค.๋ฐ๋ผ์ ๋ชจ๋ ์ ํจ semantic/test intent๋ stronger successor #520์ ์์ ์น๊ณ๋๊ณ , #520์ protected
main@90717c6e9954bf3b7a351137995ebe89975e46c2๋๋นbehind_by=0์ธ non-force descendant์ ๋๋ค.