Feat: Implement API Key Authentication and Standardize Logging#68
Feat: Implement API Key Authentication and Standardize Logging#68alebmorais merged 1 commit intomainfrom
Conversation
This commit introduces two major improvements to the application: 1. **API Key Authentication:** All API endpoints are now protected by an API key. This is a critical security enhancement to prevent unauthorized access. The API key is configured via the `API_KEY` environment variable and must be sent in the `X-API-Key` header. 2. **Standardized Logging:** All `print` statements have been replaced with the `logging` module. A centralized logger is configured to write to `logs/app.log`, providing a structured and persistent record of application events and errors. This will significantly improve debugging and observability. In addition to these main changes, a duplicate `set_sense_mode` method was removed to improve code clarity.
WalkthroughAdded API key authentication to FastAPI endpoints via a dependency injection pattern, replacing print statements with centralized structured logging to file and stdout, and removed the internal mode-switching logic from the PiProductivity class. Updated endpoint signatures across Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI as FastAPI Endpoint
participant get_api_key as get_api_key Dependency
participant Endpoint Logic as Endpoint Logic
Client->>FastAPI: Request + X-API-Key Header
FastAPI->>get_api_key: Extract & Validate API Key
alt Valid API Key
get_api_key-->>FastAPI: Return api_key
FastAPI->>Endpoint Logic: Proceed with Request
Endpoint Logic-->>FastAPI: Response
FastAPI-->>Client: 200 OK + Data
else Invalid/Missing API Key
get_api_key-->>FastAPI: Raise HTTPException(401)
FastAPI-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @alebmorais, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces crucial security and maintainability enhancements to the application. It secures all API endpoints with a new API key authentication mechanism and centralizes application logging using Python's standard Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces important enhancements by implementing API key authentication for all endpoints and standardizing logging across the application. The changes are well-executed, replacing print statements with the logging module and securing endpoints as described. I have two main suggestions: one to improve the security of the API key comparison to prevent timing attacks, and another to refine the logging configuration to avoid duplicate output when running with Uvicorn. Overall, this is a great step forward for the application's security and maintainability.
| async def get_api_key(api_key: str = Depends(api_key_header)): | ||
| if not API_KEY or api_key != API_KEY: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="Invalid or missing API Key", | ||
| ) | ||
| return api_key |
There was a problem hiding this comment.
For security, it's crucial to use a constant-time comparison for secrets like API keys to prevent timing attacks. A simple string comparison with != can leak information through response time differences. Python's secrets.compare_digest is designed for this purpose and should be used here.
Additionally, since this function doesn't perform any await operations, it can be defined as a synchronous function (def) instead of async def. FastAPI handles synchronous dependencies correctly.
You will need to add import secrets at the top of the file to use compare_digest.
def get_api_key(api_key: str = Depends(api_key_header)):
if not API_KEY or not secrets.compare_digest(api_key, API_KEY):
raise HTTPException(
status_code=status.HTTP_401_UNAUTHORIZED,
detail="Invalid or missing API Key",
)
return api_key| logging.basicConfig( | ||
| level=logging.INFO, | ||
| format="%(asctime)s - %(levelname)s - %(message)s", | ||
| handlers=[ | ||
| logging.FileHandler(LOG_DIR / "app.log"), | ||
| logging.StreamHandler(), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
The use of logging.basicConfig with a StreamHandler is likely to cause duplicate log messages in the console. Uvicorn, which is used to run the application, configures its own stream handler for console output. Log messages will be processed by Uvicorn's handler and then propagate to the root logger, which will process them again with this StreamHandler, leading to repeated entries. To fix this, you can remove the StreamHandler from the handlers list.
logging.basicConfig(
level=logging.INFO,
format="%(asctime)s - %(levelname)s - %(message)s",
handlers=[
logging.FileHandler(LOG_DIR / "app.log"),
],
)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Lines 484 to 489 in 1e263ec
WebSocket stream left unauthenticated
The commit protects the HTTP routes with get_api_key, but the /ws WebSocket endpoint still accepts connections without any credential check. Because the broadcast loop pushes live Sense HAT readings and task data through this socket, anyone who can reach the server can subscribe and view that information even when the API key is configured, defeating the “all endpoints are protected” goal. Consider validating the API key during the WebSocket handshake (e.g., header or query parameter) and rejecting unauthorized clients.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @app.post("/sense/mode", response_class=JSONResponse) | ||
| async def set_sense_mode_endpoint(mode_name: str = Form(...)): | ||
| async def set_sense_mode_endpoint( | ||
| mode_name: str = Form(...), api_key: str = Depends(get_api_key) | ||
| ): | ||
| new_mode = await run_in_threadpool(sense.set_sense_mode, mode_name) | ||
| return JSONResponse({"status": "success", "mode": new_mode}) | ||
|
|
||
| @app.post("/ocr", response_class=JSONResponse) | ||
| async def run_ocr_endpoint(): | ||
| async def run_ocr_endpoint(api_key: str = Depends(get_api_key)): | ||
| try: | ||
| img_path, txt_path, text = await run_in_threadpool(sense.run_ocr_once) | ||
| return JSONResponse({"status": "success", "image_path": img_path, "text_path": txt_path, "text": text}) | ||
| except Exception as e: | ||
| # 1. (SEGURO) Loga o erro completo no seu terminal/console | ||
| # Assim VOCÊ pode ver o que deu errado. | ||
| print("--- ERRO NO ENDPOINT DE OCR ---") | ||
| traceback.print_exc() | ||
| print("-------------------------------") | ||
| logging.error("Error in OCR endpoint", exc_info=True) | ||
|
|
||
| # 2. (SEGURO) Envia uma mensagem genérica para o usuário | ||
| # O usuário/invasor não vê nenhuma informação sensível. | ||
| return JSONResponse( | ||
| {"status": "error", "message": "Ocorreu um erro interno ao processar a imagem."}, | ||
| status_code=500 | ||
| {"status": "error", "message": "Ocorreu um erro interno ao processar a imagem."}, | ||
| status_code=500, | ||
| ) | ||
| @app.get("/camera.jpg") | ||
| async def camera_jpeg(): | ||
| async def camera_jpeg(api_key: str = Depends(get_api_key)): | ||
| frame = await run_in_threadpool(sense.read_jpeg) | ||
| data = frame or b'' # Return empty bytes if no frame | ||
| data = frame or b'' # Return empty bytes if no frame | ||
| return StreamingResponse(io.BytesIO(data), media_type="image/jpeg") | ||
|
|
||
| @app.get("/api/week-calendar", response_class=JSONResponse) | ||
| async def get_week_calendar(): | ||
| async def get_week_calendar(api_key: str = Depends(get_api_key)): |
There was a problem hiding this comment.
Adding Depends(get_api_key) to /sense/mode, /ocr, /camera.jpg, and /api/week-calendar means every call to those routes now checks the X-API-Key header. The shipped front-end (static/app.js) still submits the form and fetch requests without that header, so each interaction with the UI will immediately return 401/403 and core features (setting modes, OCR, camera refresh, weekly calendar) stop working. Either the client must be updated to send the API key or the server needs another way to authenticate browser users.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.env.example (1)
7-8: Consider removing quotes from the API_KEY value.In
.envfiles, quotes around simple string values (without spaces or special characters) are unnecessary and will be included as part of the value itself by some parsers.Apply this diff:
# A secret key to protect your API endpoints -API_KEY="your_secret_api_key" +API_KEY=your_secret_api_keyBased on static analysis hints.
main.py (1)
533-533: Store the task reference to prevent potential issues.Not storing the reference returned by
asyncio.create_task()can lead to the task being garbage collected and makes lifecycle management and debugging more difficult.Apply this diff:
- asyncio.create_task(broadcast_loop()) + broadcast_task = asyncio.create_task(broadcast_loop())Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.env.example(1 hunks)main.py(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
main.py (4)
sense_mode.py (4)
is_running(133-135)stop(128-131)clear(36-38)start(120-126)motion_client.py (2)
list_all_tasks_simple(106-142)get(32-43)task_database.py (2)
upsert_motion_tasks(71-98)fetch_items_for_display(189-248)epaper.py (1)
render_list(66-91)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 8-8: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
🪛 Ruff (0.14.1)
main.py
134-134: Consider moving this statement to an else block
(TRY300)
451-451: Unused function argument: api_key
(ARG001)
457-457: Unused function argument: api_key
(ARG001)
461-461: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
471-471: Unused function argument: api_key
(ARG001)
477-477: Unused function argument: api_key
(ARG001)
533-533: Store a reference to the return value of asyncio.create_task
(RUF006)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (11)
main.py (11)
47-54: LGTM! Well-structured logging configuration.The centralized logging setup with both file and console handlers follows best practices for production applications.
67-78: Verify API key configuration at startup.The authentication logic correctly rejects requests when
API_KEYis not set or doesn't match. However, consider adding a startup check to warn developers ifAPI_KEYis not configured, as all protected endpoints will return 401 until it's set.Optional enhancement:
# In the on_startup function, add: if not API_KEY: logging.warning("API_KEY environment variable is not set. All protected endpoints will require authentication but no valid key exists.")
124-137: LGTM! Appropriate logging levels throughout camera initialization.The replacement of print statements with structured logging at appropriate levels (warning, info, error) improves observability.
347-355: LGTM! Enhanced visual feedback for joystick actions.Adding yellow flash feedback when triggering posture checks or OCR captures improves user experience by providing immediate visual confirmation.
359-361: LGTM! Defensive check prevents errors with MockSenseHat.The
hasattrcheck appropriately guards againstAttributeErrorwhen running without physical Sense HAT hardware.
456-469: LGTM! Secure error handling with generic client response.The endpoint correctly:
- Enforces API key authentication
- Logs detailed errors server-side with
exc_info=True- Returns a generic error message to prevent information leakage
Note: The unused exception variable
e(flagged by static analysis) is acceptable here sinceexc_info=Truecaptures the full exception details.
470-474: LGTM! API key authentication properly enforced.The endpoint correctly requires authentication via the dependency injection pattern. The static analysis warning about the unused
api_keyparameter is a false positive—FastAPI'sDepends()mechanism validates the key before the endpoint executes.
476-482: LGTM! Consistent authentication pattern.API key authentication is properly enforced. The static analysis warning about the unused parameter is a false positive (see previous comment on line 471).
449-454: LGTM! Endpoint properly secured with API key.Authentication is consistently applied. The static analysis warnings about unused
api_keyparameters throughout these endpoints are false positives—the parameters are used by FastAPI's dependency injection system to validate requests before reaching the endpoint handler.
500-537: LGTM! Comprehensive startup logging improves operational visibility.The startup sequence now provides clear logging for:
- Initialization failures with full tracebacks
- Graceful degradation when hardware is unavailable
- File system locations for logs and database
- Server startup confirmation
This significantly improves operational and debugging capabilities.
303-321: AI summary inconsistency flagged.The AI-generated summary claims the
set_sense_modemethod was removed from thePiProductivityclass, but the method is still present in the code. This is likely an error in the summary rather than an issue with the code itself.
1 participant
This commit introduces two major improvements to the application:
API Key Authentication: All API endpoints are now protected by an API key. This is a critical security enhancement to prevent unauthorized access. The API key is configured via the
API_KEYenvironment variable and must be sent in theX-API-Keyheader.Standardized Logging: All
printstatements have been replaced with theloggingmodule. A centralized logger is configured to write tologs/app.log, providing a structured and persistent record of application events and errors. This will significantly improve debugging and observability.In addition to these main changes, a duplicate
set_sense_modemethod was removed to improve code clarity.Summary by CodeRabbit
New Features
Chores