Skip to content

fix(security): reject unsafe link schemes#130

Merged
smiggleworth merged 3 commits into
mainfrom
fix/security-unsafe-link-schemes
Jul 26, 2026
Merged

fix(security): reject unsafe link schemes#130
smiggleworth merged 3 commits into
mainfrom
fix/security-unsafe-link-schemes

Conversation

@smiggleworth

Copy link
Copy Markdown
Contributor

Closes #129

Rejects javascript, data, vbscript, file, and obfuscated executable href schemes across Link, client intrinsic attributes, and SSR output. Safe documented schemes and relative URLs remain supported.

Verification:

  • focused unsafe-href tests: 7 passed
  • architecture tests: 13 passed
  • npm run check: passed

Copilot AI review requested due to automatic review settings July 26, 2026 17:43

Copilot AI 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.

Pull request overview

This PR introduces a shared URL-scheme allowlist for href values to prevent executable/local-file schemes from reaching client rendering, Link, and SSR attribute output.

Changes:

  • Added isSafeHref() (allowlist) for validating/sanitizing href values.
  • Enforced the policy in Link, the client intrinsic attribute renderer, and SSR attribute rendering.
  • Added jsdom regression coverage for unsafe href schemes across Link, client output, and SSR output.

Reviewed changes

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

Show a summary per file
File Description
src/common/url.ts Defines shared href scheme allowlist helper (isSafeHref).
src/components/link.tsx Throws when Link receives an unsafe href.
src/renderer/attributes.ts Omits unsafe href values when applying intrinsic props in the client renderer.
src/ssr/attrs.ts Omits unsafe href values during SSR attribute rendering.
tests/jsdom/renderer/unsafe-href.test.tsx Adds regression tests for unsafe href schemes across Link, client rendering, and SSR.
Comments suppressed due to low confidence (2)

src/ssr/attrs.ts:188

  • Same case-sensitivity issue exists in renderAttrs() (string path): attrName === 'href' can be bypassed with mixed/upper-case keys (e.g. spread props containing HREF). This should be case-insensitive to match HTML behavior.
    } else {
      const strValue = String(value);
      if (attrName === 'href' && !isSafeHref(strValue)) continue;
      attrParts.push(` ${attrName}="${getEscapedAttrValue(strValue)}"`);

src/renderer/attributes.ts:361

  • applyScalarPropValue() has the same case-sensitive href check as the static path. This leaves a bypass via spread props with HREF/mixed casing, which will still be rendered as href for HTML elements. Make the check case-insensitive.
  } else if (key === 'value' || key === 'checked') {
    applyFormControlProp(el, key, value, tagName);
  } else if (key === 'href' && !isSafeHref(String(value))) {
    removeRenderedAttribute(el, key);
  } else {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/ssr/attrs.ts
Comment thread src/renderer/attributes.ts
Comment thread tests/jsdom/renderer/unsafe-href.test.tsx
Comment thread src/components/link.tsx
@smiggleworth
smiggleworth force-pushed the fix/security-unsafe-link-schemes branch from f882d50 to 13e7157 Compare July 26, 2026 17:54
@smiggleworth
smiggleworth merged commit 10e7f3b into main Jul 26, 2026
3 checks passed
@smiggleworth
smiggleworth deleted the fix/security-unsafe-link-schemes branch July 26, 2026 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: reject executable URL schemes in links

2 participants