From 569b91417f8153afa6d3cd72d2ace45db1e48d9d Mon Sep 17 00:00:00 2001
From: Will Norris <will@tailscale.com>
Date: Mon, 8 Jan 2024 11:18:16 -0800
Subject: [PATCH] client/web: ensure path prefix has a leading slash

This is simply an extra check to prevent hypothetical issues if a prefix
such as `--prefix="javascript:alert(1)"` was provided.  This isn't
really necessary since the prefix is a configuration flag provided by
the device owner, not user input.  But it does enforce that we are
always interpreting the provided value as a path relative to the root.

Fixes: tailscale/corp#16268

Signed-off-by: Will Norris <will@tailscale.com>
---
 client/web/web.go      | 11 ++++----
 client/web/web_test.go | 63 ++++++++++++++++++++++++++++++------------
 2 files changed, 52 insertions(+), 22 deletions(-)

diff --git a/client/web/web.go b/client/web/web.go
index 93aeb68e8..b67cb5645 100644
--- a/client/web/web.go
+++ b/client/web/web.go
@@ -176,11 +176,12 @@ func NewServer(opts ServerOpts) (s *Server, err error) {
 		waitAuthURL: opts.WaitAuthURL,
 	}
 	if opts.PathPrefix != "" {
-		// In enforcePrefix, we add the necessary leading '/'. If we did not
-		// strip 1 or more leading '/'s here, we would end up redirecting
-		// clients to e.g. //example.com (a schema-less URL that points to
-		// another site). See https://github.com/tailscale/corp/issues/16268.
-		s.pathPrefix = strings.TrimLeft(path.Clean(opts.PathPrefix), "/\\")
+		// Enforce that path prefix always has a single leading '/'
+		// so that it is treated as a relative URL path.
+		// We strip multiple leading '/' to prevent schema-less offsite URLs like "//example.com".
+		//
+		// See https://github.com/tailscale/corp/issues/16268.
+		s.pathPrefix = "/" + strings.TrimLeft(path.Clean(opts.PathPrefix), "/\\")
 	}
 	if s.mode == ManageServerMode {
 		if opts.NewAuthURL == nil {
diff --git a/client/web/web_test.go b/client/web/web_test.go
index ee46551fe..13e2e590b 100644
--- a/client/web/web_test.go
+++ b/client/web/web_test.go
@@ -939,36 +939,65 @@ func TestServeAPIAuthMetricLogging(t *testing.T) {
 	}
 }
 
-func TestNoOffSiteRedirect(t *testing.T) {
-	options := ServerOpts{
-		Mode: LoginServerMode,
-		// Emulate the admin using a --prefix option with leading slashes:
-		PathPrefix: "//evil.example.com/goat",
-		CGIMode:    true,
-	}
-	s, err := NewServer(options)
-	if err != nil {
-		t.Error(err)
-	}
-
+// TestPathPrefix tests that the provided path prefix is normalized correctly.
+// If a leading '/' is missing, one should be added.
+// If multiple leading '/' are present, they should be collapsed to one.
+// Additionally verify that this prevents open redirects when enforcing the path prefix.
+func TestPathPrefix(t *testing.T) {
 	tests := []struct {
 		name         string
-		target       string
-		wantHandled  bool
+		prefix       string
+		wantPrefix   string
 		wantLocation string
 	}{
+		{
+			name:         "no-leading-slash",
+			prefix:       "javascript:alert(1)",
+			wantPrefix:   "/javascript:alert(1)",
+			wantLocation: "/javascript:alert(1)/",
+		},
 		{
 			name:   "2-slashes",
-			target: "http://localhost//evil.example.com/goat",
+			prefix: "//evil.example.com/goat",
 			// We must also get the trailing slash added:
+			wantPrefix:   "/evil.example.com/goat",
 			wantLocation: "/evil.example.com/goat/",
 		},
+		{
+			name:   "absolute-url",
+			prefix: "http://evil.example.com",
+			// We must also get the trailing slash added:
+			wantPrefix:   "/http:/evil.example.com",
+			wantLocation: "/http:/evil.example.com/",
+		},
+		{
+			name:   "double-dot",
+			prefix: "/../.././etc/passwd",
+			// We must also get the trailing slash added:
+			wantPrefix:   "/etc/passwd",
+			wantLocation: "/etc/passwd/",
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			options := ServerOpts{
+				Mode:       LoginServerMode,
+				PathPrefix: tt.prefix,
+				CGIMode:    true,
+			}
+			s, err := NewServer(options)
+			if err != nil {
+				t.Error(err)
+			}
+
+			// verify provided prefix was normalized correctly
+			if s.pathPrefix != tt.wantPrefix {
+				t.Errorf("prefix was not normalized correctly; want=%q, got=%q", tt.wantPrefix, s.pathPrefix)
+			}
+
 			s.logf = t.Logf
-			r := httptest.NewRequest(httpm.GET, tt.target, nil)
+			r := httptest.NewRequest(httpm.GET, "http://localhost/", nil)
 			w := httptest.NewRecorder()
 			s.ServeHTTP(w, r)
 			res := w.Result()
@@ -976,7 +1005,7 @@ func TestNoOffSiteRedirect(t *testing.T) {
 
 			location := w.Header().Get("Location")
 			if location != tt.wantLocation {
-				t.Errorf("request(%q) got wrong location; want=%q, got=%q", tt.target, tt.wantLocation, location)
+				t.Errorf("request got wrong location; want=%q, got=%q", tt.wantLocation, location)
 			}
 		})
 	}