From 7b25c0cf632e56c6e8b02c4eff6676a1eaeda7e2 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Sun, 1 Nov 2026 08:43:57 +0800 Subject: [PATCH 1/3] fix(extension): encode RARSet output without HTML escaping RARSet re-encoded details with json.Marshal, which escapes '<', '>' and '&' as six-byte <-style sequences. A consent page that reads requested details with RARGet and grants them re-encoded with RARSet, as the RAR guide shows, could therefore grow a request that was validated within MaxBytesPerObject and the registry's total bound past them, and CompleteAuthorization then refused the grant after the user had consented. RARSet now encodes with the same unescaped encoder the server uses for granted and stored details, so the granted bytes and the token's claim match what was validated. Co-Authored-By: Claude Opus 5.5 --- extension/rar.go | 12 +++++-- extension/rar_test.go | 16 +++++++++ server/rar_html_escape_test.go | 60 +++++++++++++++++++++++++++------- 3 files changed, 74 insertions(+), 14 deletions(-) diff --git a/extension/rar.go b/extension/rar.go index ba5fd0d0..72faf5ef 100644 --- a/extension/rar.go +++ b/extension/rar.go @@ -236,8 +236,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 +257,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 } diff --git a/extension/rar_test.go b/extension/rar_test.go index 8f5188fa..c3a9bd4f 100644 --- a/extension/rar_test.go +++ b/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&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&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 { diff --git a/server/rar_html_escape_test.go b/server/rar_html_escape_test.go index 2ae1bff6..1d5eaabb 100644 --- a/server/rar_html_escape_test.go +++ b/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) } From 0403b7e855213c3d6a09b516970981baa551a59c Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Sun, 1 Nov 2026 08:43:58 +0800 Subject: [PATCH 2/3] fix(server): refuse typed-nil trust fields before validating them X5CAttesterChain.validate called Revocation's own validate before its typed-nil check, so New panicked on a nil *ClientCertificateCRLs instead of returning an error. TrustedClientCAs.validate had the same call with no typed-nil check at all. Both now refuse a typed nil in an interface field first, naming the field, and RegisteredAttesterKeys refuses a typed-nil Keys the same way rather than failing on the first attestation. Co-Authored-By: Claude Opus 5.5 --- server/attester_trust.go | 11 ++++-- server/client_certificate_trust.go | 7 ++++ server/trust_typed_nil_test.go | 61 ++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 3 deletions(-) create mode 100644 server/trust_typed_nil_test.go diff --git a/server/attester_trust.go b/server/attester_trust.go index f9d18860..550874d4 100644 --- a/server/attester_trust.go +++ b/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 } @@ -272,14 +275,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: diff --git a/server/client_certificate_trust.go b/server/client_certificate_trust.go index 1260f780..8aa83135 100644 --- a/server/client_certificate_trust.go +++ b/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) diff --git a/server/trust_typed_nil_test.go b/server/trust_typed_nil_test.go new file mode 100644 index 00000000..b717add7 --- /dev/null +++ b/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) + } +} From 165e6f198b4e29b1dfa598d5824b1ea4441cd6dc Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Sun, 1 Nov 2026 08:44:00 +0800 Subject: [PATCH 3/3] docs: describe RAR hook pitfalls and forwarding client details Spell out that a nil ValidateNarrowing falls back to ValidateGrant and fails open at the token endpoint for server-minted members; that typed hooks can't tell an absent or null member from its zero value and compare floats after rounding; that greedy matching depends on the order a client sends objects in; and that an embedder forwarding a client's authorization_details to IssueAccessToken or IssueRefreshToken should run RARRegistry.ParseRequest on them itself. Co-Authored-By: Claude Opus 5.5 --- docs/guides/rar.md | 25 ++++++++++++++++++++++++- extension/rar.go | 23 ++++++++++++++++++++++- server/embedder_access_token.go | 4 ++++ server/embedder_refresh.go | 4 ++++ 4 files changed, 54 insertions(+), 2 deletions(-) diff --git a/docs/guides/rar.md b/docs/guides/rar.md index 2f8783b2..4d22a433 100644 --- a/docs/guides/rar.md +++ b/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 diff --git a/extension/rar.go b/extension/rar.go index 72faf5ef..0b531ca8 100644 --- a/extension/rar.go +++ b/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 } @@ -439,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 { diff --git a/server/embedder_access_token.go b/server/embedder_access_token.go index ceb1f4c2..ef9b626b 100644 --- a/server/embedder_access_token.go +++ b/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 diff --git a/server/embedder_refresh.go b/server/embedder_refresh.go index 97195cc3..07a26036 100644 --- a/server/embedder_refresh.go +++ b/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