fix(server): native-redirect review fixes before v0.44.0

ARCHITECTURE.md

@@ -134,12 +134,15 @@ identically:
   `MarshalText()` all redact; `Reveal()` is the only way to get the raw
   value out. `client.TokenSet`, `server.TokenResult` and any resource
   claim that carries a raw token value all use it.
-- **`URL`** (constructed via `ParseIssuerURL` / `ParseEndpointURL`, not a
-  bare `string`) — enforces absolute, HTTPS (except an explicitly
-  enabled loopback development mode), no fragment, no embedded
+- **`URL`** (constructed via `ParseIssuerURL` / `ParseEndpointURL` /
+  `ParseRedirectURL`, not a bare `string`) — enforces absolute, HTTPS
+  (except an explicitly enabled loopback exception, or, for a native
+  app's redirect, a private-use scheme), no fragment, no embedded
   credentials, normalized host. Registered redirect URIs are compared
   under OAuth registration semantics, never generic URL equivalence or
-  automatic normalization — see `fapi.RegisteredRedirectURI`.
+  automatic normalization — see `fapi.RegisteredRedirectURI` — but for
+  a native client's loopback redirect URI, which matches on any port
+  (RFC 8252 §7.3, §8.4).
 
 `SignatureAlgorithm` is a closed enum (`ES256`, `PS256`, ...), never a
 bare string accepted from a caller or read directly out of a JWT header

GETTING_STARTED.md

@@ -115,8 +115,11 @@ A mobile or desktop app — a wallet, say — registers with
 redirect URIs RFC 8252 gives native apps, in production too: a
 private-use scheme in reverse-domain form
 (`com.example.wallet:/callback`), or loopback http to `127.0.0.1` or
-`[::1]`, which matches on whatever port the app listens on. A web
-client can use neither. A native client still authenticates like any
+`[::1]`, which matches on whatever port the app listens on. Write the
+private-use form with a single slash, as RFC 8252 §7.1 does:
+`com.example.wallet:/callback`, not `com.example.wallet://callback`. In
+production a web client can use neither; under development assurance
+it may use loopback http, matched exactly. A native client still authenticates like any
 other: give each app instance its own credentials, as attestation-based
 client authentication does.
 

README.md

@@ -46,6 +46,7 @@ variants and OAuth 2.0 attestation-based client authentication.
 - JAR / JARM · RAR (RFC 9396) · CIBA (poll & ping delivery)
 - Refresh tokens (not rotated, per FAPI 2.0) and whole-grant revocation
 - OpenID Connect: the `claims` parameter with per-claim consent, `acr_values` and enforced `max_age`, signed and encrypted ID tokens and UserInfo
+- Native apps (RFC 8252): private-use URI scheme and any-port loopback redirect URIs, for clients registered as native
 - Grants you serve yourself at the token endpoint (OpenID4VCI's `pre-authorized_code`, say), with the server's own client authentication and DPoP/mTLS checks
 - OpenID Federation 1.0 (trust chains, automatic client registration, trust marks)
 - OpenID Certified™ for OP, RP and FAPI-CIBA OP conformance profiles — see below
@@ -276,10 +277,12 @@ role is tested against the OpenID Foundation conformance suite.
 - [RFC 8705 — Mutual TLS Client Authentication and Certificate-Bound Access Tokens][mtls]
 - [OpenID Connect Client-Initiated Backchannel Authentication (CIBA) Core 1.0][ciba]
 - [RFC 9396 — Rich Authorization Requests][rar]
+- [RFC 8252 — OAuth 2.0 for Native Apps][native]
 
 [fapi2]: https://openid.net/specs/fapi-security-profile-2_0-final.html
 [fapi2-sign]: https://openid.net/specs/fapi-2_0-message-signing.html
 [par]: https://www.rfc-editor.org/info/rfc9126
+[native]: https://www.rfc-editor.org/info/rfc8252
 [dpop]: https://www.rfc-editor.org/info/rfc9449
 [mtls]: https://www.rfc-editor.org/info/rfc8705
 [ciba]: https://openid.net/specs/openid-client-initiated-backchannel-authentication-core-1_0.html

redirect_url_test.go

@@ -1,6 +1,7 @@
 package fapi_test
 
 import (
+	"strings"
 	"testing"
 
 	fapi "github.com/idfoundry/fapigo"
@@ -17,6 +18,8 @@ func TestParseRedirectURL(t *testing.T) {
 		"myapp:/cb":                                   false, // not a reverse domain (RFC 8252 §8.4)
 		"com.example.app:cb":                          false, // opaque, no path
 		"com.example.app://host/cb":                   false, // an authority
+		"com.example.app:///cb":                       false, // an empty authority
+		"com.example.app:////evil.example/cb":         false,
 		"com..app:/cb":                                false,
 		"com.-example.app:/cb":                        false,
 		"com.example.app:/cb#frag":                    false,
@@ -58,3 +61,17 @@ func FuzzParseRedirectURL(f *testing.F) {
 		}
 	})
 }
+
+// TestParseRedirectURLSaysWhatToChange covers the two refusals an
+// integrator meets: the common two-slash form, and a private-use URI
+// passed without the option.
+func TestParseRedirectURLSaysWhatToChange(t *testing.T) {
+	_, err := fapi.ParseRedirectURL("com.example.app://callback", fapi.AllowPrivateUseScheme())
+	if err == nil || !strings.Contains(err.Error(), "com.example.app:/path") {
+		t.Errorf("ParseRedirectURL(two slashes) = %v, want it to show the single-slash form", err)
+	}
+	_, err = fapi.ParseRedirectURL("com.example.app:/callback")
+	if err == nil || strings.Contains(err.Error(), "must be absolute") {
+		t.Errorf("ParseRedirectURL(no host, no option) = %v, want it to say the host is missing, not that it isn't absolute", err)
+	}
+}

server/assurance.go

@@ -19,9 +19,11 @@ const (
 	// AssuranceDevelopment permits a configuration meant for local
 	// development only — most importantly, it does not require an
 	// AuditSink or a store that declares storage.StoreAssurance
-	// capabilities. It also accepts loopback http redirect URIs
-	// (RFC 8252 §7.3, e.g. "http://localhost:8080/callback"), which
-	// AssuranceProduction refuses at the pushed authorization request.
+	// capabilities. It also accepts a web client's loopback http redirect
+	// URIs (RFC 8252 §7.3, e.g. "http://localhost:8080/callback"), which
+	// AssuranceProduction refuses at the pushed authorization request. A
+	// client registered as storage.ApplicationTypeNative may use loopback
+	// http, and a private-use scheme, under either level.
 	AssuranceDevelopment
 
 	// AssuranceProduction rejects a configuration missing anything this
@@ -66,10 +68,11 @@ const (
 	// token, jti and DPoP nonce this server issues is only as
 	// unguessable as that reader, and nothing about an io.Reader says
 	// whether it is a CSPRNG.
-	// Unlike every check above, which New performs once, a client's
+	// Unlike every check above, which New performs once, a web client's
 	// loopback http redirect URI is refused per request, at the pushed
 	// authorization request, as invalid_request — redirect URIs belong
-	// to client registrations, which New never sees.
+	// to client registrations, which New never sees. A native client's
+	// (storage.ApplicationTypeNative) is accepted, as FAPI 2.0 allows.
 	AssuranceProduction
 )
 

server/authorization.go

@@ -207,6 +207,11 @@ func (s *Server) BuildAuthorizationErrorRedirect(ctx context.Context, client sto
 	if !client.HasRedirectURI(redirectURI) {
 		return fapi.URL{}, newError(ErrorInvalidRequest, 400, "redirect_uri is not registered for this client", nil)
 	}
+	// What the pushed authorization request would have held it to: this
+	// redirect never went through one.
+	if _, err := s.parseRedirectURI(client, redirectURI); err != nil {
+		return fapi.URL{}, newError(ErrorInvalidRequest, 400, "redirect_uri is not an acceptable redirect destination", err)
+	}
 	dest, buildErr := s.buildAuthorizationResponse(ctx, client.ID(), redirectURI, map[string]string{
 		"error": errorCode, "state": state, "error_description": description,
 	})

server/complete_authorization.go

@@ -232,9 +232,10 @@ func (s *Server) buildAuthorizationResponse(ctx context.Context, clientID fapi.C
 		base.RawQuery = q.Encode()
 	}
 
-	// The pushed authorization request already held redirect_uri to the
-	// client's registered type (parseRedirectURI); this parse only turns
-	// the stored value, with the response parameters added, into a URL.
+	// The caller already held redirect_uri to the client's registered type
+	// (parseRedirectURI): the pushed authorization request, or
+	// BuildAuthorizationErrorRedirect. This parse only turns the value,
+	// with the response parameters added, into a URL.
 	destination, err := fapi.ParseRedirectURL(base.String(), fapi.AllowLoopbackHTTP(), fapi.AllowPrivateUseScheme())
 	if err != nil {
 		return fapi.URL{}, newError(ErrorServerError, 500, "failed to construct redirect destination", err)
@@ -259,10 +260,21 @@ func (s *Server) parseRedirectURI(client storage.RegisteredClient, raw string) (
 	if client.ApplicationType() == storage.ApplicationTypeNative {
 		return fapi.ParseRedirectURL(raw, fapi.AllowLoopbackHTTP(), fapi.AllowPrivateUseScheme())
 	}
+	var u fapi.URL
+	var err error
 	if s.cfg.Assurance == AssuranceProduction {
-		return fapi.ParseRedirectURL(raw)
+		u, err = fapi.ParseRedirectURL(raw)
+	} else {
+		u, err = fapi.ParseRedirectURL(raw, fapi.AllowLoopbackHTTP())
+	}
+	if err != nil {
+		if _, nativeErr := fapi.ParseRedirectURL(raw, fapi.AllowLoopbackHTTP(), fapi.AllowPrivateUseScheme()); nativeErr == nil {
+			// The mistake a native app's integrator makes first: say
+			// which registration would admit it.
+			return fapi.URL{}, fmt.Errorf("%w (a native app's redirect URI: acceptable for a client registered with ApplicationType storage.ApplicationTypeNative)", err)
+		}
 	}
-	return fapi.ParseRedirectURL(raw, fapi.AllowLoopbackHTTP())
+	return u, err
 }
 
 func validateGrantedScopeSubset(granted []string, requestedSpaceDelimited string) error {

server/native_redirect_test.go

@@ -2,8 +2,11 @@
 
 import (
 	"context"
+	"strings"
 	"testing"
 
+	fapi "github.com/idfoundry/fapigo"
+
 	"github.com/idfoundry/fapigo/internal/clientassertion"
 	"github.com/idfoundry/fapigo/server"
 	"github.com/idfoundry/fapigo/storage"
@@ -119,3 +122,55 @@
 		t.Errorf("response query = %v, want a JARM response", q)
 	}
 }
+
+// TestBuildAuthorizationErrorRedirectHoldsTheRedirectPolicy covers the
+// error redirect a caller builds without a pushed authorization request:
+// it gets the same redirect policy PAR applies. Under production, a web
+// client's registered loopback or private-use URI is refused; a native
+// client's is accepted.
+func TestBuildAuthorizationErrorRedirectHoldsTheRedirectPolicy(t *testing.T) {
+	for name, tc := range map[string]struct {
+		uri     string
+		appType storage.ApplicationType
+		ok      bool
+	}{
+		"web, loopback":       {"http://127.0.0.1/callback", storage.ApplicationTypeWeb, false},
+		"web, localhost":      {"http://localhost/callback", storage.ApplicationTypeWeb, false},
+		"web, private-use":    {testPrivateUseRedirectURI, storage.ApplicationTypeWeb, false},
+		"native, private-use": {testPrivateUseRedirectURI, storage.ApplicationTypeNative, true},
+		"native, loopback":    {"http://127.0.0.1/callback", storage.ApplicationTypeNative, true},
+	} {
+		t.Run(name, func(t *testing.T) {
+			h := newHarnessWithApplicationType(t, server.ProfileFAPISecurity, true, testRedirectURI, server.AssuranceProduction, storage.ApplicationTypeWeb)
+			client, err := storage.NewRegisteredClient(storage.RegisteredClientConfig{
+				ID: testClientID, RedirectURIs: []fapi.RegisteredRedirectURI{fapi.RegisteredRedirectURI(tc.uri)},
+				ClientAssertionAlgorithm: fapi.ES256, AllowedScopes: []string{"openid"}, ApplicationType: tc.appType,
+			})
+			if err != nil {
+				t.Fatalf("NewRegisteredClient: %v", err)
+			}
+			_, err = h.server.BuildAuthorizationErrorRedirect(context.Background(), client, tc.uri, "s", "access_denied", "")
+			if got := err == nil; got != tc.ok {
+				t.Errorf("BuildAuthorizationErrorRedirect(%q) = %v, want accepted %v", tc.uri, err, tc.ok)
+			}
+		})
+	}
+}
+
+// TestPARNamesTheNativeRegistration covers the refusal a native app's
+// integrator meets first — a native redirect URI on a client registered
+// as web — naming the registration that admits it, for the logs.
+func TestPARNamesTheNativeRegistration(t *testing.T) {
+	h := newHarnessWithApplicationType(t, server.ProfileFAPISecurity, true, testPrivateUseRedirectURI, server.AssuranceProduction, storage.ApplicationTypeWeb)
+	_, err := pushWithRedirectURI(t, h, testPrivateUseRedirectURI)
+	if code := serverErrorCode(t, err); code != server.ErrorInvalidRequest {
+		t.Fatalf("error code = %q, want %q", code, server.ErrorInvalidRequest)
+	}
+	if !strings.Contains(err.Error(), "ApplicationTypeNative") {
+		t.Errorf("cause = %v, want it to name ApplicationTypeNative", err)
+	}
+	h = newHarnessWithApplicationType(t, server.ProfileFAPISecurity, true, "http://rp.example/callback", server.AssuranceProduction, storage.ApplicationTypeWeb)
+	if _, err := pushWithRedirectURI(t, h, "http://rp.example/callback"); err == nil || strings.Contains(err.Error(), "ApplicationTypeNative") {
+		t.Errorf("cause = %v, want no native hint for a URI no registration admits", err)
+	}
+}

storage/client_repository.go

@@ -51,7 +51,8 @@ const (
 	// RFC 8252's native-app redirect URIs, in production as well:
 	//
 	//   - a private-use URI scheme (RFC 8252 §7.1), a domain name in
-	//     reverse order: com.example.app:/callback;
+	//     reverse order, then a single slash: com.example.app:/callback,
+	//     not com.example.app://callback;
 	//   - loopback http (§7.3) to the IP literal 127.0.0.1 or [::1], never
 	//     "localhost" (§8.3), matched on any port at request time;
 	//   - a claimed https URI (§7.2), as for a web application.
@@ -791,7 +792,8 @@ func (c RegisteredClient) AllowsAuthorizationCodeGrant() bool { return len(c.red
 
 // HasRedirectURI reports whether candidate is exactly one of this
 // client's registered redirect URIs (RegisteredRedirectURI.Equal
-// semantics — exact match, no normalization).
+// semantics — exact match, no normalization), but for a native client's
+// loopback redirect URI, which matches on any port (RFC 8252 §7.3).
 func (c RegisteredClient) HasRedirectURI(candidate string) bool {
 	for _, u := range c.redirectURIs {
 		if u.Equal(candidate) {
@@ -839,8 +841,9 @@ func checkApplicationType(cfg RegisteredClientConfig) error {
 // isLoopbackLiteral reports whether host is the IP literal 127.0.0.1 or
 // ::1, the loopback addresses RFC 8252 §7.3 names.
 func isLoopbackLiteral(host string) bool {
-	ip := net.ParseIP(host)
-	return ip != nil && (ip.Equal(net.IPv4(127, 0, 0, 1)) || ip.Equal(net.IPv6loopback))
+	// Spelled exactly: net.IP.Equal would also take the IPv4-mapped
+	// ::ffff:127.0.0.1.
+	return host == "127.0.0.1" || host == "::1"
 }
 
 // loopbackMatchesAnyPort reports whether candidate is the loopback http

storage/enums.go

@@ -171,3 +171,38 @@ func ParseBackchannelTokenDeliveryMode(s string) (BackchannelTokenDeliveryMode,
 		return 0, fmt.Errorf("storage: unrecognized backchannel token delivery mode %q", s)
 	}
 }
+
+// String returns the canonical wire value for t — OpenID Connect Dynamic
+// Client Registration's application_type, "web" or "native" — or "" if t
+// is not one of this package's recognized values.
+func (t ApplicationType) String() string {
+	switch t {
+	case ApplicationTypeWeb:
+		return "web"
+	case ApplicationTypeNative:
+		return "native"
+	default:
+		return ""
+	}
+}
+
+// IsValid reports whether t is one of this package's recognized
+// ApplicationType values, including the zero value ApplicationTypeWeb.
+func (t ApplicationType) IsValid() bool {
+	return t == ApplicationTypeWeb || t == ApplicationTypeNative
+}
+
+// ParseApplicationType maps a wire value (see String) to its
+// ApplicationType. It rejects every other string, including "": OpenID
+// Connect's own default when the field is absent is "web", but that is
+// the caller's decision, as for ParseClientAuthMethod.
+func ParseApplicationType(s string) (ApplicationType, error) {
+	switch s {
+	case "web":
+		return ApplicationTypeWeb, nil
+	case "native":
+		return ApplicationTypeNative, nil
+	default:
+		return 0, fmt.Errorf("storage: unrecognized application type %q", s)
+	}
+}

storage/native_app_test.go

@@ -23,6 +23,7 @@ func TestNativeRedirectURIsAtRegistration(t *testing.T) {
 		"myapp:/callback":                             false, // RFC 8252 §8.4: no "."
 		"http://localhost/callback":                   false, // §8.3: not a name
 		"http://127.0.0.2/callback":                   false,
+		"http://[::ffff:127.0.0.1]/callback":          false, // IPv4-mapped: not the literal
 		"http://wallet.example/callback":              false,
 		"com.example.app://host/callback":             false,
 	} {
@@ -77,3 +78,20 @@ func TestNativeLoopbackMatchesAnyPort(t *testing.T) {
 		t.Error("ApplicationType() doesn't report the registration")
 	}
 }
+
+func TestApplicationTypeWireValues(t *testing.T) {
+	for _, typ := range []ApplicationType{ApplicationTypeWeb, ApplicationTypeNative} {
+		got, err := ParseApplicationType(typ.String())
+		if err != nil || got != typ || !typ.IsValid() {
+			t.Errorf("ParseApplicationType(%q) = %v, %v; want %v", typ.String(), got, err, typ)
+		}
+	}
+	for _, s := range []string{"", "Native", "mobile"} {
+		if _, err := ParseApplicationType(s); err == nil {
+			t.Errorf("ParseApplicationType(%q) = nil error", s)
+		}
+	}
+	if ApplicationType(9).IsValid() || ApplicationType(9).String() != "" {
+		t.Error("an unknown ApplicationType is valid, or has a wire value")
+	}
+}