Opened 9 years ago

Closed 9 years ago

#23682 closed New feature (fixed)

Stronger redirect loop detection in the test client

Reported by: Wojtek Ruszczewski Owned by: nobody
Component: Testing framework Version: dev
Severity: Normal Keywords:
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 test client has a mechanism to detect circular redirects. I'd like to propose two changes to it;

  • when a loop is detected, throw an exception rather than break silently;
  • detect and complain about very (possibly infinitely) long redirect chains with differing URLs (at least when they only differ by a view argument value or query string).

A few tests that are not part of the proposed changes, but demonstrate how one could try to test for loops (currently 3 of these just past, while a browser would run into a loop, and 2 actually loop indefinitely).

Change History (10)

comment:1 by Wojtek Ruszczewski, 9 years ago

Has patch: set

Proposed changes:

  • defined a new RedirectCycleError;
  • changed the loop detection to throw the exception on repeated URLs and chains longer than 20;
  • added a "redirect to self" test with a changing query argument.

Could all the loop prevention tests maybe live in test_client rather than test_client_regress?

Last edited 9 years ago by Wojtek Ruszczewski (previous) (diff)

comment:2 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature

It seems reasonable to me as infinite redirects could indeed be something a developer would want to be notified about. Skimming the original ticket where this was added (#4476), I couldn't find anything about why the decision was made.

Combining the test_client and test_client_regress tests would probably be welcome, but making it a separate commit/ticket is best.

comment:3 by Preston Timmons, 9 years ago

I reviewed and tested this patch. It looks good to me, although the import statements should reflect Django's convention of being alphabetized.

Do you want to create a pull request from your branch? This has the nice side-effect of triggering an automated test run and verifying the merge status.

comment:4 by Wojtek Ruszczewski, 9 years ago

Thanks. Did some imports reordering (see also #23860) and created a pull request.

comment:5 by Preston Timmons, 9 years ago

Needs documentation: set
Patch needs improvement: set

Thanks for adding the pull request. I left a few comments on it.

comment:6 by Wojtek Ruszczewski, 9 years ago

Thanks again; rebased, undid the unnecessary reordering. added docs on the new exception and a point to the 1.8 release notes.

comment:7 by Wojtek Ruszczewski, 9 years ago

Needs documentation: unset
Patch needs improvement: unset

comment:8 by Preston Timmons, 9 years ago

This patch looks good to me.

comment:9 by Preston Timmons, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:10 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In 056a3c6c374f15e23746ea8568cd5b11bfe7d442:

Fixed #23682 -- Enhanced circular redirects detection in tests.

When the test client detects a redirect to a URL seen in the
currently followed chain it will now raise a RedirectCycleError
instead of just returning the first repeated response.

It will also complain when a single chain of redirects is longer
than 20, as this often means a redirect loop with varying URLs,
and even if it's not actually one, such long chains are likely
to be treated as loops by browsers.

Thanks Preston Timmons, Berker Peksag, and Tim Graham for reviews.

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