Opened 4 weeks ago

Closed 2 weeks ago

#29364 closed Cleanup/optimization (wontfix)

CommonMiddleware.get_full_path_with_slash should raise exception for POST / PUT / PATCH requests even if settings.DEBUG = False

Reported by: Nilesh Trivedi Owned by: nobody
Component: HTTP handling Version: 2.0
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no


Redirecting POST / PUT / PATCH to GET in should_redirect_with_slash() is undesirable behavior even if APPEND_SLASH = True and DEBUG = False.

Change History (6)

comment:1 Changed 4 weeks ago by Tim Graham

Component: UncategorizedHTTP handling
Type: BugCleanup/optimization

It's not immediately obvious to me why the proposed behavior (which would invoke handler500 and display an "internal server error" page) is better than the current behavior. The original commit, 3aa6b0556f74b823673bc854c3c8be92d8fbabf0, does not have much information. Perhaps the reason for only doing the check in debug mode is that the problem will usually be detected in development. Doing the check unconditionally could allow malicious requests to fill error logs.

comment:2 Changed 4 weeks ago by Claude Paroz

In that situation, returning a 404 instead could be acceptable.

comment:3 Changed 3 weeks ago by Tim Graham

I would like the better understand the rationale for the proposed change. Why is the current behavior problematic and why would returning a 404 be better?

comment:4 Changed 3 weeks ago by Nilesh Trivedi

I don't know whether 500 is better or 404. Either of this would be an improvement over the current behavior which redirects to GET (with slash appended) instead. Here is how our customers ran into this:

We have a route /sequence_items/ which supports both GET and POST requests but the contract for both is different. GET returns the existing sequence of items, whereas POST allows you to modify the sequence.

Our customer sent a POST request to /sequence_items (notice the missing slash). Their intention was to change the sequence, so they sent the correct request type, but slash was missed. Current behavior of Django, added the slash, but ALSO changed the request type to GET (by sending a 301/302 redirect instead of 307) which meant that the addition of slash silently changed the semantics of the API call. An error of some kind would have been better.

Hope this helps. BTW, I found the discussion here relevant:

Last edited 3 weeks ago by Nilesh Trivedi (previous) (diff)

comment:5 Changed 3 weeks ago by Carlton Gibson

I'd learn towards wontfix. I'd prefer to see a user land solution here before altering the built-in behaviour.

Some thoughts:

  • Disable APPEND_SLASH. This gives you your 404. It may not actually be desired behaviour.
  • As per the CommonMiddleware docstring, subclass CommonMiddleware and overriding the response_redirect_class attribute, returning the 307 response. (Whether that's wise I can't say.)
  • Otherwise subclassing CommonMiddleware to allow a more sophisticated (conditional) handling.
  • Adjust URL pattern to make trailing slash optional.
  • (... and so on...)
Last edited 3 weeks ago by Carlton Gibson (previous) (diff)

comment:6 Changed 2 weeks ago by Tim Graham

Resolution: wontfix
Status: newclosed

Sounds fine to me.

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