Skip to content

fix: validate URL protocols in data-driven href attributes to prevent XSS - #143

Merged
steipete merged 1 commit into
openclaw:mainfrom
SebTardif:fix/validate-data-urls
May 29, 2026
Merged

fix: validate URL protocols in data-driven href attributes to prevent XSS#143
steipete merged 1 commit into
openclaw:mainfrom
SebTardif:fix/validate-data-urls

Conversation

@SebTardif

@SebTardif SebTardif commented May 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Hardens data-driven external links before they are rendered into href attributes.

Current-main rewrite from the original PR:

  • adds sanitizeUrl() in src/lib/sanitize-url.ts
  • applies it to JSON/content-driven links on the homepage, shoutouts, showcase, press, and blog author metadata
  • drops obsolete trust-page hunks that no longer exist on current main
  • adds focused Bun regression tests for dangerous protocols, case variants, percent-encoded protocol attempts, control-character tricks, malformed URLs, and allowed http/https/mailto/relative URLs

Proof

$ bun test
4 pass, 0 fail, 14 expect() calls

$ bun run build
16 page(s) built

$ /Users/steipete/Projects/agent-scripts/skills/autoreview/scripts/autoreview --mode commit --commit HEAD
autoreview clean: no accepted/actionable findings reported

@vercel

vercel Bot commented May 17, 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.

@vercel vercel Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Additional Suggestion:

URLs from i18n/trust.json files are rendered in href attributes without sanitization in zh-cn, ko, and ja trust pages, creating an inconsistency with sanitized URLs used in testimonials, press, and showcase pages.

Fix on Vercel

Comment thread src/lib/sanitize-url.ts Outdated
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels May 21, 2026
@clawsweeper

clawsweeper Bot commented May 21, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed May 28, 2026, 10:39 PM ET / 02:39 UTC.

Summary
Adds a shared URL protocol sanitizer, wraps JSON/content-driven hrefs on the homepage, shoutouts, showcase, press, and blog author metadata, adds Bun regression tests and a test script, and adds a changelog entry.

Reproducibility: yes. source-reproducible: current main maps testimonial, community-build, press, and author URL fields directly into href attributes. I did not run a local build during this read-only review, but the code path and the PR regression tests make the issue boundary clear.

Review metrics: 2 noteworthy metrics.

  • Href call sites wrapped: 8 call sites. The diff hardens several rendered anchors across multiple public pages, so reviewers should verify the helper is consistently applied to the intended data sources.
  • Release-owned file touched: 1 CHANGELOG.md line added. OpenClaw release notes are release-owned, making this a small but concrete cleanup before a normal merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

  • [P2] Remove the release-owned CHANGELOG.md entry before merge.

Risk before merge

  • [P1] The release-owned CHANGELOG.md edit would merge release-note ownership into a normal PR unless a release owner explicitly keeps it.

Maintainer options:

  1. Decide the mitigation before merge
    Land the sanitizer and regression tests after removing the release-owned changelog entry or having release ownership carry the release note separately.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] A narrow mechanical repair can remove the release-owned CHANGELOG.md entry; the sanitizer code itself is ready for maintainer security review.

Security
Cleared: The diff narrows href protocol exposure and adds no dependencies, lifecycle hooks, CI permission changes, secrets handling, downloaded artifacts, or package-resolution changes.

Review findings

  • [P3] Remove the release-owned changelog edit — CHANGELOG.md:6
Review details

Best possible solution:

Land the sanitizer and regression tests after removing the release-owned changelog entry or having release ownership carry the release note separately.

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

Yes, source-reproducible: current main maps testimonial, community-build, press, and author URL fields directly into href attributes. I did not run a local build during this read-only review, but the code path and the PR regression tests make the issue boundary clear.

Is this the best way to solve the issue?

Yes for the mitigation: a shared allowlist sanitizer at the data-driven href call sites is narrow and tested. The changelog entry should be removed from the branch or handled by the release owner separately.

Full review comments:

  • [P3] Remove the release-owned changelog edit — CHANGELOG.md:6
    OpenClaw keeps CHANGELOG.md release-owned for normal PR review. Please drop this Unreleased entry from the branch and keep the release-note context in the PR body or commit message unless a release owner explicitly carries it.
    Confidence: 0.92

Overall correctness: patch is correct
Overall confidence: 0.88

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

