Skip to content
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

Pin redir_protocols #169

Merged
merged 22 commits into from
Sep 16, 2024
Merged

Pin redir_protocols #169

merged 22 commits into from
Sep 16, 2024

Conversation

yoannchaudet
Copy link
Contributor

@yoannchaudet yoannchaudet commented Sep 9, 2024

This issue addresses https://github.com/github/pages-engineering/issues/5244.

Limit the redirection protocols we follow to HTTP/HTTPS only.

I did not find a way to test that with the mocking lib in the way. The redirect_count method also does not exactly returned what we needed. When you cannot fake it, just do it...

So to test the proper behaviors, we start a few real TCP servers in parallel to running the tests. We can confirm we get the behavior we need based on a log file written by the TCP servers. It is a bit clunky but does validate the behavior we are getting is the one we expect.

@yoannchaudet yoannchaudet requested a review from a team as a code owner September 9, 2024 21:10
Copy link
Contributor

@parkr parkr left a comment

Choose a reason for hiding this comment

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

A test would be ideal if there's behavior you're trying to avoid and want to prevent regressions but LGTM 😀

@yoannchaudet
Copy link
Contributor Author

yoannchaudet commented Sep 10, 2024

That one will be annoying to test but I'll try to come up with something. I am getting the changes themselves vetted first ;) Switching to draft to clarify that state.

@yoannchaudet yoannchaudet marked this pull request as draft September 10, 2024 16:11
@yoannchaudet yoannchaudet marked this pull request as ready for review September 13, 2024 21:07
@yoannchaudet yoannchaudet changed the title Try to pin redir_protocols Pin redir_protocols Sep 13, 2024
script/cibuild-docker Outdated Show resolved Hide resolved
script/cibuild-docker Outdated Show resolved Hide resolved
script/cibuild Outdated Show resolved Hide resolved
@yoannchaudet yoannchaudet merged commit f44d1b8 into master Sep 16, 2024
5 of 6 checks passed
@yoannchaudet yoannchaudet deleted the protocol-pinning branch September 16, 2024 19:57
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