3 files changed, 22 insertions, 8 deletions

diff --git a/custom/extensions/auth-file.lua b/custom/extensions/auth-file.luaindex 6cbb8e8..1ad2eb8 100644--- a/custom/extensions/auth-file.lua+++ b/custom/extensions/auth-file.lua@@ -327,11 +327,13 @@ function get_secret() 	return secret end --- A redirect target is unsafe if a browser would read it as another origin.--- The only such form cgit can be tricked into signing is a scheme-relative--- "//host" or "/\host". Everything else stays on this host.+-- A redirect target is safe only as a path on this host, which starts with+-- a single "/". A browser reads "//host" and "/\host" as another origin, and+-- a target that does not start with "/" can name a scheme. cgit signs the+-- page's own url, which starts with virtual-root, so logins need that to+-- be a path, as it is by default. function is_safe_redirect(url)-	if type(url) ~= "string" then+	if type(url) ~= "string" or url:sub(1, 1) ~= "/" then 		return false 	end 	local head = url:sub(1, 2)
diff --git a/custom/extensions/auth-file.lua b/custom/extensions/auth-file.lua
index 6cbb8e8..1ad2eb8 100644
--- a/custom/extensions/auth-file.lua
+++ b/custom/extensions/auth-file.lua
@@ -327,11 +327,13 @@ function get_secret()
 	return secret
 end
 
--- A redirect target is unsafe if a browser would read it as another origin.
--- The only such form cgit can be tricked into signing is a scheme-relative
--- "//host" or "/\host". Everything else stays on this host.
+-- A redirect target is safe only as a path on this host, which starts with
+-- a single "/". A browser reads "//host" and "/\host" as another origin, and
+-- a target that does not start with "/" can name a scheme. cgit signs the
+-- page's own url, which starts with virtual-root, so logins need that to
+-- be a path, as it is by default.
 function is_safe_redirect(url)
-	if type(url) ~= "string" then
+	if type(url) ~= "string" or url:sub(1, 1) ~= "/" then
 		return false
 	end
 	local head = url:sub(1, 2)
diff --git a/custom/extensions/auth-inline.lua b/custom/extensions/auth-inline.luaindex b97bb27..77084e2 100644--- a/custom/extensions/auth-inline.lua+++ b/custom/extensions/auth-inline.lua@@ -290,11 +290,13 @@ function get_secret() 	return secret end --- A redirect target is unsafe if a browser would read it as another origin.--- The only such form cgit can be tricked into signing is a scheme-relative--- "//host" or "/\host". Everything else stays on this host.+-- A redirect target is safe only as a path on this host, which starts with+-- a single "/". A browser reads "//host" and "/\host" as another origin, and+-- a target that does not start with "/" can name a scheme. cgit signs the+-- page's own url, which starts with virtual-root, so logins need that to+-- be a path, as it is by default. function is_safe_redirect(url)-	if type(url) ~= "string" then+	if type(url) ~= "string" or url:sub(1, 1) ~= "/" then 		return false 	end 	local head = url:sub(1, 2)
diff --git a/custom/extensions/auth-inline.lua b/custom/extensions/auth-inline.lua
index b97bb27..77084e2 100644
--- a/custom/extensions/auth-inline.lua
+++ b/custom/extensions/auth-inline.lua
@@ -290,11 +290,13 @@ function get_secret()
 	return secret
 end
 
--- A redirect target is unsafe if a browser would read it as another origin.
--- The only such form cgit can be tricked into signing is a scheme-relative
--- "//host" or "/\host". Everything else stays on this host.
+-- A redirect target is safe only as a path on this host, which starts with
+-- a single "/". A browser reads "//host" and "/\host" as another origin, and
+-- a target that does not start with "/" can name a scheme. cgit signs the
+-- page's own url, which starts with virtual-root, so logins need that to
+-- be a path, as it is by default.
 function is_safe_redirect(url)
-	if type(url) ~= "string" then
+	if type(url) ~= "string" or url:sub(1, 1) ~= "/" then
 		return false
 	end
 	local head = url:sub(1, 2)
