Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle MultiPartParserError to avoid internal sentry crash #4001

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

orhanhenrik
Copy link
Contributor

Handles an internal error in sentry_sdk if there is an issue with parsing request.POST. See attached stack trace of this internal error that we experienced in production.

It would be better to handle this exception without request data instead of crashing and not reporting anything.

 [sentry] ERROR: Internal error in sentry_sdk
Traceback (most recent call last):
  File "/usr/src/app/.venv/lib/python3.13/site-packages/sentry_sdk/integrations/django/__init__.py", line 588, in parsed_body
    return self.request.data
           ^^^^^^^^^^^^^^^^^
AttributeError: 'WSGIRequest' object has no attribute 'data'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/usr/src/app/.venv/lib/python3.13/site-packages/sentry_sdk/integrations/django/__init__.py", line 508, in wsgi_request_event_processor
    DjangoRequestExtractor(request).extract_into_event(event)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^
  File "/usr/src/app/.venv/lib/python3.13/site-packages/sentry_sdk/integrations/_wsgi_common.py", line 118, in extract_into_event
    parsed_body = self.parsed_body()
  File "/usr/src/app/.venv/lib/python3.13/site-packages/sentry_sdk/integrations/django/__init__.py", line 590, in parsed_body
    return RequestExtractor.parsed_body(self)
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^
  File "/usr/src/app/.venv/lib/python3.13/site-packages/sentry_sdk/integrations/_wsgi_common.py", line 152, in parsed_body
    form = self.form()
  File "/usr/src/app/.venv/lib/python3.13/site-packages/sentry_sdk/integrations/django/__init__.py", line 575, in form
    return self.request.POST
           ^^^^^^^^^^^^^^^^^
  File "/usr/src/app/.venv/lib/python3.13/site-packages/django/core/handlers/wsgi.py", line 93, in _get_post
    self._load_post_and_files()
    ~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/usr/src/app/.venv/lib/python3.13/site-packages/django/http/request.py", line 374, in _load_post_and_files
    self._post, self._files = self.parse_file_upload(self.META, data)
                              ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^
  File "/usr/src/app/.venv/lib/python3.13/site-packages/django/http/request.py", line 321, in parse_file_upload
    parser = MultiPartParser(META, post_data, self.upload_handlers, self.encoding)
  File "/usr/src/app/.venv/lib/python3.13/site-packages/django/http/multipartparser.py", line 89, in __init__
    raise MultiPartParserError(
        "Invalid boundary in multipart: %s" % force_str(boundary)
    )
django.http.multipartparser.MultiPartParserError: Invalid boundary in multipart: None

Thank you for contributing to sentry-python! Please add tests to validate your changes, and lint your code using tox -e linters.

Running the test suite on your PR might require maintainer approval. The AWS Lambda tests additionally require a maintainer to add a special label, and they will fail until this label is added.

@sentrivana
Copy link
Contributor

Hey @orhanhenrik, thanks for the PR!

I agree that we should handle this better, but I'd try to catch the exception at the source and try to be more defensive in general. The exception is originally thrown here, so we could do something like this there:

    def parsed_body(self):
        # type: () -> Optional[Dict[str, Any]]
-       form = self.form()
+       try:
+           form = self.form()
+       except Exception:
+           form = None
-       files = self.files()
+       try:
+           files = self.files()
+       except Exception:
+           files = None
+
        if form or files:
            data = {}
            if form:
                data = dict(form.items())
            if files:
                for key in files.keys():
                    data[key] = AnnotatedValue.removed_because_raw_data()

            return data

        return self.json()

I'm also wrapping the self.files() call to be safe.

I expect we might also need to wrap the self.json() call -- might be that will also throw, in which case we should just return None. Needs testing.

Handles an internal error in sentry_sdk if there is an issue with parsing request.POST. See attached stack trace of this internal error that we experienced in production.

It would be better to handle this exception without request data instead of crashing and not reporting anything.

