fix: replace leaked exception details with generic error messages#287
fix: replace leaked exception details with generic error messages#287SatishKumar620 wants to merge 3 commits intoOWASP-BLT:mainfrom
Conversation
📊 Monthly LeaderboardHi @SatishKumar620! Here's how you rank for March 2026:
Scoring this month (across OWASP-BLT org): Open PRs (+1 each), Merged PRs (+10), Closed (not merged) (−2), Reviews (+5; first two per PR in-month), Comments (+2, excludes CodeRabbit). Run |
🍃 PR Readiness CheckCheck the readiness of this PR on Leaf: Leaf reviews pull requests for operational readiness, security risks, and production-impacting changes before they ship. |
|
👋 Hi @SatishKumar620! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you!
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaced detailed exception texts returned to clients with a uniform JSON error payload Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers.py (1)
897-907:⚠️ Potential issue | 🟠 MajorMissing fix:
handle_statusstill leaks exception details to clients.The PR objective mentions 13 handlers were affected, but
handle_statusstill returns raw exception details via'error': str(e)in the response. This is inconsistent with the other fixes and could expose database configuration errors, connection strings, or internal paths.🔒 Proposed fix to remove exception leakage
except Exception as e: # Database not configured + print(f"Error in handle_status: {type(e).__name__}: {str(e)}") + await notify_slack_exception(getattr(env, 'SLACK_ERROR_WEBHOOK', ''), e, context={'handler': 'handle_status'}) return Response.new(json.dumps({ 'database_configured': False, - 'error': str(e), + 'error': 'Database connection failed', 'environment': getattr(env, 'ENVIRONMENT', 'unknown'), 'row_counts': { 'prs': 0, 'timeline_cache': 0 } }), {'headers': {'Content-Type': 'application/json'}})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers.py` around lines 897 - 907, In handle_status, stop returning raw exception details to clients: replace the `'error': str(e)` entry in the Response payload with a non-sensitive generic value (e.g. `'error': 'unavailable'` or remove the key entirely) and instead log the exception server-side (e.g. logging.exception or processLogger.exception) using the caught variable `e`; keep the rest of the response structure (database_configured, environment, row_counts) unchanged and ensure Response.new still sets Content-Type application/json.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/handlers.py`:
- Around line 897-907: In handle_status, stop returning raw exception details to
clients: replace the `'error': str(e)` entry in the Response payload with a
non-sensitive generic value (e.g. `'error': 'unavailable'` or remove the key
entirely) and instead log the exception server-side (e.g. logging.exception or
processLogger.exception) using the caught variable `e`; keep the rest of the
response structure (database_configured, environment, row_counts) unchanged and
ensure Response.new still sets Content-Type application/json.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f7942043-3747-4e54-8595-c03e6e966f54
📒 Files selected for processing (1)
src/handlers.py
|
@coderabbitai Please assess the PR's completeness and merge readiness. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me examine the codebase to verify completeness of the fix. 🏁 Script executed: Length of output: 35612
✅ PR Completeness AssessmentPrimary Objective — Fully AchievedThe search confirms zero remaining instances of The 14 handlers confirmed clean in
|
| Area | Status |
|---|---|
All 13 targeted type(e).__name__: str(e) 500 patterns removed |
✅ Complete |
str(e) leaks in 400 responses (lines 102, 214, 739) |
|
str(e) leak in debug endpoint response body (line 897) |
|
str(e) leak in batch error list in 200 response (line 693) |
|
| Consistent error message wording (line 871) | 🔸 Minor inconsistency |
The PR is complete for its stated scope and safe to merge. The residual issues above are pre-existing and were not introduced by this PR. I'd recommend opening a follow-up issue to track the remaining str(e) leaks — especially the debug endpoint (lines 897–902) and batch error list (line 693). Would you like me to open a follow-up GitHub issue for those?
Multiple API error handlers were returning raw Python exception details
(type, message, and internal strings) directly to clients in HTTP
responses, exposing internal implementation details such as database
schema, column names, and logic.
Changes:
- Replaced 12 instances of f"{type(e).__name__}: {str(e)}" with
'Internal server error' in 500 responses
- Replaced str(e) with 'Database not configured' in status endpoint
- Replaced str(update_error) with 'Update failed' in batch refresh
- Replaced f'Failed to fetch organization repos: {error_msg}' with
'Failed to fetch organization repos' in 2 places
- Standardized 'Internal server error fetching rate status' to
'Internal server error' for consistency
Internal error details are still logged server-side via print() and
Slack notifications so observability is not lost.
Fixes OWASP-BLT#285
SatishKumar620
force-pushed
the
fix/internal-error-details-leaked-in-responses
branch
from
32558ba to
eb3f601
Compare
|
@coderabbitai Please assess the PR's completeness and merge readiness. |
|
Tip For best results, initiate chat on the files or code changes.
✅ PR
|
| Location | Issue | Risk |
|---|---|---|
| Lines 102 & 739 | str(e) embedded in 400 response messages |
Medium |
| Line 214 | str(e) in a 400 response |
Medium |
| Line 693 | str(e) per-PR errors in a 200 batch response |
Notable |
| Lines 897–902 | str(e) in debug endpoint response body |
Higher risk |
| Line 871 | Message reads 'Internal server error fetching rate status' instead of 'Internal server error' |
Minor inconsistency |
Verdict
✅ Safe to merge. The PR fully achieves its stated scope. The residual issues above are pre-existing and were not introduced by this PR.
Would you like me to open a follow-up GitHub issue to track the remaining str(e) leaks — especially the debug endpoint (lines 897–902) and the batch error accumulation (line 693)?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/handlers.py (1)
897-907: Status endpoint error handling improved, consider HTTP status code.The error message 'Database not configured' appropriately hides raw exception details. However, this returns HTTP 200 for what is effectively a database configuration error. For a health/status endpoint, consider returning HTTP 503 (Service Unavailable) when the database is not configured, as this more accurately represents the system state to monitoring tools.
🔧 Optional: Return 503 for misconfigured state
except Exception as e: # Database not configured return Response.new(json.dumps({ 'database_configured': False, 'error': 'Database not configured', 'environment': getattr(env, 'ENVIRONMENT', 'unknown'), 'row_counts': { 'prs': 0, 'timeline_cache': 0 } - }), {'headers': {'Content-Type': 'application/json'}}) + }), {'status': 503, 'headers': {'Content-Type': 'application/json'}})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers.py` around lines 897 - 907, In the except Exception as e block that returns Response.new with the JSON payload hiding exception details (the 'Database not configured' branch), change the HTTP status from the implicit 200 to 503 Service Unavailable so health checks/monitors see the service is degraded; update the Response.new call used there (the same call that sets headers Content-Type: application/json and the JSON body) to include a 503 status while keeping the same JSON structure and headers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/handlers.py`:
- Around line 897-907: In the except Exception as e block that returns
Response.new with the JSON payload hiding exception details (the 'Database not
configured' branch), change the HTTP status from the implicit 200 to 503 Service
Unavailable so health checks/monitors see the service is degraded; update the
Response.new call used there (the same call that sets headers Content-Type:
application/json and the JSON body) to include a 503 status while keeping the
same JSON structure and headers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 748a4fbb-07db-4fcb-a29b-8897187b0681
📒 Files selected for processing (1)
src/handlers.py
|
@coderabbitai Please assess the PR's completeness and merge readiness. |
|
Tip For best results, initiate chat on the files or code changes.
✅ PR
|
| Check | Status |
|---|---|
All 13 targeted type(e).__name__: str(e) patterns removed from 500 responses |
✅ Complete |
Pre-existing str(e) leaks in 400 responses (Lines 102, 214, 739) |
|
Pre-existing str(e) in debug endpoint (Lines 897–902) |
|
Pre-existing str(e) in batch error list in 200 response (Line 693) |
|
Minor inconsistency: 'Internal server error fetching rate status' vs 'Internal server error' (Line 871) |
🔸 Minor, pre-existing |
The residual issues listed above were not introduced by this PR. Would you like me to open a follow-up GitHub issue to track them — particularly the debug endpoint leak (Lines 897–902) and the batch error accumulation (Line 693)?
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/handlers.py (1)
210-216: Pre-existing:str(e)still exposed in 400 response (out of scope).This
ValueErrorhandler still returnsstr(e)to clients. Per the PR comments assessment, this is a pre-existing issue outside the current PR's scope (which targets 500 responses withtype(e).__name__: {str(e)}).For tracking purposes:
parse_pr_urllikely raisesValueErrorwith messages like "Invalid GitHub PR URL format", which are relatively benign, but for consistency you may want to address this in the follow-up issue mentioned in the PR discussion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers.py` around lines 210 - 216, The handler catches ValueError from parse_pr_url and returns str(e) to clients via Response.new (in the except block), which exposes internal error text; change the response body to a generic client-safe message (e.g., {'error': 'Invalid PR URL'} or {'error': 'Bad Request'}) instead of str(e), and log the original exception server-side for diagnostics (use existing logger), keeping the Response.new status 400 and Content-Type as-is; update the except ValueError block around parse_pr_url to implement this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/handlers.py`:
- Around line 210-216: The handler catches ValueError from parse_pr_url and
returns str(e) to clients via Response.new (in the except block), which exposes
internal error text; change the response body to a generic client-safe message
(e.g., {'error': 'Invalid PR URL'} or {'error': 'Bad Request'}) instead of
str(e), and log the original exception server-side for diagnostics (use existing
logger), keeping the Response.new status 400 and Content-Type as-is; update the
except ValueError block around parse_pr_url to implement this behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1a6867cb-5303-44d5-9bb0-59ebad52561e
📒 Files selected for processing (1)
src/handlers.py
SatishKumar620
force-pushed
the
fix/internal-error-details-leaked-in-responses
branch
from
85d204b to
3921b58
Compare
|
@coderabbitai Please assess the PR's completeness and merge readiness. And approve these changes |
|
✅ PR
|
| Check | Status |
|---|---|
All 13 targeted type(e).__name__: str(e) patterns removed from 500 responses |
✅ Complete |
Pre-existing str(e) leaks in 400 responses (Lines 102, 214, 739) |
|
Pre-existing str(e) in debug endpoint (Lines 897–902) |
|
Pre-existing str(e) in batch error list in 200 response (Line 693) |
|
Minor inconsistency: 'Internal server error fetching rate status' vs 'Internal server error' (Line 871) |
🔸 Minor, pre-existing |
The PR fully achieves its stated scope — all targeted patterns are addressed and no regressions were introduced. Approving now. The residual pre-existing issues should be tracked in a follow-up issue; let me know if you'd like me to open one.
✅ Actions performed
Comments resolved and changes approved.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/handlers.py (3)
213-213: Use explicit f-string conversion flag instead ofstr(e).Per the static analysis hint, prefer
{e!s}or just{e}over{str(e)}for cleaner f-string usage.✨ Minor cleanup
- print(f"Invalid PR URL from client: {str(e)}") + print(f"Invalid PR URL from client: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers.py` at line 213, Replace the use of str(e) inside the f-string in the print call that logs invalid PR URLs (the print statement containing "Invalid PR URL from client: {str(e)}") with an explicit f-string conversion like {e!s} (or simply {e}) to follow the static-analysis recommendation and cleaner f-string usage; update the print statement in src/handlers.py accordingly.
97-103: Consider logging the original error for debugging purposes.Unlike other error handlers in this file, the org fetch failure path captures
error_msgbut doesn't log it before returning the generic response. This could make debugging org fetch issues harder.💡 Suggested improvement
except Exception as e: error_msg = str(e) if 'status=403' in error_msg: return Response.new(json.dumps({'error': 'Rate Limit Exceeded'}), {'status': 403, 'headers': {'Content-Type': 'application/json'}}) + print(f"Failed to fetch org repos for {org_owner}: {error_msg}") return Response.new(json.dumps({'error': 'Failed to fetch organization repos'}), {'status': 400, 'headers': {'Content-Type': 'application/json'}})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers.py` around lines 97 - 103, The except block that handles org repo fetch failures (the "except Exception as e" branch that sets error_msg and returns via Response.new) should log the original exception before returning; update the handler to call the module logger (e.g., logger.error or logger.exception) with the error or error_msg so the full stack/trace is captured for debugging, then continue to return the same JSON Response.new payloads for the 403 and 400 cases.
735-741: Consider logging the error for debugging (same as lines 97-103).Similar to
handle_add_pr, this captureserror_msgbut doesn't log it before returning the generic response.💡 Suggested improvement
except Exception as e: error_msg = str(e) if 'status=403' in error_msg: return Response.new(json.dumps({'error': 'Rate Limit Exceeded'}), {'status': 403, 'headers': {'Content-Type': 'application/json'}}) + print(f"Failed to fetch org repos for {org}: {error_msg}") return Response.new(json.dumps({'error': 'Failed to fetch organization repos'}), {'status': 400, 'headers': {'Content-Type': 'application/json'}})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers.py` around lines 735 - 741, The except block that builds error_msg currently returns a generic Response without logging; mirror the pattern used in handle_add_pr (lines 97-103) by logging the exception before returning: call the module/logger instance used elsewhere (e.g., process_logger.error or logger.error) with the error_msg (and optionally exception info) in this except block that handles fetching organization repos so the actual error is recorded prior to returning the 403/400 Response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/handlers.py`:
- Line 213: Replace the use of str(e) inside the f-string in the print call that
logs invalid PR URLs (the print statement containing "Invalid PR URL from
client: {str(e)}") with an explicit f-string conversion like {e!s} (or simply
{e}) to follow the static-analysis recommendation and cleaner f-string usage;
update the print statement in src/handlers.py accordingly.
- Around line 97-103: The except block that handles org repo fetch failures (the
"except Exception as e" branch that sets error_msg and returns via Response.new)
should log the original exception before returning; update the handler to call
the module logger (e.g., logger.error or logger.exception) with the error or
error_msg so the full stack/trace is captured for debugging, then continue to
return the same JSON Response.new payloads for the 403 and 400 cases.
- Around line 735-741: The except block that builds error_msg currently returns
a generic Response without logging; mirror the pattern used in handle_add_pr
(lines 97-103) by logging the exception before returning: call the module/logger
instance used elsewhere (e.g., process_logger.error or logger.error) with the
error_msg (and optionally exception info) in this except block that handles
fetching organization repos so the actual error is recorded prior to returning
the 403/400 Response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5b0b4688-4f08-4ebb-be36-cf4d34c8bdfa
📒 Files selected for processing (1)
src/handlers.py
owasp-blt
bot
added
unresolved-conversations: 0
and removed
unresolved-conversations: 0
labels
…repo fetch failure responses
1 participant
13 API error handlers were returning raw Python exception details (type and message) directly to clients in HTTP 500 responses:
This exposed internal implementation details such as database schema, column names, and internal logic to any API client. Internal details are already logged server-side via print() and Slack notifications.
Fix: replace all 12 instances with the safe generic message already used by handle_add_pr:
Fixes #285
Summary by CodeRabbit