Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added support for brotli ('br') content-encoding #406

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Added support for brotli ('br') content-encoding #406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
180f373
Added support for brotli ('br') content-encoding
danielgindi Jul 10, 2020
466d618
Update README.md
danielgindi Jul 12, 2020
ff788ac
DRYed error throwing (the one with encoding specified)
danielgindi Aug 7, 2020
c08e899
Updated brotli detection method
danielgindi Aug 25, 2020
d9ff5de
Allow "__core-js_shared__" leak in mocha
danielgindi Dec 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 9 additions & 5 deletions README.md
View file
Open in desktop
Expand Up @@ -16,6 +16,8 @@ before trusting. For example, `req.body.foo.toString()` may fail in multiple
ways, for example the `foo` property may not be there or may not be a string,
and `toString` may not be a function and instead a string or other user input.

**Note** Brotli is supported only since Node.js versions v11.7.0 and v10.16.0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think that the information about version support should be along with the br support docs, otherwise users may not see it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might be confusing the compression repo. In this one there's no br support section. As there's nothing to configure, it's just decoding.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/expressjs/body-parser/pull/406/files#diff-04c6e90faac2675aa89e2176d2eec7d8R74

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, where in the README do you want this? There's no explicit section for br...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the spots where you added the br mentions that br is one of the options.


