From 69104b30ba8b2bdc312bce8662ce1a20dd6fb461 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 21:47:05 +0000 Subject: [PATCH 1/4] Initial plan From c68d54539086059fe7636b2cdbb31f6c55eb3631 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 21:56:12 +0000 Subject: [PATCH 2/4] fix: strip on* event handlers and style attrs from allowed HTML tags in convertXmlTags() Agent-Logs-Url: https://github.com/github/gh-aw/sessions/beb0e828-1969-46c4-b4c5-bd52b7e12530 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/sanitize_content.test.cjs | 65 ++++++++++++++++++++++ actions/setup/js/sanitize_content_core.cjs | 20 ++++++- 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/actions/setup/js/sanitize_content.test.cjs b/actions/setup/js/sanitize_content.test.cjs index 2e2116c0fec..2f2fec5fc3d 100644 --- a/actions/setup/js/sanitize_content.test.cjs +++ b/actions/setup/js/sanitize_content.test.cjs @@ -484,6 +484,71 @@ describe("sanitize_content.cjs", () => { }); }); + describe("XML/HTML tag conversion: dangerous attribute stripping", () => { + it("should strip ontoggle event handler from details tag", () => { + const input = '
content
'; + const result = sanitizeContent(input); + expect(result).toBe("
content
"); + }); + + it("should strip style attribute enabling CSS overlay attacks from span tag", () => { + const input = 'overlay'; + const result = sanitizeContent(input); + expect(result).toBe("overlay"); + }); + + it("should strip onclick event handler from allowed tags", () => { + const result = sanitizeContent('

text

'); + expect(result).toBe("

text

