<%= @client.name %>
<% if @client.dynamically_registered? %> -Registered by a local tool. Only authorize if you trust it.
+ <% redirect = URI.parse(@redirect_uri) %> + <% if Oauth.loopback_host?(redirect.host) %> +Registered by a local tool. Only authorize if you trust it.
+ <% else %> +Self-registered app that will receive access at <%= redirect.port == redirect.default_port ? redirect.host : "#{redirect.host}:#{redirect.port}" %>. Only authorize if you trust it.
+ <% end %> <% end %> diff --git a/test/integration/oauth_flow_test.rb b/test/integration/oauth_flow_test.rb index 8c84a0ba7f..4b919a2b96 100644 --- a/test/integration/oauth_flow_test.rb +++ b/test/integration/oauth_flow_test.rb @@ -98,6 +98,74 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_match "code_challenge", redirect_params["error_description"] end + test "authorization errors for a self-registered https client render here instead of redirecting" do + sign_in_as :david + client = Oauth::Client.create!(name: "Hosted", redirect_uris: %w[ https://connector.example.com/callback ], dynamically_registered: true) + + get_consent_screen client: client, code_challenge: nil + + assert_response :bad_request + assert_select "code", text: "invalid_request" + end + + test "no pre-consent error redirects to a self-registered https host" do + sign_in_as :david + client = Oauth::Client.create!(name: "Hosted", redirect_uris: %w[ https://connector.example.com/callback ], dynamically_registered: true) + + { + { scope: "admin" } => "invalid_scope", + { state: "s" * 2049 } => "invalid_request", + { response_type: "token" } => "unsupported_response_type", + { code_challenge_method: "plain" } => "invalid_request" + }.each do |overrides, error| + get_consent_screen client: client, **overrides + + assert_response :bad_request, "#{overrides} should not redirect" + assert_nil response.location + assert_select "code", text: error + end + end + + test "pre-consent errors still redirect to an operator-provisioned https client" do + sign_in_as :david + + get_consent_screen client: oauth_clients(:trusted_client), scope: "admin" + + assert_response :redirect + assert_match %r{\Ahttps://app\.example\.com/oauth/callback\?}, response.location + assert_equal "invalid_scope", Rack::Utils.parse_query(URI.parse(response.location).query)["error"] + end + + test "authorization errors for a self-registered loopback client still redirect" do + sign_in_as :david + + get_consent_screen code_challenge: nil + + assert_response :redirect + assert_equal "invalid_request", Rack::Utils.parse_query(URI.parse(response.location).query)["error"] + end + + test "consent names a self-registered redirect's port when it isn't the default" do + sign_in_as :david + client = Oauth::Client.create!(name: "Hosted", redirect_uris: %w[ https://connector.example.com:8443/callback ], dynamically_registered: true) + + get_consent_screen client: client + + assert_response :success + assert_select "strong", text: "connector.example.com:8443" + end + + test "denying a self-registered https client still redirects, after the consent screen named its host" do + sign_in_as :david + client = Oauth::Client.create!(name: "Hosted", redirect_uris: %w[ https://connector.example.com/callback ], dynamically_registered: true) + + post_consent client: client, error: "access_denied" + + assert_response :redirect + assert_match %r{\Ahttps://connector\.example\.com/callback\?}, response.location + assert_equal "access_denied", Rack::Utils.parse_query(URI.parse(response.location).query)["error"] + end + test "authorization consent issues code" do sign_in_as :david client = oauth_clients(:mcp_client) @@ -836,12 +904,29 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_response :created end - test "DCR rejects non-loopback redirect" do + test "DCR creates client with https redirect" do + assert_difference "Oauth::Client.count", 1 do + untenanted do + post oauth_clients_path, params: { + client_name: "Hosted Connector", + redirect_uris: [ "https://connector.example.com/callback" ] + }, as: :json + end + end + + assert_response :created + body = response.parsed_body + + assert_not_nil body["client_id"] + assert_equal [ "https://connector.example.com/callback" ], body["redirect_uris"] + end + + test "DCR rejects plain http non-loopback redirect" do assert_no_difference "Oauth::Client.count" do untenanted do post oauth_clients_path, params: { client_name: "Evil Client", - redirect_uris: [ "https://evil.com/steal" ] + redirect_uris: [ "http://evil.com/steal" ] }, as: :json end end @@ -888,6 +973,127 @@ class OauthFlowTest < ActionDispatch::IntegrationTest assert_equal "MCP Client", response.parsed_body["client_name"] end + test "DCR rejects https loopback redirect" do + assert_no_difference "Oauth::Client.count" do + untenanted do + post oauth_clients_path, params: { + client_name: "HTTPS Loopback", + redirect_uris: [ "https://127.0.0.1:8888/callback" ] + }, as: :json + end + end + + assert_response :bad_request + assert_equal "invalid_redirect_uri", response.parsed_body["error"] + end + + test "DCR rejects https loopback redirect regardless of host case" do + assert_no_difference "Oauth::Client.count" do + untenanted do + post oauth_clients_path, params: { + client_name: "Cased Loopback", + redirect_uris: [ "https://LOCALHOST:8888/callback" ] + }, as: :json + end + end + + assert_response :bad_request + assert_equal "invalid_redirect_uri", response.parsed_body["error"] + end + + test "DCR rejects percent-encoded https loopback redirect" do + assert_no_difference "Oauth::Client.count" do + untenanted do + post oauth_clients_path, params: { + client_name: "Encoded Loopback", + redirect_uris: [ "https://%6cocalhost:8888/callback" ] + }, as: :json + end + end + + assert_response :bad_request + assert_equal "invalid_redirect_uri", response.parsed_body["error"] + end + + test "DCR rejects a redirect whose authority isn't plain" do + %w[ https://%65vil.example/callback https://user:secret@connector.example.com/callback + https://connector.example.com:65536/callback http://user@127.0.0.1:8888/callback http://%5B%3A%3A1%5D:8888/callback ].each do |uri| + assert_no_difference "Oauth::Client.count" do + untenanted { post oauth_clients_path, params: { client_name: "Unplain", redirect_uris: [ uri ] }, as: :json } + end + + assert_response :bad_request, uri + assert_equal "invalid_redirect_uri", response.parsed_body["error"], uri + end + end + + test "DCR rejects a redirect whose host decodes to invalid UTF-8" do + assert_no_difference "Oauth::Client.count" do + untenanted do + post oauth_clients_path, params: { + client_name: "Broken Host", + redirect_uris: [ "https://%FF/callback" ] + }, as: :json + end + end + + assert_response :bad_request + assert_equal "invalid_redirect_uri", response.parsed_body["error"] + end + + test "DCR rejects https redirect with fragment" do + assert_no_difference "Oauth::Client.count" do + untenanted do + post oauth_clients_path, params: { + client_name: "Fragment Client", + redirect_uris: [ "https://connector.example.com/callback#section" ] + }, as: :json + end + end + + assert_response :bad_request + assert_equal "invalid_redirect_uri", response.parsed_body["error"] + end + + test "https redirect requires exact match in authorization" do + sign_in_as :david + + untenanted do + post oauth_clients_path, params: { + client_name: "Hosted Connector", + redirect_uris: [ "https://connector.example.com/callback" ] + }, as: :json + end + client_id = response.parsed_body["client_id"] + + untenanted do + get new_oauth_authorization_path, params: { + client_id: client_id, + redirect_uri: "https://connector.example.com/callback", + response_type: "code", + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + code_challenge_method: "S256", + scope: "read", + state: "xyz123" + } + end + assert_response :success + assert_match "connector.example.com", response.body + + untenanted do + get new_oauth_authorization_path, params: { + client_id: client_id, + redirect_uri: "https://connector.example.com:8443/callback", + response_type: "code", + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + code_challenge_method: "S256", + scope: "read", + state: "xyz123" + } + end + assert_response :bad_request + end + test "DCR requires redirect_uris" do untenanted do post oauth_clients_path, params: { client_name: "No Redirect" }, as: :json diff --git a/test/models/oauth/client_test.rb b/test/models/oauth/client_test.rb index e4a0d14207..1a6d4f1fbd 100644 --- a/test/models/oauth/client_test.rb +++ b/test/models/oauth/client_test.rb @@ -26,14 +26,60 @@ class Oauth::ClientTest < ActiveSupport::TestCase assert_includes client.errors[:redirect_uris], "can't be blank" end - test "dynamically registered clients must use http loopback URIs" do + test "dynamically registered clients can use https URIs" do + client = Oauth::Client.new( + name: "Hosted Connector", + redirect_uris: %w[ https://connector.example.com/callback ], + dynamically_registered: true + ) + assert client.valid? + end + + test "dynamically registered clients reject plain http for non-loopback hosts" do client = Oauth::Client.new( name: "External", - redirect_uris: %w[ https://evil.com/callback ], + redirect_uris: %w[ http://example.com/callback ], dynamically_registered: true ) assert_not client.valid? - assert_includes client.errors[:redirect_uris], "must be a local loopback URI for dynamically registered clients" + assert_includes client.errors[:redirect_uris], "must be an https or local loopback URI for dynamically registered clients" + end + + test "dynamically registered clients reject https loopback regardless of host case" do + client = Oauth::Client.new( + name: "Cased Loopback", + redirect_uris: %w[ https://LOCALHOST:8888/callback ], + dynamically_registered: true + ) + assert_not client.valid? + assert_includes client.errors[:redirect_uris], "must be an https or local loopback URI for dynamically registered clients" + end + + test "dynamically registered clients reject percent-encoded https loopback" do + client = Oauth::Client.new( + name: "Encoded Loopback", + redirect_uris: %w[ https://%6cocalhost:8888/callback ], + dynamically_registered: true + ) + assert_not client.valid? + assert_includes client.errors[:redirect_uris], "must be an https or local loopback URI for dynamically registered clients" + end + + test "dynamically registered clients name their redirect authority plainly" do + %w[ https://%65vil.example/callback https://user:secret@connector.example.com/callback http://user@127.0.0.1:8888/callback + https://connector.example.com:65536/callback https://connector.example.com:0/callback http://%5B%3A%3A1%5D:8888/callback + https://999.999.999.999/callback https://10.1/callback https://example.123/callback https://0177.0.0.1/callback https://08.0.0.1/callback + https://0x7f.0.0.1/callback https://example.0x1/callback ].each do |uri| + client = Oauth::Client.new(name: "Unplain", redirect_uris: [ uri ], dynamically_registered: true) + + assert_not client.valid?, uri + assert_includes client.errors[:redirect_uris], "must be an https or local loopback URI for dynamically registered clients", uri + end + + %w[ https://connector.example.com:8443/callback https://Connector.Example.com/callback http://[::1]:8888/callback http://localhost/callback + https://203.0.113.7/callback https://3com.example/callback https://10.0.0.1/callback ].each do |uri| + assert Oauth::Client.new(name: "Plain", redirect_uris: [ uri ], dynamically_registered: true).valid?, uri + end end test "dynamically registered clients reject https loopback" do @@ -43,7 +89,17 @@ class Oauth::ClientTest < ActiveSupport::TestCase dynamically_registered: true ) assert_not client.valid? - assert_includes client.errors[:redirect_uris], "must be a local loopback URI for dynamically registered clients" + assert_includes client.errors[:redirect_uris], "must be an https or local loopback URI for dynamically registered clients" + end + + test "dynamically registered clients reject custom schemes" do + client = Oauth::Client.new( + name: "Native App", + redirect_uris: %w[ myapp://callback ], + dynamically_registered: true + ) + assert_not client.valid? + assert_includes client.errors[:redirect_uris], "must be an https or local loopback URI for dynamically registered clients" end test "redirect URIs must not contain fragments" do @@ -56,6 +112,16 @@ class Oauth::ClientTest < ActiveSupport::TestCase assert_includes client.errors[:redirect_uris], "must not contain fragments" end + test "redirect URIs must not contain empty fragments" do + client = Oauth::Client.new( + name: "Empty Fragment", + redirect_uris: %w[ https://connector.example.com/callback# ], + dynamically_registered: true + ) + assert_not client.valid? + assert_includes client.errors[:redirect_uris], "must not contain fragments" + end + test "dynamically registered clients can use 127.0.0.1" do client = Oauth::Client.new( name: "Loopback", @@ -83,16 +149,6 @@ class Oauth::ClientTest < ActiveSupport::TestCase assert client.valid? end - test "loopback? returns true for loopback-only clients" do - client = Oauth::Client.new(redirect_uris: %w[ http://127.0.0.1:8888/cb http://localhost:9999/cb ]) - assert client.loopback? - end - - test "loopback? returns false for non-loopback clients" do - client = Oauth::Client.new(redirect_uris: %w[ https://example.com/cb ]) - assert_not client.loopback? - end - test "allows_redirect? matches exact URI" do client = Oauth::Client.new(redirect_uris: %w[ http://127.0.0.1:8888/callback ]) assert client.allows_redirect?("http://127.0.0.1:8888/callback") @@ -133,11 +189,41 @@ class Oauth::ClientTest < ActiveSupport::TestCase assert_not client.allows_redirect?("https://127.0.0.1:9443/callback") end + test "allows_redirect? never varies a loopback port onto different userinfo" do + client = Oauth::Client.new(redirect_uris: %w[ http://127.0.0.1:8888/callback https://connector.example.com/callback ]) + assert_not client.allows_redirect?("http://alice:secret@127.0.0.1:9999/callback") + assert_not client.allows_redirect?("http://alice@127.0.0.1:8888/callback") + end + + test "allows_redirect? never varies a loopback port out of the usable range" do + client = Oauth::Client.new(redirect_uris: %w[ http://127.0.0.1:8888/callback https://connector.example.com/callback ]) + assert client.allows_redirect?("http://127.0.0.1:65535/callback") + assert_not client.allows_redirect?("http://127.0.0.1:65536/callback") + assert_not client.allows_redirect?("http://127.0.0.1:0/callback") + end + test "allows_redirect? requires matching path for loopback flexibility" do client = Oauth::Client.new(redirect_uris: %w[ http://127.0.0.1:8888/callback ]) assert_not client.allows_redirect?("http://127.0.0.1:9999/other") end + test "allows_redirect? never varies a loopback port onto a fragment" do + client = oauth_clients(:mcp_client) + assert_not client.allows_redirect?("http://127.0.0.1:9999/callback#fragment") + assert_not client.allows_redirect?("http://127.0.0.1:9999/callback#") + end + + test "allows_redirect? keeps loopback flexibility for mixed registrations" do + client = Oauth::Client.new(redirect_uris: %w[ http://127.0.0.1:8888/callback https://connector.example.com/callback ]) + assert client.allows_redirect?("http://127.0.0.1:9999/callback") + assert client.allows_redirect?("https://connector.example.com/callback") + assert_not client.allows_redirect?("https://connector.example.com:8443/callback") + + https_loopback = Oauth::Client.new(redirect_uris: %w[ https://127.0.0.1:8443/callback https://connector.example.com/callback ]) + assert https_loopback.allows_redirect?("https://127.0.0.1:8443/callback") + assert_not https_loopback.allows_redirect?("https://127.0.0.1:9443/callback") + end + test "allows_scope? checks client scopes" do client = Oauth::Client.new(scopes: %w[ read write ]) assert client.allows_scope?("read")