diff --git a/internal/server/handlers.go b/internal/server/handlers.go index 399ac03..0156b5b 100644 --- a/internal/server/handlers.go +++ b/internal/server/handlers.go @@ -10,30 +10,14 @@ import ( ) // parseRepoPath parses a /repos/... URL path into the full repo name and the -// endpoint string that follows the "/-/" separator. +// endpoint string that follows the "/-/" separator. Delegates to splitRepoPath. // // /repos/acme/myrepo/-/tree → ("acme/myrepo", "tree", true) // /repos/acme/team/sub/-/commit → ("acme/team/sub", "commit", true) // /repos/acme/myrepo → ("acme/myrepo", "", true) // bare repo // /something/else → ("", "", false) -// -// NOTE: repoAndSubPath in middleware.go parses the same "/-/" URL format for -// RBAC purposes. Both functions must be kept in sync if the URL structure changes. func parseRepoPath(path string) (repoName, endpoint string, ok bool) { - const prefix = "/repos/" - if !strings.HasPrefix(path, prefix) { - return "", "", false - } - rest := path[len(prefix):] - if rest == "" { - return "", "", false - } - repoName, endpoint, found := strings.Cut(rest, "/-/") - if !found { - // Bare /repos/:repopath — no endpoint - return rest, "", true - } - return repoName, endpoint, true + return splitRepoPath(path) } // handleReposPrefix is the catch-all handler for all /repos/... paths (except diff --git a/internal/server/middleware.go b/internal/server/middleware.go index c96b2bc..d670608 100644 --- a/internal/server/middleware.go +++ b/internal/server/middleware.go @@ -192,24 +192,16 @@ func RBACMiddleware(roles RoleStore, bootstrapAdmin string) func(http.Handler) h // repoAndSubPath extracts the repo name and sub-path from a /repos/.../-/subpath URL. // Returns ("", "") for non-repo paths or bare /repos/:repopath paths with no /-/ separator. +// Delegates to splitRepoPath for the core parsing logic. // // Examples: // // /repos/acme/myrepo/-/commit → ("acme/myrepo", "commit") // /repos/acme/team/sub/-/tree → ("acme/team/sub", "tree") // /repos/acme/myrepo → ("", "") — bare repo, no RBAC check needed -// -// NOTE: parseRepoPath in handlers.go parses the same "/-/" URL format for routing. -// Both functions must be kept in sync if the URL structure changes. func repoAndSubPath(path string) (repo, subPath string) { - const prefix = "/repos/" - if !strings.HasPrefix(path, prefix) { - return "", "" - } - rest := path[len(prefix):] - repo, subPath, found := strings.Cut(rest, "/-/") - if !found { - // /repos/:repopath with no /-/ — no sub-path, no RBAC check needed. + repo, subPath, ok := splitRepoPath(path) + if !ok || subPath == "" { return "", "" } return repo, subPath diff --git a/internal/server/middleware_test.go b/internal/server/middleware_test.go index 209c09f..028437d 100644 --- a/internal/server/middleware_test.go +++ b/internal/server/middleware_test.go @@ -1066,9 +1066,51 @@ func TestJobTokenMiddleware_WrongIssuer(t *testing.T) { } // --------------------------------------------------------------------------- -// repoAndSubPath tests +// URL parsing tests // --------------------------------------------------------------------------- +// TestSplitRepoPathConsistency verifies that parseRepoPath and repoAndSubPath +// agree on the repo name and endpoint for every path that contains a "/-/" +// separator. Both functions delegate to splitRepoPath, so any divergence would +// indicate a bug in the wrapper logic. +func TestSplitRepoPathConsistency(t *testing.T) { + paths := []string{ + "/repos/acme/myrepo/-/tree", + "/repos/acme/myrepo/-/commit", + "/repos/acme/myrepo/-/branch/feature/with/slashes", + "/repos/acme/team/sub/-/tree", + "/repos/acme/team/sub/-/roles/bob@example.com", + "/repos/org/myrepo/-/branches", + // Paths without /-/ — both functions return empty strings for repo/endpoint. + "/repos/myrepo", + "/repos/org/myrepo", + "/healthz", + "", + } + for _, path := range paths { + parsedRepo, parsedEndpoint, parsedOK := parseRepoPath(path) + rbacRepo, rbacEndpoint := repoAndSubPath(path) + + // For paths with a /-/ separator, both functions must agree on repo and endpoint. + if parsedOK && parsedEndpoint != "" { + if parsedRepo != rbacRepo { + t.Errorf("path=%q: repo mismatch: parseRepoPath=%q repoAndSubPath=%q", + path, parsedRepo, rbacRepo) + } + if parsedEndpoint != rbacEndpoint { + t.Errorf("path=%q: endpoint mismatch: parseRepoPath=%q repoAndSubPath=%q", + path, parsedEndpoint, rbacEndpoint) + } + } else { + // No /-/ separator: repoAndSubPath must return ("", ""). + if rbacRepo != "" || rbacEndpoint != "" { + t.Errorf("path=%q: repoAndSubPath should return (\"\", \"\") for bare/non-repo path, got (%q, %q)", + path, rbacRepo, rbacEndpoint) + } + } + } +} + func TestRepoAndSubPath(t *testing.T) { tests := []struct { path string diff --git a/internal/server/url.go b/internal/server/url.go new file mode 100644 index 0000000..8164f67 --- /dev/null +++ b/internal/server/url.go @@ -0,0 +1,28 @@ +package server + +import "strings" + +// splitRepoPath parses a /repos/... URL path into the full repo name and the +// endpoint string that follows the "/-/" separator. It is the single source of +// truth for the "/-/" URL format; both parseRepoPath (handlers.go) and +// repoAndSubPath (middleware.go) delegate here. +// +// /repos/acme/myrepo/-/tree → ("acme/myrepo", "tree", true) +// /repos/acme/team/sub/-/commit → ("acme/team/sub", "commit", true) +// /repos/acme/myrepo → ("acme/myrepo", "", true) // bare repo, no endpoint +// /something/else → ("", "", false) +func splitRepoPath(path string) (repoName, endpoint string, ok bool) { + const prefix = "/repos/" + if !strings.HasPrefix(path, prefix) { + return "", "", false + } + rest := path[len(prefix):] + if rest == "" { + return "", "", false + } + repoName, endpoint, found := strings.Cut(rest, "/-/") + if !found { + return rest, "", true + } + return repoName, endpoint, true +}