Django

Code

Ticket #9199 (closed: fixed)

Opened 2 months ago

Last modified 2 months ago

Common middleware PREPEND_WWW broken

Reported by: gonz Assigned to: nobody
Milestone: Component: Core framework
Version: SVN Keywords:
Cc: gonz Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

I think theres a bug in CommonMiddleware?'s PREPEND_WWW and it's not working as expected.

I think the bug was introduced after [8456], it will prepend 'www.' only if request.path_info doesn't have a trailing slash. I have reproduced this running under the dev web server and apache. Patch attached.

Attachments

patch.diff (501 bytes) - added by gonz on 09/24/08 11:24:34.
patch2.diff (2.9 kB) - added by gonz on 10/01/08 18:53:26.
patch2_tests_added.diff (4.4 kB) - added by gonz on 10/02/08 15:55:35.
Added tests to patch2.diff

Change History

09/24/08 11:24:34 changed by gonz

  • attachment patch.diff added.

09/26/08 13:29:16 changed by romke

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

works for me

09/30/08 16:12:15 changed by SmileyChris

  • needs_tests set to 1.
  • stage changed from Unreviewed to Accepted.

Patch looks good - it does look like that was a bad change in r8456

This would be a good ticket to add tests for PREPEND_WWW.

09/30/08 20:58:58 changed by mtredinnick

  • needs_better_patch set to 1.
  • needs_tests deleted.

No, this patch is bad. The problem report looks valid enough, but it doesn't seem to be caused by this line (or, if it is, it needs to be fixed differently). Any calls to Django's URL resolver must use request.path_info and new_url[1] is the same as request.path, which will include the SCRIPT_NAME prefix. Django's URL resolver doesn't care (and shouldn't know) about the script name prefix -- it won't work if you pass in something with such a script name. That's exactly why r8456 was introduced (as discussed in the ticket the changeset refers to; all the history is available).

This stuff is almost impossible to test in the automated test suite as a practical matter, since it requires control over domain names and installation paths, so I'm not too worried about manual testing for this stuff. But the patch still has to be correct.

10/01/08 03:28:30 changed by SmileyChris

Oops, I brushed over the details and put my foot in it. Malcolm's right that this isn't the correct fix - but the issue is definitely around this line.

The whole logic being used here is a bit messed up. That resolve depends on needing a slash to be entered - it could just be a www change which is needed.

10/01/08 18:52:47 changed by gonz

  • cc set to gonz.
  • needs_better_patch deleted.
  • needs_tests set to 1.

Thank you Malcom, now the problem it's much clear.

I think we should just move the last resolve. Chris is right and It only makes sense to try that resolve if we are adding a trailing slash. I'm attaching a new patch.

10/01/08 18:53:26 changed by gonz

  • attachment patch2.diff added.

10/02/08 15:55:35 changed by gonz

  • attachment patch2_tests_added.diff added.

Added tests to patch2.diff

10/02/08 16:00:21 changed by gonz

  • needs_tests deleted.

Added some simple test cases.

10/07/08 03:24:36 changed by mtredinnick

  • status changed from new to closed.
  • resolution set to fixed.

Fixed in r9184.

10/07/08 03:26:32 changed by mtredinnick

(In [9185]) [1.0.X] Fixed #9199 -- We were erroneously only prepending "www" to the domain if we also needed to append a slash (when PREPEND_WWW=True).

Based on a patch and tests from gonz. Thanks.

Backport of r9184 from trunk.

Add/Change #9199 (Common middleware PREPEND_WWW broken)




Change Properties
Action