fix(permission): classify an abbreviated long option like the flag it reaches - #781
Conversation
|
ANIRUDDHA ADAK (@aniruddhaadak80) is attempting to deploy a commit to the InkVell Team on Vercel. A member of the Team first needs to authorize it. |
Ishaan Gangwani (ishaan1124)
left a comment
There was a problem hiding this comment.
Thanks. sed --in-p and sort --outp do reach --in-place and --output, so the bug is real. Three problems with the change in backend/cli/src/permission/shell-risk.ts (lines 220–236):
- Regression: the new
haslowercases the argument but not the expected value, and thetsccheck passes the camelCase"--noEmit". Sotsc --noEmit, our most common typecheck, now classifies as risky and prompts in Approve mode. No existing test covers it. - Incomplete: the
=form still gets through:sed --in-p=.bak,sort --outp=xandgit diff --out=xare all still classified as contained. - Side effects: the same helper feeds checks where a match makes a command allowed (
--check,--noEmit,--list), so abbreviations widen those too;unzip --lbecomes contained. It also adds false positives:git diff --textmatches--textconv,tar --checkpointmatches--checkpoint-action,eslint --fmatches--fix.
Suggested shape: no lowercasing; compare only the option name before =:
const name = arg.split("=", 1)[0]
const reaches = name.length > 2 && name.startsWith("--") && value.startsWith(name)Use that prefix match only in the checks where a match makes a command risky, and keep exact matching for the allow checks. Please add tests pinning tsc --noEmit as contained and sed --in-p=.bak as risky. The tar case should use -tf: with -cf it's already risky on main because of -cf, so it doesn't exercise --to-c.
… reaches GNU tools accept any unambiguous prefix of a long option, so `sed --in-p` edits in place, `sort --outp` writes its output file and `tar --to-c 'cmd'` runs cmd per extracted file. The shared flag matcher only recognised exact spellings, so those classified as contained and skipped the confirmation a destructive command always gets. Match a long-option prefix too. Short options are not abbreviations, so find's blocklist is unchanged.
…level GNU tools accept any unambiguous prefix of a long option, so `sed --in-p` edits in place and `sort --outp` writes its output file while spelling the flag differently. Comparing only the option name before `=` closes the `--in-p=.bak` form as well, and the match is no longer case-folded, which had made `tsc --noEmit` read as risky and prompt in Approve mode. The prefix rule is now confined to the checks where a hit makes a command risky and stays out of the ones where a hit makes it allowed, so `unzip --l` no longer widens into a listing and `cargo fmt --check`, `prettier --check` and `tar -t` are unaffected. Short options and exact long options keep the plain match, so `sort -o` and `date -s` are unchanged.
27431a3 to
be05b53
Compare
|
Done in be05b53.
|
|
All checks are green on |
The requested changes are in and verified; CI is green.
# Conflicts: # CHANGELOG.md
1107568
into
synthetic-sciences:main
What does this PR do, and why?
ShellRiskclassifies a bash command so the permission layer can force aconfirmation on the destructive ones. Commands that are not on the
MUTATINGblocklist —
find,sed,sort,tar,date,file— are checked flag byflag through one shared matcher:
Exact match, or
value=. That misses how GNU tools actually parse options:getopt_longaccepts any unambiguous prefix of a long option. So thedestructive flags are reachable with a different spelling, and the spelling is
what decides whether the user is asked.
I verified each one against real GNU tools (sed 4.9, coreutils sort 8.32,
tar 1.35) rather than reasoning about the parser:
sed --in-p 's/alpha/ALPHA/' a.txta.txtbecameALPHA— edited in placesort --outp sorted.txt in.txtsorted.txtwith the sorted outputtar --to-c 'echo PWNED-BY-TAR' -xf out.tarPWNED-BY-TAR— arbitrary command executionEach of those classifies as
containedonmain, andcontainedis exactly thelevel that skips the floor:
So in a project set to Approve for me with
bashallowed,sed --in-pandtar --to-crun with no confirmation while the commands they are spelled asalways prompt. That is an oversight rather than a design choice —
MUTATINGalready refuses
ed,perl,python,patch,viandnanooutrightprecisely because in-place editors are destructive.
The fix teaches
hasabout long-option prefixes:One change covers
sed,sort,tar,dateandfileon both the allow andthe deny side. Over-refusing an abbreviation the tool would have rejected is the
safe direction for an approval floor.
What I did not change, and why
The report I started from also claimed
find . -type f -deland-flreach-deleteand-fls. I tested that and it is false — GNU findutils 4.10answers
find: unknown predicate '-del', and the file survives. Short optionsare not abbreviations, so find's short-flag blocklist is not bypassable this way
and is left exactly as it is. There is a test pinning that, so a future change
cannot widen it by accident.
Linked issue
Self-identified. I have not filed a separate report because this turn's issue
budget went to the two
apply_patchdefects; happy to file one if you wouldrather have it tracked.
How did you verify it?
Three cases in
backend/cli/test/permission/next.test.ts:an abbreviated long option still classifies as risky—sed --in-p,sort --outp,tar --to-c; all three arecontainedon3e94875cshort options are not abbreviations and stay unaffected—find -delstayscontained, which is correctthe read-only spellings stay contained—find -name,sed -n, plainsort, so the fix is not "refuse more"Commands run:
bun test --timeout 120000 ./test/permission/next.test.ts→ 80 pass, 0 failbun run --cwd backend/cli typecheck→ exit 0The whole file matters more than the new cases: making a shared matcher stricter
could easily have flipped one of the 77 existing
containedassertions, and nonemoved.
The GNU behaviour above was checked with the Git-Bash toolchain on this machine
(
C:\Program Files\Git\bin\bash.exe), not assumed.Checklist
bun run checkis green (format, typecheck, backend + frontend/ui + SDK tests) — blocked on this Windows checkout by the CRLF and symlink artifacts in my other pull requests; backend typecheck is clean and the full permission suite is greenbun run --cwd frontend/workspace buildsucceeds if I touchedfrontend/workspaceorfrontend/ui— not touched./tooling/repo/generate.tswas run and thetooling/sdkoutput committed if I changedbackend/cli/src/server— not touchedfrontend/docs/src/content/openscience/is updated if behavior changed — no doc change needed: the risky/contained classification is internal, and the user-visible effect is that an approval prompt appears where one was being skippedpackage.jsonversions and tags are written by the release workflow)installandfrontend/landing/public/installare still byte-identical if I touched either — not touched