No content length on DELETE and OPTIONS

[ronag]

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?

[addaleax]

/cc @nodejs/http

[awwright]

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.

[lpinca]

This is done on purpose, see

• aef0960
• 1df32af

• node/lib/_http_outgoing.js

  Lines 353 to 354 in ac95c2f

  	} else if (!this.useChunkedEncodingByDefault) {
  	this._last = true;

[awwright]

Oh, 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.

[awwright]

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 curl to 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.

[awwright]

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: close which should cause the Node.js server to ignore additional requests.

Is Node.js doing anything to handle the Connection: close header?

[lpinca]

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();
});

[ronag]

@awwright referenced this useful document in the other issue, https://httpwg.org/specs/rfc7230.html#message.body.length

[awwright]

You are writing request headers in the response body in the above example

No, 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> .

[lpinca]

No, what Node.js actually writes appears to the server to be a second request!

Oh I get it now.

[ronag]

@lpinca do we do anything here or close?

[lpinca]

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).

[ronag]

@lpinca I don't actually understand any of this... @awwright would you mind extracting your comments into an easier to follow issue?