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

fix(brotli) remove re2 dependency #1813

Merged
merged 1 commit into from
Aug 13, 2024
Merged

fix(brotli) remove re2 dependency #1813

merged 1 commit into from
Aug 13, 2024

Conversation

yigaldviri
Copy link
Contributor

Use a fixed version of re2 to maintain support for Node14 (now need 14.21.3)

Checklist

  • [V] I have ensured my pull request is not behind the main or master branch of the original repository.
  • [V] I have rebased all commits where necessary so that reviewing this pull request can be done without having to merge it first.
  • [V] I have written a commit message that passes commitlint linting.
  • [V] I have ensured that my code changes pass linting tests.
  • [V] I have ensured that my code changes pass unit tests.
  • [V] I have described my pull request and the reasons for code changes along with context if necessary.

Copy link

socket-security bot commented Aug 6, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher

🚮 Removed packages: npm/re2@1.21.3

View full report↗︎

Copy link

socket-security bot commented Aug 6, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

@yigaldviri
Copy link
Contributor Author

Hi @titanism - can we merge it? It will help to keep support for Node 14 as well

@titanism
Copy link
Collaborator

It looks like this causes problems on some architectures, so we may need to look at an alternative.

#1814

At a glance, it seems like the regular expressions here are actually safe:

superagent/src/utils.js

Lines 121 to 139 in 134cdd7

/**
* Check if the response is compressed using Gzip or Deflate.
* @param {Object} res
* @return {Boolean}
*/
exports.isGzipOrDeflateEncoding = (res) => {
return new SafeRegExp(/^\s*(?:deflate|gzip)\s*$/).test(res.headers['content-encoding']);
};
/**
* Check if the response is compressed using Brotli.
* @param {Object} res
* @return {Boolean}
*/
exports.isBrotliEncoding = (res) => {
return new SafeRegExp(/^\s*(?:br)\s*$/).test(res.headers['content-encoding']);
};

> require('safe-regex2')(/^\s*(?:deflate|gzip)\s*$/)
true
> require('safe-regex2')(/^\s*(?:br)\s*$/)
true

So we may not need RE2 after all. Though this might need tested.

@yigaldviri yigaldviri changed the title fix(brotli) use a fixed version of re2 to support Node 14 fix(brotli) remove re2 dependency Aug 11, 2024
@yigaldviri
Copy link
Contributor Author

@titanism - I removed re2. we have unit tests around that area

@titanism
Copy link
Collaborator

superagent v10.0.1 is released (we removed re2 dependency)

https://github.com/ladjs/superagent/releases/tag/v10.0.1

@yigaldviri yigaldviri deleted the re2 branch August 13, 2024 05:07
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.

2 participants