Conversation
WalkthroughThe PR migrates the kg-default-nodes package from a JS/Rollup/CommonJS layout to a TypeScript/ESM+CJS layout: Rollup config and many built JS artifacts were removed; new TypeScript source files were added under src/ (including a new central src/kg-default-nodes.ts), numerous node classes, parsers, renderers, and utilities were rewritten/typed, and lib/ artifacts were removed. package.json was updated to use build outputs and an exports map. ESLint config was refactored to a defineConfig TypeScript-centric setup. Tests and test-utils were converted from CommonJS to ESM/TypeScript, with new test typings and helpers. Many APIs gained explicit type signatures and Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
packages/kg-default-nodes/src/utils/size-byte-converter.ts (1)
1-25:⚠️ Potential issue | 🟡 MinorHandle malformed and out-of-range sizes before formatting.
sizeToBytes('foo KB')currently returnsNaN, andbytesToSize(1024 ** 5)returns1 undefinedbecause the unit index runs pastTB. The new type annotations do not protect either runtime value, so imported file metadata can still leak broken text into rendering.Possible fix
export function sizeToBytes(size: string) { if (!size) { return 0; } const sizes = ['Bytes', 'KB', 'MB', 'GB', 'TB']; - const sizeParts = size.split(' '); - const sizeNumber = parseFloat(sizeParts[0]); - const sizeUnit = sizeParts[1]; + const [rawNumber = '', rawUnit = ''] = size.trim().split(/\s+/, 2); + const sizeNumber = Number(rawNumber); + const sizeUnit = rawUnit === 'Byte' ? 'Bytes' : rawUnit; const sizeUnitIndex = sizes.indexOf(sizeUnit); - if (sizeUnitIndex === -1) { + if (!Number.isFinite(sizeNumber) || sizeNumber < 0 || sizeUnitIndex === -1) { return 0; } return Math.round(sizeNumber * Math.pow(1024, sizeUnitIndex)); } export function bytesToSize(bytes: number) { - if (!bytes) { + if (!Number.isFinite(bytes) || bytes < 1) { return '0 Byte'; } const sizes = ['Bytes', 'KB', 'MB', 'GB', 'TB']; - if (bytes === 0) { - return '0 Byte'; - } - const i = Math.floor(Math.log(bytes) / Math.log(1024)); + const i = Math.min(sizes.length - 1, Math.floor(Math.log(bytes) / Math.log(1024))); return Math.round((bytes / Math.pow(1024, i))) + ' ' + sizes[i]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/utils/size-byte-converter.ts` around lines 1 - 25, sizeToBytes: validate the input string and parsed number before computing—ensure size is a non-empty string, split yields at least 2 parts, parseFloat returns a finite number, and the unit is one of the allowed values (sizes array); if any check fails return 0. bytesToSize: validate bytes is a finite non-negative number and handle the unit index overflow by clamping the computed index i to the last index of sizes (so very large values map to 'TB' instead of undefined); also treat 0 or negative/invalid bytes as '0 Byte'. Use the existing function names sizeToBytes and bytesToSize and the sizes array to locate where to add these guards and clamping logic.packages/kg-default-nodes/src/nodes/header/parsers/header-parser.ts (1)
18-40:⚠️ Potential issue | 🟡 MinorCoerce nullable DOM reads back to strings before
new HeaderNode(payload).
getAttribute()andtextContentcan still returnnull, butpackages/kg-default-nodes/src/nodes/header/HeaderNode.tsdeclares these fields as strings. Packing them intoRecord<string, unknown>only hides the mismatch and lets malformed imports storenullinto string properties. Use|| ''(or omit the key) for the nullable reads before constructing the node.Also applies to: 57-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/header/parsers/header-parser.ts` around lines 18 - 40, The DOM reads for backgroundImageSrc, header, subheader, buttonUrl and buttonText can be null but are passed into new HeaderNode(payload) where those fields are declared as strings; coerce each nullable value to a string (e.g. using value || '') before building the payload in header-parser.ts so HeaderNode never receives nulls (apply same fix for the other block referenced around lines 57-84); ensure you update variables backgroundImageSrc, header, subheader, buttonUrl and buttonText (or omit keys) prior to constructing the HeaderNode instance.packages/kg-default-nodes/src/nodes/button/button-renderer.ts (1)
50-72:⚠️ Potential issue | 🟠 MajorEscape
buttonTextandbuttonUrlbefore interpolating email HTML.
packages/kg-default-nodes/src/utils/tagged-template-fns.tsis raw string concatenation, so these branches inject node data straight intoinnerHTML. A button label containing</&or a crafted URL with quotes will break the markup and creates an HTML-injection sink. Escape the dynamic values first, and ideally whitelistalignmentrather than interpolating it raw.Possible fix
+import {escapeHtml} from '../../utils/escape-html.js'; + function emailTemplate(node: ButtonNodeData, options: RenderOptions, document: Document) { const {buttonUrl, buttonText} = node; + const safeButtonUrl = escapeHtml(buttonUrl); + const safeButtonText = escapeHtml(buttonText); @@ - <a href="${buttonUrl}">${buttonText}</a> + <a href="${safeButtonUrl}">${safeButtonText}</a> @@ - <a href="${buttonUrl}">${buttonText}</a> + <a href="${safeButtonUrl}">${safeButtonText}</a>Also applies to: 97-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/button/button-renderer.ts` around lines 50 - 72, In emailTemplate, avoid injecting raw node.buttonText and node.buttonUrl into cardHtml and element.innerHTML: HTML-escape buttonText and properly validate/encode buttonUrl before interpolation, and whitelist node.alignment to a small set of allowed values (e.g., "left","center","right") instead of interpolating it raw; then build the string using the escaped values (or set element.textContent / anchor.href on created elements instead of innerHTML) and replace the existing innerHTML assignments in emailTemplate and the other identical branch that also sets innerHTML so dynamic values cannot break the markup or create an HTML-injection sink.packages/kg-default-nodes/test/nodes/markdown.test.ts (1)
40-43:⚠️ Potential issue | 🟡 MinorMisleading test description: refers to
$isImageNodeinstead of$isMarkdownNode.The test description says "matches node with $isImageNode" but the test actually verifies
$isMarkdownNode. This appears to be a copy-paste error.📝 Suggested fix
- it('matches node with $isImageNode', editorTest(function () { + it('matches node with $isMarkdownNode', editorTest(function () { const markdownNode = $createMarkdownNode(dataset); $isMarkdownNode(markdownNode).should.be.true(); }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/test/nodes/markdown.test.ts` around lines 40 - 43, The test description is incorrect: update the it(...) description string that currently reads 'matches node with $isImageNode' to correctly refer to $isMarkdownNode (or a neutral description like 'matches node with $isMarkdownNode') so it matches the assertion using $createMarkdownNode and $isMarkdownNode in the test; ensure the test name reflects the behavior being verified in the it block.packages/kg-default-nodes/src/nodes/call-to-action/calltoaction-parser.ts (1)
27-31:⚠️ Potential issue | 🟡 MinorType mismatch:
imageDatatypes don't align withCallToActionNodeinterface.The inline type for
imageDataspecifiesimageUrl: string | numberand dimensions asstring | number | null, but theCallToActionNodeinterface (from the relevant snippet) definesimageUrl: string | nulland dimensions asnumber | null. This inconsistency could cause type errors downstream.🔧 Proposed fix to align types
- const imageData: {imageUrl: string | number; imageWidth: string | number | null; imageHeight: string | number | null} = { + const imageData: {imageUrl: string; imageWidth: number | null; imageHeight: number | null} = { imageUrl: '', imageWidth: null, imageHeight: null };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/call-to-action/calltoaction-parser.ts` around lines 27 - 31, The inline type and initial values for the imageData constant in calltoaction-parser.ts don't match the CallToActionNode interface; change imageData's type to imageUrl: string | null and imageWidth/imageHeight: number | null and update the initializer values accordingly (use null for missing values rather than '' or null-for-numeric mismatches) so imageData conforms to CallToActionNode.packages/kg-default-nodes/test/nodes/signup.test.ts (1)
688-694:⚠️ Potential issue | 🟡 MinorPotential test bug: Test creates a
PaywallNodeinstead ofSignupNode.The
getTextContenttest forSignupNodecreates aPaywallNodeinstead of aSignupNode. This appears to be unintentional and tests the wrong node type.🐛 Proposed fix
describe('getTextContent', function () { it('returns contents', editorTest(function () { - const node = $createPaywallNode({}); + const node = $createSignupNode({}); // signup nodes don't have text content node.getTextContent().should.equal(''); })); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/test/nodes/signup.test.ts` around lines 688 - 694, The test in the describe('getTextContent') block incorrectly instantiates a PaywallNode via $createPaywallNode; replace that with a SignupNode creation (use $createSignupNode or the factory used elsewhere for SignupNode) so the test exercises SignupNode.getTextContent(), keep the assertion that getTextContent() returns an empty string and leave the surrounding editorTest wrapper intact.packages/kg-default-nodes/src/nodes/embed/embed-renderer.ts (1)
47-55:⚠️ Potential issue | 🟡 MinorPotential runtime error:
thumbnail_widthandthumbnail_heightmay be undefined.The
isVideoWithThumbnailcheck on line 47 only validatesthumbnail_url, but line 53 uses non-null assertions onthumbnail_widthandthumbnail_height. If these are undefined, the division will produceNaN, causing invalid spacer dimensions.🛡️ Proposed fix to guard thumbnail dimensions
- const isVideoWithThumbnail = node.embedType === 'video' && metadata && metadata.thumbnail_url; + const isVideoWithThumbnail = node.embedType === 'video' && metadata && metadata.thumbnail_url && metadata.thumbnail_width && metadata.thumbnail_height;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/embed/embed-renderer.ts` around lines 47 - 55, The code computes spacer dimensions using metadata.thumbnail_width and metadata.thumbnail_height with non-null assertions even though isVideoWithThumbnail only checked metadata.thumbnail_url; update the logic in embed-renderer.ts (the isVideoWithThumbnail / email thumbnail branch where spacerWidth/spacerHeight are computed) to first verify that metadata.thumbnail_width and metadata.thumbnail_height are present and are finite numbers before using them, and if not present fall back to safe defaults or skip creating the thumbnail spacer (e.g., use a default aspect ratio or skip the spacer calculation) so the division (thumbnail_width/thumbnail_height) cannot produce NaN or throw at runtime.packages/kg-default-nodes/src/nodes/call-to-action/calltoaction-renderer.ts (1)
52-60:⚠️ Potential issue | 🔴 CriticalFix unanchored color regex allowing attribute injection.
The regex
/^[a-zA-Z\d-]+|#([a-fA-F\d]{3}|[a-fA-F\d]{6})$/lacks proper grouping around the alternation. The first alternative^[a-zA-Z\d-]+has no end anchor, so it matches any string starting with alphanumeric characters. This allows payloads likeaccent" onclick="..."to pass validation and be directly interpolated into HTML attributes (e.g.,class="kg-cta-bg-${dataset.backgroundColor}"), creating an attribute injection vulnerability.Apply the fix to both locations (lines 52-60 and 395-399):
Suggested fix
+const VALID_COLOR_PATTERN = /^(?:[a-zA-Z\d-]+|#(?:[a-fA-F\d]{3}|[a-fA-F\d]{6}))$/; + function ctaCardTemplate(dataset: CTADataset) { // Add validation for buttonColor - if (!dataset.buttonColor || !dataset.buttonColor.match(/^[a-zA-Z\d-]+|#([a-fA-F\d]{3}|[a-fA-F\d]{6})$/)) { + if (!dataset.buttonColor || !VALID_COLOR_PATTERN.test(dataset.buttonColor)) { dataset.buttonColor = 'accent'; }Also apply to the
backgroundColorvalidation (lines 395-399):- if (!dataset.backgroundColor || !dataset.backgroundColor.match(/^[a-zA-Z\d-]+|#([a-fA-F\d]{3}|[a-fA-F\d]{6})$/)) { + if (!dataset.backgroundColor || !VALID_COLOR_PATTERN.test(dataset.backgroundColor)) { dataset.backgroundColor = 'white'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/call-to-action/calltoaction-renderer.ts` around lines 52 - 60, The regex used to validate dataset.buttonColor is unanchored around the alternation, allowing values like 'accent" onclick="...' to pass and enabling attribute injection; update the validation in ctaCardTemplate to use a single anchored grouped alternation (e.g., require the whole string to match either a simple token or a hex color) and keep the existing fallback to 'accent' when invalid, then mirror the same correction for dataset.backgroundColor validation elsewhere; specifically fix the validation logic that assigns dataset.buttonColor and dataset.backgroundColor so they use a grouped/anchored pattern (ensuring the full string is matched) before using the value in class or style interpolation.
🟡 Minor comments (14)
packages/kg-default-nodes/src/utils/truncate.ts-3-5 (1)
3-5:⚠️ Potential issue | 🟡 MinorGuard non-positive length inputs to avoid malformed truncation output.
At Line 5 and in
truncateHtml, non-positivemaxLength/maxLengthMobilecan lead to unintended substring behavior and output that violates expected max-length semantics.💡 Proposed fix
export function truncateText(text: string, maxLength: number) { + if (maxLength <= 0) { + return ''; + } if (text && text.length > maxLength) { return text.substring(0, maxLength - 1).trim() + '\u2026'; } else { return text ?? ''; } } export function truncateHtml(text: string, maxLength: number, maxLengthMobile?: number) { + if (maxLength <= 0) { + return ''; + } + if (typeof maxLengthMobile === 'number' && maxLengthMobile <= 0) { + return escapeHtml(truncateText(text, maxLength)); + } // If no mobile length specified or mobile length is larger than desktop, // just do a simple truncate if (!maxLengthMobile || maxLength <= maxLengthMobile) { return escapeHtml(truncateText(text, maxLength)); }Also applies to: 11-15
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/utils/truncate.ts` around lines 3 - 5, Guard against non-positive length inputs in truncateText and truncateHtml by returning an empty string when maxLength (or maxLengthMobile) is <= 0 to avoid calling substring with a negative length or producing malformed output; add an early check in truncateText (parameter maxLength) and in truncateHtml (parameter maxLengthMobile and any other maxLength usages) that returns '' for non-positive values, otherwise keep the existing substring/ellipsis logic (or use Math.max(0, maxLength - 1) if you prefer a defensive clamp before substring).packages/kg-default-nodes/src/utils/rgb-to-hex.ts-8-12 (1)
8-12:⚠️ Potential issue | 🟡 MinorGood null check, but incomplete validation for malformed RGB strings.
The null check handles the case where no digits are found, but a malformed string like
"rgb(255)"would pass this check and result ingandbbeingundefined, producing invalid hex output like"#ffNaNNaN".🛡️ Proposed fix to validate component count
const match = rgb.match(/\d+/g); - if (!match) { + if (!match || match.length < 3) { return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/utils/rgb-to-hex.ts` around lines 8 - 12, The current null check after const match = rgb.match(/\d+/g) only ensures digits exist but doesn't validate the count, so malformed inputs like "rgb(255)" yield undefined g/b; update the rgb-to-hex validation to verify match.length >= 3 before destructuring [r, g, b] (and return null on failure), and ideally coerce/validate each component is a number within 0–255 (in the function handling conversion, e.g., the rgbToHex helper that uses match and [r,g,b]) so you never produce NaN or invalid hex strings.packages/kg-default-nodes/src/utils/visibility.ts-43-49 (1)
43-49:⚠️ Potential issue | 🟡 MinorPotential runtime error from non-null assertions when
weborThe
isOldVisibilityFormatfunction uses non-null assertions (visibility.web!,visibility.email!) on lines 46-48, but these are only safe ifvisibility.webandvisibility.emailexist. The function checks for their presence usinghasOwnPropertyon lines 44-45, but if either check fails, the function should short-circuit and returntruebefore reaching the assertions.However, due to the
||chain, ifvisibility.webexists butvisibility.emaildoesn't, line 48 (visibility.email!.memberSegment) will throw.🐛 Proposed fix using optional chaining
export function isOldVisibilityFormat(visibility: Visibility) { return !Object.prototype.hasOwnProperty.call(visibility, 'web') || !Object.prototype.hasOwnProperty.call(visibility, 'email') - || !Object.prototype.hasOwnProperty.call(visibility.web!, 'nonMember') - || isNullish(visibility.web!.memberSegment) - || isNullish(visibility.email!.memberSegment); + || !Object.prototype.hasOwnProperty.call(visibility.web, 'nonMember') + || isNullish(visibility.web?.memberSegment) + || isNullish(visibility.email?.memberSegment); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/utils/visibility.ts` around lines 43 - 49, The function isOldVisibilityFormat uses non-null assertions (visibility.web! and visibility.email!) that can still throw when one of those props is missing; change the checks to safely access nested props with optional chaining and/or reorder to short-circuit: keep the hasOwnProperty checks for 'web' and 'email' and replace uses of visibility.web! and visibility.email! with visibility.web? and visibility.email? (e.g., check hasOwnProperty.call(visibility.web, 'nonMember') replaced or guarded and use isNullish(visibility.web?.memberSegment) and isNullish(visibility.email?.memberSegment)) so the function returns true when web/email are absent without dereferencing undefined in isOldVisibilityFormat.packages/kg-default-nodes/src/nodes/image/ImageNode.ts-4-13 (1)
4-13:⚠️ Potential issue | 🟡 MinorType mismatch:
width/heightare nullable here but non-nullable inImageNodeData.The
ImageNodeinterface defineswidthandheightasnumber | null, butImageNodeDatainimage-renderer.ts(lines 10-11) defines them asnumber. This inconsistency can lead to type errors when passingImageNodedata to the renderer, since the renderer assumes non-null values.Consider aligning these types—either make
ImageNodeData.width/heightnullable, or ensure the renderer handles null values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/image/ImageNode.ts` around lines 4 - 13, The types for image dimensions are inconsistent: ImageNode declares width and height as number | null while ImageNodeData (in image-renderer.ts) declares them as number; align them by making ImageNodeData.width and ImageNodeData.height nullable (number | null) or update the renderer to handle nulls before using ImageNodeData (e.g., provide fallback values or conditional logic). Locate the ImageNode interface and the ImageNodeData type in image-renderer.ts and choose one approach—either change ImageNodeData to number | null to match ImageNode, or add null-checks/defaults in the renderer where ImageNodeData.width/height are accessed.packages/kg-default-nodes/src/nodes/image/image-renderer.ts-132-133 (1)
132-133:⚠️ Potential issue | 🟡 MinorNon-null assertion on regex match risks runtime error.
If
node.srcdoesn't match the expected pattern/(.*\/content\/images)\/(.*)/, the!assertion will cause a runtime error when destructuring. WhileisLocalContentImageat line 124 provides some validation, it uses a different regex pattern that may not guarantee this match succeeds.🛡️ Proposed defensive check
- const [, imagesPath, filename] = node.src.match(/(.*\/content\/images)\/(.*)/)!; - img.setAttribute('src', `${imagesPath}/size/w${srcWidth}/${filename}`); + const match = node.src.match(/(.*\/content\/images)\/(.*)/); + if (match) { + const [, imagesPath, filename] = match; + img.setAttribute('src', `${imagesPath}/size/w${srcWidth}/${filename}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/image/image-renderer.ts` around lines 132 - 133, The destructuring uses a non-null assertion on node.src.match(...) which can crash if the regex doesn't match; replace it by storing the match result in a variable (e.g., const m = node.src.match(/(.*\/content\/images)\/(.*)/)) and add a defensive check (if (!m) { handle gracefully — e.g., skip size rewrite, log a warning, or leave img.src as node.src }) before using m[1]/m[2]; update the code around the image-rendering logic (the lines that set img.setAttribute('src', ...)) to use the validated match parts so no runtime error occurs when the pattern isn't present.packages/kg-default-nodes/test/serializers/paragraph.test.ts-8-8 (1)
8-8:⚠️ Potential issue | 🟡 MinorDescribe block name doesn't match filename.
The describe block says
'Serializers: linebreak'but the file is namedparagraph.test.ts. This mismatch could cause confusion. Consider renaming either the describe block to match the filename or vice versa.Suggested fix
-describe('Serializers: linebreak', function () { +describe('Serializers: paragraph', function () {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/test/serializers/paragraph.test.ts` at line 8, The describe block string 'Serializers: linebreak' does not match the test filename paragraph.test.ts; rename the describe block to 'Serializers: paragraph' (or rename the file to match if intended) so the describe label and filename are consistent; update the string in the describe(...) call to match paragraph.test.ts and run tests to confirm no other references need changing.packages/kg-default-nodes/src/utils/tagged-template-fns.ts-8-10 (1)
8-10:⚠️ Potential issue | 🟡 MinorPreserve falsy interpolation values.
Current interpolation on Line 9 converts
0/falseto empty string. Use nullish coalescing instead.🐛 Proposed fix
- const result = strings.reduce((acc: string, str: string, i: number) => { - return acc + str + (values[i] || ''); + const result = strings.reduce((acc: string, str: string, i: number) => { + return acc + str + String(values[i] ?? ''); }, '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/utils/tagged-template-fns.ts` around lines 8 - 10, The interpolation reducer in the tagged-template function uses (values[i] || '') which drops falsy but valid values like 0 or false; update the reduction logic in the strings.reduce callback (the expression that computes result) to use nullish coalescing (values[i] ?? '') so null/undefined become empty strings while preserving 0 and false.packages/kg-default-nodes/src/nodes/embed/types/twitter.ts-79-83 (1)
79-83:⚠️ Potential issue | 🟡 MinorNon-null assertions on optional fields may cause runtime errors.
public_metricsandcreated_atare typed as optional inTweetData, but the code uses!to assert they exist. If Twitter API returns incomplete data, this will throw at runtime.Consider adding defensive checks or providing fallback values:
🛡️ Suggested defensive handling
- const retweetCount = numberFormatter.format(tweetData.public_metrics!.retweet_count); - const likeCount = numberFormatter.format(tweetData.public_metrics!.like_count); + const retweetCount = numberFormatter.format(tweetData.public_metrics?.retweet_count ?? 0); + const likeCount = numberFormatter.format(tweetData.public_metrics?.like_count ?? 0); const authorUser = tweetData.users && tweetData.users.find((user: TwitterUser) => user.id === tweetData.author_id); - const tweetTime = DateTime.fromISO(tweetData.created_at!).toLocaleString(DateTime.TIME_SIMPLE); - const tweetDate = DateTime.fromISO(tweetData.created_at!).toLocaleString(DateTime.DATE_MED); + const tweetTime = tweetData.created_at ? DateTime.fromISO(tweetData.created_at).toLocaleString(DateTime.TIME_SIMPLE) : ''; + const tweetDate = tweetData.created_at ? DateTime.fromISO(tweetData.created_at).toLocaleString(DateTime.DATE_MED) : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/embed/types/twitter.ts` around lines 79 - 83, The code uses non-null assertions on tweetData.public_metrics and tweetData.created_at when computing retweetCount, likeCount, tweetTime and tweetDate; replace these assertions with defensive checks: verify tweetData.public_metrics exists before formatting retweet_count/like_count and provide default values (e.g., 0 or "0") for retweetCount/likeCount, and verify tweetData.created_at is present before calling DateTime.fromISO, providing a fallback string (e.g., empty or "Unknown date") for tweetTime/tweetDate; also keep the authorUser lookup as-is but handle a potential undefined authorUser when later used. Ensure you update the logic around retweetCount, likeCount, tweetTime, and tweetDate to use optional chaining and defaults instead of the ! operator.packages/kg-default-nodes/src/nodes/embed/types/twitter.ts-92-94 (1)
92-94:⚠️ Potential issue | 🟡 MinorNon-null assertion on
includesmay fail if attachments exist but media is missing.The condition checks
tweetData.attachments?.media_keysbut then assumestweetData.includes!.media[0]exists. Ifincludesis undefined ormediais empty, this will throw.🛡️ Suggested defensive handling
let tweetImageUrl = null; const hasImageOrVideo = tweetData.attachments && tweetData.attachments && tweetData.attachments.media_keys; - if (hasImageOrVideo) { - tweetImageUrl = tweetData.includes!.media[0].preview_image_url || tweetData.includes!.media[0].url; + if (hasImageOrVideo && tweetData.includes?.media?.[0]) { + tweetImageUrl = tweetData.includes.media[0].preview_image_url || tweetData.includes.media[0].url; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/embed/types/twitter.ts` around lines 92 - 94, The code assigns tweetImageUrl using tweetData.includes!.media[0] while only checking tweetData.attachments; update the check and assignment around hasImageOrVideo and tweetImageUrl to defensively verify that tweetData.includes exists and includes.media is a non-empty array (and ideally that a media entry matching an attachments.media_keys exists) before accessing media[0]; change references in this block (hasImageOrVideo, tweetImageUrl, and the tweetData.includes usage) to use optional chaining and length checks or lookup by media_key so you never dereference includes or media when they are undefined/empty.packages/kg-default-nodes/test/nodes/file.test.ts-67-68 (1)
67-68:⚠️ Potential issue | 🟡 MinorType mismatch:
fileSizeis declared asstringbut should benumberfor consistency.The
FileNodeinterface declaresfileSize: string(line 11 of FileNode.ts), yet thefile-rendererinterface expectsfileSize: number(line 11 of file-renderer.ts). The test assignment at line 67 uses a double castas unknown as stringto bypass type checking and assign a number, then asserts it equals the number123456at line 68. Additionally, FileNode.ts line 46 converts fileSize to a number before use (Number(this.fileSize)). This type inconsistency should be resolved by either changing the FileNode interface to declarefileSize: numberor ensuring all usages treat it consistently as a string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/test/nodes/file.test.ts` around lines 67 - 68, The test is forcing a number into FileNode.fileSize despite FileNode declaring fileSize as string; make fileSize consistently a number across the codebase by changing the FileNode interface/implementation to declare fileSize: number (update any constructors or parsing logic that currently converts via Number(this.fileSize)), update file-renderer expectations if necessary, and adjust the test in file.test.ts to assign 123456 without casts and assert equality; ensure all uses of FileNode.fileSize (e.g., in FileNode class methods and file-renderer) treat it as a number so no runtime Number(...) conversions are required.packages/kg-default-nodes/src/nodes/file/FileNode.ts-6-12 (1)
6-12:⚠️ Potential issue | 🟡 MinorType inconsistency:
fileSizedeclared asstringbut used asnumberthroughout.The interface declares
fileSize: string, but the actual usage suggests it should benumber:
formattedFileSizecallsNumber(this.fileSize)(line 46)- Tests assign numeric values
- The property represents byte count, which is naturally numeric
Consider updating the type to
numberornumber | stringto match actual usage.♻️ Suggested interface update
export interface FileNode { src: string; fileTitle: string; fileCaption: string; fileName: string; - fileSize: string; + fileSize: number; }Note: This would also require updating the property default from
''to0ornull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/file/FileNode.ts` around lines 6 - 12, The FileNode interface incorrectly types fileSize as string while code (formattedFileSize) and tests treat it as a numeric byte count; change the FileNode declaration to use fileSize: number (or number | string if you need backward compatibility), update any default initialization from '' to 0 or null, and adjust related constructors/assignments and tests to pass numeric values; ensure formattedFileSize no longer needs Number(this.fileSize) if you make it a number and update any places that assumed a string.packages/kg-default-nodes/src/nodes/header/renderers/v2/header-renderer.ts-237-239 (1)
237-239:⚠️ Potential issue | 🟡 MinorRedundant
createDocument!()call.A
documentis already created at Line 215. Creating anotheremailDocis unnecessary overhead.🔧 Proposed fix
if (options.target === 'email') { - const emailDoc = options.createDocument!(); - const emailDiv = emailDoc.createElement('div'); + const emailDiv = document.createElement('div'); emailDiv.innerHTML = emailTemplate(node, options)?.trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/header/renderers/v2/header-renderer.ts` around lines 237 - 239, The new call to options.createDocument!() is redundant because this renderer already creates a document earlier; replace usage of the newly introduced emailDoc with the existing document variable (the document created earlier in this renderer) when target === 'email', i.e. create emailDiv via the existing document.createElement instead of calling options.createDocument(), and remove the emailDoc variable and its createDocument invocation (adjust references to emailDiv accordingly); keep using options.target and options.createDocument where needed only for initial document creation.packages/kg-default-nodes/src/nodes/TKNode.ts-17-20 (1)
17-20:⚠️ Potential issue | 🟡 MinorFilter empty theme class tokens before
classList.add().
config.theme.tk?.split(' ')produces empty strings for cases like''or'a b', andclassList.add('')throws aSyntaxErrorper the DOM specification.packages/kg-default-nodes/src/nodes/at-link/AtLinkNode.tsalready filters these tokens (lines 42-43), soTKNodeshould do the same.Suggested fix
- const classes = config.theme.tk?.split(' ') || []; - element.classList.add(...classes); + const classes = config.theme.tk?.split(' ').filter(Boolean) ?? []; + element.classList.add(...classes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/TKNode.ts` around lines 17 - 20, The TKNode.createDOM method currently builds classes with config.theme.tk?.split(' ') then calls element.classList.add(...classes), which can pass empty strings and throw; update createDOM (TKNode.createDOM) to filter out empty tokens from the split (e.g., use .filter(Boolean) or equivalent) before calling element.classList.add so only non-empty class names are added to the element.packages/kg-default-nodes/src/nodes/audio/audio-parser.ts-26-30 (1)
26-30:⚠️ Potential issue | 🟡 MinorReject malformed durations instead of storing
NaN.The
catchblock here won't catchparseInt()failures with string inputs—it silently returnsNaNinstead. Malformed input like1:xxwill assignNaNtopayload.duration. Validate that both parsed values are numbers before assigning.Suggested fix
if (durationText) { const [minutes, seconds = '0'] = durationText.split(':'); - try { - payload.duration = parseInt(minutes) * 60 + parseInt(seconds); - } catch { - // ignore duration - } + const parsedMinutes = Number.parseInt(minutes, 10); + const parsedSeconds = Number.parseInt(seconds, 10); + + if (!Number.isNaN(parsedMinutes) && !Number.isNaN(parsedSeconds)) { + payload.duration = parsedMinutes * 60 + parsedSeconds; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/audio/audio-parser.ts` around lines 26 - 30, The current parsing of durationText (in the audio parsing logic where durationText is split and parsed) can yield NaN because parseInt does not throw; replace the try/catch approach by explicitly validating the numeric results: split durationText into minutes and seconds, parse them, then check both parsed values with Number.isFinite (or !Number.isNaN) before assigning payload.duration; if validation fails, do not set payload.duration (or remove it) and handle the malformed input path instead of assigning NaN. Ensure you update the code around the durationText handling and payload.duration assignment (the block that destructures [minutes, seconds = '0'] and sets payload.duration).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4d28d24-3b2a-4673-8324-4dde83b5928c
⛔ Files ignored due to path filters (1)
packages/kg-default-nodes/src/nodes/at-link/kg-link.svgis excluded by!**/*.svg
📒 Files selected for processing (161)
packages/kg-default-nodes/eslint.config.mjspackages/kg-default-nodes/index.jspackages/kg-default-nodes/lib/kg-default-nodes.jspackages/kg-default-nodes/lib/nodes/at-link/index.jspackages/kg-default-nodes/lib/nodes/codeblock/codeblock-parser.jspackages/kg-default-nodes/lib/nodes/horizontalrule/horizontalrule-parser.jspackages/kg-default-nodes/lib/nodes/horizontalrule/horizontalrule-renderer.jspackages/kg-default-nodes/lib/nodes/html/html-parser.jspackages/kg-default-nodes/lib/nodes/markdown/markdown-renderer.jspackages/kg-default-nodes/lib/nodes/paywall/paywall-parser.jspackages/kg-default-nodes/lib/utils/is-unsplash-image.jspackages/kg-default-nodes/package.jsonpackages/kg-default-nodes/rollup.config.mjspackages/kg-default-nodes/src/KoenigDecoratorNode.tspackages/kg-default-nodes/src/generate-decorator-node.tspackages/kg-default-nodes/src/index.tspackages/kg-default-nodes/src/kg-default-nodes.tspackages/kg-default-nodes/src/nodes/ExtendedHeadingNode.tspackages/kg-default-nodes/src/nodes/ExtendedQuoteNode.tspackages/kg-default-nodes/src/nodes/ExtendedTextNode.tspackages/kg-default-nodes/src/nodes/TKNode.tspackages/kg-default-nodes/src/nodes/aside/AsideNode.tspackages/kg-default-nodes/src/nodes/aside/AsideParser.tspackages/kg-default-nodes/src/nodes/at-link/AtLinkNode.tspackages/kg-default-nodes/src/nodes/at-link/AtLinkSearchNode.tspackages/kg-default-nodes/src/nodes/at-link/index.tspackages/kg-default-nodes/src/nodes/audio/AudioNode.tspackages/kg-default-nodes/src/nodes/audio/audio-parser.tspackages/kg-default-nodes/src/nodes/audio/audio-renderer.tspackages/kg-default-nodes/src/nodes/bookmark/BookmarkNode.tspackages/kg-default-nodes/src/nodes/bookmark/bookmark-parser.tspackages/kg-default-nodes/src/nodes/bookmark/bookmark-renderer.tspackages/kg-default-nodes/src/nodes/button/ButtonNode.tspackages/kg-default-nodes/src/nodes/button/button-parser.tspackages/kg-default-nodes/src/nodes/button/button-renderer.tspackages/kg-default-nodes/src/nodes/call-to-action/CallToActionNode.tspackages/kg-default-nodes/src/nodes/call-to-action/calltoaction-parser.tspackages/kg-default-nodes/src/nodes/call-to-action/calltoaction-renderer.tspackages/kg-default-nodes/src/nodes/callout/CalloutNode.tspackages/kg-default-nodes/src/nodes/callout/callout-parser.tspackages/kg-default-nodes/src/nodes/callout/callout-renderer.tspackages/kg-default-nodes/src/nodes/codeblock/CodeBlockNode.tspackages/kg-default-nodes/src/nodes/codeblock/codeblock-parser.tspackages/kg-default-nodes/src/nodes/codeblock/codeblock-renderer.tspackages/kg-default-nodes/src/nodes/email-cta/EmailCtaNode.tspackages/kg-default-nodes/src/nodes/email-cta/email-cta-renderer.tspackages/kg-default-nodes/src/nodes/email/EmailNode.tspackages/kg-default-nodes/src/nodes/email/email-renderer.tspackages/kg-default-nodes/src/nodes/embed/EmbedNode.tspackages/kg-default-nodes/src/nodes/embed/embed-parser.tspackages/kg-default-nodes/src/nodes/embed/embed-renderer.tspackages/kg-default-nodes/src/nodes/embed/types/twitter.tspackages/kg-default-nodes/src/nodes/file/FileNode.tspackages/kg-default-nodes/src/nodes/file/file-parser.tspackages/kg-default-nodes/src/nodes/file/file-renderer.tspackages/kg-default-nodes/src/nodes/gallery/GalleryNode.tspackages/kg-default-nodes/src/nodes/gallery/gallery-parser.tspackages/kg-default-nodes/src/nodes/gallery/gallery-renderer.tspackages/kg-default-nodes/src/nodes/header/HeaderNode.tspackages/kg-default-nodes/src/nodes/header/parsers/header-parser.tspackages/kg-default-nodes/src/nodes/header/renderers/v1/header-renderer.tspackages/kg-default-nodes/src/nodes/header/renderers/v2/header-renderer.tspackages/kg-default-nodes/src/nodes/horizontalrule/HorizontalRuleNode.tspackages/kg-default-nodes/src/nodes/horizontalrule/horizontalrule-parser.tspackages/kg-default-nodes/src/nodes/horizontalrule/horizontalrule-renderer.tspackages/kg-default-nodes/src/nodes/html/HtmlNode.tspackages/kg-default-nodes/src/nodes/html/html-parser.tspackages/kg-default-nodes/src/nodes/html/html-renderer.tspackages/kg-default-nodes/src/nodes/image/ImageNode.tspackages/kg-default-nodes/src/nodes/image/image-parser.tspackages/kg-default-nodes/src/nodes/image/image-renderer.tspackages/kg-default-nodes/src/nodes/markdown/MarkdownNode.tspackages/kg-default-nodes/src/nodes/markdown/markdown-renderer.tspackages/kg-default-nodes/src/nodes/paywall/PaywallNode.tspackages/kg-default-nodes/src/nodes/paywall/paywall-parser.tspackages/kg-default-nodes/src/nodes/paywall/paywall-renderer.tspackages/kg-default-nodes/src/nodes/product/ProductNode.tspackages/kg-default-nodes/src/nodes/product/product-parser.tspackages/kg-default-nodes/src/nodes/product/product-renderer.tspackages/kg-default-nodes/src/nodes/signup/SignupNode.tspackages/kg-default-nodes/src/nodes/signup/signup-parser.tspackages/kg-default-nodes/src/nodes/signup/signup-renderer.tspackages/kg-default-nodes/src/nodes/toggle/ToggleNode.tspackages/kg-default-nodes/src/nodes/toggle/toggle-parser.tspackages/kg-default-nodes/src/nodes/toggle/toggle-renderer.tspackages/kg-default-nodes/src/nodes/transistor/TransistorNode.tspackages/kg-default-nodes/src/nodes/transistor/transistor-renderer.tspackages/kg-default-nodes/src/nodes/video/VideoNode.tspackages/kg-default-nodes/src/nodes/video/video-parser.tspackages/kg-default-nodes/src/nodes/video/video-renderer.tspackages/kg-default-nodes/src/nodes/zwnj/ZWNJNode.tspackages/kg-default-nodes/src/serializers/linebreak.tspackages/kg-default-nodes/src/serializers/paragraph.tspackages/kg-default-nodes/src/svg.d.tspackages/kg-default-nodes/src/utils/add-create-document-option.tspackages/kg-default-nodes/src/utils/build-clean-basic-html-for-element.tspackages/kg-default-nodes/src/utils/clean-dom.tspackages/kg-default-nodes/src/utils/escape-html.tspackages/kg-default-nodes/src/utils/get-available-image-widths.tspackages/kg-default-nodes/src/utils/get-resized-image-dimensions.tspackages/kg-default-nodes/src/utils/is-local-content-image.tspackages/kg-default-nodes/src/utils/is-unsplash-image.tspackages/kg-default-nodes/src/utils/read-caption-from-element.tspackages/kg-default-nodes/src/utils/read-image-attributes-from-element.tspackages/kg-default-nodes/src/utils/read-text-content.tspackages/kg-default-nodes/src/utils/render-empty-container.tspackages/kg-default-nodes/src/utils/render-helpers/email-button.tspackages/kg-default-nodes/src/utils/replacement-strings.tspackages/kg-default-nodes/src/utils/rgb-to-hex.tspackages/kg-default-nodes/src/utils/set-src-background-from-parent.tspackages/kg-default-nodes/src/utils/size-byte-converter.tspackages/kg-default-nodes/src/utils/slugify.tspackages/kg-default-nodes/src/utils/srcset-attribute.tspackages/kg-default-nodes/src/utils/tagged-template-fns.tspackages/kg-default-nodes/src/utils/truncate.tspackages/kg-default-nodes/src/utils/visibility.tspackages/kg-default-nodes/test/generate-decorator-node.test.tspackages/kg-default-nodes/test/nodes/aside.test.tspackages/kg-default-nodes/test/nodes/at-link-search.test.tspackages/kg-default-nodes/test/nodes/at-link.test.tspackages/kg-default-nodes/test/nodes/audio.test.tspackages/kg-default-nodes/test/nodes/bookmark.test.tspackages/kg-default-nodes/test/nodes/button.test.tspackages/kg-default-nodes/test/nodes/call-to-action.test.tspackages/kg-default-nodes/test/nodes/callout.test.tspackages/kg-default-nodes/test/nodes/codeblock.test.tspackages/kg-default-nodes/test/nodes/email-cta.test.tspackages/kg-default-nodes/test/nodes/email.test.tspackages/kg-default-nodes/test/nodes/embed.test.tspackages/kg-default-nodes/test/nodes/file.test.tspackages/kg-default-nodes/test/nodes/gallery.test.tspackages/kg-default-nodes/test/nodes/header.test.tspackages/kg-default-nodes/test/nodes/horizontalrule.test.tspackages/kg-default-nodes/test/nodes/html.test.tspackages/kg-default-nodes/test/nodes/image.test.tspackages/kg-default-nodes/test/nodes/markdown.test.tspackages/kg-default-nodes/test/nodes/paywall.test.tspackages/kg-default-nodes/test/nodes/product.test.tspackages/kg-default-nodes/test/nodes/signup.test.tspackages/kg-default-nodes/test/nodes/tk.test.tspackages/kg-default-nodes/test/nodes/toggle.test.tspackages/kg-default-nodes/test/nodes/transistor.test.tspackages/kg-default-nodes/test/nodes/video.test.tspackages/kg-default-nodes/test/nodes/zwnj.test.tspackages/kg-default-nodes/test/serializers/linebreak.test.tspackages/kg-default-nodes/test/serializers/paragraph.test.tspackages/kg-default-nodes/test/test-utils/assertions.jspackages/kg-default-nodes/test/test-utils/assertions.tspackages/kg-default-nodes/test/test-utils/html-minifier.d.tspackages/kg-default-nodes/test/test-utils/index.jspackages/kg-default-nodes/test/test-utils/index.tspackages/kg-default-nodes/test/test-utils/overrides.jspackages/kg-default-nodes/test/test-utils/overrides.tspackages/kg-default-nodes/test/test-utils/should-assertions.d.tspackages/kg-default-nodes/test/test-utils/should.d.tspackages/kg-default-nodes/test/utils/rgb-to-hex.test.tspackages/kg-default-nodes/test/utils/tagged-template-fns.test.tspackages/kg-default-nodes/test/utils/visibility.test.tspackages/kg-default-nodes/tsconfig.cjs.jsonpackages/kg-default-nodes/tsconfig.jsonpackages/kg-default-nodes/tsconfig.test.json
💤 Files with no reviewable changes (14)
- packages/kg-default-nodes/index.js
- packages/kg-default-nodes/lib/nodes/at-link/index.js
- packages/kg-default-nodes/lib/nodes/horizontalrule/horizontalrule-parser.js
- packages/kg-default-nodes/lib/nodes/markdown/markdown-renderer.js
- packages/kg-default-nodes/lib/nodes/paywall/paywall-parser.js
- packages/kg-default-nodes/lib/nodes/horizontalrule/horizontalrule-renderer.js
- packages/kg-default-nodes/lib/utils/is-unsplash-image.js
- packages/kg-default-nodes/lib/nodes/html/html-parser.js
- packages/kg-default-nodes/rollup.config.mjs
- packages/kg-default-nodes/test/test-utils/overrides.js
- packages/kg-default-nodes/test/test-utils/index.js
- packages/kg-default-nodes/test/test-utils/assertions.js
- packages/kg-default-nodes/lib/nodes/codeblock/codeblock-parser.js
- packages/kg-default-nodes/lib/kg-default-nodes.js
| if (options.target === 'email') { | ||
| const emailDoc = options.createDocument(); | ||
| const emailDoc = options.createDocument!(); | ||
| const emailDiv = emailDoc.createElement('div'); | ||
|
|
||
| emailDiv.innerHTML = emailCTATemplate(dataset, options); | ||
|
|
||
| return renderWithVisibility({element: emailDiv.firstElementChild}, node.visibility, options); | ||
| return renderWithVisibility({element: emailDiv.firstElementChild as RenderOutput['element']}, node.visibility as Visibility, options); |
There was a problem hiding this comment.
Sanitize sponsorLabel before the email early-return.
Lines 412-416 only clean the label on the web path. When options.target === 'email', the function returns at Line 407 with raw dataset.sponsorLabel still interpolated into emailCTATemplate().
Suggested fix
+ if (dataset.hasSponsorLabel) {
+ const cleanBasicHtml = buildCleanBasicHtmlForElement(document.createElement('div'));
+ const cleanedHtml = cleanBasicHtml(dataset.sponsorLabel, {firstChildInnerContent: true});
+ dataset.sponsorLabel = cleanedHtml || '';
+ }
+
if (options.target === 'email') {
const emailDoc = options.createDocument!();
const emailDiv = emailDoc.createElement('div');
@@
- if (dataset.hasSponsorLabel) {
- const cleanBasicHtml = buildCleanBasicHtmlForElement(element);
- const cleanedHtml = cleanBasicHtml(dataset.sponsorLabel, {firstChildInnerContent: true});
- dataset.sponsorLabel = cleanedHtml || '';
- }
const htmlString = ctaCardTemplate(dataset);Also applies to: 412-416
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 404-404: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: emailDiv.innerHTML = emailCTATemplate(dataset, options)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 404-404: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: emailDiv.innerHTML = emailCTATemplate(dataset, options)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/src/nodes/call-to-action/calltoaction-renderer.ts`
around lines 401 - 407, The email branch returns early but uses raw
dataset.sponsorLabel, so sanitize the sponsor label before that early return:
ensure you normalize/clean dataset.sponsorLabel the same way as the web path
does (the sanitation logic currently applied near the web rendering code), then
pass the cleaned value into emailCTATemplate(dataset, options) so the email path
uses the sanitized sponsorLabel; reference the dataset.sponsorLabel field, the
emailCTATemplate(...) call, and the renderWithVisibility(...) return path in
calltoaction-renderer.ts to locate and apply the change.
| ${node.fileTitle ? ` | ||
| <table cellspacing="0" cellpadding="0" border="0" width="100%"><tr><td> | ||
| <a href="${escapeHtml(options.postUrl)}" class="kg-file-title">${escapeHtml(node.fileTitle)}</a> | ||
| <a href="${escapeHtml(options.postUrl || '')}" class="kg-file-title">${escapeHtml(node.fileTitle)}</a> | ||
| </td></tr></table> | ||
| ` : ``} | ||
| ${node.fileCaption ? ` | ||
| <table cellspacing="0" cellpadding="0" border="0" width="100%"><tr><td> | ||
| <a href="${escapeHtml(options.postUrl)}" class="kg-file-description">${escapeHtml(node.fileCaption)}</a> | ||
| <a href="${escapeHtml(options.postUrl || '')}" class="kg-file-description">${escapeHtml(node.fileCaption)}</a> | ||
| </td></tr></table> | ||
| ` : ``} | ||
| <table cellspacing="0" cellpadding="0" border="0" width="100%"><tr><td> | ||
| <a href="${escapeHtml(options.postUrl)}" class="kg-file-meta"><span class="kg-file-name">${escapeHtml(node.fileName)}</span> • ${bytesToSize(node.fileSize)}</a> | ||
| <a href="${escapeHtml(options.postUrl || '')}" class="kg-file-meta"><span class="kg-file-name">${escapeHtml(node.fileName)}</span> • ${bytesToSize(node.fileSize)}</a> | ||
| </td></tr></table> | ||
| </td> | ||
| <td width="80" valign="middle" class="kg-file-thumbnail"> | ||
| <a href="${escapeHtml(options.postUrl)}" style="display: block; top: 0; right: 0; bottom: 0; left: 0;"> | ||
| <a href="${escapeHtml(options.postUrl || '')}" style="display: block; top: 0; right: 0; bottom: 0; left: 0;"> | ||
| <img src="https://static.ghost.org/v4.0.0/images/download-icon-darkmode.png" style="${escapeHtml(iconCls)}"> |
There was a problem hiding this comment.
Avoid emitting empty links when postUrl is missing.
postUrl is optional in RenderOptions, but every email anchor now renders href="" when it is not provided. That leaves the card visually clickable while every click target is broken; fall back to node.src or skip the <a> entirely instead.
Possible fix
function emailTemplate(node: FileNodeData, document: Document, options: RenderOptions) {
+ const href = escapeHtml(options.postUrl || node.src);
+
let iconCls;
@@
- <a href="${escapeHtml(options.postUrl || '')}" class="kg-file-title">${escapeHtml(node.fileTitle)}</a>
+ <a href="${href}" class="kg-file-title">${escapeHtml(node.fileTitle)}</a>
@@
- <a href="${escapeHtml(options.postUrl || '')}" class="kg-file-description">${escapeHtml(node.fileCaption)}</a>
+ <a href="${href}" class="kg-file-description">${escapeHtml(node.fileCaption)}</a>
@@
- <a href="${escapeHtml(options.postUrl || '')}" class="kg-file-meta"><span class="kg-file-name">${escapeHtml(node.fileName)}</span> • ${bytesToSize(node.fileSize)}</a>
+ <a href="${href}" class="kg-file-meta"><span class="kg-file-name">${escapeHtml(node.fileName)}</span> • ${bytesToSize(node.fileSize)}</a>
@@
- <a href="${escapeHtml(options.postUrl || '')}" style="display: block; top: 0; right: 0; bottom: 0; left: 0;">
+ <a href="${href}" style="display: block; top: 0; right: 0; bottom: 0; left: 0;">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/src/nodes/file/file-renderer.ts` around lines 53 -
69, The anchors in file-renderer.ts always use options.postUrl which may be
empty, producing empty hrefs; update the rendering logic in the file rendering
block (where kg-file-title, kg-file-description, kg-file-meta and
kg-file-thumbnail anchors are created) to use options.postUrl || node.src as the
href and, if both are missing, render the inner content without an <a> wrapper
(i.e., skip emitting the anchor entirely); ensure you still escape the chosen
URL with escapeHtml and preserve bytesToSize and existing classes/styles when
wrapping or not wrapping the content.
| if (isLocalContentImage(image.src, options.siteUrl) && options.canTransformImage && options.canTransformImage(image.src)) { | ||
| // find available image size next up from 2x600 so we can use it for the "retina" src | ||
| const availableImageWidths = getAvailableImageWidths(image, options.imageOptimization.contentImageSizes); | ||
| const availableImageWidths = getAvailableImageWidths(image, options.imageOptimization!.contentImageSizes!); | ||
| const srcWidth = availableImageWidths.find(width => width >= 1200); | ||
|
|
||
| if (!srcWidth || srcWidth === image.width) { | ||
| // do nothing, width is smaller than retina or matches the original payload src | ||
| } else { | ||
| const [, imagesPath, filename] = image.src.match(/(.*\/content\/images)\/(.*)/); | ||
| const [, imagesPath, filename] = image.src.match(/(.*\/content\/images)\/(.*)/)!; | ||
| img.setAttribute('src', `${imagesPath}/size/w${srcWidth}/${filename}`); |
There was a problem hiding this comment.
Guard contentImageSizes before the retina-src rewrite.
imageOptimization and contentImageSizes are optional in GalleryRenderOptions, but this branch only checks canTransformImage. With a caller that enables transforms without supplying the size map, the new non-null assertion becomes a runtime throw during email rendering.
Possible fix
- if (isLocalContentImage(image.src, options.siteUrl) && options.canTransformImage && options.canTransformImage(image.src)) {
+ const contentImageSizes = options.imageOptimization?.contentImageSizes;
+ if (contentImageSizes && isLocalContentImage(image.src, options.siteUrl) && options.canTransformImage?.(image.src)) {
// find available image size next up from 2x600 so we can use it for the "retina" src
- const availableImageWidths = getAvailableImageWidths(image, options.imageOptimization!.contentImageSizes!);
+ const availableImageWidths = getAvailableImageWidths(image, contentImageSizes);
const srcWidth = availableImageWidths.find(width => width >= 1200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/src/nodes/gallery/gallery-renderer.ts` around lines
145 - 154, The branch uses options.imageOptimization!.contentImageSizes! without
guarding for imageOptimization/contentImageSizes; update the condition that
currently checks isLocalContentImage(...) && options.canTransformImage &&
options.canTransformImage(image.src) to also require
options.imageOptimization?.contentImageSizes (or options.imageOptimization &&
options.imageOptimization.contentImageSizes) before calling
getAvailableImageWidths in GalleryRenderer (and remove the non-null assertions),
so getAvailableImageWidths(image, options.imageOptimization.contentImageSizes)
is only called when the sizes map exists.
| if (node.caption) { | ||
| let figcaption = document.createElement('figcaption'); | ||
| const figcaption = document.createElement('figcaption'); | ||
| figcaption.innerHTML = node.caption; | ||
| figure.appendChild(figcaption); |
There was a problem hiding this comment.
Sanitize caption before assigning it to innerHTML.
node.caption is card data, and this writes it straight into the DOM as trusted HTML. A malformed or hostile gallery payload can inject arbitrary markup or event handlers from here; sanitize with the same allowlist used elsewhere first, or use textContent if rich markup is not required.
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 180-180: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: figcaption.innerHTML = node.caption
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 180-180: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: figcaption.innerHTML = node.caption
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/src/nodes/gallery/gallery-renderer.ts` around lines
179 - 182, The code assigns node.caption directly to figcaption.innerHTML in
gallery-renderer.ts which allows unsafe HTML; change this to sanitize the
caption before inserting (reuse the existing HTML allowlist/sanitizer used
elsewhere in the project) or, if rich HTML is not required, set
figcaption.textContent = node.caption instead of using innerHTML; locate the
block that creates figcaption (the node.caption check and figcaption.innerHTML
assignment) and replace it with a call to the shared sanitizer utility or
textContent assignment to prevent XSS.
| while (nextNode && !isHtmlEndComment(nextNode)) { | ||
| const currentNode = nextNode; | ||
| html.push((currentNode as Element).outerHTML); | ||
| nextNode = currentNode.nextSibling; | ||
| // remove nodes as we go so that they don't go through the parser | ||
| currentNode.remove(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's an existing test that covers text nodes between HTML comments
rg -n "kg-card-begin.*html" packages/kg-default-nodes/test/Repository: TryGhost/Koenig
Length of output: 1599
🏁 Script executed:
cat -n packages/kg-default-nodes/src/nodes/html/html-parser.tsRepository: TryGhost/Koenig
Length of output: 2417
🏁 Script executed:
head -n 50 packages/kg-default-nodes/test/nodes/html.test.tsRepository: TryGhost/Koenig
Length of output: 1700
🏁 Script executed:
sed -n '155,230p' packages/kg-default-nodes/test/nodes/html.test.tsRepository: TryGhost/Koenig
Length of output: 4392
🏁 Script executed:
grep -n "parseHtmlNode\|#comment\|kg-card-begin" packages/kg-default-nodes/test/nodes/html.test.ts | head -20Repository: TryGhost/Koenig
Length of output: 1149
🏁 Script executed:
sed -n '1,100p' packages/kg-default-nodes/test/nodes/html.test.ts | grep -A 10 "describe\|it("Repository: TryGhost/Koenig
Length of output: 2437
🏁 Script executed:
grep -rn "parseHtmlNode" packages/kg-default-nodes/src/ --include="*.ts"Repository: TryGhost/Koenig
Length of output: 401
🏁 Script executed:
grep -n "parseHtmlNode\|conversion\|kg-card-begin.*test" packages/kg-default-nodes/test/nodes/html.test.ts | head -30Repository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
find packages/kg-default-nodes/test -name "*.test.ts" -exec grep -l "kg-card-begin" {} \;Repository: TryGhost/Koenig
Length of output: 212
🏁 Script executed:
sed -n '150,180p' packages/kg-default-nodes/test/nodes/html.test.tsRepository: TryGhost/Koenig
Length of output: 1338
🏁 Script executed:
grep -rn "generateNodesFromDOM\|kg-card-begin.*html.*conversion" packages/kg-default-nodes/test --include="*.ts" -A 5 -B 5Repository: TryGhost/Koenig
Length of output: 50372
🏁 Script executed:
grep -n "describe\|it(" packages/kg-default-nodes/test/nodes/html.test.ts | tail -50Repository: TryGhost/Koenig
Length of output: 4007
🏁 Script executed:
wc -l packages/kg-default-nodes/test/nodes/html.test.tsRepository: TryGhost/Koenig
Length of output: 112
🏁 Script executed:
sed -n '360,388p' packages/kg-default-nodes/test/nodes/html.test.tsRepository: TryGhost/Koenig
Length of output: 1573
🏁 Script executed:
grep -A 20 "describe('importDOM'" packages/kg-default-nodes/test/nodes/html.test.tsRepository: TryGhost/Koenig
Length of output: 1088
🏁 Script executed:
sed -n '361,368p' packages/kg-default-nodes/test/nodes/html.test.tsRepository: TryGhost/Koenig
Length of output: 455
🏁 Script executed:
grep -B 5 -A 10 "outerHTML\|nodeType" packages/kg-default-nodes/src/nodes/html/html-parser.tsRepository: TryGhost/Koenig
Length of output: 2058
🏁 Script executed:
grep -n "exportDOM\|kg-card-begin\|kg-card-end" packages/kg-default-nodes/src/nodes/html/HtmlNode.tsRepository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
cat -n packages/kg-default-nodes/src/nodes/html/HtmlNode.tsRepository: TryGhost/Koenig
Length of output: 1069
🏁 Script executed:
grep -rn "kg-card-begin: html\|kg-card-end: html" packages/kg-default-nodes/src --include="*.ts" | head -20Repository: TryGhost/Koenig
Length of output: 212
🏁 Script executed:
sed -n '1,50p' packages/kg-default-nodes/src/nodes/html/html-renderer.tsRepository: TryGhost/Koenig
Length of output: 1280
🏁 Script executed:
sed -n '10,25p' packages/kg-default-nodes/src/nodes/html/html-parser.tsRepository: TryGhost/Koenig
Length of output: 861
🏁 Script executed:
python3 << 'EOF'
# Simulate what happens with outerHTML on different node types
class MockNode:
def __init__(self, node_type, value=None):
self.nodeType = node_type
self.textContent = value
text_node = MockNode(3, " \n ") # Text node
element_node = MockNode(1) # Element node
# Text nodes don't have outerHTML in real DOM
print("Text node has outerHTML?", hasattr(text_node, 'outerHTML'))
print("Text node.outerHTML would be:", getattr(text_node, 'outerHTML', 'UNDEFINED'))
# Simulate the array behavior
html_array = [None, "<p>test</p>"]
result = '\n'.join(str(x) for x in html_array).strip()
print(f"Array with None: {result}")
EOFRepository: TryGhost/Koenig
Length of output: 163
Unsafe cast to Element at line 14 — text nodes lack outerHTML property.
The cast (currentNode as Element).outerHTML assumes all siblings are Elements, but text nodes (e.g., whitespace or comment nodes) could exist between markers. Text nodes don't have outerHTML and would return undefined, polluting the HTML output.
While current tests pass with well-formed element-only HTML, the code should defensively check nodeType before accessing outerHTML:
Suggested fix
while (nextNode && !isHtmlEndComment(nextNode)) {
const currentNode = nextNode;
- html.push((currentNode as Element).outerHTML);
+ if (currentNode.nodeType === 1) {
+ html.push((currentNode as Element).outerHTML);
+ }
nextNode = currentNode.nextSibling;
// remove nodes as we go so that they don't go through the parser
currentNode.remove();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while (nextNode && !isHtmlEndComment(nextNode)) { | |
| const currentNode = nextNode; | |
| html.push((currentNode as Element).outerHTML); | |
| nextNode = currentNode.nextSibling; | |
| // remove nodes as we go so that they don't go through the parser | |
| currentNode.remove(); | |
| } | |
| while (nextNode && !isHtmlEndComment(nextNode)) { | |
| const currentNode = nextNode; | |
| if (currentNode.nodeType === 1) { | |
| html.push((currentNode as Element).outerHTML); | |
| } | |
| nextNode = currentNode.nextSibling; | |
| // remove nodes as we go so that they don't go through the parser | |
| currentNode.remove(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/src/nodes/html/html-parser.ts` around lines 12 -
18, The loop assumes siblings are Elements and unsafely uses (currentNode as
Element).outerHTML; instead, check node type before accessing outerHTML: when
iterating nextNode/currentNode in the while loop that uses isHtmlEndComment(),
test currentNode.nodeType === Node.ELEMENT_NODE and only then push (currentNode
as Element).outerHTML into the html array; for other node types (e.g., text
nodes) either skip them or push currentNode.textContent if you intend to
preserve text, then continue removing the node and advancing nextNode as before.
| const formTemplate = ` | ||
| <form class="kg-signup-card-form" data-members-form="signup"> | ||
| ${nodeData.labels.map(label => `<input data-members-label type="hidden" value="${label}" />`).join('\n')} | ||
| ${nodeData.labels.map((label: string) => `<input data-members-label type="hidden" value="${label}" />`).join('\n')} |
There was a problem hiding this comment.
Escape label values before injecting into value="...".
At Line 63, label is inserted raw into an HTML attribute. A crafted label containing " or < can break attribute context and inject markup.
🔒 Suggested fix
+function escapeHtmlAttr(value: string) {
+ return value
+ .replaceAll('&', '&')
+ .replaceAll('"', '"')
+ .replaceAll('<', '<')
+ .replaceAll('>', '>');
+}
+
function cardTemplate(nodeData: SignupNodeData) {
@@
- ${nodeData.labels.map((label: string) => `<input data-members-label type="hidden" value="${label}" />`).join('\n')}
+ ${nodeData.labels.map((label: string) => `<input data-members-label type="hidden" value="${escapeHtmlAttr(label)}" />`).join('\n')}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ${nodeData.labels.map((label: string) => `<input data-members-label type="hidden" value="${label}" />`).join('\n')} | |
| function escapeHtmlAttr(value: string) { | |
| return value | |
| .replaceAll('&', '&') | |
| .replaceAll('"', '"') | |
| .replaceAll('<', '<') | |
| .replaceAll('>', '>'); | |
| } | |
| function cardTemplate(nodeData: SignupNodeData) { | |
| ${nodeData.labels.map((label: string) => `<input data-members-label type="hidden" value="${escapeHtmlAttr(label)}" />`).join('\n')} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/src/nodes/signup/signup-renderer.ts` at line 63,
The label values from nodeData.labels are being injected raw into the input
value attribute in the template generation (the map that produces `<input
data-members-label ... value="${label}" />`), so escape attribute-special
characters before interpolating; create or reuse a small helper (e.g.,
escapeHtml or escapeAttribute) that replaces &, <, >, ", ', and ` with their
HTML entities and call it when mapping labels (use the helper inside the map
that builds the `<input data-members-label ... />` string) to prevent
attribute/context-breaking characters and XSS.
| export const isUnsplashImage = function (url: string) { | ||
| return /images\.unsplash\.com/.test(url); |
There was a problem hiding this comment.
Use hostname validation instead of substring matching.
This regex matches anywhere in the string, so non-Unsplash URLs can be misclassified if they contain images.unsplash.com in query/path text. Parse the URL and validate hostname directly.
Proposed fix
export const isUnsplashImage = function (url: string) {
- return /images\.unsplash\.com/.test(url);
+ try {
+ return new URL(url).hostname === 'images.unsplash.com';
+ } catch {
+ return false;
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const isUnsplashImage = function (url: string) { | |
| return /images\.unsplash\.com/.test(url); | |
| export const isUnsplashImage = function (url: string) { | |
| try { | |
| return new URL(url).hostname === 'images.unsplash.com'; | |
| } catch { | |
| return false; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/src/utils/is-unsplash-image.ts` around lines 1 - 2,
Replace the substring/regex check in isUnsplashImage with proper URL hostname
validation: in the isUnsplashImage function, attempt to construct a new URL(url)
inside a try/catch and return false for invalid URLs, then compare
urlObj.hostname strictly to "images.unsplash.com" (or use an appropriate
endsWith check if you want subdomains) instead of testing the string with
/images\.unsplash\.com/ so only actual hosts match.
| const atLinkNode = $createAtLinkNode('bold' as unknown as number); | ||
| atLinkNode.getLinkFormat()!.should.equal('bold'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "at-link" -e tsRepository: TryGhost/Koenig
Length of output: 317
🏁 Script executed:
cat -n packages/kg-default-nodes/src/nodes/at-link/AtLinkNode.tsRepository: TryGhost/Koenig
Length of output: 3981
🏁 Script executed:
cat -n packages/kg-default-nodes/test/nodes/at-link.test.ts | head -100Repository: TryGhost/Koenig
Length of output: 4644
🏁 Script executed:
rg -i "format|BOLD|ITALIC" packages/kg-default-nodes/src --type ts -A 2 -B 2 | head -60Repository: TryGhost/Koenig
Length of output: 4657
🏁 Script executed:
rg "lexical" packages/kg-default-nodes/package.jsonRepository: TryGhost/Koenig
Length of output: 289
🌐 Web query:
Lexical text format constants BOLD ITALIC numeric values
💡 Result:
In Lexical (the Meta/Facebook editor), text formatting is stored as bit flags:
- BOLD:
IS_BOLD = 1 - ITALIC:
IS_ITALIC = 1 << 1(numeric value 2) [1]
Source
- [1] Lexical
LexicalConstants.ts(text node formatting flags) https://github.com/facebook/lexical/blob/80ed4aa6/packages/lexical/src/LexicalConstants.ts
Use real Lexical numeric format values instead of string casts.
packages/kg-default-nodes/src/nodes/at-link/AtLinkNode.ts declares linkFormat as number | null, but these tests cast the string 'bold' through as unknown as number. Lexical uses numeric bit flags for text formatting (e.g., 1 for bold, 2 for italic), so these tests validate an impossible state the production API does not type-safely allow. Any numeric-format logic will go untested.
Replace 'bold' as unknown as number with actual numeric values like 1 (bold) or 2 (italic), or use null where appropriate.
Applies to: 48-49, 53-57, 61-75, 94-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/test/nodes/at-link.test.ts` around lines 48 - 49,
Tests createAtLinkNode('bold' as unknown as number) and assert getLinkFormat(),
but they cast strings to numbers which is invalid for AtLinkNode.linkFormat
(number | null) and misses real numeric-format behavior; update all such uses of
$createAtLinkNode(...) in the at-link tests to pass actual Lexical numeric
bit-flags (e.g., 1 for bold, 2 for italic) or null where appropriate, then
adjust assertions to expect those numeric values (or null) so numeric-format
logic is exercised (refer to $createAtLinkNode and AtLinkNode.getLinkFormat /
linkFormat).
| productNode.productImageWidth!.should.equal('200'); | ||
| productNode.productImageHeight!.should.equal('100'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how productImageWidth/Height are set in the parser
rg -n "productImageWidth|productImageHeight" packages/kg-default-nodes/src/nodes/product/Repository: TryGhost/Koenig
Length of output: 2078
🏁 Script executed:
# Read the test around lines 628-629 to see the full context
sed -n '620,640p' packages/kg-default-nodes/test/nodes/product.test.ts
# Check if the parser does any string-to-number conversion
sed -n '20,35p' packages/kg-default-nodes/src/nodes/product/product-parser.tsRepository: TryGhost/Koenig
Length of output: 2075
Type definition mismatch: productImageWidth and productImageHeight should be string | null, not number | null.
The parser uses getAttribute('width') and getAttribute('height') (which return strings) and stores them directly without conversion. The test correctly compares against string values '200' and '100'. However, ProductNode.ts declares these properties as number | null, creating a type safety issue. The type definition must be corrected to string | null to match the actual runtime values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/test/nodes/product.test.ts` around lines 628 - 629,
ProductNode declares productImageWidth and productImageHeight as number | null
but the parser assigns string values from
getAttribute('width')/getAttribute('height'); change the type signatures of
productImageWidth and productImageHeight in the ProductNode class to string |
null and update any related method signatures or assignments in ProductNode
(e.g., constructor/parse/fromDOM functions) to reflect string|null so runtime
values and the test comparisons ('200', '100') match the type system.
| declare namespace should { | ||
| interface Assertion { | ||
| prettifyTo(expected: string): this; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for duplicate prettifyTo declarations in test-utils
fd -e d.ts . packages/kg-default-nodes/test/test-utils --exec cat {}Repository: TryGhost/Koenig
Length of output: 511
Consolidate conflicting prettifyTo declarations to resolve TypeScript ambiguity.
Multiple .d.ts files in packages/kg-default-nodes/test/test-utils declare prettifyTo with conflicting return types: this in one location and void in others. This causes TypeScript type confusion. Consolidate these declarations into a single file, using this as the return type to support method chaining.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/test/test-utils/should-assertions.d.ts` around
lines 1 - 5, Consolidate the conflicting TypeScript declarations for prettifyTo
by keeping a single declaration that returns this: remove or update other .d.ts
declarations so only the declaration inside the should namespace's Assertion
interface defines prettifyTo(expected: string): this; (ensure you
reference/modify the declarations that currently return void and replace them
with the this-returning signature for method chaining).
- Remove Rollup build (rollup.config.mjs, rollup-plugin-svg, @babel/*) - Move lib/ to src/, rename .js to .ts - Add tsconfig.json (strict, NodeNext, ESM) - Add "type": "module" to package.json - Convert 100 source files and 36 test files to ESM with Lexical types - Inline SVG import in AtLinkNode (replaces rollup-plugin-svg) - Replace .eslintrc.js with eslint.config.js (flat config) - Output to build/ via tsc (replaces cjs/ and es/ dirs) - 668 tests passing
kevinansfield
force-pushed
the
ts/kg-default-nodes
branch
from
ef572c1 to
25f1493
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
packages/kg-default-nodes/src/nodes/gallery/gallery-renderer.ts (2)
180-182:⚠️ Potential issue | 🔴 CriticalAvoid unsanitized
innerHTMLfor captions.Line 181 writes
node.captiondirectly toinnerHTML, which is an XSS sink. Sanitize with the shared allowlist utility (preferred) or fall back totextContentif markup is not required.Minimal safe fallback
- figcaption.innerHTML = node.caption; + figcaption.textContent = node.caption;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/gallery/gallery-renderer.ts` around lines 180 - 182, Replace the unsafe figcaption.innerHTML = node.caption assignment in gallery-renderer.ts: import and use the shared allowlist sanitizer utility to sanitize node.caption before setting innerHTML on the created figcaption element (e.g., sanitized = allowlistSanitize(node.caption); figcaption.innerHTML = sanitized), and if the allowlist sanitizer is not available or returns null/empty, fall back to assigning the plain text via figcaption.textContent = node.caption; ensure the sanitizer function is referenced and imported where the figcaption is created and handle undefined/null captions safely.
145-147:⚠️ Potential issue | 🟠 MajorGuard
contentImageSizesbefore retina-size lookup.Line 147 dereferences
options.imageOptimization!.contentImageSizes!even though both are optional inGalleryRenderOptions; this can throw at runtime in email rendering when transform is enabled without size metadata.Suggested fix
- if (isLocalContentImage(image.src, options.siteUrl) && options.canTransformImage && options.canTransformImage(image.src)) { + const contentImageSizes = options.imageOptimization?.contentImageSizes; + if (contentImageSizes && isLocalContentImage(image.src, options.siteUrl) && options.canTransformImage?.(image.src)) { // find available image size next up from 2x600 so we can use it for the "retina" src - const availableImageWidths = getAvailableImageWidths(image, options.imageOptimization!.contentImageSizes!); + const availableImageWidths = getAvailableImageWidths(image, contentImageSizes); const srcWidth = availableImageWidths.find(width => width >= 1200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/gallery/gallery-renderer.ts` around lines 145 - 147, The code in GalleryRenderOptions assumes options.imageOptimization and contentImageSizes exist when calling getAvailableImageWidths(image, options.imageOptimization!.contentImageSizes!), which can throw; update the gallery-renderer.ts flow (around the block using isLocalContentImage(image.src, options.siteUrl) and getAvailableImageWidths) to first check options.imageOptimization and options.imageOptimization.contentImageSizes are defined before performing the retina-size lookup, and if missing skip or short-circuit the retina logic (use the non-retina src fallback) so getAvailableImageWidths is never called with undefined; reference the image variable, options, getAvailableImageWidths and the retina-size lookup in your change.packages/kg-default-nodes/src/nodes/html/html-parser.ts (1)
12-18:⚠️ Potential issue | 🟠 MajorUnsafe cast to
Element— text/comment nodes lackouterHTML.The loop assumes all siblings are Elements, but text nodes (whitespace, line breaks) or other comment nodes between the markers would cause
outerHTMLto returnundefined, polluting the HTML output with "undefined" strings.This issue was flagged in a previous review but appears unresolved.
🐛 Proposed fix
while (nextNode && !isHtmlEndComment(nextNode)) { const currentNode = nextNode; - html.push((currentNode as Element).outerHTML); + if (currentNode.nodeType === 1) { + html.push((currentNode as Element).outerHTML); + } nextNode = currentNode.nextSibling; // remove nodes as we go so that they don't go through the parser currentNode.remove(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/html/html-parser.ts` around lines 12 - 18, The loop in html-parser.ts assumes every sibling is an Element and uses (currentNode as Element).outerHTML, which produces "undefined" for text/comment nodes; update the while loop that uses nextNode/currentNode and isHtmlEndComment to guard by node type: if currentNode.nodeType === Node.ELEMENT_NODE (or currentNode instanceof Element) push its outerHTML, else if currentNode.nodeType === Node.TEXT_NODE push its textContent (trim or skip if only whitespace), and skip comment nodes (Node.COMMENT_NODE); keep removing currentNode after handling it so nodes still don't reach the parser.packages/kg-default-nodes/src/nodes/call-to-action/calltoaction-renderer.ts (1)
401-416:⚠️ Potential issue | 🟠 MajorSanitize
sponsorLabelbefore the email early-return.The email rendering path (lines 401-408) returns before
sponsorLabelis sanitized (lines 412-416). This means the email template at line 405 receives raw, unsanitizedsponsorLabelcontent, while the web template gets sanitized content.Move the sanitization before the email branch:
🐛 Proposed fix
+ if (dataset.hasSponsorLabel) { + const cleanBasicHtml = buildCleanBasicHtmlForElement(document.createElement('div')); + const cleanedHtml = cleanBasicHtml(dataset.sponsorLabel, {firstChildInnerContent: true}); + dataset.sponsorLabel = cleanedHtml || ''; + } + if (options.target === 'email') { const emailDoc = options.createDocument!(); const emailDiv = emailDoc.createElement('div'); emailDiv.innerHTML = emailCTATemplate(dataset, options); return renderWithVisibility({element: emailDiv.firstElementChild as RenderOutput['element']}, node.visibility as Visibility, options); } const element = document.createElement('div'); - if (dataset.hasSponsorLabel) { - const cleanBasicHtml = buildCleanBasicHtmlForElement(element); - const cleanedHtml = cleanBasicHtml(dataset.sponsorLabel, {firstChildInnerContent: true}); - dataset.sponsorLabel = cleanedHtml || ''; - } const htmlString = ctaCardTemplate(dataset);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/call-to-action/calltoaction-renderer.ts` around lines 401 - 416, The email rendering path returns before sanitizing dataset.sponsorLabel, so the emailCTATemplate receives unsanitized HTML; move the sponsorLabel sanitization (use buildCleanBasicHtmlForElement and cleanBasicHtml) to occur before the options.target === 'email' branch and assign the sanitized string back to dataset.sponsorLabel, then proceed to call emailCTATemplate and renderWithVisibility (preserving node.visibility and options) so both email and web paths use the cleaned sponsorLabel.
🧹 Nitpick comments (10)
packages/kg-default-nodes/src/nodes/ExtendedHeadingNode.ts (1)
47-50: ConsistentDOMConverterFntype pattern.This type definition mirrors the one in
ExtendedTextNode.ts. Consider extracting this shared type to a common utilities module if more extended nodes are added in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/ExtendedHeadingNode.ts` around lines 47 - 50, The DOMConverterFn type is duplicated (see DOMConverterFn and patchParagraphConversion) and should be extracted to a shared utility to keep types consistent across extended nodes; create a new exported type (e.g., DOMConverterFn) in a common utils/types module, replace the inline DOMConverterFn declaration in this file and in ExtendedTextNode by importing that shared type, and update any references in patchParagraphConversion and related conversion functions to use the imported type.packages/kg-default-nodes/src/nodes/product/product-renderer.ts (2)
54-61: Consider the innerHTML security context.The static analysis flags the
innerHTMLassignment as a potential XSS vector. In this CMS renderer context, the risk is mitigated because:
- Data originates from the Ghost editor/database (trusted input)
- Content sanitization typically occurs at write time in CMS architectures
However, if any of the template data fields (
productUrl,productTitle,productButton,productDescription,productImageSrc) could contain user-controlled content that bypasses editor sanitization, this would be exploitable.If stricter security is desired, consider sanitizing URL attributes with a URL validator or encoding HTML entities for text content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/product/product-renderer.ts` around lines 54 - 61, The assignment to element.innerHTML using htmlString (produced by emailCardTemplate or cardTemplate) is flagged as a potential XSS vector; ensure template data fields (productUrl, productTitle, productButton, productDescription, productImageSrc) are sanitized/validated before rendering by either validating/normalizing URLs (e.g., allow only http(s) and data:image for productImageSrc/productUrl) and HTML-encoding any free-text fields, or by switching to safe DOM creation rather than innerHTML; update the rendering flow in product-renderer.ts so that htmlString is produced from sanitized templateData (or replace element.innerHTML usage with element.appendChild/fromDocumentFragment built using createElement/setAttribute/textContent) while keeping selection between emailCardTemplate and cardTemplate.
96-105: Type assertions are acceptable but consider stronger typing in future.The
as numberassertions on lines 98-99 and 102 are necessary because the truthiness check doesn't narrowRecord<string, unknown>to a specific type. This pattern is common when working with loosely-typed data structures.For future improvement, consider defining a more specific interface for template data (e.g.,
ProductTemplateDatawithproductImageWidth?: number) to eliminate the need for runtime type assertions. This would be a broader refactor across renderers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/product/product-renderer.ts` around lines 96 - 105, The code uses runtime type assertions (data.productImageWidth as number, data.productImageHeight as number) because Record<string, unknown> prevents proper type narrowing; to fix, introduce a stronger type for the template data (e.g., interface ProductTemplateData { productImageWidth?: number; productImageHeight?: number; ... }) and annotate the renderer function parameter so `data` is ProductTemplateData, then remove the `as number` casts and rely on the existing truthiness checks before using `imageDimensions` and calling getResizedImageDimensions; alternatively, if changing the parameter type is not possible right now, add a small type guard function (e.g., isNumber) and use it to narrow data.productImageWidth/data.productImageHeight before assigning to imageDimensions to avoid unsafe assertions.packages/kg-default-nodes/src/nodes/signup/SignupNode.ts (1)
108-110: Consider tightening the parameter type for better type safety.The function accepts
Record<string, unknown>but passes it to a constructor expectingSignupData. This type mismatch allows invalid property types to slip through at compile time (e.g.,labelsas a non-array).If the loose type is intentional for parsing flexibility from external sources, consider documenting it or adding minimal runtime validation for critical fields.
💡 Option: Use SignupData for stricter typing
-export const $createSignupNode = (dataset: Record<string, unknown>) => { +export const $createSignupNode = (dataset: SignupData) => { return new SignupNode(dataset); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/signup/SignupNode.ts` around lines 108 - 110, The factory $createSignupNode currently types its parameter as Record<string, unknown> but immediately passes it to new SignupNode which expects SignupData; tighten the parameter to SignupData (replace the dataset type) to ensure compile-time safety, or if loose parsing is required, add minimal runtime validation inside $createSignupNode to coerce/verify critical fields (e.g., ensure labels is an array) and document why Record<string, unknown> is used; update references to SignupData, $createSignupNode, and SignupNode accordingly so the constructor always receives a valid SignupData object.packages/kg-default-nodes/src/nodes/bookmark/BookmarkNode.ts (1)
114-116: Consider stricter parameter typing for$createBookmarkNode.The function accepts
Record<string, unknown>but passes it to a constructor expectingBookmarkData. This reduces type safety—callers can pass any shape without compile-time validation.If you want to preserve flexibility for deserialization scenarios while improving type safety for direct usage:
♻️ Suggested improvement
-export const $createBookmarkNode = (dataset: Record<string, unknown>) => { +export const $createBookmarkNode = (dataset: BookmarkData) => { return new BookmarkNode(dataset); };Callers with
Record<string, unknown>data can cast explicitly:$createBookmarkNode(data as BookmarkData).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/bookmark/BookmarkNode.ts` around lines 114 - 116, The $createBookmarkNode function currently accepts a loose Record<string, unknown> but passes it to the BookmarkNode constructor which expects BookmarkData; tighten the signature to accept BookmarkData (or BookmarkData | Record<string, unknown> if you must preserve deserialization flexibility) and, if allowing the union, perform an explicit cast/validation before calling new BookmarkNode(dataset) so callers get compile-time safety; update the $createBookmarkNode parameter type and any callers that construct nodes from known BookmarkData to pass BookmarkData (or cast deserialized objects with `as BookmarkData`) and ensure the constructor argument to BookmarkNode is the properly typed BookmarkData.packages/kg-default-nodes/src/nodes/markdown/markdown-renderer.ts (1)
8-13: The recommendation to use the renderer's exported options type is not feasible. The@tryghost/kg-markdown-html-rendererpackage does not export a TypeScript options type—RenderOptionsis internal and inaccessible. The cast toRecord<string, unknown>on line 19 is necessary becauseMarkdownRenderOptionsincludes properties (createDocument,dom,target) that therender()function doesn't accept (it only supportsghostVersion). A better alternative would be to extract only the relevant options before passing them torender(), or to acknowledge the type boundary and document the cast intent with a comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/markdown/markdown-renderer.ts` around lines 8 - 13, The custom MarkdownRenderOptions includes createDocument, dom, and target which aren't accepted by the external render() (internal RenderOptions only accepts ghostVersion), so remove the unsafe broad cast: extract only the supported options (e.g., const { ghostVersion } = options) or build a new object containing just the allowed keys before calling render(), and update the call site that currently casts to Record<string, unknown> to pass this filtered object; if you must keep the cast, replace it with a short comment explaining the intentional type boundary and why createDocument/dom/target are excluded.packages/kg-default-nodes/test/test-utils/index.ts (1)
6-16: Consider adding error handling for Prettier formatting.
Prettier.format()can throw if the input HTML is severely malformed. While test inputs are typically controlled, wrapping this in a try-catch or documenting this behavior would improve robustness.That said, for test utilities where inputs are controlled, this is acceptable as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/test/test-utils/index.ts` around lines 6 - 16, The html test helper calls Prettier.format(output, {parser: 'html'}) which can throw on malformed input; wrap the Prettier.format call in a try-catch inside the html function (or explicitly document the potential exception) so tests don't crash on formatting errors—on catch, return the raw output or a safe fallback and optionally log or rethrow with additional context mentioning the html helper and the failing output so the failure is clear.packages/kg-default-nodes/test/nodes/paywall.test.ts (1)
90-94: Consider a more targeted type assertion forexportDOM.The
as unknown as LexicalEditorcast is a broad escape hatch. TheexportDOMmethod signature expects aLexicalEditor, but you're passing render options. This works at runtime because the decorator node'sexportDOMimplementation uses these options, but the type mismatch suggests the base type definition may need adjustment upstream.For now, this is acceptable as a migration workaround, but consider creating a dedicated type or interface for export options to avoid the double cast pattern across test files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/test/nodes/paywall.test.ts` around lines 90 - 94, The test is using a broad double-cast (as unknown as LexicalEditor) when calling paywallNode.exportDOM; instead create a narrow mock or interface that matches what exportDOM actually reads instead of pretending to be a full LexicalEditor: either define a small ExportDOMOptions/RenderExportOptions type that describes the properties exportDOM uses and cast exportOptions to that, or construct a minimal mock object implementing just the methods/properties the $createPaywallNode().exportDOM(exportOptions) implementation accesses, then pass that instead of casting to LexicalEditor; update the test to call exportDOM with this targeted mock/interface so you remove the double-cast while keeping behavior unchanged.packages/kg-default-nodes/src/nodes/call-to-action/calltoaction-renderer.ts (1)
136-142: Non-null assertion onmatch()at line 138 may throw if regex doesn't match.The
!assertion assumes the regex will always match. WhileisLocalContentImagelikely validates that the URL contains/content/images/, if that assumption is violated, this will throw a runtime error.Consider adding a defensive check:
♻️ Suggested defensive fix
if (isLocalContentImage(dataset.imageUrl, options.siteUrl) && options.canTransformImage?.(dataset.imageUrl)) { - const [, imagesPath, filename] = dataset.imageUrl.match(/(.*\/content\/images)\/(.*)/)!; + const match = dataset.imageUrl.match(/(.*\/content\/images)\/(.*)/); + if (!match) { + // URL doesn't match expected format, skip transformation + return; + } + const [, imagesPath, filename] = match; const iconSize = options?.imageOptimization?.internalImageSizes?.['email-cta-minimal-image'] || {width: 256, height: 256}; dataset.imageUrl = `${imagesPath}/size/w${iconSize.width}h${iconSize.height}/${filename}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/nodes/call-to-action/calltoaction-renderer.ts` around lines 136 - 142, The code uses a non-null assertion on dataset.imageUrl.match(...) inside the block that checks isLocalContentImage(...) and options.canTransformImage(...), which can still throw if the regex doesn't match; update the transformation in calltoaction-renderer.ts to first assign the match result to a variable, verify it's not null before destructuring (or bail out/leave dataset.imageUrl unchanged), and only build the optimized URL when the match succeeds; reference the dataset.imageUrl match, the isLocalContentImage(...) guard, and the iconSize calculation so the logic flow remains the same but safe against missing regex matches.packages/kg-default-nodes/src/generate-decorator-node.ts (1)
241-253: Consider adding a type guard for versioned renderer lookup.The version-based renderer lookup uses
numberindexing butnodeVersioncould be a number or come fromthis.__version. The current castas numberworks but the logic assumes version is always a valid key.💡 Optional: Add defensive check for missing versioned renderer
The current code throws a helpful error when the versioned renderer is missing, which is good. The implementation handles this case properly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-default-nodes/src/generate-decorator-node.ts` around lines 241 - 253, The node renderer lookup assumes nodeVersion is a valid numeric key; add a type guard and normalize nodeVersion before indexing into nodeRenderers: retrieve nodeVersion from the local variable or this.__version, coerce/validate it to a finite number or string key (e.g., ensure typeof nodeVersion === 'number' && Number.isFinite(nodeVersion) or convert from string safely), then use that normalized key when accessing (render as Record<string|number,RenderFn>)[normalizedVersion]; keep the existing error throw if the versioned renderer is missing and update references to nodeVersion/this.__version and the local render variable in generateDecoratorNode to use the validated normalizedVersion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/kg-default-nodes/package.json`:
- Around line 19-21: The package.json scripts ("build", "prepare", "pretest")
run tsc but do not clean stale artifacts; modify each script to remove/clean the
build directory before running tsc (e.g., run a cross-platform cleaner like
rimraf build or rm -rf build && mkdir -p build) so old JS/.d.ts files are
deleted before emitting, then run the existing tsc commands and the echo step;
update "build", "prepare", and "pretest" to include this pre-clean step (or add
a separate "clean" script and invoke it from those scripts).
In `@packages/kg-default-nodes/src/nodes/embed/embed-renderer.ts`:
- Around line 51-55: The code in the isEmail && isVideoWithThumbnail branch uses
non-null assertions on metadata.thumbnail_width and metadata.thumbnail_height
(line computing thumbnailAspectRatio) which can be missing or zero and lead to
division-by-zero or invalid spacerHeight; update the guard in that block to
verify metadata.thumbnail_width and metadata.thumbnail_height are present and >
0 before computing thumbnailAspectRatio and spacerHeight, and if they are
absent/invalid either skip thumbnail sizing logic (avoid computing spacerHeight)
or use a safe default aspect ratio (e.g., 16/9) so spacerHeight and spacerWidth
are always valid; adjust the calculations that set spacerHeight/spacerWidth (and
any downstream use) accordingly to reference the validated values.
In `@packages/kg-default-nodes/src/nodes/markdown/markdown-renderer.ts`:
- Around line 19-23: The code assigns unsanitized HTML from
render(node.markdown) directly to element.innerHTML in markdown-renderer.ts,
creating an XSS risk; fix by sanitizing the rendered HTML (e.g., run the output
of render(...) through a sanitizer like DOMPurify before assigning to
element.innerHTML) or, if only plain text is needed, set element.textContent to
node.markdown instead; update the code paths around render, node.markdown and
element.innerHTML to use the sanitizer API (or textContent) and add a short
comment documenting the trust assumption if you opt to keep raw HTML.
In `@packages/kg-default-nodes/src/nodes/video/video-renderer.ts`:
- Around line 16-21: The VideoRenderOptions interface currently makes postUrl
optional which allows target === 'email' with postUrl undefined; change
VideoRenderOptions to a discriminated union (e.g., one variant where target:
'email' and postUrl: string, and another for other targets where postUrl?:
string) so TypeScript enforces postUrl when target is 'email'; update any
references or function signatures that accept VideoRenderOptions (e.g., the
function that uses emailCardTemplate) to use the new union type so the places
that interpolate postUrl into href must have a defined string.
---
Duplicate comments:
In `@packages/kg-default-nodes/src/nodes/call-to-action/calltoaction-renderer.ts`:
- Around line 401-416: The email rendering path returns before sanitizing
dataset.sponsorLabel, so the emailCTATemplate receives unsanitized HTML; move
the sponsorLabel sanitization (use buildCleanBasicHtmlForElement and
cleanBasicHtml) to occur before the options.target === 'email' branch and assign
the sanitized string back to dataset.sponsorLabel, then proceed to call
emailCTATemplate and renderWithVisibility (preserving node.visibility and
options) so both email and web paths use the cleaned sponsorLabel.
In `@packages/kg-default-nodes/src/nodes/gallery/gallery-renderer.ts`:
- Around line 180-182: Replace the unsafe figcaption.innerHTML = node.caption
assignment in gallery-renderer.ts: import and use the shared allowlist sanitizer
utility to sanitize node.caption before setting innerHTML on the created
figcaption element (e.g., sanitized = allowlistSanitize(node.caption);
figcaption.innerHTML = sanitized), and if the allowlist sanitizer is not
available or returns null/empty, fall back to assigning the plain text via
figcaption.textContent = node.caption; ensure the sanitizer function is
referenced and imported where the figcaption is created and handle
undefined/null captions safely.
- Around line 145-147: The code in GalleryRenderOptions assumes
options.imageOptimization and contentImageSizes exist when calling
getAvailableImageWidths(image, options.imageOptimization!.contentImageSizes!),
which can throw; update the gallery-renderer.ts flow (around the block using
isLocalContentImage(image.src, options.siteUrl) and getAvailableImageWidths) to
first check options.imageOptimization and
options.imageOptimization.contentImageSizes are defined before performing the
retina-size lookup, and if missing skip or short-circuit the retina logic (use
the non-retina src fallback) so getAvailableImageWidths is never called with
undefined; reference the image variable, options, getAvailableImageWidths and
the retina-size lookup in your change.
In `@packages/kg-default-nodes/src/nodes/html/html-parser.ts`:
- Around line 12-18: The loop in html-parser.ts assumes every sibling is an
Element and uses (currentNode as Element).outerHTML, which produces "undefined"
for text/comment nodes; update the while loop that uses nextNode/currentNode and
isHtmlEndComment to guard by node type: if currentNode.nodeType ===
Node.ELEMENT_NODE (or currentNode instanceof Element) push its outerHTML, else
if currentNode.nodeType === Node.TEXT_NODE push its textContent (trim or skip if
only whitespace), and skip comment nodes (Node.COMMENT_NODE); keep removing
currentNode after handling it so nodes still don't reach the parser.
---
Nitpick comments:
In `@packages/kg-default-nodes/src/generate-decorator-node.ts`:
- Around line 241-253: The node renderer lookup assumes nodeVersion is a valid
numeric key; add a type guard and normalize nodeVersion before indexing into
nodeRenderers: retrieve nodeVersion from the local variable or this.__version,
coerce/validate it to a finite number or string key (e.g., ensure typeof
nodeVersion === 'number' && Number.isFinite(nodeVersion) or convert from string
safely), then use that normalized key when accessing (render as
Record<string|number,RenderFn>)[normalizedVersion]; keep the existing error
throw if the versioned renderer is missing and update references to
nodeVersion/this.__version and the local render variable in
generateDecoratorNode to use the validated normalizedVersion.
In `@packages/kg-default-nodes/src/nodes/bookmark/BookmarkNode.ts`:
- Around line 114-116: The $createBookmarkNode function currently accepts a
loose Record<string, unknown> but passes it to the BookmarkNode constructor
which expects BookmarkData; tighten the signature to accept BookmarkData (or
BookmarkData | Record<string, unknown> if you must preserve deserialization
flexibility) and, if allowing the union, perform an explicit cast/validation
before calling new BookmarkNode(dataset) so callers get compile-time safety;
update the $createBookmarkNode parameter type and any callers that construct
nodes from known BookmarkData to pass BookmarkData (or cast deserialized objects
with `as BookmarkData`) and ensure the constructor argument to BookmarkNode is
the properly typed BookmarkData.
In `@packages/kg-default-nodes/src/nodes/call-to-action/calltoaction-renderer.ts`:
- Around line 136-142: The code uses a non-null assertion on
dataset.imageUrl.match(...) inside the block that checks
isLocalContentImage(...) and options.canTransformImage(...), which can still
throw if the regex doesn't match; update the transformation in
calltoaction-renderer.ts to first assign the match result to a variable, verify
it's not null before destructuring (or bail out/leave dataset.imageUrl
unchanged), and only build the optimized URL when the match succeeds; reference
the dataset.imageUrl match, the isLocalContentImage(...) guard, and the iconSize
calculation so the logic flow remains the same but safe against missing regex
matches.
In `@packages/kg-default-nodes/src/nodes/ExtendedHeadingNode.ts`:
- Around line 47-50: The DOMConverterFn type is duplicated (see DOMConverterFn
and patchParagraphConversion) and should be extracted to a shared utility to
keep types consistent across extended nodes; create a new exported type (e.g.,
DOMConverterFn) in a common utils/types module, replace the inline
DOMConverterFn declaration in this file and in ExtendedTextNode by importing
that shared type, and update any references in patchParagraphConversion and
related conversion functions to use the imported type.
In `@packages/kg-default-nodes/src/nodes/markdown/markdown-renderer.ts`:
- Around line 8-13: The custom MarkdownRenderOptions includes createDocument,
dom, and target which aren't accepted by the external render() (internal
RenderOptions only accepts ghostVersion), so remove the unsafe broad cast:
extract only the supported options (e.g., const { ghostVersion } = options) or
build a new object containing just the allowed keys before calling render(), and
update the call site that currently casts to Record<string, unknown> to pass
this filtered object; if you must keep the cast, replace it with a short comment
explaining the intentional type boundary and why createDocument/dom/target are
excluded.
In `@packages/kg-default-nodes/src/nodes/product/product-renderer.ts`:
- Around line 54-61: The assignment to element.innerHTML using htmlString
(produced by emailCardTemplate or cardTemplate) is flagged as a potential XSS
vector; ensure template data fields (productUrl, productTitle, productButton,
productDescription, productImageSrc) are sanitized/validated before rendering by
either validating/normalizing URLs (e.g., allow only http(s) and data:image for
productImageSrc/productUrl) and HTML-encoding any free-text fields, or by
switching to safe DOM creation rather than innerHTML; update the rendering flow
in product-renderer.ts so that htmlString is produced from sanitized
templateData (or replace element.innerHTML usage with
element.appendChild/fromDocumentFragment built using
createElement/setAttribute/textContent) while keeping selection between
emailCardTemplate and cardTemplate.
- Around line 96-105: The code uses runtime type assertions
(data.productImageWidth as number, data.productImageHeight as number) because
Record<string, unknown> prevents proper type narrowing; to fix, introduce a
stronger type for the template data (e.g., interface ProductTemplateData {
productImageWidth?: number; productImageHeight?: number; ... }) and annotate the
renderer function parameter so `data` is ProductTemplateData, then remove the
`as number` casts and rely on the existing truthiness checks before using
`imageDimensions` and calling getResizedImageDimensions; alternatively, if
changing the parameter type is not possible right now, add a small type guard
function (e.g., isNumber) and use it to narrow
data.productImageWidth/data.productImageHeight before assigning to
imageDimensions to avoid unsafe assertions.
In `@packages/kg-default-nodes/src/nodes/signup/SignupNode.ts`:
- Around line 108-110: The factory $createSignupNode currently types its
parameter as Record<string, unknown> but immediately passes it to new SignupNode
which expects SignupData; tighten the parameter to SignupData (replace the
dataset type) to ensure compile-time safety, or if loose parsing is required,
add minimal runtime validation inside $createSignupNode to coerce/verify
critical fields (e.g., ensure labels is an array) and document why
Record<string, unknown> is used; update references to SignupData,
$createSignupNode, and SignupNode accordingly so the constructor always receives
a valid SignupData object.
In `@packages/kg-default-nodes/test/nodes/paywall.test.ts`:
- Around line 90-94: The test is using a broad double-cast (as unknown as
LexicalEditor) when calling paywallNode.exportDOM; instead create a narrow mock
or interface that matches what exportDOM actually reads instead of pretending to
be a full LexicalEditor: either define a small
ExportDOMOptions/RenderExportOptions type that describes the properties
exportDOM uses and cast exportOptions to that, or construct a minimal mock
object implementing just the methods/properties the
$createPaywallNode().exportDOM(exportOptions) implementation accesses, then pass
that instead of casting to LexicalEditor; update the test to call exportDOM with
this targeted mock/interface so you remove the double-cast while keeping
behavior unchanged.
In `@packages/kg-default-nodes/test/test-utils/index.ts`:
- Around line 6-16: The html test helper calls Prettier.format(output, {parser:
'html'}) which can throw on malformed input; wrap the Prettier.format call in a
try-catch inside the html function (or explicitly document the potential
exception) so tests don't crash on formatting errors—on catch, return the raw
output or a safe fallback and optionally log or rethrow with additional context
mentioning the html helper and the failing output so the failure is clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 135dc1ea-e8d5-49ae-bf9a-a3a3d15f4faf
📒 Files selected for processing (150)
packages/kg-default-nodes/eslint.config.mjspackages/kg-default-nodes/index.jspackages/kg-default-nodes/package.jsonpackages/kg-default-nodes/rollup.config.mjspackages/kg-default-nodes/src/KoenigDecoratorNode.tspackages/kg-default-nodes/src/generate-decorator-node.tspackages/kg-default-nodes/src/index.tspackages/kg-default-nodes/src/kg-default-nodes.tspackages/kg-default-nodes/src/nodes/ExtendedHeadingNode.tspackages/kg-default-nodes/src/nodes/ExtendedQuoteNode.tspackages/kg-default-nodes/src/nodes/ExtendedTextNode.tspackages/kg-default-nodes/src/nodes/TKNode.tspackages/kg-default-nodes/src/nodes/aside/AsideNode.tspackages/kg-default-nodes/src/nodes/aside/AsideParser.tspackages/kg-default-nodes/src/nodes/at-link/AtLinkNode.tspackages/kg-default-nodes/src/nodes/at-link/AtLinkSearchNode.tspackages/kg-default-nodes/src/nodes/at-link/index.tspackages/kg-default-nodes/src/nodes/audio/AudioNode.tspackages/kg-default-nodes/src/nodes/audio/audio-parser.tspackages/kg-default-nodes/src/nodes/audio/audio-renderer.tspackages/kg-default-nodes/src/nodes/bookmark/BookmarkNode.tspackages/kg-default-nodes/src/nodes/bookmark/bookmark-parser.tspackages/kg-default-nodes/src/nodes/bookmark/bookmark-renderer.tspackages/kg-default-nodes/src/nodes/button/ButtonNode.tspackages/kg-default-nodes/src/nodes/button/button-parser.tspackages/kg-default-nodes/src/nodes/button/button-renderer.tspackages/kg-default-nodes/src/nodes/call-to-action/CallToActionNode.tspackages/kg-default-nodes/src/nodes/call-to-action/calltoaction-parser.tspackages/kg-default-nodes/src/nodes/call-to-action/calltoaction-renderer.tspackages/kg-default-nodes/src/nodes/callout/CalloutNode.tspackages/kg-default-nodes/src/nodes/callout/callout-parser.tspackages/kg-default-nodes/src/nodes/callout/callout-renderer.tspackages/kg-default-nodes/src/nodes/codeblock/CodeBlockNode.tspackages/kg-default-nodes/src/nodes/codeblock/codeblock-parser.tspackages/kg-default-nodes/src/nodes/codeblock/codeblock-renderer.tspackages/kg-default-nodes/src/nodes/email-cta/EmailCtaNode.tspackages/kg-default-nodes/src/nodes/email-cta/email-cta-renderer.tspackages/kg-default-nodes/src/nodes/email/EmailNode.tspackages/kg-default-nodes/src/nodes/email/email-renderer.tspackages/kg-default-nodes/src/nodes/embed/EmbedNode.tspackages/kg-default-nodes/src/nodes/embed/embed-parser.tspackages/kg-default-nodes/src/nodes/embed/embed-renderer.tspackages/kg-default-nodes/src/nodes/embed/types/twitter.tspackages/kg-default-nodes/src/nodes/file/FileNode.tspackages/kg-default-nodes/src/nodes/file/file-parser.tspackages/kg-default-nodes/src/nodes/file/file-renderer.tspackages/kg-default-nodes/src/nodes/gallery/GalleryNode.tspackages/kg-default-nodes/src/nodes/gallery/gallery-parser.tspackages/kg-default-nodes/src/nodes/gallery/gallery-renderer.tspackages/kg-default-nodes/src/nodes/header/HeaderNode.tspackages/kg-default-nodes/src/nodes/header/parsers/header-parser.tspackages/kg-default-nodes/src/nodes/header/renderers/v1/header-renderer.tspackages/kg-default-nodes/src/nodes/header/renderers/v2/header-renderer.tspackages/kg-default-nodes/src/nodes/horizontalrule/HorizontalRuleNode.tspackages/kg-default-nodes/src/nodes/horizontalrule/horizontalrule-parser.tspackages/kg-default-nodes/src/nodes/horizontalrule/horizontalrule-renderer.tspackages/kg-default-nodes/src/nodes/html/HtmlNode.tspackages/kg-default-nodes/src/nodes/html/html-parser.tspackages/kg-default-nodes/src/nodes/html/html-renderer.tspackages/kg-default-nodes/src/nodes/image/ImageNode.tspackages/kg-default-nodes/src/nodes/image/image-parser.tspackages/kg-default-nodes/src/nodes/image/image-renderer.tspackages/kg-default-nodes/src/nodes/markdown/MarkdownNode.tspackages/kg-default-nodes/src/nodes/markdown/markdown-renderer.tspackages/kg-default-nodes/src/nodes/paywall/PaywallNode.tspackages/kg-default-nodes/src/nodes/paywall/paywall-parser.tspackages/kg-default-nodes/src/nodes/paywall/paywall-renderer.tspackages/kg-default-nodes/src/nodes/product/ProductNode.tspackages/kg-default-nodes/src/nodes/product/product-parser.tspackages/kg-default-nodes/src/nodes/product/product-renderer.tspackages/kg-default-nodes/src/nodes/signup/SignupNode.tspackages/kg-default-nodes/src/nodes/signup/signup-parser.tspackages/kg-default-nodes/src/nodes/signup/signup-renderer.tspackages/kg-default-nodes/src/nodes/toggle/ToggleNode.tspackages/kg-default-nodes/src/nodes/toggle/toggle-parser.tspackages/kg-default-nodes/src/nodes/toggle/toggle-renderer.tspackages/kg-default-nodes/src/nodes/transistor/TransistorNode.tspackages/kg-default-nodes/src/nodes/transistor/transistor-renderer.tspackages/kg-default-nodes/src/nodes/video/VideoNode.tspackages/kg-default-nodes/src/nodes/video/video-parser.tspackages/kg-default-nodes/src/nodes/video/video-renderer.tspackages/kg-default-nodes/src/nodes/zwnj/ZWNJNode.tspackages/kg-default-nodes/src/serializers/linebreak.tspackages/kg-default-nodes/src/serializers/paragraph.tspackages/kg-default-nodes/src/svg.d.tspackages/kg-default-nodes/src/utils/add-create-document-option.tspackages/kg-default-nodes/src/utils/build-clean-basic-html-for-element.tspackages/kg-default-nodes/src/utils/clean-dom.tspackages/kg-default-nodes/src/utils/escape-html.tspackages/kg-default-nodes/src/utils/get-available-image-widths.tspackages/kg-default-nodes/src/utils/get-resized-image-dimensions.tspackages/kg-default-nodes/src/utils/is-local-content-image.tspackages/kg-default-nodes/src/utils/is-unsplash-image.tspackages/kg-default-nodes/src/utils/read-caption-from-element.tspackages/kg-default-nodes/src/utils/read-image-attributes-from-element.tspackages/kg-default-nodes/src/utils/read-text-content.tspackages/kg-default-nodes/src/utils/render-empty-container.tspackages/kg-default-nodes/src/utils/render-helpers/email-button.tspackages/kg-default-nodes/src/utils/replacement-strings.tspackages/kg-default-nodes/src/utils/rgb-to-hex.tspackages/kg-default-nodes/src/utils/set-src-background-from-parent.tspackages/kg-default-nodes/src/utils/size-byte-converter.tspackages/kg-default-nodes/src/utils/slugify.tspackages/kg-default-nodes/src/utils/srcset-attribute.tspackages/kg-default-nodes/src/utils/tagged-template-fns.tspackages/kg-default-nodes/src/utils/truncate.tspackages/kg-default-nodes/src/utils/visibility.tspackages/kg-default-nodes/test/generate-decorator-node.test.tspackages/kg-default-nodes/test/nodes/aside.test.tspackages/kg-default-nodes/test/nodes/at-link-search.test.tspackages/kg-default-nodes/test/nodes/at-link.test.tspackages/kg-default-nodes/test/nodes/audio.test.tspackages/kg-default-nodes/test/nodes/bookmark.test.tspackages/kg-default-nodes/test/nodes/button.test.tspackages/kg-default-nodes/test/nodes/call-to-action.test.tspackages/kg-default-nodes/test/nodes/callout.test.tspackages/kg-default-nodes/test/nodes/codeblock.test.tspackages/kg-default-nodes/test/nodes/email-cta.test.tspackages/kg-default-nodes/test/nodes/email.test.tspackages/kg-default-nodes/test/nodes/embed.test.tspackages/kg-default-nodes/test/nodes/file.test.tspackages/kg-default-nodes/test/nodes/gallery.test.tspackages/kg-default-nodes/test/nodes/header.test.tspackages/kg-default-nodes/test/nodes/horizontalrule.test.tspackages/kg-default-nodes/test/nodes/html.test.tspackages/kg-default-nodes/test/nodes/image.test.tspackages/kg-default-nodes/test/nodes/markdown.test.tspackages/kg-default-nodes/test/nodes/paywall.test.tspackages/kg-default-nodes/test/nodes/product.test.tspackages/kg-default-nodes/test/nodes/signup.test.tspackages/kg-default-nodes/test/nodes/tk.test.tspackages/kg-default-nodes/test/nodes/toggle.test.tspackages/kg-default-nodes/test/nodes/transistor.test.tspackages/kg-default-nodes/test/nodes/video.test.tspackages/kg-default-nodes/test/nodes/zwnj.test.tspackages/kg-default-nodes/test/serializers/linebreak.test.tspackages/kg-default-nodes/test/serializers/paragraph.test.tspackages/kg-default-nodes/test/test-utils/assertions.tspackages/kg-default-nodes/test/test-utils/html-minifier.d.tspackages/kg-default-nodes/test/test-utils/index.tspackages/kg-default-nodes/test/test-utils/overrides.tspackages/kg-default-nodes/test/test-utils/should-assertions.d.tspackages/kg-default-nodes/test/test-utils/should.d.tspackages/kg-default-nodes/test/utils/rgb-to-hex.test.tspackages/kg-default-nodes/test/utils/tagged-template-fns.test.tspackages/kg-default-nodes/test/utils/visibility.test.tspackages/kg-default-nodes/tsconfig.cjs.jsonpackages/kg-default-nodes/tsconfig.jsonpackages/kg-default-nodes/tsconfig.test.jsonpackages/koenig-lexical/test/unit/hooks/useVisibilityToggle.test.js
💤 Files with no reviewable changes (2)
- packages/kg-default-nodes/index.js
- packages/kg-default-nodes/rollup.config.mjs
✅ Files skipped from review due to trivial changes (27)
- packages/kg-default-nodes/src/serializers/paragraph.ts
- packages/kg-default-nodes/src/utils/get-available-image-widths.ts
- packages/kg-default-nodes/src/utils/render-empty-container.ts
- packages/kg-default-nodes/src/utils/escape-html.ts
- packages/kg-default-nodes/test/utils/tagged-template-fns.test.ts
- packages/kg-default-nodes/src/index.ts
- packages/kg-default-nodes/src/utils/is-unsplash-image.ts
- packages/kg-default-nodes/src/nodes/at-link/index.ts
- packages/kg-default-nodes/src/utils/build-clean-basic-html-for-element.ts
- packages/kg-default-nodes/src/serializers/linebreak.ts
- packages/kg-default-nodes/src/svg.d.ts
- packages/kg-default-nodes/src/utils/slugify.ts
- packages/kg-default-nodes/src/nodes/zwnj/ZWNJNode.ts
- packages/kg-default-nodes/src/utils/render-helpers/email-button.ts
- packages/kg-default-nodes/src/nodes/paywall/paywall-parser.ts
- packages/kg-default-nodes/test/nodes/horizontalrule.test.ts
- packages/kg-default-nodes/tsconfig.json
- packages/kg-default-nodes/src/utils/truncate.ts
- packages/kg-default-nodes/tsconfig.test.json
- packages/kg-default-nodes/test/nodes/gallery.test.ts
- packages/kg-default-nodes/tsconfig.cjs.json
- packages/kg-default-nodes/src/nodes/paywall/PaywallNode.ts
- packages/kg-default-nodes/test/nodes/codeblock.test.ts
- packages/kg-default-nodes/src/nodes/callout/callout-parser.ts
- packages/kg-default-nodes/test/nodes/header.test.ts
- packages/kg-default-nodes/test/nodes/html.test.ts
- packages/kg-default-nodes/src/nodes/embed/types/twitter.ts
🚧 Files skipped from review as they are similar to previous changes (90)
- packages/kg-default-nodes/src/nodes/product/ProductNode.ts
- packages/kg-default-nodes/src/utils/clean-dom.ts
- packages/kg-default-nodes/src/utils/rgb-to-hex.ts
- packages/kg-default-nodes/src/utils/read-caption-from-element.ts
- packages/kg-default-nodes/src/KoenigDecoratorNode.ts
- packages/kg-default-nodes/test/test-utils/should-assertions.d.ts
- packages/kg-default-nodes/src/utils/is-local-content-image.ts
- packages/kg-default-nodes/src/utils/get-resized-image-dimensions.ts
- packages/kg-default-nodes/src/utils/add-create-document-option.ts
- packages/kg-default-nodes/src/nodes/horizontalrule/horizontalrule-renderer.ts
- packages/kg-default-nodes/test/utils/rgb-to-hex.test.ts
- packages/kg-default-nodes/src/nodes/aside/AsideParser.ts
- packages/kg-default-nodes/src/nodes/button/button-parser.ts
- packages/kg-default-nodes/test/test-utils/overrides.ts
- packages/kg-default-nodes/src/nodes/header/HeaderNode.ts
- packages/kg-default-nodes/src/utils/size-byte-converter.ts
- packages/kg-default-nodes/src/utils/tagged-template-fns.ts
- packages/kg-default-nodes/src/nodes/email-cta/email-cta-renderer.ts
- packages/kg-default-nodes/src/nodes/file/file-renderer.ts
- packages/kg-default-nodes/test/nodes/zwnj.test.ts
- packages/kg-default-nodes/src/nodes/file/file-parser.ts
- packages/kg-default-nodes/test/test-utils/html-minifier.d.ts
- packages/kg-default-nodes/test/test-utils/should.d.ts
- packages/kg-default-nodes/src/nodes/button/button-renderer.ts
- packages/kg-default-nodes/src/nodes/email/EmailNode.ts
- packages/kg-default-nodes/test/nodes/button.test.ts
- packages/kg-default-nodes/test/nodes/toggle.test.ts
- packages/kg-default-nodes/src/utils/read-text-content.ts
- packages/kg-default-nodes/src/nodes/aside/AsideNode.ts
- packages/kg-default-nodes/src/nodes/image/ImageNode.ts
- packages/kg-default-nodes/src/nodes/video/video-parser.ts
- packages/kg-default-nodes/src/nodes/call-to-action/CallToActionNode.ts
- packages/kg-default-nodes/test/nodes/signup.test.ts
- packages/kg-default-nodes/src/nodes/bookmark/bookmark-renderer.ts
- packages/kg-default-nodes/src/nodes/audio/audio-parser.ts
- packages/kg-default-nodes/src/nodes/transistor/transistor-renderer.ts
- packages/kg-default-nodes/test/nodes/at-link-search.test.ts
- packages/kg-default-nodes/src/nodes/ExtendedQuoteNode.ts
- packages/kg-default-nodes/src/nodes/header/renderers/v1/header-renderer.ts
- packages/kg-default-nodes/test/nodes/audio.test.ts
- packages/kg-default-nodes/src/utils/replacement-strings.ts
- packages/kg-default-nodes/src/nodes/email/email-renderer.ts
- packages/kg-default-nodes/src/nodes/html/html-renderer.ts
- packages/kg-default-nodes/src/utils/set-src-background-from-parent.ts
- packages/kg-default-nodes/test/serializers/paragraph.test.ts
- packages/kg-default-nodes/src/utils/read-image-attributes-from-element.ts
- packages/kg-default-nodes/src/nodes/paywall/paywall-renderer.ts
- packages/kg-default-nodes/test/nodes/aside.test.ts
- packages/kg-default-nodes/src/nodes/button/ButtonNode.ts
- packages/kg-default-nodes/test/nodes/transistor.test.ts
- packages/kg-default-nodes/src/nodes/signup/signup-parser.ts
- packages/kg-default-nodes/src/nodes/gallery/GalleryNode.ts
- packages/kg-default-nodes/src/utils/srcset-attribute.ts
- packages/kg-default-nodes/src/nodes/image/image-parser.ts
- packages/kg-default-nodes/src/nodes/audio/AudioNode.ts
- packages/kg-default-nodes/src/nodes/audio/audio-renderer.ts
- packages/kg-default-nodes/src/nodes/file/FileNode.ts
- packages/kg-default-nodes/test/nodes/markdown.test.ts
- packages/kg-default-nodes/src/nodes/html/HtmlNode.ts
- packages/kg-default-nodes/test/nodes/video.test.ts
- packages/kg-default-nodes/test/nodes/tk.test.ts
- packages/kg-default-nodes/src/nodes/toggle/ToggleNode.ts
- packages/kg-default-nodes/src/nodes/markdown/MarkdownNode.ts
- packages/kg-default-nodes/src/nodes/header/renderers/v2/header-renderer.ts
- packages/kg-default-nodes/src/nodes/TKNode.ts
- packages/kg-default-nodes/src/nodes/codeblock/codeblock-parser.ts
- packages/kg-default-nodes/test/nodes/at-link.test.ts
- packages/kg-default-nodes/src/nodes/product/product-parser.ts
- packages/kg-default-nodes/test/nodes/email-cta.test.ts
- packages/kg-default-nodes/src/nodes/call-to-action/calltoaction-parser.ts
- packages/kg-default-nodes/test/nodes/product.test.ts
- packages/kg-default-nodes/src/nodes/gallery/gallery-parser.ts
- packages/kg-default-nodes/src/nodes/bookmark/bookmark-parser.ts
- packages/kg-default-nodes/src/nodes/codeblock/CodeBlockNode.ts
- packages/kg-default-nodes/src/nodes/callout/CalloutNode.ts
- packages/kg-default-nodes/src/nodes/header/parsers/header-parser.ts
- packages/kg-default-nodes/src/nodes/image/image-renderer.ts
- packages/kg-default-nodes/src/nodes/callout/callout-renderer.ts
- packages/kg-default-nodes/test/nodes/file.test.ts
- packages/kg-default-nodes/src/nodes/email-cta/EmailCtaNode.ts
- packages/kg-default-nodes/src/nodes/transistor/TransistorNode.ts
- packages/kg-default-nodes/test/nodes/embed.test.ts
- packages/kg-default-nodes/test/nodes/image.test.ts
- packages/kg-default-nodes/test/nodes/email.test.ts
- packages/kg-default-nodes/src/nodes/at-link/AtLinkNode.ts
- packages/kg-default-nodes/src/nodes/signup/signup-renderer.ts
- packages/kg-default-nodes/src/nodes/at-link/AtLinkSearchNode.ts
- packages/kg-default-nodes/test/nodes/callout.test.ts
- packages/kg-default-nodes/test/nodes/bookmark.test.ts
- packages/kg-default-nodes/src/kg-default-nodes.ts
| "build": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", | ||
| "prepare": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", | ||
| "pretest": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json && tsc -p tsconfig.test.json", |
There was a problem hiding this comment.
Clean build/ before emitting artifacts.
tsc does not remove outputs for renamed or deleted source files. Because this package now publishes the whole build directory, stale JS/DTs can survive a local rebuild and get shipped.
🧹 Suggested script update
"scripts": {
"dev": "tsc --watch --preserveWatchOutput",
- "build": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json",
- "prepare": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json",
- "pretest": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json && tsc -p tsconfig.test.json",
+ "clean": "node -e \"require('node:fs').rmSync('build', {recursive: true, force: true})\"",
+ "build": "yarn clean && tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json",
+ "prepare": "yarn build",
+ "pretest": "yarn build && tsc -p tsconfig.test.json",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "build": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", | |
| "prepare": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", | |
| "pretest": "tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json && tsc -p tsconfig.test.json", | |
| "scripts": { | |
| "dev": "tsc --watch --preserveWatchOutput", | |
| "clean": "node -e \"require('node:fs').rmSync('build', {recursive: true, force: true})\"", | |
| "build": "yarn clean && tsc && tsc -p tsconfig.cjs.json && echo '{\"type\":\"module\"}' > build/esm/package.json", | |
| "prepare": "yarn build", | |
| "pretest": "yarn build && tsc -p tsconfig.test.json", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/package.json` around lines 19 - 21, The
package.json scripts ("build", "prepare", "pretest") run tsc but do not clean
stale artifacts; modify each script to remove/clean the build directory before
running tsc (e.g., run a cross-platform cleaner like rimraf build or rm -rf
build && mkdir -p build) so old JS/.d.ts files are deleted before emitting, then
run the existing tsc commands and the echo step; update "build", "prepare", and
"pretest" to include this pre-clean step (or add a separate "clean" script and
invoke it from those scripts).
| if (isEmail && isVideoWithThumbnail) { | ||
| const emailTemplateMaxWidth = 600; | ||
| const thumbnailAspectRatio = metadata.thumbnail_width / metadata.thumbnail_height; | ||
| const thumbnailAspectRatio = metadata.thumbnail_width! / metadata.thumbnail_height!; | ||
| const spacerWidth = Math.round(emailTemplateMaxWidth / 4); | ||
| const spacerHeight = Math.round(emailTemplateMaxWidth / thumbnailAspectRatio); |
There was a problem hiding this comment.
Guard thumbnail dimensions before computing aspect ratio.
Line 53 uses non-null assertions on optional metadata fields. If width/height is missing or zero, this can generate invalid spacerHeight and broken email HTML.
Suggested fix
- if (isEmail && isVideoWithThumbnail) {
+ const hasValidThumbnailSize =
+ typeof metadata.thumbnail_width === 'number' &&
+ typeof metadata.thumbnail_height === 'number' &&
+ metadata.thumbnail_width > 0 &&
+ metadata.thumbnail_height > 0;
+
+ if (isEmail && isVideoWithThumbnail && hasValidThumbnailSize) {
const emailTemplateMaxWidth = 600;
- const thumbnailAspectRatio = metadata.thumbnail_width! / metadata.thumbnail_height!;
+ const thumbnailAspectRatio = metadata.thumbnail_width / metadata.thumbnail_height;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isEmail && isVideoWithThumbnail) { | |
| const emailTemplateMaxWidth = 600; | |
| const thumbnailAspectRatio = metadata.thumbnail_width / metadata.thumbnail_height; | |
| const thumbnailAspectRatio = metadata.thumbnail_width! / metadata.thumbnail_height!; | |
| const spacerWidth = Math.round(emailTemplateMaxWidth / 4); | |
| const spacerHeight = Math.round(emailTemplateMaxWidth / thumbnailAspectRatio); | |
| const hasValidThumbnailSize = | |
| typeof metadata.thumbnail_width === 'number' && | |
| typeof metadata.thumbnail_height === 'number' && | |
| metadata.thumbnail_width > 0 && | |
| metadata.thumbnail_height > 0; | |
| if (isEmail && isVideoWithThumbnail && hasValidThumbnailSize) { | |
| const emailTemplateMaxWidth = 600; | |
| const thumbnailAspectRatio = metadata.thumbnail_width / metadata.thumbnail_height; | |
| const spacerWidth = Math.round(emailTemplateMaxWidth / 4); | |
| const spacerHeight = Math.round(emailTemplateMaxWidth / thumbnailAspectRatio); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/src/nodes/embed/embed-renderer.ts` around lines 51
- 55, The code in the isEmail && isVideoWithThumbnail branch uses non-null
assertions on metadata.thumbnail_width and metadata.thumbnail_height (line
computing thumbnailAspectRatio) which can be missing or zero and lead to
division-by-zero or invalid spacerHeight; update the guard in that block to
verify metadata.thumbnail_width and metadata.thumbnail_height are present and >
0 before computing thumbnailAspectRatio and spacerHeight, and if they are
absent/invalid either skip thumbnail sizing logic (avoid computing spacerHeight)
or use a safe default aspect ratio (e.g., 16/9) so spacerHeight and spacerWidth
are always valid; adjust the calculations that set spacerHeight/spacerWidth (and
any downstream use) accordingly to reference the validated values.
| const html = render(node.markdown || '', options as Record<string, unknown>); | ||
|
|
||
| const element = document.createElement('div'); | ||
| element.innerHTML = html; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the currently used version of @tryghost/kg-markdown-html-renderer, is output HTML sanitized by default? If yes, provide the exact sanitization guarantees and configuration options.
💡 Result:
No. In the current release (@tryghost/kg-markdown-html-renderer 7.1.16), the renderer does not sanitize output HTML by default. [1]
What it does instead
- It instantiates
markdown-itwithhtml: true, meaning raw HTML embedded in the Markdown is allowed through. [2] - The exported
render(markdown, options)function simply returnsmarkdownIt.render(markdown)with no sanitization step (no DOMPurify/sanitize-html/allowlist cleaning). [2]
Sanitization guarantees
- None are provided by this package (no documented or implemented XSS/unsafe-HTML stripping/attribute filtering guarantees). [2]
Configuration options related to sanitization
- None. The only exposed option is
ghostVersion?: string, used to select a renderer variant; it does not enable/disable sanitization. [2]
Sources: jsDelivr package page for current version [1]; implementation in src/markdown-html-renderer.ts [2].
🏁 Script executed:
cd packages/kg-default-nodes && cat -n src/nodes/markdown/markdown-renderer.ts | head -50Repository: TryGhost/Koenig
Length of output: 1127
🏁 Script executed:
rg -A 5 -B 5 "node\.markdown" packages/kg-default-nodes/src/nodes/markdown/markdown-renderer.tsRepository: TryGhost/Koenig
Length of output: 478
🏁 Script executed:
rg "render\(" packages/kg-default-nodes/src/nodes/markdown/ -A 3 -B 3Repository: TryGhost/Koenig
Length of output: 771
🏁 Script executed:
fd "markdown" packages/kg-default-nodes/src --type f | head -20Repository: TryGhost/Koenig
Length of output: 185
🏁 Script executed:
cat -n packages/kg-default-nodes/src/nodes/markdown/MarkdownNode.tsRepository: TryGhost/Koenig
Length of output: 946
🏁 Script executed:
rg "MarkdownNode" packages/kg-default-nodes/src --type ts -B 2 -A 2 | head -60Repository: TryGhost/Koenig
Length of output: 4557
🏁 Script executed:
rg "DOMPurify|sanitize|xss" packages/kg-default-nodes/src --type ts | head -20Repository: TryGhost/Koenig
Length of output: 41
🏁 Script executed:
rg "innerHTML" packages/kg-default-nodes/src --type ts -B 3 -A 1Repository: TryGhost/Koenig
Length of output: 29434
XSS vulnerability: innerHTML receives unsanitized HTML from renderer.
Line 22 writes HTML directly into the DOM. The @tryghost/kg-markdown-html-renderer package does not sanitize output by default—it passes raw HTML through as-is. If node.markdown contains untrusted input, this creates an exploitable XSS vulnerability.
Recommended fixes:
- Sanitize the output before assignment (use DOMPurify or similar), or
- Switch to
textContentif plain text rendering is sufficient, or - Enforce strict input validation on
node.markdownupstream and document the assumption that input is always trusted.
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 21-21: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: element.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 21-21: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: element.innerHTML = html
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/src/nodes/markdown/markdown-renderer.ts` around
lines 19 - 23, The code assigns unsanitized HTML from render(node.markdown)
directly to element.innerHTML in markdown-renderer.ts, creating an XSS risk; fix
by sanitizing the rendered HTML (e.g., run the output of render(...) through a
sanitizer like DOMPurify before assigning to element.innerHTML) or, if only
plain text is needed, set element.textContent to node.markdown instead; update
the code paths around render, node.markdown and element.innerHTML to use the
sanitizer API (or textContent) and add a short comment documenting the trust
assumption if you opt to keep raw HTML.
| interface VideoRenderOptions { | ||
| createDocument?: () => Document; | ||
| dom?: { window: { document: Document } }; | ||
| target?: string; | ||
| postUrl?: string; | ||
| [key: string]: unknown; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether any email-target renderVideoNode call lacks postUrl
# Expected: count_without_postUrl should be 0
set -euo pipefail
echo "All renderVideoNode calls with target: 'email':"
ast-grep --lang ts --pattern "renderVideoNode($NODE, {$$$, target: 'email', $$$})" || true
count_all=$(ast-grep --lang ts --pattern "renderVideoNode($NODE, {$$$, target: 'email', $$$})" | wc -l | tr -d ' ')
count_with_post=$(ast-grep --lang ts --pattern "renderVideoNode($NODE, {$$$, target: 'email', $$$, postUrl: $URL, $$$})" | wc -l | tr -d ' ')
echo "count_all_email_target_calls=$count_all"
echo "count_with_postUrl=$count_with_post"
echo "count_without_postUrl=$((count_all-count_with_post))"Repository: TryGhost/Koenig
Length of output: 165
🏁 Script executed:
cat -n packages/kg-default-nodes/src/nodes/video/video-renderer.ts | head -200Repository: TryGhost/Koenig
Length of output: 10560
🏁 Script executed:
fd -e ts -e js | xargs rg "renderVideoNode" -A 2 -B 2Repository: TryGhost/Koenig
Length of output: 1373
🏁 Script executed:
rg "renderVideoNode\s*\(" -A 3 | head -100Repository: TryGhost/Koenig
Length of output: 474
🏁 Script executed:
rg "defaultRenderFn|target.*email|email.*target" -A 2 -B 2 | head -150Repository: TryGhost/Koenig
Length of output: 12550
🏁 Script executed:
fd -e test.ts -e test.js | xargs rg -l "video|Video" | head -10Repository: TryGhost/Koenig
Length of output: 656
🏁 Script executed:
cat -n packages/kg-default-nodes/test/nodes/video.test.tsRepository: TryGhost/Koenig
Length of output: 30846
🏁 Script executed:
rg "target.*email" -A 5 -B 5 packages/kg-default-nodes/test/ | head -200Repository: TryGhost/Koenig
Length of output: 18445
🏁 Script executed:
rg "target.*email" -A 3 packages/kg-default-nodes/test/nodes/product.test.ts | head -80Repository: TryGhost/Koenig
Length of output: 816
🏁 Script executed:
fd -e ts "product.*renderer" packages/kg-default-nodes/src/Repository: TryGhost/Koenig
Length of output: 122
🏁 Script executed:
rg "postUrl|href=" packages/kg-default-nodes/src/nodes/product/product-renderer.ts -B 2 -A 2Repository: TryGhost/Koenig
Length of output: 1480
🏁 Script executed:
sed -n '113,164p' packages/kg-default-nodes/src/nodes/video/video-renderer.tsRepository: TryGhost/Koenig
Length of output: 3324
Make postUrl required when target is 'email' using discriminated union types.
The interface allows postUrl to be optional (line 20), but emailCardTemplate directly interpolates it into href attributes on lines 129 and 153 without null-checking. If target === 'email' but postUrl is undefined, this generates href="undefined" and breaks email CTA links.
🔧 Suggested typing fix (discriminated options)
-interface VideoRenderOptions {
+interface BaseVideoRenderOptions {
createDocument?: () => Document;
dom?: { window: { document: Document } };
- target?: string;
- postUrl?: string;
[key: string]: unknown;
}
+type VideoRenderOptions =
+ | (BaseVideoRenderOptions & {target: 'email'; postUrl: string})
+ | (BaseVideoRenderOptions & {target?: string; postUrl?: string});
+
export function renderVideoNode(node: VideoNodeData, options: VideoRenderOptions = {}) {
@@
-export function emailCardTemplate({node, options, cardClasses}: {node: VideoNodeData, options: VideoRenderOptions, cardClasses: string}) {
+type EmailVideoRenderOptions = BaseVideoRenderOptions & {target: 'email'; postUrl: string};
+
+export function emailCardTemplate({node, options, cardClasses}: {node: VideoNodeData, options: EmailVideoRenderOptions, cardClasses: string}) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-default-nodes/src/nodes/video/video-renderer.ts` around lines 16
- 21, The VideoRenderOptions interface currently makes postUrl optional which
allows target === 'email' with postUrl undefined; change VideoRenderOptions to a
discriminated union (e.g., one variant where target: 'email' and postUrl:
string, and another for other targets where postUrl?: string) so TypeScript
enforces postUrl when target is 'email'; update any references or function
signatures that accept VideoRenderOptions (e.g., the function that uses
emailCardTemplate) to use the new union type so the places that interpolate
postUrl into href must have a defined string.
1 participant
Summary
anytypes andeslint-disablecomments from source (Visibility interface, set-src-background-from-parent browser types, generate-decorator-node exportJSON)tsconfig.test.jsonwith full test type-checking — all 30+ test files converted frombuild/cjstosrcimports with proper type annotations"node"condition),moduleResolution→NodeNext,tseslint.config()→defineConfig()overrides.tstyping withas unknown aspattern