```
 [sentry] ERROR: Internal error in sentry_sdk
Traceback (most recent call last):
  File "/usr/src/app/.venv/lib/python3.13/site-packages/sentry_sdk/integrations/django/__init__.py", line 588, in parsed_body
    return self.request.data
           ^^^^^^^^^^^^^^^^^
AttributeError: 'WSGIRequest' object has no attribute 'data'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/usr/src/app/.venv/lib/python3.13/site-packages/sentry_sdk/integrations/django/__init__.py", line 508, in wsgi_request_event_processor
    DjangoRequestExtractor(request).extract_into_event(event)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^
  File "/usr/src/app/.venv/lib/python3.13/site-packages/sentry_sdk/integrations/_wsgi_common.py", line 118, in extract_into_event
    parsed_body = self.parsed_body()
  File "/usr/src/app/.venv/lib/python3.13/site-packages/sentry_sdk/integrations/django/__init__.py", line 590, in parsed_body
    return RequestExtractor.parsed_body(self)
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^
  File "/usr/src/app/.venv/lib/python3.13/site-packages/sentry_sdk/integrations/_wsgi_common.py", line 152, in parsed_body
    form = self.form()
  File "/usr/src/app/.venv/lib/python3.13/site-packages/sentry_sdk/integrations/django/__init__.py", line 575, in form
    return self.request.POST
           ^^^^^^^^^^^^^^^^^
  File "/usr/src/app/.venv/lib/python3.13/site-packages/django/core/handlers/wsgi.py", line 93, in _get_post
    self._load_post_and_files()
    ~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/usr/src/app/.venv/lib/python3.13/site-packages/django/http/request.py", line 374, in _load_post_and_files
    self._post, self._files = self.parse_file_upload(self.META, data)
                              ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^
  File "/usr/src/app/.venv/lib/python3.13/site-packages/django/http/request.py", line 321, in parse_file_upload
    parser = MultiPartParser(META, post_data, self.upload_handlers, self.encoding)
  File "/usr/src/app/.venv/lib/python3.13/site-packages/django/http/multipartparser.py", line 89, in __init__
    raise MultiPartParserError(
        "Invalid boundary in multipart: %s" % force_str(boundary)
    )
django.http.multipartparser.MultiPartParserError: Invalid boundary in multipart: None
```
@orhanhenrik
Copy link
Contributor Author

Sounds good to be more defensive. I've updated the PR with your suggestions.

Ideally it would be nice to show in Sentry UI that it failed to parse POST/files of the request, but this PR should be an improvement either way, since the error is reported to Sentry instead of being silently ignored.

@sentrivana sentrivana added the Trigger: tests using secrets PR code is safe; run CI label Jan 31, 2025
@sentrivana sentrivana enabled auto-merge (squash) January 31, 2025 12:24
@sentrivana sentrivana closed this Jan 31, 2025
auto-merge was automatically disabled January 31, 2025 12:24

Pull request was closed

@sentrivana sentrivana reopened this Jan 31, 2025
@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Jan 31, 2025
@sentrivana
Copy link
Contributor

sentrivana commented Jan 31, 2025

Sounds good to be more defensive. I've updated the PR with your suggestions.

Thank you!

Ideally it would be nice to show in Sentry UI that it failed to parse POST/files of the request, but this PR should be an improvement either way, since the error is reported to Sentry instead of being silently ignored.

Agree that this can be handled better but it's a good start. I'm not sure why the WSGIRequest doesn't have a data attribute to start with -- it might be this is some deeper problem with our patching. But in any case, this is a good bandaid for now.

(Btw I didn't mean to close the PR, I just misclicked -- this is good to merge from my POV. Thanks again!)

@sentrivana sentrivana enabled auto-merge (squash) January 31, 2025 12:26
@sentrivana sentrivana added the Trigger: tests using secrets PR code is safe; run CI label Jan 31, 2025
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 80.15%. Comparing base (8c25c73) to head (1f6f71b).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
sentry_sdk/integrations/_wsgi_common.py 50.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4001      +/-   ##
==========================================
- Coverage   80.16%   80.15%   -0.02%     
==========================================
  Files         139      139              
  Lines       15397    15403       +6     
  Branches     2597     2597              
==========================================
+ Hits        12343    12346       +3     
- Misses       2210     2213       +3     
  Partials      844      844              
Files with missing lines Coverage Δ
sentry_sdk/integrations/_wsgi_common.py 77.20% <50.00%> (-2.03%) ⬇️

... and 2 files with indirect coverage changes

@sentrivana sentrivana merged commit 91bf322 into getsentry:master Jan 31, 2025
259 of 305 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Trigger: tests using secrets PR code is safe; run CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants