fix(FLEETMDM-002): CU-86akbhhtv 93 review findings across 36 files

[flamingo]

Closes 93 review findings across 36 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	🔴 55 low — review closely	updateAppConfigJSON uses github.com/pkg/errors.Wrap instead of ctxerr in server/ package	server/datastore/mysql/migrations/tables/migration.go:230
2	🟡 60 medium	indexExists and indexExistsTx are near-identical duplicated functions differing only by *sqlx.DB vs *sql.Tx	server/datastore/mysql/migrations/tables/migration.go:196
3	🔴 50 low — review closely	errors.Wrap swallows nil error into non-nil wrapped error in basicMigrationStep	server/datastore/mysql/migrations/tables/migration.go:29
4	🔴 30 low — review closely	Progress goroutine in incrementalMigrationStep can print a final tick after stepComplete is signalled, racing on outputTo	server/datastore/mysql/migrations/tables/migration.go:64
5	🔴 45 low — review closely	fkExists/constraintExists/columnExists/tableExists/indexExists silently swallow SQL errors as 'does not exist'	server/datastore/mysql/migrations/tables/migration.go:108
6	🟢 90 high	Stale/garbled comment in incrementalMigrationStep	server/datastore/mysql/migrations/tables/migration.go:62
7	🔴 40 low — review closely	tableExists queries information_schema.columns rather than information_schema.tables	server/datastore/mysql/migrations/tables/migration.go:175
8	🟢 90 high	errors.New used instead of ctxerr in server/datastore/mysql package	server/datastore/mysql/nanomdm_storage.go:179
9	🟢 90 high	errors.New used instead of ctxerr in NanoDEPStorage.StoreAuthTokens	server/datastore/mysql/nanomdm_storage.go:416
10	🔴 55 low — review closely	New MDM lock/unlock/wipe logic added to upstream nanomdm_storage.go without OPENFRAME sentinel comments	server/datastore/mysql/nanomdm_storage.go:26
11	🟡 70 medium	Bare return err without wrapping in enqueueCommandDB call sites	server/datastore/mysql/nanomdm_storage.go:259
12	🔴 15 low — review closely	In-memory push-cert staleness cache is not pod-safe in a multi-replica deployment	server/datastore/mysql/nanomdm_storage.go:108
13	🟡 85 medium	Bare errors.New used instead of ctxerr.New in ee/server/service/hostidentity/depot/depot.go	ee/server/service/hostidentity/depot/depot.go:88
14	🟡 75 medium	fmt.Errorf used instead of ctxerr.Wrap in depot.go CA/Serial/Put methods	ee/server/service/hostidentity/depot/depot.go:53
15	🟡 80 medium	fmt.Errorf used for pubKeyRaw creation failure instead of ctxerr.Wrap	ee/server/service/hostidentity/depot/depot.go:103
16	🔴 20 low — review closely	Rate-limit check in Put() is TOCTOU-vulnerable: existence check and insert/revoke are not in the same transaction	ee/server/service/hostidentity/depot/depot.go:110
17	🔴 30 low — review closely	HasCN in depot.go is a stub that always returns false, silently disabling SCEP renewal semantics	ee/server/service/hostidentity/depot/depot.go:81
18	🟡 85 medium	Plain errors.New used in server/service/carves.go instead of ctxerr	server/service/carves.go:96
19	🟡 85 medium	errors.New used for request_id mismatch instead of ctxerr.New	server/service/carves.go:407
20	🟢 90 high	base64 decode buffer sized with RawStdEncoding but decoded with StdEncoding, risking buffer under-allocation	server/service/carves.go:351
21	🟡 80 medium	Carve-block streaming parser reads raw bytes from request body without checking for short reads	server/service/carves.go:261
22	🔴 20 low — review closely	CarveBlock ownership check silently no-ops when allow_body_auth_fallback defaults to true, weakening cross-host carve protection	server/service/carves.go:398
23	🟢 90 high	Plain errors.New used in server-layer mdm_scep.go instead of ctxerr	server/service/mdm_scep.go:69
24	🟢 90 high	errors.New used for 'no signed certificate' instead of ctxerr	server/service/mdm_scep.go:78
25	🟢 90 high	GetNextCACert returns raw errors.New instead of ctxerr.New	server/service/mdm_scep.go:91
26	🟢 90 high	NewSCEPService discards its logger parameter and always uses a discard handler	server/service/mdm_scep.go:95
27	🟡 70 medium	fmt.Errorf used instead of ctxerr in server-layer encryption service	server/service/openframe/openframe-encryption-service.go:47
28	🔴 45 low — review closely	GCM nonce reuse risk not addressed and no key length validation before AES cipher creation	server/service/openframe/openframe-encryption-service.go:33
29	🟡 65 medium	AES cipher and GCM construction errors returned unwrapped in Decrypt	server/service/openframe/openframe-encryption-service.go:33
30	🔴 55 low — review closely	decryptErrCount and readErrCount are not concurrency-safe despite being mutated from potentially concurrent callers	server/service/openframe/openframe-encryption-service.go:12
31	🟡 85 medium	errors.New used for domain error instead of ctxerr in server package	ee/server/service/condaccess/depot/depot.go:60
32	🟡 85 medium	errors.New used instead of ctxerr in Put() server-layer certificate handling	ee/server/service/condaccess/depot/depot.go:92
33	🟢 90 high	context.TODO() used instead of the request-scoped context in Put()	ee/server/service/condaccess/depot/depot.go:154
34	🔴 25 low — review closely	Rate-limit check-then-insert in Put() is not atomic, allowing a TOCTOU race to bypass cooldown	ee/server/service/condaccess/depot/depot.go:114
35	🟡 85 medium	Plain errors.New/fmt.Errorf used instead of ctxerr in server layer	server/service/packs.go:28
36	🟡 80 medium	fmt.Errorf used for domain error instead of ctxerr in DeletePack	server/service/packs.go:407
37	🟡 60 medium	DeletePack nil-pack dereference risk when pack lookup succeeds but pack is nil	server/service/packs.go:402
38	🔴 55 low — review closely	Bare error propagation without context wrapping in DeletePack	server/service/packs.go:131
39	🟢 95 high	GetCertificateTemplate returns raw datastore error without ctxerr.Wrap	server/service/certificates.go:329
40	🔴 55 low — review closely	New certificate-template endpoints added to upstream fleet server package without OPENFRAME sentinel comments	server/service/certificates.go:1
41	🟡 60 medium	SkipAuthorization called on the error path before returning, masking a potential authz issue	server/service/certificates.go:329
42	🟢 95 high	errors.New used instead of ctxerr in server/service/global_policies.go	server/service/global_policies.go:57
43	🟢 95 high	errors.New used instead of ctxerr in ApplyPolicySpecs authentication check	server/service/global_policies.go:458
44	🔴 35 low — review closely	OPENFRAME sentinel wraps unrelated upstream policy-deletion authorization logic	server/service/global_policies.go:178
45	🔴 55 low — review closely	Test comment asserts ownership failure must bypass ctxerr.Wrap, contradicting FLEETMDM-002	server/service/carves_test.go:465
46	🔴 45 low — review closely	New OpenFrame-specific ownership check in carves_test.go lacks OPENFRAME sentinel comments	server/service/carves_test.go:426
47	🔴 40 low — review closely	Host ownership check for CarveBlock relies on optional context and is easily bypassed	server/service/carves_test.go:518
48	🟢 92 high	notFound() error path bypasses ctxerr wrap chain, mixing raw errors with ctxerr in certificate_authorities.go	server/datastore/mysql/certificate_authorities.go:54
49	🔴 35 low — review closely	New CertificateAuthority feature added to upstream Fleet schema without OPENFRAME sentinel comments	server/datastore/mysql/certificate_authorities.go:1
50	🔴 50 low — review closely	UpdateCertificateAuthorityByID compares sameName incorrectly when ca.Name is nil vs when unchanged	server/datastore/mysql/certificate_authorities.go:429
51	🟡 85 medium	doFetch and related helpers use fmt.Errorf/errors.New instead of ctxerr in server package	server/mdm/maintainedapps/sync.go:76
52	🟡 70 medium	doFetch uses errors.New for a 404 case instead of wrapping with call context	server/mdm/maintainedapps/sync.go:95
53	🔴 20 low — review closely	doFetch treats any HTTP status other than 200/404 as an error, silently excluding 2xx/3xx handled elsewhere in DownloadInstaller	server/mdm/maintainedapps/sync.go:92
54	🟡 75 medium	Server-layer chart service returns raw *platform_http.BadRequestError instead of ctxerr-wrapped errors	server/chart/internal/service/service.go:102
55	🔴 55 low — review closely	CollectDatasets swallows every dataset collection error and always returns nil	server/chart/internal/service/service.go:48
56	🔴 20 low — review closely	entityIDs is declared but never assigned, always passed as nil to GetSCDData	server/chart/internal/service/service.go:141
57	🟡 85 medium	WSTEPStoreCertificate and WSTEPNewSerial return bare errors instead of ctxerr-wrapped errors	server/datastore/mysql/wstep.go:24
58	🔴 30 low — review closely	WSTEPNewSerial has unresolved TODOs regarding serial number safety in production path	server/datastore/mysql/wstep.go:47
59	🟡 85 medium	Bare error return from AppConfig in validateReadyForLinuxEscrow bypasses ctxerr convention	ee/server/service/devices.go:243
60	🔴 55 low — review closely	Bare error returns without wrapping in devices.go BypassConditionalAccess-adjacent helper paths	ee/server/service/devices.go:233
61	🟡 60 medium	fmt.Errorf used instead of ctxerr in validateCertificateTemplateFleetVariables (server layer)	server/service/certificate_templates.go:36
62	🟡 75 medium	certificate SAN validation only checks emptiness of value, not leading/trailing whitespace inside multi-value SAN entries like IP lists	server/service/certificate_templates.go:92
63	🟡 85 medium	errors.New used for runtime error in runLiveQueryOnHost instead of ctxerr	server/service/live_queries.go:186
64	🟢 90 high	Bare error return without %w wrapping in GetCampaignReader	server/service/live_queries.go:383
65	🔴 55 low — review closely	cmd/msrc/generate.go uses panic instead of ctxerr in a repo-wide error convention violation	cmd/msrc/generate.go:19
66	🟡 70 medium	MSRC generate.go: cleanEnvVar check inverted — 'MSRC_CLEAN' semantics look backwards	cmd/msrc/generate.go:53
67	🟡 85 medium	server/ package uses fmt.Errorf instead of ctxerr.New despite having ctx in scope	ee/server/service/calendar.go:24
68	🔴 40 low — review closely	Package-level asyncCalendarProcessing flag combined with goroutine leaks event context across requests	ee/server/service/calendar.go:16
69	🔴 55 low — review closely	errors.New used for constructor validation instead of ctxerr	server/logging/webhook.go:19
70	🟡 60 medium	webhookLogWriter.Write logs and swallows send failure without ctxerr, losing observability	server/logging/webhook.go:46
71	🔴 55 low — review closely	downloadRemoteMacosBootstrapPackage in server/service uses fmt.Errorf/errors.New instead of ctxerr	server/service/client_mdm.go:196
72	🟡 60 medium	UploadBootstrapPackage silently discards a parsed response value it never uses	server/service/client_mdm.go:147
73	🔴 55 low — review closely	errors.New used instead of ctxerr.New in server-layer validation code	server/service/validation_setup.go:34
74	🔴 30 low — review closely	ValidateServerURL has a known-incomplete TODO for URL validation	server/service/validation_setup.go:30
75	🟢 95 high	upsertHostOperatingSystemDB returns bare DB error instead of ctxerr.Wrap	server/datastore/mysql/operating_systems.go:177
76	🟢 90 high	HostFeatures returns bare datastore errors instead of ctxerr.Wrap	ee/server/service/appconfig.go:16
77	🟢 95 high	Plain errors.New used in server-layer InviteNewUser instead of ctxerr.New	server/service/invites.go:65
78	🟢 90 high	Bare error return in server/ package bypasses ctxerr wrapping	server/mdm/acme/internal/mysql/authorization.go:39
79	🟡 70 medium	commonFailingStore.Sign uses fmt.Errorf instead of ctxerr	server/datastore/failing/common_store.go:33
80	🔴 40 low — review closely	New errors in ee/server/service/vpp_users.go use ctxerr correctly, but errMissingManagedAppleID sentinel bypasses ctxerr.New	ee/server/service/vpp_users.go:18
81	🟡 60 medium	ee/server/licensing/licensing.go uses errors.New instead of ctxerr in server-layer package	ee/server/licensing/licensing.go:26
82	🔴 55 low — review closely	Multiple errors.New calls in licensing.go's validate() bypass ctxerr	ee/server/licensing/licensing.go:90
83	🟡 70 medium	server-layer scep.go uses errors.New/fmt.Errorf instead of ctxerr in several places despite having ctx available	ee/server/service/hostidentity/scep.go:117
84	🔴 45 low — review closely	renewalMiddleware ignores UpdateHostIdentityCertHostIDBySerial failure and still returns the new cert, leaving host_id unset silently	ee/server/service/hostidentity/scep.go:204
85	🟡 85 medium	challengeMiddleware uses plain errors.New instead of ctxerr in server-layer code	ee/server/service/condaccess/scep.go:96
86	🔴 55 low — review closely	PKIOperation returns certRep.Raw even when msg.Fail/msg.Success itself errored	ee/server/service/condaccess/scep.go:195
87	🟢 95 high	vpp_verification.go verifyVPPInstalls wraps errors correctly but Run() double-wraps the returned error with a misleading message	server/worker/vpp_verification.go:43
88	🟢 92 high	chartacl.FleetViewerAdapter.ViewerScope returns errors.New instead of ctxerr.New despite having ctx available	server/acl/chartacl/fleet_adapter.go:39

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: 3ba0fd37-c226-436b-ba81-b544a7d0a515

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-86akbhhtv FleetMDM bulk review findings sweep (10 PRs)