admin: address PR #673 review (slog key, validation parity, 501 sweep)

Three findings from claude + Gemini review on 70213e0:

1) **Issue 1 — `logUnexpectedSourceError` slog key was "table"**
   When called for bucket operations the field key was `"table"` but
   the value was a bucket name. Log queries on `table=` would find
   spurious bucket-error entries; queries on `bucket=` would miss
   the audit lines entirely. Renamed the parameter and the slog
   key to `resource` so the same forensic query works for both
   resource families.

2) **Gemini security-high + Claude Issue 2 — validation divergence**
   `handleCreateBucket` only checked `strings.TrimSpace(name) == ""`
   while the HTTP path's `validateCreateBucketRequest` also rejects
   whitespace-padded names like `" bucket "`. The forward path
   would have accepted them, then hit the adapter's
   `validateS3BucketName` with a less actionable error message —
   different SPA behaviour depending on whether the request was
   leader-direct or follower-forwarded. Fix: call
   `validateCreateBucketRequest(body)` in `handleCreateBucket`
   exactly like `decodeCreateTableRequest` is shared between the
   table-side handlers.

3) **Issue 3 — only CREATE_BUCKET tested for nil-BucketsSource → 501**
   `DELETE_BUCKET` and `PUT_BUCKET_ACL` had identical
   `if s.buckets == nil` guards but no coverage. Replaced
   `TestForwardServer_CreateBucket_NoBucketsSourceReturns501` with
   a table-driven `TestForwardServer_BucketOps_NoBucketsSourceReturns501`
   sweeping all three operations. A future op added without the
   nil guard fails CI immediately.

Plus a new `TestForwardServer_CreateBucket_RejectsWhitespacePaddedName`
that pins the validation-parity fix from #2.

Rebased onto the latest `feat/admin-s3-writes` (which now carries
the slice 2a review fixes) so the stack stays clean.

Author: bootjp

internal/admin/forward_server.go

@@ -261,9 +261,15 @@ func (s *ForwardServer) handleCreateBucket(ctx context.Context, principal AuthPr
 		return rejectForward(http.StatusBadRequest, "invalid_body",
 			"create-bucket payload has trailing data")
 	}
