Opened 11 months ago
Closed 8 months ago
#36000 closed Cleanup/optimization (fixed)
Update default from http to https in urlize when protocol not provided
| Reported by: | Saravana | Owned by: | Ahmed Nassar |
|---|---|---|---|
| Component: | Template system | Version: | 5.1 |
| Severity: | Normal | Keywords: | |
| Cc: | Saravana | Triage Stage: | Ready for checkin |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
In django/utils/html.py, urlize there is:
url = smart_urlquote("http://%s" % html.unescape(middle))
When user input does not include a protocol it defaults to http (Insecure Protocol).
Example :
Considered a web app using urlize() for password reset email template
input = "Password reset link myapp.com/password/reset/{token}"
output:
"Password reset link <a href="http://myapp.com/password/reset/{token}"/>"
so when end user of myapp clicks it the url with token sent in http insecure protocol.
This behavior could potentially lead to man-in-the-middle attacks
Suggested Fix:
Default to HTTPS: If the URL doesn't specify a protocol, Django could default to https://
Change History (22)
comment:1 by , 11 months ago
| Description: | modified (diff) |
|---|---|
| Has patch: | unset |
| Summary: | Insecure URL Handling (HTTP Protocol Default) in urlize → Update default from http to https in urlize when protocol not provided |
| Triage Stage: | Unreviewed → Accepted |
comment:2 by , 11 months ago
| Component: | HTTP handling → Template system |
|---|
comment:3 by , 11 months ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
comment:4 by , 10 months ago
I think we can use a plan similar to how #34380 shook out, with a plan like:
- Introduce a transitional setting (
URLIZE_ASSUME_HTTPS) that defaults toFalse. This goes on the deprecation plan for removal in N+2 versions. - When the responsible code path is hit (which should be fairly rare as it only applies to limited domains), check the setting. If it’s
False, warn and use 'http', otherwise use 'https'.
comment:8 by , 8 months ago
comment:9 by , 8 months ago
| Owner: | changed from to |
|---|
comment:10 by , 8 months ago
I've updated the PR with improved documentation and fixed all sphinx-build warnings:
Changes made:
- Added detailed documentation for urlize() function in docs/ref/utils.txt
- Fixed sphinx-build warnings related to function references
- Added proper parameter descriptions following Django's style
- Included a practical example showing function usage
- Followed Django's documentation guidelines
- All documentation now builds without warnings
Documentation improvements:
- Added :param: descriptions for all parameters
- Added clear return value description
- Added working code example
- Fixed cross-references between template filter and utility function
- Maintained consistent documentation style
The PR now includes:
- URLIZE_ASSUME_HTTPS setting implementation
- Deprecation warning for http:// default
- Complete test coverage
- Comprehensive documentation
- Release notes
PR: https://github.com/django/django/pull/19240
All checks are passing and the documentation builds cleanly. Ready for review.
comment:11 by , 8 months ago
| Has patch: | set |
|---|
comment:12 by , 8 months ago
| Patch needs improvement: | set |
|---|
comment:13 by , 8 months ago
I have completed all requested changes in the PR https://github.com/django/django/pull/19240, including:
Addressing all review comments.
Pushing a commit with the latest updates.
Ensuring all tests pass successfully.
Updating the documentation to reflect the introduced URLIZE_ASSUME_HTTPS setting.
Additionally, I have added and updated some tests and documentation to ensure proper coverage and clarity. Given that the Trac ticket currently has "Needs tests: No" and "Needs documentation: No", should these be updated to "Needs tests: Yes" and "Needs documentation: Yes" to reflect these additions?
comment:14 by , 8 months ago
Additionally, I have added and updated some tests and documentation to ensure proper coverage and clarity. Given that the Trac ticket currently has "Needs tests: No" and "Needs documentation: No", should these be updated to "Needs tests: Yes" and "Needs documentation: Yes" to reflect these additions?
Yes when you want another review, you should do this as it will put the ticket in the review queue. I have given a review, so once you've resolved those comments you can update this ticket
comment:15 by , 8 months ago
Thank you for the review and clarification.
- Addressed and resolved all comments in the PR https://github.com/django/django/pull/19240
- Ensured passing all tests and documentation checks correctly.
Looking forward to your review and feedback!
comment:16 by , 8 months ago
| Needs documentation: | set |
|---|---|
| Needs tests: | set |
comment:17 by , 8 months ago
| Needs documentation: | unset |
|---|---|
| Needs tests: | unset |
comment:18 by , 8 months ago
| Patch needs improvement: | unset |
|---|
comment:19 by , 8 months ago
| Patch needs improvement: | set |
|---|
comment:20 by , 8 months ago
| Patch needs improvement: | unset |
|---|
comment:21 by , 8 months ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Thank you!
Note that the security team discussed this and agreed this can be handled publicly. This is similar to #34380.