Fix unsafe shell invocation in amber exec#1376
Conversation
…22) * Harden editor invocation against shell injection; fix Crystal 1.20 deprecations exec.cr and encrypt.cr previously called system("#{editor} #{filename}"), allowing shell metacharacters in the EDITOR env var or filename to execute arbitrary commands. Replace with Process.parse_arguments + Process.run so the editor string is word-split (supporting "code -w", "vim -u none", etc.) and the filename is passed as a literal argument with no shell involvement. Add regression specs asserting shell metacharacters in editor/filename do not execute. Fix pattern contributed by @tcoatswo in amberframework/amber#1376. process_runner.cr used Time.monotonic which is deprecated in Crystal 1.20.0 in favour of Time.instant (Time::Instant, introduced in 1.20.0). Replace both call sites. The shard.yml crystal floor is raised to >= 1.20.0 to match (see version bump commit). Fix contributed by @renich in amberframework/amber#1379. amber.js Channel.join/leave/push previously called this.socket.ws.send() without error handling; a dead WebSocket throws an uncaught DOMException in the browser. Wrap each send in try/catch with a fallback to _reconnect(). Fix pattern contributed by @jonsilverman50-star in amberframework/amber#1375. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Bump version to 2.0.1; raise Crystal floor to >= 1.20.0 Time.instant (used in process_runner.cr) was introduced in Crystal 1.20.0. Raise the shard.yml crystal constraint from >= 1.10.0 to >= 1.20.0 to reflect this requirement. Bump shard version 2.0.0 -> 2.0.1 to signal the patch release containing the exec/encrypt hardening and deprecation fixes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
Thank you for tracking down both injection vectors and writing regression specs to cover them. Replacing the shell-interpolated Both PRs are valid and complementary (#1376 → master, #1377 → stable backport). Merging both. You'll also want to know your fix outlived v1: the standalone CLI that replaces the bundled one had independently fixed the file-copy path with Maintainer attention has been on V2 (now public: #1383), which is why these sat for a few weeks — sorry about that, and grateful for the security work. |
2 participants
Summary
Replace shell-interpolated process execution in amber exec with argument-array Process.run calls.
Changes
Verification
I could not run the Crystal spec suite in this environment because the host does not have crystal or shards installed. This PR includes regression specs for the affected code paths for maintainers/CI to exercise.