Ensure markdown README gets sanitised

.forgejo/workflows/ci.yml

@@ -15,7 +15,7 @@ jobs:
             image: docker.io/library/debian:13
             python: python3
           - distro: almalinux
-            image: docker.io/library/almalinux:9
+            image: docker.io/library/almalinux:10
             python: python3.11
 
     container:

README.md

@@ -266,7 +266,7 @@ enroll validate ./harvest --fail-on-warnings
 
 By default, `enroll` does **not** assume how you handle secrets in Ansible. It will attempt to avoid harvesting likely sensitive data (private keys, passwords, tokens, etc.). This can mean it skips some config files you may ultimately want to manage.
 
-Safe-mode content scanning is intentionally conservative. It treats common assignment-style credential keys as sensitive, including names such as `password`, `client_secret`, `secret_key`, `auth_token`, `api_key`, `aws_access_key_id`, `aws_secret_access_key`, `azure_client_secret`, `GOOGLE_APPLICATION_CREDENTIALS`, and service-account key names.
+Safe-mode content scanning is intentionally conservative. It treats common assignment-style credential keys as sensitive, including names such as `password` (and abbreviations like `passwd`, `pwd`, and `pw`, e.g. `db_pw`), `client_secret`, `secret_key`, `auth_token`, `api_key`, `aws_access_key_id`, `aws_secret_access_key`, `azure_client_secret`, `GOOGLE_APPLICATION_CREDENTIALS`, and service-account key names.
 
 **IMPORTANT**: Enroll ignores comments in files! If you have commented out *real secrets*, there's still a risk that Enroll could capture that data even without `--dangerous`. If you are in doubt, play it safe: use `--sops` and/or encrypt the output at rest in a way that makes sense to you.
 

enroll/ansible.py

@@ -8,7 +8,13 @@ from dataclasses import dataclass
 from pathlib import Path
 from typing import Any, Dict, List, Optional, Set, Tuple
 
