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

Bump secure_headers gem to a more recent version #19752

Merged
merged 1 commit into from
Jan 27, 2020

Conversation

agrare
Copy link
Member

@agrare agrare commented Jan 23, 2020

Update the secure_headers gem to a more recent version

GHSA-xq52-rv6w-397c

@Fryguy
Copy link
Member

Fryguy commented Jan 23, 2020

@martinpovolny @himdel Can you also please review? If I recall, this affects the UI.

@Fryguy
Copy link
Member

Fryguy commented Jan 23, 2020

Not sure if this affects the API or not...I don't think so, but I could be wrong.

@himdel
Copy link
Contributor

himdel commented Jan 23, 2020

Agreed, not seeing any potential issues in the changelog.

The UI is overriding CSP for iframe menu items (/dashboard/iframe)
for websockets (every request),
and for html5 consoles (/vm*/launch_html5_console).

Websockets seem to keep working,
/dashboard/iframe fails to set x_frame_options now, but that seems caused by #6908, not here,
EDIT: dashboard/iframe works with domains which do not set their own x_frame_options,
(not testing remote consoles).

I think LGTM :)

@Fryguy
Copy link
Member

Fryguy commented Jan 23, 2020

Related to GHSA-xq52-rv6w-397c

@Fryguy
Copy link
Member

Fryguy commented Jan 23, 2020

Adam can you update the commit message to link to GHSA-xq52-rv6w-397c ?

@agrare
Copy link
Member Author

agrare commented Jan 23, 2020

👍 yeah, wasn't sure if this was like a "get the fix in before making the flaw public" thing

Update the secure_headers gem to a more recent version

GHSA-xq52-rv6w-397c
@Fryguy
Copy link
Member

Fryguy commented Jan 23, 2020

The flaw's already public 😅

@agrare
Copy link
Member Author

agrare commented Jan 23, 2020

True, the security issue on ManageIQ wasn't public (e.g. on the security tab) though

@miq-bot
Copy link
Member

miq-bot commented Jan 23, 2020

Checked commit agrare@822cebb with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@Fryguy Fryguy merged commit d1f056d into ManageIQ:master Jan 27, 2020
@Fryguy Fryguy added this to the Sprint 129 Ending Feb 3, 2020 milestone Jan 27, 2020
@agrare agrare deleted the bump_secure_headers_gem branch January 27, 2020 15:22
@himdel
Copy link
Contributor

himdel commented Jan 29, 2020

Causes API failure on travis...

Failures:
1864
1865  1) Headers Response Headers returns some headers related to security
1866     Failure/Error: expect(content_security_policy_for("frame-src")).to include("'self'")
1867       expected nil to include "'self'", but it does not respond to `include?`
1868     # ./spec/requests/headers_spec.rb:84:in `block (3 levels) in <top (required)>'

@himdel
Copy link
Contributor

himdel commented Jan 29, 2020

(Fixed in ManageIQ/manageiq-api#729)

@himdel
Copy link
Contributor

himdel commented Jan 29, 2020

Given ManageIQ/manageiq-api#729 (comment),
are there any plans to upgrade to a newer secure_headers branch?

@agrare
Copy link
Member Author

agrare commented Jan 29, 2020

@himdel if you want to go to ~>6.3 I'd be all for that, for a security fix I wanted to stay on the same major version in the hopes that it'd minimize breakage

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