From 99cad8720ea3fc47e44322126e4b13949bef62a6 Mon Sep 17 00:00:00 2001 From: Oscar Sanderson Date: Fri, 2 Oct 2026 16:03:39 +0800 Subject: [PATCH] fix(extension): refuse RAR members that differ only in case at any depth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The case-folded duplicate check from #511 looked at a detail's top-level members only. Its rationale was that DisallowUnknownFields leaves nested duplicates nothing to smuggle, but it matches names case-insensitively too. So {"type":"payment","instructedAmount":{"value":"1000.00","VALUE":"1.00",...}} was accepted. encoding/json, and so the consent page, read 1.00, while a case-sensitive reader of the issued token (which carries these bytes) read 1000.00. A lone "ACTIONS" was likewise actions to one reader and no actions at all to the other. - RARRegistry.Parse and ParseGrantedRAR now refuse a repeated member, compared under Unicode simple case folding, in every object at every depth, as ErrDuplicateMember. - Parse and RARGet refuse a member spelled other than its field's json tag, using strictjson's new CheckTaggedFieldCase. Untagged fields keep encoding/json's matching, so detail types without tags work as before. internal/strictjson compared names with strings.ToLower, which misses the long s: encoding/json reads "iſſ" as "iss". It now uses FoldKey, the same fold encoding/json applies, which moves here from extension. Co-Authored-By: Claude Opus 5.5 --- extension/errors.go | 9 ++- extension/rar.go | 100 +++++++++++++++---------- extension/rar_granted.go | 14 ++-- extension/rar_nested_member_test.go | 78 +++++++++++++++++++ internal/strictjson/strictjson.go | 82 +++++++++++++++----- internal/strictjson/strictjson_test.go | 28 +++++++ 6 files changed, 243 insertions(+), 68 deletions(-) create mode 100644 extension/rar_nested_member_test.go diff --git a/extension/errors.go b/extension/errors.go index aa345de8..9b8ccf60 100644 --- a/extension/errors.go +++ b/extension/errors.go @@ -21,10 +21,11 @@ var ( // with a slice T, or vice versa). ErrCardinalityMismatch = errors.New("extension: cardinality does not match the definition's type") - // ErrDuplicateMember indicates a RAR detail object had the same - // top-level JSON member name more than once, compared - // case-insensitively as encoding/json matches names, or spelled its - // "type" member other than exactly "type". + // ErrDuplicateMember indicates a RAR detail object, or an object + // nested at any depth in one, had the same JSON member name more than + // once, compared case-insensitively as encoding/json matches names, + // or that the detail spelled its "type" member other than exactly + // "type". ErrDuplicateMember = errors.New("extension: duplicate JSON member") // ErrRARTooLarge indicates an authorization_details array exceeded diff --git a/extension/rar.go b/extension/rar.go index 07c828eb..faa8a5fa 100644 --- a/extension/rar.go +++ b/extension/rar.go @@ -8,7 +8,8 @@ import ( "reflect" "sort" "strings" - "unicode" + + "github.com/idfoundry/fapigo/internal/strictjson" ) // RARDefinition captures the wire contract for one Rich Authorization @@ -75,9 +76,15 @@ func (d RARDefinition[T]) decodeCheck(raw json.RawMessage) error { return fmt.Errorf("extension: authorization_details type %q: malformed value: %w", d.Type, err) } } + // A member spelled other than its field's json tag — "ACTIONS" for + // "actions" — is one encoding/json reads and a case-sensitive reader + // of the issued token doesn't. + var v T + if err := strictjson.CheckTaggedFieldCase(raw, &v); err != nil { + return fmt.Errorf("extension: authorization_details type %q: malformed value: %w", d.Type, err) + } dec := json.NewDecoder(bytes.NewReader(raw)) dec.DisallowUnknownFields() - var v T if err := dec.Decode(&v); err != nil { return fmt.Errorf("extension: authorization_details type %q: malformed value: %w", d.Type, err) } @@ -134,6 +141,9 @@ func RARGet[T any](values RARValues, def RARDefinition[T]) ([]RARDetail[T], erro out := make([]RARDetail[T], 0, len(raws)) for _, raw := range raws { var v T + if err := strictjson.CheckTaggedFieldCase(raw, &v); err != nil { + return nil, fmt.Errorf("extension: authorization_details type %q: %w", def.Type, err) + } if err := json.Unmarshal(raw, &v); err != nil { return nil, fmt.Errorf("extension: authorization_details type %q: %w", def.Type, err) } @@ -265,7 +275,7 @@ func (r *RARRegistry) Parse(raw json.RawMessage) (RARValues, error) { values := RARValues{byType: make(map[string][]json.RawMessage)} counts := make(map[string]int, len(r.byType)) for _, objRaw := range objects { - if err := checkNoDuplicateTopLevelKeys(objRaw); err != nil { + if err := checkMembers(objRaw); err != nil { return RARValues{}, err } @@ -374,47 +384,34 @@ func checkJSONDepth(raw []byte, maxDepth int) error { } } -// checkNoDuplicateTopLevelKeys reports whether raw — expected to be a -// JSON object — repeats a top-level member name, or spells "type" other -// than exactly so. Names that differ only in case count as repeats: -// encoding/json matches a member to a field case-insensitively, so of -// {"amount":"1","AMOUNT":"1000"} it reads the last, where a -// case-sensitive reader of the same issued token reads the first, and it -// reads {"TYPE":"b"} as type "b" where that reader finds no type at all. -// It does not recurse -// into nested objects/arrays; encoding/json's own decode already applies -// DisallowUnknownFields for whatever shape a RARDefinition's T declares, -// so a duplicate nested member can only smuggle in a value the target -// struct doesn't expose to begin with. -// foldKey maps name to one spelling shared by every name it equals under -// Unicode simple case folding (strings.EqualFold), as encoding/json -// compares member names: each rune becomes the least of its fold orbit, -// so "K", "k" and the Kelvin sign all map to "K". -func foldKey(name string) string { - var b strings.Builder - for _, r := range name { - least := r - for f := unicode.SimpleFold(r); f != r; f = unicode.SimpleFold(f) { - least = min(least, f) - } - b.WriteRune(least) - } - return b.String() -} - -func checkNoDuplicateTopLevelKeys(raw json.RawMessage) error { +// checkMembers reports whether raw — expected to be a JSON object — +// repeats a member name in any object at any depth, or spells its "type" +// other than exactly so. Names that differ only in case count as +// repeats: encoding/json matches a member to a field +// case-insensitively, so of {"amount":"1","AMOUNT":"1000"} it reads the +// last, where a case-sensitive reader of the same issued token reads the +// first — nested as much as at the top, since DisallowUnknownFields +// matches names the same way — and it reads {"TYPE":"b"} as type "b" +// where that reader finds no type at all. A lone member spelled other +// than its field's tag is RARDefinition.decodeCheck's to refuse, since +// only the detail type knows its fields. +func checkMembers(raw json.RawMessage) error { dec := json.NewDecoder(bytes.NewReader(raw)) tok, err := dec.Token() if err != nil { return fmt.Errorf("extension: %w", err) } - delim, ok := tok.(json.Delim) - if !ok || delim != '{' { + if delim, ok := tok.(json.Delim); !ok || delim != '{' { return fmt.Errorf("extension: authorization_details object must be a JSON object") } + return checkObjectMembers(dec, true) +} - seen := make(map[string]string) // by foldKey, the name as spelled - typeKey := foldKey("type") +// checkObjectMembers checks the members of the object whose "{" dec has +// just read, and everything inside them, through its "}". +func checkObjectMembers(dec *json.Decoder, top bool) error { + seen := make(map[string]string) // by strictjson.FoldKey, the name as spelled + typeKey := strictjson.FoldKey("type") for dec.More() { keyTok, err := dec.Token() if err != nil { @@ -424,19 +421,40 @@ func checkNoDuplicateTopLevelKeys(raw json.RawMessage) error { if !ok { return fmt.Errorf("extension: malformed object key") } - folded := foldKey(key) + folded := strictjson.FoldKey(key) if first, dup := seen[folded]; dup { return fmt.Errorf("%w: %q and %q", ErrDuplicateMember, first, key) } - if folded == typeKey && key != "type" { + if top && folded == typeKey && key != "type" { return fmt.Errorf("%w: %q is not \"type\"", ErrDuplicateMember, key) } seen[folded] = key + if err := checkValueMembers(dec); err != nil { + return err + } + } + _, err := dec.Token() // "}" + return err +} - var skip json.RawMessage - if err := dec.Decode(&skip); err != nil { - return fmt.Errorf("extension: %w", err) +// checkValueMembers checks the next value dec reads: any object in it, +// at any depth. +func checkValueMembers(dec *json.Decoder) error { + tok, err := dec.Token() + if err != nil { + return fmt.Errorf("extension: %w", err) + } + switch tok { + case json.Delim('{'): + return checkObjectMembers(dec, false) + case json.Delim('['): + for dec.More() { + if err := checkValueMembers(dec); err != nil { + return err + } } + _, err := dec.Token() // "]" + return err } return nil } diff --git a/extension/rar_granted.go b/extension/rar_granted.go index 523c7c98..0b85a39d 100644 --- a/extension/rar_granted.go +++ b/extension/rar_granted.go @@ -21,11 +21,13 @@ const AuthorizationDetailsClaim = "authorization_details" // // A token without the claim was granted none: the result is empty. A // claim that isn't an array of objects each with a string "type" is an -// error, never an empty grant read as "nothing to check". Each object's -// members are checked as RARRegistry.Parse checks a request's (no member -// twice, compared as encoding/json compares names, and "type" spelled -// exactly), so this reads the claim as any case-sensitive reader of the -// same token would. It doesn't otherwise validate the details: the +// error, never an empty grant read as "nothing to check". Every object +// in it, at any depth, is checked as RARRegistry.Parse checks a +// request's: no member twice, compared as encoding/json compares names, +// and "type" spelled exactly. RARGet then refuses a member spelled other +// than its field's json tag. So the claim reads as any case-sensitive +// reader of the same token would read it. It doesn't otherwise validate +// the details: the // authorization server did that when it granted them, and RARGet decodes // only the types asked for, so a type this resource server doesn't know // is left alone. @@ -39,7 +41,7 @@ func ParseGrantedRAR(claim json.RawMessage) (RARValues, error) { } byType := make(map[string][]json.RawMessage, len(objects)) for i, obj := range objects { - if err := checkNoDuplicateTopLevelKeys(obj); err != nil { + if err := checkMembers(obj); err != nil { return RARValues{}, fmt.Errorf("extension: granted authorization_details object %d: %w", i, err) } var head rarObjectHead diff --git a/extension/rar_nested_member_test.go b/extension/rar_nested_member_test.go new file mode 100644 index 00000000..232f5a56 --- /dev/null +++ b/extension/rar_nested_member_test.go @@ -0,0 +1,78 @@ +package extension_test + +import ( + "encoding/json" + "errors" + "testing" + + "github.com/idfoundry/fapigo/extension" +) + +type nestedAmount struct { + Value string `json:"value"` + Currency string `json:"currency"` +} + +type nestedPayment struct { + InstructedAmount nestedAmount `json:"instructedAmount"` + Actions []string `json:"actions,omitempty"` + Creditors []nestedAmount `json:"creditors,omitempty"` + Reference untaggedNested `json:"reference,omitempty"` +} + +// untaggedNested has no json tags: encoding/json matches its members to +// the Go names in any case, as it always has. +type untaggedNested struct { + Text string +} + +var nestedPaymentType = extension.RARDefinition[nestedPayment]{Type: "payment", MaxObjects: 1, MaxBytesPerObject: 512} + +// TestRARParseRejectsNestedCaseVariants covers members below the top +// level that encoding/json reads differently from a case-sensitive +// reader of the issued token: a second "value" spelled "VALUE" makes the +// consent page show 1.00 where that reader sees 1000.00, and a lone +// "ACTIONS" is actions to one and no actions at all to the other. +func TestRARParseRejectsNestedCaseVariants(t *testing.T) { + reg, err := extension.NewRARRegistry(4096, 5, nestedPaymentType) + if err != nil { + t.Fatal(err) + } + for name, detail := range map[string]string{ + "value and VALUE": `{"type":"payment","instructedAmount":{"value":"1000.00","currency":"EUR","VALUE":"1.00"}}`, + "value twice": `{"type":"payment","instructedAmount":{"value":"1000.00","currency":"EUR","value":"1.00"}}`, + "in an array": `{"type":"payment","instructedAmount":{"value":"1.00","currency":"EUR"},"creditors":[{"value":"1.00","Value":"9.00","currency":"EUR"}]}`, + "a lone ACTIONS": `{"type":"payment","instructedAmount":{"value":"1.00","currency":"EUR"},"ACTIONS":["read"]}`, + "a lone nested VALUE": `{"type":"payment","instructedAmount":{"VALUE":"1.00","currency":"EUR"}}`, + "long s for s in currency": "{\"type\":\"payment\",\"instructedAmount\":{\"value\":\"1.00\",\"currenſy\":\"EUR\"}}", + } { + if _, err := reg.Parse(json.RawMessage(`[` + detail + `]`)); err == nil { + t.Errorf("%s: Parse = nil error, want refusal", name) + } + } + for name, detail := range map[string]string{ + "exact names": `{"type":"payment","instructedAmount":{"value":"1.00","currency":"EUR"},"actions":["read"]}`, + "an untagged field, as before": `{"type":"payment","instructedAmount":{"value":"1.00","currency":"EUR"},"reference":{"text":"inv-1"}}`, + } { + if _, err := reg.Parse(json.RawMessage(`[` + detail + `]`)); err != nil { + t.Errorf("%s: Parse = %v, want nil", name, err) + } + } +} + +// TestGrantedRARRejectsNestedCaseVariants covers the resource side: a +// claim with a nested repeat is refused by ParseGrantedRAR, and a lone +// variant by RARGet, which knows the detail type. +func TestGrantedRARRejectsNestedCaseVariants(t *testing.T) { + _, err := extension.ParseGrantedRAR(json.RawMessage(`[{"type":"payment","instructedAmount":{"value":"1000.00","VALUE":"1.00","currency":"EUR"}}]`)) + if !errors.Is(err, extension.ErrDuplicateMember) { + t.Errorf("ParseGrantedRAR(nested repeat) = %v, want ErrDuplicateMember", err) + } + granted, err := extension.ParseGrantedRAR(json.RawMessage(`[{"type":"payment","instructedAmount":{"value":"1.00","currency":"EUR"},"ACTIONS":["read"]}]`)) + if err != nil { + t.Fatalf("ParseGrantedRAR: %v", err) + } + if _, err := extension.RARGet(granted, nestedPaymentType); err == nil { + t.Error("RARGet(a lone ACTIONS) = nil error, want refusal") + } +} diff --git a/internal/strictjson/strictjson.go b/internal/strictjson/strictjson.go index 6537b766..7807ebad 100644 --- a/internal/strictjson/strictjson.go +++ b/internal/strictjson/strictjson.go @@ -19,6 +19,7 @@ import ( "fmt" "reflect" "strings" + "unicode" ) // Unmarshal is json.Unmarshal, after CheckFieldCase. @@ -39,16 +40,46 @@ func CheckFieldCase(data []byte, v any) error { if t == nil { return nil } - return check(data, t) + return check(data, t, false) } -func check(raw []byte, t reflect.Type) error { +// CheckTaggedFieldCase is CheckFieldCase for fields with an explicit +// json tag only: a field without one has no spelling of its own on the +// wire, so encoding/json's case-insensitive match to its Go name is +// left alone. It is for types an application defines, which needn't tag +// every field. +func CheckTaggedFieldCase(data []byte, v any) error { + t := reflect.TypeOf(v) + if t == nil { + return nil + } + return check(data, t, true) +} + +// FoldKey maps name to one spelling shared by every name it equals under +// Unicode simple case folding (strings.EqualFold), as encoding/json +// compares member names: each rune becomes the least of its fold orbit, +// so "K", "k" and the Kelvin sign all map to "K", and "s" and "ſ" to +// "S". +func FoldKey(name string) string { + var b strings.Builder + for _, r := range name { + least := r + for f := unicode.SimpleFold(r); f != r; f = unicode.SimpleFold(f) { + least = min(least, f) + } + b.WriteRune(least) + } + return b.String() +} + +func check(raw []byte, t reflect.Type, taggedOnly bool) error { for t.Kind() == reflect.Pointer { t = t.Elem() } switch t.Kind() { case reflect.Struct: - return checkStruct(raw, t) + return checkStruct(raw, t, taggedOnly) case reflect.Slice, reflect.Array: if t.Elem().Kind() == reflect.Uint8 { // []byte decodes from a base64 string return nil @@ -58,7 +89,7 @@ func check(raw []byte, t reflect.Type) error { return nil } for _, e := range elems { - if err := check(e, t.Elem()); err != nil { + if err := check(e, t.Elem(), taggedOnly); err != nil { return err } } @@ -68,7 +99,7 @@ func check(raw []byte, t reflect.Type) error { return nil } for _, m := range members { - if err := check(m, t.Elem()); err != nil { + if err := check(m, t.Elem(), taggedOnly); err != nil { return err } } @@ -76,7 +107,7 @@ func check(raw []byte, t reflect.Type) error { return nil } -func checkStruct(raw []byte, t reflect.Type) error { +func checkStruct(raw []byte, t reflect.Type, taggedOnly bool) error { if t == reflect.TypeOf(json.RawMessage{}) { return nil } @@ -90,27 +121,43 @@ func checkStruct(raw []byte, t reflect.Type) error { fields := fieldTypes(t) byFold := make(map[string]string, len(fields)) for name := range fields { - byFold[strings.ToLower(name)] = name + byFold[FoldKey(name)] = name } for name, value := range members { - if ft, ok := fields[name]; ok { - if err := check(value, ft); err != nil { + if f, ok := fields[name]; ok { + if err := check(value, f.typ, taggedOnly); err != nil { return err } continue } - if exact, ok := byFold[strings.ToLower(name)]; ok { - return fmt.Errorf("strictjson: member %q is not %q: JSON member names are case-sensitive", name, exact) + exact, ok := byFold[FoldKey(name)] + if !ok { + continue + } + if taggedOnly && !fields[exact].tagged { + // encoding/json decodes it into the untagged field: check + // what's inside. + if err := check(value, fields[exact].typ, taggedOnly); err != nil { + return err + } + continue } + return fmt.Errorf("strictjson: member %q is not %q: JSON member names are case-sensitive", name, exact) } return nil } +// field is a struct field as JSON sees it. +type field struct { + typ reflect.Type + tagged bool // named by a json tag, not by its Go name +} + // fieldTypes maps each JSON member name t decodes (following // encoding/json's naming, including promoted fields of embedded -// structs) to its field type. -func fieldTypes(t reflect.Type) map[string]reflect.Type { - out := map[string]reflect.Type{} +// structs) to its field. +func fieldTypes(t reflect.Type) map[string]field { + out := map[string]field{} for i := 0; i < t.NumField(); i++ { f := t.Field(i) tag := f.Tag.Get("json") @@ -125,10 +172,11 @@ func fieldTypes(t reflect.Type) map[string]reflect.Type { if !f.IsExported() { continue } - if name == "" { + tagged := name != "" + if !tagged { name = f.Name } - out[name] = f.Type + out[name] = field{typ: f.Type, tagged: tagged} } return out } @@ -148,7 +196,7 @@ func untaggedEmbeddedStruct(f reflect.StructField, name string) (reflect.Type, b } // addMissing adds to out each of from's members out doesn't already have. -func addMissing(out, from map[string]reflect.Type) { +func addMissing(out, from map[string]field) { for k, v := range from { if _, exists := out[k]; !exists { out[k] = v diff --git a/internal/strictjson/strictjson_test.go b/internal/strictjson/strictjson_test.go index 518387d9..3bb35643 100644 --- a/internal/strictjson/strictjson_test.go +++ b/internal/strictjson/strictjson_test.go @@ -91,3 +91,31 @@ func TestUnmarshalLeavesMalformedJSONToDecoder(t *testing.T) { t.Fatalf("mistyped containers: %v", err) } } + +// TestCheckFieldCaseFoldsAsEncodingJSONDoes covers a member that only +// Unicode case folding makes equal to a field — "ſ" (long s) for "s" — +// which encoding/json matches and a lowercase comparison misses. +func TestCheckFieldCaseFoldsAsEncodingJSONDoes(t *testing.T) { + var v struct { + Iss string `json:"iss"` + } + if err := CheckFieldCase([]byte("{\"iſſ\":\"https://evil.example\"}"), &v); err == nil { + t.Error("CheckFieldCase(\"iſſ\" for iss) = nil error") + } +} + +func TestCheckTaggedFieldCase(t *testing.T) { + var v struct { + Tagged string `json:"amount"` + Untagged string + } + if err := CheckTaggedFieldCase([]byte(`{"AMOUNT":"1"}`), &v); err == nil { + t.Error(`CheckTaggedFieldCase("AMOUNT" for a tagged field) = nil error`) + } + if err := CheckTaggedFieldCase([]byte(`{"amount":"1","untagged":"x"}`), &v); err != nil { + t.Errorf("CheckTaggedFieldCase(an untagged field in another case) = %v, want nil", err) + } + if err := CheckFieldCase([]byte(`{"untagged":"x"}`), &v); err == nil { + t.Error("CheckFieldCase(an untagged field in another case) = nil error: it checks every field") + } +}