[LTS] Kestrel considers Connection: keep-alive, upgrade as non-upgrade request

[muratg]

Original issue: #1170.

We'll need to fix this in both 1.1.0 and 1.0.2 versions of Kestrel.

[muratg]

Make sure to bump Hosting dependency to 1.0.1 once aspnet/Hosting#873 is in.

[Eilon]

This patch is approved, please ensure it is merged into the correct branch and building as part of the patch train.

[cesarblum]

Was there a particular reason why we decided to port this to 1.0.2? A request with Connection: keep-alive, upgrade to Kestrel 1.0.1 gets a MessageBody type of ForRemainingData, which is expected.

@anurse Did you repro the original issue on Kestrel 1.0.1? Do you know up to what version Firefox was sending Connection: keep-alive, upgrade? Latest version sends Connection: Upgrade.

[analogrelay]

I did not repro the original issue on 1.0.1, but neither did I try to repro it :). @BrennanConroy had the original repro. If the behavior is already correct in 1.0.1, I don't see a need to port the fix.

As for which version of Firefox had the issue, I'm not sure. It's possible, given the fast pace of updates, they've changed it since @BrennanConroy originally reproduced it.

[BrennanConroy]

I can check tomorrow

[Eilon]

Derp 😄 Let's see what happens here - it's easy to not check something in 😄

[BrennanConroy]

So 1.0.1 worked because it didn't even check for "upgrade" and by default returns a ForRemainingData, you can see this change 73656f6 is when it "broke"

So 1.0.2 can probably stay as is.

[cesarblum]

Alright I'm gonna close this then.

[analogrelay]

Cool. Could we port the test over so we ensure that the behavior is never regressed?

[muratg]

Agreed, @CesarBS could you port the test? Also the labels on this bug :)

[halter73]

The tests added in #1178 were mostly unit tests that make ensure Kestrel's properly interprets the header in it's internal data structures.

We need a new test verifying that it's possible to read all the data sent by the client over the upgraded connection given a Connection: keep-alive, upgrade request. This test could be merged into 1.0.* and dev.

[cesarblum]

@halter73 Yep, this one should do: https://github.com/aspnet/KestrelHttpServer/pull/1178/files#diff-91e1a141d6812899520ca7d9e201e647R294

[halter73]

👍 It looks like that should pass in 1.0.*. A functional test might be nice too.

[Eilon]

Ok so we're not fixing any product code, but we're just updating tests?

[cesarblum]

@Eilon Correct.

[Eilon]

Ok, I'll remove it from my OneNote tracking page. Thanks!

[muratg]

Removing the patch-approved label...