New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relative rediects per RFC7231 as the new default #749

Open
defnull opened this Issue Apr 2, 2015 · 9 comments

Comments

Projects
None yet
5 participants
@defnull
Member

defnull commented Apr 2, 2015

In #747 @polymorphm pojnted out that according to RFC7231 (which obsoletes RFC2616) relative URLs in the Location header are allowed.

Current behavior: Bottle tries to turn relative URLs in bottle.redirect() calls into absolute URLs by joining with the request.url string. See https://github.com/bottlepy/bottle/blob/master/bottle.py#L2445

New behavior: Bottle just passes the provided URI as is. No URL-joining will be performed.

This behavior change should have no effect on well behaved clients. Relative URLs in Location headers are supported by all client libraries I know of because it is a common (previously non-standard but now allowed) practice for years now.

It is still a behavior change with no obvious upgrade path. This is why I opened this issue. If you can think of a situation where this change breaks an existing application, or introduce a security risk, please speak up.

@defnull defnull added the Needs input label Apr 2, 2015

@shakna-israel

This comment has been minimized.

shakna-israel commented Apr 30, 2015

What effect would this have on cross-domain redirects?

@avelino

This comment has been minimized.

Member

avelino commented Apr 30, 2015

It had entered for the 0.12+ or just 0.13?

@polymorphm

This comment has been minimized.

Contributor

polymorphm commented Apr 30, 2015

What effect would this have on cross-domain redirects?

@shakna-israel , like this (?) --

bottle.redirect('//my-other-domain.com/my-page/123')

or something another?

@shakna-israel

This comment has been minimized.

shakna-israel commented Apr 30, 2015

@polymorphm, That sounds good. Sorry for weighing in, just developing a CORS app at the moment, and it tends to be the kind of thing that breaks when trying to match URI compliance.

@polymorphm

This comment has been minimized.

Contributor

polymorphm commented Apr 30, 2015

@shakna-israel , please , try check this changing in your CORS-apps

diff --git a/bottle.py b/bottle.py
index 535ddd3..359ceaf 100644
--- a/bottle.py
+++ b/bottle.py
@@ -2418,7 +2418,7 @@ def redirect(url, code=None):
     res = response.copy(cls=HTTPResponse)
     res.status = code
     res.body = ""
-    res.set_header('Location', urljoin(request.url, url))
+    res.set_header('Location', url)
     raise res

I think this check-result -- is will be important for all

@shakna-israel

This comment has been minimized.

shakna-israel commented May 2, 2015

@polymorphm, it seems to work absolutely fine.

@avelino

This comment has been minimized.

Member

avelino commented May 2, 2015

The solution proposed by @polymorphm meets our case.

@goodmami

This comment has been minimized.

Contributor

goodmami commented Aug 14, 2015

I think this should be linked to the following related issues: #748 and (the much older) #293

In my case, I have an application deployed on Apache with mod_wsgi at a subdirectory (e.g. example.com/myapp/). From my virtualhost config:

WSGIScriptAlias /myapp /var/www/myapp/app.wsgi
# ...

And in myapp.py:

@route('/<dirname>')
def bare_dir(dirname):
    redirect('/%s/' % dirname)

Even though the /<dirname> path matches example.com/myapp/abc, the browser is redirected to example.com/abc/. I need to use the following:

@route('/<dirname>')
def bare_dir(dirname):
    redirect('%s/' % dirname)  # no leading slash

I'm not an expert on web standards, so maybe this is the intended usage? But I expected a /url/ redirect to go to the application root and not the server root (as described in #293).

@defnull

This comment has been minimized.

Member

defnull commented Jun 9, 2017

The server/app root problem is a very common and annoying thing. Unfortunately, there is no reliable way to know about the 'application root' other than configuration. A proxy could redirect arbitrary urls to the application, rewriting the URL in any way desirable. There is SCRIPT_PATH but that has its own issues.

Currently bottle resolves relative URLs against something it thinks is the server root. It uses request.url which is reconstructed out of different CGI parameters and breaks easily for proxied applications. So, by removing the automatic urljoin(), we would not change behavior for most apps expect for broken proxy configurations, which is acceptable I think.

Magically prefixing redirect paths with SCRIPT_PATH or a configurable 'application root' would however break existing applications, so that is not an option. You could define your own patched redirect() function in two or three lines of code it you really want this behavior.

I tent do (finally) just pull this simple change, and if we find out later that the missing urljoin breaks reasonable existing applications, we could still make it configurable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment