diff --git a/appview/metrics/middleware.go b/appview/metrics/middleware.go index cce45ddb..18026b54 100644 --- a/appview/metrics/middleware.go +++ b/appview/metrics/middleware.go @@ -8,6 +8,7 @@ import ( "time" "github.com/go-chi/chi/v5" + "github.com/samber/lo" ) type statusRecorder struct { @@ -32,19 +33,23 @@ func Middleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { rec := &statusRecorder{ResponseWriter: w, status: http.StatusOK} start := time.Now() + returned := false - next.ServeHTTP(rec, r) + defer func() { + // use the matched route pattern to avoid high cardinality + routePattern := chi.RouteContext(r.Context()).RoutePattern() + if routePattern == "" { + routePattern = "unknown" + } - // use the matched route pattern to avoid high cardinality - routePattern := chi.RouteContext(r.Context()).RoutePattern() - if routePattern == "" { - routePattern = "unknown" - } + status := lo.Ternary(returned, fmt.Sprintf("%d", rec.status), "aborted") + duration := time.Since(start).Seconds() - status := fmt.Sprintf("%d", rec.status) - duration := time.Since(start).Seconds() + HttpRequestsTotal.WithLabelValues(r.Method, routePattern, status).Inc() + HttpRequestDuration.WithLabelValues(r.Method, routePattern, status).Observe(duration) + }() - HttpRequestsTotal.WithLabelValues(r.Method, routePattern, status).Inc() - HttpRequestDuration.WithLabelValues(r.Method, routePattern, status).Observe(duration) + next.ServeHTTP(rec, r) + returned = true }) } diff --git a/appview/pages/templates/repo/fragments/artifactList.html b/appview/pages/templates/repo/fragments/artifactList.html index eae6816f..f79d6154 100644 --- a/appview/pages/templates/repo/fragments/artifactList.html +++ b/appview/pages/templates/repo/fragments/artifactList.html @@ -13,7 +13,7 @@
{{ i "archive" "w-4 h-4" }} - + Source code (.tar.gz)
diff --git a/appview/pages/templates/repo/fragments/cloneDropdown.html b/appview/pages/templates/repo/fragments/cloneDropdown.html index 5dd22e5b..3a923377 100644 --- a/appview/pages/templates/repo/fragments/cloneDropdown.html +++ b/appview/pages/templates/repo/fragments/cloneDropdown.html @@ -68,14 +68,14 @@
{{ i "download" "w-4 h-4" }} Download tar.gz {{ i "download" "w-4 h-4" }} diff --git a/appview/repo/archive.go b/appview/repo/archive.go index a035a359..740d1c59 100644 --- a/appview/repo/archive.go +++ b/appview/repo/archive.go @@ -30,7 +30,7 @@ func (rp *Repo) DownloadArchive(w http.ResponseWriter, r *http.Request) { lo.Ternary(status == http.StatusServiceUnavailable, rp.pages.Error503, rp.pages.Error404)(w) } - params, err := parseArchiveRequest(r) + request, err := parseArchiveRequest(r) if err != nil { l.Warn("rejecting archive request", "err", err) fail(http.StatusNotFound) @@ -44,12 +44,11 @@ func (rp *Repo) DownloadArchive(w http.ResponseWriter, r *http.Request) { return } - name := gitutil.RepoName(f.Slug()) - params.Prefix = params.Prefix.OrDefault(name, params.Rev) + served := request.params.Serve(gitutil.RepoName(f.Slug())) // build the xrpc url xrpcURL := fmt.Sprintf("%s/xrpc/%s?%s", - rp.config.KnotMirror.Url, tangled.GitTempGetArchiveNSID, params.Query(f.RepoDid).Encode()) + rp.config.KnotMirror.Url, tangled.GitTempGetArchiveNSID, served.Query(f.RepoDid).Encode()) // make the get request req, err := http.NewRequestWithContext(r.Context(), http.MethodGet, xrpcURL, nil) @@ -58,6 +57,7 @@ func (rp *Repo) DownloadArchive(w http.ResponseWriter, r *http.Request) { fail(http.StatusServiceUnavailable) return } + gitutil.ForwardHeaders(req.Header, r.Header, "If-None-Match") resp, err := rp.archiveClient.Do(req) if err != nil { l.Error("failed to call XRPC repo.archive", "err", err) @@ -66,26 +66,54 @@ func (rp *Repo) DownloadArchive(w http.ResponseWriter, r *http.Request) { } defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - l.Error("XRPC repo.archive failed", "status", resp.StatusCode, "ref", params.Rev) + revalidated := resp.StatusCode == http.StatusNotModified + if resp.StatusCode != http.StatusOK && !revalidated { + l.Error("XRPC repo.archive failed", "status", resp.StatusCode, "ref", request.params.Rev) overloaded := resp.StatusCode >= http.StatusInternalServerError || resp.StatusCode == http.StatusTooManyRequests fail(lo.Ternary(overloaded, http.StatusServiceUnavailable, http.StatusNotFound)) return } - params.SetHeaders(w.Header(), name) + served.SetHeaders(w.Header()) + gitutil.ForwardHeaders(w.Header(), resp.Header, "Etag") - if resolvedRev, err := gitutil.ParseImmutableLink(resp.Header.Get("Link")); err == nil { - w.Header().Set("Link", gitutil.ImmutableLink(rp.immutableArchiveURL(f, params.WithRev(resolvedRev)))) + resolvedRev, _ := gitutil.ParseImmutableLink(resp.Header.Get("Link")) + if resolvedRev != "" { + w.Header().Set("Link", gitutil.ImmutableLink(rp.immutableArchiveURL(f, served.WithRev(resolvedRev)))) } + setArchiveCache(w.Header(), request, resolvedRev) + + if revalidated { + w.WriteHeader(http.StatusNotModified) + return + } + gitutil.ForwardHeaders(w.Header(), resp.Header, "Content-Length") // stream the archive data directly - if _, err := io.Copy(w, resp.Body); err != nil { + body := gitutil.NewResponseBody(w) + if _, err := io.Copy(body, resp.Body); err != nil { l.Error("failed to write response", "err", err) + body.Fail() + } +} + +type archiveRequest struct { + params gitutil.ArchiveParams + guessed bool +} + +const immutableArchiveCache = "public, max-age=31536000" + +func setArchiveCache(h http.Header, request archiveRequest, resolved gitutil.Rev) { + addressed := resolved.IsObjectID() && request.params.Rev == resolved && + request.params.Prefix != "" && !request.guessed + h.Set("Cache-Control", lo.Ternary(addressed, immutableArchiveCache, "no-cache")) + if request.guessed { + h.Add("Vary", "User-Agent") } } -func parseArchiveRequest(r *http.Request) (gitutil.ArchiveParams, error) { +func parseArchiveRequest(r *http.Request) (archiveRequest, error) { ref := chi.URLParam(r, "*") if unescaped, err := url.PathUnescape(ref); err == nil && r.URL.RawPath != "" { ref = unescaped @@ -100,34 +128,33 @@ func parseArchiveRequest(r *http.Request) (gitutil.ArchiveParams, error) { rev, err := gitutil.ParseRev(ref) if err != nil { - return gitutil.ArchiveParams{}, err + return archiveRequest{}, err } query := r.URL.Query() query.Del("ref") - query.Set("format", archiveFormat(query.Get("format"), suffix, r.UserAgent()).String()) + format, guessed := archiveFormat(query.Get("format"), suffix, r.UserAgent()) + query.Set("format", format.String()) params, err := gitutil.ParseArchiveParams(query) if err != nil { - return gitutil.ArchiveParams{}, err + return archiveRequest{}, err } - return params.WithRev(rev), nil + return archiveRequest{params: params.WithRev(rev), guessed: guessed}, nil } -func archiveFormat(requested string, suffix gitutil.ArchiveFormat, userAgent string) gitutil.ArchiveFormat { +func archiveFormat(requested string, suffix gitutil.ArchiveFormat, userAgent string) (gitutil.ArchiveFormat, bool) { if format, err := gitutil.ParseArchiveFormat(requested); err == nil { - return format + return format, false } if suffix != "" { - return suffix + return suffix, false } ua := strings.ToLower(userAgent) windows := lo.SomeBy([]string{"windows", "win64", "win32"}, func(s string) bool { return strings.Contains(ua, s) }) - return lo.Ternary(windows, gitutil.ArchiveZip, gitutil.ArchiveTarGz) + return lo.Ternary(windows, gitutil.ArchiveZip, gitutil.ArchiveTarGz), true } -func (rp *Repo) immutableArchiveURL(f *models.Repo, params gitutil.ArchiveParams) string { - return fmt.Sprintf("%s/%s/archive/%s.%s?%s", - rp.config.Core.BaseUrl(), f.RepoIdentifier(), - url.PathEscape(params.Rev.String()), params.Format, - url.Values{"prefix": {params.Prefix.String()}}.Encode()) +func (rp *Repo) immutableArchiveURL(f *models.Repo, archive gitutil.ServedArchive) string { + return fmt.Sprintf("%s/%s/archive/%s", + rp.config.Core.BaseUrl(), f.RepoIdentifier(), archive.SuffixURL()) } diff --git a/appview/repo/archive_test.go b/appview/repo/archive_test.go index 1f79a3c4..d20c74cb 100644 --- a/appview/repo/archive_test.go +++ b/appview/repo/archive_test.go @@ -4,6 +4,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "slices" "strings" "testing" @@ -20,75 +21,139 @@ const ( testRepoPath = "/boltless.dev/squid" ) -func TestParseArchiveRequest(t *testing.T) { +func driveArchiveRoute(t *testing.T, target, userAgent string) (archiveRequest, error) { + t.Helper() + var ( - got gitutil.ArchiveParams - gotErr error + request archiveRequest + err error ) router := chi.NewRouter() router.Get("/{user}/{repo}"+archiveRoute, func(w http.ResponseWriter, r *http.Request) { - got, gotErr = parseArchiveRequest(r) + request, err = parseArchiveRequest(r) }) - resolvedParams := gitutil.ArchiveParams{ - Rev: "6f1d3a2b4c5d6e7f8091a2b3c4d5e6f708192a3b", - Format: gitutil.ArchiveZip, - Prefix: gitutil.ArchivePrefix("").OrDefault("squid", "refs/heads/feat/uni"), + if parsed, parseErr := url.Parse(target); parseErr == nil && parsed.Host != "" { + target = parsed.RequestURI() } + if rest, isRepoDid := strings.CutPrefix(target, "/"+testRepoDid); isRepoDid { + target = "/" + testRepoOwner + "/" + testRepoRkey + rest + } + + req := httptest.NewRequest(http.MethodGet, target, nil) + req.Header.Set("User-Agent", userAgent) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("%s: status = %d, want the archive route to match", target, rec.Code) + } + return request, err +} + +func TestImmutableArchiveURL(t *testing.T) { rp := &Repo{config: &config.Config{Core: config.CoreConfig{Dev: true, AppviewHost: "tangled.org"}}} - immutable, err := url.Parse(rp.immutableArchiveURL( - &models.Repo{Did: testRepoOwner, Rkey: testRepoRkey, Name: "squid", RepoDid: testRepoDid}, - resolvedParams, - )) - if err != nil { - t.Fatalf("the immutable URL must parse: %v", err) + repo := &models.Repo{Did: testRepoOwner, Rkey: testRepoRkey, Name: "squid", RepoDid: testRepoDid} + rev := gitutil.Rev("6f1d3a2b4c5d6e7f8091a2b3c4d5e6f708192a3b") + targz, zip := gitutil.ArchiveTarGz, gitutil.ArchiveZip + base := "http://tangled.org/" + testRepoDid + "/archive/" + rev.String() + + for _, tc := range []struct { + params gitutil.ArchiveParams + want string + }{ + {gitutil.ArchiveParams{Rev: rev, Format: zip, Prefix: "did:plc:boltless"}, base + ".zip?prefix=did%3Aplc%3Aboltless"}, + {gitutil.ArchiveParams{Rev: "refs/heads/feat/uni", Format: targz}, base + ".tar.gz?prefix=squid-feat-uni"}, + {gitutil.ArchiveParams{Rev: rev, Format: targz, Prefix: "kelp&format=zip#uni"}, base + ".tar.gz?prefix=kelp%26format%3Dzip%23uni"}, + } { + archive := tc.params.Serve("squid").WithRev(rev) + got := rp.immutableArchiveURL(repo, archive) + if got != tc.want { + t.Fatalf("immutable URL = %q, want %q, where the URL spells out the prefix that the knot will serve, percent-escaped", got, tc.want) + } + + request, err := driveArchiveRoute(t, got, "") + if err != nil || request.params.Serve("squid") != archive || request.guessed { + t.Fatalf("parsing %q gave %+v (guessed %v, err %v), want the archive %+v off the suffix", got, request.params, request.guessed, err, archive) + } + if again := rp.immutableArchiveURL(repo, request.params.Serve("squid")); again != got { + t.Errorf("rebuilding gave %q, want an immutable URL that is its own fixed point at %q", again, got) + } } +} +func TestParseArchiveRequest(t *testing.T) { windows := "Mozilla/5.0 (Windows NT 10.0; Win64; x64)" targz, zip := gitutil.ArchiveTarGz, gitutil.ArchiveZip cases := []struct { - name string - path string - userAgent string - want gitutil.ArchiveParams - wantErr bool + name string + path string + userAgent string + want gitutil.ArchiveParams + wantGuessed bool + wantErr bool }{ - {"short ref", testRepoPath + "/archive/v1.0.0?format=tar.gz", "", gitutil.ArchiveParams{Rev: "v1.0.0", Format: targz}, false}, - {"full ref unescaped", testRepoPath + "/archive/refs/tags/v1.0.0?prefix=did:plc:boltless", "", gitutil.ArchiveParams{Rev: "refs/tags/v1.0.0", Format: targz, Prefix: "did:plc:boltless"}, false}, - {"full ref escaped", testRepoPath + "/archive/refs%2Ftags%2Fv1.0.0?prefix=did:plc:boltless", "", gitutil.ArchiveParams{Rev: "refs/tags/v1.0.0", Format: targz, Prefix: "did:plc:boltless"}, false}, - {"format from suffix", testRepoPath + "/archive/refs/tags/v1.0.0.zip", "", gitutil.ArchiveParams{Rev: "refs/tags/v1.0.0", Format: zip}, false}, - {"unknown format query with a zip suffix", testRepoPath + "/archive/refs/tags/v1.0.0.zip?format=tar.xz", "", gitutil.ArchiveParams{Rev: "refs/tags/v1.0.0", Format: zip}, false}, - {"zip for a windows user agent", testRepoPath + "/archive/main?format=tar.xz", windows, gitutil.ArchiveParams{Rev: "main", Format: zip}, false}, - {"percent in the ref itself", testRepoPath + "/archive/refs/tags/a%252Fb", "", gitutil.ArchiveParams{Rev: "refs/tags/a%2Fb", Format: targz}, false}, - {"prefix wrapped in slashes", testRepoPath + "/archive/main?prefix=/kelp/", "", gitutil.ArchiveParams{Rev: "main", Format: targz, Prefix: "kelp"}, false}, - {"traversal escaped", testRepoPath + "/archive/..%2F..%2Fetc", "", gitutil.ArchiveParams{Rev: "../../etc", Format: targz}, false}, - {"parse deletes a ref query", testRepoPath + "/archive/main?ref=other", "", gitutil.ArchiveParams{Rev: "main", Format: targz}, false}, - {"our own immutable URL", immutable.RequestURI(), "", resolvedParams, false}, - - {"empty ref", testRepoPath + "/archive/", "", gitutil.ArchiveParams{}, true}, - {"escaped space", testRepoPath + "/archive/refs/tags/a%20b", "", gitutil.ArchiveParams{}, true}, - {"ref that git would read as an option", testRepoPath + "/archive/--output=%2Ftmp%2Fevil", "", gitutil.ArchiveParams{}, true}, + {"short ref", testRepoPath + "/archive/v1.0.0?format=tar.gz", "", gitutil.ArchiveParams{Rev: "v1.0.0", Format: targz}, false, false}, + {"full ref unescaped", testRepoPath + "/archive/refs/tags/v1.0.0?prefix=did:plc:boltless", "", gitutil.ArchiveParams{Rev: "refs/tags/v1.0.0", Format: targz, Prefix: "did:plc:boltless"}, true, false}, + {"full ref escaped", testRepoPath + "/archive/refs%2Ftags%2Fv1.0.0?prefix=did:plc:boltless", "", gitutil.ArchiveParams{Rev: "refs/tags/v1.0.0", Format: targz, Prefix: "did:plc:boltless"}, true, false}, + {"format from suffix", testRepoPath + "/archive/refs/tags/v1.0.0.zip", "", gitutil.ArchiveParams{Rev: "refs/tags/v1.0.0", Format: zip}, false, false}, + {"escaped ref with a format suffix", testRepoPath + "/archive/refs%2Ftags%2Fv1.0.0.tar.gz", "", gitutil.ArchiveParams{Rev: "refs/tags/v1.0.0", Format: targz}, false, false}, + {"a tag ending in a format suffix", testRepoPath + "/archive/refs%2Ftags%2Fv1.0.0.zip.tar.gz", "", gitutil.ArchiveParams{Rev: "refs/tags/v1.0.0.zip", Format: targz}, false, false}, + {"unknown format query with a zip suffix", testRepoPath + "/archive/refs/tags/v1.0.0.zip?format=tar.xz", "", gitutil.ArchiveParams{Rev: "refs/tags/v1.0.0", Format: zip}, false, false}, + {"zip for a windows user agent", testRepoPath + "/archive/main?format=tar.xz", windows, gitutil.ArchiveParams{Rev: "main", Format: zip}, true, false}, + {"percent in the ref itself", testRepoPath + "/archive/refs/tags/a%252Fb", "", gitutil.ArchiveParams{Rev: "refs/tags/a%2Fb", Format: targz}, true, false}, + {"prefix wrapped in slashes", testRepoPath + "/archive/main?prefix=/kelp/", "", gitutil.ArchiveParams{Rev: "main", Format: targz, Prefix: "kelp"}, true, false}, + {"traversal escaped", testRepoPath + "/archive/..%2F..%2Fetc", "", gitutil.ArchiveParams{Rev: "../../etc", Format: targz}, true, false}, + {"parse deletes a ref query", testRepoPath + "/archive/main?ref=other", "", gitutil.ArchiveParams{Rev: "main", Format: targz}, true, false}, + + {"empty ref", testRepoPath + "/archive/", "", gitutil.ArchiveParams{}, false, true}, + {"escaped space", testRepoPath + "/archive/refs/tags/a%20b", "", gitutil.ArchiveParams{}, false, true}, + {"ref that git would read as an option", testRepoPath + "/archive/--output=%2Ftmp%2Fevil", "", gitutil.ArchiveParams{}, false, true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got, gotErr = gitutil.ArchiveParams{}, nil - path := tc.path - if rest, isRepoDid := strings.CutPrefix(path, "/"+testRepoDid); isRepoDid { - path = "/" + testRepoOwner + "/" + testRepoRkey + rest - } - - req := httptest.NewRequest(http.MethodGet, path, nil) - req.Header.Set("User-Agent", tc.userAgent) - rec := httptest.NewRecorder() - router.ServeHTTP(rec, req) - - if rec.Code != http.StatusOK { - t.Fatalf("%s: status = %d, want the archive route to match", path, rec.Code) + got, gotErr := driveArchiveRoute(t, tc.path, tc.userAgent) + if got.params != tc.want || (gotErr != nil) != tc.wantErr { + t.Fatalf("params = %+v with err %v, want %+v and rejected = %v", got.params, gotErr, tc.want, tc.wantErr) } - if got != tc.want || (gotErr != nil) != tc.wantErr { - t.Errorf("params = %+v with err %v, want %+v and rejected = %v", got, gotErr, tc.want, tc.wantErr) + if got.guessed != tc.wantGuessed { + t.Errorf("format guessed from the user agent = %v, want %v", got.guessed, tc.wantGuessed) } }) } } + +func TestSetArchiveCache(t *testing.T) { + hash := gitutil.Rev("6f1d3a2b4c5d6e7f8091a2b3c4d5e6f708192a3b") + sha256Hash := gitutil.Rev(strings.Repeat("6f1d3a2b", 8)) + withPrefix := func(rev gitutil.Rev) gitutil.ArchiveParams { + return gitutil.ArchiveParams{Rev: rev, Format: gitutil.ArchiveTarGz, Prefix: "squid-main"} + } + + for _, tc := range []struct { + name string + request archiveRequest + resolved gitutil.Rev + want string + }{ + {"a commit and an explicit prefix in the URL earn a year", archiveRequest{params: withPrefix(hash)}, hash, immutableArchiveCache}, + {"a sha256 URL addresses a commit 64 characters wide", archiveRequest{params: withPrefix(sha256Hash)}, sha256Hash, immutableArchiveCache}, + {"a defaulted prefix comes from the repo name, which a rename changes", archiveRequest{params: gitutil.ArchiveParams{Rev: hash, Format: gitutil.ArchiveTarGz}}, hash, "no-cache"}, + {"a URL with a branch in it doesn't address one commit, since the branch moves", archiveRequest{params: withPrefix("main")}, hash, "no-cache"}, + {"a knot that doesn't advertise an immutable link leaves the commit unresolved", archiveRequest{params: withPrefix(hash)}, "", "no-cache"}, + {"a knot echoing a branch back as immutable doesn't earn that URL a year", archiveRequest{params: withPrefix("main")}, "main", "no-cache"}, + } { + got := http.Header{} + setArchiveCache(got, tc.request, tc.resolved) + if control, vary := got.Get("Cache-Control"), got.Values("Vary"); control != tc.want || len(vary) > 0 { + t.Errorf("%s: Cache-Control = %q varying on %v, want %q without a Vary", tc.name, control, vary, tc.want) + } + } + + guessed := http.Header{"Vary": {"Accept-Encoding"}} + setArchiveCache(guessed, archiveRequest{params: withPrefix(hash), guessed: true}, hash) + if want := []string{"Accept-Encoding", "User-Agent"}; guessed.Get("Cache-Control") != "no-cache" || !slices.Equal(guessed.Values("Vary"), want) { + t.Errorf("a user-agent guess gave %q varying on %v, want no-cache and %v, since a guessed format varies the bytes with the user agent", + guessed.Get("Cache-Control"), guessed.Values("Vary"), want) + } +} diff --git a/gitutil/archive.go b/gitutil/archive.go index dd30ab32..1bade909 100644 --- a/gitutil/archive.go +++ b/gitutil/archive.go @@ -2,14 +2,16 @@ package gitutil import ( "bytes" + "cmp" "context" + "crypto/sha256" + "encoding/hex" "fmt" "io" "mime" "net/http" "net/url" "os/exec" - "path" "slices" "strings" "syscall" @@ -64,53 +66,48 @@ func ParseRev(raw string) (Rev, error) { func RevFromHash(h plumbing.Hash) Rev { return Rev(h.String()) } +const sha1HexLen, sha256HexLen, hexDigits = 40, 64, "0123456789abcdef" + +func (r Rev) IsObjectID() bool { + return slices.Contains([]int{sha1HexLen, sha256HexLen}, len(r)) && + strings.TrimLeft(string(r), hexDigits) == "" +} + func (r Rev) String() string { return string(r) } func (r Rev) Slug() string { return pathSeparators.Replace(plumbing.ReferenceName(r).Short()) } func (r Rev) Or(fallback Rev) Rev { return lo.Ternary(r == "", fallback, r) } -func (r Rev) OrHash(h plumbing.Hash) Rev { return r.Or(RevFromHash(h)) } - type ArchivePrefix string const MaxArchivePrefixLen = 255 func ParseArchivePrefix(raw string) (ArchivePrefix, error) { switch { - case len(raw) > MaxArchivePrefixLen: - return "", fmt.Errorf("prefix is %d bytes, over the %d byte limit", len(raw), MaxArchivePrefixLen) case strings.ContainsFunc(raw, unicode.IsControl): return "", fmt.Errorf("prefix contains a control character: %q", raw) case strings.Contains(raw, `\`): return "", fmt.Errorf("prefix contains a backslash: %q", raw) } - trimmed := strings.Trim(raw, "/") - if trimmed == "" { - return "", nil - } - cleaned := path.Clean(trimmed) - if cleaned == "." || cleaned == ".." || strings.HasPrefix(cleaned, "../") { + components := lo.Filter(strings.Split(raw, "/"), func(component string, _ int) bool { + return component != "" && component != "." + }) + if slices.Contains(components, "..") { return "", fmt.Errorf("prefix escapes the archive root: %q", raw) } - return ArchivePrefix(cleaned), nil + prefix := ArchivePrefix(strings.Join(components, "/")) + if len(prefix) > MaxArchivePrefixLen { + return "", fmt.Errorf("prefix is %d bytes, over the %d byte limit", len(prefix), MaxArchivePrefixLen) + } + return prefix, nil } func (p ArchivePrefix) String() string { return string(p) } -func (p ArchivePrefix) OrDefault(repo RepoName, rev Rev) ArchivePrefix { - if p == "" { - return ArchivePrefix(archiveStem(repo, rev)) - } - return p -} - -func archiveStem(repo RepoName, rev Rev) string { - stem := pathSeparators.Replace(string(repo)) + "-" + rev.Slug() - if len(stem) <= MaxArchivePrefixLen { - return stem - } - return strings.ToValidUTF8(stem[:MaxArchivePrefixLen], "") +func archiveStem(repo RepoName, rev Rev) ArchivePrefix { + stem := strings.ToValidUTF8(pathSeparators.Replace(string(repo))+"-"+rev.Slug(), "�") + return ArchivePrefix(strings.ToValidUTF8(stem[:min(len(stem), MaxArchivePrefixLen)], "")) } type ArchiveParams struct { @@ -143,23 +140,81 @@ func (p ArchiveParams) WithRev(rev Rev) ArchiveParams { return p } -func (p ArchiveParams) Query(repo string) url.Values { +type ServedArchive struct { + rev Rev + format ArchiveFormat + prefix ArchivePrefix +} + +func (p ArchiveParams) Serve(repo RepoName) ServedArchive { + return ServedArchive{p.Rev, p.Format, cmp.Or(p.Prefix, archiveStem(repo, p.Rev))} +} + +func (a ServedArchive) WithRev(rev Rev) ServedArchive { + a.rev = rev + return a +} + +func (a ServedArchive) Query(repo string) url.Values { return url.Values{ "repo": {repo}, - "ref": {p.Rev.String()}, - "format": {p.Format.String()}, - "prefix": {p.Prefix.String()}, + "ref": {a.rev.String()}, + "format": {a.format.String()}, + "prefix": {a.prefix.String()}, } } -func (p ArchiveParams) SetHeaders(h http.Header, repo RepoName) { - h.Set("Content-Type", p.Format.contentType()) +func (a ServedArchive) SuffixURL() string { + return fmt.Sprintf("%s.%s?%s", url.PathEscape(a.rev.String()), a.format, + url.Values{"prefix": {a.prefix.String()}}.Encode()) +} + +func (a ServedArchive) SetHeaders(h http.Header) { + h.Set("Content-Type", a.format.contentType()) h.Set("Content-Disposition", mime.FormatMediaType("attachment", map[string]string{ - "filename": archiveStem(repo, p.Rev) + "." + p.Format.String(), + "filename": pathSeparators.Replace(a.prefix.String()) + "." + a.format.String(), })) h.Set("X-Content-Type-Options", "nosniff") } +const archiveETagDomain = "gitutil.archive.v1" + +type RepoIdentity string + +func (a ServedArchive) ETag(repo RepoIdentity) string { + digest := sha256.Sum256([]byte(strings.Join( + []string{archiveETagDomain, string(repo), a.rev.String(), a.format.String(), a.prefix.String()}, "\x00", + ))) + return `"` + hex.EncodeToString(digest[:]) + `"` +} + +func (a ServedArchive) ServeNotModified(w http.ResponseWriter, r *http.Request, repo RepoIdentity) bool { + etag := a.ETag(repo) + w.Header().Set("Etag", etag) + w.Header().Set("Cache-Control", "no-cache") + if !ETagMatches(r.Header, etag) { + return false + } + w.WriteHeader(http.StatusNotModified) + return true +} + +func ETagMatches(h http.Header, etag string) bool { + offered := strings.Split(strings.Join(h.Values("If-None-Match"), ","), ",") + return lo.SomeBy(offered, func(candidate string) bool { + trimmed := strings.TrimSpace(candidate) + return trimmed == "*" || strings.TrimPrefix(trimmed, "W/") == etag + }) +} + +func ForwardHeaders(dst, src http.Header, keys ...string) { + lo.ForEach(keys, func(key string, _ int) { + if values := src.Values(key); len(values) > 0 { + dst[http.CanonicalHeaderKey(key)] = slices.Clone(values) + } + }) +} + func ImmutableLink(target string) string { return `<` + target + `>; rel="immutable"` } func ParseImmutableLink(header string) (Rev, error) { @@ -171,17 +226,41 @@ func ParseImmutableLink(header string) (Rev, error) { return ParseRev(parsed.Query().Get("ref")) } -const archiveWaitDelay = 10 * time.Second +type ResponseBody struct { + inner http.ResponseWriter + started bool +} -func WriteArchive(ctx context.Context, w io.Writer, repoPath string, rev Rev, format ArchiveFormat, prefix ArchivePrefix) error { - args := []string{"archive", "--format=" + format.String()} - if prefix != "" { - args = append(args, "--prefix="+prefix.String()+"/") +func NewResponseBody(w http.ResponseWriter) *ResponseBody { return &ResponseBody{inner: w} } + +func (b *ResponseBody) Write(p []byte) (int, error) { + n, err := b.inner.Write(p) + b.started = b.started || n > 0 + return n, err +} + +func (b *ResponseBody) Fail() { + if b.started { + panic(http.ErrAbortHandler) } + header := b.inner.Header() + lo.ForEach([]string{"Content-Length", "Content-Type", "Content-Disposition", "Etag", "Link"}, + func(key string, _ int) { header.Del(key) }) + header.Set("Cache-Control", "no-store") + b.inner.WriteHeader(http.StatusInternalServerError) +} + +const archiveWaitDelay = 10 * time.Second - cmd := exec.CommandContext(ctx, "git", append(args, "--", rev.String())...) +func WriteArchive(ctx context.Context, w io.Writer, repoPath string, archive ServedArchive) error { + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + cmd := exec.CommandContext(ctx, "git", "archive", + "--format="+archive.format.String(), + "--prefix="+archive.prefix.String()+"/", + "--", archive.rev.String()) cmd.Dir = repoPath - cmd.Stdout = w stderr := new(bytes.Buffer) cmd.Stderr = stderr cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} @@ -191,7 +270,19 @@ func WriteArchive(ctx context.Context, w io.Writer, repoPath string, rev Rev, fo return lo.Ternary(err == syscall.ESRCH, nil, err) } - if err := cmd.Run(); err != nil { + stdout, err := cmd.StdoutPipe() + if err != nil { + return err + } + if err := cmd.Start(); err != nil { + return err + } + if _, err := io.Copy(w, stdout); err != nil { + cancel() + _ = cmd.Wait() + return fmt.Errorf("writing the archive body: %w", err) + } + if err := cmd.Wait(); err != nil { return fmt.Errorf("%w, stderr: %s", err, stderr.String()) } return nil diff --git a/gitutil/archive_test.go b/gitutil/archive_test.go index c5cda8e3..2500aca5 100644 --- a/gitutil/archive_test.go +++ b/gitutil/archive_test.go @@ -4,8 +4,10 @@ import ( "archive/zip" "bytes" "context" + "errors" "mime" "net/http" + "net/http/httptest" "net/url" "os" "os/exec" @@ -40,10 +42,10 @@ func TestParseArchiveParams(t *testing.T) { "every param", url.Values{"ref": {"refs/tags/v1.0.0"}, "format": {"zip"}, "prefix": {"/kelp/"}}, "", ArchiveParams{Rev: "refs/tags/v1.0.0", Format: ArchiveZip, Prefix: "kelp"}, - "kelp", "squid-v1.0.0.zip", "application/zip", "", + "kelp", "kelp.zip", "application/zip", "", }, - {"branch", url.Values{"ref": {"main"}}, "", ArchiveParams{Rev: "main", Format: ArchiveTarGz}, "squid-main", "squid-main.tar.gz", "application/gzip", ""}, + {"nested prefix in the filename", url.Values{"ref": {"main"}, "prefix": {"kelp/uni"}}, "", ArchiveParams{Rev: "main", Format: ArchiveTarGz, Prefix: "kelp/uni"}, "kelp/uni", "kelp-uni.tar.gz", "application/gzip", ""}, {"full ref", url.Values{"ref": {"refs/heads/feat/uni"}}, "", ArchiveParams{Rev: "refs/heads/feat/uni", Format: ArchiveTarGz}, "squid-feat-uni", "squid-feat-uni.tar.gz", "application/gzip", ""}, {"head", url.Values{"ref": {"HEAD"}}, "", ArchiveParams{Rev: "HEAD", Format: ArchiveTarGz}, "squid-HEAD", "", "", ""}, {"trailing slash", url.Values{"ref": {"refs/heads/main/"}}, "", ArchiveParams{Rev: "refs/heads/main/", Format: ArchiveTarGz}, "squid-main-", "", "", ""}, @@ -57,16 +59,17 @@ func TestParseArchiveParams(t *testing.T) { {"did prefix", url.Values{"prefix": {"did:plc:boltless"}}, "", ArchiveParams{Format: ArchiveTarGz, Prefix: "did:plc:boltless"}, "", "", "", ""}, {"nested prefix", url.Values{"prefix": {"squid/main"}}, "", ArchiveParams{Format: ArchiveTarGz, Prefix: "squid/main"}, "", "", "", ""}, {"prefix wrapped in slashes", url.Values{"prefix": {"/squid/main/"}}, "", ArchiveParams{Format: ArchiveTarGz, Prefix: "squid/main"}, "", "", "", ""}, - {"redundant prefix segments", url.Values{"prefix": {"squid/../limpet"}}, "", ArchiveParams{Format: ArchiveTarGz, Prefix: "limpet"}, "", "", "", ""}, {"space in a prefix", url.Values{"prefix": {"squid main"}}, "", ArchiveParams{Format: ArchiveTarGz, Prefix: "squid main"}, "", "", "", ""}, + {"bare dot prefix", url.Values{"prefix": {"."}}, "", ArchiveParams{Format: ArchiveTarGz}, "", "", "", ""}, + {"repeated separators and dot segments", url.Values{"prefix": {"/kelp//./uni/"}}, "", ArchiveParams{Format: ArchiveTarGz, Prefix: "kelp/uni"}, "", "", "", ""}, {"unsupported format", url.Values{"format": {"tar"}}, "", ArchiveParams{}, "", "", "", "only tar.gz and zip formats are supported"}, {"space in a ref", url.Values{"ref": {"refs/tags/a b"}}, "", ArchiveParams{}, "", "", "", "ref contains whitespace"}, {"control character in a ref", url.Values{"ref": {"refs/tags/a\nb"}}, "", ArchiveParams{}, "", "", "", "ref contains whitespace"}, {"ref that git would read as an option", url.Values{"ref": {"--output=/tmp/evil"}}, "", ArchiveParams{}, "", "", "", "ref starts with a dash"}, {"prefix escaping the root", url.Values{"prefix": {"../../evil"}}, "", ArchiveParams{}, "", "", "", "prefix escapes the archive root"}, - {"prefix escaping after cleaning", url.Values{"prefix": {"squid/../../evil"}}, "", ArchiveParams{}, "", "", "", "prefix escapes the archive root"}, - {"bare dot prefix", url.Values{"prefix": {"."}}, "", ArchiveParams{}, "", "", "", "prefix escapes the archive root"}, + {"prefix escaping below the root", url.Values{"prefix": {"squid/../../evil"}}, "", ArchiveParams{}, "", "", "", "prefix escapes the archive root"}, + {"a dot-dot segment the knot would reject", url.Values{"prefix": {"squid/../limpet"}}, "", ArchiveParams{}, "", "", "", "prefix escapes the archive root"}, {"control character in a prefix", url.Values{"prefix": {"squid\nmain"}}, "", ArchiveParams{}, "", "", "", "prefix contains a control character"}, {"windows separator in a prefix", url.Values{"prefix": {`..\..\evil`}}, "", ArchiveParams{}, "", "", "", "prefix contains a backslash"}, {"prefix over the length limit", url.Values{"prefix": {strings.Repeat("a", MaxArchivePrefixLen+1)}}, "", ArchiveParams{}, "", "", "", "over the 255 byte limit"}, @@ -87,12 +90,14 @@ func TestParseArchiveParams(t *testing.T) { if tc.repo != "" { repo = tc.repo } - if stem := got.Prefix.OrDefault(repo, got.Rev); tc.wantStem != "" && stem != tc.wantStem { - t.Errorf("default prefix = %q, want %q", stem, tc.wantStem) + served := got.Serve(repo) + query := served.Query("did:plc:limpet") + if stem := ArchivePrefix(query.Get("prefix")); tc.wantStem != "" && stem != tc.wantStem { + t.Errorf("served prefix = %q, want %q", stem, tc.wantStem) } if tc.wantFilename != "" { header := http.Header{} - got.SetHeaders(header, repo) + served.SetHeaders(header) mediatype, fields, err := mime.ParseMediaType(header.Get("Content-Disposition")) if err != nil || mediatype != "attachment" || fields["filename"] != tc.wantFilename { t.Errorf("Content-Disposition = %q (err %v), want an attachment with filename %q", header.Get("Content-Disposition"), err, tc.wantFilename) @@ -102,9 +107,8 @@ func TestParseArchiveParams(t *testing.T) { } } - query := got.Query("did:plc:limpet") - if back, err := ParseArchiveParams(query); err != nil || back != got { - t.Errorf("query round trip = %+v (err %v), want %+v", back, err, got) + if back, err := ParseArchiveParams(query); err != nil || back.Serve(repo) != served { + t.Errorf("query round trip = %+v (err %v), want the served archive %+v", back, err, served) } back, err := ParseImmutableLink(ImmutableLink(testArchiveEndpoint + "?" + query.Encode())) if got.Rev != "" && (err != nil || back != got.Rev) { @@ -115,9 +119,9 @@ func TestParseArchiveParams(t *testing.T) { } func TestArchiveFallbacks(t *testing.T) { - hash := plumbing.NewHash("6f1d3a2b4c5d6e7f8091a2b3c4d5e6f708192a3b") - if kept, filled := Rev("refs/heads/main").OrHash(hash), Rev("").OrHash(hash); kept != "refs/heads/main" || filled != RevFromHash(hash) { - t.Errorf("OrHash kept %q and filled %q, want refs/heads/main and the hash %q", kept, filled, hash) + hash := RevFromHash(plumbing.NewHash("6f1d3a2b4c5d6e7f8091a2b3c4d5e6f708192a3b")) + if kept, filled := Rev("refs/heads/main").Or(hash), Rev("").Or(hash); kept != "refs/heads/main" || filled != hash { + t.Errorf("Or kept %q and filled %q, want refs/heads/main and the hash %q", kept, filled, hash) } if got := Rev("").Or(RevHead); got != RevHead { t.Errorf("empty rev = %q, want HEAD", got) @@ -131,9 +135,92 @@ func TestArchiveFallbacks(t *testing.T) { t.Errorf("WithRev = %+v leaving the receiver at %q, want only the rev replaced", got, params.Rev) } - long := ArchivePrefix("").OrDefault("squid", Rev("refs/heads/"+strings.Repeat("ü", 400))) - if _, err := ParseArchivePrefix(long.String()); len(long) > MaxArchivePrefixLen || !utf8.ValidString(long.String()) || err != nil { - t.Errorf("default prefix is %d bytes %q (err %v), want at most %d bytes ending on a rune boundary", len(long), long, err, MaxArchivePrefixLen) + served := params.Serve("squid") + if got := served.WithRev("6f1d3a2"); got != (ArchiveParams{Rev: "6f1d3a2", Format: ArchiveZip, Prefix: "kelp"}).Serve("squid") || served.rev != "main" { + t.Errorf("WithRev = %+v leaving the receiver at %q, want only the rev replaced", got, served.rev) + } + + for _, stem := range []ArchivePrefix{ + archiveStem("squid", Rev("refs/heads/"+strings.Repeat("ü", 400))), + archiveStem(RepoName(strings.Repeat("\xff", 300)), "main"), + } { + _, err := ParseArchivePrefix(stem.String()) + if stem == "" || len(stem) > MaxArchivePrefixLen || !utf8.ValidString(stem.String()) || err != nil { + t.Errorf("default prefix is %d bytes %q (err %v), want a non-empty valid prefix within %d bytes ending on a rune boundary", + len(stem), stem, err, MaxArchivePrefixLen) + } + } +} + +func TestRevIsObjectID(t *testing.T) { + sha1Hex, sha256Hex := "6f1d3a2b4c5d6e7f8091a2b3c4d5e6f708192a3b", strings.Repeat("6f1d3a2b", 8) + for rev, want := range map[Rev]bool{ + Rev(sha1Hex): true, Rev(sha256Hex): true, + "": false, "main": false, "refs/heads/main": false, "6f1d3a2": false, RevHead: false, + Rev(strings.ToUpper(sha1Hex)): false, Rev(sha1Hex + "b"): false, + Rev(sha1Hex[:39] + "g"): false, Rev(strings.Repeat("6", 48)): false, + } { + assert.Equal(t, want, rev.IsObjectID(), "%q: only 40 or 64 characters of lowercase hex identify one commit forever", rev) + } +} + +func TestArchiveResponseHeaders(t *testing.T) { + upstream := http.Header{"Etag": {`"6f1d3a2b"`}, "Content-Length": {"4096"}, "Cache-Control": {"no-cache"}} + + got := http.Header{"Etag": {`"limpet"`, `"conch"`}} + ForwardHeaders(got, upstream, "Etag", "Content-Length", "Link") + assert.Equal(t, http.Header{"Etag": {`"6f1d3a2b"`}, "Content-Length": {"4096"}}, got, + "ForwardHeaders overwrites Etag, leaves the unlisted Cache-Control alone, and skips the Link that the knot never sent") + + revalidation := http.Header{} + ForwardHeaders(revalidation, upstream, "Etag") + assert.Equal(t, http.Header{"Etag": {`"6f1d3a2b"`}}, revalidation, "the 304 path forwards the validator without a length") + + validators := http.Header{} + ForwardHeaders(validators, http.Header{"If-None-Match": {`"kelp"`, `"uni"`}}, "if-none-match") + assert.Equal(t, []string{`"kelp"`, `"uni"`}, validators.Values("If-None-Match"), + "ForwardHeaders canonicalizes a lowercase key and sends every validator that a client offered") + + untouched := httptest.NewRecorder() + ArchiveParams{Rev: "main", Format: ArchiveTarGz}.Serve("squid").SetHeaders(untouched.Header()) + untouched.Header().Set("Content-Length", "4096") + untouched.Header().Set("Etag", `"6f1d3a2b"`) + untouched.Header().Set("Link", ImmutableLink(testArchiveEndpoint+"?ref=6f1d3a2b")) + untouched.Header().Set("Cache-Control", "public, max-age=31536000") + NewResponseBody(untouched).Fail() + assert.Equal(t, http.StatusInternalServerError, untouched.Code) + assert.Equal(t, http.Header{"Cache-Control": {"no-store"}, "X-Content-Type-Options": {"nosniff"}}, untouched.Header(), + "Fail deletes the length, the type, the filename, the validator and the link, because a 500 doesn't describe the archive they came from") + + body := NewResponseBody(httptest.NewRecorder()) + written, err := body.Write([]byte("PK\x03\x04")) + require.NoError(t, err) + assert.Equal(t, 4, written) + assert.PanicsWithError(t, http.ErrAbortHandler.Error(), body.Fail, + "a truncated body has to abort the connection, since a clean return reads as a complete archive") + + archive := ArchiveParams{Rev: "6f1d3a2b", Format: ArchiveTarGz}.Serve("squid") + etag := archive.ETag("did:plc:limpet") + assert.Regexp(t, `^"[0-9a-f]{64}"$`, etag) + assert.NotContains(t, []string{ + archive.ETag("did:plc:conch"), + archive.WithRev("main").ETag("did:plc:limpet"), + ArchiveParams{Rev: "6f1d3a2b", Format: ArchiveZip}.Serve("squid").ETag("did:plc:limpet"), + ArchiveParams{Rev: "6f1d3a2b", Format: ArchiveTarGz}.Serve("kelp").ETag("did:plc:limpet"), + }, etag, "a repo, rev, format or prefix that shapes different bytes gets a different validator") + + for offered, want := range map[string]bool{ + etag: true, "W/" + etag: true, "*": true, `"conch", ` + etag: true, `"conch"`: false, "": false, + } { + request := httptest.NewRequest(http.MethodGet, testArchiveEndpoint, nil) + if offered != "" { + request.Header.Set("If-None-Match", offered) + } + recorder := httptest.NewRecorder() + assert.Equal(t, want, archive.ServeNotModified(recorder, request, "did:plc:limpet"), "If-None-Match %q", offered) + assert.Equal(t, lo.Ternary(want, http.StatusNotModified, http.StatusOK), recorder.Code) + assert.Equal(t, etag, recorder.Header().Get("Etag"), "the knot sets the validator on every answer, so a client with a stale copy learns it too") + assert.Equal(t, "no-cache", recorder.Header().Get("Cache-Control")) } } @@ -143,7 +230,8 @@ func TestWriteArchive(t *testing.T) { for _, args := range [][]string{ {"init", "-q", "-b", "main"}, {"add", "README.md"}, - {"-c", "user.name=nel", "-c", "user.email=nel@nel.pet", "commit", "-qm", "Initial commit"}, + {"-c", "user.name=nel", "-c", "user.email=noreply@nel.pet", "commit", "-qm", "Initial commit"}, + {"branch", "feat/uni"}, } { cmd := exec.Command("git", args...) cmd.Dir = repoPath @@ -153,26 +241,23 @@ func TestWriteArchive(t *testing.T) { canceled, cancel := context.WithCancel(context.Background()) cancel() + head := ArchiveParams{Rev: RevHead, Format: ArchiveZip} cases := []struct { - name string - ctx context.Context - prefix ArchivePrefix - want []string + name string + ctx context.Context + archive ServedArchive + want []string }{ - { - "prefix on every entry", - context.Background(), - ArchivePrefix("").OrDefault("squid", "refs/heads/feat/uni"), - []string{"squid-feat-uni/", "squid-feat-uni/README.md"}, - }, - {"empty prefix", context.Background(), "", []string{"README.md"}}, - {"canceled context", canceled, "squid-main", nil}, + {"the defaulted prefix begins every entry", context.Background(), ArchiveParams{Rev: "refs/heads/feat/uni", Format: ArchiveZip}.Serve("squid"), []string{"squid-feat-uni/", "squid-feat-uni/README.md"}}, + {"a requested prefix replaces the default", context.Background(), ArchiveParams{Rev: RevHead, Format: ArchiveZip, Prefix: "kelp"}.Serve("squid"), []string{"kelp/", "kelp/README.md"}}, + {"canceled context", canceled, head.Serve("squid"), nil}, + {"a ref the repo doesn't have", context.Background(), ArchiveParams{Rev: "refs/heads/limpet", Format: ArchiveZip}.Serve("squid"), nil}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { var body bytes.Buffer - err := WriteArchive(tc.ctx, &body, repoPath, RevHead, ArchiveZip, tc.prefix) + err := WriteArchive(tc.ctx, &body, repoPath, tc.archive) if tc.want == nil { assert.Error(t, err) return @@ -184,4 +269,13 @@ func TestWriteArchive(t *testing.T) { assert.Equal(t, tc.want, lo.Map(entries.File, func(f *zip.File, _ int) string { return f.Name })) }) } + + err := WriteArchive(context.Background(), refusingWriter{}, repoPath, head.Serve("squid")) + assert.ErrorIs(t, err, errClientGone, "WriteArchive returns the write error itself") } + +var errClientGone = errors.New("the client stopped reading") + +type refusingWriter struct{} + +func (refusingWriter) Write(p []byte) (int, error) { return 0, errClientGone } diff --git a/knot2/crates/knot-git/src/archive.rs b/knot2/crates/knot-git/src/archive.rs index 14b41bac..0dd218df 100644 --- a/knot2/crates/knot-git/src/archive.rs +++ b/knot2/crates/knot-git/src/archive.rs @@ -41,22 +41,55 @@ impl ArchiveFormat { } } +fn inside_root(value: String, kind: &'static str, limit: usize) -> Result { + let safe = value.len() <= limit + && !value.contains('\0') + && !value.starts_with(['/', '\\']) + && value.split(['/', '\\']).all(|component| component != ".."); + match safe { + true => Ok(value), + false => Err(ParseError::Invalid { kind, value }), + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct ArchivePrefix(String); impl ArchivePrefix { + pub const MAX_BYTES: usize = 255; + pub fn new(value: impl Into) -> Result { - let value = value.into(); - let safe = !value.contains('\0') - && !value.starts_with(['/', '\\']) - && value.split(['/', '\\']).all(|component| component != ".."); - match safe { - true => Ok(Self(value)), - false => Err(ParseError::Invalid { - kind: "archive prefix", - value, - }), - } + inside_root(value.into(), "archive prefix", Self::MAX_BYTES).map(Self) + } + + pub fn stem(repo_name: &str, safe_ref: &str) -> Self { + let joined = format!("{repo_name}-{safe_ref}").replace(['/', '\\', '\0'], "-"); + Self( + joined + .char_indices() + .take_while(|(offset, character)| offset + character.len_utf8() <= Self::MAX_BYTES) + .map(|(_, character)| character) + .collect(), + ) + } + + pub fn as_str(&self) -> &str { + &self.0 + } + + pub fn into_tree_prefix(self) -> TreePrefix { + TreePrefix(format!("{}/", self.0)) + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct TreePrefix(String); + +impl TreePrefix { + pub const MAX_BYTES: usize = ArchivePrefix::MAX_BYTES + 1; + + pub fn new(value: impl Into) -> Result { + inside_root(value.into(), "archive tree prefix", Self::MAX_BYTES).map(Self) } pub fn as_str(&self) -> &str { @@ -78,7 +111,7 @@ impl Repo { &self, tree: Oid, format: ArchiveFormat, - prefix: Option<&ArchivePrefix>, + prefix: Option<&TreePrefix>, limit: ArchiveLimit, out: impl std::io::Write + std::io::Seek, ) -> Result<(), GitError> { @@ -185,7 +218,7 @@ impl Seek for BoundedSpool { #[cfg(test)] mod tests { - use super::{ArchiveLimit, ArchivePrefix, BoundedSpool}; + use super::{ArchiveLimit, ArchivePrefix, BoundedSpool, TreePrefix}; use std::io::{Seek, SeekFrom, Write}; fn spool(limit: u64) -> BoundedSpool>> { @@ -250,6 +283,10 @@ mod tests { ArchivePrefix::new("dotted-..-name/").is_ok(), "a component that merely contains dot-dot is not a traversal" ); + assert!( + TreePrefix::new("dotted-..-name//").is_ok(), + "TreePrefix accepts the empty final component that a trailing separator adds" + ); } #[test] @@ -258,4 +295,38 @@ mod tests { assert!(ArchivePrefix::new("\\windows").is_err()); assert!(ArchivePrefix::new("good\0bad").is_err()); } + + #[test] + fn a_prefix_is_bounded_and_a_stem_always_parses() { + let longest = "u".repeat(ArchivePrefix::MAX_BYTES); + assert!(ArchivePrefix::new(format!("{longest}u")).is_err()); + + let tree = ArchivePrefix::new(longest).unwrap().into_tree_prefix(); + assert_eq!(tree.as_str().len(), TreePrefix::MAX_BYTES); + assert!(TreePrefix::new(tree.as_str()).is_ok()); + assert!(TreePrefix::new("u".repeat(TreePrefix::MAX_BYTES + 1)).is_err()); + + let (long_name, truncated) = ("ü".repeat(400), "ü".repeat(127)); + for (repo_name, safe_ref, want) in [ + ("squid", "main", "squid-main"), + ("did:plc:limpet", "feat/uni", "did:plc:limpet-feat-uni"), + ("kelp/../conch", "..", "kelp-..-conch-.."), + ("/etc", "\0", "-etc--"), + ("", "", "-"), + (long_name.as_str(), "main", truncated.as_str()), + ] { + let stem = ArchivePrefix::stem(repo_name, safe_ref); + assert_eq!(stem.as_str(), want); + assert!( + ArchivePrefix::new(stem.as_str()).is_ok(), + "a stem passes the check that its own constructor skips, within {} bytes and on a char boundary", + ArchivePrefix::MAX_BYTES + ); + assert_eq!( + stem.into_tree_prefix().as_str(), + format!("{want}/"), + "into_tree_prefix appends the separator that puts a stem at the archive root" + ); + } + } } diff --git a/knot2/crates/knot-git/src/lib.rs b/knot2/crates/knot-git/src/lib.rs index 0ef29958..c27746a9 100644 --- a/knot2/crates/knot-git/src/lib.rs +++ b/knot2/crates/knot-git/src/lib.rs @@ -12,7 +12,7 @@ mod reads; mod repo; mod staging; -pub use archive::{ArchiveFormat, ArchiveLimit, ArchivePrefix}; +pub use archive::{ArchiveFormat, ArchiveLimit, ArchivePrefix, TreePrefix}; pub use bitmap::{reachable_via_bitmap, verbatim_clone_pack, write_bitmap, write_midx_bitmap}; pub use error::{GitError, SelectionLimit}; pub use maintenance::{PackRefsReport, ReflogReport}; diff --git a/knot2/crates/knot-git/tests/reads.rs b/knot2/crates/knot-git/tests/reads.rs index fba900d2..9c0ce920 100644 --- a/knot2/crates/knot-git/tests/reads.rs +++ b/knot2/crates/knot-git/tests/reads.rs @@ -627,7 +627,7 @@ fn archives_round_trip_through_tar() { bare.write_archive( tree, knot_git::ArchiveFormat::TarGz, - Some(&knot_git::ArchivePrefix::new("squid-main/").unwrap()), + Some(&knot_git::TreePrefix::new("squid-main/").unwrap()), knot_git::ArchiveLimit::new(u64::MAX), &mut out, ) diff --git a/knot2/crates/knot-pack/src/archive.rs b/knot2/crates/knot-pack/src/archive.rs index 6b8e6188..8a297102 100644 --- a/knot2/crates/knot-pack/src/archive.rs +++ b/knot2/crates/knot-pack/src/archive.rs @@ -1,6 +1,6 @@ use std::io::{self, Read, Seek, SeekFrom}; -use knot_git::{ArchiveFormat, ArchiveLimit, ArchivePrefix, Repo}; +use knot_git::{ArchiveFormat, ArchiveLimit, Repo, TreePrefix}; use knot_types::Oid; use crate::error::PackError; @@ -9,7 +9,7 @@ use crate::pkt; struct Request { treeish: String, format: ArchiveFormat, - prefix: Option, + prefix: Option, } pub fn stream( @@ -85,8 +85,11 @@ fn interpret(args: &[String]) -> Result { .iter() .find_map(|arg| arg.strip_prefix("--prefix=")) .map(|raw| { - ArchivePrefix::new(raw).map_err(|_| { - PackError::Protocol("archive prefix must not escape archive root".to_string()) + TreePrefix::new(raw).map_err(|_| { + PackError::Protocol(format!( + "archive prefix mustn't escape archive root or exceed {} bytes", + TreePrefix::MAX_BYTES + )) }) }) .transpose()?; diff --git a/knot2/crates/knot-xrpc/src/reads.rs b/knot2/crates/knot-xrpc/src/reads.rs index 9f8c75fc..9f899efc 100644 --- a/knot2/crates/knot-xrpc/src/reads.rs +++ b/knot2/crates/knot-xrpc/src/reads.rs @@ -1133,11 +1133,25 @@ struct ArchivePrefixArg(Option); impl<'de> Deserialize<'de> for ArchivePrefixArg { fn deserialize>(deserializer: D) -> Result { let raw = String::deserialize(deserializer)?; - match raw.is_empty() { + let cleaned = raw + .split('/') + .filter(|component| !component.is_empty() && *component != ".") + .collect::>() + .join("/"); + match cleaned.is_empty() { true => Ok(Self(None)), - false => knot_git::ArchivePrefix::new(raw) + false => knot_git::ArchivePrefix::new(cleaned) + .ok() + .filter(|prefix| { + !prefix.as_str().contains('\\') && !prefix.as_str().contains(char::is_control) + }) .map(|prefix| Self(Some(prefix))) - .map_err(|_| de::Error::custom("archive prefix mustn't escape archive root")), + .ok_or_else(|| { + de::Error::custom(format!( + "archive prefix must stay inside the archive root within {} bytes, and mustn't contain a backslash or a control character", + knot_git::ArchivePrefix::MAX_BYTES + )) + }), } } } @@ -1153,21 +1167,18 @@ pub(crate) struct ArchiveParams { prefix: ArchivePrefixArg, } -fn short_ref(refspec: &str) -> String { - refspec - .trim_start_matches("refs/heads/") - .trim_start_matches("refs/tags/") - .trim_start_matches("refs/remotes/") - .replace('/', "-") +fn short_ref(refspec: &str) -> &str { + ["refs/heads/", "refs/tags/", "refs/remotes/", "refs/"] + .into_iter() + .find_map(|prefix| refspec.strip_prefix(prefix)) + .unwrap_or(refspec) } fn sanitize_filename(name: &str) -> String { - name.chars() - .map(|c| match c.is_ascii_control() || matches!(c, '"' | '\\') { - true => '-', - false => c, - }) - .collect() + name.replace( + |c: char| c.is_ascii_control() || matches!(c, '"' | '\\' | '/'), + "-", + ) } fn rfc5987_encode(name: &str) -> String { @@ -1259,11 +1270,16 @@ pub(crate) async fn repo_archive( let did = resolve_repo(&state, ¶ms.repo)?; let format = params.format; let format_name = format.name(); - let repo_name = params.repo.basename().to_string(); - let safe_ref = short_ref(params.refspec.as_str()); let archive_prefix = match ¶ms.prefix.0 { - None => format!("{repo_name}-{safe_ref}"), - Some(prefix) => prefix.as_str().to_string(), + Some(prefix) => prefix.clone(), + None => { + let registered = state.index.rkey_of(&did); + let name = match ®istered { + Resolved::Ready(Some(rkey)) => rkey.as_str(), + _ => params.repo.basename(), + }; + knot_git::ArchivePrefix::stem(name, short_ref(params.refspec.as_str())) + } }; let (resolved, modified_secs) = run_blocking({ @@ -1282,12 +1298,27 @@ pub(crate) async fn repo_archive( }) .await?; - let etag = archive_etag(&did, resolved, format.format(), &archive_prefix); + let link = { + let mut query = url::form_urlencoded::Serializer::new(String::new()); + query.append_pair("format", format_name); + query.append_pair("prefix", archive_prefix.as_str()); + query.append_pair("ref", &resolved.to_hex()); + query.append_pair("repo", ¶ms.repo.to_param()); + format!( + "<{}/xrpc/sh.tangled.repo.archive?{}>; rel=\"immutable\"", + state.knot_service_url.as_str(), + query.finish() + ) + }; + + let etag = archive_etag(&did, resolved, format.format(), archive_prefix.as_str()); + let disposition = content_disposition(&format!("{}.{format_name}", archive_prefix.as_str())); if etag_matches(request.headers(), &etag) { return Ok(( StatusCode::NOT_MODIFIED, [ (header::ETAG, etag), + (header::LINK, link), (header::CACHE_CONTROL, "no-cache".to_string()), ], ) @@ -1298,8 +1329,7 @@ pub(crate) async fn repo_archive( let layout = state.layout.clone(); let did = did.clone(); let archive_limit = state.byte_limits.archive; - let tree_prefix = knot_git::ArchivePrefix::new(format!("{archive_prefix}/")) - .expect("validated prefix with trailing slash stays valid"); + let tree_prefix = archive_prefix.into_tree_prefix(); move || { let repo = open(&layout, &did)?; let tree = repo.peel_to_tree(resolved)?; @@ -1333,20 +1363,7 @@ pub(crate) async fn repo_archive( }) .await?; - let immutable = { - let mut query = url::form_urlencoded::Serializer::new(String::new()); - query.append_pair("format", format_name); - query.append_pair("prefix", &archive_prefix); - query.append_pair("ref", &resolved.to_hex()); - query.append_pair("repo", ¶ms.repo.to_param()); - format!( - "{}/xrpc/sh.tangled.repo.archive?{}", - state.knot_service_url.as_str(), - query.finish() - ) - }; let content_type = format.content_type(); - let disposition = content_disposition(&format!("{repo_name}-{safe_ref}.{format_name}")); reconcile_if_range(&mut request, &etag, modified_secs); let serve_response = ServeFile::new(temp.path()) @@ -1362,10 +1379,7 @@ pub(crate) async fn repo_archive( HeaderValue::from_str(value).map_err(|error| XrpcError::internal(error.to_string())) }; headers.insert(header::CONTENT_DISPOSITION, header_value(&disposition)?); - headers.insert( - header::LINK, - header_value(&format!("<{immutable}>; rel=\"immutable\""))?, - ); + headers.insert(header::LINK, header_value(&link)?); headers.insert(header::ETAG, header_value(&etag)?); headers.insert(header::CACHE_CONTROL, HeaderValue::from_static("no-cache")); Ok(response) diff --git a/knot2/crates/knot-xrpc/tests/common/mod.rs b/knot2/crates/knot-xrpc/tests/common/mod.rs index b9b21a03..e34abf7b 100644 --- a/knot2/crates/knot-xrpc/tests/common/mod.rs +++ b/knot2/crates/knot-xrpc/tests/common/mod.rs @@ -637,20 +637,26 @@ pub async fn archive_full(world: &World, did: &RepoDid) -> (String, String, Byte (etag, last_modified, body) } +fn immutable_target(headers: &HeaderMap) -> &str { + headers[header::LINK] + .to_str() + .unwrap() + .trim_start_matches('<') + .split('>') + .next() + .unwrap() + .strip_prefix(&format!("https://{KNOT_HOST}")) + .expect("the immutable link points at this knot") +} + pub async fn assert_immutable_round_trip( world: &World, headers: &HeaderMap, full: &Bytes, etag: &str, ) { - let link = headers.get(header::LINK).unwrap().to_str().unwrap(); - let immutable = link - .trim_start_matches('<') - .split('>') - .next() - .unwrap() - .strip_prefix(&format!("https://{KNOT_HOST}")) - .expect("the immutable link points at this knot"); + let immutable = immutable_target(headers); + let (status, immutable_headers, immutable_body) = get(world, immutable).await; assert_eq!(status, StatusCode::OK); assert_eq!( @@ -658,14 +664,31 @@ pub async fn assert_immutable_round_trip( "following the immutable link regenerates the very bytes it was attached to" ); assert_eq!( - immutable_headers - .get(header::ETAG) - .unwrap() - .to_str() - .unwrap(), + immutable_headers[header::ETAG], etag, "the immutable link shares the etag of the response that advertised it" ); + assert_eq!( + immutable_target(&immutable_headers), + immutable, + "the immutable response advertises the same link, so one commit has one archive URL" + ); + assert_eq!( + immutable_headers[header::CONTENT_DISPOSITION], + headers[header::CONTENT_DISPOSITION], + "the prefix in the URL shapes the filename on both responses" + ); + + let mut conditional = HeaderMap::new(); + conditional.insert(header::IF_NONE_MATCH, etag.parse().unwrap()); + let (status, revalidated, body) = get_with_headers(world, immutable, conditional).await; + assert_eq!(status, StatusCode::NOT_MODIFIED); + assert!(body.is_empty()); + assert_eq!( + immutable_target(&revalidated), + immutable, + "the 304 advertises the immutable link too, because a proxy that only revalidates never sees the 200" + ); } pub async fn assert_warming(world: &World, path: &str, expected_error: Option<&str>) { diff --git a/knot2/crates/knot-xrpc/tests/reads.rs b/knot2/crates/knot-xrpc/tests/reads.rs index d7885e7b..8ce7f394 100644 --- a/knot2/crates/knot-xrpc/tests/reads.rs +++ b/knot2/crates/knot-xrpc/tests/reads.rs @@ -521,7 +521,7 @@ async fn archive_conditional_and_range_semantics() { .unwrap() .to_str() .unwrap(), - format!("attachment; filename=\"{did}-main.tar.gz\"") + "attachment; filename=\"nautilus-main.tar.gz\"" ); let link = headers.get(header::LINK).unwrap().to_str().unwrap(); assert!(link.contains("rel=\"immutable\"")); @@ -706,6 +706,42 @@ async fn archive_etag_distinguishes_refs_that_share_a_commit() { ); } +#[tokio::test] +async fn archive_link_advertises_the_prefix_it_served() { + let world = World::new(); + let (did, _work) = seeded(&world, "periwinkle"); + let query = |suffix: &str| format!("/xrpc/sh.tangled.repo.archive?repo={did}&ref=main{suffix}"); + + for (suffix, prefix, filename) in [ + ("", "periwinkle-main", "periwinkle-main.tar.gz"), + ("&prefix=kelp/uni", "kelp%2Funi", "kelp-uni.tar.gz"), + ("&prefix=/kelp//./uni/", "kelp%2Funi", "kelp-uni.tar.gz"), + ] { + let (status, headers, full) = get(&world, &query(suffix)).await; + assert_eq!(status, StatusCode::OK); + let link = headers[header::LINK].to_str().unwrap(); + assert!( + link.contains(&format!("prefix={prefix}")), + "the link repeats the prefix that the knot served, percent-encoded, since a stem built from the resolved commit would differ: {suffix} gave {link}" + ); + assert_eq!( + headers[header::CONTENT_DISPOSITION], + format!("attachment; filename=\"{filename}\""), + "the filename follows the prefix with the separator flattened, and the knot spells a did-addressed repo with the rkey it registered under" + ); + let etag = headers[header::ETAG].to_str().unwrap().to_string(); + assert_immutable_round_trip(&world, &headers, &full, &etag).await; + } + + for prefix in ["u".repeat(256), "kelp%5Cuni".to_string()] { + assert_eq!( + get_error(&world, &query(&format!("&prefix={prefix}"))).await, + (StatusCode::BAD_REQUEST, "InvalidRequest".to_string()), + "prefix {prefix}" + ); + } +} + #[tokio::test] async fn archive_serves_a_sha256_repo_with_a_stable_etag() { let world = World::sha256(); @@ -1439,8 +1475,7 @@ async fn archive_rejects_traversal_prefixes_and_sanitizes_the_filename() { .to_str() .unwrap(); assert_eq!( - disposition, - format!("attachment; filename=\"{did}-a-b.tar.gz\""), + disposition, "attachment; filename=\"cockle-a-b.tar.gz\"", "quote in the ref name mustn't break the header quoting" ); } diff --git a/knotmirror/xrpc/git_get_archive.go b/knotmirror/xrpc/git_get_archive.go index 198c82a9..4c85b3c5 100644 --- a/knotmirror/xrpc/git_get_archive.go +++ b/knotmirror/xrpc/git_get_archive.go @@ -62,18 +62,20 @@ func (x *Xrpc) GetArchive(w http.ResponseWriter, r *http.Request) { return } - name := gitutil.RepoName(mirrored.Name) - resolvedRev := gitutil.RevFromHash(commit.Hash) - params.Rev = params.Rev.OrHash(commit.Hash) - params.Prefix = params.Prefix.OrDefault(name, params.Rev) + hash := gitutil.RevFromHash(commit.Hash) + served := params.WithRev(params.Rev.Or(hash)).Serve(gitutil.RepoName(mirrored.Name)).WithRev(hash) - params.SetHeaders(w.Header(), name) + served.SetHeaders(w.Header()) w.Header().Set("Link", gitutil.ImmutableLink(fmt.Sprintf("%s/xrpc/%s?%s", - x.cfg.BaseUrl(), tangled.GitTempGetArchiveNSID, params.WithRev(resolvedRev).Query(repo.String()).Encode(), + x.cfg.BaseUrl(), tangled.GitTempGetArchiveNSID, served.Query(repo.String()).Encode(), ))) + if served.ServeNotModified(w, r, gitutil.RepoIdentity(repo.String())) { + return + } - if err := gitutil.WriteArchive(ctx, w, repoPath, resolvedRev, params.Format, params.Prefix); err != nil { + body := gitutil.NewResponseBody(w) + if err := gitutil.WriteArchive(ctx, body, repoPath, served); err != nil { l.Error("writing archive", "err", err.Error(), "format", params.Format) - w.WriteHeader(http.StatusInternalServerError) + body.Fail() } } diff --git a/knotmirror/xrpc/git_get_blob.go b/knotmirror/xrpc/git_get_blob.go index c854293b..9339a7eb 100644 --- a/knotmirror/xrpc/git_get_blob.go +++ b/knotmirror/xrpc/git_get_blob.go @@ -2,6 +2,7 @@ package xrpc import ( "crypto/sha256" + "encoding/hex" "fmt" "io" "net/http" @@ -12,6 +13,7 @@ import ( "github.com/bluesky-social/indigo/atproto/atclient" "github.com/bluesky-social/indigo/atproto/syntax" + "tangled.org/core/gitutil" "tangled.org/core/knotmirror/xrpc/gitea" ) @@ -58,13 +60,14 @@ func (x *Xrpc) GetBlob(w http.ResponseWriter, r *http.Request) { } defer reader.Close() - w.Header().Set("Content-Length", strconv.FormatInt(size, 10)) - // default to octet-stream for large blobs if size > 1024*1024 { // 1MiB + w.Header().Set("Content-Length", strconv.FormatInt(size, 10)) w.Header().Set("Content-Type", "application/octet-stream") - if _, err := io.Copy(w, reader); err != nil { + body := gitutil.NewResponseBody(w) + if _, err := io.Copy(body, reader); err != nil { l.Error("failed to serve the blob", "err", err) + body.Fail() } return } @@ -76,11 +79,12 @@ func (x *Xrpc) GetBlob(w http.ResponseWriter, r *http.Request) { return } - eTag := fmt.Sprintf("\"%x\"", sha256.Sum256(contents)) - if clientETag := r.Header.Get("If-None-Match"); clientETag == eTag { + eTag := blobETag(contents) + if gitutil.ETagMatches(r.Header, eTag) { w.WriteHeader(http.StatusNotModified) return } + w.Header().Set("Content-Length", strconv.Itoa(len(contents))) w.Header().Set("ETag", eTag) w.Header().Set("X-Content-Type-Options", "nosniff") @@ -113,6 +117,15 @@ func (x *Xrpc) GetBlob(w http.ResponseWriter, r *http.Request) { w.Write(contents) } +const blobETagDomain = "knotmirror.blob.v1" + +func blobETag(contents []byte) string { + digest := sha256.New() + digest.Write([]byte(blobETagDomain + "\x00")) + digest.Write(contents) + return fmt.Sprintf("%q", hex.EncodeToString(digest.Sum(nil))) +} + var textualMimeTypes = []string{ "application/json", "application/xml", diff --git a/knotmirror/xrpc/git_get_blob_test.go b/knotmirror/xrpc/git_get_blob_test.go new file mode 100644 index 00000000..c82e910b --- /dev/null +++ b/knotmirror/xrpc/git_get_blob_test.go @@ -0,0 +1,24 @@ +package xrpc + +import ( + "crypto/sha256" + "encoding/hex" + "regexp" + "testing" +) + +func TestBlobETag(t *testing.T) { + contents := []byte("kelp and periwinkle\n") + eTag := blobETag(contents) + bare := sha256.Sum256(contents) + + if !regexp.MustCompile(`^"[0-9a-f]{64}"$`).MatchString(eTag) { + t.Errorf("etag = %s, want a quoted sha256 digest", eTag) + } + if eTag == `"`+hex.EncodeToString(bare[:])+`"` { + t.Error("the mirror must separate its validator from a bare content digest, because a knot answering the same proxied request hashes the same bytes") + } + if same, other := blobETag(contents), blobETag([]byte("conch\n")); same != eTag || other == eTag { + t.Errorf("etag = %s, then %s for the same blob and %s for another, want one validator per blob", eTag, same, other) + } +} diff --git a/knotmirror/xrpc/proxy.go b/knotmirror/xrpc/proxy.go index b903e09b..2794979b 100644 --- a/knotmirror/xrpc/proxy.go +++ b/knotmirror/xrpc/proxy.go @@ -19,6 +19,7 @@ import ( "github.com/go-git/go-git/v5/plumbing/filemode" "github.com/samber/lo" "tangled.org/core/api/tangled" + "tangled.org/core/gitutil" "tangled.org/core/knotmirror/db" "tangled.org/core/knotmirror/models" "tangled.org/core/repoident" @@ -136,6 +137,7 @@ func (x *Xrpc) proxyToKnot(w http.ResponseWriter, r *http.Request, repoDid synta return false } req.Header.Set(forwardedForHeader, forwardedFor(r)) + gitutil.ForwardHeaders(req.Header, r.Header, "If-None-Match") resp, err := x.httpClient.Do(req) if err != nil { diff --git a/knotserver/git/merge_test.go b/knotserver/git/merge_test.go index 35383e89..f3a9efcf 100644 --- a/knotserver/git/merge_test.go +++ b/knotserver/git/merge_test.go @@ -219,7 +219,7 @@ index 0000000..ce01362 opts := MergeOptions{ CommitMessage: "Add scallop.txt", CommitterName: "nel", - CommitterEmail: "nel@nel.pet", + CommitterEmail: "noreply@nel.pet", FormatPatch: false, } diff --git a/knotserver/xrpc/repo_archive.go b/knotserver/xrpc/repo_archive.go index a1225f09..6ebc0078 100644 --- a/knotserver/xrpc/repo_archive.go +++ b/knotserver/xrpc/repo_archive.go @@ -34,23 +34,23 @@ func (x *Xrpc) RepoArchive(w http.ResponseWriter, r *http.Request) { return } - hash := gr.Hash() - params.Rev = params.Rev.OrHash(hash) - params.Prefix = params.Prefix.OrDefault(resolved.name, params.Rev) - - params.SetHeaders(w.Header(), resolved.name) - w.Header().Set("Link", gitutil.ImmutableLink( - x.archiveURL(repo, params.WithRev(gitutil.RevFromHash(hash))), - )) - - if err := gitutil.WriteArchive(r.Context(), w, resolved.path, gitutil.RevFromHash(hash), params.Format, params.Prefix); err != nil { - // once we start writing to the body we can't report error anymore - // so we are only left with logging the error + hash := gitutil.RevFromHash(gr.Hash()) + served := params.WithRev(params.Rev.Or(hash)).Serve(resolved.name).WithRev(hash) + + served.SetHeaders(w.Header()) + w.Header().Set("Link", gitutil.ImmutableLink(x.archiveURL(repo, served))) + if served.ServeNotModified(w, r, gitutil.RepoIdentity(resolved.path)) { + return + } + + body := gitutil.NewResponseBody(w) + if err := gitutil.WriteArchive(r.Context(), body, resolved.path, served); err != nil { x.Logger.Error("writing archive", "error", err.Error(), "format", params.Format) + body.Fail() } } -func (x *Xrpc) archiveURL(repo string, params gitutil.ArchiveParams) string { +func (x *Xrpc) archiveURL(repo string, params gitutil.ServedArchive) string { scheme := "https" if x.Config.Server.Dev { scheme = "http"