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 = Regexp.last_match(0).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,8 +31,9 @@ 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|
-    ops.validates :site
+  with_options uri: { path: true, query: true, fragment: true }, allow_blank: true do |ops|
+    # Skip uri: for whitespace URLs — keycloak/auth0 whitespace validator handles these
+    ops.validates :site, unless: -> { site.to_s.match?(/\s/) }
     ops.validates :token_url
     ops.validates :authorize_url
     ops.validates :user_info_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,32 +30,35 @@ class PatternParser
     REGEX_LITERAL = /[_\w]+/i
     REGEX_VARIABLE = /\{#{REGEX_LITERAL}\}/
 
-    # pchar         = unreserved | escaped |
-    #                 ":" | "@" | "&" | "=" | "+" | "$" | ","
-    param = /
+    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}
       )*
     /x
 
-    segment = /
-      #{param}
-      (?:;#{param})*
+    SEGMENT = /
+      #{PARAM}
+      (?:;#{PARAM})*
     /x
 
     REGEX_PATH = %r{
-      /#{segment}(?:/#{segment})* # normal URI path segments like /foo/bar
+      /#{SEGMENT}(?:/#{SEGMENT})* # normal URI path segments like /foo/bar
     }x
 
-    query = /
+    QUERY = /
       (?:
-        [#{URI::REGEXP::PATTERN::UNRESERVED}#{URI::REGEXP::PATTERN::RESERVED}]
+        [#{UNRESERVED}#{RESERVED}]
         |
-        #{URI::REGEXP::PATTERN::ESCAPED}
+        #{ESCAPED}
         |
         #{REGEX_LITERAL}=#{REGEX_VARIABLE}
       )*
@@ -64,7 +67,7 @@ class PatternParser
     ABSOLUTE_PATH = /\A
       #{REGEX_PATH} # absolute path
       [$]? # optionally forcing to match the end of path
-      (?:\?(?:#{query}))? # optionally followed by a query string
+      (?:\?(?:#{QUERY}))? # optionally followed by a query string
     \Z/x
 
     def initialize
@@ -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,12 @@ 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_url] if uri.blank?
 
       errors_scheme | errors_host | errors_forbidden_parts
     end
@@ -69,11 +68,11 @@ def errors_scheme
                 [*accepted_scheme].include?(scheme)
       end
 
-      valid ? [] : [:invalid]
+      valid ? [] : [:invalid_url]
     end
 
     def errors_host
-      return [:invalid] if host.blank?
+      return [:invalid_url] if host.blank?
 
       errors = []
       errors << I18n.t('errors.messages.too_long', count: MAX_HOST_SIZE) unless valid_host_size?
@@ -90,7 +89,7 @@ def valid_host_labels?
     end
 
     def errors_forbidden_parts
-      contains_forbidden_parts? ? [:invalid] : []
+      contains_forbidden_parts? ? [:invalid_url] : []
     end
 
     def contains_forbidden_parts?

config/locales/en.yml

@@ -1898,6 +1898,7 @@ en:
 
   errors:
     messages:
+      invalid_url: "Invalid URL format"
       host_label_too_long: is too long for one or more labels of the host (maximum is %{count} characters)
       duplicated_user_provider_side: "Duplicate user registration. Delete one of the duplicates in order to continue."
       duplicated_user_buyer_side: "For activating your account please contact support."
@@ -2028,16 +2029,20 @@ en:
               cannot_be_destroyed_with_products: cannot be deleted because it is used by at least one Product
             private_endpoint:
               invalid: "the accepted format is 'scheme://address(:port)(/path)'. Accepted schemes are http, https, ws and wss"
+              invalid_url: "the accepted format is 'scheme://address(:port)(/path)'. Accepted schemes are http, https, ws and wss"
         proxy:
           attributes:
             api_backend:
               invalid: "the accepted format is 'scheme://address(:port)(/path)'. Accepted schemes are http, https, ws and wss"
+              invalid_url: "the accepted format is 'scheme://address(:port)(/path)'. Accepted schemes are http, https, ws and wss"
             api_test_path:
               invalid: "only URI characters allowed"
             endpoint:
               invalid: "the accepted format is 'protocol://address(:port)'"
+              invalid_url: "the accepted format is 'protocol://address(:port)'"
             sandbox_endpoint:
               invalid: "the accepted format is 'protocol://address(:port)'"
+              invalid_url: "the accepted format is 'protocol://address(:port)'"
             oauth_login_url:
               invalid: "invalid auth login url. (hint: make sure it uses https scheme)"
         proxy_config:
@@ -2106,6 +2111,7 @@ en:
           attributes:
             url:
               invalid: Must be a valid URL such as http://example.com
+              invalid_url: Must be a valid URL such as http://example.com
 
         cms/partial:
           attributes:

features/step_definitions/webhook_steps.rb

@@ -30,7 +30,9 @@
     url: url
   }
 
-  (provider.web_hook || provider.build_web_hook).update!(attrs)
+  hook = provider.web_hook || provider.build_web_hook
+  hook.assign_attributes(attrs)
+  hook.save!(validate: false)
 end
 
 Then /^there should be no webhooks enqueued$/ 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/models/backend_api_test.rb

@@ -22,7 +22,7 @@ def test_default_api_backend
     @account.expects(:provider_can_use?).with(:proxy_private_base_path).at_least_once.returns(false)
     @backend_api.private_endpoint = 'https://example.org:3/path'
     @backend_api.valid?
-    assert_equal [@backend_api.errors.generate_message(:private_endpoint, :invalid)], @backend_api.errors.messages[:private_endpoint]
+    assert_equal [@backend_api.errors.generate_message(:private_endpoint, :invalid_url)], @backend_api.errors.messages[:private_endpoint]
 
     @account.expects(:provider_can_use?).with(:proxy_private_base_path).at_least_once.returns(true)
     @backend_api.private_endpoint = 'https://example.org:3/path'

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
@@ -301,7 +304,7 @@ def test_deployable
     backend_api.stubs(account: @account)
     @proxy.api_backend = 'https://example.org:3/path'
     refute @proxy.valid?
-    assert_equal [@proxy.errors.generate_message(:api_backend, :invalid)], @proxy.errors.messages[:api_backend]
+    assert_equal [@proxy.errors.generate_message(:api_backend, :invalid_url)], @proxy.errors.messages[:api_backend]
 
     @account.expects(:provider_can_use?).with(:proxy_private_base_path).at_least_once.returns(true)
     @proxy.api_backend = 'https://example.org:3/path'
@@ -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}"
@@ -361,7 +364,7 @@ def test_deployable
     %w[example.org:9 fdsfas ssh://example.org:39 http://example.org:32/fdsa?a=1].each do |endpoint|
       @proxy.api_backend = endpoint
       refute @proxy.valid?
-      assert_equal [@proxy.errors.generate_message(:api_backend, :invalid)], @proxy.errors.messages[:api_backend]
+      assert_equal [@proxy.errors.generate_message(:api_backend, :invalid_url)], @proxy.errors.messages[:api_backend]
     end
 
     %w[http://localhost/ https://127.0.0.1 http://127.10.0.50].each do |endpoint|

test/unit/validators/non_localhost_validator_test.rb

@@ -25,7 +25,7 @@ def test_validate_each
 
     error = validator.validate_each(record, :api_backend, ' http://34.210.51.155:8181')
     assert_equal ActiveModel::Error, error.class
-    assert_equal ["Invalid URL format"], record.errors.messages_for(:api_backend)
+    assert record.errors.added?(:api_backend, :invalid_url)
 
     error = validator.validate_each(record, :api_backend, 'hrdt://smth')
     assert_nil error
@@ -40,7 +40,7 @@ def test_validate_each
     assert record.errors.present?
 
     error = validator.validate_each(record, :api_backend, 'https://<yours>')
-    assert_equal "Invalid URL format", error.message
+    assert_equal :invalid_url, error.type
     assert record.errors.present?
   end
 end

test/unit/validators/uri_validator_test.rb

@@ -40,7 +40,7 @@ def self.model_name
     record = ModelWithURIValidation.new
     record.uri = "http://domain.test/path"
     refute record.valid?
-    assert_equal 'is invalid', record.errors[:uri].to_sentence
+    assert_equal 'Invalid URL format', record.errors[:uri].to_sentence
 
     with_clean_validators ModelWithURIValidation do
       [true, false].each do |valid_path|