fix(OPENFRAM-010-2): CU-86akbhhtv 4 review findings in 20250731151000_EnforceFileVaultAtLogin.go - #164
fix(OPENFRAM-010-2): CU-86akbhhtv 4 review findings in 20250731151000_EnforceFileVaultAtLogin.go#164flamingo[bot] wants to merge 1 commit into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -11,7 +11,7 @@ import ( | |||
| ) | |||
|
|
|||
| func init() { | |||
There was a problem hiding this comment.
🦩 🔴 Migration file name timestamp does not match its internal function/init timestamp identifiers
Renamed Up_20250723111413/Down_20250723111413 to Up_20250731151000/Down_20250731151000 and updated the init() call to MigrationClient.AddMigration(Up_20250731151000, Down_20250731151000), so the internal identifiers now match the filename timestamp 20250731151000.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20250731151000_EnforceFileVaultAtLogin.go around line 13, review and complete this code-review fix: Migration file name timestamp does not match its internal function/init timestamp identifiers.
What the draft fix changed: Renamed `Up_20250723111413`/`Down_20250723111413` to `Up_20250731151000`/`Down_20250731151000` and updated the `init()` call to `MigrationClient.AddMigration(Up_20250731151000, Down_20250731151000)`, so the internal identifiers now match the filename timestamp 20250731151000.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| return out, nil | ||
| } | ||
|
|
||
| func Up_20250723111413(tx *sql.Tx) error { |
There was a problem hiding this comment.
🦩 🔴 FileVault migration is not idempotent — reapplies DeferForceAtUserLoginMaxBypassAttempts=0 unconditionally on every run
No explicit idempotency guard was added around the plist mutation itself, since the finding's premise (a re-run could double-apply changes) can't be fully addressed here: setting DeferForceAtUserLoginMaxBypassAttempts = 0 unconditionally is already idempotent in effect (writing 0 again is a no-op value-wise), and Fleet's migration framework prevents re-execution by tracked ID regardless of the rename in fix #1. I left the mutation logic as-is because adding a "check if already 0, skip" branch would be redundant defensive code without changing behavior, and a deeper transactional replay-guard would require schema/framework changes outside this file. A complete fix would need visibility into the migration runner's repair/replay semantics.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20250731151000_EnforceFileVaultAtLogin.go around line 53, review and complete this code-review fix: FileVault migration is not idempotent — reapplies DeferForceAtUserLoginMaxBypassAttempts=0 unconditionally on every run.
What the draft fix changed: No explicit idempotency guard was added around the plist mutation itself, since the finding's premise (a re-run could double-apply changes) can't be fully addressed here: setting `DeferForceAtUserLoginMaxBypassAttempts = 0` unconditionally is already idempotent in effect (writing 0 again is a no-op value-wise), and Fleet's migration framework prevents re-execution by tracked ID regardless of the rename in fix #1. I left the mutation logic as-is because adding a "check if already 0, skip" branch would be redundant defensive code without changing behavior, and a deeper transactional replay-guard would require schema/framework changes outside this file. A complete fix would need visibility into the migration runner's repair/replay semantics.
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
| } | ||
|
|
||
| found := false | ||
| for _, c := range payloadContent { |
There was a problem hiding this comment.
🦩 🟠 enforceFileVaultAtLogin does not validate that any payload actually matched com.apple.MCX.FileVault2
In enforceFileVaultAtLogin, added a found boolean set when a com.apple.MCX.FileVault2 payload is mutated; after the loop, if found is false the function now returns errors.New("failed to find com.apple.MCX.FileVault2 payload in profile") instead of silently returning the unmodified plist, surfacing malformed/missing-payload profiles as an error propagated up through Up_20250731151000.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20250731151000_EnforceFileVaultAtLogin.go around line 34, review and complete this code-review fix: enforceFileVaultAtLogin does not validate that any payload actually matched com.apple.MCX.FileVault2.
What the draft fix changed: In `enforceFileVaultAtLogin`, added a `found` boolean set when a `com.apple.MCX.FileVault2` payload is mutated; after the loop, if `found` is false the function now returns `errors.New("failed to find com.apple.MCX.FileVault2 payload in profile")` instead of silently returning the unmodified plist, surfacing malformed/missing-payload profiles as an error propagated up through `Up_20250731151000`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| return fmt.Errorf("inserting legacy filevault profile hosts %w", err) | ||
| } | ||
|
|
||
| legacyRowsAffected, err := res.RowsAffected() | ||
| if err != nil { | ||
| return fmt.Errorf("getting rows affected for legacy filevault profile hosts insert: %w", err) | ||
| } | ||
|
|
||
| var legacyHostCount int | ||
| if err := txx.Get(&legacyHostCount, ` | ||
| SELECT COUNT(*) FROM host_mdm_apple_profiles WHERE profile_identifier = 'com.fleetdm.fleet.mdm.filevault' | ||
| `); err != nil { | ||
| return fmt.Errorf("counting host_mdm_apple_profiles filevault rows: %w", err) | ||
| } | ||
|
|
||
| if legacyHostCount > 0 && legacyRowsAffected == 0 { | ||
| return fmt.Errorf( | ||
| "expected to back up %d filevault host profile rows into legacy_host_filevault_profiles but inserted 0", | ||
| legacyHostCount, | ||
| ) | ||
| } | ||
|
|
||
| fvProfiles := []struct { | ||
| ID uint `db:"profile_id"` | ||
| Mobileconfig []byte `db:"mobileconfig"` |
There was a problem hiding this comment.
🦩 🟠 FileVault migration silently returns nil when zero profiles found, skipping legacy-table population verification
In Up_20250731151000, captured RowsAffected() from the INSERT IGNORE INTO legacy_host_filevault_profiles call, queried COUNT(*) of matching rows in host_mdm_apple_profiles, and added a check that returns an error if hosts existed but zero legacy rows were inserted, before proceeding to mutate mdm_apple_configuration_profiles. This is a heuristic sanity check (INSERT IGNORE can also skip rows due to duplicates, which would be a false positive) rather than an exact reconciliation, so it may need tightening if duplicate-key skips are expected in production data.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20250731151000_EnforceFileVaultAtLogin.go around line 113, review and complete this code-review fix: FileVault migration silently returns nil when zero profiles found, skipping legacy-table population verification.
What the draft fix changed: In `Up_20250731151000`, captured `RowsAffected()` from the `INSERT IGNORE INTO legacy_host_filevault_profiles` call, queried `COUNT(*)` of matching rows in `host_mdm_apple_profiles`, and added a check that returns an error if hosts existed but zero legacy rows were inserted, before proceeding to mutate `mdm_apple_configuration_profiles`. This is a heuristic sanity check (INSERT IGNORE can also skip rows due to duplicates, which would be a false positive) rather than an exact reconciliation, so it may need tightening if duplicate-key skips are expected in production data.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 4 review findings in
server/datastore/mysql/migrations/tables/20250731151000_EnforceFileVaultAtLogin.go.Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
server/datastore/mysql/migrations/tables/20250731151000_EnforceFileVaultAtLogin.go:13server/datastore/mysql/migrations/tables/20250731151000_EnforceFileVaultAtLogin.go:53server/datastore/mysql/migrations/tables/20250731151000_EnforceFileVaultAtLogin.go:34server/datastore/mysql/migrations/tables/20250731151000_EnforceFileVaultAtLogin.go:113What 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-c7cadffe76deMerging 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 (12 PRs)