fix: prevent internal exception details from leaking in 500 error responses#3304
Open
deacon-mp wants to merge 3 commits into
Open
fix: prevent internal exception details from leaking in 500 error responses#3304deacon-mp wants to merge 3 commits into
deacon-mp wants to merge 3 commits into
Conversation
Add internal_error_middleware to prevent leaking exception details to clients.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an aiohttp middleware to prevent internal exception details from being returned to clients in 500 responses, while still logging the underlying exception server-side.
Changes:
- Added
internal_error_middlewareto catch unhandled exceptions, log them, and return a generic JSON 500. - Added security-focused tests for generic 500 responses and pass-through behavior for non-500 cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/api/v2/responses.py | Introduces middleware that logs unhandled exceptions and replaces client-facing 500 bodies with a generic JSON error. |
| tests/security/test_internal_error_middleware.py | Adds unit tests validating the middleware’s behavior for unhandled exceptions, HTTP exceptions, and normal responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+75
to
+76
| except web.HTTPException: | ||
| raise # Let aiohttp handle HTTP exceptions normally |
Comment on lines
+78
to
+82
| logging.getLogger('caldera').exception('Unhandled exception in request handler') | ||
| raise web.HTTPInternalServerError( | ||
| content_type='application/json', | ||
| text='{"error": "An internal server error occurred"}' | ||
| ) |
Comment on lines
+23
to
+27
| def test_http_exceptions_pass_through(self): | ||
| from app.api.v2.responses import internal_error_middleware | ||
|
|
||
| request = MagicMock() | ||
| handler = AsyncMock(side_effect=web.HTTPNotFound()) |
- Catch 5xx HTTPExceptions separately so handlers raising HTTPInternalServerError with internal detail are sanitised before sending to clients; only 4xx are re-raised unchanged - Replace hard-coded JSON string with json.dumps() for correctness and easier future evolution of the error payload shape - Add tests: sanitisation of HTTPInternalServerError raised by a handler, and verification that 4xx exceptions pass through unmodified
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an aiohttp middleware to prevent leaking internal exception details to clients by replacing 5xx error bodies with a generic JSON message, plus a dedicated test suite validating sanitized 500 behavior.
Changes:
- Added
internal_error_middlewareto sanitize 5xx error responses and log server-side details. - Added unit tests covering unhandled exceptions, HTTP 4xx pass-through, and sanitization of HTTP 500 details.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/api/v2/responses.py | Introduces middleware that catches exceptions, logs details, and returns a generic JSON 500 response. |
| tests/security/test_internal_error_middleware.py | Adds tests to ensure exception details are not present in client-facing 500 responses and 4xx/normal responses are unaffected. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+84
to
+86
| # 5xx: log and replace with sanitised response to avoid leaking detail | ||
| logging.getLogger('caldera').exception('HTTP %d in request handler', exc.status_code) | ||
| raise web.HTTPInternalServerError( |
Comment on lines
+86
to
+95
| raise web.HTTPInternalServerError( | ||
| content_type='application/json', | ||
| text=json.dumps({'error': 'An internal server error occurred'}) | ||
| ) | ||
| except Exception: | ||
| logging.getLogger('caldera').exception('Unhandled exception in request handler') | ||
| raise web.HTTPInternalServerError( | ||
| content_type='application/json', | ||
| text=json.dumps({'error': 'An internal server error occurred'}) | ||
| ) |
Comment on lines
+14
to
+21
| loop = asyncio.new_event_loop() | ||
| try: | ||
| with self.assertRaises(web.HTTPInternalServerError) as ctx: | ||
| loop.run_until_complete(internal_error_middleware(request, handler)) | ||
| self.assertIn('internal server error', ctx.exception.text) | ||
| self.assertNotIn('secret db error', ctx.exception.text) | ||
| finally: | ||
| loop.close() |
Comment on lines
+81
to
+89
| except web.HTTPException as exc: | ||
| if exc.status_code < 500: | ||
| raise # 4xx: let aiohttp handle normally, details are safe to expose | ||
| # 5xx: log and replace with sanitised response to avoid leaking detail | ||
| logging.getLogger('caldera').exception('HTTP %d in request handler', exc.status_code) | ||
| raise web.HTTPInternalServerError( | ||
| content_type='application/json', | ||
| text=json.dumps({'error': 'An internal server error occurred'}) | ||
| ) |
|
Address Copilot review: - 5xx HTTPExceptions now preserve their original status code and headers (e.g. 503 with Retry-After) instead of being rewritten to 500 - Factored generic error body into module-level constant - Use module-level logger instead of per-request getLogger call - Middleware returns web.Response instead of raising, so 5xx responses are properly sanitised while keeping status/headers - Added tests for 503 and 502 status code preservation - Rewrote all tests to use pytest.mark.asyncio instead of manual loops - Removed __main__ block
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an aiohttp middleware to ensure 5xx responses don’t expose internal exception details to clients, while keeping diagnostics in server logs.
Changes:
- Introduces
internal_error_middlewareto sanitize 5xx error bodies and log exceptions - Adds security-focused tests validating sanitization behavior and status/header preservation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/api/v2/responses.py | Adds middleware that converts unhandled exceptions / 5xx HTTPExceptions into generic JSON error bodies while preserving status codes (and some headers). |
| tests/security/test_internal_error_middleware.py | Adds tests covering generic 5xx responses, 4xx passthrough behavior, and preservation of non-500 5xx status codes and Retry-After. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+96
to
+102
| except Exception: | ||
| _log.exception('Unhandled exception in request handler') | ||
| return web.Response( | ||
| status=500, | ||
| content_type='application/json', | ||
| text=_GENERIC_ERROR_BODY, | ||
| ) |
Comment on lines
+90
to
+94
| return web.Response( | ||
| status=exc.status_code, | ||
| content_type='application/json', | ||
| text=_GENERIC_ERROR_BODY, | ||
| headers=exc.headers, |
Comment on lines
1
to
9
| import json | ||
| import logging | ||
|
|
||
| from aiohttp import web | ||
|
|
||
| _log = logging.getLogger('caldera') | ||
|
|
||
| _GENERIC_ERROR_BODY = json.dumps({'error': 'An internal server error occurred'}) | ||
| from json import JSONDecodeError |
Comment on lines
+17
to
+19
| assert resp.status == 500 | ||
| assert 'internal server error' in resp.text | ||
| assert 'secret db error' not in resp.text |
5 tasks
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
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.
Summary
500 error responses in
app/api/v2/responses.pyincluded full exception messages and stack traces, leaking internal implementation details to clients. Added middleware to strip exception details from responses before they reach clients.Changes
app/api/v2/responses.py: added middleware to sanitize 500 error response bodies, replacing internal exception details with a generic error messageSecurity Impact
Exception messages and stack traces expose file paths, variable names, library versions, and logic details that help attackers fingerprint the application and craft targeted exploits. Stripping this information from client-facing responses while retaining it in server logs is a defence-in-depth measure.
Test plan