search

workarea-commerce / workarea

Workarea is an enterprise-grade Ruby on Rails commerce platform
https://www.workarea.com
Other
326 stars 66 forks source link

RefererParser::InvalidUriError (issue for builds from 3.4.0-beta1 to 3.5.0-beta1) #531

Closed GesJeremie closed 3 years ago

GesJeremie commented 3 years ago

Edit: We probably want to label this not as a bug but more as enhancement (my bad)

Describe the bug Sentry is reporting an error for one of our build:

image

Investigation

Bonus

If anyone is running into the same issue and are not able to upgrade at the moment, here's my fix (v3.4.32):

# lib/workarea/ext/freedom_patches/referer_parser.rb
module RefererParser
  class Parser
    #
    # Patched to take in account the new scheme `android-app`.
    #
    # See: https://github.com/snowplow-referer-parser/referer-parser/pull/145/commits/737a892f628347a610e5633d62153c8d6518f4e5
    #
    # The PR above has never been officialy released, therefore the patch.
    #
    def parse(obj)
      url = obj.is_a?(URI) ? obj : URI.parse(obj.to_s)

      # This section changed
      if !['android-app', 'http', 'https'].include?(url.scheme)
        raise InvalidUriError.new("Only Android-App, HTTP and HTTPS schemes are supported -- #{url.scheme}")
      end

      data = { :known => false, :uri => url.to_s }

      domain, name_key = domain_and_name_key_for(url)
      if domain and name_key
        referer_data = @name_hash[name_key]
        data[:known] = true
        data[:source] = referer_data[:source]
        data[:medium] = referer_data[:medium]
        data[:domain] = domain

        # Parse parameters if the referer uses them
        if url.query and referer_data[:parameters]
          query_params = CGI.parse(url.query)
          referer_data[:parameters].each do |param|
            # If there is a matching parameter, get the first non-blank value
            if !(values = query_params[param]).empty?
              data[:term] = values.select { |v| v.strip != "" }.first
              break if data[:term]
            end
          end
        end
      end

      data
    rescue URI::InvalidURIError
      raise InvalidUriError.new("Unable to parse URI, not a URI? -- #{obj.inspect}", $!)
    end
  end
end
# test/lib/workarea/ext/freedom_patches/referer_parser_test.rb
# A silly test to sleep well at night
require 'test_helper'

module RefererParser
  class ParserTest < Workarea::TestCase
    def test_parse_android_app_referrers
      referrers = [
        'android-app://com.linkedin.android',
        'android-app://org.telegram.plus',
        'android-app://com.twitter.android'
      ]

      assert_nothing_raised do
        referrers.each do |referrer|
          Parser.new.parse(referrer)
        end
      end
    end
  end
end
# You probably want to override Workarea::CurrentReferrer as well to backport the rescue
# app/controllers/workarea/current_referrer.rb
module Workarea
  module CurrentReferrer
    #
    # Base implementation (workarea-core v3.4.32) is prone
    # to raise an error for `RefererParser::Parser.new.parse(referrer)`
    # which will block the user to be able to checkout properly.
    #
    # @source https://github.com/workarea-commerce/workarea/blob/v3.4.32/core/app/controllers/workarea/current_referrer.rb
    #
    # The latest implementation (workarea-core 3.5.18) mitigates the problem
    # by rescue-ing the attributes.
    #
    # @source https://github.com/workarea-commerce/workarea/blob/v3.5.18/core/lib/workarea/visit.rb#L72
    #
    # I took inspiration from the latest implementation and applied the changes here.
    #
    def current_referrer
      return @current_referrer if defined?(@current_referrer)

      referrer = cookies['workarea_referrer']
      return unless referrer.present?

      # This rescue is gold, you don't want the user to reach a 404 during checkout ===> -$$$$
      attributes = RefererParser::Parser.new.parse(referrer) rescue {}

      @current_referrer ||= TrafficReferrer.new(
        attributes.slice(:source, :medium, :uri)
      )
    end
  end
end
GesJeremie commented 3 years ago

After more thinking, we probably want to close this ticket:

Anyway, sorry for all the noise :)

bencrouse commented 3 years ago

I agree that if this is interrupting checkout, it's worth fixing for v3.4. I've made a note for our next sprint. Thanks for bringing it up!