admin: tighten Forward decoding (Gemini security + Codex P1)

Two findings on the leader-side AdminForward handler:

- Gemini security-medium: handleDelete unmarshalled the raw payload
  with no size cap, so a hostile follower could push a multi-MiB
  body and consume memory before the JSON parser noticed. Apply a
  64 KiB cap (mirrors the HTTP path defaultBodyLimit) on both
  handleCreate and handleDelete; payloads past the cap return
  413 payload_too_large without ever touching json.Unmarshal.

- Codex P1: handleCreate decoded with plain json.Unmarshal,
  bypassing the strict checks the HTTP path runs through
  decodeCreateTableRequest (DisallowUnknownFields, dec.More()
  trailing-token rejection, slash-in-name validation, the rest of
  validateCreateTableRequest). Reuse decodeCreateTableRequest so a
  forwarded create cannot smuggle past validations a leader-direct
  create would have caught.

handleDelete also gains DisallowUnknownFields + dec.More() so its
payload contract matches the create path.

Tests cover the new 413 paths, the unknown-field rejection, and the
slash-in-name rejection on the forwarded create path.

Author: bootjp

internal/admin/forward_server.go

@@ -1,6 +1,7 @@
 package admin
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"log/slog"
@@ -10,6 +11,15 @@ import (
 	"github.com/goccy/go-json"
 )
 
+// adminForwardPayloadLimit caps the JSON payload the leader will
+// decode for any Forward operation. Mirrors defaultBodyLimit on the
+// HTTP path (64 KiB) so a single Forward call cannot consume more
+// memory than the same operation would over /admin/api/v1/dynamo/.
+// gRPC has its own 4 MiB max-message default, but that is way too
+// permissive for admin: a follower-forwarded request must obey the
+// same 64 KiB ceiling we promise on the public API surface.
+const adminForwardPayloadLimit = 64 << 10
+
 // ForwardServer is the leader-side gRPC handler for the AdminForward
 // RPC (design Section 3.3). The follower's admin HTTP layer calls it
 // when the local node is not the Raft leader; this server then
