Skip to content

Fix patch policy bugs #43420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jkatz01 merged 8 commits into from
Apr 11, 2026
Merged

Fix patch policy bugs #43420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
93da312
Fix nil error dereference and unable to rename in gitops
jkatz01 Apr 10, 2026
0cb4659
claude: datastore test
jkatz01 Apr 10, 2026
9e0976f
Check that global patch policy cant be added
jkatz01 Apr 10, 2026
3181a1b
Get PatchSoftwareTitleID for policies in config to compare with list …
jkatz01 Apr 10, 2026
9214b6b
changefile
jkatz01 Apr 10, 2026
f175b9c
check that policy id didnt change in test
jkatz01 Apr 10, 2026
e7b64bf
coderabbit pr feedback
jkatz01 Apr 10, 2026
0b1af18
only update name if necessary
jkatz01 Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changes/43389-patch-policy-gitops-bugs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Fixed bug where renaming a patch policy in a GitOps file caused it to be deleted initially.
- Fixed a nil pointer dereference in the contributor api spec/policies.
22 changes: 21 additions & 1 deletion server/datastore/mysql/policies.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1478,6 +1478,11 @@ func (ds *Datastore) ApplyPolicySpecs(ctx context.Context, authorID uint, specs

// generate new up-to-date patch policy
if spec.Type == fleet.PolicyTypePatch {
if fmaTitleID == nil {
return ctxerr.Wrap(ctx, &fleet.BadRequestError{
Message: fmt.Sprintf("fleet_maintained_app_slug must be set for patch policy: %s", spec.Name),
})
}
Comment on lines +1481 to +1485
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

This validation still misses the sql.ErrNoRows path.

fmaTitleID == nil only happens if the earlier slug lookup returned a row with NULL title_id. When the slug is invalid or not available for that team, the lookup at Line 1320 already fails with sql.ErrNoRows, so this block never runs and the request still bubbles up as an internal datastore error instead of the intended 400.

Suggested fix 
err := sqlx.GetContext(ctx, queryerContext, &fmaTitleID, `
	SELECT si.title_id FROM software_installers si
	JOIN fleet_maintained_apps fma ON si.fleet_maintained_app_id = fma.id
	WHERE fma.slug = ?
	AND si.global_or_team_id = ?
`, spec.FleetMaintainedAppSlug, teamNameToID[spec.Team])
if err != nil {
	if errors.Is(err, sql.ErrNoRows) {
		return ctxerr.Wrap(ctx, &fleet.BadRequestError{
			Message: fmt.Sprintf("fleet_maintained_app_slug %q is invalid for patch policy %q", spec.FleetMaintainedAppSlug, spec.Name),
		})
	}
	return ctxerr.Wrap(ctx, err, "get fma title id")
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@server/datastore/mysql/policies.go` around lines 1481 - 1485, The validation
only checks for fmaTitleID == nil and misses the sql.ErrNoRows case from the
earlier lookup, so convert the lookup that sets fmaTitleID (the sqlx.GetContext
/ query that selects si.title_id joining fleet_maintained_apps using
spec.FleetMaintainedAppSlug and teamNameToID[spec.Team]) to capture the returned
error; if errors.Is(err, sql.ErrNoRows) return a ctxerr.Wrap(ctx,
&fleet.BadRequestError{Message: fmt.Sprintf("fleet_maintained_app_slug %q is
invalid for patch policy %q", spec.FleetMaintainedAppSlug, spec.Name)})
otherwise wrap and return the original error (e.g., ctxerr.Wrap(ctx, err, "get
fma title id")), keeping the existing nil check for NULL title_id.

Copy link
Copy Markdown
Member Author

@jkatz01 jkatz01 Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter because ds.getPatchPolicyInstaller(ctx, ptr.ValOrZero(teamID), *fmaTitleID) should return a proper error if the installer is not found

installer, err := ds.getPatchPolicyInstaller(ctx, ptr.ValOrZero(teamID), *fmaTitleID)
if err != nil {
return ctxerr.Wrap(ctx, err, "getting patch policy installer")
Expand Down Expand Up @@ -1517,6 +1522,7 @@ func (ds *Datastore) ApplyPolicySpecs(ctx context.Context, authorID uint, specs
var (
shouldRemoveAllPolicyMemberships bool
removePolicyStats bool
shouldUpdatePatchPolicyName bool
)
if insertOnDuplicateDidInsertOrUpdate(res) {
// Figure out if the query, platform, software installer, VPP app, or script changed.
Expand Down Expand Up @@ -1548,7 +1554,16 @@ func (ds *Datastore) ApplyPolicySpecs(ctx context.Context, authorID uint, specs
if teamID == nil {
err = sqlx.GetContext(ctx, tx, &lastID, "SELECT id FROM policies WHERE name = ? AND team_id is NULL", spec.Name)
} else {
err = sqlx.GetContext(ctx, tx, &lastID, "SELECT id FROM policies WHERE name = ? AND team_id = ?", spec.Name, teamID)
// Patch policies are unique by patch_software_title_id so we need to get them by that, and update their name
// so that it doesn't get deleted later.
if spec.Type == fleet.PolicyTypePatch {
err = sqlx.GetContext(ctx, tx, &lastID, "SELECT id FROM policies WHERE patch_software_title_id = ? AND team_id = ?", fmaTitleID, teamID)
if _, ok := teamIDToPoliciesByName[teamID][spec.Name]; !ok {
shouldUpdatePatchPolicyName = true
}
} else {
err = sqlx.GetContext(ctx, tx, &lastID, "SELECT id FROM policies WHERE name = ? AND team_id = ?", spec.Name, teamID)
}
Comment on lines +1557 to +1566
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Rename fallback loses the interrupted-cleanup retry signal.

This branch only reloads id. The later prev.NeedsFullMembershipCleanup check still depends on teamIDToPoliciesByName[spec.Name], which is empty for a renamed patch policy, so a GitOps retry after an interrupted cleanup won't immediately rerun cleanup for that policy. The cron fallback will eventually heal it, but this retry path still regresses.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@server/datastore/mysql/policies.go` around lines 1557 - 1564, The rename
branch for patch policies only loads lastID (in the if spec.Type ==
fleet.PolicyTypePatch path) which leaves teamIDToPoliciesByName[spec.Name] empty
and breaks the later prev.NeedsFullMembershipCleanup check; after you SELECT id
using fmaTitleID/teamID (and set shouldUpdatePatchPolicyName), also load the
full previous policy row (or at least its NeedsFullMembershipCleanup value and
name) and populate teamIDToPoliciesByName[spec.Name] (or set
prev.NeedsFullMembershipCleanup) so the later logic that inspects prev by name
still sees the original policy for renamed patch policies (use the same
tx/context and the policies table query used elsewhere in policies.go).

Copy link
Copy Markdown
Member Author

@jkatz01 jkatz01 Apr 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this situation would only happen the policy was renamed again after the cleanups job interruption?
IMO it might be fine to leave a comment rather than add confusing code, but fixing it is fine too.

Edit: This seems exceedingly unlikely, considering how we expect patch policies to be used. I also couldn't get needs_full_membership_cleanup to actually be 1 on a patch policy, I attempted to change the software installer automation in gitops but it didn't do anything. That might be a different problem but probably out of scope for this fix.

Copy link
Copy Markdown
Member Author

@jkatz01 jkatz01 Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, after looking into all of this I (claude) added an extra check using that map to avoid updating the name unnecessarily (shouldn't matter anyway)

}
if err != nil {
return ctxerr.Wrap(ctx, err, "select policies id")
Expand Down Expand Up @@ -1595,6 +1610,11 @@ func (ds *Datastore) ApplyPolicySpecs(ctx context.Context, authorID uint, specs
return ctxerr.Wrap(ctx, err, "setting needs_full_membership_cleanup flag")
}
}
if shouldUpdatePatchPolicyName {
if _, err := tx.ExecContext(ctx, `UPDATE policies SET name = ?, checksum = `+policiesChecksumComputedColumn()+` WHERE id = ?`, spec.Name, policyID); err != nil {
return ctxerr.Wrap(ctx, err, "setting name for patch policy")
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// Defer cleanup outside the transaction to avoid long-held row locks on
// policy_membership.
pendingCleanups = append(pendingCleanups, policyCleanupArgs{
Expand Down
102 changes: 102 additions & 0 deletions server/datastore/mysql/policies_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7720,6 +7720,108 @@ func testTeamPatchPolicy(t *testing.T, ds *Datastore) {
require.Equal(t, "Windows - Maintained2 up to date", p5.Name)
require.Equal(t, "windows", p5.Platform)
require.Equal(t, "SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM programs WHERE name = 'Maintained2' AND version_compare(version, '1.0') < 0);", p5.Query)

//////////////////////////////////////////////
// ApplyPolicySpecs

// Test ApplyPolicySpecs with patch policies using a separate team.
team2, err := ds.NewTeam(ctx, &fleet.Team{Name: "team2"})
require.NoError(t, err)

// Set up an FMA installer on team2 for the valid slug test.
team2Payload := &fleet.UploadSoftwareInstallerPayload{
InstallScript: "hello",
PreInstallQuery: "SELECT 1",
PostInstallScript: "world",
StorageID: "storage-team2",
Filename: "maintained1-team2",
Title: "Maintained1",
Version: "1.0",
Source: "apps",
Platform: "darwin",
BundleIdentifier: "fleet.maintained1",
UserID: user1.ID,
TeamID: &team2.ID,
ValidatedLabels: &fleet.LabelIdentsWithScope{},
FleetMaintainedAppID: &maintainedApp.ID,
}
_, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, team2Payload)
require.NoError(t, err)

// ApplyPolicySpecs with type=patch and empty fleet_maintained_app_slug should return an error.
err = ds.ApplyPolicySpecs(ctx, user1.ID, []*fleet.PolicySpec{
{
Name: "patch-no-slug",
Query: "SELECT 1;",
Team: "team2",
Type: fleet.PolicyTypePatch,
},
})
require.Error(t, err)

// ApplyPolicySpecs with type=patch and a non-existent fleet_maintained_app_slug should return an error.
err = ds.ApplyPolicySpecs(ctx, user1.ID, []*fleet.PolicySpec{
{
Name: "patch-bad-slug",
Query: "SELECT 1;",
Team: "team2",
Type: fleet.PolicyTypePatch,
FleetMaintainedAppSlug: "nonexistent-slug",
},
})
require.Error(t, err)

// ApplyPolicySpecs with type=patch and a valid fleet_maintained_app_slug should succeed.
err = ds.ApplyPolicySpecs(ctx, user1.ID, []*fleet.PolicySpec{
{
Name: "patch-valid-slug",
Query: "SELECT 1;",
Team: "team2",
Type: fleet.PolicyTypePatch,
FleetMaintainedAppSlug: "maintained1",
},
})
require.NoError(t, err)

// Verify the policy was created with the expected auto-generated query and platform.
policies, _, err := ds.ListTeamPolicies(ctx, team2.ID, fleet.ListOptions{}, fleet.ListOptions{}, "")
require.NoError(t, err)
require.Len(t, policies, 1)
require.Equal(t, "patch-valid-slug", policies[0].Name)
require.Equal(t, fleet.PolicyTypePatch, policies[0].Type)
require.Equal(t, "darwin", policies[0].Platform)
require.Contains(t, policies[0].Query, "fleet.maintained1")

// Renaming a patch policy via ApplyPolicySpecs should update it, not delete it.
previousID := policies[0].ID
var previousChecksum []byte
ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error {
return sqlx.GetContext(ctx, q, &previousChecksum, `SELECT checksum FROM policies WHERE id = ?`, previousID)
})

err = ds.ApplyPolicySpecs(ctx, user1.ID, []*fleet.PolicySpec{
{
Name: "patch-renamed",
Query: "SELECT 1;",
Team: "team2",
Type: fleet.PolicyTypePatch,
FleetMaintainedAppSlug: "maintained1",
},
})
require.NoError(t, err)

policies, _, err = ds.ListTeamPolicies(ctx, team2.ID, fleet.ListOptions{}, fleet.ListOptions{}, "")
require.NoError(t, err)
require.Len(t, policies, 1)
require.Equal(t, previousID, policies[0].ID)
require.Equal(t, "patch-renamed", policies[0].Name)
require.Equal(t, fleet.PolicyTypePatch, policies[0].Type)

var newChecksum []byte
ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error {
return sqlx.GetContext(ctx, q, &newChecksum, `SELECT checksum FROM policies WHERE id = ?`, previousID)
})
require.NotEqual(t, previousChecksum, newChecksum)
}

func testTeamPolicyAutomationFilter(t *testing.T, ds *Datastore) {
Expand Down
12 changes: 12 additions & 0 deletions server/fleet/policies.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ var (
errPolicyPatchAndQuerySet = errors.New("If the \"type\" is \"patch\", the \"query\" field is not supported.")
errPolicyPatchAndPlatformSet = errors.New("If the \"type\" is \"patch\", the \"platform\" field is not supported.")
errPolicyPatchNoTitleID = errors.New("If the \"type\" is \"patch\", the \"patch_software_title_id\" field is required.")
errPatchPolicyRequiresTeam = errors.New("If the \"type\" is \"patch\", the \"team\" field is required.")
errPolicyQueryUpdated = errors.New("\"query\" can't be updated")
errPolicyPlatformUpdated = errors.New("\"platform\" can't be updated")
errPolicyConditionalAccessEnabledInvalidPlatform = errors.New("\"conditional_access_enabled\" is only valid on \"darwin\" and \"windows\" policies")
Expand Down Expand Up @@ -207,6 +208,13 @@ func verifyPolicyPlatforms(platforms string) error {
return nil
}

func verifyPatchPolicy(team string, typ string) error {
if typ == PolicyTypePatch && emptyString(team) {
return errPatchPolicyRequiresTeam
}
return nil
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

func PolicyVerifyConditionalAccess(conditionalAccessEnabled bool, platform string) error {
if conditionalAccessEnabled && !strings.Contains(platform, "darwin") && !strings.Contains(platform, "windows") {
return errPolicyConditionalAccessEnabledInvalidPlatform
Expand Down Expand Up @@ -473,6 +481,7 @@ type PolicySpec struct {

Type string `json:"type"`
FleetMaintainedAppSlug string `json:"fleet_maintained_app_slug"`
PatchSoftwareTitleID uint `json:"-"`
}

// PolicySoftwareTitle contains software title data for policies.
Expand Down Expand Up @@ -507,6 +516,9 @@ func (p PolicySpec) Verify() error {
if err := PolicyVerifyConditionalAccess(p.ConditionalAccessEnabled, p.Platform); err != nil {
return err
}
if err := verifyPatchPolicy(p.Team, p.Type); err != nil {
return err
}
return nil
}

Expand Down
13 changes: 13 additions & 0 deletions server/service/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3000,6 +3000,11 @@ func (c *Client) doGitOpsPolicies(config *spec.GitOps, teamSoftwareInstallers []
}
config.Policies[i].ScriptID = &scriptID
}

// Get patch policy title IDs for the team
for i := range config.Policies {
config.Policies[i].PatchSoftwareTitleID = softwareTitleIDsBySlug[config.Policies[i].FleetMaintainedAppSlug]
}
}

// Get the ids and names of current policies to figure out which ones to delete
Expand Down Expand Up @@ -3037,6 +3042,7 @@ func (c *Client) doGitOpsPolicies(config *spec.GitOps, teamSoftwareInstallers []
}
}
}

var policiesToDelete []uint
for _, oldItem := range policies {
found := false
Expand All @@ -3045,6 +3051,13 @@ func (c *Client) doGitOpsPolicies(config *spec.GitOps, teamSoftwareInstallers []
found = true
break
}
// patch policies are unique by patch_software_title_id so matching by name doesn't always work
if newItem.Type == fleet.PolicyTypePatch && oldItem.PatchSoftware != nil {
if oldItem.PatchSoftware.SoftwareTitleID == newItem.PatchSoftwareTitleID {
found = true
break
}
}
}
if !found {
policiesToDelete = append(policiesToDelete, oldItem.ID)
Expand Down
13 changes: 13 additions & 0 deletions server/service/integration_enterprise_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27438,6 +27438,19 @@ func (s *integrationEnterpriseTestSuite) TestPatchPolicies() {
require.Equal(t, "dynamic", getPolicyResp.Policy.Type)
require.Empty(t, getPolicyResp.Policy.PatchSoftware)
require.Empty(t, getPolicyResp.Policy.PatchSoftwareTitleID)

// Should not be able to apply global patch policy
spec := &fleet.PolicySpec{
Name: "team patch policy",
Query: "SELECT 1",
Type: fleet.PolicyTypePatch,
FleetMaintainedAppSlug: "zoom/windows",
}
applyResp := fleet.ApplyPolicySpecsResponse{}
s.DoJSON("POST", "/api/latest/fleet/spec/policies",
fleet.ApplyPolicySpecsRequest{Specs: []*fleet.PolicySpec{spec}},
http.StatusBadRequest, &applyResp,
)
})

t.Run("patch_policy_lifecycle", func(t *testing.T) {
Expand Down
Loading