Accept https redirect URIs for dynamically registered clients

app/controllers/oauth/authorizations_controller.rb

@@ -115,8 +115,11 @@ def valid_state?
       params[:state].nil? || (params[:state].is_a?(String) && params[:state].length <= STATE_MAXIMUM_LENGTH)
     end
 
+    # Errors are found before the user sees the consent screen, so they only
+    # redirect to a vetted target; Deny redirects anywhere allowed, since the
+    # consent screen has named the host by then.
     def redirect_with_error(error, description)
-      if params[:redirect_uri].present? && @client&.allows_redirect?(params[:redirect_uri])
+      if params[:redirect_uri].present? && @client&.allows_redirect?(params[:redirect_uri]) && @client.vetted_redirect?(params[:redirect_uri])
         redirect_to error_redirect_uri(error, description), allow_other_host: true
       else
         @error = error

app/controllers/oauth/clients_controller.rb

@@ -7,7 +7,7 @@ class Oauth::ClientsController < Oauth::BaseController
   before_action :require_issuance_enabled
   rate_limit to: 10, within: 1.minute, only: :create, with: :oauth_rate_limit_exceeded
   before_action :validate_redirect_uris
-  before_action :validate_loopback_uris
+  before_action :validate_redirect_uri_origins
   before_action :validate_auth_method
 
   def create
@@ -47,9 +47,9 @@ def bounded_redirect_uris?(uris)
         uris.all? { |uri| !uri.is_a?(String) || uri.length <= Oauth::Client::MAXIMUM_REDIRECT_URI_LENGTH })
     end
 
-    def validate_loopback_uris
-      unless performed? || all_loopback_uris?(params[:redirect_uris])
-        oauth_error "invalid_redirect_uri", "Only loopback redirect URIs are allowed for dynamic registration"
+    def validate_redirect_uri_origins
+      unless performed? || all_registrable_uris?(params[:redirect_uris])
+        oauth_error "invalid_redirect_uri", "Only https or local loopback redirect URIs are allowed for dynamic registration"
       end
     end
 
@@ -59,16 +59,14 @@ def validate_auth_method
       end
     end
 
-    def all_loopback_uris?(uris)
+    def all_registrable_uris?(uris)
       uris.is_a?(Array) &&
-        uris.all? { |uri| uri.is_a?(String) && valid_loopback_uri?(uri) }
+        uris.all? { |uri| uri.is_a?(String) && registrable_uri?(uri) }
     end
 
-    def valid_loopback_uri?(uri)
+    def registrable_uri?(uri)
       parsed = URI.parse(uri)
-      parsed.scheme == "http" &&
-        Oauth::LOOPBACK_HOSTS.include?(parsed.host) &&
-        parsed.fragment.nil?
+      parsed.fragment.nil? && (loopback_uri?(parsed) || https_uri?(parsed))
     rescue URI::InvalidURIError
       false
     end
@@ -77,6 +75,14 @@ def registered_client_name
       (params[:client_name] if params[:client_name].is_a?(String)).presence || "MCP Client"
     end
 
+    def loopback_uri?(parsed)
+      parsed.scheme == "http" && Oauth.plain_authority?(parsed) && Oauth.loopback_host?(parsed.host)
+    end
+
+    def https_uri?(parsed)
+      parsed.scheme == "https" && Oauth.plain_authority?(parsed) && !Oauth.loopback_host?(parsed.host)
+    end
+
     def validated_scopes
       requested = case params[:scope]
       when String then params[:scope].split

app/models/oauth.rb

@@ -7,6 +7,43 @@ def self.canonical_scope(scope)
     scope.to_s.split.include?("write") ? "read write" : "read"
   end
 
+  def self.loopback_host?(host)
+    LOOPBACK_HOSTS.include?(URI.decode_www_form_component(host.to_s).downcase)
+  rescue ArgumentError
+    # A percent-encoding that decodes to invalid UTF-8 is not a usable host.
+    # Surface it as an invalid URI so every caller — all of which already
+    # rescue URI::InvalidURIError — rejects it instead of raising a 500.
+    raise URI::InvalidURIError, "invalid percent-encoding in host"
+  end
+
+  # A host made of letters, digits, hyphens and dots, or a bracketed IPv6
+  # literal. Percent-encoding, which browsers decode, isn't plain.
+  PLAIN_HOST = /\A(?:\[[0-9a-f:.]+\]|[a-z0-9](?:[a-z0-9-]*[a-z0-9])?(?:\.[a-z0-9](?:[a-z0-9-]*[a-z0-9])?)*)\z/i
+
+  # A self-registered redirect must name its destination plainly, so that
+  # consent shows the host a browser will actually reach and loopback matching
+  # compares like with like. That means no userinfo, a plain host, and a port
+  # a browser accepts. Rejecting anything else at registration, rather than
+  # canonicalizing it wherever it's shown or compared, keeps one rule in one place.
+  def self.plain_authority?(uri)
+    uri.userinfo.nil? && plain_host?(uri.host.to_s) && uri.port.to_i.between?(1, 65535)
+  end
+
+  # Browsers parse a host whose last label is a number, decimal or 0x hex, as
+  # IPv4 (WHATWG URL's ends-in-a-number check), and read a leading zero as
+  # octal. So such a host is plain only as a dotted quad of canonical decimal
+  # octets: 0-255, with no leading zeroes.
+  DECIMAL_OCTET = /(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)/
+  DOTTED_QUAD = /\A#{DECIMAL_OCTET}(?:\.#{DECIMAL_OCTET}){3}\z/
+
+  def self.plain_host?(host)
+    if host.match?(/(?:\A|\.)(?:\d+|0x\h*)\z/i)
+      host.match?(DOTTED_QUAD)
+    else
+      host.match?(PLAIN_HOST)
+    end
+  end
+
   def self.table_name_prefix
     "oauth_"
   end

app/models/oauth/client.rb

@@ -20,12 +20,15 @@ class Oauth::Client < ApplicationRecord
   scope :dynamically_registered, -> { where dynamically_registered: true }
 
 
-  def loopback?
-    redirect_uris.all? { |uri| loopback_uri?(uri) }
+  def allows_redirect?(uri)
+    redirect_uris.include?(uri) || (loopback_uri?(uri) && matching_loopback?(uri))
   end
 
-  def allows_redirect?(uri)
-    redirect_uris.include?(uri) || (loopback? && loopback_uri?(uri) && matching_loopback?(uri))
+  # Whether we may send the browser to this redirect before the user has seen
+  # the consent screen. A self-registered https host is one nobody vetted, so
+  # bouncing to it unprompted would make us an open redirector (RFC 9700 §4.11.2).
+  def vetted_redirect?(uri)
+    !dynamically_registered? || loopback_uri?(uri)
   end
 
   def allows_scope?(requested_scope)
@@ -45,42 +48,49 @@ def redirect_uris_are_valid
     def validate_redirect_uri(uri)
       parsed = URI.parse(uri)
 
-      if parsed.fragment.present?
+      unless parsed.fragment.nil?
         errors.add :redirect_uris, "must not contain fragments"
       end
 
-      if dynamically_registered? && !valid_loopback_uri?(parsed)
-        errors.add :redirect_uris, "must be a local loopback URI for dynamically registered clients"
+      if dynamically_registered? && !valid_loopback_uri?(parsed) && !valid_https_uri?(parsed)
+        errors.add :redirect_uris, "must be an https or local loopback URI for dynamically registered clients"
       end
     rescue URI::InvalidURIError
       errors.add :redirect_uris, "includes an invalid URI"
     end
 
     def loopback_uri?(uri)
-      Oauth::LOOPBACK_HOSTS.include?(URI.parse(uri).host)
+      Oauth.loopback_host?(URI.parse(uri).host)
     rescue URI::InvalidURIError
       false
     end
 
     def valid_loopback_uri?(parsed)
-      parsed.scheme == "http" && parsed.host.in?(Oauth::LOOPBACK_HOSTS)
+      parsed.scheme == "http" && Oauth.plain_authority?(parsed) && Oauth.loopback_host?(parsed.host)
+    end
+
+    def valid_https_uri?(parsed)
+      parsed.scheme == "https" && Oauth.plain_authority?(parsed) && !Oauth.loopback_host?(parsed.host)
     end
 
     # Only the port may vary, and only for an http loopback redirect (RFC 8252
-    # §7.3): https or a native scheme on loopback stays exact. The host must match too:
+    # §7.3): https or a native scheme on loopback stays exact. The host and any
+    # userinfo must match too:
     # 127.0.0.1, localhost and ::1 are not interchangeable, and localhost
     # may not even resolve to loopback (RFC 8252 §8.3).
     def matching_loopback?(uri)
       parsed = URI.parse(uri)
 
-      redirect_uris.any? do |redirect_uri|
+      Oauth.plain_authority?(parsed) && redirect_uris.any? do |redirect_uri|
         redirect = URI.parse(redirect_uri)
 
         redirect.scheme == "http" && parsed.scheme == "http" &&
-          redirect.host.in?(Oauth::LOOPBACK_HOSTS) &&
+          Oauth.loopback_host?(redirect.host) &&
           redirect.host.casecmp?(parsed.host.to_s) &&
+          redirect.userinfo == parsed.userinfo &&
           redirect.path == parsed.path &&
-          redirect.query == parsed.query
+          redirect.query == parsed.query &&
+          parsed.fragment.nil?
       end
     rescue URI::InvalidURIError
       false

app/views/oauth/authorizations/new.html.erb

@@ -17,7 +17,12 @@
       <strong>Application</strong>
       <p class="margin-none"><%= @client.name %></p>
       <% if @client.dynamically_registered? %>
-        <p class="txt-small txt-muted margin-none">Registered by a local tool. Only authorize if you trust it.</p>
+        <% redirect = URI.parse(@redirect_uri) %>
+        <% if Oauth.loopback_host?(redirect.host) %>
+          <p class="txt-small txt-muted margin-none">Registered by a local tool. Only authorize if you trust it.</p>
+        <% else %>
+          <p class="txt-small txt-muted margin-none">Self-registered app that will receive access at <strong><%= redirect.port == redirect.default_port ? redirect.host : "#{redirect.host}:#{redirect.port}" %></strong>. Only authorize if you trust it.</p>
+        <% end %>
       <% end %>
     </div>