fix: RARSet without HTML escaping; typed-nil trust fields; RAR hook docs

docs/guides/rar.md

@@ -46,7 +46,19 @@ later token request narrows the grant; left nil, `ValidateGrant` decides
 that too. Set it when `ValidateGrant` lets the server add a member at
 approval: consent may add one, but a token request should only keep or
 drop what was granted, never add a value of the client's own choosing to
-a grant that holds none.
+a grant that holds none. Leaving it nil there fails open: `ValidateGrant`
+can't tell a client's token request from the server's own grant, so it
+would accept the client's value. A type with a member the server mints
+must set `ValidateNarrowing`.
+
+The hooks see objects decoded into your Go type, so they can't tell a
+member that's absent, or `null`, from one holding its zero value: a
+missing `actions` list passes a "subset of the requested actions" check,
+while a resource server may read it as no restriction at all.
+Floating-point numbers are compared after rounding to `float64`, while
+the token carries the digits as sent. Where that matters, require the member in
+`Validate`, or give it a pointer, `json.Number` or `json.RawMessage`
+type.
 
 ## Authorization server: register, gate, approve
 
@@ -99,6 +111,17 @@ and refuses anything malformed or wider with
 `invalid_authorization_details`. An empty array gets a token without
 details; no parameter gets the whole grant. The refresh token always
 keeps the whole grant, so a later refresh can ask for any of it.
+Objects are matched greedily, in the order the client sends them: with a
+hook that accepts more than an exact match, the client may be refused a
+narrowing that it could have had by sending the same objects in another
+order. Matching never accepts more than the grant allows.
+
+**Issuing tokens yourself.** `Server.IssueAccessToken` and
+`IssueRefreshToken` grant the `AuthorizationDetails` you pass them
+after checking them with `Parse`, not `ParseRequest`: you decide what
+to grant, so `ValidateRequest` doesn't run. If you forward details a
+client sent you (an OpenID4VCI pre-authorized code token request, for
+example), run `RARRegistry.ParseRequest` on them yourself first.
 
 ## Client: ask with the same definition
 

extension/rar.go

