diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 6b9fd7ad5..ee75769b1 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -63,6 +63,34 @@ jobs: name: unit-tests-artifact path: ./coverage-unit-tests.out + windows-tests: + name: Plugin Tests (Windows) + runs-on: windows-latest + steps: + - name: Check out the code + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Set up Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: go.mod + check-latest: true + + - name: Run plugin library tests + run: go test -count=1 ./internal/plugins/... + + - name: Run plugin command tests + if: ${{ !cancelled() }} + run: go test -count=1 -run 'Plugin|Approvals|Commands|InterruptGuard|OwnedFlags|NamespaceJSONHelp|RegistryNextAction' ./internal/cli + + - name: Run plugin library tests with the test registry key + if: ${{ !cancelled() }} + run: go test -count=1 -tags plugintest ./internal/plugins/... + + - name: Run plugin command tests with the test registry key + if: ${{ !cancelled() }} + run: go test -count=1 -tags plugintest -run 'Plugin|Approvals|Commands|InterruptGuard|OwnedFlags|NamespaceJSONHelp|RegistryNextAction' ./internal/cli + integration-tests: name: Integration Tests runs-on: ubuntu-latest diff --git a/internal/cli/plugin_discovery.go b/internal/cli/plugin_discovery.go index b42c860e7..df20436ca 100644 --- a/internal/cli/plugin_discovery.go +++ b/internal/cli/plugin_discovery.go @@ -282,6 +282,8 @@ func writePluginHint(path string, shown time.Time) { _, writeErr := temporary.Write(data) closeErr := temporary.Close() + // This is os.Rename and not plugins.Rename: the hint is best effort, and the + // retry could hold up a command by over a second for a write that may fail. if writeErr != nil || closeErr != nil || os.Rename(temporary.Name(), path) != nil { _ = os.Remove(temporary.Name()) } diff --git a/internal/cli/plugin_discovery_test.go b/internal/cli/plugin_discovery_test.go index d63a1e116..1b7472335 100644 --- a/internal/cli/plugin_discovery_test.go +++ b/internal/cli/plugin_discovery_test.go @@ -10,7 +10,6 @@ import ( "runtime" "strconv" "strings" - "syscall" "testing" "time" @@ -897,29 +896,6 @@ func TestReadPluginHint(t *testing.T) { assert.False(t, ok) }) - t.Run("a named pipe does not hold the command up", func(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("no named pipes here") - } - - pipe := filepath.Join(dir, "pipe") - require.NoError(t, syscall.Mkfifo(pipe, 0o600)) - - done := make(chan bool, 1) - - go func() { - _, ok := readPluginHint(pipe) - done <- ok - }() - - select { - case ok := <-done: - assert.False(t, ok) - case <-time.After(10 * time.Second): - t.Fatal("reading the hint file blocked on a named pipe") - } - }) - t.Run("a symbolic link is not followed", func(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("no symbolic links without a privilege") diff --git a/internal/cli/plugin_discovery_unix_test.go b/internal/cli/plugin_discovery_unix_test.go new file mode 100644 index 000000000..77f3c64c1 --- /dev/null +++ b/internal/cli/plugin_discovery_unix_test.go @@ -0,0 +1,32 @@ +//go:build !windows + +package cli + +import ( + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestReadPluginHint_NamedPipe(t *testing.T) { + pipe := filepath.Join(t.TempDir(), "pipe") + require.NoError(t, syscall.Mkfifo(pipe, 0o600)) + + done := make(chan bool, 1) + + go func() { + _, ok := readPluginHint(pipe) + done <- ok + }() + + select { + case ok := <-done: + assert.False(t, ok) + case <-time.After(10 * time.Second): + t.Fatal("reading the hint file blocked on a named pipe") + } +} diff --git a/internal/cli/plugin_run_test.go b/internal/cli/plugin_run_test.go index 9f62fe6b9..677e6c743 100644 --- a/internal/cli/plugin_run_test.go +++ b/internal/cli/plugin_run_test.go @@ -20,7 +20,6 @@ import ( "strconv" "strings" "sync" - "syscall" "testing" "time" @@ -68,7 +67,7 @@ type fixtureReportDoc struct { // --code=, --path= and --marker=. Everything else is only reported, which is how // a test sees the arguments that the CLI passed on. func runPluginFixture(args []string) (code int, ok bool) { - if strings.TrimSuffix(filepath.Base(args[0]), ".exe") != pluginTestBin { + if filepath.Base(args[0]) != registrytest.BinName(plugins.CurrentPlatform(), pluginTestBin) { return 0, false } @@ -372,11 +371,12 @@ func (r *runEnv) releaseFixture(version string, commands []plugins.Command) regi data, err := json.Marshal(manifest) require.NoError(r.t, err) - archive := fixtureArchive(r.t, pluginTestBin, data) + bin := registrytest.BinName(r.platform, pluginTestBin) + archive := fixtureArchive(r.t, bin, data) path := "/auth0/auth0-cli-plugins/releases/download/v" + version + "/" + pluginTestBin + ".zip" record.Artifacts = []registry.Artifact{ - registrytest.NewArtifact(r.platform, "https://github.com"+path, registry.ArtifactFormatZip, pluginTestBin, archive), + registrytest.NewArtifact(r.platform, "https://github.com"+path, registry.ArtifactFormatZip, bin, archive), } r.server.Set(path, archive) @@ -495,16 +495,6 @@ func waitForMarker(t *testing.T, path string) int { return pid } -// processAlive reports whether a process with the ID exists. -func processAlive(pid int) bool { - process, err := os.FindProcess(pid) - if err != nil { - return false - } - - return process.Signal(syscall.Signal(0)) == nil -} - func TestRegisterPluginCommands_Gate(t *testing.T) { e := newPluginEnv(t) e.publish(e.index(e.release("1.0.0"))) @@ -1005,6 +995,16 @@ func TestPluginRun_Environment(t *testing.T) { t.Setenv("LANG", "en_US.UTF-8") allowed := func(name string) bool { + // Windows names are not case sensitive, and a plugin gets the variables that + // it needs to find its way around the system. + if runtime.GOOS == "windows" { + for _, system := range []string{"SystemRoot", "TEMP", "TMP", "USERPROFILE", "APPDATA", "LOCALAPPDATA", "PATHEXT", "ComSpec"} { + if strings.EqualFold(name, system) { + return true + } + } + } + switch name { case "PATH", "HOME", "LANG", "LC_ALL", "LC_CTYPE", "TERM", "TMPDIR", "HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", "http_proxy", "https_proxy", "no_proxy", @@ -1627,9 +1627,7 @@ func TestPluginRun_Results(t *testing.T) { assert.Less(t, time.Since(started), 30*time.Second) assert.JSONEq(t, `{"pluginId":"auth0/checkmate","version":"1.0.0"}`, string(buildErrorEnvelope(err).Error.Details)) - if runtime.GOOS != "windows" { - assert.False(t, processAlive(waitForMarker(t, marker)), "the plugin must be gone") - } + assert.False(t, processAlive(waitForMarker(t, marker)), "the plugin must be gone") }) t.Run("the timeout comes from the registry when the command has none", func(t *testing.T) { @@ -1666,10 +1664,6 @@ func TestPluginRun_Results(t *testing.T) { } func TestPluginRun_Interrupt(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("the process tree is stopped differently on Windows") - } - r := newRunEnv(t, fixtureCommands()) r.approve() @@ -1701,35 +1695,6 @@ func TestPluginRun_Interrupt(t *testing.T) { assert.False(t, processAlive(pid), "the plugin must be gone") } -func TestPluginRun_Hangup(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("the process tree is stopped differently on Windows") - } - - r := newRunEnv(t, fixtureCommands()) - r.approve() - - marker := filepath.Join(t.TempDir(), "pid") - done := make(chan error, 1) - - go func() { done <- r.exec("checkmate", "--mode=sleep", "--marker="+marker) }() - - pid := waitForMarker(t, marker) - - // A terminal that is closed sends SIGHUP to the CLI. The run has taken the - // signal by now, as the plugin only starts after that. - require.NoError(t, syscall.Kill(os.Getpid(), syscall.SIGHUP)) - - err := <-done - - require.Error(t, err) - - class, reason := reasonOf(err) - assert.Equal(t, "plugin", class) - assert.Equal(t, "plugin_interrupted", reason) - assert.False(t, processAlive(pid), "the plugin must be gone") -} - func TestPluginRun_AmbiguousSubcommand(t *testing.T) { // The root command declares the boolean flag mode, so that cobra does not take // the word after it as its value. @@ -1945,16 +1910,35 @@ func TestPluginEnvironment_LoopbackIsNotProxied(t *testing.T) { r.approve() t.Setenv("NO_PROXY", "internal.example.com") - t.Setenv("no_proxy", "") t.Setenv("HTTPS_PROXY", "http://proxy.example.com:3128") + // On Windows this is the same variable as NO_PROXY, which would be cleared. + if runtime.GOOS != "windows" { + t.Setenv("no_proxy", "") + } + require.NoError(t, r.exec("checkmate")) env := r.report(t).Env - assert.Equal(t, "internal.example.com,127.0.0.1,localhost,::1", env["NO_PROXY"]) - assert.Equal(t, "127.0.0.1,localhost,::1", env["no_proxy"]) - assert.Equal(t, "http://proxy.example.com:3128", env["HTTPS_PROXY"], "the proxy of the user is kept") + // On Windows a name has one spelling in the environment of the plugin, and it is + // not necessarily the one that was set. + lookup := func(name string) string { + for key, value := range env { + if key == name || (runtime.GOOS == "windows" && strings.EqualFold(key, name)) { + return value + } + } + + return "" + } + + assert.Equal(t, "internal.example.com,127.0.0.1,localhost,::1", lookup("NO_PROXY")) + assert.Equal(t, "http://proxy.example.com:3128", lookup("HTTPS_PROXY"), "the proxy of the user is kept") + + if runtime.GOOS != "windows" { + assert.Equal(t, "127.0.0.1,localhost,::1", env["no_proxy"]) + } } func TestRenderNamespaceJSONHelp_LeavesPluginCommandsAlone(t *testing.T) { diff --git a/internal/cli/plugin_run_unix_test.go b/internal/cli/plugin_run_unix_test.go new file mode 100644 index 000000000..9b5b52dc8 --- /dev/null +++ b/internal/cli/plugin_run_unix_test.go @@ -0,0 +1,48 @@ +//go:build !windows + +package cli + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// processAlive reports whether a process with the ID exists. +func processAlive(pid int) bool { + process, err := os.FindProcess(pid) + if err != nil { + return false + } + + return process.Signal(syscall.Signal(0)) == nil +} + +func TestPluginRun_Hangup(t *testing.T) { + r := newRunEnv(t, fixtureCommands()) + r.approve() + + marker := filepath.Join(t.TempDir(), "pid") + done := make(chan error, 1) + + go func() { done <- r.exec("checkmate", "--mode=sleep", "--marker="+marker) }() + + pid := waitForMarker(t, marker) + + // A terminal that is closed sends SIGHUP to the CLI. The run has taken the + // signal by now, as the plugin only starts after that. + require.NoError(t, syscall.Kill(os.Getpid(), syscall.SIGHUP)) + + err := <-done + + require.Error(t, err) + + class, reason := reasonOf(err) + assert.Equal(t, "plugin", class) + assert.Equal(t, "plugin_interrupted", reason) + assert.False(t, processAlive(pid), "the plugin must be gone") +} diff --git a/internal/cli/plugin_run_windows_test.go b/internal/cli/plugin_run_windows_test.go new file mode 100644 index 000000000..1efa268a1 --- /dev/null +++ b/internal/cli/plugin_run_windows_test.go @@ -0,0 +1,29 @@ +package cli + +import ( + "golang.org/x/sys/windows" +) + +// stillActive is the exit code that GetExitCodeProcess reports for a process +// that has not ended. +const stillActive = 259 + +// processAlive reports whether a process with the ID is still running. A process +// that has ended can stay in the table while something holds a handle to it, so +// the exit code decides, not whether the process can be opened. +func processAlive(pid int) bool { + handle, err := windows.OpenProcess(windows.PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(pid)) + if err != nil { + return false + } + + defer func() { _ = windows.CloseHandle(handle) }() + + var code uint32 + + if err := windows.GetExitCodeProcess(handle, &code); err != nil { + return false + } + + return code == stillActive +} diff --git a/internal/cli/plugin_test.go b/internal/cli/plugin_test.go index 8b33d984b..7820a0ceb 100644 --- a/internal/cli/plugin_test.go +++ b/internal/cli/plugin_test.go @@ -218,6 +218,7 @@ func (e *pluginEnv) releaseAs(id string, platform plugins.Platform, version stri e.t.Helper() name := plugins.NameOf(id) + bin := registrytest.BinName(platform, name) record := registrytest.NewVersion(version) record.Platforms = []plugins.Platform{platform} @@ -241,20 +242,20 @@ func (e *pluginEnv) releaseAs(id string, platform plugins.Platform, version stri } binary = append(binary, '\n') - archive := registrytest.Package(id, record, registry.ArtifactFormatTarGz, name, binary) + archive := registrytest.Package(id, record, registry.ArtifactFormatTarGz, bin, binary) if e.ManifestDescription != "" { description, err := json.Marshal(e.ManifestDescription) require.NoError(e.t, err) manifest := bytes.Replace(registrytest.Manifest(id, record), []byte(`"description":"Test plugin `+id+`"`), append([]byte(`"description":`), description...), 1) - archive = registrytest.TarGz(registrytest.ArchiveEntry{Name: name, Body: binary}, registrytest.ArchiveEntry{Name: "plugin.json", Body: manifest}) + archive = registrytest.TarGz(registrytest.ArchiveEntry{Name: bin, Body: binary}, registrytest.ArchiveEntry{Name: "plugin.json", Body: manifest}) } path := pluginArtifactPath(name, version) record.Artifacts = []registry.Artifact{ - registrytest.NewArtifact(platform, "https://github.com"+path, registry.ArtifactFormatTarGz, name, archive), + registrytest.NewArtifact(platform, "https://github.com"+path, registry.ArtifactFormatTarGz, bin, archive), } e.server.Set(path, archive) @@ -850,7 +851,7 @@ func TestInfoPluginCmd(t *testing.T) { e.publish(e.index(e.release("1.0.0"))) e.install("1.0.0") - binary := filepath.Join(e.Dir, "store", "auth0", "checkmate", "1.0.0", pluginTestBin) + binary := filepath.Join(e.Dir, "store", "auth0", "checkmate", "1.0.0", registrytest.BinName(e.platform, pluginTestBin)) require.NoError(t, os.WriteFile(binary, []byte("tampered"), 0o700)) require.NoError(t, e.run("plugin", "info", "checkmate")) @@ -2411,7 +2412,7 @@ func TestApprovePluginCmd(t *testing.T) { require.EqualError(t, err, "the plugin auth0/nothing is not installed. Installed plugins: auth0/checkmate") assertValidationError(t, err, "plugin_not_installed") - require.NoError(t, os.WriteFile(filepath.Join(e.Dir, "store", "auth0", "checkmate", "1.0.0", pluginTestBin), []byte("tampered"), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(e.Dir, "store", "auth0", "checkmate", "1.0.0", registrytest.BinName(e.platform, pluginTestBin)), []byte("tampered"), 0o700)) err = e.run("plugin", "approve", "checkmate", "--tenant", pluginTestTenant) diff --git a/internal/plugins/approvals/file.go b/internal/plugins/approvals/file.go index 51d780bc3..5513ada0a 100644 --- a/internal/plugins/approvals/file.go +++ b/internal/plugins/approvals/file.go @@ -49,7 +49,7 @@ var ( ) // renameFile replaces a file, which tests replace to make it fail. -var renameFile = os.Rename +var renameFile = plugins.Rename // approval is what is stored for one plugin on one tenant. type approval struct { diff --git a/internal/plugins/approvals/internal_test.go b/internal/plugins/approvals/internal_test.go index fc7ecf489..e2ad54617 100644 --- a/internal/plugins/approvals/internal_test.go +++ b/internal/plugins/approvals/internal_test.go @@ -30,7 +30,7 @@ func TestFailedRenameKeepsTheOldFile(t *testing.T) { renameFile = func(string, string) error { return errors.New("disk is read only") } - t.Cleanup(func() { renameFile = os.Rename }) + t.Cleanup(func() { renameFile = plugins.Rename }) require.ErrorContains(t, a.Approve(t.Context(), tenant, "auth0/checkmate", hashB), "disk is read only") diff --git a/internal/plugins/manifest_test.go b/internal/plugins/manifest_test.go index 319e97038..ef5911700 100644 --- a/internal/plugins/manifest_test.go +++ b/internal/plugins/manifest_test.go @@ -1,6 +1,7 @@ package plugins import ( + "bytes" "encoding/json" "errors" "os" @@ -25,7 +26,9 @@ func readValidManifest(t *testing.T) []byte { data, err := os.ReadFile("testdata/valid-manifest.json") require.NoError(t, err) - return data + // A checkout on Windows can turn the line breaks into CRLF, and the tests that + // edit the text look for the LF. + return bytes.ReplaceAll(data, []byte("\r\n"), []byte("\n")) } // mutateManifest applies fn to the valid manifest and returns the new JSON. diff --git a/internal/plugins/registry/cache.go b/internal/plugins/registry/cache.go index c06b9d1a9..97d6c5084 100644 --- a/internal/plugins/registry/cache.go +++ b/internal/plugins/registry/cache.go @@ -102,7 +102,7 @@ type previousFile struct { // replace to make them fail. var ( stageFile = plugins.StageTempFile - renameFile = os.Rename + renameFile = plugins.Rename ) // verified is what a refresh accepted, and persist stores. diff --git a/internal/plugins/registry/index.go b/internal/plugins/registry/index.go index 83907099d..a2adf897b 100644 --- a/internal/plugins/registry/index.go +++ b/internal/plugins/registry/index.go @@ -518,8 +518,9 @@ func checkVersions(path string, versions []VersionRecord) plugins.ProblemList { } // checkArtifacts checks that every artifact of a version record is for a -// supported platform that the record lists, and that no platform has two -// artifacts. The record is at recordPath. +// supported platform that the record lists, that no platform has two artifacts, +// and that the executable of a Windows artifact is an .exe file. The record is +// at recordPath. func checkArtifacts(recordPath string, record VersionRecord) plugins.ProblemList { var found plugins.ProblemList @@ -539,6 +540,13 @@ func checkArtifacts(recordPath string, record VersionRecord) plugins.ProblemList found.Addf("%s: duplicate artifact for platform %q, already used by %s", artifactPath, platform, first) } + // A script cannot be a plugin: Windows runs .bat and .cmd files through the + // command interpreter, which does not read the arguments the way a program + // does. + if ok && platform.OS() == "windows" && !strings.HasSuffix(strings.ToLower(artifact.Bin), ".exe") { + found.Addf("%s.bin: %q must end in .exe, because a plugin on Windows is a program and not a .bat or .cmd script", artifactPath, artifact.Bin) + } + artifactPaths[platform] = artifactPath } diff --git a/internal/plugins/registry/index_test.go b/internal/plugins/registry/index_test.go index d471398e5..8ee16facb 100644 --- a/internal/plugins/registry/index_test.go +++ b/internal/plugins/registry/index_test.go @@ -684,6 +684,43 @@ func TestParseIndex_SemanticFailures(t *testing.T) { } } +func TestParseIndex_WindowsExecutable(t *testing.T) { + tests := []struct { + name string + platform plugins.Platform + bin string + + // The expected error, which is empty when the index is valid. + wantErr string + }{ + {name: "an exe", platform: plugins.PlatformWindowsX64, bin: "plugin.exe"}, + {name: "an exe in upper case", platform: plugins.PlatformWindowsX64, bin: "PLUGIN.EXE"}, + {name: "a name that holds another extension before the exe", platform: plugins.PlatformWindowsX64, bin: "plugin.cmd.exe"}, + {name: "a name without an extension", platform: plugins.PlatformWindowsX64, bin: "plugin", wantErr: `plugins[0].versions[0].artifacts[2].bin: "plugin" must end in .exe`}, + {name: "a bat script", platform: plugins.PlatformWindowsX64, bin: "plugin.bat", wantErr: `plugins[0].versions[0].artifacts[2].bin: "plugin.bat" must end in .exe`}, + {name: "a cmd script in upper case", platform: plugins.PlatformWindowsX64, bin: "PLUGIN.CMD", wantErr: `plugins[0].versions[0].artifacts[2].bin: "PLUGIN.CMD" must end in .exe`}, + {name: "a script name on a platform that is not Windows", platform: plugins.PlatformLinuxArm64, bin: "plugin.cmd"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + record := registrytest.NewVersion("1.0.0") + record.Platforms = append(record.Platforms, test.platform) + record.Artifacts = append(record.Artifacts, registrytest.NewArtifact(test.platform, "https://downloads.example.com/1.0.0/plugin.zip", registry.ArtifactFormatZip, test.bin, []byte("archive"))) + + _, err := registry.ParseIndex(registrytest.NewIndexBuilder().AddPlugin(registrytest.NewPlugin(testPluginID, record)).Build()) + + if test.wantErr == "" { + require.NoError(t, err) + + return + } + + requireInvalid(t, err, test.wantErr) + }) + } +} + func TestParseIndex_DuplicateKeys(t *testing.T) { // The helper "after" adds a key right after the first time the marker // appears, which makes the object that the marker opens have that key twice. diff --git a/internal/plugins/registry/registrytest/artifacts.go b/internal/plugins/registry/registrytest/artifacts.go index 0b991742f..5c40ab123 100644 --- a/internal/plugins/registry/registrytest/artifacts.go +++ b/internal/plugins/registry/registrytest/artifacts.go @@ -174,6 +174,17 @@ func NewArtifact(platform plugins.Platform, url string, format registry.Artifact } } +// BinName returns the file name of the executable of the plugin with the given +// short name on the platform. The registry requires one for Windows to end in +// .exe. +func BinName(platform plugins.Platform, name string) string { + if platform.OS() == "windows" { + return name + ".exe" + } + + return name +} + // Package returns an archive of the given format that holds the binary and the // manifest of the record, under the names bin and plugin.json. func Package(id string, record registry.VersionRecord, format registry.ArtifactFormat, bin string, binary []byte) []byte { diff --git a/internal/plugins/registry/registrytest/artifacts_test.go b/internal/plugins/registry/registrytest/artifacts_test.go new file mode 100644 index 000000000..a487e11d6 --- /dev/null +++ b/internal/plugins/registry/registrytest/artifacts_test.go @@ -0,0 +1,26 @@ +package registrytest + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/auth0/auth0-cli/internal/plugins" +) + +func TestBinName(t *testing.T) { + tests := []struct { + platform plugins.Platform + want string + }{ + {plugins.PlatformWindowsX64, "checkmate.exe"}, + {plugins.PlatformLinuxX64, "checkmate"}, + {plugins.PlatformDarwinArm64, "checkmate"}, + } + + for _, test := range tests { + t.Run(string(test.platform), func(t *testing.T) { + assert.Equal(t, test.want, BinName(test.platform, "checkmate")) + }) + } +} diff --git a/internal/plugins/rename.go b/internal/plugins/rename.go new file mode 100644 index 000000000..244b99aca --- /dev/null +++ b/internal/plugins/rename.go @@ -0,0 +1,57 @@ +package plugins + +import ( + "errors" + "syscall" + "time" +) + +// The Win32 error codes that a rename reports while another process holds the +// file or directory open without sharing it for deletion. They are written as +// numbers because the windows package only builds on Windows, and the retry +// logic is tested everywhere. On Windows they are the same values as +// windows.ERROR_ACCESS_DENIED, ERROR_SHARING_VIOLATION and ERROR_LOCK_VIOLATION. +const ( + errorAccessDenied syscall.Errno = 5 + errorSharingViolation syscall.Errno = 32 + errorLockViolation syscall.Errno = 33 +) + +// The retry policy of Rename: at most renameTries attempts, with a wait after +// each failure that starts at renameFirstDelay, doubles, and stops growing at +// renameMaxDelay. The waits add up to about 1.3 seconds. +const ( + renameTries = 10 + renameFirstDelay = 20 * time.Millisecond + renameMaxDelay = 200 * time.Millisecond +) + +// isRetryable reports whether err is one that a later attempt at the same rename +// can get past, because a reader that has the file open closes it soon. +func isRetryable(err error) bool { + return errors.Is(err, errorAccessDenied) || + errors.Is(err, errorSharingViolation) || + errors.Is(err, errorLockViolation) +} + +// renameDelay returns how long to wait after the failed attempt with the given +// number, which starts at zero. +func renameDelay(attempt int) time.Duration { + // Four doublings already pass the cap, and limiting the shift keeps a large + // attempt number from overflowing. + return min(renameFirstDelay< 0 +} diff --git a/internal/plugins/runner/proc_test.go b/internal/plugins/runner/proc_test.go new file mode 100644 index 000000000..5943e12fd --- /dev/null +++ b/internal/plugins/runner/proc_test.go @@ -0,0 +1,61 @@ +package runner + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestMapExitCode(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + code uint32 + want int + }{ + {"success", 0, 0}, + {"Ctrl-C is 128 plus SIGINT", 0xC000013A, 130}, + {"an ordinary failure is kept", 7, 7}, + {"the largest code that fits in an int on every system is kept", 0x7FFFFFFF, 0x7FFFFFFF}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, test.want, mapExitCode(test.code)) + }) + } +} + +func TestMapExitCode_OnlyCtrlCIsMapped(t *testing.T) { + t.Parallel() + + assert.NotEqual(t, 130, mapExitCode(controlCExit-1)) + assert.NotEqual(t, 130, mapExitCode(controlCExit+1)) +} + +func TestJobLingering(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + active uint32 + err error + want bool + }{ + {"nothing is left", 0, nil, false}, + {"one process is left", 1, nil, true}, + {"a query that failed counts as lingering", 0, errors.New("failed"), true}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, test.want, jobLingering(test.active, test.err)) + }) + } +} diff --git a/internal/plugins/runner/proc_unix.go b/internal/plugins/runner/proc_unix.go index 43152bf5f..adf4d4f7e 100644 --- a/internal/plugins/runner/proc_unix.go +++ b/internal/plugins/runner/proc_unix.go @@ -9,52 +9,69 @@ import ( "syscall" ) +// tree is what the runner needs to stop the processes of one run. On Unix that +// is the plugin and, when it is not interactive, its process group. +type tree struct { + cmd *exec.Cmd + interactive bool +} + // prepare puts a plugin that is not interactive in a process group of its own, // which is what lets the runner stop everything it starts. An interactive plugin // stays in the group of the CLI, because a background group stops on its first // read from the terminal. -func prepare(cmd *exec.Cmd, interactive bool) { +func prepare(cmd *exec.Cmd, interactive bool) *tree { if !interactive { cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} } + + return &tree{cmd: cmd, interactive: interactive} +} + +// started is called after the plugin has started. Nothing is needed on Unix. +func (*tree) started() error { + return nil } // terminate asks the plugin, and its group when it has one, to stop. -func terminate(cmd *exec.Cmd, interactive bool) { - send(cmd, interactive, syscall.SIGTERM) +func (t *tree) terminate() { + t.send(syscall.SIGTERM) } // kill ends the plugin, and its group when it has one, without a chance to clean // up. -func kill(cmd *exec.Cmd, interactive bool) { - send(cmd, interactive, syscall.SIGKILL) +func (t *tree) kill() { + t.send(syscall.SIGKILL) } // send delivers a signal to the group of the plugin, or to the plugin alone when // it is interactive. A process that is already gone is not an error. -func send(cmd *exec.Cmd, interactive bool, signal syscall.Signal) { - if interactive { - _ = cmd.Process.Signal(signal) +func (t *tree) send(signal syscall.Signal) { + if t.interactive { + _ = t.cmd.Process.Signal(signal) return } - _ = syscall.Kill(-cmd.Process.Pid, signal) + _ = syscall.Kill(-t.cmd.Process.Pid, signal) } // lingering reports whether the group of the plugin still has a process in it, // after the plugin itself has gone. An interactive plugin has no group of its // own, so nothing is left to wait for. -func lingering(cmd *exec.Cmd, interactive bool) bool { - if interactive { +func (t *tree) lingering() bool { + if t.interactive { return false } - err := syscall.Kill(-cmd.Process.Pid, 0) + err := syscall.Kill(-t.cmd.Process.Pid, 0) return err == nil || errors.Is(err, syscall.EPERM) } +// release is called when the run is over. There is nothing to free on Unix. +func (*tree) release(bool) {} + // exitCode returns the exit code of a finished process, or 128 plus the number // of the signal that ended it. func exitCode(state *os.ProcessState) int { diff --git a/internal/plugins/runner/proc_unix_test.go b/internal/plugins/runner/proc_unix_test.go index 0e9561ed2..f1eb8ce87 100644 --- a/internal/plugins/runner/proc_unix_test.go +++ b/internal/plugins/runner/proc_unix_test.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "os" + "os/signal" "path/filepath" "strconv" "strings" @@ -29,6 +30,11 @@ func init() { } } +// ignoreStop makes the process ignore the request to stop. +func ignoreStop() { + signal.Ignore(syscall.SIGTERM) +} + func TestRun_ProcessGroup(t *testing.T) { t.Parallel() diff --git a/internal/plugins/runner/proc_windows.go b/internal/plugins/runner/proc_windows.go index 016664c3a..ee5e3f68f 100644 --- a/internal/plugins/runner/proc_windows.go +++ b/internal/plugins/runner/proc_windows.go @@ -3,34 +3,200 @@ package runner import ( + "fmt" "os" "os/exec" + "syscall" + "unsafe" + + "golang.org/x/sys/windows" ) -// TODO(DXCDT-2610): Job Objects. Until then a plugin on Windows runs as a plain -// child: a timeout or Ctrl-C ends the plugin process only, and anything it -// started can outlive it. +// ntResumeProcess is the call that resumes every thread of a process. Go closes +// the handle of the first thread right after it creates a process, so a process +// that starts suspended cannot be resumed with ResumeThread. +var ntResumeProcess = windows.NewLazySystemDLL("ntdll.dll").NewProc("NtResumeProcess") + +// jobAccounting is JOBOBJECT_BASIC_ACCOUNTING_INFORMATION, which the windows +// package does not define. +type jobAccounting struct { + TotalUserTime int64 + TotalKernelTime int64 + ThisPeriodTotalUserTime int64 + ThisPeriodTotalKernelTime int64 + TotalPageFaultCount uint32 + TotalProcesses uint32 + ActiveProcesses uint32 + TotalTerminatedProcesses uint32 +} + +// The structure must be as large as the one the system fills in, 48 bytes. The +// array types are the same only when the length is, so a structure that is too +// large or too small does not compile. +var _ [48]struct{} = [unsafe.Sizeof(jobAccounting{})]struct{}{} -// prepare does nothing on Windows. -func prepare(*exec.Cmd, bool) {} +// tree is what the runner needs to stop the processes of one run. On Windows that +// is a job object that holds the plugin and everything it starts. +type tree struct { + cmd *exec.Cmd + interactive bool -// terminate ends the plugin, because Windows has no signal that asks a process to -// stop. -func terminate(cmd *exec.Cmd, _ bool) { - _ = cmd.Process.Kill() + // The field job is zero until started has created it. Release closes it. + job windows.Handle } -// kill ends the plugin. -func kill(cmd *exec.Cmd, _ bool) { - _ = cmd.Process.Kill() +// prepare makes the plugin start suspended, so that it can be put in its job +// object before it runs any code. A plugin that is not interactive also gets a +// process group of its own, which is what lets the runner send it Ctrl-Break. An +// interactive plugin stays in the console group of the CLI, so Ctrl-C reaches it. +func prepare(cmd *exec.Cmd, interactive bool) *tree { + flags := uint32(windows.CREATE_SUSPENDED) + if !interactive { + flags |= windows.CREATE_NEW_PROCESS_GROUP + } + + cmd.SysProcAttr = &syscall.SysProcAttr{CreationFlags: flags} + + return &tree{cmd: cmd, interactive: interactive} } -// lingering is always false on Windows, where only the plugin itself is tracked. -func lingering(*exec.Cmd, bool) bool { - return false +// started creates the job object, puts the suspended plugin in it, and lets the +// plugin run. Any process the plugin starts from then on is in the job too. +// +// It must be called after the plugin has started and before Wait is called. The +// handle comes from the Process of the command, which stays open until Wait, so +// it cannot refer to another process even if the plugin is killed meanwhile. If +// a step fails the job is still held, and release closes it. +func (t *tree) started() error { + job, err := windows.CreateJobObject(nil, nil) + if err != nil { + return fmt.Errorf("could not create a job object: %w", err) + } + + t.job = job + + // Closing the last handle of the job ends the processes in it. That is what + // stops the plugin when the CLI itself dies, and release lifts it again when + // the plugin exits by itself. + if err := setLimitFlags(job, windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE); err != nil { + return fmt.Errorf("could not set the limits of the job object: %w", err) + } + + var stepErr error + + err = t.cmd.Process.WithHandle(func(handle uintptr) { + process := windows.Handle(handle) + + if err := windows.AssignProcessToJobObject(job, process); err != nil { + stepErr = fmt.Errorf("could not put the plugin in its job object: %w", err) + + return + } + + if err := resume(process); err != nil { + stepErr = fmt.Errorf("could not resume the plugin: %w", err) + } + }) + if err != nil { + return fmt.Errorf("could not get the handle of the plugin: %w", err) + } + + return stepErr +} + +// terminate sends Ctrl-Break to the group of a plugin that is not interactive. +// It is best effort: it fails when the CLI has no console, and the kill after the +// grace period does not depend on it. An interactive plugin is not asked, since +// Ctrl-C has already reached it through the console. +func (t *tree) terminate() { + if t.interactive { + return + } + + _ = windows.GenerateConsoleCtrlEvent(windows.CTRL_BREAK_EVENT, uint32(t.cmd.Process.Pid)) +} + +// kill ends every process in the job at once, with exit code 1. If the job cannot +// be ended it ends the plugin alone. +func (t *tree) kill() { + if err := windows.TerminateJobObject(t.job, 1); err != nil { + // Without the job at least the plugin itself must go, or the run would wait + // for it for ever. + _ = t.cmd.Process.Kill() + } +} + +// lingering reports whether the job still has a process in it, after the plugin +// itself has gone. +func (t *tree) lingering() bool { + var info jobAccounting + + err := queryJobInformation(t.job, windows.JobObjectBasicAccountingInformation, uintptr(unsafe.Pointer(&info)), uint32(unsafe.Sizeof(info))) + + return jobLingering(info.ActiveProcesses, err) +} + +// release closes the handle of the job. When killRest is false the plugin exited +// by itself, and what it left running keeps running, as it does on Unix, so the +// kill on close is lifted first. If that fails the close ends them: the safe way +// to fail. +func (t *tree) release(killRest bool) { + if t.job == 0 { + return + } + + if !killRest { + _ = setLimitFlags(t.job, 0) + } + + _ = windows.CloseHandle(t.job) + t.job = 0 } // exitCode returns the exit code of a finished process. func exitCode(state *os.ProcessState) int { - return state.ExitCode() + return mapExitCode(uint32(state.ExitCode())) +} + +// resume lets every thread of a suspended process run. +func resume(process windows.Handle) error { + if err := ntResumeProcess.Find(); err != nil { + return err + } + + status, _, _ := ntResumeProcess.Call(uintptr(process)) + + // An NTSTATUS is a failure when it is negative. + if int32(status) < 0 { + return fmt.Errorf("NtResumeProcess failed with status %#x", uint32(status)) + } + + return nil +} + +// setLimitFlags sets the limit flags of the job and nothing else. +func setLimitFlags(job windows.Handle, flags uint32) error { + var info windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION + + info.BasicLimitInformation.LimitFlags = flags + + return setJobInformation(job, windows.JobObjectExtendedLimitInformation, uintptr(unsafe.Pointer(&info)), uint32(unsafe.Sizeof(info))) +} + +// setJobInformation calls SetInformationJobObject. The directive keeps what info +// points to alive and in place for the duration of the call, as it does for the +// arguments of syscall.Syscall. +// +//go:uintptrescapes +func setJobInformation(job windows.Handle, class uint32, info uintptr, size uint32) error { + _, err := windows.SetInformationJobObject(job, class, info, size) + + return err +} + +// queryJobInformation calls QueryInformationJobObject, with the same directive. +// +//go:uintptrescapes +func queryJobInformation(job windows.Handle, class int32, info uintptr, size uint32) error { + return windows.QueryInformationJobObject(job, class, info, size, nil) } diff --git a/internal/plugins/runner/proc_windows_test.go b/internal/plugins/runner/proc_windows_test.go new file mode 100644 index 000000000..bc1c2094b --- /dev/null +++ b/internal/plugins/runner/proc_windows_test.go @@ -0,0 +1,122 @@ +package runner + +import ( + "fmt" + "os" + "os/signal" + "path/filepath" + "strconv" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" +) + +func init() { + // Leave starts a process that beats, waits until it is beating, and + // exits by itself. + helperModes["leave"] = func(args []string) { + child := helperChild("beat", args[0]) + + if err := child.Start(); err != nil { + fmt.Fprintln(os.Stderr, err) + exitHelper(3) + } + + for i := 0; i < 6000; i++ { + if _, err := os.Stat(filepath.Join(args[0], "ready")); err == nil { + return + } + + time.Sleep(5 * time.Millisecond) + } + + exitHelper(4) + } + + // Breakaway tries to start a process outside of its job object, and + // writes to /result whether that was refused. + helperModes["breakaway"] = func(args []string) { + child := helperChild("sleep") + child.SysProcAttr = &syscall.SysProcAttr{CreationFlags: windows.CREATE_BREAKAWAY_FROM_JOB} + + result := "refused" + + if err := child.Start(); err == nil { + result = "escaped" + + _ = child.Process.Kill() + } + + _ = os.WriteFile(filepath.Join(args[0], "result"), []byte(result), 0o600) + } +} + +// ignoreStop makes the process ignore Ctrl-Break, which is the request to stop. +// Go ends a process on Ctrl-Break unless a program asks to be told about it. +func ignoreStop() { + signal.Notify(make(chan os.Signal, 1), os.Interrupt) +} + +// terminateBeat ends the process that beats in dir. +func terminateBeat(dir string) { + data, err := os.ReadFile(filepath.Join(dir, "ready")) + if err != nil { + return + } + + pid, err := strconv.Atoi(string(data)) + if err != nil { + return + } + + process, err := windows.OpenProcess(windows.PROCESS_TERMINATE, false, uint32(pid)) + if err != nil { + return + } + + defer windows.CloseHandle(process) + + _ = windows.TerminateProcess(process, 1) +} + +func TestRun_CtrlCIsReportedAs130(t *testing.T) { + t.Parallel() + + err := Run(t.Context(), helper("exit", strconv.FormatUint(controlCExit, 10))) + + var exit *ExitError + require.ErrorAs(t, err, &exit) + assert.Equal(t, 130, exit.Code) +} + +func TestRun_ExitingByItselfLeavesWhatItStarted(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + + // The process is not ours to leave behind once the test is done. + t.Cleanup(func() { terminateBeat(dir) }) + + require.NoError(t, Run(t.Context(), helper("leave", dir))) + + before := beats(t, dir) + + assert.Eventually(t, func() bool { return beats(t, dir) != before }, 5*time.Second, 20*time.Millisecond, "the process did not keep running") +} + +func TestRun_AProcessCannotLeaveTheJob(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + + require.NoError(t, Run(t.Context(), helper("breakaway", dir))) + + result, err := os.ReadFile(filepath.Join(dir, "result")) + require.NoError(t, err) + + assert.Equal(t, "refused", string(result)) +} diff --git a/internal/plugins/runner/runner.go b/internal/plugins/runner/runner.go index 6fc74afe8..7670cc4ba 100644 --- a/internal/plugins/runner/runner.go +++ b/internal/plugins/runner/runner.go @@ -55,8 +55,10 @@ type Spec struct { Stderr io.Writer // Interactive is true for a command that talks to the user. It stays in the - // process group of the CLI, so the terminal keeps working, and on a timeout - // only the plugin process itself is stopped. + // process group of the CLI (on Windows, the console group), so the terminal + // keeps working. On Unix a timeout then stops only the plugin process itself; + // on Windows the whole tree is still stopped, because it is tracked by a job + // object. Interactive bool // The field terminal replaces the check whether Stdin is a terminal, for tests. @@ -101,10 +103,24 @@ func (e *InterruptedError) Error() string { // // When the timeout or ctx ends the run, the plugin and every process it started // are asked to stop, and killed if they are still there after the grace period. +// // On Unix a plugin that is not interactive is in a process group of its own, so // that covers the whole tree. The kill goes to the group by its number, so a // process that left the group is not reached, and in the instant between the // plugin ending and the group being checked the number could in theory be reused. +// +// On Windows the plugin starts suspended and is put in a job object of its own +// before it runs, so everything it starts belongs to the job, and the kill goes +// to the job itself, which no process number can mix up. A process can leave the +// job only if the job allows it, and this one does not. The request to stop is a +// Ctrl-Break sent to the group of a plugin that is not interactive; it is best +// effort, because it needs a console, and it goes by process number. An +// interactive plugin is not asked at all, since Ctrl-C has already reached it +// through the console, and it is killed after the grace period. +// +// When the plugin exits by itself, what it left running is not stopped, on every +// system. If the plugin cannot be put in its job object or resumed, it is killed +// and Run returns an error before the plugin has executed any code. func Run(ctx context.Context, spec Spec) error { if spec.Bin == "" || filepath.Base(spec.Bin) == spec.Bin { return fmt.Errorf("the plugin executable %q is not a path", spec.Bin) @@ -134,12 +150,27 @@ func Run(ctx context.Context, spec Spec) error { cmd.Stdin = spec.Stdin } - prepare(cmd, spec.Interactive) + procs := prepare(cmd, spec.Interactive) if err := cmd.Start(); err != nil { return fmt.Errorf("could not start the plugin: %w", err) } + // Release runs on every path from here on. It kills what is left of the tree + // unless the plugin exited by itself. + killRest := true + + defer func() { procs.release(killRest) }() + + if err := procs.started(); err != nil { + // Fail closed: the plugin must not run outside of its tree. Wait is called + // so the process is reaped and its handle closed. + _ = cmd.Process.Kill() + _ = cmd.Wait() + + return fmt.Errorf("could not start the plugin: %w", err) + } + // The result of Wait is stored before done is closed, so it is read after done. var waitErr error @@ -162,13 +193,15 @@ func Run(ctx context.Context, spec Spec) error { select { case <-done: + killRest = false + return result(waitErr) case <-expired: - stop(cmd, spec.Interactive, done) + stop(procs, done) return &TimeoutError{After: spec.Timeout} case <-ctx.Done(): - stop(cmd, spec.Interactive, done) + stop(procs, done) return &InterruptedError{} } @@ -195,8 +228,8 @@ func result(err error) error { // stop asks the plugin to stop, and kills it when it and what it started are not // gone when the grace period ends. It returns once the plugin has been waited // for. -func stop(cmd *exec.Cmd, interactive bool, done <-chan struct{}) { - terminate(cmd, interactive) +func stop(procs *tree, done <-chan struct{}) { + procs.terminate() deadline := time.NewTimer(grace) defer deadline.Stop() @@ -207,7 +240,7 @@ func stop(cmd *exec.Cmd, interactive bool, done <-chan struct{}) { exited := false for { - if exited && !lingering(cmd, interactive) { + if exited && !procs.lingering() { return } @@ -216,7 +249,7 @@ func stop(cmd *exec.Cmd, interactive bool, done <-chan struct{}) { exited = true done = nil case <-deadline.C: - kill(cmd, interactive) + procs.kill() if !exited { <-done diff --git a/internal/plugins/runner/runner_test.go b/internal/plugins/runner/runner_test.go index 3c3cd0e9f..edff8c76e 100644 --- a/internal/plugins/runner/runner_test.go +++ b/internal/plugins/runner/runner_test.go @@ -9,12 +9,9 @@ import ( "io/fs" "os" "os/exec" - "os/signal" "path/filepath" - "runtime" "strconv" "strings" - "syscall" "testing" "time" @@ -60,7 +57,7 @@ var helperModes = map[string]func(args []string){ // while, in case a test leaves it behind. "beat": func(args []string) { if len(args) > 1 && args[1] == "ignore-term" { - signal.Ignore(syscall.SIGTERM) + ignoreStop() } _ = os.WriteFile(filepath.Join(args[0], "beat"), []byte("0"), 0o600) @@ -78,8 +75,7 @@ var helperModes = map[string]func(args []string){ // Spawn [ignore-term] starts a grandchild that beats, and waits to be // stopped itself. "spawn": func(args []string) { - child := exec.Command(executable(), append([]string{"beat"}, args...)...) - child.Env = []string{"GO_WANT_HELPER=1"} + child := helperChild(append([]string{"beat"}, args...)...) if err := child.Start(); err != nil { fmt.Fprintln(os.Stderr, err) @@ -92,8 +88,7 @@ var helperModes = map[string]func(args []string){ // Daemon exits at once and leaves a process behind that keeps its output // open. "daemon": func([]string) { - child := exec.Command(executable(), "pause") - child.Env = []string{"GO_WANT_HELPER=1"} + child := helperChild("pause") child.Stdout = os.Stdout child.Stderr = os.Stderr @@ -103,6 +98,15 @@ var helperModes = map[string]func(args []string){ }, } +// helperChild returns a command that runs a mode of the test binary with a +// minimal environment, for a helper that starts a process of its own. +func helperChild(args ...string) *exec.Cmd { + child := exec.Command(executable(), args...) + child.Env = []string{"GO_WANT_HELPER=1"} + + return child +} + func executable() string { path, err := os.Executable() if err != nil { @@ -169,6 +173,15 @@ func requireStopped(t *testing.T, dir string) { assert.Equal(t, before, beats(t, dir), "the process is still running") } +// requireStoppedByRun fails when Run took about as long as the helper processes +// live. Without it a tree that was never stopped passes requireStopped, because +// Run only returns when the helpers have ended on their own. +func requireStoppedByRun(t *testing.T, started time.Time) { + t.Helper() + + require.Less(t, time.Since(started), 30*time.Second, "Run did not stop the process tree, it waited for the processes to end") +} + func TestRun_Results(t *testing.T) { t.Parallel() @@ -330,7 +343,8 @@ func TestRun_DoesNotStart(t *testing.T) { err := Run(t.Context(), spec) - require.ErrorIs(t, err, fs.ErrNotExist) + // Windows reports a missing file as a failed lookup of the program. + require.True(t, errors.Is(err, fs.ErrNotExist) || errors.Is(err, exec.ErrNotFound), "got %v", err) var exit *ExitError assert.False(t, errors.As(err, &exit)) @@ -382,10 +396,6 @@ func TestRun_Timeout(t *testing.T) { func TestRun_TimeoutStopsTheWholeTree(t *testing.T) { t.Parallel() - if runtime.GOOS == "windows" { - t.Skip("the process tree is only stopped on Unix until DXCDT-2610") - } - dir := t.TempDir() // The limit is long because a process can take a second to start when the @@ -393,9 +403,12 @@ func TestRun_TimeoutStopsTheWholeTree(t *testing.T) { spec := helper("spawn", dir) spec.Timeout = 3 * time.Second + started := time.Now() + var timeout *TimeoutError require.ErrorAs(t, Run(t.Context(), spec), &timeout) require.FileExists(t, filepath.Join(dir, "ready"), "the grandchild did not start before the timeout") + requireStoppedByRun(t, started) requireStopped(t, dir) } @@ -403,10 +416,6 @@ func TestRun_TimeoutStopsTheWholeTree(t *testing.T) { func TestRun_CancelStopsTheWholeTree(t *testing.T) { t.Parallel() - if runtime.GOOS == "windows" { - t.Skip("the process tree is only stopped on Unix until DXCDT-2610") - } - dir := t.TempDir() ctx, cancel := context.WithCancel(t.Context()) @@ -417,8 +426,11 @@ func TestRun_CancelStopsTheWholeTree(t *testing.T) { cancel() }() + started := time.Now() + var interrupted *InterruptedError require.ErrorAs(t, Run(ctx, helper("spawn", dir)), &interrupted) + requireStoppedByRun(t, started) requireStopped(t, dir) } @@ -426,10 +438,6 @@ func TestRun_CancelStopsTheWholeTree(t *testing.T) { func TestRun_KillsWhatIgnoresTheRequestToStop(t *testing.T) { t.Parallel() - if runtime.GOOS == "windows" { - t.Skip("a request to stop is a kill on Windows") - } - // Both start a process that beats and ignores the request to stop. The first // is the plugin itself, the second one it started. for name, args := range map[string][]string{ diff --git a/internal/plugins/store/extract_test.go b/internal/plugins/store/extract_test.go index e3f08faff..6ca4a85fe 100644 --- a/internal/plugins/store/extract_test.go +++ b/internal/plugins/store/extract_test.go @@ -4,6 +4,7 @@ import ( "archive/tar" "bytes" "compress/gzip" + "runtime" "testing" "github.com/stretchr/testify/assert" @@ -223,6 +224,10 @@ func TestExtract_RefusesFilesThatAreNotArchives(t *testing.T) { } func TestExtract_RefusesAnExecutableNamedLikeTheManifest(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("an index refuses a Windows executable that does not end in .exe, so this name never reaches the store") + } + for _, bin := range []string{"plugin.json", "Plugin.JSON"} { t.Run(bin, func(t *testing.T) { f := newFixture(t) diff --git a/internal/plugins/store/fileinfo_unix.go b/internal/plugins/store/fileinfo_unix.go index 13623662f..cdf565c98 100644 --- a/internal/plugins/store/fileinfo_unix.go +++ b/internal/plugins/store/fileinfo_unix.go @@ -10,8 +10,8 @@ import ( // unsafeFile says why another local account could change the file or directory // after it was checked, or returns an empty string when none could: it is // writable by its group or by everyone, or it is a regular file with another -// hard link that can be written through. -func unsafeFile(info os.FileInfo) string { +// hard link that can be written through. The open file is only used on Windows. +func unsafeFile(_ *os.File, info os.FileInfo) string { if info.Mode().Perm()&0o022 != 0 { return "is writable by other users" } diff --git a/internal/plugins/store/fileinfo_windows.go b/internal/plugins/store/fileinfo_windows.go index e532f23a2..42162bf06 100644 --- a/internal/plugins/store/fileinfo_windows.go +++ b/internal/plugins/store/fileinfo_windows.go @@ -1,9 +1,41 @@ package store -import "os" +import ( + "fmt" + "os" + + "golang.org/x/sys/windows" +) + +// unsafeFile says why another local account could change the file after it was +// checked, or returns an empty string when none could: it is a regular file with +// another hard link that can be written through. A directory is never reported, +// and nothing is read for it. The mode bits of Go are made up on Windows and +// access is decided by ACLs, so there is no group or world write bit to read, +// and the ACLs are not checked. +// +// The file is the open file that info describes. The link count is read from that +// handle, so it is the count of the file that is about to be read and not of +// whatever the path names a moment later. A regular file without an open file +// is refused, because its link count cannot be read. +func unsafeFile(file *os.File, info os.FileInfo) string { + if !info.Mode().IsRegular() { + return "" + } + + if file == nil { + return "has a link count that cannot be read" + } + + var data windows.ByHandleFileInformation + + if err := windows.GetFileInformationByHandle(windows.Handle(file.Fd()), &data); err != nil { + return fmt.Sprintf("has a link count that cannot be read (%v)", err) + } + + if data.NumberOfLinks > 1 { + return "has more than one hard link" + } -// unsafeFile reports nothing on Windows, where the mode bits of Go are made up -// and access is decided by ACLs, so there is no group or world write bit to read. -func unsafeFile(os.FileInfo) string { return "" } diff --git a/internal/plugins/store/fileinfo_windows_test.go b/internal/plugins/store/fileinfo_windows_test.go new file mode 100644 index 000000000..1e5f5c5dc --- /dev/null +++ b/internal/plugins/store/fileinfo_windows_test.go @@ -0,0 +1,60 @@ +package store_test + +import ( + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +// junction makes a directory junction, which any account can do. +func junction(t *testing.T, target, link string) { + t.Helper() + + output, err := exec.Command("cmd", "/c", "mklink", "/J", link, target).CombinedOutput() + + require.NoError(t, err, string(output)) +} + +func TestVerify_RefusesADirectoryThatIsAJunction(t *testing.T) { + t.Run("the version directory", func(t *testing.T) { + f := installed(t) + + moved := f.versionDir("1.0.0") + ".real" + require.NoError(t, os.Rename(f.versionDir("1.0.0"), moved)) + junction(t, moved, f.versionDir("1.0.0")) + + requireRefused(t, f, f.versionDir("1.0.0")+" is not a directory") + }) + + t.Run("the plugin directory", func(t *testing.T) { + f := installed(t) + + pluginDir := filepath.Dir(f.versionDir("1.0.0")) + moved := pluginDir + ".real" + require.NoError(t, os.Rename(pluginDir, moved)) + junction(t, moved, pluginDir) + + requireRefused(t, f, pluginDir+" is not a directory") + }) +} + +// The link count is read from the file that was opened, so it neither fails nor +// is failed by a process that has the file open without sharing it for deletion, +// as a running plugin or a virus scanner does. +func TestVerify_ReadsTheLinkCountOfAFileThatIsOpen(t *testing.T) { + f := installed(t) + + for _, name := range []string{testBin, "plugin.json"} { + holder, err := os.Open(filepath.Join(f.versionDir("1.0.0"), name)) + require.NoError(t, err) + + t.Cleanup(func() { _ = holder.Close() }) + } + + _, err := f.store.Verify(testID) + + require.NoError(t, err) +} diff --git a/internal/plugins/store/helpers_test.go b/internal/plugins/store/helpers_test.go index 22d5dd4a1..00adb6cf5 100644 --- a/internal/plugins/store/helpers_test.go +++ b/internal/plugins/store/helpers_test.go @@ -19,10 +19,10 @@ import ( "github.com/auth0/auth0-cli/internal/plugins/store" ) -const ( - testID = "auth0/checkmate" - testBin = "checkmate" -) +const testID = "auth0/checkmate" + +// testBin is the file name of the executable of the test plugin on this machine. +var testBin = registrytest.BinName(plugins.CurrentPlatform(), "checkmate") // release describes one version of the test plugin that a test publishes. type release struct { @@ -105,7 +105,6 @@ func (f *fixture) release(version string, mutate ...func(*release)) registry.Ver t: f.t, id: testID, version: version, - bin: testBin, format: registry.ArtifactFormatTarGz, binary: []byte("#!/bin/sh\necho checkmate " + version + "\n"), record: registrytest.NewVersion(version), @@ -117,6 +116,12 @@ func (f *fixture) release(version string, mutate ...func(*release)) registry.Ver change(r) } + // A test that does not name the executable gets the one for the platform of + // the artifact. + if r.bin == "" { + r.bin = registrytest.BinName(r.platform, "checkmate") + } + r.record = registrytest.Seal(r.record) var archive []byte @@ -336,15 +341,38 @@ func (f *fixture) filesSize(versions ...string) int64 { return total } -// skipOnWindows skips a test that relies on Unix file modes or symlinks. +// skipOnWindows skips a test that relies on Unix file modes. func skipOnWindows(t *testing.T) { t.Helper() if runtime.GOOS == "windows" { - t.Skip("relies on Unix file modes and symlinks") + t.Skip("relies on Unix file modes") } } +// installed returns a fixture with 1.0.0 installed and active. +func installed(t *testing.T) *fixture { + t.Helper() + + f := newFixture(t) + f.install(f.index(f.release("1.0.0")), "1.0.0") + + return f +} + +// requireRefused checks that Verify refuses the plugin, with a reason that holds +// want and the command that reinstalls it. +func requireRefused(t *testing.T, f *fixture, want string) { + t.Helper() + + _, err := f.store.Verify(testID) + + var verification *store.VerificationError + require.ErrorAs(t, err, &verification) + assert.Contains(t, err.Error(), want) + assert.Contains(t, err.Error(), "Reinstall it with: auth0 plugin install checkmate") +} + // goodEntries returns the two good entries of the test plugin, for a test to // change. func goodEntries(r *release) []registrytest.ArchiveEntry { diff --git a/internal/plugins/store/helpers_unix_test.go b/internal/plugins/store/helpers_unix_test.go new file mode 100644 index 000000000..686d8250e --- /dev/null +++ b/internal/plugins/store/helpers_unix_test.go @@ -0,0 +1,17 @@ +//go:build !windows + +package store_test + +import ( + "os" + "testing" + + "github.com/stretchr/testify/require" +) + +// symlink makes a symbolic link. +func symlink(t *testing.T, target, link string) { + t.Helper() + + require.NoError(t, os.Symlink(target, link)) +} diff --git a/internal/plugins/store/helpers_windows_test.go b/internal/plugins/store/helpers_windows_test.go new file mode 100644 index 000000000..ee1b09439 --- /dev/null +++ b/internal/plugins/store/helpers_windows_test.go @@ -0,0 +1,24 @@ +package store_test + +import ( + "errors" + "os" + "testing" + + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" +) + +// symlink makes a symbolic link, and skips the test on a machine where the +// account may not make one: that takes a privilege that an account without +// developer mode does not have. The CI runner has it. +func symlink(t *testing.T, target, link string) { + t.Helper() + + err := os.Symlink(target, link) + if errors.Is(err, windows.ERROR_PRIVILEGE_NOT_HELD) { + t.Skip("this account may not create symbolic links") + } + + require.NoError(t, err) +} diff --git a/internal/plugins/store/internal_test.go b/internal/plugins/store/internal_test.go index 7134f07ee..d63fcb1a6 100644 --- a/internal/plugins/store/internal_test.go +++ b/internal/plugins/store/internal_test.go @@ -86,6 +86,23 @@ func TestCheckDownloadURL(t *testing.T) { } } +func TestPluginPath(t *testing.T) { + s := &Store{Dir: filepath.Join("home", "plugins")} + + path, err := s.pluginPath("auth0/checkmate") + + require.NoError(t, err) + assert.Equal(t, filepath.Join("home", "plugins", "store", "auth0", "checkmate"), path) + + // Names that only start like a device name are fine. The names that Windows + // reserves are refused by badFileName, which TestBadFileName covers. + for _, id := range []string{"auth0/console", "auth0/com10", "nulled/checkmate"} { + _, err := s.pluginPath(id) + + require.NoError(t, err, id) + } +} + func TestBadFileName(t *testing.T) { good := []string{"plugin", "plugin.exe", "plugin.json", "my-plugin_1.0", "console", "connect.exe", "com10", "com0", "lpt", "lpt10.exe", "au.x", "nulled"} diff --git a/internal/plugins/store/lockfile_test.go b/internal/plugins/store/lockfile_test.go index 572fecf8b..14680edcd 100644 --- a/internal/plugins/store/lockfile_test.go +++ b/internal/plugins/store/lockfile_test.go @@ -83,8 +83,8 @@ func TestLockfile_IsRefusedWhenItIsNotTrustworthy(t *testing.T) { {"a plugin key that is a path and agrees with its fields", strings.Replace(replace(`"auth0/checkmate": {`, `"../checkmate": {`), `"registry": "auth0"`, `"registry": ".."`, 1)}, {"a version that is a path", strings.ReplaceAll(text, `"1.0.0"`, `"../../1.0.0"`)}, {"a version with a suffix", strings.ReplaceAll(text, `"1.0.0"`, `"1.0.0-beta.1"`)}, - {"an executable name that is a path", replace(`"bin": "checkmate"`, `"bin": "../checkmate"`)}, - {"an executable named like the manifest", replace(`"bin": "checkmate"`, `"bin": "plugin.json"`)}, + {"an executable name that is a path", replace(`"bin": "`+testBin+`"`, `"bin": "../checkmate"`)}, + {"an executable named like the manifest", replace(`"bin": "`+testBin+`"`, `"bin": "plugin.json"`)}, {"a hash that is not hex", strings.Replace(text, `"binSha256": "`, `"binSha256": "zz`, 1)}, {"an upper case hash", replace(`"permissionsHash": "sha256:`, `"permissionsHash": "SHA256:`)}, {"a null entry", replace(`"auth0/checkmate": {`, `"auth0/checkmate": null, "auth0/other": {`)}, diff --git a/internal/plugins/store/manage.go b/internal/plugins/store/manage.go index af95711c5..49d283596 100644 --- a/internal/plugins/store/manage.go +++ b/internal/plugins/store/manage.go @@ -122,6 +122,10 @@ func (s *Store) Remove(ctx context.Context, id string) (int64, error) { var problems []error if err := os.RemoveAll(dir); err != nil { + if plugins.IsInUse(err) { + err = fmt.Errorf("the plugin is running or in use, so stop it and delete the directory by hand: %w", err) + } + problems = append(problems, fmt.Errorf("failed to delete %s: %w", dir, err)) } diff --git a/internal/plugins/store/manage_windows_test.go b/internal/plugins/store/manage_windows_test.go new file mode 100644 index 000000000..f1b90e0be --- /dev/null +++ b/internal/plugins/store/manage_windows_test.go @@ -0,0 +1,35 @@ +package store_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A file that is open cannot be deleted on Windows, which is the case for the +// executable of a plugin that is running. Go opens a file for reading in a way +// that does not let it be deleted, as the system does for a running program. +func TestRemove_ExplainsAPluginThatIsInUse(t *testing.T) { + f := installed(t) + + holder, err := os.Open(filepath.Join(f.versionDir("1.0.0"), testBin)) + require.NoError(t, err) + + t.Cleanup(func() { _ = holder.Close() }) + + _, err = f.store.Remove(t.Context(), testID) + + require.Error(t, err) + assert.Contains(t, err.Error(), "checkmate was uninstalled, but not everything was removed") + assert.Contains(t, err.Error(), "failed to delete "+filepath.Dir(f.versionDir("1.0.0"))) + assert.Contains(t, err.Error(), "the plugin is running or in use") + + require.NoError(t, holder.Close()) + + // The plugin is no longer installed, and its files are left for the person to + // delete. + assert.DirExists(t, f.versionDir("1.0.0")) +} diff --git a/internal/plugins/store/resolve_test.go b/internal/plugins/store/resolve_test.go index 50f50d335..e49dc6243 100644 --- a/internal/plugins/store/resolve_test.go +++ b/internal/plugins/store/resolve_test.go @@ -208,7 +208,7 @@ func TestPluginID_MustBeSafeToUseAsAPath(t *testing.T) { for name, operation := range operations { t.Run(name, func(t *testing.T) { - for _, id := range []string{"../escape", "auth0/../../escape", "auth0/Check", "checkmate", ""} { + for _, id := range []string{"../escape", "auth0/../../escape", "auth0/Check", "checkmate", "", "auth0/con", "nul/checkmate"} { f := newFixture(t) assert.ErrorContains(t, operation(f, id), "is not a plugin ID in the form registry/name", id) diff --git a/internal/plugins/store/store.go b/internal/plugins/store/store.go index a3362dd95..61890872c 100644 --- a/internal/plugins/store/store.go +++ b/internal/plugins/store/store.go @@ -23,6 +23,7 @@ import ( "path/filepath" "regexp" "runtime" + "strings" "github.com/auth0/auth0-cli/internal/plugins" "github.com/auth0/auth0-cli/internal/plugins/filelock" @@ -154,7 +155,7 @@ type Result struct { // The functions that the store uses to move files into place, which tests // replace to make them fail. -var renameFile = os.Rename +var renameFile = plugins.Rename // The functions that tests replace to see what the store reads: readDir lists a // version directory when the size of a plugin is added up, and currentPlatform @@ -165,12 +166,20 @@ var ( ) // pluginPath returns where the directory of a plugin is, or an error when the -// ID is not safe to use as a path. +// ID is not safe to use as a path. Both parts of the ID become directory names, +// so neither may be a name that Windows reserves, even on a system where that +// would work: a plugin that installs here can then be installed everywhere. func (s *Store) pluginPath(id string) (string, error) { if !idPattern.MatchString(id) { return "", fmt.Errorf("%q is not a plugin ID in the form registry/name", id) } + for _, part := range strings.Split(id, "/") { + if reason := badFileName(part); reason != "" { + return "", fmt.Errorf("%q is not a plugin ID in the form registry/name: %q cannot be a directory name because %s", id, part, reason) + } + } + return filepath.Join(s.Dir, storeDirName, filepath.FromSlash(id)), nil } diff --git a/internal/plugins/store/verify.go b/internal/plugins/store/verify.go index 79c6b414c..1bab89dc3 100644 --- a/internal/plugins/store/verify.go +++ b/internal/plugins/store/verify.go @@ -46,9 +46,10 @@ type Verified struct { // because another account or process could then rewrite the file after it was // hashed. The plugins directory and the directories between it and the version // directory must be real directories and not links, and not writable by the -// group or by everyone either. The checks of write access and hard links are -// skipped on Windows, which controls access with ACLs. Any difference is a *VerificationError, and nothing falls back to -// unverified files. The whole executable is hashed every time, without a +// group or by everyone either. The check of write access is skipped on Windows, +// which controls access with ACLs, and the hard link check is not. Any +// difference is a *VerificationError, and nothing falls back to unverified +// files. The whole executable is hashed every time, without a // shortcut based on its size or modification time, because those can be set to // anything. // @@ -188,7 +189,9 @@ func (s *Store) verifyVersion(id string, e *entry, version string) (Verified, er return fail("%s is not a directory", path) } - if reason := unsafeFile(info); reason != "" { + // A directory has no open file and needs none: unsafeFile reads nothing + // else for it. + if reason := unsafeFile(nil, info); reason != "" { return fail("%s %s", path, reason) } } @@ -278,7 +281,7 @@ func openRegular(path string) (*os.File, error) { return nil, fmt.Errorf("%s %w", path, errNotRegular) } - if reason := unsafeFile(after); reason != "" { + if reason := unsafeFile(file, after); reason != "" { _ = file.Close() return nil, fmt.Errorf("%s %s", path, reason) diff --git a/internal/plugins/store/verify_test.go b/internal/plugins/store/verify_test.go index e002e6c7f..6ca9c6d92 100644 --- a/internal/plugins/store/verify_test.go +++ b/internal/plugins/store/verify_test.go @@ -23,18 +23,8 @@ func chmod(name string, mode os.FileMode) func(t *testing.T, f *fixture) { } func TestVerify(t *testing.T) { - // Install returns a fixture with 1.0.0 installed and active. - install := func(t *testing.T) *fixture { - t.Helper() - - f := newFixture(t) - f.install(f.index(f.release("1.0.0")), "1.0.0") - - return f - } - t.Run("passes for a clean install and returns the parsed manifest", func(t *testing.T) { - f := install(t) + f := installed(t) verified, err := f.store.Verify(testID) @@ -49,7 +39,7 @@ func TestVerify(t *testing.T) { }) t.Run("fails for a plugin that is not installed", func(t *testing.T) { - f := install(t) + f := installed(t) _, err := f.store.Verify("auth0/other") @@ -70,14 +60,14 @@ func TestVerify(t *testing.T) { content[len(content)/2] ^= 0xff require.NoError(t, os.WriteFile(path, content, 0o755)) - }, "the hash of checkmate changed"}, + }, "the hash of " + testBin + " changed"}, {"a binary with a byte appended", func(t *testing.T, f *fixture) { file, err := os.OpenFile(filepath.Join(f.versionDir("1.0.0"), testBin), os.O_APPEND|os.O_WRONLY, 0) require.NoError(t, err) _, err = file.WriteString("x") require.NoError(t, err) require.NoError(t, file.Close()) - }, "the hash of checkmate changed"}, + }, "the hash of " + testBin + " changed"}, {"a manifest that was modified", func(t *testing.T, f *fixture) { path := filepath.Join(f.versionDir("1.0.0"), "plugin.json") content, err := os.ReadFile(path) @@ -122,38 +112,32 @@ func TestVerify(t *testing.T) { } }) }, "is not valid"}, + {"a binary with another hard link", func(t *testing.T, f *fixture) { + other := filepath.Join(filepath.Dir(f.versionDir("1.0.0")), "linked") + require.NoError(t, os.Link(filepath.Join(f.versionDir("1.0.0"), testBin), other)) + }, "has more than one hard link"}, + {"a manifest with another hard link", func(t *testing.T, f *fixture) { + other := filepath.Join(filepath.Dir(f.versionDir("1.0.0")), "linked") + require.NoError(t, os.Link(filepath.Join(f.versionDir("1.0.0"), "plugin.json"), other)) + }, "has more than one hard link"}, } for _, test := range refused { t.Run("refuses "+test.name, func(t *testing.T) { - f := install(t) + f := installed(t) test.change(t, f) - _, err := f.store.Verify(testID) - - var verification *store.VerificationError - require.ErrorAs(t, err, &verification) - assert.Contains(t, err.Error(), test.want) - assert.Contains(t, err.Error(), "Reinstall it with: auth0 plugin install checkmate") + requireRefused(t, f, test.want) }) } - // These rely on Unix modes and link counts, which Verify does not check on - // Windows. + // These rely on Unix modes, which Verify does not check on Windows. unsafe := []struct { name string change func(t *testing.T, f *fixture) want string }{ - {"a binary with another hard link", func(t *testing.T, f *fixture) { - other := filepath.Join(filepath.Dir(f.versionDir("1.0.0")), "linked") - require.NoError(t, os.Link(filepath.Join(f.versionDir("1.0.0"), testBin), other)) - }, "has more than one hard link"}, - {"a manifest with another hard link", func(t *testing.T, f *fixture) { - other := filepath.Join(filepath.Dir(f.versionDir("1.0.0")), "linked") - require.NoError(t, os.Link(filepath.Join(f.versionDir("1.0.0"), "plugin.json"), other)) - }, "has more than one hard link"}, {"a binary writable by its group", chmod(testBin, 0o775), "is writable by other users"}, {"a manifest writable by everyone", chmod("plugin.json", 0o666), "is writable by other users"}, {"a version directory writable by its group", chmod("", 0o770), "is writable by other users"}, @@ -167,25 +151,18 @@ func TestVerify(t *testing.T) { t.Run("refuses "+test.name, func(t *testing.T) { skipOnWindows(t) - f := install(t) + f := installed(t) test.change(t, f) - _, err := f.store.Verify(testID) - - var verification *store.VerificationError - require.ErrorAs(t, err, &verification) - assert.Contains(t, err.Error(), test.want) - assert.Contains(t, err.Error(), "Reinstall it with: auth0 plugin install checkmate") + requireRefused(t, f, test.want) }) } t.Run("refuses a binary or a manifest that was swapped for a link to identical content", func(t *testing.T) { - skipOnWindows(t) - for _, name := range []string{testBin, "plugin.json"} { t.Run(name, func(t *testing.T) { - f := install(t) + f := installed(t) path := filepath.Join(f.versionDir("1.0.0"), name) content, err := os.ReadFile(path) @@ -194,48 +171,32 @@ func TestVerify(t *testing.T) { elsewhere := filepath.Join(t.TempDir(), name) require.NoError(t, os.WriteFile(elsewhere, content, 0o755)) require.NoError(t, os.Remove(path)) - require.NoError(t, os.Symlink(elsewhere, path)) - - _, err = f.store.Verify(testID) + symlink(t, elsewhere, path) - var verification *store.VerificationError - require.ErrorAs(t, err, &verification) - assert.Contains(t, err.Error(), "is not a regular file") + requireRefused(t, f, "is not a regular file") }) } }) t.Run("refuses a version directory that was swapped for a link", func(t *testing.T) { - skipOnWindows(t) - - f := install(t) + f := installed(t) moved := f.versionDir("1.0.0") + ".real" require.NoError(t, os.Rename(f.versionDir("1.0.0"), moved)) - require.NoError(t, os.Symlink(moved, f.versionDir("1.0.0"))) + symlink(t, moved, f.versionDir("1.0.0")) - _, err := f.store.Verify(testID) - - var verification *store.VerificationError - require.ErrorAs(t, err, &verification) - assert.Contains(t, err.Error(), "is not a directory") + requireRefused(t, f, f.versionDir("1.0.0")+" is not a directory") }) t.Run("refuses a plugin directory that was swapped for a link", func(t *testing.T) { - skipOnWindows(t) - - f := install(t) + f := installed(t) pluginDir := filepath.Dir(f.versionDir("1.0.0")) moved := pluginDir + ".real" require.NoError(t, os.Rename(pluginDir, moved)) - require.NoError(t, os.Symlink(moved, pluginDir)) - - _, err := f.store.Verify(testID) + symlink(t, moved, pluginDir) - var verification *store.VerificationError - require.ErrorAs(t, err, &verification) - assert.Contains(t, err.Error(), pluginDir+" is not a directory") + requireRefused(t, f, pluginDir+" is not a directory") }) t.Run("checks the active version only", func(t *testing.T) { @@ -254,17 +215,8 @@ func TestVerify(t *testing.T) { } func TestManifest(t *testing.T) { - install := func(t *testing.T) *fixture { - t.Helper() - - f := newFixture(t) - f.install(f.index(f.release("1.0.0")), "1.0.0") - - return f - } - t.Run("returns the manifest of the active version", func(t *testing.T) { - f := install(t) + f := installed(t) manifest, err := f.store.Manifest(testID) @@ -287,7 +239,7 @@ func TestManifest(t *testing.T) { }) t.Run("does not hash the executable", func(t *testing.T) { - f := install(t) + f := installed(t) require.NoError(t, os.WriteFile(filepath.Join(f.versionDir("1.0.0"), testBin), []byte("tampered"), 0o755)) @@ -300,7 +252,7 @@ func TestManifest(t *testing.T) { }) t.Run("fails for a plugin that is not installed", func(t *testing.T) { - f := install(t) + f := installed(t) _, err := f.store.Manifest("auth0/other") @@ -309,7 +261,7 @@ func TestManifest(t *testing.T) { }) t.Run("fails for an ID that is not a path-safe plugin ID", func(t *testing.T) { - f := install(t) + f := installed(t) _, err := f.store.Manifest("../auth0/checkmate") @@ -317,7 +269,7 @@ func TestManifest(t *testing.T) { }) t.Run("fails for a damaged lockfile", func(t *testing.T) { - f := install(t) + f := installed(t) require.NoError(t, os.WriteFile(f.lockfilePath(), []byte("{"), 0o600)) @@ -353,7 +305,7 @@ func TestManifest(t *testing.T) { for _, test := range refused { t.Run("refuses "+test.name, func(t *testing.T) { - f := install(t) + f := installed(t) test.change(t, f) @@ -367,9 +319,7 @@ func TestManifest(t *testing.T) { } t.Run("refuses a manifest that was swapped for a link", func(t *testing.T) { - skipOnWindows(t) - - f := install(t) + f := installed(t) path := filepath.Join(f.versionDir("1.0.0"), "plugin.json") content, err := os.ReadFile(path) @@ -378,7 +328,7 @@ func TestManifest(t *testing.T) { elsewhere := filepath.Join(t.TempDir(), "plugin.json") require.NoError(t, os.WriteFile(elsewhere, content, 0o600)) require.NoError(t, os.Remove(path)) - require.NoError(t, os.Symlink(elsewhere, path)) + symlink(t, elsewhere, path) _, err = f.store.Manifest(testID)