Conversation
Collaborator
Author
|
Closing this one as a duplicate of #1518, which implements the same fix for issue #1516 and was opened before this PR became visible to this cycle's scan (open PRs were scanned at 16:27, #1518 was created at 16:36). The two are independent implementations. Everything this branch pins that #1518 does not — the arithmetic rows — is reported as measured technical feedback on #1518 (comment above), so nothing is lost by closing this. The branch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1516.
A command substitution written inside double quotes reaches the guard as one token — quote
semantics are kept there deliberately (issue #1162: a
>inside a quoted argument must notbecome an operator) — so neither payload reader had a command word to recurse into, and the
tier question was answered about nothing. The reporter's own measurement (
read-only): thequoted spellings answered ALLOW with an empty target list and executed, while
git checkout .on its own was refused.The change
_nested_command_textsgains an optionaltext— the masked line the caller tokenized._substitution_payloadsreads the bodies of$( … )and` … `off that line, which isthe only place the quote type is still visible, and hands each body back to the same
recursion the named wrapper already uses: the readers judge a payload by their own rules
instead of by a second copy of them. All three callers (
_extract_write_targets,_find_git_mutator,_cwd_left_workspace) pass it; omittingtextkeeps the token-onlyreading the walk had.
Three rules, each with its direction:
echo '$(git checkout .)'runsnothing and stays allowed; inside double quotes an apostrophe is an ordinary character, which
is why the scan tracks the quote it stands in (
echo "it's $(git checkout .)"is read);$(( … ))is arithmetic: its text is not handed over as a command, but a substitutionnested inside it is (
echo "$(($(git checkout .)))");named-wrapper walk already performs, and the fail-closed side of this guard.
A backslash is read the way
_protect_windows_backslasheslets the tokenizer read it, so thescanner and the tokenizer cannot disagree about which word a
$(sits in:echo "\$(git checkout .)"stays literal on POSIX and is read (refused) on Windows, where a backslash is apath character. The test reads that expectation off the module's own flag rather than writing
it down twice.
Measurement
Parity with the bare twin, at both tiers,
_check_sandboxonly, nothing executed. Theworkspace-writecolumn allows an in-workspace git mutator by design (the dirty-tree rule isa
read-onlyrule); what must not move is the row's relation to its own bare spelling:git checkout .(the control)echo "$(git checkout .)",echo "`git checkout .`",x="$(git checkout .)",echo "tail $(git checkout .)",echo "$(sh -c 'git checkout .')",echo "$(git checkout .)" > /dev/nullecho "$(rm -rf /tmp/x)",echo "$(touch /tmp/x)"echo "$(cd <outside>)" && echo x > out.txtcd <outside> && …echo '$(git checkout .)',echo "not a substitution",echo "$(date)",echo "a > b"Mutation arm (reproduced while writing this): with
_substitution_payloadsreturning[]the same rows answer ALLOW at
read-onlyagain — 13/13 — so the new refusals are the reader'sand no other rule's. That arm is pinned as an assertion in the test file rather than left as a
claim.
Verification
tests/test_quoted_substitution.py— 63 tests, both directions, driven through_check_sandboxat both tiers: the quoted spellings refused for the command they run, theirsingle-quoted twins allowed, the existing sandbox: read-only says no writes allowed, but only four command patterns block a write; plain rm and sed -i are allowed #1162 shapes (a quoted
>/|) unchanged, thereader pinned spell by spell, and the mutation arm above.
uv run pytest tests/ -q: 4492 passed, 214 skipped. The failures are the 7 knownenvironmental ones (1
test_check_merge_order.py::TestAgainstRealGitHistory::test_the_shapes_git_really_prints— this host's git rejects
merge-tree --write-tree --quieton the baseline too — and 6test_review_queue.py) plustest_the_index_derived_scans_reach_new_files, which asks for thenew file to be staged and passes once it is (
10 passed).uv run python -c "from emrg.client.app import run_client"anduv run python -m emrg --help:both clean.
Note on scope
This closes the quoted-substitution hole for the three readers that consume the walk. It does
not make the guard resolve a substitution that a variable carries (
x='$(git checkout .)'in one command,
echo "$x"in the next), which is the unresolved-word class of issue #1244 anda separate reading.