dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.08k stars 1.56k forks source link

dart:io HttpServer allows serving a 304 response with a body #26526

Open munificent opened 8 years ago

munificent commented 8 years ago

The HTTP spec says 304 responses must not include a body, not even an empty one:

For response messages, whether or not a message-body is included with a message is dependent on both the request method and the response status code (section 6.1.1). All responses to the HEAD request method MUST NOT include a message-body, even though the presence of entity-header fields might lead one to believe they do. All 1xx (informational), 204 (no content), and 304 (not modified) responses MUST NOT include a message-body. All other responses do include a message-body, although it MAY be of zero length.

But dart:io will serve an empty body if you give it an empty stream:

If I run this:

import 'dart:async';
import 'dart:io';

main() async {
  var requestServer =
      await HttpServer.bind(InternetAddress.LOOPBACK_IP_V4, 4040);
  await for (HttpRequest request in requestServer) {
    request.response.headers.chunkedTransferEncoding = true;
    request.response.statusCode = 304;
    await request.response.addStream(new Stream.empty());
    request.response.close();
  }
}

And then this:

$ printf "GET /index.html HTTP/1.1\n\n" | nc localhost 4040

I see:

HTTP/1.1 304 Not Modified
content-type: text/plain; charset=utf-8
x-frame-options: SAMEORIGIN
x-xss-protection: 1; mode=block
transfer-encoding: chunked
x-content-type-options: nosniff

0

The "\n0\n" is wrong. It's an empty chunked body. It should just be:

HTTP/1.1 304 Not Modified
content-type: text/plain; charset=utf-8
x-frame-options: SAMEORIGIN
x-xss-protection: 1; mode=block
transfer-encoding: chunked
x-content-type-options: nosniff
sgjesse commented 8 years ago

In dart:io there is no checking of the consistency between the status code and whether a body is allowed or not. The only exception to this is that for a HEAD request any body added to the response is silently discarded (that way the normal serving mechanism can be used for HEAD requests).

In the provided example chunked transfer encoding is set in the header (which is also implicit of content length is not provided). In that case the chunk end must be sent for the receiving parser to be able to parse the response.

I would argue that this response:

HTTP/1.1 304 Not Modified
transfer-encoding: chunked

0

is a response with no message-body. It is chunked encoded but it is empty.

Explicitly setting the content length to 0 in the code will have the desired effect:

import 'dart:async';
import 'dart:io';

main() async {
  var requestServer =
      await HttpServer.bind(InternetAddress.LOOPBACK_IP_V4, 4040);
  await for (HttpRequest request in requestServer) {
    request.response.contentLength = 0;
    request.response.statusCode = 304;
    await request.response.addStream(new Stream.empty());
    request.response.close();
  }
}

produces:

HTTP/1.1 304 Not Modified
content-type: text/plain; charset=utf-8
x-frame-options: SAMEORIGIN
x-xss-protection: 1; mode=block
x-content-type-options: nosniff
content-length: 0

With content length set, the number of bytes added to the response body is checked, and adding the empty stream is (of cause) supported as it generated no body bytes.

If we where to add more logic to the status code for responses we could perhaps set the content length to 0 when a status code which MUST not include a message-body is send. Making these three equivalent:

request.response.statusCode = 304;
await request.response.addStream(new Stream.empty());
request.response.close();
request.response.headers.chunkedTransferEncoding = true;
request.response.statusCode = 304;
await request.response.addStream(new Stream.empty());
request.response.close();
request.response.contentLength = 0;
request.response.statusCode = 304;
await request.response.addStream(new Stream.empty());
request.response.close();

And making these a server side error

request.response.statusCode = 304;
await request.response.addStream(new Stream.fromIterable([[65,66,67]]));
request.response.close();
request.response.contentLength = 3;
request.response.statusCode = 304;
await request.response.addStream(new Stream.fromIterable([[65,66,67]]));
request.response.close();
munificent commented 8 years ago

In the provided example chunked transfer encoding is set in the header (which is also implicit of content length is not provided).

Oh, sorry, I didn't mean to leave that line in. Even without it, the result is the same:

import 'dart:async';
import 'dart:io';

main() async {
  var requestServer =
      await HttpServer.bind(InternetAddress.LOOPBACK_IP_V4, 4040);
  await for (HttpRequest request in requestServer) {
    request.response.statusCode = 304;
    await request.response.addStream(new Stream.empty());
    request.response.close();
  }
}

I guess dart:io defaults to chunked?

The only exception to this is that for a HEAD request any body added to the response is silently discarded

Yes, my intuition is dart:io should do something similar for 304s. The HTTP spec says they must not have a body, not even an empty one. If dart:io serves a 304 with a body, it's not compliant. That may be because the app is misusing the API, but I don't think it makes sense to pass that mistake on to the network. I would expect dart:io to either silently discard the body, or to raise an error if the body is non-empty.

For usability reasons, I'd like it to not complain if I give it an empty body, and instead just safely treat that as no body.

If we where to add more logic to the status code for responses we could perhaps set the content length to 0 when a status code which MUST not include a message-body is send. Making these three equivalent:

...

And making these a server side error

...

Yes, those look exactly like I would expect it to work. That sounds great to me, and it means shelf would do the right things without any modifications. Right now, for a 304, it adds an empty stream body. It would be nice to not have to special case that in shelf.

natebosch commented 7 years ago
HTTP/1.1 304 Not Modified
transfer-encoding: chunked

0

Is not considered spec compliant AFAIK. At least it can cause problems with some proxies which do not expect to get those 5 extra bytes. \r\n\r\n0