Codex review notes: model gpt-5.5, reasoning high; reviewed against bc410810fe84.

Label changes

Label justifications:

  • P2: This is a bounded static-site XSS hardening PR for contributor-controlled content fields, with limited blast radius and no production outage signal.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): Sufficient terminal proof: the current PR body reports Bun sanitizer tests and an Astro build, and the earlier review record documents red-green terminal proof for a malicious testimonial URL on the same mitigation.
  • proof: sufficient: Contributor real behavior proof is sufficient. Sufficient terminal proof: the current PR body reports Bun sanitizer tests and an Astro build, and the earlier review record documents red-green terminal proof for a malicious testimonial URL on the same mitigation.
Evidence reviewed

Acceptance criteria:

  • [P1] git diff --check.
  • [P1] bun test.
  • [P1] bun run build.

What I checked:

  • Current main has direct data hrefs: Current main maps testimonial, community-build, press, and author URL fields directly into href attributes before this PR. (src/pages/index.astro:537, bc410810fe84)
  • PR adds the sanitizer: The PR adds sanitizeUrl with an allowed protocol set, percent-decoding before protocol parsing, control-character stripping, and a fallback to '#'. (src/lib/sanitize-url.ts:1, dfb9be623bd6)
  • PR wraps the affected href call sites: The diff applies sanitizeUrl to the named homepage, shoutouts, showcase, press, and blog author hrefs. (src/pages/index.astro:538, dfb9be623bd6)
  • Regression tests cover protocol bypasses: The new Bun tests cover allowed URLs, dangerous protocols, percent-encoded protocol attempts, control-character tricks, and unparsable input. (tests/sanitize-url.test.ts:1, dfb9be623bd6)
  • Prior review thread resolved an encoded-protocol concern: The Vercel review first flagged percent-encoded dangerous protocols, then followed up that the current decode-plus-control-character implementation resolves that bypass. (src/lib/sanitize-url.ts:5, dfb9be623bd6)
  • Release-owned changelog file is touched: The branch adds an Unreleased entry to CHANGELOG.md, which conflicts with the OpenClaw release-note review rule that normal PRs should leave CHANGELOG.md release-owned. (CHANGELOG.md:6, dfb9be623bd6)

Likely related people:

  • steipete: Peter Steinberger recently rewrote the site pages that contain the affected href surfaces, touched adjacent avatar/link rendering, and authored the current PR head commit. (role: recent area contributor and PR branch committer; confidence: high; commits: 233678a98807, 1bb818334986, dfb9be623bd6; files: src/pages/index.astro, src/pages/shoutouts.astro, src/pages/showcase.astro)
  • Tilakraj0308: The grafted initial website commit added many of the current page/data files and direct href render paths that this PR now hardens. (role: introduced affected website surface; confidence: medium; commits: ed7950ff81f1; files: src/pages/index.astro, src/pages/shoutouts.astro, src/pages/showcase.astro)
  • SebTardif: Sebastien Tardif has recent merged security hardening in adjacent trust-page sanitization and current site/page work, so they are relevant beyond opening this PR. (role: recent related security-hardening contributor; confidence: medium; commits: 11291a44f6b1, 5108f7b95859, bc410810fe84; files: src/lib/sanitize-html.ts, src/pages/trust/ja/index.astro, src/pages/press.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.

@clawsweeper

clawsweeper Bot commented May 21, 2026

Copy link
Copy Markdown

ClawSweeper PR egg

✨ Hatched: 🥚 common Mossy Crabkin

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🥚 common.
Trait: purrs at green checks.
Image traits: location flaky test forest; accessory commit compass; palette charcoal, cyan, and signal green; mood curious; pose waving from a small platform; shell woven fiber shell; lighting warm desk-lamp glow; background small review tokens.
Share on X: post this hatch
Copy: My PR egg hatched a 🥚 common Mossy Crabkin in ClawSweeper.

What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 22, 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: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels May 22, 2026
@SebTardif
SebTardif force-pushed the fix/validate-data-urls branch from d91b2e0 to d016691 Compare May 24, 2026 11:52
Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
@steipete
steipete force-pushed the fix/validate-data-urls branch from d016691 to dfb9be6 Compare May 29, 2026 02:33
@steipete
steipete merged commit e65fcdf into openclaw:main May 29, 2026
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants