Skip to content

fix(seo): escape JSON-LD script content to prevent breakout - #208

Merged
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/json-ld-script-escape
Jul 12, 2026
Merged

fix(seo): escape JSON-LD script content to prevent breakout#208
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/json-ld-script-escape

Conversation

@SebTardif

@SebTardif SebTardif commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Layout.astro embeds structured data with
set:html={JSON.stringify(item)}. If any string field contains a raw
</script> sequence, the HTML parser closes the ld+json script element
early (classic JSON-in-script breakout).

Evidence

Fixed escape uses a literal \u003c sequence in the serialized JSON
(not a JS string that is already <).

$ node proof.js
layout_line: <script type="application/ld+json" set:html={JSON.stringify(item).replace(/</g, "\\u003c")} />
serialized: {"name":"\u003c/script>\u003cscript>alert(1)\u003c/script>","n":1}
contains_raw_close: false
contains_unicode_escape: true
naive_script_splits: 2
roundtrip_name: </script><script>alert(1)</script>

contains_raw_close: false means the HTML stream has no raw </script>
inside the JSON text. JSON.parse still restores the original name.

Summary

  • Escape < as the six-character sequence \u003c after JSON.stringify
  • JS source must be "\\u003c" (a prior "\u003c" form was a no-op)

Real behavior proof

  • Behavior or issue addressed: JSON-LD script breakout via raw less-than in stringified structured data
  • Real environment tested: macOS, Node 22+, branch fix/json-ld-script-escape at current HEAD
  • Exact steps or command run after this patch: node demo of the exact Layout expression; confirm layout line contains double-backslash escape
  • Evidence after fix: terminal output above (no raw close sequence; roundtrip preserves name)
  • Observed result after fix: serialized JSON cannot close the script tag early; data still parses
  • What was not tested: full astro build HTML on this machine (design-system package resolution blocked in worktree); logic matches the committed Layout expression

Related

  • ClawSweeper review caught the no-op "\u003c" form
  • React/Next JSON-LD escape guidance

@vercel

vercel Bot commented Jul 10, 2026

Copy link
Copy Markdown

@SebTardif is attempting to deploy a commit to the Jamie's projects Team on Vercel.

A member of the Team first needs to authorize it.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 10, 2026
@clawsweeper

clawsweeper Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 11, 2026, 8:02 PM ET / July 12, 2026, 00:02 UTC.

Summary
The PR routes the layout’s JSON-LD raw-script sink through a serializer that escapes literal less-than characters and adds hostile-payload unit tests plus generated-page parseability checks.

Reproducibility: yes. Current main directly inserts JSON.stringify output into an HTML raw-text script element, so structured data containing a literal closing-script sequence gives a deterministic source-level reproduction; the supplied browser proof confirms the corrected path.

Review metrics: 3 noteworthy metrics.

  • Production boundary: 1 sink changed. The patch fixes the shared JSON-LD raw-script boundary rather than duplicating escaping across page callers.
  • Regression coverage: 1 new unit suite, 1 built-output check expanded. Serialization semantics and generated-site integration are independently covered.
  • Patch scope: 4 files, +72/-1. The security correction remains focused and reviewable.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Next step before merge

  • [P2] Only ordinary maintainer approval and merge handling remain; there is no contributor-facing repair for ClawSweeper to queue.

Security
Cleared: The patch correctly hardens the HTML script boundary without adding dependencies, permissions, secret access, downloaded execution, or other supply-chain surface.

Review details

Best possible solution:

Merge the exact head after ordinary maintainer approval, retaining centralized post-serialization escaping at the sole JSON-LD sink and both unit and generated-output regression layers.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main directly inserts JSON.stringify output into an HTML raw-text script element, so structured data containing a literal closing-script sequence gives a deterministic source-level reproduction; the supplied browser proof confirms the corrected path.

Is this the best way to solve the issue?

Yes. Escaping literal less-than characters after JSON serialization at the shared sink is the narrowest maintainable solution because it preserves parsed values, removes every HTML tag opener, and covers all current structured-data callers.

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning high; reviewed against a6f92f38a2b0.

Label changes

Label changes:

  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

  • P1: Current main has a concrete public-page script breakout path, and this PR provides an urgent, narrowly scoped, strongly proven correction.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): Maintainer evidence includes a production build, adversarial browser exercise, visible console assertions, successful JSON parsing, and repository CI; the final head only removes the prohibited changelog line.
  • proof: sufficient: Contributor real behavior proof is sufficient. Maintainer evidence includes a production build, adversarial browser exercise, visible console assertions, successful JSON parsing, and repository CI; the final head only removes the prohibited changelog line.
Evidence reviewed

What I checked:

  • Current-main vulnerable path: Current main serializes structured data directly into the sole application/ld+json raw-HTML script sink, allowing a literal closing-script sequence to reach HTML raw text. (src/layouts/Layout.astro:79, a6f92f38a2b0)
  • Exact-head mitigation: The shared serializer performs JSON serialization first and then replaces every literal less-than sign with a literal JSON Unicode escape, preserving JSON.parse semantics while eliminating HTML tag openers. (src/lib/serialize-json-ld.ts:4, de6a5e282c1b)
  • Complete sink coverage: Repository-wide source inspection shows one JSON-LD raw-script sink, and the exact head routes that boundary through the serializer for all current structured-data callers. (src/layouts/Layout.astro:80, de6a5e282c1b)
  • Regression coverage: The new tests verify hostile and deterministic nested values contain no raw less-than signs and round-trip through JSON.parse; built-asset checks parse every generated JSON-LD block. (tests/serialize-json-ld.test.ts:15, de6a5e282c1b)
  • Previous finding resolved: The exact head differs from the previously reviewed implementation head only by deleting the PR-authored CHANGELOG.md entry, resolving the release-owned changelog finding without altering production or test behavior. (CHANGELOG.md:4, de6a5e282c1b)
  • Runtime proof and checks: The discussion provides production-build and Chrome-console proof against the implementation candidate, including one parseable JSON-LD block and no breakout side effect; the final head changes only CHANGELOG.md, and GitHub reports it mergeable with repository-owned checks passing or in progress. (de6a5e282c1b)

Likely related people:

  • steipete: The structured-data sink traces to commit 34d5fdc, and steipete authored the hardened implementation, follow-up policy cleanup, and exact-head proof report. (role: introduced behavior and current branch integrator; confidence: high; commits: 34d5fdc4f403, c3a15ed1a19b, de6a5e282c1b; files: src/layouts/Layout.astro, src/lib/serialize-json-ld.ts, tests/assert-built-assets.mjs)
  • Vincent Koc: Recent commits expanded structured-data callers and modified the built-asset test surface now extended by this PR. (role: recent structured-data and built-asset contributor; confidence: medium; commits: 51aacf002017, bd9fa89b520d; files: src/layouts/Layout.astro, src/pages/blog/authors/[slug].astro, tests/assert-built-assets.mjs)
  • Patrick Erichsen: Commit 67273bf recently refactored Layout.astro to the shared design system, making Patrick relevant for adjacent layout integration review. (role: recent layout refactor contributor; confidence: medium; commits: 67273bf80ce1; files: src/layouts/Layout.astro)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (8 earlier review cycles)
  • reviewed 2026-07-10T14:28:05.540Z sha 0741604 :: needs real behavior proof before merge. :: [P1] Emit a literal JSON escape instead of decoding it back to
  • reviewed 2026-07-10T16:27:13.729Z sha 21076ec :: needs real behavior proof before merge. :: [P1] Emit a literal JSON escape instead of decoding it back to
  • reviewed 2026-07-10T16:34:13.868Z sha 21076ec :: needs real behavior proof before merge. :: [P1] Emit a literal JSON escape instead of decoding it back to
  • reviewed 2026-07-10T16:48:46.895Z sha 296a742 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-10T16:53:41.624Z sha 296a742 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-10T16:58:41.812Z sha 296a742 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-11T23:45:53.663Z sha c3a15ed :: needs real behavior proof before merge. :: [P3] Remove the release-owned changelog entry
  • reviewed 2026-07-11T23:55:22.180Z sha c3a15ed :: needs changes before merge. :: [P3] Remove the release-owned changelog entry

@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Added What Problem This Solves, Evidence, and Real behavior proof with node escape demo (raw </script> removed, JSON roundtrip preserved).

@clawsweeper

clawsweeper Bot commented Jul 10, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Fixed the no-op escape: source is now "\\u003c" so the payload contains a literal backslash-u escape, not a raw less-than. Terminal proof shows contains_raw_close: false and JSON roundtrip still works.

@clawsweeper

clawsweeper Bot commented Jul 10, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 10, 2026
Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
@steipete
steipete force-pushed the fix/json-ld-script-escape branch from 296a742 to c3a15ed Compare July 11, 2026 23:39
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 11, 2026
@steipete

steipete commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Maintainer proof for final exact head de6a5e282c1bd73f333b04c629c885492f89ad86:

  • Root cause: raw serialized JSON-LD was passed to an HTML script sink, so an adversarial closing-script sequence could terminate the data block.
  • Fix: a dedicated serializer escapes every literal less-than sign after JSON serialization; parsing restores the original value. The layout uses that helper at the only JSON-LD raw-HTML boundary.
  • Regression coverage: bun test, bun run build, and bun run test:built-assets passed. Tests cover mixed-case and malformed closing-tag payloads, comments, nested values, deterministic property payloads, exact escape emission, and every generated HTML page's JSON-LD parseability. Build output: 33 pages.
  • Exact browser behavior: the built preview was exercised in the existing Chrome profile through Peekaboo with a unique adversarial title. The page rendered normally; Chrome console checks returned [true, true, 1, true]: no breakout node, no script side effect, exactly one JSON-LD block, and successful JSON.parse.
  • Candidate identity: the only changes after browser proof were mainline workflow metadata and removal of this PR's changelog line; the complete runtime/test tree remains byte-identical to the built/browser-tested candidate.
  • Review closeout: the hosted finding about PR-owned changelog churn was accepted and fixed. Fresh structured Codex autoreview reports no accepted/actionable findings (patch is correct, confidence 0.98).
  • Hosted CI: final-head runs 29172964411 and 29172964412 passed every repository-owned Linux, Windows, installer, unit, smoke, and shellcheck job. The macOS conditional job is intentionally skipped.
  • Preview note: both Vercel contexts refused deployment because fork authorization is required; they did not execute and are not code failures. The local production build plus existing-profile browser proof exercises the changed rendering boundary without deployment mutation.
  • Public Model Identifier Gate: PASS; the final candidate diff, fixtures, generated-output checks, CI/proof text, and browser evidence introduce no model-bearing identifier.

Risk: low after proof. Files: 4, +72/-1. Sebastien remains credited as PR author and commit co-author.

Merge recommendation: supported.

@clawsweeper

clawsweeper Bot commented Jul 11, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 11, 2026
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 12, 2026
@steipete

Copy link
Copy Markdown
Contributor

@clawsweeper re-review

Final exact-head proof is in the updated maintainer comment above; the changelog finding is fixed.

@clawsweeper

clawsweeper Bot commented Jul 12, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@steipete
steipete merged commit 10f1d9d into openclaw:main Jul 12, 2026
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants