Skip to content

Commit

Permalink
Avoiding to set the "Content-Length" header for GET requests (#1460)
Browse files Browse the repository at this point in the history
* Avoiding to set the "Content-Length" header for GET requests

* code formatting
  • Loading branch information
marcellobarile authored and niftylettuce committed Apr 8, 2019
1 parent ed3e235 commit 804c35c
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 1 deletion.
7 changes: 6 additions & 1 deletion src/node/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -951,6 +951,7 @@ Request.prototype._end = function() {
let data = this._data;
const { req } = this;
const { method } = this;
const methodsWithBody = ['PUT', 'POST', 'PATCH'];

This comment has been minimized.

Copy link
@niftylettuce

niftylettuce Apr 8, 2019

Collaborator

There was a bug here, you need to support OPTIONS having a body. Technically any other methods than TRACE can have a body as far as I can tell from https://stackoverflow.com/questions/16339198/which-http-methods-require-a-body. This is why the tests are failing, see test/basic.js:

describe('req.options()', () => {
  it('should allow request body', done => {
    request
      .options(`${uri}/options/echo/body`)
      .send({ foo: 'baz' })
      .end((err, res) => {
        try {
          assert.equal(err, null);
          assert.strictEqual(res.body.foo, 'baz');
          done();
        } catch (err2) {
          done(err2);
        }
      });
  });
});

This comment has been minimized.

Copy link
@niftylettuce

niftylettuce Apr 8, 2019

Collaborator

If no body was set, then Content-Length shouldn't be applied. It should only be applied if there was an actual body. You may need to rework your pull request and re-submit. Thanks!

This comment has been minimized.

Copy link
@marcellobarile

marcellobarile Apr 9, 2019

Author Contributor

oki, thanks for the clarification ;)


this._setTimeouts();

Expand All @@ -970,7 +971,11 @@ Request.prototype._end = function() {
}

// content-length
if (data && !req.getHeader('Content-Length')) {
if (
methodsWithBody.includes(method) &&
data &&
!req.getHeader('Content-Length')
) {
req.setHeader(
'Content-Length',
Buffer.isBuffer(data) ? data.length : Buffer.byteLength(data)
Expand Down
15 changes: 15 additions & 0 deletions test/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,21 @@ describe('request', function() {
});
});

it('GET should not send the content-length header', next => {
request
.get(`${uri}/content-length`)
.send({ foo: 'bar' })
.then(res => {
try {
assert(!res.badRequest);
next();
} catch (err) {
next(err);
}
})
.catch(next);
});

it('get()', next => {
request.get(`${uri}/notfound`).end((err, res) => {
try {
Expand Down
12 changes: 12 additions & 0 deletions test/support/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -578,5 +578,17 @@ app.get('/error/redirect-error:id', (req, res) => {
}
});

app.get('/content-length', (req, res) => {
const { headers } = req;
if (
headers.hasOwnProperty('content-length') &&
headers['content-length'] > 0
) {
res.status(400).send('bad request');
} else {
res.status(200).send('ok');
}
});

const server = http.createServer(app);
server.listen(process.env.ZUUL_PORT);

0 comments on commit 804c35c

Please sign in to comment.