fix: add HTTPS/SSL configuration to Nginx template#979
Conversation
🚨 Missing Open Issue LinkThis pull request does not reference any open GitHub issue via the Development section. PRs must be linked to an open issue using GitHub’s "Development" section (on the right sidebar). How to fix this:
This PR will be automatically closed. You can reopen it after linking an open issue. Thank you! |
WalkthroughAdds an HTTP server (port 80) that serves ACME challenges and 308-redirects other requests to HTTPS; adds an HTTPS server (port 443) with conditional server_name, conditional Let's Encrypt vs snakeoil certs, TLSv1.2/1.3 and strong ciphers, plus static/media and proxied app (WebSocket) handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Nginx80 as Nginx (port 80)
participant Nginx443 as Nginx (port 443)
participant Certs as Certificates
participant App as Backend App
Client->>Nginx80: HTTP request
alt ACME challenge path
Nginx80->>Client: Serve ACME challenge
else Other paths
Nginx80->>Client: 308 Redirect to https://$host$request_uri
Client->>Nginx443: HTTPS request
Nginx443->>Certs: Select certs (Let's Encrypt if domain_name provided, else snakeoil)
Nginx443->>Client: TLS handshake + response
alt Static or media request
Nginx443->>Client: Serve /static/ or /media/
else App route (including WebSocket)
Nginx443->>App: Proxy request / upgrade websocket
App->>Nginx443: Response
Nginx443->>Client: Response
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds HTTPS support to the Ansible-managed Nginx config so the app can be served over TLS and HTTP traffic is redirected to HTTPS.
Changes:
- Redirect all HTTP (port 80) traffic to HTTPS while still allowing ACME challenge paths.
- Add an HTTPS (port 443) server block with TLS settings and proxying to the app.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible/nginx-http.conf.j2`:
- Around line 54-55: Add standard TLS hardening adjacent to the existing
ssl_protocols and ssl_ciphers settings: enable server cipher preference by
setting ssl_prefer_server_ciphers on; configure SSL session caching and timeout
via ssl_session_cache and ssl_session_timeout to improve performance; and add a
Strict-Transport-Security response header (add_header Strict-Transport-Security
...) with an appropriate max-age and includeSubDomains/preload options to
enforce HTTPS. Update the server block where ssl_protocols and ssl_ciphers are
declared (look for the ssl_protocols and ssl_ciphers symbols) to include these
directives and ensure they are applied to HTTPS listeners.
- Around line 52-53: The SSL directives ssl_certificate and ssl_certificate_key
currently interpolate {{ domain_name }} unguarded and will produce invalid paths
when domain_name is unset; update the ansible/nginx-http.conf.j2 template to
wrap those two directives in the same Jinja2 conditional used for server_name
(i.e., only emit ssl_certificate and ssl_certificate_key when domain_name is
defined/non-empty), or alternatively add an assertion in the Ansible playbook to
fail fast if domain_name is undefined (use an assert or fail task) so that
nginx-http.conf.j2 is only rendered when domain_name exists.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible/nginx-http.conf.j2`:
- Line 29: The server_name directive's conditional for adding a www alias is
inconsistent between the HTTP and HTTPS server blocks: update the template so
both server_name occurrences use the same check on domain_name (only append
"www." when domain_name.count('.') == 1 and not domain_name.startswith('www.'))
to ensure the www alias is added only for single-dot domains and not for
multi-level hostnames, preventing unintended TLS name coverage.
- Line 39: The redirect uses an untrusted $host in the line "return 308
https://$host$request_uri;", allowing host-header open redirects; change the
template in ansible/nginx-http.conf.j2 to use a configured canonical host when
available (e.g. domain_name) and only fall back to $host if not set—implement a
Jinja conditional around the existing "return 308 ..." so it emits "return 308
https://<configured canonical host>$request_uri" when domain_name is provided,
otherwise keep the original behavior.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ansible/nginx-http.conf.j2 (1)
39-39:⚠️ Potential issue | 🟠 MajorGuard the HTTPS redirect when
domain_nameis missing.Line 39 always interpolates
{{ domain_name }}. Ifdomain_nameis unset/empty, redirect generation becomes invalid (or template rendering can fail), breaking HTTP→HTTPS behavior.🔧 Proposed fix
location / { - return 308 https://{{ domain_name }}$request_uri; + {% if domain_name %} + return 308 https://{{ domain_name }}$request_uri; + {% else %} + return 308 https://$host$request_uri; + {% endif %} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible/nginx-http.conf.j2` at line 39, The HTTPS redirect always interpolates {{ domain_name }} in the return directive (the line with "return 308 https://{{ domain_name }}$request_uri;"), which will break if domain_name is unset; update the nginx-http.conf.j2 template to guard this: render the redirect only when domain_name is defined/non-empty, and otherwise either omit the redirect or fall back to a safe alternative such as using $host (e.g., generate "return 308 https://$host$request_uri;" when domain_name is missing); wrap the return directive in a Jinja conditional that checks domain_name before emitting the domain-based redirect or emits the safe fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ansible/nginx-http.conf.j2`:
- Line 39: The HTTPS redirect always interpolates {{ domain_name }} in the
return directive (the line with "return 308 https://{{ domain_name
}}$request_uri;"), which will break if domain_name is unset; update the
nginx-http.conf.j2 template to guard this: render the redirect only when
domain_name is defined/non-empty, and otherwise either omit the redirect or fall
back to a safe alternative such as using $host (e.g., generate "return 308
https://$host$request_uri;" when domain_name is missing); wrap the return
directive in a Jinja conditional that checks domain_name before emitting the
domain-based redirect or emits the safe fallback.
|
@coderabbitai can we remove this? location / { |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2457
However, I spotted a potential issue: The redirect hardcodes Suggested fix: location / {
{% if domain_name %}
return 308 https://{{ domain_name }}$request_uri;
{% else %}
return 308 https://$host$request_uri;
{% endif %}
}Or more simply, always use location / {
return 308 https://$host$request_uri;
}The 🧠 Learnings used |
|
@coderabbitai why 308? |
|
Tip For best results, initiate chat on the files or code changes.
Comparison:
This is especially important for:
If you used 301 and someone POSTed to TL;DR: 308 is the modern, RFC-compliant way to do permanent HTTPS redirects while preserving the original request method. 🔒 🧠 Learnings used |
Related issues
Fixes #977
Checklist
Summary by CodeRabbit