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

Merged
merged 4 commits into from
Jan 7, 2025
Merged

Conversation

alga
Copy link
Contributor

@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.

@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
Contributor 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
Contributor Author

alga commented Mar 13, 2024

Ping @graingert, so is this good to go?

@alga
Copy link
Contributor 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?

@agroszer
Copy link

agroszer commented Jan 3, 2025

@graingert please merge and release

@agroszer
Copy link

agroszer commented Jan 3, 2025

or @kevin1024
thank you

@jairhenrique
Copy link
Collaborator

@hartwork can you help to review this!?

Copy link
Collaborator

@hartwork hartwork left a comment

Choose a reason for hiding this comment

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

@hartwork can you help to review this!?

@jairhenrique I have some open questions about proxying in general, but I had a look now and played with it a bit and what I found that should likely be changed — please see below — is non-critical; looks like good work in general — approving.

CC @alga

tests/integration/test_proxy.py Outdated Show resolved Hide resolved
tests/integration/test_proxy.py Outdated Show resolved Hide resolved
tests/integration/test_proxy.py Outdated Show resolved Hide resolved
@alga alga force-pushed the alga-https-proxy branch from 53faa64 to 3ddff27 Compare January 7, 2025 19:48
@hartwork hartwork merged commit 440bc20 into kevin1024:master Jan 7, 2025
13 checks passed
@agroszer
Copy link

agroszer commented Jan 8, 2025

thank you
@kevin1024 any chance for a release please?

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.

6 participants