igrigorik / vimgolf

Real Vim ninjas count every keystroke - do you?
http://www.vimgolf.com/
MIT License
678 stars 65 forks source link

Update OmniAuth to use POST in order to address CVE-2015-9284 #362

Closed filbranden closed 1 year ago

filbranden commented 2 years ago

OmniAuth was warning about this issue in the logs:

WARN -- omniauth: (twitter)
  You are using GET as an allowed request method for OmniAuth. This may leave
  you open to CSRF attacks. As of v2.0.0, OmniAuth by default allows only POST
  to its own routes. You should review the following resources to guide your
  mitigation:
  https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284
  https://github.com/omniauth/omniauth/issues/960
  https://nvd.nist.gov/vuln/detail/CVE-2015-9284
  https://github.com/omniauth/omniauth/pull/809

The trouble is that the fix requires using POST instead of GET in the "Sign in with Twitter" link, but doing so with a link requires using JavaScript. This can be done using Rails' UJS (Unobtrusive JavaScript) library, but that would require us to actually adopt it (since it's currently not used anywhere), it also makes the code harder and more expensive to test, since the testing environment starts needing a JavaScript engine.

The alternative to JavaScript is switching to using a form with a submit button to perform the action with a POST method. I ended up taking this latter approach of using a form with a submit button, and then using CSS to make it render the same as a hyperlink.

The CSS I used to render a button as a link was based on the following answer from StackOverflow: https://stackoverflow.com/a/12642009/9447571

With some extra modifications to adapt to the link colors and the underline on hover of our current links. The effect looked good enough to me on my local testing with a couple of browsers.

There are some links to "signing in" from the box with instructions of how to play one of the challenges, or how to unlock more answers. I ended up handling those using JavaScript, to locate the "signin" form by HTML element id and then submit that form.

I also had to update the RSpec tests to use click_button instead of click_link, since the "Sign in with Twitter" is now a button and not a link anymore.

Additionally to the changes described above, I also updated the "Sign Out" link to use a form and a button and the /signout URL to only accept requests via POST.

Tested locally, also confirmed that all RSpec tests passed as expected.

codecov-commenter commented 2 years ago

Codecov Report

Merging #362 (a804786) into master (3141b7e) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #362   +/-   ##
=======================================
  Coverage   92.89%   92.89%           
=======================================
  Files          27       27           
  Lines         802      802           
=======================================
  Hits          745      745           
  Misses         57       57           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.