From d7b170cc73cf6330d0d6b1513255a7b2265babbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Chopin?= Date: Mon, 19 Oct 2026 13:04:41 +0200 Subject: [PATCH 1/2] fix: don't leak ancestor props into elements whose bindings are all omitted An element shadows its parent's `props` scope only when it has its own attributes. The check counted the resolved attributes, so an element whose bindings were all omitted (for example an unsafe `:href`) let the parent's (HTML) or grandparent's (Vue, React, Svelte, Angular) props leak into its children. The check now uses the raw attributes, ignoring `$`. Extracted from #490. Co-authored-by: Adam DeHaven --- .../src/components/markdown-node.component.ts | 19 ++++- .../comark-angular/test/props-scope.test.ts | 69 +++++++++++++++++++ packages/comark-html/test/props-scope.test.ts | 32 +++++++++ .../src/components/MarkdownDocument.tsx | 11 ++- .../comark-react/test/props-scope.test.tsx | 46 +++++++++++++ .../src/components/MarkdownNode.svelte | 13 +++- .../comark-svelte/test/props-scope.test.ts | 34 +++++++++ .../test/test-components/Probe.svelte | 5 ++ .../src/components/MarkdownDocument.ts | 11 ++- packages/comark-vue/test/props-scope.test.ts | 46 +++++++++++++ .../comark/src/internal/stringify/state.ts | 18 +++-- 11 files changed, 291 insertions(+), 13 deletions(-) create mode 100644 packages/comark-angular/test/props-scope.test.ts create mode 100644 packages/comark-html/test/props-scope.test.ts create mode 100644 packages/comark-react/test/props-scope.test.tsx create mode 100644 packages/comark-svelte/test/props-scope.test.ts create mode 100644 packages/comark-svelte/test/test-components/Probe.svelte create mode 100644 packages/comark-vue/test/props-scope.test.ts diff --git a/packages/comark-angular/src/components/markdown-node.component.ts b/packages/comark-angular/src/components/markdown-node.component.ts index 50cc7de8..f6c38118 100644 --- a/packages/comark-angular/src/components/markdown-node.component.ts +++ b/packages/comark-angular/src/components/markdown-node.component.ts @@ -138,8 +138,15 @@ export class MarkdownNode implements OnChanges { // Resolve attributes (:binding support) const resolved = resolveAttributes(nodeProps, this.renderData, { parseJson: true }) - // Build childrenRenderData - only shadow parent scope when element has own attrs - const hasOwnAttrs = Object.keys(resolved).length > 0 + // Build childrenRenderData - only shadow parent scope when element has own attrs. + // The check uses the raw attributes: resolving can omit all of them. + let hasOwnAttrs = false + for (const key in nodeProps) { + if (key !== '$') { + hasOwnAttrs = true + break + } + } const childrenRenderData: NodeRenderData = hasOwnAttrs ? { ...this.renderData, props: resolved } : this.renderData if ((customComponent as StructuralComponent | undefined)?.ɵcomarkIf) { @@ -373,7 +380,13 @@ export class MarkdownNode implements OnChanges { } } else { const resolved = resolveAttributes(childProps, renderData, { parseJson: true }) - const hasOwnAttrs = Object.keys(resolved).length > 0 + let hasOwnAttrs = false + for (const key in childProps) { + if (key !== '$') { + hasOwnAttrs = true + break + } + } const childRenderData: NodeRenderData = hasOwnAttrs ? { ...renderData, props: resolved } : renderData this.renderNativeEl(parentEl, childTag, resolved, grandChildren, childRenderData, child) } diff --git a/packages/comark-angular/test/props-scope.test.ts b/packages/comark-angular/test/props-scope.test.ts new file mode 100644 index 00000000..e18799e0 --- /dev/null +++ b/packages/comark-angular/test/props-scope.test.ts @@ -0,0 +1,69 @@ +import '@angular/compiler' +import { describe, expect, it } from 'vitest' +import { Component, provideZonelessChangeDetection, type Type } from '@angular/core' +import { bootstrapApplication } from '@angular/platform-browser' +import { renderApplication } from '@angular/platform-server' +import { parseMarkdown, type MarkdownDocument as MarkdownDocumentType } from 'comark' +import { MarkdownDocument } from '../src/components/markdown-document.component.ts' + +class Probe { + text = '' +} +Component({ + selector: 'app-probe', + standalone: true, + inputs: ['text'], + template: '[{{ text }}]', +})(Probe) + +async function renderMarkdown(markdown: string): Promise { + const document = await parseMarkdown(markdown) + const components: Record> = { probe: Probe as Type } + + class App { + document: MarkdownDocumentType = document + components = components + data = { bad: 'javascript:alert(1)' } + } + Component({ + selector: 'app-root', + standalone: true, + imports: [MarkdownDocument], + template: ` + + `, + })(App) + + return renderApplication( + (context) => bootstrapApplication(App, { providers: [provideZonelessChangeDetection()] }, context), + { document: '' } + ) +} + +describe('MarkdownDocument: props scope when every binding is omitted', () => { + it('hides the grandparent props from children of an element with all attributes omitted', async () => { + const html = await renderMarkdown( + '::::wrap{label="Outer"}\n:::div{:href="data.bad"}\n::probe{:text="props.label"}\n::\n:::\n::::' + ) + expect(html).toContain('class="probe"') + expect(html).not.toContain('[Outer]') + }) + + it('exposes the resolved attributes of the middle element to its children', async () => { + const html = await renderMarkdown( + '::::wrap{label="Outer"}\n:::div{:href="data.bad" id="keep"}\n::probe{:text="props.id"}\n::\n:::\n::::' + ) + expect(html).toContain('[keep]') + expect(html).not.toContain('[Outer]') + }) + + it('keeps the parent props visible through bare wrappers', async () => { + const html = await renderMarkdown('::::wrap{label="Outer"}\n- :probe{:text="props.label"}\n::::') + expect(html).toContain('
    ') + expect(html).toContain('[Outer]') + }) +}) diff --git a/packages/comark-html/test/props-scope.test.ts b/packages/comark-html/test/props-scope.test.ts new file mode 100644 index 00000000..79384085 --- /dev/null +++ b/packages/comark-html/test/props-scope.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, it } from 'vitest' +import { parseMarkdown } from 'comark' +import { renderHtmlFromDocument } from '../src/index' + +const UNSAFE = { bad: 'javascript:alert(1)' } + +async function render(markdown: string, data: Record = UNSAFE): Promise { + return renderHtmlFromDocument(await parseMarkdown(markdown), { data }) +} + +describe('props scope when every binding is omitted', () => { + it('does not leak the parent props into children of an element whose bindings are all omitted', async () => { + const html = await render('::card{title="T"}\n:::div{:href="data.bad"}\n:span[x]{:data-x="props.title"}\n:::\n::') + expect(html).not.toContain('javascript:') + expect(html).not.toContain('data-x="T"') + }) + + it('exposes the resolved attributes of the middle element to its children', async () => { + const html = await render( + '::card{title="T"}\n:::div{:href="data.bad" id="keep"}\n:span[x]{:data-x="props.id"}\n:::\n::' + ) + expect(html).toContain('data-x="keep"') + expect(html).not.toContain('data-x="T"') + }) + + it('keeps the parent props visible through bare wrappers', async () => { + const html = await render('::card{title="T"}\n- :span[x]{:data-x="props.title"}\n::') + expect(html).toContain('
      ') + expect(html).toContain('data-x="T"') + expect(html).not.toMatch(/<(ul|li)[^>]* title=/) + }) +}) diff --git a/packages/comark-react/src/components/MarkdownDocument.tsx b/packages/comark-react/src/components/MarkdownDocument.tsx index e948f69d..23e0f4d6 100644 --- a/packages/comark-react/src/components/MarkdownDocument.tsx +++ b/packages/comark-react/src/components/MarkdownDocument.tsx @@ -176,8 +176,15 @@ function renderNode( // Only shadow the parent's `props` scope when the current element has its // own attributes. Bare wrappers (`

      `, `

        `, `
      • `, …) must keep the - // parent's scope so bindings like `{{ props.x }}` reach across them. - const hasOwnAttrs = Object.keys(resolved).length > 0 + // parent's scope so bindings like `{{ props.x }}` reach across them. The + // check uses the raw attributes: resolving can omit all of them. + let hasOwnAttrs = false + for (const key in nodeProps) { + if (key !== '$') { + hasOwnAttrs = true + break + } + } const childrenRenderData: NodeRenderData = hasOwnAttrs ? { ...renderData, props: scopeProps } : renderData // Separate template elements (slots) from regular children const slots: Record = {} diff --git a/packages/comark-react/test/props-scope.test.tsx b/packages/comark-react/test/props-scope.test.tsx new file mode 100644 index 00000000..93db405d --- /dev/null +++ b/packages/comark-react/test/props-scope.test.tsx @@ -0,0 +1,46 @@ +import { describe, expect, it } from 'vitest' +import React from 'react' +import { renderToString } from 'react-dom/server' +import { parseMarkdown } from 'comark' +import { MarkdownDocument } from '../src/components/MarkdownDocument' + +function Probe({ text = '' }: { text?: string }) { + return {`[${text}]`} +} + +async function renderMarkdown(markdown: string, props: Record = {}) { + const tree = await parseMarkdown(markdown) + return renderToString( + + ) +} + +describe('MarkdownDocument: props scope when every binding is omitted', () => { + it('hides the grandparent props from children of an element with all attributes omitted', async () => { + const html = await renderMarkdown( + '::::wrap{label="Outer"}\n:::div{:href="data.bad"}\n::probe{:text="props.label"}\n::\n:::\n::::', + { data: { bad: 'javascript:alert(1)' } } + ) + expect(html).toContain('class="probe"') + expect(html).not.toContain('[Outer]') + }) + + it('exposes the resolved attributes of the middle element to its children', async () => { + const html = await renderMarkdown( + '::::wrap{label="Outer"}\n:::div{:href="data.bad" id="keep"}\n::probe{:text="props.id"}\n::\n:::\n::::', + { data: { bad: 'javascript:alert(1)' } } + ) + expect(html).toContain('[keep]') + expect(html).not.toContain('[Outer]') + }) + + it('keeps the parent props visible through bare wrappers', async () => { + const html = await renderMarkdown('::::wrap{label="Outer"}\n- :probe{:text="props.label"}\n::::') + expect(html).toContain('
          ') + expect(html).toContain('[Outer]') + }) +}) diff --git a/packages/comark-svelte/src/components/MarkdownNode.svelte b/packages/comark-svelte/src/components/MarkdownNode.svelte index 2bc3776d..67d1a3a0 100644 --- a/packages/comark-svelte/src/components/MarkdownNode.svelte +++ b/packages/comark-svelte/src/components/MarkdownNode.svelte @@ -191,11 +191,20 @@ naturally appears inline after the deepest trailing text node. // Only shadow the parent's `props` scope when the current element has its // own attributes. Bare wrappers (`

          `, `

            `, `
          • `, …) must keep the - // parent's scope so bindings like `{{ props.x }}` reach across them. + // parent's scope so bindings like `{{ props.x }}` reach across them. The + // check uses the raw attributes: resolving can omit all of them. let nativeProps = $derived(tag ? toNativeAttributes(tag, node, mappedProps) : mappedProps) + let hasOwnAttrs = $derived.by(() => { + const attrs: Record = (Array.isArray(node) && node.length >= 2 ? node[1] : null) ?? {} + for (const key in attrs) { + if (key !== '$') return true + } + return false + }) + let childrenRenderData = $derived( - Object.keys(mappedProps).length > 0 + hasOwnAttrs ? { ...renderData, props: mappedProps } : renderData, ) diff --git a/packages/comark-svelte/test/props-scope.test.ts b/packages/comark-svelte/test/props-scope.test.ts new file mode 100644 index 00000000..3605eee0 --- /dev/null +++ b/packages/comark-svelte/test/props-scope.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from 'vitest' +import { render } from 'svelte/server' +import { parseMarkdown } from 'comark' +import MarkdownDocument from '../src/components/MarkdownDocument.svelte' +import Probe from './test-components/Probe.svelte' + +async function renderMarkdown(markdown: string, data: Record = { bad: 'javascript:alert(1)' }) { + const value = await parseMarkdown(markdown) + return render(MarkdownDocument, { props: { value, components: { probe: Probe }, data } }).body +} + +describe('MarkdownDocument: props scope when every binding is omitted', () => { + it('hides the grandparent props from children of an element with all attributes omitted', async () => { + const html = await renderMarkdown( + '::::wrap{label="Outer"}\n:::div{:href="data.bad"}\n::probe{:text="props.label"}\n::\n:::\n::::' + ) + expect(html).toContain('class="probe"') + expect(html).not.toContain('[Outer]') + }) + + it('exposes the resolved attributes of the middle element to its children', async () => { + const html = await renderMarkdown( + '::::wrap{label="Outer"}\n:::div{:href="data.bad" id="keep"}\n::probe{:text="props.id"}\n::\n:::\n::::' + ) + expect(html).toContain('[keep]') + expect(html).not.toContain('[Outer]') + }) + + it('keeps the parent props visible through bare wrappers', async () => { + const html = await renderMarkdown('::::wrap{label="Outer"}\n- :probe{:text="props.label"}\n::::') + expect(html).toContain('
              ') + expect(html).toContain('[Outer]') + }) +}) diff --git a/packages/comark-svelte/test/test-components/Probe.svelte b/packages/comark-svelte/test/test-components/Probe.svelte new file mode 100644 index 00000000..d4b3434a --- /dev/null +++ b/packages/comark-svelte/test/test-components/Probe.svelte @@ -0,0 +1,5 @@ + + +[{text}] diff --git a/packages/comark-vue/src/components/MarkdownDocument.ts b/packages/comark-vue/src/components/MarkdownDocument.ts index 1201fdd8..127cfdf0 100644 --- a/packages/comark-vue/src/components/MarkdownDocument.ts +++ b/packages/comark-vue/src/components/MarkdownDocument.ts @@ -171,8 +171,15 @@ function renderNode( // Only shadow the parent's `props` scope when the current element has its // own attributes. Bare wrappers (`

              `, `

                `, `
              • `, …) must keep the - // parent's scope so bindings like `{{ props.x }}` reach across them. - const hasOwnAttrs = Object.keys(resolved).length > 0 + // parent's scope so bindings like `{{ props.x }}` reach across them. The + // check uses the raw attributes: resolving can omit all of them. + let hasOwnAttrs = false + for (const key in nodeProps) { + if (key !== '$') { + hasOwnAttrs = true + break + } + } const childrenRenderData = hasOwnAttrs ? { ...renderData, props: scopeProps } : renderData // Separate template elements (slots) from regular children const slots: Record (VNode | string)[]> = {} diff --git a/packages/comark-vue/test/props-scope.test.ts b/packages/comark-vue/test/props-scope.test.ts new file mode 100644 index 00000000..356d364b --- /dev/null +++ b/packages/comark-vue/test/props-scope.test.ts @@ -0,0 +1,46 @@ +import { describe, expect, it } from 'vitest' +import { createSSRApp, defineComponent, h } from 'vue' +import { renderToString } from '@vue/server-renderer' +import { parseMarkdown } from 'comark' +import { MarkdownDocument } from '../src/components/MarkdownDocument' + +const Probe = defineComponent({ + props: { text: { type: String, default: '' } }, + setup(props) { + return () => h('i', { class: 'probe' }, `[${props.text}]`) + }, +}) + +async function renderMarkdown(markdown: string, props: Record = {}) { + const tree = await parseMarkdown(markdown) + const app = createSSRApp({ + setup: () => () => h(MarkdownDocument, { value: tree, components: { probe: Probe }, ...props }), + }) + return renderToString(app as any) +} + +describe('MarkdownDocument: props scope when every binding is omitted', () => { + it('hides the grandparent props from children of an element with all attributes omitted', async () => { + const html = await renderMarkdown( + '::::wrap{label="Outer"}\n:::div{:href="data.bad"}\n::probe{:text="props.label"}\n::\n:::\n::::', + { data: { bad: 'javascript:alert(1)' } } + ) + expect(html).toContain('class="probe"') + expect(html).not.toContain('[Outer]') + }) + + it('exposes the resolved attributes of the middle element to its children', async () => { + const html = await renderMarkdown( + '::::wrap{label="Outer"}\n:::div{:href="data.bad" id="keep"}\n::probe{:text="props.id"}\n::\n:::\n::::', + { data: { bad: 'javascript:alert(1)' } } + ) + expect(html).toContain('[keep]') + expect(html).not.toContain('[Outer]') + }) + + it('keeps the parent props visible through bare wrappers', async () => { + const html = await renderMarkdown('::::wrap{label="Outer"}\n- :probe{:text="props.label"}\n::::') + expect(html).toContain('
                  ') + expect(html).toContain('[Outer]') + }) +}) diff --git a/packages/comark/src/internal/stringify/state.ts b/packages/comark/src/internal/stringify/state.ts index a18612d4..1cad594e 100644 --- a/packages/comark/src/internal/stringify/state.ts +++ b/packages/comark/src/internal/stringify/state.ts @@ -58,12 +58,22 @@ export async function one(node: Node, state: State, parent?: ElementNode, atLine // element's values, regardless of which handler (html / ansi / user) runs. // Elements with no attributes (e.g. an auto-generated `

                  ` wrapper) must // NOT shadow the parent's scope, otherwise `{{ props.* }}` inside them would - // resolve to nothing. + // resolve to nothing. The check uses the raw attributes, because resolving + // can drop all of them and the parent's props must not leak back in. const prevRenderData = state.renderData if (state.renderData && node[1]) { - const resolved = resolveAttributes(node[1] as Record, prevRenderData) - if (Object.keys(resolved).length > 0) { - state.renderData = { ...prevRenderData, props: resolved } + let hasOwnAttrs = false + for (const key in node[1]) { + if (key !== '$') { + hasOwnAttrs = true + break + } + } + if (hasOwnAttrs) { + state.renderData = { + ...prevRenderData, + props: resolveAttributes(node[1] as Record, prevRenderData), + } } } From 7af700dc1a5f3959d2b1ee1bfe39b5e5743d13c0 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 19 Oct 2026 13:33:36 +0000 Subject: [PATCH 2/2] test: update bundle size snapshot --- test/bundle.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/bundle.test.ts b/test/bundle.test.ts index 52349dcc..a49eae26 100644 --- a/test/bundle.test.ts +++ b/test/bundle.test.ts @@ -60,13 +60,13 @@ describe('package bundle size', { timeout: 60_000 }, () => { expect(report).toMatchInlineSnapshot(` { - "@comark/angular": "56.2k (72 files)", + "@comark/angular": "56.5k (72 files)", "@comark/ansi": "37.3k (98 files)", "@comark/html": "16.5k (58 files)", "@comark/nuxt": "11.9k (58 files)", - "@comark/react": "38.0k (76 files)", - "@comark/svelte": "45.8k (84 files)", - "@comark/vue": "56.2k (80 files)", + "@comark/react": "38.1k (76 files)", + "@comark/svelte": "46.0k (84 files)", + "@comark/vue": "56.4k (80 files)", "comark": "385k (152 files)", } `)