Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#16705 closed Bug (fixed)

r13746 makes invalid assumption that QUERY_STRING exists

Reported by: raylu Owned by: Aymeric Augustin
Component: Generic views Version: master
Severity: Release blocker Keywords:
Cc: preston@… 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 (last modified by Ramiro Morales)

r13746 (from #9966) makes the assumption that QUERY_STRING exists. As this is a bad assumption, please apply the attached patch.

See also: #9435.

Attachments (2)

13746_query_string.patch (459 bytes) - added by raylu 6 years ago.
don't assume QUERY_STRING exists
16705.patch (5.8 KB) - added by Aymeric Augustin 6 years ago.

Download all attachments as: .zip

Change History (16)

Changed 6 years ago by raylu

Attachment: 13746_query_string.patch added

don't assume QUERY_STRING exists

comment:1 Changed 6 years ago by Russell Keith-Magee

Easy pickings: set
Needs tests: set
Patch needs improvement: set
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Patch needs tests.

A patch is also required against the new-style class-based views, as it makes the same bad (albeit, almost universally honored) assumption.

Technically this is a release blocker too, since it is a new feature in the previous release.

comment:2 Changed 6 years ago by raylu

This one line change is tested by the same tests as the rest of r13746.

Last edited 6 years ago by raylu (previous) (diff)

comment:3 Changed 6 years ago by Russell Keith-Magee

@raylu -- Your claim is demonstrably false. If it were true, there would be a failing case in the test suite.

The case of query_string=True and query_string=False may be tested; but the non-existence of QUERY_STRING in the WSGI environment *isn't* tested, because the Django test client sets QUERY_STRING by default. We need a test that demonstrates that if QUERY_STRING *isn't* in the environment, the current code fails, but your patch makes the test pass.

comment:4 Changed 6 years ago by raylu

Oh. I see what you mean.

I'm not very familiar with how to write a Django test nor have I any idea what the new class-based views are, so I'll step down.

comment:5 Changed 6 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin

comment:6 Changed 6 years ago by Aymeric Augustin

Easy pickings: unset
Patch needs improvement: unset

I had to refactor the test client a bit to write tests for this issue. I modified the default environ dictionary used by django.test.client.RequestFactory to contain exactly the variables required by the WSGI spec (plus HTTP_COOKIE because we want to support coookies here). The test suite still passes :)

This change is slightly backwards incompatible. If third-party code uses Django's RequestFactory or Client for testing, and contains bugs similar to this one (ie. rely on a variable whose presence isn't guaranteed), its tests may fail after this change. I think this minor incompatibility is acceptable in the context of a bug fix. We could even argue it's a good thing to reveal unnoticed bugs.

I made two other small changes:

  • CommonMiddleware used if request.GET: to test if there is a query string, and then accessed request.META['QUERY_STRING']. I replaced the check with: if request.META.get('QUERY_STRING', ''): to make it obvious that it's safe to access request.META['QUERY_STRING'] at the next line. This changes very subtly the behavior of "append slash" and "prepend www" redirects. Previously, invalid query strings (like '&') wouldn't be passed on. I can't say if it was intentional. Now they are. It's really an edge case and I think it's worth making this code more explicit. If you disagree, just don't commit this part.
  • I stumbled upon an unrelated typo in the generic_views tests and fixed it.

Changed 6 years ago by Aymeric Augustin

Attachment: 16705.patch added

comment:7 Changed 6 years ago by Preston Holmes

Cc: preston@… added

comment:8 Changed 6 years ago by Preston Holmes

Needs tests: unset
Triage Stage: AcceptedReady for checkin

There could be objections to the changes to the test client, however they seem coupled pretty strongly to testing this patch.

There are a couple small other fixes tacked in, these seem minor enough to allow as riders to this patch.

patch applies to trunk@16829 - tests pass

comment:9 Changed 6 years ago by raylu

It seems like in django/middleware/ what you really want is

if 'QUERY_STRING' in request.META:

Everywhere else,

args = request.META.get('QUERY_STRING')

seems to make more sense since the check on the next line is merely if args

comment:10 Changed 6 years ago by Ramiro Morales

Description: modified (diff)

comment:11 in reply to:  9 Changed 6 years ago by Aymeric Augustin

Replying to raylu:

It seems like in django/middleware/ what you really want is
if 'QUERY_STRING' in request.META:

The code is written that way on purpose: if request.META['QUERY_STRING'] is the empty string, I don't want to append a query string.

Thanks for the review!

comment:12 Changed 6 years ago by Aymeric Augustin

Resolution: fixed
Status: newclosed

In [16933]:

Fixed #16705 - Made the test client adhere to the WSGI spec -- in particular, removed the assumption that environQUERY_STRING? exists.

comment:13 Changed 6 years ago by Julien Phalip

In [17763]:

Fixed #17828 -- Ensured that when a list filter's queryset() method fails, it does so loudly instead of getting swallowed by a IncorrectLookupParameters exception. This also properly fixes #16705, which hadn't been addressed correctly in [16705].

comment:14 Changed 6 years ago by Julien Phalip

Sorry, I meant to reference #16716, not this ticket, in the commit message above.

Note: See TracTickets for help on using tickets.
Back to Top