From d1954534179609330c08c807f9b356ae604444e9 Mon Sep 17 00:00:00 2001 From: vadym Date: Thu, 3 Sep 2020 17:01:35 +0800 Subject: [PATCH] define boundary cases in documentation, refactor implementation of pattern url matching, more tests. Signed-off-by: vadym --- server/oauth2.go | 100 ++++++++++----------------- server/oauth2_test.go | 157 +++++++++++++++++++++++++++++++++--------- 2 files changed, 161 insertions(+), 96 deletions(-) diff --git a/server/oauth2.go b/server/oauth2.go index 21e72f33ed..ef06ccabed 100644 --- a/server/oauth2.go +++ b/server/oauth2.go @@ -601,7 +601,8 @@ func validateRedirectURI(client storage.Client, wildcardCache *lru.ARCCache, red if urlRequested, err := url.Parse(redirectURI); err == nil && urlRequested != nil { // validly-formed url clobberMatcher := getClobberHostMatcher(uri, wildcardCache) - if clobberMatcher.HostMatcher != nil && matchesClobber(clobberMatcher, urlRequested) { + validClobber := clobberMatcher.HostMatcher != nil && clobberMatcher.ClientRedirectURL != nil + if validClobber && matchesClobber(clobberMatcher, urlRequested) { // clobber match success return true } @@ -631,16 +632,16 @@ func validateRedirectURI(client storage.Client, wildcardCache *lru.ARCCache, red } func matchesClobber(clobberMatcher ClientRedirectClobberMatcher, urlRequested *url.URL) bool { - if !clobberMatcher.HostMatcher.MatchString(urlRequested.Host) { - // failed host match + if clobberMatcher.ClientRedirectURL.Path != urlRequested.Path { + // failed path match return false } - if clobberMatcher.ClientRedirectURLSections.Scheme != urlRequested.Scheme { + if clobberMatcher.ClientRedirectURL.Scheme != urlRequested.Scheme { // failed scheme match return false } - if clobberMatcher.ClientRedirectURLSections.RawPath != urlRequested.Path { - // failed path match + if !clobberMatcher.HostMatcher.MatchString(urlRequested.Host) { + // failed host match return false } return true @@ -651,108 +652,79 @@ type HostSplit struct { SubDomainPrefix string // e.g. dom.abc (from dom.abc.example.com) } -type ClobberURLSectional struct { - Scheme string // scheme: `http` or `https` - HostPair *HostSplit // leading from top-level to subsequent subdomains - RawPath string // path: requires leading slash; and includes fragments and query params -} - type ClientRedirectClobberMatcher struct { - ClientRedirectURL string - ClientRedirectURLSections *ClobberURLSectional - HostMatcher *regexp.Regexp + ClientRedirectURL *url.URL + HostMatcher *regexp.Regexp } -// Returns a clobber/wildcard subdomain matcher struct. The `ClientRedirectURLSections` -// and `ClientRedirectURLSections` pointers will be nil if clientRedirectURI does not meet -// the requirements to perform as a clobber/wildcard matcher. ClientRedirectURL is always set. +// Returns a clobber/wildcard subdomain matcher struct func createClientRedirectClobberMatcher(clientRedirectURI string) ClientRedirectClobberMatcher { - splitResult := splitClobberHttpRedirectUrl(clientRedirectURI) - if splitResult == nil || splitResult.HostPair == nil { - // does not meet basic wildcard usage requirements regarding scheme or path - return ClientRedirectClobberMatcher{ - ClientRedirectURL: clientRedirectURI, - } + redirectUrl, _ := url.Parse(clientRedirectURI) + if redirectUrl == nil { + return ClientRedirectClobberMatcher{} } - - clobberMatcher := ClientRedirectClobberMatcher{ - ClientRedirectURL: clientRedirectURI, - ClientRedirectURLSections: splitResult, + splitResult := splitClobberHttpRedirectUrl(redirectUrl) + if splitResult == nil { + // not wildcard or invalid + return ClientRedirectClobberMatcher{} } // first replace double-asterisk globber with .+ - var subdomain = strings.ReplaceAll(regexp.QuoteMeta(splitResult.HostPair.SubDomainPrefix), "\\*\\*", ".+") + var subdomain = strings.ReplaceAll(regexp.QuoteMeta(splitResult.SubDomainPrefix), "\\*\\*", ".+") subdomain = strings.ReplaceAll(subdomain, "\\*", "[^.]+") - hostNameRegExStr := "^" + subdomain + "\\." + splitResult.HostPair.HostSuffix + "$" + hostNameRegExStr := "^" + subdomain + "\\." + regexp.QuoteMeta(splitResult.HostSuffix) + "$" hostNameRegEx, err := regexp.Compile(hostNameRegExStr) if err != nil { // bad pattern - unexpected - return clobberMatcher + return ClientRedirectClobberMatcher{} } - clobberMatcher.HostMatcher = hostNameRegEx - return clobberMatcher + return ClientRedirectClobberMatcher{ + ClientRedirectURL: redirectUrl, + HostMatcher: hostNameRegEx, + } } -// This method is similar to url.Parse stdlib, expect that it does not validate a url, it is -// used to extract the scheme, host and path for wildcard checks. +// extract the scheme, host and path for wildcard checks. // If the scheme is not http or https, or does not specify an absolute path beginning with '/' // the parser will return nil. -func splitClobberHttpRedirectUrl(clientRedirectUrlSpec string) *ClobberURLSectional { - schemaRe, err := regexp.Compile("^([hH][tT]{2}[pP][sS]?)://([^/]+?)(:\\d+)?(/.*)$") - if err != nil { - return nil // bad regex pattern - } - splitRes := schemaRe.FindStringSubmatch(clientRedirectUrlSpec) - if splitRes == nil { - // not using http or https - return nil - } - - hostPortion := splitRes[2] - if !strings.Contains(hostPortion, "*") { +func splitClobberHttpRedirectUrl(clientRedirectUrlSpec *url.URL) *HostSplit { + host := clientRedirectUrlSpec.Host + if !strings.Contains(host, "*") { // for efficiency, check wildcard before DomainComponents return nil } - if strings.Contains(hostPortion, "***") { - // more than wildcards chars are not permitted + if strings.Contains(host, "***") { + // a maximum of 2 '*' characters are permitted in sequence return nil } - // port := splitRes[3] - // sub-domain portion is first portion (greedy) - hostSplit, err := regexp.Compile("^(.+)\\.([^.]*[A-Za-z][^.]*\\.[^.]*[A-Za-z][^.]*[^.])$") + hostSplit, _ := regexp.Compile("^(.+)\\.([^.]*[A-Za-z][^.]*\\.[^.]*[A-Za-z][^.]+)$") if hostSplit == nil { // unexpected issue with regex return nil } - hostRes := hostSplit.FindStringSubmatch(hostPortion) + hostRes := hostSplit.FindStringSubmatch(host) if hostRes == nil { // does not conform to requirements of a subdomain spec followed by two higher order domains return nil } - return &ClobberURLSectional { - Scheme: splitRes[1], - HostPair: &HostSplit{ - HostSuffix: hostRes[2], - SubDomainPrefix: hostRes[1], - }, - RawPath: splitRes[4], + return &HostSplit{ + HostSuffix: hostRes[2], + SubDomainPrefix: hostRes[1], } } func getClobberHostMatcher(clientRedirectUri string, wildcardCache *lru.ARCCache) ClientRedirectClobberMatcher { if wildcardCache == nil { // unexpected error condition where cache is unavailable - return ClientRedirectClobberMatcher { - ClientRedirectURL: clientRedirectUri, - } + return ClientRedirectClobberMatcher{} } if redirectMatcherStruct, ok := wildcardCache.Get(clientRedirectUri); ok { diff --git a/server/oauth2_test.go b/server/oauth2_test.go index ad6d080bff..9deb64cf01 100644 --- a/server/oauth2_test.go +++ b/server/oauth2_test.go @@ -333,67 +333,60 @@ func TestSplitHttpRedirectUrl(t *testing.T) { tests := []struct { redirectURI string expectValid bool - expectScheme string - expectPath string expectHostSplit *HostSplit }{ { - redirectURI: "http://*.example.com/%23267#222", - expectValid: true, - expectScheme: "http", + redirectURI: "http://*.example.com/%23267#222", + expectValid: true, expectHostSplit: &HostSplit{ - HostSuffix: "example.com", + HostSuffix: "example.com", SubDomainPrefix: "*", }, - expectPath: "/%23267#222", }, { // ip addresses cannot - redirectURI: "http://1.2.3.4/", - expectValid: false, + redirectURI: "http://1.2.3.4/", + expectValid: false, }, { // cannot act on top-level domain - redirectURI: "http://*.com/", - expectValid: false, + redirectURI: "http://*.com/", + expectValid: false, }, { // no wildcard - redirectURI: "http://example.com/", - expectValid: false, + redirectURI: "http://example.com/", + expectValid: false, }, { redirectURI: "http://1.2.3.4", expectValid: false, // no trailing slash }, - { - redirectURI: "ftp://*.example.com/", - expectValid: false, // wrong schema - }, { // ipv6 address not used for wildcard (dns only) - redirectURI: "https://fdda:5cc1:23:4::1f/foo", - expectValid: false, + redirectURI: "https://[fdda:5cc1:23:4::1f]/foo", + expectValid: false, }, } tAssert := assert.New(t) for _, test := range tests { - result := splitClobberHttpRedirectUrl(test.redirectURI) - if result == nil { - tAssert.False(test.expectValid, "Valid result expected") - } else { - tAssert.Equal(test.expectScheme,result.Scheme, "scheme should match") - tAssert.Equal(test.expectPath,result.RawPath, "path should match") - tAssert.Equal(test.expectHostSplit,result.HostPair, "DomainComponents should match") - + testRedirectUrl, err := url.Parse(test.redirectURI) + tAssert.Nilf(err, "Invalid test url %v", test.redirectURI) + tAssert.NotNilf(testRedirectUrl, "Invalid test url %v", test.redirectURI) + result := splitClobberHttpRedirectUrl(testRedirectUrl) + tAssert.Equal(test.expectValid, result != nil, "Valid result expected for %v", test.redirectURI) + if test.expectHostSplit != nil { + tAssert.Equal(result, test.expectHostSplit, "Host split validation failed for %v", test.redirectURI) + } + if result != nil { fullResult := createClientRedirectClobberMatcher(test.redirectURI) tAssert.True(fullResult.HostMatcher != nil) + tAssert.Equal(fullResult.ClientRedirectURL, testRedirectUrl) } } } - func TestValidRedirectURI(t *testing.T) { tests := []struct { client storage.Client @@ -407,6 +400,22 @@ func TestValidRedirectURI(t *testing.T) { redirectURI: "http://foo.com/bar", wantValid: true, }, + { + // invalid schema + client: storage.Client{ + RedirectURIs: []string{"http://foo.com/bar"}, + }, + redirectURI: "https://foo.com/bar", + wantValid: false, + }, + { + // invalid schema with pattern + client: storage.Client{ + RedirectURIs: []string{"http://*.foo.com/bar"}, + }, + redirectURI: "https://test.foo.com/bar", + wantValid: false, + }, { // check https is valid protocol client: storage.Client{ @@ -422,6 +431,14 @@ func TestValidRedirectURI(t *testing.T) { redirectURI: "http://foo.com/bar/baz", wantValid: false, }, + { + // invalid path with pattern + client: storage.Client{ + RedirectURIs: []string{"http://*.foo.com/bar"}, + }, + redirectURI: "http://test.foo.com/baz", + wantValid: false, + }, { client: storage.Client{ RedirectURIs: []string{"http://*.foo.com/bar"}, @@ -443,13 +460,27 @@ func TestValidRedirectURI(t *testing.T) { redirectURI: "http://abc.foo.com/bar", wantValid: false, }, + { + client: storage.Client{ + RedirectURIs: []string{"http://b*.foo.com/bar"}, + }, + redirectURI: "http://b.foo.com/bar", + wantValid: false, + }, + { + client: storage.Client{ + RedirectURIs: []string{"http://b*.foo.com/bar"}, + }, + redirectURI: "http://bar.foo.com/bar", + wantValid: true, + }, { // URL must use http: or https: protocol client: storage.Client{ RedirectURIs: []string{"unknown://*.foo.com/bar"}, }, redirectURI: "unknown://abc.foo.com/bar", - wantValid: false, + wantValid: true, }, { // wildcard can be located in subdomain as long as not in top two domains @@ -467,6 +498,13 @@ func TestValidRedirectURI(t *testing.T) { redirectURI: "http://abc.123.foo.com/bar", wantValid: true, }, + { + client: storage.Client{ + RedirectURIs: []string{"http://**.foo.com/bar"}, + }, + redirectURI: "http://test.foo.com/bar", + wantValid: true, + }, { client: storage.Client{ RedirectURIs: []string{"http://a**.foo.com/bar"}, @@ -474,6 +512,13 @@ func TestValidRedirectURI(t *testing.T) { redirectURI: "http://abc.123.foo.com/bar", wantValid: true, }, + { + client: storage.Client{ + RedirectURIs: []string{"http://a**.foo.com/bar"}, + }, + redirectURI: "http://a.foo.com/bar", + wantValid: false, + }, { // wildcard may be prefixed and/or suffixed with additional valid hostname characters // https://pre-*-post.foo.com will work @@ -489,15 +534,63 @@ func TestValidRedirectURI(t *testing.T) { client: storage.Client{ RedirectURIs: []string{"https://*.foo.com/"}, }, - redirectURI: "https://123.abc.foo.com/bar", + redirectURI: "https://123.abc.foo.com/", + wantValid: false, + }, + { + // check escaping + client: storage.Client{ + RedirectURIs: []string{"https://*.foo.com/"}, + }, + redirectURI: "https://abc.foo0com/", + wantValid: false, + }, + { + // with port + client: storage.Client{ + RedirectURIs: []string{"http://*.sld.com:6000/path"}, + }, + redirectURI: "http://test.sld.com:6000/path", + wantValid: true, + }, + { + // negative logic with port + client: storage.Client{ + RedirectURIs: []string{"http://*.sld.com:6000/path"}, + }, + redirectURI: "http://test.sld.com/path", wantValid: false, }, { // wildcard must be located in a subdomain within a hostname component. http://*.com is not permitted. client: storage.Client{ - RedirectURIs: []string{"http://*.com/bar"}, + RedirectURIs: []string{"http://*.*.com/bar"}, }, - redirectURI: "http://foo.com/bar", + redirectURI: "http://test.foo.com/bar", + wantValid: false, + }, + { + // partial wildcard in SLD is prohibited + client: storage.Client{ + RedirectURIs: []string{"http://*.foo*.com/bar"}, + }, + redirectURI: "http://test.foo.com/bar", + wantValid: false, + }, + { + // wildcard in TLD is prohibited + client: storage.Client{ + RedirectURIs: []string{"http://*.example.*/bar"}, + }, + redirectURI: "http://test.foo.com/bar", + wantValid: false, + }, + { + // partial wildcard in TLD is prohibited + client: storage.Client{ + RedirectURIs: []string{"http://*.example.io*/bar"}, + }, + redirectURI: "http://test.foo.com/bar", wantValid: false, }, {