Respect X-Forward-* headers and generate correct URLs in url_for
#1465
Conversation
|
I think if you pull the master branch as of this morning with the updated pytest, the build checks that are failing due to pytest version conflict will be resolved. |
Codecov Report
@@ Coverage Diff @@
## master #1465 +/- ##
==========================================
+ Coverage 91.59% 91.67% +0.07%
==========================================
Files 19 19
Lines 2082 2101 +19
Branches 390 393 +3
==========================================
+ Hits 1907 1926 +19
Misses 137 137
Partials 38 38
Continue to review full report at Codecov.
|
| 'X-Forwarded-Proto': 'https', | ||
| }) | ||
| assert app.url_for('view_name') == '/another_view' | ||
| assert app.url_for('view_name', _external=True) == 'http:///another_view' |
There was a problem hiding this comment.
That result is fishy. http:/// ?
There was a problem hiding this comment.
That's the missing UT case for Sanic.url_for. But there's no good way to fix it since getting a valid hostname needs Request context as in this PR.
There was a problem hiding this comment.
You can provide some kwargs to url_for, such as _server that can have your valid hostname embed into, like _server=request.headers.get("Host") or _scheme, like _scheme=request.headers.get("X-Forwarded-Proto"). You can check most of them here. I think we should properly document those kwargs from the url_for method.
There was a problem hiding this comment.
First: why is the url_for method mentioned in the PR title?
Furthermore, I think this might be a delicate question to handle. As @harshanarayana mentioned, some may have X-Url-Scheme while others just X-Scheme and I particularly don't like the way Sanic does this right now, to be fair.
I really like the aiohttp-remotes project, which gives you some sort of flexibility over this same issue (amongst others). We could lean toward this direction, at least when it comes to flexibility. Perhaps, instead of have all these header values hard-coded inside Sanic, make them somewhat configurable? This way, I can put for X-My-Special-Scheme if I want to 😉
|
Hi, @vltr @harshanarayana thanks for the comments. Regarding the header naming problem, There're only a limited number of headers used like de-facto standard, and we don't need to support fully customized headers like In this PR I'm mainly trying to make Sanic works with Nginx / HAProxy out of the box. The flexibility requirements could be addressed as another issue. Besides, there's also a standard Also, as meintioned by @andreymal in #801
It's surely a security risk using these headers, though in a good production setup it's not a issue since these headers would be filtered out or replaced by a proxy layer, but it still nice to have some configuration options to control this behavior. |
I understand you completely. There's a lot of other tools out there (such as AWS (ELB), Cloudflare, etc) that might have other headers as well and that's why I thought on making them somewhat configurable: we'll be swiping two problems at once. @huge-success/sanic-core-devs , your input would be mostly welcome on this. |
|
I think we do need to implement X-Forwarded-* - they're included both by common load balancers and for application routers like Traefik. I'm fine with making them configurable in the future, but I don't know that future features should be a blocker for this PR. |
|
@vltr @harshanarayana could you review again when you get a chance? |
|
@BananaWanted can you fix/rebase/rework this PR? I don't know what the Sanic maintainers think about it now, but I think this is an important change, but it seems that it conflicts with my PR #1539 |
|
@andreymal Speaking for myself, I agree that it is important. |
|
@yunstanford or @sjsadowski Can you run this as a "squash and merge"? |
Resolving these issues:
url_for#1380