amadan
console: name the changed fields in the instance-update audit line
Review finding: "updated instance <host> (cli)" was field-agnostic to a fault — on a security surface, "did anyone turn on steering for this host last week?" needs to stay answerable from the audit log, and a collapsed detail string couldn't answer it. Build the changed-fields list from the same req.Addrs/req.Probe/req.Steering nil checks the mutation already made, so the audit line can never drift from what was actually applied: "updated instance <host> (addrs, steering) (cli)" etc. Also fixes a stale doc-comment quote in instances_cli.go that still named the pre-Task-3 two-field refusal text.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 files changed,
+80
−5
instances_cli.go+3 −2internal/console/apiinstances.go+19 −3internal/console/apiinstances_test.go+58 −0
diff --git a/instances_cli.go b/instances_cli.go| index 7882c100f..951f87137 100644 |
| --- a/instances_cli.go |
| +++ b/instances_cli.go |
| @@ -430,8 +430,9 @@ func runInstancesDelete(args []string) { |
| // spec §9). |
| // |
| // Neither -addrs nor a -health-* flag is required by this parser: the |
| -// console's own refusal ("nothing to update — send addrs, probe, or both") |
| -// is left to fire when a caller gives neither, so that refusal's wording |
| +// console's own refusal ("nothing to update — send addrs, probe, steering, |
| +// or a combination") is left to fire when a caller gives neither, so that |
| +// refusal's wording |
| // reaches the operator verbatim rather than being paraphrased here — the |
| // self-review rule this task carries (ValidateShape's own sentences are |
| // operator guidance, not something to summarize). |
diff --git a/internal/console/apiinstances.go b/internal/console/apiinstances.go| index 637b9e586..8139c4452 100644 |
| --- a/internal/console/apiinstances.go |
| +++ b/internal/console/apiinstances.go |
| @@ -827,9 +827,25 @@ func (s *Server) handleAPIInstanceUpdate(w http.ResponseWriter, r *http.Request) |
| return |
| } |
| // "upstreams" no longer names the whole of what this door can change |
| - // (steering joined it in Task 3), so the audit line stays field-agnostic |
| - // rather than claiming a change this particular patch may not have made. |
| - s.audit(acct, app, principal, "instance-update", "updated instance "+host+" (cli)") |
| + // (steering joined it in Task 3), so the audit line names exactly the |
| + // fields THIS request carried rather than either overclaiming ("upstreams" |
| + // on a steering-only patch) or going field-agnostic ("updated instance |
| + // <host> (cli)" alone) — this is a security surface, and "did anyone turn |
| + // on steering for this host last week?" has to stay answerable from the |
| + // audit log. Built from the same req.* nil checks the mutation above |
| + // already made, so it can never drift from what was actually applied. |
| + var changed []string |
| + if req.Addrs != nil { |
| + changed = append(changed, "addrs") |
| + } |
| + if req.Probe != nil { |
| + changed = append(changed, "probe") |
| + } |
| + if req.Steering != nil { |
| + changed = append(changed, "steering") |
| + } |
| + s.audit(acct, app, principal, "instance-update", |
| + "updated instance "+host+" ("+strings.Join(changed, ", ")+") (cli)") |
| // Strictly after the write landed, and exactly once: the record is what a |
| // box's reconcile leg converges upstreams from (provision.convergeOwned), |
| // so this announcement is what makes the new set live in seconds instead |
diff --git a/internal/console/apiinstances_test.go b/internal/console/apiinstances_test.go| index e99be750d..8da4b3b74 100644 |
| --- a/internal/console/apiinstances_test.go |
| +++ b/internal/console/apiinstances_test.go |
| @@ -1406,6 +1406,64 @@ func TestAPIInstanceUpdateSteeringRoundTrip(t *testing.T) { |
| } |
| } |
| +// lastEventDetail returns the most recent instance-update audit row's |
| +// Detail — the field this test cares about — or fails the test if none |
| +// exists. Events is newest-first (the activity tab's own assumption, and |
| +// every other audit test in this file's). |
| +func lastEventDetail(t *testing.T, fx *consoleFixture, app string) string { |
| + t.Helper() |
| + events, err := fx.st.Events(OpsAccountID, app, 20) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + for _, e := range events { |
| + if e.Action == "instance-update" { |
| + return e.Detail |
| + } |
| + } |
| + t.Fatalf("events = %v, want an instance-update row", events) |
| + return "" |
| +} |
| + |
| +// TestAPIInstanceUpdateAuditNamesChangedFields: review finding (Task 3 |
| +// follow-up) — a field-agnostic "updated instance <host> (cli)" audit line |
| +// collapses three distinct member-facing changes (addrs/probe/steering) |
| +// into one indistinguishable string on a security surface: "did anyone turn |
| +// on steering for this host last week?" must stay answerable from the audit |
| +// log alone. The detail names exactly the fields THIS request carried, built |
| +// from the same req.* nil checks the mutation itself uses, so it can never |
| +// claim a change the request didn't make (or omit one it did). |
| +func TestAPIInstanceUpdateAuditNamesChangedFields(t *testing.T) { |
| + fx, token := instanceFixture(t, "code-inst-audit-fields") |
| + enableInstances(t, fx, "titogo", sharedPoolManifest()) |
| + u := instancesURL(fx, "bab", "titogo") |
| + if status, _, out := rawReq(t, http.MethodPost, u, token, |
| + `{"host":"audit.go.tito.io","region":"ie","addrs":["127.0.0.1:7001"]}`); status != http.StatusCreated { |
| + t.Fatalf("create = %d %s", status, out) |
| + } |
| + pu := updateURL(fx, "bab", "titogo", "audit.go.tito.io") |
| + |
| + // Single-field case: a steering-only patch names ONLY steering — not |
| + // "upstreams", not addrs, not probe. |
| + if status, _, out := rawReq(t, http.MethodPatch, pu, token, `{"steering":"latency"}`); status != http.StatusOK { |
| + t.Fatalf("patch steering = %d %s, want 200", status, out) |
| + } |
| + if detail := lastEventDetail(t, fx, "titogo"); detail != "updated instance audit.go.tito.io (steering) (cli)" { |
| + t.Fatalf("audit detail = %q, want it to name exactly steering", detail) |
| + } |
| + |
| + // Multi-field case: a patch touching addrs AND steering together names |
| + // both, in the order the request struct declares them (addrs, probe, |
| + // steering) — not just one of the two, and not a generic "updated". |
| + if status, _, out := rawReq(t, http.MethodPatch, pu, token, |
| + `{"addrs":["127.0.0.1:7002"],"steering":""}`); status != http.StatusOK { |
| + t.Fatalf("patch addrs+steering = %d %s, want 200", status, out) |
| + } |
| + if detail := lastEventDetail(t, fx, "titogo"); detail != "updated instance audit.go.tito.io (addrs, steering) (cli)" { |
| + t.Fatalf("audit detail = %q, want it to name both addrs and steering", detail) |
| + } |
| +} |
| + |
| // TestAPIInstanceUpdateSteeringInvalidValue: the door's own vocabulary rail |
| // (provision.ValidateShape -> ValidSteering), surfaced as a 400 with the |
| // door's own sentence — a caller sending anything but "" or "latency" is |