-
-
Notifications
You must be signed in to change notification settings - Fork 84
feat: Add -A/--authorized-fetch option to fedify inbox command
#474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,137 @@ | ||||||
| import { assertEquals } from "@std/assert"; | ||||||
| import { signRequest } from "../sig/http.ts"; | ||||||
| import { | ||||||
| createInboxContext, | ||||||
| createRequestContext, | ||||||
| } from "../testing/context.ts"; | ||||||
| import { mockDocumentLoader } from "../testing/docloader.ts"; | ||||||
| import { | ||||||
| rsaPrivateKey3, | ||||||
| rsaPublicKey3, | ||||||
| } from "../testing/keys.ts"; | ||||||
| import { test } from "../testing/mod.ts"; | ||||||
| import { Create, Note, Person } from "../vocab/vocab.ts"; | ||||||
| import type { ActorDispatcher } from "./callback.ts"; | ||||||
| import { handleInbox } from "./handler.ts"; | ||||||
| import { MemoryKvStore } from "./kv.ts"; | ||||||
| import { createFederation } from "./middleware.ts"; | ||||||
|
|
||||||
| test("handleInbox() with requireHttpSignature option", async () => { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests for It would be beneficial to add a test case for when an activity is successfully parsed from a Linked Data Signature (so A suggested test flow would be:
This would provide more complete coverage for this new security feature. |
||||||
| const activity = new Create({ | ||||||
| id: new URL("https://example.com/activities/1"), | ||||||
| actor: new URL("https://example.com/person2"), | ||||||
| object: new Note({ | ||||||
| id: new URL("https://example.com/notes/1"), | ||||||
| attribution: new URL("https://example.com/person2"), | ||||||
| content: "Hello, world!", | ||||||
| }), | ||||||
| }); | ||||||
|
|
||||||
| const unsignedRequest = new Request("https://example.com/inbox", { | ||||||
| method: "POST", | ||||||
| headers: { "Content-Type": "application/activity+json" }, | ||||||
| body: JSON.stringify(await activity.toJsonLd()), | ||||||
| }); | ||||||
|
|
||||||
| const federation = createFederation<void>({ kv: new MemoryKvStore() }); | ||||||
| const unsignedContext = createRequestContext({ | ||||||
| federation, | ||||||
| request: unsignedRequest, | ||||||
| url: new URL(unsignedRequest.url), | ||||||
| data: undefined, | ||||||
| }); | ||||||
|
|
||||||
| const actorDispatcher: ActorDispatcher<void> = (_ctx, identifier) => { | ||||||
| if (identifier !== "testuser") return null; | ||||||
| return new Person({ name: "Test User" }); | ||||||
| }; | ||||||
|
|
||||||
| const onNotFound = () => new Response("Not found", { status: 404 }); | ||||||
|
|
||||||
| const baseInboxOptions = { | ||||||
| kv: new MemoryKvStore(), | ||||||
| kvPrefixes: { | ||||||
| activityIdempotence: ["_fedify", "activityIdempotence"] as const, | ||||||
| publicKey: ["_fedify", "publicKey"] as const, | ||||||
| }, | ||||||
| actorDispatcher, | ||||||
| onNotFound, | ||||||
| signatureTimeWindow: { minutes: 5 } as const, | ||||||
| skipSignatureVerification: false, | ||||||
| }; | ||||||
|
|
||||||
| let response = await handleInbox(unsignedRequest, { | ||||||
| recipient: null, | ||||||
| context: unsignedContext, | ||||||
| inboxContextFactory(_activity) { | ||||||
| return createInboxContext({ ...unsignedContext, clone: undefined }); | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
|
||||||
| }, | ||||||
| ...baseInboxOptions, | ||||||
| requireHttpSignature: false, | ||||||
| }); | ||||||
| assertEquals( | ||||||
| response.status, | ||||||
| 401, | ||||||
| "Without HTTP Sig and no LD Sig/OIP, should return 401", | ||||||
| ); | ||||||
|
|
||||||
| response = await handleInbox(unsignedRequest.clone() as Request, { | ||||||
| recipient: null, | ||||||
| context: unsignedContext, | ||||||
| inboxContextFactory(_activity) { | ||||||
| return createInboxContext({ ...unsignedContext, clone: undefined }); | ||||||
| }, | ||||||
| ...baseInboxOptions, | ||||||
| requireHttpSignature: true, | ||||||
| }); | ||||||
| assertEquals( | ||||||
| response.status, | ||||||
| 401, | ||||||
| "With requireHttpSignature: true and no HTTP Sig, should return 401", | ||||||
| ); | ||||||
|
|
||||||
| const signedRequest = await signRequest( | ||||||
| unsignedRequest.clone() as Request, | ||||||
| rsaPrivateKey3, | ||||||
| rsaPublicKey3.id!, | ||||||
| ); | ||||||
| const signedContext = createRequestContext({ | ||||||
| federation, | ||||||
| request: signedRequest, | ||||||
| url: new URL(signedRequest.url), | ||||||
| data: undefined, | ||||||
| documentLoader: mockDocumentLoader, | ||||||
| }); | ||||||
|
|
||||||
| response = await handleInbox(signedRequest, { | ||||||
| recipient: null, | ||||||
| context: signedContext, | ||||||
| inboxContextFactory(_activity) { | ||||||
| return createInboxContext({ ...signedContext, clone: undefined }); | ||||||
| }, | ||||||
| ...baseInboxOptions, | ||||||
| requireHttpSignature: true, | ||||||
| }); | ||||||
| assertEquals( | ||||||
| response.status, | ||||||
| 202, | ||||||
| "With requireHttpSignature: true and valid HTTP Sig, should succeed", | ||||||
| ); | ||||||
|
|
||||||
| // `skipSignatureVerification` takes precedence over `requireHttpSignature` | ||||||
| response = await handleInbox(unsignedRequest.clone() as Request, { | ||||||
| recipient: null, | ||||||
| context: unsignedContext, | ||||||
| inboxContextFactory(_activity) { | ||||||
| return createInboxContext({ ...unsignedContext, clone: undefined }); | ||||||
| }, | ||||||
| ...baseInboxOptions, | ||||||
| skipSignatureVerification: true, | ||||||
| requireHttpSignature: true, | ||||||
| }); | ||||||
| assertEquals( | ||||||
| response.status, | ||||||
| 202, | ||||||
| "With skipSignatureVerification: true, should succeed even if requireHttpSignature: true", | ||||||
| ); | ||||||
| }); | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation adds a new
requireHttpSignaturefield toFederationFetchOptionsandInboxHandlerParameters, which duplicates functionality that already exists in Fedify's.authorize()callback API.According to our access control documentation, the standard way to enable authorized fetch in Fedify is through the
.authorize()method:The
.authorize()callback provides a flexible, dispatcher-level mechanism for access control that:Instead of adding a new field that bypasses the existing API, I suggest implementing the
-A/--authorized-fetchoption by registering an.authorize()callback in the CLI code: