From dc0f0cbfc97862d8e5b9dae8b1af2782df5a5d77 Mon Sep 17 00:00:00 2001 From: Arjun Naik Date: Tue, 8 Feb 2022 16:52:15 +0100 Subject: [PATCH] Ignore port when the redirect URI is a loopback address When the registered redirect URI is a loopback address(IPv4 or IPv6) the URI validation should ignore any port which is specified along with the loopback address by the client. This is prescribed in [rfc8252](https://www.rfc-editor.org/rfc/rfc8252.html#section-7.3) which cover OAuth Authorization Code Grant Flow for native applications. --- urivalidate.go | 32 +++++++--- urivalidate_test.go | 138 +++++++++++++++++++++++++++++++++----------- 2 files changed, 128 insertions(+), 42 deletions(-) diff --git a/urivalidate.go b/urivalidate.go index 0fce22d..48c3117 100644 --- a/urivalidate.go +++ b/urivalidate.go @@ -3,6 +3,7 @@ package osin import ( "errors" "fmt" + "net" "net/url" "strings" ) @@ -18,7 +19,7 @@ func newUriValidationError(msg string, base string, redirect string) UriValidati return UriValidationError(fmt.Sprintf("%s: %s / %s", msg, base, redirect)) } -// Parse urls, resolving uri references to base url +// ParseUrls parses URLs, resolving references to the base url func ParseUrls(baseUrl, redirectUrl string) (retBaseUrl, retRedirectUrl *url.URL, err error) { var base, redirect *url.URL // parse base url @@ -41,14 +42,31 @@ func ParseUrls(baseUrl, redirectUrl string) (retBaseUrl, retRedirectUrl *url.URL return nil, nil, newUriValidationError("scheme mismatch", baseUrl, redirectUrl) } - // Host must match - if redirect.Host != base.Host { + var ( + redirectMatch bool + host string + ) + + // verify the hosts match + if redirect.Host == base.Host { + redirectMatch = true + host = base.Host + } else if baseIP := net.ParseIP(base.Host); baseIP != nil && baseIP.IsLoopback() && base.Scheme == "http" { + // if the registered redirect URI is lookback, then match the input redirect without the port. + if redirectIP := net.ParseIP(redirect.Hostname()); redirectIP != nil && redirectIP.IsLoopback() { + redirectMatch = true + host = redirect.Host + } + } + + // Hosts must match + if !redirectMatch { return nil, nil, newUriValidationError("host mismatch", baseUrl, redirectUrl) } // resolve references to base url - retBaseUrl = (&url.URL{Scheme: base.Scheme, Host: base.Host, Path: "/"}).ResolveReference(&url.URL{Path: base.Path}) - retRedirectUrl = (&url.URL{Scheme: base.Scheme, Host: base.Host, Path: "/"}).ResolveReference(&url.URL{Path: redirect.Path, RawQuery: redirect.RawQuery}) + retBaseUrl = (&url.URL{Scheme: base.Scheme, Host: host, Path: "/"}).ResolveReference(&url.URL{Path: base.Path}) + retRedirectUrl = (&url.URL{Scheme: base.Scheme, Host: host, Path: "/"}).ResolveReference(&url.URL{Path: redirect.Path, RawQuery: redirect.RawQuery}) return } @@ -103,10 +121,10 @@ func ValidateUri(baseUri string, redirectUri string) (realRedirectUri string, er return "", newUriValidationError("path prefix doesn't match", baseUri, redirectUri) } - return redirect.String(),nil + return redirect.String(), nil } -// Returns the first uri from an uri list +// FirstUri returns the first uri from a URI list func FirstUri(baseUriList string, separator string) string { if separator != "" { slist := strings.Split(baseUriList, separator) diff --git a/urivalidate_test.go b/urivalidate_test.go index 30ed17c..c5e2748 100644 --- a/urivalidate_test.go +++ b/urivalidate_test.go @@ -1,131 +1,199 @@ package osin import ( + "fmt" "testing" ) func TestURIValidate(t *testing.T) { - valid := [][]string{ - { - // Exact match + valid := []struct { + name string + clientRedirectURI string + inputRedirectURI string + normalized string + }{ + { + "Exact match", "http://localhost:14000/appauth", "http://localhost:14000/appauth", "http://localhost:14000/appauth", }, { - // Trailing slash + "Trailing slash", "http://www.google.com/myapp", "http://www.google.com/myapp/", "http://www.google.com/myapp/", }, { - // Exact match with trailing slash + "Exact match with trailing slash", "http://www.google.com/myapp/", "http://www.google.com/myapp/", "http://www.google.com/myapp/", }, { - // Subpath + "Subpath", "http://www.google.com/myapp", "http://www.google.com/myapp/interface/implementation", "http://www.google.com/myapp/interface/implementation", }, { - // Subpath with trailing slash + "Subpath with trailing slash", "http://www.google.com/myapp/", "http://www.google.com/myapp/interface/implementation", "http://www.google.com/myapp/interface/implementation", }, { - // Subpath with things that are close to path traversals, but aren't + "Subpath with things that are close to path traversals, but aren't", "http://www.google.com/myapp", "http://www.google.com/myapp/.../..implementation../...", "http://www.google.com/myapp/.../..implementation../...", }, { - // If the allowed basepath contains path traversals, allow them? + "If the allowed basepath contains path traversals, allow them?", "http://www.google.com/traversal/../allowed", "http://www.google.com/traversal/../allowed/with/subpath", "http://www.google.com/allowed/with/subpath", }, { - // Backslashes + "Backslashes", "https://mysafewebsite.com/secure/redirect", "https://mysafewebsite.com/secure/redirect/\\../\\../\\../evil", "https://mysafewebsite.com/secure/redirect/%5C../%5C../%5C../evil", }, { - // Backslashes - "https://mysafewebsite.com/secure/redirect", - "https://mysafewebsite.com/secure/redirect/\\..\\../\\../evil", - "https://mysafewebsite.com/secure/redirect/%5C..%5C../%5C../evil", - }, - { - // Query string must be kept + "Query string must be kept", "http://www.google.com/myapp/redir", "http://www.google.com/myapp/redir?a=1&b=2", "http://www.google.com/myapp/redir?a=1&b=2", }, + { + "IPv4 loopback address", + "http://127.0.0.1/callback", + "http://127.0.0.1:8081/callback", + "http://127.0.0.1:8081/callback", + }, + { + "Uncommon IPv4 loopback address", + "http://127.0.0.1/callback", + "http://127.0.0.8:8081/callback", + "http://127.0.0.8:8081/callback", + }, + { + "IPv6 loopback address", + "http://127.0.0.1/callback", + "http://[::1]:8081/callback", + "http://[::1]:8081/callback", + }, + { + "No port in IPv4 loopback address", + "http://127.0.0.1/callback", + "http://127.0.0.1/callback", + "http://127.0.0.1/callback", + }, + { + "No port in IPv6 loopback address", + "http://127.0.0.1/callback", + "http://[0:0:0:0:0:0:0:1]/callback", + "http://[0:0:0:0:0:0:0:1]/callback", + }, } for _, v := range valid { - if realRedirectUri, err := ValidateUri(v[0], v[1]); err != nil { - t.Errorf("Expected ValidateUri(%s, %s) to succeed, got %v", v[0], v[1], err) - } else if len(v) == 3 && realRedirectUri != v[2] { - t.Errorf("Expected ValidateUri(%s, %s) to return uri %s, got %s", v[0], v[1], v[2], realRedirectUri) - } + t.Run(fmt.Sprintf("valid/%s", v.name), func(t *testing.T) { + if realRedirectUri, err := ValidateUri(v.clientRedirectURI, v.inputRedirectURI); err != nil { + t.Errorf("Expected ValidateUri(%s, %s) to succeed, got %v", v.clientRedirectURI, v.inputRedirectURI, err) + } else if realRedirectUri != v.normalized { + t.Errorf("Expected ValidateUri(%s, %s) to return uri %s, got %s", v.clientRedirectURI, v.inputRedirectURI, v.normalized, realRedirectUri) + } + }) } - invalid := [][]string{ + invalid := []struct { + name string + clientRedirectURI string + inputRedirectURI string + }{ { - // Doesn't satisfy base path + "Doesn't satisfy base path", "http://localhost:14000/appauth", "http://localhost:14000/app", }, { - // Doesn't satisfy base path + "Doesn't satisfy base path", "http://localhost:14000/app/", "http://localhost:14000/app", }, { - // Not a subpath of base path + "Not a subpath of base path", "http://localhost:14000/appauth", "http://localhost:14000/appauthmodifiedpath", }, { - // Host mismatch + "Host mismatch", "http://www.google.com/myapp", "http://www2.google.com/myapp", }, { - // Scheme mismatch + "Scheme mismatch", "http://www.google.com/myapp", "https://www.google.com/myapp", }, { - // Path traversal + "Path traversal", "http://www.google.com/myapp", "http://www.google.com/myapp/..", }, { - // Embedded path traversal + "Embedded path traversal", "http://www.google.com/myapp", "http://www.google.com/myapp/../test", }, { - // Not a subpath + "Not a subpath", "http://www.google.com/myapp", "http://www.google.com/myapp../test", }, { - // Backslashes + "Backslashes", "https://mysafewebsite.com/secure/redirect", "https://mysafewebsite.com/secure%2fredirect/../evil", }, + { + "Mismatching ports", + "https://example.com:8081/redirect", + "https://example.com:8080/redirect", + }, + { + "Non loopback address", + "http://127.0.0.1/callback", + "http://localhost:8080/callback", + }, + { + "Invalid input redirect URI", + "http://127.0.0.1/callback", + "http://127.0.0.1:abc/callback", + }, + { + "Non http scheme in redirect URI", + "custom://127.0.0.1/callback", + "custom://127.0.0.1:8080/callback", + }, + { + "Redirect URI is loopback, input is a domain name with port", + "http://127.0.0.1/callback", + "http://example.com:8081/callback", + }, + { + "Redirect URI is loopback, input is a domain name without port", + "http://127.0.0.1/callback", + "http://example.com/callback", + }, } for _, v := range invalid { - if _, err := ValidateUri(v[0], v[1]); err == nil { - t.Errorf("Expected ValidateUri(%s, %s) to fail", v[0], v[1]) - } + t.Run(fmt.Sprintf("invalid/%s", v.name), func(t *testing.T) { + if _, err := ValidateUri(v.clientRedirectURI, v.inputRedirectURI); err == nil { + t.Errorf("Expected ValidateUri(%s, %s) to fail", v.clientRedirectURI, v.inputRedirectURI) + } + }) } }