From 7a0a64b1e91ccc1d7f45e689917f6ce378f21929 Mon Sep 17 00:00:00 2001 From: Lewis Date: Mon, 6 Jul 2026 11:24:36 +0300 Subject: [PATCH] appview/middleware: fix renames that collide w/ live repo's slug Lewis: May this revision serve well! --- appview/ingester_repo.go | 6 ++ appview/ingester_repo_test.go | 52 ++++++++++++++++ appview/middleware/middleware.go | 10 ++- appview/middleware/resolve_repo_test.go | 83 +++++++++++++++++++++++++ 4 files changed, 148 insertions(+), 3 deletions(-) create mode 100644 appview/middleware/resolve_repo_test.go diff --git a/appview/ingester_repo.go b/appview/ingester_repo.go index 953bf1f3..248faa67 100644 --- a/appview/ingester_repo.go +++ b/appview/ingester_repo.go @@ -101,6 +101,9 @@ func (i *Ingester) ingestRepoCreate(ctx context.Context, e *jmodels.Event, l *sl if err := db.RecordRepoRename(tx, e.Did, prev.Rkey, repoDid); err != nil { return fmt.Errorf("failed to record rename history: %w", err) } + if err := db.DeleteRepoRename(tx, e.Did, strings.ToLower(newName)); err != nil { + return fmt.Errorf("failed to clear colliding rename alias: %w", err) + } renamed := *prev renamed.Rkey = e.Commit.RKey @@ -160,6 +163,9 @@ func (i *Ingester) ingestRepoCreate(ctx context.Context, e *jmodels.Event, l *sl if err := db.AddRepo(tx, repo); err != nil { return fmt.Errorf("failed to insert repo: %w", err) } + if err := db.DeleteRepoRename(tx, e.Did, strings.ToLower(repo.Slug())); err != nil { + return fmt.Errorf("failed to clear colliding rename alias: %w", err) + } if err := tx.Commit(); err != nil { return fmt.Errorf("failed to commit insert tx: %w", err) } diff --git a/appview/ingester_repo_test.go b/appview/ingester_repo_test.go index 44302d81..dd45e218 100644 --- a/appview/ingester_repo_test.go +++ b/appview/ingester_repo_test.go @@ -807,3 +807,55 @@ func TestIngestRepo_UpdateRejectsRepoDidMutation(t *testing.T) { t.Errorf("metadata from repoDid-mutating update applied: %+v", akshay) } } + +func renameAliasExists(t *testing.T, ing *Ingester, ownerDid, oldRkey string) bool { + t.Helper() + var n int + if err := ing.Db.QueryRow( + `select count(*) from repo_renames where owner_did = ? and old_rkey = ?`, + ownerDid, oldRkey, + ).Scan(&n); err != nil { + t.Fatalf("count repo_renames %q: %v", oldRkey, err) + } + return n > 0 +} + +func TestIngestRepo_RenameClearsCollidingAlias(t *testing.T) { + ing, _ := newTestIngester(t) + seedRepoRow(t, ing, "did:plc:akshay", "knot.example", "anemone-old", "anemone-old", "did:plc:anemone") + if err := db.RecordRepoRename(ing.Db, "did:plc:akshay", "anemone", "did:plc:anemone"); err != nil { + t.Fatalf("RecordRepoRename: %v", err) + } + + e := makeEvent(t, jmodels.CommitOperationCreate, "did:plc:akshay", "3mpxmsvicr2zn", tangled.Repo{ + Knot: "knot.example", Name: ptr("anemone"), RepoDid: ptr("did:plc:anemone"), + }) + if err := ingestAcceptingOwner(t, ing, e); err != nil { + t.Fatalf("ingestRepo: %v", err) + } + + if renameAliasExists(t, ing, "did:plc:akshay", "anemone") { + t.Error("alias equal to the new live slug must be cleared to avoid a self-redirect loop") + } + if !renameAliasExists(t, ing, "did:plc:akshay", "anemone-old") { + t.Error("alias for the prior slug must be recorded and survive") + } +} + +func TestIngestRepo_InsertClearsCollidingAlias(t *testing.T) { + ing, _ := newTestIngester(t) + if err := db.RecordRepoRename(ing.Db, "did:plc:akshay", "clam", "did:plc:clams-former-repo"); err != nil { + t.Fatalf("RecordRepoRename: %v", err) + } + + e := makeEvent(t, jmodels.CommitOperationCreate, "did:plc:akshay", "3mpxmfgowwck3", tangled.Repo{ + Knot: "knot.example", Name: ptr("clam"), RepoDid: ptr("did:plc:clam-repo"), + }) + if err := ingestAcceptingOwner(t, ing, e); err != nil { + t.Fatalf("ingestRepo: %v", err) + } + + if renameAliasExists(t, ing, "did:plc:akshay", "clam") { + t.Error("stale alias must be cleared when a live repo claims that slug") + } +} diff --git a/appview/middleware/middleware.go b/appview/middleware/middleware.go index b0becfd7..c89e46b3 100644 --- a/appview/middleware/middleware.go +++ b/appview/middleware/middleware.go @@ -8,6 +8,7 @@ import ( "log/slog" "net/http" "net/url" + "path" "slices" "strconv" "strings" @@ -287,9 +288,12 @@ func (mw Middleware) ResolveRepo() middlewareFunc { if id.Handle.IsInvalidHandle() || handle == "" { handle = id.DID.String() } - target := reporesolver.CanonicalRedirectTarget(req, reporesolver.CanonicalRepoPath(handle, repo)) - http.Redirect(w, req, target, http.StatusMovedPermanently) - return + canonical := reporesolver.CanonicalRepoPath(handle, repo) + if path.Join(chi.URLParam(req, "user"), repoName) != canonical { + target := reporesolver.CanonicalRedirectTarget(req, canonical) + http.Redirect(w, req, target, http.StatusMovedPermanently) + return + } } ctx := context.WithValue(req.Context(), "repo", repo) diff --git a/appview/middleware/resolve_repo_test.go b/appview/middleware/resolve_repo_test.go new file mode 100644 index 00000000..b576e13b --- /dev/null +++ b/appview/middleware/resolve_repo_test.go @@ -0,0 +1,83 @@ +package middleware + +import ( + "context" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "path/filepath" + "testing" + + "github.com/bluesky-social/indigo/atproto/identity" + "github.com/bluesky-social/indigo/atproto/syntax" + "github.com/go-chi/chi/v5" + "tangled.org/core/appview/db" + "tangled.org/core/appview/models" +) + +func TestResolveRepo_RenameAlias(t *testing.T) { + const ownerDid, handle, knot = "did:plc:boltless", "boltless.dev", "knot1.tangled.sh" + cases := []struct { + name string + repoName, repoRkey, did string + alias, reqRepo string + wantLocation string // empty => expect the repo to be served without a redirect + }{ + {"alias equal to live slug is served, not looped", "anemone", "3mpxmsvicr2zn", "did:plc:anemone", "anemone", "anemone", ""}, + {"genuine rename still redirects", "whelk", "whelk", "did:plc:whelk", "conch", "conch", "/boltless.dev/whelk"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + d, err := db.Make(context.Background(), filepath.Join(t.TempDir(), "test.db")) + if err != nil { + t.Fatalf("Make: %v", err) + } + t.Cleanup(func() { d.Close() }) + + tx, err := d.Begin() + if err != nil { + t.Fatalf("Begin: %v", err) + } + if err := db.AddRepo(tx, &models.Repo{Did: ownerDid, Name: tc.repoName, Knot: knot, Rkey: tc.repoRkey, RepoDid: tc.did}); err != nil { + t.Fatalf("AddRepo: %v", err) + } + if err := tx.Commit(); err != nil { + t.Fatalf("Commit: %v", err) + } + if err := db.RecordRepoRename(d, ownerDid, tc.alias, tc.did); err != nil { + t.Fatalf("RecordRepoRename: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/"+handle+"/"+tc.reqRepo, nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("user", handle) + rctx.URLParams.Add("repo", tc.reqRepo) + ctx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx) + ctx = context.WithValue(ctx, "resolvedId", identity.Identity{DID: syntax.DID(ownerDid), Handle: syntax.Handle(handle)}) + + rec := httptest.NewRecorder() + served := false + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { served = true }) + mw := Middleware{db: d, logger: slog.New(slog.NewTextHandler(io.Discard, nil))} + mw.ResolveRepo()(next).ServeHTTP(rec, req.WithContext(ctx)) + + if tc.wantLocation == "" { + if !served { + t.Fatalf("expected repo served, got code=%d Location=%q", rec.Code, rec.Header().Get("Location")) + } + return + } + if served { + t.Fatal("expected a redirect, but the repo was served") + } + if rec.Code != http.StatusMovedPermanently { + t.Fatalf("got %d, want 301", rec.Code) + } + if got := rec.Header().Get("Location"); got != tc.wantLocation { + t.Errorf("Location = %q, want %q", got, tc.wantLocation) + } + }) + } +} -- 2.51.2