-
Notifications
You must be signed in to change notification settings - Fork 332
fix(cli): don't skip image and link substitution after a literal < in markdown #17315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| - summary: | | ||
| Fix local images and links being published as build-machine filesystem paths (e.g. | ||
| `/home/runner/work/.../assets/image.png`) when an earlier line on the page contains a literal | ||
| `<` in prose or inline code. The markdown scanner treated any `<` as the start of a tag and | ||
| skipped ahead to the next `>`, dropping every image and link substitution in between. | ||
| type: fix |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,30 @@ interface ImageOccurrence { | |
| type: "markdown-image" | "markdown-link" | "jsx-src" | "jsx-href"; | ||
| } | ||
|
|
||
| const JSX_TAG_NAME_START_REGEX = /[A-Za-z]/; | ||
|
|
||
| /** | ||
| * A `<` only opens a tag when a tag name (or `/`) follows it and it isn't escaped. Comparisons in | ||
| * prose such as `a < b`, `<=`, or `\<` are literal text: scanning them as tags makes the scan run | ||
| * to the next `>` anywhere in the page, silently skipping every image and link in between. | ||
| */ | ||
| function isJsxTagStart(content: string, index: number): boolean { | ||
| if (content[index] !== "<" || content[index - 1] === "\\") { | ||
| return false; | ||
| } | ||
| const nameStart = content[index + 1] === "/" ? content[index + 2] : content[index + 1]; | ||
| return nameStart != null && JSX_TAG_NAME_START_REGEX.test(nameStart); | ||
| } | ||
|
|
||
| /** | ||
| * Tags never span a blank line, so the scan is bounded there. Without a bound, an unterminated `<` | ||
| * consumes the remainder of the page. | ||
| */ | ||
| function findJsxTagScanLimit(content: string, start: number): number { | ||
| const blankLine = content.indexOf("\n\n", start); | ||
| return blankLine === -1 ? content.length : blankLine; | ||
| } | ||
|
|
||
| function streamingScanForImages( | ||
| content: string, | ||
| metadata: AbsolutePathMetadata | ||
|
|
@@ -273,36 +297,43 @@ function parseJsxTag( | |
| filepaths: Set<AbsoluteFilePath>, | ||
| edits: Edit[] | ||
| ): { nextIndex: number } | null { | ||
| if (!isJsxTagStart(content, start)) { | ||
| return null; | ||
| } | ||
|
|
||
| let i = start + 1; | ||
| const len = content.length; | ||
| const limit = findJsxTagScanLimit(content, start); | ||
| // Buffered so a `<` that turns out not to be a tag leaves no edits behind. | ||
| const tagFilepaths: AbsoluteFilePath[] = []; | ||
| const tagEdits: Edit[] = []; | ||
|
|
||
| while (i < len && content[i] !== ">" && content[i] !== " " && content[i] !== "\n") { | ||
| while (i < limit && content[i] !== ">" && content[i] !== " " && content[i] !== "\n") { | ||
| i++; | ||
| } | ||
|
|
||
| while (i < len && content[i] !== ">") { | ||
| while (i < len && (content[i] === " " || content[i] === "\n")) { | ||
| while (i < limit && content[i] !== ">") { | ||
| while (i < limit && (content[i] === " " || content[i] === "\n")) { | ||
| i++; | ||
| } | ||
|
|
||
| const attrStart = i; | ||
| while (i < len && content[i] !== "=" && content[i] !== ">" && content[i] !== " " && content[i] !== "\n") { | ||
| while (i < limit && content[i] !== "=" && content[i] !== ">" && content[i] !== " " && content[i] !== "\n") { | ||
| i++; | ||
| } | ||
|
|
||
| const attrName = content.slice(attrStart, i).trim(); | ||
|
|
||
| if (content[i] === "=") { | ||
| i++; | ||
| while (i < len && (content[i] === " " || content[i] === "\n")) { | ||
| while (i < limit && (content[i] === " " || content[i] === "\n")) { | ||
| i++; | ||
| } | ||
|
|
||
| if (content[i] === '"' || content[i] === "'") { | ||
| const quote = content[i]; | ||
| i++; | ||
| const valueStart = i; | ||
| while (i < len && content[i] !== quote) { | ||
| while (i < limit && content[i] !== quote) { | ||
| if (content[i] === "\\") { | ||
| i += 2; | ||
| } else { | ||
|
|
@@ -316,15 +347,15 @@ function parseJsxTag( | |
| const src = trimAnchor(value); | ||
| const resolvedPath = resolvePath(src, metadata); | ||
| if (src && resolvedPath) { | ||
| filepaths.add(resolvedPath); | ||
| edits.push({ start: valueStart, end: valueStart + value.length, replacement: resolvedPath }); | ||
| tagFilepaths.push(resolvedPath); | ||
| tagEdits.push({ start: valueStart, end: valueStart + value.length, replacement: resolvedPath }); | ||
| } | ||
| } | ||
| } else if (content[i] === "{") { | ||
| i++; | ||
| let braceDepth = 1; | ||
| const exprStart = i; | ||
| while (i < len && braceDepth > 0) { | ||
| while (i < limit && braceDepth > 0) { | ||
| if (content[i] === "{") { | ||
| braceDepth++; | ||
| } else if (content[i] === "}") { | ||
|
|
@@ -340,8 +371,8 @@ function parseJsxTag( | |
| const src = trimAnchor(value); | ||
| const resolvedPath = resolvePath(src, metadata); | ||
| if (src && resolvedPath) { | ||
| filepaths.add(resolvedPath); | ||
| edits.push({ | ||
| tagFilepaths.push(resolvedPath); | ||
| tagEdits.push({ | ||
| start: exprStart + 1, | ||
| end: exprStart + 1 + value.length, | ||
| replacement: resolvedPath | ||
|
|
@@ -354,8 +385,8 @@ function parseJsxTag( | |
| const src = trimAnchor(value); | ||
| const resolvedPath = resolvePath(src, metadata); | ||
| if (src && resolvedPath) { | ||
| filepaths.add(resolvedPath); | ||
| edits.push({ | ||
| tagFilepaths.push(resolvedPath); | ||
| tagEdits.push({ | ||
| start: exprStart + 1, | ||
| end: exprStart + 1 + value.length, | ||
| replacement: resolvedPath | ||
|
|
@@ -367,9 +398,15 @@ function parseJsxTag( | |
| } | ||
| } | ||
|
|
||
| if (i < len && content[i] === ">") { | ||
| i++; | ||
| if (i >= limit || content[i] !== ">") { | ||
| return null; | ||
| } | ||
| i++; | ||
|
|
||
| for (const filepath of tagFilepaths) { | ||
| filepaths.add(filepath); | ||
| } | ||
| edits.push(...tagEdits); | ||
|
|
||
| return { nextIndex: i }; | ||
| } | ||
|
|
@@ -765,8 +802,32 @@ export function replaceImagePathsAndUrls( | |
| let hasUnhandledExpressions = false; | ||
| let i = 0; | ||
| const len = content.length; | ||
| let inCodeFence = false; | ||
| let inInlineCode = false; | ||
|
|
||
| while (i < len) { | ||
| if ((i === 0 || content[i - 1] === "\n") && content.slice(i, i + 3) === "```") { | ||
| inCodeFence = !inCodeFence; | ||
| i += 3; | ||
| continue; | ||
| } | ||
|
|
||
| if (inCodeFence) { | ||
| i++; | ||
| continue; | ||
| } | ||
|
|
||
| if (content[i] === "`" && content[i - 1] !== "\\") { | ||
| inInlineCode = !inInlineCode; | ||
| i++; | ||
| continue; | ||
| } | ||
|
|
||
| if (inInlineCode) { | ||
| i++; | ||
| continue; | ||
| } | ||
|
Comment on lines
+877
to
+885
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 A single unmatched backtick on a page stops every later image and link from being rewritten The rewriting pass now treats everything after an unmatched backtick as inline code ( How the inline-code state leaks across the whole documentPass 2 ( Because pass 1 for normal-sized files uses the mdast path, it correctly rewrites image sources to absolute paths; pass 2 then silently skips the substitution, and the absolute path is written into the published markdown. Per CommonMark, code spans cannot contain a blank line and unmatched backticks are literal text, so the scanner should at minimum clear Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid — fixed in 10d466b. Both passes no longer carry
New tests: unmatched backtick in prose, backtick inside an indented fence, unterminated fence, plus one asserting an image inside a real fence is still left alone. |
||
|
|
||
| if (content[i] === "!" && content[i + 1] === "[") { | ||
| const result = parseMarkdownImage(content, i, metadata); | ||
| if (result) { | ||
|
|
@@ -807,6 +868,10 @@ export function replaceImagePathsAndUrls( | |
| j++; | ||
| } | ||
| } | ||
| if (parenDepth !== 0) { | ||
| i++; | ||
| continue; | ||
| } | ||
| const urlEnd = j - 1; | ||
| const href = content.slice(urlStart, urlEnd).trim(); | ||
| const destination = splitDestinationAndTitle(href); | ||
|
|
@@ -828,18 +893,21 @@ export function replaceImagePathsAndUrls( | |
| i = j; | ||
| continue; | ||
| } | ||
| } else if (content[i] === "<") { | ||
| } else if (isJsxTagStart(content, i)) { | ||
| const limit = findJsxTagScanLimit(content, i); | ||
| // Edits collected while scanning are discarded unless the tag is properly terminated. | ||
| const editsBeforeTag = edits.length; | ||
| let j = i + 1; | ||
| while (j < len && content[j] !== ">" && content[j] !== " " && content[j] !== "\n") { | ||
| while (j < limit && content[j] !== ">" && content[j] !== " " && content[j] !== "\n") { | ||
| j++; | ||
| } | ||
| while (j < len && content[j] !== ">") { | ||
| while (j < len && (content[j] === " " || content[j] === "\n")) { | ||
| while (j < limit && content[j] !== ">") { | ||
| while (j < limit && (content[j] === " " || content[j] === "\n")) { | ||
| j++; | ||
| } | ||
| const attrStart = j; | ||
| while ( | ||
| j < len && | ||
| j < limit && | ||
| content[j] !== "=" && | ||
| content[j] !== ">" && | ||
| content[j] !== " " && | ||
|
|
@@ -854,7 +922,7 @@ export function replaceImagePathsAndUrls( | |
| // Skip past the closing } | ||
| let braceDepth = 0; | ||
| j = attrStart; | ||
| while (j < len) { | ||
| while (j < limit) { | ||
| if (content[j] === "{") { | ||
| braceDepth++; | ||
| } else if (content[j] === "}") { | ||
|
|
@@ -866,7 +934,7 @@ export function replaceImagePathsAndUrls( | |
| } else if (content[j] === '"' || content[j] === "'") { | ||
| const q = content[j]; | ||
| j++; | ||
| while (j < len && content[j] !== q) { | ||
| while (j < limit && content[j] !== q) { | ||
| if (content[j] === "\\") { | ||
| j++; | ||
| } | ||
|
|
@@ -879,23 +947,23 @@ export function replaceImagePathsAndUrls( | |
| } | ||
| if (content[j] === "=") { | ||
| j++; | ||
| while (j < len && (content[j] === " " || content[j] === "\n")) { | ||
| while (j < limit && (content[j] === " " || content[j] === "\n")) { | ||
| j++; | ||
| } | ||
| // Handle plain quotes: attr="value" or attr='value' | ||
| // Also handle JSX expression: attr={'value'} or attr={"value"} | ||
| const isCurlyWrapped = content[j] === "{"; | ||
| if (isCurlyWrapped) { | ||
| j++; // skip { | ||
| while (j < len && (content[j] === " " || content[j] === "\n")) { | ||
| while (j < limit && (content[j] === " " || content[j] === "\n")) { | ||
| j++; | ||
| } | ||
| } | ||
| if (content[j] === '"' || content[j] === "'") { | ||
| const quote = content[j]; | ||
| j++; | ||
| const valueStart = j; | ||
| while (j < len && content[j] !== quote) { | ||
| while (j < limit && content[j] !== quote) { | ||
| if (content[j] === "\\") { | ||
| j += 2; | ||
| } else { | ||
|
|
@@ -905,10 +973,10 @@ export function replaceImagePathsAndUrls( | |
| const value = content.slice(valueStart, j); | ||
| j++; // skip closing quote | ||
| if (isCurlyWrapped) { | ||
| while (j < len && (content[j] === " " || content[j] === "\n")) { | ||
| while (j < limit && (content[j] === " " || content[j] === "\n")) { | ||
| j++; | ||
| } | ||
| if (j < len && content[j] === "}") { | ||
| if (j < limit && content[j] === "}") { | ||
| j++; // skip } | ||
| } | ||
| } | ||
|
|
@@ -947,15 +1015,15 @@ export function replaceImagePathsAndUrls( | |
| hasUnhandledExpressions = true; | ||
| // Skip past the closing } | ||
| let braceDepth = 1; | ||
| while (j < len && braceDepth > 0) { | ||
| while (j < limit && braceDepth > 0) { | ||
| if (content[j] === "{") { | ||
| braceDepth++; | ||
| } else if (content[j] === "}") { | ||
| braceDepth--; | ||
| } else if (content[j] === '"' || content[j] === "'") { | ||
| const q = content[j]; | ||
| j++; | ||
| while (j < len && content[j] !== q) { | ||
| while (j < limit && content[j] !== q) { | ||
| if (content[j] === "\\") { | ||
| j++; | ||
| } | ||
|
|
@@ -967,9 +1035,12 @@ export function replaceImagePathsAndUrls( | |
| } | ||
| } | ||
| } | ||
| if (j < len && content[j] === ">") { | ||
| j++; | ||
| if (j >= limit || content[j] !== ">") { | ||
| edits.length = editsBeforeTag; | ||
| i++; | ||
| continue; | ||
| } | ||
| j++; | ||
| i = j; | ||
| continue; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Windows-style line endings defeat the new safety bound, so a stray angle bracket can again skip images
The new bound that stops a tag scan at the next empty line looks only for two consecutive Unix newlines (
content.indexOf("\n\n", start)atpackages/cli/docs-markdown-utils/src/parseImagePaths.ts:82), so on pages saved with Windows line endings the scan runs to the end of the page and the images and links it passes over keep their build-machine paths.Impact: Docs pages authored with Windows line endings can still publish local filesystem paths and broken images when a tag-like
<is left unterminated.Mechanism
findJsxTagScanLimitis the only guard preventing an unterminated tag-like<from consuming the rest of the document, and it is used by both passes (packages/cli/docs-markdown-utils/src/parseImagePaths.ts:305and:897). With CRLF content, blank lines are\r\n\r\n, which does not contain\n\n, soindexOfreturns -1 and the limit becomescontent.length. Nothing in this package normalizes line endings (no CRLF handling exists inpackages/cli/docs-markdown-utils/src). Matching/\n[ \t]*\r?\n/(or normalizing\r) would restore the bound.Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 10d466b — the bound (now
findScanLimit, shared by the tag scan and the inline-code scan) matches/ [ \t]*\r? /, so CRLF and whitespace-only blank lines are handled. Added a CRLF regression case.