core: add stricter route parsing

Move the route parsing done in 'bundle-server.go' to a new method
'core.ParseRoute'. In addition to the removal of excess path separators the
method already does, limit the characters in each segment of the route
(owner, repo, filename) to alphanumeric characters plus '-', '_', and '.'.
Also, explicitly block '.' and '..' path components. These measures are
added to prevent serving content from arbitrary relative paths in the bundle
server. If less stringent path validation is desired in the future, the
route parsing can be updated accordingly.

Update all functions that use a user-supplied route
('bundleWebServer.serve', 'repoProvider.CreateRepository',
'repoProvider.RemoveRoute') to use the new parsing function.

Signed-off-by: Victoria Dye <[email protected]>

Author: vdye

cmd/git-bundle-web-server/bundle-server.go

@@ -9,7 +9,6 @@ import (
 	"os"
 	"os/signal"
 	"path/filepath"
-	"strings"
 	"sync"
 	"syscall"
 	"time"
@@ -81,30 +80,14 @@ func NewBundleWebServer(logger log.TraceLogger,
 	return bundleServer, nil
 }
 
-func (b *bundleWebServer) parseRoute(ctx context.Context, path string) (string, string, string, error) {
-	elements := strings.FieldsFunc(path, func(char rune) bool { return char == '/' })
-	switch len(elements) {
-	case 0:
-		return "", "", "", fmt.Errorf("empty route")
-	case 1:
-		return "", "", "", fmt.Errorf("route has owner, but no repo")
-	case 2:
-		return elements[0], elements[1], "", nil
-	case 3:
-		return elements[0], elements[1], elements[2], nil
-	default:
-		return "", "", "", fmt.Errorf("path has depth exceeding three")
-	}
-}
-
 func (b *bundleWebServer) serve(w http.ResponseWriter, r *http.Request) {
 	ctx := r.Context()
 
 	ctx, exitRegion := b.logger.Region(ctx, "http", "serve")
 	defer exitRegion()
 
 	path := r.URL.Path
-	owner, repo, filename, err := b.parseRoute(ctx, path)
+	owner, repo, filename, err := core.ParseRoute(path, false)
 	if err != nil {
 		w.WriteHeader(http.StatusNotFound)
 		fmt.Printf("Failed to parse route: %s\n", err)

internal/core/funcs.go

@@ -1,6 +1,7 @@
 package core
 
 import (
+	"fmt"
 	"regexp"
 	"strings"
 )
@@ -27,3 +28,33 @@ func GetRouteFromUrl(url string) (string, bool) {
 
 	return "", false
 }
+
+func ParseRoute(route string, repoOnly bool) (string, string, string, error) {
+	elements := strings.FieldsFunc(route, func(char rune) bool { return char == '/' })
+	validElementPattern := regexp.MustCompile(`^[\w\.-]+$`)
+	for _, e := range elements {
+		if !validElementPattern.MatchString(e) {
+			return "", "", "",
+				fmt.Errorf("invalid element '%s'; route may only contain alphanumeric characters, '.', '_', and/or '-'", e)
+		}
+		if e == "." || e == ".." {
+			return "", "", "", fmt.Errorf("invalid route element '%s'", e)
+		}
+	}
+
+	switch len(elements) {
+	case 0:
+		return "", "", "", fmt.Errorf("empty route")
+	case 1:
+		return "", "", "", fmt.Errorf("route has owner, but no repo")
+	case 2:
+		return elements[0], elements[1], "", nil
+	case 3:
+		if repoOnly {
+			return "", "", "", fmt.Errorf("route is too deep")
+		}
+		return elements[0], elements[1], elements[2], nil
+	default:
+		return "", "", "", fmt.Errorf("route is too deep")
+	}
+}

internal/core/funcs_test.go

@@ -120,3 +120,116 @@ func TestGetRouteFromUrl(t *testing.T) {
 		})
 	}
 }
+
+var parseRouteTests = []struct {
+	route         string
+	repoOnly      bool
+	expectedOwner string
+	expectedRepo  string
+	expectedFile  string
+	expectedError bool
+}{
+	// Valid routes
+	{
+		"test/repo/1.bundle",
+		false,
+		"test", "repo", "1.bundle",
+		false,
+	},
+	{
+		"test/repo",
+		false,
+		"test", "repo", "",
+		false,
+	},
+	{
+		"test_with_undercore/repo-with-dash",
+		false,
+		"test_with_undercore", "repo-with-dash", "",
+		false,
+	},
+	{
+		"//lots/of////path_separators...bundle//",
+		false,
+		"lots", "of", "path_separators...bundle",
+		false,
+	},
+	{
+		"test/repo",
+		true,
+		"test", "repo", "",
+		false,
+	},
+
+	// Invalid routes
+	{
+		"",
+		false,
+		"", "", "",
+		true,
+	},
+	{
+		"//",
+		false,
+		"", "", "",
+		true,
+	},
+	{
+		"too-short",
+		false,
+		"", "", "",
+		true,
+	},
+	{
+		"much/much/MUCH/too/long",
+		false,
+		"", "", "",
+		true,
+	},
+	{
+		"test/repo with spaces",
+		false,
+		"", "", "",
+		true,
+	},
+	{
+		"test/./repo",
+		true,
+		"", "", "",
+		true,
+	},
+	{
+		"../test/repo",
+		true,
+		"", "", "",
+		true,
+	},
+	{
+		"test/repo/1.bundle",
+		true,
+		"", "", "",
+		true,
+	},
+}
+
+func TestParseRoute(t *testing.T) {
+	for _, tt := range parseRouteTests {
+		title := tt.route
+		if tt.repoOnly {
+			title += " (repo only)"
+		}
+
+		t.Run(title, func(t *testing.T) {
+			owner, repo, file, err := core.ParseRoute(tt.route, tt.repoOnly)
+
+			if tt.expectedError {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+				assert.Equal(t, tt.expectedOwner, owner)
+				assert.Equal(t, tt.expectedRepo, repo)
+				assert.Equal(t, tt.expectedFile, file)
+			}
+		})
+	}
+}

internal/core/repo.go

@@ -50,6 +50,12 @@ func (r *repoProvider) CreateRepository(ctx context.Context, route string) (*Rep
 	ctx, exitRegion := r.logger.Region(ctx, "repo", "create_repo")
 	defer exitRegion()
 
+	ownerName, repoName, _, err := ParseRoute(route, true)
+	if err != nil {
+		return nil, err
+	}
+	route = ownerName + "/" + repoName
+
 	user, err := r.user.CurrentUser()
 	if err != nil {
 		return nil, err
@@ -93,6 +99,12 @@ func (r *repoProvider) RemoveRoute(ctx context.Context, route string) error {
 	ctx, exitRegion := r.logger.Region(ctx, "repo", "remove_route")
 	defer exitRegion()
 
+	ownerName, repoName, _, err := ParseRoute(route, true)
+	if err != nil {
+		return err
+	}
+	route = ownerName + "/" + repoName
+
 	repos, err := r.GetRepositories(ctx)
 	if err != nil {
 		return fmt.Errorf("failed to parse routes file: %w", err)