🛡️ Sentinel: [MEDIUM] Fix error message leaking exception details#75
🛡️ Sentinel: [MEDIUM] Fix error message leaking exception details#75socialawy-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 enhances API security by preventing the leakage of internal state and unvalidated user input in error responses. Dynamic details in HTTPException responses have been replaced with static messages, while the specific error details are now captured via internal logging. The review feedback suggests improving these logs by using logger.exception() to capture full stack traces for server errors and utilizing the !r conversion flag to sanitize user-provided strings against log injection vulnerabilities.
| 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: {e}") |
There was a problem hiding this comment.
When logging an exception that results in a 500 Internal Server Error, it is better to use logger.exception() instead of logger.error() with a stringified exception. logger.exception() automatically captures the full stack trace, which is essential for debugging the root cause of the failure while ensuring that internal details remain hidden from the client response.
| logger.error(f"Upload failed: {e}") | |
| logger.exception("Upload failed") |
| raise HTTPException( | ||
| status_code=400, detail=f"Engine '{request.engine}' not found" | ||
| ) | ||
| logger.warning(f"Engine '{request.engine}' not found") |
There was a problem hiding this comment.
Logging raw user input (request.engine) can lead to log injection vulnerabilities if the input contains newline characters. Since this input is not validated before this point, it should be escaped or sanitized before logging. Using the !r conversion flag in the f-string is a simple and effective way to escape potentially dangerous characters.
| logger.warning(f"Engine '{request.engine}' not found") | |
| logger.warning(f"Engine {request.engine!r} not found") |
| return voices | ||
| except KeyError: | ||
| raise HTTPException(status_code=404, detail=f"Engine '{name}' not found") | ||
| logger.warning(f"Engine '{name}' not found") |
There was a problem hiding this comment.
Similar to the preview endpoint, logging the raw name parameter here poses a log injection risk. It should be escaped to prevent malicious input from corrupting log files or misleading administrators.
| logger.warning(f"Engine '{name}' not found") | |
| logger.warning(f"Engine {name!r} not found") |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
2 participants
🚨 Severity: MEDIUM
💡 Vulnerability: API endpoints returning raw user input and internal exception details in HTTP responses.
🎯 Impact: Could leak internal stack/state or echo unescaped user input.
🔧 Fix: Replaced dynamic details in HTTPException with static messages and added internal logging.
✅ Verification: Ensure tests pass and the f-strings in HTTPException detail have been removed.
PR created automatically by Jules for task 7791079199552038347 started by @socialawy