Re: Advice on #13260 - '%' in args to urlresolvers.reverse()

[Aaron Sokoloski]

On Tue, Aug 23, 2011 at 4:31 PM, Jonathan Slenders <
jonathan.slend...@gmail.com> wrote:

> Do you mean that when a URL is resolved, the view still gets quoted
> args/kwargs at the moment?
> If so, it feels to me we should change that. It does not really make
> much sense to retain the escape characters in a view's parameters.
>

Yes.  Looking at the code now it appears that yes, the args and kwargs are
still quoted when they get to the view.  I'd imagine that most of the time
there is nothing to unquote, as most user-specified data will be in query
params instead.  So, although changing this would be backwards-incompatible,
it would not affect a lot of users.  But I am not a big enough contributor
to Django to feel justified in recommending this kind of
backward-incompatible change.

And since I've looked at the docs for urllib.quote and unquote, I've
realised that by default they assume that you mean for any '/' characters to
be present in the final url, so if you want to encode those, you have to be
explicit.  I think the cleaner use pattern is to call quote on each
arg/kwarg for reverse(...), and to say that every reserved character should
be quoted.

Maybe my preferred solution, then, is to do that by default, but have a way
to turn off quoting for those rare cases when you don't want it.  Perhaps an
optional argument to reverse(...) and url(...).

Cheers,
Aaron

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Advice on #13260 - '%' in args to urlresolvers.reverse()

[Jonathan Slenders]

On 22 août, 10:02, Aaron Sokoloski  wrote:
> Hmm, that's a tricky issue.  If it is set in stone that url parameters are
> not unquoted automatically before they hit the view function, that implies
> that reverse should not quote automatically.

Do you mean that when a URL is resolved, the view still gets quoted
args/kwargs at the moment?
If so, it feels to me we should change that. It does not really make
much sense to retain the escape characters in a view's parameters.

The use of quote/unquote should be consistent everywere. No matter
whether these functions really change anything in the specific case
where it's used. If reverse does not quote anything, you should always
quote it yourself, like here:
>> "http://; + urllib.quote(reverse(...)) + "?param=value"
Even if reverse did not output any special character like '?' or '%'.
If wrapping reverse into quote is required in some cases, we should do
it everywhere before outputting the url into HTML.  Not being
consistent in escaping is how injection vulnerabilities happen...

I think in Django, that one should consider the result of "reverse" as
a final url path. Without query string and protocol off course. But
still not something where you should do any string operation (like
substitution) upon, before passing it to quote.

Someone could do the following, which would not work if reverse did
automatically quoting, but I consider this bad design.
>> reverse('my_page', args=['some-%i-url-slug']) % some_integer
Or this, which is a concatenation of a quoted and unquoted string.
(Which sounds like bad design to me but can be overcome by wrapping
the prefix in quote manually.)
>> "/prefix" + reverse('my_page')

We should also make a decision about whether the regex in the
urlpatterns are written quoted or unquoted.
I would say, keep these unquoted for readability.
>> url(r'^some-%-url/$', ...)

If we have the following patterns:
>> url(r'^(?Psome-%-url)/$', ...)
It should call the view with **{'param': 'some-%-url' }

Like Aaron, I can also see no reason for not quoting the reverse call.

Cheers,
Jonathan

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Advice on #13260 - '%' in args to urlresolvers.reverse()

[Thomas Guettler]

Am 22.08.2011 07:14, schrieb Ben Sturmfels:
> Hi Folks,
> 
> Could I get a tie-breaking opinion on whether this "quoting of
> parameters to reverse()" ticket #13260 is expected behaviour or not?
> 
> http://www.mail-archive.com/django-developers@googlegroups.com/msg25678.html
> 
> 
> The original message got two responses; one saying quote automatically,
> the other saying it's up to the user.

The ticket is in "design decision needed" since 14 months:
   https://code.djangoproject.com/ticket/13260

Who can make this decision?

  Thomas


-- 
Thomas Guettler, http://www.thomas-guettler.de/
E-Mail: guettli (*) thomas-guettler + de

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Advice on #13260 - '%' in args to urlresolvers.reverse()

[Aaron Sokoloski]

Hmm, that's a tricky issue.  If it is set in stone that url parameters are
not unquoted automatically before they hit the view function, that implies
that reverse should not quote automatically.

But maybe they should both be changed.  My opinion is that whatever we do,
we should do the same on both ends.  I can't think of a good reason not to
make reverse() quote automatically, and then unquote the parts before they
get to the view, but I suspect that that is likely to be due to a lack of
imagination on my part.

I apologise -- I did not break the tie with my opinion.

Cheers,
Aaron

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Advice on #13260 - '%' in args to urlresolvers.reverse()

[Ben Sturmfels]

Hi Folks,

Could I get a tie-breaking opinion on whether this "quoting of 
parameters to reverse()" ticket #13260 is expected behaviour or not?


http://www.mail-archive.com/django-developers@googlegroups.com/msg25678.html

The original message got two responses; one saying quote automatically, 
the other saying it's up to the user.


Cheers,
Ben

--
You received this message because you are subscribed to the Google Groups "Django 
developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Advice on #13260 - '%' in args to urlresolvers.reverse()

[Harro]

The thing is, quoted get variables will get unquoted, quoted url
parameters won't be unquoted automatically. That's up to the developer
to handle.

Ran into the same issue today, and for me it made sense to do the
quoting before passing it to the reverse function, but for me it was
an URL which was passed to another url which clearly needs escaping
(apache doesn't like double / by default :D)

I can however see how you might not know the input and thus forget to
quote it. The example above is a very good one for that.
You can't just start escaping everything without breaking code in
unexpected ways.


On Apr 22, 4:53 pm, Thomas Guettler  wrote:
>  reverse('special', args=[r'+\$*'])
> > '/special_chars/+%5C$*/'
>
> > It would have to output:
>
> > '/special_chars/%2B%5C%24%2A/'
>
> Hi,
>
> I think the current test case is broken. All args/kwargs should
> be quoted.
>
> --
> Thomas Guettler,http://www.thomas-guettler.de/
> E-Mail: guettli (*) thomas-guettler + de
>
> --
> You received this message because you are subscribed to the Google Groups 
> "Django developers" group.
> To post to this group, send email to django-develop...@googlegroups.com.
> To unsubscribe from this group, send email to 
> django-developers+unsubscr...@googlegroups.com.
> For more options, visit this group 
> athttp://groups.google.com/group/django-developers?hl=en.

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Advice on #13260 - '%' in args to urlresolvers.reverse()

[Thomas Guettler]

 reverse('special', args=[r'+\$*'])
> '/special_chars/+%5C$*/'
> 
> It would have to output:
> 
> '/special_chars/%2B%5C%24%2A/'

Hi,

I think the current test case is broken. All args/kwargs should
be quoted.


-- 
Thomas Guettler, http://www.thomas-guettler.de/
E-Mail: guettli (*) thomas-guettler + de

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Advice on #13260 - '%' in args to urlresolvers.reverse()

[Ben Sturmfels]

Hi,
I'm working on ticket #13260 and looking for some advice. The ticket is
about quoting of '%' in args and kwargs for urlresolvers.reverse().
Please ignore my initial patch as it's not quite right.

The reporter claims this is a bug:

>>> reverse('myapp.views.download', args=['100% completed.png']
"/download/100%%20completed.png"

and that output should be:

"/download/100%25%20completed.png"

My question is, is this expected behaviour, is a bug?

If expected behavior

Currently, reverse assumes that the args and kwargs are correctly quoted
for insertion into a URI. The iri_to_url() conversion does escape some
disallowed characters, like space, but doesn't modify reserved
delimiters like '#' --- it assumes you know what your are doing.

If bug
--
Alternatively, if this is considered a bug, then urlquote should be
applied to all items in args and kwargs for reverse. That would mean
that this test is wrong though:

>>> reverse('special', args=[r'+\$*'])
'/special_chars/+%5C$*/'

It would have to output:

'/special_chars/%2B%5C%24%2A/'

Cheers,
Ben


signature.asc
Description: This is a digitally signed message part