fix: stop 13 routes from leaking internal errors (OPE-79)#254
Merged
DevanshuNEU merged 2 commits intoFeb 23, 2026
Merged
Conversation
Before: 13 route handlers returned raw exception strings in HTTP responses. In production this leaked file paths, database connection details, API key fragments, and third-party service errors. After: All catch blocks return generic error messages. The actual error is logged via logger.error() + capture_exception() for debugging in Sentry/logs. Changes by file: - analysis.py: Fixed all 5 endpoints. Also added missing 'except HTTPException: raise' (was swallowing 404s as 500s) and added logger.error + capture_exception (had zero logging). - repos.py: Fixed 4 endpoints (add, delete, index, async index). - search.py: Fixed 2 endpoints (search, explain). - search_v2.py: Fixed 1 endpoint. - playground.py: Fixed 1 endpoint. Secondary fix in analysis.py: - get_repo_or_404 raises HTTPException(404) but the bare 'except Exception' was catching it and re-raising as 500. Added 'except HTTPException: raise' before 'except Exception' on all 5 endpoints. Tests: 3 new tests verify that Pinecone connection strings, file paths, and git credentials are NOT exposed in error responses. 284 tests pass (281 existing + 3 new). Closes OPE-79
|
@DevanshuNEU is attempting to deploy a commit to the Dev's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughMultiple backend API routes have been updated to standardize HTTP error responses and add observability instrumentation. Instead of exposing raw exception strings in 500 responses, endpoints now return generic error messages. New observability calls capture exceptions with contextual metadata. A test module validates that sensitive details aren't leaked in error responses. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Collaborator
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/routes/repos.py (2)
143-149:⚠️ Potential issue | 🟠 MajorHTTP 400 is semantically wrong for a server-side failure — should be 500.
The exceptions reaching this catch block are git clone failures, disk errors, DB errors, etc. — all server faults. HTTP 400 signals a client fault ("Bad Request"), which will mislead callers, dashboards, and alerting rules that rely on 4xx vs 5xx classification. The companion test at
test_error_leaking.pyline 63 should be updated as well.🔧 Proposed fix
- raise HTTPException(status_code=400, detail="Failed to add repository") + raise HTTPException(status_code=500, detail="Failed to add repository")In
test_error_leaking.py:- assert resp.status_code == 400 + assert resp.status_code == 500🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/repos.py` around lines 143 - 149, The catch-all exception handler in backend/routes/repos.py currently converts server-side failures into HTTPException(status_code=400) which is incorrect; update the handler in the add-repository flow (the except Exception as e block that logs with logger.error and calls capture_exception) to raise HTTPException(status_code=500, detail="Failed to add repository") instead, keeping the existing logger.error and capture_exception calls and leaving the earlier except HTTPException re-raise intact; also update the corresponding expectation in test_error_leaking.py (the assertion at the test around line 63) to expect a 500 response rather than 400.
388-400:⚠️ Potential issue | 🟠 MajorResidual information-disclosure paths missed by this PR: WebSocket and Redis pub/sub both still send
str(e)to clients.Two pre-existing leaks remain unaddressed:
websocket_index(line 576) sends the raw exception string directly to the WebSocket client:await websocket.send_json({"type": "error", "message": str(e)})
_run_async_indexing(lines 393-400) publishes the raw exception string to the Redis pub/sub channel:publisher.publish_error(repo_id, error="indexing_failed", message=str(e), ...)Any WebSocket subscriber listening on that channel will receive the verbatim exception message.
Both are the same class of leak that OPE-79 targets (Pinecone endpoints, internal paths, git credential fragments can appear in exception strings).
🔧 Proposed fix sketch
# websocket_index (~line 576) - await websocket.send_json({"type": "error", "message": str(e)}) + await websocket.send_json({"type": "error", "message": "Indexing failed"}) # _run_async_indexing (~line 397) - publisher.publish_error(repo_id, error="indexing_failed", message=str(e), recoverable=True) + publisher.publish_error(repo_id, error="indexing_failed", message="Indexing failed", recoverable=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/repos.py` around lines 388 - 400, The websocket and Redis pub/sub still send raw exception strings to clients: in websocket_index (websocket.send_json) and in _run_async_indexing (publisher.publish_error). Replace str(e) usage with a non-sensitive, generic client-facing message (e.g. "indexing_failed" or "An internal error occurred while indexing") or send an error_id/token tied to the server-side log, while keeping the full exception only in logger.error/capture_exception for internal diagnostics; update websocket.send_json({... "message": ...}) and publisher.publish_error(... message=...) accordingly and ensure logger.error/capture_exception continue to record the original exception.backend/routes/playground.py (2)
578-583:⚠️ Potential issue | 🟠 Major
_fetch_repo_metadatastill leaksstr(e)to the HTTP response — missed by this PR.At line 582 the raw exception is stored in the returned dict:
return {"error": "request_failed", "message": str(e)}Both callers then put
metadata.get("message", …)directly into an HTTP 502 response body:
validate_github_repolines 700-703start_anonymous_indexinglines 900-906A Pinecone / DB / internal-network exception string becomes visible to the client through the
502.detail.messagefield, which is exactly the class of leak OPE-79 targets.🔧 Proposed fix
- except Exception as e: - logger.error("GitHub API request failed", error=str(e)) - return {"error": "request_failed", "message": str(e)} + except Exception as e: + logger.error("GitHub API request failed", error=str(e)) + return {"error": "request_failed", "message": "Failed to contact GitHub API"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/playground.py` around lines 578 - 583, The _fetch_repo_metadata function currently returns the raw exception string in {"error": "request_failed", "message": str(e)}, which leaks internal errors to clients; change it to return a non-sensitive, generic message (e.g. {"error": "request_failed", "message": "failed to fetch repository metadata"}) and keep the full exception in the server logs via logger.exception or logger.error with the exception context. Update _fetch_repo_metadata (and related exception handling) to avoid exposing str(e); callers validate_github_repo and start_anonymous_indexing will then use metadata.get("message", ...) safely without leaking internal details. Ensure httpx.TimeoutException still returns the explicit timeout message while all other exceptions return the generic message and the error is logged with traceback.
486-490:⚠️ Potential issue | 🟡 MinorMissing
capture_exceptioncall — inconsistent with the rest of the PR.Every other updated handler (
search.py,search_v2.py,repos.py,analysis.py) callscapture_exceptionbefore raising the generic 500.playground.pyonly logs to stdout, so failures here are invisible to Sentry.🔧 Proposed fix
-from services.observability import logger +from services.observability import logger, capture_exceptionexcept Exception as e: logger.error("Playground search failed", error=str(e)) + capture_exception(e, operation="playground_search") raise HTTPException(status_code=500, detail="Search failed")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/playground.py` around lines 486 - 490, The except block in playground.py currently only logs the error and raises HTTPException; add a call to capture_exception(e) (as done in search.py/search_v2.py/repos.py/analysis.py) before raising to ensure Sentry receives the failure; update the except Exception as e handler in the Playground search handler to call capture_exception(e) and then logger.error(...) and raise HTTPException(status_code=500, detail="Search failed"); if capture_exception is not already imported in this module, import it from the same location used by the other handlers.
🧹 Nitpick comments (1)
backend/routes/analysis.py (1)
79-82:capture_exceptioninanalyze_impactis missing thefile_pathcontext present in thelogger.errorcall on the line above.This means Sentry reports for impact-analysis failures won't include the file path, making them harder to reproduce. The
explain_codehandler insearch.py(lines 173-179) consistently passesfile_pathtocapture_exception.♻️ Proposed fix
- capture_exception(e, operation="impact_analysis", repo_id=repo_id) + capture_exception(e, operation="impact_analysis", repo_id=repo_id, file_path=request.file_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/analysis.py` around lines 79 - 82, The catch block in analyze_impact is logging the error with file_path but calling capture_exception without file_path, so Sentry events lack that context; update the capture_exception call in the except block of analyze_impact (the same scope where logger.error is called) to include file_path= request.file_path (match how explain_code in search.py passes file_path) so Sentry receives the file_path alongside operation="impact_analysis" and repo_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/test_error_leaking.py`:
- Around line 47-66: The test currently only checks for a 400 and absence of
sensitive substrings, which can pass if InputValidator.validate_repo_name or
validate_git_url reject the request before repo_manager.add_repo is called;
update the test (test_repo_add_error_hides_details) to assert that
resp.json()["detail"] equals the exact generic error message that the route
returns when repo_manager.add_repo raises (the redacted message emitted by the
route's exception handler), and keep the repo_manager.add_repo side_effect in
place so the assertion guarantees the add_repo error path was exercised;
reference the validator functions InputValidator.validate_repo_name /
validate_git_url and the mocked repo_manager.add_repo to locate the relevant
behavior to pin the assertion to the canonical redacted message.
- Around line 9-26: The test can silently bypass the code path because
cache.get_search_results may return a hit; patch cache.get_search_results to
force a miss so indexer.semantic_search is invoked (e.g., patch the cache used
by routes.search so get_search_results returns None or an empty list) before
patching indexer.semantic_search; update the test_search_error_hides_details to
patch cache.get_search_results (or the cache object used by search_code) to
return a cache miss, then keep the existing patch of indexer.semantic_search to
raise the RuntimeError.
---
Outside diff comments:
In `@backend/routes/playground.py`:
- Around line 578-583: The _fetch_repo_metadata function currently returns the
raw exception string in {"error": "request_failed", "message": str(e)}, which
leaks internal errors to clients; change it to return a non-sensitive, generic
message (e.g. {"error": "request_failed", "message": "failed to fetch repository
metadata"}) and keep the full exception in the server logs via logger.exception
or logger.error with the exception context. Update _fetch_repo_metadata (and
related exception handling) to avoid exposing str(e); callers
validate_github_repo and start_anonymous_indexing will then use
metadata.get("message", ...) safely without leaking internal details. Ensure
httpx.TimeoutException still returns the explicit timeout message while all
other exceptions return the generic message and the error is logged with
traceback.
- Around line 486-490: The except block in playground.py currently only logs the
error and raises HTTPException; add a call to capture_exception(e) (as done in
search.py/search_v2.py/repos.py/analysis.py) before raising to ensure Sentry
receives the failure; update the except Exception as e handler in the Playground
search handler to call capture_exception(e) and then logger.error(...) and raise
HTTPException(status_code=500, detail="Search failed"); if capture_exception is
not already imported in this module, import it from the same location used by
the other handlers.
In `@backend/routes/repos.py`:
- Around line 143-149: The catch-all exception handler in
backend/routes/repos.py currently converts server-side failures into
HTTPException(status_code=400) which is incorrect; update the handler in the
add-repository flow (the except Exception as e block that logs with logger.error
and calls capture_exception) to raise HTTPException(status_code=500,
detail="Failed to add repository") instead, keeping the existing logger.error
and capture_exception calls and leaving the earlier except HTTPException
re-raise intact; also update the corresponding expectation in
test_error_leaking.py (the assertion at the test around line 63) to expect a 500
response rather than 400.
- Around line 388-400: The websocket and Redis pub/sub still send raw exception
strings to clients: in websocket_index (websocket.send_json) and in
_run_async_indexing (publisher.publish_error). Replace str(e) usage with a
non-sensitive, generic client-facing message (e.g. "indexing_failed" or "An
internal error occurred while indexing") or send an error_id/token tied to the
server-side log, while keeping the full exception only in
logger.error/capture_exception for internal diagnostics; update
websocket.send_json({... "message": ...}) and publisher.publish_error(...
message=...) accordingly and ensure logger.error/capture_exception continue to
record the original exception.
---
Nitpick comments:
In `@backend/routes/analysis.py`:
- Around line 79-82: The catch block in analyze_impact is logging the error with
file_path but calling capture_exception without file_path, so Sentry events lack
that context; update the capture_exception call in the except block of
analyze_impact (the same scope where logger.error is called) to include
file_path= request.file_path (match how explain_code in search.py passes
file_path) so Sentry receives the file_path alongside
operation="impact_analysis" and repo_id.
Code fixes: 1. repos.py add_repo: status 400 -> 500 (server failure, not client error) 2. repos.py _run_async_indexing: publisher.publish_error leaks str(e) to WebSocket clients. Now sends generic message. 3. repos.py websocket_index: send_json leaks str(e). Now generic. 4. playground.py _fetch_repo_metadata: returned raw str(e) in error dict. Now returns generic message, logs actual error. 5. playground.py search handler: added capture_exception (was missing entirely, not even imported). 6. analysis.py impact: added file_path to capture_exception kwargs (logger had it, Sentry didn't). Test improvements: 7. test_repo_add: assert status 500 (was 400), assert exact detail text. 8. test_search: explicitly patch cache.get_search_results to return None instead of relying on conftest mock behavior. 284 tests pass.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
backend/tests/test_error_leaking.py (2)
6-7: Consider addingcapture_exceptionpatches to prevent unintended Sentry calls in tests.If Sentry is initialized in CI (or becomes initialized later), these tests will fire real
capture_exceptioncalls. Patching it also lets you assert it was called with the expected args, strengthening the test.♻️ Example for one test (apply same pattern to all three)
def test_search_error_hides_details(self, client, valid_headers): """Search failure should not expose Pinecone/OpenAI error strings.""" with patch("routes.search.indexer") as mock_indexer, \ - patch("routes.search.cache") as mock_cache: + patch("routes.search.cache") as mock_cache, \ + patch("routes.search.capture_exception") as mock_capture: mock_cache.get_search_results.return_value = None ... + + mock_capture.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_error_leaking.py` around lines 6 - 7, Patch out Sentry's capture calls in these tests by mocking sentry_sdk.capture_exception (e.g., with unittest.mock.patch or pytest monkeypatch) inside TestErrorResponsesHideInternals (or each individual test) so tests don't call real Sentry in CI; update the tests to assert the mock was called with the expected exception/context and restore the original after the test. Locate the tests in TestErrorResponsesHideInternals and wrap the request/handler invocation with a patched capture_exception, or add a setup/teardown that applies the patch for all three tests, then replace any direct assertions about external effects with assertions against the mock.
1-68: Missing type hints on test method parameters.Per the coding guidelines,
backend/**/*.pyfiles should have type hints on all function signatures. The test methods lack them onself,client, andvalid_headers.♻️ Example fix
- def test_search_error_hides_details(self, client, valid_headers): + def test_search_error_hides_details(self, client: "TestClient", valid_headers: dict) -> None:As per coding guidelines,
backend/**/*.py: "Type hints on all function signatures in Python backend".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_error_leaking.py` around lines 1 - 68, The test methods test_search_error_hides_details, test_dependency_graph_error_hides_details, and test_repo_add_error_hides_details are missing type hints on their parameters; add annotations (e.g., self: "TestErrorResponsesHideInternals", client: TestClient or FlaskClient, valid_headers: dict[str, str]) to each signature and import any needed typing (from typing import Dict or use built-in dict[str, str]) so the file complies with the backend/**/*.py type-hint rule.backend/routes/playground.py (1)
488-491: Minor:capture_exceptionis called beforelogger.error— reversed from all other handlers.Every other error handler in this PR logs first, then captures (e.g.,
analysis.pyLines 43–44,repos.pyLines 146–147). Here the order is flipped. Not a bug, but inconsistent.♻️ Suggested reorder for consistency
except Exception as e: - capture_exception(e, operation="playground_search") logger.error("Playground search failed", error=str(e)) + capture_exception(e, operation="playground_search") raise HTTPException(status_code=500, detail="Search failed")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/playground.py` around lines 488 - 491, In the playground error handler for the playground_search operation, make the logging order consistent with other handlers by calling logger.error(...) before capture_exception(...); locate the except block that currently calls capture_exception(e, operation="playground_search") then logger.error(...) and swap them so the error is logged first (including the error string) and then capture_exception is invoked, leaving the HTTPException raise unchanged.backend/routes/repos.py (1)
146-148:capture_exception(e)calls in repos.py lack operation context — inconsistent with analysis.py.In
analysis.py, everycapture_exceptioncall passesoperation=andrepo_id=for Sentry context. Here (and in the other handlers at Lines 176, 269, 390, 480), only the bare exception is sent. This makes Sentry alerts harder to triage.♻️ Suggested improvement (same pattern for Lines 176, 269, 390, 480)
- capture_exception(e) + capture_exception(e, operation="add_repository", user_id=user_id)- capture_exception(e) + capture_exception(e, operation="delete_repository", repo_id=repo_id)- capture_exception(e) + capture_exception(e, operation="index_repository", repo_id=repo_id)- capture_exception(e) + capture_exception(e, operation="async_indexing", repo_id=repo_id)- capture_exception(e) + capture_exception(e, operation="start_async_indexing", repo_id=repo_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/repos.py` around lines 146 - 148, The capture_exception calls in repos.py should include Sentry context like the rest of the codebase; update each bare capture_exception(e) (the one following logger.error("Failed to add repository", error=str(e), user_id=user_id) and the similar handlers at the other error sites) to pass operation and repo_id (or repo_id=None when unavailable) — e.g., capture_exception(e, operation="add_repository", repo_id=repo_id) — and ensure the accompanying logger.error retains user_id and error details so Sentry events match the local logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/routes/playground.py`:
- Around line 488-491: In the playground error handler for the playground_search
operation, make the logging order consistent with other handlers by calling
logger.error(...) before capture_exception(...); locate the except block that
currently calls capture_exception(e, operation="playground_search") then
logger.error(...) and swap them so the error is logged first (including the
error string) and then capture_exception is invoked, leaving the HTTPException
raise unchanged.
In `@backend/routes/repos.py`:
- Around line 146-148: The capture_exception calls in repos.py should include
Sentry context like the rest of the codebase; update each bare
capture_exception(e) (the one following logger.error("Failed to add repository",
error=str(e), user_id=user_id) and the similar handlers at the other error
sites) to pass operation and repo_id (or repo_id=None when unavailable) — e.g.,
capture_exception(e, operation="add_repository", repo_id=repo_id) — and ensure
the accompanying logger.error retains user_id and error details so Sentry events
match the local logs.
In `@backend/tests/test_error_leaking.py`:
- Around line 6-7: Patch out Sentry's capture calls in these tests by mocking
sentry_sdk.capture_exception (e.g., with unittest.mock.patch or pytest
monkeypatch) inside TestErrorResponsesHideInternals (or each individual test) so
tests don't call real Sentry in CI; update the tests to assert the mock was
called with the expected exception/context and restore the original after the
test. Locate the tests in TestErrorResponsesHideInternals and wrap the
request/handler invocation with a patched capture_exception, or add a
setup/teardown that applies the patch for all three tests, then replace any
direct assertions about external effects with assertions against the mock.
- Around line 1-68: The test methods test_search_error_hides_details,
test_dependency_graph_error_hides_details, and test_repo_add_error_hides_details
are missing type hints on their parameters; add annotations (e.g., self:
"TestErrorResponsesHideInternals", client: TestClient or FlaskClient,
valid_headers: dict[str, str]) to each signature and import any needed typing
(from typing import Dict or use built-in dict[str, str]) so the file complies
with the backend/**/*.py type-hint rule.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
13 route handlers returned
detail=str(e)in HTTP 500 responses. In production this leaked:/srv/repos/...)Fix
All 13 catch blocks now return generic error messages. The actual error is logged to Sentry/stdout for debugging.
except HTTPException: raise(was swallowing 404s as 500s) + added logging (had zero)Secondary bug fixed
analysis.py had a hidden bug:
get_repo_or_404()raisesHTTPException(404)but the bareexcept Exceptionwas catching it and re-wrapping as a 500. Users requesting a non-existent repo got500 Internal Server Errorinstead of404 Not Found. Fixed by addingexcept HTTPException: raisebeforeexcept Exceptionon all 5 analysis endpoints.Tests
3 new tests verify that internal details (Pinecone URLs, file paths, git credentials) are NOT exposed in error responses.
284 tests pass (281 existing + 3 new).
Closes OPE-79
Summary by CodeRabbit
Bug Fixes
Tests