Skip to content

Conversation

@alxndrsn
Copy link
Contributor

@alxndrsn alxndrsn commented Dec 29, 2025

Closes #1588

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

Adapted existing nginx tests to run with both:

  1. SSL_TYPE=selfsign (as happened previously), and
  2. SSL_TYPE=upstream (previously untested, as exposed in Update X-Forwarded-Proto directive in backend.conf #1586)

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

This PR adapts existing tests and runs them for SSL_TYPE=upstream. This is in addition to SSL_TYPE=selfsign, which was already being tested.

This approach has shown its worth by exposing another bug with SSL_TYPE=upstream, where /csp-report-related nginx config was being stripped by an overly-broad perl regex.

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?

Should reduce risk from nginx config changes for users using SSL_TYPE=upstream.

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

});
socket.on('end', resolve);
socket.on('error', reject);
}));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are specific to SSL_TYPE=selfsign, and have been moved above.

res.on('error', reject);

const body = new Readable({ _read: () => {} });
const body = new Readable({ read:() => {} });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bug, but only shows when the Readable is used with http.request, not with https.request.

});
socket.on('end', resolve);
socket.on('error', reject);
}));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are specific to SSL_TYPE=selfsign, and have been moved up from below.

@alxndrsn alxndrsn requested a review from lognaturel December 29, 2025 17:37
@alxndrsn alxndrsn marked this pull request as ready for review December 29, 2025 17:37
Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests @alxndrsn and for the fix @jniles!

@alxndrsn alxndrsn merged commit 242416c into getodk:next Dec 30, 2025
5 checks passed
@alxndrsn alxndrsn deleted the tests-for-upstream-sssl branch December 30, 2025 06:53
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