fix(explorer): harden markdown viewer review findings

This commit is contained in:
Sameer6305
2026-08-19 15:37:12 +05:30
parent 5a3bdc393d
commit 7b2b2efe6b
4 changed files with 234 additions and 21 deletions
+21 -10
View File
@@ -80,6 +80,7 @@
"integrity": "sha512-QdxmAo/ikZqqRGA8s43ww8lcql6naWRvEz0FFrl6MIlc7Gi6TroXnSdWa5U/kq6fzcpqpHesicQxFZIieZbyIA==",
"dev": true,
"license": "MIT",
"peer": true,
"dependencies": {
"@babel/code-frame": "^7.29.0",
"@babel/generator": "^7.29.6",
@@ -1602,8 +1603,7 @@
"version": "2.0.46",
"resolved": "https://registry.npmjs.org/@types/hammerjs/-/hammerjs-2.0.46.tgz",
"integrity": "sha512-ynRvcq6wvqexJ9brDMS4BnBLzmr0e14d6ZJTEShTBWKymQiHwlAyGu0ZPEFI2Fh1U53F7tN9ufClWM5KvqkKOw==",
"license": "MIT",
"peer": true
"license": "MIT"
},
"node_modules/@types/hast": {
"version": "3.0.5",
@@ -1642,6 +1642,7 @@
"integrity": "sha512-A1sre26ke7HDIuY/M23nd9gfB+nrmhtYyMINbjI1zHJxYteKR6qSMX56FsmjMcDb3SMcjJg5BiRRgOCC/yBD0g==",
"devOptional": true,
"license": "MIT",
"peer": true,
"dependencies": {
"undici-types": "~7.16.0"
}
@@ -1651,6 +1652,7 @@
"resolved": "https://registry.npmjs.org/@types/react/-/react-19.2.14.tgz",
"integrity": "sha512-ilcTH/UniCkMdtexkoCN0bI7pMcJDvmQFPvuPvmEaYA/NSfFTAgdUSLAoVjaRJm7+6PvcM+q1zYOwS4wTYMF9w==",
"license": "MIT",
"peer": true,
"dependencies": {
"csstype": "^3.2.2"
}
@@ -1670,8 +1672,7 @@
"resolved": "https://registry.npmjs.org/@types/trusted-types/-/trusted-types-2.0.7.tgz",
"integrity": "sha512-ScaPdn1dQczgbl0QFTeTOmVHFULt394XJgOQNoyVhZ6r2vLnMLJfBPd53SB52T/3G36VI1/g2MZaX0cwDuXsfw==",
"license": "MIT",
"optional": true,
"peer": true
"optional": true
},
"node_modules/@types/unist": {
"version": "3.0.3",
@@ -1724,6 +1725,7 @@
"integrity": "sha512-/Zb/xaIDfxeJnvishjGdcR4jmr7S+bda8PKNhRGdljDM+elXhlvN0FyPSsMnLmJUrVG9aPO6dof80wjMawsASg==",
"dev": true,
"license": "MIT",
"peer": true,
"dependencies": {
"@typescript-eslint/scope-manager": "8.58.2",
"@typescript-eslint/types": "8.58.2",
@@ -1993,6 +1995,7 @@
"integrity": "sha512-xRQbDb9BnwDafYNn6Vwl839DYVjqXYb1XVGtWAZ1kcDc6iwAL4hg3B1dZlRiuENFeO2H53gFG3in621AdERVAg==",
"dev": true,
"license": "MIT",
"peer": true,
"bin": {
"acorn": "bin/acorn"
},
@@ -2112,6 +2115,7 @@
}
],
"license": "MIT",
"peer": true,
"dependencies": {
"baseline-browser-mapping": "^2.10.12",
"caniuse-lite": "^1.0.30001782",
@@ -2217,8 +2221,7 @@
"version": "2.20.3",
"resolved": "https://registry.npmjs.org/commander/-/commander-2.20.3.tgz",
"integrity": "sha512-GpVkmM8vF2vQUkj2LvZmD35JxeJOLCwJ9cUkugyk2nuhbv3+mJvpLYYt+0+USMxE+oj+ey/lJEnhZw75x/OMcQ==",
"license": "MIT",
"peer": true
"license": "MIT"
},
"node_modules/component-emitter": {
"version": "1.3.1",
@@ -2256,8 +2259,7 @@
"version": "0.0.10",
"resolved": "https://registry.npmjs.org/cssfilter/-/cssfilter-0.0.10.tgz",
"integrity": "sha512-FAaLDaplstoRsDR8XGYH51znUN0UY7nMc6Z9/fvE8EXGwvJE9hu7W2vHwx1+bd6gCYnln9nLbzxFTrcO9YQDZw==",
"license": "MIT",
"peer": true
"license": "MIT"
},
"node_modules/csstype": {
"version": "3.2.3",
@@ -2322,6 +2324,7 @@
"resolved": "https://registry.npmjs.org/d3-selection/-/d3-selection-3.0.0.tgz",
"integrity": "sha512-fmTRWbNMmsmWq6xJV8D19U/gw/bwrHfNXxrIN+HfZgnzqTHp9jOmKMhsTUjXOJnZOdZY9Q28y4yebKzqDKlxlQ==",
"license": "ISC",
"peer": true,
"engines": {
"node": ">=12"
}
@@ -2454,7 +2457,6 @@
"resolved": "https://registry.npmjs.org/dompurify/-/dompurify-3.4.13.tgz",
"integrity": "sha512-2vmYIoqjze2d+kakP8S/nS5shfsl587kzwEjcGlTdiksUVgFHnFCsLYDVj/JNqJVOQZGSYBTmuycv0PodwmnMQ==",
"license": "(MPL-2.0 OR Apache-2.0)",
"peer": true,
"optionalDependencies": {
"@types/trusted-types": "^2.0.7"
}
@@ -2537,6 +2539,7 @@
"integrity": "sha512-nuKKvN+oIBO0koN7Tm7dlkmnkc21mtt0QJLwAKzjLq14y6lRTdVG36MZHJ8eQHwdJMwZbQNMlPOYedMq/oVJvQ==",
"dev": true,
"license": "MIT",
"peer": true,
"workspaces": [
"packages/*"
],
@@ -3336,7 +3339,6 @@
"resolved": "https://registry.npmjs.org/marked/-/marked-14.0.0.tgz",
"integrity": "sha512-uIj4+faQ+MgHgwUW1l2PsPglZLOLOT1uErt06dAPtx2kjteLAkbsd/0FiYg/MGS+i7ZKLb7w2WClxHkzOOuryQ==",
"license": "MIT",
"peer": true,
"bin": {
"marked": "bin/marked.js"
},
@@ -4412,6 +4414,7 @@
"integrity": "sha512-QP88BAKvMam/3NxH6vj2o21R6MjxZUAd6nlwAS/pnGvN9IVLocLHxGYIzFhg6fUQ+5th6P4dv4eW9jX3DSIj7A==",
"dev": true,
"license": "MIT",
"peer": true,
"engines": {
"node": ">=12"
},
@@ -4548,6 +4551,7 @@
"resolved": "https://registry.npmjs.org/react/-/react-19.2.5.tgz",
"integrity": "sha512-llUJLzz1zTUBrskt2pwZgLq59AemifIftw4aB7JxOqf1HY2FDaGDxgwpAPVzHU1kdWabH7FauP4i1oEeer2WCA==",
"license": "MIT",
"peer": true,
"engines": {
"node": ">=0.10.0"
}
@@ -4613,6 +4617,7 @@
"resolved": "https://registry.npmjs.org/react-dom/-/react-dom-19.2.5.tgz",
"integrity": "sha512-J5bAZz+DXMMwW/wV3xzKke59Af6CHY7G4uYLN1OvBcKEsWOs4pQExj86BBKamxl/Ik5bx9whOrvBlSDfWzgSag==",
"license": "MIT",
"peer": true,
"dependencies": {
"scheduler": "^0.27.0"
},
@@ -4858,6 +4863,7 @@
"resolved": "https://registry.npmjs.org/sigma/-/sigma-3.0.2.tgz",
"integrity": "sha512-/BUbeOwPGruiBOm0YQQ6ZMcLIZ6tf/W+Jcm7dxZyAX0tK3WP9/sq7/NAWBxPIxVahdGjCJoGwej0Gdrv0DxlQQ==",
"license": "MIT",
"peer": true,
"dependencies": {
"events": "^3.3.0",
"graphology-utils": "^2.5.2"
@@ -4983,6 +4989,7 @@
"integrity": "sha512-X8EX+XV4QR5xCsrgxaED954zTDfY8KqlDtskKEL0cHhyS/P8b4IFOvGDQpsC9Q1XnLq915wEfwwY/zzskCtmhg==",
"dev": true,
"license": "MIT",
"peer": true,
"dependencies": {
"esbuild": "~0.28.0"
},
@@ -5015,6 +5022,7 @@
"integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==",
"dev": true,
"license": "Apache-2.0",
"peer": true,
"bin": {
"tsc": "bin/tsc",
"tsserver": "bin/tsserver"
@@ -5238,6 +5246,7 @@
"resolved": "https://registry.npmjs.org/vis-data/-/vis-data-8.0.3.tgz",
"integrity": "sha512-jhnb6rJNqkKR1Qmlay0VuDXY9ZlvAnYN1udsrP4U+krgZEq7C0yNSKdZqmnCe13mdnf9AdVcdDGFOzy2mpPoqw==",
"license": "(Apache-2.0 OR MIT)",
"peer": true,
"funding": {
"type": "opencollective",
"url": "https://opencollective.com/visjs"
@@ -5292,6 +5301,7 @@
"integrity": "sha512-NTKlcQjlAK7MlQoyb6LgaqHc8sso/pVyUJYWMws3jg21uTJw/LddqIFPcPqP6PzpgbIcZyKI85sFE4HBrQDA8A==",
"dev": true,
"license": "MIT",
"peer": true,
"dependencies": {
"esbuild": "^0.25.0",
"fdir": "^6.4.4",
@@ -5430,6 +5440,7 @@
"integrity": "sha512-rftlrkhHZOcjDwkGlnUtZZkvaPHCsDATp4pGpuOOMDaTdDDXF91wuVDJoWoPsKX/3YPQ5fHuF3STjcYyKr+Qhg==",
"dev": true,
"license": "MIT",
"peer": true,
"funding": {
"url": "https://github.com/sponsors/colinhacks"
}
@@ -414,13 +414,19 @@ export function GraphInspectorPanel({
</div>
) : null}
{/* Content Section */}
<details className="node-panel-collapse" open>
<summary className="node-panel-summary">Content</summary>
<div className="node-panel-body" style={{ marginTop: 8 }}>
<MarkdownContentViewer content={nodeContent} />
</div>
</details>
{/* Content Section — only rendered when the node carries actual content.
This matches the existing inspector convention: sections that have no
data for the current node are either hidden (temporal bounds) or closed
by default (Source Attribution, Properties). Always showing an open
empty panel would add noise for every relationship/predicate node. */}
{nodeContent && (
<details className="node-panel-collapse" open>
<summary className="node-panel-summary">Content</summary>
<div className="node-panel-body" style={{ marginTop: 8 }}>
<MarkdownContentViewer content={nodeContent} />
</div>
</details>
)}
{/* Actions */}
<section style={sectionStyle}>
@@ -13,6 +13,11 @@ export interface MarkdownContentViewerProps {
export function isSafeUrl(url?: string): boolean {
if (!url) return false;
const trimmed = url.trim();
// Reject whitespace-only strings — new URL("", base) would resolve to the base
// protocol and produce a false positive. This guards direct callers of the exported
// function; markdown parsers normalise whitespace-only destinations to "" which
// already fails the !url check above.
if (!trimmed) return false;
if (trimmed.startsWith("//")) return false;
if (trimmed.startsWith("#")) return true;
if (trimmed.startsWith("/")) return true;
@@ -31,8 +36,24 @@ export function MarkdownContentViewer({
}: MarkdownContentViewerProps) {
const [activeMode, setActiveMode] = useState<"preview" | "source">(defaultMode);
const [copied, setCopied] = useState(false);
// Track the content value for which the copied indicator is valid.
// When content changes (i.e. the user selects a different node), reset the
// copied indicator inline during render rather than in a useEffect — this
// avoids a cascading-render lint error and is the React-recommended pattern
// for resetting derived visual state on prop changes.
const [copiedForContent, setCopiedForContent] = useState<string | null | undefined>(content);
if (copiedForContent !== content) {
setCopiedForContent(content);
if (copied) {
// Clear the stale indicator synchronously so the new node's copy button
// never shows "Copied" from the previous selection.
setCopied(false);
}
}
const copyTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
// Clean up any outstanding timeout on unmount.
useEffect(() => {
return () => {
if (copyTimeoutRef.current) {
@@ -113,12 +134,41 @@ export function MarkdownContentViewer({
<ReactMarkdown
remarkPlugins={[remarkGfm]}
components={{
a: ({ href, children, ...props }) => {
// C-1: react-markdown passes a HAST `node` prop (the raw AST
// Element) to every custom component override via passNode:true.
// In React 19 any unknown prop spreads onto a native element are
// serialised as HTML attributes, producing node="[object Object]"
// on every rendered link. Fix: destructure `node` by name so it
// is explicitly discarded, then spread `...rest` to preserve all
// other legitimate HAST/remark-gfm attributes — e.g. the `id`,
// `aria-describedby`, `aria-label`, `data-footnote-ref`,
// `data-footnote-backref`, and `class` attrs that GFM footnotes
// require for correct in-page navigation and accessibility.
//
// C-2: fragment links (#anchor, GFM footnote backlinks) must
// navigate within the current document. External links continue
// to use target="_blank" with noopener noreferrer.
//
// eslint-disable-next-line @typescript-eslint/no-unused-vars
a: ({ href, children, title, node: _node, ...rest }) => {
if (!isSafeUrl(href)) {
return <span style={{ color: GRAPH_THEME.ui.text.muted, textDecoration: "line-through" }}>{children}</span>;
}
// isSafeUrl returning true guarantees href is a non-empty string.
const safeHref = href ?? "";
// Fragment links (#section, footnote backlinks like
// #user-content-fnref-1) are in-document anchors. Opening them
// in a new tab would break GFM footnote back-navigation.
const isFragment = safeHref.startsWith("#");
if (isFragment) {
return (
<a href={safeHref} title={title} style={linkStyle} {...rest}>
{children}
</a>
);
}
return (
<a href={href} target="_blank" rel="noopener noreferrer" style={linkStyle} {...props}>
<a href={safeHref} title={title} target="_blank" rel="noopener noreferrer" style={linkStyle} {...rest}>
{children}
<ExternalLink size={10} style={{ marginLeft: 3, verticalAlign: "middle", display: "inline" }} />
</a>
@@ -151,10 +201,12 @@ export function MarkdownContentViewer({
th: ({ children }) => <th style={{ padding: "6px 8px", textAlign: "left", fontWeight: 700, color: GRAPH_THEME.ui.text.strong, borderRight: `1px solid ${GRAPH_THEME.ui.surface.panelBorder}` }}>{children}</th>,
td: ({ children }) => <td style={{ padding: "6px 8px", color: GRAPH_THEME.ui.text.body, borderRight: `1px solid ${GRAPH_THEME.ui.surface.panelBorder}` }}>{children}</td>,
pre: ({ children }) => <pre style={preBlockStyle}>{children}</pre>,
code: ({ className: codeClass, children, ...props }) => {
// C-1: discard `node` here too — code elements are custom components
// and would otherwise receive node="[object Object]" in the DOM.
code: ({ className: codeClass, children }) => {
const isInline = !codeClass && typeof children === "string" && !children.includes("\n");
return (
<code style={isInline ? inlineCodeStyle : blockCodeStyle} {...props}>
<code style={isInline ? inlineCodeStyle : blockCodeStyle}>
{children}
</code>
);
@@ -30,6 +30,18 @@ test("isSafeUrl rejects protocol-relative URLs and dangerous schemes", () => {
assert.equal(isSafeUrl(undefined), false);
});
// ─── C URL contract: whitespace-only strings ────────────────────────────────
// The CommonMark parser normalises whitespace-only link destinations to "" so
// these values are unreachable through normal markdown rendering. However, the
// function is exported and its direct-call contract must be correct.
test("isSafeUrl rejects whitespace-only strings (contract correctness)", () => {
assert.equal(isSafeUrl(" "), false, "single space must be rejected");
assert.equal(isSafeUrl("\t"), false, "tab must be rejected");
assert.equal(isSafeUrl("\n"), false, "newline must be rejected");
assert.equal(isSafeUrl(" "), false, "multiple spaces must be rejected");
assert.equal(isSafeUrl(" \t\n "), false, "mixed whitespace must be rejected");
});
test("renders Preview mode with formatted Markdown elements and tabs", () => {
const markdown = `# Main Title\n\n**Bold Statement**\n\n* Item A\n* Item B`;
const html = renderToString(React.createElement(MarkdownContentViewer, { content: markdown, defaultMode: "preview" }));
@@ -69,6 +81,118 @@ test("renders raw HTML safely as escaped text without executing elements", () =>
assert.equal(html.includes("&lt;script&gt;"), true);
});
// ─── C-1: HAST node prop must not reach the DOM ─────────────────────────────
// react-markdown passes a HAST `node` (Element) object to custom component
// overrides. Before this fix, ...props spread caused React 19 to serialise it
// as node="[object Object]" on every <a> and <code> element.
test("rendered links do not expose the HAST node object as a DOM attribute", () => {
const content = `[Example](https://example.com)\n\nInline \`code\` here.`;
const html = renderToString(React.createElement(MarkdownContentViewer, { content, defaultMode: "preview" }));
// The rendered HTML must not contain the serialised HAST object
assert.equal(html.includes("node="), false, "node= attribute must not appear in rendered HTML");
assert.equal(html.includes("[object Object]"), false, "serialised HAST object must not appear in rendered HTML");
// The link must still render correctly with the right href
assert.equal(html.includes('href="https://example.com"'), true, "href must be present");
});
// ─── C-2: Fragment links must not open in a new tab ─────────────────────────
// Links to in-document anchors such as #section or GFM footnote backlinks like
// #user-content-fn-1 must stay in the current document. Only external links
// use target="_blank".
test("fragment links render in the current document without target blank", () => {
const content = `[Jump to section](#introduction)\n\n[External](https://example.com)`;
const html = renderToString(React.createElement(MarkdownContentViewer, { content, defaultMode: "preview" }));
// Fragment link must have the href
assert.equal(html.includes('href="#introduction"'), true, "fragment href must be present");
// Confirm no target=_blank attribute appears anywhere near the fragment link.
// We check that the output contains a fragment href WITHOUT target="_blank"
// by verifying the two strings are not both present (the external link has
// target blank; the fragment link must not).
const fragmentLinkIdx = html.indexOf('href="#introduction"');
assert.notEqual(fragmentLinkIdx, -1, "fragment link must be rendered");
// Inspect the 80 chars around the fragment href — should not contain target
const fragmentContext = html.slice(Math.max(0, fragmentLinkIdx - 10), fragmentLinkIdx + 90);
assert.equal(fragmentContext.includes('target="_blank"'), false, "fragment link must not have target=_blank");
// External link must still have target blank
assert.equal(html.includes('href="https://example.com"'), true, "external href must be present");
assert.equal(html.includes('target="_blank"'), true, "external link must have target=_blank");
assert.equal(html.includes('rel="noopener noreferrer"'), true, "external link must have rel");
});
test("GFM footnote backlinks render without target blank", () => {
// GFM footnote syntax: footnote ref in text + definition below
const content = `See the note[^1] for more.\n\n[^1]: This is the footnote text.`;
const html = renderToString(React.createElement(MarkdownContentViewer, { content, defaultMode: "preview" }));
// The footnote reference link (#user-content-fn-1) and backlink
// (#user-content-fnref-1) are fragment links and must not open in a new tab.
// We verify no fragment href is paired with target=_blank.
// Extract all href="#..." occurrences and confirm none is adjacent to target=_blank.
const anchorMatches = [...html.matchAll(/href="#[^"]*"/g)];
assert.ok(anchorMatches.length > 0, "GFM footnotes must produce fragment links");
for (const match of anchorMatches) {
const start = match.index ?? 0;
const context = html.slice(Math.max(0, start - 10), start + 120);
assert.equal(
context.includes('target="_blank"'),
false,
`fragment link ${match[0]} must not have target=_blank`,
);
}
});
// ─── C-1-R: GFM footnote attributes must be preserved (regression test) ─────
// The C-1 fix (removing the HAST `node` prop) must NOT silently drop other
// legitimate HAST attributes. remark-gfm generates the following on footnote
// links that are required for correct in-page navigation and accessibility:
//
// Footnote reference anchor:
// id="user-content-fnref-1" ← backlink target
// data-footnote-ref="true"
// aria-describedby="footnote-label"
//
// Footnote back-link anchor:
// data-footnote-backref=""
// aria-label="Back to reference 1" ← screen-reader label
// class="data-footnote-backref"
//
// If these are absent, clicking the ↩ back-link cannot scroll back to the
// in-text reference, and screen readers cannot announce the backlink purpose.
test("GFM footnote links preserve generated id, aria, and class attributes", () => {
const content = `See the note[^1] for more.\n\n[^1]: This is the footnote text.`;
const html = renderToString(React.createElement(MarkdownContentViewer, { content, defaultMode: "preview" }));
// The HAST `node` object must not appear serialised as a DOM attribute.
assert.equal(html.includes("node="), false, "node= attribute must not appear in HTML");
assert.equal(html.includes("[object Object]"), false, "serialised HAST object must not appear in HTML");
// Footnote reference anchor must retain its id so the backlink can navigate to it.
assert.equal(
html.includes('id="user-content-fnref-1"'),
true,
"footnote reference anchor must retain id for back-navigation",
);
// Footnote backlink must retain its aria-label for screen-reader accessibility.
assert.equal(
html.includes('aria-label="Back to reference 1"'),
true,
"footnote backlink must retain aria-label for accessibility",
);
// Footnote backlink must retain its class attribute.
assert.equal(
html.includes('class="data-footnote-backref"'),
true,
"footnote backlink must retain class attribute",
);
});
test("renders safe links as <a> with target blank and unclickable span for unsafe links", () => {
const content = `[Safe Link](https://getsemantica.ai)\n\n[Unsafe Scheme](javascript:alert(1))\n\n[Protocol Relative](//evil.com)`;
const html = renderToString(React.createElement(MarkdownContentViewer, { content, defaultMode: "preview" }));
@@ -118,3 +242,23 @@ test("handles very large Markdown content without failure", () => {
const html = renderToString(React.createElement(MarkdownContentViewer, { content: largeContent, defaultMode: "preview" }));
assert.equal(html.includes("Large Knowledge Node"), true);
});
// ─── H-2: Stale copied state lifecycle (SSR-compatible portion) ─────────────
// Full state-transition testing (Node A → copy → Node B) requires an interactive
// framework. The lifecycle correctness is guaranteed by the render-phase
// previous-prop synchronisation pattern: a `copiedForContent` state value tracks
// the content for which the copied indicator was set; when `content` changes, the
// mismatch is detected during render and `copied` is reset to false in the same
// React batch, before the new node's UI is painted. What we CAN verify in SSR
// is that the initial render for any content value shows the Copy button (not the
// Copied indicator), which confirms the initial state is always clean.
test("copy button always starts in un-copied state on initial render", () => {
const html = renderToString(React.createElement(MarkdownContentViewer, {
content: "# Some Node\n\nDescription text.",
defaultMode: "preview",
}));
// Initial render must show 'Copy', never 'Copied'
assert.equal(html.includes("Copy"), true, "Copy button must be present on initial render");
assert.equal(html.includes("Copied"), false, "Copied indicator must NOT be present on initial render");
});