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

bump request to 2.87.0 #1492

Closed
wants to merge 4 commits into from
Closed

Conversation

Rohithzr
Copy link
Contributor

@Rohithzr Rohithzr commented Jul 4, 2018

Checklist
Description of change

In reference to request/request#2943 , the request package should be upgraded to fix security issues.

This still supports node < 4 and fixes the hoek/hawk security issue for both node-gyp v3 and v4 (#1471).

@apellerano-pw
Copy link

I'm interested in seeing this merged as well, but is ^2.9.0 a better fit? It will both allow upgrading to 2.87.0 and leave others alone.

I couldn't find the reason for why the range was pinned at 2.82.0 in the first place, but even safer might be to specify >=2.9.0 <2.88.0

@Rohithzr Rohithzr changed the title bump request to 2.8.7 bump request to 2.87.0 Jul 5, 2018
@Rohithzr
Copy link
Contributor Author

Rohithzr commented Jul 5, 2018

@apellerano-pw
For security reasons (that's what this is all about), anything < 2.87 will give the hoek security warning. so maybe stick to v2.87.x and as this version supports node < 4 it should be fine for everyone.

@xzyfer
Copy link

xzyfer commented Jul 8, 2018

Duplicate of #1471

@Rohithzr
Copy link
Contributor Author

Rohithzr commented Jul 8, 2018

@xzyfer I made this one in support for 2.87 request bump in the node-gyp 3.x.

@xzyfer
Copy link

xzyfer commented Jul 8, 2018

@Rohithzr you're right. My mistake.

@padraic-edwards
Copy link

Is there any ETA on when this PR might be merged and included in a release? Thanks.

@Rohithzr
Copy link
Contributor Author

@padraic-edwards i too am waiting, maybe i think that people are on vacation or something, anyways, they will merge it when they can. Should be soon

@henray
Copy link

henray commented Jul 16, 2018

Hello, I am here to redirect my enthusiasm from gulp-sass to here 👍

@njwest
Copy link

njwest commented Jul 19, 2018

+1 please merge, ugly to see "Moderate Severity Vulnerability" on npm audit

@njwest
Copy link

njwest commented Jul 19, 2018

Tweeted at some of the folks on the NPM page for node-gyp, mebbe hop on my tweet to help get their attention: https://twitter.com/gweilo_fi/status/1019784220703580160

@Fishrock123
Copy link
Member

@Fishrock123
Copy link
Member

Has the same failures as 3.x HEAD: https://ci.nodejs.org/job/nodegyp-test-pull-request/64/

@brodybits
Copy link

brodybits commented Jul 20, 2018

CI run: https://ci.nodejs.org/job/nodegyp-test-pull-request/62/

I see the CI failure on Windows, does not look like it should be a result of this change. @Fishrock123 sorry I missed your recent update

FYI I could not find a link to the console text that shows the actual error. Here is the link to the console text in plaintext, according to the pattern from nodejs/node#9767 (comment):

(Same information should be in https://ci.nodejs.org/job/nodegyp-test-commit/327/nodes=win2016-vs2017/console as linked by https://ci.nodejs.org/job/nodegyp-test-commit/327/nodes=win2016-vs2017/)

@Rohithzr
Copy link
Contributor Author

@Fishrock123 seems to be an odd error on the Windows Systems.
#64
https://ci.nodejs.org/job/nodegyp-test-commit/337/nodes=win2016-vs2017/console

#62
https://ci.nodejs.org/job/nodegyp-test-commit/327/nodes=win2016-vs2017/console

both with same errors. Can you shed some light?

21:04:31 Node version:
21:04:31 + node -v
21:04:31 /c/workspace/nodegyp-test-commit/nodes/win2016-vs2017/local_node/node: line 1: syntax error near unexpected token `newline'
21:04:31 /c/workspace/nodegyp-test-commit/nodes/win2016-vs2017/local_node/node: line 1: `<!DOCTYPE html>'
21:04:31 Build step 'Execute shell' marked build as failure
21:04:31 Notifying upstream projects of job completion
21:04:31 Finished: FAILURE

@Fishrock123
Copy link
Member

I have no clue.

@joaocgreis
Copy link
Member

CI: https://ci.nodejs.org/job/nodegyp-test-pull-request/70/

@Rohithzr
Copy link
Contributor Author

@joaocgreis finally it's all green. I will open an issue regarding the 0.10.x test failing for documentation purposes.

@brodybits
Copy link

Really looking forward to the published release to clear the npm audit warnings on famous packages such as npm-cli and node-sass.

@joaocgreis
Copy link
Member

@Fishrock123 @mmarchini I haven't reviewed this but don't mind landing if your approvals still hold. However, I can't publish a new version so @Fishrock123 perhaps you can take this from here?

Copy link
Contributor

@mmarchini mmarchini left a comment

Choose a reason for hiding this comment

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

Still LGTM

@co3k
Copy link

co3k commented Jul 30, 2018

I'm waiting a new version for a long time because node-sass depends node-gyp and npm audit always alerts this vulnerability. It causes crying-wolf syndrome so I really want to be fixed it.

@maclover7
Copy link
Contributor

Per npm's website, @bnoordhuis, @Fishrock123, and @rvagg are those with publish access -- would someone be willing to give this PR a once-over and then publish a new 3.x release (I believe it would be v3.7.1)?

@co3k
Copy link

co3k commented Aug 2, 2018

Dear sass-loader users: Now you can use dart-sass in sass-loader v7.1.0 instead of node-sass. As a result, you can avoid this problem if you don't use any other packages that depend on node-gyp.

@ndxbn ndxbn mentioned this pull request Aug 6, 2018
@Rohithzr
Copy link
Contributor Author

Rohithzr commented Aug 6, 2018

@Fishrock123 what is causing the delay?

@rvagg
Copy link
Member

rvagg commented Aug 8, 2018

let's get this closed, new code needs quite a bit of work to make it consistent with the node-gyp codebase though. Moving to #1521.

rvagg pushed a commit that referenced this pull request Aug 9, 2018
PR-URL: #1492
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg pushed a commit that referenced this pull request Aug 9, 2018
…lable.

PR-URL: #1492
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg pushed a commit that referenced this pull request Aug 9, 2018
PR-URL: #1492
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg pushed a commit that referenced this pull request Aug 9, 2018
…lable.

PR-URL: #1492
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg
Copy link
Member

rvagg commented Aug 9, 2018

v3.8.0 is out

@rvagg rvagg closed this Aug 9, 2018
rvagg pushed a commit that referenced this pull request Apr 24, 2019
PR-URL: #1492
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg rvagg mentioned this pull request Apr 24, 2019
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.

None yet