Update omniauthable tests for OmniAuth 2.0#5331
Conversation
jkowens
force-pushed
the
ca-omniauth-2
branch
from
bf9ed99 to
a0915d1
Compare
|
Thanks @jkowens, I'll take a look and report back. |
| <%- if devise_mapping.omniauthable? %> | ||
| <%- resource_class.omniauth_providers.each do |provider| %> | ||
| <%= link_to "Sign in with #{OmniAuth::Utils.camelize(provider)}", omniauth_authorize_path(resource_name, provider) %><br /> | ||
| <%= link_to "Sign in with #{OmniAuth::Utils.camelize(provider)}", omniauth_authorize_path(resource_name, provider), method: :post %><br /> |
There was a problem hiding this comment.
@BobbyMcWho mind if I ask for some clarification here?
In the CVE wiki it mentions using link_to with method=post or a button_to should be enough, but in the release notes for OmniAuth v2 you mention specifically:
If you are using hyperlinks or buttons styled to redirect to your login route, you should update these to be a submit input or a submit type button wrapped in a form.
I'm assuming you meant those as being GET requests, that should be updated to using POST instead? (since Rails button_to or link_to + method=post use forms under the hood?)
There was a problem hiding this comment.
We want to make sure that whatever way we send the POST, we're including a CSRF token to be validated in the request_validation_phase. I suggested wrapping inputs with a rails form helper since by default those automatically generate a per-form token that can be validated by Rails' RequestForgeryProtection class
There was a problem hiding this comment.
A link_to with method=post will submit a form with an authenticity token. I assume it is a per-form token, but not sure 🤔
There was a problem hiding this comment.
Right, thanks for the clarification 👍. I believe link_to + method=post or a button_to should be fine then, like @jkowens mentioned they should also include the authenticity token by default in Rails. (whether they are per-form or not is controlled by the application via the individual form and/or global Rails config iirc.)
|
I'm merging this to my branch and will update the dependencies there now that omniauth-openid has been released as v2.0.1. Thanks again! |
3 participants
Hope this helps with #5327. Updated omniauthable links to use
method: :postand revised corresponding tests. I opened a PR with omniauth-openid (ruby-openid/omniauth-openid#44), but for now I updated the Gemfile to pull it from a branch on my fork.