Merge pull request #63 from vdye/vdye/stricter-route-parsing

Stricter route parsing (+ CODEOWNERS update)

Author: vdye

CODEOWNERS

@@ -1,2 +1,2 @@
 # This repository is maintained by:
-*       @vdye @derrickstolee @ldennington
+*       @git-ecosystem/hubbers

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)