Skip to content

Commit e4798ba

Browse files
committed
Address review feedback on public audience normalization
- Iterate over own keys only (Object.keys) instead of for...in in hasPublicCurieInAddressing and rewritePublicAudience, so enumerable inherited properties on the incoming (potentially adversarial) JSON-LD record are not copied into the normalized output. Addresses fedify-dev#710 (comment) and fedify-dev#710 (comment). - Add a URDNA2015 fast-path for JSON-LD documents whose @context is composed entirely of string IRIs. In that case no inline context entry can redefine the `as:` prefix or the bare `Public` term, so the rewrite is provably semantics-preserving and the canonicalization equivalence check can be skipped. Only documents that embed an inline @context object continue to pay the canonicalization cost. Addresses fedify-dev#710 (comment). - Accept either the on-wire form or the normalized form in verifyProof(). createProof() signs the bytes produced *after* normalization, but signObject()'s return value still serializes back to the `as:Public` CURIE form by default, so a caller doing an in-memory sign -> reserialize -> verify round-trip would have seen a spurious signature mismatch. verifyProof() now tries the input as-is first (preserving verification of signatures produced by other implementations that signed the CURIE form) and falls back to the normalized form when the original fails, restoring the signObject()/verifyProof() API contract. Addresses fedify-dev#710 (comment). Added regression tests: the string-only-context fast path uses a throwing contextLoader to prove URDNA2015 is not invoked; a prototype-pollution test confirms inherited keys are not rewritten; and the signObject() test now also verifies a proof directly from `signed.toJsonLd({ format: "compact" })` output (which still contains `as:Public`), exercising the verifyProof() fallback. Assisted-By: Claude Code:claude-opus-4-7
1 parent e77a972 commit e4798ba

4 files changed

Lines changed: 131 additions & 43 deletions

File tree

packages/fedify/src/compat/public-audience.test.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,45 @@ test("normalizePublicAudience() leaves non-addressing fields untouched", async (
7676
assertEquals(output.to, PUBLIC_URI);
7777
});
7878

