Hello Anthony:

In message <225309719.20170912121932@inbox.ru>,
Anthony writes:
>Anthony added the comment:
>> What is your use case where this must be a relative url?
>
>Just something like
><input type="hidden" name="__redirect_to" value="issue">
>
> I think this  variant  has  a much better readability.

I agree with that.

>Also I see no reason to constraint URL to be absolute inside
>tracker's html. Am I wrong?

If there is an xss attack the __redirect_to can be sent to anywhere. E.G.

  https:/mybadsite.com/tracker/I/will/steal/your/credentials

I.E. the __redirect_to is totally under the control of the attacker
and can be used to redirect the victim to any site. Given the number
of ways to encode/mangle the url (puny code, url encoding...) passed
in __redirect_to, I require an exact match to the base url for the
tracker with no url encoding or other representations. However I am
not sure the code as written is enough.

>> That's correct and intentional. The __redirect_to (and _came_from)
>> parameter is accessible to the user or to any nasty malware in the
>> browser.
>>>From the comment right above that block:
>>        To try to prevent XSS attacks, validate that the url that is
>>        passed in is under self.base for the tracker. This is used to
>>        clean up "__came_from" and "__redirect_to" form variables used
>>        by the LoginAction and NewItemAction actions.
>
>I  think  the  relative  url  that  is passed in is under self.base by
>definition. Is it true?

AFAIK the relative url is "appended" to whatever the page declares as
a base url when the relative url is used *in the page*.

In this case there is no page around the relative url. It is used as
part of a 302/301 redirect. So I am not quite sure what happens in
this case.  Hence I required absolute fully qualified url's.

As a result I think your code to handle relative urls needs to
generate a fully specified url. At the very least the scheme:
http/https should be specified to prevent malicious code in the
browser from downgrading https to http.

Making sure that the url is sanitized and valid is tricky and there
have been multiple issues with that in other code bases.

I see where you are decoding the url to make sure that %2F doesn't
sneak through. That's good. I am just not sure that is sufficient.

>As you see in patch I pass URL base checking only in case when it is a
>relative URL.

Bern, Ralf comments on this? I don't want to introduce a new security
hole like I did when I originally coded the feature that required this
sanitizing patch.

I have a bad feeling that my code isn't a good enough sanitation as
it stands and could be exploited. Adding a new use case (relative
url's) which could also be exploited is not the direction I want to go.

However to address one of Anthony's concerns maybe a
python:make_absolute_url('url'[, 'query']) helper function that is the
equivalent of:

    tal:attributes= "value
       string:${request/base}${request/env/PATH_INFO}?${request/env/QUERY_STRING}"

might make things more readable. I agree the whole string/tal above is
mouthful and not exactly obvious.

Also if we do handle relative url's in __redirect_to and __came_from,
I think we must always return absolute URL's in the redirect by
prepending the request base and any path info when we raise the
redirect exception.

Comments?