diff --git a/emrg/tools/bash_tool.py b/emrg/tools/bash_tool.py index e81adc92..815fbf73 100644 --- a/emrg/tools/bash_tool.py +++ b/emrg/tools/bash_tool.py @@ -361,18 +361,73 @@ def _translate_windows_heredocs(cmd: str) -> tuple[str, str | None]: # # `-n` is the dry-run letter, and it is the one letter in this family that turns # the run into a read; every rsync short option that takes a *value* (`-e`, `-f`, -# `-T`, `-M`, `-B`) has a different letter, so no value can be mistaken for it, -# while `--dry-run`, `--list-only` and `-n` inside a cluster (`-an`, `-avzn`) all +# `-T`, `-M`, `-B`, and GNU's `-@`) has a different letter, so no value can be +# mistaken for it (the table below is where those spellings are enumerated), while +# `--dry-run`, `--list-only` and `-n` inside a cluster (`-an`, `-avzn`) all # mean the same thing. _RSYNC_READ_LETTERS = frozenset({"n"}) _RSYNC_READ_LONG = frozenset({"--dry-run", "--list-only"}) +# The options that take their value as the **next token**. Without this table a trailing +# spaced value displaces the destination: consumed as an operand, it becomes the last +# one, which is the position the rule above reads as `DEST`. +# +# Measured on master `edba48ca` with the real predicate, nothing executed, the +# destination outside every allowed root: `rsync -a src/ /out/dest/ --exclude pat` +# reported `['pat']` and **ALLOW at both tiers**, while the same command without the +# trailing option reported `['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/out/dest/']` and was refused — so the option's value, not +# the operand rule, was what displaced it. The run really does write the destination: +# `rsync -a src/ dst/ --exclude pat` in a scratch tree left the file at `dst/` (rc=0). +# The same displacement was measured for `-e ssh`, `-f …`, `-B …`, `-T …`, +# `--out-format …`, and for the clustered `-ve ssh`. Option-**first** spellings +# (`rsync -a --exclude pat src/ dst/`) were already correct, which is why the file's +# other tests never caught it: they pin that spelling and the attached `--exclude=pat`. +# +# The table is keyed by option *shape*, not by run, so it must cover both implementations +# the guard meets. On this host (`openrsync`, "rsync version 2.6.9 compatible", +# 2026-09-20) each entry was measured in a scratch tree against flag controls +# (`--delete`, `--stats`, `--progress`, `-v`, `-r` all came back *not* value-taking, so +# the discriminator was shown to discriminate before it was believed): an entry is listed +# when the following token was consumed — rc=0 with the extra source left uncopied, or a +# diagnostic naming that very token as a bad numeric/filter/directory argument. The long +# options openrsync rejects outright ("unknown option") are still listed when GNU rsync +# takes a value for them: an option the running tool rejects cannot have a path for a +# value either, and CI runs GNU rsync, where it takes one. +_RSYNC_OPTIONS_WITH_VALUE = frozenset({ + # Short spellings, measured here. `-@` is GNU's `--modify-window`, which openrsync + # spells in the long form only (listed below). + "-e", "-f", "-B", "-M", "-T", "-@", + # Measured value-taking on the installed openrsync. + "--exclude", "--include", "--filter", "--exclude-from", "--files-from", + "--chmod", "--bwlimit", "--timeout", "--max-size", "--log-file", + "--out-format", "--log-format", "--log-file-format", "--suffix", + "--backup-dir", "--temp-dir", "--partial-dir", "--link-dest", "--rsync-path", + "--port", "--protocol", "--sockopts", "--address", "--modify-window", + "--compress-level", "--checksum-seed", "--contimeout", "--max-delete", + "--write-batch", "--only-write-batch", "--password-file", + # GNU-only spellings: rejected here, value-taking there. + "--min-size", "--max-alloc", "--compare-dest", "--copy-dest", + "--checksum-choice", "--cc", "--compress-choice", "--zc", + "--compress-threads", "--zt", "--skip-compress", "--block-size", "--stderr", + "--info", "--debug", "--usermap", "--groupmap", "--chown", "--early-input", + "--outbuf", "--stop-at", "--stop-after", "--time-limit", "--confine-root", + "--config", "--dparam", "--remote-option", "--copy-as", "--iconv", +}) + +# The letters of the short entries above, **derived** rather than written twice. A +# trailing value can also arrive inside a cluster, where an exact-token test finds +# nothing: `rsync -a src/ dst/ -ve ssh` carries its value in the cluster's last letter, +# and a cluster's last letter is the one that takes the value. +_RSYNC_SHORT_VALUE_LETTERS = frozenset( + opt[1] for opt in _RSYNC_OPTIONS_WITH_VALUE if len(opt) == 2 and opt[0] == "-" +) + # Named residual of the rule above: `--write-batch=` / # `--only-write-batch=` make rsync write a *second* path — the option's own # value — beside the destination operand. It is left unnamed because a batch file -# is a debugging artefact of a transfer, not the transfer, and adding it means -# reading one more option's value in both spellings; the destination operand this -# rule exists for is named either way. +# is a debugging artefact of a transfer, not the transfer; the destination operand +# this rule exists for is named either way, and the table below consumes the value +# in both spellings so it is never mistaken for that operand. # `split` writes a **family** of derived paths, and its last operand is the only # place their common prefix is spelled: `split -b 3 in.txt pre` creates `preaa`, @@ -1108,7 +1163,10 @@ def _rsync_run_is_a_read(tokens: list[str], i: int) -> bool: def _positional_args( - tokens: list[str], i: int, options_with_value: frozenset | None = None + tokens: list[str], + i: int, + options_with_value: frozenset | None = None, + cluster_value_letters: frozenset[str] = frozenset(), ) -> list[str]: """The non-option *operands* of the command starting at ``tokens[i]``. @@ -1126,6 +1184,14 @@ def _positional_args( table. Omitting it keeps the historical flat table, which is what the earlier callers (`rm`, `mv`, `cp`, `find`, the in-place writers) still read. + ``cluster_value_letters`` is the table's **short letters**, and it is opt-in: a + value can also arrive inside a cluster, where the exact-token test above finds + nothing (``rsync -a src/ dst/ -ve ssh`` carries `ssh` as `-ve`'s value, since a + cluster's last letter is the one that takes one). It stays opt-in because adding + the rule to a caller that did not ask for it can *lose* a destination rather than + gain one: `cp -at src` is read by `_target_directory_values`, which does not + parse clusters, so consuming `` here would leave one operand and name nothing. + A ``--`` **ends option parsing**, and that sentence was here before the loop below obeyed it (issue #1433): the loop skipped the ``--`` and went on dropping every dash-led token, so a command whose operand is a file whose own @@ -1172,6 +1238,12 @@ def _positional_args( # spaced form consumes the next one. if tok in table: skip_next = True + elif ( + cluster_value_letters + and not tok.startswith("--") + and tok[-1] in cluster_value_letters + ): + skip_next = True continue out.append(tok) return out @@ -1925,7 +1997,9 @@ def is_separator(index: int) -> bool: # destination, and it is rewritten — unless the run is a read form # (`-n`/`--dry-run`/`--list-only`), which writes nothing at all. if not _rsync_run_is_a_read(tokens, i): - args = _positional_args(tokens, i, _NO_OPTION_WITH_VALUE) + args = _positional_args( + tokens, i, _RSYNC_OPTIONS_WITH_VALUE, _RSYNC_SHORT_VALUE_LETTERS + ) # A single operand is a *listing* of the source, not a copy. if len(args) >= 2: targets.append(args[-1]) diff --git a/tests/test_bash_tool_rsync_destination.py b/tests/test_bash_tool_rsync_destination.py index 18f50b19..b5a7a5e7 100644 --- a/tests/test_bash_tool_rsync_destination.py +++ b/tests/test_bash_tool_rsync_destination.py @@ -114,6 +114,86 @@ def test_the_protected_daemon_file_is_refused_at_both_tiers(): assert "protected" in (ww_reason or ""), ww_reason +# ── a trailing option's value is a value, not the destination ─────────────────────── +# +# Every writing form above carries its options **before** the operands, and the attached +# `--exclude=foo` row is attached. Neither spelling can see the defect below: when a +# value-taking option *follows* the operands, its value is the last non-option token, +# which is exactly the position the last-operand rule reads as `DEST` — so the option's +# value displaced the destination and the run was allowed. Measured on master `edba48ca` +# with the real predicate, nothing executed, destination outside every allowed root: +# `rsync -a src/ /outside/emrg/dst --exclude pat` answered `['pat']` and **ALLOW at both +# tiers**, while the same command without the trailing option answered +# `['/outside/emrg/dst']` and was refused. It is not a false alarm: in a scratch tree +# `rsync -a src/ dst/ --exclude pat` is rc=0 and the file really lands under `dst/`. +# The option-first spelling was already correct, which is why the rows above stayed green +# through it — the two spellings differ only in where the option sits. +TRAILING_VALUE_FORMS = ( + ("exclude", f"rsync -a {WORKSPACE}/src/ {{dest}} --exclude pat"), + ("include", f"rsync -a {WORKSPACE}/src/ {{dest}} --include pat"), + ("rsh", f"rsync -a {WORKSPACE}/src/ {{dest}} -e ssh"), + ("rsh inside a cluster", f"rsync -a {WORKSPACE}/src/ {{dest}} -ve ssh"), + ("temp-dir", f"rsync -a {WORKSPACE}/src/ {{dest}} -T /tmp/scratch"), + ("block-size", f"rsync -a {WORKSPACE}/src/ {{dest}} -B 4096"), + ("filter", f"rsync -a {WORKSPACE}/src/ {{dest}} -f rule"), + ("out-format", f"rsync -a {WORKSPACE}/src/ {{dest}} --out-format %n"), + ("log-file", f"rsync -a {WORKSPACE}/src/ {{dest}} --log-file /tmp/scratch/log"), + ("two trailing values", f"rsync -a {WORKSPACE}/src/ {{dest}} --exclude a --include b"), +) + + +@pytest.mark.parametrize( + "row,cmd", TRAILING_VALUE_FORMS, ids=[row for row, _ in TRAILING_VALUE_FORMS] +) +def test_a_trailing_value_does_not_displace_the_destination(row, cmd): + """The option's value must be consumed, so the operand is still the one named.""" + dest = f"{OUTSIDE}/dst" + assert _extract_write_targets(cmd.format(dest=dest)) == [dest], row + + +@pytest.mark.parametrize( + "row,cmd", TRAILING_VALUE_FORMS, ids=[row for row, _ in TRAILING_VALUE_FORMS] +) +def test_a_displaced_destination_is_refused_at_both_tiers(row, cmd): + """The same two tiers as every other writing form, for the same reason.""" + for tier in ("read-only", "workspace-write"): + allowed, reason, _ = _check_sandbox( + cmd.format(dest=f"{OUTSIDE}/dst"), tier, WORKSPACE + ) + assert allowed is False, f"{row}: {tier} allowed a write to {OUTSIDE}" + assert reason, f"{row}: {tier} refused without a reason" + + +def test_a_trailing_flag_does_not_eat_the_destination(): + """The table's own control: an entry the option does not have would trade the hole + for the opposite mistake. + + A value-taking entry that a flag does not deserve consumes the operand the rule + names — a false block with two operands left, a silent miss with one. Each spelling + here was measured on this host as a *flag*, in a scratch tree: the token after it + stayed a source and was really copied as a second one (`--delete`, `--stats`, + `--progress`, `-v`, `-r` all came back not-value-taking). The table is only + trustworthy with the positive rows above and this negative row together. + """ + for flags in ("--delete", "--stats", "--progress", "-v", "-avz", "--ignore-times"): + cmd = f"rsync -a {WORKSPACE}/src/ {OUTSIDE}/dst {flags}" + assert _extract_write_targets(cmd) == [f"{OUTSIDE}/dst"], flags + + +def test_the_cluster_rule_is_the_rsync_branch_s_alone(): + """`cp -at src` keeps naming its operand: the cluster letters are not shared. + + The clustered-value rule is opt-in for this reason. `cp` reads `-t` through + `_target_directory_values`, which does not parse clusters, so consuming a cluster's + value inside `_positional_args` for every caller would leave one operand and name + nothing — the `rsync` table's letters are passed by the `rsync` branch only. + """ + cmd = f"cp -at {OUTSIDE}/dst {WORKSPACE}/src.txt" + assert _extract_write_targets(cmd) == [f"{OUTSIDE}/dst"] + allowed, _reason, _ = _check_sandbox(cmd, "workspace-write", WORKSPACE) + assert allowed is False + + # ── read forms: the operand is not written, so naming it would be a false block ───── READING_FORMS = ( ("-an cluster", f"rsync -an {WORKSPACE}/src.txt {{dest}}"),