79+
test("normalizePublicAudience() rewrites without canonicalization for string-only contexts", async () => {
80+
// A contextLoader that always throws ensures the canonicalization path
81+
// is not taken: if the implementation hit URDNA2015, it would fail.
82+
const rejecting: Parameters<typeof normalizePublicAudience>[1] = () => {
83+
throw new Error(
84+
"contextLoader should not be called for a string-only @context",
85+
);
86+
};
87+
const singleString = await normalizePublicAudience({
88+
"@context": AS_CONTEXT,
89+
type: "Note",
90+
id: "https://example.com/notes/fast1",
91+
to: "as:Public",
92+
}, rejecting) as Record<string, unknown>;
93+
assertEquals(singleString.to, PUBLIC_URI);
94+
const arrayOfStrings = await normalizePublicAudience({
95+
"@context": [AS_CONTEXT, "https://w3id.org/security/data-integrity/v1"],
96+
type: "Note",
97+
id: "https://example.com/notes/fast2",
98+
to: "as:Public",
99+
}, rejecting) as Record<string, unknown>;
100+
assertEquals(arrayOfStrings.to, PUBLIC_URI);
101+
});
102+
103+
test("normalizePublicAudience() does not traverse prototype-polluted keys", async () => {
104+
const polluted = Object.create({ to: "as:Public" });
105+
polluted["@context"] = AS_CONTEXT;
106+
polluted.type = "Note";
107+
polluted.id = "https://example.com/notes/proto";
108+
const output = await normalizePublicAudience(polluted) as Record<
109+
string,
110+
unknown
111+
>;
112+
// The inherited `to` is not copied into the normalized record, so `to`
113+
// stays inherited from the prototype with its original CURIE value and
114+
// is not a rewritten own property.
115+
assertEquals(Object.hasOwn(output, "to"), false);
116+
});
117+
79118
test("normalizePublicAudience() bails out when the rewrite changes semantics", async () => {
80119
const input = {
81120
"@context": {

packages/fedify/src/compat/public-audience.ts

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ function hasPublicCurieInAddressing(
2727
}
2828
if (typeof value !== "object" || value == null) return false;
2929
const record = value as Record<string, unknown>;
30-
for (const key in record) {
30+
for (const key of Object.keys(record)) {
3131
if (hasPublicCurieInAddressing(record[key], key)) return true;
3232
}
3333
return false;
@@ -48,28 +48,48 @@ function rewritePublicAudience(value: unknown, parentKey?: string): unknown {
4848
if (typeof value !== "object" || value == null) return value;
4949
const record = value as Record<string, unknown>;
5050
const normalized: Record<string, unknown> = {};
51-
for (const key in record) {
51+
for (const key of Object.keys(record)) {
5252
normalized[key] = rewritePublicAudience(record[key], key);
5353
}
5454
return normalized;
5555
}
5656

57+
/**
58+
* Checks whether the `@context` of a JSON-LD document consists exclusively
59+
* of string (IRI) entries. When that is the case, an application-defined
60+
* inline context cannot redefine the `as:` prefix or the bare `Public`
61+
* term, so the rewrite can be applied without running URDNA2015
62+
* canonicalization to verify equivalence.
63+
*/
64+
function hasOnlyStringContext(jsonLd: unknown): boolean {
65+
if (typeof jsonLd !== "object" || jsonLd == null) return false;
66+
const record = jsonLd as Record<string, unknown>;
67+
if (!Object.hasOwn(record, "@context")) return false;
68+
const ctx = record["@context"];
69+
if (typeof ctx === "string") return true;
70+
if (Array.isArray(ctx)) return ctx.every((item) => typeof item === "string");
71+
return false;
72+
}
73+
5774
/**
5875
* Rewrites the compact `as:Public` / `Public` CURIE appearing in activity
5976
* addressing fields (`to`, `cc`, `bto`, `bcc`, `audience`) to the fully
6077
* expanded `https://www.w3.org/ns/activitystreams#Public` URI.
6178
*
62-
* Several ActivityPub implementationsLemmy among them match these
79+
* Several ActivityPub implementations, Lemmy among them, match these
6380
* fields as plain URLs without running JSON-LD expansion, and silently
6481
* drop activities whose public addressing appears in CURIE form. This
6582
* helper works around that gap.
6683
*
67-
* The rewrite is gated on a JSON-LD equivalence check: both forms are
68-
* canonicalized with URDNA2015 and the resulting N-Quads are compared.
69-
* When they differ — which indicates an application-defined `@context`
70-
* that redefines the `as:` prefix or the bare `Public` term — the
71-
* original document is returned unchanged so semantics are preserved.
72-
* Canonicalization failures also fall back to the original document.
84+
* For documents whose `@context` consists only of string IRIs, the rewrite
85+
* is applied directly: external contexts cannot redefine the `as:` prefix
86+
* or the bare `Public` term for a given document, so the semantics are
87+
* preserved by construction. When `@context` includes an inline object
88+
* that might redefine those terms, the rewrite is gated on a JSON-LD
89+
* equivalence check; both forms are canonicalized with URDNA2015 and the
90+
* resulting N-Quads are compared. When they differ, the original
91+
* document is returned unchanged. Canonicalization failures also fall
92+
* back to the original document.
7393
*
7494
* Must be called before any signing step that canonicalizes the
7595
* compact form byte-for-byte (for example, Object Integrity Proofs
@@ -82,6 +102,7 @@ export async function normalizePublicAudience(
82102
): Promise<unknown> {
83103
if (!hasPublicCurieInAddressing(jsonLd)) return jsonLd;
84104
const normalized = rewritePublicAudience(jsonLd);
105+
if (hasOnlyStringContext(jsonLd)) return normalized;
85106
const loader = contextLoader ?? getDocumentLoader();
86107
try {
87108
const [before, after] = await Promise.all([

packages/fedify/src/sig/proof.test.ts

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,7 @@ test("signObject()", async () => {
302302
) as Record<string, unknown>;
303303
assertEquals(signedJson.to, PUBLIC_COLLECTION.href);
304304
const verifyCache: Record<string, CryptographicKey | Multikey | null> = {};
305-
const verifyingKey = await verifyProof(signedJson, proof, {
305+
const verifyOptions: VerifyProofOptions = {
306306
contextLoader: mockDocumentLoader,
307307
documentLoader: mockDocumentLoader,
308308
keyCache: {
@@ -312,8 +312,28 @@ test("signObject()", async () => {
312312
return Promise.resolve();
313313
},
314314
},
315-
});
315+
};
316+
const verifyingKey = await verifyProof(signedJson, proof, verifyOptions);
316317
assertInstanceOf(verifyingKey, Multikey);
318+
319+
// Round-trip regression guard: `signObject()` returns a vocab object
320+
// whose default `toJsonLd({ format: "compact" })` output still compacts
321+
// the public audience to the `as:Public` CURIE, even though the bytes
322+
// signed by `createProof()` were first normalized to the expanded URI.
323+
// `verifyProof()` must accept either form so the in-memory pipeline
324+
// (sign, reserialize, verify) continues to work without every caller
325+
// having to know about the public-audience compat helper.
326+
const signedJsonWithCurie = await signed.toJsonLd(options) as Record<
327+
string,
328+
unknown
329+
>;
330+
assertEquals(signedJsonWithCurie.to, "as:Public");
331+
const verifyingKeyFromCurie = await verifyProof(
332+
signedJsonWithCurie,
333+
proof,
334+
verifyOptions,
335+
);
336+
assertInstanceOf(verifyingKeyFromCurie, Multikey);
317337
});
318338

319339
test("hasProofLike()", () => {

packages/fedify/src/sig/proof.ts

Lines changed: 40 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -358,18 +358,19 @@ async function verifyProofInternal(
358358
proofPurpose: proof.proofPurpose,
359359
created: proof.created.toString(),
360360
};
361-
const proofCanon = serialize(proofConfig);
362361
const encoder = new TextEncoder();
363-
const proofBytes = encoder.encode(proofCanon);
362+
const proofBytes = encoder.encode(serialize(proofConfig));
364363
const proofDigest = await crypto.subtle.digest("SHA-256", proofBytes);
365364
const msg = { ...jsonLd };
366365
if ("proof" in msg) delete msg.proof;
367-
const msgCanon = serialize(msg);
368-
const msgBytes = encoder.encode(msgCanon);
369-
const msgDigest = await crypto.subtle.digest("SHA-256", msgBytes);
370-
const digest = new Uint8Array(proofDigest.byteLength + msgDigest.byteLength);
371-
digest.set(new Uint8Array(proofDigest), 0);
372-
digest.set(new Uint8Array(msgDigest), proofDigest.byteLength);
366+
// Try the on-wire form first, then fall back to the public-audience
367+
// normalized form so that signatures created by `createProof` (which
368+
// signs the normalized bytes) still verify when the caller passes the
369+
// default `toJsonLd({ format: "compact" })` output that still carries
370+
// the `as:Public` CURIE.
371+
const candidates: unknown[] = [msg];
372+
const normalized = await normalizePublicAudience(msg, options.contextLoader);
373+
if (normalized !== msg) candidates.push(normalized);
373374
let fetchedKey: FetchKeyResult<Multikey> | null;
374375
try {
375376
fetchedKey = await publicKeyPromise;
@@ -410,34 +411,41 @@ async function verifyProofInternal(
410411
);
411412
return null;
412413
}
413-
const verified = await crypto.subtle.verify(
414-
"Ed25519",
415-
publicKey.publicKey,
416-
proof.proofValue.slice(),
417-
digest,
418-
);
419-
if (!verified) {
420-
if (fetchedKey.cached) {
421-
logger.debug(
422-
"Failed to verify the proof with the cached key {keyId}; retrying " +
423-
"with the freshly fetched key...",
424-
{ keyId: proof.verificationMethodId.href, proof },
425-
);
426-
return await verifyProof(jsonLd, proof, {
427-
...options,
428-
keyCache: {
429-
get: () => Promise.resolve(undefined),
430-
set: async (keyId, key) => await options.keyCache?.set(keyId, key),
431-
},
432-
});
433-
}
414+
for (const candidate of candidates) {
415+
const msgBytes = encoder.encode(serialize(candidate));
416+
const msgDigest = await crypto.subtle.digest("SHA-256", msgBytes);
417+
const digest = new Uint8Array(
418+
proofDigest.byteLength + msgDigest.byteLength,
419+
);
420+
digest.set(new Uint8Array(proofDigest), 0);
421+
digest.set(new Uint8Array(msgDigest), proofDigest.byteLength);
422+
const verified = await crypto.subtle.verify(
423+
"Ed25519",
424+
publicKey.publicKey,
425+
proof.proofValue.slice(),
426+
digest,
427+
);
428+
if (verified) return publicKey;
429+
}
430+
if (fetchedKey.cached) {
434431
logger.debug(
435-
"Failed to verify the proof with the fetched key {keyId}:\n{proof}",
432+
"Failed to verify the proof with the cached key {keyId}; retrying " +
433+
"with the freshly fetched key...",
436434
{ keyId: proof.verificationMethodId.href, proof },
437435
);
438-
return null;
436+
return await verifyProof(jsonLd, proof, {
437+
...options,
438+
keyCache: {
439+
get: () => Promise.resolve(undefined),
440+
set: async (keyId, key) => await options.keyCache?.set(keyId, key),
441+
},
442+
});
439443
}
440-
return publicKey;
444+
logger.debug(
445+
"Failed to verify the proof with the fetched key {keyId}:\n{proof}",
446+
{ keyId: proof.verificationMethodId.href, proof },
447+
);
448+
return null;
441449
}
442450

443451
/**

0 commit comments

Comments
 (0)