#641

feat(interpreter): allow static glob arguments in streaming pipelines

Draftasafyishwants to merge intofrom

Allow static glob arguments through the existing streaming eligibility gate. Existing argument expansion owns sorting, quoting and nullglob; dynamic substitutions remain on the buffered executor.

Draft while #453 remains unmerged. This branch is rebased onto current main with the shared prerequisite fixes. Focused follow-up diff. The full GitHub diff includes the prerequisite; it should be rebased again once that prerequisite lands.

Validation: focused behavior tests, TypeScript, scoped Biome, knip and banned-pattern checks pass. No manual workflow rerun or review-bot request was made.

Activity

14
vercel[bot]12d ago

@asafyish is attempting to deploy a commit to the Vercel Labs Team on Vercel.

A member of the Team first needs to authorize it.

auto-maintain[bot]12d ago

🤖 auto-maintain review

Automated, advisory triage for @asafyish's PR. Facts below are read from the GitHub API.

CheckResult
Author's merged PRs (this repo)1
Account established✅ (age 5392d · 13 followers · 7 public repos)
Commits signed/verified⚠️ 0/10
Changeset included✅ (.changeset/streaming-pipelines.md)

Review panel: 🔴 high highest severity

just-bash maintainer code review: 🟢 low

No actionable correctness or maintainability issues found in the complete base-to-head diff.

General code review: 🟡 medium

The streaming path introduces a chunk-size-dependent input-limit regression for early-closing `head` pipelines.

  • packages/just-bash/src/commands/head/head.ts:48 — Streaming `head` reads and charges an entire pipe chunk before selecting the requested prefix. With a small `maxInputBytes`, `seq 100 | head -c 1` can fail because the first ~4 KiB chunk exceeds the limit even though `head` needs only one byte. Support bounded reads or retain the unconsumed suffix so only consumed bytes count.

Adversarial security: 🔴 high

The streaming result-accounting path creates a concrete sandbox bypass for untrusted extensions running beside trusted stages.

  • packages/just-bash/src/interpreter/streaming-pipeline.ts:243 — Untrusted streaming results are re-inspected after leaving runUntrustedAsync. A frozen result can retain a stateful Proxy in internalOutputAccounting; its getter runs here while a concurrent trusted stage keeps the shared execution trusted, bypassing blocked-global protections. Normalize this metadata inside the untrusted scope or avoid re-reading extension results.

Adversarial security (second opinion): 🟡 medium

Streaming-pipeline change is well-gated and the trust-scope and output-accounting hazards are handled; one actionable defect: `execWithInheritedStdin` silently inherits empty stdin inside a streaming stage while `ctx.stdin` throws.

  • packages/just-bash/src/interpreter/builtin-dispatch.ts:852 — `execWithInheritedStdin` builds its child stdin from `effectiveStdin`, which is always empty for a streaming stage (the stage is invoked with stdin "" and `groupStdin` is excluded by the gate). The sibling `ctx.stdin` getter was deliberately made to throw in that situation, but this path silently substitutes empty input instead: a `streaming: true` custom command that delegates with `ctx.execWithInheritedStdin("wc -c")` now runs the child against no data where the buffered pipeline passed the real upstream bytes, producing silently wrong results rather than an error. Either read through `ctx.stdio`/`readCommandStdin` here or make it fail loudly like `ctx.stdin`.

Standard Bash and host portability: 🟢 low

No actionable Standard Bash or ordinary-host portability issues found.

Posted by auto-maintain. This automated code review is advisory; a human maintainer makes the call.

asafyish11d ago

Confirmed and fixed in f45829ac. With /1.txt containing one\n, shopt -s extglob; cat /@($(seq 1)).txt | cat previously leaked 1\n into the pipeline and reported bad operands. It now produces one\n, matching native Bash. The same regression covers backtick substitution.

The gate now reuses globPatternHasVarRef and patternHasCommandSubstitution; dynamic patterns stay on the existing buffered path. No changes to nested execution or glob expansion were needed.

Also carried the shared streaming trust fix in 14f57732: explicitly untrusted command dispatch uses the existing runUntrustedAsync wrapper. A synchronized two-stage regression demonstrated the original leak and passes with the one-line dispatch change. This is shared foundation work, independent of glob eligibility.

Validation: 1,407 interpreter/custom-command/defense tests pass (one existing skip), including the new regressions; build/type checking, formatting and banned-pattern checks pass. The two build-dependent defense tests were rerun successfully after building this checkout.

asafyish10d ago

Verified the shared result-getter issue reported on #642 and carried its fix here in f63e40f8. Result accounting and copying now stay inside the existing command trust scope; the existing overlap regression also covers a returned getter. 41 focused glob/pipeline tests and typecheck pass.

The mixed cat prefix - finding is real, but is inherited from #453 and is independent of this static-glob eligibility change. No additional glob behavior was changed.

auto-maintain[bot]10d ago

⚠️ auto-maintain: review severity raised to 🔴 high

A new push changed this PR and the review now contains a higher-severity finding. See the updated review comment above.

asafyish9d ago

Verified all three findings and fixed them in the shared foundation. Frozen empty extension results now have accounting metadata normalized inside the command trust scope; the regression attempts the stateful getter bypass and confirms it cannot escape. Pipe reads honor the remaining input allowance, so seq 100 | head -c 1 succeeds with maxInputBytes: 10 without changing the public stdio API. execWithInheritedStdin now raises the same explicit stdio error as ctx.stdin in a streaming command instead of silently passing empty input. Rebased onto current main and retained the small glob-only follow-up.

vercel-security-reviewer[bot]9d ago

This pull request is a draft

Merging is unavailable until it is marked ready for review.