Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

947 fix security vulnerability warning in survey runner #1996

Closed

Conversation

samiwel
Copy link
Contributor

@samiwel samiwel commented Feb 15, 2019

What is the context of this PR?

A high severity security vulnerability is being reported in one of the project dependencies. See https://nvd.nist.gov/vuln/detail/CVE-2017-18342 for more details.

This change updates the version of PyYAML to a patched version to mitigate the issue.

The dependency can be changed back to '*' in the Pipfile once a released version has been made available on PyPI.

How to review

All tests and checks should pass.

Checklist

  • New static content marked up for translation
  • Newly defined schema content included in eq-translations repo

A high serverity security vulnerability is being reported in one of the project dependencies.
See CVE-2017-18342 for further details. https://nvd.nist.gov/vuln/detail/CVE-2017-18342

This change updates the version of PyYAML to a patched version to mitigate the issue.
The dependency can be changed back to "*" in the Pipfile once a released version has been made available on PyPI.
@bitdivision
Copy link
Contributor

I previously avoided doing this because the only version with a fix for it is a prerelease version.

If you actually look at it, the github notification is not great, it's been an issue in pyyaml for a long time (years). They're just saying that you shouldn't use load and should always use safe_load. This is noted clearly in the docs.

@samiwel
Copy link
Contributor Author

samiwel commented Feb 15, 2019

@bitdivision Yeah I assumed this was the case, but a card made its way onto our board to investigate. Should I close the PR in that case? Are we happy to accept the risk until a release is published?

@bitdivision
Copy link
Contributor

bitdivision commented Feb 15, 2019

I think admins have the ability to dismiss those vulnerability alerts?

Up to you really, I felt a bit weird about having prerelease versions of a library pinned, and eventually gave up because I had to change the pipenv command in travis to allow prerelease I think.

There's quite a bit of discussion around this here: yaml/pyyaml#243

I'll approve as I think it's probably fine to merge this if you want to.

@samiwel samiwel changed the title 947 fix security vulnerability in survey runner 947 fix security vulnerability warning in survey runner Feb 15, 2019
@ajmaddaford
Copy link
Contributor

@samiwel @bitdivision Given what Rich has said I don't think we should merge this pr. I've checked the code and we only load yaml in two places (secrets and keys) and in both instances we use safe_load.

@samiwel
Copy link
Contributor Author

samiwel commented Feb 18, 2019

Closing this PR as per discussion above.

@samiwel samiwel closed this Feb 18, 2019
@samiwel samiwel deleted the 947-fix-security-vulnerability-in-survey-runner branch February 18, 2019 09:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants