Code

Opened 6 years ago

Closed 2 years ago

#6892 closed Cleanup/optimization (wontfix)

Warn about raw "%" in redirect_to strings.

Reported by: daonb <bennydaon@…> Owned by: nobody
Component: Generic views Version: master
Severity: Normal Keywords: unicode, url
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

from my urls.py:

url(r'^password/change/done/$',
    django.views.generic.simple.redirect_to,
    {'url': '/toviva/myroom/?' + urlencode({'message': _('Password change successful')})})

When I access the url I get an error:

ValueError at /toviva/accounts/password/change/done/
unsupported format character 'D' (0x44) at index 25

printing the url parameters in redirect_to:

/toviva/myroom/?message=%D7%94%D7%A1%D7%99%D7%A1%D7%9E%D7%94+%D7%A9%D7%95%D7%A0%D7%AA%D7%94+%D7%91%D7%94%D7%A6%D7%9C%D7%97%D7%94

In redirect_to the line that fails is:

   return HttpResponsePermanentRedirect(url % kwargs)

I found another way to do it:

  url(r'^password/change/done/$',
    django.views.generic.simple.redirect_to,
    {'url': '/toviva/myroom/?message=%(message)s', 'message': urlquote_plus(_('Password change successful'))}),

Which works for me, but still - I'd be happy if someone can find a way to make redirect_to more robust.

Attachments (3)

6892v1.diff (1.3 KB) - added by daonb <bennydaon@…> 6 years ago.
adding a call to iri_to_uri in redirect_to
6892v2.diff (736 bytes) - added by daonb <bennydaon@…> 6 years ago.
A documentation patch
simple.diff (1.3 KB) - added by furryRevenge 3 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 6 years ago by PhiR

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

AFAIK urls aren't supposed to be in unicode. urlquote is the way to go.

comment:2 Changed 6 years ago by daonb <bennydaon@…>

  • Resolution invalid deleted
  • Status changed from closed to reopened

Seems like I wasn't clear enough, sorry. For the love of Django, I'll try once more:

According to urllib.qoute documentation: "Replace special characters in string using the "%xx" escape....Example: quote('/~connolly/') yields '/%7econnolly/'"

After urlquote you get an ascii string with "%xx" escape for every special or unicode character. Using the "%" operator on this string crashes because it has too many '%' characters, usually with unsupported format characters - such as 7 in the Python Library Reference example (redirect_to does it in "url % kwargs") .

Hope it makes sense, in any case I'm re-opening the ticket. Hope I'm not violating the protocol, just want to make sure someone reads this.

comment:3 Changed 6 years ago by ubernostrum

Changed 6 years ago by daonb <bennydaon@…>

adding a call to iri_to_uri in redirect_to

comment:4 Changed 6 years ago by daonb <bennydaon@…>

  • Has patch set
  • Summary changed from unicode support in redirect_to to IRI support in redirect_to

6892v1 changes redirect_to so it can accept an IRI in the url parameter. To keep it backward compatible, the url parameter name can not be changed. The patch is based on Django's unicode documentation (thanks ubernostrum):

>>> iri_to_uri(u'/favorites/François/%s' % urlquote(u'Paris & Orléans'))
'/favorites/Fran%C3%A7ois/Paris%20%26%20Orl%C3%A9ans'

comment:5 Changed 6 years ago by Simon Greenhill

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:6 Changed 6 years ago by mtredinnick

  • Has patch unset
  • Needs documentation set
  • Summary changed from IRI support in redirect_to to Warn about raw "%" in redirect_to strings.
  • Triage Stage changed from Ready for checkin to Accepted

This patch doesn't look like it's fixing the right problem. The problem is this: the url parameter passed to redirect_to() can contain dictionary-based format strings to accept parameters from the URL. In the example in the example in the description, the reporter is also including %-encoded characters as part of the URL. So we need to tell the difference between to uses of "%":

url = "%(param)s %7D"

The first one requires a substitution and the second should be left alone when we are subtituting the kwargs (and hence that "%" should really be "%%" in the string).

I suspect this is really a documentation issue. We could try to automatically detect the context of "%" signs in the url string, but that sounds like solving the problem at the wrong level. People using redirect_to() need to remember that they are passing in a format string and so any stray % signs need to be doubled (escaped as "%%"). Something like

my_string.replace("%", "%%")

So a documentation patch will be welcomed, but the current patch here isn't fixing the problem, since it looks like it doesn't solve the original error as described.

Changed 6 years ago by daonb <bennydaon@…>

A documentation patch

comment:7 Changed 6 years ago by daonb <bennydaon@…>

  • Has patch set

Changed 3 years ago by furryRevenge

comment:8 Changed 3 years ago by furryRevenge

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set

comment:9 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:10 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:11 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:12 Changed 2 years ago by claudep

  • Resolution set to wontfix
  • Status changed from reopened to closed

The documentation for the new class-based view RedirectView is now clear:
Because keyword interpolation is always done (even if no arguments are passed in), any "%" characters in the URL must be written as "%%" so that Python will convert them to a single percent sign on output.
https://docs.djangoproject.com/en/dev/ref/class-based-views/#django.views.generic.base.RedirectView

As for the use-case of the OP, I think that a .replace('%', '%%') should be appended to the urlencode() call.

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.