Harden active rendered surfaces against XSS#82
Conversation
📝 WalkthroughWalkthroughAdded comprehensive XSS prevention across the component library by introducing a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The review scope is substantial with 18 affected files including new security-critical modules ( Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nostr-profile/render.ts (1)
61-68:⚠️ Potential issue | 🟠 MajorEscape
npubbefore interpolating it into the follow-button attribute.
npubis a plain string inRenderProfileOptions; inserting it raw into an HTML attribute can reopen attribute-injection XSS if an invalid value reaches this renderer.🛡️ Proposed fix
const banner = sanitizeHttpUrl(userProfile?.banner || ''); const safeDisplayName = escapeHtml(displayName); + const safeNpub = escapeHtml(npub); const renderFollowButton = () => { if (!showFollow || npub === '') return ''; return ` <nostr-follow-button - npub="${npub}"> + npub="${safeNpub}"> </nostr-follow-button> `; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nostr-profile/render.ts` around lines 61 - 68, The follow-button interpolation uses raw npub which can enable attribute-injection XSS; in renderFollowButton (the function returning the <nostr-follow-button> string) escape npub before inserting it into the npub attribute (use the existing escapeHtml helper like safeDisplayName does) and interpolate the escaped value instead of the raw npub, keeping the existing showFollow and empty-npub guard logic.
🧹 Nitpick comments (5)
src/nostr-livestream/render.ts (1)
170-194: Optional: don't echo the rejected URL back to the user.When the recording URL fails sanitization, the fallback renders
Watch Recording: ${escapeHtml(url)}which surfaces ajavascript:…/data:…/ malformed string to the reader. It's escaped so it can't execute, but it's also not useful information. A generic "Recording unavailable" (or simply omitting the block) would be a better UX without changing the security posture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nostr-livestream/render.ts` around lines 170 - 194, In renderRecordingLink, avoid echoing the rejected URL back to the user on sanitize failure; instead, when sanitizeHttpUrl(url) returns falsy, render a generic fallback (e.g., "Recording unavailable" or omit the block) rather than using escapeHtml(url). Update the false-branch in renderRecordingLink to show a non-clickable message without the original URL string and keep the clickable branch unchanged.src/common/sanitize.ts (1)
32-47:sanitizeHttpUrlreturns HTML-entity-escaped output; callers must only inject it into HTML attribute context.
escapeHtml(parsedUrl.toString())turns&into&,"into", etc. That is correct insidehref="..."/src="...", but the returned value is not a plain URL — using it as a fetch target, as awindow.locationassignment, as an<a>'s.hrefDOM property, or for equality comparison will be wrong (e.g.,?a=1&b=2). Consider:
- renaming to something like
sanitizeHttpUrlForAttrto signal the contract, and/or- adding a short JSDoc note warning that the result is attribute-safe (HTML-escaped), not a bare URL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/sanitize.ts` around lines 32 - 47, The function sanitizeHttpUrl currently returns an HTML-escaped string (via escapeHtml(parsedUrl.toString())), which is safe for injecting into HTML attributes but not for programmatic use (fetch, window.location, .href, comparisons); update the API to make this contract explicit by either renaming sanitizeHttpUrl to something like sanitizeHttpUrlForAttr and adding a JSDoc on that function stating the output is HTML-escaped and only for attribute contexts, and also provide (or suggest adding) a separate sanitizeHttpUrlRaw (or an option/flag) that returns the unescaped validated URL string; locate references to sanitizeHttpUrl and adjust callsites or add the new function accordingly so callers that need a raw URL use the unescaped validator instead of the escaped output.src/common/__tests__/sanitize.test.ts (1)
1-42: Consider adding a test forsanitizePostInlineFragment.The third exported helper isn't covered here. A quick test in the Node/Vitest environment would lock in the non-browser fallback behavior (whatever you decide it should be — see the comment on
sanitize.tsline 53-66) and make regressions loud. Even a single assertion confirming that a crafted<script>payload is neutralized would be valuable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common/__tests__/sanitize.test.ts` around lines 1 - 42, Add a unit test for the exported function sanitizePostInlineFragment alongside the existing sanitizeHttpUrl/sanitizeMultilineText tests: import sanitizePostInlineFragment and assert that a crafted inline payload (e.g., containing a <script> tag or on* attribute) is neutralized and returned as safe HTML (matching the non-browser fallback behavior), plus at least one assertion that preserves allowed content/line breaks if applicable; this will lock in sanitizePostInlineFragment's Node/Vitest behavior and catch regressions.src/nostr-zap-button/__tests__/dialog-zappers.test.ts (1)
6-27: Good XSS regression test; consider expanding coverage.The test nails the core cases (HTML escape, newline→
<br />,javascript:picture neutralized). Two cheap additions that would strengthen this suite without much effort:
- Assert the
<div class="zap-entry" …>contains the expected escapeddata-author-pubkeywhenauthorPubkeycontains"/<characters.- Cover the
authorNameescape path with a<script>-bearing name to pin down that contract too.Not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nostr-zap-button/__tests__/dialog-zappers.test.ts` around lines 6 - 27, The test should be extended to assert that renderZapEntry properly escapes special chars in attributes and author name: update the existing test (or add a new one) that calls renderZapEntry with an authorPubkey containing characters like '"' and '<' and an authorName containing a <script> payload, then assert the resulting HTML's <div class="zap-entry" ...> contains the expected escaped value in the data-author-pubkey attribute and that the authorName appears escaped (no raw <script> tag present). Use the same renderZapEntry helper in dialog-zappers.test.ts so the new assertions verify attribute-escaping and name-escaping paths alongside the existing comment-escaping checks.src/nostr-profile/__tests__/render.test.ts (1)
6-103: LGTM — covers both escaping and URL-scheme omission paths.Good mix:
displayName/errorMessageescaping plusjavascript:/data:omission for picture, website, and banner with placeholder assertions. One small nit: consider also asserting thathref="https://…"does appear for a validwebsitein a positive-path test to guard against an over-aggressive regression insanitizeHttpUrllater.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nostr-profile/__tests__/render.test.ts` around lines 6 - 103, Add a positive-path assertion that a valid http(s) website is rendered to guard against over-aggressive URL sanitization: update the test that calls renderProfile (the second "omits invalid banner..." case or add a new small test) to include a userProfile.website set to "https://example.com" (or similar) and assert the produced HTML contains href="https://example.com" (or a properly escaped equivalent). Locate the call to renderProfile in this test and add the assertion so it verifies renderProfile (and underlying sanitizeHttpUrl) allows legitimate https URLs while still omitting javascript: and data: schemes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/common/sanitize.ts`:
- Around line 53-66: sanitizePostInlineFragment currently falls back to
sanitizeMultilineText in non-browser environments which re-escapes already-safe,
synthesized markup and mangles <a>/<span> mention HTML; change the non-browser
branch in sanitizePostInlineFragment so that when getPostInlinePurifier()
returns null it returns the input html unchanged (instead of calling
sanitizeMultilineText). This preserves the already-escaped dynamic pieces
produced by renderPostInlineText (which uses escapeHtml and sanitizeHttpUrl) and
the whitelisted tags/attrs injected by the pipeline.
In `@src/nostr-post/inline-fragment.ts`:
- Around line 10-52: The token encoding fails for payloads containing literal
parentheses because encodeURIComponent doesn't escape "(" or ")", so update the
token builders to percent-encode parentheses after encodeURIComponent: in
createProfileMentionToken call encodeURIComponent on href and displayName then
replace "(" with "%28" and ")" with "%29" (same for createUsernameMentionToken
on username) so the existing PROFILE_MENTION_TOKEN_REGEX and
USERNAME_MENTION_TOKEN_REGEX continue to parse unambiguously; no changes to
decode side required.
In `@src/nostr-post/parse-text.ts`:
- Around line 31-56: The loop in parse-text.ts that processes noteMatches uses
textContent.replace(fullMatch, ' ') which only replaces the first occurrence;
update all replacements (the occurrences using textContent.replace(fullMatch,
...), textContent.replace(replacement.original, replacement.replacement), and
similar calls) to replace every instance by using String.prototype.replaceAll or
a global RegExp with the literal escaped match (ensuring special regex chars are
escaped) so that all duplicate nostr:, note references, and `@username`
occurrences are removed/replaced; locate the logic around noteMatches, the
embeddedNotes handling, and the replacement code paths (variables fullMatch,
noteId, textContent, and replacement.original/replacement.replacement) and
switch them to replaceAll or an escaped global-regex alternative.
In `@src/nostr-post/render-content.ts`:
- Around line 145-152: The embedded post HTML inserts the date directly into the
template in renderEmbeddedPost, so escape the date before interpolation to avoid
XSS: call escapeHtml(date) where the template uses ${date} (same place that uses
displayName and nip05), update the construction of embedHtml (or the template
variable used before assigning to temp.innerHTML) to use the escaped value, and
ensure the rest of the flow (placeholder, temp, temp.firstElementChild!) remains
unchanged.
---
Outside diff comments:
In `@src/nostr-profile/render.ts`:
- Around line 61-68: The follow-button interpolation uses raw npub which can
enable attribute-injection XSS; in renderFollowButton (the function returning
the <nostr-follow-button> string) escape npub before inserting it into the npub
attribute (use the existing escapeHtml helper like safeDisplayName does) and
interpolate the escaped value instead of the raw npub, keeping the existing
showFollow and empty-npub guard logic.
---
Nitpick comments:
In `@src/common/__tests__/sanitize.test.ts`:
- Around line 1-42: Add a unit test for the exported function
sanitizePostInlineFragment alongside the existing
sanitizeHttpUrl/sanitizeMultilineText tests: import sanitizePostInlineFragment
and assert that a crafted inline payload (e.g., containing a <script> tag or on*
attribute) is neutralized and returned as safe HTML (matching the non-browser
fallback behavior), plus at least one assertion that preserves allowed
content/line breaks if applicable; this will lock in
sanitizePostInlineFragment's Node/Vitest behavior and catch regressions.
In `@src/common/sanitize.ts`:
- Around line 32-47: The function sanitizeHttpUrl currently returns an
HTML-escaped string (via escapeHtml(parsedUrl.toString())), which is safe for
injecting into HTML attributes but not for programmatic use (fetch,
window.location, .href, comparisons); update the API to make this contract
explicit by either renaming sanitizeHttpUrl to something like
sanitizeHttpUrlForAttr and adding a JSDoc on that function stating the output is
HTML-escaped and only for attribute contexts, and also provide (or suggest
adding) a separate sanitizeHttpUrlRaw (or an option/flag) that returns the
unescaped validated URL string; locate references to sanitizeHttpUrl and adjust
callsites or add the new function accordingly so callers that need a raw URL use
the unescaped validator instead of the escaped output.
In `@src/nostr-livestream/render.ts`:
- Around line 170-194: In renderRecordingLink, avoid echoing the rejected URL
back to the user on sanitize failure; instead, when sanitizeHttpUrl(url) returns
falsy, render a generic fallback (e.g., "Recording unavailable" or omit the
block) rather than using escapeHtml(url). Update the false-branch in
renderRecordingLink to show a non-clickable message without the original URL
string and keep the clickable branch unchanged.
In `@src/nostr-profile/__tests__/render.test.ts`:
- Around line 6-103: Add a positive-path assertion that a valid http(s) website
is rendered to guard against over-aggressive URL sanitization: update the test
that calls renderProfile (the second "omits invalid banner..." case or add a new
small test) to include a userProfile.website set to "https://example.com" (or
similar) and assert the produced HTML contains href="https://example.com" (or a
properly escaped equivalent). Locate the call to renderProfile in this test and
add the assertion so it verifies renderProfile (and underlying sanitizeHttpUrl)
allows legitimate https URLs while still omitting javascript: and data: schemes.
In `@src/nostr-zap-button/__tests__/dialog-zappers.test.ts`:
- Around line 6-27: The test should be extended to assert that renderZapEntry
properly escapes special chars in attributes and author name: update the
existing test (or add a new one) that calls renderZapEntry with an authorPubkey
containing characters like '"' and '<' and an authorName containing a <script>
payload, then assert the resulting HTML's <div class="zap-entry" ...> contains
the expected escaped value in the data-author-pubkey attribute and that the
authorName appears escaped (no raw <script> tag present). Use the same
renderZapEntry helper in dialog-zappers.test.ts so the new assertions verify
attribute-escaping and name-escaping paths alongside the existing
comment-escaping checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1e7bb25-6d3b-4042-8d3a-3f537009ae31
📒 Files selected for processing (18)
src/common/__tests__/sanitize.test.tssrc/common/sanitize.tssrc/common/utils.tssrc/nostr-follow-button/render.tssrc/nostr-like-button/dialog-likers.tssrc/nostr-livestream/render.tssrc/nostr-post/inline-fragment.tssrc/nostr-post/nostr-post.tssrc/nostr-post/parse-text.tssrc/nostr-post/render-content.tssrc/nostr-post/render.tssrc/nostr-profile-badge/render.tssrc/nostr-profile/__tests__/render.test.tssrc/nostr-profile/render.tssrc/nostr-profile/style.tssrc/nostr-zap-button/__tests__/dialog-zappers.test.tssrc/nostr-zap-button/dialog-zappers.tssrc/nostr-zap-button/render-zap-entry.ts
| export function sanitizePostInlineFragment(html: string): string { | ||
| const purifier = getPostInlinePurifier(); | ||
|
|
||
| if (!purifier) { | ||
| return sanitizeMultilineText(html); | ||
| } | ||
|
|
||
| return purifier.sanitize(html, { | ||
| ALLOWED_TAGS: POST_INLINE_ALLOWED_TAGS, | ||
| ALLOWED_ATTR: POST_INLINE_ALLOWED_ATTRS, | ||
| ALLOW_DATA_ATTR: false, | ||
| ALLOWED_URI_REGEXP: POST_INLINE_URI_REGEXP, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the only caller of sanitizePostInlineFragment and its input shape.
rg -nP -C3 '\bsanitizePostInlineFragment\s*\('Repository: saiy2k/nostr-components
Length of output: 836
🏁 Script executed:
# First, let's read the inline-fragment.ts file to see the full function
cat -n src/nostr-post/inline-fragment.tsRepository: saiy2k/nostr-components
Length of output: 2504
🏁 Script executed:
# Check the sanitizeMultilineText function to confirm it escapes HTML
rg -A10 'function sanitizeMultilineText' src/common/sanitize.tsRepository: saiy2k/nostr-components
Length of output: 392
🏁 Script executed:
# Check what tags and attributes are allowed in the POST_INLINE configuration
rg -B2 -A10 'POST_INLINE_ALLOWED_TAGS|POST_INLINE_ALLOWED_ATTRS|POST_INLINE_URI_REGEXP' src/common/sanitize.tsRepository: saiy2k/nostr-components
Length of output: 818
🏁 Script executed:
# Check the token replacement functions to understand what markup they generate
rg -B2 -A15 'function replaceProfileMentionTokens|function replaceUsernameMentionTokens' src/nostr-post/inline-fragment.tsRepository: saiy2k/nostr-components
Length of output: 1217
Non-browser environment will visibly mangle constructed mention HTML.
renderPostInlineText in src/nostr-post/inline-fragment.ts calls sanitizePostInlineFragment after it has synthesized <a …>@name</a> and <span class="nostr-mention" …>@user</span> markup from escaped tokens. In the browser, DOMPurify preserves those tags per the allowlist. In a non-browser environment (SSR, unit tests, or any consumer without window), the function falls back to sanitizeMultilineText, which calls escapeHtml() on the entire input — converting the freshly-built <a>/<span> tags into <a href=…>@alice</a> literal text rather than rendered mentions.
The input to sanitizePostInlineFragment is already safe: all dynamic content (href, displayName, username) is escaped by escapeHtml(), URLs are validated via sanitizeHttpUrl(), and only whitelisted tags (<a>, <span>, <br />) with whitelisted attributes are injected by the pipeline itself. Return the input unchanged in the non-browser path rather than re-escaping trusted internal markup.
Proposed fix
export function sanitizePostInlineFragment(html: string): string {
const purifier = getPostInlinePurifier();
if (!purifier) {
- return sanitizeMultilineText(html);
+ // No DOMPurify available (SSR / tests). The pipeline in
+ // renderPostInlineText has already escaped all dynamic text and only
+ // injected mention markup from a small, trusted allowlist, so return
+ // the fragment as-is rather than re-escaping our own <a>/<span>/<br>.
+ return html;
}
return purifier.sanitize(html, {📝 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 function sanitizePostInlineFragment(html: string): string { | |
| const purifier = getPostInlinePurifier(); | |
| if (!purifier) { | |
| return sanitizeMultilineText(html); | |
| } | |
| return purifier.sanitize(html, { | |
| ALLOWED_TAGS: POST_INLINE_ALLOWED_TAGS, | |
| ALLOWED_ATTR: POST_INLINE_ALLOWED_ATTRS, | |
| ALLOW_DATA_ATTR: false, | |
| ALLOWED_URI_REGEXP: POST_INLINE_URI_REGEXP, | |
| }); | |
| } | |
| export function sanitizePostInlineFragment(html: string): string { | |
| const purifier = getPostInlinePurifier(); | |
| if (!purifier) { | |
| // No DOMPurify available (SSR / tests). The pipeline in | |
| // renderPostInlineText has already escaped all dynamic text and only | |
| // injected mention markup from a small, trusted allowlist, so return | |
| // the fragment as-is rather than re-escaping our own <a>/<span>/<br>. | |
| return html; | |
| } | |
| return purifier.sanitize(html, { | |
| ALLOWED_TAGS: POST_INLINE_ALLOWED_TAGS, | |
| ALLOWED_ATTR: POST_INLINE_ALLOWED_ATTRS, | |
| ALLOW_DATA_ATTR: false, | |
| ALLOWED_URI_REGEXP: POST_INLINE_URI_REGEXP, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/sanitize.ts` around lines 53 - 66, sanitizePostInlineFragment
currently falls back to sanitizeMultilineText in non-browser environments which
re-escapes already-safe, synthesized markup and mangles <a>/<span> mention HTML;
change the non-browser branch in sanitizePostInlineFragment so that when
getPostInlinePurifier() returns null it returns the input html unchanged
(instead of calling sanitizeMultilineText). This preserves the already-escaped
dynamic pieces produced by renderPostInlineText (which uses escapeHtml and
sanitizeHttpUrl) and the whitelisted tags/attrs injected by the pipeline.
| const PROFILE_MENTION_TOKEN_PREFIX = '__NOSTRC_PROFILE_MENTION__('; | ||
| const PROFILE_MENTION_TOKEN_REGEX = | ||
| /__NOSTRC_PROFILE_MENTION__\(([^)]*)\)\(([^)]*)\)__/g; | ||
| const USERNAME_MENTION_TOKEN_PREFIX = '__NOSTRC_USERNAME_MENTION__('; | ||
| const USERNAME_MENTION_TOKEN_REGEX = | ||
| /__NOSTRC_USERNAME_MENTION__\(([^)]*)\)__/g; | ||
|
|
||
| export function createProfileMentionToken( | ||
| href: string, | ||
| displayName: string, | ||
| ): string { | ||
| return `${PROFILE_MENTION_TOKEN_PREFIX}${encodeURIComponent(href)})(${encodeURIComponent(displayName)})__`; | ||
| } | ||
|
|
||
| export function createUsernameMentionToken(username: string): string { | ||
| return `${USERNAME_MENTION_TOKEN_PREFIX}${encodeURIComponent(username)})__`; | ||
| } | ||
|
|
||
| function replaceProfileMentionTokens(fragment: string): string { | ||
| return fragment.replace( | ||
| PROFILE_MENTION_TOKEN_REGEX, | ||
| (_match, encodedHref, encodedDisplayName) => { | ||
| const href = sanitizeHttpUrl(decodeURIComponent(encodedHref)); | ||
| const displayName = escapeHtml(decodeURIComponent(encodedDisplayName)); | ||
|
|
||
| if (!href) { | ||
| return `@${displayName}`; | ||
| } | ||
|
|
||
| return `<a href="${href}" target="_blank" rel="noopener noreferrer">@${displayName}</a>`; | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| function replaceUsernameMentionTokens(fragment: string): string { | ||
| return fragment.replace( | ||
| USERNAME_MENTION_TOKEN_REGEX, | ||
| (_match, encodedUsername) => { | ||
| const username = escapeHtml(decodeURIComponent(encodedUsername)); | ||
| return `<span class="nostr-mention" data-username="${username}">@${username}</span>`; | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Token payload can contain literal ), which breaks the regex parser.
encodeURIComponent deliberately does not escape ( or ) (MDN reference). So a display name like "Alice (work)" or any username/href containing ) ends up inside the token body, and the greedy ([^)]*) groups in PROFILE_MENTION_TOKEN_REGEX / USERNAME_MENTION_TOKEN_REGEX either match the wrong span or fail entirely — leaving raw __NOSTRC_PROFILE_MENTION__(…)(…)__ strings visible to the user.
Encode ( and ) manually when building/consuming the token so the delimiters are unambiguous:
Proposed fix
+function encodeTokenPart(value: string): string {
+ return encodeURIComponent(value).replace(/\(/g, '%28').replace(/\)/g, '%29');
+}
+
export function createProfileMentionToken(
href: string,
displayName: string,
): string {
- return `${PROFILE_MENTION_TOKEN_PREFIX}${encodeURIComponent(href)})(${encodeURIComponent(displayName)})__`;
+ return `${PROFILE_MENTION_TOKEN_PREFIX}${encodeTokenPart(href)})(${encodeTokenPart(displayName)})__`;
}
export function createUsernameMentionToken(username: string): string {
- return `${USERNAME_MENTION_TOKEN_PREFIX}${encodeURIComponent(username)})__`;
+ return `${USERNAME_MENTION_TOKEN_PREFIX}${encodeTokenPart(username)})__`;
}(decodeURIComponent already handles %28/%29, so the decode side needs no changes.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/nostr-post/inline-fragment.ts` around lines 10 - 52, The token encoding
fails for payloads containing literal parentheses because encodeURIComponent
doesn't escape "(" or ")", so update the token builders to percent-encode
parentheses after encodeURIComponent: in createProfileMentionToken call
encodeURIComponent on href and displayName then replace "(" with "%28" and ")"
with "%29" (same for createUsernameMentionToken on username) so the existing
PROFILE_MENTION_TOKEN_REGEX and USERNAME_MENTION_TOKEN_REGEX continue to parse
unambiguously; no changes to decode side required.
| for (const match of noteMatches) { | ||
| const fullMatch = match[0]; | ||
| const noteId = match[2]; | ||
| const position = match.index || 0; | ||
|
|
||
| // Store the note ID and its position for later processing | ||
| embeddedNotes.push({ | ||
| id: noteId, | ||
| position: position, | ||
| }); | ||
|
|
||
| // Fetch the embedded post | ||
| try { | ||
| if (!embeddedPosts.has(noteId)) { | ||
| const embeddedPost = await nostrService.getPost(noteId); | ||
| if (embeddedPost) { | ||
| embeddedPosts.set(noteId, embeddedPost); | ||
| } | ||
| } catch (error) { | ||
| console.error(`Failed to fetch embedded post ${noteId}:`, error); | ||
| } | ||
|
|
||
| // Remove the note reference from the text to prevent @ symbols being added | ||
| textContent = textContent.replace(fullMatch, ' '); | ||
| } catch (error) { | ||
| console.error(`Failed to fetch embedded post ${noteId}:`, error); | ||
| } | ||
|
|
||
| // Handle Nostr URI schema for mentions - batch process to avoid multiple async operations | ||
| const nostrURISchemaMatches = [...textContent.matchAll(new RegExp(nip21.NOSTR_URI_REGEX, 'g'))]; | ||
| const uriReplacements: { original: string; replacement: string }[] = []; | ||
|
|
||
| // Process all URIs concurrently | ||
| const uriPromises = nostrURISchemaMatches.map(async (match) => { | ||
| try { | ||
| const parsedNostrURI = nip21.parse(match[0]); | ||
| const decordedData = parsedNostrURI.decoded.data; | ||
|
|
||
| let pubkey = ''; | ||
| if (typeof decordedData === 'string') { | ||
| pubkey = decordedData; | ||
| } else { | ||
| pubkey = (decordedData as ProfilePointer).pubkey; | ||
| } | ||
|
|
||
| if (pubkey) { | ||
| const user = nostrService.getNDK().getUser({ pubkey }); | ||
| const profile = await user.fetchProfile(); | ||
| const name = profile?.displayName || ''; | ||
| // Remove the note reference from the text to prevent @ symbols being added | ||
| textContent = textContent.replace(fullMatch, ' '); | ||
| } |
There was a problem hiding this comment.
Minor: String.prototype.replace with a string arg only replaces the first occurrence.
In the three occurrences where textContent.replace(fullMatch, ...) / textContent.replace(replacement.original, replacement.replacement) are called (lines 55, 100-103, 123-126), passing a string as the first argument only replaces the first match. If the same nostr:… URI, note reference, or @username appears more than once in a post, later occurrences will remain in the text and may be rendered raw or mismatched against subsequent regex passes. Sanitization downstream (renderPostInlineText → DOMPurify) still prevents XSS, but the user-visible rendering is subtly broken for duplicate mentions/notes.
This appears to be pre-existing behavior; just worth flagging since the refactor reorganized this codepath. A simple fix is to use replaceAll (or a global regex) with the literal escaped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/nostr-post/parse-text.ts` around lines 31 - 56, The loop in parse-text.ts
that processes noteMatches uses textContent.replace(fullMatch, ' ') which only
replaces the first occurrence; update all replacements (the occurrences using
textContent.replace(fullMatch, ...), textContent.replace(replacement.original,
replacement.replacement), and similar calls) to replace every instance by using
String.prototype.replaceAll or a global RegExp with the literal escaped match
(ensuring special regex chars are escaped) so that all duplicate nostr:, note
references, and `@username` occurrences are removed/replaced; locate the logic
around noteMatches, the embeddedNotes handling, and the replacement code paths
(variables fullMatch, noteId, textContent, and
replacement.original/replacement.replacement) and switch them to replaceAll or
an escaped global-regex alternative.
| const temp = document.createElement('div'); | ||
| temp.innerHTML = embedHtml; | ||
|
|
||
| // Replace the placeholder with the embedded post | ||
| placeholder.parentNode?.replaceChild( | ||
| temp.firstElementChild!, | ||
| placeholder | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export async function renderEmbeddedPostContent( | ||
| noteId: string, | ||
| embeddedPosts: Map<string, NDKEvent>, | ||
| event: NDKEvent | null, | ||
| nostrService: any | ||
| ): Promise<string> { | ||
| const post = embeddedPosts.get(noteId); | ||
| if (!post) return '<div class="embedded-post-error">Post not found</div>'; | ||
|
|
||
| let authorProfile: NDKUserProfile | null = null; | ||
| try { | ||
| authorProfile = await post.author.fetchProfile(); | ||
| } catch (error) { | ||
| console.error( | ||
| `Failed to fetch profile for embedded post ${noteId}:`, | ||
| error | ||
| // Replace the placeholder with the embedded post | ||
| placeholder.parentNode?.replaceChild( | ||
| temp.firstElementChild!, | ||
| placeholder, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect renderEmbeddedPost to confirm it escapes displayName / nip05
fd -t f 'render.ts' src/nostr-post | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'Repository: saiy2k/nostr-components
Length of output: 8621
Escape the date parameter in renderEmbeddedPost.
displayName and nip05 are correctly escaped via escapeHtml() before interpolation, mitigating the original concern. However, the date parameter is inserted directly into the template without escaping (<div class="embedded-post-date">${date}</div>). Apply escapeHtml(date) to prevent XSS if this parameter becomes user-controllable.
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 145-145: 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: temp.innerHTML = embedHtml
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] 145-145: 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: temp.innerHTML = embedHtml
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 `@src/nostr-post/render-content.ts` around lines 145 - 152, The embedded post
HTML inserts the date directly into the template in renderEmbeddedPost, so
escape the date before interpolation to avoid XSS: call escapeHtml(date) where
the template uses ${date} (same place that uses displayName and nip05), update
the construction of embedHtml (or the template variable used before assigning to
temp.innerHTML) to use the escaped value, and ensure the rest of the flow
(placeholder, temp, temp.firstElementChild!) remains unchanged.
1 participant
Closes #44
Summary
This PR implements a focused XSS hardening pass across the active/exported rendered surfaces, with the primary fix centered on
nostr-postand extended to the other active components and dialogs that were still interpolating untrusted text or URLs too loosely.The approach stays intentionally narrow:
nostr-postinline content path uses a limited DOMPurify pass only for the small generated inline fragmentWhat Changed
Shared sanitization layer
Added a shared sanitization module under
src/commonwith:sanitizeHttpUrl(url)http:/https:''for invalid or disallowed URLssanitizeMultilineText(text)<br />sanitizePostInlineFragment(html)nostr-postinline fragment patha,span,brhref,target,rel,class,data-usernameAlso updated
escapeHtml()to a Node-safe string implementation so the helper and render tests can run cleanly in the current Vitest environment.nostr-posthardeningRe-secured the post parse/render flow so user text is never inserted raw:
This preserves intended behavior like:
njump.melinkswhile preventing raw injected markup/scripts from surviving.
Other active surface hardening
Applied the same escape-or-validate rule to the remaining active renderers:
nostr-profilebanner,picture, andwebsitenostr-profile-badgenostr-follow-buttonnostr-zap-buttonzappers dialognostr-like-buttonlikers dialognostr-livestreamTesting
Added targeted Node-safe tests for:
Commands run:
npm test -- --runnpm run buildnpm run build-storybookVerification Notes
The new tests passed.
buildandbuild-storybookstill surface pre-existing TypeScript/declaration warnings in untouched legacy/disabled areas such as:src/nostr-comment/*src/nostr-dm/*src/nostr-live-chat/*src/nostr-zap-button/zap-utils.tsThese warnings were already outside the scope of this issue and were not introduced by this PR.
Summary by CodeRabbit
Bug Fixes
Improvements