SAAD: GitOps for DDM assets#49046
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai stop review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Adds GitOps + API support for Apple Declarative Device Management (DDM) assets, including server-side asset CRUD/batch endpoints, GitOps parsing/application ordering, and reconciliation/token changes so asset edits trigger host resyncs even when declarations are unchanged.
Changes:
- Introduces Apple DDM asset API endpoints (
/fleet/assets+ batch) with authz, datastore support, and fleetctl client methods. - Extends DDM reconciliation/tokening to incorporate
assets_updated_at(and serves referenced assets via the DDM “declaration-items”/“declaration/asset/…” paths). - Updates GitOps parsing and
fleetctl generate-gitopsto round-tripcontrols.apple_settings.assets.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/mdm.go | Batch profile upload now resolves and attaches declaration→asset reference UUIDs. |
| server/service/integration_mdm_profiles_test.go | Updates expected error message for forbidden declaration types in batch endpoint. |
| server/service/integration_mdm_ddm_test.go | Adds integration coverage for asset-only updates triggering DDM resync + token changes; updates EffectiveDDMToken call sites. |
| server/service/integration_core_test.go | Updates NewMDMAppleDeclaration call signature in integration test. |
| server/service/handler.go | Registers new /fleet/assets CRUD + batch routes under MDM middleware. |
| server/service/client.go | Applies assets before declarations/profiles in GitOps; adds local asset file loading helper + asset spec extraction. |
| server/service/client_teams.go | Adds GitOps client helper to batch-set assets for a team. |
| server/service/client_profiles.go | Adds client methods to list/download DDM assets (for generate-gitops). |
| server/service/apple_mdm.go | Adds asset reference discovery for declarations; adds FOSS stubs for asset endpoints; serves DDM asset declarations to devices. |
| server/service/apple_mdm_test.go | Updates service unit tests for new declaration signature + asset reference validation. |
| server/service/apple_mdm_ddm_test.go | Updates declaration creation calls for new signature. |
| server/mock/service/service_mock.go | Extends service mock with DDM asset API methods. |
| server/mock/datastore_mock.go | Extends datastore mock with asset CRUD/batch/reference methods and new declaration signature. |
| server/mdm/apple/reconcile.go | Treats assets_updated_at as a resync trigger and stamps it into host declaration rows. |
| server/mdm/apple/reconcile_test.go | Adds unit coverage for assets_updated_at-driven deltas. |
| server/fleet/service.go | Extends Service interface with Apple DDM asset methods. |
| server/fleet/request.go | Adds max request size constant for asset uploads. |
| server/fleet/datastore.go | Extends Datastore interface with asset CRUD/batch/reference methods; updates declaration signature. |
| server/fleet/apple_mdm.go | Adds assets_updated_at plumbing, declaration asset reference UUIDs, EffectiveDDMToken update, and asset-related types. |
| server/fleet/apple_mdm_test.go | Adds/updates tests for forbidden type messaging, host declaration equality, and EffectiveDDMToken hashing order. |
| server/fleet/app.go | Adds macos_settings.assets field for GitOps parsing (not persisted in AppConfig). |
| server/datastore/mysql/teams_test.go | Updates declaration creation calls for new signature. |
| server/datastore/mysql/secret_variables_test.go | Updates declaration creation calls for new signature. |
| server/datastore/mysql/labels_test.go | Updates declaration creation calls for new signature. |
| server/datastore/mysql/hosts_test.go | Updates declaration creation calls for new signature. |
| server/datastore/mysql/apple_mdm.go | Persists declaration↔asset refs, updates DDM token SQL for assets_updated_at, adds asset CRUD/batch/reference queries, and serves assets to devices. |
| server/datastore/mysql/apple_mdm_test.go | Updates datastore tests for new declaration signature. |
| server/datastore/mysql/apple_mdm_ddm_test.go | Adds datastore tests for asset CRUD/batch and assets_updated_at token behavior; updates declaration signature call sites. |
| server/datastore/mysql/apple_mdm_batched.go | Loads MAX(uploaded_at) across referenced assets for reconcile and persists assets_updated_at in host declaration rows. |
| server/authz/policy.rego | Adds ddm_asset authz rules for global/team roles. |
| server/authz/policy_test.go | Adds coverage for ddm_asset read/write authorization matrix. |
| pkg/spec/gitops.go | Parses/expands controls.apple_settings.assets like profiles (path resolution + secret collection). |
| pkg/spec/gitops_test.go | Adds test ensuring assets paths are resolved and secrets are collected. |
| ee/server/service/apple_mdm.go | Implements DDM asset endpoints (authz, validation, batch-set semantics) in EE. |
| ee/server/service/apple_mdm_test.go | Adds EE service tests for asset CRUD/batch behaviors and authz. |
| cmd/fleetctl/fleetctl/gitops_test.go | Updates datastore mock signature for NewMDMAppleDeclaration in fleetctl GitOps tests. |
| cmd/fleetctl/fleetctl/generate_gitops.go | Emits assets: list and writes asset JSON files during generate-gitops. |
| cmd/fleetctl/fleetctl/generate_gitops_test.go | Adds coverage for generate-gitops emitting/writing assets. |
| changes/38986-support-ddm-assets | User-visible changes entry (content excluded from review by policy). |
Files excluded by content exclusion policy (1)
- changes/38986-support-ddm-assets
Comments suppressed due to low confidence (1)
server/datastore/mysql/apple_mdm.go:8162
- GetAppleDDMAssetForDelivery joins host_mdm_apple_declarations without restricting operation_type. That means an asset could still be retrievable for a host even if the host currently only has a pending/verified REMOVE row for the referencing declaration (i.e., it’s no longer meant to be delivered/advertised).
To match the intent (“current declarations”) and avoid serving assets for removed declarations, filter to install operations in the join/where clause.
err := sqlx.GetContext(ctx, ds.reader(ctx), &asset, `SELECT a.asset_uuid, a.team_id, a.identifier, a.name, a.token, a.created_at, a.uploaded_at, a.raw_json
FROM
hosts h
JOIN host_mdm_apple_declarations hmad ON hmad.host_uuid = h.uuid
JOIN mdm_apple_declaration_asset_references r ON r.declaration_uuid = hmad.declaration_uuid
JOIN mdm_apple_declaration_assets a ON a.asset_uuid = r.asset_uuid
AND a.team_id = COALESCE(h.team_id, 0)
WHERE
h.uuid = ?
AND a.identifier = ?`, hostUUID, identifier)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Resolve DDM asset references for declarations so the batch path links each | ||
| // declaration to the assets it references (mirroring the single-upload | ||
| // path). GitOps applies assets before declarations, so referenced assets | ||
| // already exist by this point. | ||
| for _, d := range appleDeclsSlice { | ||
| d.TeamID = tmID | ||
| assetRefs, err := svc.handleDeclarationAssetReferences(ctx, d) | ||
| if err != nil { | ||
| return ctxerr.Wrap(ctx, err, "handling declaration asset references") | ||
| } | ||
| d.AssetReferenceUUIDs = assetRefs | ||
| } |
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis change adds Apple DDM asset support across the stack. It introduces new asset types and interface methods, updates datastore persistence and reconciliation to track asset references and freshness, adds service and HTTP endpoints for listing, fetching, creating, deleting, and batch-setting assets, extends GitOps parsing and Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)server/service/integration_core_test.goast-grep timed out on this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
server/datastore/mysql/apple_mdm_batched.go (1)
888-923: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMinor:
argscapacity hint is stale.
args := make([]any, 0, len(batch)*10)still assumes 10 args per row, but each row now appends 11 values (assets_updated_atadded). Not a correctness issue — just an extra reallocation per batch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_mdm_batched.go` around lines 888 - 923, In the batched declaration insert logic, the args slice capacity hint is outdated because each row now appends 11 values in the loop that builds `valueParts` and `args`. Update the `args := make([]any, 0, len(batch)*10)` allocation in the `host_mdm_apple_declarations` batch write path to match the current per-row argument count so the `batch` processing in this function avoids unnecessary reallocations.server/datastore/mysql/apple_mdm_ddm_test.go (1)
700-832: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo test coverage for channel/scope isolation of asset delivery.
testGetAppleDDMAssetForDeliverythoroughly covers team scoping (global vs. team, cross-team leakage, unknown host) but doesn't exercise device- vs. user-channel (PayloadScope) isolation, unliketestDDMChannelScopeIsolationwhich does this for declarations. GivenGetAppleDDMAssetForDelivery(ctx, identifier, hostUUID)takes no scope parameter (seeserver/service/apple_mdm.go'shandleDeclarationAsset, which also doesn't receive/forwardscope), it's worth confirming assets referenced only by a user-scoped declaration aren't deliverable via the device channel and vice versa.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_mdm_ddm_test.go` around lines 700 - 832, testGetAppleDDMAssetForDelivery only covers team scoping and cross-team leakage, but not device-vs-user PayloadScope isolation for asset delivery. Add coverage that exercises GetAppleDDMAssetForDelivery with declarations tied to both device and user channels, verifying assets referenced only by a user-scoped declaration are not returned on the device channel and vice versa. Use the existing helpers around handleDeclarationAsset and align the new assertions with the behavior already validated in testDDMChannelScopeIsolation for declarations.server/datastore/mysql/apple_mdm.go (2)
5016-5030: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAsset reference insert uses
declUUIDbefore the post-upsert reload.The
assetReferencesinsert (using the locally-generateddeclUUID) happens beforereloadStmtre-reads the actual persisteddeclaration_uuidat line 5031. Currently this is safe becauseassetReferencesis only ever non-nil viaNewMDMAppleDeclaration(an insert-only path that errors on conflict rather than updating), so the pre- and post-reload UUIDs are identical. ButSetOrUpdateMDMAppleDeclaration'sON DUPLICATE KEY UPDATEpath does not updatedeclaration_uuid, so ifassetReferenceswere ever passed non-nil through that path, this ordering would silently associate assets with the wrong (freshly-generated, unused) UUID instead of the actual existing declaration row.Moving the assetReferences insert after the reload (like the labels/variables associations already do) would remove this footgun.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_mdm.go` around lines 5016 - 5030, The asset reference insert in SetOrUpdateMDMAppleDeclaration uses the locally generated declUUID before reloadStmt re-reads the persisted declaration UUID, which is a footgun for the ON DUPLICATE KEY UPDATE path. Move the mdm_apple_declaration_asset_references insert to after the reload step, matching the labels/variables handling, so asset references always use the actual persisted declaration_uuid. Keep the change scoped around SetOrUpdateMDMAppleDeclaration and the assetReferences insert block.
4587-4649: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffN+1 per-declaration round trips for asset association reconciliation.
updateDeclarationsAssetAssociationsruns a DELETE (and often an INSERT) per declaration in the incoming batch, inside the same transaction. For GitOps batch applies with many declarations this means O(n) sequential round trips even for declarations whose asset references haven't changed, adding latency toBatchSetMDMProfiles.Consider batching the delete/insert across all declarations in one or two statements (e.g., delete stale rows for all declaration UUIDs in one query using
sqlx.Inwith a list of decl UUIDs, and bulk-insert all(declaration_uuid, asset_uuid)pairs across declarations in a single multi-row INSERT).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_mdm.go` around lines 4587 - 4649, updateDeclarationsAssetAssociations is doing per-declaration DELETE/INSERT round trips, causing an N+1 pattern during batch reconciliation. Refactor the loop in updateDeclarationsAssetAssociations to batch stale-row deletion for all declaration UUIDs at once using sqlx.In, and similarly bulk-insert all declaration_uuid/asset_uuid pairs in one multi-row INSERT. Keep the existing reconciliation behavior and updatedDB tracking, but avoid issuing one statement per declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ee/server/service/apple_mdm.go`:
- Around line 233-256: The team lookup failure paths in BatchSetAppleDDMAssets
are returning before authz is marked as skipped, which causes authzcheck to flag
them as missed checks. Move the svc.authz.SkipAuthorization(ctx) call so it runs
before returning any team-resolution error or not-found result in the
BatchSetAppleDDMAssets flow, while keeping the existing team_id/team_name
validation and later Authorize call unchanged.
- Around line 164-206: The DDM asset validation path in validateAppleDDMAsset
currently ignores the timestamp returned by ExpandEmbeddedSecretsAndUpdatedAt,
so secret rotations do not affect the stored asset token. Update the MDM Apple
declaration asset model and token generation flow to persist and use
secrets_updated_at on mdm_apple_declaration_assets, mirroring the behavior used
by MDMAppleDeclaration, and ensure the DDM asset token includes that value so
changes to referenced $FLEET_SECRET_* entries trigger re-delivery.
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 8145-8170: The query in GetAppleDDMAssetForDelivery is missing the
same operation_type scoping used by sibling declaration-delivery paths, so it
can return assets from remove declarations. Update the SQL joined through
host_mdm_apple_declarations/MDMAppleDDMDeclarationsResponse-style logic to
filter for the install operation only, using the appropriate operation_type
condition alongside hostUUID and identifier so the delivered asset matches the
host’s currently applicable declaration state.
---
Nitpick comments:
In `@server/datastore/mysql/apple_mdm_batched.go`:
- Around line 888-923: In the batched declaration insert logic, the args slice
capacity hint is outdated because each row now appends 11 values in the loop
that builds `valueParts` and `args`. Update the `args := make([]any, 0,
len(batch)*10)` allocation in the `host_mdm_apple_declarations` batch write path
to match the current per-row argument count so the `batch` processing in this
function avoids unnecessary reallocations.
In `@server/datastore/mysql/apple_mdm_ddm_test.go`:
- Around line 700-832: testGetAppleDDMAssetForDelivery only covers team scoping
and cross-team leakage, but not device-vs-user PayloadScope isolation for asset
delivery. Add coverage that exercises GetAppleDDMAssetForDelivery with
declarations tied to both device and user channels, verifying assets referenced
only by a user-scoped declaration are not returned on the device channel and
vice versa. Use the existing helpers around handleDeclarationAsset and align the
new assertions with the behavior already validated in
testDDMChannelScopeIsolation for declarations.
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 5016-5030: The asset reference insert in
SetOrUpdateMDMAppleDeclaration uses the locally generated declUUID before
reloadStmt re-reads the persisted declaration UUID, which is a footgun for the
ON DUPLICATE KEY UPDATE path. Move the mdm_apple_declaration_asset_references
insert to after the reload step, matching the labels/variables handling, so
asset references always use the actual persisted declaration_uuid. Keep the
change scoped around SetOrUpdateMDMAppleDeclaration and the assetReferences
insert block.
- Around line 4587-4649: updateDeclarationsAssetAssociations is doing
per-declaration DELETE/INSERT round trips, causing an N+1 pattern during batch
reconciliation. Refactor the loop in updateDeclarationsAssetAssociations to
batch stale-row deletion for all declaration UUIDs at once using sqlx.In, and
similarly bulk-insert all declaration_uuid/asset_uuid pairs in one multi-row
INSERT. Keep the existing reconciliation behavior and updatedDB tracking, but
avoid issuing one statement per declaration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1565b6fb-4583-44af-8453-b2683abf2b14
📒 Files selected for processing (39)
changes/38986-support-ddm-assetscmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.gocmd/fleetctl/fleetctl/gitops_test.goee/server/service/apple_mdm.goee/server/service/apple_mdm_test.gopkg/spec/gitops.gopkg/spec/gitops_test.goserver/authz/policy.regoserver/authz/policy_test.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_batched.goserver/datastore/mysql/apple_mdm_ddm_test.goserver/datastore/mysql/apple_mdm_test.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/labels_test.goserver/datastore/mysql/secret_variables_test.goserver/datastore/mysql/teams_test.goserver/fleet/app.goserver/fleet/apple_mdm.goserver/fleet/apple_mdm_test.goserver/fleet/datastore.goserver/fleet/request.goserver/fleet/service.goserver/mdm/apple/reconcile.goserver/mdm/apple/reconcile_test.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_ddm_test.goserver/service/apple_mdm_test.goserver/service/client.goserver/service/client_profiles.goserver/service/client_teams.goserver/service/handler.goserver/service/integration_core_test.goserver/service/integration_mdm_ddm_test.goserver/service/integration_mdm_profiles_test.goserver/service/mdm.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (4)
server/datastore/mysql/apple_mdm_batched.go (1)
888-923: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMinor:
argscapacity hint is stale.
args := make([]any, 0, len(batch)*10)still assumes 10 args per row, but each row now appends 11 values (assets_updated_atadded). Not a correctness issue — just an extra reallocation per batch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_mdm_batched.go` around lines 888 - 923, In the batched declaration insert logic, the args slice capacity hint is outdated because each row now appends 11 values in the loop that builds `valueParts` and `args`. Update the `args := make([]any, 0, len(batch)*10)` allocation in the `host_mdm_apple_declarations` batch write path to match the current per-row argument count so the `batch` processing in this function avoids unnecessary reallocations.server/datastore/mysql/apple_mdm_ddm_test.go (1)
700-832: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo test coverage for channel/scope isolation of asset delivery.
testGetAppleDDMAssetForDeliverythoroughly covers team scoping (global vs. team, cross-team leakage, unknown host) but doesn't exercise device- vs. user-channel (PayloadScope) isolation, unliketestDDMChannelScopeIsolationwhich does this for declarations. GivenGetAppleDDMAssetForDelivery(ctx, identifier, hostUUID)takes no scope parameter (seeserver/service/apple_mdm.go'shandleDeclarationAsset, which also doesn't receive/forwardscope), it's worth confirming assets referenced only by a user-scoped declaration aren't deliverable via the device channel and vice versa.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_mdm_ddm_test.go` around lines 700 - 832, testGetAppleDDMAssetForDelivery only covers team scoping and cross-team leakage, but not device-vs-user PayloadScope isolation for asset delivery. Add coverage that exercises GetAppleDDMAssetForDelivery with declarations tied to both device and user channels, verifying assets referenced only by a user-scoped declaration are not returned on the device channel and vice versa. Use the existing helpers around handleDeclarationAsset and align the new assertions with the behavior already validated in testDDMChannelScopeIsolation for declarations.server/datastore/mysql/apple_mdm.go (2)
5016-5030: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAsset reference insert uses
declUUIDbefore the post-upsert reload.The
assetReferencesinsert (using the locally-generateddeclUUID) happens beforereloadStmtre-reads the actual persisteddeclaration_uuidat line 5031. Currently this is safe becauseassetReferencesis only ever non-nil viaNewMDMAppleDeclaration(an insert-only path that errors on conflict rather than updating), so the pre- and post-reload UUIDs are identical. ButSetOrUpdateMDMAppleDeclaration'sON DUPLICATE KEY UPDATEpath does not updatedeclaration_uuid, so ifassetReferenceswere ever passed non-nil through that path, this ordering would silently associate assets with the wrong (freshly-generated, unused) UUID instead of the actual existing declaration row.Moving the assetReferences insert after the reload (like the labels/variables associations already do) would remove this footgun.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_mdm.go` around lines 5016 - 5030, The asset reference insert in SetOrUpdateMDMAppleDeclaration uses the locally generated declUUID before reloadStmt re-reads the persisted declaration UUID, which is a footgun for the ON DUPLICATE KEY UPDATE path. Move the mdm_apple_declaration_asset_references insert to after the reload step, matching the labels/variables handling, so asset references always use the actual persisted declaration_uuid. Keep the change scoped around SetOrUpdateMDMAppleDeclaration and the assetReferences insert block.
4587-4649: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffN+1 per-declaration round trips for asset association reconciliation.
updateDeclarationsAssetAssociationsruns a DELETE (and often an INSERT) per declaration in the incoming batch, inside the same transaction. For GitOps batch applies with many declarations this means O(n) sequential round trips even for declarations whose asset references haven't changed, adding latency toBatchSetMDMProfiles.Consider batching the delete/insert across all declarations in one or two statements (e.g., delete stale rows for all declaration UUIDs in one query using
sqlx.Inwith a list of decl UUIDs, and bulk-insert all(declaration_uuid, asset_uuid)pairs across declarations in a single multi-row INSERT).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_mdm.go` around lines 4587 - 4649, updateDeclarationsAssetAssociations is doing per-declaration DELETE/INSERT round trips, causing an N+1 pattern during batch reconciliation. Refactor the loop in updateDeclarationsAssetAssociations to batch stale-row deletion for all declaration UUIDs at once using sqlx.In, and similarly bulk-insert all declaration_uuid/asset_uuid pairs in one multi-row INSERT. Keep the existing reconciliation behavior and updatedDB tracking, but avoid issuing one statement per declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ee/server/service/apple_mdm.go`:
- Around line 233-256: The team lookup failure paths in BatchSetAppleDDMAssets
are returning before authz is marked as skipped, which causes authzcheck to flag
them as missed checks. Move the svc.authz.SkipAuthorization(ctx) call so it runs
before returning any team-resolution error or not-found result in the
BatchSetAppleDDMAssets flow, while keeping the existing team_id/team_name
validation and later Authorize call unchanged.
- Around line 164-206: The DDM asset validation path in validateAppleDDMAsset
currently ignores the timestamp returned by ExpandEmbeddedSecretsAndUpdatedAt,
so secret rotations do not affect the stored asset token. Update the MDM Apple
declaration asset model and token generation flow to persist and use
secrets_updated_at on mdm_apple_declaration_assets, mirroring the behavior used
by MDMAppleDeclaration, and ensure the DDM asset token includes that value so
changes to referenced $FLEET_SECRET_* entries trigger re-delivery.
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 8145-8170: The query in GetAppleDDMAssetForDelivery is missing the
same operation_type scoping used by sibling declaration-delivery paths, so it
can return assets from remove declarations. Update the SQL joined through
host_mdm_apple_declarations/MDMAppleDDMDeclarationsResponse-style logic to
filter for the install operation only, using the appropriate operation_type
condition alongside hostUUID and identifier so the delivered asset matches the
host’s currently applicable declaration state.
---
Nitpick comments:
In `@server/datastore/mysql/apple_mdm_batched.go`:
- Around line 888-923: In the batched declaration insert logic, the args slice
capacity hint is outdated because each row now appends 11 values in the loop
that builds `valueParts` and `args`. Update the `args := make([]any, 0,
len(batch)*10)` allocation in the `host_mdm_apple_declarations` batch write path
to match the current per-row argument count so the `batch` processing in this
function avoids unnecessary reallocations.
In `@server/datastore/mysql/apple_mdm_ddm_test.go`:
- Around line 700-832: testGetAppleDDMAssetForDelivery only covers team scoping
and cross-team leakage, but not device-vs-user PayloadScope isolation for asset
delivery. Add coverage that exercises GetAppleDDMAssetForDelivery with
declarations tied to both device and user channels, verifying assets referenced
only by a user-scoped declaration are not returned on the device channel and
vice versa. Use the existing helpers around handleDeclarationAsset and align the
new assertions with the behavior already validated in
testDDMChannelScopeIsolation for declarations.
In `@server/datastore/mysql/apple_mdm.go`:
- Around line 5016-5030: The asset reference insert in
SetOrUpdateMDMAppleDeclaration uses the locally generated declUUID before
reloadStmt re-reads the persisted declaration UUID, which is a footgun for the
ON DUPLICATE KEY UPDATE path. Move the mdm_apple_declaration_asset_references
insert to after the reload step, matching the labels/variables handling, so
asset references always use the actual persisted declaration_uuid. Keep the
change scoped around SetOrUpdateMDMAppleDeclaration and the assetReferences
insert block.
- Around line 4587-4649: updateDeclarationsAssetAssociations is doing
per-declaration DELETE/INSERT round trips, causing an N+1 pattern during batch
reconciliation. Refactor the loop in updateDeclarationsAssetAssociations to
batch stale-row deletion for all declaration UUIDs at once using sqlx.In, and
similarly bulk-insert all declaration_uuid/asset_uuid pairs in one multi-row
INSERT. Keep the existing reconciliation behavior and updatedDB tracking, but
avoid issuing one statement per declaration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1565b6fb-4583-44af-8453-b2683abf2b14
📒 Files selected for processing (39)
changes/38986-support-ddm-assetscmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.gocmd/fleetctl/fleetctl/gitops_test.goee/server/service/apple_mdm.goee/server/service/apple_mdm_test.gopkg/spec/gitops.gopkg/spec/gitops_test.goserver/authz/policy.regoserver/authz/policy_test.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_batched.goserver/datastore/mysql/apple_mdm_ddm_test.goserver/datastore/mysql/apple_mdm_test.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/labels_test.goserver/datastore/mysql/secret_variables_test.goserver/datastore/mysql/teams_test.goserver/fleet/app.goserver/fleet/apple_mdm.goserver/fleet/apple_mdm_test.goserver/fleet/datastore.goserver/fleet/request.goserver/fleet/service.goserver/mdm/apple/reconcile.goserver/mdm/apple/reconcile_test.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_ddm_test.goserver/service/apple_mdm_test.goserver/service/client.goserver/service/client_profiles.goserver/service/client_teams.goserver/service/handler.goserver/service/integration_core_test.goserver/service/integration_mdm_ddm_test.goserver/service/integration_mdm_profiles_test.goserver/service/mdm.go
🛑 Comments failed to post (3)
ee/server/service/apple_mdm.go (2)
164-206: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the relevant code paths and schema references. rg -n "validateAppleDDMAsset|CreateAppleDDMAsset|BatchSetAppleDDMAssets|MDMAppleDDMAssetToSet|SecretsUpdatedAt|AppleDeclarationForReconcile|handleDeclarationAsset|mdm_apple_declaration_assets" ee server . -g '*.go' -g '*.sql' -g '*.md' -g '*.yaml' -g '*.yml' # Map the main file(s) before reading targeted sections. ast-grep outline ee/server/service/apple_mdm.go --view expanded || true # Read the relevant sections in apple_mdm.go. sed -n '150,260p' ee/server/service/apple_mdm.go sed -n '880,980p' ee/server/service/apple_mdm.go # Inspect the DDM asset/declaration model types and datastore interfaces. rg -n "type .*DDMAsset|type .*Declaration|SecretsUpdatedAt|UpdatedAt" ee server -g '*.go' # Search for schema/SQL around the DDM asset tables. rg -n "mdm_apple_declaration_assets|apple_ddm|ddm_asset|declaration_asset" . -g '*.sql' -g '*.go'Repository: fleetdm/fleet
Length of output: 1949
🌐 Web query:
fleetdm fleet validateAppleDDMAsset SecretsUpdatedAt AppleDeclarationForReconcile handleDeclarationAsset mdm_apple_declaration_assets💡 Result:
The terms provided relate to internal implementation details within FleetDM's MDM architecture, specifically concerning Declarative Device Management (DDM) and data modeling for assets and profiles. mdm_apple_declaration_assets is a database table used within FleetDM to store Apple DDM asset declarations [1]. This table is part of the schema design for managing asset lifecycle and synchronization [2]. Its design omits a scope column because an asset's channel is derived from referencing configurations [1]. SecretsUpdatedAt is a field used in FleetDM's MDM data structures, such as MDMWindowsProfilePayload [3]. It tracks the timestamp of when secrets associated with a specific profile or payload were last updated, allowing for equality checks and synchronization verification [3]. While the specific functions validateAppleDDMAsset, AppleDeclarationForReconcile, and handleDeclarationAsset were not found in public documentation, they are associated with the backend logic for asset API handlers and DDM synchronization processes within the FleetDM codebase [2]. These components are part of the broader effort to implement DDM features, involving relationships between host declarations, asset references, and the underlying asset storage tables [2].
Citations:
🌐 Web query:
site:github.com/fleetdm/fleet "validateAppleDDMAsset" "apple_mdm.go"💡 Result:
The function validateAppleDDMAsset is associated with development work for the Fleet device management platform, specifically concerning the handling of Apple Declarative Device Management (DDM) assets [1]. It appears in the context of implementing Asset API handlers and DDM synchronization features [1]. In the development of this functionality within the fleet repository, the function is used to validate assets during the DDM sync process [1]. This involves processes such as performing a recursive walk of JSON keys that have the AssetReference suffix against an allowlist to ensure proper asset references [1].
Citations:
- 1: #48568
🌐 Web query:
site:github.com/fleetdm/fleet "AppleDeclarationForReconcile" "SecretsUpdatedAt" "apple_mdm.go"💡 Result:
I couldn’t find an exact GitHub hit for the full string set
AppleDeclarationForReconcile+SecretsUpdatedAt+apple_mdm.goinfleetdm/fleet. The closest relevant result is a Fleet PR titled “Extract Apple MDM initialization out of runServeCmd” (#46517), which suggests the code is in the Apple MDM path but not necessarily in a file indexed by that exact query. (github.com)If you want, I can try a narrower search for the exact symbol or file path in Fleet’s repo.
Citations:
🌐 Web query:
site:github.com/fleetdm/fleet "handleDeclarationAsset" "secrets" "MDM"💡 Result:
I couldn’t verify a public hit for that exact
handleDeclarationAsset/secrets/MDMcombination infleetdm/fleetfrom web-indexed results. GitHub’s code search page for that repo/query is reachable, but it requires sign-in to view code results. (github.com)If you want, I can try a broader repo search (e.g. nearby symbols like
DeclarationAsset,handleDeclaration, orMDM secrets) and summarize likely matches.Citations:
🌐 Web query:
site:github.com/fleetdm/fleet/issues/48568 DDM asset secret rotation updatedAt asset token💡 Result:
Issue
#48568is a Fleet backend task for Apple DDM asset sync. It says asset changes should propagate viaassets_updated_at(mirroringvariables_updated_at) instead of folding asset tokens into the top-level token. Editing an asset updates itsuploaded_at, which changes the config’s per-host token and causes the reconciler to re-poke the host automatically. (github.com)It was opened on July 1, 2026 and is tagged as a sub-task for the Apple device product group. (github.com)
Citations:
🌐 Web query:
site:github.com/fleetdm/fleet/issues/48566 DDM asset secretsUpdatedAt💡 Result:
Issue
#48566is “SAAD: Migrations for DDM Asset tables”, opened on July 1, 2026. It proposes adding two new tables and one column for DDM assets, includingsecrets_updated_atonmdm_apple_declaration_assets, plus a generatedtokenbased onraw_jsonandsecrets_updated_at. It also addshost_mdm_apple_declarations.assets_updated_atto propagate asset changes into config sync. (github.com)Citations:
- 1: #48566
Track
secrets_updated_atfor DDM assets (ee/server/service/apple_mdm.go:164-206)
validateAppleDDMAssetdrops the timestamp fromExpandEmbeddedSecretsAndUpdatedAt, so rotating a referenced$FLEET_SECRET_*won’t change the stored asset token. Persistsecrets_updated_atonmdm_apple_declaration_assetsand include it in token generation, likeMDMAppleDeclaration, so secret changes force re-delivery.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/server/service/apple_mdm.go` around lines 164 - 206, The DDM asset validation path in validateAppleDDMAsset currently ignores the timestamp returned by ExpandEmbeddedSecretsAndUpdatedAt, so secret rotations do not affect the stored asset token. Update the MDM Apple declaration asset model and token generation flow to persist and use secrets_updated_at on mdm_apple_declaration_assets, mirroring the behavior used by MDMAppleDeclaration, and ensure the DDM asset token includes that value so changes to referenced $FLEET_SECRET_* entries trigger re-delivery.
233-256: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Call
SkipAuthorizationbefore returning on team lookup failures.
Otherwiseauthzchecktreats these paths as a missed authorization check and replaces theteam not found/ lookup error withCheckMissingWithResponse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/server/service/apple_mdm.go` around lines 233 - 256, The team lookup failure paths in BatchSetAppleDDMAssets are returning before authz is marked as skipped, which causes authzcheck to flag them as missed checks. Move the svc.authz.SkipAuthorization(ctx) call so it runs before returning any team-resolution error or not-found result in the BatchSetAppleDDMAssets flow, while keeping the existing team_id/team_name validation and later Authorize call unchanged.server/datastore/mysql/apple_mdm.go (1)
8145-8170: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
GetAppleDDMAssetForDeliverydoesn't filter byoperation_type, unlike sibling declaration-delivery queries.
MDMAppleDDMDeclarationsResponse(line 5298) explicitly filtersAND operation_type = ?(install) when serving a declaration to a host. This query joinshost_mdm_apple_declarationswithout anyoperation_typefilter, so an asset can still be delivered to a host even while the referencing declaration is pending/verified for removal (operation_type = 'remove'), until the pending-remove row is cleaned up byMDMAppleStoreDDMStatusReport. This is a missing filtering criterion that can return unintended data for a query meant to serve "currently applicable" content for a specific host.🐛 Proposed fix to scope delivery to install-operation declarations
var asset fleet.DownloadableDDMAsset err := sqlx.GetContext(ctx, ds.reader(ctx), &asset, `SELECT a.asset_uuid, a.team_id, a.identifier, a.name, a.token, a.created_at, a.uploaded_at, a.raw_json FROM hosts h JOIN host_mdm_apple_declarations hmad ON hmad.host_uuid = h.uuid JOIN mdm_apple_declaration_asset_references r ON r.declaration_uuid = hmad.declaration_uuid JOIN mdm_apple_declaration_assets a ON a.asset_uuid = r.asset_uuid AND a.team_id = COALESCE(h.team_id, 0) WHERE h.uuid = ? - AND a.identifier = ?`, hostUUID, identifier) + AND a.identifier = ? + AND hmad.operation_type = ?`, hostUUID, identifier, fleet.MDMOperationTypeInstall)As per path instructions: "When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity... Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// GetAppleDDMAssetForDelivery returns the asset with the given identifier that // belongs to the given team, and is referenced by the given host's current declarations. func (ds *Datastore) GetAppleDDMAssetForDelivery(ctx context.Context, identifier string, hostUUID string) (*fleet.DownloadableDDMAsset, error) { if identifier == "" { return nil, ctxerr.New(ctx, "asset identifier is required") } var asset fleet.DownloadableDDMAsset err := sqlx.GetContext(ctx, ds.reader(ctx), &asset, `SELECT a.asset_uuid, a.team_id, a.identifier, a.name, a.token, a.created_at, a.uploaded_at, a.raw_json FROM hosts h JOIN host_mdm_apple_declarations hmad ON hmad.host_uuid = h.uuid JOIN mdm_apple_declaration_asset_references r ON r.declaration_uuid = hmad.declaration_uuid JOIN mdm_apple_declaration_assets a ON a.asset_uuid = r.asset_uuid AND a.team_id = COALESCE(h.team_id, 0) WHERE h.uuid = ? AND a.identifier = ? AND hmad.operation_type = ?`, hostUUID, identifier, fleet.MDMOperationTypeInstall) if err != nil { if errors.Is(err, sql.ErrNoRows) { return nil, notFound("Asset").WithName(identifier) } return nil, ctxerr.Wrap(ctx, err, "getting apple ddm asset by identifier") } return &asset, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/apple_mdm.go` around lines 8145 - 8170, The query in GetAppleDDMAssetForDelivery is missing the same operation_type scoping used by sibling declaration-delivery paths, so it can return assets from remove declarations. Update the SQL joined through host_mdm_apple_declarations/MDMAppleDDMDeclarationsResponse-style logic to filter for the install operation only, using the appropriate operation_type condition alongside hostUUID and identifier so the delivered asset matches the host’s currently applicable declaration state.Source: Path instructions
|
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 48568-asset-api-ddm-sync #49046 +/- ##
============================================================
- Coverage 68.08% 68.08% -0.01%
============================================================
Files 3709 3710 +1
Lines 235098 236219 +1121
Branches 12353 12353
============================================================
+ Hits 160073 160822 +749
- Misses 60637 60935 +298
- Partials 14388 14462 +74
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
MagnusHJensen
force-pushed
the
48568-asset-api-ddm-sync
branch
from
2ec5c9a to
05657d0
Compare
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/fleetctl/fleetctl/generate_gitops.go (1)
1579-1616: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSanitize
asset.Namebefore using it in a file path.Unlike
generateProfiles, which derives filenames via a dedicatedgenerateProfileFilenamehelper, this uses the raw assetNamedirectly (assets/%s.json). IfNamecontains path separators or other unsafe characters, this can produce unexpected nested paths or malformed output filenames.♻️ Suggested approach
- fileName := fmt.Sprintf("assets/%s.json", asset.Name) + safeName := sanitizeFilename(asset.Name) // reuse/extract similar logic to generateProfileFilename + fileName := fmt.Sprintf("assets/%s.json", safeName)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/fleetctl/fleetctl/generate_gitops.go` around lines 1579 - 1616, The generateAssets method is building filenames directly from asset.Name, which can create unsafe or malformed paths. Update generateAssets to sanitize asset.Name before composing the assets/%s.json filename, using a dedicated helper similar to generateProfileFilename if needed, and keep the rest of the path construction in generateAssets unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/fleetctl/fleetctl/generate_gitops.go`:
- Around line 1579-1616: The generateAssets method is building filenames
directly from asset.Name, which can create unsafe or malformed paths. Update
generateAssets to sanitize asset.Name before composing the assets/%s.json
filename, using a dedicated helper similar to generateProfileFilename if needed,
and keep the rest of the path construction in generateAssets unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33c6d494-d0e5-4702-9508-10ffb6d59cc2
📒 Files selected for processing (20)
cmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.goee/server/service/apple_mdm.goee/server/service/apple_mdm_test.gopkg/spec/gitops.gopkg/spec/gitops_test.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_ddm_test.goserver/fleet/app.goserver/fleet/apple_mdm.goserver/fleet/datastore.goserver/fleet/service.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/apple_mdm.goserver/service/client.goserver/service/client_profiles.goserver/service/client_teams.goserver/service/handler.goserver/service/mdm.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/integration_mdm_profiles_test.go (1)
4991-5000: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
dtis passed as a message arg, not part of the assertion.
require.Contains(t, errMsg, "is a forbidden declaration type", dt)passesdtasmsgAndArgs, not as content to verify. The check now only verifies the generic substring"is a forbidden declaration type"regardless of whichdtwas posted, so the loop no longer confirms the error message actually names the specific forbidden type under test.Proposed fix
- require.Contains(t, errMsg, "is a forbidden declaration type", dt) + require.Contains(t, errMsg, fmt.Sprintf("%s is a forbidden declaration type", dt))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/integration_mdm_profiles_test.go` around lines 4991 - 5000, The loop in integration_mdm_profiles_test.go is not asserting that the returned error mentions the specific forbidden declaration type because dt is being passed as msgAndArgs to require.Contains instead of being checked in the text. Update the assertion around extractServerErrorText(res.Body) so it verifies errMsg contains the actual dt value along with the forbidden-type message, using the batch MDM profile test case that posts declarationForTestWithType and iterates over fleet.ForbiddenDeclTypes.
🧹 Nitpick comments (2)
cmd/fleetctl/fleetctl/generate_gitops.go (1)
1579-1615: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSanitize
asset.Namebefore using it in file paths.
generateAssetsusesasset.Namedirectly infmt.Sprintf("assets/%s.json", asset.Name), whilegenerateProfilesroutes filenames throughgenerateProfileFilenamefor sanitization. An asset name containing/,.., or OS-invalid characters could create unexpected directory structures or path traversal in the output directory.♻️ Proposed fix
func (cmd *GenerateGitopsCommand) generateAssets(teamId *uint, teamName string) ([]map[string]any, error) { assets, err := cmd.Client.ListDDMAssets(teamId) if err != nil { fmt.Fprintf(cmd.CLI.App.ErrWriter, "Error getting assets: %v\n", err) return nil, err } if len(assets) == 0 { return nil, nil } result := make([]map[string]any, 0, len(assets)) for _, asset := range assets { contents, err := cmd.Client.DownloadDDMAsset(asset.AssetUUID) if err != nil { fmt.Fprintf(cmd.CLI.App.ErrWriter, "Error getting asset contents: %s\n", err) return nil, err } - fileName := fmt.Sprintf("assets/%s.json", asset.Name) + safeName := sanitizeFilename(asset.Name) + fileName := fmt.Sprintf("assets/%s.json", safeName) if teamId == nil { fileName = fmt.Sprintf("lib/%s", fileName) } else { fileName = fmt.Sprintf("lib/%s/%s", teamName, fileName) } cmd.FilesToWrite[fileName] = string(contents) path := fmt.Sprintf("./%s", fileName) if teamId != nil { path = fmt.Sprintf("../%s", fileName) } result = append(result, map[string]any{"path": path}) } return result, nil }A simple sanitizer could be:
func sanitizeFilename(name string) string { // Replace path separators and other problematic characters. r := strings.NewReplacer("/", "_", "\\", "_", "..", "_", ":", "_") return r.Replace(name) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/fleetctl/fleetctl/generate_gitops.go` around lines 1579 - 1615, `generateAssets` is building output paths directly from `asset.Name`, which can create unsafe or unexpected filenames. Update `GenerateGitopsCommand.generateAssets` to pass the asset name through the same kind of filename sanitization used for profiles (or a dedicated helper like `sanitizeFilename`) before constructing the `assets/...json` path, so generated files cannot include separators, traversal segments, or invalid characters.pkg/spec/gitops.go (1)
1210-1224: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider requiring a file reference for asset entries.
expandBaseItemsis called withoutRequireFileReference, so anassets:entry with neitherpathnorpathssilently passes expansion and later fails inresolveAndUpdateProfilePathwith a confusingfailed to read profile file <cwd>-style error instead of a clearasset entry has no "path" or "paths" field. Since DDM assets always require file content (unlike some profile use cases), settingRequireFileReference: truehere would surface a much clearer error.Proposed fix
macOSSettings.Assets, errs = expandBaseItems(macOSSettings.Assets, controlsDir, "asset", GlobExpandOptions{ - AllowedExtensions: map[string]bool{".json": true}, - LogFn: logFn, + AllowedExtensions: map[string]bool{".json": true}, + RequireFileReference: true, + LogFn: logFn, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/spec/gitops.go` around lines 1210 - 1224, The macOSSettings.Assets expansion is allowing asset entries with no file reference, which later produces an unclear error from resolveAndUpdateProfilePath. Update the expandBaseItems call in the asset-handling block to require a file reference by setting RequireFileReference: true in the GlobExpandOptions used for assets, so entries without path or paths fail early with a clear asset-specific validation error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/service/integration_mdm_profiles_test.go`:
- Around line 4991-5000: The loop in integration_mdm_profiles_test.go is not
asserting that the returned error mentions the specific forbidden declaration
type because dt is being passed as msgAndArgs to require.Contains instead of
being checked in the text. Update the assertion around
extractServerErrorText(res.Body) so it verifies errMsg contains the actual dt
value along with the forbidden-type message, using the batch MDM profile test
case that posts declarationForTestWithType and iterates over
fleet.ForbiddenDeclTypes.
---
Nitpick comments:
In `@cmd/fleetctl/fleetctl/generate_gitops.go`:
- Around line 1579-1615: `generateAssets` is building output paths directly from
`asset.Name`, which can create unsafe or unexpected filenames. Update
`GenerateGitopsCommand.generateAssets` to pass the asset name through the same
kind of filename sanitization used for profiles (or a dedicated helper like
`sanitizeFilename`) before constructing the `assets/...json` path, so generated
files cannot include separators, traversal segments, or invalid characters.
In `@pkg/spec/gitops.go`:
- Around line 1210-1224: The macOSSettings.Assets expansion is allowing asset
entries with no file reference, which later produces an unclear error from
resolveAndUpdateProfilePath. Update the expandBaseItems call in the
asset-handling block to require a file reference by setting
RequireFileReference: true in the GlobExpandOptions used for assets, so entries
without path or paths fail early with a clear asset-specific validation error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33c6d494-d0e5-4702-9508-10ffb6d59cc2
📒 Files selected for processing (39)
changes/38986-support-ddm-assetscmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.gocmd/fleetctl/fleetctl/gitops_test.goee/server/service/apple_mdm.goee/server/service/apple_mdm_test.gopkg/spec/gitops.gopkg/spec/gitops_test.goserver/authz/policy.regoserver/authz/policy_test.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_batched.goserver/datastore/mysql/apple_mdm_ddm_test.goserver/datastore/mysql/apple_mdm_test.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/labels_test.goserver/datastore/mysql/secret_variables_test.goserver/datastore/mysql/teams_test.goserver/fleet/app.goserver/fleet/apple_mdm.goserver/fleet/apple_mdm_test.goserver/fleet/datastore.goserver/fleet/request.goserver/fleet/service.goserver/mdm/apple/reconcile.goserver/mdm/apple/reconcile_test.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_ddm_test.goserver/service/apple_mdm_test.goserver/service/client.goserver/service/client_profiles.goserver/service/client_teams.goserver/service/handler.goserver/service/integration_core_test.goserver/service/integration_mdm_ddm_test.goserver/service/integration_mdm_profiles_test.goserver/service/mdm.go
Related issue: Resolves #48570
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information. Added in a previous PR
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
For unreleased bug fixes in a release candidate, one of:
Database migrations
COLLATE utf8mb4_unicode_ci).New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsfleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit
macOS settingsassets.