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(UriPatterns::HYPERLINK_SCANNER) do
+      url = Regexp.last_match(0).sub(/\.$/, '').sub(/:$/, '')
       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/lib/uri_patterns.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+# The uri gem's RFC3986_PARSER does not provide composable string-pattern
+# constants (.pattern only exists on RFC2396_PARSER).
+# These RFC2396 patterns are used intentionally to preserve compatibility.
+module UriPatterns
+  UNRESERVED = URI::RFC2396_REGEXP::PATTERN::UNRESERVED
+  ESCAPED    = URI::RFC2396_REGEXP::PATTERN::ESCAPED
+  RESERVED   = URI::RFC2396_REGEXP::PATTERN::RESERVED
+
+  HOSTNAME = URI::RFC2396_PARSER.pattern[:HOSTNAME]
+  ABS_PATH = URI::RFC2396_PARSER.pattern[:ABS_PATH]
+  QUERY    = URI::RFC2396_PARSER.pattern[:QUERY]
+
+  # URL scanner for free-form text (e.g. message bodies).
+  HYPERLINK_SCANNER = %r{https?://(?:[^\s()\[\]>]|\([^\s()]*\)|\[[^\s\[\]]*\])+}
+end

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 — each subclass 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/authentication_provider/custom.rb

@@ -1,3 +1,5 @@
 class AuthenticationProvider::Custom < AuthenticationProvider
   self.authorization_scope = :iam_tools
+
+  validates :site, format: { without: /\s/, message: :contains_whitespace }
 end

app/models/authentication_provider/github.rb

@@ -1,6 +1,8 @@
 class AuthenticationProvider::GitHub < AuthenticationProvider
   self.authorization_scope = :branding
 
+  validates :site, format: { without: /\s/, message: :contains_whitespace }
+
   after_initialize :set_defaults, unless: :persisted?
 
   state_machine :branding_state, initial: :initial_state.to_proc do

app/models/proxy.rb

@@ -29,12 +29,11 @@ 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
+  OPTIONAL_QUERY_FORMAT = "(?:\\?(#{UriPatterns::QUERY}))?"
+  URI_PATH_PART = Regexp.new('\A' + UriPatterns::ABS_PATH + OPTIONAL_QUERY_FORMAT + '\z')
+  HOST = Regexp.new('\A' + UriPatterns::HOSTNAME + '(:\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 +674,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 = UriPatterns::UNRESERVED
+    ESCAPED    = UriPatterns::ESCAPED
+    RESERVED   = UriPatterns::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

@@ -780,6 +780,10 @@ en:
           success: Referrer filter has been deleted
 
       service_discovery:
+        auth:
+          show:
+            success: Service discovery authentication was successful
+            error: Service discovery authentication failed
         services:
           create:
             error: Cannot create product
@@ -1898,6 +1902,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."
@@ -2027,17 +2032,17 @@ en:
             base:
               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:
@@ -2105,7 +2110,7 @@ en:
         web_hook:
           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/provider/admin/webhooks/edit.feature

@@ -28,10 +28,10 @@ Feature: Provider webhooks
     Then they should see a toast alert with text "http://3scale-test.org responded with 200"
 
   Scenario: Webhook endpoint is wrong
-    Given the provider has a webhook with endpoint "http:://banana"
     When they go to the edit webhooks page
-    And press "Ping!"
-    Then they should see a toast alert with text "Hostname not supplied: 'http:://banana'"
+    And the form is submitted with:
+      | URL | http:://banana |
+    Then they should see "Must be a valid URL such as http://example.com"
 
   Scenario: Webhooks switch is denied
     Given the provider has "web_hooks" switch denied

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

@@ -38,6 +38,22 @@ 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'
+
+    get @callback_url, params: { referrer: '/p/admin/hello+world' }
+    assert_redirected_to '/p/admin/hello+world'
+  end
+
   test 'Red Hat Customer Portal disabled' do
     ThreeScale.config.redhat_customer_portal.stubs(enabled: false)
 

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

@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+class Provider::Admin::ServiceDiscovery::AuthControllerTest < ActionDispatch::IntegrationTest
+  setup do
+    @provider = FactoryBot.create(:provider_account)
+    @callback_url = "/p/admin/auth/#{ServiceDiscovery::AuthenticationProviderSupport::SERVICE_DISCOVERY_SYSTEM_NAME}/callback"
+
+    ThreeScale.config.service_discovery.stubs(enabled: true, authentication_method: 'oauth')
+    Rails.application.reload_routes!
+
+    login_provider @provider
+    host! @provider.external_admin_domain
+  end
+
+  test 'callback decodes referrer url' do
+    user_data = ThreeScale::OAuth2::UserData.new(username: 'discovery_user')
+    ServiceDiscovery::OAuthConfiguration.instance.stubs(
+      token_endpoint: 'https://oauth.example.com/token',
+      authorization_endpoint: 'https://oauth.example.com/authorize',
+      userinfo_endpoint: 'https://oauth.example.com/userinfo'
+    )
+    ThreeScale::OAuth2::ServiceDiscoveryClient.any_instance.stubs(:authenticate!).returns(user_data)
+    ThreeScale::OAuth2::ServiceDiscoveryClient.any_instance.stubs(:access_token).returns(stub(token: 'tok', expires_at: 1.hour.from_now.to_i))
+
+    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'
+
+    get @callback_url, params: { referrer: '/p/admin/hello+world' }
+    assert_redirected_to '/p/admin/hello+world'
+  end
+end