diff --git a/CHANGES.md b/CHANGES.md index 6189ca7a..c01c76b2 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,17 @@ Version 2.1.0 To be released. +### @fedify/fedify + + - Fixed `RequestContext.getSignedKeyOwner()` to return `null` instead of + throwing an error when the remote server requires authorized fetch and + returns `401 Unauthorized` for the key owner lookup. Previously, this + caused a `500 Internal Server Error` when interoperating with servers like + GoToSocial that have authorized fetch enabled. [[#473], [#589]] + +[#473]: https://github.com/fedify-dev/fedify/issues/473 +[#589]: https://github.com/fedify-dev/fedify/pull/589 + ### @fedify/init - Changed `fedify init` to add `"temporal"` to `deno.json`'s `"unstable"` diff --git a/docs/manual/access-control.md b/docs/manual/access-control.md index c89c1147..d5004f7f 100644 --- a/docs/manual/access-control.md +++ b/docs/manual/access-control.md @@ -166,14 +166,33 @@ Instance actor -------------- When you enable authorized fetch, you need to fetch actors from other servers -to retrieve their public keys. However, fetching resources from other servers -may cause an infinite loop if the other server also requires authorized fetch, -which causes another request to your server for the public key, and so on. - -The most common way to prevent it is a pattern called [instance actor], which -is an actor that represents the whole instance and exceptionally does not -require authorized fetch. You can use the instance actor to fetch resources -from other servers without causing an infinite loop. +to retrieve their public keys. However, this can cause problems when the other +server also has authorized fetch enabled: + + - *Unauthenticated appearance*: If the remote server requires a signed + request to fetch its actor, and your server fetches it without a signature, + the remote server returns an HTTP 401 error. In this case, + `~RequestContext.getSignedKeyOwner()` returns `null`, so the requester + appears unauthenticated to your `AuthorizePredicate`—which will typically + deny the request. + + - *Infinite loop*: If both servers require authorized fetch, fetching the + remote actor requires your server to be authenticated, which in turn + requires fetching *your* actor, which requires authentication, and so on. + +> [!NOTE] +> Even without the infinite loop, if the remote server requires authorized +> fetch, `~RequestContext.getSignedKeyOwner()` returns `null` for requests +> from that server (since fetching the key owner fails with HTTP 401). +> This means such requests appear unauthenticated to your +> `AuthorizePredicate`. To properly authenticate those requests, implement +> the instance actor pattern below. + +The most common way to prevent both problems is a pattern called +[instance actor], which is an actor that represents the whole instance and +exceptionally does not require authorized fetch. You can use the instance +actor to fetch resources from other servers with a valid signature, without +causing an infinite loop. Usually, many ActivityPub implementations name their instance actor as their domain name, such as `example.com@example.com`. Here is an example of how to diff --git a/packages/fedify/src/federation/middleware.test.ts b/packages/fedify/src/federation/middleware.test.ts index dd2cb5a4..7f065673 100644 --- a/packages/fedify/src/federation/middleware.test.ts +++ b/packages/fedify/src/federation/middleware.test.ts @@ -35,7 +35,7 @@ import { rsaPublicKey2, rsaPublicKey3, } from "../testing/keys.ts"; -import { getDocumentLoader } from "@fedify/vocab-runtime"; +import { FetchError, getDocumentLoader } from "@fedify/vocab-runtime"; import { getAuthenticatedDocumentLoader } from "../utils/docloader.ts"; const documentLoader = getDocumentLoader(); @@ -942,6 +942,52 @@ test({ ); }); + await t.step( + "RequestContext.getSignedKeyOwner() returns null on FetchError", + async () => { + // Regression test for : + // When the key owner actor fetch fails (e.g., GoToSocial returns 401 for + // authorized fetch), getSignedKeyOwner() should return null instead of + // throwing a FetchError. + // + // Custom document loader that simulates a server with authorized fetch + // enabled (returns 401 for actor URL but allows key URL with fragment): + const customDocumentLoader = async (url: string) => { + if (url === "https://example.com/person2#key3") { + // Key URL (with fragment): return actor document for sig verification + return await mockDocumentLoader("https://example.com/person2"); + } + if (url === "https://example.com/person2") { + // Actor URL (without fragment): simulate 401 Unauthorized + throw new FetchError( + new URL(url), + "HTTP 401: Unauthorized", + ); + } + return mockDocumentLoader(url); + }; + + const signedReq = await signRequest( + new Request("https://example.com/", { + headers: { accept: "application/activity+json" }, + }), + rsaPrivateKey3, + rsaPublicKey3.id!, + ); + + const fed = createFederation({ + kv, + documentLoaderFactory: () => customDocumentLoader, + contextLoaderFactory: () => mockDocumentLoader, + }); + const ctx = fed.createContext(signedReq, undefined); + + // Before fix: throws FetchError (causes 500 Internal Server Error) + // After fix: returns null gracefully + assertEquals(await ctx.getSignedKeyOwner(), null); + }, + ); + await t.step("RequestContext.clone()", () => { const federation = createFederation({ kv, diff --git a/packages/fedify/src/federation/middleware.ts b/packages/fedify/src/federation/middleware.ts index 0e2a7cef..476f53be 100644 --- a/packages/fedify/src/federation/middleware.ts +++ b/packages/fedify/src/federation/middleware.ts @@ -22,7 +22,7 @@ import type { DocumentLoaderFactoryOptions, GetUserAgentOptions, } from "@fedify/vocab-runtime"; -import { getDocumentLoader } from "@fedify/vocab-runtime"; +import { FetchError, getDocumentLoader } from "@fedify/vocab-runtime"; import type { LookupWebFingerOptions, ResourceDescriptor, @@ -2705,11 +2705,24 @@ class RequestContextImpl extends ContextImpl if (this.#signedKeyOwner != null) return this.#signedKeyOwner; const key = await this.getSignedKey(options); if (key == null) return this.#signedKeyOwner = null; - return this.#signedKeyOwner = await getKeyOwner(key, { - contextLoader: options.contextLoader ?? this.contextLoader, - documentLoader: options.documentLoader ?? this.documentLoader, - tracerProvider: options.tracerProvider ?? this.tracerProvider, - }); + try { + return this.#signedKeyOwner = await getKeyOwner(key, { + contextLoader: options.contextLoader ?? this.contextLoader, + documentLoader: options.documentLoader ?? this.documentLoader, + tracerProvider: options.tracerProvider ?? this.tracerProvider, + }); + } catch (error) { + if (error instanceof FetchError) { + getLogger(["fedify", "federation", "actor"]).warn( + "Failed to fetch the key owner {keyOwner} of {keyId} while " + + "verifying the request signature; treating the request as " + + "unauthenticated: {error}", + { keyId: key.id?.href, keyOwner: error.url.href, error }, + ); + return this.#signedKeyOwner = null; + } + throw error; + } } }