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

fix: process http over https proxy correctly #129

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

barjin
Copy link
Contributor

@barjin barjin commented Jan 31, 2024

Previously, HTTP request (http://example.com/resource) over HTTPS proxy (https://proxy.com) was sent as:

  • HTTP GET -> https://proxy.com/example.com/resource.
    • this causes errors, as we're sending an HTTP request to an HTTPS proxy.

After a discussion with @jirimoravcik , we figured that the "pathname" proxies are a marginal thing (CONNECT is more popular nowadays).

This PR fixes that - got-scraping now does:

  • HTTPS CONNECT -> https://proxy.com/ -> HTTP GET http://example.com/resource.

Yet, there is still this comment:
// Upstream proxies hang up connections on CONNECT + unsecure HTTP
maybe @szmarczak knows what that was supposed to mean? 😅 seems dangerously close to what we want to do now.


Fixes #126

@barjin barjin added bug Something isn't working. t-tooling Issues with this label are in the ownership of the tooling team. labels Jan 31, 2024
@barjin barjin requested a review from vladfrangu January 31, 2024 17:35
@barjin barjin self-assigned this Jan 31, 2024
@barjin
Copy link
Contributor Author

barjin commented Feb 16, 2024

Huh, so the description of this PR mentions the existence of "no-CONNECT" proxies. I'm not sure whether there is an easy way of determining whether a proxy supports the CONNECT method (an easy way other than a preflight CONNECT request).

@barjin barjin force-pushed the fix/http-over-https-proxy branch from f3db15a to 078084d Compare March 25, 2024 13:19
@github-actions github-actions bot added this to the 86th sprint - Tooling team milestone Mar 25, 2024
@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Mar 25, 2024
@barjin
Copy link
Contributor Author

barjin commented Mar 25, 2024

As discussed w/ @B4nan, we'll go through with this - there is no repro to be found for the "no-CONNECT" proxies - worst case, we can always revert (and we'll have the actual repro by then :))

@barjin barjin merged commit ed636cb into master Mar 25, 2024
8 checks passed
@barjin barjin deleted the fix/http-over-https-proxy branch March 25, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP traffic over HTTPS proxy
1 participant