Fix error handling on the os_versions API endpoint (#49899)

**Related issue:** Resolves #49483

## What & why

The `/os_versions` API endpoint returned misleading success responses
for three invalid inputs. This PR makes each return a proper error:

1. **Invalid `platform` filter** (e.g. `?platform=notrealplatform`)
previously returned `count: 0` with `200 OK`, indistinguishable from "no
matching OS versions." It now returns a `422` validation error listing
the supported platforms (`darwin`, `windows`, `linux`, `chrome`, `ios`,
`ipados`, `android` — matching the documented filter values).

2. **Unknown OS version id** (e.g. `/os_versions/99999`) previously
returned `200 OK` with a null/zero-filled `os_version` object. It now
returns a not-found (`404`) error.

3. **Negative `max_vulnerabilities`** (e.g. `?max_vulnerabilities=-5`)
returned a message reading `must be >= 0` — Go's JSON encoder
HTML-escapes `>`. The message is reworded to `max_vulnerabilities cannot
be negative`, which is clearer and avoids the escaped character.

### ⚠️ Note for reviewer (fix #2)
The single-version handler previously swallowed the datastore's
not-found error and returned an empty result on purpose, with the
comment: *"It is possible the os version exists, but the aggregation job
has not run yet."* This PR removes that swallow so a missing id returns
`404`. If you'd prefer to preserve the empty-result behavior for the
"not yet aggregated" case, I'm happy to adjust — flagging so the change
is intentional and visible.

# Checklist for submitter

- [x] Changes file added for user-visible changes in `changes/`.
- [x] Input data is properly validated, `SELECT *` is avoided, SQL
injection is prevented.

## Testing

- [x] Added/updated automated tests (`TestOSVersionsErrorHandling` in
`server/service/hosts_test.go`, covering all three cases).
- [x] QA'd all new/changed functionality manually

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Unsupported OS platform filters now return a clear validation error
instead of an empty-like result.
* Unknown OS version IDs now return HTTP **404 Not Found** rather than a
success response with null/zero fields.
* `max_vulnerabilities` validation now rejects negative values with an
accurate, readable message and consistent HTTP **422** responses.
* Error responses for OS versions endpoints now reflect the correct
status codes.
* **Tests**
* Updated and added coverage to assert the new error-handling and HTTP
status expectations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

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.

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 {

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)

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")

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})))

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")
 	})
 }