Shopify/omniauth-shopify-oauth2

build_access_token raises un-rescued Faraday error that bubble to apps using shopify-app

ewalk153 opened this issue · 0 comments

We expect that the underlying omniauth gem will reliably raise a connection errors with a fail when we call build_access_token:

https://github.com/omniauth/omniauth-oauth2/blob/10e1a42c7a49ad4488fe9085f814681e8848fbf6/lib/omniauth/strategies/oauth2.rb#L66-L83

      def callback_phase # rubocop:disable AbcSize, CyclomaticComplexity, MethodLength, PerceivedComplexity
        error = request.params["error_reason"] || request.params["error"]
        if error
          fail!(error, CallbackError.new(request.params["error"], request.params["error_description"] || request.params["error_reason"], request.params["error_uri"]))
        elsif !options.provider_ignores_state && (request.params["state"].to_s.empty? || request.params["state"] != session.delete("omniauth.state"))
          fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected"))
        else
          self.access_token = build_access_token
          self.access_token = access_token.refresh! if access_token.expired?
          super
        end
      rescue ::OAuth2::Error, CallbackError => e
        fail!(:invalid_credentials, e)
      rescue ::Timeout::Error, ::Errno::ETIMEDOUT => e
        fail!(:timeout, e)
      rescue ::SocketError => e
        fail!(:failed_to_connect, e)
      end

The underlying connection can close, which normally raises a Faraday ConnectionFailed error.

vendor/ruby-2.5.3/lib/ruby/2.5.0/net/protocol.rb:189:in `rbuf_fill': end of file reached (Faraday::ConnectionFailed)
    from vendor/ruby-2.5.3/lib/ruby/2.5.0/net/protocol.rb:157:in `readuntil'
    from vendor/ruby-2.5.3/lib/ruby/2.5.0/net/protocol.rb:167:in `readline'
    from vendor/ruby-2.5.3/lib/ruby/2.5.0/net/http/response.rb:40:in `read_status_line'
    from vendor/ruby-2.5.3/lib/ruby/2.5.0/net/http/response.rb:29:in `read_new'
[snip]

Recently, we saw some internal app fail to authenticate in a new fashion which stress a new failure mode to these request.

Options that I I see:

  1. We rescue in the omniauth-shopify-oauth2 as the oauth code path in shopify-app is merely asking for a token and shouldn't care about why it failed
  2. We push this concern up to the superclass omniauth-oauth2 to make sure Faraday errors are rescued
  3. We let it bubble up to shopify-app and rescue it there

In any event, this should not reach Shopify apps without being first handled.