diff --git a/tests/extensions/test-auth.lua b/tests/extensions/test-auth.luaindex 3470741..86f8f89 100644--- a/tests/extensions/test-auth.lua+++ b/tests/extensions/test-auth.lua@@ -256,8 +256,14 @@ h.check("differing lengths do not", not constant_equals("a", "ab")) h.check("non strings do not", not constant_equals(nil, "a"))  h.check("a local path is a safe redirect", is_safe_redirect("/x"))+h.check("and so is the root", is_safe_redirect("/")) h.check("a scheme relative target is not", not is_safe_redirect("//evil")) h.check("a backslash variant is not", not is_safe_redirect("/\\evil"))+h.check("nor one of backslashes alone", not is_safe_redirect("\\\\evil"))+h.check("nor an absolute url", not is_safe_redirect("https://evil.example/"))+h.check("nor any other scheme", not is_safe_redirect("javascript:x"))+h.check("nor a relative path", not is_safe_redirect("x/y"))+h.check("nor an empty target", not is_safe_redirect("")) h.check("a missing target is not", not is_safe_redirect(nil))  -- Signing and verification.@@ -464,6 +470,8 @@ h.excludes("over https it says nothing of the kind", out, "only works over https out = run_action("body", { url = "//evil.example/x" }) h.equals("an unsafe destination is swapped for the login page", way_back_in(out), "/?p=login") h.equals("and is not taken for a failed login", out:find("login-error", 1, true), nil)+out = run_action("body", { url = "https://evil.example/x" })+h.equals("and so is one with a scheme", way_back_in(out), "/?p=login")  out = run_action("body", { url = "/repo/log/?q=x&login=failed" }) h.contains("a failed login says so", out,@@ -571,6 +579,8 @@ h.contains("a post without the signed token is not served", out, "Status: 404")  out = login("Alice", password, secure_value("redirect", "//evil.example/", 0)) h.contains("even a signed unsafe destination is not followed", out, "Status: 404")+out = login("Alice", password, secure_value("redirect", "https://evil.example/", 0))+h.contains("nor a signed one with a scheme", out, "Status: 404")  -- Over plain http the cookie cannot be Secure, and so loses the __Host- name. load_script({ { "local cookie_insecure = false", "local cookie_insecure = true" } })
diff --git a/tests/extensions/test-auth.lua b/tests/extensions/test-auth.lua
index 3470741..86f8f89 100644
--- a/tests/extensions/test-auth.lua
+++ b/tests/extensions/test-auth.lua
@@ -256,8 +256,14 @@ h.check("differing lengths do not", not constant_equals("a", "ab"))
 h.check("non strings do not", not constant_equals(nil, "a"))
 
 h.check("a local path is a safe redirect", is_safe_redirect("/x"))
+h.check("and so is the root", is_safe_redirect("/"))
 h.check("a scheme relative target is not", not is_safe_redirect("//evil"))
 h.check("a backslash variant is not", not is_safe_redirect("/\\evil"))
+h.check("nor one of backslashes alone", not is_safe_redirect("\\\\evil"))
+h.check("nor an absolute url", not is_safe_redirect("https://evil.example/"))
+h.check("nor any other scheme", not is_safe_redirect("javascript:x"))
+h.check("nor a relative path", not is_safe_redirect("x/y"))
+h.check("nor an empty target", not is_safe_redirect(""))
 h.check("a missing target is not", not is_safe_redirect(nil))
 
 -- Signing and verification.
@@ -464,6 +470,8 @@ h.excludes("over https it says nothing of the kind", out, "only works over https
 out = run_action("body", { url = "//evil.example/x" })
 h.equals("an unsafe destination is swapped for the login page", way_back_in(out), "/?p=login")
 h.equals("and is not taken for a failed login", out:find("login-error", 1, true), nil)
+out = run_action("body", { url = "https://evil.example/x" })
+h.equals("and so is one with a scheme", way_back_in(out), "/?p=login")
 
 out = run_action("body", { url = "/repo/log/?q=x&login=failed" })
 h.contains("a failed login says so", out,
@@ -571,6 +579,8 @@ h.contains("a post without the signed token is not served", out, "Status: 404")
 
 out = login("Alice", password, secure_value("redirect", "//evil.example/", 0))
 h.contains("even a signed unsafe destination is not followed", out, "Status: 404")
+out = login("Alice", password, secure_value("redirect", "https://evil.example/", 0))
+h.contains("nor a signed one with a scheme", out, "Status: 404")
 
 -- Over plain http the cookie cannot be Secure, and so loses the __Host- name.
 load_script({ { "local cookie_insecure = false", "local cookie_insecure = true" } })