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 HTTPS proxy handling #809

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

alga
Copy link

@alga alga commented Jan 22, 2024

When HTTPS requests were made through a proxy, the proxy host name and port ended up in the cassette URLs.

In this PR, I extend the Proxy fixture to handle the CONNECT method, reproduce the bug and fix it.

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3a5ff1c) 92.32% compared to head (53faa64) 92.34%.
Report is 1 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #809      +/-   ##
==========================================
+ Coverage   92.32%   92.34%   +0.01%     
==========================================
  Files          27       27              
  Lines        1811     1815       +4     
  Branches      338      339       +1     
==========================================
+ Hits         1672     1676       +4     
  Misses         92       92              
  Partials       47       47              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alga alga force-pushed the alga-https-proxy branch from d1719bd to 53faa64 Compare January 23, 2024 14:54
@graingert
Copy link
Collaborator

Previously you set the host of the proxy server to an explicit 127.0.0.x IP. Is that needed?

@alga
Copy link
Author

alga commented Jan 24, 2024

Previously you set the host of the proxy server to an explicit 127.0.0.x IP. Is that needed?

Not strictly speaking, just the proxy having a different IP than httpbin was convenient for debugging and made the test a bit more precise. If the URLs vary just by the port number, there can conceivably be a regression uncaught by tests if the host selection logic breaks.

I reverted it to minimize the random changes in the diff when resolving a conflict. Do you think it's worth putting it back?

@alga
Copy link
Author

alga commented Mar 13, 2024

Ping @graingert, so is this good to go?

@alga
Copy link
Author

alga commented Jul 7, 2024

Hello, guys, this is a genuine bugfix with a repro.

@agroszer
Copy link

please merge this

@agroszer
Copy link

@graingert any chance to merge this before the next release?

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.

4 participants