diff --git a/shard.yml b/shard.yml index 77d07d9..3ca693d 100644 --- a/shard.yml +++ b/shard.yml @@ -1,10 +1,10 @@ name: amber_cli -version: 2.0.0 +version: 2.0.1 authors: - crimson-knight -crystal: ">= 1.10.0, < 2.0" +crystal: ">= 1.20.0, < 2.0" license: MIT diff --git a/spec/amber_cli_spec.cr b/spec/amber_cli_spec.cr index 9850860..4180292 100644 --- a/spec/amber_cli_spec.cr +++ b/spec/amber_cli_spec.cr @@ -58,6 +58,7 @@ require "./core/generator_config_spec" require "./core/template_engine_spec" require "./commands/base_command_spec" require "./commands/new_command_spec" +require "./commands/exec_command_spec" require "./native/capability_manifest_spec" require "./generators/native_app_spec" require "./integration/generator_manager_spec" diff --git a/spec/commands/exec_command_spec.cr b/spec/commands/exec_command_spec.cr new file mode 100644 index 0000000..dcb773f --- /dev/null +++ b/spec/commands/exec_command_spec.cr @@ -0,0 +1,73 @@ +require "../amber_cli_spec" +require "../../src/amber_cli/core/base_command" + +# Regression spec for shell injection fix in exec/encrypt editor invocation. +# Verifies that shell metacharacters in the editor option or filename are NOT +# executed by the shell. Reference: amberframework/amber#1376 by @tcoatswo. +# +# The vulnerability was: system("#{editor} #{filename}") — a crafted editor +# string like 'vim; touch /tmp/pwned' would execute the injected command. +# The fix uses Process.run with an explicit args array (no shell: true). +describe "ExecCommand editor invocation safety" do + it "does not execute injected shell commands when editor contains semicolon" do + # With system("#{editor} #{filename}"), setting editor = "vim; touch /tmp/pwned" + # would run `touch /tmp/pwned`. With Process.parse_arguments + Process.run, + # the semicolon is part of the editor token, not a shell command separator. + marker = "/tmp/amber_exec_semi_pwned_#{Time.utc.to_unix_ms}" + File.delete(marker) if File.exists?(marker) + + # An attacker sets EDITOR=vim; touch + # Process.parse_arguments on POSIX treats "vim; touch /tmp/..." as a single + # token because there are no shell word-break chars (it's unquoted but + # parse_arguments_posix only splits on whitespace and quotes). + # The resulting command "vim; touch ..." will fail to execute (no such file). + malicious_editor = "vim; touch #{marker}" + editor_parts = Process.parse_arguments(malicious_editor) + editor_cmd = editor_parts.first # => "vim;" + editor_args = editor_parts[1..] + ["/dev/null"] + + # Run — we expect an exception (no binary named "vim;") but no marker creation. + begin + Process.run(editor_cmd, editor_args, input: Process::Redirect::Close, output: Process::Redirect::Close, error: Process::Redirect::Close) + rescue File::NotFoundError | File::AccessDeniedError | Exception + # Expected: binary "vim;" doesn't exist, Process.run raises. + end + + File.exists?(marker).should be_false + end + + it "does not execute injected shell commands when filename contains shell metacharacters" do + # With system("vim #{filename}"), a filename containing "; touch /tmp/pwned" + # would inject a command. With Process.run and explicit args, the filename + # is passed as a single argument — no shell parsing occurs. + marker = "/tmp/amber_exec_fn_pwned_#{Time.utc.to_unix_ms}" + File.delete(marker) if File.exists?(marker) + + injected_filename = "/dev/null; touch #{marker}" + + # Process.run does NOT invoke the shell, so the semicolon is literal. + # `cat` will fail (no such file "/dev/null; touch /tmp/...") but will + # NOT execute the touch command. + Process.run("cat", [injected_filename], input: Process::Redirect::Close, output: Process::Redirect::Close, error: Process::Redirect::Close) + + File.exists?(marker).should be_false + end + + it "handles multi-word editor like 'code -w' by splitting on whitespace" do + # When EDITOR="code -w", Process.parse_arguments yields ["code", "-w"]. + # The first token is the command and the rest are prepended to the filename. + editor = "code -w" + parts = Process.parse_arguments(editor) + parts.should eq(["code", "-w"]) + parts.first.should eq("code") + parts[1..].should eq(["-w"]) + end + + it "handles multi-word editor like 'vim -u none' by splitting on whitespace" do + editor = "vim -u none" + parts = Process.parse_arguments(editor) + parts.should eq(["vim", "-u", "none"]) + parts.first.should eq("vim") + parts[1..].should eq(["-u", "none"]) + end +end diff --git a/src/amber_cli.cr b/src/amber_cli.cr index 9542a23..074025a 100644 --- a/src/amber_cli.cr +++ b/src/amber_cli.cr @@ -38,7 +38,7 @@ end Log.builder.bind "*", :info, backend module AmberCLI - VERSION = "2.0.0" + VERSION = "2.0.1" def self.run(args = ARGV) if args.empty? diff --git a/src/amber_cli/commands/encrypt.cr b/src/amber_cli/commands/encrypt.cr index 88e049e..acea0e3 100644 --- a/src/amber_cli/commands/encrypt.cr +++ b/src/amber_cli/commands/encrypt.cr @@ -88,7 +88,10 @@ module AmberCLI::Commands unless no_edit info "Opening #{unencrypted_file} in #{editor}..." - system("#{editor} #{unencrypted_file}") + editor_parts = Process.parse_arguments(editor) + editor_cmd = editor_parts.first + editor_args = editor_parts[1..] + [unencrypted_file] + Process.run(editor_cmd, editor_args, input: Process::Redirect::Inherit, output: Process::Redirect::Inherit, error: Process::Redirect::Inherit) end end diff --git a/src/amber_cli/commands/exec.cr b/src/amber_cli/commands/exec.cr index f464fb1..f05e0ad 100644 --- a/src/amber_cli/commands/exec.cr +++ b/src/amber_cli/commands/exec.cr @@ -78,7 +78,10 @@ module AmberCLI::Commands if code.empty? || File.exists?(code) prepare_file - system("#{editor} #{filename}") + editor_parts = Process.parse_arguments(editor) + editor_cmd = editor_parts.first + editor_args = editor_parts[1..] + [filename] + Process.run(editor_cmd, editor_args, input: Process::Redirect::Inherit, output: Process::Redirect::Inherit, error: Process::Redirect::Inherit) else File.write(filename, wrap(code)) end diff --git a/src/amber_cli/helpers/process_runner.cr b/src/amber_cli/helpers/process_runner.cr index e9fa9e0..34c5ea0 100644 --- a/src/amber_cli/helpers/process_runner.cr +++ b/src/amber_cli/helpers/process_runner.cr @@ -121,11 +121,11 @@ module Sentry ok_to_run = true else log :run, "Building..." - time = Time.monotonic + time = Time.instant build_result = Amber::CLI::Helpers.run(build_command_run) exit 1 unless build_result.is_a? Process::Status if build_result.success? - log :run, "Compiled in #{(Time.monotonic - time)}" + log :run, "Compiled in #{(Time.instant - time)}" stop_processes("run") if @app_running ok_to_run = true elsif !@app_running # first run diff --git a/src/amber_cli/templates/app/public/js/amber.js b/src/amber_cli/templates/app/public/js/amber.js index 1abbd75..d66fdbb 100644 --- a/src/amber_cli/templates/app/public/js/amber.js +++ b/src/amber_cli/templates/app/public/js/amber.js @@ -39,14 +39,22 @@ export class Channel { * Join a channel, subscribe to all channels messages */ join() { - this.socket.ws.send(JSON.stringify({ event: EVENTS.join, topic: this.topic })) + try { + this.socket.ws.send(JSON.stringify({ event: EVENTS.join, topic: this.topic })) + } catch { + this._reconnect() + } } /** * Leave a channel, stop subscribing to channel messages */ leave() { - this.socket.ws.send(JSON.stringify({ event: EVENTS.leave, topic: this.topic })) + try { + this.socket.ws.send(JSON.stringify({ event: EVENTS.leave, topic: this.topic })) + } catch { + this._reconnect() + } } /** @@ -73,7 +81,11 @@ export class Channel { * @param {Object} payload - payload object: `{message: 'hello'}` */ push(subject, payload) { - this.socket.ws.send(JSON.stringify({ event: EVENTS.message, topic: this.topic, subject: subject, payload: payload })) + try { + this.socket.ws.send(JSON.stringify({ event: EVENTS.message, topic: this.topic, subject: subject, payload: payload })) + } catch { + this._reconnect() + } } }