diff --git a/changes/50097-fma-auto-update-keeps-stale-install-script b/changes/50097-fma-auto-update-keeps-stale-install-script new file mode 100644 index 0000000000..b6e39bd4e9 --- /dev/null +++ b/changes/50097-fma-auto-update-keeps-stale-install-script @@ -0,0 +1 @@ +- Fixed a bug where the Fleet-maintained app auto-update job could keep an app's previous install script (which references the old installer filename) after downloading a newer version, causing the install to fail. diff --git a/ee/server/service/maintained_apps_auto_update.go b/ee/server/service/maintained_apps_auto_update.go index bbc617ccb3..379db0be05 100644 --- a/ee/server/service/maintained_apps_auto_update.go +++ b/ee/server/service/maintained_apps_auto_update.go @@ -285,16 +285,24 @@ func downloadNewVersionIfEligible( // Preserve admin-customized scripts across auto-updates. The active installer // (still the previous version here; promotion happens later) is the one to // carry forward from. Detect customization per-script by comparing against the - // manifest: the install script is a version-independent template (direct - // compare), but the uninstall script is version-specific after $PACKAGE_ID / - // $UPGRADE_CODE substitution, so compare against the manifest template - // substituted with the active version's package IDs. + // manifest, first neutralizing the parts that legitimately change between + // versions so a routine version bump isn't mistaken for an edit: the install + // script hardcodes the versioned installer filename, and the uninstall script + // is version-specific after $PACKAGE_ID / $UPGRADE_CODE substitution. active, err := ds.GetSoftwareInstallerMetadataByTeamAndTitleID(ctx, c.TeamID, c.TitleID, true) if err != nil && !fleet.IsNotFound(err) { return ctxerr.Wrap(ctx, err, "getting active installer to preserve custom scripts") } if active != nil { - if strings.TrimSpace(active.InstallScript) != strings.TrimSpace(app.InstallScript) { + // Compare with the old and new filenames replaced by a common placeholder + // (active.Name is the filename column); otherwise the filename difference + // alone reads as an admin edit and the stale script is kept against the + // newly downloaded installer. FMAs whose script embeds a name unrelated to + // the stored installer filename (e.g. a versioned pkg inside a dmg) aren't + // neutralized and fall back to preserving, as before. + activeInstall := normalizeInstallerFilename(strings.TrimSpace(active.InstallScript), active.Name) + manifestInstall := normalizeInstallerFilename(strings.TrimSpace(app.InstallScript), payload.Filename) + if activeInstall != manifestInstall { payload.InstallScript = active.InstallScript } defaultUninstall := &fleet.UploadSoftwareInstallerPayload{ @@ -396,3 +404,23 @@ func teamIDForLog(p *uint) any { } return *p } + +// normalizeInstallerFilename replaces the version-specific installer filename in +// a generated FMA install script with a fixed placeholder, so two scripts that +// differ only because the installer filename changed between versions compare +// equal. A missing filename leaves the script unchanged. +func normalizeInstallerFilename(script, filename string) string { + if filename == "" { + return script + } + const placeholder = "__FLEET_INSTALLER_FILE__" + // Replace only where the filename is the installer path argument. A short URL + // basename (e.g. "dmg") would otherwise rewrite free-floating occurrences such + // as /tmp/dmg_mount_XXXXXX. + script = strings.ReplaceAll(script, `"$TMPDIR/`+filename+`"`, `"$TMPDIR/`+placeholder+`"`) + // The unquoted (choices) form is always followed by " -target", so bound the + // match with the trailing space; otherwise a filename would prefix-match a + // longer path that merely starts with it. + script = strings.ReplaceAll(script, `"$TMPDIR"/`+filename+" ", `"$TMPDIR"/`+placeholder+" ") + return script +} diff --git a/ee/server/service/maintained_apps_auto_update_download_test.go b/ee/server/service/maintained_apps_auto_update_download_test.go index 8fa46d8479..2c2d88863e 100644 --- a/ee/server/service/maintained_apps_auto_update_download_test.go +++ b/ee/server/service/maintained_apps_auto_update_download_test.go @@ -36,6 +36,7 @@ type fakeManifestServer struct { sha string bytes []byte version string // manifest version to advertise (default testFMALatest) + install string // install script ref body (default "echo install") uninstall string // uninstall script ref body (default "echo uninstall") upgradeCode string // manifest upgrade_code (default empty) manifestHits int @@ -44,7 +45,7 @@ type fakeManifestServer struct { } func newFakeManifestServer(t *testing.T) *fakeManifestServer { - f := &fakeManifestServer{bytes: []byte("fake installer payload"), version: testFMALatest, uninstall: "echo uninstall"} + f := &fakeManifestServer{bytes: []byte("fake installer payload"), version: testFMALatest, install: "echo install", uninstall: "echo uninstall"} sum := sha256.Sum256(f.bytes) f.sha = hex.EncodeToString(sum[:]) @@ -64,7 +65,7 @@ func newFakeManifestServer(t *testing.T) *fakeManifestServer { Queries: ma.FMAQueries{Exists: "SELECT 1", Patched: "SELECT 2"}, DefaultCategories: []string{"Browsers"}, }}, - Refs: map[string]string{"i": "echo install", "u": f.uninstall}, + Refs: map[string]string{"i": f.install, "u": f.uninstall}, } _ = json.NewEncoder(w).Encode(manifest) }) @@ -379,3 +380,100 @@ func TestAutoUpdatePreservesCustomScripts(t *testing.T) { require.Equal(t, "echo CUSTOM install", gotPayload.InstallScript, "custom install script carried forward") require.Equal(t, "echo CUSTOM uninstall", gotPayload.UninstallScript, "custom uninstall script carried forward") } + +// TestAutoUpdateAdoptsNewInstallScriptWhenOnlyFilenameChanged guards against a +// regression where the cron kept the active version's install script (which +// hardcodes the old installer filename) against a newly downloaded installer, +// because FMA install scripts embed the versioned filename and the whole-string +// compare misread that difference as an admin customization. The unedited script +// must adopt the new manifest. +func TestAutoUpdateAdoptsNewInstallScriptWhenOnlyFilenameChanged(t *testing.T) { + srv := newFakeManifestServer(t) + // New manifest script references the new installer file. The byte-dedup path + // derives the payload filename from the installer URL basename ("installer.pkg"). + srv.install = `sudo installer -pkg "$TMPDIR/installer.pkg" -target /` + ds := baseDownloadStore(t, "149.0.0", 9) + // Active installer holds the canonical script for the OLD version — identical + // except the hardcoded installer filename (Name is the filename column). + ds.GetSoftwareInstallerMetadataByTeamAndTitleIDFunc = func(ctx context.Context, tmID *uint, titleID uint, withScriptContents bool) (*fleet.SoftwareInstaller, error) { + return &fleet.SoftwareInstaller{ + Name: "installer-149.0.0.pkg", + InstallScript: `sudo installer -pkg "$TMPDIR/installer-149.0.0.pkg" -target /`, + UninstallScript: "echo uninstall", + Extension: "pkg", + }, nil + } + var gotPayload *fleet.UploadSoftwareInstallerPayload + ds.InsertFleetMaintainedAppVersionFunc = func(ctx context.Context, activeInstallerID uint, payload *fleet.UploadSoftwareInstallerPayload) (uint, error) { + gotPayload = payload + return 13, nil + } + + store := memStore(srv.sha) // byte-dedup: no download, filename comes from the URL + require.NoError(t, AutoUpdateFleetMaintainedApps(context.Background(), ds, store, discardLogger())) + require.NotNil(t, gotPayload) + require.Equal(t, "installer.pkg", gotPayload.Filename) + require.Equal(t, srv.install, gotPayload.InstallScript, "unedited script must adopt the new manifest, not keep the old filename") + require.NotContains(t, gotPayload.InstallScript, "installer-149.0.0.pkg") +} + +// TestAutoUpdatePreservesCustomInstallScriptBeyondFilename is the counterpart: +// filename normalization must not clobber a genuine admin edit. When the active +// script differs from the manifest by more than the installer filename, it is +// preserved. +func TestAutoUpdatePreservesCustomInstallScriptBeyondFilename(t *testing.T) { + srv := newFakeManifestServer(t) + srv.install = `sudo installer -pkg "$TMPDIR/installer.pkg" -target /` + ds := baseDownloadStore(t, "149.0.0", 9) + custom := `sudo installer -pkg "$TMPDIR/installer-149.0.0.pkg" -target /` + "\necho admin custom step" + ds.GetSoftwareInstallerMetadataByTeamAndTitleIDFunc = func(ctx context.Context, tmID *uint, titleID uint, withScriptContents bool) (*fleet.SoftwareInstaller, error) { + return &fleet.SoftwareInstaller{ + Name: "installer-149.0.0.pkg", + InstallScript: custom, + UninstallScript: "echo uninstall", + Extension: "pkg", + }, nil + } + var gotPayload *fleet.UploadSoftwareInstallerPayload + ds.InsertFleetMaintainedAppVersionFunc = func(ctx context.Context, activeInstallerID uint, payload *fleet.UploadSoftwareInstallerPayload) (uint, error) { + gotPayload = payload + return 13, nil + } + + store := memStore(srv.sha) + require.NoError(t, AutoUpdateFleetMaintainedApps(context.Background(), ds, store, discardLogger())) + require.NotNil(t, gotPayload) + require.Equal(t, custom, gotPayload.InstallScript, "a customization beyond the filename must be preserved") +} + +// TestNormalizeInstallerFilename verifies the filename is neutralized only where +// it's the installer path argument — not free-floating text elsewhere. A URL +// basename can resolve to a short token (e.g. "dmg") that also appears in a +// mount path, and a whole-script replace would mangle it and break the compare. +func TestNormalizeInstallerFilename(t *testing.T) { + const ph = "__FLEET_INSTALLER_FILE__" + + // Short token that also appears free-floating in the mount path. + dmg := "MOUNT_POINT=$(mktemp -d /tmp/dmg_mount_XXXXXX)\n" + `sudo cp -R "$TMPDIR/dmg" "$APPDIR"` + got := normalizeInstallerFilename(dmg, "dmg") + require.Contains(t, got, "/tmp/dmg_mount_XXXXXX", "free-floating token must not be replaced") + require.Contains(t, got, `"$TMPDIR/`+ph+`"`, "installer path argument is neutralized") + + // Quoted pkg form. + require.Equal(t, + `sudo installer -pkg "$TMPDIR/`+ph+`" -target /`, + normalizeInstallerFilename(`sudo installer -pkg "$TMPDIR/Foo-1.0.pkg" -target /`, "Foo-1.0.pkg")) + + // Choices form (filename unquoted after $TMPDIR). + require.Equal(t, + `sudo installer -pkg "$TMPDIR"/`+ph+` -target / -applyChoiceChangesXML "$X"`, + normalizeInstallerFilename(`sudo installer -pkg "$TMPDIR"/Foo-1.0.pkg -target / -applyChoiceChangesXML "$X"`, "Foo-1.0.pkg")) + + // Unquoted form must not prefix-match a longer path that only starts with the filename. + require.Equal(t, + `sudo installer -pkg "$TMPDIR"/dmg_mount_XXXXXX -target /`, + normalizeInstallerFilename(`sudo installer -pkg "$TMPDIR"/dmg_mount_XXXXXX -target /`, "dmg")) + + // Empty filename is a no-op. + require.Equal(t, "unchanged", normalizeInstallerFilename("unchanged", "")) +}