fix(agentnetwork): require access_log_retention_days on the settings PUT

The settings PUT replaces every mutable field, but retention was optional
in the schema while the other three toggles were required. That was not
merely inconsistent: the handler applies the request to a zero-valued
Settings and UpdateSettings copies each field onto the stored row
unconditionally, so an omitted retention was written as 0 — which the API
documents as "keep indefinitely". A client sending only the required
fields silently switched the account from bounded to unbounded access-log
retention, with no error and no signal.

The nil check in FromAPIRequest looked like it guarded against this but
never did: the receiver is a fresh struct, not the loaded row, so skipping
the assignment preserved nothing.

Marking the field required changes the generated client type from *int to
int, so a generated client can no longer omit it. Nothing validates
OpenAPI required-ness at runtime, so a hand-rolled body without the field
still lands as 0 — the same latitude the three booleans already have, left
consistent rather than special-cased, and now pinned by a test that says
so explicitly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Brad Ison
2026-08-08 15:52:50 +02:00
parent 13f9bde30e
commit e60e7c9089
6 changed files with 61 additions and 16 deletions

View File

@@ -142,14 +142,21 @@ func TestSettingsRoundTrip(t *testing.T) {
require.NotEmpty(t, before.Endpoint, "settings must carry the bootstrapped endpoint")
require.NotEmpty(t, before.ProxyAddress, "settings must carry the bootstrapped proxy address")
require.NotNil(t, before.AccessLogRetentionDays, "bootstrapped settings must carry a retention")
beforeRetention := *before.AccessLogRetentionDays
flipped, err := srv.UpdateSettings(ctx, api.AgentNetworkSettingsRequest{
EnableLogCollection: !before.EnableLogCollection,
EnablePromptCollection: !before.EnablePromptCollection,
RedactPii: !before.RedactPii,
AccessLogRetentionDays: beforeRetention,
})
require.NoError(t, err, "update settings")
assert.Equal(t, !before.EnableLogCollection, flipped.EnableLogCollection, "log collection toggle must flip")
assert.Equal(t, !before.EnablePromptCollection, flipped.EnablePromptCollection, "prompt collection toggle must flip")
require.NotNil(t, flipped.AccessLogRetentionDays)
assert.Equal(t, beforeRetention, *flipped.AccessLogRetentionDays,
"retention sent unchanged must round-trip, not reset to the zero value")
assert.Equal(t, before.Endpoint, flipped.Endpoint, "endpoint must be immutable across updates")
assert.Equal(t, before.ProxyAddress, flipped.ProxyAddress, "proxy address must be immutable across updates")
@@ -165,6 +172,7 @@ func TestSettingsRoundTrip(t *testing.T) {
EnableLogCollection: before.EnableLogCollection,
EnablePromptCollection: before.EnablePromptCollection,
RedactPii: before.RedactPii,
AccessLogRetentionDays: beforeRetention,
})
require.NoError(t, err, "restore settings")
}

View File

@@ -57,7 +57,8 @@ func TestSettingsBootstrapViaPost(t *testing.T) {
// A PUT has no row to update yet — bootstrap is the explicit POST.
_, err = fresh.UpdateSettings(ctx, api.AgentNetworkSettingsRequest{
EnableLogCollection: true,
EnableLogCollection: true,
AccessLogRetentionDays: 30,
})
requireClientError(t, err)
@@ -95,8 +96,11 @@ func TestSettingsBootstrapViaPost(t *testing.T) {
EnableLogCollection: true,
EnablePromptCollection: false,
RedactPii: true,
AccessLogRetentionDays: 21,
})
require.NoError(t, err, "post-bootstrap update must succeed")
require.NotNil(t, persisted.AccessLogRetentionDays)
assert.Equal(t, 21, *persisted.AccessLogRetentionDays, "retention from the update must apply")
assert.Equal(t, bootstrapped.Endpoint, persisted.Endpoint, "endpoint must survive updates untouched")
assert.Equal(t, cluster, persisted.ProxyAddress, "proxy address must survive updates untouched")
assert.True(t, persisted.EnableLogCollection, "post-bootstrap toggle must apply")

View File

@@ -166,10 +166,9 @@ func TestSettingsHandler_PutBeforeBootstrapIs404(t *testing.T) {
}
// TestSettingsHandler_PutReplacesMutableFields pins the update contract shared
// with the other PUT endpoints: the request replaces every mutable field, so a
// toggle absent from the JSON lands as its zero value rather than being
// preserved. The identity fields are not part of the PUT schema at all, so
// the endpoint and proxy address survive updates by construction.
// with the other PUT endpoints: the request replaces every mutable field, all
// four of which the schema requires. The identity fields are not part of the
// PUT schema at all, so the endpoint and proxy address survive by construction.
func TestSettingsHandler_PutReplacesMutableFields(t *testing.T) {
f := newAgentNetworkHandlerFixture(t)
@@ -181,7 +180,7 @@ func TestSettingsHandler_PutReplacesMutableFields(t *testing.T) {
require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &before))
rec = f.do(t, http.MethodPut, "/agent-network/settings",
`{"enable_log_collection": true, "enable_prompt_collection": false, "redact_pii": false}`)
`{"enable_log_collection": true, "enable_prompt_collection": false, "redact_pii": false, "access_log_retention_days": 7}`)
require.Equal(t, http.StatusOK, rec.Code, "update PUT must succeed: %s", rec.Body.String())
var got api.AgentNetworkSettings
@@ -190,8 +189,36 @@ func TestSettingsHandler_PutReplacesMutableFields(t *testing.T) {
assert.False(t, got.EnablePromptCollection, "sent toggle must apply")
assert.False(t, got.RedactPii, "sent toggle must apply")
require.NotNil(t, got.AccessLogRetentionDays)
assert.Equal(t, 0, *got.AccessLogRetentionDays,
"retention absent from the request must land as the zero value — PUT replaces all mutable fields")
assert.Equal(t, 7, *got.AccessLogRetentionDays, "sent retention must apply")
assert.Equal(t, before.Endpoint, got.Endpoint, "endpoint must survive updates untouched")
assert.Equal(t, before.ProxyAddress, got.ProxyAddress, "proxy address must survive updates untouched")
}
// TestSettingsHandler_PutOmittedRetentionLandsAsZero documents a residual the
// required-ness of access_log_retention_days does not remove. Marking the field
// required changes the generated client type from *int to int, so a generated
// client cannot omit it — but nothing validates OpenAPI required-ness at
// runtime, so a hand-rolled body without the field still decodes as 0, which
// the API documents as "keep indefinitely".
//
// That is the same latitude the three booleans already have, so it is left
// consistent rather than special-cased. This test exists to make the gap
// explicit: if request validation is ever added, this expectation is what
// changes.
func TestSettingsHandler_PutOmittedRetentionLandsAsZero(t *testing.T) {
f := newAgentNetworkHandlerFixture(t)
rec := f.do(t, http.MethodPost, "/agent-network/settings",
`{"proxy_address": "eu.proxy.netbird.io", "access_log_retention_days": 14}`)
require.Equal(t, http.StatusOK, rec.Code, "bootstrap POST must succeed: %s", rec.Body.String())
rec = f.do(t, http.MethodPut, "/agent-network/settings",
`{"enable_log_collection": true, "enable_prompt_collection": false, "redact_pii": false}`)
require.Equal(t, http.StatusOK, rec.Code, "update PUT must succeed: %s", rec.Body.String())
var got api.AgentNetworkSettings
require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &got))
require.NotNil(t, got.AccessLogRetentionDays)
assert.Equal(t, 0, *got.AccessLogRetentionDays,
"a non-conforming body that omits retention still replaces it with the zero value")
}

View File

@@ -102,17 +102,22 @@ func (s *Settings) ToAPIResponse() *api.AgentNetworkSettings {
return resp
}
// FromAPIRequest applies the update request onto the receiver: the mutable
// collection fields are always replaced with the request values. The identity
// fields (Domain, ProxyAddress) are assigned at bootstrap and are not part of
// the update schema at all — immutability by shape, not by rejection.
// FromAPIRequest applies the update request onto the receiver: every mutable
// field is replaced with the request value. The identity fields (Domain,
// ProxyAddress) are assigned at bootstrap and are not part of the update schema
// at all — immutability by shape, not by rejection.
//
// All four fields are required by the schema, so none is presence-sensitive.
// AccessLogRetentionDays in particular must stay required: the caller receives
// a zero-valued Settings, and UpdateSettings copies each field onto the stored
// row unconditionally, so an omitted value would be written as 0 — which the
// API documents as "keep indefinitely". Making retention optional would
// therefore let a client silently maximise log retention by leaving it out.
func (s *Settings) FromAPIRequest(req *api.AgentNetworkSettingsRequest) {
s.EnableLogCollection = req.EnableLogCollection
s.EnablePromptCollection = req.EnablePromptCollection
s.RedactPii = req.RedactPii
if req.AccessLogRetentionDays != nil {
s.AccessLogRetentionDays = *req.AccessLogRetentionDays
}
s.AccessLogRetentionDays = req.AccessLogRetentionDays
}
// FromAPICreateRequest applies the optional collection toggles of a bootstrap

View File

@@ -6290,6 +6290,7 @@ components:
- enable_log_collection
- enable_prompt_collection
- redact_pii
- access_log_retention_days
AgentNetworkBudgetRule:
type: object
description: Account-level budget rule. A limit-only rule bound to groups and/or users that applies across all policies as a min-wins ceiling. Empty targets means it applies to every caller.

View File

@@ -2414,7 +2414,7 @@ type AgentNetworkSettingsCreateRequest struct {
// AgentNetworkSettingsRequest Account-level Agent Network settings update. The request replaces every mutable field (the collection toggles and retention). The endpoint and proxy address are assigned at bootstrap (POST) and are not part of this schema.
type AgentNetworkSettingsRequest struct {
// AccessLogRetentionDays Days to retain full access-log rows; older rows are swept. 0 or less means keep indefinitely.
AccessLogRetentionDays *int `json:"access_log_retention_days,omitempty"`
AccessLogRetentionDays int `json:"access_log_retention_days"`
// EnableLogCollection Whether per-request access-log entries are collected for this account's agent-network traffic.
EnableLogCollection bool `json:"enable_log_collection"`