Repository navigation
No content length on DELETE and OPTIONS #27880
Description
Activity
/cc @nodejs/http
I don't think node-http-proxy is doing that correctly, methods do not affect the length of a request body in HTTP. (Some methods such as TRACE prohibit request bodies, however, these are considered client errors by the origin server, they are otherwise perfectly parsable messages.) According to HTTP/1.1 the message body length is dependent on:
- The HTTP version;
- if the message is a request or response;
- for the response, the request method and/or response status;
- and several headers that indicate length (Transfer-Encoding and Content-Length).
[edit] See discussion below, it turns out Node.js has behavior inconsistent with HTTP/1.1 messaging.
Reacted by Alex YangOh, I didn't realize Node.js did this.
Setting per-method defaults on what transfer method to use seems like the wrong way to go about this. POST can use or prohibit any transfer method - one endpoint might prohibit a request body and chunked encoding, another might require chunked encoding.
Setting transfer codings (or lack thereof) based on request method isn't specified by HTTP and doesn't solve the general issue.
I wrote a little script to determine and log how requests are being made:
// httpd const http = require('http'); http.createServer(function(req, res){ console.log(req.url); res.setHeader('Content-Type', 'text/plain'); for(var i=0; i<req.rawHeaders.length; i+=2){ res.write('' + req.rawHeaders[i]+': '+req.rawHeaders[i+1]+'\r\n'); } res.end(); return; }).listen(8081, '0.0.0.0');
var http = require('http'); var expectedHeaders = { 'DELETE': ['host', 'connection'], 'GET': ['host', 'connection'], 'HEAD': ['host', 'connection'], 'POST': ['host', 'connection', 'transfer-encoding'], 'PUT': ['host', 'connection', 'transfer-encoding'] }; var expectedMethods = Object.keys(expectedHeaders); var i=0; function next(){ method = expectedMethods[i++]; if(!method) return; console.log(''); console.log(method); var req = http.request({ method: method, port: 8081, }, function(res){ res.pipe(process.stdout); res.on('end', next); res.on('error', console.error) }); // req.removeHeader('Content-Length'); // req.setHeader('Transfer-Encoding', 'chunked'); // req.setHeader('Connection', 'Transfer-Encoding'); req.write('1'); req.end(); req.on('error', console.error); } next();
I can use
curlto make DELETE requests with a body.But to get Node.js to, I have to explicitly set
req.setHeader('Transfer-Encoding', 'chunked');, otherwise it emits an invalid HTTP request, it emits a request without a Transfer-Encoding, without a Content-Length, but writes three bytes to the request body anyways. The server tries to parse this as the start of the next message, and it closes the connection.This seems like a Node.js bug after all.
This is... something. If that DELETE request with a phantom body looks like an HTTP request (e.g.
req.write('PUT / HTTP/1.1\r\n\r\n');), the downstream server won't close the socket, which confirms that Node.js is sending as HTTP headers what is being accepted as message data.This opens the potential for a carefully crafted message to bypass limits on the forwarded HTTP message.
On top of that, it's strange that the Node.js server is accepting this phantom request, even though the first request specified
Connection: closewhich should cause the Node.js server to ignore additional requests.Is Node.js doing anything to handle the
Connection: closeheader?the downstream server won't close the socket, which confirms that Node.js is sending as HTTP headers what is being accepted as message data.
I don't understand can you please clarify? You are writing request headers in the response body in the above example. What's wrong with this
const assert = require('assert'); const http = require('http'); const data = 'PUT / HTTP/1.1\r\n\r\n'; const server = http.createServer(function(req, res) { req.on('data', function(chunk) { assert.strictEqual(chunk, Buffer.from(data)) }); res.setHeader('Content-Type', 'text/plain'); for (let i = 0; i < req.rawHeaders.length; i += 2) { res.write(`${req.rawHeaders[i]}: ${req.rawHeaders[i + 1]}\r\n`); } res.end(); }); server.listen(8080, function() { const req = http.request({ method: 'DELETE', port: 8080 }, function(res) { console.log(res.headers); const chunks = []; res.on('data', function(chunk) { chunks.push(chunk); }); res.on('end', function() { assert.deepStrictEqual( Buffer.concat(chunks), Buffer.from('Host: localhost:8080\r\nConnection: close\r\n') ); }); }); req.write(data); req.end(); });
@awwright referenced this useful document in the other issue, https://httpwg.org/specs/rfc7230.html#message.body.length
- You are writing request headers in the response body in the above exampleNo, what Node.js actually writes appears to the server to be a second request! Since there's no Content-Length header nor a Transfer-Encoding header, in requests this means a zero-length payload. So when the client writes what appears to be the payload, it's actually raw HTTP, and you can do request smuggling. My suggestion is that if the first write on the request is req.write, Node.js should set the transfer coding, regardless of method. The behavior for writing request payloads does not change with method in HTTP.…On Mon, May 27, 2019, 02:12 Luigi Pinca ***@***.***> wrote: the downstream server won't close the socket, which confirms that Node.js is sending as HTTP headers what is being accepted as message data. I don't understand can you please clarify? You are writing request headers in the response body in the above example. What's wrong with this const assert = require('assert');const http = require('http'); const server = http.createServer(function(req, res) { res.setHeader('Content-Type', 'text/plain'); for (let i = 0; i < req.rawHeaders.length; i += 2) { res.write(`${req.rawHeaders[i]}: ${req.rawHeaders[i + 1]}\r\n`); } res.end(); }); server.listen(8080, function() { const req = http.request({ method: 'DELETE', port: 8080 }, function(res) { console.log(res.headers); const chunks = []; res.on('data', function(chunk) { chunks.push(chunk); }); res.on('end', function() { assert.deepStrictEqual( Buffer.concat(chunks), Buffer.from('Host: localhost:8080\r\nConnection: close\r\n') ); }); }); req.write('PUT / HTTP/1.1\r\n\r\n'); req.end(); }); — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#27880?email_source=notifications&email_token=AADK2DIJ5FAORL6R2MPR6RTPXOQYBA5CNFSM4HPWGZ42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWJIF7A#issuecomment-496141052>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADK2DKFIUAWXLKMJ3LDYRLPXOQYBANCNFSM4HPWGZ4Q> .Reacted by Luigi PincaReacted by Darien Maillet Valentine
No, what Node.js actually writes appears to the server to be a second request!
Oh I get it now.
The issue reported by @awwright is bad and should be fixed. We can close this but only because discussion is confusing here. If we close we should open a new issue only for #27880 (comment) and #27880 (comment).
11 remaining items
Is this available for contribution? I would like to look into this.
Reacted by Alex YangReacted by Alex Yang@JayHelton sure.
I'm interested in working on this. I see two action points from the above conversation
- Finish up the smaller changes in this test diff
- Come up with a proposal on how we are going to set the headers like Content-Length, Transfer-Encoding. Based on the above conversation, I think this RFC has all the answers.
- added a commit that references this issue
on Jun 27, 2023 Fixed via nodejs/undici#2305 I believe?
- added a commit that references this issue
on May 12, 2024 - added a commit that references this issue
on May 12, 2024 - added a commit that references this issue
on Jun 20, 2024 - added a commit that references this issue
on Nov 14, 2025 - added a commit that references this issue
on Sep 3, 2026
Looking through http-party/node-http-proxy@8a24a1e and reading through the node code. I do believe we have an unhandled case.
I believe we are missing something here: https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L349
To ensure content-length and transfer-encoding are correct.
Am I on to something or should I drop this?