Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

Handle response content-length mis-match errors #175

Closed
Tratcher opened this issue Aug 26, 2015 · 9 comments
Closed

Handle response content-length mis-match errors #175

Tratcher opened this issue Aug 26, 2015 · 9 comments
Assignees

Comments

@Tratcher
Copy link
Member

If the application sets the content-length response header and then does not write that exact number of bytes then this corrupts the connection state. There are three scenarios:
A) The application writes more bytes than specified. The client will truncate at the specified content-length and then fail trying to parse the remainder as the start of the next response. The client should then abort the connection.
B) The applications writes fewer bytes than specified. The client will hang waiting for the remaining data. Eventually it should time out and disconnect.
C) The application specifies a non-zero content length but doesn't write any data. The client will hang just as in (B)

For (A) it would be nice to track the content length and throw from Write if too many bytes are written. The connection should be aborted.

For (B) we need to track the content length and disconnect if the response ends without writing enough data. There's no way to raise this error to the application except in the logs. This scenario should be treated the same as if there were an unhandled exception after the response body had started.

(C) we could detect even without tracking content written because we know if the response has started or not. The response should be changed to a 500 internal server error and logged as an error.

@muratg
Copy link
Contributor

muratg commented Dec 21, 2015

@lodejard What do you think about this one?

@cesarblum
Copy link
Contributor

I'll chime in. It's a nice feature but I wonder if it's worth the added complexity to the code.

  • Why/when would any of those 3 scenarios take place?
  • How often are them expected to occur?

All 3 scenarios manifest their symptoms in the client, which would eventually lead to an investigation on the server side. Unless Kestrel itself could be the source of these issues, I'd say it's up to the application to ensure it is sending the right data.

@Tratcher
Copy link
Member Author

Application developers make mistakes. This is more about how readily can they diagnose those mistakes. B and C are difficult to diagnose as they manifest as hangs on the client side (or data corruption in a pipelining scenario). The average browser has built in mitigations for A where they just drain any remaining data.

@muratg
Copy link
Contributor

muratg commented Jan 21, 2016

Moving this to Backlog as we will be in RC2 ask mode very soon. If you feel strongly about this issue, please ping me.

@muratg muratg added this to the Backlog milestone Jan 21, 2016
@lodejard
Copy link
Contributor

Catching up on notification backlog - we talked about this one the other day actually. This could be part of a middleware in the diagnostics package, actually, where response length and numerous other "application correctness" checks could be done by wrapping many features of the http request/response

@cesarblum
Copy link
Contributor

@lodejard I like that idea. I don't think Kestrel should be trying to account for potential application mistakes. It would make every correct application pay a price for whatever applications out there that are not doing the right thing regarding Content-Length.

@Tratcher
Copy link
Member Author

Kestrel is ultimately responsible for implementing the HTTP protocol and maintaining the connection integrity. It's one thing to recommend diagnostic middleware to help developers interactively debug application issues. It's another to let the connection state become corrupt.

@muratg muratg removed this from the Backlog milestone Sep 7, 2016
@muratg
Copy link
Contributor

muratg commented Sep 7, 2016

Removing from the backlog to discuss this in the next triage. We may need to have this for the edge server scenario.

@rynowak
Copy link
Member

rynowak commented Sep 7, 2016

I'm going to pop up again and say that I'd love it if our streams implemented Position without throwing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants