fix: send authorization_details unescaped from the client; refuse typed-nil access-token sources

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 <Ltd>"}`
+
+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)
+		}
+	})
+}

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)
 		}

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)
 		}

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)
 		}

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)
 	}

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")

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)
+			}
+		})
+	}
+}

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")