-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
forbid http request with invalid version #102423
Conversation
if (value != HttpVersion.Version10 && value != HttpVersion.Version11 && value != HttpVersion.Version20 && value != HttpVersion.Version30) | ||
{ | ||
throw new ArgumentOutOfRangeException(nameof(value), SR.net_http_request_invalid_version); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it extensible for future? What if a third-party http client implementation that accepts higher versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it extensible for future?
What do you mean by extensible ? Is it handling new versions as they come off ? or giving users an api so they choose the versions they want ?
What if a third-party http client implementation that accepts higher versions?
See #72185 (comment)
Question is whether that could break someone. For instance, someone asking for 2.5 and getting HTTP/2.
If we'd chose to go the strict way, we should probably get it in early in the release to see if someone gets borked by it.
This change is made as an experiment, it will probably be rolled back if people complain about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it handling new versions as they come off ? or giving users an api so they choose the versions they want ?
HttpRequestMessage
is decoupled from HttpClient
and can be used by others as well. I can assume when a new HTTP version comes out in the future there will be some 3rd implementations using the HttpRequestMessage
, but this change will block them reusing the built-in type.
There isn't any restriction that doesn't allow users to create a HttpRequestMessage
with unknown versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you prefer doing validation higher in HttpClient.Send methods or below the stack in Socket/Connection Handlers ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this should be handled at the specific handler level. You can check out
Line 399 in 0588f24
public async ValueTask<HttpResponseMessage> SendWithVersionDetectionAndRetryAsync(HttpRequestMessage request, bool async, bool doRequestAuth, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you. This might be considered breaking change, since we were much more lenient before and now it might throw for some people.
@dotnet/ncl Any concerns here? Or can we merge? |
/azp run runtime-libraries-coreclr outerloop (just in case) |
No pipelines are associated with this pull request. |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Outerloop failures are unrelated and being handled in #102986 |
Fixes #72185