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 rubyzip to 2.0.0 #19629

Merged
merged 1 commit into from
Dec 12, 2019
Merged

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Dec 11, 2019

We're currently using rubyzip 1.3.0 with "Zip.validate_entry_sizes = true" to address CVE-2019-16892 and should fix it.

Tested the automate side.

@miq-bot assign @jrafanie
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1781195

Depends on

ManageIQ/manageiq-automation_engine#397

update:
this is Jason:
https://giphy.com/gifs/reactiongifs-bTzmG7ok7Dc6A/tile

@Fryguy
Copy link
Member

Fryguy commented Dec 11, 2019

Can you also annotate in the commit that this "Fixes #19622" ?

@Fryguy
Copy link
Member

Fryguy commented Dec 11, 2019

@d-m-u I expected to see a line removing Zip.validate_entry_sizes = true, because that's now the default and there's no need to set it with 2.0

@jrafanie
Copy link
Member

Can we remove:

lib/vmdb/util.rb:      Zip.validate_entry_sizes = true
spec/lib/vmdb/util_spec.rb:    Zip.validate_entry_sizes = true

d-m-u added a commit to d-m-u/manageiq-automation_engine that referenced this pull request Dec 11, 2019
@d-m-u
Copy link
Contributor Author

d-m-u commented Dec 11, 2019

@Fryguy yeah gimme a minute and you meant #19622

@d-m-u
Copy link
Contributor Author

d-m-u commented Dec 11, 2019

@miq-bot add_label security

@Fryguy
Copy link
Member

Fryguy commented Dec 11, 2019

@d-m-u Can you squash your commits (cause one of them is pointing to the wrong issue)?

@Fryguy
Copy link
Member

Fryguy commented Dec 11, 2019

@d-m-u Also, since this is red due to cross-repo reasons, can you run the cross-repo tests? Also include UI as well

@jrafanie
Copy link
Member

I manually tested the zip logs part of log collection, in addition to the tests on travis so as soon as the other PR is merged and this one gets 💚 , I'll be 👍

@miq-bot
Copy link
Member

miq-bot commented Dec 12, 2019

Checked commit d-m-u@4889c8f with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM, cross repo is good: ManageIQ/manageiq-cross_repo-tests#37

Will merge when automation is merged.

@jrafanie
Copy link
Member

Merging now that ManageIQ/manageiq-automation_engine#397 is merged

@jrafanie jrafanie merged commit bc83fb6 into ManageIQ:master Dec 12, 2019
@jrafanie jrafanie added this to the Sprint 127 Ending Jan 6, 2020 milestone Dec 12, 2019
@d-m-u d-m-u deleted the updating_rubyzip_again branch December 12, 2019 16:25
simaishi pushed a commit that referenced this pull request Dec 13, 2019
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit fbeb6ea912703284827f619c3533bfa4a5fc449d
Author: Joe Rafaniello <jrafanie@users.noreply.github.com>
Date:   Thu Dec 12 11:25:42 2019 -0500

    Merge pull request #19629 from d-m-u/updating_rubyzip_again

    Update rubyzip to 2.0.0

    (cherry picked from commit bc83fb6a2ce1d3bddd29098fe50114bddadb3bcd)

    https://bugzilla.redhat.com/show_bug.cgi?id=1783403

@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit 671a41ab5398d5c13eb9ece46daf353989939866
Author: Joe Rafaniello <jrafanie@users.noreply.github.com>
Date:   Thu Dec 12 11:25:42 2019 -0500

    Merge pull request #19629 from d-m-u/updating_rubyzip_again

    Update rubyzip to 2.0.0

    (cherry picked from commit bc83fb6a2ce1d3bddd29098fe50114bddadb3bcd)

    https://bugzilla.redhat.com/show_bug.cgi?id=1783401

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants