-
Notifications
You must be signed in to change notification settings - Fork 66
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
Fix: Django not detect that our site is HTTPS #282
base: development
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request fixes an issue where Django was not correctly detecting HTTPS connections due to TLS termination at the Nginx proxy. It introduces the Sequence diagram for HTTPS redirection with Nginx proxysequenceDiagram
actor User
participant Browser
participant Nginx
participant Django
User->>Browser: Clicks on a link
Browser->>Nginx: HTTPS Request to the site
Nginx->>Django: Forwards request with X-Forwarded-Proto header
Django->>Django: Reads SECURE_PROXY_SSL_HEADER setting
Django->>Django: Detects HTTPS based on header
Django-->>Nginx: HTTPS Response
Nginx-->>Browser: HTTPS Response
Browser-->>User: Renders page securely
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @hongquan - I've reviewed your changes - here's some feedback:
Overall Comments:
- It might be worth adding a comment explaining why
SECURE_PROXY_SSL_HEADER
is needed.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@sourcery-api The explanation comment is right on top of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look ok, but please do not mix two different things.
This PR now contains:
- the described fix to add
SECURE_PROXY_SSL_HEADER
- unrelated typing changes
Fixes #267
Add
SECURE_PROXY_SSL_HEADER
settings. This guides Django to look into Nginx header to detect that our website is in HTTPS. We need this settings because the TLS termination is done on Nginx, not on Django, so Django originally cannot know if our site is HTTP or HTTPS.How has this been tested?
This is the page where user see the link:
Before this PR, user will see this after clicking the link:
User no longer sees this page after this PR.
Checklist
Summary by Sourcery
Bug Fixes: