Fix critical authentication bypass vulnerability in inbox handler

This fixes a severe security vulnerability where activities were processed
before verifying that the HTTP signature key belonged to the claimed actor,
allowing attackers to impersonate any ActivityPub user.

The fix moves the authentication check (doesActorOwnKey) to occur before
calling routeActivity(), ensuring that malicious activities with mismatched
signatures are rejected before any processing occurs.

A comprehensive test case has been added to verify the fix and prevent
regression of this critical security issue.

GHSA-6jcc-xgcr-q3h4

Author: dahlia

CHANGES.md

@@ -8,6 +8,15 @@ Version 1.3.20
 
 To be released.
 
+ -  Fixed a critical authentication bypass vulnerability in the inbox handler
+    that allowed unauthenticated attackers to impersonate any ActivityPub actor.
+    The vulnerability occurred because activities were processed before
+    verifying that the HTTP Signatures key belonged to the claimed actor.
+    Now authentication verification is performed before activity processing to
+    prevent actor impersonation attacks.  [[CVE-2025-54888]]
+
+[CVE-2025-54888]: https://github.com/fedify-dev/fedify/security/advisories/GHSA-6jcc-xgcr-q3h4
+
 
 Version 1.3.19
 --------------

src/federation/handler.test.ts

@@ -35,6 +35,7 @@ import {
   respondWithObject,
   respondWithObjectIfAcceptable,
 } from "./handler.ts";
+import { InboxListenerSet } from "./inbox.ts";
 import { MemoryKvStore } from "./kv.ts";
 
 test("acceptsJsonLd()", () => {
@@ -1279,6 +1280,86 @@ test("respondWithObject()", async () => {
   });
 });
 
+test("handleInbox() - authentication bypass vulnerability", async () => {
+  // This test reproduces the authentication bypass vulnerability where
+  // activities are processed before verifying the signing key belongs
+  // to the claimed actor
+
+  let processedActivity: Create | undefined;
+  const inboxListeners = new InboxListenerSet<void>();
+  inboxListeners.add(Create, (_ctx, activity) => {
+    // Track that the malicious activity was processed
+    processedActivity = activity;
+  });
+
+  // Create malicious activity claiming to be from victim actor
+  const maliciousActivity = new Create({
+    id: new URL("https://attacker.example.com/activities/malicious"),
+    actor: new URL("https://victim.example.com/users/alice"), // Impersonating victim
+    object: new Note({
+      id: new URL("https://attacker.example.com/notes/forged"),
+      attribution: new URL("https://victim.example.com/users/alice"),
+      content: "This is a forged message from the victim!",
+    }),
+  });
+
+  // Sign request with attacker's key (not victim's key)
+  const maliciousRequest = await signRequest(
+    new Request("https://example.com/", {
+      method: "POST",
+      body: JSON.stringify(await maliciousActivity.toJsonLd()),
+    }),
+    rsaPrivateKey3, // Attacker's private key
+    rsaPublicKey3.id!, // Attacker's public key ID
+  );
+
+  const maliciousContext = createRequestContext({
+    request: maliciousRequest,
+    url: new URL(maliciousRequest.url),
+    data: undefined,
+    documentLoader: mockDocumentLoader,
+  });
+
+  const actorDispatcher: ActorDispatcher<void> = (_ctx, identifier) => {
+    if (identifier !== "someone") return null;
+    return new Person({ name: "Someone" });
+  };
+
+  const response = await handleInbox(maliciousRequest, {
+    recipient: "someone",
+    context: maliciousContext,
+    inboxContextFactory(_activity) {
+      return createInboxContext({ ...maliciousContext, recipient: "someone" });
+    },
+    kv: new MemoryKvStore(),
+    kvPrefixes: {
+      activityIdempotence: ["_fedify", "activityIdempotence"],
+      publicKey: ["_fedify", "publicKey"],
+    },
+    actorDispatcher,
+    inboxListeners,
+    onNotFound: () => new Response("Not found", { status: 404 }),
+    signatureTimeWindow: { minutes: 5 },
+    skipSignatureVerification: false,
+  });
+
+  // The vulnerability: Even though the response is 401 (unauthorized),
+  // the malicious activity was already processed by routeActivity()
+  assertEquals(response.status, 401);
+  assertEquals(await response.text(), "The signer and the actor do not match.");
+
+  assertEquals(
+    processedActivity,
+    undefined,
+    `SECURITY VULNERABILITY: Malicious activity with mismatched signature was processed! ` +
+      `Activity ID: ${processedActivity?.id?.href}, ` +
+      `Claimed actor: ${processedActivity?.actorId?.href}`,
+  );
+
+  // If we reach here, the vulnerability is fixed - activities with mismatched
+  // signatures are properly rejected before processing
+});
+
 test("respondWithObjectIfAcceptable", async () => {
   let request = new Request("https://example.com/", {
     headers: { Accept: "application/activity+json" },

src/federation/handler.ts

@@ -649,20 +649,6 @@ async function handleInboxInternal<TContextData>(
     span.setAttribute("activitypub.activity.id", activity.id.href);
   }
   span.setAttribute("activitypub.activity.type", getTypeId(activity).href);
-  const routeResult = await routeActivity({
-    context: ctx,
-    json,
-    activity,
-    recipient,
-    inboxListeners,
-    inboxContextFactory,
-    inboxErrorHandler,
-    kv,
-    kvPrefixes,
-    queue,
-    span,
-    tracerProvider,
-  });
   if (
     httpSigKey != null && !await doesActorOwnKey(activity, httpSigKey, ctx)
   ) {
@@ -685,6 +671,20 @@ async function handleInboxInternal<TContextData>(
       headers: { "Content-Type": "text/plain; charset=utf-8" },
     });
   }
+  const routeResult = await routeActivity({
+    context: ctx,
+    json,
+    activity,
+    recipient,
+    inboxListeners,
+    inboxContextFactory,
+    inboxErrorHandler,
+    kv,
+    kvPrefixes,
+    queue,
+    span,
+    tracerProvider,
+  });
   if (routeResult === "alreadyProcessed") {
     return new Response(
       `Activity <${activity.id}> has already been processed.`,