Skip to content

fix(OPENFRAM-010-2): CU-86akbhhtv 4 review findings in 20250731151000_EnforceFileVaultAtLogin.go #164

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

Draft
flamingo wants to merge 1 commit into
base: main
Choose a base branch
from
+29 −4
Draft

fix(OPENFRAM-010-2): CU-86akbhhtv 4 review findings in 20250731151000_EnforceFileVaultAtLogin.go #164

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 29 additions & 4 deletions server/datastore/mysql/migrations/tables/20250731151000_EnforceFileVaultAtLogin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
)

func init() {

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 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

MigrationClient.AddMigration(Up_20250723111413, Down_20250723111413)
MigrationClient.AddMigration(Up_20250731151000, Down_20250731151000)
}

// enforceFileVaultAtLogin is used to set
Expand All @@ -31,6 +31,7 @@ func enforceFileVaultAtLogin(original []byte) ([]byte, error) {
return nil, errors.New("failed to access PayloadContent element")
}

found := false
for _, c := range payloadContent {

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 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

payload, ok := c.(map[string]interface{})
if !ok {
Expand All @@ -39,9 +40,14 @@ func enforceFileVaultAtLogin(original []byte) ([]byte, error) {

if payload["PayloadType"] == "com.apple.MCX.FileVault2" {
payload["DeferForceAtUserLoginMaxBypassAttempts"] = 0
found = true
}
}

if !found {
return nil, errors.New("failed to find com.apple.MCX.FileVault2 payload in profile")
}

out, err := plist.MarshalIndent(configuration, " ")
if err != nil {
return nil, fmt.Errorf("failed to marshal new payload: %w", err)
Expand All @@ -50,7 +56,7 @@ func enforceFileVaultAtLogin(original []byte) ([]byte, error) {
return out, nil
}

func Up_20250723111413(tx *sql.Tx) error {

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 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

func Up_20250731151000(tx *sql.Tx) error {
// Idempotent migration.
txx := sqlx.Tx{Tx: tx, Mapper: reflectx.NewMapperFunc("db", sqlx.NameMapper)}

Expand Down Expand Up @@ -79,7 +85,7 @@ CREATE TABLE IF NOT EXISTS legacy_host_filevault_profiles (
return err
}

_, err = txx.Exec(`
res, err := txx.Exec(`
INSERT IGNORE INTO legacy_host_filevault_profiles
(host_uuid, status, operation_type, profile_uuid, detail, command_uuid, scope, created_at, updated_at)
SELECT
Expand All @@ -99,6 +105,25 @@ CREATE TABLE IF NOT EXISTS legacy_host_filevault_profiles (
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"`
Comment on lines 105 to 129

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 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

Expand Down Expand Up @@ -132,6 +157,6 @@ CREATE TABLE IF NOT EXISTS legacy_host_filevault_profiles (
return nil
}

func Down_20250723111413(tx *sql.Tx) error {
func Down_20250731151000(tx *sql.Tx) error {
return nil
}