-	if strings.TrimSpace(body.BucketName) == "" {
-		return rejectForward(http.StatusBadRequest, "invalid_body",
-			"bucket_name is required")
+	// Reuse the HTTP handler's validateCreateBucketRequest so the
+	// forwarded path enforces identical rules — empty / whitespace-
+	// padded bucket_name produces the same 400 message a leader-
+	// direct call would, instead of slipping through here and
+	// hitting the adapter's lower-level validateS3BucketName with
+	// a less actionable error (Gemini security-high + Claude #2 on
+	// PR #673).
+	if err := validateCreateBucketRequest(body); err != nil {
+		return rejectForward(http.StatusBadRequest, "invalid_body", err.Error())
 	}
 	summary, err := s.buckets.AdminCreateBucket(ctx, principal, body)
 	if err != nil {
@@ -503,12 +509,18 @@ func forwardErrorResponse(op string, err error) *pb.AdminForwardResponse {
 // regressions, and logging them at LevelError would drown the
 // operational signal. The HTTP path's writeTablesError applies
 // the same policy (Codex P2 on PR #635 flagged the silent path).
-func (s *ForwardServer) logUnexpectedSourceError(ctx context.Context, op, table, forwardedFrom string, err error) {
+//
+// The resource argument carries either a Dynamo table name or an
+// S3 bucket name, depending on op. The slog key is "resource" so
+// log queries do not have to know which resource family produced a
+// given line — Claude review on PR #673 caught the prior "table"
+// key, which made bucket-error queries miss the audit entries.
+func (s *ForwardServer) logUnexpectedSourceError(ctx context.Context, op, resource, forwardedFrom string, err error) {
 	if isStructuredSourceError(err) {
 		return
 	}
 	s.logger.LogAttrs(ctx, slog.LevelError, "admin_forward_"+op+"_failed",
-		slog.String("table", table),
+		slog.String("resource", resource),
 		slog.String("forwarded_from", forwardedFrom),
 		slog.String("error", err.Error()),
 	)

internal/admin/forward_server_test.go

@@ -474,19 +474,65 @@ func TestForwardServer_CreateBucket_HappyPath(t *testing.T) {
 	require.Equal(t, "public-assets", summary.Name)
 }
 
-func TestForwardServer_CreateBucket_NoBucketsSourceReturns501(t *testing.T) {
+func TestForwardServer_BucketOps_NoBucketsSourceReturns501(t *testing.T) {
 	// Builds without S3 do not call WithBucketsSource; the leader
-	// must still reject CREATE_BUCKET cleanly with 501 instead of
-	// reaching for a nil receiver and panicking.
-	srv := newForwardServerForTest(&stubTablesSource{tables: map[string]*DynamoTableSummary{}}, fullPrincipalRoleStore())
+	// must still reject every bucket operation cleanly with 501
+	// instead of reaching for a nil receiver and panicking. Sweep
+	// over all three operations so a future op added without a
+	// nil-receiver guard fails CI immediately (Claude review on
+	// PR #673 caught the original test only covering CREATE_BUCKET).
+	cases := []struct {
+		name    string
+		op      pb.AdminOperation
+		payload []byte
+	}{
+		{
+			name:    "create",
+			op:      pb.AdminOperation_ADMIN_OP_CREATE_BUCKET,
+			payload: mustJSON(t, CreateBucketRequest{BucketName: "x"}),
+		},
+		{
+			name:    "delete",
+			op:      pb.AdminOperation_ADMIN_OP_DELETE_BUCKET,
+			payload: []byte(`{"name":"x"}`),
+		},
+		{
+			name:    "put_acl",
+			op:      pb.AdminOperation_ADMIN_OP_PUT_BUCKET_ACL,
+			payload: []byte(`{"name":"x","acl":"private"}`),
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			srv := newForwardServerForTest(&stubTablesSource{tables: map[string]*DynamoTableSummary{}}, fullPrincipalRoleStore())
+			resp, err := srv.Forward(context.Background(), &pb.AdminForwardRequest{
+				Principal: &pb.AdminPrincipal{AccessKey: "AKIA_FULL", Role: "full"},
+				Operation: tc.op,
+				Payload:   tc.payload,
+			})
+			require.NoError(t, err)
+			require.Equal(t, int32(http.StatusNotImplemented), resp.GetStatusCode())
+			require.Contains(t, string(resp.GetPayload()), "not_implemented")
+		})
+	}
+}
+
+func TestForwardServer_CreateBucket_RejectsWhitespacePaddedName(t *testing.T) {
+	// Validation parity with the HTTP path's
+	// validateCreateBucketRequest: a name like " bucket " must
+	// produce the same 400 invalid_body the leader-direct path
+	// emits, instead of slipping through and hitting the lower-
+	// level adapter validator with a less actionable error
+	// (Gemini security-high + Claude #2 on PR #673).
+	srv := newForwardServerWithBucketsForTest(&stubBucketsSource{}, fullPrincipalRoleStore())
 	resp, err := srv.Forward(context.Background(), &pb.AdminForwardRequest{
 		Principal: &pb.AdminPrincipal{AccessKey: "AKIA_FULL", Role: "full"},
 		Operation: pb.AdminOperation_ADMIN_OP_CREATE_BUCKET,
-		Payload:   mustJSON(t, CreateBucketRequest{BucketName: "x"}),
+		Payload:   mustJSON(t, CreateBucketRequest{BucketName: " padded "}),
 	})
 	require.NoError(t, err)
-	require.Equal(t, int32(http.StatusNotImplemented), resp.GetStatusCode())
-	require.Contains(t, string(resp.GetPayload()), "not_implemented")
+	require.Equal(t, int32(http.StatusBadRequest), resp.GetStatusCode())
+	require.Contains(t, string(resp.GetPayload()), "leading or trailing whitespace")
 }
 
 func TestForwardServer_CreateBucket_BadJSONReturns400(t *testing.T) {