-
Notifications
You must be signed in to change notification settings - Fork 10k
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.
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
Mark bytes as consumed #13394
Mark bytes as consumed #13394
Conversation
This looks ready but we need an approver (beyond QB-approval, which this has) |
Can we get this reviewed and checked in so we can get a build for Sunday night validation? |
|
||
Assert.Equal(2, accumulator.KeyCount); | ||
var dict = accumulator.GetResults(); | ||
Assert.Equal("\"%-.<>\\^_`{|}~", dict["\"%-.<>\\^_`{|}~"]); | ||
Assert.Equal("wow", dict["\"%-.<>\\^_`{|}"]); | ||
} | ||
|
||
[Fact] | ||
public void TryParseFormValues_MultiSegmentFastPathWorks() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this would have failed before the fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this covers the repro scenario.
@JunTaoLuo @Pilchie admin merge please. |
#13372 @Pilchie
This looks like a preview9 regression from #12749 where we refactored the form parser. It's externally reported as impacting OpenIdConnect auth, but I was only able to reproduce it using WsFed auth. The request fails and the client is unable to log in. I don't see any immediate workarounds.
The issue happens if the form is long enough to take the slow path (multiple buffer segments), but the last key=value pair is short and takes the single segment fast path. In that case it fails to mark the last bytes as consumed and the parser mistakenly aborts.
Compare the fix to what happens in the slow path on lines 294 and 295 bellow.