Address PR #436 R5 P1: token-boundary py-exec match + ln-symlink overwrite + regression tests

Reviewer flagged: both python-execution regexes used `\b` after `.py`,
which matches between `y` (word) and a following `.` (non-word). So
`python3 /tmp/notebook_md_extract.py.bak` matched as `/tmp/notebook_md_extract.py`,
and the captured path was treated as the allowlisted exact path. Same
class of bypass for `.py~`, `.pyc`, `.pyx`, `.py.evil`.

Fixes:
- Replace `\b` after `.py` and after `tmp_path` with `(?=[\s;|&]|$)`
  shell-token-boundary lookahead in:
  - test_workflow_python_file_execution_uses_only_allowlisted_paths
    (the allowlist check)
  - test_workflow_allowlisted_tmp_python_executions_have_base_sha_staging
    (staging_re, py_exec_re, overwrite_re — all three)
- Add `ln`-based overwrite patterns to overwrite_re (`ln -s`, `ln -sf`,
  `ln -f`, etc.). Preempts a future `ln -sf evil.py /tmp/<path>`
  symlink attack between staging and execution.
- Add 2 new regression tests:
  - test_python_exec_regex_rejects_suffixed_paths: asserts legit
    forms (with arg, with ;, with |, with &&, end-of-line) match
    correctly AND suffix bypasses (`.py.bak`, `.py~`, `.pyc`,
    `.pyx`, `.py.evil`) are rejected.
  - test_overwrite_regex_rejects_suffixed_paths: asserts real
    overwrites (>, >>, cp, mv, tee [-a], ln -s/-sf) match AND
    suffix-only writes do NOT match (they don't corrupt the
    allowlisted exact path).

All 10 guard tests pass. Test count grew from 8 to 10.

Author: igerber

tests/test_openai_review.py

