Check for duplicate linux software installers - #44234
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44234 +/- ##
==========================================
+ Coverage 66.73% 66.79% +0.05%
==========================================
Files 2627 2627
Lines 211192 211202 +10
Branches 9505 9422 -83
==========================================
+ Hits 140938 141065 +127
+ Misses 57466 57318 -148
- Partials 12788 12819 +31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
WalkthroughThis pull request implements platform-specific conflict detection logic to prevent duplicate software installer entries. A new datastore method Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/datastore/mysql/software_installers_test.go`:
- Around line 5053-5300: The test
testMatchOrCreateSoftwareInstallerDuplicateConflicts only covers team-scoped
uploads; add matching global-scope variants (TeamID nil) for each conflict
scenario to exercise the global path of MatchOrCreateSoftwareInstaller and
ensure VPP bundle ID, Title/version, and Windows UpgradeCode conflict checks are
exercised globally; for each existing team-scoped call that expects
require.ErrorContains(t, err, conflictMsg) or require.NoError, duplicate the
call but omit TeamID (leave it nil) and use distinct StorageID/Filename to avoid
hash duplicates so the test verifies conflict behavior for global
UploadSoftwareInstallerPayloads as well.
In `@server/datastore/mysql/software_installers.go`:
- Around line 3746-3753: The pre-insert read using checkInstallerExistsByName
with ds.reader(ctx) is racy and can still allow duplicates under concurrency;
fix by enforcing uniqueness at the write boundary: add a DB uniqueness
constraint/index on (team_id, title, source, platform) and modify the
create/insert path to either (A) perform the insert inside a writer transaction
and catch/translate the DB duplicate-key error into alreadyExists("installer",
payload.Title), or (B) wrap the read+write in the same writer transaction and
use a SELECT ... FOR UPDATE (or equivalent) before inserting; update the code
paths that call checkInstallerExistsByName to rely on the DB constraint or the
transactional lock and remove the standalone racy pre-check.
🪄 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: 3c161809-c42b-4bd0-8ba6-eb932440963d
📒 Files selected for processing (5)
changes/43959-duplicate-installersserver/datastore/mysql/in_house_apps.goserver/datastore/mysql/software_installers.goserver/datastore/mysql/software_installers_test.goserver/service/integration_enterprise_test.go
| func testMatchOrCreateSoftwareInstallerDuplicateConflicts(t *testing.T, ds *Datastore) { | ||
| ctx := context.Background() | ||
|
|
||
| user := test.NewUser(t, ds, "Alice", "alice@example.com", true) | ||
| team, err := ds.NewTeam(ctx, &fleet.Team{Name: t.Name()}) | ||
| require.NoError(t, err) | ||
|
|
||
| const conflictMsg = "already has an installer available for" | ||
|
|
||
| // macOS installer conflicting with a VPP app on the same bundle id. | ||
| test.CreateInsertGlobalVPPToken(t, ds) | ||
| _, err = ds.InsertVPPAppWithTeam(ctx, &fleet.VPPApp{ | ||
| VPPAppTeam: fleet.VPPAppTeam{VPPAppID: fleet.VPPAppID{AdamID: "adam_vpp_mac", Platform: fleet.MacOSPlatform}}, | ||
| Name: "Mac VPP", | ||
| BundleIdentifier: "com.example.vpp", | ||
| }, &team.ID) | ||
| require.NoError(t, err) | ||
|
|
||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "mac-vpp-clash-storage", | ||
| Filename: "vpp-clash.pkg", | ||
| Title: "Mac VPP Clash", | ||
| BundleIdentifier: "com.example.vpp", | ||
| Extension: "pkg", | ||
| Source: "apps", | ||
| Platform: "darwin", | ||
| Version: "1.0", | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.ErrorContains(t, err, conflictMsg) | ||
|
|
||
| // macOS installer sharing a bundle id with an in-house app is allowed: | ||
| // in-house apps only target iOS/iPadOS, so they don't conflict with macOS. | ||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "iha-storage", | ||
| Filename: "iha.ipa", | ||
| Title: "iOS App", | ||
| BundleIdentifier: "com.example.iha", | ||
| Extension: "ipa", | ||
| Source: "ios_apps", | ||
| Platform: "ios", | ||
| Version: "1.0", | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "mac-iha-coexist-storage", | ||
| Filename: "mac-iha-coexist.pkg", | ||
| Title: "Mac IHA Coexist", | ||
| BundleIdentifier: "com.example.iha", | ||
| Extension: "pkg", | ||
| Source: "apps", | ||
| Platform: "darwin", | ||
| Version: "1.0", | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // macOS installer conflicting with the same installer at a newer version. | ||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "mac-base-storage", | ||
| Filename: "mac-app.pkg", | ||
| Title: "Mac App", | ||
| BundleIdentifier: "com.example.mac", | ||
| Extension: "pkg", | ||
| Source: "apps", | ||
| Platform: "darwin", | ||
| Version: "1.0", | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "mac-v2-storage", | ||
| Filename: "mac-app-v2.pkg", | ||
| Title: "Mac App", | ||
| BundleIdentifier: "com.example.mac", | ||
| Extension: "pkg", | ||
| Source: "apps", | ||
| Platform: "darwin", | ||
| Version: "2.0", | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.ErrorContains(t, err, conflictMsg) | ||
|
|
||
| // Windows installer conflicting with the same Title at a newer version. | ||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "win-base-storage", | ||
| Filename: "win-app.msi", | ||
| Title: "Win App", | ||
| Extension: "msi", | ||
| Source: "programs", | ||
| Platform: "windows", | ||
| Version: "1.0", | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "win-v2-storage", | ||
| Filename: "win-app-v2.msi", | ||
| Title: "Win App", | ||
| Extension: "msi", | ||
| Source: "programs", | ||
| Platform: "windows", | ||
| Version: "2.0", | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.ErrorContains(t, err, conflictMsg) | ||
|
|
||
| // Windows installer conflicting on the upgrade code with a different Title. | ||
| const winUpgradeCode = "{ABCDEF12-3456-7890-ABCD-EF1234567890}" | ||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "win-uc-base-storage", | ||
| Filename: "win-uc.msi", | ||
| Title: "Win UC App", | ||
| Extension: "msi", | ||
| Source: "programs", | ||
| Platform: "windows", | ||
| Version: "1.0", | ||
| UpgradeCode: winUpgradeCode, | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "win-uc-v2-storage", | ||
| Filename: "win-uc-other.msi", | ||
| Title: "Win UC App Renamed", | ||
| Extension: "msi", | ||
| Source: "programs", | ||
| Platform: "windows", | ||
| Version: "2.0", | ||
| UpgradeCode: winUpgradeCode, | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.ErrorContains(t, err, conflictMsg) | ||
|
|
||
| // Windows: existing installer has an upgrade code, new upload has the same | ||
| // Title but no upgrade code. | ||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "win-uc-existing-storage", | ||
| Filename: "win-uc-existing.msi", | ||
| Title: "Win UC Same Name", | ||
| Extension: "msi", | ||
| Source: "programs", | ||
| Platform: "windows", | ||
| Version: "1.0", | ||
| UpgradeCode: "{11111111-1111-1111-1111-111111111111}", | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "win-uc-noupgrade-storage", | ||
| Filename: "win-uc-custom.msi", | ||
| Title: "Win UC Same Name", | ||
| Extension: "msi", | ||
| Source: "programs", | ||
| Platform: "windows", | ||
| Version: "2.0", | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.ErrorContains(t, err, conflictMsg) | ||
|
|
||
| // Reverse: existing installer has no upgrade code, new upload has the same | ||
| // Title with an upgrade code. | ||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "win-plain-base-storage", | ||
| Filename: "win-plain.msi", | ||
| Title: "Win Plain App", | ||
| Extension: "msi", | ||
| Source: "programs", | ||
| Platform: "windows", | ||
| Version: "1.0", | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "win-plain-uc-storage", | ||
| Filename: "win-plain-uc.msi", | ||
| Title: "Win Plain App", | ||
| Extension: "msi", | ||
| Source: "programs", | ||
| Platform: "windows", | ||
| Version: "2.0", | ||
| UpgradeCode: "{22222222-2222-2222-2222-222222222222}", | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.ErrorContains(t, err, conflictMsg) | ||
|
|
||
| // Linux installer conflicting with the same Title at a newer version. | ||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "linux-base-storage", | ||
| Filename: "linux-app.deb", | ||
| Title: "Linux App", | ||
| Extension: "deb", | ||
| Source: "deb_packages", | ||
| Platform: "linux", | ||
| Version: "1.0", | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, &fleet.UploadSoftwareInstallerPayload{ | ||
| StorageID: "linux-v2-storage", | ||
| Filename: "linux-app-v2.deb", | ||
| Title: "Linux App", | ||
| Extension: "deb", | ||
| Source: "deb_packages", | ||
| Platform: "linux", | ||
| Version: "2.0", | ||
| UserID: user.ID, | ||
| ValidatedLabels: &fleet.LabelIdentsWithScope{}, | ||
| TeamID: &team.ID, | ||
| }) | ||
| require.ErrorContains(t, err, conflictMsg) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether duplicate-conflict coverage already exists for nil-TeamID / global-scope uploads.
rg -n -C 2 'MatchOrCreateSoftwareInstallerDuplicateConflicts|TeamID:\s*nil|already has an installer available for' \
server/datastore/mysql/software_installers_test.go \
server/service/integration_enterprise_test.goRepository: fleetdm/fleet
Length of output: 14232
🏁 Script executed:
#!/bin/bash
# Look for other test functions that call MatchOrCreateSoftwareInstaller
# and check if any use TeamID: nil
rg -A 50 'func test.*Duplicate|func test.*Conflict' server/datastore/mysql/software_installers_test.go | head -100Repository: fleetdm/fleet
Length of output: 3764
🏁 Script executed:
#!/bin/bash
# Check for any tests combining nil-TeamID with MatchOrCreateSoftwareInstaller
# to see if the global path has existing coverage
grep -n 'TeamID.*nil' server/datastore/mysql/software_installers_test.go | \
head -20Repository: fleetdm/fleet
Length of output: 116
Add global-scope test cases to cover the conflict scenarios.
The new subtest only exercises team-scoped uploads (TeamID: &team.ID). While testMatchOrCreateSoftwareInstallerDuplicateHash does test global scope, it covers duplicate hash detection—not the conflict scenarios introduced here (VPP app bundle ID conflicts, title conflicts, upgrade code conflicts). If the conflict-detection logic diverges between team and global scopes, regressions in the global path will not be caught.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/datastore/mysql/software_installers_test.go` around lines 5053 - 5300,
The test testMatchOrCreateSoftwareInstallerDuplicateConflicts only covers
team-scoped uploads; add matching global-scope variants (TeamID nil) for each
conflict scenario to exercise the global path of MatchOrCreateSoftwareInstaller
and ensure VPP bundle ID, Title/version, and Windows UpgradeCode conflict checks
are exercised globally; for each existing team-scoped call that expects
require.ErrorContains(t, err, conflictMsg) or require.NoError, duplicate the
call but omit TeamID (leave it nil) and use distinct StorageID/Filename to avoid
hash duplicates so the test verifies conflict behavior for global
UploadSoftwareInstallerPayloads as well.
There was a problem hiding this comment.
From claude:
Finding 1 (test coverage for global-scope) — Not a real gap.**
The conflict-check helpers (checkInstallerExistsByName, checkInstallerOrInHouseAppExists, checkVPPAppExistsForTitleIdentifier) all normalize *uint → globalOrTeamID via ptr.ValOrZero(teamID) (or the equivalent inline pattern) before the SQL runs. The query is identical for nil/0 vs &team.ID; only a scalar value differs. Duplicating every scenario for TeamID: nil would just exercise the same SQL with a different number, not a different code path.
The no-team path is already covered by the integration tests in server/service/integration_enterprise_test.go (lines 12636, 12762, 12954 — all TeamID: nil or TeamID: ptr.Uint(0)), which now pass after the message-string update earlier in this conversation. No change needed.
| // check by name before any software title renaming side effects can happen | ||
| exists, err := ds.checkInstallerExistsByName(ctx, ds.reader(ctx), payload.TeamID, payload.Title, payload.Source, payload.Platform) | ||
| if err != nil { | ||
| return ctxerr.Wrap(ctx, err, "check if installer exists by name") | ||
| } | ||
| if exists { | ||
| return alreadyExists("installer", payload.Title) | ||
| } |
There was a problem hiding this comment.
Non-atomic duplicate check can still allow concurrent duplicates.
Line 3747 performs a pre-insert read check outside the insert transaction. Under concurrent requests, both calls can observe “not exists” and proceed to insert, so duplicates can still slip through.
Please enforce this at the write boundary (e.g., DB uniqueness constraint aligned with the rule, or lock/select-for-update in the same transaction that performs the insert).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/datastore/mysql/software_installers.go` around lines 3746 - 3753, The
pre-insert read using checkInstallerExistsByName with ds.reader(ctx) is racy and
can still allow duplicates under concurrency; fix by enforcing uniqueness at the
write boundary: add a DB uniqueness constraint/index on (team_id, title, source,
platform) and modify the create/insert path to either (A) perform the insert
inside a writer transaction and catch/translate the DB duplicate-key error into
alreadyExists("installer", payload.Title), or (B) wrap the read+write in the
same writer transaction and use a SELECT ... FOR UPDATE (or equivalent) before
inserting; update the code paths that call checkInstallerExistsByName to rely on
the DB constraint or the transactional lock and remove the standalone racy
pre-check.
There was a problem hiding this comment.
From claude:
Finding 2 (pre-insert read is racy) — Not a real issue, way out of scope.**
Already verified: software_installers has UNIQUE KEY idx_software_installers_team_id_title_id (global_or_team_id, title_id) from migration 20240515200020_AddSoftwareInstallerTables.go:77. A race-induced double insert is caught at the DB layer — the pre-check is just for fast-fail with a friendly error message.
The reviewer's suggested fix (new constraint on (team_id, title, source, platform), plus migration, plus moving everything into a writer transaction with SELECT ... FOR UPDATE or duplicate-key error translation) is a substantial architectural change that goes far beyond the scope of this ticket (#43959 — fixing detection of duplicate Linux installers). Software installer uploads aren't a high-concurrency surface (operator-initiated, behind auth), and the existing UNIQUE constraint provides correctness; the pre-check provides UX. No change needed.
| @@ -3699,68 +3699,63 @@ LIMIT 1` | |||
| } | |||
|
|
|||
| func (ds *Datastore) checkSoftwareConflictsByIdentifier(ctx context.Context, payload *fleet.UploadSoftwareInstallerPayload) error { | |||
There was a problem hiding this comment.
The "by identifier" isn't really correct anymore (and wasn't for a while) but I didn't rename the function in this PR.
2 participants
Related issue: Resolves #43959 #44038
Refactored
checkSoftwareConflictsByIdentifierto a switch statement with different logic per platformChecklist 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.
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
Summary by CodeRabbit
Bug Fixes