Fix patch policy for swiftDialog to not pick up orbit installation#49362
Fix patch policy for swiftDialog to not pick up orbit installation#49362jkatz01 wants to merge 4 commits into
Conversation
…t installed version
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #49362 +/- ##
==========================================
+ Coverage 67.97% 67.99% +0.02%
==========================================
Files 3823 3857 +34
Lines 241031 245526 +4495
Branches 12702 12702
==========================================
+ Hits 163831 166956 +3125
- Misses 62325 63432 +1107
- Partials 14875 15138 +263
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:
|
WalkthroughSwiftDialog Homebrew ingestion now excludes applications installed under 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🤖 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/maintained-apps/ingesters/homebrew/ingester.go`:
- Around line 234-240: Update the swiftdialog handling in the ingester’s Exists
query generation to exclude apps whose path matches /opt/orbit/bin/, while
preserving the bundle identifier filter and ensuring the SQL includes the
required WHERE criteria. Update the corresponding swiftdialog out.Queries.Exists
assertion in ingester_test.go to reflect this filtering.
🪄 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: b1f97c83-7c87-4ab5-b5df-38d45b76a1f7
📒 Files selected for processing (3)
ee/maintained-apps/ingesters/homebrew/ingester.goee/maintained-apps/ingesters/homebrew/ingester_test.goee/maintained-apps/outputs/swiftdialog/darwin.json
| if input.Token == "swiftdialog" { | ||
| // Orbit installs swiftDialog v2.5.6 for setup experience, MDM migration, or enrollment profile renewal; do not consider it for patch status. | ||
| out.Queries.Patched = fmt.Sprintf( | ||
| "SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM apps WHERE bundle_identifier = '%s' AND version_compare(bundle_short_version, '%s') < 0 AND path NOT LIKE '/opt/orbit/bin/%%');", | ||
| out.UniqueIdentifier, out.Version, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Consider excluding the Orbit path in the Exists query as well.
The Patched query correctly ignores Orbit's swiftDialog installation, ensuring the patch policy passes. However, the Exists query (generated at line 149) still only filters by bundle_identifier.
If Fleet relies on the Exists query to determine whether a managed app is already installed before running its installer, returning true for Orbit's copy could prevent users from successfully deploying the user-facing FMA version of swiftDialog to hosts that currently only have the Orbit copy (since the system will erroneously consider the FMA app already installed).
Consider applying the same path exclusion to the Exists query to fully isolate FMA's evaluation from Orbit's internal copy. As per path instructions, ensure that appropriate filtering criteria are applied and check for missing WHERE clauses that could lead to incorrect results.
💡 Proposed change
if input.Token == "swiftdialog" {
// Orbit installs swiftDialog v2.5.6 for setup experience, MDM migration, or enrollment profile renewal; do not consider it for patch status.
+ out.Queries.Exists = fmt.Sprintf(
+ "SELECT 1 FROM apps WHERE bundle_identifier = '%s' AND path NOT LIKE '/opt/orbit/bin/%%';",
+ out.UniqueIdentifier,
+ )
out.Queries.Patched = fmt.Sprintf(
"SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM apps WHERE bundle_identifier = '%s' AND version_compare(bundle_short_version, '%s') < 0 AND path NOT LIKE '/opt/orbit/bin/%%');",
out.UniqueIdentifier, out.Version,
)
}(Note: If applied, the corresponding out.Queries.Exists assertion for swiftdialog in ingester_test.go will also need to be updated).
📝 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.
| if input.Token == "swiftdialog" { | |
| // Orbit installs swiftDialog v2.5.6 for setup experience, MDM migration, or enrollment profile renewal; do not consider it for patch status. | |
| out.Queries.Patched = fmt.Sprintf( | |
| "SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM apps WHERE bundle_identifier = '%s' AND version_compare(bundle_short_version, '%s') < 0 AND path NOT LIKE '/opt/orbit/bin/%%');", | |
| out.UniqueIdentifier, out.Version, | |
| ) | |
| } | |
| if input.Token == "swiftdialog" { | |
| // Orbit installs swiftDialog v2.5.6 for setup experience, MDM migration, or enrollment profile renewal; do not consider it for patch status. | |
| out.Queries.Exists = fmt.Sprintf( | |
| "SELECT 1 FROM apps WHERE bundle_identifier = '%s' AND path NOT LIKE '/opt/orbit/bin/%%';", | |
| out.UniqueIdentifier, | |
| ) | |
| out.Queries.Patched = fmt.Sprintf( | |
| "SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM apps WHERE bundle_identifier = '%s' AND version_compare(bundle_short_version, '%s') < 0 AND path NOT LIKE '/opt/orbit/bin/%%');", | |
| out.UniqueIdentifier, out.Version, | |
| ) | |
| } |
🤖 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/maintained-apps/ingesters/homebrew/ingester.go` around lines 234 - 240,
Update the swiftdialog handling in the ingester’s Exists query generation to
exclude apps whose path matches /opt/orbit/bin/, while preserving the bundle
identifier filter and ensuring the SQL includes the required WHERE criteria.
Update the corresponding swiftdialog out.Queries.Exists assertion in
ingester_test.go to reflect this filtering.
Source: Path instructions
|
@jkatz01 thanks! Let me discuss with @noahtalerman in an upcoming Unpacking meeting before merging. I don't like the idea that our patch policy will be green/passing but a potentially vulnerable version of software is still lingering on the device. That will make us and the customer's IT and security teams look bad should something happen. This might be reason enough for us to keep the orbit-managed version of swiftDialog up to date. As well as the other tools we deploy... |
|
@allenhouchins thanks, I'll keep this PR as a draft for now. I think there are reasons why orbit keeps swiftDialog at version 2: https://fleetdm.slack.com/archives/C019WG4GH0A/p1781807932716669?thread_ts=1781807771.352189&cid=C019WG4GH0A |
Thanks! I'll dig into what some of those challenges might have been with upgrading the orbit-managed version of swiftDialog. Worst case, I'll update the patch policy with the more explicit path you proposed: |
3 participants
Related issue: Resolves #48667
Changes:
Adds
AND path NOT LIKE '/opt/orbit/bin/%to the path policy to not pick up the swiftDialog version orbit installs in/opt/orbit/bin/swiftDialog/macos/stable/Dialog.app.I kept it pretty open, but something like
AND path != '/opt/orbit/bin/swiftDialog/macos/stable/Dialog.app'would also work.Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
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
Summary by CodeRabbit