🛡️ Sentinel: [MEDIUM] Fix Exception Details Leakage in API response#70
🛡️ Sentinel: [MEDIUM] Fix Exception Details Leakage in API response#70socialawy-dev wants to merge 1 commit into
Conversation
Co-authored-by: socialawy <24765060+socialawy@users.noreply.github.com>
|
👋 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. |
![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 addresses a security vulnerability in the ingest endpoint where internal exception details were exposed to clients. The changes implement server-side logging and return a generic error message, along with updating the sentinel documentation. Review feedback recommends using logger.exception() instead of logger.error() in both the implementation and the documentation to ensure full stack traces are captured for debugging purposes.
| except Exception as e: | ||
| shutil.rmtree(tmp_dir, ignore_errors=True) | ||
| raise HTTPException(status_code=500, detail=f"Upload failed: {e}") | ||
| logger.error(f"Upload failed for project {project_id}: {e}") |
There was a problem hiding this comment.
When catching an exception to log it, it is best practice to use logger.exception(). This method automatically captures the full stack trace, which is much more helpful for debugging than just the exception string. Using logger.error() with an f-string only logs the message and the exception's string representation, losing the traceback context.
| logger.error(f"Upload failed for project {project_id}: {e}") | |
| logger.exception(f"Upload failed for project {project_id}") |
| ## 2025-02-21 - Exception Details Leakage in Ingest Endpoint | ||
| **Vulnerability:** The `/projects/{project_id}/ingest` API endpoint in `src/audioformation/server/routes.py` returned the raw exception string in the `detail` field of the HTTP 500 response (`Upload failed: {e}`). | ||
| **Learning:** Exposing internal exception details (like file system errors, internal state, or stack traces) to the client can give attackers insight into the internal workings of the application. Error handling should log the details server-side and return generic error messages to the client. | ||
| **Prevention:** Always log the exception object using a logger (e.g., `logger.error()`) internally, and return a sanitized, generic error message (e.g., `detail="Upload failed"`) to the client in the `HTTPException`. |
There was a problem hiding this comment.
The prevention advice should recommend using logger.exception() instead of logger.error() when handling exceptions. logger.exception() is specifically designed for use inside except blocks to capture the full traceback, which is essential for internal troubleshooting while still allowing the API to return a generic error message to the client.
Understood. Acknowledging that this work is superseded and stopping work on this task. |
2 participants
🚨 Severity: MEDIUM
💡 Vulnerability: The
/projects/{project_id}/ingestAPI endpoint returned raw internal exceptions in the HTTP 500 response directly to the client, exposing details about the file system or internal logic.🎯 Impact: Attackers can potentially gain insight into internal paths, system state, or server logic by analyzing exposed error strings, which aids in crafting further attacks.
🔧 Fix: Updated exception handling to log the raw exception securely on the server using
logger.errorand return a generic "Upload failed" message in the HTTP response.✅ Verification: Ran the test suite successfully and verified that HTTP 500 exceptions now only return generic text.
PR created automatically by Jules for task 3853378492521698343 started by @socialawy