From 6c02f928dadb62dcaf4bb44fa1f0b599ada6db0e Mon Sep 17 00:00:00 2001 From: Gerrard Lindsay Date: Sat, 13 May 2023 13:09:26 -0400 Subject: [PATCH] http: prevent writing to the body when not allowed by HTTP spec PR-URL: https://github.com/nodejs/node/pull/47732 Reviewed-By: Robert Nagy Reviewed-By: Paolo Insogna Reviewed-By: Marco Ippolito Reviewed-By: Matteo Collina --- doc/api/errors.md | 5 + doc/api/http.md | 7 +- lib/_http_outgoing.js | 21 ++-- lib/_http_server.js | 14 ++- lib/http.js | 1 + lib/internal/errors.js | 2 + ...test-http-head-response-has-no-body-end.js | 2 +- ...-http-head-throw-on-response-body-write.js | 102 ++++++++++++++++++ 8 files changed, 142 insertions(+), 12 deletions(-) create mode 100644 test/parallel/test-http-head-throw-on-response-body-write.js diff --git a/doc/api/errors.md b/doc/api/errors.md index d5fa7f43005ad8..1f259dccade7f8 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1331,6 +1331,11 @@ When using [`fs.cp()`][], `src` or `dest` pointed to an invalid path. +### `ERR_HTTP_BODY_NOT_ALLOWED` + +An error is thrown when writing to an HTTP response which does not allow +contents. + ### `ERR_HTTP_CONTENT_LENGTH_MISMATCH` Response body size doesn't match with the specified content-length header value. diff --git a/doc/api/http.md b/doc/api/http.md index 1ff58b61fa339c..07194ca90110d3 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -2129,9 +2129,10 @@ it will switch to implicit header mode and flush the implicit headers. This sends a chunk of the response body. This method may be called multiple times to provide successive parts of the body. -In the `node:http` module, the response body is omitted when the -request is a HEAD request. Similarly, the `204` and `304` responses -_must not_ include a message body. +Writing to the body is not allowed when the request method or response status +do not support content. If an attempt is made to write to the body for a +HEAD request or as part of a `204` or `304`response, a synchronous `Error` +with the code `ERR_HTTP_BODY_NOT_ALLOWED` is thrown. `chunk` can be a string or a buffer. If `chunk` is a string, the second parameter specifies how to encode it into a byte stream. diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 6f42a1ccd4b8be..ce875a9c92066e 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -61,6 +61,7 @@ const { ERR_HTTP_HEADERS_SENT, ERR_HTTP_INVALID_HEADER_VALUE, ERR_HTTP_TRAILER_INVALID, + ERR_HTTP_BODY_NOT_ALLOWED, ERR_INVALID_HTTP_TOKEN, ERR_INVALID_ARG_TYPE, ERR_INVALID_ARG_VALUE, @@ -86,6 +87,7 @@ const kUniqueHeaders = Symbol('kUniqueHeaders'); const kBytesWritten = Symbol('kBytesWritten'); const kErrored = Symbol('errored'); const kHighWaterMark = Symbol('kHighWaterMark'); +const kRejectNonStandardBodyWrites = Symbol('kRejectNonStandardBodyWrites'); const nop = () => {}; @@ -151,6 +153,7 @@ function OutgoingMessage(options) { this[kErrored] = null; this[kHighWaterMark] = options?.highWaterMark ?? getDefaultHighWaterMark(); + this[kRejectNonStandardBodyWrites] = options?.rejectNonStandardBodyWrites ?? false; } ObjectSetPrototypeOf(OutgoingMessage.prototype, Stream.prototype); ObjectSetPrototypeOf(OutgoingMessage, Stream); @@ -880,6 +883,17 @@ function write_(msg, chunk, encoding, callback, fromEnd) { err = new ERR_STREAM_DESTROYED('write'); } + if (!msg._hasBody) { + if (msg[kRejectNonStandardBodyWrites]) { + throw new ERR_HTTP_BODY_NOT_ALLOWED(); + } else { + debug('This type of response MUST NOT have a body. ' + + 'Ignoring write() calls.'); + process.nextTick(callback); + return true; + } + } + if (err) { if (!msg.destroyed) { onError(msg, err, callback); @@ -912,13 +926,6 @@ function write_(msg, chunk, encoding, callback, fromEnd) { msg._implicitHeader(); } - if (!msg._hasBody) { - debug('This type of response MUST NOT have a body. ' + - 'Ignoring write() calls.'); - process.nextTick(callback); - return true; - } - if (!fromEnd && msg.socket && !msg.socket.writableCorked) { msg.socket.cork(); process.nextTick(connectionCorkNT, msg.socket); diff --git a/lib/_http_server.js b/lib/_http_server.js index b38e6cb1ab9e17..c010d8fa96a1c2 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -483,6 +483,14 @@ function storeHTTPOptions(options) { validateBoolean(joinDuplicateHeaders, 'options.joinDuplicateHeaders'); } this.joinDuplicateHeaders = joinDuplicateHeaders; + + const rejectNonStandardBodyWrites = options.rejectNonStandardBodyWrites; + if (rejectNonStandardBodyWrites !== undefined) { + validateBoolean(rejectNonStandardBodyWrites, 'options.rejectNonStandardBodyWrites'); + this.rejectNonStandardBodyWrites = rejectNonStandardBodyWrites; + } else { + this.rejectNonStandardBodyWrites = false; + } } function setupConnectionsTracking(server) { @@ -1018,7 +1026,11 @@ function parserOnIncoming(server, socket, state, req, keepAlive) { } } - const res = new server[kServerResponse](req, { highWaterMark: socket.writableHighWaterMark }); + const res = new server[kServerResponse](req, + { + highWaterMark: socket.writableHighWaterMark, + rejectNonStandardBodyWrites: server.rejectNonStandardBodyWrites, + }); res._keepAliveTimeout = server.keepAliveTimeout; res._maxRequestsPerSocket = server.maxRequestsPerSocket; res._onPendingData = updateOutgoingData.bind(undefined, diff --git a/lib/http.js b/lib/http.js index b4ff4fc88f2d68..9a7b944d302d3f 100644 --- a/lib/http.js +++ b/lib/http.js @@ -54,6 +54,7 @@ let maxHeaderSize; * maxHeaderSize?: number; * joinDuplicateHeaders?: boolean; * highWaterMark?: number; + * rejectNonStandardBodyWrites?: boolean; * }} [opts] * @param {Function} [requestListener] * @returns {Server} diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 9c9eae6848b76f..19422047360d24 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1154,6 +1154,8 @@ E('ERR_HTTP2_TRAILERS_NOT_READY', 'Trailing headers cannot be sent until after the wantTrailers event is ' + 'emitted', Error); E('ERR_HTTP2_UNSUPPORTED_PROTOCOL', 'protocol "%s" is unsupported.', Error); +E('ERR_HTTP_BODY_NOT_ALLOWED', + 'Adding content for this request method or response status is not allowed.', Error); E('ERR_HTTP_CONTENT_LENGTH_MISMATCH', 'Response body\'s content-length of %s byte(s) does not match the content-length of %s byte(s) set in header', Error); E('ERR_HTTP_HEADERS_SENT', diff --git a/test/parallel/test-http-head-response-has-no-body-end.js b/test/parallel/test-http-head-response-has-no-body-end.js index 824a1bafe3a5e3..3e0e6cec240ba8 100644 --- a/test/parallel/test-http-head-response-has-no-body-end.js +++ b/test/parallel/test-http-head-response-has-no-body-end.js @@ -29,7 +29,7 @@ const http = require('http'); const server = http.createServer(function(req, res) { res.writeHead(200); - res.end('FAIL'); // broken: sends FAIL from hot path. + res.end(); }); server.listen(0); diff --git a/test/parallel/test-http-head-throw-on-response-body-write.js b/test/parallel/test-http-head-throw-on-response-body-write.js new file mode 100644 index 00000000000000..7352b20d84df6e --- /dev/null +++ b/test/parallel/test-http-head-throw-on-response-body-write.js @@ -0,0 +1,102 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +{ + const server = http.createServer((req, res) => { + res.writeHead(200); + res.end('this is content'); + }); + server.listen(0); + + server.on('listening', common.mustCall(function() { + const req = http.request({ + port: this.address().port, + method: 'HEAD', + path: '/' + }, common.mustCall((res) => { + res.resume(); + res.on('end', common.mustCall(function() { + server.close(); + })); + })); + req.end(); + })); +} + +{ + const server = http.createServer({ + rejectNonStandardBodyWrites: true, + }, (req, res) => { + res.writeHead(204); + assert.throws(() => { + res.write('this is content'); + }, { + code: 'ERR_HTTP_BODY_NOT_ALLOWED', + name: 'Error', + message: 'Adding content for this request method or response status is not allowed.' + }); + res.end(); + }); + server.listen(0); + + server.on('listening', common.mustCall(function() { + const req = http.request({ + port: this.address().port, + method: 'GET', + path: '/' + }, common.mustCall((res) => { + res.resume(); + res.on('end', common.mustCall(function() { + server.close(); + })); + })); + req.end(); + })); +} + +{ + const server = http.createServer({ + rejectNonStandardBodyWrites: false, + }, (req, res) => { + res.writeHead(200); + res.end('this is content'); + }); + server.listen(0); + + server.on('listening', common.mustCall(function() { + const req = http.request({ + port: this.address().port, + method: 'HEAD', + path: '/' + }, common.mustCall((res) => { + res.resume(); + res.on('end', common.mustCall(function() { + server.close(); + })); + })); + req.end(); + })); +}