Repository navigation
fix: preserve run script argument boundaries - #7817
Conversation
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7817 +/- ##
=======================================
Coverage 2.82% 2.82%
=======================================
Files 669 669
Lines 72527 72527
Branches 990 990
=======================================
Hits 2048 2048
Misses 70479 70479
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
All checks are passing. Happy to adjust the implementation if maintainers prefer a different approach for the pnpm forwarding case. |
knsv-bot
left a comment
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Review: preserve run script argument boundaries
Thanks for picking this up, @Adi-Beker — and for the careful diagnosis on the issue. You correctly identified that the real bug wasn't the ${@:2} slice itself (that part was already fine) but the scalar assignment args=${@:2} collapsing the forwarded arguments into a single whitespace-joined string, which then got re-split (and glob-expanded) at each use site. Switching to a Bash array with quoted expansions is exactly the right fix.
What's working well
- 🎉 The
pnpmcase is handled really well. Replacingsh -c "pnpm $args"(interpolating into the command string — fragile, and a shell-injection footgun) withsh -c 'pnpm "$@"' sh "${args[@]}"is the correct, idiomatic way to forward arguments across the nested-shell boundary:$0becomes the placeholdersh,$1..become the array elements, andpnpm "$@"preserves every boundary. Arguments containing spaces (e.g../run pnpm vitest 'a test name') now survive intact. - 🎉 Consistent and complete. All three consumers of
args(build,pnpm,cypress) were updated — no stragglers left on the old scalar form — andargs=("${@:2}")is the proper array capture. Since the script is#!/usr/bin/env bashwith noset -u, the empty-args case (e.g../run cypress) expands"${args[@]}"to nothing safely.
Security
I assessed this directly rather than through the usual SVG/XSS pass — there's no rendering surface here. The relevant angle for a shell script is command injection, and this change reduces that surface: it removes the sh -c "pnpm $args" interpolation in favor of passing arguments as positional parameters, so user-supplied args are no longer re-parsed by the inner shell. Net safety improvement. ✅
Notes (non-blocking)
- 💡 Out of scope for this PR — line 11's
name=$(basename $0)has an unquoted$0(same general class of quoting issue, just for the script's own path). Not worth widening this PR for; flagging only in case you want a quick follow-up someday. - On validation:
bash -n runconfirms syntax, which is a reasonable bar for a Docker wrapper that isn't easily unit-tested. If you have Docker handy, a quick./run pnpm vitest 'some name with spaces'makes a nice end-to-end sanity check. No changeset is needed here —runis a root dev-tooling helper, not a published package.
Clean, well-targeted fix that resolves the underlying issue. LGTM. 🚀
|
@Adi-Beker, Thank you for the contribution! |
3 participants
Description
This PR fixes argument forwarding in the root
runhelper script.The script previously assigned forwarded arguments with
args=${@:2}, which converted the remaining positional parameters into a scalar string. This could lose the original argument boundaries when forwarding arguments to Docker, pnpm, or Cypress commands.This change stores forwarded arguments in a Bash array and expands them safely where needed.
Related Issue
Fixes #7797
Validation
bash -n runto verify the script syntax.