Opened 6 years ago

Closed 6 years 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

Description

https://docs.djangoproject.com/en/dev/_modules/django/middleware/common/#CommonMiddleware

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 by Tim Graham, 6 years ago

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 by Claude Paroz, 6 years ago

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

comment:3 by Tim Graham, 6 years ago

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 by Nilesh Trivedi, 6 years ago

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: https://softwareengineering.stackexchange.com/questions/99894/why-doesnt-http-have-post-redirect

Last edited 6 years ago by Nilesh Trivedi (previous) (diff)

comment:5 by Carlton Gibson, 6 years ago

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...)
Version 0, edited 6 years ago by Carlton Gibson (next)

comment:6 by Tim Graham, 6 years ago

Resolution: wontfix
Status: newclosed

Sounds fine to me.

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