Opened 9 years ago

Closed 5 years ago

#6213 closed Bug (fixed)

PREPEND_WWW and APPEND_SLASH settings don't work with flatpages middleware

Reported by: Jason Davies 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 Jason Davies 9 years ago.
Patch for CommonMiddleware to fix handling of APPEND_SLASH and PREPEND_WWW
common_middleware.2.py.diff (8.9 KB) - added by Jason Davies 8 years ago.
Updated patch with unit tests.
common_middleware.3.py.diff (9.1 KB) - added by Jason Davies 8 years ago.
Updated patch with slightly improved unit tests (more descriptive docstring comments).
common_middleware.4.py.diff (9.1 KB) - added by Jason Davies 8 years ago.
Updated patch

Download all attachments as: .zip

Change History (34)

comment:1 Changed 9 years ago by Chris Beaven

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Summary: PREPEND_WWW and APPEND_SLASH settings don't work with flatpages middlewarePREPEND_WWW setting doesn't work with flatpages middleware
Triage Stage: UnreviewedAccepted

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

comment:2 Changed 9 years ago by Jason Davies

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 9 years ago by Chris Beaven

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 9 years ago by Jason Davies

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 9 years ago by Jason Davies

Attachment: common_middleware.py.diff added

Patch for CommonMiddleware to fix handling of APPEND_SLASH and PREPEND_WWW

comment:5 Changed 9 years ago by Jason Davies

Summary: PREPEND_WWW setting doesn't work with flatpages middlewarePREPEND_WWW and APPEND_SLASH settings don't work with flatpages middleware

comment:6 Changed 9 years ago by Jason Davies

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

comment:7 Changed 9 years ago by Jason Davies

This patch also fixes #6167.

comment:8 Changed 9 years ago by Chris Beaven

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 9 years ago by Chris Beaven

Keywords: easy-pickings added

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

Cc: semente@… added

Changed 8 years ago by Jason Davies

Attachment: common_middleware.2.py.diff added

Updated patch with unit tests.

Changed 8 years ago by Jason Davies

Attachment: common_middleware.3.py.diff added

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

comment:11 Changed 8 years ago by Jason Davies

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 Changed 8 years ago by Luke Plant

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

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

?

comment:13 Changed 8 years ago by Jacob

Resolution: fixed
Status: newclosed

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

comment:14 in reply to:  12 Changed 8 years ago by Jason Davies

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 8 years ago by Jason Davies

Resolution: fixed
Status: closedreopened

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 8 years ago by Jason Davies

Attachment: common_middleware.4.py.diff added

Updated patch

comment:16 Changed 8 years ago by Jason Davies

Cc: xarquonster@… added

comment:17 Changed 8 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 8 years ago by jonas

Cc: jm.bugtracking@… added

comment:19 in reply to:  13 ; Changed 8 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 8 years ago by che@…

Cc: che@… added

comment:21 in reply to:  19 Changed 6 years ago by Clay McClure

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 6 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 6 years ago by sjl

Cc: sjl added

comment:24 Changed 6 years ago by anonymous

Owner: changed from nobody to sswang
Status: reopenednew

comment:25 Changed 6 years ago by anonymous

Owner: changed from sswang to nobody

comment:26 Changed 6 years ago by orthagonal

Owner: changed from nobody to orthagonal

comment:27 Changed 5 years ago by Gabriel Hurley

Severity: Normal
Type: Bug

comment:28 Changed 5 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 5 years ago by Jannis Leidel

Component: Core (Other)contrib.flatpages

comment:30 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

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.

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