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 }