Fix a number of issues with the Range header handling for serving files

pkg/http_server/lib/src/virtual_directory.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 part of http_server;
 
 
 // Used for signal a directory redirecting, where a tailing slash is missing.
 class _DirectoryRedirect {
   const _DirectoryRedirect();
@@ skipping 192 matching lines @@
           !lastModified.isAfter(request.headers.ifModifiedSince)) {
         response.statusCode = HttpStatus.NOT_MODIFIED;
         response.close();
         return null;
       }
 
       response.headers.set(HttpHeaders.LAST_MODIFIED, lastModified);
       response.headers.set(HttpHeaders.ACCEPT_RANGES, "bytes");
 
       if (request.method == 'HEAD') {
         response.close();

[kustermann, 2014/11/14 13:25:26]

Maybe set the content-length header in case of HEAD requests, so people can
figure out how big the file is?

[Søren Gjesse, 2014/11/14 15:27:29]

Yes, done. Also added it for serving the whole file - as that was also missing.

Moved the HEAD check to just before serving content to get all the headers.

-        return null;
+        return;
       }
 
       return file.length().then((length) {
-        String range = request.headers.value("range");
+        String range = request.headers.value(HttpHeaders.RANGE);
         if (range != null) {

[Anders Johnsen, 2014/11/13 16:12:31]

This range handling is getting quite complicated. Perhaps we should pull it out
to a separate method, that e.g. returns 'false' if it it didn't end up sending
anything.

[Søren Gjesse, 2014/11/14 15:27:29]

I will postpone this to a separate CL, as I will also add support for multiple
ranges.

           // We only support one range, where the standard support several.
           Match matches = new RegExp(r"^bytes=(\d*)\-(\d*)$").firstMatch(range);
           // If the range header have the right format, handle it.
-          if (matches != null) {
+          if (matches != null &&
+              (matches[1].isNotEmpty || matches[2].isNotEmpty)) {
             // Serve sub-range.
-            int start;
-            int end;
+            int start;  // First byte position - inclusive.
+            int end;  // Last byte position - inclusive.
             if (matches[1].isEmpty) {
-              start = matches[2].isEmpty ?
-                  length :
-                  length - int.parse(matches[2]);
-              end = length;
+              start = length - int.parse(matches[2]);
+              if (start < 0) start = 0;
+              end = length - 1;
             } else {
               start = int.parse(matches[1]);
-              end = matches[2].isEmpty ? length : int.parse(matches[2]) + 1;
+              end = matches[2].isEmpty ? length - 1: int.parse(matches[2]);
             }
+            // If the range is syntactically invalid the Range header
+            // MUST be ignored (RFC 2616 section 14.35.1).
+            if (start <= end) {
+              if (end > length) {

[kustermann, 2014/11/14 13:25:26]

Since end is inclusive, shouldn't this be "end >= length" instead of "end >
length"?

[Søren Gjesse, 2014/11/14 15:27:29]

Yes. Good catch. Added additional test.

+                end = length - 1;
+              }
 
-            // Override Content-Length with the actual bytes sent.
-            response.headers.set(HttpHeaders.CONTENT_LENGTH, end - start);
+              if (start >= length) {
+                response.statusCode =
+                    HttpStatus.REQUESTED_RANGE_NOT_SATISFIABLE;
+                response.close();
+                return;

[kustermann, 2014/11/14 13:25:27]

Maybe use cascades:

response
    ..statusCode = 
    ..close();

[Søren Gjesse, 2014/11/14 15:27:29]

Done.

+              }
 
-            // Set 'Partial Content' status code.
-            response.statusCode = HttpStatus.PARTIAL_CONTENT;
-            response.headers.set(HttpHeaders.CONTENT_RANGE,
-                                 "bytes $start-${end - 1}/$length");
+              // Override Content-Length with the actual bytes sent.
+              response.headers.set(HttpHeaders.CONTENT_LENGTH, end - start + 1);
 
-            // Pipe the 'range' of the file.
-            file.openRead(start, end)
-                .pipe(new _VirtualDirectoryFileStream(response, file.path))
-                .catchError((_) {
-                  // TODO(kevmoo): log errors
-                });
-            return;
+              // Set 'Partial Content' status code.
+              response.statusCode = HttpStatus.PARTIAL_CONTENT;
+              response.headers.set(HttpHeaders.CONTENT_RANGE,
+                                   "bytes $start-$end/$length");

[kustermann, 2014/11/14 13:25:27]

Cascades?

[Søren Gjesse, 2014/11/14 15:27:29]

Done.

+
+              // Pipe the 'range' of the file.
+              file.openRead(start, end + 1)
+                  .pipe(new _VirtualDirectoryFileStream(response, file.path))
+                  .catchError((_) {
+                    // TODO(kevmoo): log errors
+                  });
+              return;
+            }
           }
         }
 
         file.openRead()
             .pipe(new _VirtualDirectoryFileStream(response, file.path))
             .catchError((_) {
               // TODO(kevmoo): log errors
             });
       });
     }).catchError((_) {
@@ skipping 182 matching lines @@
 
   Future close() => new Future.value();
 
   void setMimeType(List<int> bytes) {
     var mimeType = lookupMimeType(path, headerBytes: bytes);
     if (mimeType != null) {
       response.headers.contentType = ContentType.parse(mimeType);
     }
   }
 }