Harden exec/encrypt editor invocation, fix Crystal 1.20 deprecations

shard.yml

@@ -1,10 +1,10 @@
 name: amber_cli
-version: 2.0.0
+version: 2.0.1
 
 authors:
   - crimson-knight <crimsonknightstudios@gmail.com>
 
-crystal: ">= 1.10.0, < 2.0"
+crystal: ">= 1.20.0, < 2.0"
 
 license: MIT
 

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"

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 <marker>
+    # 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

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?

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
 

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

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

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()
+    }
   }
 }