fix!: hold a redirect URI's query, path and port to its sent form too

GETTING_STARTED.md

@@ -433,7 +433,7 @@ private-use scheme in reverse-domain form
 private-use form with a single slash, as RFC 8252 §7.1 writes it:
 `com.example.wallet:/callback`. The form with an authority,
 `com.example.wallet://callback`, which some deployed wallets register,
-is accepted too, with an authority of letters, digits, `-`, `.`, `_`
+is accepted too, with an authority of lowercase letters, digits, `-`, `.`, `_`
 and `~` (no port, userinfo or IP address), and is matched exactly like
 any other. In
 production a web client can use neither; under development assurance

UPGRADING.md

@@ -15,6 +15,48 @@ Most production-assurance changes only affect `Config.Assurance =
 AssuranceProduction`. A development-assurance setup built on `memstore`
 and `keys/ephemeral` needs only the steps not marked *production only*.
 
+## v0.75.0
+
+### Redirect URIs: query text, dot segments, ports and private-use authorities (fapi, storage, client)
+
+**Affects:** a client registered with, or a `client.Config.RedirectURI`
+of, a redirect URI that:
+
+- has a query with a character RFC 3986 doesn't allow, or a malformed
+  `%`-encoding (`?a=<x>`, `?a b`, `?a=%zz`);
+- has a `.` or `..` path segment, written or percent-encoded
+  (`/a/../cb`, `/%2e/cb`);
+- has an empty or impossible port (`https://rp.example:/cb`, `:0`,
+  `:99999`);
+- is a private-use redirect with an uppercase authority
+  (`com.example.app://Wallet/cb`).
+
+Every other redirect URI is unaffected.
+
+**Why:** v0.74.0 required a redirect URI to be written as it's sent, but
+Go's `net/url` gives a query back byte for byte, so a query was never
+checked. A browser resolves dot segments away before requesting the
+path. An uppercase private-use authority passed `fapi.ParseRedirectURL`,
+and so `client.New`, but `storage.NewRegisteredClient` refused it, so no
+server could register what the client was configured with.
+
+**What fails:**
+
+- `storage.NewRegisteredClient` refuses the client.
+- A client stored before the upgrade, which sqlstore and similar
+  repositories rebuild with `NewRegisteredClient`, fails to resolve, as
+  in v0.74.0.
+- `fapi.ParseRedirectURL` refuses the URI, so `client.New` refuses such a
+  `Config.RedirectURI`.
+
+The new `fapi.CheckRedirectURIForm` is the form check registration
+applies.
+
+**What to change:** register the URI percent-encoded (`?a=%3Cx%3E`), with
+its path resolved (`/cb`), with no `:` or with a real port, and with a
+lowercase authority (`com.example.app://wallet/cb`). Update the client's
+configuration to match.
+
 ## v0.74.0
 
 ### Redirect URIs must be written as they're sent (fapi, storage, client)

redirect_url_test.go

