fix(registry): adversarial review round 6 findings

Addresses 7 findings from the round-6 adversarial review and
documents the eighth.

#1 (high) Capability consent bypass [registry.ts, RegistryPluginDetail.tsx]
The drift check was gated on the client sending acknowledgedDeclaredAccess.
If the publisher's release record had no extension, the admin saw an
empty permission dialog, omitted the acknowledgement, and the server
skipped the check entirely -- letting a bundle whose manifest declares
real capabilities slip through behind an empty consent UI. Server now
extracts capabilities from the bundle manifest after download and
refuses with DECLARED_ACCESS_REQUIRED if the bundle declares any
capabilities and no acknowledgement was sent. Client always sends the
list (empty when no extension) so the new server check is always armed.

#2 (high) Concurrent install bundle deletion [registry.ts]
Two parallel installs of the same (did, slug, version) both passed the
pre-existing-row check, both uploaded to the same deterministic R2
prefix, and one then won the state-row PK race. The loser's catch block
deleted the R2 bundle the winner had just written. On state-write
failure we now re-query the state row: if a winner exists, we lost the
race and must not touch the R2 bundle. Cleanup runs only when the
failure is a real DB error, not a lost concurrent install.

#3 (high) SSRF via DNS-resolving public hostnames [registry.ts, ssrf.ts moved]
Literal-IP blocklist alone left a DNS-rebinding gap: any public DNS
service resolving an attacker-chosen hostname to loopback / RFC1918 /
169.254.169.254 passed the URL check. The import pipeline already
shipped resolveAndValidateExternalUrl which does Cloudflare DoH
resolution and rejects on any forbidden resolved address; reuse it
for artifact downloads. Move src/import/ssrf.ts to src/security/ssrf.ts
to reflect that it's not import-specific. Leave a re-export shim at
the old path so 13 existing callers keep working unchanged. Add
#security/* path alias.

#5 (high) Aggregator-supplied handles treated as verified [PublisherHandle.tsx]
usePublisherHandle returned status: 'ok' with the aggregator-supplied
handle whenever one was present, skipping local DID->handle round-trip.
A compromised aggregator could label an attacker DID as e.g.
'stripe.com' and the UI would render it as verified. Always run
LocalActorResolver via resolveDidToHandle; use the aggregator handle
only for a cross-check. If the aggregator's claim differs from the
verified handle, mark the publisher invalid.

#6 (medium) Postgres migration 038 schema-qualification [038_registry_plugin_state.ts]
The columns probe queried information_schema.columns without filtering
by table_schema. A _plugin_state table in another schema (multi-tenant
Postgres, per-test schemas) could make the migration skip the column
adds. Filter by table_schema = current_schema().

#7 (medium) Install errors leak full artifact URLs [registry.ts]
fetchArtifact recorded each full URL in the joined error message that
bubbled up to the admin client. Artifacts hosted on storage backends
often carry presigned tokens in the query string; failed installs were
leaking those into HTTP responses and logs. Strip query and fragment
when building client-visible errors (origin + path only); log the full
URL server-side for debugging.

#8 (medium) Credentialed aggregator URLs accepted [config.ts]
validateAggregatorUrl accepted https://user:pass@example.com.
The normalized URL ends up in the admin manifest and is shipped to
every admin browser; browser fetch() also rejects credentialed URLs
outright. Reject them at config-validation time.

#4 (high, documented not fixed) Aggregator-trust-root scope [types.ts]
Full MST proof / publisher signature verification is not in this PR;
the server still trusts the aggregator-supplied (did, slug, checksum,
artifact URL). Expand the JSDoc on EmDashConfig.experimental.registry
to spell out exactly what the v1 trust contract is, what EmDash does
verify independently (checksum, manifest id/version/capabilities), and
what it doesn't (release-record signatures, replay). Recommendation:
point aggregatorUrl only at an aggregator you operate or trust at
centralized-source level until signature verification lands.

Author: ascorbic

packages/admin/src/components/PublisherHandle.tsx

@@ -1,23 +1,30 @@
 /**
  * Renders an atproto publisher's identity, with three branches:
  *
- *   - **Verified handle**: shows `@handle`. Either the aggregator
- *     already resolved the handle at ingest (we trust that), or our
- *     local `LocalActorResolver` round-tripped the DID document's
- *     `alsoKnownAs` back to the same DID.
+ *   - **Verified handle**: shows `@handle`. Our local
+ *     `LocalActorResolver` round-tripped the DID document's
+ *     `alsoKnownAs` back to the same DID (verified by DNS TXT or
+ *     `.well-known`, not by the aggregator).
  *   - **Unverified publisher**: DID document claims a handle but the
- *     handle's domain doesn't point back to the same DID. Treat as
- *     untrusted -- the publisher might be impersonating someone else.
- *     Surface as `Unverified publisher` in error styling. Callers
- *     should also disable destructive actions (install, etc.).
- *   - **Missing handle**: no claimed handle at all (or DID document
- *     resolution failed entirely). Fall back to the raw DID.
+ *     handle's domain doesn't point back to the same DID, OR the
+ *     aggregator's claimed handle doesn't match the bidirectionally
+ *     verified one. Treat as untrusted -- the publisher might be
+ *     impersonating someone else, or the aggregator might be lying
+ *     about a handle. Surface as `Unverified publisher` in error
+ *     styling. Callers should also disable destructive actions
+ *     (install, etc.).
+ *   - **Missing handle**: no handle claimed in the DID document (no
+ *     `alsoKnownAs`), or the DID document couldn't be fetched
+ *     (network error, unsupported DID method).
  *
  * `aggregatorHandle` is what the registry's `searchPackages` /
- * `resolvePackage` endpoint returned for this DID -- best-effort, may
- * be `null`. When absent, this component falls back to a per-DID
- * `LocalActorResolver` lookup via `resolveDidToHandle`, cached in
- * localStorage for 24h so repeat renders don't refetch.
+ * `resolvePackage` endpoint returned for this DID. It is NEVER trusted
+ * on its own -- the aggregator is an untrusted indexer that could be
+ * compromised or buggy. We always run our own DID->handle round-trip
+ * via `LocalActorResolver` (cached in localStorage for 24h) and use
+ * the aggregator's value only to *cross-check*: if the aggregator
+ * claims a handle that differs from what the DID document
+ * bidirectionally verifies, the publisher is marked invalid.
  */
 
 import { useLingui } from "@lingui/react/macro";
