Skip to content

Fix for setting the correct forward proto when behind ssl proxy #2043

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

Merged
merged 2 commits into from
Jul 4, 2025

Conversation

starkmapper
Copy link
Contributor

@starkmapper starkmapper commented Jul 1, 2025

Description
Fixes #1737

Feeds are generated using the "Forward protocol".
This was set to "http" by default in the nginx config, regardless if it is set or not.

This change will set it to https, if that's what it was set to already, or to $scheme if it's unset.

Checklist
Please check all that apply.

  • I've tested the changes locally
  • I've updated relevant comments
  • I've assigned reviewers for this PR - Working on it...
  • [ ] I've added unit tests that cover the changes Not applicable in my opinion.

@gantoine gantoine requested review from adamantike and Copilot and removed request for adamantike July 1, 2025 17:16
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR ensures that the X-Forwarded-Proto header reflects the original request scheme when behind an SSL proxy by introducing a conditional mapping.

  • Adds an Nginx map block to derive the correct forward protocol ($forwardscheme).
  • Updates the proxy_set_header X-Forwarded-Proto line to use the new $forwardscheme variable.
Comments suppressed due to low confidence (2)

docker/nginx/templates/default.conf.template:19

  • Consider adding tests (e.g., integration or config linting checks) to verify that X-Forwarded-Proto is correctly set to $forwardscheme for both HTTPS and default ($scheme) scenarios.
    proxy_set_header X-Forwarded-Proto $forwardscheme;

docker/nginx/templates/default.conf.template:6

  • [nitpick] Consider renaming $forwardscheme to $forwarded_scheme (snake_case) for improved readability and consistency with common naming conventions.
map $http_x_forwarded_proto $forwardscheme {

@@ -2,6 +2,12 @@
# by using `envsubst` to replace the environment variables in the template with
# their actual values.

# Helper to get scheme regardless if we are behind a prox or not
Copy link
Preview

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typo in the comment: prox should be proxy to correct the spelling.

Suggested change
# Helper to get scheme regardless if we are behind a prox or not
# Helper to get scheme regardless if we are behind a proxy or not

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Contributor

@adamantike adamantike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for your contribution!

@gantoine gantoine merged commit 8908d21 into rommapp:master Jul 4, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants