[TAN-8575] Restrict OAuth redirect_uri to http(s) at registration, validation and navigation

back/app/controllers/oauth/registrations_controller.rb

@@ -9,12 +9,23 @@ class RegistrationsController < ApplicationController
 
     wrap_parameters :oauth_application
 
+    ALLOWED_REDIRECT_URI_SCHEMES = %w[http https].freeze
+
     # Rate limiting is handled by Rack::Attack (see config/initializers/rack_attack.rb).
 
     def create
+      redirect_uris = Array(oauth_application_params[:redirect_uris])
+
+      if redirect_uris.empty? || redirect_uris.any? { |uri| !allowed_redirect_uri?(uri) }
+        return render json: {
+          error: 'invalid_redirect_uri',
+          error_description: 'redirect_uris must be absolute http(s) URIs'
+        }, status: :bad_request
+      end
+
       application = Doorkeeper::Application.new(
         name: oauth_application_params[:client_name],
-        redirect_uri: Array(oauth_application_params[:redirect_uris]).join("\n"),
+        redirect_uri: redirect_uris.join("\n"),
         confidential: false
       )
 
@@ -26,8 +37,9 @@ def create
           redirect_uris: application.redirect_uri.split
         }, status: :created
       else
+        error = application.errors.include?(:redirect_uri) ? 'invalid_redirect_uri' : 'invalid_client_metadata'
         render json: {
-          error: 'invalid_client_metadata',
+          error: error,
           error_description: application.errors.full_messages.join(', ')
         }, status: :bad_request
       end
@@ -38,5 +50,17 @@ def create
     def oauth_application_params
       params.require(:oauth_application).permit(:client_name, redirect_uris: [])
     end
+
+    def allowed_redirect_uri?(value)
+      return false unless value.is_a?(String)
+
+      uri = URI.parse(value)
+      ALLOWED_REDIRECT_URI_SCHEMES.include?(uri.scheme.to_s.downcase) &&
+        uri.host.present? &&
+        # RFC 6749 §3.1.2 (redirection endpoint should not hold fragment)
+        uri.fragment.nil?
+    rescue URI::InvalidURIError
+      false
+    end
   end
 end

back/config/initializers/doorkeeper.rb

@@ -289,16 +289,14 @@
   # callback during Dynamic Client Registration. Without this the
   # default (force SSL outside development) rejects http://localhost:<port>/...
   force_ssl_in_redirect_uri do |uri|
-    !Rails.env.development? && %w[localhost 127.0.0.1 ::1].exclude?(uri.host)
+    !Rails.env.development? && %w[localhost 127.0.0.1 ::1].exclude?(uri.hostname&.downcase)
   end
 
   # Specify what redirect URI's you want to block during Application creation.
-  # Any redirect URI is allowed by default.
   #
-  # You can use this option in order to forbid URI's with 'javascript' scheme
-  # for example.
-  #
-  # forbid_redirect_uri { |uri| uri.scheme.to_s.downcase == 'javascript' }
+  # Scheme allowlist only — a browser must never be able to execute a stored
+  # redirect_uri. Whether http is acceptable is force_ssl_in_redirect_uri's job.
+  forbid_redirect_uri { |uri| %w[http https].exclude?(uri.scheme.to_s.downcase) }
 
   # Allows to set blank redirect URIs for Applications in case Doorkeeper configured
   # to use URI-less OAuth grant flows like Client Credentials or Resource Owner

back/spec/config/doorkeeper_redirect_uri_spec.rb

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+# Defence in depth behind the allowlist in Oauth::RegistrationsController: whatever
+# creates a Doorkeeper::Application, the redirect_uri rules configured in
+# config/initializers/doorkeeper.rb (forbid_redirect_uri + force_ssl_in_redirect_uri)
+# must refuse any scheme a browser would execute.
+describe Doorkeeper::Application do
+  describe 'redirect_uri validation' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:case_name, :redirect_uri, :valid) do
+      'https URI'                 | 'https://client.example.com/cb'             | true
+      # RFC 8252 loopback callbacks, which is what MCP clients register.
+      # force_ssl_in_redirect_uri exempts them, so http must stay valid here.
+      'loopback host'             | 'http://localhost:33418/cb'                 | true
+      'loopback IPv4'             | 'http://127.0.0.1:33418/cb'                 | true
+      'loopback IPv6'             | 'http://[::1]:33418/cb'                     | true
+      'non-loopback http URI'     | 'http://client.example.com/cb'              | false
+      'javascript scheme'         | 'javascript:alert(document.cookie)'         | false
+      'javascript with authority' | 'javascript://x%0Aalert(document.cookie)'   | false
+      'data scheme'               | 'data:text/html,<script>alert(1)</script>'  | false
+      'vbscript scheme'           | 'vbscript:msgbox(1)'                        | false
+      'https plus hostile URI'    | "https://ok.example.com/cb\njavascript://x%0Aalert(1)" | false
+      'loopback host with upcase' | 'http://LOCALHOST:33418/cb' | true
+    end
+
+    with_them do
+      it 'accepts only http(s), and http only on loopback' do
+        application = described_class.new(name: 'Test MCP Client', redirect_uri: redirect_uri, confidential: false)
+
+        expect(application.valid?).to eq valid
+      end
+    end
+  end
+end

