Make the default HTTP server configuration more secure

Make the default HTTP server configuration more secure

Add a mechanism for setting default headers to add to all responses.

By default each the HTTP response now will contain the following headers:

  Content-Type: text/plain; charset=utf-8
  X-Frame-Options: SAMEORIGIN
  X-Content-Type-Options: nosniff
  X-XSS-Protection: 1; mode=block

New cookies created are now 'httpOnly' by default.

BUG=http://dartbug.com/19676
R=ajohnsen@google.com

Committed: https://code.google.com/p/dart/source/detail?r=39118

[Anders Johnsen, 2014-08-08 06:15:45]

Have you tested what this does to pkg/pub?

https://codereview.chromium.org/443373003/diff/1/sdk/lib/io/http_headers.dart
File sdk/lib/io/http_headers.dart (right):

https://codereview.chromium.org/443373003/diff/1/sdk/lib/io/http_headers.dart...
sdk/lib/io/http_headers.dart:256: _headers.clear();
What about all the fields?

Please add test with e.g. 

var headers = ...;
headers.contentLength = 0;
headers.clear();
Expect.equals(headers.contentLength, -1);

https://codereview.chromium.org/443373003/diff/1/sdk/lib/io/http_headers.dart...
sdk/lib/io/http_headers.dart:812: httpOnly = false;
Is this because of spec? Can you add comment?

https://codereview.chromium.org/443373003/diff/1/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/443373003/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:509: defaultHeaders.forEach((name, value) =>
headers.add(name, value));
I'm curious to the overhead of this. Maybe we should have a headers._clone, that
will just copy the private fields (headers.add involves is-checks, to-lower-case
etc).

https://codereview.chromium.org/443373003/diff/1/tests/standalone/io/http_ser...
File tests/standalone/io/http_server_test.dart (right):

https://codereview.chromium.org/443373003/diff/1/tests/standalone/io/http_ser...
tests/standalone/io/http_server_test.dart:12: void testDefaultResponseHeaders()
{
Also test actual changing the headers, including encoding, contentLength and
chunkedEncoding.

Add test of default encoding being UTF8 now and that clearing contentType will
make it Latin1.

[Søren Gjesse, 2014-08-11 14:10:10]

PTAL

pkg and pub tests pass.

https://codereview.chromium.org/443373003/diff/1/sdk/lib/io/http_headers.dart
File sdk/lib/io/http_headers.dart (right):

https://codereview.chromium.org/443373003/diff/1/sdk/lib/io/http_headers.dart...
sdk/lib/io/http_headers.dart:256: _headers.clear();
On 2014/08/08 06:15:45, Anders Johnsen wrote:
> What about all the fields?
> 
> Please add test with e.g. 
> 
> var headers = ...;
> headers.contentLength = 0;
> headers.clear();
> Expect.equals(headers.contentLength, -1);

Done.

https://codereview.chromium.org/443373003/diff/1/sdk/lib/io/http_headers.dart...
sdk/lib/io/http_headers.dart:812: httpOnly = false;
On 2014/08/08 06:15:45, Anders Johnsen wrote:
> Is this because of spec? Can you add comment?

Changed the default back to false, and set it to true in the non-parsing
constructor. Added a comment there.

https://codereview.chromium.org/443373003/diff/1/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/443373003/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:509: defaultHeaders.forEach((name, value) =>
headers.add(name, value));
On 2014/08/08 06:15:45, Anders Johnsen wrote:
> I'm curious to the overhead of this. Maybe we should have a headers._clone,
that
> will just copy the private fields (headers.add involves is-checks,
to-lower-case
> etc).

Don't know. Pass the default header values to the HttpHeaders constructor for
faster cloning instead.

https://codereview.chromium.org/443373003/diff/1/tests/standalone/io/http_ser...
File tests/standalone/io/http_server_test.dart (right):

https://codereview.chromium.org/443373003/diff/1/tests/standalone/io/http_ser...
tests/standalone/io/http_server_test.dart:12: void testDefaultResponseHeaders()
{
On 2014/08/08 06:15:45, Anders Johnsen wrote:
> Also test actual changing the headers, including encoding, contentLength and
> chunkedEncoding.
> 
> Add test of default encoding being UTF8 now and that clearing contentType will
> make it Latin1.

Done.

[Anders Johnsen, 2014-08-12 05:49:13]

LGTM, with a few comments.

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http.dart
File sdk/lib/io/http.dart (right):

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http.dart#new...
sdk/lib/io/http.dart:604: void clear();
clear->clearDefaultHeaders?

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http_headers....
File sdk/lib/io/http_headers.dart (right):

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http_headers....
sdk/lib/io/http_headers.dart:31: _chunkedTransferEncoding =
initialHeaders._chunkedTransferEncoding;
This may be problematic if protocolVersions doesn't match, e.g. wget is used and
enforces 1.0 when chunked is set.

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http_impl.dar...
sdk/lib/io/http_impl.dart:2162: HttpHeaders defaultResponseHeaders;
Change to:

  final HttpHeaders defaultResponseHeaders = _initDefaultResponseHeaders();

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http_impl.dar...
sdk/lib/io/http_impl.dart:2207: _initDefaultResponseHeaders() {
Make this a static function and call it directly in the field initializer.

[Søren Gjesse, 2014-08-12 06:53:49]

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http.dart
File sdk/lib/io/http.dart (right):

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http.dart#new...
sdk/lib/io/http.dart:604: void clear();
On 2014/08/12 05:49:13, Anders Johnsen wrote:
> clear->clearDefaultHeaders?

This works on all headers objects, and clears all, e.g.

  request.response.headers.add('a', 'b');
  request.response.clear();

will leave an empty set of response headers clearing not only whatever default
headers where added, but also the headers added by the user.

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http_headers....
File sdk/lib/io/http_headers.dart (right):

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http_headers....
sdk/lib/io/http_headers.dart:31: _chunkedTransferEncoding =
initialHeaders._chunkedTransferEncoding;
On 2014/08/12 05:49:13, Anders Johnsen wrote:
> This may be problematic if protocolVersions doesn't match, e.g. wget is used
and
> enforces 1.0 when chunked is set.

Good point. Clearing it for 1.0. Anyway setting chunked transfer encoding as a
default header is also asking for trouble...

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http_impl.dar...
sdk/lib/io/http_impl.dart:2162: HttpHeaders defaultResponseHeaders;
On 2014/08/12 05:49:13, Anders Johnsen wrote:
> Change to:
> 
>   final HttpHeaders defaultResponseHeaders = _initDefaultResponseHeaders();

Done.

https://codereview.chromium.org/443373003/diff/40001/sdk/lib/io/http_impl.dar...
sdk/lib/io/http_impl.dart:2207: _initDefaultResponseHeaders() {
On 2014/08/12 05:49:13, Anders Johnsen wrote:
> Make this a static function and call it directly in the field initializer.

Done.

[Søren Gjesse, 2014-08-12 07:12:13]

Committed patchset #4 manually as 39118 (presubmit successful).