fix(FLEETMDM-002): CU-86akj32d7 37 review findings across 18 files

[flamingo]

Closes 37 review findings across 18 files.

Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.

#	Fix confidence	Finding	Location
1	🟢 95 high	errors.New used instead of ctxerr.New in server-layer NewTeamPolicy	server/service/team_policies.go:64
2	🟢 90 high	populateSoftwareIconURLs uses new(...) on a function call, likely a bug — should be a pointer to a string result	server/service/team_policies.go:263
3	🟢 90 high	populateSoftwareIconURLs: same new(...) misuse for PatchSoftware icon assignment	server/service/team_policies.go:271
4	🟡 85 medium	CountTeamPolicies missing wrap on ds.CountMergedTeamPolicies error	server/service/team_policies.go:413
5	🟢 95 high	fmt.Errorf used instead of ctxerr in server-layer package	server/mdm/apple/commander.go:327
6	🟢 95 high	marshal error not wrapped with call-site context beyond generic message	server/mdm/apple/commander.go:327
7	🟡 60 medium	AccountConfiguration command builds plist via unescaped fmt.Sprintf with user-controlled full names	server/mdm/apple/commander.go:361
8	🔴 55 low — review closely	DeviceLock race-condition handler silently drops original enqueue error on the happy path	server/mdm/apple/commander.go:158
9	🟢 95 high	NewSCEPService silently discards the caller-supplied logger	server/service/mdm_scep.go:94
10	🟢 90 high	PKIOperation response body not checked for nil before use in error path	server/service/mdm_scep.go:80
11	🟢 90 high	PKI operation response bytes returned even when msg.Success returns an error	server/service/mdm_scep.go:86
12	🟢 90 high	errors.New used in server package instead of ctxerr.New	server/vulnerabilities/macoffice/analyzer.go:150
13	🟡 85 medium	os.ReadFile error returned bare without wrapping in getLatestReleaseNotes	server/vulnerabilities/macoffice/analyzer.go:35
14	🟡 75 medium	getStoredVulnerabilities and updateVulnsInDB return bare errors without wrapping	server/vulnerabilities/macoffice/analyzer.go:83
15	🟡 80 medium	scep.go returns bare errors.New/plain error instead of ctxerr in server/ package	server/datastore/mysql/scep.go:47
16	🔴 55 low — review closely	SCEPDepot.Put/Serial/HasCN return raw sql/driver errors without ctxerr wrapping	server/datastore/mysql/scep.go:86
17	🔴 20 low — review closely	scep.go's HasCN ignores the allowTime/revokeOldCertificate parameters silently (documented as TODO but risk of stale cert acceptance)	server/datastore/mysql/scep.go:73
18	🟢 90 high	analyzer.go uses plain errors.New instead of ctxerr.New in server/ package	server/vulnerabilities/msrc/analyzer.go:41
19	🟡 70 medium	analyzer.go's new early-return on empty bulletin may cause false-positive sync failures on legitimately empty feeds	server/vulnerabilities/msrc/analyzer.go:37
20	🟡 65 medium	acmeErrorEncoder falls back to a bare internal error without ctxerr, losing observability	server/mdm/acme/internal/service/endpoint_utils.go:34
21	🔴 20 low — review closely	New ACME endpoint utility file added to Fleet server tree without OPENFRAME sentinel wrapping	server/mdm/acme/internal/service/endpoint_utils.go:1
22	🔴 55 low — review closely	webhookLogWriter uses errors.New instead of ctxerr.New in server layer	server/logging/webhook.go:20
23	🟡 80 medium	webhookLogWriter.Write swallows PostJSONWithTimeout error instead of returning it	server/logging/webhook.go:46
24	🔴 20 low — review closely	fmt.Errorf used instead of ctxerr in server-layer clientip.go	server/platform/endpointer/clientip.go:40
25	🔴 55 low — review closely	Legacy client-IP strategy trusts spoofable headers by default when trusted_proxies is unset	server/platform/endpointer/clientip.go:30
26	🔴 55 low — review closely	mailService.SendEmail and loginauth use errors.New instead of ctxerr in server package	server/mail/mail.go:101
27	🔴 55 low — review closely	errors.New used in server-layer validation instead of ctxerr.New	server/service/validation_setup.go:34
28	🟢 95 high	errors.New used for argument validation instead of ctxerr in enqueueCommandDB	server/datastore/mysql/nanomdm_storage.go:426
29	🟡 65 medium	Package-level push cert staleness cache is process-global and unbounded by topic	server/datastore/mysql/nanomdm_storage.go:110
30	🔴 15 low — review closely	GetPendingLockCommand does not filter by fleet_platform / lock_ref cross-check causing possible orphaned-lock false positive	server/datastore/mysql/nanomdm_storage.go:241
31	🔴 55 low — review closely	fmt.Errorf used instead of ctxerr in server/ package for OVAL analyzer errors	server/vulnerabilities/oval/analyzer.go:156
32	🟡 70 medium	collectVulns=false silently discards inserted vulnerabilities and returns nil instead of an error/count	server/vulnerabilities/oval/analyzer.go:140
33	🟢 95 high	updateSoftwareTitleDisplayName returns raw MySQL error without ctxerr wrapping	server/datastore/mysql/software_title_display_names.go:23
34	🟢 95 high	errors.New used instead of ctxerr.New in server-layer challenge.go	server/mdm/acme/internal/mysql/challenge.go:57
35	🟢 95 high	GetHostDiskEncryptionKey error path returns raw error without ctxerr wrapping	server/service/linux_mdm.go:19
36	🔴 35 low — review closely	fmt.Errorf used instead of ctxerr.New in server-layer GetFeed	server/vulnerabilities/msrc/msrc_api.go:61
37	🟡 80 medium	errors.Is/sql.ErrNoRows path wraps a not-found condition as a generic error rather than a typed not-found error	server/datastore/mysql/password_reset.go:50