@@ -122,9 +132,20 @@ func (s *ForwardServer) validatePrincipal(p *pb.AdminPrincipal) (AuthPrincipal,
 }
 
 func (s *ForwardServer) handleCreate(ctx context.Context, principal AuthPrincipal, req *pb.AdminForwardRequest) (*pb.AdminForwardResponse, error) {
-	var body CreateTableRequest
-	if err := json.Unmarshal(req.GetPayload(), &body); err != nil {
-		return rejectForward(http.StatusBadRequest, "invalid_body", "request body is not valid JSON")
+	payload := req.GetPayload()
+	if len(payload) > adminForwardPayloadLimit {
+		return rejectForward(http.StatusRequestEntityTooLarge, "payload_too_large",
+			"forwarded payload exceeds the 64 KiB admin limit")
+	}
+	// Reuse the HTTP handler's strict decoder so the forwarded
+	// path enforces the same shape contract — DisallowUnknownFields,
+	// trailing-token rejection, slash-in-name rejection, and the
+	// rest of validateCreateTableRequest. Bypassing it here would
+	// let a hostile follower (or a misbehaving SPA on the follower
+	// side) sneak past validations the leader-direct path enforces.
+	body, err := decodeCreateTableRequest(bytes.NewReader(payload))
+	if err != nil {
+		return rejectForward(http.StatusBadRequest, "invalid_body", err.Error())
 	}
 	summary, err := s.source.AdminCreateTable(ctx, principal, body)
 	if err != nil {
@@ -145,10 +166,23 @@ func (s *ForwardServer) handleDelete(ctx context.Context, principal AuthPrincipa
 	// proto stays operation-agnostic — there is no operation-specific
 	// field in AdminForwardRequest, by design (adding one per op
 	// would couple every new admin endpoint to the proto schema).
+	payload := req.GetPayload()
+	if len(payload) > adminForwardPayloadLimit {
+		return rejectForward(http.StatusRequestEntityTooLarge, "payload_too_large",
+			"forwarded payload exceeds the 64 KiB admin limit")
+	}
+	dec := json.NewDecoder(bytes.NewReader(payload))
+	dec.DisallowUnknownFields()
 	var body struct {
 		Name string `json:"name"`
 	}
-	if err := json.Unmarshal(req.GetPayload(), &body); err != nil || body.Name == "" {
+	if err := dec.Decode(&body); err != nil {
+		return rejectForward(http.StatusBadRequest, "invalid_body", "delete payload is not valid JSON")
+	}
+	if dec.More() {
+		return rejectForward(http.StatusBadRequest, "invalid_body", "delete payload has trailing data")
+	}
+	if body.Name == "" {
 		return rejectForward(http.StatusBadRequest, "invalid_body", "delete payload missing name")
 	}
 	if err := s.source.AdminDeleteTable(ctx, principal, body.Name); err != nil {

internal/admin/forward_server_test.go

@@ -163,6 +163,75 @@ func TestForwardServer_UnknownOperationRejected(t *testing.T) {
 	require.Contains(t, string(resp.GetPayload()), "unknown admin operation")
 }
 
+// TestForwardServer_CreateTable_PayloadTooLargeReturns413 exercises
+// the size cap added in response to the Gemini security-medium
+// finding. The leader must refuse to decode payloads bigger than
+// the HTTP path's 64 KiB limit; gRPC's own 4 MiB default is too
+// permissive for the admin surface.
+func TestForwardServer_CreateTable_PayloadTooLargeReturns413(t *testing.T) {
+	srv := newForwardServerForTest(&stubTablesSource{tables: map[string]*DynamoTableSummary{}}, fullPrincipalRoleStore())
+	oversize := make([]byte, adminForwardPayloadLimit+1)
+	for i := range oversize {
+		oversize[i] = 'x'
+	}
+	resp, err := srv.Forward(context.Background(), &pb.AdminForwardRequest{
+		Principal: &pb.AdminPrincipal{AccessKey: "AKIA_FULL", Role: "full"},
+		Operation: pb.AdminOperation_ADMIN_OP_CREATE_TABLE,
+		Payload:   oversize,
+	})
+	require.NoError(t, err)
+	require.Equal(t, int32(http.StatusRequestEntityTooLarge), resp.GetStatusCode())
+	require.Contains(t, string(resp.GetPayload()), "payload_too_large")
+}
+
+// TestForwardServer_CreateTable_RejectsUnknownFields confirms the
+// decode path now reuses decodeCreateTableRequest's
+// DisallowUnknownFields setting (Codex P1). Without this, a
+// follower could smuggle silently-ignored fields the leader-direct
+// HTTP path would have rejected.
+func TestForwardServer_CreateTable_RejectsUnknownFields(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: pb.AdminOperation_ADMIN_OP_CREATE_TABLE,
+		Payload:   []byte(`{"table_name":"u","partition_key":{"name":"id","type":"S"},"unknown_field":1}`),
+	})
+	require.NoError(t, err)
+	require.Equal(t, int32(http.StatusBadRequest), resp.GetStatusCode())
+	require.Contains(t, string(resp.GetPayload()), "invalid_body")
+}
+
+// TestForwardServer_CreateTable_RejectsSlashInName confirms the
+// strict validation pulled in via decodeCreateTableRequest also
+// catches the slash-rejection rule the HTTP path enforces.
+func TestForwardServer_CreateTable_RejectsSlashInName(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: pb.AdminOperation_ADMIN_OP_CREATE_TABLE,
+		Payload:   []byte(`{"table_name":"foo/bar","partition_key":{"name":"id","type":"S"}}`),
+	})
+	require.NoError(t, err)
+	require.Equal(t, int32(http.StatusBadRequest), resp.GetStatusCode())
+	require.Contains(t, string(resp.GetPayload()), "must not contain")
+}
+
+// TestForwardServer_DeleteTable_PayloadTooLargeReturns413 mirrors
+// the create-side cap: an oversized delete payload must be refused
+// before the JSON decoder runs, regardless of how the gRPC layer
+// configures its own message limits.
+func TestForwardServer_DeleteTable_PayloadTooLargeReturns413(t *testing.T) {
+	srv := newForwardServerForTest(&stubTablesSource{tables: map[string]*DynamoTableSummary{}}, fullPrincipalRoleStore())
+	oversize := make([]byte, adminForwardPayloadLimit+1)
+	resp, err := srv.Forward(context.Background(), &pb.AdminForwardRequest{
+		Principal: &pb.AdminPrincipal{AccessKey: "AKIA_FULL", Role: "full"},
+		Operation: pb.AdminOperation_ADMIN_OP_DELETE_TABLE,
+		Payload:   oversize,
+	})
+	require.NoError(t, err)
+	require.Equal(t, int32(http.StatusRequestEntityTooLarge), resp.GetStatusCode())
+}
+
 func TestForwardServer_CreateTable_GenericErrorReturns500(t *testing.T) {
 	// A non-sentinel error from the source must surface as 500 with
 	// a sanitised message. The leader logs the raw error; nothing