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

Reviving old proposal: Add check for config.force_ssl = true #1181

Closed
JacobEvelyn opened this issue Apr 24, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@JacobEvelyn
Copy link
Contributor

commented Apr 24, 2018

I noticed that brakeman doesn't warn when config.force_ssl = true is not set in the production configuration (e.g. production.rb). I also noticed that this check was proposed six years ago in #202 and the ultimate decision there was to not add the check.

I want to humbly propose that we revisit that decision, for a few reasons:

  1. Brakeman's stated policy is to errs on the side of reporting too much rather than not reporting actual vulnerabilities.
  2. As of #368, brakeman now has a robust mechanism for marking warnings to be ignored, so this warning shouldn't cause any real headache for users who want to ignore it.

Thoughts?

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Apr 24, 2018

As far as (1), that still has to be balanced against people's tolerance of false positives 😄(which is generally low).

I don't mind adding a check for this setting...but I wonder if it should be optional? Blindly turning it on can be dangerous.

@JacobEvelyn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

I'll leave the decision about whether the check should be optional or not up to you. Either way is fine with me, as we're running brakeman with --ensure-latest --run-all-checks --interprocedural --branch-limit -1 --add-libs-path . to cast as wide a net as possible. 😀

Let me know if I can be of assistance with building/testing this check!

presidentbeef added a commit that referenced this issue Jan 11, 2019

presidentbeef added a commit that referenced this issue Jan 12, 2019

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Jan 12, 2019

Oops, referenced the wrong issue again. Please ignore.

I am working on this next.

@JacobEvelyn

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Just noticed this is still open. Anything I can do to help, @presidentbeef ?

presidentbeef added a commit that referenced this issue Apr 5, 2019

presidentbeef added a commit that referenced this issue Apr 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.