Harden chunked encoding parsing

[Youssef1313]

Fixes #66794

• It's easier, IMO, to read the whole updated class compared to reading the diff.

[Copilot]

Pull request overview

This PR hardens Kestrel’s HTTP/1.1 chunked transfer-encoding parsing to strictly reject invalid chunk extensions (notably CR/LF inside chunk-ext, including quoted-string cases) per RFC 9110/9112, and updates tests accordingly. It also removes the legacy insecure chunked parsing AppContext switch from the mainline implementation.

Changes:

• Tighten Http1ChunkedEncodingMessageBody chunk-size and chunk-extension parsing (including stricter token/quoted-string validation and CRLF handling).
• Expand/adjust chunked request test coverage for invalid/valid extension forms and boundary splitting.
• Update existing tests’ chunk-extension strings to avoid now-invalid characters (e.g., spaces in tokens).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs	Reworks chunk-size and chunk-ext parsing to be stricter and RFC-aligned (including quoted-string rules).
src/Servers/Kestrel/Core/test/MessageBodyTests.cs	Adds new tests for incomplete chunk-extension/value parsing and max hex digit chunk-size parsing.
src/Servers/Kestrel/test/InMemory.FunctionalTests/ChunkedRequestTests.cs	Expands valid/invalid chunk extension theory cases; updates extension examples; adjusts RemoteExecutor options.
src/Servers/Kestrel/test/InMemory.FunctionalTests/MaxRequestBodySizeTests.cs	Updates chunk-extension strings in payloads to remain valid under stricter parsing.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cincuranet]

With MaxRequestBodySize = 10 and

Transfer-Encoding: chunked\r\n
\r\n
5;a="bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb

the 413 is not returned.

With 5;a="aaaaaaaaaaaa... ← 1 MB of 'a', no closing quote, no CRLF, written in 64-byte segments, because the code never advances consumed until a whole ;name[=value] unit parses, every arrival re-scans the entire pending extension. Serious CPU burn. The quoted variant is worst because it's parsed byte-at-a-time through a Func<byte,bool> delegate.

Finally, I'm missing tests for stuff like 2;a="x\r\ny"\r\n and 2;a="x\<CR>"\r\n.

[Youssef1313]

With MaxRequestBodySize = 10 and

Transfer-Encoding: chunked\r\n
\r\n
5;a="bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb

the 413 is not returned.

With 5;a="aaaaaaaaaaaa... ← 1 MB of 'a', no closing quote, no CRLF, written in 64-byte segments, because the code never advances consumed until a whole ;name[=value] unit parses, every arrival re-scans the entire pending extension. Serious CPU burn. The quoted variant is worst because it's parsed byte-at-a-time through a Func<byte,bool> delegate.

Finally, I'm missing tests for stuff like 2;a="x\r\ny"\r\n and 2;a="x\<CR>"\r\n.

Let me see what's the best way to handle this. I guess we will need more "internal" modes to allow us to track where we were at in parsing extensions and then we can consume things more frequently.

[DeagleGross]

Jiri raised a great point about code re-reading the whole payload on the next iteration; but I think we can do in the follow-up PR - this PR can focus on char rejection only. If you decide to do in follow-up - lets create a separate issue to track this?

If i would be doing this, i would create a struct that holds mode, readOffset (which keeps track of how much data we already parsed from the input buffer), and chunkSize which does not require us re-reading the first byte to determine size. Then you can continue from last not read byte.

[dotnet-policy-service]

Thanks for identifying a breaking change.

no assignees, after you commit this PR please take the following actions, as part of the breaking changes announcement process:

• Create an announcement issue by using the ASP.NET Core breaking change issue template.
• Link the breaking change announcement issue from this PR.
• Remove the needs-breaking-change-announcement label.

[DeagleGross]

ChunkedExtensionParser only hold single State which is enum. Currently you are allocating a new ref-type object on each extension section parse - could we instead store State _state here, and pass it into ChunkedExtensionParser.Consume? Then ChunkedExtensionParser becomes fully static class, and we do not allocate.

[Youssef1313]

I didn't care much about allocation here since we don't expect typical requests to have them.

It's fine to move the state here, but I wanted to fully isolate the implementation. So maybe we can make it a struct instead of a class then, if we care about the extra allocation.

[DeagleGross]

IMHO you did great with isolating the parsing logic, there is no problem passing the state to the method. If you prefer struct - I am OK with that (anyway it will probably be created once per request given a standard client)

[DeagleGross]

should this be done right after reader.TryPeek(out ch)?

[Youssef1313]

We don't want to consume the ; here because that's the thing that extension parsing expects. That's why I did TryPeek instead of TryRead, and then return if we see that we are about to start an extension (without advancing), and then advancing only if we are going to consume this as part of the chunk prefix. The comment on the TryPeek call was my attempt to explain this. Does it help if I add an additional comment here, like the following?

// See comment above the TryPeek call to understand why we advance at this specific point.
reader.Advance(1);

There can still be a question of whether we advance before or after the exception in the !EnableChunkedExtensions check. But I guess if we are throwing an exception and failing already, it doesn't matter at all if we advance or not? The code might read better if we advance before the exception though?

[Youssef1313]

I think it's already fine. It's not the responsibility of the prefix parsing to consume the BWS or the semicolon, so we never consume that.

[DeagleGross]

comment is here, but we do not advance for 2nd read?

[Youssef1313]

That line looks like a dead code. Thanks for catching it