THREESCALE-15550: Upgrade uri gem

Gemfile

@@ -71,13 +71,7 @@ gem 'formtastic', '~> 5.0'
 gem 'htmlentities', '~>4.3', '>= 4.3.4'
 gem 'json', '~> 2.7', '>= 2.7.1'
 gem 'responders', '~> 3.2' # For respond_with support
-# uri >= 1.0.0 switched the default parser from RFC2396 to RFC3986 (ruby/uri#107),
-# removed URI::DEFAULT_PARSER, dropped URI.decode, and the RFC3986 parser has no
-# `registry` component. We rely on all of these across models (Proxy, ProxyRule,
-# WebHook, AuthenticationProvider) and controllers. Upgrading is worthwhile but
-# requires careful inspection of every call-site and should be deployed/tested
-# separately from the Faraday update.
-gem 'uri', '< 1.0.0'
+gem 'uri', '~> 1.0'
 
 gem 'mysql2', '~> 0.5.3'
 

Gemfile.lock

@@ -975,7 +975,7 @@ GEM
       rack
       unicorn
     uniform_notifier (1.18.0)
-    uri (0.13.3)
+    uri (1.1.1)
     useragent (0.16.11)
     version_gem (1.1.3)
     webmock (3.24.0)
@@ -1170,7 +1170,7 @@ DEPENDENCIES
   uglifier
   unicorn
   unicorn-rails
-  uri (< 1.0.0)
+  uri (~> 1.0)
   webmock (~> 3.24.0)
   webrick (~> 1.8.2)
   will_paginate (~> 3.3)

app/controllers/provider/admin/redhat/auth_controller.rb

@@ -31,7 +31,7 @@ def authentication_provider
   def referrer_url
     url = params.permit(:referrer)[:referrer]
     if url
-      URI.decode(url)
+      CGI.unescapeURIComponent(url)
     else
       provider_admin_account_path
     end

app/controllers/provider/admin/service_discovery/auth_controller.rb

@@ -24,9 +24,9 @@ def show
   protected
 
   def referrer_url
-    url = params[:referrer]
+    url = params.permit(:referrer)[:referrer]
     if url
-      URI.decode(url)
+      CGI.unescapeURIComponent(url)
     else
       new_admin_service_path
     end

app/helpers/messages_helper.rb

@@ -24,9 +24,8 @@ def message_subject(message)
   def hyperlink_urls(text)
     text = h(text)
 
