feat: add <img> to safe-outputs HTML tag allowlist#29401
Conversation
Add `img` to the `allowedTags` array in `convertXmlTags()` so that `<img>` tags are preserved in safe-output content instead of being converted to parentheses. Dangerous attributes (on* event handlers and style) are already stripped by `stripDangerousAttributes()`, and the `src` URL is validated by `sanitizeUrlDomains()`. Update tests to reflect the new behaviour: - `<img/>` is preserved rather than converted - `<img src=x>` in mixed content is preserved - XSS test updated: `onerror`/`onload` are stripped, but the `<img>` tag itself is kept - New positive tests for layout attributes (align, width, alt) and XSS attribute stripping Agent-Logs-Url: https://github.com/github/gh-aw/sessions/5076fa54-7dd9-4f3a-bed2-9364d21e3c62 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
<img> to safe-outputs HTML tag allowlist
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. |
There was a problem hiding this comment.
Pull request overview
Enables preservation of HTML <img> tags in “safe-output” sanitized Markdown so image alignment/sizing attributes (e.g. align, width) are not lost during XML-tag neutralization.
Changes:
- Added
imgto theconvertXmlTags()safe HTML allowlist so<img>is preserved rather than converted to parentheses form. - Updated and added sanitizer tests to expect
<img>to pass through while ensuring dangerouson*attributes are stripped.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/sanitize_content_core.cjs | Adds img to the safe HTML tag allowlist and documents expected safety controls. |
| actions/setup/js/sanitize_content.test.cjs | Updates tag-conversion and XSS tests for the new <img> allowlist behavior and adds new img-specific assertions. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
actions/setup/js/sanitize_content_core.cjs:625
- The new note says
srcis covered bysanitizeUrlDomains(), but URL sanitization here is actually split acrosssanitizeUrlProtocols()(blocksdata:,javascript:,http:etc) andsanitizeUrlDomains()(only allowlists absolutehttps://and protocol-relative//URLs). Consider updating this comment to reflect both steps and clarify that relative/non-URLsrcvalues aren’t domain-validated bysanitizeUrlDomains().
// b, blockquote, br, code, details, em, h1–h6, hr, i, img, li, ol, p, pre, strong, sub, summary, sup, table, tbody, td, th, thead, tr, ul
// Plus GFM inline tags: abbr, del, ins, kbd, mark, s, span
// Note: img on* event handlers and style are stripped by stripDangerousAttributes(); src is covered by sanitizeUrlDomains()
actions/setup/js/sanitize_content.test.cjs:542
- The new img XSS test covers
onerrorwhen it’s written in the normal whitespace-separated form. Given the allowlisting change, please also add a regression test for malformed-but-common browser parsing edge cases like<img/onerror=alert(1) src=x>(slash immediately after tag name), which currently won’t match the\s+on\w+stripping pattern and could leave the handler intact.
it("should strip dangerous event-handler attributes from img tags", () => {
const result = sanitizeContent('<img src=x onerror="alert(1)">');
expect(result).toContain("<img");
expect(result).not.toContain("onerror");
});
- Files reviewed: 2/2 changed files
- Comments generated: 2
| function convertXmlTags(s) { | ||
| // Allow safe HTML tags supported by GitHub Flavored Markdown: | ||
| // b, blockquote, br, code, details, em, h1–h6, hr, i, li, ol, p, pre, strong, sub, summary, sup, table, tbody, td, th, thead, tr, ul | ||
| // b, blockquote, br, code, details, em, h1–h6, hr, i, img, li, ol, p, pre, strong, sub, summary, sup, table, tbody, td, th, thead, tr, ul | ||
| // Plus GFM inline tags: abbr, del, ins, kbd, mark, s, span | ||
| // Note: img on* event handlers and style are stripped by stripDangerousAttributes(); src is covered by sanitizeUrlDomains() |
There was a problem hiding this comment.
Allowlisting img makes it possible to preserve malformed-but-browser-tolerated tags like <img/onerror=alert(1) src=x>. Because stripDangerousAttributes() only strips on*/style when preceded by whitespace, /onerror (immediately after the tag name) would bypass stripping and could leave an executable event handler in the output. Please harden the attribute stripping to handle a / directly after the tag name (or otherwise tokenize attributes), and add a regression test for this form.
This issue also appears on line 623 of the same file.
| it("should preserve self-closing img tags", () => { | ||
| const result = sanitizeContent("Hello <img/> world"); | ||
| expect(result).toBe("Hello (img/) world"); | ||
| expect(result).toBe("Hello <img/> world"); | ||
| }); |
There was a problem hiding this comment.
This change repurposes the prior test that validated conversion of disallowed self-closing tags (e.g. <img/> → (img/)). Now that img is allowed, we’re missing coverage that a non-allowlisted self-closing tag is still converted (e.g. <div/> → (div/)). Please add a replacement assertion for a disallowed self-closing tag so the behavior stays protected.
This issue also appears on line 538 of the same file.
🧪 Test Quality Sentinel ReportTest Quality Score: 75/100
Test Classification DetailsView All Test Results (4 tests)
Flagged Tests — Suggestions
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 75/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). Minor suggestions posted in the comment above (missing src attribute assertion, slight onerror duplication, test inflation noted but contextually justified for a security feature).
|
@copilot review all comments and https://github.com/github/gh-aw/actions/runs/25194524570/job/73872015354?pr=29401 |
… test failures - Change \s+ to [\s/]+ in stripDangerousAttributes() regex so that malformed tags like <img/onerror=alert(1) src=x> cannot bypass dangerous-attribute stripping (the / immediately after the tag name is now treated as a separator, same as whitespace) - Update compute_text.test.cjs and sanitize_output.test.cjs to expect <img> to be preserved now that img is in the allowlist (fixes 4 CI failures) - Add test: disallowed self-closing tag <div/> still converts to (div/) - Add test: slash-prefixed dangerous attribute is stripped from img Agent-Logs-Url: https://github.com/github/gh-aw/sessions/69fecc69-c347-40a7-b18d-82ba5ff2091f Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all review comments and the CI failures in commit 16c9015:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@lpcox fyi |
<img>tags in safe-output bodies (e.g.create-issue,update-issue) were being converted to parentheses form — breaking alignment/sizing patterns common in GitHub Markdown. Standardsyntax can't replicatealignorwidthattributes.Changes
sanitize_content_core.cjs— adds"img"to theallowedTagsarray inconvertXmlTags();on*event handlers andstyleare already stripped bystripDangerousAttributes(), andsrcis already validated bysanitizeUrlDomains()sanitize_content.test.cjs— updates tests to reflect<img>being preserved:<img/>preserved instead of converted to(img/)<img src=x>to pass throughonerror/onloadare stripped from the tag rather than asserting<imgis absentalign,width,alt) preserved;onerrorstrippedBefore / after:
XSS safety is unchanged —
<img src=x onerror="alert(1)">becomes<img src=x>.