Expand our usage of FriendsOfPHP/security-advisories
Open, NormalPublic

Description

We currently run a check once a day against all of the production dependencies that are deployed in mediawiki/vendor. I propose that in addition to that, we start checking on every commit whether there are any security issues for both production dependencies and development dependencies. At least two of our dev dependencies (codesniffer and phpunit) have had security issues in the past.

This task should be made public once T180231 is disclosed.

Legoktm created this task.Nov 11 2017, 4:14 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 11 2017, 4:14 AM

https://github.com/FriendsOfPHP/security-advisories/issues/237 asks for what usage limits are.

Also we are currently only checking against master. We need to check all supported release branches.

bd808 added a subscriber: bd808.Nov 12 2017, 12:14 AM

What is the main expected advantage of checking every commit?

Mostly ease of implementation when I was thinking about it. It would be pretty easy to add in the check after we've run "composer install" and have a composer.lock file to check against. Also I was thinking of how parsoid runs https://github.com/nodesecurity/nsp on every commit.

But at the same time that requires people to actively be commiting for the checks to be run, which is unlikely for release branches.

MaxSem added a subscriber: MaxSem.Nov 12 2017, 12:48 AM

The problem with unsafe dependencies is usually not someone introducing a dependency on something broken - it's holes being found in existing dependencies. That would just result in unrelated commits breaking.

Checking every commit of mediawiki/vendor.git would be reasonable, but checking every commit of mediawiki/core.git (and every extension and skin?) seems like a lot of wasted work for our CI infrastructure and for the upstream service. As noted it it probably not very helpful for increasing coverage of branches other than master as well.

The main benefit of per-commit is that it forces people to deal with the issue immediately since it stops people from committing - no potential issue with alerts going to the wrong people.

Other than that, I tend to agree that once a day is probably fine.

Legoktm changed the visibility from "Custom Policy" to "Public (No Login Required)".Nov 16 2017, 7:09 AM
Legoktm added a project: Composer.

There was a comment on the github about https://packagist.org/packages/roave/security-advisories which might be an easy option since we run composer on every commit.

Bawolff triaged this task as Normal priority.Nov 29 2017, 7:06 PM
Bawolff added a project: Security-Team.
Reedy added a subscriber: Reedy.Fri, Apr 13, 2:54 PM

https://github.com/Roave/SecurityAdvisories is another potential variant of the FriendsOfPHP stuff....