back/spec/requests/oauth_registrations_spec.rb

@@ -0,0 +1,90 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+# RFC 7591 Dynamic Client Registration. The endpoint is public and
+# unauthenticated, so its redirect_uri scheme allowlist is what stops anyone from
+# registering a client whose redirect_uri runs script in the platform origin once
+# the SPA consent screen navigates to it (on approve as well as on deny).
+describe Oauth::RegistrationsController do
+  let(:headers) { { 'CONTENT_TYPE' => 'application/json' } }
+
+  def register(redirect_uris, client_name: 'Test MCP Client')
+    post '/oauth/registrations',
+      params: { client_name: client_name, redirect_uris: redirect_uris }.to_json,
+      headers: headers
+  end
+
+  describe 'POST /oauth/registrations' do
+    it 'registers a client with an https redirect_uri' do
+      expect { register(['https://client.example.com/oauth/callback']) }
+        .to change(Doorkeeper::Application, :count).by(1)
+
+      expect(response).to have_http_status(:created)
+      expect(response.parsed_body['client_id']).to be_present
+      expect(response.parsed_body['redirect_uris']).to eq ['https://client.example.com/oauth/callback']
+    end
+
+    # RFC 8252: native apps get their authorization code on a loopback http
+    # callback, and those apps are what this endpoint exists for. The guard is on
+    # the scheme, not on https, precisely so loopback registration keeps working.
+    it 'registers a client with a loopback http redirect_uri' do
+      expect { register(['http://localhost:33418/callback']) }
+        .to change(Doorkeeper::Application, :count).by(1)
+
+      expect(response).to have_http_status(:created)
+    end
+
+    it 'registers a client with several valid redirect_uris' do
+      register(['https://a.example.com/cb', 'http://127.0.0.1:5000/cb'])
+
+      expect(response).to have_http_status(:created)
+      expect(response.parsed_body['redirect_uris']).to eq ['https://a.example.com/cb', 'http://127.0.0.1:5000/cb']
+    end
+
+    # The other side of the error-code split: a failure that has nothing to do
+    # with redirect_uri keeps the generic RFC 7591 code.
+    it 'reports a non-redirect_uri problem with the generic error code' do
+      expect { register(['https://ok.example.com/cb'], client_name: nil) }
+        .not_to change(Doorkeeper::Application, :count)
+
+      expect(response).to have_http_status(:bad_request)
+      expect(response.parsed_body['error']).to eq 'invalid_client_metadata'
+    end
+
+    describe 'redirect_uri scheme allowlist' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:case_name, :redirect_uris) do
+        'javascript scheme'           | ['javascript:alert(document.cookie)']
+        # Parses as a URI with a host, so it survives Doorkeeper's default validators.
+        'javascript with authority'   | ['javascript://x%0Aalert(document.cookie)']
+        'javascript in mixed case'    | ['JavaScript:alert(1)']
+        'data scheme'                 | ['data:text/html,<script>alert(1)</script>']
+        'vbscript scheme'             | ['vbscript:msgbox(1)']
+        # Doorkeeper stores redirect URIs newline-separated and splits them on
+        # whitespace, so a URI smuggled into one entry would become a second
+        # registered redirect_uri. A prefix check on the entry would miss it.
+        'newline-smuggled URI'        | ["https://ok.example.com/cb\njavascript://x%0Aalert(1)"]
+        'valid URI plus hostile one'  | ['https://ok.example.com/cb', 'javascript:alert(1)']
+        'relative URI'                | ['/oauth/callback']
+        'scheme-relative URI'         | ['//evil.example.com/cb']
+        'URI with a fragment'         | ['https://ok.example.com/cb#fragment']
+        'no redirect_uris'            | []
+        'non-string entry'            | [{ 'uri' => 'https://ok.example.com/cb' }]
+        # Passes the controller allowlist (http is a valid scheme) and is refused one layer down
+        # by force_ssl_in_redirect_uri. Same code either way.
+        'plaintext non-loopback URI' | ['http://client.example.com/cb']
+      end
+
+      with_them do
+        it 'is rejected with invalid_redirect_uri and registers nothing' do
+          expect { register(redirect_uris) }.not_to change(Doorkeeper::Application, :count)
+
+          expect(response).to have_http_status(:bad_request)
+          expect(response.parsed_body['error']).to eq 'invalid_redirect_uri'
+        end
+      end
+    end
+  end
+end

front/app/containers/OAuthAuthorize/index.test.tsx

