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 fix. Upgrade to gulp 4.0.0 #10

Merged
merged 3 commits into from
Nov 13, 2018
Merged

Conversation

isholgueras
Copy link
Contributor

The security issue is with gulp 3.9.1 and the fix is to upgrade to gulp 4.0.0
Following the official documentation and this post, I've upgraded this to gulp 4.0.0.

It's important to point out that things like:
gulp.task('default', ['a', 'b'], function() { /*code*/ });
become
gulp.task('default', gulp.series('a', 'b', function() { /*code*/ }));

and also, tasks must be defined before being called. You must define tasks like sass, sass:lint,... before calling it. Otherwise you will get this error:
AssertionError [ERR_ASSERTION]: Task never defined: sass:lint

I didn't see anything in the README.md to indicate how to upgrade, from 3.9.1 to 4.0.0 so I didn't added anything there.

@jameswilson
Copy link
Member

@isholgueras does this also resolve the upstream security issue identified a few months ago on #3 ?

Copy link
Member

@jameswilson jameswilson left a comment

Choose a reason for hiding this comment

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

I'm fine with updating gulp here, but wonder what kind of effects this is going to have on all projects that use this. Will they also need to update gulp? or just update gulp-frontend-tasks? I'm not sure what the upgrade process is for projects that use this framework. I think you could look at the obermeyer theme to figure out.

.gitignore Outdated Show resolved Hide resolved
@isholgueras
Copy link
Contributor Author

The process is to upgrade gulp for this project. If this project is part of the client project, we should update the client project as well, but imo, if we use this just for compile sass to scss files, uglify js and so forth, we're fine.

The security issue comes if you're using this library to get requests from internet as a application server

@isholgueras
Copy link
Contributor Author

isholgueras commented Oct 15, 2018

And yes @jameswilson , this PR fixes all the security issues (high, mid, low) this project have.
Basically, I've executed npm audit to verify what is happening, then a npm audit fix to update just those packages with security issue in their minor and patch updates. minimatch@2.0.10 doesn't upgrade because of its depencencies comes from gulp@3.9.1, so to upgrade to minimatch@3.0.2 I was forced to upgrade to gulp@4.0.0

I followed the official documentation and this post:
https://www.liquidlight.co.uk/blog/article/how-do-i-update-to-gulp-4/

@jameswilson jameswilson merged commit 80bbb93 into master Nov 13, 2018
@jameswilson jameswilson deleted the security-fix-gulp-4 branch November 13, 2018 23:57
@jameswilson
Copy link
Member

Thanks! Merged.

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