From f55082d4dbf833c03626b7ebe63bb9cda8daff3a Mon Sep 17 00:00:00 2001 From: James Elliott Date: Mon, 18 Jul 2022 14:59:13 +1000 Subject: [PATCH] fix(authorization): final slash in url matches ignored (#3717) This fixes an issue with the URL matching machinery which ignores the final slash of a URL. Introduced in 664d65d7fb4cd77f66629dc42c0a2c4a3ccc36a4. Fixes #3692 --- internal/authorization/authorizer_test.go | 9 ++++++++- internal/utils/url.go | 22 +++++++++++++++++----- internal/utils/url_test.go | 16 ++++++++++------ 3 files changed, 35 insertions(+), 12 deletions(-) diff --git a/internal/authorization/authorizer_test.go b/internal/authorization/authorizer_test.go index 4bf7b3c03..99c7b64dc 100644 --- a/internal/authorization/authorizer_test.go +++ b/internal/authorization/authorizer_test.go @@ -669,16 +669,22 @@ func (s *AuthorizerSuite) TestShouldCheckResourceMatching() { Policy: twoFactor, Resources: createSliceRegexRule(s.T(), []string{"^/a/longer/rule/.*$"}), }). + WithRule(schema.ACLRule{ + Domains: []string{"resource.example.com"}, + Policy: twoFactor, + Resources: createSliceRegexRule(s.T(), []string{"^/an/exact/path/$"}), + }). Build() tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/", "GET", Bypass) tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/abc", "GET", Bypass) - tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/", "GET", Denied) + tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/", "GET", Bypass) tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/one_factor/abc", "GET", OneFactor) tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/xyz/embedded/abc", "GET", Bypass) tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/a/longer/rule/abc", "GET", TwoFactor) tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/case/abc", "GET", Bypass) tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/case/ABC", "GET", Denied) + tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/an/exact/path/", "GET", TwoFactor) tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/../a/longer/rule/abc", "GET", TwoFactor) tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/..//a/longer/rule/abc", "GET", TwoFactor) tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/..%2f/a/longer/rule/abc", "GET", TwoFactor) @@ -697,6 +703,7 @@ func (s *AuthorizerSuite) TestShouldCheckResourceMatching() { tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/%2E%2E%2fa/longer/rule/abc", "GET", TwoFactor) tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/%2E%2E%2F/a/longer/rule/abc", "GET", TwoFactor) tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/%2E%2E%2Fa/longer/rule/abc", "GET", TwoFactor) + tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/%2E%2E%2Fan/exact/path/", "GET", TwoFactor) } // This test assures that rules without domains (not allowed by schema validator at this time) will pass validation correctly. diff --git a/internal/utils/url.go b/internal/utils/url.go index 9d912a1ae..c2a227a89 100644 --- a/internal/utils/url.go +++ b/internal/utils/url.go @@ -7,11 +7,23 @@ import ( // URLPathFullClean returns a URL path with the query parameters appended (full path) with the path portion parsed // through path.Clean given a *url.URL. -func URLPathFullClean(u *url.URL) (p string) { - switch len(u.RawQuery) { - case 0: - return path.Clean(u.Path) - default: +func URLPathFullClean(u *url.URL) (output string) { + lengthPath := len(u.Path) + lengthQuery := len(u.RawQuery) + appendForwardSlash := lengthPath > 1 && u.Path[lengthPath-1] == '/' + + switch { + case lengthPath == 1 && lengthQuery == 0: + return u.Path + case lengthPath == 1: return path.Clean(u.Path) + "?" + u.RawQuery + case lengthQuery != 0 && appendForwardSlash: + return path.Clean(u.Path) + "/?" + u.RawQuery + case lengthQuery != 0: + return path.Clean(u.Path) + "?" + u.RawQuery + case appendForwardSlash: + return path.Clean(u.Path) + "/" + default: + return path.Clean(u.Path) } } diff --git a/internal/utils/url_test.go b/internal/utils/url_test.go index 5e6bce016..a61cdc18c 100644 --- a/internal/utils/url_test.go +++ b/internal/utils/url_test.go @@ -14,13 +14,17 @@ func TestURLPathFullClean(t *testing.T) { have string expected string }{ + {"ShouldReturnFullPathSingleSlash", "https://example.com/", "/"}, + {"ShouldReturnFullPathSingleSlashWithQuery", "https://example.com/?query=1&alt=2", "/?query=1&alt=2"}, {"ShouldReturnFullPathNormal", "https://example.com/test", "/test"}, - {"ShouldReturnFullPathWithQuery", "https://example.com/test?query=1", "/test?query=1"}, - {"ShouldReturnCleanedPath", "https://example.com/five/../test?query=1", "/test?query=1"}, - {"ShouldReturnCleanedPathEscaped", "https://example.com/five/..%2ftest?query=1", "/test?query=1"}, - {"ShouldReturnCleanedPathEscapedExtra", "https://example.com/five/..%2ftest?query=1", "/test?query=1"}, - {"ShouldReturnCleanedPathEscapedExtraSurrounding", "https://example.com/five/%2f..%2f/test?query=1", "/test?query=1"}, - {"ShouldReturnCleanedPathEscapedPeriods", "https://example.com/five/%2f%2e%2e%2f/test?query=1", "/test?query=1"}, + {"ShouldReturnFullPathNormalWithSlashSuffix", "https://example.com/test/", "/test/"}, + {"ShouldReturnFullPathNormalWithSlashSuffixAndQuery", "https://example.com/test/?query=1&alt=2", "/test/?query=1&alt=2"}, + {"ShouldReturnFullPathWithQuery", "https://example.com/test?query=1&alt=2", "/test?query=1&alt=2"}, + {"ShouldReturnCleanedPath", "https://example.com/five/../test?query=1&alt=2", "/test?query=1&alt=2"}, + {"ShouldReturnCleanedPathEscaped", "https://example.com/five/..%2ftest?query=1&alt=2", "/test?query=1&alt=2"}, + {"ShouldReturnCleanedPathEscapedExtra", "https://example.com/five/..%2ftest?query=1&alt=2", "/test?query=1&alt=2"}, + {"ShouldReturnCleanedPathEscapedExtraSurrounding", "https://example.com/five/%2f..%2f/test?query=1&alt=2", "/test?query=1&alt=2"}, + {"ShouldReturnCleanedPathEscapedPeriods", "https://example.com/five/%2f%2e%2e%2f/test?query=1&alt=2", "/test?query=1&alt=2"}, } for _, tc := range testCases {