admin: enforce JWT role gate in S3Handler.principalForWrite

Codex P2 + coderabbitai Minor on PR #669 caught that the S3
admin handler diverged from the Dynamo path in production
wiring. The previous shape was:

  if h.roles == nil { /* JWT check */ ... return principal }
  /* h.roles != nil — production: skip JWT, only live role */
  role, _ := h.roles.LookupRole(...)
  if !role.AllowsWrite() { 403 }

So a session whose JWT was minted as `read-only` could
**escalate** to write capability the moment the live role index
promoted that access key to `full` — exactly the kind of
between-login-and-revocation window the JWT was supposed to
freeze. DynamoHandler.principalForWrite (dynamo_handler.go:413)
fires the JWT gate unconditionally and then layers the live
role check on top, which is the safer ordering: the JWT can
only narrow capability, never widen it.

Aligned the S3 handler with that contract. The fix also picks
up a second case the Dynamo path already covers: a JWT minted
as `full` whose live role has since been demoted to read-only
or removed entirely is now rejected with 403 instead of
silently sliding through.

Per CLAUDE.md "test the bug first" — `newS3HandlerForTest`
never called `WithRoleStore`, so every existing test ran the
`h.roles == nil` branch and the production path was unproven.
Added two new tests:

  TestS3Handler_PrincipalForWrite_JWTGateFiresWithRoleStore
    — table sweep over all three S3 admin write endpoints
    (create, put_acl, delete) with WithRoleStore wired and
    JWT=read-only / live=full. All three must 403.

  TestS3Handler_PrincipalForWrite_LiveRoleNarrowsJWT
    — JWT=full + live=read-only or removed-from-index.
    The full live-role-narrowing branch was unproven before;
    this pins both demotion and revocation cases.

Also documented the AdminDeleteBucket TOCTOU that coderabbitai
flagged as 🔴 / 🟠 in the same review:

  AdminDeleteBucket scans ObjectManifestPrefixForBucket at
  readTS (limit=1, an "is bucket empty?" probe) and dispatches
  the BucketMetaKey delete with that point key only in
  ReadKeys. A concurrent PutObject inserting a manifest key
  in the scanned prefix between readTS and commit will not
  conflict — the OCC validator only inspects keys appearing
  in ReadKeys, and there is no ReadRanges mechanism.

This is a pre-existing race that adapter/s3.go:deleteBucket
(SigV4 path) inherits as well — closing the gap requires
either bumping BucketGenerationKey on every PutObject so it
serves as an OCC token here, or extending OperationGroup with
ReadRanges and teaching the FSM to validate range emptiness
atomically with commit. Both are larger schema changes outside
this PR's scope. Recorded the limitation as a code comment on
AdminDeleteBucket so the next reader of the function knows the
contract; tracking the fix as a follow-up rather than expanding
the PR's surface.

go test -race ./internal/admin/ — passes.
golangci-lint run ./internal/admin/... ./adapter/... — 0 issues.

Author: bootjp

adapter/s3_admin.go

