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

Remove Uri scheme validation from HttpRequestMessage #55035

Merged
merged 7 commits into from
Jul 10, 2021
Merged

Remove Uri scheme validation from HttpRequestMessage #55035

merged 7 commits into from
Jul 10, 2021

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Jul 1, 2021 •
edited
Loading

Fixes #52836

Removed the Uri scheme check from HttpRequestMessage / HttpClient.BaseAddress and moved it in SocketsHttpHandler.ValidateAndNormalizeRequest.

Any scheme will be passed to handlers.

No merge: Should other handlers have filters for specific schemes?
WinHttpHandler takes 85 seconds to figure out it can't process foo://httpbin.org without a fail-fast like this for example.
Added http / https checks to WinHttpHandler.

@MihaZupan MihaZupan added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 1, 2021
@MihaZupan MihaZupan added this to the 6.0.0 milestone Jul 1, 2021
@MihaZupan MihaZupan requested a review from a team July 1, 2021 19:50
@MihaZupan MihaZupan self-assigned this Jul 1, 2021
@ghost
Copy link

ghost commented Jul 1, 2021

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

Issue Details

Fixes #52836

Removed the Uri scheme check from HttpRequestMessage / HttpClient.BaseAddress and moved it in SocketsHttpHandler.ValidateAndNormalizeRequest.

Any scheme will be passed to handlers.

No merge: Should other handlers have filters for specific schemes?
WinHttpHandler takes 85 seconds to figure out it can't process foo://httpbin.org without a fail-fast like this for example.

Author: MihaZupan
Assignees: MihaZupan
Labels:

* NO MERGE *, area-System.Net
Milestone: 6.0.0

@geoffkizer
Copy link
Contributor

No merge: Should other handlers have filters for specific schemes?
WinHttpHandler takes 85 seconds to figure out it can't process foo://httpbin.org without a fail-fast like this for example.

Yeah, we should add the same logic to WinHttpHandler.

@MihaZupan MihaZupan removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 2, 2021
@MihaZupan
Copy link
Member Author

With this PR:

  • HttpRequestMessage does not throw on any Uri
  • SocketsHttpHandler behavior remains the same (http, https, ws, wss)
  • WinHttpHandler only accepts http and https
  • BrowserHttpHandler will receive any Url (any absolute scheme + any relative)

@lewing is this the desired behavior for Browser, or should we be filtering out relative Uris?

@MihaZupan
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

stephentoub
stephentoub reviewed Jul 9, 2021
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs
HttpTelemetry.Log.IsEnabled() &&
!request.WasSentByHttpClient() &&
request.RequestUri is Uri requestUri &&
requestUri.IsAbsoluteUri;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we disable telemetry if the Uri isn't absolute?

Copy link
Member Author

Choose a reason for hiding this comment

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

The call to RequestStart that follows this check will access Uri fields that would throw for relative Uris.

Adding this check here so that you get the meaningful exception message from SocketsHttpHandler instead.

stephentoub
stephentoub reviewed Jul 9, 2021
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpUtilities.cs
{
return task.ContinueWith(continuation, state, CancellationToken.None,
TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default);
}
Copy link
Member

Choose a reason for hiding this comment

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

Good riddance. Thanks!

@MihaZupan MihaZupan force-pushed the defer-scheme-check branch from 9eb1416 to da8844d Compare July 9, 2021 15:33
@MihaZupan MihaZupan merged commit 9da4d07 into dotnet:main Jul 10, 2021
@karelz karelz mentioned this pull request Jul 20, 2021
Closed
@ManickaP ManickaP mentioned this pull request Jul 20, 2021
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@geoffkizer geoffkizer geoffkizer left review comments

@campersau campersau campersau left review comments

@stephentoub stephentoub stephentoub approved these changes

Assignees

@MihaZupan MihaZupan

Labels
area-System.Net
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Support to request extensions://** by HttpClient
4 participants
@MihaZupan @geoffkizer @stephentoub @campersau