diff --git a/GETTING_STARTED.md b/GETTING_STARTED.md index 66815db2..3ebf6795 100644 --- a/GETTING_STARTED.md +++ b/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 diff --git a/UPGRADING.md b/UPGRADING.md index c17afe49..000071df 100644 --- a/UPGRADING.md +++ b/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=`, `?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) diff --git a/redirect_url_test.go b/redirect_url_test.go index 7f0faa41..d49b813d 100644 --- a/redirect_url_test.go +++ b/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=": 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=": 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) + } + } +} diff --git a/server/redirect_query_retention_test.go b/server/redirect_query_retention_test.go index 9365bfe9..ebf22c3e 100644 --- a/server/redirect_query_retention_test.go +++ b/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) { diff --git a/storage/client_repository.go b/storage/client_repository.go index c1f705d0..d16c3261 100644 --- a/storage/client_repository.go +++ b/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,10 +987,13 @@ 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 @@ -997,13 +1001,8 @@ func checkRedirectURIQueries(cfg RegisteredClientConfig) error { // 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 diff --git a/storage/redirect_uris_test.go b/storage/redirect_uris_test.go index 46d2247c..2774c8e0 100644 --- a/storage/redirect_uris_test.go +++ b/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=", 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) } }) } diff --git a/url.go b/url.go index 62fc018a..aaf4ce76 100644 --- a/url.go +++ b/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 } }