Code

Opened 6 years ago

Closed 3 years ago

#6213 closed Bug (fixed)

PREPEND_WWW and APPEND_SLASH settings don't work with flatpages middleware

Reported by: esaj Owned by: orthagonal
Component: contrib.flatpages Version: master
Severity: Normal Keywords: APPEND_SLASH PREPEND_WWW flatpages easy-pickings
Cc: semente@…, xarquonster@…, jm.bugtracking@…, che@…, sjl Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

The PREPEND_WWW and APPEND_SLASH settings don't force a redirect when FlatpageFallbackMiddleware is being used.

This is because a redirect only happens if the target URL can be resolved using urlresolvers.resolve() in the code.

This behaviour has occurred since the change in [6852].

If the target URL cannot be resolved immediately using urlresolvers.resolve(), perhaps we should check in process_response() to make absolutely sure the response status code will be 404? If some intermediate middleware, e.g. FlatpageFallbackMiddleware has returned a successful response code, we can then issue a redirect as per the old behaviour.

Attachments (4)

common_middleware.py.diff (5.0 KB) - added by esaj 6 years ago.
Patch for CommonMiddleware to fix handling of APPEND_SLASH and PREPEND_WWW
common_middleware.2.py.diff (8.9 KB) - added by esaj 6 years ago.
Updated patch with unit tests.
common_middleware.3.py.diff (9.1 KB) - added by esaj 6 years ago.
Updated patch with slightly improved unit tests (more descriptive docstring comments).
common_middleware.4.py.diff (9.1 KB) - added by esaj 6 years ago.
Updated patch

Download all attachments as: .zip

Change History (34)

comment:1 Changed 6 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from PREPEND_WWW and APPEND_SLASH settings don't work with flatpages middleware to PREPEND_WWW setting doesn't work with flatpages middleware
  • Triage Stage changed from Unreviewed to Accepted

#6167 handles APPEND_SLASH (someone didn't search before adding a ticket :P). Rewording summary to just cover PREPEND_WWW

comment:2 Changed 6 years ago by esaj

Ah, I didn't see #6167, but I don't think it's exactly the same: this ticket concerns the use of FlatpageFallbackMiddleware whereas #6167 seems to concern the use of the flatpage view (at least from looking at the patch).

It's actually a more general problem, in that if a middleware returns a successful response code (even when no view is mapped to the current URL), then ideally we should honour the APPEND_SLASH and PREPEND_WWW settings and issue a redirect.

comment:3 Changed 6 years ago by SmileyChris