@@ -19,7 +19,7 @@ func TestParseRedirectURL(t *testing.T) {
 		"com.example.app:cb":                          false, // opaque, no path
 		"com.example.app://host/cb":                   true,  // an authority
 		"eu.europa.ec.euidi://authorization":          true,
-		"com.example.app://Host_1.a~b/cb?x=1":         true,
+		"com.example.app://host_1.a~b/cb?x=1":         true,
 		"com.example.app:///cb":                       false, // an empty authority
 		"com.example.app://":                          false,
 		"com.example.app://host:443/cb":               false, // a port
@@ -40,6 +40,21 @@ func TestParseRedirectURL(t *testing.T) {
 		"http://127.0.0.1:51004/c b":                  false,
 		"https://rp.example/a%7eb?x=a%20b&y=1;z":      true,
 		"https://rp.example:8443/":                    true,
+		"com.example.app://Wallet/cb":                 false, // an uppercase authority, sent lowercased
+		"https://rp.example/cb?a=<x>":                 false, // a query RFC 3986 doesn't allow
+		"https://rp.example/cb?a b":                   false,
+		"https://rp.example/cb?a=%zz":                 false,
+		"https://rp.example/cb?a=%2":                  false,
+		"https://rp.example/cb?a=%2F&b=x:y@z/?":       true,
+		"https://rp.example/a/../cb":                  false, // a dot segment, resolved away
+		"https://rp.example/./cb":                     false,
+		"https://rp.example/a/%2e%2E/cb":              false,
+		"com.example.app:/a/../cb":                    false,
+		"https://rp.example/a.b/..c/cb":               true,
+		"https://rp.example:/cb":                      false, // an empty port
+		"https://rp.example:0/cb":                     false,
+		"https://rp.example:99999/cb":                 false,
+		"http://[::1]:/cb":                            false,
 		"https://rp.example":                          true,
 		"https://rp.example/cb?":                      true,
 		"com.example.app:////evil.example/cb":         false,
@@ -65,7 +80,7 @@ func TestParseRedirectURL(t *testing.T) {
 	if _, err := fapi.ParseEndpointURL("com.example.app:/cb", fapi.AllowPrivateUseScheme()); err == nil {
 		t.Error("ParseEndpointURL accepted a private-use scheme: the option is for redirects only")
 	}
-	for _, raw := range []string{"org.idfoundry.oid4vcgo.demowallet:/callback", "eu.europa.ec.euidi://authorization", "com.example.app://Host_1.a~b/cb?x=1"} {
+	for _, raw := range []string{"org.idfoundry.oid4vcgo.demowallet:/callback", "eu.europa.ec.euidi://authorization", "com.example.app://host_1.a~b/cb?x=1"} {
 		u, err := fapi.ParseRedirectURL(raw, native...)
 		if err != nil || u.String() != raw {
 			t.Errorf("ParseRedirectURL(%q) round trip = %q, %v", raw, u.String(), err)
@@ -74,15 +89,22 @@ func TestParseRedirectURL(t *testing.T) {
 }
 
 // FuzzParseRedirectURL covers ParseRedirectURL on any input: it never
-// panics, with no options accepts https alone, and gives back any URI it
-// accepts exactly as written.
+// panics, with no options accepts https alone, gives back any URI it
+// accepts exactly as written, and accepts none that registration
+// (CheckRedirectURIForm) would refuse.
 func FuzzParseRedirectURL(f *testing.F) {
-	for _, s := range []string{"https://rp.example/cb", "com.example.app:/cb", "http://[::1]:1/cb", "a.b:/", "a.b://x", "a.b://x/cb?y=1", "a.b:///x", "a.b://127.0.0.1/", "A.0:/ ", "https://RP.example/c b"} {
+	for _, s := range []string{"https://rp.example/cb", "com.example.app:/cb", "http://[::1]:1/cb", "a.b:/", "a.b://x", "a.b://x/cb?y=1", "a.b:///x", "a.b://127.0.0.1/", "A.0:/ ", "https://RP.example/c b", "https://rp.example/cb?a=%zz", "a.b://W/cb", "https://rp.example/a/../b", "https://rp.example:/cb"} {
 		f.Add(s)
 	}
 	f.Fuzz(func(t *testing.T, raw string) {
-		if u, err := fapi.ParseRedirectURL(raw, fapi.AllowLoopbackHTTP(), fapi.AllowPrivateUseScheme()); err == nil && u.String() != raw {
-			t.Fatalf("ParseRedirectURL(%q) = %q, not as written", raw, u.String())
+		if u, err := fapi.ParseRedirectURL(raw, fapi.AllowLoopbackHTTP(), fapi.AllowPrivateUseScheme()); err == nil {
+			if u.String() != raw {
+				t.Fatalf("ParseRedirectURL(%q) = %q, not as written", raw, u.String())
+			}
+			// What the server accepts at a request, registration accepts.
+			if err := fapi.CheckRedirectURIForm(raw); err != nil {
+				t.Fatalf("ParseRedirectURL(%q) accepted it, CheckRedirectURIForm refused it: %v", raw, err)
+			}
 		}
 		if u, err := fapi.ParseRedirectURL(raw); err == nil {
 			if v := u.URL(); v.Scheme != "https" {
@@ -109,3 +131,25 @@ func TestParseRedirectURLSaysWhatToChange(t *testing.T) {
 		t.Errorf("ParseRedirectURL(rewritten URI) = %v, want it to show the form it would be sent as", err)
 	}
 }
+
+// TestCheckRedirectURIForm covers registration's check: the form rules
+// ParseRedirectURL applies, whatever the scheme, and a URL that doesn't
+// parse left to ParseRedirectURL.
+func TestCheckRedirectURIForm(t *testing.T) {
+	for raw, want := range map[string]bool{
+		"https://rp.example/cb?a=1":   true,
+		"http://rp.example/cb":        true, // a scheme a client may not use is the server's to refuse
+		"myapp:/cb":                   true,
+		"https://rp.example/%zz":      true, // doesn't parse: left to ParseRedirectURL
+		"https://RP.example/cb":       false,
+		"com.example.app://Wallet/cb": false,
+		"https://rp.example/cb?a=<x>": false,
+		"https://rp.example/a/../cb":  false,
+		"https://rp.example:0/cb":     false,
+		"https://rp.example:/cb":      false,
+	} {
+		if err := fapi.CheckRedirectURIForm(raw); (err == nil) != want {
+			t.Errorf("CheckRedirectURIForm(%q) = %v, want accepted %v", raw, err, want)
+		}
+	}
+}

server/redirect_query_retention_test.go

@@ -14,14 +14,16 @@ import (
 // when adding additional query parameters". The response's parameters
 // follow the registered query, which is kept byte for byte — not
 // reordered, and with a value-less name, a ";" and an escape
-// url.ParseQuery can't decode all left as registered.
+// url.Values.Encode would rewrite ("%2b", not "%2B") all left as
+// registered. (A malformed escape can't be registered:
+// fapi.CheckRedirectURIForm.)
 func TestAuthorizationResponseRetainsRedirectQuery(t *testing.T) {
 	for name, profile := range map[string]server.Profile{"plain": server.ProfileFAPISecurity, "message signing": server.ProfileFAPISecurityWithMessageSigning} {
 		for _, registered := range []string{
 			"https://rp.example/callback?b=2&a=1",
 			"https://rp.example/callback?flag",
 			"https://rp.example/callback?x=1;y=2",
-			"https://rp.example/callback?bad=%zz&keep=1",
+			"https://rp.example/callback?plus=%2b&keep=1",
 			"https://rp.example/callback?",
 		} {
 			t.Run(name+" "+registered, func(t *testing.T) {

storage/client_repository.go

@@ -12,7 +12,6 @@ import (
 	"unicode/utf8"
 
 	fapi "github.com/idfoundry/fapigo"
-	"github.com/idfoundry/fapigo/internal/canonical"
 	"github.com/idfoundry/fapigo/internal/querystring"
 	"github.com/idfoundry/fapigo/internal/resourceindicator"
 )
@@ -258,7 +257,9 @@ type RegisteredClientConfig struct {
 	// "#" (RFC 6749 §3.1.2). A URI must be written exactly as a redirect
 	// to it is sent, its normalized form: scheme and host in lowercase,
 	// and any character RFC 3986 doesn't allow in it percent-encoded
-	// ("/c%20b", not "/c b").
+	// ("/c%20b", not "/c b"), in the query too; and it must have no "."
+	// or ".." path segment and no empty or impossible port
+	// (fapi.CheckRedirectURIForm).
 	RedirectURIs []fapi.RegisteredRedirectURI
 
 	// ApplicationType is ApplicationTypeWeb (the zero value) or
@@ -986,24 +987,22 @@ func checkRedirectURIQueries(cfg RegisteredClientConfig) error {
 	return nil
 }
 
-// checkRedirectURIsWrittenAsSent refuses a redirect URI that isn't in
-// the normalized form a redirect to it would be sent to: net/url's
-// string form, with the host's ASCII letters lowercased, as
-// fapi.ParseRedirectURL requires. A registered URI is matched exactly,
+// checkRedirectURIsWrittenAsSent refuses a redirect URI whose form
+// fapi.CheckRedirectURIForm refuses, as fapi.ParseRedirectURL does: one
+// not in the normalized form a redirect to it would be sent to (net/url's
+// string form, with the host's ASCII letters lowercased), with a query
+// RFC 3986 doesn't allow, a "." or ".." path segment or an impossible
+// port. Whether a client may use the URI's scheme and host is left to
+// the server, per request. A registered URI is matched exactly,
 // so "https://rp.example/c b", sent as "https://rp.example/c%20b", or
 // "https://RP.example/cb", sent as "https://rp.example/cb", would
 // redirect somewhere other than the string registered. Checked here for
 // every client, so a web client's is refused at registration, not first
 // at a pushed authorization request.
 func checkRedirectURIsWrittenAsSent(cfg RegisteredClientConfig) error {
 	for _, uri := range cfg.RedirectURIs {
-		u, err := url.Parse(string(uri))
-		if err != nil {
-			continue // refused for what it is where it's parsed as a redirect URI
-		}
-		u.Host = canonical.LowerASCII(u.Host)
-		if sent := u.String(); sent != string(uri) {
-			return fmt.Errorf("storage: client %q: redirect URI %q must be written exactly as it is sent, %q: percent-encode any character RFC 3986 doesn't allow, and write the scheme and host in lowercase", cfg.ID, uri, sent)
+		if err := fapi.CheckRedirectURIForm(string(uri)); err != nil {
+			return fmt.Errorf("storage: client %q: redirect URI %q: %w", cfg.ID, uri, err)
 		}
 	}
 	return nil

storage/redirect_uris_test.go

@@ -148,6 +148,10 @@ func TestNewRegisteredClientRefusesRedirectURINotWrittenAsSent(t *testing.T) {
 		{"Com.Example.App:/cb", true, false},
 		{"https://rp.example/c%20b?x=a%20b", false, true},
 		{"https://rp.example:8443/cb", false, true},
+		{"https://rp.example/cb?a=<x>", false, false},
+		{"https://rp.example/a/../cb", false, false},
+		{"https://rp.example:0/cb", false, false},
+		{"com.example.app://Wallet/cb", true, false},
 		{"com.example.app:/cb", true, true},
 	} {
 		t.Run(tc.uri, func(t *testing.T) {
@@ -162,8 +166,8 @@ func TestNewRegisteredClientRefusesRedirectURINotWrittenAsSent(t *testing.T) {
 			if (err == nil) != tc.want {
 				t.Fatalf("NewRegisteredClient(%q) = %v, want accepted %v", tc.uri, err, tc.want)
 			}
-			if !tc.want && !strings.Contains(err.Error(), "written exactly as it is sent") {
-				t.Errorf("NewRegisteredClient(%q) = %v, want it to say how to write the URI", tc.uri, err)
+			if !tc.want && !strings.Contains(err.Error(), "redirect URI") {
+				t.Errorf("NewRegisteredClient(%q) = %v, want it to name the redirect URI", tc.uri, err)
 			}
 		})
 	}

url.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net"
 	"net/url"
+	"strconv"
 	"strings"
 
 	"github.com/idfoundry/fapigo/internal/canonical"
@@ -51,7 +52,8 @@ func AllowLoopbackHTTP() URLOption {
 // "com.example.app://oauth2redirect", which some deployed wallets
 // register. The authority, which the operating system ignores when it
 // dispatches the redirect to the app, must be a name of ASCII letters,
-// digits, "-", ".", "_" and "~": no userinfo, port or IP address. The
+// digits, "-", ".", "_" and "~", the letters lowercase, as a host is
+// written: no userinfo, port or IP address. The
 // scheme alone identifies the app, so show it, not the authority, where
 // a user is told where they'll be sent. Either form is matched against
 // the registered string exactly. Enable it only for a client registered
@@ -91,9 +93,88 @@ func ParseRedirectURL(raw string, opts ...URLOption) (URL, error) {
 	if sent := u.String(); sent != raw {
 		return URL{}, fmt.Errorf("fapi: parse redirect URL: redirect URI must be written exactly as it is sent, %q: percent-encode any character RFC 3986 doesn't allow, and write the scheme and host in lowercase", sent)
 	}
+	if err := checkRedirectComponents(u.value); err != nil {
+		return URL{}, fmt.Errorf("fapi: parse redirect URL: %w", err)
+	}
 	return u, nil
 }
 
+// CheckRedirectURIForm checks raw's form as a redirect URI, whatever its
+// scheme, without deciding whether any client may use it: that it's
+// written exactly as a redirect to it is sent (its normalized string
+// form, scheme and host in lowercase), with RFC 3986 query text, no "."
+// or ".." path segment and no empty or impossible port. It's what
+// ParseRedirectURL also requires of every URI it accepts, and what
+// storage.NewRegisteredClient holds each registered redirect URI to. A
+// raw that doesn't parse as a URL at all passes, left for
+// ParseRedirectURL to refuse where it's used.
+func CheckRedirectURIForm(raw string) error {
+	parsed, err := url.Parse(raw)
+	if err != nil {
+		return nil
+	}
+	parsed.Host = canonical.LowerASCII(parsed.Host)
+	if sent := parsed.String(); sent != raw {
+		return fmt.Errorf("fapi: redirect URI must be written exactly as it is sent, %q: percent-encode any character RFC 3986 doesn't allow, and write the scheme and host in lowercase", sent)
+	}
+	if err := checkRedirectComponents(*parsed); err != nil {
+		return fmt.Errorf("fapi: %w", err)
+	}
+	return nil
+}
+
+// checkRedirectComponents refuses what the string-form comparison in
+// ParseRedirectURL lets through but a browser would still send
+// differently, or not at all: a query with a character RFC 3986 §3.4
+// doesn't allow, or a malformed percent-encoding (net/url gives a query
+// back byte for byte, unchecked); a "." or ".." path segment, which a
+// browser resolves away (RFC 3986 §5.2.4) before requesting the path;
+// and a port that is empty or outside 1 to 65535.
+func checkRedirectComponents(u url.URL) error {
+	if !isQueryText(u.RawQuery) {
+		return fmt.Errorf("redirect URI's query must be RFC 3986 query text: percent-encode any other character, and every %% as %%25")
+	}
+	for _, segment := range strings.Split(u.EscapedPath(), "/") {
+		if s := strings.ToLower(segment); s == "." || s == ".." || s == "%2e" || s == "%2e%2e" || s == ".%2e" || s == "%2e." {
+			return fmt.Errorf("redirect URI's path must not have a \".\" or \"..\" segment, which a browser resolves away")
+		}
+	}
+	if u.Host != "" && strings.HasSuffix(u.Host, ":") {
+		return fmt.Errorf("redirect URI's port must not be empty: leave out the \":\" for the default port")
+	}
+	if port := u.Port(); port != "" {
+		if n, err := strconv.Atoi(port); err != nil || n < 1 || n > 65535 {
+			return fmt.Errorf("redirect URI's port must be 1 to 65535")
+		}
+	}
+	return nil
+}
+
+// isQueryText reports whether q is RFC 3986 §3.4 query text: unreserved
+// characters, sub-delims, ":", "@", "/", "?", and %-encodings of two
+// hex digits.
+func isQueryText(q string) bool {
+	for i := 0; i < len(q); i++ {
+		c := q[i]
+		switch {
+		case c == '%':
+			if i+2 >= len(q) || !isHex(q[i+1]) || !isHex(q[i+2]) {
+				return false
+			}
+			i += 2
+		case 'a' <= c && c <= 'z', 'A' <= c && c <= 'Z', '0' <= c && c <= '9':
+		case strings.IndexByte("-._~!$&'()*+,;=:@/?", c) >= 0:
+		default:
+			return false
+		}
+	}
+	return true
+}
+
+func isHex(c byte) bool {
+	return '0' <= c && c <= '9' || 'a' <= c && c <= 'f' || 'A' <= c && c <= 'F'
+}
+
 // parsePrivateUseURL validates parsed, raw's parse, as a private-use
 // URI scheme redirect (RFC 8252 §7.1, §8.4).
 func parsePrivateUseURL(raw string, parsed *url.URL) (URL, error) {
@@ -107,7 +188,7 @@ func parsePrivateUseURL(raw string, parsed *url.URL) (URL, error) {
 		// adds nothing another app claiming the scheme (§8.6) could use,
 		// and the registered string is still matched exactly.
 		if parsed.User != nil || !isPrivateUseAuthority(parsed.Host) {
-			return URL{}, fmt.Errorf("private-use redirect URI's authority must be a name of letters, digits, \"-\", \".\", \"_\" and \"~\", with no userinfo, port or IP address (%s:/path is RFC 8252 §7.1's preferred form)", parsed.Scheme)
+			return URL{}, fmt.Errorf("private-use redirect URI's authority must be a name of lowercase letters, digits, \"-\", \".\", \"_\" and \"~\", with no userinfo, port or IP address (%s:/path is RFC 8252 §7.1's preferred form)", parsed.Scheme)
 		}
 	} else if parsed.Opaque != "" || parsed.User != nil || !strings.HasPrefix(parsed.Path, "/") || strings.HasPrefix(parsed.String(), parsed.Scheme+"://") {
 		// com.example.app:/callback; not com.example.app:callback, with no
@@ -153,7 +234,8 @@ func isReverseDomainScheme(scheme string) bool {
 }
 
 // isPrivateUseAuthority reports whether host, a private-use redirect
-// URI's authority, is a non-empty name of ASCII letters, digits, "-",
+// URI's authority, is a non-empty name of lowercase ASCII letters,
+// digits, "-",
 // ".", "_" and "~" (RFC 3986 §2.3's unreserved characters) that isn't an
 // IP address: one net/url gives back as written, with no port, and that
 // can't be mistaken for a loopback or other network destination.
@@ -162,7 +244,7 @@ func isPrivateUseAuthority(host string) bool {
 		return false
 	}
 	for _, r := range host {
-		if !isLabelRune(r) && r != '.' && r != '_' && r != '~' {
+		if 'A' <= r && r <= 'Z' || !isLabelRune(r) && r != '.' && r != '_' && r != '~' {
 			return false
 		}
 	}