"); + }); + + it("should strip onerror event handler from allowed tags", () => { + const result = sanitizeContent('text'); + expect(result).toBe("text"); + }); + + it("should strip style attribute with single-quoted value", () => { + const result = sanitizeContent("text"); + expect(result).toBe("text"); + }); + + it("should strip style attribute with unquoted value", () => { + const result = sanitizeContent("text"); + expect(result).toBe("text"); + }); + + it("should strip on* attributes case-insensitively (uppercase)", () => { + const result = sanitizeContent('text'); + expect(result).toBe("text"); + }); + + it("should strip multiple dangerous attributes from a single tag", () => { + const result = sanitizeContent('text'); + expect(result).toBe('text'); + }); + + it("should preserve safe attributes (title, class, open) while stripping dangerous ones", () => { + const result = sanitizeContent('
content
'); + expect(result).toBe("
content
"); + }); + + it("should preserve span title attribute after stripping style", () => { + const result = sanitizeContent('text'); + expect(result).toBe('text'); + }); + + it("should strip on* event handler from closing tags (no-op but consistent)", () => { + // Closing tags never carry attributes in valid HTML, but verify robustness + const result = sanitizeContent("text"); + expect(result).toBe("text"); + }); + + it("should not affect disallowed tags (still converted to parentheses with attributes)", () => { + const result = sanitizeContent('
content
'); + expect(result).toBe('(div onclick="bad()")content(/div)'); + }); + }); + describe("XML/HTML tag conversion: code-region awareness", () => { it("should preserve angle brackets inside fenced code blocks (backticks)", () => { const input = "Before\n```\nVBuffer x;\n```\nAfter"; diff --git a/actions/setup/js/sanitize_content_core.cjs b/actions/setup/js/sanitize_content_core.cjs index 4e69b11f27d..a06dcdb175f 100644 --- a/actions/setup/js/sanitize_content_core.cjs +++ b/actions/setup/js/sanitize_content_core.cjs @@ -568,17 +568,33 @@ function convertXmlTags(s) { return `(![CDATA[${convertedContent}]])`; }); + /** + * Strips dangerous HTML attributes from an allowed tag's content string. + * Removes on* event handler attributes (e.g. onclick, ontoggle) and style + * attributes in all quoting forms (double-quoted, single-quoted, unquoted, bare). + * Safe attributes such as title, class, open, lang, id, etc. are preserved. + * @param {string} tagContent - Tag content without surrounding angle brackets + * @returns {string} Tag content with dangerous attributes removed + */ + function stripDangerousAttributes(tagContent) { + // Match: optional whitespace + (on* | style) + optional =value + // Value forms: "...", '...', or unquoted (no whitespace / >), or bare (no =) + return tagContent.replace(/\s+(?:on\w+|style)(?:\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>]*))?/gi, ""); + } + // Convert opening tags: or to (tag) or (tag attr="value") // Convert closing tags: to (/tag) // Convert self-closing tags: or to (tag/) or (tag /) - // But preserve allowed safe tags + // But preserve allowed safe tags (with dangerous attributes stripped) return s.replace(/<(\/?[A-Za-z!][^>]*?)>/g, (match, tagContent) => { // Extract tag name from the content (handle closing tags and attributes) const tagNameMatch = tagContent.match(/^\/?\s*([A-Za-z][A-Za-z0-9]*)/); if (tagNameMatch) { const tagName = tagNameMatch[1].toLowerCase(); if (allowedTags.includes(tagName)) { - return match; // Preserve allowed tags + // Strip dangerous attributes (on* event handlers and style) before preserving + const sanitizedContent = stripDangerousAttributes(tagContent); + return `<${sanitizedContent}>`; } } return `(${tagContent})`; // Convert other tags to parentheses From e10b8b14ff24aff776bf34d513860cd810f482a0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 21:58:50 +0000 Subject: [PATCH 3/4] fix: address code review - improve regex comment, exclude quotes from unquoted value pattern, add simple unquoted test Agent-Logs-Url: https://github.com/github/gh-aw/sessions/beb0e828-1969-46c4-b4c5-bd52b7e12530 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/sanitize_content.test.cjs | 9 +++++++-- actions/setup/js/sanitize_content_core.cjs | 12 ++++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/actions/setup/js/sanitize_content.test.cjs b/actions/setup/js/sanitize_content.test.cjs index 2f2fec5fc3d..bd14873dbe1 100644 --- a/actions/setup/js/sanitize_content.test.cjs +++ b/actions/setup/js/sanitize_content.test.cjs @@ -517,6 +517,11 @@ describe("sanitize_content.cjs", () => { expect(result).toBe("text"); }); + it("should strip style attribute with unquoted value (simple, no special chars)", () => { + const result = sanitizeContent("text"); + expect(result).toBe("text"); + }); + it("should strip on* attributes case-insensitively (uppercase)", () => { const result = sanitizeContent('text'); expect(result).toBe("text"); @@ -537,8 +542,8 @@ describe("sanitize_content.cjs", () => { expect(result).toBe('text'); }); - it("should strip on* event handler from closing tags (no-op but consistent)", () => { - // Closing tags never carry attributes in valid HTML, but verify robustness + it("should preserve closing tags of allowed elements unchanged", () => { + // Closing tags do not carry attributes in HTML; verify they pass through unmodified const result = sanitizeContent("text"); expect(result).toBe("text"); }); diff --git a/actions/setup/js/sanitize_content_core.cjs b/actions/setup/js/sanitize_content_core.cjs index a06dcdb175f..d3bd4c15917 100644 --- a/actions/setup/js/sanitize_content_core.cjs +++ b/actions/setup/js/sanitize_content_core.cjs @@ -573,13 +573,21 @@ function convertXmlTags(s) { * Removes on* event handler attributes (e.g. onclick, ontoggle) and style * attributes in all quoting forms (double-quoted, single-quoted, unquoted, bare). * Safe attributes such as title, class, open, lang, id, etc. are preserved. + * + * Note: `\s+` (requiring at least one whitespace before the attribute name) is + * intentional — HTML attributes are always separated from the tag name and from + * each other by at least one whitespace character. Using `\s*` would risk false + * matches inside tag names (e.g. matching "ong" inside "strong"). + * * @param {string} tagContent - Tag content without surrounding angle brackets * @returns {string} Tag content with dangerous attributes removed */ function stripDangerousAttributes(tagContent) { - // Match: optional whitespace + (on* | style) + optional =value + // Match: one-or-more whitespace + (on* | style) + optional =value // Value forms: "...", '...', or unquoted (no whitespace / >), or bare (no =) - return tagContent.replace(/\s+(?:on\w+|style)(?:\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>]*))?/gi, ""); + // The [^\s>"'] unquoted form excludes >, whitespace, and quote chars so it + // cannot consume the closing > of the tag. + return tagContent.replace(/\s+(?:on\w+|style)(?:\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>"']*))?/gi, ""); } // Convert opening tags: or to (tag) or (tag attr="value") From 3b7e3daffc2a7966034b2b3f00c92ac9d3025b93 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 22:01:10 +0000 Subject: [PATCH 4/4] fix: also exclude backtick from unquoted attr value pattern, add edge-case tests Agent-Logs-Url: https://github.com/github/gh-aw/sessions/beb0e828-1969-46c4-b4c5-bd52b7e12530 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/sanitize_content.test.cjs | 12 ++++++++++++ actions/setup/js/sanitize_content_core.cjs | 8 ++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/actions/setup/js/sanitize_content.test.cjs b/actions/setup/js/sanitize_content.test.cjs index bd14873dbe1..f9ecdcd97f1 100644 --- a/actions/setup/js/sanitize_content.test.cjs +++ b/actions/setup/js/sanitize_content.test.cjs @@ -548,6 +548,18 @@ describe("sanitize_content.cjs", () => { expect(result).toBe("text"); }); + it("should strip on* attribute with extra whitespace around equals sign", () => { + const result = sanitizeContent('text'); + expect(result).toBe("text"); + }); + + it("should treat concatenated tag+attribute name as a single unknown tag (not strip attributes)", () => { + // is not a valid tag — the tag name is "spanonclick" + // which is not in the allowlist, so it gets converted to parentheses entirely + const result = sanitizeContent('text'); + expect(result).toBe('(spanonclick="bad()")text(/spanonclick)'); + }); + it("should not affect disallowed tags (still converted to parentheses with attributes)", () => { const result = sanitizeContent('
content
'); expect(result).toBe('(div onclick="bad()")content(/div)'); diff --git a/actions/setup/js/sanitize_content_core.cjs b/actions/setup/js/sanitize_content_core.cjs index d3bd4c15917..20814313302 100644 --- a/actions/setup/js/sanitize_content_core.cjs +++ b/actions/setup/js/sanitize_content_core.cjs @@ -584,10 +584,10 @@ function convertXmlTags(s) { */ function stripDangerousAttributes(tagContent) { // Match: one-or-more whitespace + (on* | style) + optional =value - // Value forms: "...", '...', or unquoted (no whitespace / >), or bare (no =) - // The [^\s>"'] unquoted form excludes >, whitespace, and quote chars so it - // cannot consume the closing > of the tag. - return tagContent.replace(/\s+(?:on\w+|style)(?:\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>"']*))?/gi, ""); + // Value forms: "...", '...', or unquoted (no whitespace / > / quote chars), or bare (no =) + // The unquoted form excludes >, whitespace, and all quote characters (', ", `) so it + // cannot consume the closing > of the tag or straddle other attribute values. + return tagContent.replace(/\s+(?:on\w+|style)(?:\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>"'`]*))?/gi, ""); } // Convert opening tags: or to (tag) or (tag attr="value")