Correct, they are different issues (hence why I didn't close).

To make APPEND_SLASH always append a slash (on a successful response code) goes against the new behaviour. It should only try the slash if it 404s.
Sure, fix PREPEND_WWW like that though (even though I think it's a stupid setting that should belong to a web server, not the Django framework).

comment:4 Changed 6 years ago by esaj

Ah yes, what I should have said is that in CommonMiddleware's process_response() we should append a slash if it 404s and APPEND_SLASH is set.

The problem is that if you use FlatpageFallbackMiddleware, redirects do not happen even if they did before the change in [6852], as the change relies on being able to resolve the URL using the URLconf.

For example, if the URL "/foo" isn't found by FlatpageFallbackMiddleware, we should still redirect if APPEND_SLASH is set.

Changed 6 years ago by esaj

Patch for CommonMiddleware to fix handling of APPEND_SLASH and PREPEND_WWW

comment:5 Changed 6 years ago by esaj

  • Summary changed from PREPEND_WWW setting doesn't work with flatpages middleware to PREPEND_WWW and APPEND_SLASH settings don't work with flatpages middleware

comment:6 Changed 6 years ago by esaj

Changed the summary back since this ticket still covers the APPEND_SLASH problem.

comment:7 Changed 6 years ago by esaj

This patch also fixes #6167.

comment:8 Changed 6 years ago by SmileyChris

  • Has patch set
  • Needs tests set

From a brief overview, I'm not 100% sure this fixes #6167 (it will always resolve the non-slash if you're using flatpages as the fallback in urlconf rather than using the middleware).

Go ahead and steal the tests in the other ticket, make sure this ticket fixes every case and then close #6167 as superceeded by this one. (You'll probably need some more tests for the things you're fixing)

comment:9 Changed 6 years ago by SmileyChris

  • Keywords easy-pickings added

comment:10 Changed 6 years ago by Guilherme M. Gondim <semente@…>

  • Cc semente@… added

Changed 6 years ago by esaj

Updated patch with unit tests.

Changed 6 years ago by esaj

Updated patch with slightly improved unit tests (more descriptive docstring comments).

comment:11 Changed 6 years ago by esaj

  • Needs tests unset

The unit tests catch the problem reported by this ticket, as well as checking some cases in #6167, which this ticket supercedes.

comment:12 follow-up: Changed 6 years ago by lukeplant

Jason: In your patch, file django/middleware/common.py, line 108,

if resolve and new_url[0] == new_url[0] and ...

?

comment:13 follow-up: Changed 6 years ago by jacob

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

(In [8218]) Fixed #6213: flatpage view now correctly redirects if settings.APPEND_SLASH. Thanks, crankycoder.

comment:14 in reply to: ↑ 12 Changed 6 years ago by esaj

Replying to lukeplant:

Jason: In your patch, file django/middleware/common.py, line 108,

if resolve and new_url[0] == new_url[0] and ...

Oops, that should be:

if resolve and new_url[0] == old_url[0] and ...

comment:15 Changed 6 years ago by esaj

  • Resolution fixed deleted
  • Status changed from closed to reopened

This issue has still not been fixed: if settings.PREPEND_WWW is True and I have a flatpage mapped to '/', http://jasondavies.com/ doesn't redirect to http://www.jasondavies.com/

This is because in the code at the moment, a redirect only happens if the target URL can be resolved using urlresolvers.resolve() , which obviously doesn't work when FlatpageFallbackMiddleware is used. My patch fixes this problem by checking for a successful response code from any preceding middleware, and issuing a redirect if necessary as per the old behaviour.

Changed 6 years ago by esaj

Updated patch

comment:16 Changed 6 years ago by esaj

  • Cc xarquonster@… added

comment:17 Changed 6 years ago by jonas

The current solution also leads to an infinite "302 Found" redirect if APPEND_SLASH is True and a flatpage exists at "/".

Should I file a new bug for this?

comment:18 Changed 6 years ago by jonas

  • Cc jm.bugtracking@… added

comment:19 in reply to: ↑ 13 ; follow-up: Changed 6 years ago by che@…

Replying to jacob:

(In [8218]) Fixed #6213: flatpage view now correctly redirects if settings.APPEND_SLASH. Thanks, crankycoder.

Changeset [8218] also causes flatpages to trigger redirect on all URLs that don't end with a slash and return 404 response, regardless whether any page exists.

For example, if you have flatpage '/about/', flagpages are going to redirect '/about' -> '/about/', but also '/robots.txt' to '/robots.txt/' (if you don't have the file handled in urlconf), etc.

comment:20 Changed 6 years ago by che@…

  • Cc che@… added

comment:21 in reply to: ↑ 19 Changed 3 years ago by clay

  • Patch needs improvement set

I echo the concern noted above, that the patch in [8218] blindly redirects without testing to see whether the target of the redirect is valid (i.e., matches a URLconf or a FlatPage). At best, this adds extra request-response latency to 404 handling. At worst, it obscures the nature of the 404. Clients requesting /robots.txt should receive a 404 if that resource does not exist, and should not be redirected to a different resource (/robots.txt/).

For me, this problem manifests as a failing test case. I expect my REST API (which uses non-slash-terminated URLs) to return a 404 under certain conditions, but was surprised to find it returning a 302 -- due to FlatpageFallbackMiddleware blindly redirecting clients to a hypothetical slash-terminated resource instead of passing the 404 through to the client.

In short, FlatpageFallbackMiddleware should honor the APPEND_SLASH logic described in #3228, with one addition [shown in CAPS]:

"If APPEND_SLASH is set and the initial URL doesn't end with a slash, and it is not found in urlpatterns, a new URL is formed by appending a slash at the end. If this new URL is found in urlpatterns OR THE FLATPAGE TABLE, then an HTTP-redirect is returned to this new URL; otherwise the initial URL is processed as usual."

comment:22 Changed 3 years ago by sjl

I've sent a pull request to fix the issue clay mentioned (and add some tests): https://github.com/django/django/pull/14

comment:23 Changed 3 years ago by sjl

  • Cc sjl added

comment:24 Changed 3 years ago by anonymous

  • Owner changed from nobody to sswang
  • Status changed from reopened to new

comment:25 Changed 3 years ago by anonymous

  • Owner changed from sswang to nobody

comment:26 Changed 3 years ago by orthagonal

  • Owner changed from nobody to orthagonal

comment:27 Changed 3 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Bug

comment:28 Changed 3 years ago by sjl

I've merged that pull request with upstream so it's up to date. Anyone want to take a look at it?

comment:29 Changed 3 years ago by jezdez

  • Component changed from Core (Other) to contrib.flatpages

comment:30 Changed 3 years ago by jezdez

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

In [16048]:

Fixed #6213 -- Updated the flatpages app to only append a slash if the flatpage actually exist.

The FlatpageFallbackMiddleware (and the view) now only add a trailing slash and redirect if the resulting URL refers to an existing flatpage. Previously requesting /notaflatpageoravalidurl would redirect to /notaflatpageoravalidurl/, which would then raise a 404. Requesting /notaflatpageoravalidurl now will immediately raise a 404. Also, Redirects returned by flatpages are now permanent (301 status code) to match the behaviour of the CommonMiddleware.

Thanks to Steve Losh for the initial work on the patch.

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.