Skip to content

Conversation

@MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Oct 30, 2025

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 host that we pass to UriBuilder is more likely to be just the host.
Hit in #121083 that tries to add more validation to the UriBuilder.Host setter.

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@host as-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.TryCreate instead, but we currently accept some interesting inputs like domain\foo:[email protected], so we'd still have to do some manual processing.
dotnet/corefx#31232 (comment)

@MihaZupan MihaZupan added this to the 11.0.0 milestone Oct 30, 2025
@MihaZupan MihaZupan self-assigned this Oct 30, 2025
@MihaZupan MihaZupan marked this pull request as ready for review October 30, 2025 15:37
Copilot AI review requested due to automatic review settings October 30, 2025 15:37
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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('@') to IndexOf('@') 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 TryParse to handle the remaining string
  • Improved documentation clarity for the GetUriFromString method
  • 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

@MihaZupan MihaZupan enabled auto-merge (squash) November 4, 2025 17:16
@MihaZupan
Copy link
Member Author

/ba-g Android timeouts

@MihaZupan MihaZupan merged commit 69d8087 into dotnet:main Nov 4, 2025
83 of 87 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants