-
Notifications
You must be signed in to change notification settings - Fork 0
fix(app): Find resolves programs on Windows — PATHEXT candidates, no mode bit #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| // SPDX-License-Identifier: EUPL-1.2 | ||
|
|
||
| // Application identity for the Core framework. | ||
|
|
@@ -47,26 +47,45 @@ | |
| // r := core.App{}.Find("node", "Node.js") | ||
| // if r.OK { app := r.Value.(*App) } | ||
| func (a App) Find(filename, name string) Result { | ||
| // If filename contains a separator, check it directly | ||
| if Contains(filename, string(PathSeparator)) { | ||
| pathExt := Env("PATHEXT") | ||
| if pathExt == "" && OS() == "windows" { | ||
| // Go's own default when the variable is unset. | ||
| pathExt = ".COM;.EXE;.BAT;.CMD" | ||
| } | ||
| return a.findWith(filename, name, Env("PATH"), pathExt) | ||
| } | ||
|
|
||
| // findWith is Find with the environment as arguments, so tests can drive | ||
| // the Windows resolution rules from any platform against a fixture PATH. | ||
| // An empty extension list means POSIX semantics; a non-empty one means | ||
| // Windows PATHEXT semantics — the list IS the platform switch. | ||
| func (a App) findWith(filename, name, pathEnv, pathExt string) Result { | ||
| // A path rather than a bare name is checked directly. Go accepts "/" | ||
| // as a separator on Windows too, so both spellings mean "path" — | ||
| // matching only the platform separator sent "bin/tool" on a PATH | ||
| // hunt there instead of probing it. | ||
| if Contains(filename, string(PathSeparator)) || Contains(filename, "/") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Treat drive-relative Windows paths as direct paths. A valid Windows path such as Proposed fix- if Contains(filename, string(PathSeparator)) || Contains(filename, "/") {
+ if Contains(filename, string(PathSeparator)) || Contains(filename, "/") ||
+ (len(splitPathExt(pathExt)) != 0 && Contains(filename, ":")) {🤖 Prompt for AI Agents |
||
| abs := PathAbs(filename) | ||
| if !abs.OK { | ||
| return abs | ||
| } | ||
| if isExecutable(abs.Value.(string)) { | ||
| return Result{&App{Name: name, Filename: filename, Path: abs.Value.(string)}, true} | ||
| for _, candidate := range executableCandidates(abs.Value.(string), pathExt) { | ||
| if isExecutableWith(candidate, pathExt) { | ||
| return Result{&App{Name: name, Filename: filename, Path: candidate}, true} | ||
| } | ||
| } | ||
| return Result{E("app.Find", Concat(filename, " not found"), nil), false} | ||
| } | ||
|
|
||
| // Search PATH | ||
| pathEnv := Env("PATH") | ||
| if pathEnv == "" { | ||
| return Result{E("app.Find", "PATH is empty", nil), false} | ||
| } | ||
| for _, dir := range Split(pathEnv, string(PathListSeparator)) { | ||
| candidate := PathJoin(dir, filename) | ||
| if isExecutable(candidate) { | ||
| for _, candidate := range executableCandidates(PathJoin(dir, filename), pathExt) { | ||
| if !isExecutableWith(candidate, pathExt) { | ||
| continue | ||
| } | ||
| abs := PathAbs(candidate) | ||
| if !abs.OK { | ||
| continue | ||
|
|
@@ -77,8 +96,78 @@ | |
| return Result{E("app.Find", Concat(filename, " not found on PATH"), nil), false} | ||
| } | ||
|
|
||
| // isExecutable checks if a path exists and is executable. | ||
| // executableCandidates lists the filenames a program name may resolve to. | ||
| // With no extension list (POSIX) the path stands alone. With one (Windows | ||
| // PATHEXT) the name is tried as itself when it already carries a listed | ||
| // extension, then with each listed extension appended — "git" on disk is | ||
| // "git.exe", and the bare join alone would never find it. | ||
| func executableCandidates(path, pathExt string) []string { | ||
| exts := splitPathExt(pathExt) | ||
| if len(exts) == 0 { | ||
| return []string{path} | ||
| } | ||
| out := make([]string, 0, len(exts)+1) | ||
| if hasListedExt(path, exts) { | ||
| out = append(out, path) | ||
| } | ||
| for _, ext := range exts { | ||
| out = append(out, Concat(path, ext)) | ||
| } | ||
| return out | ||
| } | ||
|
|
||
| // splitPathExt parses a PATHEXT value (";"-separated, case-insensitive) | ||
| // into normalised lower-case ".ext" entries. Empty input yields nil — | ||
| // the POSIX arm. | ||
| func splitPathExt(pathExt string) []string { | ||
| if pathExt == "" { | ||
| return nil | ||
| } | ||
| var out []string | ||
| for _, e := range Split(pathExt, ";") { | ||
| e = Lower(Trim(e)) | ||
| if e == "" { | ||
| continue | ||
| } | ||
| if !HasPrefix(e, ".") { | ||
| e = Concat(".", e) | ||
| } | ||
| out = append(out, e) | ||
| } | ||
| return out | ||
| } | ||
|
|
||
| // hasListedExt reports whether path's extension appears in exts | ||
| // (already lower-cased ".ext" entries). | ||
| func hasListedExt(path string, exts []string) bool { | ||
| got := Lower(PathExt(path)) | ||
| if got == "" { | ||
| return false | ||
| } | ||
| for _, e := range exts { | ||
| if got == e { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // isExecutable checks if a path exists and is executable on this platform. | ||
| func isExecutable(path string) bool { | ||
| pathExt := Env("PATHEXT") | ||
| if pathExt == "" && OS() == "windows" { | ||
| pathExt = ".COM;.EXE;.BAT;.CMD" | ||
| } | ||
| return isExecutableWith(path, pathExt) | ||
| } | ||
|
|
||
| // isExecutableWith is isExecutable with the extension list as an argument. | ||
| // POSIX (empty list) asks the mode bits. Windows has no execute bit — | ||
| // Stat synthesises 0666, or 0444 for a read-only file — so the old | ||
| // mode&0111 test rejected EVERY file on the platform, git.exe included, | ||
| // and Find could never succeed there. With a list present the answer is | ||
| // "a regular file whose extension is listed", and the mode is not asked. | ||
| func isExecutableWith(path, pathExt string) bool { | ||
| r := Stat(path) | ||
| if !r.OK { | ||
| return false | ||
|
|
@@ -87,6 +176,13 @@ | |
| IsDir() bool | ||
| Mode() FileMode | ||
| }) | ||
| // Regular file with at least one execute bit | ||
| return !info.IsDir() && info.Mode()&0111 != 0 | ||
| if info.IsDir() { | ||
| return false | ||
| } | ||
| exts := splitPathExt(pathExt) | ||
| if len(exts) == 0 { | ||
| // Regular file with at least one execute bit | ||
| return info.Mode()&0111 != 0 | ||
| } | ||
| return hasListedExt(path, exts) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,3 +14,90 @@ func TestApp_isExecutable_Bad(t *T) { | |
| func TestApp_isExecutable_Ugly(t *T) { | ||
| AssertFalse(t, isExecutable(t.TempDir())) | ||
| } | ||
|
|
||
| func TestApp_isExecutableWith_Good(t *T) { | ||
| // The Windows receipt: a 0644 file — no execute bit anywhere — IS | ||
| // executable when its extension is listed. The old mode&0111 test | ||
| // rejected exactly this file, which is why Find never succeeded on | ||
| // the platform. | ||
| path := Path(t.TempDir(), "git.exe") | ||
| RequireTrue(t, WriteFile(path, []byte("MZ"), 0o644).OK) | ||
|
|
||
| AssertTrue(t, isExecutableWith(path, ".COM;.EXE;.BAT;.CMD")) | ||
| } | ||
| func TestApp_isExecutableWith_Bad(t *T) { | ||
| // An unlisted extension is not executable under PATHEXT semantics, | ||
| // however runnable its mode bits claim it is. | ||
| path := Path(t.TempDir(), "notes.txt") | ||
| RequireTrue(t, WriteFile(path, []byte("x"), 0o755).OK) | ||
|
|
||
| AssertFalse(t, isExecutableWith(path, ".COM;.EXE")) | ||
| } | ||
| func TestApp_isExecutableWith_Ugly(t *T) { | ||
| // An empty list is the POSIX arm: mode bits decide, so a 0644 file | ||
| // stays non-executable and a directory never qualifies either way. | ||
| path := Path(t.TempDir(), "plain") | ||
| RequireTrue(t, WriteFile(path, []byte("x"), 0o644).OK) | ||
|
|
||
| AssertFalse(t, isExecutableWith(path, "")) | ||
| AssertFalse(t, isExecutableWith(t.TempDir(), ".EXE")) | ||
| } | ||
|
|
||
| func TestApp_findWith_Good(t *T) { | ||
| // Bare name "git" resolves to git.exe on a fixture PATH under | ||
| // PATHEXT semantics — candidate generation plus the extension | ||
| // check working together, from any host platform. | ||
| dir := t.TempDir() | ||
| RequireTrue(t, WriteFile(Path(dir, "git.exe"), []byte("MZ"), 0o644).OK) | ||
|
|
||
| r := App{}.findWith("git", "Git", dir, ".COM;.EXE;.BAT;.CMD") | ||
| RequireTrue(t, r.OK) | ||
| AssertEqual(t, Path(dir, "git.exe"), r.Value.(*App).Path) | ||
| } | ||
| func TestApp_findWith_Bad(t *T) { | ||
| // Nothing on the fixture PATH satisfies the name; and an empty | ||
| // PATH is its own distinct failure. | ||
| AssertFalse(t, App{}.findWith("git", "Git", t.TempDir(), ".EXE").OK) | ||
| AssertFalse(t, App{}.findWith("git", "Git", "", ".EXE").OK) | ||
| } | ||
| func TestApp_findWith_Ugly(t *T) { | ||
| // A forward-slash path is a direct check on every platform — Go | ||
| // accepts "/" on Windows, so "sub/tool" must never be hunted on | ||
| // PATH. The candidate resolves via extension there too. | ||
| dir := t.TempDir() | ||
| RequireTrue(t, MkdirAll(Path(dir, "sub"), 0o755).OK) | ||
| RequireTrue(t, WriteFile(Path(dir, "sub", "tool.exe"), []byte("MZ"), 0o644).OK) | ||
|
|
||
| r := App{}.findWith(Concat(dir, "/sub/tool"), "Tool", "", ".EXE") | ||
| RequireTrue(t, r.OK) | ||
| AssertEqual(t, Path(dir, "sub", "tool.exe"), r.Value.(*App).Path) | ||
| } | ||
|
|
||
| func TestApp_splitPathExt_Good(t *T) { | ||
| exts := splitPathExt(".COM;.EXE;bat; .Cmd ;") | ||
| AssertEqual(t, 4, len(exts)) | ||
| AssertEqual(t, ".com", exts[0]) | ||
| AssertEqual(t, ".exe", exts[1]) | ||
| AssertEqual(t, ".bat", exts[2]) | ||
| AssertEqual(t, ".cmd", exts[3]) | ||
| } | ||
| func TestApp_splitPathExt_Bad(t *T) { | ||
| AssertEqual(t, 0, len(splitPathExt(""))) | ||
| } | ||
| func TestApp_executableCandidates_Good(t *T) { | ||
| // A name already carrying a listed extension is tried as itself | ||
| // first, then with each extension appended; a bare name only with | ||
| // the extensions; POSIX (empty list) is the path alone. | ||
| withExt := executableCandidates("dir/git.exe", ".COM;.EXE") | ||
| AssertEqual(t, 3, len(withExt)) | ||
| AssertEqual(t, "dir/git.exe", withExt[0]) | ||
|
|
||
| bare := executableCandidates("dir/git", ".COM;.EXE") | ||
| AssertEqual(t, 2, len(bare)) | ||
| AssertEqual(t, "dir/git.com", bare[0]) | ||
| AssertEqual(t, "dir/git.exe", bare[1]) | ||
|
|
||
| posix := executableCandidates("dir/git", "") | ||
| AssertEqual(t, 1, len(posix)) | ||
| AssertEqual(t, "dir/git", posix[0]) | ||
| } | ||
|
Comment on lines
+76
to
+103
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Add the mandatory coverage states.
As per coding guidelines, “All three coverage states are mandatory: 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict
PATHEXTprocessing to Windows.A populated
PATHEXTon POSIX changes these methods from execute-bit checks to extension checks. This breaks the stated unchanged POSIX behaviour.app.go#L50-L55: initialisepathExtonly whenOS() == "windows".app.go#L157-L161: initialisepathExtonly whenOS() == "windows".📍 Affects 1 file
app.go#L50-L55(this comment)app.go#L157-L161🤖 Prompt for AI Agents