206 Partial Content requests raise/crash ERR_HTTP_TRAILER_INVALID

[menelike]

When piping audio files from S3 through Meteor-Files with interceptDownload() and serve() the server crashed on some files with ERR_HTTP_TRAILER_INVALID from https://github.com/nodejs/node/blob/d01a06a916efd30844e1e0a38e79dc0054fc4451/lib/_http_outgoing.js#L458-L460 (tested on node 12.6.1).

I think the reason for this is that on Status code 206 both Content-Range and Transfer-Encoding are set, and if I am not mistaken they conflict. If I understand the specs correctly those are not allowed to be used together:

• https://developer.mozilla.org/en-US/docs/Web/HTTP/Range_requests#Comparison_to_chunked_Transfer-Encoding
• https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Trailer

        case '206':
          headers.Pragma               = 'private';
          headers.Trailer              = 'expires';
          headers['Transfer-Encoding'] = 'chunked';
          break;

https://github.com/VeliovGroup/Meteor-Files/blob/master/server.js#L242-L246

if (!http.response.headersSent) {
        http.response.setHeader('Content-Range', `bytes ${reqRange.start}-${reqRange.end}/${vRef.size}`);
      }

https://github.com/VeliovGroup/Meteor-Files/blob/master/server.js#L1840

My knowledge of HTTP headers is limited, hopefully, this gives you some clues @dr-dimitru .

My current workaround is to pass my own responseHeaders() without the case 206 part.

[jankapunkt]

That's interesting, we basically stream audio content from GridFS via 206 and the slightlycustomized interceptDownload example from the wiki (customized for GridFSBucket) and we never ran into any of these errors. Not downgrading your issue, rather I am aware that Murphy's law will hit us, too.

Any hint how to reproduce this?

[menelike]

You must have a request including byte-ranges. This can be tested with an mp3 file (tested with a 3MB one)

The flag range is important here.

This should then

1. hit https://github.com/VeliovGroup/Meteor-Files/blob/master/server.js#L1729
2. and then https://github.com/VeliovGroup/Meteor-Files/blob/master/server.js#L1742
3. and in the end https://github.com/VeliovGroup/Meteor-Files/blob/master/server.js#L1840

The response should typically look like:

I observed this locally with minio and did not test this against AWS S3, but I doubt that this should change the result.

The second and all upcoming requests look like this then, notice that the range increases:

Though you won't see this as the server crashes on the first request.

[menelike]

Hope this helps, I reproduced the crash with this:

curl https://your.server/cdn/storage/ServiceFiles/kEXcy7pEbHw56v32E/stream/kEXcy7pEbHw56v32E.mp3 -i -H "Range: bytes=0-" --cookie "x_mtok=foo" --output foo.mp3

=> Meteor server restarted_http_outgoing.js:444
    throw new ERR_HTTP_TRAILER_INVALID();
    ^

Error [ERR_HTTP_TRAILER_INVALID] [ERR_HTTP_TRAILER_INVALID]: Trailers are invalid with this transfer encoding
    at ServerResponse._storeHeader (_http_outgoing.js:444:11)
    at ServerResponse.writeHead (_http_server.js:312:8)
    at Array.writeStatusCode (/Users/human/.meteor/packages/meteor-tool/.1.10.2.mlbtbh.xl9i++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/http-proxy/lib/http-proxy/passes/web-outgoing.js:132:11)
    at ClientRequest.<anonymous> (/Users/human/.meteor/packages/meteor-tool/.1.10.2.mlbtbh.xl9i++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/http-proxy/lib/http-proxy/passes/web-incoming.js:166:20)
    at ClientRequest.emit (events.js:311:20)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (_http_client.js:603:27)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:119:17)
    at Socket.socketOnData (_http_client.js:476:22)
    at Socket.emit (events.js:311:20)
    at addChunk (_stream_readable.js:294:12)
    at readableAddChunk (_stream_readable.js:275:11)
    at Socket.Readable.push (_stream_readable.js:209:10)
    at TCP.onStreamRead (internal/stream_base_commons.js:186:23) {
  code: 'ERR_HTTP_TRAILER_INVALID'
}

[menelike]

@jankapunkt If you're not able to reproduce this, I'll make a repro by tomorrow.

[jankapunkt]

Thanks a lot I will check this out against our current builds

[menelike]

Small update, I am currently working on a reproduction, so far I could not get https://github.com/VeliovGroup/Meteor-Files-Demos/tree/master/demo-simplest-streaming to crash.

[menelike]

This seems to be caused in conjunction with Nginx, this comment helped a lot. I added proxy_http_version 1.1; to my location block like this:

    location /cdn/ {
        client_body_buffer_size 128k;

        proxy_pass http://app;
        proxy_redirect      off;
        proxy_set_header    Host              $host;
        proxy_set_header    X-Real-IP         $remote_addr;
        proxy_set_header    X-Forwarded-For   $proxy_add_x_forwarded_for;
        proxy_set_header    X-Forwarded-Proto $scheme;
        proxy_http_version 1.1;
    }

and the crash is gone. Nginx uses proxy_http_version 1.0; per default.

I still try to figure out what exactly happens here. This is especially confusing as https://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html e.g. HTTP 1.1 introduced Chunked Transfer Coding and using HTTP 1.0 lets it crash, but 1.1 doesn't. It should be the other way around.

[menelike]

@dr-dimitru @jankapunkt

I am finally able to reproduce this with https://github.com/VeliovGroup/Meteor-Files/tree/master/demo-simplest-streaming. Run that project, grab the mp3 URL, and run the following (--http1.0 is important here!):

curl http://localhost:3001/cdn/storage/Sounds/foo/original/foo.mp3 -i -H "Range: bytes=0-500" -v --http1.0 > /dev/null => 💥

while

curl http://localhost:3001/cdn/storage/Sounds/foo/original/foo.mp3 -i -H "Range: bytes=0-500" -v --http1.1 > /dev/null does not fail.

This should mean that we can DOS attack Meteor-Files Servers now 🚨. Though I could not crash https://files.veliov.com/ as it enforces HTTP1.1 which modern web proxies should always do nowadays 😅

I think that https://github.com/VeliovGroup/Meteor-Files/blob/master/server.js#L242-L246 needs to cover https://github.com/VeliovGroup/Meteor-Files/blob/master/server.js#L1840 as well and set one of those headers, depending on the request e.g. if range requested or not, or if it is an HTTP1 or HTTP1.1 request.

[jankapunkt]

Wow really great work @menelike