bodyReader does not work with both gunzip and maxBodySize

[jfilipczyk]

Bug Report

bodyReader plugin setup with maxBodySize when called with Content-Encoding: gzip with body size over the body limit causes zlib buffer error and eventually stops process.

Restify Version

8.3.2

Node.js Version

10.14.2

Expected behaviour

Return "413 Body Too Long". Same behaviour as for not gzipped content.

Actual behaviour

server is down with error:

events.js:167
      throw er; // Unhandled 'error' event
      ^

Error: unexpected end of file
    at Zlib.zlibOnError [as onerror] (zlib.js:154:17)
Emitted 'error' event at:
    at Zlib.zlibOnError [as onerror] (zlib.js:157:8)

Repro case

'use strict';

const restify = require('restify');
const got = require('got');
const zlib = require('zlib');

const payload = { apple: 'red' };
const body = zlib.gzipSync(JSON.stringify(payload));

// set maxBodySize smaller than gzipped body
const maxBodySize = body.byteLength - 1;

const server = restify.createServer();
server.listen(3000, '127.0.0.1');
server.use(restify.plugins.bodyParser({ maxBodySize }));
server.post('/foo', (req, res, next) => {
    res.send();
    next();
});

got({
    url: 'http://127.0.0.1:3000/foo',
    body,
    json: false,
    throwHttpErrors: false,
    headers: {
        'Content-Encoding': 'gzip',
        'Content-Type': 'application/json'
    }
})
    .then(response => console.log(response.statusCode))
    .catch(err => console.log(err));

Cause

bodyReader done handler function is not triggered when gunzip transformer emits error due to incomplete buffer when it is incomplete because maxBodySize limit was reached.

Are you willing and able to fix this?

Yes. PR is on the way.

[wraithgar]

This bug extends beyond just the example given here. It also affects requests that specify a content-type and a content-encoding of gzip, but have no request body.

User requests with invalid gzip data in the request body should result in a 400, not a 500.

What would it take to update the PR to include this other use case, and what would it take to land it? I'm prepared to help with this if needed.

Minimum reproduction of other use case:

const restify = require('restify')
const server = restify.createServer()
server.use(restify.plugins.bodyParser())
server.get('/', async (req, res) => res.send(200, 'ok'))
server.listen('8080')

$ curl -v -H 'Content-encoding: gzip' -H 'Content-type: json' http://localhost:8080/
*   Trying [::1]:8080...
* Connected to localhost (::1) port 8080
> GET / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/8.4.0
> Accept: */*
> Content-encoding: gzip
> Content-type: json
> 
* Empty reply from server
* Closing connection
curl: (52) Empty reply from server

$ node server.js
(node:85775) [DEP0111] DeprecationWarning: Access to process.binding('http_parser') is deprecated.
(Use `node --trace-deprecation ...` to show where the warning was created)
node:events:492
      throw er; // Unhandled 'error' event
      ^

Error: unexpected end of file
    at Zlib.zlibOnError [as onerror] (node:zlib:189:17)
Emitted 'error' event on Gunzip instance at:
    at emitErrorNT (node:internal/streams/destroy:151:8)
    at emitErrorCloseNT (node:internal/streams/destroy:116:3)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  errno: -5,
  code: 'Z_BUF_ERROR'
}

Node.js v20.10.0

[wraithgar]

The request package had a similar bug (no-body gunzip) and it was fixed in https://github.com/request/request/pull/2176