Skip to content
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

Don't accidently create relative links #1538

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
2 participants
@hefee
Copy link

commented May 11, 2019

append_slash_redirect should not change absolute links to relative ones.
That breaks serving applications via nginx.

e.g.
PATH_INFO="/help/test" -> the Location would be "help/test/" and this is translated correctly by nginx to:
https://example.com/help/help/test

Don't accidently create relative links
append_slash_redirect should not change absolute links to relative ones.
That breaks serving applications via nginx.

e.g.
PATH_INFO="/help/test"  -> the Location would be "help/test/" and this is translated correctly by nginx to:
https://example.com/help/help/test
@davidism

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Please add tests and a changelog.

@davidism

This comment has been minimized.

Copy link
Member

commented May 11, 2019

I'm not super clear what issue you're fixing based on your description. Fairly sure this is intentional, SCRIPT_NAME and PATH_INFO will be combined later in the response and will always create an absolute path.

@hefee

This comment has been minimized.

Copy link
Author

commented May 16, 2019

@davidism at least in my case this doesn't work.

see here the environ:

{'HTTP_ACCEPT': '*/*', 'werkzeug.request': <Request 'https://example.com/help/foo' [GET]>, 'wsgi.run_once': False, 'CONTENT_TYPE': '', 'wsgi.errors': <_io.TextIOWrapper name=2
 mode='w' encoding='UTF-8'>, 'PATH_INFO': '/help/foo', 'wsgi.url_scheme': 'https', 'CONTENT_LENGTH': '', 'REQUEST_URI': '/help/foo', 'SERVER_NAME': 'example.com',
 'uwsgi.node': b'xx.example.com', 'HTTP_ACCEPT_ENCODING': 'identity', 'SCRIPT_NAME': '/', 'HTTP_HOST': 'example.com', 'HTTP_USER_AGENT': 'Wget/1.20.1 (linux-gnu)', 'REQUEST_METHOD':
'GET', 'HTTP_AUTHORIZATION': 'Basic XXXXXXXXXXXXXX', 'SERVER_PORT': '443', 'QUERY_STRING': '', 'wsgi.multithread': False, 'wsgi.multiprocess': True, 'SERVER_PROTOCOL': 'HTTP/1.1', 'REMOTE_A
DDR': '1::1', 'uwsgi.version': b'2.0.14-debian', 'HTTP_CONNECTION': 'Keep-Alive', 'REMOTE_PORT': '43586', 'HTTPS': 'on', 'REQUEST_SCHEME': 'https', 'wsgi.input': <uws
gi._Input object at 0x7f92b539bb10>, 'wsgi.file_wrapper': <built-in function uwsgi_sendfile>, 'wsgi.version': (1, 0), 'DOCUMENT_ROOT': '/pathto'} 

So far I see SCRIPT_NAME is not anywhere used within the redirect function, that's why I'm wondering who is responsible to create absolute links?

My setup is nginx <-> uwsgi <-> flask/werkzeug

@davidism

This comment has been minimized.

Copy link
Member

commented May 16, 2019

redirect returns a Response. When Response generates headers, it adjusts the Location header to make it absolute.

# make sure the location header is an absolute URL
if location is not None:
old_location = location
if isinstance(location, text_type):
# Safe conversion is necessary here as we might redirect
# to a broken URI scheme (for instance itms-services).
location = iri_to_uri(location, safe_conversion=True)
if self.autocorrect_location_header:
current_url = get_current_url(environ, strip_querystring=True)
if isinstance(current_url, text_type):
current_url = iri_to_uri(current_url)
location = url_join(current_url, location)
if location != old_location:
headers["Location"] = location

Sandro Knauß

@hefee hefee force-pushed the netzguerilla:absolute-redirects branch from d731ab5 to ca99479 May 16, 2019

@hefee

This comment has been minimized.

Copy link
Author

commented May 16, 2019

redirect returns a Response. When Response generates headers, it adjusts the Location header to make it absolute.

# make sure the location header is an absolute URL
if location is not None:
old_location = location
if isinstance(location, text_type):
# Safe conversion is necessary here as we might redirect
# to a broken URI scheme (for instance itms-services).
location = iri_to_uri(location, safe_conversion=True)
if self.autocorrect_location_header:
current_url = get_current_url(environ, strip_querystring=True)
if isinstance(current_url, text_type):
current_url = iri_to_uri(current_url)
location = url_join(current_url, location)
if location != old_location:
headers["Location"] = location

okay than it is wekrzeug, who creates the absolute url wrongly ;D

See line 607:

location = url_join(current_url, location)

current_url = "https://example.com/help/foo"
location = "help/foo"

-> location = "https://example.com/help/help/foo"

IMO current_url is wrong to use there, instead SCRIPT_NAME needs to been taking into account.

@davidism

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

The current behavior is correct. In WSGI, paths are typically relative to the script_root, which is defined by the base_url in the test client. append_slash_redirect is within the context of the current environ.

@davidism davidism closed this Jul 13, 2019

@hefee

This comment has been minimized.

Copy link
Author

commented Jul 13, 2019

@davidism: can you point me to some documentation for this?

I still think absolute links needs to handled different than relative ones. As in the case I need this fix 'SCRIPT_NAME' is '/' and 'PATH_INFO' is '/help/foo'. And I need to create a link to '/nonhelp'. With the current implementation I never can create links to '/nonhelp', as I always end up in '/help/nonhelp'. But '/nonhelp is still in namespace of SCRIPT_NAME and that means it is a valid PATH_INFO to request for.
Unfortunately I don't understand how SCRIPT_NAME and PATH_INFO are translated into base_url.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.