From 84f4e20f05db2d11b0144f95960c956f8221e657 Mon Sep 17 00:00:00 2001
From: Adam Cooke <adam@krystal.io>
Date: Fri, 24 Apr 2026 23:03:50 +0100
Subject: [PATCH] refactor(auth): tighten return_to validation

url_with_return_to only checked that return_to started with a forward
slash, which also allowed protocol-relative values like //host and
/\host. Rails 7.1 already refuses to follow those via redirect_to, so
the user just saw a 500. Reject the same shapes in the helper instead
so we fall back to the default URL cleanly.

Adds a sessions request spec covering the rejected shapes plus the
happy-path relative redirect.
---
 app/controllers/application_controller.rb |  7 ++-
 spec/requests/sessions_controller_spec.rb | 71 +++++++++++++++++++++++
 2 files changed, 76 insertions(+), 2 deletions(-)
 create mode 100644 spec/requests/sessions_controller_spec.rb

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index d2c36cd..24498f7 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -62,10 +62,13 @@ class ApplicationController < ActionController::Base
   end
 
   def url_with_return_to(url)
-    if params[:return_to].blank? || !params[:return_to].starts_with?("/")
+    return_to = params[:return_to]
+    if return_to.blank? ||
+       !return_to.start_with?("/") ||
+       return_to.start_with?("//", "/\\")
       url_for(url)
     else
-      params[:return_to]
+      return_to
     end
   end
 
diff --git a/spec/requests/sessions_controller_spec.rb b/spec/requests/sessions_controller_spec.rb
new file mode 100644
index 0000000..6a98332
--- /dev/null
+++ b/spec/requests/sessions_controller_spec.rb
@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+RSpec.describe "SessionsController", type: :request do
+  let(:user) { create(:user) }
+
+  describe "POST /login with return_to" do
+    def login_with(return_to:)
+      post "/login", params: {
+        email_address: user.email_address,
+        password: "passw0rd",
+        return_to: return_to
+      }
+    end
+
+    shared_examples "rejects unsafe return_to" do
+      it "does not redirect to the attacker-controlled location" do
+        login_with(return_to: unsafe_path)
+
+        expect(response).to have_http_status(:found)
+        # Whatever the fallback is, it must be same-origin: a Location that
+        # either omits a host or points at our own host. A browser must not
+        # end up at attacker.example.
+        location = response.location
+        expect(location).not_to include("attacker.example")
+        # Reject protocol-relative and absolute redirects entirely.
+        expect(location).not_to match(%r{\A//})
+        expect(location).not_to match(%r{\Ahttps?://attacker})
+      end
+    end
+
+    context "with a protocol-relative URL (//host)" do
+      let(:unsafe_path) { "//attacker.example/phish" }
+      include_examples "rejects unsafe return_to"
+    end
+
+    context "with a backslash-prefixed URL (/\\host)" do
+      let(:unsafe_path) { "/\\attacker.example/phish" }
+      include_examples "rejects unsafe return_to"
+    end
+
+    context "with an absolute http(s) URL" do
+      let(:unsafe_path) { "https://attacker.example/phish" }
+      include_examples "rejects unsafe return_to"
+    end
+
+    context "with a javascript: URL" do
+      let(:unsafe_path) { "javascript:alert(1)" }
+      include_examples "rejects unsafe return_to"
+    end
+
+    context "with a safe relative path" do
+      it "honours the return_to" do
+        login_with(return_to: "/org/acme/settings")
+        expect(response).to redirect_to("/org/acme/settings")
+      end
+    end
+
+    context "with no return_to" do
+      it "redirects to the default root" do
+        post "/login", params: {
+          email_address: user.email_address,
+          password: "passw0rd"
+        }
+        expect(response).to have_http_status(:found)
+        expect(response.location).not_to match(%r{\A//})
+      end
+    end
+  end
+end