@@ -2865,7 +2865,16 @@ def test_workflow_python_file_execution_uses_only_allowlisted_paths(
         run_contents = self._extract_all_run_content(workflow_text)
         assert run_contents, "No `run:` content extracted"
 
-        py_exec_re = re.compile(r"\bpython3?\s+(\S+\.py)\b")
+        # Use `(?=[\s;|&]|$)` token-boundary lookahead instead of `\b`
+        # after `.py`. The weaker `\b` matches between `y` and a
+        # following `.` (non-word), so `python3 /tmp/foo.py.bak`
+        # would match the `.py` prefix and the captured path would
+        # be `/tmp/foo.py` (in allowlist) — letting an attacker run
+        # `.py.bak`/`.py~`/`.pyx`/etc. under an allowlisted prefix.
+        # R5 fix for PR #436.
+        py_exec_re = re.compile(
+            r"\bpython3?\s+(\S+\.py)(?=[\s;|&]|$)",
+        )
 
         violations = []
         for label, content in run_contents:
@@ -2954,11 +2963,17 @@ def test_workflow_allowlisted_tmp_python_executions_have_base_sha_staging(
                 + re.escape(base_source)
                 + r'[ \t]+>[ \t]+'
                 + re.escape(tmp_path)
-                + r'\b',
+                + r'(?=[\s;|&]|$)',
             )
-            # Pattern B: python execution of this tmp_path.
+            # Pattern B: python execution of this tmp_path. Use
+            # `(?=[\s;|&]|$)` token-boundary lookahead instead of `\b`
+            # for the same reason as the sibling test above
+            # (`\bfoo.py\b` matches as a prefix of `foo.py.bak`).
+            # R5 fix for PR #436.
             py_exec_re = re.compile(
-                r'\bpython3?[ \t]+' + re.escape(tmp_path) + r'\b',
+                r'\bpython3?[ \t]+'
+                + re.escape(tmp_path)
+                + r'(?=[\s;|&]|$)',
             )
             # Pattern C: any non-staging write to tmp_path that would
             # overwrite the BASE-staged content. Matches:
@@ -2967,15 +2982,20 @@ def test_workflow_allowlisted_tmp_python_executions_have_base_sha_staging(
             #   cp <src> <path>
             #   mv <src> <path>
             #   tee [-a] <path>
+            #   ln [-s/-sf/-f] <src> <path>   (symlink, R5 preempt)
             # We exclude lines that match pattern A (the staging line
             # itself uses `>`, which would otherwise self-flag).
+            # Token-boundary lookahead `(?=[\s;|&]|$)` (not `\b`)
+            # prevents `> /tmp/foo.py.bak` from matching as `> /tmp/foo.py`.
+            tail = r'(?=[\s;|&]|$)'
             overwrite_re = re.compile(
                 r'(?:'
-                r'>>?[ \t]*' + re.escape(tmp_path) + r'\b'
-                r'|cp[ \t]+\S+[ \t]+' + re.escape(tmp_path) + r'\b'
-                r'|mv[ \t]+\S+[ \t]+' + re.escape(tmp_path) + r'\b'
-                r'|tee[ \t]+(?:-a[ \t]+)?' + re.escape(tmp_path) + r'\b'
-                r')',
+                r'>>?[ \t]*' + re.escape(tmp_path) + tail
+                + r'|cp[ \t]+\S+[ \t]+' + re.escape(tmp_path) + tail
+                + r'|mv[ \t]+\S+[ \t]+' + re.escape(tmp_path) + tail
+                + r'|tee[ \t]+(?:-[a-zA-Z]+[ \t]+)?' + re.escape(tmp_path) + tail
+                + r'|ln[ \t]+(?:-[a-zA-Z]+[ \t]+)?\S+[ \t]+' + re.escape(tmp_path) + tail
+                + r')',
             )
 
             staging_indices = []
@@ -3135,6 +3155,87 @@ def test_workflow_open_pr_checkout_uses_head_repo_and_head_sha(
             "Found checkout block:\n" + checkout_block
         )
 
+    def test_python_exec_regex_rejects_suffixed_paths(self):
+        """Regression: `python3 /tmp/notebook_md_extract.py.bak` must
+        NOT be treated as the allowlisted `/tmp/notebook_md_extract.py`.
+        Likewise for `~`, `.swp`, `.pyc` and other suffixes. R5 fix
+        for PR #436: prior `\\b` boundary matched between `y` and `.`
+        (non-word char), letting suffixed paths bypass the allowlist."""
+        import re
+
+        py_exec_re = re.compile(
+            r"\bpython3?\s+(\S+\.py)(?=[\s;|&]|$)",
+        )
+        # Should match (legit forms): captures the exact path.
+        for line, expected in [
+            ("python3 /tmp/notebook_md_extract.py", "/tmp/notebook_md_extract.py"),
+            ("python3 /tmp/notebook_md_extract.py --input foo", "/tmp/notebook_md_extract.py"),
+            ("python3 /tmp/notebook_md_extract.py;", "/tmp/notebook_md_extract.py"),
+            ("python3 /tmp/notebook_md_extract.py | tee log", "/tmp/notebook_md_extract.py"),
+            ("python /tmp/foo.py && echo done", "/tmp/foo.py"),
+        ]:
+            matches = py_exec_re.findall(line)
+            assert matches == [expected], (
+                f"Expected {expected!r} captured from {line!r}, got {matches!r}"
+            )
+        # Should NOT match (suffix bypasses): regex must reject these.
+        for line in [
+            "python3 /tmp/notebook_md_extract.py.bak",
+            "python3 /tmp/notebook_md_extract.py~",
+            "python3 /tmp/notebook_md_extract.pyc",
+            "python3 /tmp/notebook_md_extract.pyx",
+            "python3 /tmp/foo.py.evil",
+        ]:
+            matches = py_exec_re.findall(line)
+            assert matches == [], (
+                f"Suffix-bypass {line!r} must NOT match the allowlist regex; "
+                f"got {matches!r}. R5 dismissal-test fix would regress."
+            )
+
+    def test_overwrite_regex_rejects_suffixed_paths(self):
+        """Regression: an overwrite to `/tmp/foo.py.bak` must NOT be
+        treated as an overwrite to the allowlisted `/tmp/foo.py`. Used
+        to prevent the staging-vs-execution ordering check from being
+        confused by suffix-only writes (which do not actually corrupt
+        the BASE-staged file)."""
+        import re
+
+        tmp_path = "/tmp/notebook_md_extract.py"
+        tail = r"(?=[\s;|&]|$)"
+        overwrite_re = re.compile(
+            r"(?:"
+            r">>?[ \t]*" + re.escape(tmp_path) + tail
+            + r"|cp[ \t]+\S+[ \t]+" + re.escape(tmp_path) + tail
+            + r"|mv[ \t]+\S+[ \t]+" + re.escape(tmp_path) + tail
+            + r"|tee[ \t]+(?:-[a-zA-Z]+[ \t]+)?" + re.escape(tmp_path) + tail
+            + r"|ln[ \t]+(?:-[a-zA-Z]+[ \t]+)?\S+[ \t]+" + re.escape(tmp_path) + tail
+            + r")",
+        )
+        # Should match (real overwrites of the allowlisted path):
+        for line in [
+            "echo x > /tmp/notebook_md_extract.py",
+            "cat foo >> /tmp/notebook_md_extract.py",
+            "cp src.py /tmp/notebook_md_extract.py",
+            "mv src.py /tmp/notebook_md_extract.py",
+            "echo x | tee /tmp/notebook_md_extract.py",
+            "echo x | tee -a /tmp/notebook_md_extract.py",
+            "ln -sf evil.py /tmp/notebook_md_extract.py",
+            "ln -s evil.py /tmp/notebook_md_extract.py",
+        ]:
+            assert overwrite_re.search(line), (
+                f"Real overwrite {line!r} must match the overwrite regex"
+            )
+        # Should NOT match (suffix variants — different files):
+        for line in [
+            "echo x > /tmp/notebook_md_extract.py.bak",
+            "cp src.py /tmp/notebook_md_extract.py~",
+            "ln -sf evil.py /tmp/notebook_md_extract.pyc",
+        ]:
+            assert not overwrite_re.search(line), (
+                f"Suffix-only write {line!r} must NOT match the overwrite "
+                f"regex (it does not corrupt the allowlisted exact path)"
+            )
+
     def test_workflow_comment_triggers_require_author_association(
         self, workflow_text
     ):