Skip to content

Recommend Warning and Safer Defaults for url_for(..., _external=True) #5718

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.

Sign up for GitHub

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

Jump to bottom
Open
BrookeYangRui opened this issue Apr 14, 2025 · 5 comments · May be fixed by #5722
Open

Recommend Warning and Safer Defaults for url_for(..., _external=True) #5718

BrookeYangRui opened this issue Apr 14, 2025 · 5 comments · May be fixed by #5722
Labels
docs

Comments

@BrookeYangRui
Copy link

Hi Flask team,

We recently analyzed several Flask-based applications and noticed a recurring security concern related to url_for(..., _external=True) when used in untrusted request contexts. Specifically, since it uses request.host by default to construct the external URL, applications that do not explicitly configure SERVER_NAME or sanitize headers can be vulnerable to host header injection.

To improve developer awareness and reduce misuse, we suggest: Add a warning to the url_for documentation about the risk of relying on request.host, and recommend the use of SERVER_NAME or trusted_hosts when generating external URLs.

We’d be happy to help draft the relevant documentation or contribute a pull request if this direction aligns with the maintainers' goals.

Best regards,
Rui Yang and Zhengyu Liu
Johns Hopkins University
@pallets pallets deleted a comment from Yossafkarem Apr 21, 2025
@davidism
Copy link
Member

This is already documented in Deploying: https://flask.palletsprojects.com/en/stable/deploying/proxy_fix/

@davidism davidism closed this as not planned Won't fix, can't repro, duplicate, stale Apr 21, 2025
@ThiefMaster
Copy link
Member

ThiefMaster commented Apr 21, 2025 •
edited
Loading

FWIW this is only a problem when all these criteria are met:

  • the URL is used externally, and in a context where the recipient of the email may be someone else than the person making the request with the bogus Host header (password reset emails are particularly "interesting" there)
  • requests with an incorrect Host header actually reach the webapp (in most production deployments that's probably not the case) and are handled by it

@davidism I'm not sure if just mentioning ProxyFix is sufficient - you can have a simple deployment using nginx+uwsgi without any proxies/LBs involved, and depending on the nginx config you may be vulnerable.

@davidism
Copy link
Member

A section about SERVER_NAME and TRUSTED_HOSTS on the security docs page would probably be appropriate https://flask.palletsprojects.com/en/stable/web-security/. Happy to review a PR.

@davidism davidism reopened this Apr 21, 2025
@davidism davidism added the docs label Apr 21, 2025
@ranveer9
Copy link

Hi, I’m new to open source and would like to work on this issue. Can I take it?

ranveer9 added a commit to ranveer9/flask that referenced this issue Apr 27, 2025
@ranveer9
docs: add warning and best practices for url_for(..., _external=True) (…
676aa43 
…closes pallets#5718)
@ranveer9 ranveer9 linked a pull request Apr 27, 2025 that will close this issue
Open
@BrookeYangRui
Copy link
Author

Thanks for the contribution and this looks great! I appreciate the clean and thoughtful write-up.

Glad to see the issue addressed this thoroughly. Let me know if I can help further.

BlaytonV added a commit to BlaytonV/flask that referenced this issue Jun 3, 2025
@BlaytonV
Add section on host header injection and _external=True in url_for (p…
7205e1a 
…allets#5718)
@BlaytonV BlaytonV mentioned this issue Jun 3, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

docs: add warning and best practices for url_for(..., _external=True)… ranveer9/flask
4 participants
@ThiefMaster @davidism @ranveer9 @BrookeYangRui