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

url_for only returns URLs with an autrority component #843

Open
sm-Fifteen opened this issue Feb 21, 2020 · 5 comments
Open

url_for only returns URLs with an autrority component #843

sm-Fifteen opened this issue Feb 21, 2020 · 5 comments

Comments

@sm-Fifteen
Copy link

sm-Fifteen commented Feb 21, 2020

(Re-raising from #604, since that issue was only partially fixed)

TL;DR: url_for should behave like it does in Flask and render absolute paths by default (unless an optional authority parameter is passed), not complete URLs.


Unlike Flask's url_for and Django's HttpRequest.build_absolute_uri(location), Starlette's Request.url_for always returns a complete URL with authority component. This tends to be unneccessarry when rendering templates that link to internal pages, but the important problem is that it also makes it so the app cannot render valid URLs unless it is aware of the authority the client is using to connect to it. Starlette uses the Host header to determine what authority to prepend to the generated URLs, which is a routing header and is spec-mandated to be overriden by any intermediate clients so that the application is only aware of what hostname the final request was directed towards. This means url_for is always going to break when Starlette is behind a proxy. The port and server values in the ASGI scope cannot differ from the Host header either, as they are meant to mirror what WSGI and CGI do.

Those are all meant to let the application reconstruct the URL of the request, but they can't be relied upon to build externally-valid URLs. Some proxies and application servers will send the X-Forwarded-Host or Forward headers to let the application know what authority the user agent was connecting from, but even the spec defining this header says

Due to the sensitive nature of the data passed in [Forwarded] (see Sections 8.2 and 8.3), this header field should be turned off by default.

Gunicorn removed the use of proxy headers several years ago when they realized overriding REMOTE_ADDR with the values from other headers was against the WSGI and CGI spec, because an application shouldn't have that sort of responsibility.

You simply can't dynamically generate externally valid URLs with an authority component. You either have to know the authority you want to use in advance from a config file (usually for internal redirection, like pointing towards a CDN) or just omit them and let the user agent understand that it is meant to be sent to the same server it got the rendered URL from.

Important

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@tomchristie
Copy link
Member

tomchristie commented Feb 24, 2020

I don't quite agree with the assessment that it's not possible to determine the incoming host - proper setup of proxies & uvicorn should relay that information correctly.

As an example Django REST framework uses absolute URLs if it's been configured to generate hyperlinked APIs, and we don't have a stack of backlogged issues with the behavior there.

However I do agree that it'd be more resiliant to just generate URL paths as the default case. Potentially a bit of a breaking change so may need some thinking about.

@jacopofar
Copy link

This is quite critical when setting up OAuth, since the third party checks the callback URL and complains if it's not a whitelisted one. On Flask I solve this by passing _external=True to url_for, maybe it could be useful to have something like that in Starlette as well.

What I'm doing now to fix the protocol is

redirect_uri = request.url_for('auth')
if 'X-Forwarded-Proto' in request.headers:
  redirect_uri = redirect_uri.replace('http:', request.headers['X-Forwarded-Proto'] + ':')

@anentropic
Copy link

I think I have this problem too

I'm deploying an app to fly.io, I get an https://<myapp>.fly.dev url. But the static files are not displaying and it seems to be because the templates use:

<link rel="stylesheet" href="{{ url_for('admin:statics', path='css/main.css') }}">

...which results in urls like http://<myapp>.fly.dev/statics/css/main.css which fail.

It's bizarre to have the url scheme rewritten from https -> http

I saw in #538 that the recommended fix is to specify both --proxy-headers and --forwarded-allow-ips flags to uvicorn

But it seems this requires one to know the IP address of the proxy in advance in order to trust it. I can't see how to do that on fly.io

Alternatively --forwarded-allow-ips (or the uvicorn ProxyHeadersMiddleware) accept "*" wildcard to trust all X-Forwarded-For headers

I guess I will do that for this low risk demo app, but it feels like something that may allow for exploits (presumably a malicious client could spoof the hostname and inject their own js file of same name into my site?)

I'd much prefer either:

  • path-only urls
  • specify the https://<myapp>.fly.dev hostname of my site manually via Starlette app config - this is a value I can know ahead of time

are either of these possible at the moment?

@anentropic
Copy link

I have gone with --proxy-headers --forwarded-allow-ips=* args to uvicorn and can confirm that it does work at least

@fzumstein
Copy link

Why not introduce an additional path_for template function that returns the URL path instead of an absolute URL? That would be consistent with request.path and wouldn't cause any backward incompatibilities.

Anyhow, for now, I am just overriding url_for with app.url_path_for like this:

templates.env.globals["url_for"] = app.url_path_for

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants