FEATURE: add a setting to allowlist DiscourseConnect return path domains (#21110)

* FEATURE: add a setting to allowlist DiscourseConnect return path domains

This commit adds a site setting to allowlist DiscourseConnect return
path domains. The setting needs supports exact domain or wildcard
character (*) to allow for any domain as return path.

* Add more specs to clarify what is allowed in site setting

* Update setting description to explain what is allowed

app/controllers/session_controller.rb

@@ -208,7 +208,7 @@ class SessionController < ApplicationController
             uri = URI(return_path)
             if (uri.hostname == Discourse.current_hostname)
               return_path = uri.to_s
-            elsif !SiteSetting.discourse_connect_allows_all_return_paths
+            elsif !domain_redirect_allowed?(uri.hostname)
               return_path = path("/")
             end
           rescue StandardError
@@ -808,4 +808,12 @@ class SessionController < ApplicationController
     Rails.logger.warn("SSO invite redemption failed: #{e}")
     raise Invite::RedemptionFailed
   end
+
+  def domain_redirect_allowed?(hostname)
+    allowed_domains = SiteSetting.discourse_connect_allowed_redirect_domains
+    return false if allowed_domains.blank?
+    return true if allowed_domains.split("|").include?("*")
+
+    allowed_domains.split("|").include?(hostname)
+  end
 end

config/locales/server.en.yml

@@ -1743,7 +1743,7 @@ en:
     discourse_connect_overrides_profile_background: "Overrides user profile background with value from DiscourseConnect payload."
     discourse_connect_overrides_card_background: "Overrides user card background with value from DiscourseConnect payload."
     discourse_connect_not_approved_url: "Redirect unapproved DiscourseConnect accounts to this URL"
-    discourse_connect_allows_all_return_paths: "Do not restrict the domain for return_paths provided by DiscourseConnect (by default return path must be on current site)"
+    discourse_connect_allowed_redirect_domains: "Restrict to these domains for return_paths provided by DiscourseConnect (by default return path must be on current site). Use * to allow any domain for return path. Subdomain wildcards (`*.foobar.com`) are not allowed."
 
     enable_local_logins: "Enable local username and password login based accounts. WARNING: if disabled, you may be unable to log in if you have not previously configured at least one alternate login method."
     enable_local_logins_via_email: "Allow users to request a one-click login link to be sent to them via email."

config/site_settings.yml

@@ -491,7 +491,10 @@ login:
     client: true
     default: false
     validator: "EnableSsoValidator"
-  discourse_connect_allows_all_return_paths: false
+  discourse_connect_allowed_redirect_domains:
+    default: ""
+    type: list
+    list_type: simple
   verbose_discourse_connect_logging: false
   verbose_upload_logging:
     hidden: true

db/migrate/20230413121500_add_discourse_connect_allowed_redirect_domains_to_site_settings.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class AddDiscourseConnectAllowedRedirectDomainsToSiteSettings < ActiveRecord::Migration[7.0]
+  def change
+    execute <<~SQL
+      INSERT INTO site_settings(name, data_type, value, created_at, updated_at)
+      SELECT 'discourse_connect_allowed_redirect_domains', 8, '*', created_at, NOW()
+      FROM site_settings
+      WHERE name = 'discourse_connect_allows_all_return_paths' AND value = 't'
+    SQL
+
+    execute <<~SQL
+      DELETE FROM site_settings
+      WHERE name = 'discourse_connect_allows_all_return_paths'
+    SQL
+  end
+end

spec/requests/session_controller_spec.rb

@@ -826,7 +826,7 @@ RSpec.describe SessionController do
     end
 
     it "redirects to random url if it is allowed" do
-      SiteSetting.discourse_connect_allows_all_return_paths = true
+      SiteSetting.discourse_connect_allowed_redirect_domains = "gusundtrout.com|foobar.com"
 
       sso = get_sso("https://gusundtrout.com")
       sso.external_id = "666"
@@ -838,6 +838,32 @@ RSpec.describe SessionController do
       expect(response).to redirect_to("https://gusundtrout.com")
     end
 
+    it "allows wildcard character to redirect to any domain" do
+      SiteSetting.discourse_connect_allowed_redirect_domains = "*|foo.com"
+
+      sso = get_sso("https://foobar.com")
+      sso.external_id = "666"
+      sso.email = "bob@bob.com"
+      sso.name = "Sam Saffron"
+      sso.username = "sam"
+
+      get "/session/sso_login", params: Rack::Utils.parse_query(sso.payload), headers: headers
+      expect(response).to redirect_to("https://foobar.com")
+    end
+
+    it "does not allow wildcard character in domains" do
+      SiteSetting.discourse_connect_allowed_redirect_domains = "*.foobar.com|foobar.com"
+
+      sso = get_sso("https://sub.foobar.com")
+      sso.external_id = "666"
+      sso.email = "bob@bob.com"
+      sso.name = "Sam Saffron"
+      sso.username = "sam"
+
+      get "/session/sso_login", params: Rack::Utils.parse_query(sso.payload), headers: headers
+      expect(response).to redirect_to("/")
+    end
+
     it "redirects to root if the host of the return_path is different" do
       sso = get_sso("//eviltrout.com")
       sso.external_id = "666"