-from .cm import CMModule, markdown_list, snapshot_excluded_lines, snapshot_note_lines
+from .cm import (
+    CMModule,
+    markdown_list,
+    sanitize_markdown_text,
+    snapshot_excluded_lines,
+    snapshot_note_lines,
+)
 from .jinjaturtle import (
     jinjify_managed_files as _jinjify_managed_files,
     resolve_jinjaturtle_mode,
@@ -824,7 +830,7 @@ def _render_readme(
     fqdn: Optional[str] = None,
 ) -> str:
     host = state.get("host", {}) if isinstance(state.get("host"), dict) else {}
-    hostname = host.get("hostname") or "unknown"
+    hostname = sanitize_markdown_text(host.get("hostname") or "unknown") or "unknown"
     roles = state.get("roles", {}) if isinstance(state.get("roles"), dict) else {}
     excluded = snapshot_excluded_lines(roles)
     notes = snapshot_note_lines(roles)

enroll/cm.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import re
 import shlex
 from dataclasses import dataclass, field
 from pathlib import Path
@@ -713,8 +714,53 @@ def role_order_key(role: str) -> tuple[int, str]:
     return (priority.get(role, 50), role)
 
 
+# Control characters (excluding ordinary tab) that must never reach generated
+# documentation. A raw newline/carriage return in a harvested value would let it
+# break out of a Markdown list item or code span and inject new document
+# structure (a fake heading, a misleading link/command block); other C0/C1
+# control bytes can smuggle terminal escape sequences when the README is printed.
+_MARKDOWN_CONTROL_RE = re.compile(r"[\x00-\x08\x0b-\x1f\x7f-\x9f]")
+
+
+def sanitize_markdown_text(value: Any) -> str:
+    """Neutralise harvested text before it is spliced into generated Markdown.
+
+    Generated docs (the Ansible ``README.md``) embed harvested, attacker-
+    influenceable values such as the host name and captured file paths. These
+    are not executed by Ansible, but a value containing a newline, carriage
+    return, backtick, or control byte could otherwise break out of its
+    surrounding list item / code span and inject misleading Markdown structure
+    (a forged heading, a deceptive ``[link](...)``/command block) or a terminal
+    escape sequence when the file is viewed. This collapses any whitespace run
+    (including newlines and tabs) to a single space, drops other control bytes,
+    and replaces backticks with a similar-looking single quote so a value can
+    never escape an inline code span. It is deliberately lossy: the README is a
+    human-readable summary, and faithful representation of hostile bytes there
+    is not a goal.
+    """
+
+    text = str(value)
+    # Collapse any run of whitespace (newlines, CR, tabs, spaces) to one space so
+    # a harvested value stays on a single Markdown line / inside one code span.
+    text = re.sub(r"\s+", " ", text)
+    # Drop remaining control characters that survived the whitespace collapse.
+    text = _MARKDOWN_CONTROL_RE.sub("", text)
+    # A backtick would close an inline code span and let following characters be
+    # interpreted as Markdown; swap it for a visually-similar acute accent.
+    text = text.replace("`", "\u00b4")
+    return text.strip()
+
+
 def markdown_list(items: Iterable[Any], *, empty: str = "None.") -> str:
-    values = [str(item) for item in items if str(item)]
+    """Render already-composed Markdown list lines.
+
+    Callers that embed harvested values (``snapshot_note_lines``,
+    ``snapshot_excluded_lines``, ``path_reason_lines``) sanitise those values
+    with :func:`sanitize_markdown_text` before composing each line, so this
+    helper only joins lines it is given. It still drops empty entries.
+    """
+
+    values = [str(item) for item in items if str(item).strip()]
     return "\n".join(f"- {item}" for item in values) or f"- {empty}"
 
 
@@ -723,10 +769,10 @@ def path_reason_lines(
 ) -> List[str]:
     lines: List[str] = []
     for item in items or []:
-        path = str(item.get(source_key) or "")
+        path = sanitize_markdown_text(item.get(source_key) or "")
         if not path:
             continue
-        reason = str(item.get("reason") or "")
+        reason = sanitize_markdown_text(item.get("reason") or "")
         lines.append(f"{path} ({reason})" if reason else path)
     return lines
 
@@ -744,17 +790,20 @@ def iter_role_snapshots(roles: Mapping[str, Any]) -> Iterator[Mapping[str, Any]]
 def snapshot_note_lines(roles: Mapping[str, Any]) -> List[str]:
     notes: List[str] = []
     for snap in iter_role_snapshots(roles):
-        source = str(
+        source = sanitize_markdown_text(
             snap.get("role_name") or snap.get("unit") or snap.get("package") or "role"
         )
-        notes.extend(f"`{source}`: {note}" for note in snap.get("notes", []) or [])
+        notes.extend(
+            f"`{source}`: {sanitize_markdown_text(note)}"
+            for note in snap.get("notes", []) or []
+        )
     return notes
 
 
 def snapshot_excluded_lines(roles: Mapping[str, Any]) -> List[str]:
     excluded: List[str] = []
     for snap in iter_role_snapshots(roles):
-        source = str(
+        source = sanitize_markdown_text(
             snap.get("role_name") or snap.get("unit") or snap.get("package") or "role"
         )
         for line in path_reason_lines(snap.get("excluded", []) or []):

enroll/ignore.py

@@ -108,7 +108,7 @@ HIGH_CONFIDENCE_SECRET_PATTERNS = [
         (
             (?:[A-Za-z0-9]+[_.-])*
             (
-                password|passwd|passphrase|pwd|
+                password|passwd|passphrase|pwd|pw|
                 token|auth[_.-]?token|access[_.-]?token|refresh[_.-]?token|
                 secret|client[_.-]?secret|secret[_.-]?key|
                 api[_.-]?key|access[_.-]?key|private[_.-]?key|

tests/test_markdown_sanitize.py

@@ -0,0 +1,87 @@
+"""Security regression: harvested values embedded in the generated Ansible
+``README.md`` must be neutralised so an attacker-influenceable host name, file
+path, or note cannot inject Markdown structure (a forged heading, deceptive
+link/command block) or a terminal escape sequence into the documentation.
+
+Harvested values are not executed by Ansible, but the README is the
+human-readable summary an operator reads before applying a manifest, so a value
+that breaks out of its surrounding list item / inline code span and forges
+document structure is misleading and must be prevented.
+"""
+
+from __future__ import annotations
+
+from enroll.cm import (
+    markdown_list,
+    sanitize_markdown_text,
+    snapshot_excluded_lines,
+    snapshot_note_lines,
+)
+
+
+def test_sanitize_collapses_newlines_and_strips_controls():
+    # A newline would let a value break out of its Markdown line; a backtick
+    # would close an inline code span; ESC/BEL are terminal escape vectors.
+    payload = "host`\n## Injected\n[x](http://evil)\n`y\x1b[31mRED\x07\ttab"
+    out = sanitize_markdown_text(payload)
+
+    assert "\n" not in out and "\r" not in out
+    assert "`" not in out  # backticks replaced so a code span cannot be closed
+    assert "\x1b" not in out and "\x07" not in out  # control bytes stripped
+    # The visible text is preserved on a single line (heading marker is now inert
+    # because it no longer starts a line).
+    assert "## Injected" in out
+    assert "http://evil" in out
+
+
+def test_sanitize_is_idempotent_and_handles_non_str():
+    once = sanitize_markdown_text("a\nb")
+    assert sanitize_markdown_text(once) == once
+    assert sanitize_markdown_text(1234) == "1234"
+    assert sanitize_markdown_text(None) == "None"
+
+
+def test_markdown_list_does_not_gain_extra_lines_from_payload():
+    # Even if a pre-composed line still contained a newline, markdown_list must
+    # not emit more bullets than it was given.
+    out = markdown_list(["clean line", "two\nlines should be one bullet"])
+    # Two input items -> at most two leading "- " bullets.
+    assert out.count("\n- ") <= 1
+
+
+def test_snapshot_note_lines_neutralises_injected_note():
+    roles = {
+        "etc_custom": {
+            "role_name": "etc_custom",
+            "notes": ["benign`\n## PWNED\n- fake bullet\x1b[5m"],
+        }
+    }
+    lines = snapshot_note_lines(roles)
+    assert len(lines) == 1
+    line = lines[0]
+    # The role name code span is intact; the payload is folded onto one line
+    # with backticks and control bytes removed.
+    assert line.startswith("`etc_custom`: ")
+    assert "\n" not in line
+    assert "\x1b" not in line
+    # Only the (structural) backticks around the role name remain -- the
+    # payload's backtick was neutralised.
+    assert line.count("`") == 2
+
+
+def test_snapshot_excluded_lines_neutralises_injected_path():
+    roles = {
+        "etc_custom": {
+            "role_name": "etc_custom",
+            "excluded": [
+                {"path": "/etc/`\n## fake-heading\nx", "reason": "denied_path"}
+            ],
+        }
+    }
+    lines = snapshot_excluded_lines(roles)
+    assert len(lines) == 1
+    line = lines[0]
+    assert "\n" not in line
+    assert line.startswith("`etc_custom`: ")
+    assert line.count("`") == 2  # only the structural role-name span
+    assert "(denied_path)" in line

tests/test_secret_detection.py

@@ -93,6 +93,49 @@ def test_password_keyword_no_false_positives():
         assert pol._content_deny_reason("/etc/app.conf", data) is None, data
 
 
+def test_pw_abbreviation_credential_key_denied():
+    """The ``pw`` abbreviation is an extremely common password key form
+    (``db_pw``, ``DB_PW``, ``root_pw``). It must be treated like ``pwd``/``pass``
+    so a real assignment-style credential using ``pw`` is not silently harvested
+    in default safe mode."""
+    pol = IgnorePolicy(dangerous=False)
+    for data in [
+        b"pw = hunter2\n",
+        b"db_pw = SuperSecret123\n",
+        b"DB_PW=hunter2\n",
+        b"root_pw: aaaa\n",
+        b"master_pw = topsecret\n",
+        b"ftp_pw=x\n",
+        b"mysql_root_pw: x\n",
+        b"pw_hash = abc\n",
+        b'"pw": "x"\n',
+    ]:
+        assert (
+            pol._content_deny_reason("/etc/app.conf", data) == "sensitive_content"
+        ), data
+
+
+def test_pw_abbreviation_no_false_positives():
+    """Adding the short ``pw`` token must not flag ordinary keys that merely
+    contain the letters ``pw`` without it being a delimited credential token.
+    The surrounding token-boundary structure (a keyword must stand alone between
+    ``[_.-]`` delimiters) is what keeps these benign."""
+    pol = IgnorePolicy(dangerous=False)
+    for data in [
+        b"pwned = 1\n",
+        b"software = x\n",
+        b"hardware = y\n",
+        b"firmware: z\n",
+        b"spwd = z\n",
+        b"powerline = on\n",
+        b"wallpaper = img\n",
+        b"pwm_freq = 1000\n",
+        b"cpwd_check=1\n",
+        b"newpwx = 1\n",
+    ]:
+        assert pol._content_deny_reason("/etc/app.conf", data) is None, data
+
+
 def test_block_comment_open_cannot_mask_private_key():
     """Security regression: a line beginning with ``/*`` must not put the content
     scanner into a runaway block-comment state that hides later real secrets.