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

Request with HttpVersion > 3 and RequestVersionOrHigher policy sent as H/3 request #72185

Closed
greenEkatherine opened this issue Jul 14, 2022 · 5 comments · Fixed by #102423
Closed

Request with HttpVersion > 3 and RequestVersionOrHigher policy sent as H/3 request #72185

greenEkatherine opened this issue Jul 14, 2022 · 5 comments · Fixed by #102423
Labels
area-System.Net.Http bug help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged
Projects
HTTP/3
Milestone
Future

Comments

@greenEkatherine
Copy link
Contributor

greenEkatherine commented Jul 14, 2022 •
edited by karelz

Description

Request message with HttpVersion higher than currently supported 3.0 is not checked on the HttpVersionPolicy. In case when it is set to RequestVersionOrHigher we get unexpected behavior -- request is treated as H/3 request.

Reproduction Steps

var requestMessage = new HttpRequestMessage(HttpMethod.Get, uri)
{
    Version = new Version(4, 0),
    VersionPolicy = HttpVersionPolicy.RequestVersionOrHigher
};

Expected behavior

Throw request version cannot establish

Actual behavior

No exception from HttpConnection pool and sent via H/3 connection

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 14, 2022
@ghost
Copy link

ghost commented Jul 14, 2022

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

Issue Details

Description

Request message with HttpVersion higher than currently supported 3.0 is not checked on the HttpVersionPolicy. In case when it is set to RequestVersionOrHigher we get unexpected behavior -- request is treated as H/3 request.

Reproduction Steps

var requestMessage = new HttpRequestMessage(HttpMethod.Get, uri)
{
Version = new Version(4, 0),
VersionPolicy = HttpVersionPolicy.RequestVersionOrHigher
};

Expected behavior

Throw request version cannot establish

Actual behavior

No exception from HttpConnection pool and sent via H/3 connection

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: greenEkatherine
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

@wfurt wfurt removed the untriaged New issue has not been triaged by the area owner label Jul 14, 2022
@wfurt wfurt added this to To Do (Low Priority) in HTTP/3 via automation Jul 14, 2022
@wfurt wfurt added this to the 7.0.0 milestone Jul 14, 2022
@karelz karelz added the bug label Jul 21, 2022
@greenEkatherine greenEkatherine mentioned this issue Aug 5, 2022
Closed
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 5, 2022
@karelz karelz modified the milestones: 7.0.0, 8.0.0 Aug 12, 2022
@karelz
Copy link
Member

karelz commented Aug 12, 2022 •
edited

Low priority, not big impact on customers, moving to 8.0 due to time restrictions.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 23, 2022
@ManickaP
Copy link
Member

Should we check the request version against known versions and allow only that? I.e.:

runtime/src/libraries/System.Net.Primitives/src/System/Net/HttpVersion.cs

Lines 10 to 17 in 69045ad

/// <summary>Defines a <see cref="Version"/> instance for HTTP 1.0.</summary>
public static readonly Version Version10 = new Version(1, 0);
/// <summary>Defines a <see cref="Version"/> instance for HTTP 1.1.</summary>
public static readonly Version Version11 = new Version(1, 1);
/// <summary>Defines a <see cref="Version"/> instance for HTTP 2.0.</summary>
public static readonly Version Version20 = new Version(2, 0);
/// <summary>Defines a <see cref="Version"/> instance for HTTP 3.0.</summary>
public static readonly Version Version30 = new Version(3, 0);

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.

@ManickaP ManickaP added the help wanted [up-for-grabs] Good issue for external contributors label Jan 19, 2023
@ManickaP ManickaP modified the milestones: 8.0.0, Future Jan 19, 2023
@ManickaP
Copy link
Member

Triage: we don't have any customer reports complaining about this, not critical for 8.0. We would take a community PR though.

@pedrobsaila pedrobsaila mentioned this issue May 19, 2024
Merged
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label May 19, 2024
@hez2010
Copy link
Contributor

hez2010 commented May 20, 2024 •
edited

Nothing is disallowing setting an invalid version in HTTP message, you can even form a http message with an invalid version and send it with curl. IMO how to handle the version is up to the HTTP handler implementation, it shouldn't be restricted on higher levels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-System.Net.Http bug help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged
Projects
No open projects
HTTP/3
  
To Do (Low Priority)
Milestone
Future
5 participants
@karelz @ManickaP @wfurt @hez2010 @greenEkatherine