Skip to content
This repository has been archived by the owner on Oct 5, 2023. It is now read-only.

CSRF protection: "state" param #44

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Conversation

dvdplm
Copy link

@dvdplm dvdplm commented Jul 23, 2012

Currently 'rack-oauth2' passes the "state" param when provided but the devise front-end does not. This exposes users to CSRF attacks as outlined here: http://homakov.blogspot.it/2012/07/saferweb-most-common-oauth2.html

The Oauth2 draft strongly advices clients to make use of the "state" param: http://tools.ietf.org/html/draft-ietf-oauth-v2-27#section-10.12

This PR sets the @state ivar and includes it in the allow/deny form.

erickrause and others added 20 commits May 22, 2011 22:28
…p all required keys to params that were passed in to the hash
…d only to email which would fail for non-email conditions.
* original/master: (94 commits)
  bump version
  disable failing router tests
  fix tests
  fix rspec
  extract attr_accessors. fixes socialcast#39
  found myself needed to be able to refresh access_tokens ( more whitelisting )
  tidy up
  bump version
  include Authorization too
  add tests
  fixes socialcast#35 ( whitelisting user and client )
  bump version
  fix extracting client id/secret from basic auth header
  merge
  validate client id/secret from params or basic auth headers
  remove unused scope
  made tokens expire correctly
  fix nil redirect_uri
  bump version
  fix empty redirect_uri on authorization form
  ...

Conflicts:
	config/routes.rb
	devise_oauth2_providable.gemspec
	lib/devise_oauth2_providable.rb
	lib/token_endpoint.rb
	spec/rails_app/Gemfile
	spec/rails_app/app/models/user.rb
	spec/rails_app/db/schema.rb
	spec/rails_app/spec/integration/token_endpoint_spec.rb
* ordncn/engine_common_controller:
  Consolidate :authenticate_user! before_filter
  Engine controllers descend from common base class
…tegy'

* ordncn/flexible_client_credentials_strategy:
  TokensController can be subclassed
* ordncn/no_isolate_namespace:
  TokensController can be subclassed
  No isolate_namespace
@BRMatt
Copy link

BRMatt commented Sep 10, 2012

Please correct me if I'm wrong, but doesn't rack-oauth2 automatically grab the state parameter?

@scashin133
Copy link

@dvdplm This appears to have some other changes not related to the stated problem it is trying to solve. Specifically it looks like #38 got pulled in. Are there any other PRs or problems that this PR is trying to solve?

@dvdplm
Copy link
Author

dvdplm commented Feb 5, 2013

@scashin133 you're right, not a very clean PR. Is 770c59b still missing upstream? Maybe that and @brmatts changes is all we need? How can I help?

@goofrider
Copy link

Have you guys reached a conclusion yet? I just got hit by this isssue today. Omniauth-oauth2 strategy added the :state requirement as a default, and will raise a non-descriptive CallbackError if it's missing. devise_oauth2_providable will not work out-of-the-box with Omniauth-oauth2 currently.

@kiote
Copy link

kiote commented Jun 12, 2013

"state" param is implemented already as I can see here (actually I was worried too)
https://github.com/dvdplm/devise_oauth2_providable/blob/master/app/controllers/devise/oauth2_providable/authorizations_controller.rb#L33

@stwf
Copy link

stwf commented Sep 18, 2013

Is there any resolution on this? as noted by @goofrider this is now a requirement in omniauth-oaauth2. It would be greatif the pull could be merged and the results pushed to the gem server! Thanks for providing this. It's a big help!

@masterkain
Copy link

trying this pull request I'm running into uninitialized constant AuthorizationsController due to #38 being pulled in I guess, Rails 4

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants