Rescue StandardError makes all development errors failures

[pgwillia]

Since bumping omniauth from 1.9.1 to 2.0.1 in our application all development errors have become failures. For example I purposefully introduce an error to a view. It is caught by omniauth and calls fail which means that we can no longer use better_errors to debug.

195:       rescue StandardError => e
    196:        binding.pry
 => 197:         return fail!(e.message, e)
    198:       end
    199: 
    200:       @app.call(env)
    201:     end

[1] pry(#<OmniAuth::Strategies::SAML>)> e
=> #<ActionView::Template::Error: undefined local variable or method `oops' for #<#<Class:0x00007f533c178068>:0x000055e0bcf640a8>>

Is there something that we misconfigured in omniauth setup? We catch the failures to give users a message in their browser
https://github.com/ualbertalib/jupiter/blob/fe8409b9d4b4345e409c91a0cf6744b9ce4b91ba/config/initializers/omniauth.rb#L22
We can make this conditional based on environment but wondering if there is a better way?

omniauth/lib/omniauth/strategy.rb

Lines 190 to 197 in 40e354c

	begin
	return options_call if on_auth_path? && options_request?
	return request_call if on_request_path? && OmniAuth.config.allowed_request_methods.include?(request.request_method.downcase.to_sym)
	return callback_call if on_callback_path?
	return other_phase if respond_to?(:other_phase)
	rescue StandardError => e
	return fail!(e.message, e)
	end

[BobbyMcWho]

You’ll want to take a look at the OmniAuth.config.on_failure setting, by default it’s set to call the OmniAuth Failure Endpoint

[BobbyMcWho]

If you're looking to preserve raising the errors out in development I'd suggest something like

on_failure do |env|
  if Rails.env.development?
    OmniAuth::FailureEndpoint.call(env)
  else
    SessionsController.action(:failure).call(env)
  end
end

[pgwillia]

Yes. That's effectively what we've done. We still think it's strange that omniauth is intercepting all errors (not just the ones relevant to omniauth).

[BobbyMcWho]

I'll look into it a bit more, it really should only be catching errors that occur during the auth process

[trysmr]

Hi, I have the same problem. I use a omniauth-saml gem.

I found the omniauth-saml gem calls the call_app! method in the other_phase method. All errors have become failures and logs as "(saml) Authentication failure!" in the fail! method if an error occurs regardless of the auth process.

[BobbyMcWho]

Thanks for the report and additional information @trysmr, I think this is a bug in omniauth-saml and will release a patch for that gem.

[pgwillia]

I saw #1032 and it is similar. The error behaviour changed with the release of 2.0.2. Now the failure mode is a redirect loop and the error is printed in the console.

This is printed 21x in the console

[BobbyMcWho]

I have been looking into this, I haven't decided the best course of action yet. @pgwillia it looks like your session controller isn't handling the error but just redirecting to root?

[pgwillia]

@BobbyMcWho good observation. That's what we've told it to do.
https://github.com/ualbertalib/jupiter/blob/7a5ce302771c471b5411bec24c4f4c2e61babb4d/app/controllers/sessions_controller.rb#L55-L57
That explains the redirect loop because the error I introduced is part of our welcome page.

@trysmr has summarized the issue that still persists.

[BobbyMcWho]

So looking into a few strategies and how they have implemented the other_phase, it seems like they almost all call call_app! which is why this block will capture those application errors. I may have to pull that other phase out of the error handling capture.

[trysmr]

Since the other_phase is not a common process for each omniauth strategy, I think it is better to leave its error handling to each strategy.

I am facing the redirect loop problem too. Luckily, I don't implement /metadata, /slo, /spslo paths, so I have temporarily undefined it now.

[trysmr]

I just thought of one. How about adding an on_auth_path? method like this?

begin
  return options_call if on_auth_path? && options_request?
  return request_call if on_request_path? && OmniAuth.config.allowed_request_methods.include?(request.request_method.downcase.to_sym)
  return callback_call if on_callback_path?
  return other_phase if on_auth_path? && respond_to?(:other_phase)
rescue StandardError => e
  return fail!(e.message, e)
end

[bisrael]

Ran into this today too, when when using omniauth with devise this totally masks any errors that happen.

It also makes it very very difficult to find out why things are failing.

Rescuing StandardError should be considered an anti-pattern; you could instead scope the errors you catch to specifically authentication errors.

[bisrael]

Unfortunately, the commit that adds the rescue does not explain why the rescue was added.

Perhaps just undo it since there's no rationale included?

ee2ef21

[BobbyMcWho]

@bisrael this was the PR in which it was added #689

[BobbyMcWho]

If anyone can try out the branch for this pr and let me know if it resolves it for you: #1035