feat: add brotli to supported compression 🗜️ #4503
Conversation
bricss
force-pushed
the
feat/add-brotli-with-priority-opt
branch
2 times, most recently
from
448ad9e
to
fbe22de
Compare
|
Thanks for the contribution. It looks to be done to a quite high standard. It might very well make sense to include brotli by default, but this PR also contains another "priority" feature. While they are related, they are also independent (orthogonal). If you split it into two separate PR it will be easier to review or thus more likely to be merged. I have notes for both, but will wait with a review for now. |
|
@kanongil I've updated the PR and removed the "priority" feature and will submit a separate PR for it later 🙂 |
There was a problem hiding this comment.
While I think it makes sense to include brotli capability by default, I have some reservations around this PR, listed below.
As it is, this feature is a breaking change, as it will cause existing usage of server.encoder('br', …) (or .decoder()) to break when trying to register. This could probably be fixed by allowing addEncoder() / addDecoder() to override any built-in encoder.
The default enabled br decoder is arguably a breaking change, since it can drastically change the server load to handle a request. Especially since the BROTLI_PARAM_QUALITY defaults to BROTLI_MAX_QUALITY (11).
Another IMO breaking change will be that it is enabled as a decoder. This can be a security issue, since it provides an extra avenue for attacks against the server. Eg. the decoder could potentially segfault on corrupted input, or maybe allow for a something like a zip bomb. I would personally prefer to at least be able to disable it.
Fixing the above 2 points (making it explicit) is probably a non-trivial task. As such, it might make more sense to keep the current implementation, where it can be plugged into the code when desired. This allows full flexibility, and helps to force people to consider how they want to use brotli.
| @@ -8,21 +8,23 @@ const Hoek = require('@hapi/hoek'); | |||
|
|
|||
|
|
|||
| const internals = { | |||
| common: ['gzip, deflate', 'deflate, gzip', 'gzip', 'deflate', 'gzip, deflate, br'] | |||
| common: ['gzip, deflate', 'deflate, gzip', 'gzip', 'deflate', 'br', 'gzip, deflate, br'] | |||
There was a problem hiding this comment.
This addition does not seem useful. 'br' is quick to parse and I don't believe it is indeed a commonly used string.
There was a problem hiding this comment.
Maybe it worth to have it for consistency? 🤔
There was a problem hiding this comment.
I can remove br from commons, but by this logic it will make sense also to remove gzip, deflate as well 🤷
| gzip: (options) => Zlib.createGunzip(options), | ||
| deflate: (options) => Zlib.createInflate(options) | ||
| }; | ||
|
|
||
| encodings = ['identity', 'gzip', 'deflate']; | ||
| encodings = ['identity', 'gzip', 'deflate', 'br']; |
There was a problem hiding this comment.
This should probably appear before 'gzip'. Otherwise gzip will be preferred for all practical use-cases.
There was a problem hiding this comment.
IMO, the order thingy should be addressed with compression.priority feature, which I plan to add if this one will land 🛬
There was a problem hiding this comment.
IMO, the order should stay as is, bcos in current implementation gzip used by default when client sends accept-encoding: 'gzip, deflate, br', and changing the order will be a breaking change. Best way to address this is to add compression.priority feature to make it possible flexibly change the order of compression algorithms.
Talking about security, isn't it already an issue with currently available decoders as well as those that added via plugins? Btw, brotli that shipped via plugins is the same that goes with Node 👀 |
|
@kanongil I made changes to allow overriding default encoders and decoders 🙂 but I'm not sure if there is a need to add an extra method to disable decoders, bcos it should be possible to do this by adding pseudo decoder and throw error in it. |
|
What else needs to be done here to get it's moving? |
bricss
force-pushed
the
feat/add-brotli-with-priority-opt
branch
from
690afa7
to
5466504
Compare
This PR is a attempt to bring support for
brotlicompression 🗜️ into the core.Brotli compression widely supported and used by browsers, and imo it's time to have support for it out-of-the-box 📦