Fix Kestrel host header mismatch handling when port in Url #59352
Conversation
BrennanConroy
added
the
area-networking
BrennanConroy
requested review from
halter73,
JamesNK and
mgravell
as code owners
| @@ -649,11 +648,16 @@ private void ValidateNonOriginHostHeader(string hostText) | |||
| if (hostText != _absoluteRequestTarget!.Authority) | |||
| { | |||
| if (!_absoluteRequestTarget.IsDefaultPort | |||
| || hostText != _absoluteRequestTarget.Authority + ":" + _absoluteRequestTarget.Port.ToString(CultureInfo.InvariantCulture)) | |||
| || hostText != $"{_absoluteRequestTarget.Authority}:{_absoluteRequestTarget.Port}") | |||
There was a problem hiding this comment.
Unrelated to the bug, but this temporary string allocation seems like it could be optimized away.
There was a problem hiding this comment.
There was a problem hiding this comment.
We could do it ourselves. e.g. check that content before : matches _absoluteRequestTarget.Authority and content after : parses to an int and matches _absoluteRequestTarget.Port.
But if this path isn't that hot then probably not worth the complexity.
There was a problem hiding this comment.
If the host doesn't match the authority.
|
LGTM. I assume this will go to 9 and 8? |
|
/backport to release/9.0 |
|
Started backporting to release/9.0: https://github.com/dotnet/aspnetcore/actions/runs/12210325288 |
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/12210328859 |
|
@BrennanConroy backporting to release/8.0 failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix Kestrel host header mismatch handling when port in Url
Using index info to reconstruct a base tree...
M src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs
M src/Servers/Kestrel/shared/test/HttpParsingData.cs
M src/Servers/Kestrel/test/InMemory.FunctionalTests/BadHttpRequestTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Servers/Kestrel/test/InMemory.FunctionalTests/BadHttpRequestTests.cs
CONFLICT (content): Merge conflict in src/Servers/Kestrel/test/InMemory.FunctionalTests/BadHttpRequestTests.cs
Auto-merging src/Servers/Kestrel/shared/test/HttpParsingData.cs
Auto-merging src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Fix Kestrel host header mismatch handling when port in Url
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
When using the
KestrelServerOptions.AllowHostHeaderOverrideoption, if a port was included in the request URL and the Host header didn't match, the request would still fail with a 400 Bad Request due to us double including the port in the computed Host string.Fails with
Invalid Host header: 'www.foo.com:1234:1234'Worked
Since
Uri.Authoritydoesn't include the default port (80 for http, 443 for https), we should only append the port in the computed host header if it's not the default port for the scheme.