From b67f1a96065bd801a817c9bb25f3b6b73505cc4a Mon Sep 17 00:00:00 2001 From: tommy230 Date: Mon, 28 Sep 2026 19:26:23 +0000 Subject: [PATCH] fix(publisher): emit one merged rel on links and buttons instead of two base.link and base.button emitted the author's rel from the htmlAttributes bag and their own security rel as two separate attributes. Browsers keep the first, so a nofollow new-tab link published without noopener noreferrer, and the canvas showed a different rel than the page. The HTML importer also dropped every source rel on anchors as if the module regenerated it. The two anchor modules now merge the authored tokens with the security rel through one shared helper on both render paths, and the importer keeps rel. Co-Authored-By: Claude Opus 5.5 --- docs/features/html-import.md | 2 +- docs/features/modules.md | 2 +- ...base-modules-shared-render.editor.test.tsx | 17 +++++++ .../base-modules-shared-render.test.ts | 48 ++++++++++++++++++- src/__tests__/htmlImport/mapping.test.ts | 22 ++++++++- src/core/htmlImport/walkAndMap.ts | 9 +++- src/modules/base/button/ButtonEditor.tsx | 10 ++-- src/modules/base/button/index.ts | 11 +++-- src/modules/base/link/LinkEditor.tsx | 8 ++-- src/modules/base/link/index.ts | 10 ++-- src/modules/base/shared/anchorTarget.ts | 41 ++++++++++++++++ 11 files changed, 155 insertions(+), 25 deletions(-) diff --git a/docs/features/html-import.md b/docs/features/html-import.md index 7cb7bf493..6dc7c17a4 100644 --- a/docs/features/html-import.md +++ b/docs/features/html-import.md @@ -158,7 +158,7 @@ The importer is "approximate by construction". Several inputs do not survive the | Input | What happens | Why | |---|---|---| | `alt=""` on `` | Reported in `imageAlts`, not stored on the node | `base.image` has no `alt` prop — alt text lives on the media library asset, which Site Import creates with the authored value | -| Safe HTML attributes not modeled by the matched module (`id`, ARIA attrs, `role`, custom attrs, `data-*`, etc.) | Preserved in `props.htmlAttributes` on base container/text/link/button/image nodes and editable in the Properties panel Attributes view. `class` names become registry classes, inline `style` declarations become `node.inlineStyles`, event handlers are stripped, reserved editor/runtime `data-*` names are not imported, and attributes already owned by the module (for example `href` on links and `src` on images) stay in their first-class module props. | The module schema owns modeled props; `htmlAttributes` is the safe escape hatch for extra authored attributes | +| Safe HTML attributes not modeled by the matched module (`id`, ARIA attrs, `role`, custom attrs, `data-*`, etc.) | Preserved in `props.htmlAttributes` on base container/text/link/button/image nodes and editable in the Properties panel Attributes view. `class` names become registry classes, inline `style` declarations become `node.inlineStyles`, event handlers are stripped, reserved editor/runtime `data-*` names are not imported, and attributes already owned by the module (for example `href` on links and `src` on images) stay in their first-class module props. An authored `rel` on a link or button anchor is kept in the bag; at render time the module merges it with the `noopener noreferrer` it adds for `target="_blank"` and emits one `rel` attribute. | The module schema owns modeled props; `htmlAttributes` is the safe escape hatch for extra authored attributes | | Exact inline whitespace around mixed content (`
Hello world
`) | Approximated | Each text run becomes a `base.text` child with `tag: 'none'` and whitespace collapsed to single spaces. True parent-edge indentation is trimmed, but a single boundary space is preserved around element siblings so `Hello world` does not become `Helloworld`. The text itself is **preserved** and publishes without an extra wrapper. | | Whitespace-only text (newlines/indentation between tags) | Dropped in normal flow; preserved verbatim inside `
` | Normal-flow indentation carries no rendered content, while `
` whitespace is content and must retain its literal DOM text-node shape for CSS and runtime scripts. |
 | Void elements (`
