nhosoya / omniauth-apple

OmniAuth strategy for Sign In with Apple
MIT License
260 stars 99 forks source link

remove nonce comparison because `session['omniauth.nonce']` is nil #106

Closed obregonia1 closed 1 year ago

obregonia1 commented 1 year ago

I removed nonce comparison. I failed authentification with omniauth-apple 1.3.0, and I found that session['omniauth.nonce'] is nil. session['omniauth.nonce'] is assigned after called authorize_params, but authorize_params is never called. In addition, session['omniauth.nonce'] is generated in application, and id_token[:nonce] is generated server in Apple.

ref #102
ref #103

bvogel commented 1 year ago

Rather then removing the nonce claim I opted to store it additionally in a sameSite: :none cookie which only lives during the authentication with apple stage:

module OmniAuth
  module Strategies
    class Apple < OmniAuth::Strategies::OAuth2
      private

      def new_nonce
        nonce = SecureRandom.urlsafe_base64(16)
        session["omniauth.nonce"] = nonce
        cookies.encrypted[:apple_auth_params] =
          { same_site: :none, expires: 1.hour.from_now, secure: true, value: nonce }
        nonce
      end

      def stored_nonce
        nonce = session.delete("omniauth.nonce") || cookies.encrypted[:apple_auth_params]
        cookies.delete :apple_auth_params
        nonce
      end

      def cookies
        request.env["action_dispatch.cookies"]
      end
    end
  end
end

add this monkey-patch to an initializer. The session will be a new one in most cases when the callback is invoked with a POST from the apple side.

obregonia1 commented 1 year ago

@bvogel LGTM!! How about do you create PR with this solution? I close this PR, after your PR will be merged.

bvogel commented 1 year ago

@obregonia1 https://github.com/nhosoya/omniauth-apple/pull/107

boyfunky commented 1 year ago

i tried to add this to my initializer and i get this error @bvogel @obregonia1 (apple) Authentication failure! undefined method encrypted' for nil:NilClass: NoMethodError, undefined methodencrypted' for nil:NilClass

This error is when it tries to run this line cookies.encrypted[:apple_auth_params] = { same_site: :none, expires: 1.hour.from_now, secure: true, value: nonce }

Has anyone experienced something similar?

bvogel commented 1 year ago

better try using the code from PR #107 - we currently run that in our production system. On the other hand a monkey patch like the one above shouldn't be executed during startup, it's just another class definition were only class name and methods are identified.

obregonia1 commented 1 year ago

@bvogel Thanks. Due to circumstances, we no longer need to use this library, so we are not using this change.

I hope discuss in #107 will be going in the right direction.

sheuer commented 4 months ago

i tried to add this to my initializer and i get this error @bvogel @obregonia1 (apple) Authentication failure! undefined method encrypted' for nil:NilClass: NoMethodError, undefined methodencrypted' for nil:NilClass

This error is when it tries to run this line cookies.encrypted[:apple_auth_params] = { same_site: :none, expires: 1.hour.from_now, secure: true, value: nonce }

Has anyone experienced something similar?

I fixed this issue by swapping out

      def cookies
        request.env["action_dispatch.cookies"]
      end

to

      def cookies
        @_request ||= ActionDispatch::Request.new(request.env)
        @_request.cookie_jar
      end