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?

[lpinca]

@ronag basically with a single http.request() it is possible to trigger multiple 'request' events on the server.

[awwright]

@ronag I'll see if I can write up a clearer issue soon, but the write() behavior is really just a side effect of this issue; that Node.js is setting different default headers for different methods, when it should actually be based on the message payload. Fixing req.write() wouldn't solve the underlying issue.

[ronag]

@awwright: that’s my point. I think you have a fundamentally better understanding. I think you should describe what you think is the problem and best solution.

[ronag]

@awwright: ping?

[awwright]

Uh yes, it's been a while since I've run into this, but I'll try to think more about it here

[jasnell]

Given that it's not clear if there's anything remaining that is actionable here, closing.

[awwright]

@jasnell This is actionable, I just threw together some tests for the behavior here, see #34066

[ronag]

Given #34066 I believe we can consider this as a confirmed bug.

[JayHelton]

Is this available for contribution? I would like to look into this.

[lpinca]

@JayHelton sure.

[98lenvi]

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.

[brc-dd]

Fixed via nodejs/undici#2305 I believe?

[awwright]

If this is fixed then #34066 should pass and be merged in.