[Learn about the anatomy of an HTTP transaction in Node.js](https://nodejs.org/en/docs/guides/anatomy-of-an-http-transaction/).

_This does not handle multipart bodies_, due to their complex and typically
Expand Down Expand Up @@ -68,8 +70,8 @@ The various errors returned by this module are described in the

Returns middleware that only parses `json` and only looks at requests where
the `Content-Type` header matches the `type` option. This parser accepts any
Unicode encoding of the body and supports automatic inflation of `gzip` and
`deflate` encodings.
Unicode encoding of the body and supports automatic inflation of `gzip`,
`br` (brotli) and `deflate` encodings.

A new `body` object containing the parsed data is populated on the `request`
object after the middleware (i.e. `req.body`).
Expand Down Expand Up @@ -123,7 +125,8 @@ encoding of the request. The parsing can be aborted by throwing an error.

Returns middleware that parses all bodies as a `Buffer` and only looks at
requests where the `Content-Type` header matches the `type` option. This
parser supports automatic inflation of `gzip` and `deflate` encodings.
parser supports automatic inflation of `gzip`, `br` (brotli) and `deflate`
encodings.

A new `body` object containing the parsed data is populated on the `request`
object after the middleware (i.e. `req.body`). This will be a `Buffer` object
Expand Down Expand Up @@ -168,7 +171,8 @@ encoding of the request. The parsing can be aborted by throwing an error.

Returns middleware that parses all bodies as a string and only looks at
requests where the `Content-Type` header matches the `type` option. This
parser supports automatic inflation of `gzip` and `deflate` encodings.
parser supports automatic inflation of `gzip`, `br` (brotli) and `deflate`
encodings.

A new `body` string containing the parsed data is populated on the `request`
object after the middleware (i.e. `req.body`). This will be a string of the
Expand Down Expand Up @@ -218,7 +222,7 @@ encoding of the request. The parsing can be aborted by throwing an error.
Returns middleware that only parses `urlencoded` bodies and only looks at
requests where the `Content-Type` header matches the `type` option. This
parser accepts only UTF-8 encoding of the body and supports automatic
inflation of `gzip` and `deflate` encodings.
inflation of `gzip`, `br` (brotli) and `deflate` encodings.

A new `body` object containing the parsed data is populated on the `request`
object after the middleware (i.e. `req.body`). This object will contain
Expand Down
25 changes: 20 additions & 5 deletions lib/read.js
View file
Open in desktop
Expand Up @@ -23,6 +23,12 @@ var zlib = require('zlib')

module.exports = read

/**
* @const
* whether current node version has brotli support
*/
var hasBrotliSupport = 'createBrotliDecompress' in zlib

/**
* Read a request into a buffer and parse.
*
Expand Down Expand Up @@ -170,11 +176,20 @@ function contentstream (req, debug, inflate) {
stream = req
stream.length = length
break
default:
throw createError(415, 'unsupported content encoding "' + encoding + '"', {
encoding: encoding,
type: 'encoding.unsupported'
})
case 'br':
if (hasBrotliSupport) {
stream = zlib.createBrotliDecompress()
debug('brotli decompress body')
req.pipe(stream)
}
break
}

if (stream === undefined) {
throw createError(415, 'unsupported content encoding "' + encoding + '"', {
encoding: encoding,
type: 'encoding.unsupported'
})
}

return stream
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -45,7 +45,7 @@
},
"scripts": {
"lint": "eslint --plugin markdown --ext js,md .",
"test": "mocha --require test/support/env --reporter spec --check-leaks --bail test/",
"test": "mocha --require test/support/env --reporter spec --check-leaks --global __core-js_shared__ --bail test/",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really shouldn't be adding a global leak with our changes. Is there a way to fix the leak? Just adding it to ignore is not really the best solution.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dougwilson It's from one of the libs used by either one of our existing dependencies, or mocha itself. Most probably by mocha itself.
It's a global leak introduced by json5 and by loadash in the old versions that we are referencing. It's for deduping the code-js libs.
We are not using these in our code, so I don't think we can do anything about it - except increase dependency versions, which will break old node support.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Why doesn't the leak occur on master branch or other PRs, if that is the case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even in this PR, it happened on and off. Sometimes by changing a single line with no interesting sideeffects.
I don't know if you recall but we've added and removed this before.
Also this happens only on specific very old versions of node, where that core-js dependency was required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, strange. So that could just mean that these changes are causing it to be intermittent, not that it is intermittent without these changes. Of all the PRs and commits I push here, it has never happened, which is why it seems to point to the changes here somehow causing them to occur. It may help if maybe we approach this differently: can you just explain why it happens? For example, you say "happens only on specific very old versions of node, where that core-js dependency was required". So, which versions is that? Can we only add the exception to only those specific versions, then? You also say "except increase dependency versions". Increase which dependencies to which versions? If these are just test dependencies, we can change any of those to be whatever version is necessary for each different version of Node.js in our Travis CI file, like we are doing today.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not deep dive that much. I only recall it was all the 0.x versions except <=0.8.
I don't really want or care to start eliminating it line by line, as those node versions are not supported for years now, and I can't do these tests without pushing little changes again and again to your CI and wait for the results. You could look at the previous CI results and see what I mean. Or maybe the whole history of CI on this branch.

What I can say for sure is that this even happened before switching between 'brotli' in process.versions and 'createBrotliDecompress' in zlib, and stopped with this change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dougwilson More info: the users of this corejs quirk are babel (by requiring json5), and eslint.
eslint is the immediate suspect, as the modules it requires vary depending on the source code. An syntax combination somewhere could trigger another rule to be required, and then this leak will happen as a few of the rules require lodash which is the culprit.

"test-cov": "nyc --reporter=html --reporter=text npm test",
"test-travis": "nyc --reporter=text npm test"
}
Expand Down
24 changes: 24 additions & 0 deletions test/json.js
View file
Open in desktop
Expand Up @@ -6,6 +6,12 @@ var request = require('supertest')

var bodyParser = require('..')

/**
* @const
* whether current node version has brotli support
*/
var hasBrotliSupport = 'createBrotliDecompress' in require('zlib')

describe('bodyParser.json()', function () {
it('should parse JSON', function (done) {
request(createServer())
Expand Down Expand Up @@ -595,6 +601,24 @@ describe('bodyParser.json()', function () {
test.expect(200, '{"name":"论"}', done)
})

var brotlit = hasBrotliSupport ? it : it.skip
brotlit('should support brotli encoding', function (done) {
var test = request(this.server).post('/')
test.set('Content-Encoding', 'br')
test.set('Content-Type', 'application/json')
test.write(Buffer.from('8b06807b226e616d65223a22e8aeba227d03', 'hex'))
test.expect(200, '{"name":"论"}', done)
})

var nobrotlit = hasBrotliSupport ? it.skip : it
nobrotlit('should throw 415 if there\'s no brotli support', function (done) {
var test = request(this.server).post('/')
test.set('Content-Encoding', 'br')
test.set('Content-Type', 'application/json')
test.write(Buffer.from('8b06807b226e616d65223a22e8aeba227d03', 'hex'))
test.expect(415, 'unsupported content encoding "br"', done)
})

it('should be case-insensitive', function (done) {
var test = request(this.server).post('/')
test.set('Content-Encoding', 'GZIP')
Expand Down
24 changes: 24 additions & 0 deletions test/raw.js
View file
Open in desktop
Expand Up @@ -6,6 +6,12 @@ var request = require('supertest')

var bodyParser = require('..')

/**
* @const
* whether current node version has brotli support
*/
var hasBrotliSupport = 'createBrotliDecompress' in require('zlib')

describe('bodyParser.raw()', function () {
before(function () {
this.server = createServer()
Expand Down Expand Up @@ -339,6 +345,24 @@ describe('bodyParser.raw()', function () {
test.expect(200, 'buf:6e616d653de8aeba', done)
})

var brotlit = hasBrotliSupport ? it : it.skip
brotlit('should support brotli encoding', function (done) {
var test = request(this.server).post('/')
test.set('Content-Encoding', 'br')
test.set('Content-Type', 'application/octet-stream')
test.write(Buffer.from('8b03806e616d653de8aeba03', 'hex'))
test.expect(200, 'buf:6e616d653de8aeba', done)
})

var nobrotlit = hasBrotliSupport ? it.skip : it
nobrotlit('should throw 415 if there\'s no brotli support', function (done) {
var test = request(this.server).post('/')
test.set('Content-Encoding', 'br')
test.set('Content-Type', 'application/octet-stream')
test.write(Buffer.from('8b03806e616d653de8aeba03', 'hex'))
test.expect(415, 'unsupported content encoding "br"', done)
})

it('should be case-insensitive', function (done) {
var test = request(this.server).post('/')
test.set('Content-Encoding', 'GZIP')
Expand Down
24 changes: 24 additions & 0 deletions test/text.js
View file
Open in desktop
Expand Up @@ -6,6 +6,12 @@ var request = require('supertest')

var bodyParser = require('..')

/**
* @const
* whether current node version has brotli support
*/
var hasBrotliSupport = 'createBrotliDecompress' in require('zlib')

describe('bodyParser.text()', function () {
before(function () {
this.server = createServer()
Expand Down Expand Up @@ -407,6 +413,24 @@ describe('bodyParser.text()', function () {
test.expect(200, '"name is 论"', done)
})

var brotlit = hasBrotliSupport ? it : it.skip
brotlit('should support brotli encoding', function (done) {
var test = request(this.server).post('/')
test.set('Content-Encoding', 'br')
test.set('Content-Type', 'text/plain')
test.write(Buffer.from('0b05806e616d6520697320e8aeba03', 'hex'))
test.expect(200, '"name is 论"', done)
})

var nobrotlit = hasBrotliSupport ? it.skip : it
nobrotlit('should throw 415 if there\'s no brotli support', function (done) {
var test = request(this.server).post('/')
test.set('Content-Encoding', 'br')
test.set('Content-Type', 'text/plain')
test.write(Buffer.from('0b05806e616d6520697320e8aeba03', 'hex'))
test.expect(415, 'unsupported content encoding "br"', done)
})

it('should be case-insensitive', function (done) {
var test = request(this.server).post('/')
test.set('Content-Encoding', 'GZIP')
Expand Down
24 changes: 24 additions & 0 deletions test/urlencoded.js
View file
Open in desktop
Expand Up @@ -6,6 +6,12 @@ var request = require('supertest')

var bodyParser = require('..')

/**
* @const
* whether current node version has brotli support
*/
var hasBrotliSupport = 'createBrotliDecompress' in require('zlib')

describe('bodyParser.urlencoded()', function () {
before(function () {
this.server = createServer()
Expand Down Expand Up @@ -681,6 +687,24 @@ describe('bodyParser.urlencoded()', function () {
test.expect(200, '{"name":"论"}', done)
})

var brotlit = hasBrotliSupport ? it : it.skip
brotlit('should support brotli encoding', function (done) {
var test = request(this.server).post('/')
test.set('Content-Encoding', 'br')
test.set('Content-Type', 'application/x-www-form-urlencoded')
test.write(Buffer.from('8b03806e616d653de8aeba03', 'hex'))
test.expect(200, '{"name":"论"}', done)
})

var nobrotlit = hasBrotliSupport ? it.skip : it
nobrotlit('should throw 415 if there\'s no brotli support', function (done) {
var test = request(this.server).post('/')
test.set('Content-Encoding', 'br')
test.set('Content-Type', 'application/x-www-form-urlencoded')
test.write(Buffer.from('789ccb4bcc4db57db16e17001068042f', 'hex'))
test.expect(415, 'unsupported content encoding "br"', done)
})

it('should be case-insensitive', function (done) {
var test = request(this.server).post('/')
test.set('Content-Encoding', 'GZIP')
Expand Down