cedarcode / webauthn-rails-demo-app

Rails app demonstrating a WebAuthn password-less login
https://webauthn.cedarcode.com
Apache License 2.0
97 stars 39 forks source link

Unify data acquisition for `Hash` class #132

Closed soartec-lab closed 8 months ago

soartec-lab commented 9 months ago

First of all, this sample application was very useful for my development. Thank you for creating it. I've made changes to this repository to make it even more useful.

Since Ruby distinguishes between string and symbol when accessing data with Hash#[], unexpected data retrieval will occur, for example, as shown below.

session[:current_registration]
=> {:challenge=>"MU76pUDRlTBGut7yigvg_TsiEYy87VvYhOthtomV6Ss"}

session["current_registration"]
=> {:challenge=>"MU76pUDRlTBGut7yigvg_TsiEYy87VvYhOthtomV6Ss"}

# Unable to obtain expected data
session["current_registration"]["challenge"]
=> nil

session["current_registration"][:challenge]
=> "MU76pUDRlTBGut7yigvg_TsiEYy87VvYhOthtomV6Ss"

session[:current_registration][:challenge]
=> "MU76pUDRlTBGut7yigvg_TsiEYy87VvYhOthtomV6Ss"

Unify the key specification method when acquiring data of Hash class to symbol.

soartec-lab commented 8 months ago

@santiagorodriguez96

Thanks for checking

No, I don't think the current source code will be able to get the value correctly and will not work. Because in RegistrationsController#callback, session["current_registration"]["user_attributes"] returns nil. This is because :user_attributes is registered in session["current_registration"] instead of "user_attributes" inRegistrationsController#create`.

def callback
  user = User.create!(session["current_registration"]["user_attributes"])

  # seshanision["current_registration"]["user_attributes"] # => nil
  # session["current_registration"][:user_attributes] #=> {"id"=>nil, "username"=>"my_name", "created_at"=>nil, "updated_at"=>nil, "webauthn_id"=>"bEyo..."}

For reference, I will write a sample code in ruby where the value cannot be obtained when the key of Hash is different.

$ ruby -v
ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [aarch64-linux-musl]

$ irb

hash = {
  'string_key' => 'string_value',
  symbol_key: 'symbol_value',
}

hash['string_key'] #=> "string_value"
hash[:symbol_key] #=> "symbol_value"
hash['symbol_key'] #=> nil
hash[:string_key] #=> nil
santiagorodriguez96 commented 8 months ago

@santiagorodriguez96

Thanks for checking

No, I don't think the current source code will be able to get the value correctly and will not work. Because in RegistrationsController#callback, session["current_registration"]["user_attributes"] returns nil. This is because :user_attributes is registered in session["current_registration"] instead of "user_attributes" inRegistrationsController#create`.

def callback
  user = User.create!(session["current_registration"]["user_attributes"])

  # seshanision["current_registration"]["user_attributes"] # => nil
  # session["current_registration"][:user_attributes] #=> {"id"=>nil, "username"=>"my_name", "created_at"=>nil, "updated_at"=>nil, "webauthn_id"=>"bEyo..."}

For reference, I will write a sample code in ruby where the value cannot be obtained when the key of Hash is different.

$ ruby -v
ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [aarch64-linux-musl]

$ irb

hash = {
  'string_key' => 'string_value',
  symbol_key: 'symbol_value',
}

hash['string_key'] #=> "string_value"
hash[:symbol_key] #=> "symbol_value"
hash['symbol_key'] #=> nil
hash[:string_key] #=> nil

Hmm the registration in the demo works correctly for me and the user_attributes – which contains the username and webauthn_id of the user – are passed correctly as the user is correctly created. What's more, if I change to use symbol in every place it stops working – in fact, in the callback action, trying to access session[:current_registration][:user_attributes] (using all symbol keys) returns nil.

   28:     end
   29:   end
   30:
   31:   def callback
   32:     byebug
=> 33:     user = User.create!(session["current_registration"]["user_attributes"])
   34:
   35:     begin
   36:       webauthn_credential = relying_party.verify_registration(
   37:         params,
(byebug) session[:current_registration]
{"challenge"=>"jXzczVsihPejjZ3i71IQU0cM-8zkRpmz1VodgprxVR4", "user_attributes"=>{"id"=>nil, "username"=>"test", "created_at"=>nil, "updated_at"=>nil, "webauthn_id"=>"zy9ibq22a7Cca7EFvb0D-r8TG1HQ595RMAxptO3Q3-JNE5JssJYeX7jgGZalaxKIOU37jOE88gxdEGmI0P1dvQ"}}
(byebug) session[:current_registration][:user_attributes]
nil
(byebug) session["current_registration"]
{"challenge"=>"jXzczVsihPejjZ3i71IQU0cM-8zkRpmz1VodgprxVR4", "user_attributes"=>{"id"=>nil, "username"=>"test", "created_at"=>nil, "updated_at"=>nil, "webauthn_id"=>"zy9ibq22a7Cca7EFvb0D-r8TG1HQ595RMAxptO3Q3-JNE5JssJYeX7jgGZalaxKIOU37jOE88gxdEGmI0P1dvQ"}}
(byebug) session["current_registration"]["user_attributes"]
{"id"=>nil, "username"=>"test", "created_at"=>nil, "updated_at"=>nil, "webauthn_id"=>"zy9ibq22a7Cca7EFvb0D-r8TG1HQ595RMAxptO3Q3-JNE5JssJYeX7jgGZalaxKIOU37jOE88gxdEGmI0P1dvQ"}

I think that what's happening is that we are indeed setting user_attributes in the session as a symbol in the create action so trying to access to session["current_registration"]["user_attributes"] (using all strings) returns nil, as you correctly point out. However, then the session object is stored in the cookies as JSON and sent to the browser, so that, in the next action (callback action), Rails loads the session store from that JSON so the keys are strings. Here there is a very good comment on the topic: https://github.com/rails/rails/issues/22796.

soartec-lab commented 8 months ago

@santiagorodriguez96

Ok. This may be a misunderstanding on my part or something that depends on the environment. I was satisfied with your opinion. I will close this PR because I will create another PR if a new problem is discovered. Thank you for investigating.