HumanCellAtlas / metadata-schema

This repo is for the metadata schemas associated with the HCA
Apache License 2.0
65 stars 32 forks source link

Remediate Critical set-value vulnerability #1121

Closed Lilalamar closed 5 years ago

Lilalamar commented 5 years ago

GitHub reports the following Critical severity vulnerability in HumanCellAtlas/metadata-schema. Please remediate by the end of Q3 Milestone 3.

Description

set-value

Suggested Remediation

Upgrade set-value to version 2.0.1 or later.

Details

set-value is vulnerable to Prototype Pollution in versions before 2.0.1 and version 3.0.0. The function mixin-deep could be tricked into adding or modifying properties of Object.prototype using any of the constructor, prototype and proto payloads.

diekhans commented 5 years ago

Enrique 9:15 AM So, usually snyk makes the PR automatically with the necessary updates. In this case, we have discovered that the dependencies on the schema validator are not up to date and that's why it's not making the PR, so in order to fix it we will need the repo elixir-jsonschema-validator to be updated, but I don't have any idea of who owns the repository

diekhans commented 5 years ago

oh so we appear to have a circular path here

the elixir-jsonschema-validator NPM comes from https://www.npmjs.com/package/elixir-jsonschema-validator with the repo here: https://github.com/elixir-europe/json-schema-validator which is a fork of https://github.com/HumanCellAtlas/ingest-validator-js

@simonjupp ???

lauraclarke commented 5 years ago

@diekhans I know there were some initial attempts to share validation code for JSON schema between us and other EMBL-EBI projects with similar needs.

There is some work on with Melanie Courot's folks to see if we can come back in alignment

How urgent is this fix? I guess we need to resolve it before the longer-term alignment work is done?

@rolando-ebi do you understand what is happening here?

diekhans commented 5 years ago

I suspect that the security implications are very low. However, it is probably less work to fix it than to justify it is not a problem.

simonjupp commented 5 years ago

https://github.com/elixir-europe/json-schema-validator Is the official and latest. The plan was to switch ingest to use the elixir one. That may have already happened.

I can get Rolando and/or Enrique access to patch the elixir one and permissions to release it to npm tomorrow.

simonjupp commented 5 years ago

Updated the elixir validator to fix vulnerabilities, and updated the metadata test to use this new version. All pushed to develop. Currently reporting 0 vulnerabilities.

@ESapenaVentura and @rolando-ebi both have write access to https://github.com/elixir-europe/json-schema-validator should we every need to deploy again. Let me know your username on npm so I can give you both permissions to publish releases of this package too https://www.npmjs.com/package/elixir-jsonschema-validator.

ESapenaVentura commented 5 years ago

@simonjupp I talked to you on slack to give you my npm username.

@diekhans Please let us know if you are still receiving snyk notifications regarding this. Thanks!

diekhans commented 5 years ago

I reran the snyk tests and they are still show 5 problems comming in from elixir-jsonschema-validator@1.3.8

I think the easiest thing to do is create a synx account and test and fix elixir-jsonschema-validator directly, then just upgrade metadata-schema

ESapenaVentura commented 5 years ago

The elixir validator is now @ version 1.4.1, so either metadata-schema is getting the wrong version or it is not really updated for some reason?

diekhans commented 5 years ago

tests/packages.json still specifies "elixir-jsonschema-validator": "^1.3.8", on master

simonjupp commented 5 years ago

I only pushed to develop

diekhans commented 5 years ago

snyx only monitors the default branch, which I am guessing is master.

What does it take to get this merged into master???

simonjupp commented 5 years ago

Copy the package.json and package-lock.json flies from develop to master. I can do this tomorrow.

Lilalamar commented 5 years ago

Confirmed that Snyk and GitHub are no longer reporting this as an issue. Closing ticket. Thank you!