fix(builder): escape/allowlist all attribute-value sinks incl. numeric/enum props (XSS)
An adversarial pass found 5 Critical XSS sinks where props declared number/enum in TypeScript were interpolated raw into exported HTML attribute values, trusting the type — but nothing enforces it at runtime (AI update_props only validates node_id; deserialized saved state is untyped JSON). Fixed all 5 (NumberCounter data-target, StarRating aria-label, FormContainer method, ContactForm/InputField input type) plus 6 sibling sinks found by an exhaustive audit of every attribute-value interpolation across src/components: a JS-source injection into ContentSlider's inline setInterval script, a prototype-pollution-adjacent allowlist gap in Section's divider-shape lookup, TextareaField rows, Testimonials rating aria-label, HeroSimple textAlign, and MapEmbed zoom. Adds shared sanitizeFormMethod/sanitizeInputType allowlist helpers to utils/escape.ts alongside the existing escapeAttr/safeUrl/cssValue primitives. Every fix is TDD'd: a malicious-value test reproduces the raw injection against the pre-fix code, then passes after the fix. 502 tests green (npx vitest run), tsc + vite build green (npm run build). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -110,3 +110,23 @@ describe('ContentSlider.toHtml renders slide.imageSrc as a background-image (INT
|
||||
expect(html).toContain('background-color:#123456');
|
||||
});
|
||||
});
|
||||
|
||||
describe('ContentSlider.toHtml interval is NOT runtime-type-checked -- must be coerced before it reaches the inline <script> numeric context', () => {
|
||||
test('a malicious interval string cannot inject arbitrary JS into the autoplay setInterval call', () => {
|
||||
const malicious = '5000);alert(document.domain);//';
|
||||
const { html } = toHtml({ slides, autoplay: true, interval: malicious }, '');
|
||||
expect(html).not.toContain('alert(document.domain)');
|
||||
// the setInterval call must still be well-formed with a plain numeral delay
|
||||
expect(html).toMatch(/setInterval\(function\(\)\{show\(current\+1\);\},\d+\);/);
|
||||
});
|
||||
|
||||
test('a non-numeric interval falls back to a safe default delay', () => {
|
||||
const { html } = toHtml({ slides, autoplay: true, interval: 'not-a-number' }, '');
|
||||
expect(html).toMatch(/setInterval\(function\(\)\{show\(current\+1\);\},5000\);/);
|
||||
});
|
||||
|
||||
test('a normal numeric interval still renders as the exact configured delay', () => {
|
||||
const { html } = toHtml({ slides, autoplay: true, interval: 3000 }, '');
|
||||
expect(html).toMatch(/setInterval\(function\(\)\{show\(current\+1\);\},3000\);/);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -250,11 +250,24 @@ ContentSlider.craft = {
|
||||
} = props;
|
||||
|
||||
const items = slides.length > 0 ? slides : defaultSlides;
|
||||
// Number() coercion: `interval` is declared `number` in TS but is NOT
|
||||
// type-checked at runtime -- it arrives raw via the AI `update_props`
|
||||
// path or a deserialized saved-state blob and is interpolated directly
|
||||
// into the inline <script>'s `setInterval(fn, ${interval})` call below as
|
||||
// a bare JS numeral (no quotes around it). A string like
|
||||
// `5000);alert(1);//` would previously close the setInterval() call and
|
||||
// splice arbitrary JS into the page's own <script> tag -- worse than an
|
||||
// HTML attribute breakout, since it runs unconditionally on page load.
|
||||
// Number() of anything non-numeric collapses safely to NaN, so we fall
|
||||
// back to the 5000ms default rather than ever interpolating a
|
||||
// non-numeral.
|
||||
const intervalNum = Number(interval);
|
||||
const safeInterval = Number.isFinite(intervalNum) && intervalNum > 0 ? intervalNum : 5000;
|
||||
// Deterministic AND unique id, scoped on the Craft node id, for this
|
||||
// slider's slide/dot element ids and inline-script globals -- so two
|
||||
// ContentSlider instances (e.g. both left at default slides) don't
|
||||
// collide and end up driving each other's rotation.
|
||||
const uid = scopeId(nodeId, JSON.stringify(items) + interval, 'cs');
|
||||
const uid = scopeId(nodeId, JSON.stringify(items) + safeInterval, 'cs');
|
||||
|
||||
const sectionStyle = cssPropsToString({
|
||||
position: 'relative',
|
||||
@@ -339,7 +352,7 @@ ContentSlider.craft = {
|
||||
window["${uid}_prev"]=function(){ markManualNav(); show(current-1); };
|
||||
${autoplayActive ? `
|
||||
var timer=null;
|
||||
function start(){ if(!timer && document.visibilityState!=="hidden"){ timer=setInterval(function(){show(current+1);},${interval}); } }
|
||||
function start(){ if(!timer && document.visibilityState!=="hidden"){ timer=setInterval(function(){show(current+1);},${safeInterval}); } }
|
||||
function stop(){ if(timer){ clearInterval(timer); timer=null; } }
|
||||
var root=document.getElementById(uid);
|
||||
if(root){
|
||||
|
||||
@@ -0,0 +1,35 @@
|
||||
import { describe, test, expect } from 'vitest';
|
||||
import { HeroSimple } from './HeroSimple';
|
||||
|
||||
const toHtml = (HeroSimple as any).toHtml;
|
||||
|
||||
describe('HeroSimple.toHtml textAlign enum sink (attacker-controlled prop, not enforced at runtime)', () => {
|
||||
test('malicious textAlign value cannot break out of the content div style attribute', () => {
|
||||
const { html } = toHtml({
|
||||
heading: 'Hi',
|
||||
subtitle: 'There',
|
||||
textAlign: 'center;"><script>alert(1)</script>',
|
||||
}, '');
|
||||
expect(html).not.toContain('<script>alert(1)</script>');
|
||||
expect(html).not.toContain('center;">');
|
||||
});
|
||||
|
||||
test('unrecognized textAlign value falls back to a safe default rather than being echoed raw', () => {
|
||||
const { html } = toHtml({ heading: 'Hi', subtitle: 'There', textAlign: 'not-a-real-value' as any }, '');
|
||||
expect(html).not.toContain('text-align:not-a-real-value');
|
||||
});
|
||||
|
||||
test('valid textAlign values are preserved', () => {
|
||||
const { html: left } = toHtml({ heading: 'Hi', subtitle: 'There', textAlign: 'left' }, '');
|
||||
expect(left).toContain('text-align:left');
|
||||
const { html: right } = toHtml({ heading: 'Hi', subtitle: 'There', textAlign: 'right' }, '');
|
||||
expect(right).toContain('text-align:right');
|
||||
});
|
||||
|
||||
test('normal default render is sane', () => {
|
||||
const { html } = toHtml({ heading: 'Welcome', subtitle: 'Sub text' }, '');
|
||||
expect(html).toContain('Welcome');
|
||||
expect(html).toContain('Sub text');
|
||||
expect(html).toContain('text-align:center');
|
||||
});
|
||||
});
|
||||
@@ -229,7 +229,13 @@ HeroSimple.craft = {
|
||||
videoHtml = `<video src="${escapeAttr(safeUrl(props.bgVideo))}" autoplay muted loop playsinline style="position:absolute;top:0;left:0;width:100%;height:100%;object-fit:cover;z-index:0"></video>`;
|
||||
}
|
||||
|
||||
const textAlign = props.textAlign || 'center';
|
||||
// Allowlisted -- `textAlign` is declared as a 'left'|'center'|'right' union
|
||||
// but arrives unchecked via AI update_props / deserialized state; it is
|
||||
// interpolated raw into the content div's style attribute below, so any
|
||||
// other value must collapse to a known-safe default rather than being
|
||||
// echoed into the markup.
|
||||
const ALLOWED_TEXT_ALIGN = ['left', 'center', 'right'];
|
||||
const textAlign = ALLOWED_TEXT_ALIGN.includes(props.textAlign as string) ? (props.textAlign as string) : 'center';
|
||||
const justifyBtn = textAlign === 'center' ? 'center' : textAlign === 'right' ? 'flex-end' : 'flex-start';
|
||||
|
||||
const ctas = normalizeCtas(props);
|
||||
|
||||
@@ -47,3 +47,29 @@ describe('NumberCounter.toHtml deterministic + unique scope ids (thread node id,
|
||||
expect(wrapId1).not.toBe(wrapId2);
|
||||
});
|
||||
});
|
||||
|
||||
describe('NumberCounter.toHtml counter.number is NOT runtime-type-checked -- must be sanitized before it reaches data-target', () => {
|
||||
test('a malicious counter.number cannot break out of the data-target attribute to inject a <script> tag', () => {
|
||||
const malicious = [
|
||||
{ number: '150"><script>alert(1)</script>', suffix: '+', label: 'Evil' },
|
||||
];
|
||||
const { html } = toHtml({ counters: malicious }, '', 'node-nc-evil1');
|
||||
expect(html).not.toContain('<script>alert(1)</script>');
|
||||
expect(html).not.toContain('"><script>');
|
||||
});
|
||||
|
||||
test('a malicious counter.number cannot break out of the data-target attribute to inject an onmouseover handler', () => {
|
||||
const malicious = [
|
||||
{ number: '150" onmouseover="alert(1)', suffix: '+', label: 'Evil' },
|
||||
];
|
||||
const { html } = toHtml({ counters: malicious }, '', 'node-nc-evil2');
|
||||
expect(html).not.toContain('onmouseover=');
|
||||
expect(html).not.toMatch(/data-target="150" onmouseover/);
|
||||
});
|
||||
|
||||
test('normal numeric counter.number values still render as data-target="150"', () => {
|
||||
const normal = [{ number: 150, suffix: '+', label: 'Projects' }];
|
||||
const { html } = toHtml({ counters: normal }, '', 'node-nc-normal');
|
||||
expect(html).toContain('data-target="150"');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -140,8 +140,20 @@ NumberCounter.craft = {
|
||||
});
|
||||
|
||||
const countersHtml = items.map((counter, i) => {
|
||||
// Number() coercion + escapeAttr: `counter.number` is declared `number`
|
||||
// per-item inside an array prop, but is NOT type-checked at runtime --
|
||||
// it arrives raw via the AI `update_props` path or a deserialized
|
||||
// saved-state blob and was previously interpolated straight into this
|
||||
// data-target attribute, letting a string like `150"><script>...`
|
||||
// break out of the attribute and inject markup. Number() collapses any
|
||||
// non-numeric value safely to NaN (then 0), and escapeAttr is kept as
|
||||
// defense-in-depth in case Number()'s string coercion output ever
|
||||
// contains a stray character (it can't today, but the sink should never
|
||||
// rely solely on the coercion).
|
||||
const numberVal = Number(counter.number);
|
||||
const safeNumber = Number.isFinite(numberVal) ? numberVal : 0;
|
||||
return `<div style="display:flex;flex-direction:column;align-items:center;gap:8px">
|
||||
<span id="${uid}_n${i}" data-target="${counter.number}" data-suffix="${escapeAttr(counter.suffix)}" style="font-size:${numberSize};font-weight:700;color:${numberColor};line-height:1.1;font-family:Inter,sans-serif">0${escapeHtml(counter.suffix)}</span>
|
||||
<span id="${uid}_n${i}" data-target="${escapeAttr(String(safeNumber))}" data-suffix="${escapeAttr(counter.suffix)}" style="font-size:${numberSize};font-weight:700;color:${numberColor};line-height:1.1;font-family:Inter,sans-serif">0${escapeHtml(counter.suffix)}</span>
|
||||
<span style="font-size:15px;color:${labelColor};font-family:Inter,sans-serif;font-weight:500">${escapeHtml(counter.label)}</span>
|
||||
</div>`;
|
||||
}).join('\n ');
|
||||
|
||||
@@ -46,3 +46,28 @@ describe('Testimonials.toHtml decorative star icons (F2.5)', () => {
|
||||
stars.forEach((tag: string) => expect(tag).toContain('aria-hidden="true"'));
|
||||
});
|
||||
});
|
||||
|
||||
describe('Testimonials.toHtml rating aria-label sink (attacker-controlled `rating`, typed number but unchecked)', () => {
|
||||
test('malicious rating value cannot break out of the star row aria-label attribute', () => {
|
||||
const malicious = [
|
||||
{ quote: 'Q', name: 'N', title: 'T', rating: '5"><script>alert(1)</script>' as any },
|
||||
];
|
||||
const { html } = toHtml({ testimonials: malicious, layout: 'grid' }, '');
|
||||
expect(html).not.toContain('<script>alert(1)</script>');
|
||||
expect(html).not.toContain('5"><script>');
|
||||
});
|
||||
|
||||
test('non-numeric rating falls back to a safe numeric value', () => {
|
||||
const malicious = [
|
||||
{ quote: 'Q', name: 'N', title: 'T', rating: 'not-a-number' as any },
|
||||
];
|
||||
const { html } = toHtml({ testimonials: malicious, layout: 'grid' }, '');
|
||||
expect(html).toMatch(/aria-label="Rating: 0 out of 5"/);
|
||||
});
|
||||
|
||||
test('normal numeric rating still renders correctly', () => {
|
||||
const { html } = toHtml({ testimonials, layout: 'grid' }, '');
|
||||
expect(html).toContain('aria-label="Rating: 5 out of 5"');
|
||||
expect(html).toContain('aria-label="Rating: 4 out of 5"');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -41,10 +41,16 @@ function renderStars(count: number, color: string): React.ReactNode {
|
||||
}
|
||||
|
||||
function starsHtml(count: number, color: string): string {
|
||||
// `count` is `Testimonial.rating`, typed `number` but arrives unchecked via
|
||||
// AI update_props / deserialized state -- coerce to a real number before
|
||||
// it's interpolated into the aria-label attribute below (both the loop
|
||||
// comparison and the escapeAttr(String(...)) call are safe against any
|
||||
// non-numeric/garbage value).
|
||||
const safeCount = Number(count) || 0;
|
||||
const stars = [1, 2, 3, 4, 5].map((i) =>
|
||||
`<i class="fa ${i <= count ? 'fa-star' : 'fa-star-o'}" style="color:${color};font-size:14px" aria-hidden="true"></i>`
|
||||
`<i class="fa ${i <= safeCount ? 'fa-star' : 'fa-star-o'}" style="color:${color};font-size:14px" aria-hidden="true"></i>`
|
||||
).join('');
|
||||
return `<div style="display:flex;gap:2px;justify-content:center;margin-bottom:12px" role="img" aria-label="Rating: ${count} out of 5">${stars}</div>`;
|
||||
return `<div style="display:flex;gap:2px;justify-content:center;margin-bottom:12px" role="img" aria-label="${escapeAttr(`Rating: ${safeCount} out of 5`)}">${stars}</div>`;
|
||||
}
|
||||
|
||||
export const Testimonials: UserComponent<TestimonialsProps> = ({
|
||||
|
||||
Reference in New Issue
Block a user