From 12b9ec09b0807a0b362986c80d3c4b9a644c611e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 14 Feb 2018 00:18:18 +0100 Subject: [PATCH] http2: remove regular-file-only restriction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requiring `respondWithFile()` to only work with regular files is an artificial restriction on Node’s side and has become unnecessary. Offsets or lengths cannot be specified for those files, but that is an inherent property of other file types. PR-URL: https://github.com/nodejs/node/pull/18936 Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- doc/api/errors.md | 9 ++- doc/api/http2.md | 10 +++ lib/internal/errors.js | 4 +- lib/internal/http2/core.js | 34 +++++++---- .../test-http2-respond-file-error-dir.js | 2 +- ...st-http2-respond-file-error-pipe-offset.js | 61 +++++++++++++++++++ .../test-http2-respond-file-with-pipe.js | 58 ++++++++++++++++++ 7 files changed, 164 insertions(+), 14 deletions(-) create mode 100644 test/parallel/test-http2-respond-file-error-pipe-offset.js create mode 100644 test/parallel/test-http2-respond-file-with-pipe.js diff --git a/doc/api/errors.md b/doc/api/errors.md index 7d6e238f93b..f5190dc52bf 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -971,7 +971,14 @@ client. ### ERR_HTTP2_SEND_FILE An attempt was made to use the `Http2Stream.prototype.responseWithFile()` API to -send something other than a regular file. +send a directory. + + +### ERR_HTTP2_SEND_FILE_NOSEEK + +An attempt was made to use the `Http2Stream.prototype.responseWithFile()` API to +send something other than a regular file, but `offset` or `length` options were +provided. ### ERR_HTTP2_SESSION_ERROR diff --git a/doc/api/http2.md b/doc/api/http2.md index 94f25377d0a..13aa4d1e210 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -1223,6 +1223,11 @@ if the `getTrailers` callback attempts to set such header fields. #### http2stream.respondWithFD(fd[, headers[, options]]) * `fd` {number} A readable file descriptor. @@ -1313,6 +1318,11 @@ if the `getTrailers` callback attempts to set such header fields. #### http2stream.respondWithFile(path[, headers[, options]]) * `path` {string|Buffer|URL} diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 8ec72dd647b..3d0b6cf9b39 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -714,7 +714,9 @@ E('ERR_HTTP2_PING_LENGTH', 'HTTP2 ping payload must be 8 bytes', RangeError); E('ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED', 'Cannot set HTTP/2 pseudo-headers', Error); E('ERR_HTTP2_PUSH_DISABLED', 'HTTP/2 client has disabled push streams', Error); -E('ERR_HTTP2_SEND_FILE', 'Only regular files can be sent', Error); +E('ERR_HTTP2_SEND_FILE', 'Directories cannot be sent', Error); +E('ERR_HTTP2_SEND_FILE_NOSEEK', + 'Offset or length can only be specified for regular files', Error); E('ERR_HTTP2_SESSION_ERROR', 'Session closed with error code %s', Error); E('ERR_HTTP2_SOCKET_BOUND', 'The socket is already bound to an Http2Session', Error); diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 607ae3fd2d2..7137e527df0 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -42,6 +42,7 @@ const { ERR_HTTP2_PING_LENGTH, ERR_HTTP2_PUSH_DISABLED, ERR_HTTP2_SEND_FILE, + ERR_HTTP2_SEND_FILE_NOSEEK, ERR_HTTP2_SESSION_ERROR, ERR_HTTP2_SETTINGS_CANCEL, ERR_HTTP2_SOCKET_BOUND, @@ -2045,12 +2046,21 @@ function doSendFileFD(session, options, fd, headers, streamOptions, err, stat) { } if (!stat.isFile()) { - const err = new ERR_HTTP2_SEND_FILE(); - if (onError) - onError(err); - else - this.destroy(err); - return; + const isDirectory = stat.isDirectory(); + if (options.offset !== undefined || options.offset > 0 || + options.length !== undefined || options.length >= 0 || + isDirectory) { + const err = isDirectory ? + new ERR_HTTP2_SEND_FILE() : new ERR_HTTP2_SEND_FILE_NOSEEK(); + if (onError) + onError(err); + else + this.destroy(err); + return; + } + + options.offset = -1; + options.length = -1; } if (this.destroyed || this.closed) { @@ -2075,12 +2085,14 @@ function doSendFileFD(session, options, fd, headers, streamOptions, err, stat) { return; } - statOptions.length = - statOptions.length < 0 ? stat.size - (+statOptions.offset) : - Math.min(stat.size - (+statOptions.offset), - statOptions.length); + if (stat.isFile()) { + statOptions.length = + statOptions.length < 0 ? stat.size - (+statOptions.offset) : + Math.min(stat.size - (+statOptions.offset), + statOptions.length); - headers[HTTP2_HEADER_CONTENT_LENGTH] = statOptions.length; + headers[HTTP2_HEADER_CONTENT_LENGTH] = statOptions.length; + } processRespondWithFD(this, fd, headers, options.offset | 0, diff --git a/test/parallel/test-http2-respond-file-error-dir.js b/test/parallel/test-http2-respond-file-error-dir.js index 6818616227d..24a6d2dc965 100644 --- a/test/parallel/test-http2-respond-file-error-dir.js +++ b/test/parallel/test-http2-respond-file-error-dir.js @@ -15,7 +15,7 @@ server.on('stream', (stream) => { common.expectsError({ code: 'ERR_HTTP2_SEND_FILE', type: Error, - message: 'Only regular files can be sent' + message: 'Directories cannot be sent' })(err); stream.respond({ ':status': 404 }); diff --git a/test/parallel/test-http2-respond-file-error-pipe-offset.js b/test/parallel/test-http2-respond-file-error-pipe-offset.js new file mode 100644 index 00000000000..eb782e2dea6 --- /dev/null +++ b/test/parallel/test-http2-respond-file-error-pipe-offset.js @@ -0,0 +1,61 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +if (common.isWindows) + common.skip('no mkfifo on Windows'); +const child_process = require('child_process'); +const path = require('path'); +const fs = require('fs'); +const http2 = require('http2'); +const assert = require('assert'); + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +const pipeName = path.join(tmpdir.path, 'pipe'); + +const mkfifo = child_process.spawnSync('mkfifo', [ pipeName ]); +if (mkfifo.error && mkfifo.error.code === 'ENOENT') { + common.skip('missing mkfifo'); +} + +process.on('exit', () => fs.unlinkSync(pipeName)); + +const server = http2.createServer(); +server.on('stream', (stream) => { + stream.respondWithFile(pipeName, { + 'content-type': 'text/plain' + }, { + offset: 10, + onError(err) { + common.expectsError({ + code: 'ERR_HTTP2_SEND_FILE_NOSEEK', + type: Error, + message: 'Offset or length can only be specified for regular files' + })(err); + + stream.respond({ ':status': 404 }); + stream.end(); + }, + statCheck: common.mustNotCall() + }); +}); +server.listen(0, () => { + + const client = http2.connect(`http://localhost:${server.address().port}`); + const req = client.request(); + + req.on('response', common.mustCall((headers) => { + assert.strictEqual(headers[':status'], 404); + })); + req.on('data', common.mustNotCall()); + req.on('end', common.mustCall(() => { + client.close(); + server.close(); + })); + req.end(); +}); + +fs.writeFile(pipeName, 'Hello, world!\n', common.mustCall()); diff --git a/test/parallel/test-http2-respond-file-with-pipe.js b/test/parallel/test-http2-respond-file-with-pipe.js new file mode 100644 index 00000000000..2b7ca3fc9a5 --- /dev/null +++ b/test/parallel/test-http2-respond-file-with-pipe.js @@ -0,0 +1,58 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +if (common.isWindows) + common.skip('no mkfifo on Windows'); +const child_process = require('child_process'); +const path = require('path'); +const fs = require('fs'); +const http2 = require('http2'); +const assert = require('assert'); + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +const pipeName = path.join(tmpdir.path, 'pipe'); + +const mkfifo = child_process.spawnSync('mkfifo', [ pipeName ]); +if (mkfifo.error && mkfifo.error.code === 'ENOENT') { + common.skip('missing mkfifo'); +} + +process.on('exit', () => fs.unlinkSync(pipeName)); + +const server = http2.createServer(); +server.on('stream', (stream) => { + stream.respondWithFile(pipeName, { + 'content-type': 'text/plain' + }, { + onError: common.mustNotCall(), + statCheck: common.mustCall() + }); +}); + +server.listen(0, () => { + const client = http2.connect(`http://localhost:${server.address().port}`); + const req = client.request(); + + req.on('response', common.mustCall((headers) => { + assert.strictEqual(headers[':status'], 200); + })); + let body = ''; + req.setEncoding('utf8'); + req.on('data', (chunk) => body += chunk); + req.on('end', common.mustCall(() => { + assert.strictEqual(body, 'Hello, world!\n'); + client.close(); + server.close(); + })); + req.end(); +}); + +fs.open(pipeName, 'w', common.mustCall((err, fd) => { + assert.ifError(err); + fs.writeSync(fd, 'Hello, world!\n'); + fs.closeSync(fd); +}));