safeweb: handle mux pattern collisions more generally (#11801)

Fixes #11800

Signed-off-by: Chris Palmer <cpalmer@tailscale.com>

safeweb/http.go

@@ -70,12 +70,14 @@
 package safeweb
 
 import (
+	"cmp"
 	crand "crypto/rand"
 	"fmt"
 	"log"
 	"net"
 	"net/http"
 	"net/url"
+	"path"
 	"strings"
 
 	"github.com/gorilla/csrf"
@@ -195,6 +197,30 @@ func NewServer(config Config) (*Server, error) {
 	return s, nil
 }
 
+type handlerType int
+
+const (
+	unknownHandler handlerType = iota
+	apiHandler
+	browserHandler
+)
+
+// checkHandlerType returns either apiHandler or browserHandler, depending on
+// whether apiPattern or browserPattern is more specific (i.e. which pattern
+// contains more pathname components). If they are equally specific, it returns
+// unknownHandler.
+func checkHandlerType(apiPattern, browserPattern string) handlerType {
+	c := cmp.Compare(strings.Count(path.Clean(apiPattern), "/"), strings.Count(path.Clean(browserPattern), "/"))
+	switch {
+	case c > 0:
+		return apiHandler
+	case c < 0:
+		return browserHandler
+	default:
+		return unknownHandler
+	}
+}
+
 func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	_, bp := s.BrowserMux.Handler(r)
 	_, ap := s.APIMux.Handler(r)
@@ -206,24 +232,25 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	case bp == "" && ap == "": // neither match
 		http.NotFound(w, r)
 	case bp != "" && ap != "":
-		// Both muxes match the path. This can be because:
+		// Both muxes match the path. Route to the more-specific handler (as
-		//  * one of them registers a wildcard "/" handler
+		// determined by the number of components in the path). If it somehow
-		//  * there are overlapping specific handlers
+		// happens that both patterns are equally specific, something strange
+		// has happened; say so.
 		//
-		// If it's the former, route to the more-specific handler. If it's the
+		// NOTE: checkHandlerType does not know about what the serve* handlers
-		// latter - that's a bug so return an error to avoid mis-routing the
+		// will do — including, possibly, redirecting to more specific patterns.
-		// request.
+		// If you have a less-specific pattern that redirects to something more
-		//
+		// specific, this logic will not do what you wanted.
-		// TODO(awly): match the longest path instead of only special-casing
+		handler := checkHandlerType(ap, bp)
-		// "/".
+		switch handler {
-		switch {
+		case apiHandler:
-		case bp == "/":
 			s.serveAPI(w, r)
-		case ap == "/":
+		case browserHandler:
 			s.serveBrowser(w, r)
 		default:
-			log.Printf("conflicting mux paths in safeweb: request %q matches browser mux pattern %q and API mux patter %q; returning 500", r.URL.Path, bp, ap)
+			s := http.StatusInternalServerError
-			http.Error(w, "multiple handlers match this request", http.StatusInternalServerError)
+			log.Printf("conflicting mux paths in safeweb: request %q matches browser mux pattern %q and API mux pattern %q; returning %d", r.URL.Path, bp, ap, s)
+			http.Error(w, "multiple handlers match this request", s)
 		}
 	}
 }

safeweb/http_test.go

@@ -447,7 +447,7 @@ func TestRouting(t *testing.T) {
 			browserPatterns: []string{"/foo/"},
 			apiPatterns:     []string{"/foo/bar/"},
 			requestPath:     "/foo/bar/baz",
-			want:            "multiple handlers match this request",
+			want:            "api",
 		},
 		{
 			desc:            "no match",
@@ -488,3 +488,68 @@ func TestRouting(t *testing.T) {
 		})
 	}
 }
+
+func TestGetMoreSpecificPattern(t *testing.T) {
+	for _, tt := range []struct {
+		desc string
+		a    string
+		b    string
+		want handlerType
+	}{
+		{
+			desc: "identical",
+			a:    "/foo/bar",
+			b:    "/foo/bar",
+			want: unknownHandler,
+		},
+		{
+			desc: "identical prefix",
+			a:    "/foo/bar/",
+			b:    "/foo/bar/",
+			want: unknownHandler,
+		},
+		{
+			desc: "trailing slash",
+			a:    "/foo",
+			b:    "/foo/", // path.Clean will strip the trailing slash.
+			want: unknownHandler,
+		},
+		{
+			desc: "same prefix",
+			a:    "/foo/bar/quux",
+			b:    "/foo/bar/",
+			want: apiHandler,
+		},
+		{
+			desc: "almost same prefix, but not a path component",
+			a:    "/goat/sheep/cheese",
+			b:    "/goat/sheepcheese/",
+			want: apiHandler,
+		},
+		{
+			desc: "attempt to make less-specific pattern look more specific",
+			a:    "/goat/cat/buddy",
+			b:    "/goat/../../../../../../../cat", // path.Clean catches this foolishness
+			want: apiHandler,
+		},
+		{
+			desc: "2 names for / (1)",
+			a:    "/",
+			b:    "/../../../../../../",
+			want: unknownHandler,
+		},
+		{
+			desc: "2 names for / (2)",
+			a:    "/",
+			b:    "///////",
+			want: unknownHandler,
+		},
+	} {
+		t.Run(tt.desc, func(t *testing.T) {
+			got := checkHandlerType(tt.a, tt.b)
+			if got != tt.want {
+				t.Errorf("got %q, want %q", got, tt.want)
+			}
+		})
+	}
+}