Ugh, looks like I'm too slow on my work with the upgrade proposal hooks; sorry I haven't been able to give in-depth feedback yet.
On Dec 7, 2015 11:09 AM, "Stefan Eissing" <stefan.eiss...@greenbytes.de> wrote: > > Not at my machine to really check the runtime behaviour. Can do that tomorrow. My thoughts so far: > > - the return code was not checked intentionally. Once the 101 intermediate response is sent, there is nothing core can do on the connection. It has been switched to a protocol unknown to core. Failure in the protocol handler won't make it HTTP/1 again. If we do not close the connection afterwards, but try to read the next h1 request, we just expose ourselves to someone inserting payload that tricks us. Think about CORS restrictions and other stuff. +1. httpd et al need to ensure an upgrade is possible before sending 101, not after. > - I think its the protocol handlers job to deal with any request body. core does not need to do anything here. For h2 upgrade, for example, the switch is only offered for requests without body. That exposes more predictable behaviour to clients than ignoring the upgrade on some arbitrary number of bytes. Other protocols might be fine with bodies, h2 is not. This was a consensus on the http wg mailing list some months ago. By "core doesn't need to do anything" do you mean that it doesn't need to set up the incoming filter chain? Couldn't the initial request body have been sent with transfer encodings and other transformations? > - moving things to post read sounds tempting, however I'm not sure if we want to upgrade on non-authed request or not, for example. I am not sure what else we do in post read, maybe someone else has an opinion about that. It certainly looks nicer in the OPTIONS * case. WebSocket upgrades rely on authn headers and cookies; there is no (good) way after a connection has been established to say "well, I upgraded you but now I'm closing the connection because you weren't authorized." The check needs to be done before sending 101 (and otherwise a 401/403/4xx needs to be sent instead of the upgrade). --Jacob
