diff --git a/changes/41470-python-script-only-followups b/changes/41470-python-script-only-followups new file mode 100644 index 0000000000..3e462c43c5 --- /dev/null +++ b/changes/41470-python-script-only-followups @@ -0,0 +1,6 @@ +- Fixed the "Add software" error for a file whose contents don't match a supported installer format — it no longer says "Couldn't edit software" on an add and no longer implies the file extension is the problem. +- Fixed software installer validation errors reporting the wrong action verb (add vs. edit). +- Allowed `.py` script-only packages to be assigned a `setup_experience_platform` (`darwin` or `linux`), matching `.sh`. +- Added a diagnostic message when an install script can't be run (exit code -1) — e.g. when the interpreter in its shebang is missing on the host — instead of reporting no output. +- Fixed the install rejection message for `.sh`/`.py` script packages to say they can be installed on macOS and Linux hosts, rather than Linux only. +- Fixed `.py` package install scripts being written to the host with a `.sh` extension, which produced misleading Python tracebacks. diff --git a/cmd/fleetctl/integrationtest/gitops/software_test.go b/cmd/fleetctl/integrationtest/gitops/software_test.go index ca713bf931..9f0b5740cc 100644 --- a/cmd/fleetctl/integrationtest/gitops/software_test.go +++ b/cmd/fleetctl/integrationtest/gitops/software_test.go @@ -35,7 +35,7 @@ func TestGitOpsTeamSoftwareInstallers(t *testing.T) { }{ {"testdata/gitops/team_software_installer_not_found.yml", "Please make sure that URLs are reachable from your Fleet server."}, {"testdata/gitops/team_software_installer_install_script_secret.yml", "environment variable \"FLEET_SECRET_NAME\" not set"}, - {"testdata/gitops/team_software_installer_unsupported.yml", "The file should be .pkg, .msi, .exe, .zip, .deb, .rpm, .tar.gz, .sh, .py, .ipa or .ps1."}, + {"testdata/gitops/team_software_installer_unsupported.yml", "The file's content doesn't match a supported installer format. Supported types: .pkg, .msi, .exe, .zip, .deb, .rpm, .tar.gz, .sh, .py, .ipa or .ps1."}, {"testdata/gitops/team_software_installer_too_large.yml", "The maximum file size is 513MiB"}, {"testdata/gitops/team_software_installer_valid.yml", ""}, {"testdata/gitops/team_software_installer_subdir.yml", ""}, @@ -427,7 +427,7 @@ func TestGitOpsNoTeamSoftwareInstallers(t *testing.T) { wantErr string }{ {"testdata/gitops/no_team_software_installer_not_found.yml", "Please make sure that URLs are reachable from your Fleet server."}, - {"testdata/gitops/no_team_software_installer_unsupported.yml", "The file should be .pkg, .msi, .exe, .zip, .deb, .rpm, .tar.gz, .sh, .py, .ipa or .ps1."}, + {"testdata/gitops/no_team_software_installer_unsupported.yml", "The file's content doesn't match a supported installer format. Supported types: .pkg, .msi, .exe, .zip, .deb, .rpm, .tar.gz, .sh, .py, .ipa or .ps1."}, {"testdata/gitops/no_team_software_installer_too_large.yml", "The maximum file size is 513MiB"}, {"testdata/gitops/no_team_software_installer_valid.yml", ""}, {"testdata/gitops/no_team_software_installer_subdir.yml", ""}, diff --git a/ee/server/service/software_installers.go b/ee/server/service/software_installers.go index 96452787f5..d6c7915892 100644 --- a/ee/server/service/software_installers.go +++ b/ee/server/service/software_installers.go @@ -2091,7 +2091,7 @@ func (svc *Service) installSoftwareTitleUsingInstaller(ctx context.Context, host // Allow .sh and .py scripts for any unix-like platform (linux and darwin) if !((ext == ".sh" || ext == ".py") && fleet.IsUnixLike(host.Platform)) { return &fleet.BadRequestError{ - Message: fmt.Sprintf("Package (%s) can be installed only on %s hosts.", ext, requiredPlatform), + Message: fmt.Sprintf("Package (%s) can be installed only on %s hosts.", ext, humanReadableRequiredPlatforms(ext, requiredPlatform)), InternalErr: ctxerr.NewWithData( ctx, "invalid host platform for requested installer", map[string]any{"host_id": host.ID, "team_id": host.TeamID, "title_id": installer.TitleID}, @@ -2414,12 +2414,12 @@ func (svc *Service) addMetadataToSoftwarePayload(ctx context.Context, payload *f if failOnBlankScript { if payload.InstallScript == "" { return "", &fleet.BadRequestError{ - Message: "Couldn't add. Install script is required for .zip packages.", + Message: "Install script is required for .zip packages.", } } if payload.UninstallScript == "" { return "", &fleet.BadRequestError{ - Message: "Couldn't add. Uninstall script is required for .zip packages.", + Message: "Uninstall script is required for .zip packages.", } } } @@ -2431,14 +2431,16 @@ func (svc *Service) addMetadataToSoftwarePayload(ctx context.Context, payload *f meta, err := file.ExtractInstallerMetadata(payload.InstallerFile) if err != nil { if errors.Is(err, file.ErrUnsupportedType) { + // The failure comes from magic-byte detection, so the file's content + // (not its extension) is what didn't match a supported format. return "", &fleet.BadRequestError{ - Message: "Couldn't edit software. File type not supported. The file should be .pkg, .msi, .exe, .zip, .deb, .rpm, .tar.gz, .sh, .py, .ipa or .ps1.", + Message: "The file's content doesn't match a supported installer format. Supported types: .pkg, .msi, .exe, .zip, .deb, .rpm, .tar.gz, .sh, .py, .ipa or .ps1.", InternalErr: ctxerr.Wrap(ctx, err, "extracting metadata from installer"), } } if errors.Is(err, file.ErrInvalidTarball) { return "", &fleet.BadRequestError{ - Message: "Couldn't edit software. Uploaded file is not a valid .tar.gz archive.", + Message: "Uploaded file is not a valid .tar.gz archive.", InternalErr: ctxerr.Wrap(ctx, err, "extracting metadata from installer"), } } @@ -2447,7 +2449,7 @@ func (svc *Service) addMetadataToSoftwarePayload(ctx context.Context, payload *f if len(meta.PackageIDs) == 0 && meta.Extension != "tar.gz" && meta.Extension != "zip" { return "", &fleet.BadRequestError{ - Message: "Couldn't add. Unable to extract necessary metadata.", + Message: "Unable to extract necessary metadata.", InternalErr: ctxerr.New(ctx, "extracting package IDs from installer metadata"), } } @@ -2476,11 +2478,11 @@ func (svc *Service) addMetadataToSoftwarePayload(ctx context.Context, payload *f ext := strings.ToLower(payload.Extension) if ext == "zip" { return "", &fleet.BadRequestError{ - Message: "Couldn't add. Install script is required for .zip packages.", + Message: "Install script is required for .zip packages.", } } return "", &fleet.BadRequestError{ - Message: fmt.Sprintf("Couldn't add. Install script is required for .%s packages.", ext), + Message: fmt.Sprintf("Install script is required for .%s packages.", ext), } } @@ -2493,7 +2495,7 @@ func (svc *Service) addMetadataToSoftwarePayload(ctx context.Context, payload *f } if payload.UninstallScript == "" && failOnBlankScript && payload.Extension != "ipa" { return "", &fleet.BadRequestError{ - Message: fmt.Sprintf("Couldn't add. Uninstall script is required for .%s packages.", strings.ToLower(payload.Extension)), + Message: fmt.Sprintf("Uninstall script is required for .%s packages.", strings.ToLower(payload.Extension)), } } @@ -2545,7 +2547,7 @@ func (svc *Service) addScriptPackageMetadata(ctx context.Context, payload *fleet if err := fleet.ValidateHostScriptContents(scriptContents, true); err != nil { return &fleet.BadRequestError{ - Message: fmt.Sprintf("Couldn't add. Script validation failed: %s", err.Error()), + Message: fmt.Sprintf("Script validation failed: %s", err.Error()), InternalErr: ctxerr.Wrap(ctx, err, "validating script contents"), } } @@ -2554,7 +2556,7 @@ func (svc *Service) addScriptPackageMetadata(ctx context.Context, payload *fleet kind, directExecute, err := fleet.ShebangInfo(scriptContents) if err != nil { return &fleet.BadRequestError{ - Message: fmt.Sprintf("Couldn't add. Script validation failed: %s", err.Error()), + Message: fmt.Sprintf("Script validation failed: %s", err.Error()), InternalErr: ctxerr.Wrap(ctx, err, "validating script shebang"), } } @@ -2563,7 +2565,7 @@ func (svc *Service) addScriptPackageMetadata(ctx context.Context, payload *fleet // allow no shebang (defaults to /bin/sh), or a supported shell shebang. if directExecute && kind != fleet.ShebangShell { return &fleet.BadRequestError{ - Message: fmt.Sprintf("Couldn't add. Script validation failed: %s", fleet.ErrUnsupportedInterpreter.Error()), + Message: fmt.Sprintf("Script validation failed: %s", fleet.ErrUnsupportedInterpreter.Error()), InternalErr: ctxerr.New(ctx, "shell script with non-shell shebang"), } } @@ -2571,7 +2573,7 @@ func (svc *Service) addScriptPackageMetadata(ctx context.Context, payload *fleet // python scripts must be directly executable (via a python shebang). if !directExecute || kind != fleet.ShebangPython { return &fleet.BadRequestError{ - Message: "Couldn't add. Script validation failed: Python scripts must start with a python shebang (for example, \"#!/usr/bin/env python3\").", + Message: "Script validation failed: Python scripts must start with a python shebang (for example, \"#!/usr/bin/env python3\").", InternalErr: ctxerr.New(ctx, "python script without python shebang"), } } @@ -2579,7 +2581,7 @@ func (svc *Service) addScriptPackageMetadata(ctx context.Context, payload *fleet // PowerShell scripts are executed via powershell.exe, shebangs are not supported. if directExecute { return &fleet.BadRequestError{ - Message: "Couldn't add. Script validation failed: PowerShell scripts must not start with a shebang (\"#!\").", + Message: "Script validation failed: PowerShell scripts must not start with a shebang (\"#!\").", InternalErr: ctxerr.New(ctx, "powershell script with shebang"), } } @@ -4022,7 +4024,7 @@ func (svc *Service) SelfServiceInstallSoftwareTitle(ctx context.Context, host *f // Allow .sh and .py scripts for any unix-like platform (linux and darwin) if !((ext == ".sh" || ext == ".py") && fleet.IsUnixLike(host.Platform)) { return &fleet.BadRequestError{ - Message: fmt.Sprintf("Package (%s) can be installed only on %s hosts.", ext, requiredPlatform), + Message: fmt.Sprintf("Package (%s) can be installed only on %s hosts.", ext, humanReadableRequiredPlatforms(ext, requiredPlatform)), InternalErr: ctxerr.WrapWithData( ctx, err, "invalid host platform for requested installer", map[string]any{"host_id": host.ID, "team_id": host.TeamID, "title_id": softwareTitleID}, @@ -4184,6 +4186,26 @@ func installerRequiredPlatform(installer *fleet.SoftwareInstaller) (ext, require return ext, packageExtensionToPlatform(ext) } +// humanReadableRequiredPlatforms returns the platform(s) named in the +// "can be installed only on ..." rejection message. .sh/.py script packages +// are stored/derived as "linux" but the install gate also permits darwin +// (see fleet.IsUnixLike), so they need the two-platform wording. +func humanReadableRequiredPlatforms(ext, requiredPlatform string) string { + if ext == ".sh" || ext == ".py" { + return "macOS and Linux" + } + switch requiredPlatform { + case "darwin": + return "macOS" + case "windows": + return "Windows" + case "linux": + return "Linux" + default: + return requiredPlatform + } +} + // packageExtensionToPlatform returns the platform name based on the // package extension. Returns an empty string if there is no match. This is only // used as a fallback by installerRequiredPlatform when an installer has no diff --git a/ee/server/service/software_installers_test.go b/ee/server/service/software_installers_test.go index c1fb8e7ec0..1514c4ac29 100644 --- a/ee/server/service/software_installers_test.go +++ b/ee/server/service/software_installers_test.go @@ -1857,7 +1857,7 @@ func TestInstallShScriptOnWindowsFails(t *testing.T) { var bre *fleet.BadRequestError require.ErrorAs(t, err, &bre, "error should be BadRequestError") require.NotNil(t, bre) - require.Contains(t, bre.Message, "can be installed only on linux hosts") + require.Contains(t, bre.Message, "can be installed only on macOS and Linux hosts") } // .py packages are stored with platform='linux', but the unix-like exception @@ -1973,7 +1973,7 @@ func TestInstallPyScriptOnWindowsFails(t *testing.T) { var bre *fleet.BadRequestError require.ErrorAs(t, err, &bre, "error should be BadRequestError") require.NotNil(t, bre) - require.Contains(t, bre.Message, "can be installed only on linux hosts") + require.Contains(t, bre.Message, "can be installed only on macOS and Linux hosts") } // .py packages are stored with platform='linux'; the self-service install path @@ -2674,6 +2674,10 @@ func TestNormalizeSetupExperiencePlatforms(t *testing.T) { {name: "pkg any rejected", input: []string{"darwin"}, extension: "pkg", wantErr: `platform "darwin" is not a valid "setup_experience_platform" value for a .pkg package`}, {name: "msi any rejected", input: []string{"darwin"}, extension: "msi", wantErr: `platform "darwin" is not a valid "setup_experience_platform" value for a .msi package`}, {name: "sh unsupported windows", input: []string{"windows"}, extension: "sh", wantErr: `platform "windows" is not a valid "setup_experience_platform" value for a .sh package`}, + {name: "py darwin", input: []string{"darwin"}, extension: "py", want: []string{"darwin"}}, + {name: "py linux", input: []string{"linux"}, extension: "py", want: []string{"linux"}}, + {name: "py both platforms", input: []string{"darwin", "linux"}, extension: "py", want: []string{"darwin", "linux"}}, + {name: "py unsupported windows", input: []string{"windows"}, extension: "py", wantErr: `platform "windows" is not a valid "setup_experience_platform" value for a .py package`}, {name: "empty string skipped", input: []string{""}, extension: "sh", want: []string{}}, } diff --git a/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareCustomPackage/helpers.tests.tsx b/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareCustomPackage/helpers.tests.tsx index 569a462960..ed6841fa64 100644 --- a/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareCustomPackage/helpers.tests.tsx +++ b/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareCustomPackage/helpers.tests.tsx @@ -1,3 +1,6 @@ +import React from "react"; +import { render } from "@testing-library/react"; + import { getErrorMessage } from "./helpers"; jest.mock("axios", () => { @@ -30,7 +33,7 @@ describe("getErrorMessage", () => { ); }); - it("returns message for script validation error", () => { + it("prepends a single 'Couldn't add.' to an action-neutral script validation reason", () => { const err = { response: { status: 400, @@ -39,7 +42,7 @@ describe("getErrorMessage", () => { { name: "Error", reason: - "Couldn't add. Script validation failed: Script is too large. It's limited to 500,000 characters (approximately 10,000 lines).", + "Script validation failed: Script is too large. It's limited to 500,000 characters (approximately 10,000 lines).", }, ], }, @@ -50,4 +53,26 @@ describe("getErrorMessage", () => { "Couldn't add. Script validation failed: Script is too large. It's limited to 500,000 characters (approximately 10,000 lines)." ); }); + + it("prepends 'Couldn't add.' to a corrupt tarball reason without doubling the verb", () => { + const err = { + response: { + status: 400, + data: { + errors: [ + { + name: "Error", + reason: "Uploaded file is not a valid .tar.gz archive.", + }, + ], + }, + }, + }; + + const { container } = render(<>{getErrorMessage(err)}); + expect(container.textContent).toContain( + "Couldn't add. This is not a valid .tar.gz archive." + ); + expect(container.textContent).not.toContain("Couldn't add. Couldn't add."); + }); }); diff --git a/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareCustomPackage/helpers.tsx b/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareCustomPackage/helpers.tsx index 79cc8c7514..e67bd20c88 100644 --- a/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareCustomPackage/helpers.tsx +++ b/frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareCustomPackage/helpers.tsx @@ -65,7 +65,7 @@ export const getErrorMessage = (err: unknown, softwareTitle?: string) => { if (reason.includes("not a valid .tar.gz archive")) { return ( <> - This is not a valid .tar.gz archive.{" "} + {ADD_SOFTWARE_ERROR_PREFIX} This is not a valid .tar.gz archive.{" "} { + const actual = jest.requireActual("axios"); + return { + ...actual, + isAxiosError: () => true, + }; +}); + +const software = { + name: "Zoom.pkg", + display_name: "Zoom", +} as ISoftwarePackage; + +describe("getErrorMessage", () => { + it("prepends 'Couldn't edit software.' to an action-neutral validator reason", () => { + const err = { + response: { + status: 400, + data: { + errors: [ + { + name: "Error", + reason: + 'Script validation failed: Python scripts must start with a python shebang (for example, "#!/usr/bin/env python3").', + }, + ], + }, + }, + }; + + expect(getErrorMessage(err, software)).toBe( + 'Couldn\'t edit software. Script validation failed: Python scripts must start with a python shebang (for example, "#!/usr/bin/env python3").' + ); + }); + + it("does not double the verb when the reason has no recognized special case", () => { + const err = { + response: { + status: 400, + data: { + errors: [ + { + name: "Error", + reason: "Uploaded file is not a valid .tar.gz archive.", + }, + ], + }, + }, + }; + + expect(getErrorMessage(err, software)).toBe( + "Couldn't edit software. Uploaded file is not a valid .tar.gz archive." + ); + }); + + it("normalizes a backend reason that carries its own verb to the product wording (no doubling)", () => { + const err = { + response: { + status: 400, + data: { + errors: [ + { + name: "Error", + reason: + "Couldn't edit. Install script is required for .exe packages.", + }, + ], + }, + }, + }; + + expect(getErrorMessage(err, software)).toBe( + "Couldn't edit software. Install script is required for .exe packages." + ); + }); + + it("returns the default message when there is no reason", () => { + const err = { + response: { + status: 400, + data: { + errors: [], + }, + }, + }; + + expect(getErrorMessage(err, software)).toBe( + "Couldn't edit software. Please try again." + ); + }); + + it("bolds the software name for the different-file-type reshape", () => { + const err = { + response: { + status: 400, + data: { + errors: [ + { + name: "Error", + reason: "The selected package is for a different file type.", + }, + ], + }, + }, + }; + + const { container } = render(<>{getErrorMessage(err, software)}); + expect(container.textContent).toBe( + "Couldn't edit Zoom. The selected package is for a different file type." + ); + }); +}); diff --git a/frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditSoftwareModal/helpers.tsx b/frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditSoftwareModal/helpers.tsx index 504810e194..8f2107da92 100644 --- a/frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditSoftwareModal/helpers.tsx +++ b/frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditSoftwareModal/helpers.tsx @@ -8,8 +8,10 @@ import { generateSecretErrMsg, getDisplayedSoftwareName, } from "pages/SoftwarePage/helpers"; +import { ensurePeriod } from "pages/SoftwarePage/SoftwareAddPage/helpers"; -const DEFAULT_ERROR_MESSAGE = "Couldn't edit software. Please try again."; +const EDIT_SOFTWARE_ERROR_PREFIX = "Couldn't edit software."; +const DEFAULT_ERROR_MESSAGE = `${EDIT_SOFTWARE_ERROR_PREFIX} Please try again.`; // eslint-disable-next-line import/prefer-default-export export const getErrorMessage = ( @@ -22,7 +24,7 @@ export const getErrorMessage = ( const reason = getErrorReason(err); if (isTimeout) { - return "Couldn't add. Request timeout. Please make sure your server and load balancer timeout is long enough."; + return `${EDIT_SOFTWARE_ERROR_PREFIX} Request timeout. Please make sure your server and load balancer timeout is long enough.`; } else if (reason.includes("selected package is")) { return ( <> @@ -44,5 +46,16 @@ export const getErrorMessage = ( ); } - return reason || DEFAULT_ERROR_MESSAGE; + if (!reason) { + return DEFAULT_ERROR_MESSAGE; + } + // The edit modal always leads with the product-approved verb. Shared + // validators now return action-neutral reasons, but some backend messages + // still carry their own leading verb (e.g. "Couldn't edit.", "Couldn't + // update."); strip it so the UI shows a single, consistent "Couldn't edit + // software." rather than the backend's wording or a doubled verb. + const withoutLeadingVerb = reason.replace(/^Couldn't [^.]*\.\s*/, ""); + return withoutLeadingVerb + ? `${EDIT_SOFTWARE_ERROR_PREFIX} ${ensurePeriod(withoutLeadingVerb)}` + : DEFAULT_ERROR_MESSAGE; }; diff --git a/orbit/pkg/installer/installer.go b/orbit/pkg/installer/installer.go index 9aa47c5a95..cdbfb752df 100644 --- a/orbit/pkg/installer/installer.go +++ b/orbit/pkg/installer/installer.go @@ -500,13 +500,8 @@ func (r *Runner) attemptInstall(ctx context.Context, installer *fleet.SoftwareIn installerPath = extractDestination } - scriptExtension := ".sh" - if runtime.GOOS == "windows" { - scriptExtension = ".ps1" - } - logger.Info().Msg("about to run install script") - installOutput, installExitCode, err := r.runInstallerScript(ctx, installer.InstallScript, installerPath, "install-script"+scriptExtension) + installOutput, installExitCode, err := r.runInstallerScript(ctx, installer.InstallScript, installerPath, "install-script"+scriptFileExtension(installer.InstallScript, runtime.GOOS)) payload.InstallScriptOutput = &installOutput payload.InstallScriptExitCode = &installExitCode if err != nil { @@ -517,7 +512,7 @@ func (r *Runner) attemptInstall(ctx context.Context, installer *fleet.SoftwareIn if installer.PostInstallScript != "" { logger.Info().Str("installerPath", installerPath).Msg("about to run post-install script") - postOutput, postExitCode, postErr := r.runInstallerScript(ctx, installer.PostInstallScript, installerPath, "post-install-script"+scriptExtension) + postOutput, postExitCode, postErr := r.runInstallerScript(ctx, installer.PostInstallScript, installerPath, "post-install-script"+scriptFileExtension(installer.PostInstallScript, runtime.GOOS)) payload.PostInstallScriptOutput = &postOutput payload.PostInstallScriptExitCode = &postExitCode @@ -539,7 +534,7 @@ func (r *Runner) attemptInstall(ctx context.Context, installer *fleet.SoftwareIn uninstallScript = file.GetRemoveScript(ext) } uninstallOutput, uninstallExitCode, uninstallErr := r.runInstallerScript(ctx, uninstallScript, installerPath, - "rollback-script"+scriptExtension) + "rollback-script"+scriptFileExtension(uninstallScript, runtime.GOOS)) logger.Info().Msgf( "rollback status: exit code: %d, error: %s, output: %s", uninstallExitCode, uninstallErr, uninstallOutput, @@ -641,6 +636,20 @@ func isNetworkOrTransientError(err error) bool { return false } +// scriptFileExtension picks the temp script filename extension so interpreter +// error output (e.g. Python tracebacks) references a correctly-typed file. +// Windows scripts are always PowerShell; on unix we honor the script's shebang. +// goos is passed in (rather than read from runtime) so it stays a pure function. +func scriptFileExtension(contents, goos string) string { + if goos == "windows" { + return ".ps1" + } + if kind, _, err := fleet.ShebangInfo(contents); err == nil && kind == fleet.ShebangPython { + return ".py" + } + return ".sh" +} + func (r *Runner) runInstallerScript(ctx context.Context, scriptContents string, installerPath string, fileName string) (string, int, error) { // run script in installer directory installerDir := filepath.Dir(installerPath) @@ -664,7 +673,14 @@ func (r *Runner) runInstallerScript(ctx context.Context, scriptContents string, output, exitCode, err := execFn(ctx, scriptPath, env) if err != nil { - return string(output), exitCode, err + out := string(output) + // An execve failure (e.g. a missing shebang interpreter) surfaces as exit + // code -1 with no output; the only diagnostic lives in err, so fold it into + // the output so the server reports something actionable instead of blank. + if exitCode == -1 && out == "" { + out = err.Error() + } + return out, exitCode, err } return string(output), exitCode, nil diff --git a/orbit/pkg/installer/installer_test.go b/orbit/pkg/installer/installer_test.go index 787433f463..8b3c885cd7 100644 --- a/orbit/pkg/installer/installer_test.go +++ b/orbit/pkg/installer/installer_test.go @@ -1412,3 +1412,103 @@ func TestInstallSoftwareNotFoundRetryWindow(t *testing.T) { require.False(t, stillTracked, "non-404 error path must clear tracker") }) } + +// attemptInstallExtTestSetup wires a Runner whose exec fn records the base name +// of every script it runs, so tests can assert the temp file extension picked +// for each script's contents. +func attemptInstallExtTestSetup(t *testing.T, execFn func(context.Context, string, []string) ([]byte, int, error)) (*Runner, *[]string) { + t.Helper() + var executed []string + oc := &TestOrbitClient{ + downloadInstallerFn: func(installerID uint, downloadDir string) (string, error) { + return filepath.Join(downloadDir, fmt.Sprint(installerID)+".pkg"), nil + }, + } + r := &Runner{ + OrbitClient: oc, + scriptsEnabled: func() bool { return true }, + installerExecutionTimeout: time.Minute, + tempDirFn: func(string, string) (string, error) { return t.TempDir(), nil }, + removeAllFn: func(string) error { return nil }, + execCmdFn: func(ctx context.Context, scriptPath string, env []string) ([]byte, int, error) { + executed = append(executed, filepath.Base(scriptPath)) + return execFn(ctx, scriptPath, env) + }, + } + return r, &executed +} + +func TestScriptFileExtension(t *testing.T) { + t.Parallel() + + require.Equal(t, ".sh", scriptFileExtension("", "darwin"), "no shebang defaults to shell") + require.Equal(t, ".sh", scriptFileExtension("#!/bin/bash\necho hi", "darwin"), "shell shebang") + require.Equal(t, ".py", scriptFileExtension("#!/usr/bin/env python3\nprint('hi')", "darwin"), "python shebang") + require.Equal(t, ".py", scriptFileExtension("#!/usr/bin/env python3\nprint('hi')", "linux"), "python shebang on linux") + + require.Equal(t, ".ps1", scriptFileExtension("#!/usr/bin/env python3\nprint('hi')", "windows"), "windows is always powershell") + require.Equal(t, ".ps1", scriptFileExtension("", "windows")) +} + +func TestAttemptInstallScriptExtension(t *testing.T) { + success := func(context.Context, string, []string) ([]byte, int, error) { return []byte("ok"), 0, nil } + + t.Run("python install script", func(t *testing.T) { + r, executed := attemptInstallExtTestSetup(t, success) + _, err := r.attemptInstall(context.Background(), &fleet.SoftwareInstallDetails{ + InstallerID: 1, + InstallScript: "#!/usr/bin/env python3\nprint('install')", + }, &fleet.HostSoftwareInstallResultPayload{}, log.With().Logger()) + require.NoError(t, err) + require.Contains(t, *executed, "install-script.py") + }) + + t.Run("no-shebang install script", func(t *testing.T) { + r, executed := attemptInstallExtTestSetup(t, success) + _, err := r.attemptInstall(context.Background(), &fleet.SoftwareInstallDetails{ + InstallerID: 1, + InstallScript: "echo install", + }, &fleet.HostSoftwareInstallResultPayload{}, log.With().Logger()) + require.NoError(t, err) + require.Contains(t, *executed, "install-script.sh") + }) + + t.Run("python install with shell post-install and uninstall", func(t *testing.T) { + // A .py package can carry a Python install script but shell post-install + // and uninstall scripts; each temp file must reflect its own shebang. + exitPost := func(_ context.Context, scriptPath string, _ []string) ([]byte, int, error) { + if strings.Contains(scriptPath, "post-install-script") { + return []byte("boom"), 1, &exec.ExitError{} + } + return []byte("ok"), 0, nil + } + r, executed := attemptInstallExtTestSetup(t, exitPost) + _, _ = r.attemptInstall(context.Background(), &fleet.SoftwareInstallDetails{ + InstallerID: 1, + InstallScript: "#!/usr/bin/env python3\nprint('install')", + PostInstallScript: "#!/bin/sh\necho post", + UninstallScript: "#!/bin/sh\necho uninstall", + }, &fleet.HostSoftwareInstallResultPayload{}, log.With().Logger()) + require.Contains(t, *executed, "install-script.py") + require.Contains(t, *executed, "post-install-script.sh") + require.Contains(t, *executed, "rollback-script.sh") + }) +} + +// An execve failure (exit code -1, empty output) must surface the underlying +// error to the server rather than reporting a blank result. +func TestRunInstallerScriptSurfacesExecveError(t *testing.T) { + const execveErr = "fork/exec /usr/local/bin/python3: no such file or directory" + r, _ := attemptInstallExtTestSetup(t, func(context.Context, string, []string) ([]byte, int, error) { + return nil, -1, errors.New(execveErr) + }) + + payload := &fleet.HostSoftwareInstallResultPayload{} + _, err := r.attemptInstall(context.Background(), &fleet.SoftwareInstallDetails{ + InstallerID: 1, + InstallScript: "#!/usr/local/bin/python3\nprint('install')", + }, payload, log.With().Logger()) + require.Error(t, err) + require.NotNil(t, payload.InstallScriptOutput) + require.Contains(t, *payload.InstallScriptOutput, execveErr) +} diff --git a/server/fleet/scripts.go b/server/fleet/scripts.go index ff14b76e37..e8ca75a0f0 100644 --- a/server/fleet/scripts.go +++ b/server/fleet/scripts.go @@ -378,9 +378,13 @@ func (hsr HostScriptResult) UserMessage(hostTimeout bool, hostTimeoutValue *int) // process can exit with a status that collides with these values; that // ambiguity is accepted. const ( - // ExitCodeScriptTimeout is reported when a script did not terminate - // normally, e.g. fleetd killed it at the execution timeout (Go reports -1 - // for a process that did not exit cleanly). Script results only. + // ExitCodeScriptTimeout is reported when a process did not exit cleanly: + // either it never started (e.g. exec failed) or it was stopped before + // finishing (e.g. fleetd killed it at the execution timeout). Go reports + // -1 in both cases, so the two cannot be distinguished from the exit code + // alone. For script results, this renders the timeout message; for + // software installs, it renders the "couldn't run the install script" + // message. ExitCodeScriptTimeout = -1 // ExitCodeScriptsDisabled is reported by fleetd when a script or software // install can't run because scripts are disabled on the host. diff --git a/server/fleet/software_installer.go b/server/fleet/software_installer.go index 614f48945e..5b25b52036 100644 --- a/server/fleet/software_installer.go +++ b/server/fleet/software_installer.go @@ -500,9 +500,10 @@ const ( Exit code: %d (Failed) %s ` - SoftwareInstallerDownloadFailedCopy = "Installing software...\nError: Software installer download failed." - SoftwareInstallerNotFoundCopy = "Installing software...\nError: The software installer no longer exists on the server. fleetd abandoned the install after retrying for 5 minutes." - SoftwareInstallerFleetVarsFailedCopy = "Installing software...\nError: Fleet couldn't resolve variables in this software's scripts.\n%s" + SoftwareInstallerDownloadFailedCopy = "Installing software...\nError: Software installer download failed." + SoftwareInstallerNotFoundCopy = "Installing software...\nError: The software installer no longer exists on the server. fleetd abandoned the install after retrying for 5 minutes." + SoftwareInstallerFleetVarsFailedCopy = "Installing software...\nError: Fleet couldn't resolve variables in this software's scripts.\n%s" + SoftwareInstallerScriptCouldNotRunCopy = "Installing software...\nError: Fleet couldn't run the install script. The script's interpreter (from its \"#!\" shebang) may be missing or not executable on this host, or the script was stopped before it finished.\n%s" ) // EnhanceOutputDetails is used to add extra boilerplate/information to the @@ -539,6 +540,9 @@ func (h *HostSoftwareInstallerResult) EnhanceOutputDetails() { case ExitCodeFleetVarResolutionFailed: *h.Output = fmt.Sprintf(SoftwareInstallerFleetVarsFailedCopy, *h.Output) return + case ExitCodeScriptTimeout: + h.Output = new(fmt.Sprintf(SoftwareInstallerScriptCouldNotRunCopy, *h.Output)) + return default: h.Output = ptr.String(fmt.Sprintf(SoftwareInstallerInstallFailCopy, *h.Output)) return @@ -805,7 +809,7 @@ func CanonicalPlatform(p string) string { func AllowedSetupExperiencePlatformsForExtension(ext string) []string { ext = strings.TrimPrefix(strings.ToLower(ext), ".") switch ext { - case "sh": + case "sh", "py": return []string{"darwin", "linux"} default: return nil diff --git a/server/fleet/software_installer_test.go b/server/fleet/software_installer_test.go index 1c487b2977..7017019493 100644 --- a/server/fleet/software_installer_test.go +++ b/server/fleet/software_installer_test.go @@ -184,6 +184,30 @@ func TestSoftwareInstallerPlatformFromExtension(t *testing.T) { } } +func TestAllowedSetupExperiencePlatformsForExtension(t *testing.T) { + testCases := []struct { + ext string + expected []string + }{ + {".py", []string{"darwin", "linux"}}, + {"py", []string{"darwin", "linux"}}, + {".sh", []string{"darwin", "linux"}}, + {"sh", []string{"darwin", "linux"}}, + {".ps1", nil}, + {"ps1", nil}, + {".exe", nil}, + {"exe", nil}, + {"", nil}, + } + + for _, tc := range testCases { + t.Run(tc.ext, func(t *testing.T) { + result := AllowedSetupExperiencePlatformsForExtension(tc.ext) + require.Equal(t, tc.expected, result) + }) + } +} + func TestSofwareInstallerSourceFromExtensionAndName(t *testing.T) { testCases := []struct { ext string diff --git a/server/fleet/software_test.go b/server/fleet/software_test.go index 66ecb7e0ff..6a36d1bc34 100644 --- a/server/fleet/software_test.go +++ b/server/fleet/software_test.go @@ -181,6 +181,29 @@ func TestEnhanceOutputDetails(t *testing.T) { "There is no IdP username for this host. Fleet couldn't populate $FLEET_VAR_HOST_END_USER_IDP_USERNAME.")), expectedPostInstallScriptOutput: nil, }, + { + name: "non-pending status with script timeout/could-not-run exit code and empty output", + initial: HostSoftwareInstallerResult{ + Status: SoftwareInstallFailed, + InstallScriptExitCode: new(ExitCodeScriptTimeout), + Output: new(""), + }, + expectedPreInstallQueryOutput: nil, + expectedOutput: new(fmt.Sprintf(SoftwareInstallerScriptCouldNotRunCopy, "")), + expectedPostInstallScriptOutput: nil, + }, + { + name: "non-pending status with script timeout/could-not-run exit code and partial output", + initial: HostSoftwareInstallerResult{ + Status: SoftwareInstallFailed, + InstallScriptExitCode: new(ExitCodeScriptTimeout), + Output: new("partial output before the process was stopped"), + }, + expectedPreInstallQueryOutput: nil, + expectedOutput: new(fmt.Sprintf(SoftwareInstallerScriptCouldNotRunCopy, + "partial output before the process was stopped")), + expectedPostInstallScriptOutput: nil, + }, { name: "non-pending status with failed install script", initial: HostSoftwareInstallerResult{ diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 6809ae872c..3fddda8682 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -17146,7 +17146,7 @@ func (s *integrationEnterpriseTestSuite) TestPKGNoBundleIdentifier() { Filename: "no_bundle_identifier.pkg", TeamID: &team.ID, } - s.uploadSoftwareInstaller(t, payload, http.StatusBadRequest, "Couldn't add. Unable to extract necessary metadata.") + s.uploadSoftwareInstaller(t, payload, http.StatusBadRequest, "Unable to extract necessary metadata.") } func (s *integrationEnterpriseTestSuite) TestEXEPackageUploads() { @@ -17161,13 +17161,13 @@ func (s *integrationEnterpriseTestSuite) TestEXEPackageUploads() { Filename: "hello-world-installer.exe", TeamID: &team.ID, } - s.uploadSoftwareInstaller(t, payload, http.StatusBadRequest, "Couldn't add. Uninstall script is required for .exe packages.") + s.uploadSoftwareInstaller(t, payload, http.StatusBadRequest, "Uninstall script is required for .exe packages.") payload = &fleet.UploadSoftwareInstallerPayload{ Filename: "hello-world-installer.exe", TeamID: &team.ID, } - s.uploadSoftwareInstaller(t, payload, http.StatusBadRequest, "Couldn't add. Install script is required for .exe packages.") + s.uploadSoftwareInstaller(t, payload, http.StatusBadRequest, "Install script is required for .exe packages.") payload = &fleet.UploadSoftwareInstallerPayload{ InstallScript: "some installer script", @@ -23341,7 +23341,7 @@ func (s *integrationEnterpriseTestSuite) TestBatchSoftwareUploadWithSHAs() { s.DoJSON("POST", "/api/latest/fleet/software/batch", batchSetSoftwareInstallersRequest{Software: softwareToInstall}, http.StatusAccepted, &batchResponse, "team_name", team2.Name) errMsg = waitBatchSetSoftwareInstallersFailed(t, &s.withServer, team2.Name, batchResponse.RequestUUID) - require.Contains(t, errMsg, "Couldn't add. Install script is required for .exe packages.") + require.Contains(t, errMsg, "Install script is required for .exe packages.") softwareToInstall[1].InstallScript = "echo install" softwareToInstall[1].UninstallScript = "echo uninstall"