@@ -358,6 +358,30 @@ func (s *S3Server) AdminPutBucketAcl(ctx context.Context, principal AdminPrincip
 // authorisation contract as the other admin write methods. The
 // bucket-must-be-empty rule mirrors the SigV4 deleteBucket path —
 // the dashboard cannot force a recursive delete, by design.
+//
+// Known orphan-race limitation (coderabbitai 🔴 / 🟠 on PR #669):
+// the empty-bucket probe (ScanAt with limit=1 on
+// ObjectManifestPrefixForBucket) reads at readTS but the
+// subsequent BucketMetaKey delete only carries that single point
+// key in its ReadKeys set. A concurrent PutObject that inserts a
+// manifest key in the scanned prefix between readTS and the
+// delete's commitTS will not conflict — the OCC validator only
+// inspects keys that appear in ReadKeys, and there is no
+// ReadRanges mechanism today. The object's manifest key survives
+// under a now-deleted bucket meta and becomes orphaned.
+//
+// This race exists pre-existing in the SigV4 path
+// (adapter/s3.go:deleteBucket — same shape, same limitation), so
+// AdminDeleteBucket inherits the contract; closing the gap
+// requires either (a) bumping BucketGenerationKey on every
+// PutObject so it can serve as an OCC token in this read set, or
+// (b) extending OperationGroup with ReadRanges and teaching the
+// FSM to validate range emptiness atomically with commit. Both
+// are larger changes outside this PR's scope; tracked in
+// docs/design/2026_04_24_partial_admin_dashboard.md under the
+// Outstanding open items section. Operators concerned about the
+// orphan window today should pause writes against the target
+// bucket before issuing the admin delete.
 func (s *S3Server) AdminDeleteBucket(ctx context.Context, principal AdminPrincipal, name string) error {
 	if !principal.Role.canWrite() {
 		return ErrAdminForbidden

internal/admin/s3_handler.go

@@ -362,36 +362,53 @@ func (h *S3Handler) handleDelete(w http.ResponseWriter, r *http.Request, name st
 }
 
 // principalForWrite mirrors DynamoHandler.principalForWrite: pull
-// the JWT principal out of the request context, re-resolve the role
-// against the live RoleStore, and reject anything below Full. Even
-// a still-valid JWT with role=full does not get a free pass —
-// operators who revoke an access key get the change picked up on
-// the next admin write rather than waiting for the JWT to expire.
+// the JWT principal out of the request context, enforce the
+// JWT-embedded role first (defence-in-depth — a token minted as
+// read-only never escalates, even if the live role index says
+// `full` after a recent promotion), and — when a RoleStore is
+// configured — re-validate the access key against the live role
+// index so a key downgraded or revoked since login cannot keep
+// mutating with a still-valid JWT. Even a still-valid JWT with
+// role=full does not get a free pass — operators who revoke an
+// access key get the change picked up on the next admin write
+// rather than waiting for the JWT to expire.
 func (h *S3Handler) principalForWrite(w http.ResponseWriter, r *http.Request) (AuthPrincipal, bool) {
 	principal, ok := PrincipalFromContext(r.Context())
 	if !ok {
 		writeJSONError(w, http.StatusUnauthorized, "unauthenticated",
 			"no session principal")
 		return AuthPrincipal{}, false
 	}
-	if h.roles == nil {
-		// Production wiring always sets a RoleStore; nil is a test
-		// fallthrough we accept so single-handler unit tests can
-		// reach the source without standing up the auth chain.
-		if !principal.Role.AllowsWrite() {
+	// JWT role gate fires unconditionally so a session whose token
+	// was minted as read-only cannot perform writes even if the
+	// live role index later promotes the key to full. The previous
+	// shape skipped this check whenever h.roles != nil, which made
+	// it the only authorisation gate in production wiring — a
+	// security regression vs DynamoHandler that Codex P2 +
+	// coderabbitai flagged on PR #669 (the JWT freezes the role
+	// at login, the live role index can only constrain further).
+	if !principal.Role.AllowsWrite() {
+		writeJSONError(w, http.StatusForbidden, "forbidden",
+			"this endpoint requires a full-access role")
+		return AuthPrincipal{}, false
+	}
+	// Live re-validation. Skipped when no RoleStore is configured
+	// (single-handler unit tests where wiring up the role index
+	// would obscure the source-level behaviour the test is
+	// exercising); production wiring always sets one.
+	if h.roles != nil {
+		liveRole, found := h.roles.LookupRole(principal.AccessKey)
+		if !found || !liveRole.AllowsWrite() {
 			writeJSONError(w, http.StatusForbidden, "forbidden",
 				"this endpoint requires a full-access role")
 			return AuthPrincipal{}, false
 		}
-		return principal, true
-	}
-	role, found := h.roles.LookupRole(principal.AccessKey)
-	if !found || !role.AllowsWrite() {
-		writeJSONError(w, http.StatusForbidden, "forbidden",
-			"this endpoint requires a full-access role")
-		return AuthPrincipal{}, false
+		// Use the live role downstream; the JWT may carry a stale
+		// value but the live one is authoritative once both gates
+		// agree the request is allowed.
+		principal.Role = liveRole
 	}
-	return AuthPrincipal{AccessKey: principal.AccessKey, Role: role}, true
+	return principal, true
 }
 
 // writeBucketsError translates a BucketsSource error into the

internal/admin/s3_handler_test.go

@@ -440,6 +440,86 @@ func TestS3Handler_CreateBucket_ReadOnlyRoleRejected(t *testing.T) {
 		"role gate must short-circuit before the source is reached")
 }
 
+// TestS3Handler_PrincipalForWrite_JWTGateFiresWithRoleStore pins the
+// fix for Codex P2 + coderabbitai Minor on PR #669: the previous
+// shape skipped the JWT role check whenever h.roles != nil
+// (production wiring), so a session whose token was minted as
+// read-only could mutate buckets if the live role index had since
+// promoted the key to full. Aligning with DynamoHandler, the JWT
+// gate now fires unconditionally — the live store can only narrow
+// the JWT's role, never widen it.
+//
+// All three S3 admin write endpoints exercise the same
+// principalForWrite, so the table sweeps each one to lock down
+// the contract per-endpoint (a future op that forgets to call
+// principalForWrite would fail the relevant row immediately).
+func TestS3Handler_PrincipalForWrite_JWTGateFiresWithRoleStore(t *testing.T) {
+	cases := []struct {
+		name   string
+		method string
+		path   string
+		body   string
+	}{
+		{"create_bucket", http.MethodPost, pathS3Buckets, validCreateBucketBody()},
+		{"put_bucket_acl", http.MethodPut, pathS3Buckets + "/x/acl", `{"acl":"private"}`},
+		{"delete_bucket", http.MethodDelete, pathS3Buckets + "/x", ""},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			src := &stubBucketsSource{}
+			// Live RoleStore says AKIA_RO is full — exactly the
+			// pre-fix escalation window. JWT carries read-only
+			// (the "freeze at login" contract). Handler must reject.
+			roles := MapRoleStore{"AKIA_RO": RoleFull}
+			h := NewS3Handler(src).WithRoleStore(roles)
+			var body io.Reader
+			if tc.body != "" {
+				body = strings.NewReader(tc.body)
+			}
+			req := httptest.NewRequest(tc.method, tc.path, body)
+			req = withReadOnlyPrincipalForS3(req)
+			rec := httptest.NewRecorder()
+			h.ServeHTTP(rec, req)
+			require.Equal(t, http.StatusForbidden, rec.Code,
+				"JWT read-only must reject even when live role index says full")
+			require.Empty(t, src.lastCreateInput.BucketName,
+				"role gate must short-circuit before the source is reached")
+			require.Empty(t, src.lastDeleteName,
+				"role gate must short-circuit before the source is reached")
+		})
+	}
+}
+
+// TestS3Handler_PrincipalForWrite_LiveRoleNarrowsJWT covers the
+// other half of the contract: a JWT minted as full is rejected
+// when the live role index has demoted the key to read-only or
+// removed it entirely. Already covered conceptually by the
+// existing "ReadOnlyRoleRejected" tests for the h.roles == nil
+// path, but those leave the live-role-narrowing branch unproven.
+func TestS3Handler_PrincipalForWrite_LiveRoleNarrowsJWT(t *testing.T) {
+	cases := []struct {
+		name  string
+		roles MapRoleStore
+	}{
+		{"live role demoted to read-only", MapRoleStore{"AKIA_FULL": RoleReadOnly}},
+		{"live role removed from index", MapRoleStore{}},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			src := &stubBucketsSource{}
+			h := NewS3Handler(src).WithRoleStore(tc.roles)
+			req := httptest.NewRequest(http.MethodPost, pathS3Buckets,
+				strings.NewReader(validCreateBucketBody()))
+			req = withFullPrincipal(req)
+			rec := httptest.NewRecorder()
+			h.ServeHTTP(rec, req)
+			require.Equal(t, http.StatusForbidden, rec.Code)
+			require.Empty(t, src.lastCreateInput.BucketName,
+				"live role gate must short-circuit before the source is reached")
+		})
+	}
+}
+
 func TestS3Handler_CreateBucket_AlreadyExistsReturns409(t *testing.T) {
 	src := &stubBucketsSource{
 		buckets:   map[string]BucketSummary{"public-assets": {Name: "public-assets"}},