-    text.scan(URI.regexp(%w(http https))) do
-      #$& contains the whole match of the regural expression
-      url = $&.sub(/\.$/, '').sub(/\:$/,'')
+    text.scan(%r{https?://[^\s)\]>]+}) do
+      url = $&.sub(/[.:]+\z/, '')
       text = text.sub(url, link_to(url, url))
     end
 

app/lib/three_scale/oauth2/service_discovery_client.rb

@@ -74,7 +74,7 @@ def query_options
         def call
           url = super
           # OpenShift builtin OAuth has a serious bug, it does not store correctly the redirect_uri
-          ServiceDiscovery::Config.rh_sso? ? url : URI.decode(url)
+          ServiceDiscovery::Config.rh_sso? ? url : CGI.unescapeURIComponent(url)
         rescue => e
           # Do better error management
           Rails.logger.debug("[Openshift OAuth] Error decoding callback URL for builtin <%s>. Error: %s\n%s" % [url.inspect, e.message, e.backtrace.join("\n")])

app/models/authentication_provider.rb

@@ -31,7 +31,7 @@ class AuthenticationProvider < ApplicationRecord
 
   validates :client_id, :client_secret, presence: true, if: :oauth_config_required?
 
-  with_options format: { with: URI::DEFAULT_PARSER.make_regexp(%w[http https]), allow_blank: true, message: :invalid_url } do |ops|
+  with_options uri: { path: true, query: true, fragment: true }, allow_blank: true do |ops|
     ops.validates :site
     ops.validates :token_url
     ops.validates :authorize_url

app/models/proxy.rb

@@ -29,12 +29,15 @@ class Proxy < ApplicationRecord # rubocop:disable Metrics/ClassLength
 
   validates :error_status_no_match, :error_status_auth_missing, :error_status_auth_failed, :error_status_limits_exceeded, presence: true
 
-  uri_pattern = URI::DEFAULT_PARSER.pattern
+  pchar = "(?:[\\-_.!~*'()a-zA-Z\\d:@&=+$,]|%[a-fA-F\\d]{2})"
+  segment = "#{pchar}*(?:;#{pchar}*)*"
+  abs_path = "/#{segment}(?:/#{segment})*"
+  query = "(?:[\\-_.!~*'()a-zA-Z\\d;/?:@&=+$,\\[\\]]|%[a-fA-F\\d]{2})*"
+  optional_query = "(?:\\?(#{query}))?"
+  URI_PATH_PART = Regexp.new('\A' + abs_path + optional_query + '\z')
+  HOST = /\A(?:[a-zA-Z0-9\-._]|%\h\h)+(?::\d+)?\z/
 
   URI_OR_LOCALHOST  = /\A(https?:\/\/([a-zA-Z0-9._:\/?-])+|.*localhost.*)\Z/
-  OPTIONAL_QUERY_FORMAT = "(?:\\?(#{uri_pattern.fetch(:QUERY)}))?"
-  URI_PATH_PART = Regexp.new('\A' + uri_pattern.fetch(:ABS_PATH) + OPTIONAL_QUERY_FORMAT + '\z')
-  HOST = Regexp.new('\A' + uri_pattern.fetch(:HOSTNAME) + '(:\d+)?' + '\z')
 
   OAUTH_PARAMS = /(\?|&)(scope=|state=|tok=)/
 
@@ -675,7 +678,7 @@ def call(attribute)
 
       begin
         uri = URI.parse(attribute_value)
-        value = URI::Generic.new(uri.scheme, uri.userinfo, uri.host, uri.port, uri.registry, uri.path, uri.opaque, uri.query, uri.fragment).to_s
+        value = URI::Generic.new(uri.scheme, uri.userinfo, uri.host, uri.port, nil, uri.path, uri.opaque, uri.query, uri.fragment).to_s
         @model[attribute] = value unless @model[attribute] == value
       rescue URI::InvalidURIError
         @model.errors.add(attribute, 'Invalid domain')

app/models/proxy_rule.rb

@@ -30,13 +30,16 @@ class PatternParser
     REGEX_LITERAL = /[_\w]+/i
     REGEX_VARIABLE = /\{#{REGEX_LITERAL}\}/
 
-    # pchar         = unreserved | escaped |
-    #                 ":" | "@" | "&" | "=" | "+" | "$" | ","
+    UNRESERVED = "\\-_.!~*'()a-zA-Z\\d"
+    ESCAPED    = "%[a-fA-F\\d]{2}"
+    RESERVED   = ";/?:@&=+$,\\[\\]"
+
+    # pchar = unreserved / pct-encoded / ":" / "@" / "&" / "=" / "+" / "," ($ excluded intentionally)
     param = /
       (?:
-        [#{URI::REGEXP::PATTERN::UNRESERVED}:@&=+,] # note that $ is in the RFC but is removed for our purpposes
+        [#{UNRESERVED}:@&=+,] # note that $ is in the RFC but is removed for our purpposes
         |
-        #{URI::REGEXP::PATTERN::ESCAPED}
+        #{ESCAPED}
         |
         #{REGEX_VARIABLE}
       )*
@@ -53,9 +56,9 @@ class PatternParser
 
     query = /
       (?:
-        [#{URI::REGEXP::PATTERN::UNRESERVED}#{URI::REGEXP::PATTERN::RESERVED}]
+        [#{UNRESERVED}#{RESERVED}]
         |
-        #{URI::REGEXP::PATTERN::ESCAPED}
+        #{ESCAPED}
         |
         #{REGEX_LITERAL}=#{REGEX_VARIABLE}
       )*
@@ -89,7 +92,7 @@ def call(_)
   validates :http_method, inclusion: { in: ALLOWED_HTTP_METHODS }
   validate :non_repeated_parameters
   validate :no_vars_in_keys
-  validates :redirect_url, format: URI::DEFAULT_PARSER.make_regexp(%w[http https]), allow_blank: true, length: { maximum: 10000 }
+  validates :redirect_url, uri: { path: true, query: true, fragment: true }, allow_blank: true, length: { maximum: 10000 }
 
   def parameters
     Addressable::Template.new(path_pattern).variables

app/models/web_hook.rb

@@ -4,7 +4,8 @@ class WebHook < ApplicationRecord
   alias provider account
 
   validates :account_id, presence: true
-  validates :url, format: { :with => URI::DEFAULT_PARSER.make_regexp(%w[http https]), :if => :active }, length: { maximum: 255 }
+  validates :url, uri: { path: true, query: true, fragment: true }, if: :active
+  validates :url, length: { maximum: 255 }
 
   #TODO: limit association only to providers?
   #TODO validate url as url?

app/validators/uri_validator.rb

@@ -5,9 +5,7 @@ class UriValidator < ActiveModel::EachValidator
   DEFAULT_PERMISSIONS_OF_PARTS = {
     port:     true,
     userinfo: false,
-    registry: false,
     path:     false,
-    opaque:   false,
     query:    false,
     fragment: false
   }.freeze
@@ -50,11 +48,11 @@ def initialize(uri:, permissions_parts:, accepted_scheme:)
       @accepted_scheme = accepted_scheme
     end
 
-    attr_reader :uri, :permissions_parts, :accepted_scheme, :generic_error_message
+    attr_reader :uri, :permissions_parts, :accepted_scheme
     delegate :host, :scheme, to: :uri
 
     def errors
-      return [generic_error_message] if uri.blank?
+      return [:invalid] if uri.blank?
 
       errors_scheme | errors_host | errors_forbidden_parts
     end

test/integration/admin/api/account/authentication_providers_controller_test.rb

@@ -61,10 +61,10 @@ def setup
     assert_equal ["can't be blank"], JSON.parse(response.body).dig('errors', 'site')
 
     post admin_api_account_authentication_providers_path(authentication_provider_params(different_attributes: {kind: 'keycloak', site: 'invalid'}))
-    assert_equal ['Invalid URL format'], JSON.parse(response.body).dig('errors', 'site')
+    assert_equal ['invalid'], JSON.parse(response.body).dig('errors', 'site')
 
     post admin_api_account_authentication_providers_path(authentication_provider_params(different_attributes: {kind: 'keycloak', site: '  http://example.com  '}))
-    assert_equal ["can't contain whitespaces"], JSON.parse(response.body).dig('errors', 'site')
+    assert_equal ["invalid", "can't contain whitespaces"], JSON.parse(response.body).dig('errors', 'site')
   end
 
   test '#create ensures provider can use provider_sso' do

test/integration/admin/api/authentication_providers_controller_test.rb

@@ -79,10 +79,10 @@ def setup
     assert_equal ["can't be blank"], JSON.parse(response.body).dig('errors', 'site')
 
     post admin_api_authentication_providers_path(authentication_provider_params(different_attributes: {kind: 'keycloak', site: 'invalid'}))
-    assert_equal ['Invalid URL format'], JSON.parse(response.body).dig('errors', 'site')
+    assert_equal ['invalid'], JSON.parse(response.body).dig('errors', 'site')
 
     post admin_api_authentication_providers_path(authentication_provider_params(different_attributes: {kind: 'keycloak', site: '  http://example.com  '}))
-    assert_equal ["can't contain whitespaces"], JSON.parse(response.body).dig('errors', 'site')
+    assert_equal ["invalid", "can't contain whitespaces"], JSON.parse(response.body).dig('errors', 'site')
   end
 
   test '#update saves the new attributes values' do

test/integration/provider/admin/redhat/auth_controller_test.rb

@@ -38,6 +38,19 @@ def setup
     end
   end
 
+  test 'callback decodes referrer url' do
+    login_provider @provider
+    host! @provider.external_admin_domain
+    user_data = ThreeScale::OAuth2::UserData.new(username: 'redhat_user')
+    ThreeScale::OAuth2::KeycloakClient.any_instance.stubs(:authenticate!).returns(user_data)
+
+    get @callback_url, params: { referrer: '/p/admin/dashboard%3Ffoo%3Dbar' }
+    assert_redirected_to '/p/admin/dashboard?foo=bar'
+
+    get @callback_url, params: { referrer: '/p/admin/search?q=hello+world' }
+    assert_redirected_to '/p/admin/search?q=hello+world'
+  end
+
   test 'Red Hat Customer Portal disabled' do
     ThreeScale.config.redhat_customer_portal.stubs(enabled: false)
 

test/unit/authentication_provider_test.rb

@@ -42,7 +42,7 @@ class AuthenticationProviderTest < ActiveSupport::TestCase
     errors = authentication_provider.errors
 
     assert_equal %i[authorize_url site token_url user_info_url], errors.map(&:attribute).sort
-    assert_equal ['Invalid URL format'], errors.map(&:message).uniq
+    assert_equal ['invalid'], errors.map(&:message).uniq
 
     authentication_provider.site = 'https://example.org'
     authentication_provider.token_url = 'http://example.org'

test/unit/helpers/messages_helper_test.rb

@@ -121,6 +121,11 @@ def test_if_links_with_colon_at_the_end_are_shown_properly_with_http_links
                  hyperlink_urls('Some http://google.com: text')
   end
 
+  def test_if_links_with_query_string_are_fully_captured
+    assert_equal 'Visit <a href="http://google.com/path?q=1&amp;amp;r=2">http://google.com/path?q=1&amp;amp;r=2</a> now',
+                 hyperlink_urls('Visit http://google.com/path?q=1&r=2 now')
+  end
+
   # this is regression test for: https://github.com/3scale/system/issues/4819
   def test_if_random_word_with_colon_at_the_end_is_not_treated_like_link
     assert_equal %(Some random word:),

test/unit/proxy_test.rb

@@ -269,6 +269,9 @@ def test_deployable
 
     @proxy.hostname_rewrite = 'my-own.api.example.net::80'
     refute @proxy.valid?
+
+    @proxy.hostname_rewrite = 'my_proxy.internal'
+    assert @proxy.valid?
   end
 
   test 'backend' do
@@ -342,7 +345,7 @@ def test_deployable
   end
 
   test 'api_test_path formats valid' do
-    [  '/', '/i/m/a/lumberjack/42', '/~stuff', '/!not_-here']. each do |path|
+    [  '/', '/i/m/a/lumberjack/42', '/~stuff', '/!not_-here', '/path?key=value&other=123']. each do |path|
       @proxy.api_test_path = path
       @proxy.valid?
       assert_empty @proxy.errors[:api_test_path], "errors found on - #{path}"

test/unit/web_hook_test.rb

@@ -71,6 +71,32 @@ class PushingBehaviourTest < ActiveSupport::TestCase
     end
   end
 
+  test 'validate url' do
+    hook = FactoryBot.build_stubbed(:web_hook, active: true)
+
+    hook.url = 'http://example.com'
+    assert_valid hook
+
+    hook.url = 'https://example.com/webhook?token=abc'
+    assert_valid hook
+
+    hook.url = 'foo'
+    refute_valid hook
+    assert hook.errors[:url].present?
+
+    hook.url = 'ftp://example.com'
+    refute_valid hook
+    assert hook.errors[:url].present?
+
+    hook.url = ''
+    refute_valid hook
+    assert hook.errors[:url].present?
+
+    hook.active = false
+    hook.url = 'not-a-url'
+    assert_valid hook
+  end
+
   test '#ping' do
     hook = WebHook.new(url: "http://foo")