Update OmniAuth configuration in README

[jclusso]

Removed omniauth-rails_csrf_protection gem and added request validation phase configuration.

Unless, I don't understand something, there is no reason to have an extra gem since the documentation states we can just add the following line now.

OmniAuth.config.request_validation_phase = OmniAuth::AuthenticityTokenProtection.new(key: :_csrf_token)

[goldsmithb]

I have proposed an update to the associated wiki docs to reflect this new preferred approach (see that here).

Do you think it would be wise to include some context in the README or link to the wiki page? I could see two reasons to do so: one, this isn't the only way to mitigate the vulnerability, it is just the recommended way; and two, it may not be clear to users why this config is necessary for using OmniAuth in Rails. However, perhaps it is TMI for the README?

[jclusso]

I have proposed an update to the associated wiki docs to reflect this new preferred approach (see that here).

Do you think it would be wise to include some context in the README or link to the wiki page? I could see two reasons to do so: one, this isn't the only way to mitigate the vulnerability, it is just the recommended way; and two, it may not be clear to users why this config is necessary for using OmniAuth in Rails. However, perhaps it is TMI for the README?

I don't know why we need multiple ways to mitigate the vulnerability or why anyone would choose anything besides this way. I also don't know why this is even something developers have to configure. If it was up to me I'd make this automatic. Developers don't want to think about this.

[goldsmithb]

The reason this must be configured by devs and cannot be mitigated by default is because omniauth gem cannot include Rails-specific code and rails uses custom CSRF token handling.

As long as it's an existing vulnerability I think it is valuable to have the explanation documented somewhere, as devs using this set up must be confident that they are handling the CVE appropriately. But I hear you, no one wants to think about it so leave it out of the README.

I've never contributed like this before so sorry if I'm overthinking things, I just want to help

[jclusso]

The reason this must be configured by devs and cannot be mitigated by default is because omniauth gem cannot include Rails-specific code and rails uses custom CSRF token handling.

Why? Plenty of gems include hooks for other libraries to make them automatically work. Not sure why we can't here.

As long as it's an existing vulnerability I think it is valuable to have the explanation documented somewhere, as devs using this set up must be confident that they are handling the CVE appropriately. But I hear you, no one wants to think about it so leave it out of the README.

I don't know what it's an existing vulnerability. There is a simple solution. There are a two reasonable solutions in my opinion.

1. This documentation change is added.
2. A Rails hook is added like many other gems do to enable automatically. This means nobody has to worry about this.

[goldsmithb]

It comes from a desire to keep omniauth a framework-agnostic rack middleware. Perhaps they'd be open to a change like you suggest, but it was rejected in the past.

I saw it stated here and here at least.

I agree, the solution is to make the change to the rails section of the readme. I should clarify, the vulnerability only exists if someone uses omniauth in Rails without making that config change (or without using the omniauth-rails_csrf_protection gem). The release notes where they patch the vulnerability introduced the rails-specific extra steps. I guess this is as close as we can get to having this covered "by default" for Rails applications without adding framework-specific code to omniauth. I was just wondering if you think it would be wise to provide context around that, because this is a confusing point for many people (answer is no)

E: For non-rails apps, the CVE and wiki documentation is still technically relevant because the dev must add authenticity tokens to forms themself (though they should be doing that anyways).

I edited my change to the wiki page to make it more clear that there really is only one best solution (listing the options like that was confusing)

[jclusso]

It comes from a desire to keep omniauth a framework-agnostic rack middleware. Perhaps they'd be open to a change like you suggest, but it was rejected in the past.

I saw it stated here and here at least.

I see.

I agree, the solution is to make the change to the rails section of the readme. I should clarify, the vulnerability only exists if someone uses omniauth in Rails without making that config change (or without using the omniauth-rails_csrf_protection gem). The release notes where they patch the vulnerability introduced the rails-specific extra steps. I guess this is as close as we can get to having this covered "by default" for Rails applications without adding framework-specific code to omniauth. I was just wondering if you think it would be wise to provide context around that, because this is a confusing point for many people (answer is no)

E: For non-rails apps, the CVE and wiki documentation is still technically relevant because the dev must add authenticity tokens to forms themself (though they should be doing that anyways).

I edited my change to the wiki page to make it more clear that there really is only one best solution (listing the options like that was confusing)

I think if people want to look depper, they can go to the wiki. I think . I think it's more annoying to link to it and make people think they need to read and understand it.