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

Update requests library to resolve security vulnerability in urllib3 #4126

Merged
merged 2 commits into from
Nov 2, 2020

Conversation

jason-upchurch
Copy link
Contributor

@jason-upchurch jason-upchurch commented Oct 15, 2020

Summary (required)

Impacted areas of the application

List general components of the application that this PR will affect:

  • requests library

How to test

Verify vulnerability

  1. checkout develop branch
  2. from fec-cms project root, ensure develop branch requirements.txt are installed
  3. snyk test --file=requirements.txt (you should see the vulnerability listed, if not, let's discuss)

Verify vulnerability resolution

  1. checkout this PR
  2. from fec-cms project root, ensure PR branch requirements.txt are installed
  3. snyk test --file=requirements.txt (the vulnerability should not longer be listed)

Verify the vulnerability resolution did not break anything

  1. pytest
  2. I did a manual deploy of this branch to dev space and it build successfully. I verified a few datatable returns. Please feel free to deploy and test further.

Additional considerations

  1. While testing, I saw the build give some warnings that also exist in develop (note the 404s):
 HeadlessChrome 78.0.3882 (Linux 0.0.0): Executed 382 of 434 (skipped 1) SUCCESS (0 secs / 0.847 secs)
HeadlessChrome 78.0.3882 (Linux 0.0.0): Executed 383 of 434 (skipped 1) SUCCESS (0 secs / 0.849 secs)
HeadlessChrome 78.0.3882 (Linux 0.0.0): Executed 384 of 434 (skipped 1) SUCCESS (0 secs / 0.849 secs)
16 10 2020 10:52:30.492:WARN [web-server]: 404: /v1/candidates/totals/?api_key=12345&sort=-receipts&per_page=10&sort_hide_null=true&election_year=2016&election_full=true&office=P&is_active_candidate=true&page=1
16 10 2020 10:52:30.493:WARN [web-server]: 404: /v1/candidates/totals/?api_key=12345&sort=-receipts&per_page=10&sort_hide_null=true&election_year=2016&election_full=true&office=P&is_active_candidate=true&page=1
  1. I opened an issue to investigate these (Investigate test warnings #4127)
  2. the vulnerability resolution in urllib3 requires upgrade to parent package requests

@jason-upchurch jason-upchurch added the Security: high Remediate within 30 days label Oct 15, 2020
@jason-upchurch jason-upchurch added this to the PI 13 innovation 1 milestone Oct 15, 2020
@jason-upchurch jason-upchurch self-assigned this Oct 15, 2020
@jason-upchurch jason-upchurch changed the title [WIP] Update requests, pin urllib3 Update requests library to resolve security vulnerability in urllib3 Oct 16, 2020
@codecov-io
Copy link

Codecov Report

Merging #4126 into develop will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4126      +/-   ##
===========================================
+ Coverage    75.47%   75.48%   +0.01%     
===========================================
  Files          121      121              
  Lines         7417     7417              
  Branches       596      596              
===========================================
+ Hits          5598     5599       +1     
+ Misses        1819     1818       -1     
Impacted Files Coverage Δ
fec/fec/static/js/modules/calendar.js 92.64% <0.00%> (+0.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 179a5b8...5e11ea0. Read the comment docs.

Copy link
Member

@patphongs patphongs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, we use requests and urllib inside api_caller.py, so I tested to make sure that still loads

@rfultz rfultz merged commit b53afb2 into develop Nov 2, 2020
@rfultz rfultz deleted the feature/4106-snyk-high-urllib3 branch November 2, 2020 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Please review Security: high Remediate within 30 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Snyk: High] requirement - HTTP Header Injection (due 11/06/2020)
4 participants