diff --git a/appview/middleware/canonicalize_test.go b/appview/middleware/canonicalize_test.go new file mode 100644 --- /dev/null +++ b/appview/middleware/canonicalize_test.go @@ -0,0 +1,136 @@ +package middleware + +import ( + "context" + "net/http" + "net/http/httptest" + "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/models" +) + +func runCanonicalize(t *testing.T, method, urlPath, urlUser, urlRepo, handle string, repo *models.Repo) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(method, urlPath, nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("user", urlUser) + rctx.URLParams.Add("repo", urlRepo) + ctx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx) + id := identity.Identity{ + DID: syntax.DID("did:plc:boltless"), + Handle: syntax.Handle(handle), + } + ctx = context.WithValue(ctx, "resolvedId", id) + ctx = context.WithValue(ctx, "repo", repo) + req = req.WithContext(ctx) + + rec := httptest.NewRecorder() + called := false + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusOK) + }) + mw := Middleware{} + mw.CanonicalizeRepoURL()(next).ServeHTTP(rec, req) + if rec.Code == http.StatusFound { + if called { + t.Errorf("middleware both issued 302 and invoked next handler") + } + } + return rec +} + +func TestCanonicalize_CanonicalUrlPassesThrough(t *testing.T) { + repo := &models.Repo{Did: "did:plc:boltless", Name: "anemone", Rkey: "anemone"} + rec := runCanonicalize(t, "GET", "/boltless.dev/anemone/issues", "boltless.dev", "anemone", "boltless.dev", repo) + if rec.Code != http.StatusOK { + t.Errorf("canonical URL got %d, want 200; Location=%q", rec.Code, rec.Header().Get("Location")) + } +} + +func TestCanonicalize_EmptyNameUsesRkeyAsSlug(t *testing.T) { + repo := &models.Repo{Did: "did:plc:boltless", Name: "", Rkey: "anemone"} + rec := runCanonicalize(t, "GET", "/boltless.dev/anemone/pulls", "boltless.dev", "anemone", "boltless.dev", repo) + if rec.Code != http.StatusOK { + t.Errorf("rkey-as-slug canonical URL got %d, want 200; Location=%q", rec.Code, rec.Header().Get("Location")) + } +} + +func TestCanonicalize_EmptyNameOwnerDidRedirectsToHandleRkey(t *testing.T) { + repo := &models.Repo{Did: "did:plc:boltless", Name: "", Rkey: "anemone"} + rec := runCanonicalize(t, "GET", "/did:plc:boltless/anemone/pulls", "did:plc:boltless", "anemone", "boltless.dev", repo) + if rec.Code != http.StatusFound { + t.Fatalf("got %d, want 302", rec.Code) + } + if got, want := rec.Header().Get("Location"), "/boltless.dev/anemone/pulls"; got != want { + t.Errorf("Location = %q, want %q", got, want) + } +} + +func TestCanonicalize_HandleSlashTIDRedirectsToName(t *testing.T) { + repo := &models.Repo{Did: "did:plc:boltless", Name: "anemone", Rkey: "3kabcxyz"} + rec := runCanonicalize(t, "GET", "/boltless.dev/3kabcxyz/issues", "boltless.dev", "3kabcxyz", "boltless.dev", repo) + if rec.Code != http.StatusFound { + t.Fatalf("got %d, want 302", rec.Code) + } + if got, want := rec.Header().Get("Location"), "/boltless.dev/anemone/issues"; got != want { + t.Errorf("Location = %q, want %q", got, want) + } +} + +func TestCanonicalize_OwnerDidRedirectsToHandle(t *testing.T) { + repo := &models.Repo{Did: "did:plc:boltless", Name: "anemone", Rkey: "anemone"} + rec := runCanonicalize(t, "GET", "/did:plc:boltless/anemone/pulls/3", "did:plc:boltless", "anemone", "boltless.dev", repo) + if rec.Code != http.StatusFound { + t.Fatalf("got %d, want 302", rec.Code) + } + if got, want := rec.Header().Get("Location"), "/boltless.dev/anemone/pulls/3"; got != want { + t.Errorf("Location = %q, want %q", got, want) + } +} + +func TestCanonicalize_OwnerDidAndTIDRedirectsToCanonical(t *testing.T) { + repo := &models.Repo{Did: "did:plc:boltless", Name: "anemone", Rkey: "3kabcxyz"} + rec := runCanonicalize(t, "GET", "/did:plc:boltless/3kabcxyz", "did:plc:boltless", "3kabcxyz", "boltless.dev", repo) + if rec.Code != http.StatusFound { + t.Fatalf("got %d, want 302", rec.Code) + } + if got, want := rec.Header().Get("Location"), "/boltless.dev/anemone"; got != want { + t.Errorf("Location = %q, want %q", got, want) + } +} + +func TestCanonicalize_PreservesQueryString(t *testing.T) { + repo := &models.Repo{Did: "did:plc:boltless", Name: "anemone", Rkey: "anemone"} + rec := runCanonicalize(t, "GET", "/did:plc:boltless/anemone/issues?state=closed&page=2", "did:plc:boltless", "anemone", "boltless.dev", repo) + if got, want := rec.Header().Get("Location"), "/boltless.dev/anemone/issues?state=closed&page=2"; got != want { + t.Errorf("Location = %q, want %q", got, want) + } +} + +func TestCanonicalize_PostNotRedirected(t *testing.T) { + repo := &models.Repo{Did: "did:plc:boltless", Name: "anemone", Rkey: "anemone"} + rec := runCanonicalize(t, "POST", "/did:plc:boltless/anemone/issues", "did:plc:boltless", "anemone", "boltless.dev", repo) + if rec.Code != http.StatusOK { + t.Errorf("POST on non-canonical URL got %d, want 200; Location=%q", rec.Code, rec.Header().Get("Location")) + } +} + +func TestCanonicalize_InvalidHandlePassesThrough(t *testing.T) { + repo := &models.Repo{Did: "did:plc:boltless", Name: "anemone", Rkey: "anemone"} + rec := runCanonicalize(t, "GET", "/did:plc:boltless/anemone", "did:plc:boltless", "anemone", string(syntax.HandleInvalid), repo) + if rec.Code != http.StatusOK { + t.Errorf("invalid handle got %d, want 200; Location=%q", rec.Code, rec.Header().Get("Location")) + } +} + +func TestCanonicalize_DotGitSuffixStripped(t *testing.T) { + repo := &models.Repo{Did: "did:plc:boltless", Name: "anemone", Rkey: "anemone"} + rec := runCanonicalize(t, "GET", "/boltless.dev/anemone.git/", "boltless.dev", "anemone.git", "boltless.dev", repo) + if rec.Code != http.StatusOK { + t.Errorf(".git on canonical name got %d, want 200; Location=%q", rec.Code, rec.Header().Get("Location")) + } +} diff --git a/appview/middleware/middleware.go b/appview/middleware/middleware.go --- a/appview/middleware/middleware.go +++ b/appview/middleware/middleware.go @@ -17,6 +17,7 @@ "github.com/go-chi/chi/v5" "tangled.org/core/appview/cache" "tangled.org/core/appview/db" + "tangled.org/core/appview/models" "tangled.org/core/appview/oauth" "tangled.org/core/appview/pages" "tangled.org/core/appview/pagination" @@ -234,8 +235,7 @@ return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { l := mw.logger.With("middleware", "ResolveRepo") - repoName := chi.URLParam(req, "repo") - repoName = strings.TrimSuffix(repoName, ".git") + repoName := strings.TrimSuffix(chi.URLParam(req, "repo"), ".git") rkey := strings.ToLower(repoName) id, ok := req.Context().Value("resolvedId").(identity.Identity) @@ -245,63 +245,84 @@ return } - repo, err := db.GetRepo( - mw.db, - orm.FilterEq("did", id.DID.String()), - orm.FilterEq("rkey", rkey), - ) - if err != nil { - if !errors.Is(err, sql.ErrNoRows) { - l.Error("failed to resolve repo", "err", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - hint, hintErr := db.LookupRepoRename(mw.db, id.DID.String(), rkey) - if hintErr != nil && !errors.Is(hintErr, sql.ErrNoRows) { - l.Error("failed to lookup repo rename hint", "err", hintErr) - } - if hint != nil { - parts := strings.SplitN(strings.TrimPrefix(req.URL.Path, "/"), "/", 3) - target := "/" + parts[0] + "/" + hint.Rkey - if len(parts) == 3 { - target += "/" + parts[2] - } - if req.URL.RawQuery != "" { - target += "?" + req.URL.RawQuery - } - http.Redirect(w, req, target, http.StatusMovedPermanently) - return - } - nameRepos, nameErr := db.GetRepos( - mw.db, - orm.FilterEq("did", id.DID.String()), - orm.FilterEq("name", repoName), - ) - if nameErr == nil && len(nameRepos) == 1 && nameRepos[0].RepoDid != "" { - nameRepo := &nameRepos[0] - if _, tidErr := syntax.ParseTID(nameRepo.Rkey); tidErr == nil { - ctx := context.WithValue(req.Context(), "repo", nameRepo) - next.ServeHTTP(w, req.WithContext(ctx)) - return - } - parts := strings.SplitN(strings.TrimPrefix(req.URL.Path, "/"), "/", 3) - target := "/" + nameRepo.RepoDid - if len(parts) == 3 { - target += "/" + parts[2] - } - if req.URL.RawQuery != "" { - target += "?" + req.URL.RawQuery - } - http.Redirect(w, req, target, http.StatusFound) - return - } + repo, isRename := resolveRepoForOwner(mw.db, id.DID.String(), repoName, rkey, l) + if repo == nil { w.WriteHeader(http.StatusNotFound) mw.pages.ErrorKnot404(w) + return + } + if isRename { + handle := id.Handle.String() + 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 } ctx := context.WithValue(req.Context(), "repo", repo) next.ServeHTTP(w, req.WithContext(ctx)) + }) + } +} + +func resolveRepoForOwner(d db.Execer, ownerDid, repoName, rkey string, l *slog.Logger) (*models.Repo, bool) { + repo, err := db.GetRepo(d, orm.FilterEq("did", ownerDid), orm.FilterEq("rkey", rkey)) + if err == nil { + return repo, false + } + if !errors.Is(err, sql.ErrNoRows) { + l.Error("failed to resolve repo by rkey", "err", err) + return nil, false + } + + hint, hintErr := db.LookupRepoRename(d, ownerDid, rkey) + if hintErr != nil && !errors.Is(hintErr, sql.ErrNoRows) { + l.Error("failed to lookup repo rename hint", "err", hintErr) + } + if hint != nil { + return hint, true + } + + nameRepos, nameErr := db.GetRepos(d, orm.FilterEq("did", ownerDid), orm.FilterEq("name", repoName)) + if nameErr != nil { + l.Error("failed to resolve repo by name", "err", nameErr) + return nil, false + } + if len(nameRepos) == 1 { + return &nameRepos[0], false + } + return nil, false +} + +func (mw Middleware) CanonicalizeRepoURL() middlewareFunc { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if req.Method != http.MethodGet && req.Method != http.MethodHead { + next.ServeHTTP(w, req) + return + } + id, idOk := req.Context().Value("resolvedId").(identity.Identity) + repo, repoOk := req.Context().Value("repo").(*models.Repo) + if !idOk || !repoOk || id.Handle.IsInvalidHandle() { + next.ServeHTTP(w, req) + return + } + handle := id.Handle.String() + if handle == "" { + next.ServeHTTP(w, req) + return + } + canonical := reporesolver.CanonicalRepoPath(handle, repo) + urlUser := chi.URLParam(req, "user") + urlRepo := strings.TrimSuffix(chi.URLParam(req, "repo"), ".git") + if urlUser+"/"+urlRepo == canonical { + next.ServeHTTP(w, req) + return + } + + http.Redirect(w, req, reporesolver.CanonicalRedirectTarget(req, canonical), http.StatusFound) }) } } @@ -408,14 +429,21 @@ if strings.Contains(modulePath, ":") { modulePath = userutil.FlattenDid(f.Did) + "/" + f.Rkey } - html := fmt.Sprintf( - ` -`, - modulePath, fullName, - modulePath, fullName, - ) + tags := []string{ + fmt.Sprintf(``, modulePath, fullName), + fmt.Sprintf(``, modulePath, fullName), + } + if f.RepoDid != "" { + stable := userutil.FlattenDid(f.RepoDid) + if stable != modulePath { + tags = append(tags, + fmt.Sprintf(``, stable, f.RepoDid), + fmt.Sprintf(``, stable, f.RepoDid), + ) + } + } w.Header().Set("Content-Type", "text/html") - w.Write([]byte(html)) + w.Write([]byte(strings.Join(tags, "\n"))) return } } diff --git a/appview/models/repo.go b/appview/models/repo.go --- a/appview/models/repo.go +++ b/appview/models/repo.go @@ -80,6 +80,13 @@ return syntax.ATURI(fmt.Sprintf("at://%s/%s/%s", r.Did, tangled.RepoNSID, r.Rkey)) } +func (r Repo) Slug() string { + if r.Name != "" { + return r.Name + } + return r.Rkey +} + func (r Repo) RepoIdentifier() string { if r.RepoDid != "" { return r.RepoDid diff --git a/appview/models/repo_test.go b/appview/models/repo_test.go --- a/appview/models/repo_test.go +++ b/appview/models/repo_test.go @@ -84,3 +84,23 @@ t.Errorf("cosmeticName = %q, want %q", *rec.Name, "MyRepo") } } + +func TestRepoSlug(t *testing.T) { + cases := []struct { + name string + repo Repo + want string + }{ + {"name set distinct from rkey", Repo{Name: "anemone", Rkey: "3kabc"}, "anemone"}, + {"name equals rkey", Repo{Name: "scallop", Rkey: "scallop"}, "scallop"}, + {"name empty falls to rkey", Repo{Name: "", Rkey: "whelk"}, "whelk"}, + {"both empty", Repo{}, ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := c.repo.Slug(); got != c.want { + t.Errorf("Slug() = %q, want %q", got, c.want) + } + }) + } +} diff --git a/appview/pages/funcmap.go b/appview/pages/funcmap.go --- a/appview/pages/funcmap.go +++ b/appview/pages/funcmap.go @@ -88,7 +88,7 @@ } handle := ownerId.Handle if handle != "" && !handle.IsInvalidHandle() { - return string(handle) + "/" + repo.Name + return string(handle) + "/" + repo.Slug() } return repo.RepoIdentifier() }, diff --git a/appview/pages/ratchet_test.go b/appview/pages/ratchet_test.go new file mode 100644 --- /dev/null +++ b/appview/pages/ratchet_test.go @@ -0,0 +1,92 @@ +package pages + +import ( + "io/fs" + "regexp" + "strings" + "testing" +) + +var repoRkeyAllowlist = map[string]bool{ + "templates/repo/settings/sites.html": true, +} + +var repoRkeyPattern = regexp.MustCompile(`\.(Repo|RepoInfo)\.Rkey\b`) + +func TestNoRepoRkeyInTemplates(t *testing.T) { + err := fs.WalkDir(Files, "templates", func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || !strings.HasSuffix(path, ".html") { + return nil + } + if repoRkeyAllowlist[path] { + return nil + } + data, err := fs.ReadFile(Files, path) + if err != nil { + return err + } + for i, line := range strings.Split(string(data), "\n") { + if repoRkeyPattern.MatchString(line) { + t.Errorf("%s:%d uses .Repo.Rkey or .RepoInfo.Rkey in URL position. Use .Slug to prefer Name over TID-Rkey.\n %s", + path, i+1, strings.TrimSpace(line)) + } + } + return nil + }) + if err != nil { + t.Fatal(err) + } +} + +var bareDidAllowlist = map[string]bool{ + "templates/strings/string.html": true, + "templates/strings/fragments/form.html": true, + "templates/spindles/dashboard.html": true, +} + +var didCloseAsUrlSegment = regexp.MustCompile(`\.(?:Did|OwnerDid)\s*\}\}\s*/`) +var printfWithUrlFormat = regexp.MustCompile(`printf\s+"[^"]*/[^"]*%s`) +var didArgRef = regexp.MustCompile(`\b[\$\.]\w+(?:\.\w+)*\.(?:Did|OwnerDid)\b`) + +func TestNoBareDidInTemplateRepoUrls(t *testing.T) { + err := fs.WalkDir(Files, "templates", func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || !strings.HasSuffix(path, ".html") { + return nil + } + if bareDidAllowlist[path] { + return nil + } + data, err := fs.ReadFile(Files, path) + if err != nil { + return err + } + for i, line := range strings.Split(string(data), "\n") { + for _, idx := range didCloseAsUrlSegment.FindAllStringIndex(line, -1) { + openIdx := strings.LastIndex(line[:idx[0]], "{{") + if openIdx == -1 { + continue + } + action := line[openIdx:idx[1]] + if !strings.Contains(action, "resolve") { + t.Errorf("%s:%d renders raw DID as URL path segment. Wrap in `resolve` so the handle appears.\n %s", + path, i+1, strings.TrimSpace(line)) + break + } + } + if printfWithUrlFormat.MatchString(line) && didArgRef.MatchString(line) && !strings.Contains(line, "resolve ") { + t.Errorf("%s:%d builds a URL path via printf with a raw DID arg. Wrap the DID in `resolve` so handle appears.\n %s", + path, i+1, strings.TrimSpace(line)) + } + } + return nil + }) + if err != nil { + t.Fatal(err) + } +} diff --git a/appview/repo/feed.go b/appview/repo/feed.go --- a/appview/repo/feed.go +++ b/appview/repo/feed.go @@ -315,7 +315,7 @@ rp.logger.Error("failed to get resolved repo owner id") return } - ownerSlashRepo := repoOwnerId.Handle.String() + "/" + f.Rkey + ownerSlashRepo := repoOwnerId.Handle.String() + "/" + f.Slug() opts := parseFeedOpts(r) feed, err := rp.getRepoFeed(r.Context(), f, ownerSlashRepo, opts) diff --git a/appview/reporesolver/resolver.go b/appview/reporesolver/resolver.go --- a/appview/reporesolver/resolver.go +++ b/appview/reporesolver/resolver.go @@ -35,19 +35,40 @@ return &RepoResolver{config: config, enforcer: enforcer, execer: execer, rdb: rdb} } +func CanonicalRepoPath(handle string, repo *models.Repo) string { + return path.Join(handle, repo.Slug()) +} + +func CanonicalRedirectTarget(req *http.Request, canonical string) string { + parts := strings.SplitN(strings.TrimPrefix(req.URL.Path, "/"), "/", 3) + target := "/" + canonical + if len(parts) == 3 { + target += "/" + parts[2] + } + if req.URL.RawQuery != "" { + target += "?" + req.URL.RawQuery + } + return target +} + // NOTE: this... should not even be here. the entire package will be removed in future refactor func GetBaseRepoPath(r *http.Request, repo *models.Repo) string { - if repo.RepoDid != "" { - return repo.RepoDid + if id, ok := r.Context().Value("resolvedId").(identity.Identity); ok && !id.Handle.IsInvalidHandle() { + if h := id.Handle.String(); h != "" { + return CanonicalRepoPath(h, repo) + } } var ( user = chi.URLParam(r, "user") name = chi.URLParam(r, "repo") ) - if user == "" || name == "" { - return repo.RepoIdentifier() + if user != "" && name != "" { + return path.Join(user, name) } - return path.Join(user, name) + if repo.Name != "" { + return path.Join(repo.Did, repo.Name) + } + return repo.RepoIdentifier() } // TODO: move this out of `RepoResolver` struct diff --git a/appview/reporesolver/resolver_test.go b/appview/reporesolver/resolver_test.go --- a/appview/reporesolver/resolver_test.go +++ b/appview/reporesolver/resolver_test.go @@ -1,6 +1,16 @@ package reporesolver -import "testing" +import ( + "context" + "net/http" + "net/http/httptest" + "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/models" +) func TestExtractCurrentDir(t *testing.T) { tests := []struct { @@ -18,5 +28,106 @@ if got := extractCurrentDir(tt.path); got != tt.want { t.Errorf("extractCurrentDir(%q) = %q, want %q", tt.path, got, tt.want) } + } +} + +func TestCanonicalRepoPath(t *testing.T) { + cases := []struct { + name string + handle string + repo *models.Repo + want string + }{ + {"name preferred", "boltless.dev", &models.Repo{Name: "anemone", Rkey: "3kabc"}, "boltless.dev/anemone"}, + {"name equals rkey", "boltless.dev", &models.Repo{Name: "clam", Rkey: "clam"}, "boltless.dev/clam"}, + {"empty name uses rkey", "akshay.dev", &models.Repo{Rkey: "limpet"}, "akshay.dev/limpet"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := CanonicalRepoPath(c.handle, c.repo); got != c.want { + t.Errorf("CanonicalRepoPath = %q, want %q", got, c.want) + } + }) + } +} + +func reqWithChiParams(user, repo string) *http.Request { + r := httptest.NewRequest("GET", "/", nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("user", user) + rctx.URLParams.Add("repo", repo) + return r.WithContext(context.WithValue(r.Context(), chi.RouteCtxKey, rctx)) +} + +func TestGetBaseRepoPath_DoesNotVoluntaryRedirectToRepoDid(t *testing.T) { + r := reqWithChiParams("@boltless.dev", "anemone") + repo := &models.Repo{ + Did: "did:plc:boltless", + Name: "anemone", + Rkey: "3kabcxyz", + RepoDid: "did:plc:anemone", + } + got := GetBaseRepoPath(r, repo) + want := "@boltless.dev/anemone" + if got != want { + t.Errorf("GetBaseRepoPath = %q, want %q", got, want) + } +} + +func TestGetBaseRepoPath_HonorsUrlParams(t *testing.T) { + r := reqWithChiParams("did:plc:akshay", "limpet") + repo := &models.Repo{Did: "did:plc:akshay", Name: "limpet", Rkey: "limpet"} + if got, want := GetBaseRepoPath(r, repo), "did:plc:akshay/limpet"; got != want { + t.Errorf("GetBaseRepoPath = %q, want %q", got, want) + } +} + +func TestGetBaseRepoPath_NoParamsPrefersName(t *testing.T) { + r := httptest.NewRequest("GET", "/", nil) + repo := &models.Repo{Did: "did:plc:akshay", Name: "scallop", Rkey: "3koldtid"} + got := GetBaseRepoPath(r, repo) + want := "did:plc:akshay/scallop" + if got != want { + t.Errorf("GetBaseRepoPath = %q, want %q", got, want) + } +} + +func TestGetBaseRepoPath_NoParamsNoNameFallsToRepoIdentifier(t *testing.T) { + r := httptest.NewRequest("GET", "/", nil) + repo := &models.Repo{Did: "did:plc:akshay", Rkey: "3koldtid", RepoDid: "did:plc:scallop"} + if got, want := GetBaseRepoPath(r, repo), "did:plc:scallop"; got != want { + t.Errorf("GetBaseRepoPath = %q, want %q", got, want) + } +} + +func reqWithResolvedId(handle, did string) *http.Request { + r := reqWithChiParams(did, "3koldtid") + id := identity.Identity{ + DID: syntax.DID(did), + Handle: syntax.Handle(handle), + } + return r.WithContext(context.WithValue(r.Context(), "resolvedId", id)) +} + +func TestGetBaseRepoPath_PrefersResolvedHandleOverChiDid(t *testing.T) { + r := reqWithResolvedId("boltless.dev", "did:plc:boltless") + repo := &models.Repo{Did: "did:plc:boltless", Name: "anemone", Rkey: "3kabcxyz"} + got := GetBaseRepoPath(r, repo) + want := "boltless.dev/anemone" + if got != want { + t.Errorf("GetBaseRepoPath = %q, want %q", got, want) + } +} + +func TestGetBaseRepoPath_InvalidHandleFallsThroughToChi(t *testing.T) { + r := reqWithChiParams("did:plc:boltless", "limpet") + id := identity.Identity{ + DID: syntax.DID("did:plc:boltless"), + Handle: syntax.HandleInvalid, + } + r = r.WithContext(context.WithValue(r.Context(), "resolvedId", id)) + repo := &models.Repo{Did: "did:plc:boltless", Name: "limpet", Rkey: "limpet"} + if got, want := GetBaseRepoPath(r, repo), "did:plc:boltless/limpet"; got != want { + t.Errorf("GetBaseRepoPath = %q, want %q", got, want) } } diff --git a/appview/state/profile.go b/appview/state/profile.go --- a/appview/state/profile.go +++ b/appview/state/profile.go @@ -725,7 +725,7 @@ func (s *State) createPullRequestItem(pull *models.Pull, owner *identity.Identity, author *feeds.Author) *feeds.Item { return &feeds.Item{ Title: fmt.Sprintf("%s created pull request '%s' in @%s/%s", author.Name, pull.Title, owner.Handle, pull.Repo.Name), - Link: &feeds.Link{Href: fmt.Sprintf("%s/@%s/%s/pulls/%d", s.config.Core.BaseUrl(), owner.Handle, pull.Repo.Rkey, pull.PullId), Type: "text/html", Rel: "alternate"}, + Link: &feeds.Link{Href: fmt.Sprintf("%s/@%s/%s/pulls/%d", s.config.Core.BaseUrl(), owner.Handle, pull.Repo.Slug(), pull.PullId), Type: "text/html", Rel: "alternate"}, Created: pull.Created, Author: author, } @@ -734,7 +734,7 @@ func (s *State) createIssueItem(issue *models.Issue, owner *identity.Identity, author *feeds.Author) *feeds.Item { return &feeds.Item{ Title: fmt.Sprintf("%s created issue '%s' in @%s/%s", author.Name, issue.Title, owner.Handle, issue.Repo.Name), - Link: &feeds.Link{Href: fmt.Sprintf("%s/@%s/%s/issues/%d", s.config.Core.BaseUrl(), owner.Handle, issue.Repo.Rkey, issue.IssueId), Type: "text/html", Rel: "alternate"}, + Link: &feeds.Link{Href: fmt.Sprintf("%s/@%s/%s/issues/%d", s.config.Core.BaseUrl(), owner.Handle, issue.Repo.Slug(), issue.IssueId), Type: "text/html", Rel: "alternate"}, Created: issue.Created, Author: author, } @@ -754,7 +754,7 @@ return &feeds.Item{ Title: title, - Link: &feeds.Link{Href: fmt.Sprintf("%s/@%s/%s", s.config.Core.BaseUrl(), author.Name[1:], repo.Repo.Rkey), Type: "text/html", Rel: "alternate"}, // Remove @ prefix + Link: &feeds.Link{Href: fmt.Sprintf("%s/@%s/%s", s.config.Core.BaseUrl(), author.Name[1:], repo.Repo.Slug()), Type: "text/html", Rel: "alternate"}, // Remove @ prefix Created: repo.Repo.Created, Author: author, }, nil diff --git a/appview/state/router.go b/appview/state/router.go --- a/appview/state/router.go +++ b/appview/state/router.go @@ -130,11 +130,6 @@ r.With(mw.ResolveRepo()).Route("/{repo}", func(r chi.Router) { r.Use(mw.GoImport()) - r.Mount("/", s.RepoRouter(mw)) - r.Mount("/issues", s.IssuesRouter(mw)) - r.Mount("/pulls", s.PullsRouter(mw)) - r.Mount("/pipelines", s.PipelinesRouter(mw)) - r.Mount("/labels", s.LabelsRouter()) // These routes get proxied to the knot r.Get("/info/refs", s.InfoRefs) @@ -142,6 +137,14 @@ r.Post("/git-upload-pack", s.UploadPack) r.Post("/git-receive-pack", s.ReceivePack) + r.Group(func(r chi.Router) { + r.Use(mw.CanonicalizeRepoURL()) + r.Mount("/issues", s.IssuesRouter(mw)) + r.Mount("/pulls", s.PullsRouter(mw)) + r.Mount("/pipelines", s.PipelinesRouter(mw)) + r.Mount("/labels", s.LabelsRouter()) + r.Mount("/", s.RepoRouter(mw)) + }) }) }) diff --git a/appview/pages/repoinfo/repoinfo.go b/appview/pages/repoinfo/repoinfo.go --- a/appview/pages/repoinfo/repoinfo.go +++ b/appview/pages/repoinfo/repoinfo.go @@ -19,8 +19,15 @@ } } +func (r RepoInfo) Slug() string { + if r.Name != "" { + return r.Name + } + return r.Rkey +} + func (r RepoInfo) FullName() string { - return path.Join(r.owner(), r.Rkey) + return path.Join(r.owner(), r.Slug()) } func (r RepoInfo) RepoIdentifier() string { @@ -39,7 +46,7 @@ } func (r RepoInfo) FullNameWithoutAt() string { - return path.Join(r.ownerWithoutAt(), r.Rkey) + return path.Join(r.ownerWithoutAt(), r.Slug()) } func (r RepoInfo) GetTabs() [][]string { diff --git a/appview/pages/repoinfo/repoinfo_test.go b/appview/pages/repoinfo/repoinfo_test.go new file mode 100644 --- /dev/null +++ b/appview/pages/repoinfo/repoinfo_test.go @@ -0,0 +1,46 @@ +package repoinfo + +import "testing" + +func TestSlug(t *testing.T) { + cases := []struct { + name string + info RepoInfo + want string + }{ + {"name preferred over rkey", RepoInfo{Name: "barnacle", Rkey: "3kabc"}, "barnacle"}, + {"name equals rkey", RepoInfo{Name: "clam", Rkey: "clam"}, "clam"}, + {"name empty falls to rkey", RepoInfo{Rkey: "limpet"}, "limpet"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := c.info.Slug(); got != c.want { + t.Errorf("Slug() = %q, want %q", got, c.want) + } + }) + } +} + +func TestFullName_PrefersNameOverRkey(t *testing.T) { + info := RepoInfo{OwnerHandle: "boltless.dev", Name: "uni", Rkey: "3kabcxyz"} + if got, want := info.FullName(), "boltless.dev/uni"; got != want { + t.Errorf("FullName() = %q, want %q", got, want) + } + if got, want := info.FullNameWithoutAt(), "boltless.dev/uni"; got != want { + t.Errorf("FullNameWithoutAt() = %q, want %q", got, want) + } +} + +func TestFullName_FallsBackToRkey(t *testing.T) { + info := RepoInfo{OwnerHandle: "akshay.dev", Rkey: "3kabcxyz"} + if got, want := info.FullName(), "akshay.dev/3kabcxyz"; got != want { + t.Errorf("FullName() = %q, want %q", got, want) + } +} + +func TestFullNameWithoutAt_FlattensDid(t *testing.T) { + info := RepoInfo{OwnerDid: "did:plc:boltless", Name: "nautilus", Rkey: "nautilus"} + if got, want := info.FullNameWithoutAt(), "did-plc-boltless/nautilus"; got != want { + t.Errorf("FullNameWithoutAt() = %q, want %q", got, want) + } +} diff --git a/appview/pages/templates/goodfirstissues/index.html b/appview/pages/templates/goodfirstissues/index.html --- a/appview/pages/templates/goodfirstissues/index.html +++ b/appview/pages/templates/goodfirstissues/index.html @@ -46,7 +46,7 @@ {{ i "book-marked" "w-4 h-4 mr-1.5 shrink-0" }} {{ end }} {{ $repoOwner := resolve .Repo.Did }} - {{ $repoOwner }}/{{ .Repo.Name }} + {{ $repoOwner }}/{{ .Repo.Name }} @@ -90,7 +90,7 @@ {{ if gt (len .Issues) 0 }}
{{ range .Issues }} - +
diff --git a/appview/pages/templates/layouts/repobase.html b/appview/pages/templates/layouts/repobase.html --- a/appview/pages/templates/layouts/repobase.html +++ b/appview/pages/templates/layouts/repobase.html @@ -78,7 +78,7 @@
{{ i "git-fork" "w-3 h-3 shrink-0" }} forked from - + {{ $sourceOwner }}/{{ .RepoInfo.Source.Name }}
diff --git a/appview/pages/templates/user/overview.html b/appview/pages/templates/user/overview.html --- a/appview/pages/templates/user/overview.html +++ b/appview/pages/templates/user/overview.html @@ -79,7 +79,7 @@ {{ i "book-plus" "w-4 h-4" }} {{ end }} - + {{- .Repo.Name -}} diff --git a/appview/pages/templates/notifications/fragments/item.html b/appview/pages/templates/notifications/fragments/item.html --- a/appview/pages/templates/notifications/fragments/item.html +++ b/appview/pages/templates/notifications/fragments/item.html @@ -76,11 +76,11 @@ {{ define "notificationUrl" }} {{ $url := "" }} {{ if eq .Type "repo_starred" }} - {{$url = printf "/%s/%s" (resolve .Repo.Did) .Repo.Rkey}} + {{$url = printf "/%s/%s" (resolve .Repo.Did) .Repo.Slug}} {{ else if .Issue }} - {{$url = printf "/%s/%s/issues/%d" (resolve .Repo.Did) .Repo.Rkey .Issue.IssueId}} + {{$url = printf "/%s/%s/issues/%d" (resolve .Repo.Did) .Repo.Slug .Issue.IssueId}} {{ else if .Pull }} - {{$url = printf "/%s/%s/pulls/%d" (resolve .Repo.Did) .Repo.Rkey .Pull.PullId}} + {{$url = printf "/%s/%s/pulls/%d" (resolve .Repo.Did) .Repo.Slug .Pull.PullId}} {{ else if eq .Type "followed" }} {{$url = printf "/%s" (resolve .ActorDid)}} {{ else }} diff --git a/appview/pages/templates/repo/fragments/cloneDropdown.html b/appview/pages/templates/repo/fragments/cloneDropdown.html --- a/appview/pages/templates/repo/fragments/cloneDropdown.html +++ b/appview/pages/templates/repo/fragments/cloneDropdown.html @@ -38,14 +38,14 @@ {{ template "cloneUrlItem" ( dict "Label" "HTTPS" - "HandleUrl" (printf "https://tangled.org/%s/%s" $repoOwnerHandle .RepoInfo.Rkey) + "HandleUrl" (printf "https://tangled.org/%s/%s" $repoOwnerHandle .RepoInfo.Slug) "PermaUrl" (printf "https://tangled.org/%s" .RepoInfo.RepoDid) ) }} {{ template "cloneUrlItem" ( dict "Label" "SSH" - "HandleUrl" (printf "git@%s:%s/%s" (stripPort $knot) $repoOwnerHandle .RepoInfo.Rkey) + "HandleUrl" (printf "git@%s:%s/%s" (stripPort $knot) $repoOwnerHandle .RepoInfo.Slug) "PermaUrl" (printf "git@%s:%s" (stripPort $knot) .RepoInfo.RepoDid) ) }} diff --git a/appview/pages/templates/repo/pulls/pull.html b/appview/pages/templates/repo/pulls/pull.html --- a/appview/pages/templates/repo/pulls/pull.html +++ b/appview/pages/templates/repo/pulls/pull.html @@ -412,7 +412,7 @@ {{ $fullRepo := "" }} {{ if and $root.Pull.IsForkBased $root.Pull.PullSource.Repo }} - {{ $fullRepo = printf "%s/%s" $root.Pull.OwnerDid $root.Pull.PullSource.Repo.Rkey }} + {{ $fullRepo = printf "%s/%s" (resolve $root.Pull.PullSource.Repo.Did) $root.Pull.PullSource.Repo.Slug }} {{ else if $root.Pull.IsBranchBased }} {{ $fullRepo = $root.RepoInfo.FullName }} {{ end }} diff --git a/appview/pages/templates/timeline/fragments/preview.html b/appview/pages/templates/timeline/fragments/preview.html --- a/appview/pages/templates/timeline/fragments/preview.html +++ b/appview/pages/templates/timeline/fragments/preview.html @@ -121,14 +121,14 @@ {{ with $source }} {{ $sourceDid := resolve .Did }} forked - + {{ $sourceDid }}/{{ .Name }} to - {{ $repo.Name }} + {{ $repo.Name }} {{ else }} created - + {{ $repo.Name }} {{ end }} @@ -159,7 +159,7 @@ starred {{ end }} {{ template "user/fragments/pic" (list $repoOwnerHandle "size-6") }} - + {{ $repoOwnerHandle | truncateAt30 }}/{{ .Repo.Name }}
diff --git a/appview/pages/templates/timeline/fragments/timeline.html b/appview/pages/templates/timeline/fragments/timeline.html --- a/appview/pages/templates/timeline/fragments/timeline.html +++ b/appview/pages/templates/timeline/fragments/timeline.html @@ -39,14 +39,14 @@ {{ with $source }} {{ $sourceDid := resolve .Did }} forked - + {{ $sourceDid }}/{{ .Name }} to - {{ $repo.Name }} + {{ $repo.Name }} {{ else }} created - + {{ $repo.Name }} {{ end }} @@ -74,7 +74,7 @@ {{ else }} starred {{ end }} - + {{ $repoOwnerHandle | truncateAt30 }}/{{ .Repo.Name }} {{ template "repo/fragments/time" .Created }} diff --git a/appview/pages/templates/user/fragments/issueEvent.html b/appview/pages/templates/user/fragments/issueEvent.html --- a/appview/pages/templates/user/fragments/issueEvent.html +++ b/appview/pages/templates/user/fragments/issueEvent.html @@ -1,6 +1,6 @@ {{ define "user/fragments/issueEvent" }} {{ $repoOwner := resolve .Repo.Did }} - {{ $repoUrl := printf "%s/%s" $repoOwner .Repo.Rkey }} + {{ $repoUrl := printf "%s/%s" $repoOwner .Repo.Slug }} {{ $repoDisplay := printf "%s/%s" $repoOwner .Repo.Name }}
{{ if .Open }} diff --git a/appview/pages/templates/user/fragments/pullEvent.html b/appview/pages/templates/user/fragments/pullEvent.html --- a/appview/pages/templates/user/fragments/pullEvent.html +++ b/appview/pages/templates/user/fragments/pullEvent.html @@ -1,6 +1,6 @@ {{ define "user/fragments/pullEvent" }} {{ $repoOwner := resolve .Repo.Did }} - {{ $repoUrl := printf "%s/%s" $repoOwner .Repo.Rkey }} + {{ $repoUrl := printf "%s/%s" $repoOwner .Repo.Slug }} {{ $repoDisplay := printf "%s/%s" $repoOwner .Repo.Name }}
{{ if .State.IsOpen }} diff --git a/appview/pages/templates/user/fragments/repoCard.html b/appview/pages/templates/user/fragments/repoCard.html --- a/appview/pages/templates/user/fragments/repoCard.html +++ b/appview/pages/templates/user/fragments/repoCard.html @@ -27,9 +27,9 @@ {{ end }} {{ $repoOwner := resolve .Did }} {{- if $fullName -}} - {{ $repoOwner }}/{{ .Name }} + {{ $repoOwner }}/{{ .Name }} {{- else -}} - {{ .Name }} + {{ .Name }} {{- end -}}
{{ if and $starButton $root.LoggedInUser }} diff --git a/appview/pages/templates/repo/pulls/fragments/pullActions.html b/appview/pages/templates/repo/pulls/fragments/pullActions.html --- a/appview/pages/templates/repo/pulls/fragments/pullActions.html +++ b/appview/pages/templates/repo/pulls/fragments/pullActions.html @@ -34,7 +34,7 @@ {{ if .BranchDeleteStatus }}