Fix XSS vulnerabilities in markdown links and embed titles#56
Merged
Glitchii merged 2 commits intoGlitchii:mainfrom Apr 11, 2026
Merged
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideTightens markdown link rendering and embed title handling to prevent XSS by validating link URLs and HTML-encoding user-controlled text before inserting it into the DOM. Sequence diagram for safe embed title editingsequenceDiagram
actor User
participant GUI as GuiForm
participant Script as ScriptJS
participant DOM
User->>GUI: Edit embed title field
GUI->>Script: Input event with new title value
Script->>Script: Set embedObj.title = value
Script->>Script: safeTitle = encodeHTML(value)
Script->>DOM: Update guiEmbedName .text innerHTML
Note right of DOM: Uses Embed N: <span>safeTitle</span>
Script->>Script: buildEmbed(only, index)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 security issues, 2 other issues, and left some high level feedback:
Security issues:
- User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
guiEmbedName.querySelector('.text').innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
guiEmbedName.querySelector('.text').innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
General comments:
- In the markdown link replacement,
safeHrefonly replaces double quotes; consider using the sameencodeHTML(or a dedicated attribute-escaping helper) for thehrefvalue as well to robustly handle<,>, and&in URLs. - For the embed title rendering, you might consider setting
.textContenton separate nodes instead of constructing HTML strings withinnerHTML(even withencodeHTML), which would further reduce the risk of future XSS regressions when these templates are modified.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the markdown link replacement, `safeHref` only replaces double quotes; consider using the same `encodeHTML` (or a dedicated attribute-escaping helper) for the `href` value as well to robustly handle `<`, `>`, and `&` in URLs.
- For the embed title rendering, you might consider setting `.textContent` on separate nodes instead of constructing HTML strings with `innerHTML` (even with `encodeHTML`), which would further reduce the risk of future XSS regressions when these templates are modified.
## Individual Comments
### Comment 1
<location path="assets/js/script.js" line_range="459-461" />
<code_context>
// parse text in brackets and then the URL in parentheses.
- .replace(/\[([^\[\]]+)\]\((.+?)\)/g, `<a title="$1" target="_blank" class="anchor" href="$2">$1</a>`)
+ .replace(/\[([^\[\]]+)\]\((.+?)\)/g, (_, text, href) => {
+ if (!/^https?:\/\//i.test(href)) return text;
+ const safeHref = href.replace(/"/g, '"');
+ return `<a title="${encodeHTML(text)}" target="_blank" class="anchor" href="${safeHref}">${encodeHTML(text)}</a>`;
+ })
</code_context>
<issue_to_address>
**🚨 issue (security):** Escape more than just quotes in the href attribute to avoid malformed HTML and edge-case XSS vectors.
Because `href` is interpolated directly into an attribute, escaping only `"` isn’t enough. Unescaped characters like `&`, `<`, and `>` can still break the attribute or be used for XSS. Instead of `href.replace(/"/g, '"')`, pass `href` through `encodeHTML` (or a dedicated attribute-escaping helper), e.g.:
```js
const safeHref = encodeHTML(href);
return `<a title="${encodeHTML(text)}" target="_blank" class="anchor" href="${safeHref}">${encodeHTML(text)}</a>`;
```
If you need to keep raw `&` in query strings, consider a URL/attribute-specific escaper, but avoid relying solely on quote replacement.
</issue_to_address>
### Comment 2
<location path="assets/js/script.js" line_range="458-463" />
<code_context>
// parse text in brackets and then the URL in parentheses.
- .replace(/\[([^\[\]]+)\]\((.+?)\)/g, `<a title="$1" target="_blank" class="anchor" href="$2">$1</a>`)
+ .replace(/\[([^\[\]]+)\]\((.+?)\)/g, (_, text, href) => {
+ if (!/^https?:\/\//i.test(href)) return text;
+ const safeHref = href.replace(/"/g, '"');
+ return `<a title="${encodeHTML(text)}" target="_blank" class="anchor" href="${safeHref}">${encodeHTML(text)}</a>`;
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider HTML-encoding `text` even when you skip link creation.
Right now the non-`http(s)` branch returns `text` raw, while the `http(s)` branch HTML-encodes it. If `text` can contain `<`, `&`, etc., this can lead to HTML injection or at least inconsistent rendering. Consider:
```js
if (!/^https?:\/\//i.test(href)) return encodeHTML(text);
```
so all paths encode `text` and don’t rely on upstream sanitization.
```suggestion
// parse text in brackets and then the URL in parentheses.
.replace(/\[([^\[\]]+)\]\((.+?)\)/g, (_, text, href) => {
if (!/^https?:\/\//i.test(href)) return encodeHTML(text);
const safeHref = href.replace(/"/g, '"');
return `<a title="${encodeHTML(text)}" target="_blank" class="anchor" href="${safeHref}">${encodeHTML(text)}</a>`;
})
```
</issue_to_address>
### Comment 3
<location path="assets/js/script.js" line_range="593" />
<code_context>
guiEmbedName.querySelector('.text').innerHTML = `Embed ${i + 1}${embed.title ? `: <span>${encodeHTML(embed.title)}</span>` : ''}`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 4
<location path="assets/js/script.js" line_range="593" />
<code_context>
guiEmbedName.querySelector('.text').innerHTML = `Embed ${i + 1}${embed.title ? `: <span>${encodeHTML(embed.title)}</span>` : ''}`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `guiEmbedName.querySelector('.text').innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 5
<location path="assets/js/script.js" line_range="807" />
<code_context>
guiEmbedName.querySelector('.text').innerHTML = `${guiEmbedName.innerText.split(':')[0]}${value ? `: <span>${encodeHTML(value)}</span>` : ''}`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 6
<location path="assets/js/script.js" line_range="807" />
<code_context>
guiEmbedName.querySelector('.text').innerHTML = `${guiEmbedName.innerText.split(':')[0]}${value ? `: <span>${encodeHTML(value)}</span>` : ''}`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `guiEmbedName.querySelector('.text').innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
[text](url)markdown syntax injected the URL directly intohrefwithout validation. Ajavascript:protocol URL (e.g.[click](javascript:alert(1))) would execute arbitrary JS. The fix validates that onlyhttp://andhttps://URLs produce links, escapes quotes in the href, and HTML-encodes the link text viaencodeHTML.embed.titlefrom user-provided JSON was inserted intoinnerHTMLwithout escaping in two places (GUI embed name display on load and on edit). The fix appliesencodeHTML()to the title/value before insertion.Test plan
[click me](javascript:alert(1))in an embed description and verify it renders as plain text, not a clickable link[legit](https://example.com)and verify it still renders as a working link[test](data:text/html,<script>alert(1)</script>)and verify it renders as plain text<img src=x onerror=alert(1)>via JSON input and verify the HTML is escaped in the sidebar embed name<script>alert(1)</script>and verify it appears as escaped textSummary by Sourcery
Harden markdown rendering and embed title handling against XSS injection vectors.
Bug Fixes: