From 3660b546f2d12e80d696a4b8485eadd5a2e34ceb Mon Sep 17 00:00:00 2001 From: Carlo <1778532+cdcme@users.noreply.github.com> Date: Thu, 30 Jul 2026 17:42:55 -0400 Subject: [PATCH] Fix FMA auto-update keeping the stale install script (#50200) **Related issue:** Resolves #50097 ## Summary FMA auto-update preserves an admin-customized install script by comparing the active script against the new manifest's, but FMA scripts hardcode the versioned installer filename, so a routine version bump looked like an edit and the old script (old filename) was kept against the newly downloaded installer, and the install failed. The fix neutralizes the installer filename in both scripts before comparing (mirroring the existing uninstall `$PACKAGE_ID` handling), so a filename-only difference adopts the new script while a genuine edit is still preserved. # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/` (`changes/50097-fma-auto-update-keeps-stale-install-script`). ## Testing - [x] Added/updated automated tests (adopt-on-version-bump regression + preserve-genuine-edit counterpart). - [x] QA'd all new/changed functionality manually. ## Summary by CodeRabbit * **Bug Fixes** * Fixed Fleet-maintained app auto-updates that could keep an outdated install script after downloading a newer version, causing install failures. * Improved install-script change detection by ignoring version-only installer filename differences. * Continued to preserve administrator-customized install scripts when updates change more than just the installer filename. * **Tests** * Added and expanded coverage for installer-script normalization and auto-update install-script selection behavior. --- ...fma-auto-update-keeps-stale-install-script | 1 + .../service/maintained_apps_auto_update.go | 38 ++++++- ...intained_apps_auto_update_download_test.go | 102 +++++++++++++++++- 3 files changed, 134 insertions(+), 7 deletions(-) create mode 100644 changes/50097-fma-auto-update-keeps-stale-install-script 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", "")) +}