Code

Opened 2 years ago

Closed 5 weeks ago

#17552 closed Bug (fixed)

The GZip Middleware's browser checking is buggy, as it does not send compressed content to modern versions of IE that support it.

Reported by: cannona Owned by: nobody
Component: HTTP handling Version: master
Severity: Normal Keywords: gzip middleware
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The GZip middleware currently only tests if the browser is IE. However, there are only three browser versions that I was able to identify with bugs in how they handle GZipped content. They are IE5, IE6 and Netscape4. However, the current code assumes that all IE versions have such bugs, and doesn't test at all for Netscape. This means that a lot of pages that could be compressed aren't being compressed because the browser is IE. Furthermore, the broken versions of IE and Netscape came out over 10 years ago, so they are a small minority.

Attachments (1)

middleware_gzip_patch_with_docs.diff (6.2 KB) - added by cannona 2 years ago.
Patch, includes docs and tests

Download all attachments as: .zip

Change History (12)

Changed 2 years ago by cannona

Patch, includes docs and tests

comment:1 Changed 2 years ago by cannona

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

The patch does the following:

  1. adds a check for IE5, IE6 and Netscape 4, and if the browser matches, doesn't compress the content.
  2. Rearranges the code a bit so that more common cases to bail out are tested for first.
  3. Adds a few tests for the new code.
  4. Updates the docs to clarify what browsers are tested for.

comment:2 Changed 2 years ago by cannona

  • Has patch set

comment:3 Changed 2 years ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Triage Stage changed from Unreviewed to Accepted

This looks interesting.

comment:4 Changed 2 years ago by aaugustin

  • Owner changed from aaugustin to nobody
  • Patch needs improvement set

The patch is quite good, thanks!

patch_vary_headers(response, ('Accept-Encoding',)) must be done before ae = request.META.get('HTTP_ACCEPT_ENCODING', ''), because the result depends on the Accept-Encoding header from the next line on.

How did you determine the regexps for the various user agents? Is there a reference of user agents strings where I can check them?

comment:5 follow-up: Changed 5 weeks ago by aaugustin

  • Resolution set to wontfix
  • Status changed from new to closed

I think usage of IE 6 and Netscape 4 has fallen sufficiently to make this unnecessary. Sorry.

comment:6 in reply to: ↑ 5 Changed 5 weeks ago by cannona

  • Resolution wontfix deleted
  • Status changed from closed to new

Replying to aaugustin:

I think usage of IE 6 and Netscape 4 has fallen sufficiently to make this unnecessary. Sorry.

Hi. I certainly can't argue with this. However, I suspect you might be misunderstanding the nature of the bug this ticket calls out. As I understand it, the current code does not do any compression on certain types of content meant for IE, no matter the version. This is the bug I was reporting.

So, IMHO, the proper solution here, if we don't wish to support the older browsers, is to disable any checking of the user agent string, and just rely on whether or not they request compressed content. That way, we're not refusing to send compressed content to modern versions of IE when it's requested.

comment:7 Changed 5 weeks ago by aaugustin

Oops. Thank you for reopening.

comment:8 Changed 5 weeks ago by cannona

  • Summary changed from The GZip Middleware's browser checking is too broad and misses versions of Netscape which have similar bugs to The GZip Middleware's browser checking is buggy, as it does not send compressed content to modern versions of IE that support it.

comment:9 Changed 5 weeks ago by aaugustin

  • Patch needs improvement unset

comment:10 Changed 5 weeks ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

comment:11 Changed 5 weeks ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

In df09d854828bcff56eb72f48ff1ba8fce7e90c90:

Fixed #17552 -- Removed a hack for IE6 and earlier.

It prevented the GZipMiddleware from compressing some data types even on
more recent version of IE where the corresponding bug was fixed.

Thanks Aaron Cannon for the report and Tim Graham for the review.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.