chore: upgrade marked to 18.0.0#1
Conversation
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
marked@18 raises the minimum supported Node version to >=20, which is a likely breaking compatibility change unless this package also declares/enforces the same runtime requirement. The custom list rendering now bypasses marked’s built-in handling for task lists and tight/loose parsing, which can silently change HTML output. Image alt handling can regress to alt="undefined" when no text is present.
Additional notes (3)
-
Compatibility |
package.json:49-53
marked@18declaresengines: { node: '>= 20' }(also visible in the lockfile). If this package is consumed in environments running Node 18 (still common for libraries), this becomes a breaking change at install/runtime time. You should either (1) set your ownpackage.json#engines.nodeto match, and ensure CI/publishing docs reflect that, or (2) avoid the major bump and stay on amarkedversion compatible with your supported Node range. -
Compatibility |
src/utils.ts:188-188
The newlist/listitemrendering is now responsible for behaviors that themarkedparser previously injected for you. In particular: -
Task lists:
markedtypically prepends a disabled checkbox and may add task-list related classes/structure. By directlyparse(item.tokens)and wrapping it in<li>, you’ll drop checkboxes entirely. -
Tight vs loose lists:
markeddistinguishes whether list item content should be treated as “top-level” (affects paragraph wrapping). Always callingparser.parse(item.tokens)risks emitting extra<p>wrappers (or missing expected ones), changing HTML output.
Also, token.start !== "" is a suspicious condition for what should be numeric start values; it can hide bugs and makes the intent unclear.
- Security |
src/utils.ts:178-178
Links opened withtarget="_blank"should includerel="noopener noreferrer"to prevent reverse-tabnabbing in browser contexts. Since this renderer generates raw HTML, this is a straightforward hardening win.
Summary of changes
Summary of changes
- Upgraded
markedfrom7.0.4to18.0.0(and updatedpnpm-lock.yamlaccordingly). - Refactored
src/utils.tscustommarked.Rendereroverrides tomarked@18’s token-based renderer API. - Updated rendering logic to re-parse nested content via
customRenderer.parser.parse()/parseInline()(notably for blockquotes, emphasis/strong/del, headings, paragraphs, lists, tables, and images).
| customRenderer.image = ({ href, text, tokens }) => { | ||
| const altText = tokens | ||
| ? customRenderer.parser.parseInline(tokens, customRenderer.parser.textRenderer) | ||
| : text; | ||
|
|
||
| return `<img src="${href}" alt="${altText}"${ | ||
| parseCssInJsToInlineCss(finalStyles.image) !== "" | ||
| ? ` style="${parseCssInJsToInlineCss(finalStyles.image)}"` | ||
| : "" | ||
| }>`; | ||
| } |
There was a problem hiding this comment.
If text is absent, the current fallback can emit alt="undefined". That’s user-visible and tends to leak into emails/HTML output. It’s safer to coerce to an empty string.
Suggestion
Default altText to an empty string:
const altText = (
tokens
? customRenderer.parser.parseInline(tokens, customRenderer.parser.textRenderer)
: text
) ?? "";Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this tweak.
There was a problem hiding this comment.
Starting a review of PR #1 in response to the “yes please” on that image renderer comment, and I may refine the implementation in src/utils.ts to keep the alt text behavior correct with the new token-based API.
Updated: Apr 15, 14:00 UTC
I’m working and can’t be interrupted or see replies—feel free to start another task with me while I finish.
There was a problem hiding this comment.
Expand this to see my work.
- Inspected
src/utils.tsaroundcustomRenderer.imageto understand the current image renderer and howalttext is derived after themarked18.0.0 upgrade. - Hoisted HTML attribute escaping into a shared
escapeHtmlAttrhelper insrc/utils.tsand wiredcustomRenderer.imageto use it for bothsrcandaltattributes, matching the maintainer’s request in PR #1. - Reworked
altTextincustomRenderer.imageto use(tokens ? customRenderer.parser.parseInline(tokens, customRenderer.parser.textRenderer) : undefined) ?? text ?? ""so token parsing is preferred, thentext, then an empty string, preventingalt="undefined". - Resolved the earlier merge conflict around
customRenderer.imageinsrc/utils.ts, combining the attribute-escaping behavior with the robustaltTextfallback logic on branchchore/upgrade-marked-latest. - Re-ran
pnpm run lint && pnpm run build && pnpm run test, confirmingtsc,tsupbuilds, and all 19 Vitest tests (including coverage forsrc/utils.ts) pass successfully with the improvedaltTextandescapeHtmlAttrlogic. - Synced local
chore/upgrade-marked-latesttoorigin/chore/upgrade-marked-latestat commitd47b22fso the latest refactor to reuse HTML attribute escaping insrc/utils.tsis on GitHub. - Committed the
altTextfallback fix asfix: preserve image alt fallbackonchore/upgrade-marked-latestand pushed it so PR #1 reflects the finalized behavior. - Verified the upstream PR details with
gh pr view 1 --repo shellscape/md-to-react-email --json isDraft,reviewRequests,assignees,headRefName,urlto confirm it targetschore/upgrade-marked-latestand is not a draft. - Updated PR #1 metadata via
gh pr editto re-request review fromCharlieHelpsandshellscapeafter the latestsrc/utils.tschanges. - Confirmed a clean working tree on
chore/upgrade-marked-latestwithgit status --porcelainafter pushing the finalized commits for the marked 18.0.0 upgrade.
There was a problem hiding this comment.
Done — customRenderer.image now guarantees we never emit alt="undefined", and it preserves the text fallback even if token parsing returns a nullish value. This is on top of the recent src/alt attribute escaping hardening. Applied in 0f06e8e (see src/utils.ts around lines 174–193).
Verification
# TypeScript: no errors
pnpm run lint
# tsup: build succeeded
pnpm run build
# Vitest: 19 tests passed (4 files)
pnpm run test| customRenderer.image = ({ href, text, tokens }) => { | ||
| const altText = tokens | ||
| ? customRenderer.parser.parseInline(tokens, customRenderer.parser.textRenderer) | ||
| : text; | ||
|
|
||
| return `<img src="${href}" alt="${altText}"${ | ||
| parseCssInJsToInlineCss(finalStyles.image) !== "" | ||
| ? ` style="${parseCssInJsToInlineCss(finalStyles.image)}"` | ||
| : "" | ||
| }>`; | ||
| } |
There was a problem hiding this comment.
altText can be undefined (becoming the literal string "undefined" in HTML) and, more importantly, attribute values aren’t escaped here. If user-controlled Markdown is rendered, this can break markup or become an injection vector via quotes in alt text / URLs.
Suggestion
Default alt to an empty string and escape attribute values before interpolating into HTML.
const escapeAttr = (s: string) =>
s
.replace(/&/g, "&")
.replace(/"/g, """)
.replace(/</g, "<")
.replace(/>/g, ">");
const altText = tokens
? customRenderer.parser.parseInline(tokens, customRenderer.parser.textRenderer)
: text ?? "";
return `<img src="${escapeAttr(href)}" alt="${escapeAttr(altText)}"${...}>`;Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this hardening.
There was a problem hiding this comment.
Expand this to see my work.
- Confirmed that the
marked18.0.0 upgrade behavior insrc/utils.tsmatches the intent acknowledged by @shellscape in the review thread on PR #1. - Updated
customRenderer.imageinsrc/utils.tsto introduce anescapeHtmlAttrhelper, defaultaltTextandhrefto empty strings when absent, and escape attribute values before composing the<img>tag, aligning the implementation with the follow-up review discussion on PR #1. - Re-reviewed the updated
customRenderer.imageand sharedescapeHtmlAttrlogic insrc/utils.tsto ensure consistent escaping, defaulting, and type-safety around themarked18.0.0 upgrade. - Extended
escapeHtmlAttrinsrc/utils.tsto also escape single quotes to', ensuring double-quoted HTML attributes remain safe against quote injection. - Implemented the suggested readability refactor of
customRenderer.imageinsrc/utils.tsby replacing the nested nullish-coalescing/ternaryaltTextexpression with a simplelet altText = text ?? ""; if (tokens) { ... }conditional that preserves the same precedence. - Optimized
escapeHtmlAttrand image style computation insrc/utils.tsto reduce redundant work while keeping escaping semantics unchanged. - Ran
pnpm lint && pnpm build && pnpm testand verified a clean working tree forsrc/utils.tschanges, with all checks passing and coverage around ~~89.19% statements / 68.65% branches overall and ~~84.19% statements / 66.12% branches forsrc/utils.ts. - Left a security-focused review comment on
escapeHtmlAttrinsrc/utils.tson PR #1, clarifying its coverage of single quotes and double-quoted HTML attributes. - Committed and pushed the image attribute escaping, shared
escapeHtmlAttrhelper, single-quote support,altTextreadability tweak, and optimization asfix: escape image attrs,refactor: reuse html attr escaping,fix: escape single quotes in html attrs,refactor: simplify image alt text, andrefactor: optimize image attr escapingonchore/upgrade-marked-latestfor PR #1. - Verified via the PR metadata that review requests for PR #1 now include both
shellscapeandCharlieHelps, confirming the reviewer setup matches expectations.
There was a problem hiding this comment.
Applied the requested hardening around image alt handling (no more alt="undefined") and HTML-escaped the src/alt attribute values before interpolating them into the <img ...> tag. Latest commit: fdf39b2 (see src/utils.ts).
Changes
- Added
escapeHtmlAttr()and used it incustomRenderer.imageforsrc+altattributes (and keptaltdefaulting to""). - Also applied the same escaping to
customRenderer.link’shrefattribute for consistency.
Verification
# TypeScript: no errors
$ pnpm lint
# Build: tsup success
$ pnpm build
# Vitest: 19 tests passed (4 files)
$ pnpm testreviewChanges note skipped: unify escapeQuotes with escapeHtmlAttr — escapeQuotes is intentionally preserving existing CSS serialization behavior (incl. ') that’s already covered by snapshots.
There was a problem hiding this comment.
The marked@18 upgrade implicitly raises the runtime Node requirement to >=20, but the package doesn’t declare engines.node, making compatibility breaks likely for consumers. The new custom list/listitem rendering may change HTML output by dropping task list behavior and altering tight/loose paragraph handling. Link rendering should escape href and include rel="noopener noreferrer" when using target="_blank". Table cell align should be constrained/escaped for robustness and consistency with the new attribute-escaping work.
Additional notes (5)
-
Compatibility |
package.json:49-53
marked@18requires Node>=20(also reflected in the lockfile), but this package doesn’t declare anengines.nodeconstraint. That means consumers on Node 18 may fail at install/runtime in a non-obvious way (breaking change without an explicit signal). -
Security |
src/utils.ts:194-194
target="_blank"withoutrel="noopener noreferrer"is a known reverse-tabnabbing footgun in browser contexts. Also,hrefis interpolated without attribute escaping even thoughescapeHtmlAttr()now exists (so this is an inconsistent/partial hardening). -
Readability |
src/utils.ts:204-204
Thetoken.start !== ""check is suspicious and makes the intent unclear—startshould be numeric (orundefined). This can mask bugs and makes it harder to reason about whenstartis emitted. -
Compatibility |
src/utils.ts:204-204
The newlist/listitemimplementation bypassesmarked’s built-in list item semantics (notably task list checkboxes and tight-vs-loose paragraph handling).customRenderer.parser.parse(item.tokens)can change HTML output vs the default renderer (e.g., extra/missing<p>wrappers) and can drop task list UI entirely. -
Security |
src/utils.ts:288-300
align="${token.align}"is emitted without validation/escaping. Even ifmarkedusually constrains this toleft|center|right, constraining it here makes the renderer more robust (and consistent with the attribute-escaping added for images).
Summary of changes
What changed
- Upgraded
markedfrom7.0.4to18.0.0(with correspondingpnpm-lock.yamlupdates). - Refactored
src/utils.tscustommarked.Rendereroverrides tomarked@18’s token-based renderer API. - Added a reusable
escapeHtmlAttr()helper and applied it to<img>src/altrendering. - Reworked list/table rendering to manually iterate tokens and render nested content via
customRenderer.parser.parse()/parseInline().
There was a problem hiding this comment.
marked@18 raises the effective minimum Node runtime to >=20, but the package doesn’t declare engines.node, making consumer breakage likely. The link renderer still uses target="_blank" without rel="noopener noreferrer". The custom list rendering likely changes tight/loose list HTML due to parsing list items without list context. Table cell align should be constrained/escaped for consistency with the new attribute-escaping approach.
Additional notes (4)
- Compatibility |
package.json:49-53
marked@18requires Node>=20(also reflected in the lockfile), but this package doesn’t declare anengines.noderange. That’s a likely silent breaking change for consumers on Node 18: installs can fail (or runtime can break) without your package signaling an updated requirement.
You should either (a) declare and enforce the Node requirement at the package level, or (b) avoid the major bump and stay on a marked version compatible with your supported Node range.
-
Readability |
src/utils.ts:214-214
token.start !== ""is a suspicious condition (and makes intent unclear).startshould be numeric (orundefined), so checking against an empty string can mask bugs and produce unexpectedstart=""behavior if upstream ever changes. -
Compatibility |
src/utils.ts:235-235
The new list rendering takes over responsibilities thatmarkedpreviously handled internally. Unconditionally callingcustomRenderer.parser.parse(item.tokens)can change tight/loose list HTML (extra/missing<p>wrappers) compared tomarked’s default behavior, which uses list-item context to decide how to parse.
If preserving output is a goal of this upgrade, this is a high-risk area for subtle regressions.
- Security |
src/utils.ts:125-125
Code blocks/inline code are inserted without HTML-escaping. If markdown input is user-controlled (or even just untrusted), this is an injection vector (e.g.<img onerror=...>inside a fenced block would be emitted as real HTML). Themarkeddefault renderer escapes code content; this custom renderer currently does not.
Summary of changes
What changed
- Upgraded
markedfrom7.0.4to18.0.0(and updatedpnpm-lock.yamlaccordingly). - Refactored
src/utils.tscustommarked.Rendereroverrides to themarked@18token-based renderer API (blockquote,code,em/strong/del,heading,image,link,list/listitem,paragraph,table/*). - Added a shared
escapeHtmlAttr()helper and applied it to HTML attribute interpolation (notably<img src|alt>and<a href>). - Reworked list and table rendering to manually iterate tokens and render nested content via
customRenderer.parser.parse()/parseInline().
| customRenderer.link = ({ href, tokens }) => { | ||
| const text = customRenderer.parser.parseInline(tokens); | ||
| const hrefAttr = escapeHtmlAttr(href ?? ""); | ||
|
|
||
| return `<a href="${hrefAttr}" target="_blank"${ | ||
| parseCssInJsToInlineCss(finalStyles.link) !== "" | ||
| ? ` style="${parseCssInJsToInlineCss(finalStyles.link)}"` | ||
| : "" | ||
| }>${text}</a>`; | ||
| } |
There was a problem hiding this comment.
target="_blank" without rel="noopener noreferrer" is a reverse-tabnabbing footgun in browser contexts. Since this renderer emits raw HTML, it’s cheap to harden here (especially now that href is escaped).
Suggestion
Include rel="noopener noreferrer" when emitting target="_blank":
return `<a href="${hrefAttr}" target="_blank" rel="noopener noreferrer"${
linkStyle !== "" ? ` style="${linkStyle}"` : ""
}>${text}</a>`;Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this hardening.
There was a problem hiding this comment.
Expand this to see my work.
- Inspected
customRenderer.linkinsrc/utils.tson branchchore/upgrade-marked-latestto understand the current anchor rendering behavior in the context of PR #1. - Refined
customRenderer.linkinsrc/utils.tsto addrel="noopener noreferrer"while keepingtarget="_blank"and inline styles consistent with the existing anchor rendering. - Synced dependencies (including
marked@18.0.0) and iterated on theRenderercallback types so the destructured parameters incustomRendereralign with the updatedmarkedtypings and no longer trigger TypeScript errors. - Re-ran
pnpm -s lint && pnpm -s build && pnpm -s testto confirm that build, types, and runtime behavior are clean aside from the expected HTML differences due to the newrelattribute. - Updated expected HTML strings in
__tests__/parseMarkdownToJSX.test.tsso link-related snapshots now includerel="noopener noreferrer"alongsidetarget="_blank". - Re-ran
pnpm -s lint && pnpm -s build && pnpm -s testand confirmed all 19 tests pass, with coverage output still reporting ~89.5% overall andsrc/utils.tsat ~84.6%. - Checked
git statusto verify the only local changes are insrc/utils.tsand__tests__/parseMarkdownToJSX.test.tsfor the link behavior update. - Committed the link hardening and test expectation updates as
fix: add noopener rel to linksonchore/upgrade-marked-latest. - Pushed
chore/upgrade-marked-latest(includingfix: add noopener rel to links) to origin, confirmed the branch is clean viagit status --porcelain, and re-requested review fromshellscapeandCharlieHelpson PR #1. - Noted
@shellscape’s reply "@CharlieHelps yes please" on thesrc/utils.tscomment, confirming they want the suggested configurablelinkReloption wired intoinitRendererandcustomRenderer.link.
There was a problem hiding this comment.
Done — customRenderer.link now emits rel="noopener noreferrer" alongside target="_blank" (and keeps the existing href escaping + inline styles). Updated the link expectations in __tests__/parseMarkdownToJSX.test.ts accordingly.
Applied in 1198b94.
Verification
# TypeScript: no errors
pnpm lint
# Build: tsup success
pnpm build
# Vitest: 19 tests passed (4 files)
pnpm testreviewChanges note skipped: make rel configurable — this commit is implementing the explicitly requested rel="noopener noreferrer" hardening from the review thread.
| customRenderer.tablecell = (token) => { | ||
| const content = customRenderer.parser.parseInline(token.tokens); | ||
| const type = token.header ? "th" : "td"; | ||
| const styles = parseCssInJsToInlineCss( | ||
| finalStyles[flags.header ? "th" : "td"] | ||
| finalStyles[token.header ? "th" : "td"] | ||
| ); | ||
| const tag = flags.align | ||
| ? `<${type} align="${flags.align}"${ | ||
| const tag = token.align | ||
| ? `<${type} align="${token.align}"${ | ||
| styles !== "" ? ` style="${styles}"` : "" | ||
| }>` | ||
| : `<${type}${styles !== "" ? ` style="${styles}"` : ""}>`; | ||
| return tag + content + `</${type}>\n`; | ||
| } |
There was a problem hiding this comment.
align="${token.align}" is emitted without validation/escaping. Even if marked typically constrains this, it’s inconsistent with the new attribute-escaping hardening and leaves an unnecessary injection surface.
Suggestion
Constrain (and/or escape) align before interpolating:
const align = token.align && ["left", "center", "right"].includes(token.align)
? token.align
: undefined;
const tag = align
? `<${type} align="${align}"${styles !== "" ? ` style="${styles}"` : ""}>`
: `<${type}${styles !== "" ? ` style="${styles}"` : ""}>`;Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this validation.
There was a problem hiding this comment.
Expand this to see my work.
- Aligned
customRenderer.tablecellinsrc/utils.tswith the requested validation foralignon table cells and committed the change asfix: validate table cell alignon thechore/upgrade-marked-latestbranch. - Ran
pnpm lint && pnpm build && pnpm testlocally, withtscreporting multiple type errors insrc/utils.tsaround renderer callback parameter shapes (e.g.,tokens,text,href,align,items) after themarkedv18 upgrade. - Inspected the top section of
src/utils.ts(includinginitRendererand thecustomRendereroverrides likeblockquote,br,code, andcodespan) to understand the currentRenderertypings and why TypeScript now flags mismatches with the updatedmarkedtypes. - Checked PR #1 metadata to confirm it is not a draft and to see the current
reviewRequestsandheadRefNameforchore/upgrade-marked-latest. - Re-requested review from
CharlieHelpson PR #1 in response to thesrc/utils.tscomment reply (@CharlieHelps yes please). - Verified via
gh pr view 1 --json reviewRequeststhat bothshellscapeandCharlieHelpsare now listed underreviewRequestsfor PR #1.
There was a problem hiding this comment.
Applied the requested align hardening for table cells: we now only emit align="left|center|right" and drop anything else (so we’re not interpolating unvalidated values into HTML).
Change is in f370996 (src/utils.ts around L298-L314).
Verification
# Install
$ pnpm install --frozen-lockfile
# TypeScript: no errors
$ pnpm lint
# Build: tsup succeeded
$ pnpm build
# Vitest: 19 tests passed (4 files)
$ pnpm testRe-requested review from @CharlieHelps.
There was a problem hiding this comment.
This upgrade to marked@18 implicitly raises the runtime Node requirement to >=20, but the package doesn’t declare engines.node, making consumer breakage likely. The custom list/listitem rendering is high-risk for output regressions (task lists and tight/loose list semantics) and includes a suspicious token.start !== "" guard. Link rendering still uses target="_blank" without rel="noopener noreferrer". Code and inline code rendering currently emit unescaped HTML text, which can be an injection vector and deviates from marked defaults.
Additional notes (2)
-
Compatibility |
package.json:49-53
marked@18requires Node>=20(also reflected in the lockfile), but this package doesn’t declare anengines.noderange. That turns this dependency bump into a likely silent breaking change for consumers running Node 18 (install/runtime failures without your package clearly signaling the new requirement). -
Security |
src/utils.ts:125-125
Code blocks and inline code are inserted as raw text content without HTML-escaping. That deviates frommarked’s default behavior and can become an injection vector if markdown is ever user-controlled (e.g.,<img onerror=...>inside a fenced block would be emitted as real HTML).
Summary of changes
Summary
- Upgraded
markedfrom7.0.4to18.0.0inpackage.json(with correspondingpnpm-lock.yamlupdates). - Forced synchronous parsing by setting
marked.parse(..., { async: false })insrc/parser.tsand returningstring. - Refactored the custom
markedrenderer insrc/utils.tsto the token-based renderer API, including:- Nested token rendering via
customRenderer.parser.parse()/parseInline(). - New
escapeHtmlAttr()helper and usage for HTML attribute interpolation (e.g.,<img src|alt>,<a href>). - Reworked list/table rendering to manually iterate tokens and rebuild HTML strings.
- Table cell
alignhandling constrained toleft|center|right.
- Nested token rendering via
|
@CharlieHelps bump engines to node 22 minimum |
Expand this to see my work.
|
There was a problem hiding this comment.
The renderer still emits unescaped code/inline-code content, which is a concrete HTML injection vector and deviates from marked defaults. Link href is escaped but not scheme-validated, so javascript:-style URLs can still be generated. The custom list rendering contains a suspicious token.start !== "" guard and is high-risk for subtle output changes (tight/loose semantics) without explicit tests.
Additional notes (3)
-
Security |
src/utils.ts:125-125
codeandcodespaninterpolate rawtextdirectly into HTML. That means code like<img onerror=alert(1)>inside a fenced block/inline code will be emitted as real HTML, which is a straightforward injection vector and also deviates frommarked’s default behavior (it escapes code content). This became more important now that the renderer has been actively refactored and hardened elsewhere (attrs), but code content remains unescaped. -
Compatibility |
src/utils.ts:213-213
The ordered-liststartattribute logic includestoken.start !== "", which is a confusing guard (and likely always-true ifstartis numeric). More importantly,listitemnow re-parsesitem.tokensdirectly, which can subtly change list output vsmarked’s list-item context behavior (tight vs loose list paragraph wrapping, and task-list handling if you ever want it). If output stability is a goal, this area needs tighter control and test coverage. -
Maintainability |
.github/workflows/main.yml:12-16
You bumped the workflow Node version to22.x, but both workflows are still pinned to older major versions of GitHub Actions (actions/checkout@v3,actions/setup-node@v3). Keeping these updated reduces supply-chain risk and avoids surprises from deprecated action runtimes.
Summary of changes
Dependency + runtime upgrades
- Upgraded
markedfrom7.0.4→18.0.0(package.json,pnpm-lock.yaml). - Declared a package-level Node requirement via
package.json#engines.node: ">=22". - Aligned CI/publish workflows to run on Node
22.x.
Parsing behavior
- Forced synchronous markdown parsing by passing
async: falsetomarked.parse(...)and makingMarkdownParser.parse()explicitly returnstring.
Renderer refactor (marked v18 token API)
- Migrated custom renderer overrides in
src/utils.tsto token-based signatures (e.g.,blockquote,heading,paragraph,list,table*, etc.). - Added
escapeHtmlAttr()and used it for HTML attribute interpolation (notably<img src|alt>and<a href>). - Hardened links by adding
rel="noopener noreferrer"alongsidetarget="_blank". - Reworked table rendering and constrained table-cell
aligntoleft|center|right.
Tests
- Updated link-related expectations to include
rel="noopener noreferrer".
| customRenderer.link = ({ href, tokens }) => { | ||
| const text = customRenderer.parser.parseInline(tokens); | ||
| const hrefAttr = escapeHtmlAttr(href ?? ""); | ||
| const linkStyle = parseCssInJsToInlineCss(finalStyles.link); | ||
|
|
||
| return `<a href="${hrefAttr}" target="_blank" rel="noopener noreferrer"${ | ||
| linkStyle !== "" ? ` style="${linkStyle}"` : "" | ||
| }>${text}</a>`; |
There was a problem hiding this comment.
Escaping href prevents quote-breaking/injection, but it does not prevent dangerous URL schemes (e.g., javascript:). Since this library outputs raw HTML intended for rendering, it’s worth explicitly constraining/normalizing href to a safe set (or making it configurable) to avoid producing obviously unsafe anchors.
Suggestion
Add a minimal scheme allowlist (possibly configurable) before escaping.
const rawHref = (href ?? "").trim();
const isSafeHref = /^(https?:|mailto:|tel:|#|\/)/i.test(rawHref);
const hrefAttr = escapeHtmlAttr(isSafeHref ? rawHref : "");Reply with "@CharlieHelps yes please" if you’d like me to add a commit implementing this (optionally behind a renderer option to avoid breaking changes).
|
Done — bumped the package
Verification# Install
pnpm install --frozen-lockfile
# TypeScript: no errors
pnpm lint
# Build: success
pnpm build
# Vitest: 19 tests passed (4 files)
pnpm testreviewChanges note skipped: relax |
|
@CharlieHelps bump to node 24.15.0 as the default for development; .nvmrc, workflows, etc. but keep the engines property in package.json and docs as the minimum supported version. |
Expand this to see my work.
|
|
Updated the repo’s default dev Node to Changes
Commit: Verification# Install
pnpm install --frozen-lockfile
# TypeScript: no errors
pnpm -s lint
# Build: tsup success
pnpm -s build
# Vitest: 19 tests passed (4 files)
pnpm -s test |
There was a problem hiding this comment.
The renderer currently emits unescaped code/codespan content, which is a concrete HTML injection vector and should be fixed. Link hardening improved with rel="noopener noreferrer", but href is still not scheme-validated, so unsafe URLs like javascript: can be generated. Custom list rendering includes a confusing token.start !== "" guard and is high-risk for subtle output changes (tight/loose/task-list semantics) without targeted tests.
Additional notes (4)
-
Security |
src/utils.ts:125-125
codecontent is interpolated directly into HTML without escaping. That means fenced code like<img onerror=...>will be emitted as real HTML, which is a concrete injection vector and also deviates frommarked’s default escaping behavior for code blocks. -
Security |
src/utils.ts:135-135
Same issue as the fenced code renderer: inline code is emitted without HTML-escaping, so backticked content can break markup or inject HTML. -
Compatibility |
src/utils.ts:213-213
The ordered-liststartattribute logic includestoken.start !== "", which is an odd guard for a value that should be numeric/undefinedand makes the intent unclear. Also, by rebuilding list output manually (list→listitem→parser.parse(item.tokens)), you’re taking overmarked’s list semantics, which is high-risk for output regressions (tight/loose paragraph wrapping and task list behavior) unless explicitly tested. -
Compatibility |
.github/workflows/main.yml:12-16
Workflows now run using the.nvmrcversion (24.15.0), butpackage.json#engines.nodedeclares a minimum of>=22. Only testing on the latest dev version risks missing incompatibilities with the supported minimum.
Summary of changes
Summary
- Upgraded
markedfrom7.0.4to18.0.0(package.json,pnpm-lock.yaml). - Declared a minimum supported runtime via
package.json#engines.node: ">=22"and added a Changeset documenting the major bump. - Standardized CI/publish workflows to use
node-version-file: .nvmrcand added.nvmrc(24.15.0) for dev/CI defaults. - Updated the markdown renderer integration:
- Forced synchronous parsing via
marked.parse(..., { async: false })insrc/parser.ts. - Migrated
src/utils.tsrenderer overrides tomarked@18’s token-based APIs. - Added
escapeHtmlAttr()and used it for HTML attribute interpolation (notably<img src|alt>and<a href>). - Hardened external links by adding
rel="noopener noreferrer"and updated tests accordingly. - Reworked list/table rendering to build output from tokens and validate table-cell
align.
- Forced synchronous parsing via
2 participants
Summary
markedfrom7.0.4to18.0.0src/utils.tsto themarked@18token-based renderer APIparser.parse/parser.parseInlineand updating list/table/image handlingValidation
pnpm lintpnpm buildpnpm test