@@ -52,6 +52,8 @@ type RARDefinition[T any] struct {
 	// choosing a value that, with a policy that grants what was requested
 	// (such as server.AllowRequestedAuthorizationDetails) and the default
 	// exact-match ValidateGrant, would otherwise survive into the grant.
+	// A definition with such a member almost always needs
+	// ValidateNarrowing too: see its doc.
 	ValidateRequest func(T) error
 
 	// ValidateGrant, if non-nil, decides whether a granted detail object
@@ -67,6 +69,17 @@ type RARDefinition[T any] struct {
 	// consent, a server's RARPolicy decision. A client asking at the
 	// token endpoint for less than its grant holds is judged by
 	// ValidateNarrowing instead, falling back to ValidateGrant.
+	//
+	// The hook sees requested and granted decoded into T, so it can't
+	// tell a member that's absent, or null, from one holding T's zero
+	// value: a nil "actions" slice passes a "subset of the requested
+	// actions" check, while a resource server may read the absent member
+	// as no restriction at all. Floating-point members are compared
+	// after rounding to float64. Where that matters, make Validate
+	// require the member, or give it a pointer, json.Number or
+	// json.RawMessage type in T, so the hook sees what was sent. The
+	// same holds for ValidateNarrowing, where the client chooses the
+	// narrowed bytes the access token then carries.
 	ValidateGrant func(requested, granted T) error
 
 	// ValidateNarrowing, if non-nil, decides whether a detail object a
@@ -86,7 +99,8 @@ type RARDefinition[T any] struct {
 	// Without ValidateNarrowing, a grant that holds no such member would
 	// let a token request add one of the client's own choosing, since
 	// ValidateGrant can't tell a client's narrowing from the server's
-	// grant.
+	// grant: the fallback fails open at the token endpoint. A definition
+	// with a server-minted member must set ValidateNarrowing.
 	ValidateNarrowing func(granted, narrowed T) error
 }
 
@@ -236,8 +250,14 @@ func RARGet[T any](values RARValues, def RARDefinition[T]) ([]RARDetail[T], erro
 // validate data an authorization server doesn't trust from a client:
 // RARSet runs on the sending side, encoding data the caller just
 // constructed itself, so there is nothing to validate against.
+//
+// RARSet doesn't HTML-escape '<', '>' and '&' as json.Marshal does.
+// That escaping grows each to six bytes, so details a consent page
+// reads with RARGet and grants re-encoded with RARSet could grow past
+// the MaxBytesPerObject and registry bounds the request was validated
+// within, and be refused when the grant is completed.
 func RARSet[T any](def RARDefinition[T], v T) (json.RawMessage, error) {
-	encoded, err := json.Marshal(v)
+	encoded, err := strictjson.MarshalUnescaped(v)
 	if err != nil {
 		return nil, fmt.Errorf("extension: authorization_details type %q: %w", def.Type, err)
 	}
@@ -251,14 +271,14 @@ func RARSet[T any](def RARDefinition[T], v T) (json.RawMessage, error) {
 		// object either.
 		return nil, fmt.Errorf("extension: authorization_details type %q: value must encode as a JSON object", def.Type)
 	}
-	typeValue, _ := json.Marshal(def.Type) // marshaling a string cannot fail
+	typeValue, _ := strictjson.MarshalUnescaped(def.Type) // marshaling a string cannot fail
 	obj["type"] = typeValue
 	// Every value in obj is already valid JSON — either captured verbatim
 	// by the Unmarshal above (each json.RawMessage decode target only
 	// ever holds one already-well-formed sub-value) or typeValue,
 	// freshly marshaled just above — so re-marshaling the map cannot
 	// fail.
-	out, _ := json.Marshal(obj)
+	out, _ := strictjson.MarshalUnescaped(obj)
 	return out, nil
 }
 
@@ -433,6 +453,13 @@ func (r *RARRegistry) ValidateGrant(requested, granted RARValues) error {
 // hook, then to exact match. Use it rather than ValidateGrant wherever the
 // client, not the authorization server, chooses the narrower objects.
 //
+// Matching is greedy in array order, as ValidateGrant's is, and here the
+// client chooses that order: with a hook that accepts more than an exact
+// match (a lower amount, say), narrowed objects in one order may be
+// refused while the same objects in another order are accepted. It never
+// accepts a narrowing that a matcher trying every assignment would
+// refuse: an accepted narrowing is one valid assignment.
+//
 // Like ValidateGrant's granted, narrowed must already have been produced
 // by this same registry's own Parse.
 func (r *RARRegistry) ValidateNarrowing(granted, narrowed RARValues) error {

extension/rar_test.go

@@ -445,6 +445,22 @@ func TestRARSetRoundTripsThroughRegistryParseAndRARGet(t *testing.T) {
 	}
 }
 
+// TestRARSetDoesNotEscapeHTML: RARSet keeps '<', '>' and '&' as they
+// are, in the value's members and in def.Type alike, rather than growing
+// each to a six-byte \u003c-style escape as json.Marshal does — an
+// object read with RARGet and re-encoded with RARSet must stay within
+// the bounds it was validated against.
+func TestRARSetDoesNotEscapeHTML(t *testing.T) {
+	def := extension.RARDefinition[bareInstructedAmount]{Type: "a<b>&c", MaxObjects: 1, MaxBytesPerObject: 128}
+	raw, err := extension.RARSet(def, bareInstructedAmount{InstructedAmt: "<1&2>"})
+	if err != nil {
+		t.Fatalf("RARSet: %v", err)
+	}
+	if want := `{"instructed_amount":"<1&2>","type":"a<b>&c"}`; string(raw) != want {
+		t.Fatalf("RARSet = %s, want %s", raw, want)
+	}
+}
+
 func TestRARSetRejectsNonObjectValue(t *testing.T) {
 	def := extension.RARDefinition[string]{Type: "payment_initiation", MaxObjects: 1, MaxBytesPerObject: 128}
 	if _, err := extension.RARSet(def, "not an object"); err == nil {

server/attester_trust.go

@@ -117,6 +117,9 @@ func (r RegisteredAttesterKeys) validate() error {
 	if r.Keys == nil {
 		return errors.New("server: dependencies: attester trust: RegisteredAttesterKeys needs Keys, the attesters' own key source (e.g. keys.StaticAttesterKeys)")
 	}
+	if found, ok := depcheck.FindTypedNil(r, map[string]string{"Keys": "Keys"}); ok {
+		return fmt.Errorf("server: dependencies: attester trust: RegisteredAttesterKeys.%s is a nil %s", found.Name, found.Type)
+	}
 	return nil
 }
 
@@ -273,14 +276,16 @@ func (c X5CAttesterChain) validate() error {
 	if c.Revocation == nil {
 		return fmt.Errorf("server: dependencies: attester_trust: X5CAttesterChain.Revocation is required (pass ClientCertificateCRLs{...} or NoClientCertificateRevocationCheck{} to explicitly decline)")
 	}
+	// Before Revocation's own validate: a typed nil there (a nil
+	// *ClientCertificateCRLs) would panic in it.
+	if found, ok := depcheck.FindTypedNil(c, x5cAttesterChainFieldNames); ok {
+		return fmt.Errorf("server: dependencies: attester_trust: X5CAttesterChain.%s is a nil %s", found.Name, found.Type)
+	}
 	if v, ok := c.Revocation.(interface{ validate() error }); ok {
 		if err := v.validate(); err != nil {
 			return fmt.Errorf("server: dependencies: attester_trust: X5CAttesterChain.Revocation: %w", err)
 		}
 	}
-	if found, ok := depcheck.FindTypedNil(c, x5cAttesterChainFieldNames); ok {
-		return fmt.Errorf("server: dependencies: attester_trust: X5CAttesterChain.%s is a nil %s", found.Name, found.Type)
-	}
 	switch c.IssuerBinding {
 	case AttesterIssuerInCertificate, AttesterIssuerByTrustAnchors, AttesterIssuerBoundToAnchor:
 	default:

server/client_certificate_trust.go

@@ -7,6 +7,8 @@ import (
 	"fmt"
 	"slices"
 	"time"
+
+	"github.com/idfoundry/fapigo/internal/depcheck"
 )
 
 // ClientCertificateTrust decides how chain-of-trust for a client
@@ -142,6 +144,11 @@ func (t TrustedClientCAs) validate() error {
 	if t.Revocation == nil {
 		return errors.New("trusted client CAs: revocation is required (pass ClientCertificateCRLs{...} or NoClientCertificateRevocationCheck{} to explicitly decline)")
 	}
+	// Before Revocation's own validate: a typed nil there (a nil
+	// *ClientCertificateCRLs) would panic in it.
+	if found, ok := depcheck.FindTypedNil(t, nil); ok {
+		return fmt.Errorf("trusted client CAs: %s is a nil %s", found.Name, found.Type)
+	}
 	if v, ok := t.Revocation.(interface{ validate() error }); ok {
 		if err := v.validate(); err != nil {
 			return fmt.Errorf("trusted client CAs: %w", err)

server/embedder_access_token.go

@@ -39,6 +39,10 @@ type IssueAccessTokenRequest struct {
 	// authenticated, so no ID token is issued. AuthorizationDetails must
 	// parse under Config.RAR, in types the client is registered for; no
 	// RARPolicy is consulted, since the embedder decided what to grant.
+	// They're parsed with RARRegistry.Parse, not ParseRequest, so no
+	// RARDefinition.ValidateRequest runs: an embedder forwarding a
+	// client's own authorization_details should run
+	// Config.RAR.ParseRequest on them first.
 	Scope                []string
 	AuthorizationDetails []json.RawMessage
 

server/embedder_refresh.go

@@ -50,6 +50,10 @@ type IssueRefreshTokenRequest struct {
 	// ID token. AuthorizationDetails must parse under Config.RAR, in
 	// types the client is registered for; no RARPolicy is consulted,
 	// since the embedder decided what to grant.
+	// They're parsed with RARRegistry.Parse, not ParseRequest, so no
+	// RARDefinition.ValidateRequest runs: an embedder forwarding a
+	// client's own authorization_details should run
+	// Config.RAR.ParseRequest on them first.
 	Scope                []string
 	AuthorizationDetails []json.RawMessage
 

server/rar_html_escape_test.go

@@ -16,18 +16,23 @@ import (
 // rarHTMLObject is a payment detail object full of the characters
 // encoding/json escapes as HTML ('<', '>' and '&' each become a six-byte
 // \u003c-style escape), so re-encoding it with json.Marshal would grow it
-// well past the bound below.
-var rarHTMLObject = `{"type":"payment","actions":["approve"],"amount":"` + strings.Repeat("<>&", 20) + `"}`
+// well past the bound below. Its members are in the order
+// extension.RARSet encodes them, so a grant re-encoded with RARSet is
+// byte for byte the object requested.
+var rarHTMLObject = `{"actions":["approve"],"amount":"` + strings.Repeat("<>&", 20) + `","type":"payment"}`
+
+// rarHTMLDef is the "payment" definition rarHTMLHarness registers, with a
+// MaxBytesPerObject only just above rarHTMLObject's own size.
+var rarHTMLDef = extension.RARDefinition[testPaymentDetail]{
+	Type: "payment", MaxObjects: 1, MaxBytesPerObject: len(rarHTMLObject) + 8,
+}
 
 // rarHTMLHarness registers "payment" with a MaxBytesPerObject only just
 // above rarHTMLObject's own size, and a total bound only just above the
 // one-object array's.
 func rarHTMLHarness(t *testing.T) harness {
 	t.Helper()
-	def := extension.RARDefinition[testPaymentDetail]{
-		Type: "payment", MaxObjects: 1, MaxBytesPerObject: len(rarHTMLObject) + 8,
-	}
-	registry, err := extension.NewRARRegistry(len(rarHTMLObject)+16, 4, def)
+	registry, err := extension.NewRARRegistry(len(rarHTMLObject)+16, 4, rarHTMLDef)
 	if err != nil {
 		t.Fatalf("NewRARRegistry: %v", err)
 	}
@@ -38,6 +43,15 @@ func rarHTMLHarness(t *testing.T) harness {
 // rarHTMLIssueCode pushes a request for exactly rarHTMLObject, grants it
 // as the end user was shown it, and returns the authorization code.
 func rarHTMLIssueCode(t *testing.T, h harness) string {
+	t.Helper()
+	return rarHTMLIssueCodeGranting(t, h, func(*testing.T, server.InteractionRequest) []json.RawMessage {
+		return []json.RawMessage{json.RawMessage(rarHTMLObject)}
+	})
+}
+
+// rarHTMLIssueCodeGranting is rarHTMLIssueCode, granting the
+// authorization_details grant returns for the interaction.
+func rarHTMLIssueCodeGranting(t *testing.T, h harness, grant func(*testing.T, server.InteractionRequest) []json.RawMessage) string {
 	t.Helper()
 	push, err := h.server.PushAuthorizationRequest(context.Background(), server.PushAuthorizationRequest{
 		HTTP: server.FormRequest{Parameters: []server.FormParameter{
@@ -73,7 +87,7 @@ func rarHTMLIssueCode(t *testing.T, h harness) string {
 		Handle: interaction.Handle,
 		Result: server.Authorize(subject, authCtx, server.GrantedAuthorization{
 			Scope:                []string{"openid", "accounts", "offline_access"},
-			AuthorizationDetails: []json.RawMessage{json.RawMessage(rarHTMLObject)},
+			AuthorizationDetails: grant(t, interaction.Interaction),
 		}),
 	})
 	if err != nil {
@@ -135,14 +149,38 @@ func TestAuthorizationDetailsKeepTheirBytesThroughGrantAndRefresh(t *testing.T)
 	}
 }
 
+// TestAuthorizationDetailsKeepTheirBytesThroughRARSet is
+// TestAuthorizationDetailsKeepTheirBytesThroughGrantAndRefresh for a
+// consent page that reads the requested details with extension.RARGet
+// and grants them re-encoded with extension.RARSet, as the RAR guide
+// does: RARSet mustn't grow them past the bounds they were validated
+// against by escaping '<', '>' and '&'.
+func TestAuthorizationDetailsKeepTheirBytesThroughRARSet(t *testing.T) {
+	h := rarHTMLHarness(t)
+	code := rarHTMLIssueCodeGranting(t, h, func(t *testing.T, interaction server.InteractionRequest) []json.RawMessage {
+		t.Helper()
+		details, err := extension.RARGet(interaction.AuthorizationDetails, rarHTMLDef)
+		if err != nil || len(details) != 1 {
+			t.Fatalf("RARGet = %v, %v; want the one requested object", details, err)
+		}
+		granted, err := extension.RARSet(rarHTMLDef, details[0].Fields)
+		if err != nil {
+			t.Fatalf("RARSet: %v", err)
+		}
+		return []json.RawMessage{granted}
+	})
+	result, err := rarExchangeCode(t, h, code, nil)
+	if err != nil {
+		t.Fatalf("ExchangeAuthorizationCode: %v", err)
+	}
+	assertRARHTMLIssued(t, h, result)
+}
+
 // TestEmbedderAuthorizationDetailsKeepTheirBytes is
 // TestAuthorizationDetailsKeepTheirBytesThroughGrantAndRefresh for
 // details an embedder grants itself (IssueRefreshTokenRequest.AuthorizationDetails).
 func TestEmbedderAuthorizationDetailsKeepTheirBytes(t *testing.T) {
-	def := extension.RARDefinition[testPaymentDetail]{
-		Type: "payment", MaxObjects: 1, MaxBytesPerObject: len(rarHTMLObject) + 8,
-	}
-	registry, err := extension.NewRARRegistry(len(rarHTMLObject)+16, 4, def)
+	registry, err := extension.NewRARRegistry(len(rarHTMLObject)+16, 4, rarHTMLDef)
 	if err != nil {
 		t.Fatalf("NewRARRegistry: %v", err)
 	}

server/trust_typed_nil_test.go

@@ -0,0 +1,61 @@
+package server_test
+
+import (
+	"crypto/x509"
+	"strings"
+	"testing"
+
+	"github.com/idfoundry/fapigo/keys"
+	"github.com/idfoundry/fapigo/server"
+)
+
+// TestNewRefusesTypedNilTrustFields covers interface fields nested in
+// AttesterTrust and ClientCertificateTrust holding a typed nil (a nil
+// *ClientCertificateCRLs, say): New refuses each, naming the field,
+// rather than panicking in the field's own validation.
+func TestNewRefusesTypedNilTrustFields(t *testing.T) {
+	x5c := server.X5CAttesterChain{
+		TrustAnchors:  server.StaticAttesterTrustAnchors{Roots: x509.NewCertPool()},
+		IssuerBinding: server.AttesterIssuerInCertificate,
+		Revocation:    (*server.ClientCertificateCRLs)(nil),
+	}
+	for name, tc := range map[string]struct {
+		attestation bool
+		set         func(*server.Dependencies)
+		want        string
+	}{
+		"x5c chain revocation":            {true, func(d *server.Dependencies) { d.AttesterTrust = x5c }, "X5CAttesterChain.Revocation is a nil *ClientCertificateCRLs"},
+		"x5c chain by pointer revocation": {true, func(d *server.Dependencies) { d.AttesterTrust = &x5c }, "X5CAttesterChain.Revocation is a nil *ClientCertificateCRLs"},
+		"registered attester keys": {true, func(d *server.Dependencies) {
+			d.AttesterTrust = server.RegisteredAttesterKeys{Keys: keys.StaticAttesterKeys(nil)}
+		}, "RegisteredAttesterKeys.Keys is a nil keys.StaticAttesterKeys"},
+		"trusted client CAs revocation": {false, func(d *server.Dependencies) {
+			d.ClientCertificateTrust = server.TrustedClientCAs{Roots: x509.NewCertPool(), Revocation: (*server.ClientCertificateCRLs)(nil)}
+		}, "trusted client CAs: revocation is a nil *ClientCertificateCRLs"},
+	} {
+		t.Run(name, func(t *testing.T) {
+			cfg := validConfig(t)
+			if tc.attestation {
+				cfg = validAttestationConfig(t)
+			}
+			deps := validDependencies()
+			tc.set(&deps)
+			assertNewRefuses(t, cfg, deps, tc.want)
+		})
+	}
+}
+
+// assertNewRefuses checks New refuses deps with an error containing
+// want, rather than accepting them or panicking.
+func assertNewRefuses(t *testing.T, cfg server.Config, deps server.Dependencies, want string) {
+	t.Helper()
+	defer func() {
+		if r := recover(); r != nil {
+			t.Fatalf("New panicked: %v", r)
+		}
+	}()
+	_, err := server.New(cfg, deps)
+	if err == nil || !strings.Contains(err.Error(), want) {
+		t.Fatalf("New = %v, want an error containing %q", err, want)
+	}
+}