diff --git a/client/authorization_details_unescaped_test.go b/client/authorization_details_unescaped_test.go new file mode 100644 index 00000000..18291c49 --- /dev/null +++ b/client/authorization_details_unescaped_test.go @@ -0,0 +1,82 @@ +package client_test + +import ( + "context" + "encoding/json" + "testing" + + "github.com/idfoundry/fapigo/client" + "github.com/idfoundry/fapigo/internal/requestobject" +) + +// detailWithHTMLCharacters is an authorization_details entry holding <, +// > and &, which json.Marshal escapes as \u003c, \u003e and \u0026. +const detailWithHTMLCharacters = `{"type":"payment_initiation","creditorName":"A&B "}` + +var detailsWithHTMLCharacters = []json.RawMessage{json.RawMessage(detailWithHTMLCharacters)} + +// TestAuthorizationDetailsSentWithoutHTMLEscaping covers every request +// that carries authorization_details: each sends the caller's bytes as +// they were, as a form parameter or inside a signed request object, +// rather than re-escaped into a longer, differently spelled value the +// server then keeps byte for byte. +func TestAuthorizationDetailsSentWithoutHTMLEscaping(t *testing.T) { + want := "[" + detailWithHTMLCharacters + "]" + requestObjectParameter := func(t *testing.T, requestJWT string) string { + t.Helper() + obj, err := requestobject.Parse(requestJWT) + if err != nil { + t.Fatalf("parse request object: %v", err) + } + raw, ok := obj.Parameter("authorization_details") + if !ok { + t.Fatalf("request object is missing authorization_details") + } + return string(raw) + } + + t.Run("PAR form", func(t *testing.T) { + c, as, _ := newTestClient(t, false) + if _, err := c.BeginAuthorization(context.Background(), client.BeginAuthorizationRequest{ + Scope: []string{"openid"}, AuthorizationDetails: detailsWithHTMLCharacters, + }); err != nil { + t.Fatalf("BeginAuthorization: %v", err) + } + if got := as.lastPARForm.Get("authorization_details"); got != want { + t.Errorf("authorization_details = %s, want %s", got, want) + } + }) + t.Run("PAR request object", func(t *testing.T) { + c, as, _ := newTestClient(t, true) + if _, err := c.BeginAuthorization(context.Background(), client.BeginAuthorizationRequest{ + Scope: []string{"openid"}, AuthorizationDetails: detailsWithHTMLCharacters, + }); err != nil { + t.Fatalf("BeginAuthorization: %v", err) + } + if got := requestObjectParameter(t, as.lastPARForm.Get("request")); got != want { + t.Errorf("authorization_details = %s, want %s", got, want) + } + }) + t.Run("CIBA request object", func(t *testing.T) { + c, as, _ := newTestClientWithCIBA(t) + if _, err := c.BeginBackchannelAuthentication(context.Background(), client.BeginBackchannelAuthenticationRequest{ + Scope: []string{"openid"}, LoginHint: "user@example.com", AuthorizationDetails: detailsWithHTMLCharacters, + }); err != nil { + t.Fatalf("BeginBackchannelAuthentication: %v", err) + } + if got := requestObjectParameter(t, as.lastBCForm.Get("request")); got != want { + t.Errorf("authorization_details = %s, want %s", got, want) + } + }) + t.Run("client_credentials form", func(t *testing.T) { + c, as := newTestClientForClientCredentials(t, nil) + if _, err := c.RequestClientCredentialsToken(context.Background(), client.ClientCredentialsTokenRequest{ + Scope: []string{"accounts"}, AuthorizationDetails: detailsWithHTMLCharacters, + }); err != nil { + t.Fatalf("RequestClientCredentialsToken: %v", err) + } + if got := as.lastForm.Get("authorization_details"); got != want { + t.Errorf("authorization_details = %s, want %s", got, want) + } + }) +} diff --git a/client/backchannel.go b/client/backchannel.go index ef1d0854..5f10addb 100644 --- a/client/backchannel.go +++ b/client/backchannel.go @@ -16,6 +16,7 @@ import ( "github.com/idfoundry/fapigo/internal/dpop" "github.com/idfoundry/fapigo/internal/par" "github.com/idfoundry/fapigo/internal/requestobject" + "github.com/idfoundry/fapigo/internal/strictjson" "github.com/idfoundry/fapigo/internal/timeutil" "github.com/idfoundry/fapigo/keys" "github.com/idfoundry/fapigo/storage" @@ -339,7 +340,7 @@ func backchannelRequestParameters(req BeginBackchannelAuthenticationRequest) (ma objectParams["requested_expiry"] = encoded } if len(req.AuthorizationDetails) > 0 { - encoded, err := json.Marshal(req.AuthorizationDetails) + encoded, err := strictjson.MarshalUnescaped(req.AuthorizationDetails) if err != nil { return nil, newError(ErrorInternal, "failed to encode authorization_details", err) } diff --git a/client/begin_authorization.go b/client/begin_authorization.go index 45622b3a..6ad793d7 100644 --- a/client/begin_authorization.go +++ b/client/begin_authorization.go @@ -19,6 +19,7 @@ import ( "github.com/idfoundry/fapigo/internal/par" "github.com/idfoundry/fapigo/internal/pkce" "github.com/idfoundry/fapigo/internal/requestobject" + "github.com/idfoundry/fapigo/internal/strictjson" "github.com/idfoundry/fapigo/keys" "github.com/idfoundry/fapigo/storage" ) @@ -505,7 +506,7 @@ func (c *Client) buildPushedRequestForm(ctx context.Context, now time.Time, para var authorizationDetailsRaw json.RawMessage if len(authorizationDetails) > 0 { - encoded, err := json.Marshal(authorizationDetails) + encoded, err := strictjson.MarshalUnescaped(authorizationDetails) if err != nil { return nil, nil, newError(ErrorInternal, "failed to encode authorization_details", err) } diff --git a/client/client_credentials.go b/client/client_credentials.go index 254e8faa..1cb097b8 100644 --- a/client/client_credentials.go +++ b/client/client_credentials.go @@ -9,6 +9,7 @@ import ( "time" fapi "github.com/idfoundry/fapigo" + "github.com/idfoundry/fapigo/internal/strictjson" "github.com/idfoundry/fapigo/keys" "github.com/idfoundry/fapigo/storage" ) @@ -90,7 +91,7 @@ func (c *Client) RequestClientCredentialsToken(ctx context.Context, req ClientCr var authorizationDetailsJSON string if len(req.AuthorizationDetails) > 0 { - encoded, err := json.Marshal(req.AuthorizationDetails) + encoded, err := strictjson.MarshalUnescaped(req.AuthorizationDetails) if err != nil { return ClientCredentialsTokenResult{}, newError(ErrorInternal, "failed to encode authorization_details", err) } diff --git a/internal/requestobject/create.go b/internal/requestobject/create.go index 09d24517..dec1abda 100644 --- a/internal/requestobject/create.go +++ b/internal/requestobject/create.go @@ -11,6 +11,7 @@ import ( fapi "github.com/idfoundry/fapigo" "github.com/idfoundry/fapigo/internal/jose" + "github.com/idfoundry/fapigo/internal/strictjson" ) // jtiSize is the byte length of the random value used to build an @@ -116,7 +117,10 @@ func Create(p CreateParams) (string, error) { claims[k] = encoded } - payload, err := json.Marshal(claims) + // Without HTML escaping: json.Marshal would escape <, > and & in + // every json.RawMessage parameter too, so authorization_details the + // server keeps byte for byte would arrive as \u003c and so on. + payload, err := strictjson.MarshalUnescaped(claims) if err != nil { return "", fmt.Errorf("requestobject: marshal claims: %w", err) } diff --git a/resource/accesstoken.go b/resource/accesstoken.go index 15ab937c..dbbf42c2 100644 --- a/resource/accesstoken.go +++ b/resource/accesstoken.go @@ -13,6 +13,7 @@ import ( "time" fapi "github.com/idfoundry/fapigo" + "github.com/idfoundry/fapigo/internal/depcheck" "github.com/idfoundry/fapigo/internal/resourceerr" "github.com/idfoundry/fapigo/internal/token" "github.com/idfoundry/fapigo/keys" @@ -227,6 +228,11 @@ func NewJWTAccessTokens(issuerKeys keys.IssuerKeySource, issuer fapi.URL, audien // NewVerifier, which refuses a JWTAccessTokens{...} literal it would // fail (or, for a nil IssuerKeys, panic) on every request. func (j JWTAccessTokens) validate() error { + // A typed nil (a nil *keys.JWKSIssuerKeySource, say) isn't == nil, so it + // would pass the check below and panic on the first request. + if found, ok := depcheck.FindTypedNil(j, map[string]string{"IssuerKeys": "issuer keys"}); ok { + return fmt.Errorf("%s is a nil %s", found.Name, found.Type) + } switch { case j.IssuerKeys == nil: return errors.New("issuer keys is required") @@ -390,6 +396,11 @@ func NewOpaqueAccessTokens(store storage.AccessTokenStore, issuer fapi.URL) (Opa // NewVerifier, which refuses an OpaqueAccessTokens{} literal that would // panic, or refuse every token, on every request. func (o OpaqueAccessTokens) validate() error { + // A typed nil (a nil *memstore.AccessTokenStore, say) isn't == nil, + // so it would pass the check below and panic on the first request. + if found, ok := depcheck.FindTypedNil(o, nil); ok { + return fmt.Errorf("%s is a nil %s", found.Name, found.Type) + } switch { case o.Store == nil: return errors.New("store is required") diff --git a/resource/accesstoken_typed_nil_test.go b/resource/accesstoken_typed_nil_test.go new file mode 100644 index 00000000..87be4fca --- /dev/null +++ b/resource/accesstoken_typed_nil_test.go @@ -0,0 +1,61 @@ +package resource_test + +import ( + "strings" + "testing" + "time" + + fapi "github.com/idfoundry/fapigo" + "github.com/idfoundry/fapigo/keys" + "github.com/idfoundry/fapigo/resource" + "github.com/idfoundry/fapigo/storage/memstore" +) + +// TestAccessTokensRefuseTypedNilFields covers a nil pointer in +// JWTAccessTokens.IssuerKeys, OpaqueAccessTokens.Store and a custom +// resolver's AccessTokenAssurance: each is refused at construction, +// naming the field, rather than passing its nil check and panicking on +// the first request (or in the production capability check). +func TestAccessTokensRefuseTypedNilFields(t *testing.T) { + issuer, err := fapi.ParseIssuerURL(testIssuer) + if err != nil { + t.Fatal(err) + } + if _, err := resource.NewJWTAccessTokens((*keys.JWKSIssuerKeySource)(nil), issuer, testIssuer, fapi.ES256, 5*time.Minute, 8); err == nil || !strings.Contains(err.Error(), "issuer keys is a nil *keys.JWKSIssuerKeySource") { + t.Errorf("NewJWTAccessTokens(nil *JWKSIssuerKeySource) error = %v", err) + } + if _, err := resource.NewOpaqueAccessTokens((*memstore.AccessTokenStore)(nil), issuer); err == nil || !strings.Contains(err.Error(), "store is a nil *memstore.AccessTokenStore") { + t.Errorf("NewOpaqueAccessTokens(nil *AccessTokenStore) error = %v", err) + } + + for name, tc := range map[string]struct { + production bool + resolver resource.AccessTokenResolver + want string + }{ + "jwt literal": {false, resource.JWTAccessTokens{ + IssuerKeys: (*keys.JWKSIssuerKeySource)(nil), Issuer: issuer, Audience: testIssuer, + Algorithm: fapi.ES256, MaxTokenLifetime: 5 * time.Minute, MaxKeyCandidates: 8, + }, "access_tokens (JWTAccessTokens): issuer keys is a nil *keys.JWKSIssuerKeySource"}, + "opaque literal by pointer": {false, &resource.OpaqueAccessTokens{ + Store: (*memstore.AccessTokenStore)(nil), Issuer: issuer, + }, "access_tokens (OpaqueAccessTokens): store is a nil *memstore.AccessTokenStore"}, + "declared issuer keys": {true, declaringResolver{declared: resource.AccessTokenAssurance{ + IssuerKeys: (*declaredKeys)(nil), + }}, "AccessTokenAssurance.IssuerKeys is a nil *resource_test.declaredKeys"}, + "declared store": {true, declaringResolver{declared: resource.AccessTokenAssurance{ + Store: (*memstore.AccessTokenStore)(nil), + }}, "AccessTokenAssurance.Store is a nil *memstore.AccessTokenStore"}, + } { + t.Run(name, func(t *testing.T) { + cfg, deps := validConfig(t), validDependencies(t) + if tc.production { + cfg, deps = productionSetup(t) + } + deps.AccessTokens = tc.resolver + if _, err := resource.NewVerifier(cfg, deps); err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("NewVerifier error = %v, want it to contain %q", err, tc.want) + } + }) + } +} diff --git a/resource/assurance.go b/resource/assurance.go index b7bb963c..2a41e77f 100644 --- a/resource/assurance.go +++ b/resource/assurance.go @@ -292,6 +292,9 @@ func productionAccessTokenAssurance(resolver AccessTokenResolver) (AccessTokenAs if declared.IssuerKeys == nil && declared.Store == nil { return AccessTokenAssurance{}, fmt.Errorf("resource: dependencies: access_tokens' AccessTokenAssurance must name its IssuerKeys or Store under AssuranceProduction") } + if found, ok := depcheck.FindTypedNil(declared, map[string]string{"IssuerKeys": "IssuerKeys", "Store": "Store"}); ok { + return AccessTokenAssurance{}, fmt.Errorf("resource: dependencies: access_tokens' AccessTokenAssurance.%s is a nil %s", found.Name, found.Type) + } return declared, nil } return AccessTokenAssurance{}, fmt.Errorf("resource: dependencies: access_tokens must be JWTAccessTokens, OpaqueAccessTokens, or implement resource.AccessTokenResolverAssurance under AssuranceProduction")