-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(brotli): support brotli compression format #1812
Conversation
Hi @titanism is there a chance you can have a look at this one? |
I think you may need to use |
Thanks for the feedback. Actually, I moved the gzip/ deflate regex in to the file and added another regex (pretty much the same) for Brotli so I guess this vulnerability already exists. Shouldn't we handle it in a dedicated PR? WDYT? |
Hi @titanism , did you had time to look at it? |
As requested you need to use something like RE2 or add tests to test against Regex Bomb / long string attack vector |
I had a look at it and RE@ is Node only. And the other option of |
Hi @titanism can you have a look? I added what you've asked for |
You did not do what we asked, we asked for |
Hi there - apologies as we did not see your earlier reply quoted above here. For browsers you can take the approach we did here: |
Done. Please have a look |
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/tinyify@3.0.0 |
Hi @titanism - did you had a chance to have a look at it? :) |
This code needs cleaned up dc58a76#diff-3274f1a37032fb0ae4e2823def0007c634e869ae0dfc304ff6a12c36513c3a52R1-R15 |
Code refactored |
Hi @titanism can you please have a look? |
@yigaldviri what was changed? it doesn't look like anything was changed did you run |
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is an install script?Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts. Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
Sorry I forgot to And yes - tests passed successfully |
Hi @titanism - can you please have a look? |
Hi @titanism - I understand that you are probably busy. Do you know another maintainer that can have a look at this? |
@yigaldviri v10.0.0 is released to npm, can you please try it out and let us know if it works OK? 🙏 Thank you for your contribution 🎉 Ref: https://github.com/ladjs/superagent/releases/tag/v10.0.0 |
I think we have an issue. When using Yarn and Node 14 you cant download superagent since the re2 package contains node-gyp@10.2.0 that requires Node 16 (only yarn fails it. npm not). |
PR welcome? |
Seems like this package (combined with yarn) isnt compatible on Node 14.18. I tried older version but some other dependency fails us there. Do you think there's a different package we ca can use instead of it?
Here you go - #1813 |
We should just drop Node v14 support. Honestly would be best to support Node >= 16 or just Node v18+ if necessary. You can modify your PR as such @yigaldviri |
You are right. However, I'm using Node 14. Upgrading now isnt an option (on our roadmap) so dropping support for 14 will be problematic |
Solving this issue
Checklist