admin: address Claude review on PR #644

Four findings on the follower-side forward client + handler
integration:

- High: PBAdminForwardClient declared opts as `...interface{}`
  via `type grpcCallOption = interface{}`, which is incompatible
  with the proto-generated *adminForwardClient's
  `...grpc.CallOption`. Go interface satisfaction requires exact
  variadic element match, so the production bridge would have
  failed to compile when assigning pb.NewAdminForwardClient(conn)
  to PBAdminForwardClient. Switch to grpc.CallOption (already a
  transitive dependency, no new import surface).

- Medium: writeForwardResult re-emitted Content-Type and
  Cache-Control but dropped X-Content-Type-Options: nosniff that
  writeAdminJSONStatus sets on the leader-direct path. Added
  parity. New test
  TestDynamoHandler_ForwarderForwardsLeaderResponseSetsNosniff
  pins the header.

- Low: missing test for "forwarder configured AND source returns
  non-ErrTablesNotLeader". Added
  TestDynamoHandler_ForwarderNotInvokedForNonNotLeaderError —
  table-driven sweep over already-exists, validation, and
  generic error, each confirming the forwarder is never invoked
  and the appropriate non-503 status is returned.

- Nit: WithLeaderForwarder's nil semantics differ from
  WithLogger's. Added a doc comment explaining the intentional
  asymmetry — nil forwarder is a meaningful "disable" state, nil
  logger preserves the slog.Default() seed.

Author: bootjp

internal/admin/dynamo_handler.go

