diff --git a/internal/config/config.go b/internal/config/config.go index ed686c6..e7bd6b3 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -798,9 +798,18 @@ func boolVarDefault(logger *slog.Logger, name string, fallback bool) (bool, erro // // 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. +// router could not tell the two surfaces apart in the first place. +// +// BOTH SIDES REDUCE THROUGH personas.NormalizeHost — the very function the Host +// router applies to every request. That is the point: a check that reduces +// differently from the router can pass a pair the router then collapses. It +// used to canonicalize only the ORIGIN side, so the spelling its own comment +// named ("https://TDPL.IO:443") walked straight past whenever it appeared on +// the BRIDGE_HOSTNAME side instead — and BRIDGE_HOSTNAME "tdpl.io:443" with +// AP_USER_ORIGIN "https://tdpl.io" passed boot, whereupon normalizeHost folded +// both to "tdpl.io" and the router quietly entered COMPOSED mode in production, +// a shape only the dev default was ever meant to reach. +// // 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 @@ -818,7 +827,7 @@ func validateUserOrigin(origin, bridgeHostname string, isDevelopment bool) (stri return "", fmt.Errorf("config: AP_USER_ORIGIN must be https in production, got %q", canonical) } - bridge := strings.TrimSuffix(strings.ToLower(strings.TrimSpace(bridgeHostname)), ".") + bridge := canonicalBridgeHost(bridgeHostname) if host == bridge || strings.HasSuffix(host, "."+bridge) { 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) @@ -826,6 +835,24 @@ func validateUserOrigin(origin, bridgeHostname string, isDevelopment bool) (stri return canonical, nil } +// canonicalBridgeHost reduces BRIDGE_HOSTNAME to the authority the Host router +// will compare it as. It is a HOSTNAME, not a URL, but operators write it as +// one often enough that a pasted "https://tdpl.io" must not read as a different +// authority than "tdpl.io" — the whole value of this check is that it agrees +// with the router, and the router only ever sees the authority. +func canonicalBridgeHost(raw string) string { + host := strings.TrimSpace(raw) + if _, after, found := strings.Cut(host, "://"); found { + host = after + } + // A path, query, or fragment is not part of the authority; cutting at the + // first delimiter leaves the part the router would key on. + host, _, _ = strings.Cut(host, "/") + host, _, _ = strings.Cut(host, "?") + host, _, _ = strings.Cut(host, "#") + return personas.NormalizeHost(host) +} + // stringVar returns the value of an environment variable. When unset it // falls back to the logged dev default in development and errors in // production. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 07239ba..2969cfc 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -440,6 +440,44 @@ func TestLoad_APUserOriginShadowCheckIsCanonical(t *testing.T) { } } +// TestLoad_APUserOriginShadowCheckCanonicalizesTheBridgeSide: the check +// canonicalized only the ORIGIN side, so its own cited defence +// ("https://TDPL.IO:443") walked past it whenever the second spelling was on +// the BRIDGE_HOSTNAME side instead. Both sides must reduce with the SAME rule +// the Host router uses, or the check and the router disagree about what "the +// same authority" means — and the router is the one production listens to. +func TestLoad_APUserOriginShadowCheckCanonicalizesTheBridgeSide(t *testing.T) { + for _, tc := range []struct { + name string + hostname string + origin string + }{ + {"bridge side carries the default port", "tdpl.io:443", "https://tdpl.io"}, + {"bridge side is uppercase", "TDPL.IO", "https://tdpl.io"}, + {"bridge side is fully qualified", "tdpl.io.", "https://tdpl.io"}, + {"bridge side spells the port and the case", "TDPL.IO:443", "https://tdpl.io"}, + {"bridge side carries a scheme", "https://tdpl.io", "https://tdpl.io"}, + {"both sides wear a different spelling", "TDPL.IO:443", "https://tdpl.io."}, + // The composed-mode trap: neither string equals the other, the check + // passes, and then normalizeHost collapses BOTH to "tdpl.io" so the + // router silently serves the two surfaces off one authority — in + // production, where composition was never meant to happen. + {"subdomain under a differently spelled bridge host", "TDPL.IO:443", "https://users.tdpl.io"}, + } { + t.Run(tc.name, func(t *testing.T) { + clearConfigEnv(t) + t.Setenv("BRIDGE_HOSTNAME", tc.hostname) + t.Setenv("AP_USER_ORIGIN", tc.origin) + + _, err := Load(discardLogger()) + require.Error(t, err, + "BRIDGE_HOSTNAME %q and AP_USER_ORIGIN %q name one authority to the Host router", + tc.hostname, tc.origin) + assert.Contains(t, err.Error(), "AP_USER_ORIGIN") + }) + } +} + // --------------------------------------------------------------------------- // Task 14 cycle K1: the Jetstream consumer's configuration. // diff --git a/internal/echo/echo.go b/internal/echo/echo.go index dda62fe..d879b71 100644 --- a/internal/echo/echo.go +++ b/internal/echo/echo.go @@ -379,8 +379,11 @@ func schemeOf(actorID string) string { // normalizeHost reduces a URL authority to the authority it names — lowercase, // no trailing dot, no default port — so it can be compared with the // normalized_origin an actor was minted under. It is the read-side twin of -// personas' own normalizeHost; a NON-default port still carries meaning, -// because the dev origin runs on :8091 and that is a different origin. +// personas.NormalizeHost — the exported name of the rule the Host router, the +// serving surface, and config's shadow check all key on — and must stay +// behaviourally identical to it; TestNormalizeHostMatchesPersonas pins that. A +// NON-default port still carries meaning, because the dev origin runs on :8091 +// and that is a different origin. func normalizeHost(host string) string { normalized := strings.ToLower(strings.TrimSpace(host)) for _, defaultPort := range []string{":443", ":80"} { diff --git a/internal/echo/echo_test.go b/internal/echo/echo_test.go index 676335f..e3b5084 100644 --- a/internal/echo/echo_test.go +++ b/internal/echo/echo_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" "tidepool/internal/errors" + "tidepool/internal/personas" "tidepool/internal/store" "tidepool/internal/testutil" ) @@ -401,6 +402,26 @@ func TestIdentifyRefusesWhatWeDoNotServe(t *testing.T) { } } +// TestNormalizeHostMatchesPersonas pins the hand-copy to its original +// DIRECTLY, not just through classification outcomes. echo's normalizeHost is a +// duplicate of personas.NormalizeHost, and the two are the read side and the +// write side of one question — "is this authority ours?" — asked about rows +// whose normalized_origin was frozen at mint. If they ever disagree, echo +// either fails to recognize our own actor ids (duplicating content) or claims +// ids that are not ours (dropping genuine content), and no test in either +// package would otherwise notice. +func TestNormalizeHostMatchesPersonas(t *testing.T) { + for _, host := range []string{ + "", "coves.social", "COVES.SOCIAL", "coves.social.", " coves.social ", + "coves.social:443", "coves.social:80", "coves.social:8091", + "COVES.SOCIAL:443", "coves.social:443.", "localhost", "localhost:8091", + "[::1]", "[::1]:443", "[::1]:8091", "127.0.0.1:80", "tdpl.io:8443", + } { + assert.Equal(t, personas.NormalizeHost(host), normalizeHost(host), + "echo and personas must reduce %q to the same authority", host) + } +} + // TestIdentifyNormalizesTheHostTheWayServingDoes pins echo.normalizeHost, the // read-side twin of personas' own. It is a hand-copy, and a hand-copy that // drifts is invisible: replacing its body with `return host` leaves every other diff --git a/internal/personas/hostrouter.go b/internal/personas/hostrouter.go index a42901a..c680fad 100644 --- a/internal/personas/hostrouter.go +++ b/internal/personas/hostrouter.go @@ -207,8 +207,15 @@ func (b *bufferedResponse) flushTo(w http.ResponseWriter) { _, _ = w.Write(b.body.Bytes()) } -// normalizeHost reduces a Host header to the authority it names: lowercase, -// no trailing dot, and no default port for either scheme. +// NormalizeHost reduces an authority to the ONE string this bridge compares: +// lowercase, no trailing dot, and no default port for either scheme. +// +// It is THE definition of "the same authority" for the whole process. The Host +// router keys on it, serving binds each actor to it, CanonicalizeOrigin +// reduces the minted normalized_origin through it, and config's shadow check +// compares BOTH sides with it — the last one matters because a check that +// reduces differently from the router can pass a pair the router then collapses +// into composed mode. One rule, one place, no site allowed to disagree. // // Both default ports are stripped unconditionally, without consulting r.TLS. // In production TLS terminates at the proxy and the Go server sees plain HTTP @@ -216,7 +223,10 @@ func (b *bufferedResponse) flushTo(w http.ResponseWriter) { // "coves.social:443" as an unknown authority precisely where it matters. A // NON-default port still carries meaning — the dev origin runs on :8091 and // coves.social:8443 is a different origin, not a sloppy spelling of one. -func normalizeHost(host string) string { +// +// internal/echo carries a byte-identical private twin for read-side actor +// classification; the two are documented as one rule and must move together. +func NormalizeHost(host string) string { normalized := strings.ToLower(strings.TrimSpace(host)) for _, defaultPort := range []string{":443", ":80"} { if trimmed, found := strings.CutSuffix(normalized, defaultPort); found { @@ -227,6 +237,9 @@ func normalizeHost(host string) string { return strings.TrimSuffix(normalized, ".") } +// normalizeHost is the package-internal spelling of NormalizeHost. +func normalizeHost(host string) string { return NormalizeHost(host) } + // hostnameOnly strips a port and IPv6 brackets, leaving the name or address. func hostnameOnly(host string) string { if name, _, err := net.SplitHostPort(host); err == nil { diff --git a/internal/personas/identity_freeze_test.go b/internal/personas/identity_freeze_test.go new file mode 100644 index 0000000..e4a232c --- /dev/null +++ b/internal/personas/identity_freeze_test.go @@ -0,0 +1,219 @@ +package personas + +import ( + "database/sql" + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/errors" + "tidepool/internal/identity" + "tidepool/internal/store" +) + +// probeHost is the authority the origin-spelling matrix mints under. It is not +// coves.social so a broken canonicalization here cannot borrow a row minted by +// another test. +const probeHost = "probe.example" + +// originSpellings is every legal AP_USER_ORIGIN spelling of ONE authority, +// including the two MIRROR pairs (http on :443, https on :80) that the +// scheme-aware canonicalizer used to keep and the Host router always folds. +var originSpellings = []string{ + "https://" + probeHost, + "https://" + probeHost + ":443", + "https://" + probeHost + ":80", + "http://" + probeHost, + "http://" + probeHost + ":80", + "http://" + probeHost + ":443", +} + +// hostSpellings is every way that authority can arrive in a Host header once a +// proxy, a client, or a resolver has had its say. +var hostSpellings = []string{ + probeHost, + probeHost + ":443", + probeHost + ":80", + "PROBE.EXAMPLE", + probeHost + ".", +} + +// newServiceOnOrigin builds a Service minting under an arbitrary origin. +func newServiceOnOrigin(t *testing.T, database *sql.DB, origin string) *Service { + t.Helper() + custodian, err := identity.NewCustodian(testKEK) + require.NoError(t, err) + svc, err := New(Options{DB: database, Custodian: custodian, UserOrigin: origin}) + require.NoError(t, err, "origin %q must be accepted", origin) + return svc +} + +// TestCanonicalizeOriginRoundTripsThroughHostRouting is the property the +// ":443 bricks actors forever" trap violated: whatever CanonicalizeOrigin +// freezes into normalized_origin must be a FIXED POINT of the normalization +// the Host router applies to every incoming request. If the two disagree by so +// much as a port, the minted actor answers under no Host spelling at all, and +// actor_id is frozen at mint, so the mistake is permanent. +func TestCanonicalizeOriginRoundTripsThroughHostRouting(t *testing.T) { + for _, raw := range originSpellings { + t.Run(raw, func(t *testing.T) { + _, host, err := CanonicalizeOrigin(raw) + require.NoError(t, err) + assert.Equal(t, host, normalizeHost(host), + "canonical host %q must survive the router's own normalization, "+ + "or every minted actor 404s under every Host spelling", host) + }) + } +} + +// TestCanonicalizeOriginKeepsPortlessHTTPSUnchanged pins the production +// spellings. Rows already minted under these must keep resolving: widening the +// port fold must be a NO-OP here, or the fix is itself a permanent break. +func TestCanonicalizeOriginKeepsPortlessHTTPSUnchanged(t *testing.T) { + for _, tc := range []struct{ raw, wantOrigin, wantHost string }{ + {"https://coves.social", "https://coves.social", "coves.social"}, + {"https://tdpl.io", "https://tdpl.io", "tdpl.io"}, + {"http://localhost:8091", "http://localhost:8091", "localhost:8091"}, + {"https://coves.social:8443", "https://coves.social:8443", "coves.social:8443"}, + } { + t.Run(tc.raw, func(t *testing.T) { + origin, host, err := CanonicalizeOrigin(tc.raw) + require.NoError(t, err) + assert.Equal(t, tc.wantOrigin, origin) + assert.Equal(t, tc.wantHost, host) + }) + } +} + +// TestMintedActorServesUnderEveryHostSpelling is the review's probe, driven +// over HTTP. AP_USER_ORIGIN="http://probe.example:443" is legal and config +// accepts it; before the fix it minted normalized_origin "probe.example:443", +// which normalizeHost folds away on EVERY request, so the actor was +// permanently unreachable under every spelling of its own name. +func TestMintedActorServesUnderEveryHostSpelling(t *testing.T) { + database := personasTestDB(t) + for _, origin := range originSpellings { + t.Run(origin, func(t *testing.T) { + svc := newServiceOnOrigin(t, database, origin) + actor, err := svc.CreateActorForDID(t.Context(), testDID(t), "alice."+probeHost) + require.NoError(t, err) + assert.Equal(t, probeHost, actor.NormalizedOrigin, + "every spelling of one authority must mint ONE namespace") + + for _, host := range hostSpellings { + rec := serveOnHost(svc, host, http.MethodGet, actorPath(actor.DID), nil) + require.Equal(t, http.StatusOK, rec.Code, + "actor minted under %q must serve under Host %q, body=%s", + origin, host, rec.Body.String()) + assert.Equal(t, actor.ActorID, decodeJSON(t, rec)["id"]) + } + }) + } +} + +// insertCorruptActor writes an ap_actors row directly, bypassing the mint path, +// so serving is exercised against a row that no longer satisfies the invariant +// CreateActorForDID establishes. +func insertCorruptActor(t *testing.T, database *sql.DB, localPart, actorID string) *store.APActor { + t.Helper() + actors := store.NewAPActors(database) + created, err := actors.Create(t.Context(), store.APActor{ + DID: testDID(t), + Kind: store.ActorTypePerson, + ActorID: actorID, + NormalizedOrigin: userHost, + LocalPart: localPart, + RSAKeySealed: []byte("sealed"), + RSAKeyVersion: currentRSAKeyVersion, + PublicKeyPEM: "-----BEGIN PUBLIC KEY-----\nnot-a-key\n-----END PUBLIC KEY-----\n", + }) + require.NoError(t, err) + return created +} + +// TestServingFailsClosedOnCorruptActorIDEverywhere: the fail-closed actor_id +// guard has to cover EVERY surface that publishes the value, not just the actor +// document. The outbox publishes actor_id+"/outbox" and WebFinger publishes it +// as the alias AND both rel=self hrefs — the href every remote resolver caches. +// A 200 from either one is the cross-authority claim the guard exists to stop, +// cached by every peer that asked. +func TestServingFailsClosedOnCorruptActorIDEverywhere(t *testing.T) { + database := personasTestDB(t) + svc, _ := newTestService(t, database) + + for _, tc := range []struct { + name string + localPart string + actorID string + }{ + { + name: "actor_id does not parse as a URL", + localPart: "unparseable", + actorID: "://not a url", + }, + { + name: "actor_id is not absolute", + localPart: "relative", + actorID: "/ap/actor/did:plc:whatever", + }, + { + // The row claims this origin hosts it, but its id names another + // authority: publishing that pairs OUR inbox and key with THEIR id. + name: "actor_id belongs to a foreign authority", + localPart: "foreign", + actorID: "https://evil.example/ap/actor/did:plc:whatever", + }, + } { + t.Run(tc.name, func(t *testing.T) { + actor := insertCorruptActor(t, database, tc.localPart, tc.actorID) + + for _, target := range []string{ + actorPath(actor.DID), + actorPath(actor.DID) + "/outbox", + webfingerTarget("acct:" + tc.localPart + "@" + userHost), + } { + rec := serveOnUserOrigin(svc, http.MethodGet, target, nil) + assert.NotEqual(t, http.StatusOK, rec.Code, + "%s must refuse a corrupt actor_id, not publish it: body=%s", + target, rec.Body.String()) + } + }) + } +} + +// TestCreateActorForDIDRefusesUnservableDID: the DID is frozen into actor_id, +// keyId, the WebFinger href, and the signing identity — every bit as +// irreversibly as the handle-derived local part, which gets a full syntax gate. +// A DID that cannot round-trip through the serving path mints an actor that is +// permanently unfetchable, and re-minting it is not possible. +func TestCreateActorForDIDRefusesUnservableDID(t *testing.T) { + database := personasTestDB(t) + svc, _ := newTestService(t, database) + + for _, tc := range []struct{ name, did string }{ + {"empty", ""}, + {"not a DID at all", "alice"}, + {"wrong scheme", "web:example.com"}, + // ServeHTTP 404s any /ap/actor/ rest containing "/", so this actor + // could never serve its own document. + {"contains a path separator", "did:plc:abc/evil"}, + // Stored verbatim, but r.URL.Path arrives percent-DECODED, so the + // lookup by the served path can never find this row again. + {"percent-encoded", "did:web:example.com%3A8080"}, + {"whitespace", "did:plc:abc def"}, + } { + t.Run(tc.name, func(t *testing.T) { + actor, err := svc.CreateActorForDID(t.Context(), tc.did, testHandle) + require.Error(t, err, "minting %q freezes an unservable identity", tc.did) + assert.Nil(t, actor) + assert.True(t, errors.IsValidation(err), + "a malformed DID is permanent, not retryable: got %#v", err) + + _, getErr := svc.actors.GetByDID(t.Context(), tc.did) + assert.True(t, errors.IsNotFound(getErr), + "the row must be refused BEFORE keygen and INSERT, got %v", getErr) + }) + } +} diff --git a/internal/personas/origin.go b/internal/personas/origin.go index 0a7d997..9007d9d 100644 --- a/internal/personas/origin.go +++ b/internal/personas/origin.go @@ -24,6 +24,19 @@ import ( // 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. +// +// THE AUTHORITY IS REDUCED BY normalizeHost — the SAME function the Host +// router applies to every incoming request — so the canonical host is a fixed +// point of routing by construction. The two rules used to differ by one +// scheme test: this one dropped the default port only when it matched the +// scheme, the router drops :443 and :80 unconditionally (it must — in +// production TLS terminates at the proxy and this process sees plain HTTP +// carrying "Host: coves.social:443", so a scheme-keyed router would refuse the +// production shape). They agreed on "https + :443" and "http + :80" and +// disagreed on the mirror pairs, so an origin spelled "http://host:443" minted +// normalized_origin "host:443" that the routed Host — folded to "host" — +// could never match, under ANY spelling, forever. Widening the fold here is +// the only direction available: the router cannot learn the scheme. func CanonicalizeOrigin(raw string) (origin, host string, err error) { parsed, parseErr := url.Parse(raw) if parseErr != nil { @@ -45,25 +58,32 @@ func CanonicalizeOrigin(raw string) (origin, host string, err error) { 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. + // A fully-qualified name's trailing dot names the same host, and NEITHER + // scheme's default port is 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 != "" { + if port := parsed.Port(); port != "" { host += ":" + port } + host = normalizeHost(host) + + // The round-trip property, asserted rather than assumed: normalizeHost is + // idempotent today, and this is the line that fails loudly at startup + // rather than minting a permanently unroutable namespace if a future edit + // to either rule breaks the agreement. + if reduced := normalizeHost(host); reduced != host { + return "", "", errors.NewValidationError("origin", + fmt.Sprintf("canonical host %q does not survive Host normalization (%q): "+ + "actors minted here would be unreachable under every Host spelling", host, reduced)) + } return scheme + "://" + host, host, nil } diff --git a/internal/personas/personas.go b/internal/personas/personas.go index 2609258..2756207 100644 --- a/internal/personas/personas.go +++ b/internal/personas/personas.go @@ -13,6 +13,9 @@ import ( "log/slog" "net/http" "strconv" + "strings" + + "github.com/bluesky-social/indigo/atproto/syntax" "tidepool/internal/ap" "tidepool/internal/errors" @@ -146,6 +149,9 @@ func New(opts Options) (*Service, error) { // The actor is Person, not Service: Lemmy classifies Service actors as bots // and drops their votes. func (s *Service) CreateActorForDID(ctx context.Context, did, handle string) (*store.APActor, error) { + if err := validateActorDID(did); err != nil { + return nil, err + } existing, err := s.actors.GetByDID(ctx, did) if err == nil { return existing, nil @@ -220,6 +226,40 @@ func (s *Service) CreateActorForDID(ctx context.Context, did, handle string) (*s return nil, fmt.Errorf("%w: %q after %d attempts", ErrLocalPartExhausted, base, maxLocalPartAttempts) } +// validateActorDID gates the OTHER frozen identity input. DeriveLocalPart runs +// the handle through atproto handle syntax before freezing it; the DID is +// frozen just as hard — it is concatenated into actor_id, and from there into +// keyId, the WebFinger href, and the signing identity — and used to arrive with +// no gate at all. +// +// Two rules, both about SERVABILITY, since a minted actor that cannot answer +// its own URL can never be repaired (actor_id is frozen and re-minting would +// orphan every signature the published key has already made): +// +// - atproto DID syntax, the same library gate the handle gets. It rejects the +// empty string, a missing "did:" prefix, whitespace, and — the one that +// matters here — any "/", which ServeHTTP treats as a 404 on /ap/actor/, +// so such an actor could never serve its own document. +// +// - no "%". DID syntax permits percent-encoding, but the row stores the DID +// VERBATIM while r.URL.Path arrives percent-DECODED, so a peer fetching the +// exact actor_id we published looks up "did:web:example.com:8080" against a +// row keyed "did:web:example.com%3A8080" and misses forever. +// +// Failure is a ValidationError: permanent, never retryable. A caller that +// re-queued this DID would re-fail on every attempt until something upstream +// stopped handing it a malformed identifier. +func validateActorDID(did string) error { + if _, err := syntax.ParseDID(did); err != nil { + return errors.NewValidationError("did", fmt.Sprintf("%q is not a valid DID: %v", did, err)) + } + if strings.Contains(did, "%") { + return errors.NewValidationError("did", fmt.Sprintf( + "%q is percent-encoded: the served path arrives decoded, so this actor could never be looked up again", did)) + } + return nil +} + // suffixedLocalPart names the attempt'th claimant of base: the first keeps // the bare local part, later ones get "-2", "-3", ... There is no "-1" — // the bare name IS the first claim. diff --git a/internal/personas/serving.go b/internal/personas/serving.go index 8ca1a3d..c1b5c2b 100644 --- a/internal/personas/serving.go +++ b/internal/personas/serving.go @@ -271,8 +271,12 @@ func (s *Service) handleActorDocument(w http.ResponseWriter, r *http.Request, di s.writeStoreError(w, r, err) return } - if !s.servesActor(actor, r) { - http.NotFound(w, r) + // The gate runs BEFORE the tombstone branch: the Tombstone body publishes + // the same actor_id (as its id, and inside publicKey), so a row this + // refuses to serve as a Person must not slip out as a Gone document either. + origin, err := s.servedActor(actor, normalizeHost(r.Host)) + if err != nil { + s.writeServedActorError(w, r, actor, err) return } // WITHDRAWN (task 17d's destructive tier): 410 Gone, and specifically not @@ -310,13 +314,6 @@ func (s *Service) handleActorDocument(w http.ResponseWriter, r *http.Request, di return } - 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 @@ -370,8 +367,11 @@ func (s *Service) handleOutbox(w http.ResponseWriter, r *http.Request, did strin s.writeStoreError(w, r, err) return } - if !s.servesActor(actor, r) { - http.NotFound(w, r) + // Same gate as the actor document, and for the same reason: this collection's + // id IS the actor_id with a suffix, so an id the actor document refuses to + // publish must not reach a peer through the outbox instead. + if _, err := s.servedActor(actor, normalizeHost(r.Host)); err != nil { + s.writeServedActorError(w, r, actor, err) return } // The outbox answers the same way the actor does. An actor that is Gone with @@ -468,7 +468,11 @@ func (s *Service) lookupResource(ctx context.Context, resource, host string) (*s if normalizeHost(acctHost) != host { return nil, errors.NewNotFoundError("ap_actor", resource) } - return s.actors.GetByOriginLocalPart(ctx, host, strings.ToLower(local)) + actor, err := s.actors.GetByOriginLocalPart(ctx, host, strings.ToLower(local)) + if err != nil { + return nil, err + } + return s.servedActorOrError(actor, host) } parsed, err := url.Parse(resource) @@ -487,28 +491,93 @@ func (s *Service) lookupResource(ctx context.Context, resource, host string) (*s return nil, err } // The DID is global but this answer must not be: an actor minted on - // another origin does not resolve here. - if actor.NormalizedOrigin != host { - return nil, errors.NewNotFoundError("ap_actor", resource) + // another origin does not resolve here. servedActor is what enforces that, + // together with the actor_id check WebFinger needs most — its href is the + // value every remote resolver caches. + return s.servedActorOrError(actor, host) +} + +// servedActorOrError runs the served-actor gate and hands back the actor, so +// lookupResource's two spellings both return a row that has already been +// checked. WebFinger never sees an actor the other two handlers would refuse. +func (s *Service) servedActorOrError(actor *store.APActor, host string) (*store.APActor, error) { + if _, err := s.servedActor(actor, host); err != nil { + s.logUnusableActor(actor, err) + return nil, err } 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) +// writeServedActorError reports a refused actor. A host miss is an ordinary +// 404 — this origin simply does not host that account. A corrupt actor_id is +// OUR bug: 500, never 404, because a resolver caches a 404 as "no such account" +// and would stop asking about a row an operator can still repair. +func (s *Service) writeServedActorError(w http.ResponseWriter, r *http.Request, actor *store.APActor, err error) { + if errors.IsNotFound(err) { + http.NotFound(w, r) + return + } + s.logUnusableActor(actor, err) + http.Error(w, "internal error", http.StatusInternalServerError) +} + +// logUnusableActor records the offending row. Nothing else in the system would +// ever mention it: the response body says "internal error", and a row that +// cannot state its own identity is not something to discover from a traffic +// graph. +func (s *Service) logUnusableActor(actor *store.APActor, err error) { + if errors.IsNotFound(err) { + return + } + s.logger.Error("refusing to publish an actor whose stored actor_id is unusable", + "did", actor.DID, "actor_id", actor.ActorID, + "normalized_origin", actor.NormalizedOrigin, "error", err) +} + +// servedActor is THE gate every surface that publishes an actor_id passes +// through: the actor document, the outbox, and WebFinger. It answers "is this +// row safe to publish under the routed Host", and returns the origin the row's +// ids sit on so the caller does not re-derive it. +// +// It exists as ONE function because the guard is only worth what its narrowest +// coverage is. The fail-closed actor_id check used to live inside +// handleActorDocument alone, so a row it refused to serve as a Person document +// still went out as an outbox collection and — worse — as WebFinger's alias and +// both rel=self hrefs, which is the value every remote resolver caches and +// re-fetches. Three handlers publishing one field cannot each carry their own +// idea of when that field is trustworthy. +// +// Two conditions, both fail-closed: +// +// - HOST BINDING. The DID is global but the actor is not: serving a +// vanity-origin actor 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. A miss here is NotFound, so it reads as "no such account". +// +// - ID INTEGRITY. The stored actor_id must be an absolute URL on the SAME +// authority the row is bound to. A row failing that is corrupt, and every +// document built from it would pair this origin's inbox and key with an id +// that belongs to someone else — quietly, and cached by every peer that +// fetched it. This is not a miss and must not be cached as one: it surfaces +// as a plain error, which the callers log and answer 500 for. +func (s *Service) servedActor(actor *store.APActor, host string) (string, error) { + if actor.NormalizedOrigin != host { + return "", errors.NewNotFoundError("ap_actor", actor.DID) + } + origin, err := actorOrigin(actor) + if err != nil { + return "", err + } + return origin, nil } // 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. 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. +// than the configured one. It FAILS CLOSED: an actor_id that will not parse, +// or that names an authority other than the one the row is bound to, means the +// row is corrupt, and publishing it would put this origin's inbox and key on a +// document whose id belongs to a different authority. func actorOrigin(actor *store.APActor) (string, error) { parsed, err := url.Parse(actor.ActorID) if err != nil { @@ -517,6 +586,13 @@ func actorOrigin(actor *store.APActor) (string, error) { if parsed.Scheme == "" || parsed.Host == "" { return "", fmt.Errorf("actor_id %q is not an absolute URL", actor.ActorID) } + // normalized_origin is derived from actor_id at mint, so the two can only + // disagree if the row was written by something other than the mint path or + // edited afterwards. Either way the row no longer states one identity. + if host := normalizeHost(parsed.Host); host != actor.NormalizedOrigin { + return "", fmt.Errorf("actor_id %q names authority %q but the row is bound to %q", + actor.ActorID, host, actor.NormalizedOrigin) + } return parsed.Scheme + "://" + parsed.Host, nil }