Add pinned_version to edited_software activity - #48098
Conversation
Add PinnedVersion *string to ActivityTypeEditedSoftware (no omitempty, so clearing to Latest renders pinned_version: null, matching the team_id convention). UpdateSoftwareInstaller populates it on a version PATCH; TestFleetMaintainedAppVersionPin asserts it after each pin/clear. For #47679.
generateSoftware emits version: for an FMA from SoftwarePackage.PinnedVersion so a UI-set pin round-trips through GitOps; Latest/unpinned omits it. Test covers literal, caret, and Latest.
ghodss/yaml only auto-quotes number-like strings, so a pin like 1.2.3 or ^N was emitted bare. Per docs/Configuration/yaml-files.md the version must be quoted so YAML keeps it a string (an unquoted 10.0 would round-trip as the float 10). Force it with a post-marshal regex in the file-write step.
The PinnedVersion field (added in the prior commit) has no omitempty, so every edited_software activity now renders pinned_version: null. Update the existing TestSoftwareInstallerUploadDownloadAndDelete assertions to match. The uploaded_at struct/query edits this commit originally carried are already in the feature base.
versionMatchesMajor ran each cached version through Masterminds semver, which rejects 4-component strings (Chrome/Edge, e.g. 149.0.7827.115). A caret pin like ^149 on such an app failed with "Invalid Semantic Version" on both the PATCH and GitOps slug paths. Compare the leading dot-segment as a string instead. Add unit tests for versionMatchesMajor/parsePinnedVersion over the non-semver versions in ee/maintained-apps/outputs, and cover the end-to-end Google Chrome caret pin in TestFleetMaintainedAppVersionPin.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/38504-auto-update-pin-rollback-fma #48098 +/- ##
==========================================================================
Coverage ? 67.23%
==========================================================================
Files ? 3635
Lines ? 229906
Branches ? 11956
==========================================================================
Hits ? 154586
Misses ? 61428
Partials ? 13892
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:
|
The existing FMA pin fixtures used only non-number-like values (1.2.3/^123), which are strings regardless of quoting, so nothing exercised the case the version quoting actually protects against. Pin fma2 to a number-like 10.0 (must stay quoted, else it parses as the float 10) and leave fma1 unpinned to keep the no-version-line case.
![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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThis PR tracks pinned version changes in the 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)
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.0)server/service/integration_enterprise_test.goThanks 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/server/service/software_installers.go (1)
3980-3990: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject non-numeric caret pins before applying/storing them.
Line 3986 only rejects dotted caret values, so inputs like
^abccurrently pass parsing. They then fail matching, fall back to latest installer, and can still be recorded as a pinned value, which creates an invalid audit/state combination.Proposed fix
func parsePinnedVersion(ctx context.Context, version string) (majorVersion string, usesCaret bool, err error) { majorVersion, usesCaret = strings.CutPrefix(version, "^") if usesCaret { if len(majorVersion) == 0 { return "", false, fleet.NewUserMessageError(errEmptyCaretVersion, http.StatusBadRequest) } - if parts := strings.Split(version, "."); len(parts) > 1 { + if strings.Contains(majorVersion, ".") { return "", false, fleet.NewUserMessageError(errNonMajorVersion, http.StatusBadRequest) } + for _, r := range majorVersion { + if r < '0' || r > '9' { + return "", false, fleet.NewUserMessageError(errNonMajorVersion, http.StatusBadRequest) + } + } } return majorVersion, usesCaret, nil }Also applies to: 3993-3995
🤖 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/software_installers.go` around lines 3980 - 3990, The parsePinnedVersion function does not validate that the extracted majorVersion is numeric, allowing invalid values like "^abc" to pass parsing and subsequently fail during matching. After the CutPrefix call removes the caret prefix, add a validation check to ensure the majorVersion contains only numeric characters before returning success. This validation should reject non-numeric caret pins and prevent them from being recorded as pinned values in audit logs.
🤖 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 `@cmd/fleetctl/fleetctl/generate_gitops.go`:
- Around line 674-675: The softwareVersion quoting normalization applied at line
675 only affects the file-write path, but the --key output path returns earlier
(around line 645) after yaml.Marshal without applying the same quoting fix. To
ensure consistent version serialization behavior across both modes and prevent
type coercion of number-like versions in key output, apply the same
softwareVersion.ReplaceAll normalization that quotes versions to the --key
output path before it returns, matching the pattern used in the file-write code
path.
---
Outside diff comments:
In `@ee/server/service/software_installers.go`:
- Around line 3980-3990: The parsePinnedVersion function does not validate that
the extracted majorVersion is numeric, allowing invalid values like "^abc" to
pass parsing and subsequently fail during matching. After the CutPrefix call
removes the caret prefix, add a validation check to ensure the majorVersion
contains only numeric characters before returning success. This validation
should reject non-numeric caret pins and prevent them from being recorded as
pinned values in audit logs.
🪄 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: 341e9343-fb94-4e78-9341-b289f2fe72f5
📒 Files selected for processing (10)
cmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.gocmd/fleetctl/fleetctl/testdata/generateGitops/expectedTeamSoftware.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/fleets/team-a-thumbsup.ymlee/server/service/software_installers.goee/server/service/software_installers_test.goserver/fleet/activities.goserver/service/integration_enterprise_test.goserver/service/integration_software_titles_test.goserver/service/integration_vpp_install_test.go
| // Keep software versions quoted so YAML treats them as strings (e.g. "10.0" must not become a float). | ||
| b = softwareVersion.ReplaceAll(b, []byte(`${1}"${2}"`)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply version-quoting in --key output too.
Line 675 fixes quoting only in the file-write path, but the --key path returns at Line 645 after yaml.Marshal and skips this normalization entirely. That makes pinned-version serialization behavior inconsistent across modes and can reintroduce type coercion risk for number-like versions in key output.
Suggested fix
@@
- b, err = yaml.Marshal(value)
+ b, err = yaml.Marshal(value)
if err != nil {
fmt.Fprintf(cmd.CLI.App.ErrWriter, "Error marshaling value: %s\n", err)
return ErrGeneric
}
+ // Keep software versions quoted so YAML treats them as strings consistently
+ // with full-file output normalization.
+ b = softwareVersion.ReplaceAll(b, []byte(`${1}"${2}"`))
fmt.Fprintf(cmd.CLI.App.Writer, "%s", string(b))
return 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 `@cmd/fleetctl/fleetctl/generate_gitops.go` around lines 674 - 675, The
softwareVersion quoting normalization applied at line 675 only affects the
file-write path, but the --key output path returns earlier (around line 645)
after yaml.Marshal without applying the same quoting fix. To ensure consistent
version serialization behavior across both modes and prevent type coercion of
number-like versions in key output, apply the same softwareVersion.ReplaceAll
normalization that quotes versions to the --key output path before it returns,
matching the pattern used in the file-write code path.
|
Addressed: Finding 2 ( |
5864788
into
feat/38504-auto-update-pin-rollback-fma
#48293) **Related issue:** Resolves #38504 **Constituent PRs (merged into this feature branch):** - #47682 — Fleet UI: APRF Software title details page Library/Inventory layout - #47808 — Extend update software installer API to support FMA version pinning - #47944 — Fleet UI: APRF library item accordion component - #48081 — Versions modal, multi-row Library, pinned state - #48098 — Add `pinned_version` to `edited_software` activity - #48123 — Auto-update FMA cron - #48144 — Download a newly-published FMA version when pinned to it # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] 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. - [x] Timeouts are implemented and retries are limited to avoid infinite loops - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added Fleet-maintained app version pinning (Latest, exact, and major) via a new Versions modal. * Introduced premium auto-updates for maintained apps with pin-aware promotion and rollback-safe caching. * Added expandable library version rows and a Policies modal. * **Bug Fixes** * Improved pin handling, cache/manifest hydration, and safer update behavior on per-app failures and deduplication. * **UI/UX** * Refreshed the Software title details experience with new accordion/list patterns, redesigned details widget/tooltips, and updated installer presentation. * **Documentation** * Expanded Storybook component/page coverage and adjusted Storybook canvas padding. * **Tests** * Added/updated unit and integration tests for pinning, auto-update flows, and new modal/UI behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Related issue: Resolves #47679
Adds a few things:
generate-gitopsenclosed in double quotespinned_versionto the edited software activity. When set to a full or major version it shows up in details, when set to latest or unchanged it shows up aspinned_version: null(some other fields like display_name also dont show up when unchanged)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.
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
New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsSummary by CodeRabbit