Correctly handle aiohttp requests in Sentry reporting #5681
Conversation
📝 WalkthroughWalkthroughA new function, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant S as sanitize_url
participant H as sanitize_host
C->>S: Call sanitize_url(URL)
S->>S: Validate URL using regex pattern
S->>H: Call sanitize_host(extracted host)
H-->>S: Return validated host or default
S->>C: Return reconstructed URL
sequenceDiagram
participant C as Caller
participant F as filter_data
C->>F: Call filter_data(headers)
F->>F: Check & sanitize 'REFERER'
F->>F: Check & sanitize 'HEADER_TOKEN'
F->>F: Check & sanitize 'HEADER_TOKEN_OLD'
F->>F: Check & sanitize 'HOST'
F->>F: Check & sanitize 'X_FORWARDED_HOST'
F-->>C: Return modified headers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (7)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supervisor/misc/filter.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run tests Python 3.13.2
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
🔇 Additional comments (4)
supervisor/misc/filter.py (4)
3-3: Good addition for IP address handling.Adding the
ipaddressmodule is necessary for the new host validation functionality.
10-10: Appropriate import addition for Docker network validation.Including
DOCKER_NETWORK_MASKis necessary for filtering internal vs. external hosts.
17-28: Well-implemented host sanitization function.This function effectively identifies and preserves internal IP addresses within the Docker network while sanitizing external hosts. The try-except block properly handles non-IP hostnames.
32-39: Improved URL sanitization with contextual awareness.The updated implementation is more intelligent than the previous approach. It now preserves internal hosts while sanitizing external ones, which maintains security while improving functionality.
supervisor/misc/filter.py
Outdated
| headers = event["request"].get("headers", {}) | ||
| if isinstance(headers, dict): | ||
| if hdrs.REFERER in headers: | ||
| headers[hdrs.REFERER] = sanitize_url(headers[hdrs.REFERER]) | ||
| if HEADER_TOKEN in headers: | ||
| headers[HEADER_TOKEN] = "XXXXXXXXXXXXXXXXXXX" | ||
| if HEADER_TOKEN_OLD in headers: | ||
| headers[HEADER_TOKEN_OLD] = "XXXXXXXXXXXXXXXXXXX" | ||
| if hdrs.HOST in headers: | ||
| headers[hdrs.HOST] = sanitize_host(headers[hdrs.HOST]) | ||
| if hdrs.X_FORWARDED_HOST in headers: | ||
| headers[hdrs.X_FORWARDED_HOST] = sanitize_host( | ||
| headers[hdrs.X_FORWARDED_HOST] | ||
| ) |
There was a problem hiding this comment.
Fixed header handling to prevent exceptions.
This change directly addresses the PR objective by properly checking if headers is a dictionary before attempting to access it. The previous implementation likely caused exceptions when headers wasn't a dictionary, which is now handled correctly.
The code also improves header sanitization by:
- Sanitizing URLs in the REFERER header
- Masking token headers
- Sanitizing host headers using the new function
agners
changed the title
agners
force-pushed
the
fix-aiohttp-sentry-reporting
branch
from
e47a8e8 to
9da4dca
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/misc/test_sanitise_url.py (3)
3-4: Update the docstring to match the actual module path.The function docstrings mention "supervisor.utils" but the imports show the functions are in "supervisor.misc.filter". The docstrings should be updated to match the actual module path.
6-8: Add more test cases for sanitize_host.The test only checks one case (external hostname). Consider adding test cases for:
- Internal IP addresses that should not be sanitized
- IP addresses outside the Docker network
- Edge cases like localhost, empty strings, or None values
def test_sanitize_host(): """Test supervisor.misc.filter.sanitize_host.""" # External hostnames should be sanitized assert sanitize_host("my.duckdns.org") == "sanitized-host.invalid" # Internal IPs should not be sanitized assert sanitize_host("172.30.32.1") == "172.30.32.1" # Edge cases assert sanitize_host("localhost") == "localhost" assert sanitize_host("") == ""
14-22: Tests look good but could be more comprehensive.The updated tests correctly verify that external hostnames are sanitized in various URL formats. Consider adding test cases for URLs with internal IP addresses that should not be sanitized.
tests/misc/test_filter_data.py (2)
157-166: Consider making internal test assertions more specific.While the assertion that the filtered event equals the original event is valid, more specific assertions about what shouldn't be changed would make the test intent clearer and more robust against future changes.
- assert filtered == event + # Verify that internal URLs and headers remain unchanged + assert filtered["request"]["url"] == event["request"]["url"] + assert filtered["request"]["headers"]["Host"] == event["request"]["headers"]["Host"] + # Verify the entire structure remains unchanged + assert filtered == event
21-74: Add test cases for error handling.Given that the PR is addressing exceptions when unpacking headers, consider adding test cases for scenarios where headers are missing or have unexpected formats to ensure the error handling works correctly.
def test_sanitize_with_missing_headers(coresys): """Test event sanitation with missing headers.""" # Create a copy of the event with incomplete headers event = SAMPLE_EVENT_AIOHTTP_EXTERNAL.copy() event["request"] = event["request"].copy() event["request"]["headers"] = {} # Empty headers coresys.config.diagnostics = True coresys.core.state = CoreState.RUNNING with patch("shutil.disk_usage", return_value=(42, 42, 2 * (1024.0**3))): # This should not raise an exception filtered = filter_data(coresys, event, {}) # Verify basic structure is maintained assert "request" in filtered assert "headers" in filtered["request"]Also applies to: 138-166
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/misc/test_filter_data.py(2 hunks)tests/misc/test_sanitise_url.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build i386 supervisor
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.2
🔇 Additional comments (2)
tests/misc/test_filter_data.py (2)
21-74: Well-structured test data for both internal and external scenarios.The sample event dictionaries are comprehensive and provide good test cases for the filter function. They include realistic headers and URL patterns that would be encountered in production.
138-155: Test correctly verifies hostname sanitization in multiple headers.The test properly checks that the user hostname is sanitized from the URL, Host header, Referer header, and X-Forwarded-Host header.
The request header seems to be a dictionary in current Sentry SDK. The previous code actually failed with an exception when trying to unpack the header. However, it seems that Exceptions are not handled or printed in this filter function, so those issues were simply swallowed. The new code has been tested to correctly sanitize and report issues during aiohttp requests.
agners
force-pushed
the
fix-aiohttp-sentry-reporting
branch
from
9945277 to
fe30643
Compare
Proposed change
The request header seems to be a dictionary in current Sentry SDK. The previous code actually failed with an exception when trying to unpack the header. However, it seems that Exceptions are not handled or printed in this filter function, so those issues were simply swallowed.
The new code has been tested to correctly sanitize and report issues during aiohttp requests.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor