Reject malformed prior act claims at token exchange

pkg/authserver/server/tokenexchange/handler.go

@@ -15,6 +15,7 @@ import (
 	"github.com/ory/fosite/handler/oauth2"
 	"github.com/ory/x/errorsx"
 
+	coreaudit "github.com/stacklok/toolhive-core/audit"
 	"github.com/stacklok/toolhive/pkg/authserver/server"
 	"github.com/stacklok/toolhive/pkg/authserver/server/session"
 	"github.com/stacklok/toolhive/pkg/oauthproto"
@@ -154,10 +155,38 @@ func (h *Handler) HandleTokenEndpointRequest(ctx context.Context, requester fosi
 	// delegation chain remains auditable rather than being discarded.
 	act := map[string]any{"sub": actorID}
 	if priorAct, ok := validatedClaims.Extra["act"]; ok && priorAct != nil {
-		if actChainDepth(priorAct) >= maxDelegationDepth {
+		// Parse with the shared audit-side parser rather than a bespoke walker: it
+		// reports both the chain depth and any RFC 8693 Section 4.1 conformance
+		// violation, so a non-object act cannot slip past the depth gate and be
+		// re-minted. A JSON-null act is filtered by the guard above: it asserts no
+		// delegation and must not read as malformed.
+		chain := coreaudit.ParseDelegationChain(priorAct, maxDelegationDepth)
+		if chain.Malformed {
+			// RFC 8693 Section 2.2.2 nominally calls for invalid_request on a bad
+			// subject_token, but also allows that "other error codes may also be
+			// used, as appropriate": invalid_grant keeps this consistent with the
+			// depth, consent, and expiry gates in this same handler, all of which
+			// reject a structurally unacceptable subject token that way.
+			//
+			// MalformedReason is a closed, value-free enum; it is surfaced to the
+			// client and MUST stay that way — never interpolate claim contents here.
+			return errorsx.WithStack(fosite.ErrInvalidGrant.WithHintf(
+				"The subject token's delegation chain is malformed (%s).", chain.MalformedReason))
+		}
+		// Equivalent to the depth of the prior chain: the parser appends exactly one
+		// hop per level and stops at maxDelegationDepth, so len(Chain) is
+		// min(depth, maxDelegationDepth).
+		if len(chain.Chain) >= maxDelegationDepth {
 			return errorsx.WithStack(fosite.ErrInvalidGrant.WithHint(
 				"The subject token's delegation chain is too deep."))
 		}
+		// Nest priorAct verbatim rather than re-serializing from chain: core keeps
+		// each hop's extra claims in an unexported map, so rebuilding would silently
+		// drop the history trail that Section 4.1 asks us to preserve. The cost is
+		// that unknown hop claims — and the non-identity claims Section 4.1 calls
+		// "not meaningful" inside act — pass through re-signed, which sits in
+		// tension with Section 6 data minimization. Accepted here; bounding the
+		// subtree belongs with the external-issuer work in #5989.
 		act["act"] = priorAct
 	}
 	delegatedSession.JWTClaims.Extra["act"] = act
@@ -468,22 +497,3 @@ func ensureAudienceSubsetOfSubject(granted, subjectAud []string) error {
 	}
 	return nil
 }
-
-// actChainDepth returns the number of nested "act" entries in a subject
-// token's delegation chain, starting from its outermost act claim. A
-// non-map act (or nil) has depth 0.
-func actChainDepth(act any) int {
-	depth := 0
-	for {
-		m, ok := act.(map[string]any)
-		if !ok {
-			return depth
-		}
-		depth++
-		next, ok := m["act"]
-		if !ok || next == nil {
-			return depth
-		}
-		act = next
-	}
-}

pkg/authserver/server/tokenexchange/handler_test.go

@@ -81,9 +81,21 @@ func defaultFormValues(t *testing.T, tj *testJWKS) url.Values {
 	}
 }
 
+// actFormValues returns valid token exchange form values whose subject token
+// carries the given "act" claim.
+func actFormValues(t *testing.T, tj *testJWKS, act any) url.Values {
+	t.Helper()
+
+	extra := validExtraClaims()
+	extra["act"] = act
+	form := defaultFormValues(t, tj)
+	form.Set("subject_token", tj.signToken(t, validClaims(), extra))
+	return form
+}
+
 // nestedActChain builds a chain of depth nested "act" maps, each holding a
-// distinct "sub", for exercising actChainDepth boundary behavior. depth must
-// be at least 1.
+// distinct "sub", for exercising the delegation-depth cap. depth must be at
+// least 1.
 func nestedActChain(depth int) map[string]any {
 	chain := map[string]any{"sub": fmt.Sprintf("agent-%d", depth-1)}
 	for i := depth - 2; i >= 0; i-- {
@@ -709,6 +721,112 @@ func TestTokenExchangeHandler_HandleTokenEndpointRequest(t *testing.T) {
 					"the prior act chain should be nested unchanged under the new act claim")
 			},
 		},
+		{
+			// RFC 8693 Section 4.1 requires act to be a JSON object. Rejecting keeps
+			// us from re-minting the violation. A non-object array or scalar reaches
+			// the same parser branch, so the string case covers both.
+			name:   "non-object act rejected",
+			ctx:    func(_ *testing.T) context.Context { return context.Background() },
+			client: defaultClient,
+			form: func(t *testing.T) url.Values {
+				t.Helper()
+				return actFormValues(t, tj, "some-agent")
+			},
+			lifespan:     15 * time.Minute,
+			wantErr:      true,
+			wantFositeIs: fosite.ErrInvalidGrant,
+			hintContains: "act_not_object",
+		},
+		{
+			name:   "non-object nested act rejected",
+			ctx:    func(_ *testing.T) context.Context { return context.Background() },
+			client: defaultClient,
+			form: func(t *testing.T) url.Values {
+				t.Helper()
+				return actFormValues(t, tj, map[string]any{"sub": "agent-0", "act": "agent-1"})
+			},
+			lifespan:     15 * time.Minute,
+			wantErr:      true,
+			wantFositeIs: fosite.ErrInvalidGrant,
+			hintContains: "nested_act_not_object",
+		},
+		{
+			// RFC 7519 Section 4.1.2 fixes sub as a string, so a hop carrying a
+			// non-string sub is non-conformant even though the chain is an object.
+			name:   "non-string sub in act hop rejected",
+			ctx:    func(_ *testing.T) context.Context { return context.Background() },
+			client: defaultClient,
+			form: func(t *testing.T) url.Values {
+				t.Helper()
+				return actFormValues(t, tj, map[string]any{"sub": 42})
+			},
+			lifespan:     15 * time.Minute,
+			wantErr:      true,
+			wantFositeIs: fosite.ErrInvalidGrant,
+			hintContains: "sub_not_string",
+		},
+		{
+			// RFC 7519 Section 4.1.1 fixes iss as a string too. sub is kept valid
+			// here to isolate the reason: the parser reports the first violation it
+			// finds and checks iss before sub, so a hop with both would report only
+			// iss_not_string and leave this branch unpinned.
+			name:   "non-string iss in act hop rejected",
+			ctx:    func(_ *testing.T) context.Context { return context.Background() },
+			client: defaultClient,
+			form: func(t *testing.T) url.Values {
+				t.Helper()
+				return actFormValues(t, tj, map[string]any{"iss": 42, "sub": "agent-0"})
+			},
+			lifespan:     15 * time.Minute,
+			wantErr:      true,
+			wantFositeIs: fosite.ErrInvalidGrant,
+			hintContains: "iss_not_string",
+		},
+		{
+			// A JSON-null nested act ends the chain; it asserts no further
+			// delegation and must not read as malformed.
+			name:   "null nested act is accepted and nested unchanged",
+			ctx:    func(_ *testing.T) context.Context { return context.Background() },
+			client: defaultClient,
+			form: func(t *testing.T) url.Values {
+				t.Helper()
+				return actFormValues(t, tj, map[string]any{"sub": "agent-0", "act": nil})
+			},
+			lifespan: 15 * time.Minute,
+			check: func(t *testing.T, req *fosite.AccessRequest) {
+				t.Helper()
+				sess, ok := req.GetSession().(*session.Session)
+				require.True(t, ok, "session should be *session.Session")
+				actMap, ok := sess.JWTClaims.Extra["act"].(map[string]any)
+				require.True(t, ok, "act claim must be a map")
+				assert.Equal(t, testAgentClientID, actMap["sub"])
+				assert.Equal(t, map[string]any{"sub": "agent-0", "act": nil}, actMap["act"],
+					"the prior act claim should be nested verbatim, null member included")
+			},
+		},
+		{
+			// Tolerated by design: RFC 8693 Section 4.1 only requires an object, and
+			// sub is conventional rather than mandatory, so an empty hop is
+			// conformant. Pinned so a future tightening of the shared parser breaks
+			// a test rather than a caller.
+			name:   "empty act object is accepted",
+			ctx:    func(_ *testing.T) context.Context { return context.Background() },
+			client: defaultClient,
+			form: func(t *testing.T) url.Values {
+				t.Helper()
+				return actFormValues(t, tj, map[string]any{})
+			},
+			lifespan: 15 * time.Minute,
+			check: func(t *testing.T, req *fosite.AccessRequest) {
+				t.Helper()
+				sess, ok := req.GetSession().(*session.Session)
+				require.True(t, ok, "session should be *session.Session")
+				actMap, ok := sess.JWTClaims.Extra["act"].(map[string]any)
+				require.True(t, ok, "act claim must be a map")
+				assert.Equal(t, testAgentClientID, actMap["sub"])
+				assert.Equal(t, map[string]any{}, actMap["act"])
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -841,29 +959,6 @@ func TestTokenExchangeHandler_DefaultAudience(t *testing.T) {
 	}
 }
 
-func TestActChainDepth(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name string
-		act  any
-		want int
-	}{
-		{name: "nil is depth 0", act: nil, want: 0},
-		{name: "non-map is depth 0", act: "not-a-map", want: 0},
-		{name: "single act entry is depth 1", act: nestedActChain(1), want: 1},
-		{name: "nested chain reports its full depth", act: nestedActChain(5), want: 5},
-		{name: "map without a nested act claim stops at depth 1", act: map[string]any{"sub": "agent"}, want: 1},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-			assert.Equal(t, tt.want, actChainDepth(tt.act))
-		})
-	}
-}
-
 // mockConfig implements the tokenExchangeConfig interface for testing.
 type mockConfig struct {
 	scopeStrategy    fosite.ScopeStrategy