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.

Sign up for GitHub

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

Jump to bottom

Fix the proxy_bypass_registry function all returning true in some cases. #6302

Merged
merged 2 commits into from
Mar 18, 2024
Merged

Fix the proxy_bypass_registry function all returning true in some cases. #6302

merged 2 commits into from
Mar 18, 2024

Conversation

flysee
Copy link
Contributor

@flysee flysee commented Dec 7, 2022

With the context code,i think we should avoid this situation with empty strings,thx!

@sigmavirus24
Copy link
Contributor

There's no clear description of a bug, how to reproduce or even tests to show this fixes something. Further you speak of empty strings and are filtering on None which are not the same thing.

@flysee
Copy link
Contributor Author

flysee commented Dec 8, 2022 •
edited
Loading

The value of proxyOverride is controlled by a third-party program on the computer, so it is not controllable.
For example, when I use fiddler on my computer, this value is <-loopback>;.(Not all computers)
When proxyOverride splits, it's equal to ['<-loopback>', ''].
So re.match('', host, re.I) will return true,that bypassing the proxy.

And filter(function, iterable) is equivalent to the generator expression (item for item in iterable if item) if function is None,that is, all elements of iterable that are false are removed.
Ref:https://docs.python.org/3/library/functions.html#filter

I think should be avoided for code robustness.

Ousret added a commit to jawah/niquests that referenced this pull request Oct 19, 2023
@Ousret
🔖 Release 3.1.3
f05f1c5 
**Fixed**
- **oheaders** from a Response contains `Set-Cookie` entries when it should not.
- Static type checker not accepting **list\[str\]** in values for argument **param**.
- Static type checker not accepting **Iterable\[bytes\]** for **data**.
- Function proxy_bypass_registry for Windows may be fooled by insufficient control on our end.
  Patch taken from idle upstream PR psf#6302
- SSLError message related to the certificate revocation could print `None` instead of `unspecified` for the reason.

**Changed**
- Allow setting `None` in max_size for **SharableLimitedDict** to remove limits.
- Using `RLock` instead of `Lock` in **SharableLimitedDict**, and **InMemoryRevocationStatus** classes.

**Misc**
- Missing assert statements for test test_header_validation.
- Unrelated warnings are now silent in our test suite.
- Unexpected warning now trigger an error in our test suite.
- Removed `tests.compat`.
- Removed `test-readme`, `flake8`, and `publish` from Makefile.

**Added**
- Extra-dist install `http3` to force install HTTP/3 support in your environment if not present.
- Extra-dist install `ocsp` to force install certificate revocation support in your environment if not present.
@Ousret Ousret mentioned this pull request Oct 19, 2023
Merged
Ousret added a commit to jawah/niquests that referenced this pull request Oct 19, 2023
@Ousret
🔖 Release 3.1.3
b9bd9b3 
**Fixed**
- **oheaders** from a Response contains `Set-Cookie` entries when it should not.
- Static type checker not accepting **list\[str\]** in values for argument **param**.
- Static type checker not accepting **Iterable\[bytes\]** for **data**.
- Function proxy_bypass_registry for Windows may be fooled by insufficient control on our end.
  Patch taken from idle upstream PR psf#6302
- SSLError message related to the certificate revocation could print `None` instead of `unspecified` for the reason.

**Changed**
- Allow setting `None` in max_size for **SharableLimitedDict** to remove limits.
- Using `RLock` instead of `Lock` in **SharableLimitedDict**, and **InMemoryRevocationStatus** classes.

**Misc**
- Missing assert statements for test test_header_validation.
- Unrelated warnings are now silent in our test suite.
- Unexpected warning now trigger an error in our test suite.
- Removed `tests.compat`.
- Removed `test-readme`, `flake8`, and `publish` from Makefile.

**Added**
- Extra-dist install `http3` to force install HTTP/3 support in your environment if not present.
- Extra-dist install `ocsp` to force install certificate revocation support in your environment if not present.
Ousret added a commit to jawah/niquests that referenced this pull request Oct 19, 2023
@Ousret
🔖 Release 3.1.3
a5412f0 
**Fixed**
- **oheaders** from a Response contains `Set-Cookie` entries when it should not.
- Static type checker not accepting **list\[str\]** in values for argument **param**.
- Static type checker not accepting **Iterable\[bytes\]** for **data**.
- Function proxy_bypass_registry for Windows may be fooled by insufficient control on our end.
  Patch taken from idle upstream PR psf#6302
- SSLError message related to the certificate revocation could print `None` instead of `unspecified` for the reason.

**Changed**
- Allow setting `None` in max_size for **SharableLimitedDict** to remove limits.
- Using `RLock` instead of `Lock` in **SharableLimitedDict**, and **InMemoryRevocationStatus** classes.