`, `
`, etc.) | Imported as a childless `base.container` node with `tag:'custom'` and the real tag name as `customTag`. No children, no empty-container placeholder. `` imports as a form primitive instead. | React throws if children are rendered inside void element tags; the dedicated void-element rule (before the catch-all) sets `recurse:false` and the canvas renderer skips children entirely for void tags. | diff --git a/docs/features/modules.md b/docs/features/modules.md index b61509d49..9b59b0099 100644 --- a/docs/features/modules.md +++ b/docs/features/modules.md @@ -51,7 +51,7 @@ src/modules/base/ ├── slotOutlet/ — base.slot-outlet (VC author side) ├── slotInstance/ — base.slot-instance (VC consumer side) ├── shared/ -│ └── anchorTarget.ts — AnchorTargetSchema, ANCHOR_TARGET_OPTIONS, anchorRel() (button + link) +│ └── anchorTarget.ts — AnchorTargetSchema, ANCHOR_TARGET_OPTIONS, anchorRel(), mergeAnchorRel(), anchorHtmlAttributes() (button + link) ├── utils/ │ ├── escape.ts — escapeHtml, safeUrl, sanitiseCssValue, buildStyle (re-exports publisher utils) │ ├── htmlTag.ts — htmlTagControl, customHtmlTagControl (resolution lives in @core/htmlAttributes) diff --git a/src/__tests__/base-modules-shared-render.editor.test.tsx b/src/__tests__/base-modules-shared-render.editor.test.tsx index 90983f91a..47a3187bd 100644 --- a/src/__tests__/base-modules-shared-render.editor.test.tsx +++ b/src/__tests__/base-modules-shared-render.editor.test.tsx @@ -47,6 +47,16 @@ describe('LinkEditor canvas DOM matches the shared helpers', () => { expect(self.container.querySelector('a')?.getAttribute('rel')).toBe(null) }) + it('merges an authored rel with the security rel exactly as the publisher does', () => { + const blank = renderEditor(LinkModule, { href: 'https://e.com', target: '_blank', htmlAttributes: { rel: 'nofollow' } }) + const published = LinkModule.render({ ...LinkModule.defaults, href: 'https://e.com', target: '_blank', htmlAttributes: { rel: 'nofollow' } }, []).html + expect(blank.container.querySelector('a')?.getAttribute('rel')).toBe('nofollow noopener noreferrer') + expect(published).toContain('rel="nofollow noopener noreferrer"') + + const self = renderEditor(LinkModule, { href: '/page/2/', target: '_self', htmlAttributes: { rel: 'next' } }) + expect(self.container.querySelector('a')?.getAttribute('rel')).toBe('next') + }) + it('renders children when present, falls back to text when empty (== linkUsesChildren)', () => { const withChildren = renderEditor( LinkModule, @@ -75,6 +85,13 @@ describe('ButtonEditor canvas DOM matches resolveButtonAnchor', () => { const blank = renderEditor(ButtonModule, { href: 'https://e.com', target: '_blank', label: 'Go' }) expect(blank.container.querySelector('a')?.getAttribute('rel')).toBe(anchorRel('_blank')) }) + + it('merges an authored rel with the security rel exactly as the publisher does', () => { + const blank = renderEditor(ButtonModule, { href: 'https://e.com', target: '_blank', label: 'Go', htmlAttributes: { rel: 'sponsored' } }) + const published = ButtonModule.render({ ...ButtonModule.defaults, href: 'https://e.com', target: '_blank', label: 'Go', htmlAttributes: { rel: 'sponsored' } }, []).html + expect(blank.container.querySelector('a')?.getAttribute('rel')).toBe('sponsored noopener noreferrer') + expect(published).toContain('rel="sponsored noopener noreferrer"') + }) }) describe('ListEditor canvas DOM matches parseItems', () => { diff --git a/src/__tests__/base-modules-shared-render.test.ts b/src/__tests__/base-modules-shared-render.test.ts index 0719ea3f8..abd6217ee 100644 --- a/src/__tests__/base-modules-shared-render.test.ts +++ b/src/__tests__/base-modules-shared-render.test.ts @@ -25,7 +25,7 @@ import { ButtonModule } from '@modules/base/button' import { ListModule } from '@modules/base/list' import { VideoModule } from '@modules/base/video' -import { anchorRel, ANCHOR_TARGET_OPTIONS } from '@modules/base/shared/anchorTarget' +import { anchorRel, mergeAnchorRel, ANCHOR_TARGET_OPTIONS } from '@modules/base/shared/anchorTarget' import { linkUsesChildren } from '@modules/base/link/content' import { resolveButtonAnchor } from '@modules/base/button/anchor' import { parseItems } from '@modules/base/list/items' @@ -62,6 +62,52 @@ describe('anchorRel — single source for the noopener rule', () => { }) }) +describe('mergeAnchorRel — one rel attribute, authored tokens plus the security rel', () => { + it('keeps authored tokens, always adds the _blank guard, and emits each token once', () => { + expect(mergeAnchorRel(undefined, '_self')).toBeNull() + expect(mergeAnchorRel('', '_self')).toBeNull() + expect(mergeAnchorRel('nofollow', '_self')).toBe('nofollow') + expect(mergeAnchorRel(' next prev ', '_parent')).toBe('next prev') + expect(mergeAnchorRel(undefined, '_blank')).toBe(anchorRel('_blank')) + expect(mergeAnchorRel('nofollow', '_blank')).toBe('nofollow noopener noreferrer') + expect(mergeAnchorRel('noopener', '_blank')).toBe('noopener noreferrer') + expect(mergeAnchorRel('noreferrer sponsored', '_blank')).toBe('noreferrer sponsored noopener') + expect(mergeAnchorRel(42, '_blank')).toBe('noopener noreferrer') + }) + + const relAttributes = (html: string): string[] => [...html.matchAll(/\brel="([^"]*)"/g)].map((m) => m[1]!) + + it('link and button render() emit exactly one rel, keeping the authored tokens', () => { + const cases = [ + LinkModule.render({ ...LinkModule.defaults, href: '/page/2/', target: '_self', htmlAttributes: { rel: 'next' } }, []).html, + ButtonModule.render({ ...ButtonModule.defaults, href: '/page/2/', target: '_self', htmlAttributes: { rel: 'next' } }, []).html, + ] + for (const html of cases) expect(relAttributes(html)).toEqual(['next']) + }) + + it('an authored rel never removes the noopener guard on a new-tab link or button', () => { + // Two rel attributes on one element is a parse error: the browser keeps + // the first. Before the merge the author's rel was emitted before the + // module's, so a nofollow new-tab link published without noopener. + const link = LinkModule.render({ ...LinkModule.defaults, href: 'https://e.com', target: '_blank', htmlAttributes: { rel: 'nofollow' } }, []).html + const button = ButtonModule.render({ ...ButtonModule.defaults, href: 'https://e.com', target: '_blank', htmlAttributes: { rel: 'nofollow' } }, []).html + for (const html of [link, button]) { + expect(relAttributes(html)).toEqual(['nofollow noopener noreferrer']) + } + const authoredGuard = LinkModule.render({ ...LinkModule.defaults, href: 'https://e.com', target: '_blank', htmlAttributes: { rel: 'noopener noreferrer' } }, []).html + expect(relAttributes(authoredGuard)).toEqual(['noopener noreferrer']) + }) + + it('escapes the authored rel and leaves the other bag attributes alone', () => { + const html = LinkModule.render( + { ...LinkModule.defaults, href: 'https://e.com', target: '_self', htmlAttributes: { rel: 'a"b', 'data-track': 'x' } }, + [], + ).html + expect(html).toContain('rel="a"b"') + expect(html).toContain('data-track="x"') + }) +}) + describe('linkUsesChildren — children-vs-text fallback', () => { it('treats an empty children collection as "use text", not "use empty children"', () => { expect(linkUsesChildren(0)).toBe(false) diff --git a/src/__tests__/htmlImport/mapping.test.ts b/src/__tests__/htmlImport/mapping.test.ts index 85ab5ada3..fbe8987d4 100644 --- a/src/__tests__/htmlImport/mapping.test.ts +++ b/src/__tests__/htmlImport/mapping.test.ts @@ -1032,10 +1032,30 @@ describe('HTML attribute preservation — props.htmlAttributes for ordinary base const children = container.children.map((id) => result.nodes[id]!) expect(children[0]!.moduleId).toBe('base.link') - expect(children[0]!.props.htmlAttributes).toEqual({ 'data-track': 'jump' }) + expect(children[0]!.props.htmlAttributes).toEqual({ rel: 'nofollow', 'data-track': 'jump' }) expect(children[1]!.moduleId).toBe('base.image') expect(children[1]!.props.htmlAttributes).toEqual({ 'data-lazy': 'logo' }) }) + + it('keeps an authored rel on links and button anchors', () => { + // The modules regenerate only the security rel for target="_blank", so a + // source rel (nofollow, next/prev, sponsored) has no other home. A theme + // styling `a[rel="next"]` needs it to survive import. + const result = imported(` + + `) + + const nav = result.nodes[result.rootIds[0]!]! + const children = nav.children.map((id) => result.nodes[id]!) + expect(children[0]!.moduleId).toBe('base.link') + expect(children[0]!.props.htmlAttributes).toEqual({ rel: 'next' }) + expect(children[1]!.moduleId).toBe('base.button') + expect(children[1]!.props.target).toBe('_blank') + expect(children[1]!.props.htmlAttributes).toEqual({ rel: 'sponsored nofollow' }) + }) }) // --------------------------------------------------------------------------- diff --git a/src/core/htmlImport/walkAndMap.ts b/src/core/htmlImport/walkAndMap.ts index 7e0ff7be2..d164881f2 100644 --- a/src/core/htmlImport/walkAndMap.ts +++ b/src/core/htmlImport/walkAndMap.ts @@ -123,8 +123,13 @@ const HTML_ATTRIBUTE_MODULES = new Set([ 'base.form', ]) +// `rel` is deliberately absent from the anchor modules: base.link and +// base.button only ever generate the security rel for `target="_blank"`, so a +// source-declared `rel` (nofollow, sponsored, next/prev, …) has no other home +// and would be lost outright. It stays in the bag and the modules merge it +// with the security rel at render time (`anchorHtmlAttributes`). const MODULE_GENERATED_ATTRIBUTE_NAMES: Record = { - 'base.button': ['aria-disabled', 'disabled', 'href', 'rel', 'target', 'type'], + 'base.button': ['aria-disabled', 'disabled', 'href', 'target', 'type'], 'base.form': [ 'action', 'data-instatic-form-id', @@ -146,7 +151,7 @@ const MODULE_GENERATED_ATTRIBUTE_NAMES: Record = { 'style', 'width', ], - 'base.link': ['href', 'rel', 'target'], + 'base.link': ['href', 'target'], } /** diff --git a/src/modules/base/button/ButtonEditor.tsx b/src/modules/base/button/ButtonEditor.tsx index 9c6562686..08e32138a 100644 --- a/src/modules/base/button/ButtonEditor.tsx +++ b/src/modules/base/button/ButtonEditor.tsx @@ -10,7 +10,7 @@ */ import React from 'react' import type { ModuleComponentProps } from '@core/module-engine' -import { anchorRel } from '@modules/base/shared/anchorTarget' +import { anchorHtmlAttributes } from '@modules/base/shared/anchorTarget' import { htmlAttributesForReact } from '@core/htmlAttributes' import { inlineEditableElementProps } from '@modules/base/shared/inlineText' import { resolveButtonAnchor } from './anchor' @@ -23,19 +23,19 @@ export const ButtonEditor: React.FC> = ( inlineEdit, }) => { const label = props.label || 'Button' - const htmlAttrs = htmlAttributesForReact(props.htmlAttributes) const anchor = resolveButtonAnchor(props.href) // React.createElement (not JSX) so the editable element's generic // `Ref` is accepted — matching TextEditor / LinkEditor. if (anchor) { + const { attributes, rel } = anchorHtmlAttributes(props.htmlAttributes, props.target) return React.createElement( 'a', { ...nodeWrapperProps, - ...htmlAttrs, + ...attributes, href: anchor.href, target: props.target, - rel: anchorRel(props.target) ?? undefined, + rel: rel ?? undefined, className: mcClassName, ...(inlineEdit ? inlineEditableElementProps(inlineEdit) : {}), }, @@ -46,7 +46,7 @@ export const ButtonEditor: React.FC> = ( 'button', { ...nodeWrapperProps, - ...htmlAttrs, + ...htmlAttributesForReact(props.htmlAttributes), type: 'button', className: mcClassName, // A disabled button can't be focused/edited — never disable while editing. diff --git a/src/modules/base/button/index.ts b/src/modules/base/button/index.ts index 19c28b5b6..761e79c33 100644 --- a/src/modules/base/button/index.ts +++ b/src/modules/base/button/index.ts @@ -9,11 +9,12 @@ import type { ModuleDefinition } from '@core/module-engine' import { registry } from '@core/module-engine' import { CursorClickSolidIcon } from 'pixel-art-icons/icons/cursor-click-solid' import { Value } from '@core/utils/typeboxHelpers' -import { ANCHOR_TARGET_OPTIONS, anchorRel } from '@modules/base/shared/anchorTarget' +import { ANCHOR_TARGET_OPTIONS, anchorHtmlAttributes } from '@modules/base/shared/anchorTarget' import { htmlAttributesControl, } from '@modules/base/shared/htmlAttributes' import { htmlAttributesAttr } from '@core/publisher' +import { escapeHtml } from '@modules/base/utils/escape' import { resolveButtonAnchor } from './anchor' import { ButtonEditor } from './ButtonEditor' import { ButtonPropsSchema, type ButtonStoredProps } from './props' @@ -64,13 +65,13 @@ export const ButtonModule: ModuleDefinition = { render: (props) => { const label = String(props.label ?? '') - const attrs = htmlAttributesAttr(props.htmlAttributes) const anchor = resolveButtonAnchor(props.href) if (anchor) { - const rel = anchorRel(props.target) - const relAttr = rel ? ` rel="${rel}"` : '' - return { html: `${label}` } + const { attributes, rel } = anchorHtmlAttributes(props.htmlAttributes, props.target) + const relAttr = rel ? ` rel="${escapeHtml(rel)}"` : '' + return { html: `${label}` } } + const attrs = htmlAttributesAttr(props.htmlAttributes) const disabledAttr = props.disabled ? ' disabled aria-disabled="true"' : '' const buttonType = props.buttonType === 'reset' ? 'reset' : 'button' return { html: `${label}` } diff --git a/src/modules/base/link/LinkEditor.tsx b/src/modules/base/link/LinkEditor.tsx index 2c1ccaf90..83b26c67b 100644 --- a/src/modules/base/link/LinkEditor.tsx +++ b/src/modules/base/link/LinkEditor.tsx @@ -9,8 +9,7 @@ */ import React from 'react' import type { ModuleComponentProps } from '@core/module-engine' -import { anchorRel } from '@modules/base/shared/anchorTarget' -import { htmlAttributesForReact } from '@core/htmlAttributes' +import { anchorHtmlAttributes } from '@modules/base/shared/anchorTarget' import { inlineEditableElementProps } from '@modules/base/shared/inlineText' import { linkUsesChildren } from './content' import type { LinkStoredProps } from './props' @@ -20,14 +19,15 @@ export const LinkEditor: React.FC> = ({ pr // Inline editing only starts on a childless link (text mode), so when // `inlineEdit` is set the element edits its `text` prop in place. const content = linkUsesChildren(childCount) ? children : (props.text ?? 'Link text') + const { attributes, rel } = anchorHtmlAttributes(props.htmlAttributes, props.target) return React.createElement( 'a', { ...nodeWrapperProps, - ...htmlAttributesForReact(props.htmlAttributes), + ...attributes, href: props.href || '#', target: props.target, - rel: anchorRel(props.target) ?? undefined, + rel: rel ?? undefined, className: mcClassName, ...(inlineEdit ? inlineEditableElementProps(inlineEdit) : {}), }, diff --git a/src/modules/base/link/index.ts b/src/modules/base/link/index.ts index a79285328..9c9a1fde7 100644 --- a/src/modules/base/link/index.ts +++ b/src/modules/base/link/index.ts @@ -7,9 +7,9 @@ import type { ModuleDefinition } from '@core/module-engine' import { registry } from '@core/module-engine' import { LinkIcon } from 'pixel-art-icons/icons/link' -import { safeUrl } from '@modules/base/utils/escape' +import { escapeHtml, safeUrl } from '@modules/base/utils/escape' import { Value } from '@core/utils/typeboxHelpers' -import { ANCHOR_TARGET_OPTIONS, anchorRel } from '@modules/base/shared/anchorTarget' +import { ANCHOR_TARGET_OPTIONS, anchorHtmlAttributes } from '@modules/base/shared/anchorTarget' import { htmlAttributesControl, } from '@modules/base/shared/htmlAttributes' @@ -52,9 +52,9 @@ export const LinkModule: ModuleDefinition = { render: (props, renderedChildren) => { const href = safeUrl(props.href) - const attrs = htmlAttributesAttr(props.htmlAttributes) - const rel = anchorRel(props.target) - const relAttr = rel ? ` rel="${rel}"` : '' + const { attributes, rel } = anchorHtmlAttributes(props.htmlAttributes, props.target) + const attrs = htmlAttributesAttr(attributes) + const relAttr = rel ? ` rel="${escapeHtml(rel)}"` : '' const targetAttr = ` target="${String(props.target)}"` const content = linkUsesChildren(renderedChildren.length) ? renderedChildren.join('') diff --git a/src/modules/base/shared/anchorTarget.ts b/src/modules/base/shared/anchorTarget.ts index 622634693..b5a305b9e 100644 --- a/src/modules/base/shared/anchorTarget.ts +++ b/src/modules/base/shared/anchorTarget.ts @@ -12,11 +12,16 @@ * - `AnchorTargetSchema` / `AnchorTarget` — the persisted prop shape. * - `ANCHOR_TARGET_OPTIONS` — the Properties-panel select. * - `anchorRel(target)` — the single rel decision. + * - `mergeAnchorRel(authored, target)` — that decision merged with the + * author's own `rel` tokens. + * - `anchorHtmlAttributes(bag, target)` — the `htmlAttributes` bag with + * `rel` pulled out and merged. * * Lives in a non-component `.ts` so the editor components can import it without * breaking React Fast Refresh (Constraint #309). */ import { Type, type Static } from '@core/utils/typeboxHelpers' +import { normalizeHtmlAttributes } from '@core/htmlAttributes' export const AnchorTargetSchema = Type.Union( [Type.Literal('_self'), Type.Literal('_blank'), Type.Literal('_parent')], @@ -43,3 +48,39 @@ export const ANCHOR_TARGET_OPTIONS: ReadonlyArray<{ label: string; value: Anchor export function anchorRel(target: AnchorTarget): string | null { return target === '_blank' ? 'noopener noreferrer' : null } + +/** + * The single `rel` an anchor emits when the author also wrote one. Authors put + * `rel` in the `htmlAttributes` bag (the Properties-panel Attributes view, or + * an imported ``), and the module adds the security rel for + * its `target`. Emitting both as separate attributes is a duplicate-attribute + * parse error: the browser keeps the first one, so `rel="nofollow"` placed + * before the module's `rel="noopener noreferrer"` silently loses the + * reverse-tabnabbing guard on a new-tab link. This merges the two into one + * space-separated token list — the author's tokens first, then whatever + * `anchorRel(target)` requires, each token once — or `null` when there is + * nothing to emit. A `_blank` anchor therefore always carries + * `noopener noreferrer`, whatever the author wrote. + */ +export function mergeAnchorRel(authoredRel: unknown, target: AnchorTarget): string | null { + const tokens = new Set() + if (typeof authoredRel === 'string') { + for (const token of authoredRel.split(/\s+/)) if (token) tokens.add(token) + } + for (const token of anchorRel(target)?.split(' ') ?? []) tokens.add(token) + return tokens.size > 0 ? [...tokens].join(' ') : null +} + +/** + * Split an anchor's `htmlAttributes` bag into the attributes to spread as-is + * and the one merged `rel` (see `mergeAnchorRel`). Both the publisher + * `render()` and the canvas editor of `base.link` / `base.button` go through + * this so neither path can emit `rel` twice. + */ +export function anchorHtmlAttributes( + htmlAttributes: unknown, + target: AnchorTarget, +): { attributes: Record; rel: string | null } { + const { rel: authoredRel, ...attributes } = normalizeHtmlAttributes(htmlAttributes) + return { attributes, rel: mergeAnchorRel(authoredRel, target) } +}