janko / rodauth-omniauth

OmniAuth login and registration for Rodauth authentication framework
MIT License
51 stars 3 forks source link

5xx error on invalid authenticity_token #20

Closed FLX-0x00 closed 2 weeks ago

FLX-0x00 commented 3 weeks ago

We got a lot of 5xx errors on our application whem people try to manipulate the authenticity_token parameter and submit an invalid one on our OAuth rodauth rails implementations. I think the error handling of the function should be more resistent and never raise a 5xx error. Is it possible to catch this type of errors? Would be very awesome to get this type of errors away from our logs.

13:36:54 web.1  | Started GET "/en/auth/auth/github?authenticity_token=[FILTERED]" for 127.0.0.1 at 2024-08-21 13:36:54 +0200
13:36:54 web.1  |   ActiveRecord::SchemaMigration Load (0.6ms)  SELECT "schema_migrations"."version" FROM "schema_migrations" ORDER BY "schema_migrations"."version" ASC
13:36:54 web.1  |   ↳ activerecord (7.2.0) lib/active_record/log_subscriber.rb:122:in `log_query_source'
13:36:54 web.1  |   Sequel (0.6ms)  SELECT "key", "deadline" FROM "account_remember_keys" WHERE (("id" = '1') AND ("deadline" > CURRENT_TIMESTAMP)) LIMIT 1
13:36:54 web.1  |   ↳ activerecord (7.2.0) lib/active_record/log_subscriber.rb:122:in `log_query_source'
13:36:54 web.1  |   Sequel (0.6ms)  SELECT * FROM "accounts" WHERE ("id" = '1') LIMIT 1
13:36:54 web.1  |   ↳ activerecord (7.2.0) lib/active_record/log_subscriber.rb:122:in `log_query_source'
13:36:54 web.1  |   Sequel (7.7ms)  UPDATE "account_remember_keys" SET "deadline" = (CAST(CURRENT_TIMESTAMP AS timestamp) + make_interval(days := 14)) WHERE (("id" = 1) AND ("deadline" > CURRENT_TIMESTAMP))
13:36:54 web.1  |   ↳ activerecord (7.2.0) lib/active_record/log_subscriber.rb:122:in `log_query_source'
13:36:54 web.1  |   Sequel (0.3ms)  SELECT "key" FROM "account_remember_keys" WHERE (("id" = 1) AND ("deadline" > CURRENT_TIMESTAMP)) LIMIT 1
13:36:54 web.1  |   ↳ activerecord (7.2.0) lib/active_record/log_subscriber.rb:122:in `log_query_source'
13:36:54 web.1  |   Sequel (0.3ms)  SELECT "deadline" FROM "account_remember_keys" WHERE (("id" = 1) AND ("deadline" > CURRENT_TIMESTAMP)) LIMIT 1
13:36:54 web.1  |   ↳ activerecord (7.2.0) lib/active_record/log_subscriber.rb:122:in `log_query_source'
13:36:54 web.1  |   
13:36:54 web.1  | KeyError (key not found: "omniauth.auth"):
13:36:54 web.1  |   
13:36:54 web.1  | rodauth-omniauth (0.3.4) lib/rodauth/features/omniauth_base.rb:81:in `fetch'
13:36:54 web.1  | rodauth-omniauth (0.3.4) lib/rodauth/features/omniauth_base.rb:81:in `block (3 levels) in <module:Rodauth>'
13:36:54 web.1  | rodauth-omniauth (0.3.4) lib/rodauth/features/omniauth_base.rb:75:in `block (3 levels) in <module:Rodauth>'
13:36:54 web.1  | rodauth-omniauth (0.3.4) lib/rodauth/features/omniauth.rb:46:in `handle_omniauth_callback'
13:36:54 web.1  | rodauth-omniauth (0.3.4) lib/rodauth/features/omniauth.rb:41:in `route_omniauth!'
13:36:54 web.1  | rodauth-omniauth (0.3.4) lib/rodauth/features/omniauth_base.rb:45:in `route!'

image

janko commented 2 weeks ago

Are you using protect_from_forgery with: :null_session (or with no :with)? That would cause the request to proceed just with an empty session.

Yes, I agree rodauth-omniauth should handle this, probably by converting it into a 4xx error 👍🏻

FLX-0x00 commented 2 weeks ago

protect_from_forgery does not cover GET or HEAD requests, so I don't think this would be an impact. We use it with only small exceptions for the API. Is there anything you need from us to debug that?

janko commented 2 weeks ago

I don't think that authenticity token plays a role here, because as you said CSRF protection isn't activated on GET/HEAD requests. The scenario that triggers this error seems to be making a GET request to the request phase, while only POST requests are allowed. In this case, the OmniAuth app will not handle the request, but omniauth.auth won't be set either.

I just pushed a fix to master, would you mind testing it out and let me know if it fixes the issue? I will then publish a release shortly.

FLX-0x00 commented 2 weeks ago

@janko Thank you! The 5xx is gone and I receive a 404 with the github master branch. Awesome.

janko commented 2 weeks ago

Great, just released 0.4.0 with these changes 🙂