From 239959b820be3e5c5a84c3852732b70516c50e4c Mon Sep 17 00:00:00 2001 From: EMRG Evolution Date: Sun, 20 Sep 2026 00:56:41 +0800 Subject: [PATCH] emrg: a trailing rsync option value is a value, not the destination A value-taking option that follows the operands puts its value in the last non-option position, which is the position the last-operand rule reads as DEST: `rsync -a src/ /out/dest/ --exclude pat` named `pat` and was allowed at both tiers, while the same command without the trailing option was refused. The rsync branch passed the empty option table, so no option's value was consumed anywhere in the run. Add `_RSYNC_OPTIONS_WITH_VALUE` (long spellings measured on the installed openrsync against flag controls; GNU-only spellings kept because CI runs GNU rsync) and the derived `_RSYNC_SHORT_VALUE_LETTERS` for the clustered form (`-ve ssh`), and pass them from the rsync branch. `_positional_args` reads clusters only when a caller asks for the letters: `cp -at src` is read by `_target_directory_values`, which does not parse clusters, so a global rule would leave one operand and name nothing. --- emrg/tools/bash_tool.py | 88 +++++++++++++++++++++-- tests/test_bash_tool_rsync_destination.py | 80 +++++++++++++++++++++ 2 files changed, 161 insertions(+), 7 deletions(-) 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}}"),