**Misc**
- Missing assert statements for test test_header_validation.
- Unrelated warnings are now silent in our test suite.
- Unexpected warning now trigger an error in our test suite.
- Removed `tests.compat`.
- Removed `test-readme`, `flake8`, and `publish` from Makefile.

**Added**
- Extra-dist install `http3` to force install HTTP/3 support in your environment if not present.
- Extra-dist install `ocsp` to force install certificate revocation support in your environment if not present.
Ousret added a commit to jawah/niquests that referenced this pull request Oct 19, 2023
@Ousret
🔖 Release 3.1.3 (#38)
7de4041 
**Fixed**
- **oheaders** from a Response contains `Set-Cookie` entries when it should not.
- Static type checker not accepting **list\[str\]** in values for argument **param**.
- Static type checker not accepting **Iterable\[bytes\]** for **data**.
- Function proxy_bypass_registry for Windows may be fooled by insufficient control on our end.
  Patch taken from idle upstream PR psf#6302
- SSLError message related to the certificate revocation could print `None` instead of `unspecified` for the reason.

**Changed**
- Allow setting `None` in max_size for **SharableLimitedDict** to remove limits.
- Using `RLock` instead of `Lock` in **SharableLimitedDict**, and **InMemoryRevocationStatus** classes.

**Misc**
- Missing assert statements for test test_header_validation.
- Unrelated warnings are now silent in our test suite.
- Unexpected warning now trigger an error in our test suite.
- Removed `tests.compat`.
- Removed `test-readme`, `flake8`, and `publish` from Makefile.

**Added**
- Extra-dist install `http3` to force install HTTP/3 support in your environment if not present.
- Extra-dist install `ocsp` to force install certificate revocation support in your environment if not present.
@aaronhua123
Copy link

The value of proxyOverride is controlled by a third-party program on the computer, so it is not controllable. For example, when I use fiddler on my computer, this value is <-loopback>;.(Not all computers) When proxyOverride splits, it's equal to ['<-loopback>', '']. So re.match('', host, re.I) will return true,that bypassing the proxy.

And filter(function, iterable) is equivalent to the generator expression (item for item in iterable if item) if function is None,that is, all elements of iterable that are false are removed. Ref:https://docs.python.org/3/library/functions.html#filter

I think should be avoided for code robustness.

Yes, I also noticed this situation. This wastes a lot of my time discovering this problem. ProxyOverride sometimes ends with a semicolon and is difficult to reproduce. But it has not yet been merged into the main branch.

@flysee
Copy link
Contributor Author

flysee commented Mar 15, 2024

The value of proxyOverride is controlled by a third-party program on the computer, so it is not controllable. For example, when I use fiddler on my computer, this value is <-loopback>;.(Not all computers) When proxyOverride splits, it's equal to ['<-loopback>', '']. So re.match('', host, re.I) will return true,that bypassing the proxy.
And filter(function, iterable) is equivalent to the generator expression (item for item in iterable if item) if function is None,that is, all elements of iterable that are false are removed. Ref:https://docs.python.org/3/library/functions.html#filter
I think should be avoided for code robustness.

Yes, I also noticed this situation. This wastes a lot of my time discovering this problem. ProxyOverride sometimes ends with a semicolon and is difficult to reproduce. But it has not yet been merged into the main branch.

I'm glad someone else is having the same problem as me, but I also hope that no one will waste time on this issue in the future, haha.

@sigmavirus24
Copy link
Contributor

@flysee can you add tests for this change so it does not regress?

@flysee flysee force-pushed the main branch 2 times, most recently from cd18edd to b0942c8 Compare March 18, 2024 11:14
@flysee
Copy link
Contributor Author

flysee commented Mar 18, 2024

@flysee can you add tests for this change so it does not regress?

Of course, test cases have been added.

sigmavirus24
sigmavirus24 approved these changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@sigmavirus24 sigmavirus24 left a comment

Choose a reason for hiding this comment

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

@nateprewitt thoughts on getting this into the next release? 2.32 iirc?

@nateprewitt nateprewitt added this to the 2.32.0 milestone Mar 18, 2024
nateprewitt
nateprewitt approved these changes Mar 18, 2024
View reviewed changes
Copy link
Member

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

Yeah, this seems reasonable. I've put it on the 2.32.0 milestone.

@sigmavirus24 sigmavirus24 merged commit 8dd3b26 into psf:main Mar 18, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamxiwang iamxiwang iamxiwang approved these changes

@ravi-kale ravi-kale ravi-kale approved these changes

@nateprewitt nateprewitt nateprewitt approved these changes

@sigmavirus24 sigmavirus24 sigmavirus24 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.32.0
Development

Successfully merging this pull request may close these issues.
6 participants
@flysee @sigmavirus24 @aaronhua123 @nateprewitt @ravi-kale @iamxiwang