Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,15 @@ To be released.
[#1028]: https://github.com/fedify-dev/fedify/issues/1028
[#1034]: https://github.com/fedify-dev/fedify/issues/1034

### @fedify/vocab

- Updated the `fedify:url` decoder to read `@id` when `@value` is absent,
allowing it to accept IRI-valued quote URL aliases (`_misskey_quote` or
`quoteUri`). Also widened its `dataCheck()` to accept both forms.
[[#1015] by Jang Hanarae\]
Comment on lines +28 to +31

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This describes the generator rather than the change. fedify:url and dataCheck() are vocab-tools internals that no user of @fedify/vocab ever sees, and the second sentence describes something that currently generates no code at all. Entries here should say what broke for users and when.

Two smaller things. Neighbouring bug fix entries open with “Fixed”, not “Updated”. And the marker should carry the pull request number alongside the issue, the way the @fedify/redis entry above carries [[#1028], [#1034]].

Something along these lines:

 -  Fixed `Note.quoteUrl`, and the same property on `Article`, `ChatMessage`,
    and `Question`, being dropped or throwing `TypeError: Invalid URL` when
    the sender's JSON-LD context declared `_misskey_quote` or `quoteUri` with
    `"@type": "@id"`.  Such terms expand to a node object carrying `@id`
    rather than `@value`, and the parser read `@value` only.  Notes from
    Misskey-compatible servers are affected.
    [[#1015], [#1043] by Jang Hanarae]

Note the two spaces after each sentence, which the surrounding entries use. Edit changes.d/vocab/iri-valued-quote-url.md and run sacho sync rather than editing this file by hand.


[#1015]: https://github.com/fedify-dev/fedify/issues/1015


Version 2.0.27
--------------
Expand Down
8 changes: 8 additions & 0 deletions changes.d/vocab/iri-valued-quote-url.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
links:
'#1015': https://github.com/fedify-dev/fedify/issues/1015
Comment on lines +2 to +3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinning the links in front matter is the right call, but #1043 is missing. Once the entry cites the pull request as well as the issue, this needs both:

links:
  '#1015': https://github.com/fedify-dev/fedify/issues/1015
  '#1043': https://github.com/fedify-dev/fedify/pull/1043

Separately, this change spans two published packages and only one of them has a fragment. The behaviour users observe lands in @fedify/vocab, but the source change is in packages/vocab-tools/, which ships as @fedify/vocab-tools, so changes.d/vocab-tools/ should carry its own entry describing the generator change. Run sacho sync afterwards to materialize both sections into CHANGES.md.

---
- Updated the `fedify:url` decoder to read `@id` when `@value` is absent,
allowing it to accept IRI-valued quote URL aliases (`_misskey_quote` or
`quoteUri`). Also widened its `dataCheck()` to accept both forms.
[[#1015] by Jang Hanarae]
Original file line number Diff line number Diff line change
Expand Up @@ -12236,7 +12236,7 @@ proofs?: (DataIntegrityProof | URL)[];quoteUrl?: URL | null;}
if (v == null) continue;

const decoded =
new URL(v[\\"@value\\"])
new URL(typeof v[\\"@value\\"] === \\"string\\" ? v[\\"@value\\"] : v[\\"@id\\"])
;

if (!this._shouldCacheDecodedJsonLd(decoded)) {
Expand Down Expand Up @@ -29346,7 +29346,7 @@ proofs?: (DataIntegrityProof | URL)[];quoteUrl?: URL | null;}
if (v == null) continue;

const decoded =
new URL(v[\\"@value\\"])
new URL(typeof v[\\"@value\\"] === \\"string\\" ? v[\\"@value\\"] : v[\\"@id\\"])
;

if (!this._shouldCacheDecodedJsonLd(decoded)) {
Expand Down Expand Up @@ -52519,7 +52519,7 @@ proofs?: (DataIntegrityProof | URL)[];quoteUrl?: URL | null;}
if (v == null) continue;

const decoded =
new URL(v[\\"@value\\"])
new URL(typeof v[\\"@value\\"] === \\"string\\" ? v[\\"@value\\"] : v[\\"@id\\"])
;

if (!this._shouldCacheDecodedJsonLd(decoded)) {
Expand Down Expand Up @@ -71062,7 +71062,7 @@ instruments?: (Object | URL)[];exclusiveOptions?: (Object | URL)[];inclusiveOpti
if (v == null) continue;

const decoded =
new URL(v[\\"@value\\"])
new URL(typeof v[\\"@value\\"] === \\"string\\" ? v[\\"@value\\"] : v[\\"@id\\"])
;

if (!this._shouldCacheDecodedJsonLd(decoded)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12234,7 +12234,7 @@ proofs?: (DataIntegrityProof | URL)[];quoteUrl?: URL | null;}
if (v == null) continue;

const decoded =
new URL(v[\\"@value\\"])
new URL(typeof v[\\"@value\\"] === \\"string\\" ? v[\\"@value\\"] : v[\\"@id\\"])
;

if (!this._shouldCacheDecodedJsonLd(decoded)) {
Expand Down Expand Up @@ -29344,7 +29344,7 @@ proofs?: (DataIntegrityProof | URL)[];quoteUrl?: URL | null;}
if (v == null) continue;

const decoded =
new URL(v[\\"@value\\"])
new URL(typeof v[\\"@value\\"] === \\"string\\" ? v[\\"@value\\"] : v[\\"@id\\"])
;

if (!this._shouldCacheDecodedJsonLd(decoded)) {
Expand Down Expand Up @@ -52517,7 +52517,7 @@ proofs?: (DataIntegrityProof | URL)[];quoteUrl?: URL | null;}
if (v == null) continue;

const decoded =
new URL(v[\\"@value\\"])
new URL(typeof v[\\"@value\\"] === \\"string\\" ? v[\\"@value\\"] : v[\\"@id\\"])
;

if (!this._shouldCacheDecodedJsonLd(decoded)) {
Expand Down Expand Up @@ -71060,7 +71060,7 @@ instruments?: (Object | URL)[];exclusiveOptions?: (Object | URL)[];inclusiveOpti
if (v == null) continue;

const decoded =
new URL(v[\\"@value\\"])
new URL(typeof v[\\"@value\\"] === \\"string\\" ? v[\\"@value\\"] : v[\\"@id\\"])
;

if (!this._shouldCacheDecodedJsonLd(decoded)) {
Expand Down
8 changes: 4 additions & 4 deletions packages/vocab-tools/src/__snapshots__/class.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -12236,7 +12236,7 @@ proofs?: (DataIntegrityProof | URL)[];quoteUrl?: URL | null;}
if (v == null) continue;

const decoded =
new URL(v["@value"])
new URL(typeof v["@value"] === "string" ? v["@value"] : v["@id"])
;

if (!this._shouldCacheDecodedJsonLd(decoded)) {
Expand Down Expand Up @@ -29346,7 +29346,7 @@ proofs?: (DataIntegrityProof | URL)[];quoteUrl?: URL | null;}
if (v == null) continue;

const decoded =
new URL(v["@value"])
new URL(typeof v["@value"] === "string" ? v["@value"] : v["@id"])
;

if (!this._shouldCacheDecodedJsonLd(decoded)) {
Expand Down Expand Up @@ -52519,7 +52519,7 @@ proofs?: (DataIntegrityProof | URL)[];quoteUrl?: URL | null;}
if (v == null) continue;

const decoded =
new URL(v["@value"])
new URL(typeof v["@value"] === "string" ? v["@value"] : v["@id"])
;

if (!this._shouldCacheDecodedJsonLd(decoded)) {
Expand Down Expand Up @@ -71062,7 +71062,7 @@ instruments?: (Object | URL)[];exclusiveOptions?: (Object | URL)[];inclusiveOpti
if (v == null) continue;

const decoded =
new URL(v["@value"])
new URL(typeof v["@value"] === "string" ? v["@value"] : v["@id"])
;

if (!this._shouldCacheDecodedJsonLd(decoded)) {
Expand Down
10 changes: 6 additions & 4 deletions packages/vocab-tools/src/type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -308,12 +308,14 @@ const scalarTypes: Record<string, ScalarType> = {
return `${v}.href`;
},
dataCheck(v) {
return `typeof ${v} === "object" && "@value" in ${v}
&& typeof ${v}["@value"] === "string"
&& ${v}["@value"] !== "" && ${v}["@value"] !== "/"`;
return `typeof ${v} === "object" &&
(("@value" in ${v} && typeof ${v}["@value"] === "string" &&
${v}["@value"] !== "" && ${v}["@value"] !== "/") ||
("@id" in ${v} && typeof ${v}["@id"] === "string" &&
${v}["@id"] !== "" && ${v}["@id"] !== "/"))`;
Comment on lines +311 to +315

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping dataCheck() in step with decoder() is the right instinct, so this should stay. Two things to know about it though.

It generates nothing today. The guard is emitted only for properties with more than one range, and fedify:url is a sole range in all four properties that use it, namely quoteUrl on Article, ChatMessage, Note and Question. That is why the snapshot diff in this pull request touches the decoder line four times and no dataCheck() line at all. Worth knowing mostly so the changelog does not advertise it as something users can observe.

Second, fedify:gatewayUrl on main reads @id first and falls back to @value, while this reads them the other way round. The two are mutually exclusive in practice so nothing changes, but matching the existing order keeps the two definitions readable side by side.

If the decoder becomes tolerant the way I suggest below, this guard starts being emitted and its condition becomes load-bearing. It would then need to reject more than the empty string and /: both _:b0 and a relative IRI pass those checks and still fail new URL().

},
decoder(v) {
return `new URL(${v}["@value"])`;
return `new URL(typeof ${v}["@value"] === "string" ? ${v}["@value"] : ${v}["@id"])`;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shape here is right, and it matches a precedent already on main: the fedify:gatewayUrl scalar type there reads @id or @value in exactly this way, so this will merge forward cleanly.

What it does not do yet is stop the crash the issue is about. I regenerated vocab.ts with this patch applied and fed Note.fromJsonLd() a range of values under a context declaring _misskey_quote with "@type": "@id":

value result with this patch
"https://example.com/q" https://example.com/q
{"type": "Note", "id": "https://example.com/q2"} https://example.com/q2
{"type": "Note", "content": "hi"} throws TypeError: Invalid URL: 'undefined'
"_:b0" throws TypeError: Invalid URL: '_:b0'
"" throws TypeError: Invalid URL: './'
"notes/rel" throws TypeError: Invalid URL: 'notes/rel'
"at://did:plc:abc/app.bsky.feed.post/xyz" throws TypeError: Invalid URL

Row three is the same error text, from the same property, that #1015 reports. A sender that inlines the quoted object without an id still fails the entire Note, and a blank node identifier does the same. A quote URL we cannot parse should be dropped, not taken down the object around it.

The reason dataCheck() cannot protect against this today is in packages/vocab-tools/src/codec.ts: getDecoders(), which emits the guard, runs only when property.range.length > 1 (line 465), and fedify:url is a sole range in all four properties that use it. The single-range path at line 444 calls getDecoder() unguarded, so there is no undefined to skip on.

The last row deserves its own note. codec.ts lines 428 to 438 already carry an at:// special case for the non-scalar @id path, so as written the two @id paths disagree about ATProto URIs.

Making the decoder tolerant needs a small change in the generator. Dropping the single-range special case altogether, the way main does it, does not work on this branch: the dataCheck ? decoder : undefined ternary widens narrow literal unions, and fedify:proofPurpose then fails type checking with TS2345. An opt-in flag avoids that. This is the version I verified:

--- a/packages/vocab-tools/src/class.ts
+++ b/packages/vocab-tools/src/class.ts
@@ -154,6 +154,17 @@ export async function* generateClasses(
     isTemporalInstant,
 } from "@fedify/vocab-runtime/temporal";\n`;
   yield `
+function canParseIri(iri: string): boolean {
+  return URL.canParse(iri) || iri.startsWith("at://");
+}
+
+function parseIri(iri: string): URL {
+  return !URL.canParse(iri) && iri.startsWith("at://")
+    ? new URL("at://" + encodeURIComponent(iri.substring(5)))
+    : new URL(iri);
+}
+`;
+  yield `
 function isValidLanguageTag(language: string): boolean {
--- a/packages/vocab-tools/src/type.ts
+++ b/packages/vocab-tools/src/type.ts
@@ -17,6 +17,12 @@ interface ScalarType {
   dataCheck(variable: string): string;
   decoder(variable: string, baseUrlVar: string): string;
+  /**
+   * Whether a value that fails `dataCheck()` should be skipped instead of
+   * being handed to `decoder()`.  Set this for types decoded from untrusted
+   * remote input, where one malformed value must not fail the whole object.
+   */
+  skipUnparsable?: boolean;
 }
@@ -309,14 +315,15 @@ const scalarTypes: Record<string, ScalarType> = {
     dataCheck(v) {
       return `typeof ${v} === "object" &&
-      (("@value" in ${v} && typeof ${v}["@value"] === "string" &&
-        ${v}["@value"] !== "" && ${v}["@value"] !== "/") ||
-      ("@id" in ${v} && typeof ${v}["@id"] === "string" &&
-        ${v}["@id"] !== "" && ${v}["@id"] !== "/"))`;
+      ((typeof ${v}["@value"] === "string" && canParseIri(${v}["@value"])) ||
+       (typeof ${v}["@id"] === "string" && canParseIri(${v}["@id"])))`;
     },
     decoder(v) {
-      return `new URL(typeof ${v}["@value"] === "string" ? ${v}["@value"] : ${v}["@id"])`;
+      return `parseIri(
+        typeof ${v}["@value"] === "string" ? ${v}["@value"] : ${v}["@id"]
+      )`;
     },
+    skipUnparsable: true,
   },
@@ -597,6 +604,10 @@ export function getDecoder(
+export function skipsUnparsable(typeUri: string): boolean {
+  return scalarTypes[typeUri]?.skipUnparsable ?? false;
+}
+
 export function getDataCheck(
--- a/packages/vocab-tools/src/codec.ts
+++ b/packages/vocab-tools/src/codec.ts
@@ -6,11 +6,13 @@ import {
   areAllScalarTypes,
   emitOverride,
   getAllProperties,
+  getDataCheck,
   getDecoder,
   getDecoders,
   getEncoders,
   getSubtypes,
   isCompactableType,
+  skipsUnparsable,
 } from "./type.ts";
@@ -441,7 +443,13 @@ export async function* generateDecoder(
     yield `
       const decoded =
     `;
+    const lenient = property.range.length == 1 &&
+      skipsUnparsable(property.range[0]);
     if (property.range.length == 1) {
+      if (lenient) {
+        yield getDataCheck(property.range[0], types, "v");
+        yield " ? ";
+      }
       yield getDecoder(
         property.range[0],
@@ -449,6 +457,7 @@ export async function* generateDecoder(
         "options",
         `(values["@id"] == null ? options.baseUrl : new URL(values["@id"]))`,
       );
+      if (lenient) yield " : undefined";
     } else {
@@ -462,7 +471,7 @@ export async function* generateDecoder(
     yield `
       ;
     `;
-    if (property.range.length > 1) {
+    if (property.range.length > 1 || lenient) {
       yield `
       if (typeof decoded === "undefined") {

With that applied, every throwing row above becomes null, the at:// row parses to at://did%3Aplc%3Aabc%2Fapp.bsky.feed.post%2Fxyz (the same value codec.ts line 433 produces for the other @id path), and all 22,030 tests in packages/vocab pass. The vocab-tools snapshots need regenerating.

Sharing parseIri() this way also lets codec.ts lines 428 to 438 drop their inline copy of the at:// handling, and it lines the branch up with the parseIri() that @fedify/vocab-runtime already exports on main.

},
},
"fedify:publicKey": {
Expand Down
34 changes: 34 additions & 0 deletions packages/vocab/src/vocab.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,40 @@ test("Note.quoteUrl", async () => {
deepStrictEqual(loaded3.quoteUrl, new URL("https://example.com/object3"));
});

test("Note.quoteUrl (IRI-typed alias terms)", async () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a real regression test. I reverted type.ts to its base revision, forced a regeneration and confirmed it fails with TypeError: Invalid URL: 'undefined', which is the error #1015 reports.

Two gaps worth closing:

  • quoteUrl itself is never declared with "@type": "@id", only the two aliases are. The primary property goes through the same decoder, so covering it costs one line.
  • Nothing mixes the two forms, for instance a @value-shaped quoteUrl alongside an @id-shaped _misskey_quote. That is what pins the precedence order the existing Note.quoteUrl test establishes.

Once unparsable values are dropped instead of thrown, this is also the right place to assert it: a _misskey_quote expanding to a node with no @id should leave quoteUrl at null rather than fail fromJsonLd().

const jsonLd: Record<string, unknown> = {
"@context": [
"https://www.w3.org/ns/activitystreams",
{
fedibird: "http://fedibird.com/ns#",
misskey: "https://misskey-hub.net/ns#",
_misskey_quote: {
"@id": "misskey:_misskey_quote",
"@type": "@id",
},
quoteUri: {
"@id": "fedibird:quoteUri",
"@type": "@id",
},
},
],
id: "https://example.com/notes/1",
type: "Note",
_misskey_quote: "https://example.com/notes/quoted",
quoteUri: "https://example.com/notes/quoted2",
};

const loaded = await Note.fromJsonLd(jsonLd);
deepStrictEqual(loaded.quoteUrl, new URL("https://example.com/notes/quoted"));

delete jsonLd._misskey_quote;
const loaded2 = await Note.fromJsonLd(jsonLd);
deepStrictEqual(
loaded2.quoteUrl,
new URL("https://example.com/notes/quoted2"),
);
});

test("Key.publicKey", async () => {
const jwk = {
kty: "RSA",
Expand Down
Loading