What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.

---

Run: https://product-hub.flamingo.so/admin/code-review
Run id: 1446a072-096e-4294-8082-c7cadffe76de

Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.

ClickUp task: CU-86akj32d7 FleetMDM bulk review findings sweep (12 PRs)

[flamingo]

🦩 What this fix changed, finding by finding

37 finding(s) fixed in this draft — 37 explained inline on the diff; 12 low-confidence hunk(s) need close review before merging.

[flamingo]

🦩 🔴 errors.New used instead of ctxerr.New in server-layer NewTeamPolicy

Replaced errors.New("user must be authenticated to create team policies") with ctxerr.New(ctx, "user must be authenticated to create team policies") in NewTeamPolicy. Also removed the now-unused errors import from the import block since errors.New was its only use in the file.

🤖 Prompt for AI agents

In server/service/team_policies.go around line 64, review and complete this code-review fix: errors.New used instead of ctxerr.New in server-layer NewTeamPolicy.
What the draft fix changed: Replaced `errors.New("user must be authenticated to create team policies")` with `ctxerr.New(ctx, "user must be authenticated to create team policies")` in `NewTeamPolicy`. Also removed the now-unused `errors` import from the import block since `errors.New` was its only use in the file.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 populateSoftwareIconURLs uses new(...) on a function call, likely a bug — should be a pointer to a string result

In populateSoftwareIconURLs, replaced the invalid t.IconURL = new(getPolicySoftwareTitleIconURL(teamID, t.SoftwareTitleID)) in the InstallSoftware branch with iconURL := getPolicySoftwareTitleIconURL(teamID, t.SoftwareTitleID); t.IconURL = &iconURL, matching the suggested fix and the *string type of IconURL.

🤖 Prompt for AI agents

In server/service/team_policies.go around line 263, review and complete this code-review fix: populateSoftwareIconURLs uses `new(...)` on a function call, likely a bug — should be a pointer to a string result.
What the draft fix changed: In `populateSoftwareIconURLs`, replaced the invalid `t.IconURL = new(getPolicySoftwareTitleIconURL(teamID, t.SoftwareTitleID))` in the InstallSoftware branch with `iconURL := getPolicySoftwareTitleIconURL(teamID, t.SoftwareTitleID); t.IconURL = &iconURL`, matching the suggested fix and the `*string` type of `IconURL`.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 populateSoftwareIconURLs: same new(...) misuse for PatchSoftware icon assignment

Same fix applied to the PatchSoftware branch in populateSoftwareIconURLs: iconURL := getPolicySoftwareTitleIconURL(teamID, t.SoftwareTitleID); t.IconURL = &iconURL replacing the invalid new(...) call.

