Sanitise project instructions before rendering them

[jamiebenstead]

Instruction step HTML was assigned to innerHTML unsanitised, so a step could run script in the host page's origin, where the login session is stored. Every step now goes through DOMPurify, covering both the instructions attribute and steps loaded with a project.

Two tags DOMPurify drops by default are kept because real content needs them: use, which draws the icons in a scratchblocks SVG, and iframe, which is then restricted to the editor's own origins so the embedded project viewers in project instructions keep working.

closes https://github.com/RaspberryPiFoundation/digital-editor-issues/issues/1738

[Copilot]

Pull request overview

This PR addresses an XSS risk in the Instructions sidebar by sanitising instruction-step HTML before it is inserted into the DOM via innerHTML, ensuring steps coming from both the instructions attribute and loaded projects are cleaned consistently.

Changes:

• Added a dedicated sanitiseInstructions utility built on DOMPurify with hooks to preserve required Scratchblocks SVG elements and allowlisted project-viewer iframes.
• Updated InstructionsStep to sanitise both raw HTML steps and markdown-rendered steps before rendering.
• Added comprehensive unit tests covering common scriptable/XSS payloads, Scratchblocks output, and allowed/blocked iframe embeds, plus documented the behaviour in the README.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/utils/sanitiseInstructions.js	Introduces DOMPurify-based sanitiser with element-level hooks (iframe/style/use) for project instructions.
src/utils/sanitiseInstructions.test.js	Adds extensive sanitisation tests for XSS payloads, embeds, project-site markup, and scratchblocks SVG.
src/components/Menus/Sidebar/InstructionsPanel/InstructionsStep/InstructionsStep.jsx	Routes step HTML (raw or markdown-rendered) through the new sanitiser before setting innerHTML.
src/components/Menus/Sidebar/InstructionsPanel/InstructionsStep/InstructionsStep.test.jsx	Verifies scripts are stripped for both HTML and markdown step sources.
README.md	Documents why and how instructions are sanitised, including iframe allowlisting behaviour.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

src/utils/sanitiseInstructions.js:41

• Any untrusted <style> can bypass removal simply by being nested in an <svg>, e.g. <svg><style>@import url(https://evil.example/x.css); ...</style></svg>. Such rules are not sanitized by DOMPurify and can style the rest of the editor's shadow tree. Keep styles only for SVGs known to have been generated by scratchblocks, or inject the trusted scratchblocks stylesheet after sanitizing instead of trusting arbitrary SVG ancestry.

  // A stylesheet anywhere else would restyle the rest of the editor
  if (tagName === "style" && !node.closest("svg")) {
    return remove(node);

README.md:255

• This states that all data: URLs are removed, but DOMPurify deliberately permits data URIs on media elements such as <img>, and this configuration does not override that behavior. Narrow the documented guarantee to dangerous/executable URL contexts so integrators do not rely on a restriction the sanitizer does not enforce.

a project. Scripts, event handler attributes, `javascript:` and `data:` URLs and
stylesheets outside a scratchblocks SVG are removed.

src/utils/sanitiseInstructions.js:12

• This checks only the origin, so attacker-controlled instructions can retain an iframe for any route on these hosts (for example, the editor home or account pages), even though this function and the surrounding comment say only project viewers are allowed. Restrict the pathname to the supported /embed/viewer/<identifier> forms (including the optional locale) and reject same-origin non-viewer paths.

    return EMBED_ORIGINS.includes(new URL(src).origin);

[DNR500]

Nice! This is looking like it should do the job.

I've added a couple of points where things might be tighten up a little more - that maybe be worth a look at before merging