diff --git a/internal/policy/policy.go b/internal/policy/policy.go index 61c6226..8cdf494 100644 --- a/internal/policy/policy.go +++ b/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,7 +511,7 @@ 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, "~")) } @@ -515,6 +519,57 @@ func expandTilde(p string) string { 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 diff --git a/internal/policy/policy_test.go b/internal/policy/policy_test.go index 29fbe61..6e6d793 100644 --- a/internal/policy/policy_test.go +++ b/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 /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 \note.json`, got, err) + } + } // ~ must still respect the jail: from a sub-directory jail, ~ (the parent) escapes sub := filepath.Join(home, "sub") diff --git a/internal/tool/ask_test.go b/internal/tool/ask_test.go index db08d01..4d22b1d 100644 --- a/internal/tool/ask_test.go +++ b/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) + } + } +} diff --git a/internal/tool/git.go b/internal/tool/git.go index 6a12f66..6f31beb 100644 --- a/internal/tool/git.go +++ b/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