🛡️ Sentinel: [MEDIUM] Fix exception string leakage in HTTP responses#104
🛡️ Sentinel: [MEDIUM] Fix exception string leakage in HTTP responses#104socialawy-dev wants to merge 1 commit into
Conversation
Modified the /projects/{project_id}/ingest route to catch exceptions, log the raw exception internally using logger.error, and return a generic 'Upload failed' message in the HTTPException instead of exposing the raw exception string. This prevents the leakage of sensitive internal context (such as stack details, paths, or database errors) to the client.
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. |
There was a problem hiding this comment.
Code Review
This pull request addresses a security vulnerability by preventing raw exception strings from leaking to clients in API responses. In src/audioformation/server/routes.py, the exception is now logged internally, and a generic error message is returned. Additionally, the .jules/sentinel.md documentation is updated, and dependency constraints for mypy and ruff are bumped in uv.lock. The reviewer suggested using logger.exception instead of logger.error to ensure the full traceback is preserved in the logs for easier debugging.
| 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 logging caught exceptions, using logger.error with only the exception string representation ({e}) discards the traceback, which makes debugging production issues difficult. Using logger.exception instead will automatically include the traceback in the log output at the ERROR level.
| logger.error(f"Upload failed for project {project_id}: {e}") | |
| logger.exception(f"Upload failed for project {project_id}") |
🚨 Severity: MEDIUM
💡 Vulnerability: The raw string representation of caught internal exceptions (
Exception as e) was being exposed directly to clients inside HTTP 500 responses (specificallyraise HTTPException(status_code=500, detail=f"Upload failed: {e}")).🎯 Impact: Exposing internal exception strings can leak sensitive application state, stack details, or potentially infrastructure specifics (like file paths or database queries) to external users, enabling reconnaissance for further attacks.
🔧 Fix: Modified the catch block in the
ingestroute to log the exception internally usinglogger.errorand return a generic, static message ("Upload failed") to the client, following the "fail securely" principle.✅ Verification:
uv run ruff checkto ensure formatting.uv run pytest tests/to ensure no functionality is broken.src/audioformation/server/routes.pyaround line 188 to ensurelogger.erroris used andHTTPExceptionreturns a generic string.PR created automatically by Jules for task 15755530580267129863 started by @socialawy