Phase 1: embed JSON-LD data island in mashlib HTML wrapper (#7) - #343
Merged
Conversation
The mashlib HTML wrapper now carries the originating resource's JSON-LD bytes as a `<script type="application/ld+json" id="dataisland" data-uri="…">` block. Phase 1 is strictly additive: - mashlib still XHR-fetches as before (no behaviour change yet); - browsers ignore the script body since its MIME isn't JS; - but anything that knows to look for `application/ld+json` islands — search engines (Rich Results), archival crawlers, scrapers, static-site exporters, future LWS-aware tooling — now finds the data without a second HTTP request. This sets up Phase 2, where a small inline shim will patch mashlib's `fetcher` to read the island instead of refetching, eliminating the double-fetch that motivated the recent Vary/Cache work (#316, #325/#326). Format: JSON-LD (not Turtle), since: - JSS stores JSON-LD natively → zero server-side conversion; - `<script type="application/ld+json">` is the standardised way to embed structured data on the web; - aligns with our LWS / CID work — an HTML-only LWS verifier can parse the embedded JSON-LD and find the CID `service[]` entry without conneg. Sites: - src/mashlib/index.js: `generateDatabrowserHtml(url, cdn, {embedJsonLd})` emits the island when a payload is supplied. New `DATA_ISLAND_MAX_BYTES = 256 * 1024` size cap silently drops the island for oversize resources; the wrapper still works because mashlib falls back to its XHR path. - src/handlers/resource.js: passes the JSON-LD bytes when the stored content type is `application/ld+json` (resources) or whenever a container listing is generated. Security: - `</script>` substrings inside the JSON-LD body are escaped to `<\/script>` so a user-PUTted resource can't close the script tag prematurely and inject HTML. - `<!--` substrings are escaped to neutralise comment-escape tricks. - The `data-uri` attribute is HTML-entity-encoded (&, ", <, >) so attribute-quote injection isn't possible either. 9 new tests in test/data-island.test.js cover: emission shape, the back-compat omission case, the size cap, both `</script>` and `<!--` escapes, attribute encoding, and live HTTP integration for both resources and container listings — plus a negative test that the mashlib XHR path (Accept: application/ld+json) still gets RDF rather than the wrapper. Phase 1 of #7. 546/546 green.
There was a problem hiding this comment.
Pull request overview
Adds Phase-1 support for embedding a JSON-LD “data island” into the Mashlib HTML wrapper so non-mashlib consumers (and future Phase-2 mashlib optimizations) can access the RDF payload without an extra HTTP request.
Changes:
- Embed JSON-LD into the Mashlib wrapper as
<script type="application/ld+json" id="dataisland" data-uri="…">…</script>with a 256KB cap and escaping. - Pass container listings (computed JSON-LD) and JSON-LD resource bodies into the wrapper generator when serving Mashlib HTML.
- Add unit + integration tests validating emission shape, size cap, escaping, and live HTTP behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/mashlib/index.js |
Implements data-island generation, escaping, and the size cap in the HTML wrapper. |
src/handlers/resource.js |
Plumbs JSON-LD (container listing / resource bytes) into the wrapper when serving Mashlib HTML. |
test/data-island.test.js |
Adds unit + integration coverage for island emission, escaping, size cap, and conneg behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
340
to
+346
| const html = request.mashlibModule | ||
| ? generateModuleDatabrowserHtml(request.mashlibModule) | ||
| : generateDatabrowserHtml(resourceUrl, request.mashlibCdn ? request.mashlibVersion : null); | ||
| : generateDatabrowserHtml( | ||
| resourceUrl, | ||
| request.mashlibCdn ? request.mashlibVersion : null, | ||
| { embedJsonLd } | ||
| ); |
Comment on lines
+62
to
+89
| it('escapes `</script>` substrings so a malicious payload cannot close the tag', () => { | ||
| // A user could PUT JSON-LD whose content field contains a literal | ||
| // closing-script tag. Without escaping, this would terminate the | ||
| // script element early and let arbitrary subsequent bytes parse as | ||
| // inline HTML. | ||
| const trojan = '{"content":"oops</script><img src=x onerror=alert(1)>"}'; | ||
| const html = generateDatabrowserHtml( | ||
| 'https://x.test/r', | ||
| '2.0.0', | ||
| { embedJsonLd: trojan } | ||
| ); | ||
| // The verbatim closing tag must not appear inside the script body. | ||
| // Find the start of the data-island script and check until its real end. | ||
| const start = html.indexOf('id="dataisland"'); | ||
| assert.ok(start > 0, 'data island should be present'); | ||
| const tail = html.slice(start); | ||
| // The escaped form must be present; the unescaped form must NOT | ||
| // appear before our intended `</script>` terminator. Simple check: | ||
| // the body should not contain `</script>` at all (escaped is `<\/script>`). | ||
| const bodyEnd = tail.indexOf('</script>'); | ||
| const escapedHits = (tail.slice(0, bodyEnd).match(/<\\\/script>/g) || []).length; | ||
| assert.strictEqual(escapedHits, 1, | ||
| 'the trojan </script> must be present in escaped form exactly once'); | ||
| assert.doesNotMatch(tail.slice(0, bodyEnd), /<\/script>/, | ||
| 'unescaped </script> must not appear inside the script body'); | ||
| // And the image-payload portion must remain trapped inside the | ||
| // string; the parser should never see it as live HTML. | ||
| assert.match(tail, /onerror=alert\(1\)/); |
Comment on lines
+29
to
+40
| * script (wrong MIME), but a literal `</script>` substring inside the | ||
| * body would prematurely close the tag and let arbitrary subsequent | ||
| * bytes be parsed as inline HTML. Replacing `<` with `<` (only | ||
| * inside the script body) defeats that without changing the JSON-LD | ||
| * semantics — `<` is just a unicode escape for `<`. | ||
| */ | ||
| function escapeForScriptBlock(jsonLdString) { | ||
| // Targeted: only sequences that could close or open a tag inside | ||
| // the script body. | ||
| return jsonLdString | ||
| .replace(/<\/script>/gi, '<\\/script>') | ||
| .replace(/<!--/g, '\\u003c!--'); |
Comment on lines
+28
to
+33
| * type="application/ld+json">…</script>`. Browsers don't execute the | ||
| * script (wrong MIME), but a literal `</script>` substring inside the | ||
| * body would prematurely close the tag and let arbitrary subsequent | ||
| * bytes be parsed as inline HTML. Replacing `<` with `<` (only | ||
| * inside the script body) defeats that without changing the JSON-LD | ||
| * semantics — `<` is just a unicode escape for `<`. |
Comment on lines
+335
to
+339
| let embedJsonLd; | ||
| if (storedContentType === 'application/ld+json') { | ||
| const buf = await storage.read(storagePath); | ||
| if (buf) embedJsonLd = buf.toString('utf8'); | ||
| } |
Comment on lines
247
to
+253
| const html = request.mashlibModule | ||
| ? generateModuleDatabrowserHtml(request.mashlibModule) | ||
| : generateDatabrowserHtml(resourceUrl, request.mashlibCdn ? request.mashlibVersion : null); | ||
| : generateDatabrowserHtml( | ||
| resourceUrl, | ||
| request.mashlibCdn ? request.mashlibVersion : null, | ||
| { embedJsonLd } | ||
| ); |
Six points, all real: - src/mashlib/index.js: escapeForScriptBlock now encodes EVERY `<` byte as the JSON Unicode escape `<`. The previous narrow regex only caught literal `</script>` and `<!--`, but HTML parsers terminate a <script> element on the prefix `</script` regardless of what follows — `</script >`, `</script\n>`, `</SCRIPT>` and friends all close it. After this transform the body cannot contain a literal `<` at all, so no end-tag (or comment, or CDATA) can possibly start. JSON-LD semantics are preserved because JSON parsers decode `<` back to `<` natively. - src/mashlib/index.js: header comment rewritten to accurately describe the new strategy (the previous comment had been internally inconsistent and described a different approach). - src/mashlib/index.js: generateModuleDatabrowserHtml now also accepts opts.embedJsonLd and emits the data island. Previously module-mode mashlib deployments missed the feature entirely. - src/handlers/resource.js: both wrapper paths (CDN/local + module) now pass the JSON-LD content through. Module mode wraps consume the same opts shape. - src/handlers/resource.js: cap-aware short-circuit — when stats.size > DATA_ISLAND_MAX_BYTES the handler skips storage.read entirely. Previously a 10MB JSON-LD resource would load into memory on every HTML navigation only to have the island silently dropped by generateDatabrowserHtml. - test/data-island.test.js: replaced the single-variant </script> test with a parametrised loop covering exact/whitespace/ newline/uppercase/mixed-case end-tag forms plus <!--. All assert the same invariant: NO literal `<` survives in the script body. Added a module-mode emission test. Full suite: 551/551 (5 new tests).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+37
to
+41
| * The robust fix is to escape every `<` byte in the body to its | ||
| * JSON Unicode form `<`. JSON-LD is JSON, JSON parsers decode | ||
| * `<` back to `<` natively, so semantics are preserved. After | ||
| * this transform the body cannot contain `<` — so no end-tag (or | ||
| * comment, CDATA, etc.) can possibly start. |
Comment on lines
+55
to
+59
| const safeUri = String(resourceUrl) | ||
| .replace(/&/g, '&') | ||
| .replace(/"/g, '"') | ||
| .replace(/</g, '<') | ||
| .replace(/>/g, '>'); |
Comment on lines
+63
to
+68
| // The escape strategy is "encode every `<` as <". Test the wide | ||
| // variety of strings that an HTML parser would otherwise treat as a | ||
| // closing tag — `</script>`, `</script >`, `</script\n>`, | ||
| // `</SCRIPT>`, `</scRIPT >` — plus `<!--`, all of which require a | ||
| // literal `<` to start the dangerous sequence. After escaping, no | ||
| // `<` exists in the body at all. |
Three points: - src/mashlib/index.js: escapeForScriptBlock comment now says literally "the six-character JSON escape sequence \\u003c" instead of "<", which had ambiguously rendered the literal escape as just `<` and made the security rationale read backwards. - src/mashlib/index.js: dataIsland's URI escaping now reuses the existing escapeHtml() helper from this file instead of an inline duplicate. Function declarations hoist, so the call site can precede the helper definition without reordering. - test/data-island.test.js: parametrised escape-test header comment rewritten to describe the actual transform (`\\u003c`). Pure clarity / refactor; no behaviour change. Full suite: 551/551.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+38
to
+43
| * the six-character JSON escape sequence `<` (a backslash, the | ||
| * letter u, then four hex digits). JSON-LD is JSON, and a JSON parser | ||
| * decodes `<` back to `<` natively, so the document's semantics | ||
| * are preserved. After this transform the body literally cannot | ||
| * contain a `<` byte — so no end-tag (or comment, CDATA, etc.) can | ||
| * possibly start. |
Comment on lines
+67
to
+70
| * @param {object} [opts] | ||
| * @param {string} [opts.embedJsonLd] - JSON-LD bytes to inline as a | ||
| * `<script type="application/ld+json">` data island. Honors a 256 KB | ||
| * size cap; oversize payloads are silently dropped. Phase 1 of #7. |
- src/mashlib/index.js: rewrote the escapeForScriptBlock comment to describe the transform unambiguously: "the JSON string-escape for U+003C — the six characters backslash-u-0-0-3-c". The previous attempt kept losing the literal `\\u003c` text to the rendering pipeline and reading as `<` in source, inverting the security rationale. - src/mashlib/index.js: escapeForScriptBlock now coerces input via `String(jsonLdString)`, so a Buffer (e.g. straight from `storage.read()`) passes through cleanly instead of throwing on `.replace`. Buffer.byteLength already accepted both, so dataIsland's size cap was already buffer-safe. - src/mashlib/index.js: JSDoc for `opts.embedJsonLd` now declares `string|Buffer` on both wrapper functions. - test/data-island.test.js: new regression test passing a Buffer payload directly, asserting the island emits with the expected body content. Full suite: 552/552.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+56
to
+58
| if (Buffer.byteLength(jsonLdString, 'utf8') > DATA_ISLAND_MAX_BYTES) return ''; | ||
| const safeUri = escapeHtml(String(resourceUrl)); | ||
| const safeBody = escapeForScriptBlock(jsonLdString); |
DATA_ISLAND_MAX_BYTES was being checked against the *pre-escape* input. escapeForScriptBlock can expand input up to 6x (each `<` byte becomes the six-char escape `\\u003c`), so a `<`-heavy body just under the cap would balloon the HTML response well past it. Move the size check after the escape and gate on the bytes that will actually appear in the response. Comment updated accordingly. New regression test: a half-cap-size payload of pure `<` bytes — 6x expansion guarantees it would slip past the old check but is correctly rejected now. Full suite: 553/553.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+61
to
+66
| function dataIsland(resourceUrl, jsonLdString) { | ||
| if (!jsonLdString) return ''; | ||
| const safeBody = escapeForScriptBlock(jsonLdString); | ||
| if (Buffer.byteLength(safeBody, 'utf8') > DATA_ISLAND_MAX_BYTES) return ''; | ||
| const safeUri = escapeHtml(String(resourceUrl)); | ||
| return `<script type="application/ld+json" id="dataisland" data-uri="${safeUri}">${safeBody}</script>`; |
Comment on lines
+56
to
+59
| * expand input up to 6x (each `<` becomes 6 chars `<`), so | ||
| * checking the raw input size could let an HTML response balloon past | ||
| * the cap. We always escape first (it's cheap, single-pass) and then | ||
| * gate on the result. |
Comment on lines
+65
to
+66
| // explodes 6x after escaping — every byte becomes `<`. Without | ||
| // the post-escape check, this would emit a multi-megabyte island. |
Three small refinements: 1. dataIsland(): add cheap raw-byte pre-check before escaping. Since escapeForScriptBlock can only grow the body (each `<` becomes 6 bytes), a raw payload already over the cap is guaranteed to be over after escaping — skip the work. Post-escape check still guards the `<`-heavy expansion case. 2. mashlib/index.js dataIsland doc comment: clarify the 6x escape expansion using prose form for the escape sequence so it survives round-trips through tooling that might otherwise interpret the literal characters. 3. test/data-island.test.js post-escape cap test comment: same clarification. No behaviour change beyond the pre-check fast path; all 553 tests still pass.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (storedContentType === 'application/ld+json' && | ||
| stats.size <= DATA_ISLAND_MAX_BYTES) { | ||
| const buf = await storage.read(storagePath); | ||
| if (buf) embedJsonLd = buf.toString('utf8'); |
Comment on lines
+241
to
+253
| // Phase 1 of #7: also embed the container's JSON-LD listing as a | ||
| // data island so consumers that look for `<script | ||
| // type="application/ld+json">` (search-engine rich-results, | ||
| // archival crawlers, future mashlib zero-fetch path) get the data | ||
| // without a second request. | ||
| const embedJsonLd = serializeJsonLd(jsonLd); | ||
| const html = request.mashlibModule | ||
| ? generateModuleDatabrowserHtml(request.mashlibModule) | ||
| : generateDatabrowserHtml(resourceUrl, request.mashlibCdn ? request.mashlibVersion : null); | ||
| ? generateModuleDatabrowserHtml(request.mashlibModule, resourceUrl, { embedJsonLd }) | ||
| : generateDatabrowserHtml( | ||
| resourceUrl, | ||
| request.mashlibCdn ? request.mashlibVersion : null, | ||
| { embedJsonLd } | ||
| ); |
Two genuine optimizations from this round (the other flags were
re-runs of points already addressed in earlier rounds — pre-check,
Buffer coercion, stats.size guard, module-mode wrapper — all already
present in HEAD):
1. Container path: use compact JSON.stringify(jsonLd) for the embed
instead of serializeJsonLd(), which pretty-prints with 2-space
indent. The HTTP body still uses serializeJsonLd; only the inlined
data island goes compact, so we don't waste bytes against
DATA_ISLAND_MAX_BYTES on whitespace nothing reads.
2. Resource path: pass the storage.read() Buffer through to
embedJsonLd directly. dataIsland() already coerces Buffer → string
via String(jsonLdString), so the eager buf.toString('utf8') in the
handler was redundant.
All 553 tests still pass.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+245
to
249
| // without a second request. Use compact (no-whitespace) form for | ||
| // the embed so we don't burn bytes against DATA_ISLAND_MAX_BYTES | ||
| // on indentation that nothing will ever read. | ||
| const embedJsonLd = JSON.stringify(jsonLd); | ||
| const html = request.mashlibModule |
| * Phase-1 tests for the JSON-LD data island (#7). | ||
| * | ||
| * The mashlib HTML wrapper now carries the resource's JSON-LD bytes | ||
| * inside a `<script type="application/ld+json" id="dataisland">` |
| // After our escape, the body must contain NO literal `<`. | ||
| assert.doesNotMatch(inner, /</, | ||
| `script body must not contain a literal "<" — got: ${JSON.stringify(inner)}`); | ||
| // The escaped form should be present (`<`). |
Two doc-only fixes from this round; the other 7 inline comments
re-flag points already addressed in earlier rounds (raw-byte
pre-check, Buffer coercion, stats.size guard, module-mode wrapper,
compact embed serialization).
1. Header comment: include `data-uri="..."` in the script tag
example so it matches the actual emission shape the tests assert.
2. Inline comment on the escape-presence assertion: refer to the
six-character escape sequence in prose form ("backslash-u-0-0-3-c")
so the source intent survives tooling that converts the literal
characters back to `<`.
No code change; all 553 tests still pass.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The mashlib HTML wrapper now carries the originating resource's JSON-LD bytes as a
<script type="application/ld+json" id="dataisland" data-uri="…">block. Phase 1 is strictly additive — mashlib still XHR-fetches as before, browsers ignore non-JS script bodies, and anything that already knows to look forapplication/ld+jsondata islands (search engines / Rich Results, archival crawlers, scrapers, static-site exporters, future LWS-aware tooling) now finds the data without a second HTTP request.This sets up Phase 2, where a small inline shim will patch mashlib's
fetcherto read the island instead of refetching, eliminating the double-fetch that motivated the recent Vary/Cache work (#316, #325/#326).Why JSON-LD (not Turtle)
<script type="application/ld+json">is the standardised way to embed structured data on the web.service[]entry we landed in Emit CID service[] with lws:OpenIdProvider in WebID profiles (#320) #321 without conneg.Where the island appears
Size cap
DATA_ISLAND_MAX_BYTES = 256 KB. Above that, the island is dropped — fail-open to the existing XHR path so we don't make every navigation re-download a multi-megabyte resource.Security
The script body is non-JS MIME so the browser doesn't execute it, but a literal
</script>substring inside the body would prematurely close the tag and let arbitrary subsequent bytes parse as inline HTML. We escape:</script>→<\/script>(only the slash is meaningful to the HTML parser; rdflib treats both identically).<!--→<!--to neutralise HTML-comment-escape tricks.data-uriattribute is HTML-entity-encoded (&,",<,>) so attribute-quote injection isn't possible either.Test plan
test/data-island.test.js:id, MIME,data-uri)</script>escape: a trojan payload can't close the tag<!--escape: same protection against comment escapesdata-uriattribute encoding against quote / angle-bracket injectionAccept: application/ld+jsonstill serves RDF (no wrapper)What this doesn't do (Phase 2+)
Part of #7.