fix(expand): -r only converts to relative path on special vars (#63)

Fixes #61, where in certain cases the path to SCMPUFF_GIT_CMD was being converted from an absolute path to a relative path, causes issues in certain filesystem traversal.

commands/internal/arguments/arguments.go

@@ -9,31 +9,48 @@ import (
 	"strconv"
 )
 
-var expandArgDigitMatcher = regexp.MustCompile("^[0-9]{0,4}$")
-var expandArgRangeMatcher = regexp.MustCompile("^([0-9]+)-([0-9]+)$")
+var (
+	expandArgDigitMatcher = regexp.MustCompile("^[0-9]{0,4}$")
+	expandArgRangeMatcher = regexp.MustCompile("^([0-9]+)-([0-9]+)$")
+	managedEnvVar         = regexp.MustCompile(`^\$e\d+$`)
+)
 
-// EvaluateEnvironment evaluates a string of arguments and expands environment variables.
+// EvaluateEnvironment evaluates a single arguments and expands environment
+// variables.
+//
+// For scmpuff-managed position variables only (e.g. $e1, etc), the variable is
+// expanded into a locatable file path, and expandRelative is true, it will be
+// converted into a relative path when possible.
 func EvaluateEnvironment(arg string, expandRelative bool) string {
-	expandedArg := os.ExpandEnv(arg)
-	if expandRelative {
-		return convertToRelativeIfFilePath(expandedArg)
+	expanded := os.ExpandEnv(arg)
+	wasChanged := (expanded != arg)
+	if wasChanged && expandRelative && managedEnvVar.MatchString(arg) {
+		relPath, err := convertToRelativeIfFilePath(expanded)
+		if err == nil {
+			return relPath
+		}
 	}
-	return expandedArg
+	return expanded
 }
 
 // For a given arg, try to determine if it represents a file, and if so, convert
 // it to a relative filepath.
 //
 // Otherwise (or if any error conditions occur) return it unmolested.
-func convertToRelativeIfFilePath(arg string) string {
-	if _, err := os.Stat(arg); err == nil {
-		wd, err1 := os.Getwd()
-		relPath, err2 := filepath.Rel(wd, arg)
-		if err1 == nil && err2 == nil {
-			return relPath
-		}
+func convertToRelativeIfFilePath(arg string) (string, error) {
+	_, err := os.Stat(arg)
+	if err != nil {
+		return arg, err
+	}
+	wd, err := os.Getwd()
+	if err != nil {
+		return arg, err
+	}
+	relPath, err := filepath.Rel(wd, arg)
+	if err != nil {
+		return arg, err
 	}
-	return arg
+	return relPath, nil
 }
 
 // Expand takes the list of arguments received from the command line and expands

commands/internal/arguments/arguments_test.go

@@ -1,6 +1,8 @@
 package arguments
 
 import (
+	"os"
+	"path/filepath"
 	"reflect"
 	"strings"
 	"testing"
@@ -46,3 +48,54 @@ func TestExpandArg(t *testing.T) {
 		}
 	}
 }
+
+func TestEvaluateEnvironment(t *testing.T) {
+	// It would be wonderful to use fstest.MapFS here and have the function rely
+	// upon fs.StatFS, however MapFS currently does not work with absolute paths
+	// at all, which makes it useless for our testing here.  Bummer. So we use a
+	// testdata fixture instead.
+	/* var mockFS = fstest.MapFS{
+		"/foo/a.txt":         &fstest.MapFile{Mode: 0644},
+		"/foo/b.txt":         &fstest.MapFile{Mode: 0644},
+		"/foo/bar/c.txt":     &fstest.MapFile{Mode: 0644},
+		"/usr/local/bin/git": &fstest.MapFile{Mode: 0777},
+	} */
+	wd, err := os.Getwd()
+	if err != nil {
+		t.Skip("failed to get wd, cannot test")
+	}
+
+	fakegitAbsPath := filepath.Join(wd, "testdata", "bin", "fakegit")
+	t.Setenv("SCMPUFF_GIT_CMD", fakegitAbsPath)
+	t.Setenv("e1", filepath.Join(wd, "testdata", "a.txt"))
+	t.Setenv("e2", filepath.Join(wd, "testdata", "b.txt"))
+	t.Setenv("FOO_USER", "not_a_file")
+
+	t.Logf("$CWD=%v", wd)
+	t.Logf("$SCMPUFF_GIT_CMD=%v", os.Getenv("SCMPUFF_GIT_CMD"))
+	t.Logf("$e1=%v", os.Getenv("e1"))
+	t.Logf("$e2=%v", os.Getenv("e2"))
+
+	tests := []struct {
+		name           string
+		arg            string
+		expandRelative bool
+		want           string
+	}{
+		{name: "not an env var", arg: "eee", expandRelative: false, want: "eee"},
+		{name: "not file absolute", arg: "$FOO_USER", expandRelative: false, want: "not_a_file"},
+		{name: "not file relative", arg: "$FOO_USER", expandRelative: true, want: "not_a_file"},
+		{name: "absolute file", arg: "$e1", expandRelative: false, want: filepath.Join(wd, "testdata", "a.txt")},
+		{name: "relative file", arg: "$e1", expandRelative: true, want: filepath.FromSlash("testdata/a.txt")},
+		{name: "path binary dont convert relative - abs", arg: "$SCMPUFF_GIT_CMD", expandRelative: false, want: fakegitAbsPath},
+		{name: "path binary dont convert relative - rel", arg: "$SCMPUFF_GIT_CMD", expandRelative: true, want: fakegitAbsPath},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := EvaluateEnvironment(tt.arg, tt.expandRelative); got != tt.want {
+				t.Errorf("EvaluateEnvironment(%v, %v) = %v, want %v", tt.arg, tt.expandRelative, got, tt.want)
+			}
+		})
+	}
+}

commands/internal/arguments/testdata/a.txt

@@ -0,0 +1 @@
+a

commands/internal/arguments/testdata/b.txt

@@ -0,0 +1 @@
+b

features/command_expand.feature

@@ -69,16 +69,33 @@ Feature: command expansion at command line
       Then the stderr should not contain anything
         And the output should match /git\tcommit\t-m\tfoo\\;\\ bar/
 
-  Scenario: Allow user to specify --relative paths
+  Scenario Outline: Allow user to specify --relative paths
     Given a directory named "foo"
       And a directory named "foo/bar"
       And an empty file named "xxx.jpg"
       And I cd to "foo/bar"
     Given I override environment variable "e1" to the absolute path of "xxx.jpg"
-    When I successfully run `scmpuff expand 1`
-    Then the stdout from "scmpuff expand 1" should contain "/tmp/aruba/xxx.jpg"
-    When I successfully run `scmpuff expand -r -- 1`
-    Then the stdout from "scmpuff expand -r -- 1" should contain "../../xxx.jpg"
+    When I successfully run `<cmd>`
+    Then the output should match <output_regex>
+    Examples:
+      | cmd                    | output_regex                 |
+      | scmpuff expand 1       | %r<^\S*/tmp/aruba/xxx\.jpg$> |
+      | scmpuff expand -r -- 1 | %r<\.\./\.\./xxx\.jpg>       |
+
+  Scenario Outline: Do not convert anything other than special variables to --relative paths
+    Given a directory named "foo"
+      And a directory named "foo/bar"
+      And an empty file named "fakegit"
+      And an empty file named "xxx.jpg"
+      And I cd to "foo/bar"
+    Given I override environment variable "SCMPUFF_GIT_CMD" to the absolute path of "fakegit"
+      And I override environment variable "e1" to the absolute path of "xxx.jpg"
+    When I successfully run `<cmd>`
+    Then the output should match <output_regex>
+    Examples:
+      | cmd                                              | output_regex                                                  |
+      | scmpuff expand -- $SCMPUFF_GIT_CMD checkout 1    | %r<^\S*/tmp/aruba/fakegit\tcheckout\t\S*/tmp/aruba/xxx\.jpg$> |
+      | scmpuff expand -r -- $SCMPUFF_GIT_CMD checkout 1 | %r<^\S*/tmp/aruba/fakegit\tcheckout\t\.\./\.\./xxx\.jpg$>    |
 
   Scenario: Don't trim empty string args when expanding command
     There are certain situations where someone would want to actually pass an