Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

obregonia1
Copy link

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
Copy link

bvogel commented May 23, 2023

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
Copy link
Author

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

@bvogel
Copy link

bvogel commented May 30, 2023

@obregonia1 #107

@nov nov closed this Aug 14, 2023
@boyfunky
Copy link

boyfunky commented Aug 25, 2023

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 method encrypted' 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
Copy link

bvogel commented Aug 26, 2023

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
Copy link
Author

@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
Copy link

sheuer commented Apr 18, 2024

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 method encrypted' 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

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

Successfully merging this pull request may close these issues.

5 participants