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

Avoid installing middleware if Content-Encoding is set at all #1560

Merged
merged 1 commit into from
Dec 20, 2021

Conversation

adamchainz
Copy link
Contributor

The previous check for "gzip" ignored the possibility of other Content-Encoding values, such as Brotli’s br, which the middleware could also not touch. Instead, do not attempt to alter the content for any Content-Encoding.

I tested by installing django-brotli in the example app below the toolbar middleware and hitting it with:

curl http://localhost:8000/ -H 'Accept-Encoding: br'

Without the change, it would crash with:

  File "/.../debug_toolbar/middleware.py", line 87, in __call__
    content = response.content.decode(response.charset)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc7 in position 1: invalid continuation byte

After, it is fixed.

The previous check for "gzip" ignored the possibility of other `Content-Encoding` values, such as Brotli’s ``br``, which the middleware could also not touch. Instead, do not attempt to alter the content for *any* `Content-Encoding`.

I tested by installing django-brotli in the example app below the toolbar middleware and hitting it with:

```
curl http://localhost:8000/ -H 'Accept-Encoding: br'
```

Without the change, it would crash with:

```
  File "/.../debug_toolbar/middleware.py", line 87, in __call__
    content = response.content.decode(response.charset)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc7 in position 1: invalid continuation byte
```

After, it is fixed.
@matthiask matthiask merged commit 1559500 into django-commons:main Dec 20, 2021
@matthiask
Copy link
Member

Thanks! It would be relatively easy to write a test for this but.... that's basically already a concern and not introduced by this proposal (following holacracy's governance rules)

@adamchainz adamchainz deleted the any_encoding branch December 20, 2021 20:31
@adamchainz
Copy link
Contributor Author

I am happy to find a test but couldn't pin down where instantly - if you can point me I will.

@tim-schilling
Copy link
Member

Duplicating test_middleware_response_injection where it has django.middleware.gzip.GZipMiddleware applied before sending it to the toolbar's middleware would cover the regression case of it. Alternatively manipulating the Content-Encoding directly without the additional middleware.

adamchainz added a commit to adamchainz/django-debug-toolbar that referenced this pull request Dec 20, 2021
@adamchainz
Copy link
Contributor Author

Thanks Tim, test PR in #1562.

tim-schilling pushed a commit that referenced this pull request Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants