fix(comments): escape repository-derived values when rendering comments

Manifest paths and sources are file paths inside the scanned repository, so
anyone who can open a pull request controls them: a directory named with link or
tag syntax, holding a manifest, put that markup into a comment posted by a trusted
integration. Alert text comes from the API. Neither is markup the CLI authored, so
both are escaped where they are interpolated -- text nodes with html.escape,
href and src with quotes escaped too, since an unescaped quote closes the
attribute and everything after it reads as more attributes.

The alert markers are the exception: they are read back verbatim when a comment is
rewritten, so they cannot be escaped without breaking the ignore round trip. They
instead lose only the ability to terminate the comment early.

plain and raw styles are untouched. Slack, Jira and the console do not render
HTML, and escaping there would show entities to a human.

Round-trip tests render a comment with each hostile path and feed it back through
the parser, because the renderer and the parser are two halves of one loop: an
escaping choice the parser cannot read would silently stop ignores working.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: lelia

CHANGELOG.md

@@ -89,6 +89,12 @@
 - Server URLs read from `GITHUB_SERVER_URL` and `CI_SERVER_URL` are validated as
   http(s) URLs before being composed into a diff scan's external link, matching
   the check already applied to the other repository URLs read from CI.
+- Repository-derived values are escaped before they are rendered into a pull
+  request or merge request comment. Manifest paths and sources are file paths from
+  the scanned repository, and alert text comes from the API; neither is markup the
+  CLI authored, so both are now escaped at the point they are interpolated. The
+  alert markers can no longer be terminated early by a package name. Slack, Jira
+  and console output are unchanged, since none of them render HTML.
 
 ## 2.8.1
 

socketsecurity/core/messages.py

@@ -837,6 +837,37 @@ def inline_html_text(value) -> str:
             return ""
         return " ".join(str(value).split())
 
+    @staticmethod
+    def html_text(value) -> str:
+        """Flatten a value onto one line and escape it for an HTML text node.
+
+        Manifest paths and sources come from the customer's repository, so any PR
+        author controls them: a directory named ``![x](https://host/p.png)`` or
+        carrying a raw tag would otherwise render as that markup inside a comment
+        posted by a trusted integration. Alert text comes from the API and is
+        escaped for the same reason, since neither is markup the CLI authored.
+        """
+        return escape(Messages.inline_html_text(value))
+
+    @staticmethod
+    def html_attr(value) -> str:
+        """Escape a value for an HTML attribute, quotes included.
+
+        Used for href and src, where an unescaped quote closes the attribute and
+        everything after it is read as more attributes.
+        """
+        return escape(Messages.inline_html_text(value), quote=True)
+
+    @staticmethod
+    def comment_marker_text(value) -> str:
+        """Neutralize an HTML comment terminator inside a marker value.
+
+        The alert markers carry the package name so the comment can be rewritten
+        later, and the parser reads them back verbatim -- so this cannot escape the
+        value, only stop it ending the comment early.
+        """
+        return str(value or "").replace("-->", "--&gt;").replace("<!--", "&lt;!--")
+
     @staticmethod
     def normalize_comment_html(comment: str) -> str:
         """
@@ -960,43 +991,47 @@ def security_comment_template(diff: Diff, config=None) -> str:
             patched_version = Messages.get_patched_version(alert)
             patched_version_html = (
                 "<p><strong>Patched version:</strong> "
-                f"<code>{escape(Messages.inline_html_text(patched_version))}</code></p>"
+                f"<code>{Messages.html_text(patched_version)}</code></p>"
                 if patched_version else ""
             )
             # Generate proper manifest URL
             manifest_url = Messages.get_manifest_file_url(diff, alert.manifests, config)
+            pkg_label = Messages.html_text(f"{alert.pkg_name}@{alert.pkg_version}")
+            # The marker is read back verbatim when the comment is rewritten, so it
+            # keeps the raw name and only loses the ability to close the comment.
+            pkg_marker = Messages.comment_marker_text(f"{alert.pkg_name}@{alert.pkg_version}")
             # Generate a table row for each alert
             ignore_html = (
                 f"<p><em>Mark as acceptable risk:</em> To ignore this alert only in this pull request, reply with:<br/>"
-                f"<code>@SocketSecurity ignore {alert.pkg_type}/{alert.pkg_name}@{alert.pkg_version}</code><br/>"
+                f"<code>@SocketSecurity ignore {Messages.html_text(alert.pkg_type)}/{pkg_label}</code><br/>"
                 f"Or ignore all future alerts with:<br/>"
                 f"<code>@SocketSecurity ignore-all</code></p>"
             ) if show_ignore else ""
             comment += f"""
