emrg: an operator is not a write target, so no redirect spelling can hide one

emrg/tools/bash_tool.py

@@ -632,13 +632,48 @@ def _positional_args(tokens: list[str], i: int) -> list[str]:
     return out
 
 
+def _is_redirect_operator(tok: str) -> bool:
+    """True when ``tok`` is a redirect operator rather than a path.
+
+    Named here so the walk can ask the question twice — once to find a redirect,
+    once to refuse to call the *next* operator its target (a quoted `'>'` is an
+    operator token by the time quoting is gone).
+
+    Judged by **shape** rather than by a list, because the list is what the first
+    version of this predicate was and it was two operators short of the ones a
+    shell accepts. Measured on master `b0bd6188` (`bash_tool.py`
+    `00b7e8884c95d9be`): `echo x >| /etc/f` and `echo x <> /etc/f` reported the
+    targets `[]` at **both** tiers — allowed by `read-only`, whose whole job is to
+    refuse a redirect to anything but `/dev/null`, as well as by
+    `workspace-write`. Both genuinely write: in a throwaway directory bash and sh
+    each created the file for `>|` (the clobber redirect) and bash created it for
+    `<>` (read-write), so an empty target list there is a hole rather than an
+    opinion. `>>|`, `>>&` and `&>>` created nothing, so they are covered only
+    incidentally.
+
+    The shape test is: the token contains a `>` and no character that could be
+    part of a path. That also covers the fd-prefixed spellings (`1>|`, `0<>`,
+    `2>&1`, `5>&-`) without enumerating them, and it deliberately keeps a bare
+    `<` *out* of the set: `<` reads, so naming its operand a write target would
+    turn `cat < /etc/passwd` into a refusal — an over-block in exchange for
+    nothing. Erring towards "operator" is the safe direction *because of* what
+    the walk does with the answer: an operator is never recorded as a target, so
+    recognising more of them only makes the walk look further for the real one.
+    """
+    return (
+        bool(tok)
+        and ">" in tok
+        and re.fullmatch(r"[<>|&0-9-]+", tok) is not None
+    )
+
+
 def _extract_write_targets(cmd: str, _depth: int = 0) -> list[str]:
     """Write targets of ``cmd``: the paths a command appears to write.
 
     Returns path tokens the command appears to write to:
       - ``rm <path>...`` and ``rmdir <path>`` → the removed paths
       - ``mv <src> <dst>`` / ``cp <src> <dst>`` → the destination
-      - ``> / >> / 2> / &>`` redirects → the redirect target
+      - ``> / >> / 2> / &> / >| / <>`` redirects → the redirect target
 
     **Parsed, not scanned** (issue #1162). The previous version regex-scanned
     raw text, which failed in both directions:
@@ -653,9 +688,19 @@ def _extract_write_targets(cmd: str, _depth: int = 0) -> list[str]:
         unlisted writers (``sed -i``, ``truncate``, ``tee``, ``cp`` without
         ``-r``) were not destructive at all — 7 of 7 measured writes allowed.
 
-    Quoting is what distinguishes the two: the token stream already knows
-    whether ``>`` was an operator or a character in an argument, so both
-    directions are fixed by the same change.
+    Quoting is *mostly* what distinguishes the two, and the walk now covers the
+    case where it does not. The token stream knows whether a `>` sat inside a
+    longer argument (`echo "a > b"` is one token), so both directions are fixed
+    by the same change. It does **not** know whether a `>` that stood alone as a
+    word was quoted: `'>'` and `>` dequote to the same token, so a quoted
+    operator used to consume the next token as its `target` and the *real*
+    redirect's target was never reported — measured on master, `echo '>' > /etc/x`
+    produced targets `['>']` and was allowed to write `/etc/x` at
+    workspace-write while `echo x > /etc/x` was blocked. Since an operator is
+    never a path, the walk skips operator tokens rather than believing the first
+    one it meets (issue #1268). The residual is the over-block side only: a
+    quoted operator followed by a bare word is still read as a redirect, which
+    needs the lexer to preserve quoting to fix.
 
     Still deliberately non-exhaustive in *which verbs* it covers (an
     interpreter can always write a file); the honest boundary stays
@@ -671,12 +716,25 @@ def _extract_write_targets(cmd: str, _depth: int = 0) -> list[str]:
     while i < len(tokens):
         tok = tokens[i]
         word = _command_word(tok)
-        # Redirects: `>` `>>` `&>` `&>>` are their own tokens, and a numeric
-        # fd prefix arrives as a separate token (`2` `>` `e`).
-        if tok in (">", ">>", "&>", "&>>") or re.fullmatch(r"\d*>>?", tok):
-            if i + 1 < len(tokens) and tokens[i + 1] not in _COMMAND_SEPARATORS:
-                targets.append(tokens[i + 1])
-                i += 2
+        # Redirects: `>` `>>` `&>` `&>>` `>|` `<>` are their own tokens, and a
+        # numeric fd prefix arrives as a separate token (`2` `>` `e`). Which
+        # spellings count is `_is_redirect_operator`'s job — this walk only asks.
+        if _is_redirect_operator(tok):
+            # An operator is never a write target. A *quoted* `>` reaches this
+            # walk as an operator token — the tokenizer dequotes, so `'>'` and
+            # `>` are the same string here — and taking the next token blindly
+            # made the quoted one consume the real redirect's target: measured
+            # on master, `echo '>' > /etc/x` yielded targets `['>']` and was
+            # ALLOWED at workspace-write (the file was really created outside)
+            # while `echo x > /etc/x` was blocked. Skipping operator tokens
+            # reports the target the shell will actually write, so the boundary
+            # no longer depends on whether a `>` was quoted.
+            j = i + 1
+            while j < len(tokens) and _is_redirect_operator(tokens[j]):
+                j += 1
+            if j < len(tokens) and tokens[j] not in _COMMAND_SEPARATORS:
+                targets.append(tokens[j])
+                i = j + 1
                 continue
         elif word == "rm" or word == "rmdir":
             # Any operand is removed — NOT only with a recursive flag.

tests/test_bash_tool_sandbox.py

@@ -17,6 +17,7 @@
     SANDBOX_MODES,
     _check_sandbox,
     _extract_write_targets,
+    _is_redirect_operator,
     _flag_part,
     _GIT_READ_VERBS,
     _GIT_SHAPE_DECIDED,
@@ -663,6 +664,125 @@ def test_real_redirects_are_still_write_targets():
         assert allowed is False, cmd
 
 
+def test_is_redirect_operator_covers_the_spellings_the_walk_recognises():
+    """The predicate must still accept everything it accepted before.
+
+    It is now judged by shape (a `>` and nothing path-like) rather than by a
+    list, because the list was missing `>|` and `<>` — see
+    `test_a_clobber_redirect_names_the_target_it_writes`. The negatives here are
+    the ones a looser rule would break: a bare `<` reads, and `|`, `;` and a
+    plain word are not redirects.
+    """
+    for tok in (">", ">>", "&>", "&>>", "2>", "22>>"):
+        assert _is_redirect_operator(tok) is True, tok
+    for tok in (">|", "1>|", "2>|", "<>", "0<>", ">&", "2>&1", "5>&-", "&>>"):
+        assert _is_redirect_operator(tok) is True, tok
+    for tok in ("out.txt", "/tmp/y", "2", "a>b", "<", "|", ";", "&&", "<<", ">>>foo"):
+        assert _is_redirect_operator(tok) is False, tok
+
+
+def test_a_quoted_operator_cannot_hide_the_real_redirect_target():
+    """Issue #1268: a quoted `>` used to swallow the next token as its target.
+
+    `'>'` and `>` dequote to the same token, so the walk could not tell a quoted
+    operator from a real one. It took the following token blindly, which meant
+    `echo '>' > /etc/x` produced the target `['>']` — and the *real* redirect
+    vanished. Measured on master `065ee9d5` end to end through
+    `BashTool.execute()` at workspace-write: the call returned success and
+    `/Users/argszero/emrg-phantom-proof.txt` really existed afterwards, while
+    `echo x > <the same path>` was refused. The boundary was escaped by one
+    quoted character.
+
+    An operator is never a path, so the walk now skips operator tokens and names
+    the target the shell will actually write.
+    """
+    outside = "/etc/emrg-1268-probe.txt"
+    for cmd in (
+        f"echo '>' > {outside}",
+        f"echo '>' >> {outside}",
+        f"echo '>>' > {outside}",
+    ):
+        targets = _extract_write_targets(cmd)
+        assert outside in targets, f"{cmd!r} must name the real target, got {targets!r}"
+        assert ">" not in targets, f"{cmd!r} must not treat an operator as a path"
+        for tier in ("read-only", "workspace-write"):
+            allowed, reason, _ = _check_sandbox(cmd, tier)
+            assert allowed is False, f"{cmd!r} must be blocked at {tier} (got {reason!r})"
+
+
+def test_the_operator_skip_does_not_swallow_a_real_target():
+    """The positive control for the skip: no operator means no skipping.
+
+    If the walk skipped the token after every operator unconditionally, a real
+    redirect would lose its target — a hole in the other direction, and the
+    reason the fix walks only over *operators*.
+    """
+    assert _extract_write_targets("echo x > /tmp/y") == ["/tmp/y"]
+    assert _extract_write_targets("echo x >> /tmp/y") == ["/tmp/y"]
+    assert _extract_write_targets("cmd 2> err.txt") == ["err.txt"]
+    assert _extract_write_targets("echo '>' > /tmp/y") == ["/tmp/y"]
+    # A quoted operator followed by a bare word is still read as a redirect —
+    # the known over-block residual, which needs the lexer to keep quoting.
+    assert _extract_write_targets("grep -n '>' file.txt") == ["file.txt"]
+
+
+OUTSIDE_CLOBBER = "/etc/emrg-clobber-probe.txt"
+
+# The spellings a shell really writes with and the walk did not call operators.
+# Measured on master `b0bd6188` in a throwaway directory: bash and sh each
+# created the file for `>|` and bash created it for `<>`; `>>|`, `>>&` and `&>>`
+# created nothing, so they are not claimed here.
+CLOBBER_WRITES = [
+    "echo x >| {o}",
+    "echo '>' >| {o}",
+    "echo x 1>| {o}",
+    "echo x <> {o}",
+    "echo '>' <> {o}",
+    "echo x 0<> {o}",
+]
+
+
+@pytest.mark.parametrize("cmd", [c.format(o=OUTSIDE_CLOBBER) for c in CLOBBER_WRITES])
+def test_a_clobber_redirect_names_the_target_it_writes(cmd: str):
+    """`>|` and `<>` are redirects, so their operand is the target — not nothing.
+
+    Before this shape test the walk reported `[]` for `echo x >| /etc/f` at both
+    tiers, and the file was really written: on master the guard's answer and the
+    shell's behaviour disagreed in the direction that loses work.
+    """
+    targets = _extract_write_targets(cmd)
+    assert OUTSIDE_CLOBBER in targets, f"{cmd!r} must name the target, got {targets!r}"
+    for tier in ("read-only", "workspace-write"):
+        allowed, reason, _ = _check_sandbox(cmd, tier)
+        assert allowed is False, f"{cmd!r} must be blocked at {tier} (got {reason!r})"
+
+
+@pytest.mark.parametrize("cmd", [c.format(o=OUTSIDE_CLOBBER) for c in CLOBBER_WRITES])
+def test_the_same_redirects_without_the_clobber_spelling_were_already_blocked(cmd: str):
+    """The control, derived rather than hand-listed: the `>` half alone blocked.
+
+    `echo x > /etc/f` was already refused before this change, so the flip above
+    is about the operator being recognised and not about a broader refusal.
+    """
+    plain = cmd.replace(">|", ">").replace("<>", ">").replace("0>", ">").replace("1>", ">")
+    targets = _extract_write_targets(plain)
+    assert OUTSIDE_CLOBBER in targets, f"{plain!r} must name the target, got {targets!r}"
+
+
+def test_a_read_redirect_is_not_a_write_target():
+    """`<` reads, so its operand must stay out of the target list.
+
+    This is the asymmetry the shape test keeps on purpose: calling `<` an
+    operator would make `cat < /etc/passwd` name `/etc/passwd` as a *write*
+    target, i.e. refuse a read — an over-block bought with a fix that does not
+    need it.
+    """
+    assert _is_redirect_operator("<") is False
+    assert _extract_write_targets("cat < /etc/passwd") == []
+    allowed, reason, _ = _check_sandbox("cat < /etc/passwd", "workspace-write")
+    assert allowed is True, reason
+
+
 def test_non_recursive_and_unlisted_writers_are_destructive():
     """Issue #1162: one verb, two spellings, opposite verdicts.