[HTTP/3] Fix QPACK status decode #57236
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFix "Literal Header Field With Name Reference" parsing for Status code. Add tests for QPACK-ed and not QPACK-ed status receiving for all status codes (except informational). Fix Fixes #55988 Notes:
|
The sync from runtime to aspnetcore is automatic. Thanks for the heads up. |
| H3StaticTable.Status425 => 425, | ||
| H3StaticTable.Status500 => 500, | ||
| // We should never get here, at least while we only use static table. But we can still parse staticValue. | ||
| _ => ParseStatusCode(staticIndex, staticValue) |
There was a problem hiding this comment.
Should this error? I'm not familiar with this code, but I think you're using the static index to resolve to a descriptor, and the descriptor must be :status, so any other static index at this point should be impossible.
There was a problem hiding this comment.
Ah, I see a Debug.Fail. As long as weirdness here gets noticed then that's fine 👍
|
FYI I triggered the sync bot manually so we could get this into aspnetcore quickly. dotnet/aspnetcore#35293 |
ghost
locked as resolved and limited conversation to collaborators
Fix "Literal Header Field With Name Reference" parsing for Status code. Add tests for QPACK-ed and not QPACK-ed status receiving for all status codes (except informational).
Fix
QPackStaticTable.HeaderLookupto comply with spec https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#name-static-table-2 (the order was off, so lookup didn't work correctly in some cases).Fixes #55988
Notes:
While checking spec compliance, I noticed that index 34 should
access-control-allow-headersinstead ofaccess-control-allow-origin(inH3StaticTable.s_staticTable). It seems to be shared code, so I guess after this PR Kestrel will need to sync with the update? cc @JamesNKIt would be nice to get rid of
QPackStaticTable.HeaderLookupin favor ofH3StaticTable.s_staticTable, or use a generation of some sort, but I didn't check whether it will be easy to achieve, and I don't know why there are two of them in the first place.