From b62a3fc89535756de2aa05ee3f298ef563bdbd34 Mon Sep 17 00:00:00 2001
From: Chris Palmer <cpalmer@tailscale.com>
Date: Wed, 13 Dec 2023 14:28:50 -0800
Subject: [PATCH] client/web: keep redirects on-site (#10525)

Ensure we don't create Location: header URLs that have leading //, which is a
schema-less reference to arbitrary 3rd-party sites. That is, //example.com/foo
redirects off-site, while /example.com/foo is an on-site path URL.

Fixes tailscale/corp#16268

Signed-off-by: Chris Palmer <cpalmer@tailscale.com>
---
 client/web/web.go      |  8 ++++++++
 client/web/web_test.go | 45 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/client/web/web.go b/client/web/web.go
index 1fee3c62d..a3310bbfe 100644
--- a/client/web/web.go
+++ b/client/web/web.go
@@ -15,6 +15,7 @@ import (
 	"net/http"
 	"net/netip"
 	"os"
+	"path"
 	"path/filepath"
 	"slices"
 	"strings"
@@ -174,6 +175,13 @@ func NewServer(opts ServerOpts) (s *Server, err error) {
 		newAuthURL:  opts.NewAuthURL,
 		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), "/\\")
+	}
 	if s.mode == ManageServerMode {
 		if opts.NewAuthURL == nil {
 			return nil, fmt.Errorf("must provide a NewAuthURL implementation")
diff --git a/client/web/web_test.go b/client/web/web_test.go
index bbf764bc7..ee46551fe 100644
--- a/client/web/web_test.go
+++ b/client/web/web_test.go
@@ -939,6 +939,49 @@ 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)
+	}
+
+	tests := []struct {
+		name         string
+		target       string
+		wantHandled  bool
+		wantLocation string
+	}{
+		{
+			name:   "2-slashes",
+			target: "http://localhost//evil.example.com/goat",
+			// We must also get the trailing slash added:
+			wantLocation: "/evil.example.com/goat/",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			s.logf = t.Logf
+			r := httptest.NewRequest(httpm.GET, tt.target, nil)
+			w := httptest.NewRecorder()
+			s.ServeHTTP(w, r)
+			res := w.Result()
+			defer res.Body.Close()
+
+			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)
+			}
+		})
+	}
+}
+
 func TestRequireTailscaleIP(t *testing.T) {
 	self := &ipnstate.PeerStatus{
 		TailscaleIPs: []netip.Addr{
@@ -1007,7 +1050,7 @@ func TestRequireTailscaleIP(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		t.Run(tt.target, func(t *testing.T) {
+		t.Run(tt.name, func(t *testing.T) {
 			s.logf = t.Logf
 			r := httptest.NewRequest(httpm.GET, tt.target, nil)
 			w := httptest.NewRecorder()