Allow passing a redirect path to a param on the /login route (#32711)

nullchristo · web-flow · commit 305ebedfa1fb · 2025-05-15T08:48:06.000-05:00
Detects the `redirect` queryParam on the /login route. If the user is
already logged in, navigates to that page. Otherwise, sets the
"destination_url" cookie so that the user will be redirected after
logging in.

If the param doesn't start with a single slash, ignore it and follow
previous behavior (navigate to "/" or log in then navigate there)

Respects subfolder configs.
diff --git a/app/controllers/static_controller.rb b/app/controllers/static_controller.rb
@@ -27,9 +27,36 @@ class StaticController < ApplicationController
   }
   CUSTOM_PAGES = {} # Add via `#add_topic_static_page` in plugin API
 
+  def extract_redirect_param
+    redirect_path = params[:redirect]
+    if redirect_path.present?
+      begin
+        forum_host = URI(Discourse.base_url).host
+        uri = URI(redirect_path)
+
+        if uri.path.present? && !uri.path.starts_with?(login_path) &&
+             (uri.host.blank? || uri.host == forum_host) && uri.path =~ %r{\A\/{1}[^\.\s]*\z}
+          return "#{uri.path}#{uri.query ? "?#{uri.query}" : ""}"
+        end
+      rescue URI::Error, ArgumentError
+        # If the URI is invalid, return "/" below
+      end
+    end
+
+    "/"
+  end
+
   def show
-    if current_user && (params[:id] == "login" || params[:id] == "signup")
-      return redirect_to(path "/")
+    if params[:id] == "login"
+      destination = extract_redirect_param
+
+      if current_user
+        return redirect_to(path(destination), allow_other_host: false)
+      elsif destination != "/"
+        cookies[:destination_url] = path(destination)
+      end
+    elsif params[:id] == "signup" && current_user
+      return redirect_to path("/")
     end
 
     if SiteSetting.login_required? && current_user.nil? && %w[faq guidelines].include?(params[:id])
@@ -123,26 +150,7 @@ def enter
     params.delete(:username)
     params.delete(:password)
 
-    destination = path("/")
-
-    redirect_location = params[:redirect]
-    if redirect_location.present? && !redirect_location.is_a?(String)
-      raise Discourse::InvalidParameters.new(:redirect)
-    elsif redirect_location.present? &&
-          begin
-            forum_uri = URI(Discourse.base_url)
-            uri = URI(redirect_location)
-
-            if uri.path.present? && !uri.path.starts_with?(login_path) &&
-                 (uri.host.blank? || uri.host == forum_uri.host) &&
-                 uri.path =~ %r{\A\/{1}[^\.\s]*\z}
-              destination = "#{uri.path}#{uri.query ? "?#{uri.query}" : ""}"
-            end
-          rescue URI::Error
-            # Do nothing if the URI is invalid
-          end
-    end
-
+    destination = extract_redirect_param
     redirect_to(destination, allow_other_host: false)
   end
 
diff --git a/spec/requests/static_controller_spec.rb b/spec/requests/static_controller_spec.rb
@@ -150,7 +150,7 @@
       end
     end
 
-    it "should redirect to / when logged in and path is /login" do
+    it "should redirect to / when logged in and path is /login without redirect" do
       sign_in(Fabricate(:user))
       get "/login"
       expect(response).to redirect_to("/")
@@ -252,6 +252,58 @@
         get "/login"
       end.to_not change { SiteSetting.title }
     end
+
+    context "without a subfolder" do
+      it "redirects as requested when logged in and path is /login with valid redirect param" do
+        sign_in(Fabricate(:user))
+        get "/login", params: { redirect: "/foo" }
+        expect(response).to redirect_to("/foo")
+      end
+
+      it "redirects to / when logged in and path is /login with invalid redirect param" do
+        sign_in(Fabricate(:user))
+        get "/login", params: { redirect: "//foo" }
+        expect(response).to redirect_to("/")
+        get "/login", params: { redirect: "foo" }
+        expect(response).to redirect_to("/")
+        get "/login", params: { redirect: "http://foo" }
+        expect(response).to redirect_to("/")
+        get "/login", params: { redirect: "www.foo.bar" }
+        expect(response).to redirect_to("/")
+      end
+
+      it "sets the destination_url cookie when not logged in and path is /login with valid redirect param" do
+        get "/login", params: { redirect: "/foo" }
+        expect(response.cookies["destination_url"]).to eq("/foo")
+      end
+    end
+
+    context "with a subfolder" do
+      before { set_subfolder "/sub_test" }
+
+      it "redirects as requested when logged in and path is /login with valid redirect param" do
+        sign_in(Fabricate(:user))
+        get "/login", params: { redirect: "/foo" }
+        expect(response).to redirect_to("/sub_test/foo")
+      end
+
+      it "redirects to / when logged in and path is /login with invalid redirect param" do
+        sign_in(Fabricate(:user))
+        get "/login", params: { redirect: "//foo" }
+        expect(response).to redirect_to("/sub_test/")
+        get "/login", params: { redirect: "foo" }
+        expect(response).to redirect_to("/sub_test/")
+        get "/login", params: { redirect: "http://foo" }
+        expect(response).to redirect_to("/sub_test/")
+        get "/login", params: { redirect: "www.foo.bar" }
+        expect(response).to redirect_to("/sub_test/")
+      end
+
+      it "sets the destination_url cookie when not logged in and path is /login with valid redirect param" do
+        get "/login", params: { redirect: "/foo" }
+        expect(response.cookies["destination_url"]).to eq("/sub_test/foo")
+      end
+    end
   end
 
   describe "#enter" do
@@ -307,10 +359,7 @@
     context "with an array" do
       it "redirects to the root" do
         post "/login.json", params: { redirect: ["/foo"] }
-        expect(response.status).to eq(400)
-        json = response.parsed_body
-        expect(json["errors"]).to be_present
-        expect(json["errors"]).to include(I18n.t("invalid_params", message: "redirect"))
+        expect(response).to redirect_to("/")
       end
     end
 
diff --git a/spec/system/login_spec.rb b/spec/system/login_spec.rb
@@ -39,6 +39,16 @@ def wait_for_email_link(user, type)
       expect(page).to have_css(".header-dropdown-toggle.current-user")
     end
 
+    it "can login with redirect" do
+      EmailToken.confirm(Fabricate(:email_token, user: user).token)
+
+      login_form
+        .open_with_redirect("/about")
+        .fill(username: "john", password: "supersecurepassword")
+        .click_login
+      expect(page).to have_current_path("/about")
+    end
+
     it "can login and activate account" do
       login_form.open.fill(username: "john", password: "supersecurepassword").click_login
       expect(page).to have_css(".not-activated-modal")
@@ -258,6 +268,12 @@ def wait_for_email_link(user, type)
 
         expect(page).to have_css(".authorize-api-key .scopes")
       end
+
+      it "redirects when navigating to login with redirect param" do
+        mock_google_auth
+        login_form.open_with_redirect("/about")
+        expect(page).to have_current_path("/about")
+      end
     end
   end
 
@@ -294,6 +310,21 @@ def wait_for_email_link(user, type)
       expect(page).to have_css(".header-dropdown-toggle.current-user")
     end
 
+    it "can login with totp and redirect" do
+      login_form
+        .open_with_redirect("/about")
+        .fill(username: "john", password: "supersecurepassword")
+        .click_login
+
+      expect(page).to have_css(".second-factor")
+
+      totp = ROTP::TOTP.new(user_second_factor.data).now
+      find("#login-second-factor").fill_in(with: totp)
+      login_form.click_login
+
+      expect(page).to have_current_path("/about")
+    end
+
     it "can login with backup code" do
       login_form.open.fill(username: "john", password: "supersecurepassword").click_login
 
diff --git a/spec/system/page_objects/pages/login.rb b/spec/system/page_objects/pages/login.rb
@@ -16,6 +16,11 @@ def open
         self
       end
 
+      def open_with_redirect(redirect_path)
+        visit("/login?redirect=#{redirect_path}")
+        self
+      end
+
       def open_from_header
         find(".login-button").click
       end