@@ -213,6 +213,12 @@ func (h *DynamoHandler) WithRoleStore(r RoleStore) *DynamoHandler {
 // to the standard 503 leader_unavailable response. Production
 // wires this to the gRPCForwardClient in main_admin.go; tests
 // inject a stub.
+//
+// Asymmetric vs WithLogger by design: WithLogger no-ops on nil to
+// preserve the slog.Default() seeded by NewDynamoHandler, but a
+// nil forwarder here is a meaningful "disable forwarding" state
+// (the gate in tryForwardCreate / tryForwardDelete checks for
+// nil and falls back to the leader-only 503 path).
 func (h *DynamoHandler) WithLeaderForwarder(f LeaderForwarder) *DynamoHandler {
 	h.forwarder = f
 	return h
@@ -468,6 +474,12 @@ func (h *DynamoHandler) tryForwardDelete(w http.ResponseWriter, r *http.Request,
 // leader-direct call from the SPA's point of view.
 func (h *DynamoHandler) writeForwardResult(w http.ResponseWriter, r *http.Request, res *ForwardResult) {
 	w.Header().Set("Content-Type", res.ContentType)
+	// Match writeAdminJSONStatus's hardening on the leader-direct
+	// path: forwarded responses must also carry nosniff so a SPA
+	// request that happened to traverse a follower does not
+	// silently lose the MIME-sniff protection. Claude review on
+	// PR #644 caught the parity gap.
+	w.Header().Set("X-Content-Type-Options", "nosniff")
 	w.Header().Set("Cache-Control", "no-store")
 	// 503 from the leader (e.g. it stepped down mid-request) must
 	// carry Retry-After so the client retries; preserve the

internal/admin/forward_client.go

@@ -8,6 +8,7 @@ import (
 	pb "github.com/bootjp/elastickv/proto"
 	pkgerrors "github.com/cockroachdb/errors"
 	"github.com/goccy/go-json"
+	"google.golang.org/grpc"
 )
 
 // LeaderForwarder is the contract the admin HTTP handler invokes
@@ -71,14 +72,17 @@ type GRPCConnFactory interface {
 // PBAdminForwardClient narrows pb.AdminForwardClient to just the
 // methods this package uses. The narrower interface keeps the test
 // stub implementation small.
+//
+// The opts parameter must use grpc.CallOption (not interface{}) so
+// the proto-generated *adminForwardClient satisfies this interface
+// directly — Go interface satisfaction requires exact method-
+// signature match, including variadic element types. Claude review
+// on PR #644 caught the mismatch before the bridge tried to assign
+// pb.NewAdminForwardClient(conn) and the build broke.
 type PBAdminForwardClient interface {
-	Forward(ctx context.Context, in *pb.AdminForwardRequest, opts ...grpcCallOption) (*pb.AdminForwardResponse, error)
+	Forward(ctx context.Context, in *pb.AdminForwardRequest, opts ...grpc.CallOption) (*pb.AdminForwardResponse, error)
 }
 
-// grpcCallOption is a re-export of google.golang.org/grpc.CallOption
-// kept private so callers do not import proto's grpc dep directly.
-type grpcCallOption = interface{}
-
 // gRPCForwardClient is the production LeaderForwarder. Construct
 // one with NewGRPCForwardClient. Two collaborators are required:
 //   - resolver: returns the current leader address, or "" if absent

internal/admin/forward_client_test.go

@@ -9,6 +9,7 @@ import (
 	pb "github.com/bootjp/elastickv/proto"
 	"github.com/goccy/go-json"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc"
 )
 
 // stubForwardConn is the in-memory PBAdminForwardClient the
@@ -20,7 +21,7 @@ type stubForwardConn struct {
 	lastReq *pb.AdminForwardRequest
 }
 
-func (s *stubForwardConn) Forward(_ context.Context, in *pb.AdminForwardRequest, _ ...grpcCallOption) (*pb.AdminForwardResponse, error) {
+func (s *stubForwardConn) Forward(_ context.Context, in *pb.AdminForwardRequest, _ ...grpc.CallOption) (*pb.AdminForwardResponse, error) {
 	s.lastReq = in
 	if s.err != nil {
 		return nil, s.err

internal/admin/forward_integration_test.go

@@ -160,6 +160,64 @@ func TestDynamoHandler_ForwarderTransportErrorReturns503(t *testing.T) {
 	require.NotContains(t, rec.Body.String(), "transport sentinel")
 }
 
+// TestDynamoHandler_ForwarderNotInvokedForNonNotLeaderError pins
+// the gate in tryForwardCreate / tryForwardDelete: the forward
+// path must run ONLY when the source returned ErrTablesNotLeader.
+// Other source errors (already-exists, validation, generic) must
+// fall through to writeTablesError and never reach the forwarder
+// — otherwise a leader-direct rejection like 409 Conflict would
+// be silently re-applied at the leader. Claude review on PR #644
+// noted the test gap; locking it down protects against a future
+// change accidentally removing the !errors.Is guard.
+func TestDynamoHandler_ForwarderNotInvokedForNonNotLeaderError(t *testing.T) {
+	cases := []struct {
+		name     string
+		err      error
+		wantCode int
+	}{
+		{"already_exists", ErrTablesAlreadyExists, http.StatusConflict},
+		{"validation", &ValidationError{Message: "bad input"}, http.StatusBadRequest},
+		{"generic", errors.New("opaque storage failure"), http.StatusInternalServerError},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			fwd := &stubLeaderForwarder{}
+			src := &stubTablesSource{createErr: tc.err}
+			h := NewDynamoHandler(src).WithLeaderForwarder(fwd)
+			req := httptest.NewRequest(http.MethodPost, pathDynamoTables, strings.NewReader(validCreateBody()))
+			req = withWritePrincipal(req)
+			rec := httptest.NewRecorder()
+			h.ServeHTTP(rec, req)
+
+			require.Equal(t, tc.wantCode, rec.Code)
+			require.Empty(t, fwd.lastCreateInput.TableName,
+				"forwarder must not be invoked for source error: %s", tc.name)
+		})
+	}
+}
+
+// TestDynamoHandler_ForwarderForwardsLeaderResponseSetsNosniff
+// confirms the security parity Claude flagged: writeForwardResult
+// must emit X-Content-Type-Options: nosniff just like the leader-
+// direct path's writeAdminJSONStatus, otherwise a SPA hitting a
+// follower would silently lose MIME-sniff protection on
+// forwarded responses.
+func TestDynamoHandler_ForwarderForwardsLeaderResponseSetsNosniff(t *testing.T) {
+	fwd := &stubLeaderForwarder{createRes: &ForwardResult{
+		StatusCode:  http.StatusCreated,
+		Payload:     []byte(`{"name":"users"}`),
+		ContentType: "application/json; charset=utf-8",
+	}}
+	h := newFollowerHandler(t, fwd)
+	req := httptest.NewRequest(http.MethodPost, pathDynamoTables, strings.NewReader(validCreateBody()))
+	req = withWritePrincipal(req)
+	rec := httptest.NewRecorder()
+	h.ServeHTTP(rec, req)
+
+	require.Equal(t, http.StatusCreated, rec.Code)
+	require.Equal(t, "nosniff", rec.Header().Get("X-Content-Type-Options"))
+}
+
 func TestDynamoHandler_ForwarderForwardsLeaderConflictResponse(t *testing.T) {
 	// 409 Conflict from the leader (table already exists) must
 	// be relayed verbatim to the SPA, not re-classified as 503.