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

Upgrade brakeman to v6 #23124

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Upgrade brakeman to v6 #23124

merged 1 commit into from
Sep 12, 2024

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Aug 2, 2024

@jrafanie Please review.

@jrafanie
Copy link
Member

jrafanie commented Aug 2, 2024

Looks like a legit failure with newer brakeman

@Fryguy
Copy link
Member Author

Fryguy commented Aug 2, 2024

So, I think the problem is that the file path is part of the brakeman.ignore's fingerprint, and engines could live in different places 😭 - I think I have to open an issue with brakeman.

@kbrock
Copy link
Member

kbrock commented Sep 3, 2024

Message: `protect_from_forgery` should be configured with `with: :exception`
Code:
protect_from_forgery :secret => SecureRandom.hex(64),
                     :except => ([:authenticate, :external_authenticate, :kerberos_authenticate,
                     :saml_login, :initiate_saml_login, :oidc_login, :initiate_oidc_login, :csp_report]),
                     :with => :reset_session

File: application_controller.rb:13

@Fryguy
Copy link
Member Author

Fryguy commented Sep 3, 2024

@kbrock https://github.com/ManageIQ/manageiq/pull/23124/files#diff-62c4b53b7988735188b9b2ac5614a6f7a624451ebdd77f125d35dc6ee013b3d2R23

But also, the problem I'm having isn't that issue. The problem is that the issue exists in a plugin, and the file path is part of the false-positive fingerprint. So, locally that file path is different than when it's in CI, and I can't make it work in both environments. I'm really not sure how to fix it :(

# Brakeman's fingerprint check does not work properly with engines
require "brakeman/warning"
require Rails.root.join('lib/extensions/brakeman_fingerprint_patch')
Brakeman::Warning.prepend(BrakemanFingerprintPatch)
Copy link
Member Author

Choose a reason for hiding this comment

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

@jrafanie For now, I'm just copying the patching pattern we had before. I plan to have a separate PR where I do more of a "safe-patch" approach that checks the various methods, but I didn't want to introduce too much in this PR, especially if I want to backport it.

@Fryguy
Copy link
Member Author

Fryguy commented Sep 12, 2024

@jrafanie This should be ready for review now. I'm also going to cross-repo with ui-class to show that it works properly when run from within the engines.

@miq-bot cross-repo-test manageiq-ui-classic

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Sep 12, 2024
@miq-bot
Copy link
Member

miq-bot commented Sep 12, 2024

Checked commit Fryguy@efd6938 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
3 files checked, 8 offenses detected

lib/extensions/brakeman_fingerprint_patch.rb

@Fryguy
Copy link
Member Author

Fryguy commented Sep 12, 2024

@jrafanie All of the rubocops are tripping on original brakeman code, so I am not going to change those.

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.

@Fryguy thoughts on backporting this anytime in the future? Do you want to let it bake on master for a bit?

@Fryguy
Copy link
Member Author

Fryguy commented Sep 12, 2024

Yeah I'm going to let it bake a bit - maybe backport in a week or 2 to see how the automated runs go and PRs go.

@jrafanie
Copy link
Member

Merging. Whitesource can't check it out so not really a PR error.

@jrafanie jrafanie merged commit d50087b into ManageIQ:master Sep 12, 2024
7 of 8 checks passed
@Fryguy Fryguy deleted the upgrade_brakeman branch September 12, 2024 19:09
@Fryguy
Copy link
Member Author

Fryguy commented Nov 4, 2024

Backported to radjabov via merge of master into radjabov

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.

4 participants