diff --git a/internal/strategy/git/export_test.go b/internal/strategy/git/export_test.go index 52e5f4b..34bc88f 100644 --- a/internal/strategy/git/export_test.go +++ b/internal/strategy/git/export_test.go @@ -6,6 +6,10 @@ import ( "github.com/block/cachew/internal/gitclone" ) +// Exports unexported symbols for use by external test packages. + +func IsGitRequest(pathValue string) bool { return isGitRequest(pathValue) } + func (s *Strategy) GenerateAndUploadSnapshot(ctx context.Context, repo *gitclone.Repository) error { return s.generateAndUploadSnapshot(ctx, repo) } diff --git a/internal/strategy/git/git.go b/internal/strategy/git/git.go index 4862bd5..37279df 100644 --- a/internal/strategy/git/git.go +++ b/internal/strategy/git/git.go @@ -244,16 +244,6 @@ func (s *Strategy) handleRequest(w http.ResponseWriter, r *http.Request) { return } - // Proxy LFS Batch API requests directly to GitHub. Cachew doesn't cache - // individual LFS objects, but it needs to handle these requests because - // git's url.*.insteadOf rewrites the LFS endpoint URL to point at cachew. - if strings.Contains(pathValue, "/info/lfs/") { - s.metrics.recordRequest(ctx, "lfs-api") - logger.DebugContext(ctx, "Proxying LFS API request to upstream", "uri", pathValue) - s.forwardToUpstream(w, r, host, pathValue) - return - } - service := r.URL.Query().Get("service") isReceivePack := service == "git-receive-pack" || strings.HasSuffix(pathValue, "/git-receive-pack") @@ -264,8 +254,18 @@ func (s *Strategy) handleRequest(w http.ResponseWriter, r *http.Request) { return } - s.metrics.recordRequest(ctx, "upload-pack") - s.handleGitRequest(w, r, host, pathValue) + // Only handle known git smart protocol operations locally (info/refs + // discovery and git-upload-pack negotiation). Everything else (LFS API + // requests, unknown paths, etc.) is forwarded to upstream so it isn't + // mistakenly treated as a clone/fetch. + if isGitRequest(pathValue) { + s.handleGitRequest(w, r, host, pathValue) + return + } + + s.metrics.recordRequest(ctx, "forward") + logger.DebugContext(ctx, "Forwarding non-git request to upstream", "uri", pathValue) + s.forwardToUpstream(w, r, host, pathValue) } func (s *Strategy) handleGitRequest(w http.ResponseWriter, r *http.Request, host, pathValue string) { @@ -457,6 +457,13 @@ func (s *Strategy) serveWithSpool(w http.ResponseWriter, r *http.Request, host, return nil } +// isGitRequest reports whether pathValue matches a git smart HTTP protocol +// endpoint (info/refs discovery or git-upload-pack negotiation). +func isGitRequest(pathValue string) bool { + return strings.HasSuffix(pathValue, "/info/refs") || + strings.HasSuffix(pathValue, "/git-upload-pack") +} + func ExtractRepoPath(pathValue string) string { repoPath := pathValue repoPath = strings.TrimSuffix(repoPath, "/info/refs") diff --git a/internal/strategy/git/git_test.go b/internal/strategy/git/git_test.go index aa8e1ba..b6f4f52 100644 --- a/internal/strategy/git/git_test.go +++ b/internal/strategy/git/git_test.go @@ -91,6 +91,31 @@ func TestNew(t *testing.T) { } } +func TestIsGitRequest(t *testing.T) { + tests := []struct { + name string + input string + expected bool + }{ + {name: "InfoRefs", input: "org/repo/info/refs", expected: true}, + {name: "GitUploadPack", input: "org/repo/git-upload-pack", expected: true}, + {name: "GitReceivePack", input: "org/repo/git-receive-pack", expected: false}, + {name: "LFSBatchAPI", input: "org/repo.git/info/lfs/objects/batch", expected: false}, + {name: "LFSObjectDownload", input: "org/repo.git/info/lfs/objects/abc123", expected: false}, + {name: "PlainRepoPath", input: "org/repo", expected: false}, + {name: "RandomPath", input: "org/repo/some/random/path", expected: false}, + {name: "InfoRefsWithGitSuffix", input: "org/repo.git/info/refs", expected: true}, + {name: "UploadPackWithGitSuffix", input: "org/repo.git/git-upload-pack", expected: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := git.IsGitRequest(tt.input) + assert.Equal(t, tt.expected, result) + }) + } +} + func TestExtractRepoPath(t *testing.T) { tests := []struct { name string