-<!-- start-socket-alert-{alert.pkg_name}@{alert.pkg_version} -->
+<!-- start-socket-alert-{pkg_marker} -->
 <tr>
   <td><strong>{action}</strong></td>
   <td align="center">
-      <img src="{severity_icon}" alt="{alert.severity}" width="20" height="20">
+      <img src="{severity_icon}" alt="{Messages.html_attr(alert.severity)}" width="20" height="20">
   </td>
   <td>
     <details {details_open}>
-      <summary>{alert.pkg_name}@{alert.pkg_version} - {Messages.inline_html_text(alert.title)}</summary>
-      <p><strong>Note:</strong> {Messages.inline_html_text(alert.description)}</p>
+      <summary>{pkg_label} - {Messages.html_text(alert.title)}</summary>
+      <p><strong>Note:</strong> {Messages.html_text(alert.description)}</p>
       {patched_version_html}
-      <p><strong>Source:</strong> <a href="{manifest_url}">Manifest File</a></p>
+      <p><strong>Source:</strong> <a href="{Messages.html_attr(manifest_url)}">Manifest File</a></p>
       <p>ℹ️ Read more on:
-      <a href="{alert.purl}">This package</a> |
-      <a href="{alert.url}">This alert</a> |
+      <a href="{Messages.html_attr(alert.purl)}">This package</a> |
+      <a href="{Messages.html_attr(alert.url)}">This alert</a> |
       <a href="https://socket.dev/alerts/malware">What is known malware?</a></p>
       <blockquote>
-        <p><em>Suggestion:</em> {Messages.inline_html_text(alert.suggestion)}</p>
+        <p><em>Suggestion:</em> {Messages.html_text(alert.suggestion)}</p>
         {ignore_html}
       </blockquote>
     </details>
   </td>
 </tr>
-<!-- end-socket-alert-{alert.pkg_name}@{alert.pkg_version} -->
+<!-- end-socket-alert-{pkg_marker} -->
     """
 
         # Add license policy violation entries grouped by PURL
@@ -1007,38 +1042,47 @@ def security_comment_template(diff: Diff, config=None) -> str:
             # Use orange diamond for license policy violations
             license_icon = "🔶"
 
+            license_label = Messages.html_text(
+                f"{first_alert.pkg_name}@{first_alert.pkg_version}"
+            )
+            # The marker is read back verbatim when the comment is rewritten, so it
+            # keeps the raw name and only loses the ability to close the comment.
+            license_marker = Messages.comment_marker_text(
+                f"{first_alert.pkg_name}@{first_alert.pkg_version}"
+            )
+
             # Build license findings list
             license_findings = []
             for alert in alerts:
                 license_findings.append(alert.title)
 
             comment += f"""
-<!-- start-socket-alert-{first_alert.pkg_name}@{first_alert.pkg_version} -->
+<!-- start-socket-alert-{license_marker} -->
 <tr>
   <td><strong>{action}</strong></td>
   <td align="center">{license_icon}</td>
   <td>
     <details>
-      <summary>{first_alert.pkg_name}@{first_alert.pkg_version} has a License Policy Violation.</summary>
+      <summary>{license_label} has a License Policy Violation.</summary>
       <p><strong>License findings:</strong></p>
       <ul>
 """
             for finding in license_findings:
-                comment += f"        <li>{Messages.inline_html_text(finding)}</li>\n"
+                comment += f"        <li>{Messages.html_text(finding)}</li>\n"
 
 
             # Generate proper manifest URL for license violations
             license_manifest_url = Messages.get_manifest_file_url(diff, first_alert.manifests, config)
 
             license_ignore_html = (
                 f"<p><em>Mark the package as acceptable risk:</em> To ignore this alert only in this pull request, reply with the comment "
-                f"<code>@SocketSecurity ignore {first_alert.pkg_type}/{first_alert.pkg_name}@{first_alert.pkg_version}</code>. "
+                f"<code>@SocketSecurity ignore {Messages.html_text(first_alert.pkg_type)}/{license_label}</code>. "
                 f"You can also ignore all packages with <code>@SocketSecurity ignore-all</code>. "
                 f"To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.</p>"
             ) if show_ignore else ""
             comment += f"""      </ul>
