File paths: refuse a leading env reference, name the place it means

internal/policy/policy.go

@@ -6,6 +6,7 @@
 package policy
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -467,6 +468,9 @@ func (e *Engine) IsSecret(abs string) bool { return fileMatchFold(secretReadFloo
 // under the workspace, silently creating a literal "~" directory. The jail check
 // still runs after expansion, so ~ can't be used to escape it.
 func (e *Engine) Resolve(path string) (string, error) {
+	if err := unexpandedRef(path); err != nil {
+		return "", err
+	}
 	abs := expandTilde(path)
 	if !filepath.IsAbs(abs) {
 		abs = filepath.Join(e.base(), abs)
@@ -507,14 +511,65 @@ func (e *Engine) Resolve(path string) (string, error) {
 // shell behavior so the model's "~/file" lands in $HOME instead of a literal "~"
 // directory under the workspace.
 func expandTilde(p string) string {
-	if p == "~" || strings.HasPrefix(p, "~/") {
+	if p == "~" || strings.HasPrefix(p, "~/") || strings.HasPrefix(p, "~"+string(filepath.Separator)) { // ~\ on Windows
 		if home, err := os.UserHomeDir(); err == nil {
 			return filepath.Join(home, strings.TrimPrefix(p, "~"))
 		}
 	}
 	return p
 }
 
+// envRef matches an environment reference at the start of a path, in each
+// shell's own spelling: PowerShell's $env:NAME and ${env:NAME} (its "env:" in
+// any case), POSIX ${NAME} and $NAME, and cmd's %NAME% (whose names may hold
+// parentheses: %ProgramFiles(x86)%).
+var envRef = regexp.MustCompile(`^(?:\$(?i:env):([A-Za-z_][A-Za-z0-9_]*)|\$\{(?:(?i:env):)?([A-Za-z_][A-Za-z0-9_()]*)\}|\$([A-Za-z_][A-Za-z0-9_]*)|%([A-Za-z_][A-Za-z0-9_()]*)%)`)
+
+// ErrNotShellExpanded is the refusal of a path opening with an environment
+// reference (see unexpandedRef), for a caller to pass on as it is.
+var ErrNotShellExpanded = errors.New("file paths are not shell-expanded")
+
+// dirVars are the variables an unexpanded path is told the value of: places,
+// not secrets. Any other variable's value never appears — a model reading
+// "$OPENAI_API_KEY" must learn nothing from the error.
+var dirVars = map[string]bool{
+	"HOME": true, "USERPROFILE": true, "APPDATA": true, "LOCALAPPDATA": true,
+	"TEMP": true, "TMP": true, "TMPDIR": true, "PROGRAMDATA": true,
+	"PROGRAMFILES": true, "PROGRAMFILES(X86)": true, "SYSTEMROOT": true, "WINDIR": true,
+	"XDG_CONFIG_HOME": true, "XDG_DATA_HOME": true, "XDG_CACHE_HOME": true,
+}
+
+// unexpandedRef refuses a path that opens with an environment reference.
+// Nothing on the way to a file tool is a shell, so "$env:USERPROFILE\.gitconfig"
+// was joined onto the workspace as a literal "$env:USERPROFILE" directory: the
+// read failed on a path the model never wrote, after the risk check had scored
+// the home directory it meant. It is not expanded here either: the prompt, the
+// risk check and git all see the path as written, and a call must open what
+// they saw — and the value of an arbitrary variable, echoed back in an error,
+// is a way to read a secret. A reference counts only when a separator or the
+// end follows it: "$Recycle.Bin" and "Foo$Bar.class" are file names. A
+// backslash counts as a separator everywhere: models write Windows paths on
+// any system, and "$env:USERPROFILE\.gitconfig" is the same mistake on macOS.
+func unexpandedRef(p string) error {
+	m := envRef.FindStringSubmatch(p)
+	if m == nil {
+		return nil
+	}
+	if rest := p[len(m[0]):]; rest != "" && rest[0] != '/' && rest[0] != '\\' {
+		return nil
+	}
+	name := m[1] + m[2] + m[3] + m[4] // exactly one group matched
+	key := name                       // "home" is not "HOME" on Unix: only Windows folds case
+	if runtime.GOOS == "windows" {
+		key = strings.ToUpper(name)
+	}
+	hint := ""
+	if v := os.Getenv(name); v != "" && dirVars[key] && filepath.IsAbs(v) {
+		hint = fmt.Sprintf(" (%s is %s)", m[0], v)
+	}
+	return fmt.Errorf("path %q starts with %s, but %w — pass the full path%s", p, m[0], ErrNotShellExpanded, hint)
+}
+
 // maxSymlinkChase bounds resolveSymlinks' own-target chase (see below) against
 // a symlink cycle (a -> b -> a) looping forever.
 const maxSymlinkChase = 20

internal/policy/policy_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -45,6 +46,48 @@ func TestResolveJailEscapeErrorExplainsItsAHardBoundary(t *testing.T) {
 	}
 }
 
+// A path opening with an environment reference is refused, not joined onto the
+// workspace as a literal "$env:USERPROFILE" directory — and not expanded
+// either: the prompt and the risk check saw the path as written. A place's
+// value is offered so the model can retry with the full path; any other
+// variable's value never appears.
+func TestResolveRefusesALeadingEnvReference(t *testing.T) {
+	home := t.TempDir()
+	t.Setenv("HOME", home)
+	t.Setenv("IPCO_TEST_SECRET", "sk-do-not-leak")
+	c := config.Default()
+	c.Workspace = home
+	c.File = config.FilePolicy{Default: "allow", Jail: "."}
+	e := eng(t, c)
+	sep := string(filepath.Separator)
+	for _, p := range []string{
+		"$env:HOME/note.json", "$ENV:HOME" + sep + "note.json", "${env:HOME}/note.json",
+		"${HOME}/note.json", "$HOME/note.json", "%HOME%/note.json", "$HOME",
+		`$env:HOME\note.json`, // Windows spelling on any system
+	} {
+		_, err := e.Resolve(p)
+		if err == nil || !strings.Contains(err.Error(), "not shell-expanded") || !strings.Contains(err.Error(), home) {
+			t.Errorf("Resolve(%q) error = %v; want it refused, naming where HOME is", p, err)
+		}
+	}
+	if runtime.GOOS != "windows" { // "home" is its own variable on Unix, not HOME
+		t.Setenv("home", "/elsewhere/private")
+	}
+	for _, p := range []string{"$IPCO_TEST_SECRET", "$env:IPCO_TEST_SECRET/x", "%IPCO_TEST_SECRET%/x", "$home/x", "${env:ProgramFiles(x86)}/x"} {
+		_, err := e.Resolve(p)
+		if err == nil || strings.Contains(err.Error(), "sk-do-not-leak") || strings.Contains(err.Error(), "/elsewhere/private") {
+			t.Errorf("Resolve(%q) error = %v; want it refused without the value", p, err)
+		}
+	}
+	// Not references: a file name with a $ in it, and a name followed by more name.
+	for _, p := range []string{"Foo$Bar.class", "$HOME.bak", "$Recycle.Bin"} {
+		got, err := e.Resolve(p)
+		if err != nil || got != filepath.Join(home, p) {
+			t.Errorf("Resolve(%q) = %q, %v; want it left as a name in the workspace", p, got, err)
+		}
+	}
+}
+
 func TestResolveExpandsTilde(t *testing.T) {
 	home := t.TempDir()
 	setHome(t, home)
@@ -60,6 +103,12 @@ func TestResolveExpandsTilde(t *testing.T) {
 	if strings.Contains(got, "~") || filepath.Base(got) != "note.json" {
 		t.Errorf("Resolve(~/note.json) = %q, want <home>/note.json", got)
 	}
+	// ~\ is Windows' spelling of the same; elsewhere a backslash is a name character.
+	if filepath.Separator == '\\' {
+		if got, err := e.Resolve(`~\note.json`); err != nil || strings.Contains(got, "~") {
+			t.Errorf(`Resolve(~\note.json) = %q, %v; want <home>\note.json`, got, err)
+		}
+	}
 
 	// ~ must still respect the jail: from a sub-directory jail, ~ (the parent) escapes
 	sub := filepath.Join(home, "sub")

internal/tool/ask_test.go

@@ -65,3 +65,15 @@ func TestNoAnswerMarksOnlyANo(t *testing.T) {
 	}
 	NoAnswer(context.Background()) // outside an Ask: nothing to mark, and no panic
 }
+
+// git passes on the refusal of a path opening with an env reference — with
+// its hint at where the path meant — instead of calling it outside the workspace.
+func TestGitPassesOnTheUnexpandedPathHint(t *testing.T) {
+	tl := gitToolFor(t, initRepo(t), yes())
+	for _, url := range []string{"$HOME/repo", "$env:HOME/repo"} { // the second reads as host:path
+		r := tl.Call(context.Background(), "clone", map[string]any{"url": url})
+		if !r.IsError || !strings.Contains(r.Content, "not shell-expanded") {
+			t.Errorf("clone %s = %+v, want the not-shell-expanded refusal", url, r)
+		}
+	}
+}

internal/tool/git.go

@@ -131,11 +131,21 @@ func localGitPath(v string) bool {
 // fetching from one outside reads files the jail exists to keep out, and
 // pushing to one writes there (found by review).
 func (g *gitTool) checkLocalGitPath(v string) error {
+	if strings.HasPrefix(v, "$") || strings.HasPrefix(v, "%") {
+		// "$env:HOME/repo" reads as host:path to localGitPath, and git then
+		// failed on ssh to a host named "$env"; no server is named like that.
+		if _, err := g.pol.Resolve(v); errors.Is(err, policy.ErrNotShellExpanded) {
+			return err
+		}
+	}
 	if !localGitPath(v) {
 		return nil
 	}
 	p := strings.TrimPrefix(strings.TrimPrefix(v, "file://"), "file:")
 	if _, err := g.pol.Resolve(p); err != nil {
+		if errors.Is(err, policy.ErrNotShellExpanded) {
+			return err // its own hint says where the path meant
+		}
 		return errors.New("a local repository outside the workspace isn't reachable from here: " + v)
 	}
 	return nil