@@ -26,6 +33,9 @@ import * as React from "react";
 
 import { resolveDidToHandle } from "../lib/api/registry.js";
 
+/** Trailing dot(s) on an FQDN, stripped before handle comparison. */
+const TRAILING_DOT = /\.+$/;
+
 export type PublisherHandleStatus = "ok" | "invalid" | "missing";
 
 export interface PublisherHandleResult {
@@ -57,19 +67,48 @@ export function usePublisherHandle(
 	did: string,
 	aggregatorHandle?: string | null,
 ): PublisherHandleResult {
-	const { data: didHandleResolution } = useQuery({
+	// Always run the local DID->handle round-trip. We never trust the
+	// aggregator's `aggregatorHandle` on its own: a compromised
+	// aggregator could label an attacker DID as `stripe.com` and any
+	// shortcut that returns the aggregator's value as verified would
+	// let the impersonation through unchecked.
+	const { data: didHandleResolution, isPending } = useQuery({
 		queryKey: ["registry", "did-handle", did],
 		queryFn: () => resolveDidToHandle(did),
-		enabled: Boolean(did) && !aggregatorHandle,
+		enabled: Boolean(did),
 		staleTime: 5 * 60 * 1000,
 	});
 
-	if (aggregatorHandle) return { status: "ok", handle: aggregatorHandle };
-	if (!didHandleResolution) return { status: "missing" };
-	if (didHandleResolution.status === "ok") {
-		return { status: "ok", handle: didHandleResolution.handle };
+	if (isPending || !didHandleResolution) return { status: "missing" };
+
+	// DID document didn't claim a handle (or the document was
+	// unreachable). The aggregator might have one, but without our own
+	// verification we can't display it.
+	if (didHandleResolution.status === "missing") {
+		return { status: "missing" };
+	}
+
+	// DID document claims a handle but it doesn't round-trip.
+	// `invalid` always wins over an aggregator-supplied handle.
+	if (didHandleResolution.status === "invalid") {
+		return { status: "invalid" };
 	}
-	return { status: didHandleResolution.status };
+
+	// Bidirectionally verified handle. Cross-check against the
+	// aggregator's claim: if they differ, flag the publisher as
+	// invalid. The aggregator may simply be stale, but we shouldn't
+	// silently disagree with our own verification by showing the
+	// aggregator's value -- the conservative read is "something is
+	// off, surface it to the admin".
+	const verifiedHandle = didHandleResolution.handle.toLowerCase();
+	if (aggregatorHandle) {
+		const claimed = aggregatorHandle.toLowerCase().replace(TRAILING_DOT, "");
+		if (claimed !== verifiedHandle) {
+			return { status: "invalid" };
+		}
+	}
+
+	return { status: "ok", handle: didHandleResolution.handle };
 }
 
 export function PublisherHandle({

packages/admin/src/components/RegistryPluginDetail.tsx

@@ -147,17 +147,24 @@ export function RegistryPluginDetail({ pluginId, config }: RegistryPluginDetailP
 				did: pkg.did,
 				slug,
 				version: release?.version,
-				// Only send the acknowledgement when the dialog had real
-				// capability data to display. The server's drift check is
-				// gated on `acknowledgedDeclaredAccess !== undefined`, so
-				// omitting the field opts out of the check entirely --
-				// correct behaviour for the (currently common) case where
-				// the publisher's release record doesn't yet carry an
-				// extension block. The bundle's actual capabilities are
-				// still bound to the checksum-verified bytes; the drift
-				// check is a UX sanity belt for already-displayed
-				// consent, not an authorization gate.
-				acknowledgedDeclaredAccess: capabilities.length > 0 ? capabilities : undefined,
+				// Always send the acknowledgement, even when the dialog
+				// showed no permissions. The server compares this list
+				// against the bundle's actual `manifest.capabilities`
+				// after download:
+				//
+				//   - If the bundle has capabilities, the server
+				//     requires us to send a matching list (the consent
+				//     dialog is the only place the admin sees what
+				//     they're agreeing to).
+				//   - If the bundle has no capabilities, no consent is
+				//     required and the server ignores this field.
+				//
+				// Sending the empty list when the release extension was
+				// missing means a publisher who ships a bundle with
+				// permissions but no extension block can't sneak the
+				// permissions past an empty consent dialog -- the
+				// server will refuse with `DECLARED_ACCESS_REQUIRED`.
+				acknowledgedDeclaredAccess: capabilities,
 			});
 		},
 		onSuccess: () => {

packages/core/package.json

@@ -143,6 +143,7 @@
     "#menus/*": "./src/menus/*",
     "#widgets/*": "./src/widgets/*",
     "#import/*": "./src/import/*",
+    "#security/*": "./src/security/*",
     "#utils/*": "./src/utils/*",
     "#preview/*": "./src/preview/*",
     "#seed/*": "./src/seed/*",