Improve HttpEnvironmentProxy uri parsing#121206
Merged
MihaZupan merged 1 commit intodotnet:mainfrom Nov 4, 2025
Merged
Conversation
Contributor
|
Tagging subscribers to this area: @dotnet/ncl |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR improves the parsing logic for HTTP proxy environment variables in HttpEnvironmentProxy. The changes fix issues with parsing proxy URLs that contain path, query, or fragment components, and simplify the logic for extracting user authentication information.
Key changes:
- Changed from
LastIndexOf('@')toIndexOf('@')when parsing user authentication to avoid incorrect parsing when '@' appears in paths - Added logic to strip path/query/fragment delimiters ('/', '?', '#') from proxy URLs before parsing
- Simplified port extraction by removing the manual digit-stripping loop and relying on
TryParseto handle the remaining string - Improved documentation clarity for the
GetUriFromStringmethod - Optimized string comparison in bypass list matching
- Removed redundant using statements
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| HttpEnvironmentProxy.cs | Improved proxy URL parsing to handle path/query/fragment components, fixed user info extraction logic, simplified port parsing, and improved documentation |
| HttpEnvironmentProxyTest.cs | Added test cases to verify correct parsing of proxy URLs with user info containing '@', and URLs with path/query/fragment components |
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpEnvironmentProxy.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpEnvironmentProxy.cs
Show resolved
Hide resolved
Open
3 tasks
f5637b7 to
9f95616
Compare
stephentoub
approved these changes
Nov 4, 2025
Member
Author
|
/ba-g Android timeouts |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We have a test that the path may be present, but while that particular input happens to work, the parsing logic isn't actually accounting for paths as a possibility.
If the path/query/fragment contains
:or@, it'd break.This change strips out the path/query/fragment portion so that our
hostthat we pass toUriBuilderis more likely to be just the host.Hit in #121083 that tries to add more validation to the
UriBuilder.Hostsetter.This does break some weird edge cases that we happened to support currently, e.g.
host:8080abc(garbage after the port).But I kept other weird cases like
user:p@ss@hostas-is (@as part of user info), which does mean that a@in the path can still break things.Ideally this method would be using
Uri.TryCreateinstead, but we currently accept some interesting inputs likedomain\foo:pass@1.1.1.1, so we'd still have to do some manual processing.dotnet/corefx#31232 (comment)