@@ -0,0 +1,143 @@
+import React from 'react';
+
+import type {
+  IOAuthAuthorizationRedirect,
+  OAuthAuthorizationParams,
+} from 'api/oauth_authorization/types';
+
+import { screen, render, userEvent } from 'utils/testUtils/rtl';
+
+const SAFE_REDIRECT_URI = 'https://client.example.com/oauth/callback';
+// Parses as a URI with a host, so it survives naive validation; a browser runs
+// `alert(...)` because `//x` comments out the rest of the line.
+const HOSTILE_REDIRECT_URI = 'javascript://x%0Aalert(document.cookie)';
+
+// Mutable per example. jest.mock factories may only close over names prefixed
+// with `mock`, so these carry the scenario into the mocked hooks below.
+let mockConsentRedirectUri = SAFE_REDIRECT_URI;
+let mockApproveRedirectUri = SAFE_REDIRECT_URI;
+
+jest.mock('api/me/useAuthUser', () =>
+  jest.fn(() => ({ data: { id: 'user-1' }, isLoading: false }))
+);
+
+jest.mock('api/oauth_authorization/useOAuthAuthorization', () =>
+  jest.fn(() => ({
+    data: {
+      data: {
+        type: 'oauth_authorization',
+        attributes: {
+          client_id: 'client-1',
+          client_name: 'Test MCP Client',
+          scopes: ['mcp:access'],
+          redirect_uri: mockConsentRedirectUri,
+          params: { client_id: 'client-1', state: 'state-123' },
+        },
+      },
+    },
+    isLoading: false,
+    isError: false,
+  }))
+);
+
+// Approving hits the API and navigates to whatever redirect_uri comes back —
+// which Doorkeeper builds from the same client-registered URI, so it is no more
+// trustworthy than the one on the consent screen.
+jest.mock('api/oauth_authorization/useCreateOAuthAuthorization', () =>
+  jest.fn(() => ({
+    mutate: (
+      _params: OAuthAuthorizationParams,
+      options?: { onSuccess?: (res: IOAuthAuthorizationRedirect) => void }
+    ) => {
+      options?.onSuccess?.({
+        data: {
+          type: 'oauth_authorization',
+          attributes: { redirect_uri: mockApproveRedirectUri },
+        },
+      });
+    },
+    isPending: false,
+  }))
+);
+
+jest.mock('utils/router', () => ({
+  ...jest.requireActual('utils/router'),
+  useSearch: () => ({ client_id: 'client-1', state: 'state-123' }),
+}));
+
+// Only the browser call is stubbed — the scheme guard under test stays real.
+const mockNavigateToUrl = jest.fn();
+jest.mock('./utils', () => ({
+  ...jest.requireActual('./utils'),
+  navigateToUrl: (url: string) => mockNavigateToUrl(url),
+}));
+
+import OAuthAuthorize from './index';
+
+describe('OAuthAuthorize', () => {
+  beforeEach(() => {
+    mockConsentRedirectUri = SAFE_REDIRECT_URI;
+    mockApproveRedirectUri = SAFE_REDIRECT_URI;
+  });
+
+  it('renders the consent screen for an http(s) redirect_uri', () => {
+    render(<OAuthAuthorize />);
+
+    expect(
+      screen.getByRole('button', { name: 'Authorize' })
+    ).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Cancel' })).toBeInTheDocument();
+  });
+
+  it('redirects the client back on deny', async () => {
+    const user = userEvent.setup();
+    render(<OAuthAuthorize />);
+
+    await user.click(screen.getByRole('button', { name: 'Cancel' }));
+
+    expect(mockNavigateToUrl).toHaveBeenCalledWith(
+      `${SAFE_REDIRECT_URI}?error=access_denied&state=state-123`
+    );
+  });
+
+  it('redirects the client back on approve', async () => {
+    const user = userEvent.setup();
+    render(<OAuthAuthorize />);
+
+    await user.click(screen.getByRole('button', { name: 'Authorize' }));
+
+    expect(mockNavigateToUrl).toHaveBeenCalledWith(SAFE_REDIRECT_URI);
+  });
+
+  // The reported attack: a client registered with a javascript: redirect_uri,
+  // executing in the platform origin as soon as the victim clicks deny.
+  it('refuses the consent screen entirely for a non-http(s) redirect_uri', () => {
+    mockConsentRedirectUri = HOSTILE_REDIRECT_URI;
+
+    render(<OAuthAuthorize />);
+
+    expect(
+      screen.getByText('This authorization request is invalid')
+    ).toBeInTheDocument();
+    expect(
+      screen.queryByRole('button', { name: 'Authorize' })
+    ).not.toBeInTheDocument();
+    expect(
+      screen.queryByRole('button', { name: 'Cancel' })
+    ).not.toBeInTheDocument();
+    expect(mockNavigateToUrl).not.toHaveBeenCalled();
+  });
+
+  it('navigates nowhere when approving returns a non-http(s) redirect_uri', async () => {
+    mockApproveRedirectUri = HOSTILE_REDIRECT_URI;
+    const user = userEvent.setup();
+    render(<OAuthAuthorize />);
+
+    await user.click(screen.getByRole('button', { name: 'Authorize' }));
+
+    expect(mockNavigateToUrl).not.toHaveBeenCalled();
+    expect(
+      await screen.findByText('This authorization request is invalid')
+    ).toBeInTheDocument();
+  });
+});