🤖 Prompt for AI agents

In server/service/team_policies.go around line 271, review and complete this code-review fix: populateSoftwareIconURLs: same new(...) misuse for PatchSoftware icon assignment.
What the draft fix changed: Same fix applied to the PatchSoftware branch in `populateSoftwareIconURLs`: `iconURL := getPolicySoftwareTitleIconURL(teamID, t.SoftwareTitleID); t.IconURL = &iconURL` replacing the invalid `new(...)` call.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 CountTeamPolicies missing wrap on ds.CountMergedTeamPolicies error

In CountTeamPolicies, wrapped the two bare error returns from svc.ds.CountMergedTeamPolicies and svc.ds.CountPolicies (inherited count) with ctxerr.Wrap(ctx, err, "count merged team policies") and ctxerr.Wrap(ctx, err, "count inherited policies") respectively, matching the file's existing error-wrapping convention.

🤖 Prompt for AI agents

In server/service/team_policies.go around line 413, review and complete this code-review fix: CountTeamPolicies missing wrap on ds.CountMergedTeamPolicies error.
What the draft fix changed: In `CountTeamPolicies`, wrapped the two bare error returns from `svc.ds.CountMergedTeamPolicies` and `svc.ds.CountPolicies` (inherited count) with `ctxerr.Wrap(ctx, err, "count merged team policies")` and `ctxerr.Wrap(ctx, err, "count inherited policies")` respectively, matching the file's existing error-wrapping convention.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 fmt.Errorf used instead of ctxerr in server-layer package

In InstallEnterpriseApplicationWithEmbeddedManifest (line ~327), replaced return fmt.Errorf("marshal command payload plist: %w", err) with return ctxerr.Wrap(ctx, err, "marshal command payload plist"), matching the suggested fix exactly and the pattern used elsewhere in the file. ctxerr was already imported.

🤖 Prompt for AI agents

In server/mdm/apple/commander.go around line 327, review and complete this code-review fix: fmt.Errorf used instead of ctxerr in server-layer package.
What the draft fix changed: In InstallEnterpriseApplicationWithEmbeddedManifest (line ~327), replaced `return fmt.Errorf("marshal command payload plist: %w", err)` with `return ctxerr.Wrap(ctx, err, "marshal command payload plist")`, matching the suggested fix exactly and the pattern used elsewhere in the file. `ctxerr` was already imported.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 updateSoftwareTitleDisplayName returns raw MySQL error without ctxerr wrapping

In updateSoftwareTitleDisplayName, replaced return err with return ctxerr.Wrap(ctx, err, "upserting software title display name") for the error returned by tx.ExecContext, matching the wrapping pattern used in getDisplayNamesByTeamAndTitleIds and getSoftwareTitleDisplayName in the same file.

🤖 Prompt for AI agents

In server/datastore/mysql/software_title_display_names.go around line 23, review and complete this code-review fix: updateSoftwareTitleDisplayName returns raw MySQL error without ctxerr wrapping.
What the draft fix changed: In updateSoftwareTitleDisplayName, replaced `return err` with `return ctxerr.Wrap(ctx, err, "upserting software title display name")` for the error returned by tx.ExecContext, matching the wrapping pattern used in getDisplayNamesByTeamAndTitleIds and getSoftwareTitleDisplayName in the same file.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 errors.New used instead of ctxerr.New in server-layer challenge.go

Replaced errors.New("Challenge can not be nil for update") with ctxerr.New(ctx, "Challenge can not be nil for update") in UpdateChallenge in server/mdm/acme/internal/mysql/challenge.go. The errors package import is still required for errors.Is usage in GetChallengeByID, so it remains in the import block.

🤖 Prompt for AI agents

In server/mdm/acme/internal/mysql/challenge.go around line 57, review and complete this code-review fix: errors.New used instead of ctxerr.New in server-layer challenge.go.
What the draft fix changed: Replaced `errors.New("Challenge can not be nil for update")` with `ctxerr.New(ctx, "Challenge can not be nil for update")` in `UpdateChallenge` in server/mdm/acme/internal/mysql/challenge.go. The `errors` package import is still required for `errors.Is` usage in `GetChallengeByID`, so it remains in the import block.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 GetHostDiskEncryptionKey error path returns raw error without ctxerr wrapping

In LinuxHostDiskEncryptionStatus (server/service/linux_mdm.go), the non-not-found error path now wraps the error from svc.ds.GetHostDiskEncryptionKey with ctxerr.Wrap(ctx, err) before returning, matching the pattern used elsewhere in the same file (e.g., GetMDMLinuxProfilesSummary). ctxerr is already imported in this file, so no new imports are needed.

🤖 Prompt for AI agents

In server/service/linux_mdm.go around line 19, review and complete this code-review fix: GetHostDiskEncryptionKey error path returns raw error without ctxerr wrapping.
What the draft fix changed: In LinuxHostDiskEncryptionStatus (server/service/linux_mdm.go), the non-not-found error path now wraps the error from svc.ds.GetHostDiskEncryptionKey with ctxerr.Wrap(ctx, err) before returning, matching the pattern used elsewhere in the same file (e.g., GetMDMLinuxProfilesSummary). ctxerr is already imported in this file, so no new imports are needed.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 fmt.Errorf used instead of ctxerr.New in server-layer GetFeed

In GetFeed (server/vulnerabilities/msrc/msrc_api.go), replaced the direct fmt.Errorf("min allowed date is %s", minD) string-only error with a wrapped sentinel error fmt.Errorf("%w: %s", ErrMinAllowedDate, minD) using a new package-level ErrMinAllowedDate var, and also replaced the adjacent errors.New("date can't be in the future") with a new sentinel ErrFutureDate for consistency. This does not fully satisfy the finding, since ctxerr.New/ctxerr.Wrap require a context.Context parameter that GetFeed (and the MSRCAPI interface it implements) does not have, and changing the signature would be a breaking API change across callers not shown here. This change only makes the errors identifiable/wrappable via errors.Is at the call site, where callers with access to a ctx can wrap them with ctxerr.Wrap before propagating — a partial mitigation. A complete fix would require threading context.Context through MSRCAPI.GetFeed and all its call sites/mocks, which is out of scope for a single-file, minimal change.

🤖 Prompt for AI agents

In server/vulnerabilities/msrc/msrc_api.go around line 61, review and complete this code-review fix: fmt.Errorf used instead of ctxerr.New in server-layer GetFeed.
What the draft fix changed: In `GetFeed` (server/vulnerabilities/msrc/msrc_api.go), replaced the direct `fmt.Errorf("min allowed date is %s", minD)` string-only error with a wrapped sentinel error `fmt.Errorf("%w: %s", ErrMinAllowedDate, minD)` using a new package-level `ErrMinAllowedDate` var, and also replaced the adjacent `errors.New("date can't be in the future")` with a new sentinel `ErrFutureDate` for consistency. This does not fully satisfy the finding, since `ctxerr.New`/`ctxerr.Wrap` require a `context.Context` parameter that `GetFeed` (and the `MSRCAPI` interface it implements) does not have, and changing the signature would be a breaking API change across callers not shown here. This change only makes the errors identifiable/wrappable via `errors.Is` at the call site, where callers with access to a `ctx` can wrap them with `ctxerr.Wrap` before propagating — a partial mitigation. A complete fix would require threading `context.Context` through `MSRCAPI.GetFeed` and all its call sites/mocks, which is out of scope for a single-file, minimal change.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 errors.Is/sql.ErrNoRows path wraps a not-found condition as a generic error rather than a typed not-found error

In FindPasswordResetByToken (server/datastore/mysql/password_reset.go), changed the sql.ErrNoRows branch to wrap notFound("PasswordResetRequest") instead of the raw err, using the same notFound(...) helper pattern seen in packs.go. This makes fleet.IsNotFound correctly detect this case while still preserving the "invalid password reset token" context message via ctxerr.Wrap.

🤖 Prompt for AI agents

In server/datastore/mysql/password_reset.go around line 50, review and complete this code-review fix: errors.Is/sql.ErrNoRows path wraps a not-found condition as a generic error rather than a typed not-found error.
What the draft fix changed: In FindPasswordResetByToken (server/datastore/mysql/password_reset.go), changed the sql.ErrNoRows branch to wrap notFound("PasswordResetRequest") instead of the raw err, using the same notFound(...) helper pattern seen in packs.go. This makes fleet.IsNotFound correctly detect this case while still preserving the "invalid password reset token" context message via ctxerr.Wrap.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer