From 7028a24fbb4730216030edbaed5c49d59d50f1d2 Mon Sep 17 00:00:00 2001 From: Bretton Date: Sat, 1 Aug 2026 18:48:39 -0700 Subject: [PATCH] fix(communities): resolve prefix-free community handles on every path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Community actors are provisioned with a "c-" prefixed handle (c-science.coves.social) that namespaces them apart from user actors on the PDS. Clients display and link to the prefix-free form, so the AppView has to accept both. It did so in ResolveCommunityIdentifier but not in GetCommunity — the lookup backing social.coves.community.get, which is what /c/{handle} actually hits. The result was a community page whose feed loaded (200) while its metadata 404'd. The two paths had drifted because each hand-rolled its own handle lookup. Both now share LookupByHandle, along with the comment service's community filter, which had the same gap and degraded to empty results rather than an error. Also reserves the "c-" namespace at creation. Stripping exactly one leading "c-" is only unambiguous while no name starts with "c-" itself: "c-sharp" would provision c-c-sharp.coves.social, which strips back to c-sharp.coves.social — the stored handle of the *different* community named "sharp", so its own link would resolve to the wrong place. The prefixed retry is skipped for identifiers already starting with "c-", so resolution cannot break the tie either. The same namespace is held back from local user registration, where squatting c-science.coves.social would otherwise block provisioning of the "science" community. Changes: - Add LookupByHandle: exact match, then a "c-" prefixed retry on a genuine miss. Bridged communities are stored unprefixed and resolve on the first lookup, never reaching the retry. - Route GetCommunity, ResolveCommunityIdentifier, and the comment service's community filter through it. - Propagate non-NotFound repository errors instead of reporting them as "community not found", so a database outage no longer looks like a missing community or triggers a second query. - Reject community names starting with "c-" (reserved prefix). - Reject local user handles whose first label starts with "c-", scoped to this instance's domain via the new WithInstanceDomain option. External user indexing is deliberately untouched: a remote c-foo.example.com is in someone else's namespace. - Tests for GetCommunity's handle forms, both reservations, and the over-rejection boundaries (c, csharp, cats, self-host, c2-fast). Deploy note: this must ship before the frontend change that links to prefix-free handles, since older backends reject that form. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/server/wiring.go | 1 + internal/core/comments/comment_service.go | 5 +- internal/core/communities/community.go | 34 +++++ .../communities/get_community_handle_test.go | 118 ++++++++++++++++++ internal/core/communities/service.go | 38 +++--- .../communities/service_reserved_name_test.go | 75 +++++++++++ internal/core/users/reserved_handle_test.go | 99 +++++++++++++++ internal/core/users/service.go | 54 ++++++++ 8 files changed, 401 insertions(+), 23 deletions(-) create mode 100644 internal/core/communities/get_community_handle_test.go create mode 100644 internal/core/communities/service_reserved_name_test.go create mode 100644 internal/core/users/reserved_handle_test.go diff --git a/cmd/server/wiring.go b/cmd/server/wiring.go index 34b7b96..3eb9bbd 100644 --- a/cmd/server/wiring.go +++ b/cmd/server/wiring.go @@ -271,6 +271,7 @@ func (a *application) buildServices(ctx context.Context) error { turnstileVerifier, a.cfg.PDS.AdminPassword, users.WithProfileBackfill(&http.Client{Timeout: profileBackfillTimeout}), + users.WithInstanceDomain(a.cfg.Instance.Domain), ) // The OAuth handler indexes users into the AppView after login, so it diff --git a/internal/core/comments/comment_service.go b/internal/core/comments/comment_service.go index 94aac96..da2e41c 100644 --- a/internal/core/comments/comment_service.go +++ b/internal/core/comments/comment_service.go @@ -1181,8 +1181,9 @@ func (s *commentService) GetActorComments(ctx context.Context, req *GetActorComm if strings.HasPrefix(req.Community, "did:") { communityDID = &req.Community } else { - // It's a handle - resolve to DID via community repository - community, err := s.communityRepo.GetByHandle(ctx, req.Community) + // It's a handle - resolve to DID via community repository. + // LookupByHandle also accepts the prefix-free form clients display. + community, err := communities.LookupByHandle(ctx, s.communityRepo, req.Community) if err != nil { // If community not found, return empty results rather than error // This matches behavior of other endpoints diff --git a/internal/core/communities/community.go b/internal/core/communities/community.go index 798b6cf..f643e80 100644 --- a/internal/core/communities/community.go +++ b/internal/core/communities/community.go @@ -1,6 +1,7 @@ package communities import ( + "context" "fmt" "log" "strings" @@ -15,6 +16,39 @@ import ( // platforms keep their source handle and carry no prefix. const communityHandlePrefix = "c-" +// LookupByHandle fetches a community by handle, retrying with the +// communityHandlePrefix when the bare handle misses. +// +// Communities provisioned on this instance are stored prefixed +// (c-gardening.coves.social) while clients display and link to the prefix-free +// form, so a bare handle must fall back to the prefixed row. Communities +// bridged in from other platforms are stored unprefixed and resolve on the +// first lookup, never reaching the retry. +// +// Errors other than "not found" propagate untouched: a database outage must +// not masquerade as a missing community, and must not trigger a second query. +func LookupByHandle(ctx context.Context, repo Repository, handle string) (*Community, error) { + handle = strings.ToLower(handle) + + community, err := repo.GetByHandle(ctx, handle) + if err == nil { + return community, nil + } + if !IsNotFound(err) || strings.HasPrefix(handle, communityHandlePrefix) { + return nil, err + } + + prefixed, prefixedErr := repo.GetByHandle(ctx, communityHandlePrefix+handle) + if prefixedErr != nil { + if IsNotFound(prefixedErr) { + // Report the miss against the handle the caller actually asked for. + return nil, err + } + return nil, prefixedErr + } + return prefixed, nil +} + // Community represents a Coves community indexed from the firehose // Communities are federated, instance-scoped forums built on atProto type Community struct { diff --git a/internal/core/communities/get_community_handle_test.go b/internal/core/communities/get_community_handle_test.go new file mode 100644 index 0000000..8ac97a1 --- /dev/null +++ b/internal/core/communities/get_community_handle_test.go @@ -0,0 +1,118 @@ +package communities + +import ( + "context" + "errors" + "testing" +) + +// GetCommunity is the endpoint backing social.coves.community.get, the lookup +// clients hit when opening /c/{handle}. It resolves handles independently of +// ResolveCommunityIdentifier, so the prefix fallback has to be exercised here +// too — covering only the resolver left this path 404ing on the prefix-free +// handles clients actually link to. +func TestGetCommunity_HandleForms(t *testing.T) { + local := &Community{DID: "did:plc:local123", Handle: "c-gardening.coves.social", Name: "gardening"} + bridged := &Community{DID: "did:plc:bridged456", Handle: "selfhosted.lemmy-world.tdpl.io", Name: "selfhosted"} + + tests := []struct { + name string + identifier string + wantDID string + wantLookups []string + }{ + { + name: "prefixed handle resolves on the first lookup", + identifier: "c-gardening.coves.social", + wantDID: local.DID, + wantLookups: []string{"c-gardening.coves.social"}, + }, + { + name: "bare handle falls back to the prefixed form", + identifier: "gardening.coves.social", + wantDID: local.DID, + wantLookups: []string{"gardening.coves.social", "c-gardening.coves.social"}, + }, + { + name: "bridged handle resolves without a prefixed retry", + identifier: "selfhosted.lemmy-world.tdpl.io", + wantDID: bridged.DID, + wantLookups: []string{"selfhosted.lemmy-world.tdpl.io"}, + }, + { + name: "at-identifier prefix is stripped before lookup", + identifier: "@gardening.coves.social", + wantDID: local.DID, + wantLookups: []string{"gardening.coves.social", "c-gardening.coves.social"}, + }, + { + name: "handle is lowercased before lookup", + identifier: "Gardening.Coves.Social", + wantDID: local.DID, + wantLookups: []string{"gardening.coves.social", "c-gardening.coves.social"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repo := newStubRepo(local, bridged) + svc := &communityService{repo: repo, instanceDomain: "coves.social"} + + community, err := svc.GetCommunity(context.Background(), tt.identifier) + if err != nil { + t.Fatalf("GetCommunity(%q) returned error: %v", tt.identifier, err) + } + if community.DID != tt.wantDID { + t.Errorf("GetCommunity(%q).DID = %q, want %q", tt.identifier, community.DID, tt.wantDID) + } + if len(repo.handleLookups) != len(tt.wantLookups) { + t.Fatalf("handle lookups = %v, want %v", repo.handleLookups, tt.wantLookups) + } + for i, want := range tt.wantLookups { + if repo.handleLookups[i] != want { + t.Errorf("handle lookup %d = %q, want %q", i, repo.handleLookups[i], want) + } + } + }) + } +} + +func TestGetCommunity_UnknownHandle(t *testing.T) { + repo := newStubRepo() + svc := &communityService{repo: repo, instanceDomain: "coves.social"} + + _, err := svc.GetCommunity(context.Background(), "nope.coves.social") + if !errors.Is(err, ErrCommunityNotFound) { + t.Fatalf("error = %v, want it to wrap ErrCommunityNotFound", err) + } + want := []string{"nope.coves.social", "c-nope.coves.social"} + if len(repo.handleLookups) != len(want) { + t.Fatalf("handle lookups = %v, want %v", repo.handleLookups, want) + } + for i, w := range want { + if repo.handleLookups[i] != w { + t.Errorf("handle lookup %d = %q, want %q", i, repo.handleLookups[i], w) + } + } +} + +// A database failure must surface as itself rather than as a missing community, +// and must not trigger the prefixed retry, which would double the load on an +// already-failing database. +func TestGetCommunity_RepoErrorIsNotSwallowed(t *testing.T) { + dbDown := errors.New("connection refused") + repo := newStubRepo() + repo.getByHandleFn = func(string) (*Community, error) { return nil, dbDown } + svc := &communityService{repo: repo, instanceDomain: "coves.social"} + + _, err := svc.GetCommunity(context.Background(), "gardening.coves.social") + if !errors.Is(err, dbDown) { + t.Fatalf("error = %v, want it to wrap the repository error", err) + } + if errors.Is(err, ErrCommunityNotFound) { + t.Error("repository failure was misreported as ErrCommunityNotFound") + } + if len(repo.handleLookups) != 1 { + t.Errorf("handle lookups = %v, want a single attempt with no prefixed retry", repo.handleLookups) + } +} diff --git a/internal/core/communities/service.go b/internal/core/communities/service.go index 33ce400..8c5d629 100644 --- a/internal/core/communities/service.go +++ b/internal/core/communities/service.go @@ -370,10 +370,14 @@ func (s *communityService) GetCommunity(ctx context.Context, identifier string) // 3. At-identifier format: @handle (strip @ prefix) identifier = strings.TrimPrefix(identifier, "@") - // 4. Canonical handle format: c-name.domain + // 4. Canonical handle format: c-name.domain (also accepts the prefix-free + // form clients display and link to) if strings.Contains(identifier, ".") { - community, err := s.repo.GetByHandle(ctx, strings.ToLower(identifier)) + community, err := LookupByHandle(ctx, s.repo, identifier) if err != nil { + if !IsNotFound(err) { + return nil, fmt.Errorf("failed to look up community %q: %w", originalIdentifier, err) + } return nil, fmt.Errorf("community not found for identifier %q: %w", originalIdentifier, err) } return community, nil @@ -1108,31 +1112,13 @@ func (s *communityService) ResolveCommunityIdentifier(ctx context.Context, ident // 4. Canonical handle: name.community.instance.com (Bluesky standard) if strings.Contains(identifier, ".") { - handle := strings.ToLower(identifier) - - community, err := s.repo.GetByHandle(ctx, handle) + community, err := LookupByHandle(ctx, s.repo, identifier) if err == nil { return community.DID, nil } if !IsNotFound(err) { return "", fmt.Errorf("failed to look up community handle %s: %w", identifier, err) } - - // Communities provisioned on this instance store a "c-" prefixed handle - // (c-gardening.coves.social) that namespaces community actors apart from - // user actors. Clients display and link to the prefix-free form, so retry - // the prefixed handle before giving up. Bridged communities are stored - // without the prefix and resolve on the first lookup above. - if !strings.HasPrefix(handle, communityHandlePrefix) { - community, prefixedErr := s.repo.GetByHandle(ctx, communityHandlePrefix+handle) - if prefixedErr == nil { - return community.DID, nil - } - if !IsNotFound(prefixedErr) { - return "", fmt.Errorf("failed to look up community handle %s: %w", communityHandlePrefix+handle, prefixedErr) - } - } - return "", fmt.Errorf("community not found for handle %s: %w", identifier, err) } @@ -1248,6 +1234,16 @@ func (s *communityService) validateCreateRequest(req CreateCommunityRequest) err return NewValidationError("name", "must contain only alphanumeric characters and hyphens") } + // The "c-" prefix is reserved: it is what namespaces community actors apart + // from user actors, and clients strip exactly one leading "c-" to derive the + // handle they display and link to. A name like "c-sharp" would provision + // c-c-sharp.coves.social, which strips back to c-sharp.coves.social — the + // stored handle of the *different* community named "sharp". Reserving the + // prefix keeps that derivation unambiguous in both directions. + if strings.HasPrefix(strings.ToLower(req.Name), communityHandlePrefix) { + return NewValidationError("name", `must not start with "c-" (reserved prefix for community handles)`) + } + if req.Description != "" && len(req.Description) > 3000 { return NewValidationError("description", "must be 3000 characters or less") } diff --git a/internal/core/communities/service_reserved_name_test.go b/internal/core/communities/service_reserved_name_test.go new file mode 100644 index 0000000..cc110b6 --- /dev/null +++ b/internal/core/communities/service_reserved_name_test.go @@ -0,0 +1,75 @@ +package communities_test + +import ( + "testing" + + "Coves/internal/core/communities" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The "c-" prefix namespaces community actors apart from user actors on the +// PDS, and clients derive the handle they display and link to by stripping +// exactly one leading "c-". That derivation is only unambiguous while no +// community name starts with "c-" itself: +// +// name "sharp" -> stored c-sharp.coves.example -> displayed sharp.coves.example +// name "c-sharp" -> stored c-c-sharp.coves.example -> displayed c-sharp.coves.example +// +// The second display string is the first community's *stored* handle, so +// "c-sharp" would link to "sharp". Resolution cannot break the tie either: the +// prefixed retry is skipped for identifiers already starting with "c-", so the +// URL either resolves to the wrong community or 404s. Reserving the prefix at +// creation is what keeps the mapping one-to-one. +func TestCreateCommunity_ReservesTheCommunityHandlePrefix(t *testing.T) { + t.Parallel() + + reserved := []struct { + name string + reason string + }{ + {"c-sharp", "collides with the display form of the community named \"sharp\""}, + {"c-", "degenerate case: strips to an empty first label"}, + {"c-c-sharp", "nesting the prefix compounds the ambiguity"}, + {"C-Sharp", "the prefix check must not be case-sensitive, since handles are lowercased"}, + } + + for _, tc := range reserved { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + req := aValidCreateRequest() + req.Name = tc.name + + err := requireRejectedBeforeProvisioning(t, req) + var validation *communities.ValidationError + require.ErrorAs(t, err, &validation, tc.reason) + assert.Equal(t, "name", validation.Field) + }) + } +} + +// The reservation must be narrow: it keys off the "c-" prefix, not the letter +// "c", and not a hyphen anywhere in the name. Over-rejecting would refuse names +// clients are entitled to use. +func TestCreateCommunity_AllowsNamesThatMerelyResembleThePrefix(t *testing.T) { + t.Parallel() + + allowed := []string{ + "c", // the bare letter is not the prefix + "csharp", // no hyphen, no prefix + "cats", // starts with "c" but not "c-" + "self-host", // a hyphen elsewhere is fine + "c2-fast", // "c" followed by something other than "-" + } + + for _, name := range allowed { + t.Run(name, func(t *testing.T) { + t.Parallel() + req := aValidCreateRequest() + req.Name = name + + requireAcceptedBy(t, req, "name") + }) + } +} diff --git a/internal/core/users/reserved_handle_test.go b/internal/core/users/reserved_handle_test.go new file mode 100644 index 0000000..76ed19d --- /dev/null +++ b/internal/core/users/reserved_handle_test.go @@ -0,0 +1,99 @@ +package users + +import ( + "errors" + "testing" +) + +const testInstanceDomain = "coves.social" + +// Community actors on this instance are provisioned as c-{name}.{domain}. A +// user who registers c-gardening.coves.social therefore either blocks +// provisioning of the "gardening" community (PDS handle uniqueness) or holds +// the handle the AppView treats as that community's identity. Registration on +// this instance has to hold that namespace back. +func TestValidateLocalHandleNamespace_ReservesTheCommunityPrefixOnThisInstance(t *testing.T) { + t.Parallel() + + svc := &userService{instanceDomain: testInstanceDomain} + + reserved := []string{ + "c-gardening.coves.social", + "c-.coves.social", + "C-Gardening.Coves.Social", // handles are case-insensitive + } + + for _, handle := range reserved { + t.Run(handle, func(t *testing.T) { + t.Parallel() + + err := svc.validateLocalHandleNamespace(handle) + + var invalid *InvalidHandleError + if !errors.As(err, &invalid) { + t.Fatalf("validateLocalHandleNamespace(%q) = %v, want InvalidHandleError", handle, err) + } + }) + } +} + +// The reservation is scoped to OUR domain. A remote actor legitimately named +// c-foo.example.com lives in someone else's namespace; rejecting it would make +// that user un-indexable here for no security benefit. +func TestValidateLocalHandleNamespace_LeavesOtherNamespacesAlone(t *testing.T) { + t.Parallel() + + svc := &userService{instanceDomain: testInstanceDomain} + + allowed := []struct { + handle string + why string + }{ + {"c-foo.example.com", "another instance's namespace is not ours to police"}, + {"gardening.coves.social", "an ordinary local handle"}, + {"csharp.coves.social", "starts with \"c\" but not the \"c-\" prefix"}, + {"self-host.coves.social", "a hyphen elsewhere in the label is fine"}, + {"c-gardening.notcoves.social", "the suffix must match at a label boundary, not mid-label"}, + } + + for _, tc := range allowed { + t.Run(tc.handle, func(t *testing.T) { + t.Parallel() + + if err := svc.validateLocalHandleNamespace(tc.handle); err != nil { + t.Errorf("validateLocalHandleNamespace(%q) = %v, want nil (%s)", tc.handle, err, tc.why) + } + }) + } +} + +// Without a configured instance domain there is no namespace to defend, and +// guessing one would reject handles on instances that never had the convention. +func TestValidateLocalHandleNamespace_DisabledWithoutAnInstanceDomain(t *testing.T) { + t.Parallel() + + svc := &userService{} + + if err := svc.validateLocalHandleNamespace("c-gardening.coves.social"); err != nil { + t.Errorf("with no instance domain configured the check must be inert, got %v", err) + } +} + +// The check has to run on the registration path specifically — that is where an +// actor is created on this instance. +func TestValidateRegisterRequest_RejectsReservedCommunityHandles(t *testing.T) { + t.Parallel() + + svc := &userService{instanceDomain: testInstanceDomain} + + err := svc.validateRegisterRequest(RegisterAccountRequest{ + Handle: "c-gardening.coves.social", + Email: "someone@example.com", + Password: "a-sufficiently-long-password", + }) + + var invalid *InvalidHandleError + if !errors.As(err, &invalid) { + t.Fatalf("registration with a reserved handle = %v, want InvalidHandleError", err) + } +} diff --git a/internal/core/users/service.go b/internal/core/users/service.go index 1dbdebc..de0b035 100644 --- a/internal/core/users/service.go +++ b/internal/core/users/service.go @@ -58,6 +58,11 @@ type userService struct { identityResolver identity.Resolver defaultPDS string // Default PDS URL for this Coves instance (used when creating new local users via registration API) + // instanceDomain is this instance's handle domain (e.g. coves.social), used + // to reserve the "c-" community namespace against local registrations. + // Empty disables the check. See validateLocalHandleNamespace. + instanceDomain string + // turnstile verifies Cloudflare Turnstile tokens during the signup-token // handshake. nil → RequestSignupToken returns ErrSignupTokenDisabled (503). turnstile TurnstileVerifier @@ -96,6 +101,46 @@ func WithProfileBackfill(client *http.Client) UserServiceOption { } } +// WithInstanceDomain supplies this instance's handle domain (e.g. coves.social) +// so local registrations can be held out of the reserved community namespace. +// Empty (the default) disables the reservation check. +func WithInstanceDomain(domain string) UserServiceOption { + return func(s *userService) { + s.instanceDomain = strings.ToLower(strings.TrimSpace(domain)) + } +} + +// communityHandlePrefix mirrors the communities package's reserved prefix. +// Duplicated rather than imported to keep users from depending on communities; +// the two must stay in sync. +const communityHandlePrefix = "c-" + +// validateLocalHandleNamespace rejects handles that squat the community +// namespace on THIS instance. Community actors are provisioned as +// c-{name}.{instanceDomain}, so a user holding c-gardening.coves.social could +// either block provisioning of the "gardening" community or hold the handle the +// AppView treats as that community's identity. +// +// Scoped deliberately to our own domain: a remote user legitimately named +// c-foo.example.com is in someone else's namespace and must still index fine. +func (s *userService) validateLocalHandleNamespace(handle string) error { + if s.instanceDomain == "" { + return nil + } + handle = strings.ToLower(strings.TrimSpace(handle)) + if !strings.HasSuffix(handle, "."+s.instanceDomain) { + return nil + } + firstLabel, _, _ := strings.Cut(handle, ".") + if strings.HasPrefix(firstLabel, communityHandlePrefix) { + return &InvalidHandleError{ + Handle: handle, + Reason: `handles starting with "c-" are reserved for communities on this instance`, + } + } + return nil +} + // NewUserService creates a new user service. // turnstile and pdsAdminPassword may be nil/empty when the signup-token endpoint // is not enabled (e.g., integration tests that don't exercise bot protection); in @@ -194,6 +239,9 @@ func (s *userService) UpdateHandle(ctx context.Context, did, newHandle string) ( if err := validateHandle(newHandle); err != nil { return nil, err } + if err := s.validateLocalHandleNamespace(newHandle); err != nil { + return nil, err + } return s.userRepo.UpdateHandle(ctx, did, newHandle) } @@ -612,6 +660,12 @@ func (s *userService) validateRegisterRequest(req RegisterAccountRequest) error if err := validateHandle(req.Handle); err != nil { return err } + // Registration creates an actor on THIS instance, so it must not squat the + // community namespace. Deliberately not applied to user indexing, which + // ingests remote actors whose namespaces are not ours to police. + if err := s.validateLocalHandleNamespace(req.Handle); err != nil { + return err + } return nil } -- 2.51.2