-      <p><strong>From:</strong> <a href="{license_manifest_url}">Manifest File</a></p>
-      <p>ℹ️ Read more on: <a href="{first_alert.purl}">This package</a> | <a href="https://socket.dev/alerts/license">What is a license policy violation?</a></p>
+      <p><strong>From:</strong> <a href="{Messages.html_attr(license_manifest_url)}">Manifest File</a></p>
+      <p>ℹ️ Read more on: <a href="{Messages.html_attr(first_alert.purl)}">This package</a> | <a href="https://socket.dev/alerts/license">What is a license policy violation?</a></p>
       <blockquote>
         <p><em>Next steps:</em> Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at <strong>support@socket.dev</strong>.</p>
         <p><em>Suggestion:</em> Find a package that does not violate your license policy or adjust your policy to allow this package's license.</p>
@@ -1047,7 +1091,7 @@ def security_comment_template(diff: Diff, config=None) -> str:
     </details>
   </td>
 </tr>
-<!-- end-socket-alert-{first_alert.pkg_name}@{first_alert.pkg_version} -->
+<!-- end-socket-alert-{license_marker} -->
     """
 
         # Close table
@@ -1408,8 +1452,11 @@ def create_sources(alert: Issue, style="md") -> tuple[str, str]:
 
         for source, manifest in alert.introduced_by:
             if style == "md":
-                add_str = f"<li>{manifest}</li>"
-                source_str = f"<li>{source}</li>"
+                # These land in rendered Markdown, where an unescaped path is read
+                # as markup. plain and raw are consumed by Slack, Jira and the
+                # console, which do not render HTML, so they stay verbatim.
+                add_str = f"<li>{Messages.html_text(manifest)}</li>"
+                source_str = f"<li>{Messages.html_text(source)}</li>"
             elif style == "plain":
                 add_str = f"• {manifest}"
                 source_str = f"• {source}"

tests/unit/test_pr_comment_rendering.py

@@ -9,6 +9,8 @@
 
 from dataclasses import dataclass
 
+import pytest
+
 from socketsecurity.core.classes import Comment, Diff, Issue
 from socketsecurity.core.messages import Messages
 from socketsecurity.core.scm_comments import Comments
@@ -358,3 +360,146 @@ def test_strips_the_action_filter(self):
 
     def test_returns_empty_when_absent(self):
         assert Comments.extract_report_url("no link here") == ""
+
+
+# --- Escaping repo-derived values ---------------------------------------------
+#
+# Manifest paths and sources are file paths inside the customer's repository, so
+# anyone who can open a pull request controls them: a directory named
+# `![x](https://host/p.png)` holding a manifest puts that markup into a comment
+# posted by a trusted integration. GitHub and GitLab sanitize comment HTML, so the
+# exposure is external resource loading, phishing links and content spoofing
+# rather than script execution.
+
+
+@dataclass
+class _RepoConfig(_FakeConfig):
+    """A config that reaches the branch which embeds the path verbatim.
+
+    Without repo/branch, get_manifest_file_url returns "" or a percent-encoded
+    Socket link, and the path never lands in the comment -- so a test using the
+    bare config asserts nothing.
+    """
+    repo: str = "acme/widgets"
+    branch: str = "main"
+
+
+HOSTILE_PATHS = {
+    "image": "![x](https://evil.example/p.png)/package.json",
+    "link": "[click me](https://evil.example)/package.json",
+    "raw_tag": "<img src=x onerror=alert(1)>/package.json",
+    "backtick": "`code`/package.json",
+    "pipe": "a|b/package.json",
+    "quote": 'a" onmouseover="x/package.json',
+    "comment_close": "x-->y/package.json",
+}
+
+
+def _rendered_with_path(path: str) -> str:
+    return Messages.security_comment_template(
+        _make_diff([_make_alert(manifests=path)]), _RepoConfig()
+    )
+
+
+def test_the_hostile_path_actually_reaches_the_comment():
+    """Guards the fixture itself: if the path stops being rendered, the escaping
+    tests below would pass while asserting nothing."""
+    body = _rendered_with_path("sentinel-path/package.json")
+
+    assert "sentinel-path" in body
+
+
+@pytest.mark.parametrize("name,path", sorted(HOSTILE_PATHS.items()))
+def test_hostile_manifest_path_cannot_introduce_markup(name, path):
+    """In the rendered comment the path only ever lands inside an href, where
+    Markdown is inert. The property that matters there is that the value cannot
+    open a tag or close the attribute -- see create_sources for the context where
+    Markdown itself is live."""
+    body = _rendered_with_path(path)
+
+    rendered = [ln for ln in body.split("\n") if "Manifest File" in ln][0]
+    value = rendered.split('href="', 1)[1].split('"', 1)[0]
+
+    for char in ("<", ">", '"'):
+        assert char not in value, f"{char!r} survived into the href: {value!r}"
+    assert_html_block_intact(body)
+
+
+def test_quote_in_a_path_cannot_escape_the_href():
+    body = _rendered_with_path('a" onmouseover="x/package.json')
+
+    assert "&quot;" in body
+    assert 'href="https://github.com/acme/widgets/blob/main/a" ' not in body
+
+
+def test_hostile_package_name_cannot_close_the_alert_marker():
+    body = Messages.security_comment_template(
+        _make_diff([_make_alert(pkg_name="evil-->x")]), _FakeConfig()
+    )
+
+    # Exactly the terminator the CLI wrote, and no stray one inside the value.
+    for line in body.split("\n"):
+        if "socket-alert-" in line:
+            assert line.count("-->") == 1, line
+
+
+def test_alert_text_from_the_api_is_escaped():
+    body = Messages.security_comment_template(
+        _make_diff([_make_alert(description="<script>alert(1)</script>")]), _FakeConfig()
+    )
+
+    assert "<script>" not in body
+    assert "&lt;script&gt;" in body
+
+
+@pytest.mark.parametrize("name,path", sorted(HOSTILE_PATHS.items()))
+def test_hostile_path_still_round_trips_through_the_ignore_parser(name, path):
+    """Escaping must not break reading the comment back.
+
+    The renderer and the comment parser are two halves of one loop: a comment the
+    CLI writes is re-read on the next run to apply ignore commands. An escaping
+    choice the parser cannot read would silently stop ignores working.
+    """
+    security = _make_comment(_rendered_with_path(path))
+    comments = {
+        "security": security,
+        "ignore": [_make_comment("SocketSecurity ignore npm/lodash@4.17.21", comment_id=2)],
+    }
+
+    new_body = Comments.process_security_comment(security, comments)
+
+    assert "No dependency alerts to report" in new_body
+
+
+class TestCreateSourcesEscaping:
+    """create_sources' md style emits <li> into rendered Markdown, so unlike the
+    href context a path there IS interpreted as Markdown."""
+
+    def _md(self, path: str) -> str:
+        alert = _make_alert()
+        alert.introduced_by = [("direct", path)]
+        manifest_str, _ = Messages.create_sources(alert, "md")
+        return manifest_str
+
+    @pytest.mark.parametrize("name,path", sorted(HOSTILE_PATHS.items()))
+    def test_md_style_escapes_the_path(self, name, path):
+        """Escaping stops the value opening a tag. Markdown inside the value is
+        inert because GFM does not parse Markdown within a raw HTML block, which
+        is why escaping rather than code spans is the right tool for <li>."""
+        out = self._md(path)
+
+        assert "<img src=x onerror=alert(1)>" not in out
+        # Structure intact: exactly the one element create_sources wrote.
+        assert out.count("<li>") == 1 and out.count("</li>") == 1
+        inner = out[out.index("<li>") + 4 : out.index("</li>")]
+        for char in ("<", ">"):
+            assert char not in inner, f"{char!r} survived into the list item: {inner!r}"
+
+    def test_plain_style_is_left_verbatim(self):
+        """Slack, Jira and the console do not render HTML."""
+        alert = _make_alert()
+        alert.introduced_by = [("direct", "<img src=x>/package.json")]
+
+        manifest_str, _ = Messages.create_sources(alert, "plain")
+
+        assert "<img src=x>/package.json" in manifest_str