diff --git a/changes/fix-os-versions-error-handling b/changes/fix-os-versions-error-handling new file mode 100644 index 0000000000..f468991f1c --- /dev/null +++ b/changes/fix-os-versions-error-handling @@ -0,0 +1 @@ +- Fixed the OS versions API (`GET /api/latest/fleet/os_versions`) to return a validation error for an unsupported `platform` filter and a "not found" error for an unknown OS version ID, instead of a successful but empty or null-filled response. Also corrected the `max_vulnerabilities` validation message so the `>=` character is no longer returned HTML-escaped. diff --git a/server/service/hosts.go b/server/service/hosts.go index f3efbe5b23..1d4bd24317 100644 --- a/server/service/hosts.go +++ b/server/service/hosts.go @@ -3408,13 +3408,22 @@ func (svc *Service) OSVersions( // Input validation if maxVulnerabilities != nil && *maxVulnerabilities < 0 { svc.authz.SkipAuthorization(ctx) - return nil, count, nil, fleet.NewInvalidArgumentError("max_vulnerabilities", "max_vulnerabilities must be >= 0") + return nil, count, nil, fleet.NewInvalidArgumentError("max_vulnerabilities", "max_vulnerabilities cannot be negative") } if err := svc.authz.Authorize(ctx, &fleet.Host{TeamID: teamID}, fleet.ActionList); err != nil { return nil, count, nil, err } + if platform != nil { + switch *platform { + case "darwin", "windows", "linux", "chrome", "ios", "ipados", "android": + // valid platform + default: + return nil, count, nil, fleet.NewInvalidArgumentError("platform", `Invalid platform: must be one of "darwin", "windows", "linux", "chrome", "ios", "ipados", or "android".`) + } + } + if name != nil && version == nil { return nil, count, nil, &fleet.BadRequestError{Message: "Cannot specify os_name without os_version"} } @@ -3602,7 +3611,7 @@ func (svc *Service) OSVersion(ctx context.Context, osID uint, teamID *uint, incl // Input validation if maxVulnerabilities != nil && *maxVulnerabilities < 0 { svc.authz.SkipAuthorization(ctx) - return nil, nil, fleet.NewInvalidArgumentError("max_vulnerabilities", "max_vulnerabilities must be >= 0") + return nil, nil, fleet.NewInvalidArgumentError("max_vulnerabilities", "max_vulnerabilities cannot be negative") } if err := svc.authz.Authorize(ctx, &fleet.Host{TeamID: teamID}, fleet.ActionList); err != nil { @@ -3635,12 +3644,7 @@ func (svc *Service) OSVersion(ctx context.Context, osID uint, teamID *uint, incl }, ) if err != nil { - if fleet.IsNotFound(err) { - // We return an empty result here to be consistent with the fleet/os_versions behavior. - // It is possible the os version exists, but the aggregation job has not run yet. - return nil, nil, nil - } - return nil, nil, err + return nil, nil, ctxerr.Wrap(ctx, err, "get os version") } if osVersion != nil { diff --git a/server/service/hosts_test.go b/server/service/hosts_test.go index 71ba777da6..bd8d0d1108 100644 --- a/server/service/hosts_test.go +++ b/server/service/hosts_test.go @@ -3147,6 +3147,60 @@ func TestEmptyTeamOSVersions(t *testing.T) { require.Equal(t, "some unknown error", fmt.Sprint(err)) } +// TestOSVersionsErrorHandling covers the error-handling fixes from #49483: +// invalid platform, invalid OS version id, and the encoding of the +// max_vulnerabilities validation message. +func TestOSVersionsErrorHandling(t *testing.T) { + ds := new(mock.Store) + svc, ctx := newTestService(t, ds, nil, nil) + + ds.OSVersionsFunc = func( + ctx context.Context, teamFilter *fleet.TeamFilter, platform *string, name *string, version *string, + ) (*fleet.OSVersions, error) { + return &fleet.OSVersions{CountsUpdatedAt: time.Now(), OSVersions: []fleet.OSVersion{}}, nil + } + ds.OSVersionFunc = func( + ctx context.Context, osVersionID uint, teamFilter *fleet.TeamFilter, + ) (*fleet.OSVersion, *time.Time, error) { + return nil, nil, newNotFoundError() + } + ds.ListVulnsByMultipleOSVersionsFunc = func(ctx context.Context, osVersions []fleet.OSVersion, includeCVSS bool, + teamID *uint, maxVulnerabilities *int, + ) (map[string]fleet.OSVulnerabilitiesWithCount, error) { + return nil, nil + } + + admin := test.UserContext(ctx, test.UserAdmin) + + // An invalid platform is rejected with a validation error instead of + // silently returning an empty, successful result. + _, _, _, err := svc.OSVersions(admin, nil, new("notrealplatform"), nil, nil, fleet.ListOptions{}, false, nil) + require.Error(t, err) + require.Contains(t, fmt.Sprint(err), "Invalid platform") + require.False(t, ds.OSVersionsFuncInvoked, "datastore should not be queried when the platform is invalid") + + // A documented platform is still accepted. + _, _, _, err = svc.OSVersions(admin, nil, new("ios"), nil, nil, fleet.ListOptions{}, false, nil) + require.NoError(t, err) + + // A negative max_vulnerabilities returns a readable message with no ">" + // character (JSON encoding would otherwise escape it to ">"). + _, _, _, err = svc.OSVersions(admin, nil, nil, nil, nil, fleet.ListOptions{}, false, new(-5)) + require.Error(t, err) + require.Contains(t, fmt.Sprint(err), "cannot be negative") + require.NotContains(t, fmt.Sprint(err), ">") + + _, _, err = svc.OSVersion(admin, 1, nil, false, new(-5)) + require.Error(t, err) + require.Contains(t, fmt.Sprint(err), "cannot be negative") + + // A non-existent OS version id returns a not-found error rather than a + // 200 response with a null-filled object. + _, _, err = svc.OSVersion(admin, 99999, nil, false, nil) + require.Error(t, err) + require.True(t, fleet.IsNotFound(err), "expected a not-found error for a missing OS version id") +} + func TestOSVersionsListOptions(t *testing.T) { ds := new(mock.Store) svc, ctx := newTestService(t, ds, nil, nil) diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index b0fa33fcb5..89ec2f4ecc 100644 --- a/server/service/integration_core_test.go +++ b/server/service/integration_core_test.go @@ -11998,9 +11998,8 @@ func (s *integrationTestSuite) TestOSVersions() { s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/os_versions/%d", osvMap["Windows 11 Pro 21H2 10.0.22000.2 ARM64"].OSVersionID), nil, http.StatusOK, &osVersionResp) assertOSVersion(t, expectedVersion, *osVersionResp.OSVersion) - // invalid id - s.DoJSON("GET", "/api/latest/fleet/os_versions/999", nil, http.StatusOK, &osVersionResp) - assert.Zero(t, osVersionResp.OSVersion.HostsCount) + // invalid id returns a not-found error rather than an empty object + s.DoJSON("GET", "/api/latest/fleet/os_versions/999", nil, http.StatusNotFound, &osVersionResp) // name and version filters s.DoJSON("GET", "/api/latest/fleet/os_versions", nil, http.StatusOK, &osVersionsResp, "os_name", "Windows 11 Pro 21H2", "os_version", "10.0.22000.2") diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index ddf42e0186..aea241a46b 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -6490,10 +6490,9 @@ func (s *integrationEnterpriseTestSuite) TestOSVersions() { ) osVersionResp = getOSVersionResponse{} s.DoJSON( - "GET", fmt.Sprintf("/api/latest/fleet/os_versions/%d", osinfo.OSVersionID), nil, http.StatusOK, &osVersionResp, "team_id", + "GET", fmt.Sprintf("/api/latest/fleet/os_versions/%d", osinfo.OSVersionID), nil, http.StatusNotFound, &osVersionResp, "team_id", fmt.Sprintf("%d", tr.Team.ID), ) - assert.Zero(t, osVersionResp.OSVersion.HostsCount) // return empty json if UpdateOSVersions cron hasn't run yet for new team team0, err := s.ds.NewTeam(context.Background(), &fleet.Team{Name: "new team"}) @@ -6539,8 +6538,7 @@ func (s *integrationEnterpriseTestSuite) TestOSVersions() { // team1 user does not have access to team0 host s.DoJSON("GET", "/api/latest/fleet/os_versions", nil, http.StatusOK, &osVersionsResp) assert.Empty(t, osVersionsResp.OSVersions) - s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/os_versions/%d", osinfo.OSVersionID), nil, http.StatusOK, &osVersionResp) - assert.Zero(t, osVersionResp.OSVersion.HostsCount) + s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/os_versions/%d", osinfo.OSVersionID), nil, http.StatusNotFound, &osVersionResp) // Move host from team0 to team1 require.NoError(t, s.ds.AddHostsToTeam(context.Background(), fleet.NewAddHostsToTeamParams(&team1.ID, []uint{hosts[0].ID}))) diff --git a/server/service/integration_enterprise_vulns_test.go b/server/service/integration_enterprise_vulns_test.go index da822226a3..496b321c5b 100644 --- a/server/service/integration_enterprise_vulns_test.go +++ b/server/service/integration_enterprise_vulns_test.go @@ -296,7 +296,7 @@ func (s *integrationEnterpriseTestSuite) TestOSVersionsMaxVulnerabilities() { // Test 4: Request with max_vulnerabilities=-1 should return error res := s.Do("GET", "/api/latest/fleet/os_versions?max_vulnerabilities=-1", nil, http.StatusUnprocessableEntity) errMsg := extractServerErrorText(res.Body) - require.Contains(t, errMsg, "max_vulnerabilities must be >= 0") + require.Contains(t, errMsg, "max_vulnerabilities cannot be negative") }) t.Run("entity endpoint", func(t *testing.T) { @@ -322,7 +322,7 @@ func (s *integrationEnterpriseTestSuite) TestOSVersionsMaxVulnerabilities() { // Test 4: Request with max_vulnerabilities=-1 should return error res := s.Do("GET", fmt.Sprintf("/api/latest/fleet/os_versions/%d?max_vulnerabilities=-1", osVersionID), nil, http.StatusUnprocessableEntity) errMsg := extractServerErrorText(res.Body) - require.Contains(t, errMsg, "max_vulnerabilities must be >= 0") + require.Contains(t, errMsg, "max_vulnerabilities cannot be negative") }) }