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

[wasm] fix SendAsync_GetWithInvalidHostHeader_ThrowsException - doesn't throw on WASM #53874

Closed
pavelsavara opened this issue Jun 8, 2021 · 4 comments · Fixed by #71923
Closed

[wasm] fix SendAsync_GetWithInvalidHostHeader_ThrowsException - doesn't throw on WASM #53874

pavelsavara opened this issue Jun 8, 2021 · 4 comments · Fixed by #71923
Assignees
@pavelsavara
Labels
arch-wasm WebAssembly architecture area-System.Net.Http
Milestone
7.0.0

Comments

@pavelsavara
Copy link
Member

System.Net.Http.Functional.Tests.SocketsHttpHandlerTest_HttpClientHandlerTest_Headers_Http11.SendAsync_GetWithInvalidHostHeader_ThrowsException on WASM in browser expects HttpRequestMessage.Headers.Host validation.

Assert.Throws() Failure
Expected: typeof(System.Net.Http.HttpRequestException)\nActual:   (No exception was thrown)]]
   at System.Net.Http.Functional.Tests.HttpClientHandlerTest_Headers.SendAsync_GetWithInvalidHostHeader_ThrowsException()
@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-System.Net.Http labels Jun 8, 2021
@ghost
Copy link

ghost commented Jun 8, 2021

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

Issue Details

System.Net.Http.Functional.Tests.SocketsHttpHandlerTest_HttpClientHandlerTest_Headers_Http11.SendAsync_GetWithInvalidHostHeader_ThrowsException on WASM in browser expects HttpRequestMessage.Headers.Host validation.

Assert.Throws() Failure
Expected: typeof(System.Net.Http.HttpRequestException)\nActual:   (No exception was thrown)]]
   at System.Net.Http.Functional.Tests.HttpClientHandlerTest_Headers.SendAsync_GetWithInvalidHostHeader_ThrowsException()
Author: pavelsavara
Assignees: -
Labels:

arch-wasm, area-System.Net.Http
Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jun 8, 2021
@pavelsavara pavelsavara mentioned this issue Jun 8, 2021
Merged
pavelsavara added a commit to pavelsavara/runtime that referenced this issue Jun 8, 2021
@pavelsavara
- improve doc
63e863b 
- move tests to inner loop
- more granular ActiveIssue dotnet#53592 for lack of TRACE
- more granular ActiveIssue dotnet#53591 for content on GET/HEAD
- more granular ActiveIssue dotnet#53874 for HttpRequestMessage.Headers.Host
- more granular ActiveIssue dotnet#53872 for NPE on System.Net.Http.BrowserHttpHandler
- fix HTTP vs HTTPS test configuration `Http2SecureRemoteEchoServer`
- include echo middleware in xharness server
- include middleware in Helix correlation payload
@lewing lewing added this to the 6.0.0 milestone Jun 8, 2021
@lewing
Copy link
Member

lewing commented Jun 8, 2021

I'm not sure how serious this one is, it looks like we're missing validation on an invalid header and we might end up pulling in a lot of code to do that validation, but someone should take a look.

@lewing lewing removed the untriaged New issue has not been triaged by the area owner label Jun 8, 2021
pavelsavara added a commit that referenced this issue Jun 9, 2021
@pavelsavara
[wasm][testing] hosting echo server in xharness process (#52923)
fb61be4 
- move tests to inner loop
- include echo middleware in xharness server
- improve doc
- more granular ActiveIssue #53592 for lack of TRACE
- more granular ActiveIssue #53591 for content on GET/HEAD
- more granular ActiveIssue #53874 for HttpRequestMessage.Headers.Host
- more granular ActiveIssue #53872 for NPE on System.Net.Http.BrowserHttpHandler
- more granular ActiveIssue #53876
- include middleware in Helix correlation payload
@lewing lewing modified the milestones: 6.0.0, 7.0.0 Aug 8, 2021
@ilonatommy
Copy link
Member

@pavelsavara, what do you think about @lewing's hypothesis?

@pavelsavara
Copy link
Member Author

Perhaps it's not that bad ?

runtime/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpRequestHeaders.cs

Line 116 in 3c33b42

if ((value != null) && (HttpRuleParser.GetHostLength(value, 0, false) != value.Length))

runtime/src/libraries/System.Net.Http/src/System/Net/Http/HttpRuleParser.cs

Line 199 in 3c33b42

internal static int GetHostLength(string input, int startIndex, bool allowToken)

@pavelsavara pavelsavara mentioned this issue Jul 11, 2022
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 11, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@pavelsavara pavelsavara

Labels
arch-wasm WebAssembly architecture area-System.Net.Http
Projects
None yet
Milestone
7.0.0
Development

Successfully merging a pull request may close this issue.

[wasm] closing invalid host header as would not fix pavelsavara/runtime
3 participants
@lewing @pavelsavara @ilonatommy