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.

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

feat: add brotli to supported compression πŸ—œοΈ #4503

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

Conversation

bricss
Copy link

@bricss bricss commented May 19, 2024

This PR is a attempt to bring support for brotli compression πŸ—œοΈ 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 πŸ“¦

@bricss bricss force-pushed the feat/add-brotli-with-priority-opt branch 2 times, most recently from 448ad9e to fbe22de Compare May 19, 2024 10:18
@kanongil
Copy link
Contributor

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.

@bricss
Copy link
Author

bricss commented May 19, 2024

@kanongil I've updated the PR and removed the "priority" feature and will submit a separate PR for it later πŸ™‚

Copy link
Contributor

@kanongil kanongil left a comment

Choose a reason for hiding this comment

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

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.

lib/config.js Outdated Show resolved Hide resolved
@@ -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']
Copy link
Contributor

Choose a reason for hiding this comment

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

This addition does not seem useful. 'br' is quick to parse and I don't believe it is indeed a commonly used string.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe it worth to have it for consistency? πŸ€”

Copy link
Author

Choose a reason for hiding this comment

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

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'];
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably appear before 'gzip'. Otherwise gzip will be preferred for all practical use-cases.

Copy link
Author

@bricss bricss May 23, 2024

Choose a reason for hiding this comment

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

IMO, the order thingy should be addressed with compression.priority feature, which I plan to add if this one will land πŸ›¬

Copy link
Author

Choose a reason for hiding this comment

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

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.

@bricss
Copy link
Author

bricss commented May 23, 2024

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.

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 πŸ‘€

@bricss
Copy link
Author

bricss commented May 26, 2024

@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.

@bricss
Copy link
Author

bricss commented Jun 2, 2024

What else needs to be done here to get it's moving?

@bricss bricss force-pushed the feat/add-brotli-with-priority-opt branch from 690afa7 to 5466504 Compare June 12, 2024 18:15
@bricss bricss requested a review from kanongil July 10, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants