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

Security updates #3633

Merged
merged 5 commits into from
Jun 11, 2019
Merged

Security updates #3633

merged 5 commits into from
Jun 11, 2019

Conversation

dholbach
Copy link
Contributor

@dholbach dholbach commented Jun 7, 2019

I updated a bunch of Javascript dependencies to eliminate some known security issues in Scope. Probably not all of them are fixed in this round, it also brings us closer to upstream's latest and greatest.

@dholbach dholbach requested a review from foot June 7, 2019 09:33
@dholbach dholbach self-assigned this Jun 7, 2019
@@ -108,7 +108,7 @@
"start-production": "NODE_ENV=production node server.js",
"test": "jest",
"coveralls": "cat coverage/lcov.info | coveralls",
"lint": "eslint app server.js && stylelint src && sass-lint -v",
"lint": "eslint app server.js && stylelint --allow-empty-input src && sass-lint -v",
Copy link
Contributor Author

@dholbach dholbach Jun 7, 2019

Choose a reason for hiding this comment

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

@foot @fbarl Please see if we want to change in the future. I mostly just wanted to help us fix some of the security issues before we get out another release. (10.0.0 of stylelint started erroring out on empty input, it looks like we were not feeding it the right list of files for some time?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ccing @bboreham too.

@@ -108,7 +108,7 @@
"start-production": "NODE_ENV=production node server.js",
"test": "jest",
"coveralls": "cat coverage/lcov.info | coveralls",
"lint": "eslint app server.js && stylelint --allow-empty-input src && sass-lint -v",
"lint": "eslint app server.js && stylelint app/scripts && sass-lint -v",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I played around with stylelint-scss and other things, to no avail. As somebody not knowledgeable, this takes a bit much time to investigate. Happy to file an issue about this to track it further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I retract my last comment. Apparently we run sass-lint over the files in this directory. The current fix should be good. A look over it from Simon or Filip wouldn't hurt though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no directory src in the same place as app/scripts, so this line wasn't checking anything before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I didn't do any git archaeology to find out if there ever was a src directory, but at least this is checking the scripts now and as a I said, sass-lint takes care of the rest.

@bboreham
Copy link
Collaborator

bboreham commented Jun 7, 2019

Clarification: they are not "known security issues in Scope", they are "known security issues in the dependencies, which are not believed to affect Scope but best to be safe".

@dholbach
Copy link
Contributor Author

dholbach commented Jun 7, 2019

Thanks for the clarification, yes.

@dholbach dholbach force-pushed the security-updates branch from 9bc1b6c to 8602915 Compare June 7, 2019 11:33
@bboreham bboreham merged commit 4045a5c into master Jun 11, 2019
@bboreham bboreham deleted the security-updates branch June 11, 2019 13:19
@bboreham bboreham mentioned this pull request Jun 11, 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.

2 participants