tenseleyflow/shithub / ec6208f

Browse files

S35: internal/security/openredirect — Safe + SafeOr next= validator

Authored by espadonne
SHA
ec6208fcd2dda227177cd5077c81895c09ac8f5f
Parents
c2d90a4
Tree
4c08355

2 changed files

StatusFile+-
A internal/security/openredirect/openredirect.go 99 0
A internal/security/openredirect/openredirect_test.go 78 0
internal/security/openredirect/openredirect.goadded
@@ -0,0 +1,99 @@
1
+// SPDX-License-Identifier: AGPL-3.0-or-later
2
+
3
+// Package openredirect validates `next=` style redirect targets so a
4
+// crafted query parameter can't bounce a logged-in user off-host
5
+// (the canonical phishing primitive). The Safe predicate accepts:
6
+//
7
+//   - relative paths starting with a single `/` (not `//`, not `/\`)
8
+//   - absolute URLs whose host matches an operator-approved allow-list
9
+//
10
+// Anything else is rejected. Callers fall back to a known-safe default
11
+// (typically `/`) on rejection.
12
+package openredirect
13
+
14
+import (
15
+	"net/url"
16
+	"strings"
17
+)
18
+
19
+// Config governs which absolute redirect targets are allowed. The
20
+// zero value accepts only relative paths — the safest default for
21
+// most surfaces.
22
+type Config struct {
23
+	// AllowedHosts is the exact, case-insensitive host allow-list
24
+	// for absolute redirect targets. Typical content: the deployment's
25
+	// canonical hostname plus any apex/www variants. Empty disables
26
+	// absolute-URL redirects entirely.
27
+	AllowedHosts []string
28
+}
29
+
30
+// Safe reports whether candidate is a safe redirect target under cfg.
31
+// Empty input returns false; callers should pre-screen and substitute
32
+// the default themselves.
33
+func (cfg Config) Safe(candidate string) bool {
34
+	if candidate == "" {
35
+		return false
36
+	}
37
+	// Two-leading-slash protocol-relative URLs (`//attacker.tld/x`)
38
+	// are absolute under the browser's resolution; reject early so a
39
+	// later relative-path check doesn't accept them.
40
+	if strings.HasPrefix(candidate, "//") {
41
+		return false
42
+	}
43
+	// Reverse-slash trick (`/\evil.tld`): some browsers normalise the
44
+	// `\` into `/` and treat the result as protocol-relative. Block.
45
+	if strings.HasPrefix(candidate, "/\\") {
46
+		return false
47
+	}
48
+	u, err := url.Parse(candidate)
49
+	if err != nil {
50
+		return false
51
+	}
52
+	// Relative path: scheme empty AND host empty AND starts with `/`.
53
+	if u.Scheme == "" && u.Host == "" {
54
+		return strings.HasPrefix(u.Path, "/")
55
+	}
56
+	// Absolute URL: scheme must be http(s) and host must be allow-listed.
57
+	if u.Scheme != "http" && u.Scheme != "https" {
58
+		return false
59
+	}
60
+	host := u.Hostname()
61
+	if host == "" {
62
+		return false
63
+	}
64
+	for _, h := range cfg.AllowedHosts {
65
+		if equalFold(h, host) {
66
+			return true
67
+		}
68
+	}
69
+	return false
70
+}
71
+
72
+// SafeOr returns candidate when Safe(candidate), otherwise fallback.
73
+// The single-call convenience for handlers that want to default to
74
+// `/` after rejection.
75
+func (cfg Config) SafeOr(candidate, fallback string) string {
76
+	if cfg.Safe(candidate) {
77
+		return candidate
78
+	}
79
+	return fallback
80
+}
81
+
82
+func equalFold(a, b string) bool {
83
+	if len(a) != len(b) {
84
+		return false
85
+	}
86
+	for i := 0; i < len(a); i++ {
87
+		ca, cb := a[i], b[i]
88
+		if 'A' <= ca && ca <= 'Z' {
89
+			ca += 'a' - 'A'
90
+		}
91
+		if 'A' <= cb && cb <= 'Z' {
92
+			cb += 'a' - 'A'
93
+		}
94
+		if ca != cb {
95
+			return false
96
+		}
97
+	}
98
+	return true
99
+}
internal/security/openredirect/openredirect_test.goadded
@@ -0,0 +1,78 @@
1
+// SPDX-License-Identifier: AGPL-3.0-or-later
2
+
3
+package openredirect
4
+
5
+import "testing"
6
+
7
+func TestSafe_AcceptsRelativePaths(t *testing.T) {
8
+	t.Parallel()
9
+	cfg := Config{}
10
+	for _, ok := range []string{
11
+		"/",
12
+		"/owner/repo",
13
+		"/owner/repo?tab=issues",
14
+		"/owner/repo/issues/1#comment-3",
15
+	} {
16
+		if !cfg.Safe(ok) {
17
+			t.Errorf("Safe(%q) = false; want true", ok)
18
+		}
19
+	}
20
+}
21
+
22
+func TestSafe_RejectsHostHopAttempts(t *testing.T) {
23
+	t.Parallel()
24
+	cfg := Config{}
25
+	for _, bad := range []string{
26
+		"",
27
+		"//evil.tld/x",      // protocol-relative
28
+		`/\evil.tld/x`,      // backslash trick
29
+		"http://evil.tld/x", // absolute, no allow-list
30
+		"https://evil.tld/x",
31
+		"javascript:alert(1)",
32
+		"data:text/html,<script>x</script>",
33
+		"file:///etc/passwd",
34
+		"owner/repo", // missing leading slash
35
+	} {
36
+		if cfg.Safe(bad) {
37
+			t.Errorf("Safe(%q) = true; want false", bad)
38
+		}
39
+	}
40
+}
41
+
42
+func TestSafe_AcceptsAllowedHost(t *testing.T) {
43
+	t.Parallel()
44
+	cfg := Config{AllowedHosts: []string{"shithub.example", "WWW.shithub.example"}}
45
+	for _, ok := range []string{
46
+		"https://shithub.example/owner/repo",
47
+		"https://www.shithub.example/foo",
48
+		"http://shithub.example/foo",
49
+	} {
50
+		if !cfg.Safe(ok) {
51
+			t.Errorf("Safe(%q) = false; want true (allow-listed)", ok)
52
+		}
53
+	}
54
+}
55
+
56
+func TestSafe_RejectsUnlistedHostEvenForHTTPS(t *testing.T) {
57
+	t.Parallel()
58
+	cfg := Config{AllowedHosts: []string{"shithub.example"}}
59
+	for _, bad := range []string{
60
+		"https://attacker.example/foo",
61
+		"https://shithub.example.attacker.tld/foo",
62
+	} {
63
+		if cfg.Safe(bad) {
64
+			t.Errorf("Safe(%q) = true; want false", bad)
65
+		}
66
+	}
67
+}
68
+
69
+func TestSafeOr(t *testing.T) {
70
+	t.Parallel()
71
+	cfg := Config{}
72
+	if got := cfg.SafeOr("/owner/repo", "/"); got != "/owner/repo" {
73
+		t.Errorf("SafeOr good = %q; want /owner/repo", got)
74
+	}
75
+	if got := cfg.SafeOr("//evil", "/"); got != "/" {
76
+		t.Errorf("SafeOr bad = %q; want /", got)
77
+	}
78
+}