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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates requirements with security impacts #4779

Merged
merged 3 commits into from Jan 28, 2019
Merged

Conversation

hillaryj
Copy link
Contributor

@hillaryj hillaryj commented Jan 22, 2019

Updates requirement pinned versions of PyYAML and requests to fix security vulnerabilities. This affects college-costs as well as cfgov-refresh.

Changes

  • Updates PyYAML to 4.2b1 for security fixes
  • Updates requests to 2.20.0 for security fixes

Testing

  1. Go to DEV5 (build5 server, only internally available)
  2. Visit the College Costs tool /paying-for-college/compare-financial-aid-and-college-cost/ and verify the tool still works.
  3. Unit-tests
  4. Verify site functionality works

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the CFPB development guidelines
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Visually tested in supported browsers and devices (see checklist below 馃憞)
  • Project documentation has been updated
  • Reviewers requested with the Reviewers tool 鉃★笍

Testing checklist

Browsers

  • Chrome
  • Firefox
  • Safari
  • Internet Explorer 8, 9, 10, and 11
  • Edge
  • iOS Safari
  • Chrome for Android

Accessibility

  • Keyboard friendly
  • Screen reader friendly

Other

  • Is useable without CSS
  • Is useable without JS
  • Flexible from small to large screens
  • No linting errors or warnings
  • JavaScript tests are passing

@chosak
Copy link
Member

chosak commented Jan 22, 2019

Upgrading PyYAML to a new major version will conflict with the PyYAML>=3.11,<3.14 defined in college-costs. That repo should be tested and made compatible (via a change to its setup.py, and a new release) with PyYAML 4 before cfgov-refresh can be upgraded.

In college-costs, the YAML functionality only seems to be called by this management command, so accessing the tool URL as suggested in the PR description wouldn't test the upgraded version of YAML parsing.

Unrelatedly, per discussion here and here, the GitHub-reported vulnerability seems to only apply to uses of yaml.load, but college-costs uses the safer yaml.safe_load instead? So maybe we don't need to make this upgrade at all?

@willbarton
Copy link
Member

willbarton commented Jan 22, 2019

Beyond @chosak's more in-depth comments, if we must use a pre-release version of PyYAML we might as well use the latest, PyYAML==4.2.b4.

@willbarton
Copy link
Member

willbarton commented Jan 22, 2019

In the same vein, is there any reason not to use requests==2.21.0? It looks like the changes are minimal. If we need 2.20, then 2.20.1 might be the better version to pin

@cfarm
Copy link
Contributor

cfarm commented Jan 25, 2019

Next steps for this:

  • remove version bumps for PyYaml from this PR, since it's moot per Andy's comments above.
  • mark PyYaml as safe in this repo, since it's run using safe_load (like magic!)
  • bump requests again to 2.21 at will's request and use it if tests pass

@cfarm
Copy link
Contributor

cfarm commented Jan 25, 2019

We're not ruling out upgrading PyYaml in college costs but that will come in a separate PR.

@cfarm cfarm merged commit e838692 into master Jan 28, 2019
@cfarm cfarm deleted the update-req-security branch January 28, 2019 16:24
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.

None yet

4 participants