From 4f0d0c17e3dce7df5db186cce1fe1aff36c30e0c Mon Sep 17 00:00:00 2001 From: Bretton Date: Thu, 13 Aug 2026 01:06:37 -0700 Subject: [PATCH] =?UTF-8?q?fix(task13):=20second-opinion=20fix=20wave=20?= =?UTF-8?q?=E2=80=94=20origin=20canonicalization,=20host=20bindings,=20har?= =?UTF-8?q?dening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Criticals (pinned test-first): one shared origin canonicalizer (path/userinfo/query rejected; :443/:80 + trailing dot stripped; https required in prod; shadow check on canonical forms — closes the tdpl.io:443 bypass and the :443 webfinger-404s-forever hazard); actor docs/outboxes bound to their own origin (cross-DID URL-spelling webfinger pinned at both defense layers); public-IP Hosts 421 in prod (loopback-only service carve-out); Recoverer/RequestID wrap the host router. Hardening: personas logging threaded (500s, sampled 421s, exhaustion); malformed stored actor_id fails closed; ErrLocalPartExhausted sentinel (not IsAlreadyExists); truncation trims trailing separators; suffix normalization; nodeinfo identity hoisted to one shared constant; staged-contract label on federation.json; doc corrections. Co-Authored-By: Claude Fable 5 --- README.md | 6 +- cmd/tidepool/main.go | 8 +- internal/ap/service_actor.go | 10 +++ internal/config/config.go | 59 +++++++------ internal/config/config_test.go | 98 +++++++++++++++++++++ internal/identity/actor_rsa.go | 5 +- internal/identity/keys.go | 17 ++-- internal/ingest/inbox.go | 11 +-- internal/personas/create_test.go | 71 ++++++++++++++++ internal/personas/hostrouter.go | 55 ++++++++++-- internal/personas/hostrouter_test.go | 116 ++++++++++++++++++++++++- internal/personas/instance.go | 10 +-- internal/personas/localpart.go | 13 ++- internal/personas/localpart_test.go | 65 ++++++++++++++ internal/personas/origin.go | 69 +++++++++++++++ internal/personas/personas.go | 67 ++++++++++----- internal/personas/serving.go | 69 ++++++++++++--- internal/personas/serving_test.go | 123 +++++++++++++++++++++++++++ 18 files changed, 778 insertions(+), 94 deletions(-) create mode 100644 internal/personas/origin.go diff --git a/README.md b/README.md index 8d7877f..7fe27be 100644 --- a/README.md +++ b/README.md @@ -500,7 +500,11 @@ and is versioned by nsid: breaking changes ship under a new name. Its sibling under the same Tidepool-owned namespace is [`lexicons/social/coves/bridge/federation.json`](lexicons/social/coves/bridge/federation.json) (`key: literal:self`, one record per repo), the user-facing federation -preference. It is an **opt-OUT**: federation is on by default, so the record's +preference. **Staged contract:** the lexicon is published so Coves' settings +UI can write against a stable shape — the bridge does not read it yet. +Enforcement (honoring `enabled: false`, and the `deleteRemote` tier) lands +with the task-14 consumer; until then the record is inert and federation is +on for every minted actor. It is an **opt-OUT**: federation is on by default, so the record's ABSENCE means enabled and it only ever exists to turn federation down. `enabled: false` is a soft disable — the actor stops resolving via WebFinger and stops delivering, while its actor document and already-federated diff --git a/cmd/tidepool/main.go b/cmd/tidepool/main.go index aad83e8..ecc98fe 100644 --- a/cmd/tidepool/main.go +++ b/cmd/tidepool/main.go @@ -482,9 +482,15 @@ func run(logger *slog.Logger) error { return fmt.Errorf("host router: %w", err) } + // The host router runs OUTSIDE the chi router, so chi's middleware no + // longer covers the user origin's requests. The two that must apply to + // every request on this listener are re-applied here in the same order + // chi chains them (RequestID first, so a panic recovered below is logged + // with one): without Recoverer a panic in the user surface would kill the + // whole process, taking the bridge down with it. server := &http.Server{ Addr: cfg.ListenAddr, - Handler: hostRouter, + Handler: middleware.RequestID(middleware.Recoverer(hostRouter)), ReadHeaderTimeout: readHeaderTimeout, WriteTimeout: writeTimeout, IdleTimeout: idleTimeout, diff --git a/internal/ap/service_actor.go b/internal/ap/service_actor.go index 583dd6f..5ab8f23 100644 --- a/internal/ap/service_actor.go +++ b/internal/ap/service_actor.go @@ -19,6 +19,16 @@ const ServiceKeyName = "service-actor" // from (the HTTP route itself lands with the inbox in task 06). const ServiceActorPath = "/actor" +// SoftwareName and SoftwareVersion identify this implementation in every +// nodeinfo document the deployment serves. Lemmy admins allowlist by the +// exact name string, and both of the bridge's origins describe the SAME +// deployment — so the pair lives here rather than once per surface, where +// the two could drift into claiming to be different software. +const ( + SoftwareName = "tidepool" + SoftwareVersion = "0.1.0" +) + // serviceActorContext is the JSON-LD context for the service actor document: // core AS2 plus the security vocabulary that defines publicKey. const serviceActorContext = `["https://www.w3.org/ns/activitystreams","https://w3id.org/security/v1"]` diff --git a/internal/config/config.go b/internal/config/config.go index 7ff693b..2d5ded3 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -8,11 +8,12 @@ import ( "encoding/hex" "fmt" "log/slog" - "net/url" "os" "strconv" "strings" "time" + + "tidepool/internal/personas" ) const ( @@ -351,11 +352,12 @@ func Load(logger *slog.Logger) (*Config, error) { // actor_id this deployment mints, so a wrong or missing value is not a // runtime inconvenience but a set of federated identities pointing at // the wrong place, forever. - cfg.APUserOrigin, err = stringVar(logger, isDevelopment, "AP_USER_ORIGIN", "http://localhost:8091") + rawUserOrigin, err := stringVar(logger, isDevelopment, "AP_USER_ORIGIN", "http://localhost:8091") if err != nil { return nil, err } - if err := validateUserOrigin(cfg.APUserOrigin, cfg.BridgeHostname); err != nil { + cfg.APUserOrigin, err = validateUserOrigin(rawUserOrigin, cfg.BridgeHostname, isDevelopment) + if err != nil { return nil, err } @@ -586,30 +588,39 @@ func boolVarDefault(logger *slog.Logger, name string, fallback bool) (bool, erro return false, fmt.Errorf("config: %s must be a boolean (1/0, true/false, yes/no, on/off), got %q", name, raw) } -// validateUserOrigin refuses a user origin that would shadow the bridge's own -// handle namespace. Bridged handles are subdomains of BRIDGE_HOSTNAME resolved -// off r.Host, so a user origin AT that name or UNDER it would swallow them — -// and the Host router could not tell the two surfaces apart in the first -// place. The comparison is on host:port, because a different port is a -// different authority: the dev defaults are exactly that shape -// (BRIDGE_HOSTNAME localhost, user origin on :8091). Matching is on a label -// boundary, so "nottidepool.example" is not under "tidepool.example". -func validateUserOrigin(origin, bridgeHostname string) error { - parsed, err := url.Parse(origin) - if err != nil { - return fmt.Errorf("config: AP_USER_ORIGIN must be an absolute origin URL, got %q: %w", origin, err) - } - if parsed.Scheme == "" || parsed.Host == "" { - return fmt.Errorf("config: AP_USER_ORIGIN must be an absolute origin URL "+ - "(scheme and host), got %q", origin) - } - host := strings.ToLower(parsed.Host) - bridge := strings.ToLower(strings.TrimSpace(bridgeHostname)) +// validateUserOrigin canonicalizes the user origin and refuses one that would +// shadow the bridge's own handle namespace, returning the CANONICAL origin — +// the value every minted actor_id is built from, so the canonicalization has +// to happen once, here, rather than at each use site. +// +// Bridged handles are subdomains of BRIDGE_HOSTNAME resolved off r.Host, so a +// user origin AT that name or UNDER it would swallow them — and the Host +// router could not tell the two surfaces apart in the first place. The +// comparison runs on canonical forms in both directions, or a second spelling +// of BRIDGE_HOSTNAME ("https://TDPL.IO:443") walks straight past the check. +// Matching is on a label boundary, so "nottidepool.example" is not under +// "tidepool.example", and a different port is a different authority (the dev +// defaults are exactly that shape: BRIDGE_HOSTNAME localhost, origin on +// :8091). +func validateUserOrigin(origin, bridgeHostname string, isDevelopment bool) (string, error) { + canonical, host, err := personas.CanonicalizeOrigin(origin) + if err != nil { + return "", fmt.Errorf("config: AP_USER_ORIGIN: %w", err) + } + // http publishes actor ids peers fetch in plaintext, carrying signature + // verification over an unauthenticated channel. It exists for the same + // reason BRIDGE_SCHEME=http does — the local e2e harness — and is + // refused outside development for the same reason. + if !isDevelopment && !strings.HasPrefix(canonical, "https://") { + return "", fmt.Errorf("config: AP_USER_ORIGIN must be https in production, got %q", canonical) + } + + bridge := strings.TrimSuffix(strings.ToLower(strings.TrimSpace(bridgeHostname)), ".") if host == bridge || strings.HasSuffix(host, "."+bridge) { - return fmt.Errorf("config: AP_USER_ORIGIN host %q must not be BRIDGE_HOSTNAME %q "+ + return "", fmt.Errorf("config: AP_USER_ORIGIN host %q must not be BRIDGE_HOSTNAME %q "+ "or a subdomain of it: the bridged handle namespace lives there", host, bridge) } - return nil + return canonical, nil } // stringVar returns the value of an environment variable. When unset it diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 0305708..25b6cd1 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -336,3 +336,101 @@ func TestLoad_ProductionDefaultsFallthroughOff(t *testing.T) { require.NoError(t, err) assert.False(t, cfg.APHostFallthroughDev, "production never falls through") } + +// TestLoad_APUserOriginMustBeBareOrigin: the value is concatenated into +// every minted actor_id, keyId, and webfinger href. Anything past +// scheme://host — a trailing slash, a path, a query, a fragment, userinfo — +// is silently baked into identities that are FROZEN once minted, so it has +// to be refused at startup rather than discovered in a peer's parser. +func TestLoad_APUserOriginMustBeBareOrigin(t *testing.T) { + for _, origin := range []string{ + "https://coves.social/", + "https://coves.social/ap", + "https://coves.social/ap/", + "https://coves.social?x=1", + "https://coves.social/#frag", + "https://coves.social#frag", + "https://user@coves.social", + "https://user:pass@coves.social", + } { + t.Run(origin, func(t *testing.T) { + clearConfigEnv(t) + t.Setenv("BRIDGE_HOSTNAME", "tidepool.example") + t.Setenv("AP_USER_ORIGIN", origin) + + _, err := Load(discardLogger()) + require.Error(t, err, "%q is not a bare origin", origin) + assert.Contains(t, err.Error(), "AP_USER_ORIGIN") + }) + } +} + +// TestLoad_APUserOriginCanonicalized: two spellings of one authority must +// not mint two namespaces. The stored value is what actor_ids are built +// from, so canonicalization happens ONCE, here. +func TestLoad_APUserOriginCanonicalized(t *testing.T) { + for _, tc := range []struct{ raw, want string }{ + {"https://coves.social:443", "https://coves.social"}, + {"http://coves.social:80", "http://coves.social"}, + {"HTTPS://Coves.Social", "https://coves.social"}, + {"https://coves.social.", "https://coves.social"}, + {"https://coves.social", "https://coves.social"}, + // A non-default port is part of the authority and must survive. + {"http://localhost:8091", "http://localhost:8091"}, + } { + t.Run(tc.raw, func(t *testing.T) { + clearConfigEnv(t) + t.Setenv("BRIDGE_HOSTNAME", "tidepool.example") + t.Setenv("AP_USER_ORIGIN", tc.raw) + + cfg, err := Load(discardLogger()) + require.NoError(t, err) + assert.Equal(t, tc.want, cfg.APUserOrigin, + "the canonical origin is what every minted actor_id carries") + }) + } +} + +// TestLoad_APUserOriginRequiresHTTPSInProduction: an http origin in +// production would publish actor ids that peers fetch in plaintext, and +// signature verification would carry over an unauthenticated channel. +func TestLoad_APUserOriginRequiresHTTPSInProduction(t *testing.T) { + setProductionEnv(t) + t.Setenv("AP_USER_ORIGIN", "http://coves.social") + + _, err := Load(discardLogger()) + require.Error(t, err) + assert.Contains(t, err.Error(), "AP_USER_ORIGIN") + + // Development still federates with a plain-HTTP Lemmy in the compose + // network, so http stays legal there. + clearConfigEnv(t) + t.Setenv("BRIDGE_HOSTNAME", "tidepool.example") + t.Setenv("AP_USER_ORIGIN", "http://coves.social") + cfg, err := Load(discardLogger()) + require.NoError(t, err) + assert.Equal(t, "http://coves.social", cfg.APUserOrigin) +} + +// TestLoad_APUserOriginShadowCheckIsCanonical: the shadow check must run on +// the canonical host, or a second spelling of BRIDGE_HOSTNAME walks straight +// past it and takes over the bridged-handle namespace. +func TestLoad_APUserOriginShadowCheckIsCanonical(t *testing.T) { + for _, origin := range []string{ + "https://tdpl.io:443", + "https://tdpl.io.", + "https://TDPL.IO", + "https://users.tdpl.io:443", + "https://users.TDPL.io.", + } { + t.Run(origin, func(t *testing.T) { + clearConfigEnv(t) + t.Setenv("BRIDGE_HOSTNAME", "tdpl.io") + t.Setenv("AP_USER_ORIGIN", origin) + + _, err := Load(discardLogger()) + require.Error(t, err, "%q is BRIDGE_HOSTNAME wearing a different spelling", origin) + assert.Contains(t, err.Error(), "AP_USER_ORIGIN") + }) + } +} diff --git a/internal/identity/actor_rsa.go b/internal/identity/actor_rsa.go index 0459e12..64f1b5f 100644 --- a/internal/identity/actor_rsa.go +++ b/internal/identity/actor_rsa.go @@ -12,7 +12,10 @@ import ( // RSA private keys sealed under the bridge KEK, AAD-bound to their DID with // a constant DISTINCT from actorKeyAADPrefix (the K256 escrow keys), so a // ciphertext moved between columns fails to open. New keys never touch the -// plaintext-PEM path the v1 service-actor key still uses (keys.go:147). +// plaintext-PEM path the v1 SERVICE-actor key still travels — that key is +// PEM-encoded by ap.EncodePrivateKeyPEM and stored in service_keys as text +// (see ap.LoadOrCreateServiceActor); every per-actor AP key minted here is +// ciphertext at rest instead. // // The sealed plaintext is PKCS#8 DER, not PEM: PEM is base64 with a header, // so it would be ~40% larger and would put the string "-----BEGIN" inside a diff --git a/internal/identity/keys.go b/internal/identity/keys.go index 35ee245..1f8e28f 100644 --- a/internal/identity/keys.go +++ b/internal/identity/keys.go @@ -46,11 +46,18 @@ const ( rotationKeyAAD = "tidepool:plc-rotation-key:v1" ) -// Custodian seals and opens per-actor secp256k1 private keys with the -// bridge KEK (AES-256-GCM). Key claiming/migration is out of scope for v1, -// but the storage design allows it later: each actor's key is independent, -// bound to its DID via AAD, and exportable by decrypting and handing the -// key material to the user during a future claim flow. +// Custodian seals and opens the bridge's two per-actor key domains with the +// bridge KEK (AES-256-GCM): the atproto secp256k1 escrow keys of bridged +// fediverse actors (EncryptActorKey/DecryptActorKey) and the ActivityPub RSA +// keys of Coves users' own actors (EncryptActorRSAKey/DecryptActorRSAKey, in +// actor_rsa.go). One KEK, two AAD prefixes: a ciphertext from one domain +// must never open in the other, so the constants above are distinct by +// construction and a cross test pins it. +// +// Key claiming/migration is out of scope for v1, but the storage design +// allows it later: each actor's key is independent, bound to its DID via AAD, +// and exportable by decrypting and handing the key material to the user +// during a future claim flow. type Custodian struct { // aead is built once at construction and reused: DecryptActorKey sits // on every commit's hot path, and cipher.AEAD is safe for concurrent diff --git a/internal/ingest/inbox.go b/internal/ingest/inbox.go index 8ea4230..c05f7ef 100644 --- a/internal/ingest/inbox.go +++ b/internal/ingest/inbox.go @@ -74,13 +74,6 @@ const ( defaultTombstoneConfirmBurst = 10 ) -// softwareName is what nodeinfo reports; Lemmy admins allowlist by this -// name. -const ( - softwareName = "tidepool" - softwareVersion = "0.1.0" -) - // ActorFetcher fetches an AP actor document by IRI — the slice of // *ap.Client the inbox needs to confirm an actor tombstone (see // tombstonedSelfDelete). The same-authority variant is required because that @@ -532,8 +525,8 @@ func (ib *Inbox) handleNodeInfo(w http.ResponseWriter, _ *http.Request) { _ = json.NewEncoder(w).Encode(map[string]any{ "version": "2.0", "software": map[string]any{ - "name": softwareName, - "version": softwareVersion, + "name": ap.SoftwareName, + "version": ap.SoftwareVersion, }, "protocols": []any{"activitypub"}, "services": map[string]any{"inbound": []any{}, "outbound": []any{}}, diff --git a/internal/personas/create_test.go b/internal/personas/create_test.go index 2f07cc5..b896b33 100644 --- a/internal/personas/create_test.go +++ b/internal/personas/create_test.go @@ -5,6 +5,8 @@ import ( "context" "crypto/rsa" "database/sql" + stderrors "errors" + "fmt" "net/http" "strings" "sync" @@ -308,3 +310,72 @@ func countActors(t *testing.T, database *sql.DB, did string) int { `SELECT count(*) FROM ap_actors WHERE did = $1`, did).Scan(&count)) return count } + +// TestNew_CanonicalizesUserOrigin: config canonicalizes AP_USER_ORIGIN, but +// personas.New is also constructed directly (tests, future callers), so it +// normalizes defensively. A ":443" that slipped through would mint actors +// under a SECOND namespace — normalized_origin "coves.social:443" — that +// Host routing, which sees the canonical authority, could never resolve. +func TestNew_CanonicalizesUserOrigin(t *testing.T) { + database := personasTestDB(t) + custodian, err := identity.NewCustodian(testKEK) + require.NoError(t, err) + + svc, err := New(Options{ + DB: database, + Custodian: custodian, + UserOrigin: "https://coves.social:443", + }) + require.NoError(t, err) + require.NotNil(t, svc) + + did := testDID(t) + actor, err := svc.CreateActorForDID(t.Context(), did, testHandle) + require.NoError(t, err) + require.NotNil(t, actor) + + assert.Equal(t, userHost, actor.NormalizedOrigin, + "the default https port is not part of the authority webfinger routes on") + assert.Equal(t, userOrigin+"/ap/actor/"+did, actor.ActorID, + "the actor id must carry the canonical origin: it is frozen at mint") +} + +// TestCreateActorForDID_NamespaceExhaustion: the suffix search is bounded, +// and the error at the end must not read as a uniqueness conflict. A caller +// treating IsAlreadyExists as "someone else won, re-read the row" would +// spin forever on a name that has no free suffix left. +func TestCreateActorForDID_NamespaceExhaustion(t *testing.T) { + database := personasTestDB(t) + svc, _ := newTestService(t, database) + actors := store.NewAPActors(database) + ctx := t.Context() + + // Seed the whole suffix range directly: alice, alice-2 ... alice-99. + // Going through CreateActorForDID would generate 99 RSA keys for no + // added coverage. + for attempt := 1; attempt <= 99; attempt++ { + local := testLocalPart + if attempt > 1 { + local = fmt.Sprintf("%s-%d", testLocalPart, attempt) + } + did := testDID(t) + _, err := actors.Create(ctx, store.APActor{ + DID: did, + Kind: store.ActorTypePerson, + ActorID: userOrigin + "/ap/actor/" + did, + NormalizedOrigin: userHost, + LocalPart: local, + RSAKeySealed: []byte{0x01, 0x02, 0x03}, + RSAKeyVersion: 1, + PublicKeyPEM: "seeded", + }) + require.NoError(t, err, "seed %q", local) + } + + _, err := svc.CreateActorForDID(ctx, testDID(t), testHandle) + require.Error(t, err, "the 100th claimant of one name has nowhere to go") + assert.True(t, stderrors.Is(err, ErrLocalPartExhausted), + "exhaustion is a branchable condition, not a message to grep: got %v", err) + assert.False(t, errors.IsAlreadyExists(err), + "exhaustion is not a conflict a caller can resolve by re-reading: got %v", err) +} diff --git a/internal/personas/hostrouter.go b/internal/personas/hostrouter.go index 82b2053..a42901a 100644 --- a/internal/personas/hostrouter.go +++ b/internal/personas/hostrouter.go @@ -2,13 +2,21 @@ package personas import ( "bytes" + "log/slog" "net" "net/http" "strings" + "time" "tidepool/internal/errors" + "tidepool/internal/ratelimit" ) +// misdirectedLogInterval throttles the 421 refusal log: a scanner sweeping +// Hosts would otherwise write one line per probe, and the interesting signal +// is "this is happening at all", not each instance. +const misdirectedLogInterval = time.Second + // HostRouterOptions configures NewHostRouter. type HostRouterOptions struct { // ServiceHost is BRIDGE_HOSTNAME: the bridge's own surface, including @@ -22,6 +30,9 @@ type HostRouterOptions struct { // refusing them. A laptop is reached by IP, tunnel hostname, or whatever // the tunnel minted this morning; a production deployment is not. DevFallthrough bool + // Logger receives a sampled warning for refused Hosts. Nil uses + // slog.Default(). + Logger *slog.Logger } // NewHostRouter splits one listener between the bridge's service surface and @@ -50,12 +61,18 @@ func NewHostRouter(opts HostRouterOptions) (http.Handler, error) { return nil, errors.NewValidationError("user_handler", "must not be nil") } + logger := opts.Logger + if logger == nil { + logger = slog.Default() + } return &hostRouter{ serviceHost: normalizeHost(opts.ServiceHost), serviceHandler: opts.ServiceHandler, userHost: normalizeHost(opts.UserHost), userHandler: opts.UserHandler, devFallthrough: opts.DevFallthrough, + logger: logger, + refusalLog: ratelimit.NewSampler(misdirectedLogInterval), }, nil } @@ -65,6 +82,8 @@ type hostRouter struct { userHost string userHandler http.Handler devFallthrough bool + logger *slog.Logger + refusalLog *ratelimit.Sampler } func (h *hostRouter) ServeHTTP(w http.ResponseWriter, r *http.Request) { @@ -87,21 +106,35 @@ func (h *hostRouter) ServeHTTP(w http.ResponseWriter, r *http.Request) { case h.devFallthrough: h.serviceHandler.ServeHTTP(w, r) default: + if h.refusalLog.Allow(time.Now()) { + h.logger.Warn("refused request for an unrecognized Host (sampled)", + "host", host, "path", r.URL.Path) + } http.Error(w, "unrecognized Host", http.StatusMisdirectedRequest) } } // isServiceHost reports whether host belongs to the bridge's own surface: // the configured hostname, any subdomain of it (954 bridged handles resolve -// through those), or an address with no registered name at all — an absent -// Host, "localhost", or a bare IP literal, which is how container -// healthchecks and direct-IP probes arrive. +// through those), or a LOOPBACK address — an absent Host, "localhost", or +// 127.0.0.1/::1, which is how container healthchecks and local probes arrive. +// +// A PUBLIC IP literal is deliberately not in the bucket. It names no +// configured surface, and admitting it would hand an attacker a way to reach +// the service surface directly by address, bypassing whatever the proxy +// enforces per-name. Dev fallthrough still admits it — a dev box IS reached +// by its address — which is the whole reason that flag is refused in +// production. func (h *hostRouter) isServiceHost(host string) bool { if host == "" || host == h.serviceHost || strings.HasSuffix(host, "."+h.serviceHost) { return true } name := hostnameOnly(host) - return name == "localhost" || net.ParseIP(name) != nil + if name == "localhost" { + return true + } + ip := net.ParseIP(name) + return ip != nil && ip.IsLoopback() } // serveComposed runs the user surface first and replaces its 404 with the @@ -109,6 +142,14 @@ func (h *hostRouter) isServiceHost(host string) bool { // streamed: once a status line has reached the client there is no taking it // back, so a fallback would append its body to the 404 instead of replacing // it. +// +// INVARIANT: the user surface must not read the request body on any path it +// 404s. The replay hands the SAME *http.Request to the service handler, and a +// consumed body cannot be rewound — the service handler would see an empty +// one. personas.Service satisfies this (only POST /ap/inbox reads a body, and +// it never 404s after reading); a future handler that reads before deciding +// would break the fallback silently, in the direction of an inbox that +// accepts empty deliveries. func (h *hostRouter) serveComposed(w http.ResponseWriter, r *http.Request) { buffered := &bufferedResponse{header: http.Header{}} h.userHandler.ServeHTTP(buffered, r) @@ -123,7 +164,11 @@ func (h *hostRouter) serveComposed(w http.ResponseWriter, r *http.Request) { } // bufferedResponse captures a handler's response so the caller can decide -// whether to send it. +// whether to send it. It implements http.ResponseWriter and nothing else: +// no Flusher, no Hijacker, no ReaderFrom. Composition only happens when both +// configured hosts name one authority — the dev default — and nothing on the +// user surface streams, flushes, or upgrades. A future streaming route on a +// composed listener would need this to forward those interfaces. type bufferedResponse struct { header http.Header code int diff --git a/internal/personas/hostrouter_test.go b/internal/personas/hostrouter_test.go index 7783434..7df25e3 100644 --- a/internal/personas/hostrouter_test.go +++ b/internal/personas/hostrouter_test.go @@ -20,6 +20,9 @@ type marker struct { calls int hosts []string notFound map[string]bool + // statuses overrides the answer for a path, so a test can prove which + // statuses the composed handler treats as "not mine". + statuses map[string]int } func newMarker(name string, notFound ...string) *marker { @@ -37,6 +40,12 @@ func (m *marker) ServeHTTP(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) return } + if status, ok := m.statuses[r.URL.Path]; ok { + w.Header().Set("X-Handler", m.name) + w.WriteHeader(status) + _, _ = w.Write([]byte(m.name + " " + r.URL.Path)) + return + } w.Header().Set("X-Handler", m.name) w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte(m.name + " " + r.URL.Path)) @@ -93,9 +102,14 @@ func TestHostRouter_ServiceBucket(t *testing.T) { {"localhost with port", "localhost:80"}, {"loopback v4", "127.0.0.1:8080"}, {"loopback v6", "[::1]:8080"}, - {"public IP literal", "192.0.2.10"}, - {"IPv6 literal", "[2001:db8::1]:443"}, {"absent Host", ""}, + // PUBLIC IP literals used to live here. The review narrowed the + // address rule to LOOPBACK only: a bare public address names no + // configured surface, and admitting it let anyone reaching the + // process directly bypass whatever the proxy enforces per-name. + // They are now refused in production — see + // TestHostRouter_RejectsPublicIPLiterals, which also pins that dev + // fallthrough still keeps them on the service bucket. } for _, tc := range serviceHosts { @@ -330,3 +344,101 @@ func TestNewHostRouter_RequiresHandlers(t *testing.T) { }) assert.Error(t, err, "a router without a service host cannot classify anything") } + +// TestHostRouter_RejectsPublicIPLiterals: a bare IP Host has no registered +// name behind it, so it cannot be the user origin — but it can absolutely be +// an attacker probing the service surface directly, bypassing whatever the +// proxy enforces per-name. Loopback is the exception that must keep working: +// container healthchecks and local probes arrive that way. +func TestHostRouter_RejectsPublicIPLiterals(t *testing.T) { + for _, tc := range []struct { + name string + host string + }{ + {"public IPv4 literal", "192.0.2.10"}, + {"public IPv4 literal with port", "192.0.2.10:8091"}, + {"public IPv6 literal", "[2001:db8::1]:8080"}, + } { + t.Run(tc.name, func(t *testing.T) { + router, service, user := newTestRouter(t, false) + rec := routeHost(t, router, "https", tc.host, "/healthz") + assert.Equal(t, http.StatusMisdirectedRequest, rec.Code, + "a public IP Host names no configured surface: %q", tc.host) + assert.Zero(t, service.calls) + assert.Zero(t, user.calls) + }) + } + + for _, tc := range []struct { + name string + host string + }{ + {"loopback v4", "127.0.0.1:8091"}, + {"loopback v6", "[::1]"}, + {"loopback name", "localhost"}, + } { + t.Run(tc.name, func(t *testing.T) { + router, service, _ := newTestRouter(t, false) + rec := routeHost(t, router, "http", tc.host, "/healthz") + assert.Equal(t, http.StatusOK, rec.Code, + "loopback is how healthchecks and local probes arrive; body=%s", rec.Body.String()) + assert.Equal(t, 1, service.calls) + }) + } + + t.Run("dev fallthrough keeps public IPs on the service surface", func(t *testing.T) { + router, service, _ := newTestRouter(t, true) + rec := routeHost(t, router, "http", "192.0.2.10", "/healthz") + assert.Equal(t, http.StatusOK, rec.Code, "body=%s", rec.Body.String()) + assert.Equal(t, 1, service.calls, "a dev box IS reached by its address") + }) +} + +// TestHostRouter_ComposedFallbackOnlyOn404: falling through on any error +// status would replay a request the user surface already REFUSED — turning +// its 400 into a service 404 (or worse, letting a rate-limited 503 be +// retried immediately against another handler). Only "this path is not +// mine", spelled 404, hands over. +func TestHostRouter_ComposedFallbackOnlyOn404(t *testing.T) { + const shared = "localhost:8091" + newRouter := func(t *testing.T) (http.Handler, *marker, *marker) { + t.Helper() + service := newMarker("service") + user := newMarker("user", "/xrpc/_health") + user.statuses = map[string]int{ + // A malformed webfinger: the user surface OWNS this path and + // has judged the request. + "/.well-known/webfinger": http.StatusBadRequest, + // A retryable refusal from the inbox. + "/ap/inbox": http.StatusServiceUnavailable, + } + router, err := NewHostRouter(HostRouterOptions{ + ServiceHost: shared, + ServiceHandler: service, + UserHost: shared, + UserHandler: user, + DevFallthrough: true, + }) + require.NoError(t, err) + return router, service, user + } + + for _, tc := range []struct { + path string + status int + }{ + {"/.well-known/webfinger", http.StatusBadRequest}, + {"/ap/inbox", http.StatusServiceUnavailable}, + } { + t.Run(tc.path, func(t *testing.T) { + router, service, user := newRouter(t) + rec := routeHost(t, router, "http", shared, tc.path) + assert.Equal(t, tc.status, rec.Code, + "the user surface's own refusal must reach the client unchanged") + assert.Equal(t, "user", rec.Header().Get("X-Handler")) + assert.Equal(t, 1, user.calls) + assert.Zero(t, service.calls, + "a judged request must not be replayed against the other surface") + }) + } +} diff --git a/internal/personas/instance.go b/internal/personas/instance.go index b0869e4..481387d 100644 --- a/internal/personas/instance.go +++ b/internal/personas/instance.go @@ -11,12 +11,6 @@ const ( nodeInfoDiscoveryPath = "/.well-known/nodeinfo" nodeInfoSchemaPath = "/nodeinfo/2.0" nodeInfoSchemaRel = "http://nodeinfo.diaspora.software/ns/schema/2.0" - // nodeInfoSoftwareName is what Lemmy admins allowlist by; it and the - // version mirror the service surface's nodeinfo (ingest/inbox.go), which - // describes the same deployment from its other origin. - nodeInfoSoftwareName = "tidepool" - nodeInfoSoftwareVersion = "0.1.0" - // instanceOutboxPath is advertised, not served — the same shape the // service surface's instance actor publishes. Lemmy's Instance parser // REQUIRES the field but never dereferences it. @@ -101,8 +95,8 @@ func (s *Service) handleNodeInfo(w http.ResponseWriter, _ *http.Request) { writeJSON(w, "application/json", map[string]any{ "version": "2.0", "software": map[string]any{ - "name": nodeInfoSoftwareName, - "version": nodeInfoSoftwareVersion, + "name": ap.SoftwareName, + "version": ap.SoftwareVersion, }, "protocols": []any{"activitypub"}, "services": map[string]any{"inbound": []any{}, "outbound": []any{}}, diff --git a/internal/personas/localpart.go b/internal/personas/localpart.go index 5e035b8..a0da2b0 100644 --- a/internal/personas/localpart.go +++ b/internal/personas/localpart.go @@ -53,7 +53,12 @@ func DeriveLocalPart(handle, nativeSuffix string) (string, error) { if nativeSuffix == "" { return "", errors.NewValidationError("native_suffix", "must not be empty") } - suffix := strings.ToLower(nativeSuffix) + // The suffix arrives from config and from a routed Host, which spell the + // same authority several ways. It gets the SAME normalization as the + // handle: an unnormalized suffix silently demotes native handles to + // foreign ones, minting "alice.coves.social" instead of "alice" — + // permanently, since the local part is frozen. + suffix := strings.TrimSuffix(strings.ToLower(strings.TrimSpace(nativeSuffix)), ".") // Case is not identity: handles are compared and stored lowercased, and // normalizing before parsing keeps "Alice.Coves.Social" and its @@ -80,7 +85,11 @@ func DeriveLocalPart(handle, nativeSuffix string) (string, error) { local = prefix } if len(local) > MaxLocalPartLen { - local = local[:MaxLocalPartLen] + // The cut is blind, so it can land on a separator. A local part + // ending in "." or "-" is not a name anything renders or matches + // sanely: Lemmy's mention regex wants a trailing alphanumeric, and + // a trailing dot reads as a hostname root. + local = strings.TrimRight(local[:MaxLocalPartLen], ".-") } return local, nil } diff --git a/internal/personas/localpart_test.go b/internal/personas/localpart_test.go index a0fe8b8..2013b75 100644 --- a/internal/personas/localpart_test.go +++ b/internal/personas/localpart_test.go @@ -170,3 +170,68 @@ func TestDeriveLocalPart_Deterministic(t *testing.T) { assert.Equal(t, first, second) assert.Equal(t, "alice", first) } + +// dottedLongHandle is 253 chars laid out so the MaxLocalPartLen cut lands +// immediately after a dot: 63 + 1 + 63 + 1 + 63 + 1 + 58 = 250 chars, the +// separator at index 250, then a 2-char TLD. +var dottedLongHandle = strings.Repeat("a", 63) + "." + + strings.Repeat("b", 63) + "." + + strings.Repeat("c", 63) + "." + + strings.Repeat("d", 58) + ".ee" + +// hyphenLongHandle is 253 chars whose final label carries a hyphen exactly +// at the cut (index 250). +var hyphenLongHandle = strings.Repeat("a", 63) + "." + + strings.Repeat("b", 63) + "." + + strings.Repeat("c", 63) + "." + + strings.Repeat("d", 58) + "-" + strings.Repeat("e", 2) + +// TestDeriveLocalPart_TruncationShape: truncation is a blind cut, so it can +// land on a separator. A local part ending in "." or "-" is not a name any +// implementation renders or matches sanely — Lemmy's mention regex needs a +// trailing alphanumeric, and a trailing dot reads as a hostname root. +func TestDeriveLocalPart_TruncationShape(t *testing.T) { + for _, tc := range []struct { + name string + handle string + }{ + {"cut lands on a dot", dottedLongHandle}, + {"cut lands on a hyphen", hyphenLongHandle}, + } { + t.Run(tc.name, func(t *testing.T) { + require.Len(t, tc.handle, 253, "fixture must sit on the handle length limit") + require.Contains(t, ".-", string(tc.handle[MaxLocalPartLen-1]), + "fixture must make the LAST KEPT character a separator") + + got, err := DeriveLocalPart(tc.handle, "coves.social") + require.NoError(t, err) + assert.LessOrEqual(t, len(got), MaxLocalPartLen) + assert.False(t, strings.HasSuffix(got, "."), "a local part must not end in a dot: %q", got) + assert.False(t, strings.HasSuffix(got, "-"), "a local part must not end in a hyphen: %q", got) + assert.Equal(t, strings.TrimRight(tc.handle[:MaxLocalPartLen], ".-"), got, + "the cut is trimmed of trailing separators, nothing else") + }) + } +} + +// TestDeriveLocalPart_NormalizesSuffix: the suffix arrives from config and +// from a routed Host, which spell the same authority several ways. Failing +// to normalize it silently demotes native handles to foreign ones — they +// would mint as "alice.coves.social" instead of "alice", permanently. +func TestDeriveLocalPart_NormalizesSuffix(t *testing.T) { + for _, suffix := range []string{"coves.social", "Coves.Social", "coves.social.", "COVES.SOCIAL."} { + t.Run(suffix, func(t *testing.T) { + got, err := DeriveLocalPart("alice.coves.social", suffix) + require.NoError(t, err) + assert.Equal(t, "alice", got, "suffix %q names the native space", suffix) + }) + } + + // The apex is refused through every spelling too — it is the instance + // actor's own name. + for _, suffix := range []string{"coves.social.", "Coves.Social"} { + _, err := DeriveLocalPart("coves.social", suffix) + require.Error(t, err, "the apex must be refused under suffix %q", suffix) + assert.True(t, errors.IsValidation(err), "got %v", err) + } +} diff --git a/internal/personas/origin.go b/internal/personas/origin.go new file mode 100644 index 0000000..0a7d997 --- /dev/null +++ b/internal/personas/origin.go @@ -0,0 +1,69 @@ +package personas + +import ( + "fmt" + "net/url" + "strings" + + "tidepool/internal/errors" +) + +// CanonicalizeOrigin reduces an origin URL to its ONE canonical spelling and +// returns both the origin ("https://coves.social") and the authority +// ("coves.social") that Host routing and ap_actors.normalized_origin use. +// +// This is the single definition of "the same origin" for the whole bridge: +// config validates AP_USER_ORIGIN through it and stores the result, and +// personas.New runs it again for callers constructed directly. Two spellings +// of one authority must never mint two namespaces — an actor minted under +// "coves.social:443" would carry a normalized_origin that the routed Host, +// which arrives canonical, could never match, and the actor_id is FROZEN at +// mint, so the mistake is permanent. +// +// Anything past scheme://host is refused rather than trimmed: a path, query, +// fragment, or userinfo in this value would be concatenated into every +// actor_id, keyId, and webfinger href, and silently dropping it would mint +// identities the operator did not ask for. +func CanonicalizeOrigin(raw string) (origin, host string, err error) { + parsed, parseErr := url.Parse(raw) + if parseErr != nil { + return "", "", errors.NewValidationError("origin", + fmt.Sprintf("must be an absolute origin URL, got %q: %v", raw, parseErr)) + } + scheme := strings.ToLower(parsed.Scheme) + if scheme == "" || parsed.Host == "" || parsed.Opaque != "" { + return "", "", errors.NewValidationError("origin", + fmt.Sprintf("must be an absolute origin URL (scheme and host), got %q", raw)) + } + if scheme != "http" && scheme != "https" { + return "", "", errors.NewValidationError("origin", + fmt.Sprintf("must be http or https, got %q", scheme)) + } + if parsed.Path != "" || parsed.RawQuery != "" || parsed.ForceQuery || + parsed.Fragment != "" || parsed.User != nil { + return "", "", errors.NewValidationError("origin", + fmt.Sprintf("must be a bare origin (scheme://host, no path, query, fragment, or userinfo), got %q", raw)) + } + + // A fully-qualified name's trailing dot names the same host, and the + // scheme's default port is not part of the authority. Any OTHER port is: + // the dev origin runs on :8091 and that is a different origin. + hostname := strings.TrimSuffix(strings.ToLower(parsed.Hostname()), ".") + if hostname == "" { + return "", "", errors.NewValidationError("origin", + fmt.Sprintf("must name a host, got %q", raw)) + } + port := parsed.Port() + if (scheme == "https" && port == "443") || (scheme == "http" && port == "80") { + port = "" + } + host = hostname + if strings.Contains(host, ":") { + // An IPv6 literal keeps the brackets url.URL.Hostname stripped. + host = "[" + host + "]" + } + if port != "" { + host += ":" + port + } + return scheme + "://" + host, host, nil +} diff --git a/internal/personas/personas.go b/internal/personas/personas.go index 895a7db..1bb3e3d 100644 --- a/internal/personas/personas.go +++ b/internal/personas/personas.go @@ -10,10 +10,9 @@ import ( "database/sql" stderrors "errors" "fmt" + "log/slog" "net/http" - "net/url" "strconv" - "strings" "tidepool/internal/ap" "tidepool/internal/errors" @@ -25,6 +24,13 @@ import ( // version 2 alongside the published key it replaces; nothing does yet. const currentRSAKeyVersion = 1 +// ErrLocalPartExhausted reports that every suffix in the collision range is +// taken for one derived local part. It is deliberately NOT a conflict error: +// a caller that read it as "someone else won the race, re-read the row" would +// retry a name that has nowhere left to go, forever. An operator has to widen +// the range or the namespace. +var ErrLocalPartExhausted = stderrors.New("personas: local part namespace exhausted") + // maxLocalPartAttempts bounds the collision search: attempt 1 claims the bare // local part and the rest append "-2" ... "-99", the suffix range // MaxLocalPartLen reserves room for. A namespace that has genuinely exhausted @@ -53,6 +59,10 @@ type Options struct { // and queueing (ingest.Inbox.InboxHandler). Nil means the origin // advertises an inbox it cannot serve, so the route 404s. InboxHandler http.Handler + // Logger receives the conditions nobody sees from a response code: a + // 500 on the serve path, an exhausted namespace, a missing bridge + // identity. Nil uses slog.Default(). + Logger *slog.Logger } // Service mints and serves Coves user actors. @@ -71,40 +81,51 @@ type Service struct { // inboxHandler is the ingest inbox this origin's shared inbox dispatches // to. Nil means the route 404s. inboxHandler http.Handler + logger *slog.Logger } -// New builds a Service. UserOrigin is parsed once here: the host it yields -// keys every actor this service mints, so an origin that cannot produce one -// is a startup error rather than a surprise at mint time. +// New builds a Service. The origin is canonicalized once here — config +// already did it, but a Service is also constructed directly, and a ":443" +// that slipped through would mint actors under a second namespace whose +// normalized_origin the routed Host could never match. A missing dependency +// is a startup error rather than a nil-panic on the first request. func New(opts Options) (*Service, error) { - host, err := originHost(opts.UserOrigin) + if opts.DB == nil { + return nil, errors.NewValidationError("db", "must not be nil") + } + if opts.Custodian == nil { + return nil, errors.NewValidationError("custodian", "must not be nil") + } + origin, host, err := CanonicalizeOrigin(opts.UserOrigin) if err != nil { - return nil, err + return nil, fmt.Errorf("personas: user origin: %w", err) + } + logger := opts.Logger + if logger == nil { + logger = slog.Default() + } + if opts.ServiceActor == nil { + // Not fatal, but not silent either: Lemmy delivers its + // send-to-all-instances activities — Delete{Person} above all — + // ONLY to the inbox on a peer's instance-actor row. Without one + // this origin never receives them, and nothing about that failure + // is visible from the outside. + logger.Warn("user origin has no service actor: the origin apex publishes no instance actor, "+ + "so peers cannot deliver instance-wide activities (account deletions) here", + "user_origin", origin) } return &Service{ actors: store.NewAPActors(opts.DB), custodian: opts.Custodian, - userOrigin: opts.UserOrigin, + userOrigin: origin, userHost: host, serviceActor: opts.ServiceActor, inboxHandler: opts.InboxHandler, + logger: logger, }, nil } -// originHost reduces an origin URL to the scheme-less lowercase host. -func originHost(origin string) (string, error) { - parsed, err := url.Parse(origin) - if err != nil { - return "", errors.NewValidationError("user_origin", err.Error()) - } - if parsed.Host == "" { - return "", errors.NewValidationError("user_origin", - fmt.Sprintf("must be an absolute origin URL, got %q", origin)) - } - return strings.ToLower(parsed.Host), nil -} - // CreateActorForDID get-or-creates the AP Person actor for a Coves DID: // mints an RSA key, seals it via the custodian, derives and freezes the // local part from handle, and writes the ap_actors row. @@ -187,7 +208,9 @@ func (s *Service) CreateActorForDID(ctx context.Context, did, handle string) (*s } return nil, fmt.Errorf("personas: create actor for %s: %w", did, createErr) } - return nil, errors.NewConflictError("ap_actor", "local_part", base) + s.logger.Error("local part namespace exhausted: no free suffix left for a derived name", + "local_part", base, "attempts", maxLocalPartAttempts, "did", did) + return nil, fmt.Errorf("%w: %q after %d attempts", ErrLocalPartExhausted, base, maxLocalPartAttempts) } // suffixedLocalPart names the attempt'th claimant of base: the first keeps diff --git a/internal/personas/serving.go b/internal/personas/serving.go index 7eea081..d1a5957 100644 --- a/internal/personas/serving.go +++ b/internal/personas/serving.go @@ -3,6 +3,7 @@ package personas import ( "context" "encoding/json" + "fmt" "net/http" "net/url" "strings" @@ -33,8 +34,15 @@ const ( securityNamespace = "https://w3id.org/security/v1" ) -// ServeHTTP serves the user-origin surface: /.well-known/webfinger, -// /ap/actor/{did}, and /ap/actor/{did}/outbox. +// ServeHTTP serves the user-origin surface: +// +// GET /.well-known/webfinger discovery for a local part on the routed Host +// GET /ap/actor/{did} the user's Person document +// GET /ap/actor/{did}/outbox the (empty) outbox Lemmy requires +// POST /ap/inbox shared inbox, dispatched to the ingest inbox +// GET / the origin's instance (Application) actor +// GET /.well-known/nodeinfo nodeinfo discovery +// GET /nodeinfo/2.0 nodeinfo 2.0 document // // Routing reads r.URL.Path, which net/http has already percent-decoded. A // DID's colons are legal unescaped, so both "did:plc:x" and "did%3Aplc%3Ax" @@ -129,11 +137,21 @@ func (s *Service) handleInbox(w http.ResponseWriter, r *http.Request) { func (s *Service) handleActorDocument(w http.ResponseWriter, r *http.Request, did string) { actor, err := s.actors.GetByDID(r.Context(), did) if err != nil { - writeStoreError(w, err) + s.writeStoreError(w, r, err) + return + } + if !s.servesActor(actor, r) { + http.NotFound(w, r) return } - origin := actorOrigin(actor, s.userOrigin) + origin, err := actorOrigin(actor) + if err != nil { + s.logger.Error("stored actor_id is not an absolute URL", + "did", actor.DID, "actor_id", actor.ActorID, "error", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } inbox := origin + inboxPath // The display name falls back to the local part: Lemmy renders `name`, // and an empty one shows as a blank user until task 14's profile sync @@ -184,7 +202,11 @@ func (s *Service) handleActorDocument(w http.ResponseWriter, r *http.Request, di func (s *Service) handleOutbox(w http.ResponseWriter, r *http.Request, did string) { actor, err := s.actors.GetByDID(r.Context(), did) if err != nil { - writeStoreError(w, err) + s.writeStoreError(w, r, err) + return + } + if !s.servesActor(actor, r) { + http.NotFound(w, r) return } writeJSON(w, ap.ContentTypeActivityJSON, map[string]any{ @@ -213,7 +235,7 @@ func (s *Service) handleWebFinger(w http.ResponseWriter, r *http.Request) { host := normalizeHost(r.Host) actor, err := s.lookupResource(r.Context(), resource, host) if err != nil { - writeStoreError(w, err) + s.writeStoreError(w, r, err) return } if !actor.Enabled { @@ -277,16 +299,30 @@ func (s *Service) lookupResource(ctx context.Context, resource, host string) (*s return actor, nil } +// servesActor reports whether the routed Host is the actor's OWN origin. The +// DID is global but the actor is not: serving a vanity-origin actor's +// document under another Host would publish a document whose id sits on a +// different authority — the cross-authority claim ap.Client's key resolution +// refuses, and the mirror image of the binding webfinger already enforces. +func (s *Service) servesActor(actor *store.APActor, r *http.Request) bool { + return actor.NormalizedOrigin == normalizeHost(r.Host) +} + // actorOrigin recovers the scheme+host an actor was minted under from its // stored actor_id, so a vanity-origin actor advertises its own inbox rather -// than the configured one. The configured origin is only the fallback for an -// actor_id that cannot be parsed. -func actorOrigin(actor *store.APActor, fallback string) string { +// than the configured one. It FAILS CLOSED: an actor_id that will not parse +// means the row is corrupt, and falling back to the configured origin would +// publish a document whose inbox and key belong to a different authority than +// its id — quietly, and cached by every peer that fetched it. +func actorOrigin(actor *store.APActor) (string, error) { parsed, err := url.Parse(actor.ActorID) - if err != nil || parsed.Scheme == "" || parsed.Host == "" { - return fallback + if err != nil { + return "", fmt.Errorf("parse actor_id %q: %w", actor.ActorID, err) + } + if parsed.Scheme == "" || parsed.Host == "" { + return "", fmt.Errorf("actor_id %q is not an absolute URL", actor.ActorID) } - return parsed.Scheme + "://" + parsed.Host + return parsed.Scheme + "://" + parsed.Host, nil } func writeJSON(w http.ResponseWriter, contentType string, doc any) { @@ -296,14 +332,19 @@ func writeJSON(w http.ResponseWriter, contentType string, doc any) { // writeStoreError maps a store/validation error onto the status a remote // resolver will read correctly: a miss is cacheable as "no such account", a -// malformed request is the caller's fault, and anything else is ours. -func writeStoreError(w http.ResponseWriter, err error) { +// malformed request is the caller's fault, and anything else is ours — and +// the last case is LOGGED, because a 500 body says nothing and a database +// that has started failing under a peer's discovery traffic is otherwise +// invisible. +func (s *Service) writeStoreError(w http.ResponseWriter, r *http.Request, err error) { switch { case errors.IsNotFound(err): http.Error(w, "resource not found", http.StatusNotFound) case errors.IsValidation(err): http.Error(w, "malformed request", http.StatusBadRequest) default: + s.logger.Error("user origin request failed", + "path", r.URL.Path, "host", r.Host, "error", err) http.Error(w, "internal error", http.StatusInternalServerError) } } diff --git a/internal/personas/serving_test.go b/internal/personas/serving_test.go index c9a74cd..dc9b6df 100644 --- a/internal/personas/serving_test.go +++ b/internal/personas/serving_test.go @@ -1,6 +1,7 @@ package personas import ( + "database/sql" "encoding/json" "net/http" "net/http/httptest" @@ -380,3 +381,125 @@ func TestWebFinger_Errors(t *testing.T) { assert.Equal(t, http.StatusBadRequest, missing.Code, "a webfinger request without a resource parameter is malformed, not a miss") } + +// seedVanityActor inserts an actor minted under a DIFFERENT origin than the +// service's, the way a vanity-origin deployment would. +func seedVanityActor(t *testing.T, database *sql.DB) *store.APActor { + t.Helper() + did := testDID(t) + actor, err := store.NewAPActors(database).Create(t.Context(), store.APActor{ + DID: did, + Kind: store.ActorTypePerson, + ActorID: vanityOrigin + "/ap/actor/" + did, + NormalizedOrigin: vanityHost, + LocalPart: "alice", + RSAKeySealed: []byte{0x01, 0x02, 0x03}, + RSAKeyVersion: 1, + PublicKeyPEM: "seeded", + }) + require.NoError(t, err) + require.NotNil(t, actor) + return actor +} + +// TestServeActorDocument_BoundToItsOwnOrigin: the DID is global but the +// actor is not. Serving a vanity-origin actor's document under coves.social +// would publish a document whose id is on ANOTHER authority — exactly the +// cross-authority claim ap.Client's key resolution refuses, and the +// mirror-image of the binding webfinger already enforces. +func TestServeActorDocument_BoundToItsOwnOrigin(t *testing.T) { + database := personasTestDB(t) + svc, _ := newTestService(t, database) + vanity := seedVanityActor(t, database) + + for _, path := range []string{actorPath(vanity.DID), actorPath(vanity.DID) + "/outbox"} { + t.Run(path, func(t *testing.T) { + foreign := serveOnUserOrigin(svc, http.MethodGet, path, nil) + assert.Equal(t, http.StatusNotFound, foreign.Code, + "%s is hosted on %s; %s must not answer for it", path, vanityHost, userHost) + + // ... and it DOES answer under its own Host, so this is a + // binding rather than a blanket refusal. + own := serveOnHost(svc, vanityHost, http.MethodGet, path, nil) + assert.Equal(t, http.StatusOK, own.Code, + "%s must still resolve under its own origin; body=%s", path, own.Body.String()) + }) + } +} + +// TestWebFinger_URLResourceBinding: the URL spelling of a resource must be +// bound to the routed Host and gated on `enabled` exactly like the acct +// spelling — two branches, one policy. +func TestWebFinger_URLResourceBinding(t *testing.T) { + database := personasTestDB(t) + svc, _ := newTestService(t, database) + actors := store.NewAPActors(database) + vanity := seedVanityActor(t, database) + native := mintTestActor(t, svc, "bob."+userHost) + + // Two independent layers refuse a foreign actor, and each needs its own + // case or the other one hides it. + // + // Layer 1 — the resource's own authority: the URL names vanity.example, + // so it is rejected before any lookup happens. + foreign := serveOnUserOrigin(svc, http.MethodGet, webfingerTarget(vanity.ActorID), nil) + assert.Equal(t, http.StatusNotFound, foreign.Code, + "an actor URL on another origin must not resolve here") + + // Layer 2 — the FOUND actor's origin. Spelling the resource with OUR + // prefix and a foreign DID walks past layer 1 (the authority matches the + // routed Host) and past the path cut, and GetByDID succeeds because DIDs + // are global. Only the stored normalized_origin comparison stops it — + // without that check this origin would answer for an actor it does not + // host, handing a vanity actor a second identity on coves.social. + spoofed := serveOnUserOrigin(svc, http.MethodGet, + webfingerTarget(userOrigin+"/ap/actor/"+vanity.DID), nil) + assert.Equal(t, http.StatusNotFound, spoofed.Code, + "a foreign DID under this origin's prefix must not resolve; body=%s", spoofed.Body.String()) + + // The same spelling for a local actor resolves... + ok := serveOnUserOrigin(svc, http.MethodGet, webfingerTarget(native.ActorID), nil) + require.Equal(t, http.StatusOK, ok.Code, "body=%s", ok.Body.String()) + + // ... until it is disabled, which removes it from discovery through + // BOTH spellings. + require.NoError(t, actors.SetEnabled(t.Context(), native.DID, false)) + byURL := serveOnUserOrigin(svc, http.MethodGet, webfingerTarget(native.ActorID), nil) + assert.Equal(t, http.StatusNotFound, byURL.Code, + "a disabled actor must not resolve by actor URL either") + byAcct := serveOnUserOrigin(svc, http.MethodGet, + webfingerTarget("acct:"+native.LocalPart+"@"+userHost), nil) + assert.Equal(t, http.StatusNotFound, byAcct.Code) +} + +// TestServeActorDocument_PartialProfile: the cache fills field by field +// (task 14 syncs whatever the appview has), so an avatar without a summary — +// or the reverse — must render exactly the field that is known. +func TestServeActorDocument_PartialProfile(t *testing.T) { + database := personasTestDB(t) + svc, _ := newTestService(t, database) + actors := store.NewAPActors(database) + + t.Run("avatar without summary", func(t *testing.T) { + actor := mintTestActor(t, svc, testHandle) + const avatar = "https://cdn.example/only-avatar.png" + require.NoError(t, actors.UpdateProfile(t.Context(), actor.DID, + store.APActorProfile{AvatarURL: avatar})) + + doc := decodeJSON(t, serveOnUserOrigin(svc, http.MethodGet, actorPath(actor.DID), nil)) + icon, ok := doc["icon"].(map[string]any) + require.True(t, ok, "an avatar with no summary must still render an icon, got %v", doc["icon"]) + assert.Equal(t, avatar, icon["url"]) + assert.NotContains(t, doc, "summary", "an unknown summary stays absent") + }) + + t.Run("summary without avatar", func(t *testing.T) { + actor := mintTestActor(t, svc, "carol."+userHost) + require.NoError(t, actors.UpdateProfile(t.Context(), actor.DID, + store.APActorProfile{Summary: "tide pools only"})) + + doc := decodeJSON(t, serveOnUserOrigin(svc, http.MethodGet, actorPath(actor.DID), nil)) + assert.Equal(t, "tide pools only", doc["summary"]) + assert.NotContains(t, doc, "icon", "an actor with no avatar publishes no icon") + }) +} -- 2.51.2