From dfcc20d165b849efc176cb658ca7eb3dc8111ddf Mon Sep 17 00:00:00 2001 From: Dan Lorenc Date: Sat, 9 May 2026 10:46:24 -0400 Subject: [PATCH] refactor(server): consolidate duplicate URL parsing into splitRepoPath Both parseRepoPath (handlers.go) and repoAndSubPath (middleware.go) were independently implementing the same /-/ URL parsing logic, with a comment in both files warning they must stay in sync. This change introduces splitRepoPath in a new url.go file as the single source of truth. Both functions now delegate to it, eliminating the duplication. Also adds TestSplitRepoPathConsistency to verify the two wrappers agree on repo name and endpoint for the same input paths. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- internal/server/handlers.go | 20 ++------------ internal/server/middleware.go | 14 ++-------- internal/server/middleware_test.go | 44 +++++++++++++++++++++++++++++- internal/server/url.go | 28 +++++++++++++++++++ 4 files changed, 76 insertions(+), 30 deletions(-) create mode 100644 internal/server/url.go 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 +}