Skip to content

Conversation

@jniles
Copy link

@jniles jniles commented Dec 27, 2025

This PR fixes a regression in behavior reported on the ODK forum here. The regression was introduced in 924d320 during a code refactor. The code refactor moved the X-Forwarded-Proto header to a new file but did not update the regexp which pins this header to https in setup-odk.sh script to point to that file. The result is that HTTP authentication breaks for users hosting ODK behind a reverse proxy.

What has been done to verify that this works as intended?

I have made the changes on my local installation, rerun the docker compose build, and confirmed that the issues reported in this forum post are resolved. I can log into my server without throwing the httpsOnly() error.

Why is this the best possible solution? Were any other approaches considered?

I believe this is a minor regression introduced during code refactoring in error. This emulates the behavior of the build script before 924d320.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This fixes a regression in behavior, and should not introduce more breakages.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • branched off and targeted the next branch OR only changed documentation/infrastructure (master is stable and used in production)
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

Updates the nginx odk setup script to point to the correct nginx configuration file when pinning `X-Forwarded-Proto` to 'https'.  Fixes a minor regression introduced during refactoring in 924d320.

For more discussion, see https://forum.getodk.org/t/setting-up-central-behind-a-proxy-this-authentication-method-is-only-available-over-https/57236/10.
@lognaturel lognaturel requested a review from alxndrsn December 29, 2025 15:13
@alxndrsn
Copy link
Contributor

Thanks for the fix!

I believe this is a minor regression introduced during code refactoring in error.

I'd say it's quite a big regression for anyone using SSL_TYPE=upstream!

Copy link
Contributor

@alxndrsn alxndrsn left a comment

Choose a reason for hiding this comment

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

Tested locally, and looks good.

I'll follow up with relevant tests.

@alxndrsn alxndrsn merged commit 3cd6f02 into getodk:next Dec 29, 2025
9 of 10 checks passed
@alxndrsn
Copy link
Contributor

@jniles I've added tests relating to SSL_TYPE=upstream at #1589, and fixed another minor bug with that setup (relating to Sentry CSP reporting - it should not have been impacting your users).

@jniles jniles deleted the patch-1 branch December 29, 2025 17:40
@jniles
Copy link
Author

jniles commented Dec 29, 2025

Thanks @alxndrsn ! Fixes look good to me and I appreciate all your work on this.

alxndrsn added a commit that referenced this pull request Dec 30, 2025
Adapt existing nginx tests to run with both:

1. `SSL_TYPE=selfsign` (as happened previously), and
2. `SSL_TYPE=upstream` (previously untested, as exposed in #1586)
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.

2 participants