Opened 3 years ago

Closed 3 years ago

#5236 enhancement closed fixed (fixed)

t.web.util.redirectTo should reject unicode URLs

Reported by: ivank Owned by: exarkun
Priority: low Milestone:
Component: web Keywords: easy
Cc: jknight, moijes12@… Branch:
Author: Launchpad Bug:

Description

If you have something like this in your render:

return redirectTo(url, request)

and you pass a unicode object as the url, you get a rather mysterious error page from twisted.web (with the redirection headers set!):

"500 - Request did not return a string"

redirectTo should really just throw TypeError if you pass in a unicode object.

Attachments (1)

my-twisted-patch.patch (1.2 KB) - added by moijes12 3 years ago.
Check added to function twisted.web.util.redirectTo so that it raises TypeError if Unicode object passed in URL. Unit test file test_util is also updated with unit test.Comments and docstring are added and test has passed.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc jknight added

Changed 3 years ago by moijes12

Check added to function twisted.web.util.redirectTo so that it raises TypeError if Unicode object passed in URL. Unit test file test_util is also updated with unit test.Comments and docstring are added and test has passed.

comment:2 Changed 3 years ago by moijes12

  • Cc moijes12@… added
  • Keywords review added

comment:3 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to exarkun

Thanks! This looks pretty good. A few simple points:

  1. We generally don't annotate changes with a reference to the ticket they're for. svn blame does an okay job of pointing out why certain code exists, when someone wants to know.
  2. redirectTo should have had a docstring already; since it didn't, and it's being changed, it should really be given one now. :)
  3. There should also be a news fragment describing the change. This seems worthy of a feature fragment, since it is improving the behavior of a public API.

These are pretty minor points, so I'm happy to make the changes myself and then apply the result. Thanks again!

comment:4 Changed 3 years ago by moijes12

Is there anything more that I should do here ?

comment:5 Changed 3 years ago by exarkun

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

(In [33400]) Reject unicode locations early in twisted.web.util.redirectTo

Author: moijes12
Reviewer: exarkun
Fixes: #5236

Raise TypeError from redirectTo if the supplied URL is unicode instead of
str. This replaces the old behavior of returning a unicode string in this
case which would later cause a 500 response when lower-level parts of Twisted Web
rejected the non-str response body string.

Note: See TracTickets for help on using tickets.