forbid http request with invalid version#102423
Conversation
ghost
added
the
dotnet-policy-service
bot
added
the
community-contribution
| 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.
Is it extensible for future? What if a third-party http client implementation that accepts higher versions?
There was a problem hiding this comment.
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.
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.
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.
I agree, this should be handled at the specific handler level. You can check out
for Sockets handler.
ManickaP
left a comment
ManickaP
left a comment
There was a problem hiding this comment.
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 |
6 participants
Fixes #72185