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

PCHR-4046: Fix security vulnerabilities #2828

Merged
merged 14 commits into from
Aug 17, 2018

Conversation

AkA84
Copy link
Contributor

@AkA84 AkA84 commented Aug 16, 2018

This PR is about solving the security vulnerabilities in the node dependencies used across the repo.

This was achieved primarily via npm audit fix, and only if that failed by manually updating the dependencies.

The focus was on getting 0 vulnerability issues on each "project" (= folder with package.json) whenever possible.

Following is the list of projects

root

0 vulnerabilities left
No manual changes done

hrui

The package.json file was actually deleted, since after #2775 it became obsolete

org.civicrm.bootstrapcivihr

0 vulnerabilities left

Upgraded to gulp v4

Had to manually update gulp to version 4

This is a smell of bad architecture. Any gulp task is being executed by the gulp in the hrcore extension, which had already been upgraded in #2804, while the other extensions only provide additional custom task to add to the standard build pipeline.

So when an extension needs to require('gulp') in order to do things like gulp.src, it should automatically require the gulp in hrcore. Unfortunately hrcore is not a parent but a sibling of the other extensions, which can't then find its gulp. Hence the need for each extension to have its own copy of the task runner

This problem will be solved by a later ticket where the "main" gulp will be moved outside hrcore and placed in the root

Changes to civihr.css

They are just about quotes being used when processing SASS where they weren't before

- background: url(../img/spinner.svg) no-repeat center center !important;
+ background: url("../img/spinner.svg") no-repeat center center !important;

Additionally the stricter approach to glob patterns due to gulp-sass-glob led to the same outcome outlined in "uk.co.compucorp.civicrm.hrleaveandabsences"

org.civicrm.reqangular

0 vulnerabilities left

Upgraded to gulp v4

See "org.civicrm.bootstrapcivihr"

Fixes to usage of gulp-angular-templatecache

Removed trailing slashes

Couldn't figure out the reason, but with gulp v4 the paths to the templates files got a leading slash that didn't make possible for angular to find the template

- $templateCache.put('loading.html','<div>...</div>');
+ $templateCache.put('/loading.html','<div>...</div>');

Using the transformUrl function to remove the slash solved the issue

.pipe(templateCache({
  // ...
  transformUrl: function (url) {
    return url.replace(/^\//, '');
  }
}))

Used plugin option instead of manual fixes

The module where we store the templates is called common.templates, which is different than the default module the plugin would put them, called simply templates.

Before this PR, the module was renamed manually by replacing the string in the modules/templates.js file

gulp.src(path.join(commonFolder, 'templates', '/**/*.html'))
  .pipe(templateCache({ moduleSystem: 'RequireJS' }))
  .pipe(replace('module(\'templates\')', 'module(\'common.templates\', [])'))

Turns out that the plugin offers options to do just that

.pipe(templateCache({
  // ...
  module: 'common.templates',
  standalone: true,
  // ...
}))

uk.co.compucorp.civicrm.hrleaveandabsences

0 vulnerabilities left

Upgraded to gulp v4

See "org.civicrm.bootstrapcivihr"

Changes to leaveandabsence.css

Switching from gulp-sass-bulk-import to gulp-sass-glob (see "hrcore" below) fixed the output of the minified file

With the old module, this

#bootstrap-theme {
  @import 'components/*';
}

@import 'components/outside-namespace/*';

resulted in the partials inside outside-namespace/ to be imported twice (seems that by default it interpreted /* as /**/*)

The new module is much more strict in terms of the glob format, and correctly excludes the outside-namespace/ folder from the first @import

The only partial affected by this change is outside-namespace/leave-calendar.scss, which contains only the styling for .chr_leave-calendar__day-tooltip.

The tooltip had been tested to make sure it's still displayed correctly

uk.co.compucorp.civicrm.hrcore

1 vulnerability left.

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low           │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ lodash                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=4.17.5                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ backstopjs [dev]                                             │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ backstopjs > junitwriter > xmlbuilder > lodash               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/577                       │
└───────────────┴──────────────────────────────────────────────────────────────┘

This is very low priority since

  1. It's backstop, so a dev tool
  2. We update backstop regularly
  3. We're going to move backstop to a different repo

Use gulp-sass-glob instead of gulp-sass-bulk-import

gulp-sass-bulk-import is a very old plugin that hasn't been updated in 3 years and as such contains plenty of vulnerability issues in its dependency tree

As such it has been replaced with gulp-sass-glob which is actively maintained and a no outstanding security issues

The only difference between the two is that the latter plugin is stricter with the glob formatting (see "uk.co.compucorp.civicrm.hrleaveandabsences"), but other than that they're pretty much the same

Update to karma v3

Bumped the karma version to 3, the only breaking change is that they dropped support for node 4 (karma-runner/karma#3082) so it doesn't really affect us

⚠️ the build task was executed on each extension after hrcore had been updated. No changes to the js/css dist file had been detected, except for the ones mentioned in this PR. All the tests were performed successfully

uk.co.compucorp.civicrm.hremails

Was: 45 vulnerabilities (2 low, 36 moderate, 7 high)
Now: 42 vulnerabilities left (1 low, 35 moderate, 6 high)

In email-templates/ sub-folder
Was: 164 vulnerabilities (45 low, 103 moderate, 16 high)
Now: 53 vulnerabilities (12 low, 36 moderate, 5 high)

Only npm audit fix had been used to fix what it was possible to automatically fix.

There is no new version available of the foundation-cli package, and being this a dev tool that had been basically used once, the priority is very low

@AkA84 AkA84 merged commit fdd5866 into staging Aug 17, 2018
@AkA84 AkA84 deleted the PCHR-4046-fix-security-vulnerabilities branch August 17, 2018 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants