206 Partial Content requests raise/crash ERR_HTTP_TRAILER_INVALID
menelike opened this issue · 10 comments
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.
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?
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
- hit https://github.com/VeliovGroup/Meteor-Files/blob/master/server.js#L1729
- and then https://github.com/VeliovGroup/Meteor-Files/blob/master/server.js#L1742
- 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.
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'
}
@jankapunkt If you're not able to reproduce this, I'll make a repro by tomorrow.
Thanks a lot I will check this out against our current builds
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.
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.
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.
Wow really great work @menelike
@menelike @jankapunkt thank you guys for investigation.
- Fixed in the latest release;
- Added memo into FAQ section