From 664d65d7fb4cd77f66629dc42c0a2c4a3ccc36a4 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Tue, 5 Jul 2022 11:32:10 +1000 Subject: [PATCH] fix(authorization): object path not normalized (#3661) This fixes an issue where the object path is not normalized. --- internal/authorization/authorizer_test.go | 38 +++++++++++++-- internal/authorization/types.go | 20 +++----- internal/authorization/types_test.go | 59 +++++++++++++++++++++-- internal/middlewares/authelia_context.go | 5 +- internal/utils/url.go | 17 +++++++ internal/utils/url_test.go | 36 ++++++++++++++ 6 files changed, 153 insertions(+), 22 deletions(-) create mode 100644 internal/utils/url.go create mode 100644 internal/utils/url_test.go diff --git a/internal/authorization/authorizer_test.go b/internal/authorization/authorizer_test.go index ea3bcdf33..4bf7b3c03 100644 --- a/internal/authorization/authorizer_test.go +++ b/internal/authorization/authorizer_test.go @@ -257,7 +257,9 @@ func (s *AuthorizerSuite) TestShouldCheckDomainMatching() { Build() tester.CheckAuthorizations(s.T(), John, "https://public.example.com", "GET", Bypass) + tester.CheckAuthorizations(s.T(), John, "https://public.example.com:8080/", "GET", Bypass) tester.CheckAuthorizations(s.T(), Bob, "https://public.example.com", "GET", Bypass) + tester.CheckAuthorizations(s.T(), Bob, "https://public.example.com:8080", "GET", Bypass) tester.CheckAuthorizations(s.T(), AnonymousUser, "https://public.example.com", "GET", Bypass) tester.CheckAuthorizations(s.T(), John, "https://one-factor.example.com", "GET", OneFactor) @@ -650,21 +652,51 @@ func (s *AuthorizerSuite) TestShouldCheckResourceMatching() { WithRule(schema.ACLRule{ Domains: []string{"resource.example.com"}, Policy: bypass, - Resources: createSliceRegexRule(s.T(), []string{"^/bypass/[a-z]+$", "^/$", "embedded"}), + Resources: createSliceRegexRule(s.T(), []string{"^/case/[a-z]+$", "^/$"}), + }). + WithRule(schema.ACLRule{ + Domains: []string{"resource.example.com"}, + Policy: bypass, + Resources: createSliceRegexRule(s.T(), []string{"^/bypass/.*$", "^/$", "embedded"}), }). WithRule(schema.ACLRule{ Domains: []string{"resource.example.com"}, Policy: oneFactor, - Resources: createSliceRegexRule(s.T(), []string{"^/one_factor/[a-z]+$"}), + Resources: createSliceRegexRule(s.T(), []string{"^/one_factor/.*$"}), + }). + WithRule(schema.ACLRule{ + Domains: []string{"resource.example.com"}, + Policy: twoFactor, + Resources: createSliceRegexRule(s.T(), []string{"^/a/longer/rule/.*$"}), }). 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/ABC", "GET", Denied) 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/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) + tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/..%2fa/longer/rule/abc", "GET", TwoFactor) + tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/..%2F/a/longer/rule/abc", "GET", TwoFactor) + tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/..%2Fa/longer/rule/abc", "GET", TwoFactor) + tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/%2e%2e/a/longer/rule/abc", "GET", TwoFactor) + tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/%2e%2e//a/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%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/a/longer/rule/abc", "GET", TwoFactor) + tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/bypass/%2E%2E//a/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%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) } // This test assures that rules without domains (not allowed by schema validator at this time) will pass validation correctly. diff --git a/internal/authorization/types.go b/internal/authorization/types.go index 5644f39a6..d02a1081a 100644 --- a/internal/authorization/types.go +++ b/internal/authorization/types.go @@ -5,6 +5,8 @@ import ( "net" "net/url" "strings" + + "github.com/authelia/authelia/v4/internal/utils" ) // SubjectMatcher is a matcher that takes a subject. @@ -41,7 +43,8 @@ func (s Subject) IsAnonymous() bool { // Object represents a protected object for the purposes of ACL matching. type Object struct { - Scheme string + URL url.URL + Domain string Path string Method string @@ -49,7 +52,7 @@ type Object struct { // String is a string representation of the Object. func (o Object) String() string { - return fmt.Sprintf("%s://%s%s", o.Scheme, o.Domain, o.Path) + return o.URL.String() } // NewObjectRaw creates a new Object type from a URL and a method header. @@ -59,19 +62,12 @@ func NewObjectRaw(targetURL *url.URL, method []byte) (object Object) { // NewObject creates a new Object type from a URL and a method header. func NewObject(targetURL *url.URL, method string) (object Object) { - object = Object{ - Scheme: targetURL.Scheme, + return Object{ + URL: *targetURL, Domain: targetURL.Hostname(), + Path: utils.URLPathFullClean(targetURL), Method: method, } - - if targetURL.RawQuery == "" { - object.Path = targetURL.Path - } else { - object.Path = targetURL.Path + "?" + targetURL.RawQuery - } - - return object } // RuleMatchResult describes how well a rule matched a subject/object combo. diff --git a/internal/authorization/types_test.go b/internal/authorization/types_test.go index 0124c0f86..91b6a608a 100644 --- a/internal/authorization/types_test.go +++ b/internal/authorization/types_test.go @@ -15,10 +15,10 @@ func TestShouldAppendQueryParamToURL(t *testing.T) { object := NewObject(targetURL, "GET") + assert.Equal(t, "https", object.URL.Scheme) assert.Equal(t, "domain.example.com", object.Domain) - assert.Equal(t, "GET", object.Method) assert.Equal(t, "/api?type=none", object.Path) - assert.Equal(t, "https", object.Scheme) + assert.Equal(t, "GET", object.Method) } func TestShouldCreateNewObjectFromRaw(t *testing.T) { @@ -28,8 +28,59 @@ func TestShouldCreateNewObjectFromRaw(t *testing.T) { object := NewObjectRaw(targetURL, []byte("GET")) + assert.Equal(t, "https", object.URL.Scheme) assert.Equal(t, "domain.example.com", object.Domain) - assert.Equal(t, "GET", object.Method) + assert.Equal(t, "/api", object.URL.Path) assert.Equal(t, "/api", object.Path) - assert.Equal(t, "https", object.Scheme) + assert.Equal(t, "GET", object.Method) +} + +func TestShouldCleanURL(t *testing.T) { + testCases := []struct { + have string + havePath string + method string + + expectedScheme, expectedDomain, expectedPath, expectedPathClean string + }{ + {"https://a.com", "/a/../t", "GET", "https", "a.com", "/a/../t", "/t"}, + {"https://a.com", "/a/..%2f/t", "GET", "https", "a.com", "/a/..//t", "/t"}, + {"https://a.com", "/a/..%2ft", "GET", "https", "a.com", "/a/../t", "/t"}, + {"https://a.com", "/a/..%2F/t", "GET", "https", "a.com", "/a/..//t", "/t"}, + {"https://a.com", "/a/..%2Ft", "GET", "https", "a.com", "/a/../t", "/t"}, + {"https://a.com", "/a/..%2Ft", "GET", "https", "a.com", "/a/../t", "/t"}, + {"https://a.com", "/a/%2F..%2Ft", "GET", "https", "a.com", "/a//../t", "/t"}, + {"https://a.com", "/a/%2F%2e%2e%2Ft", "GET", "https", "a.com", "/a//../t", "/t"}, + } + + for _, tc := range testCases { + t.Run(tc.have, func(t *testing.T) { + have, err := url.Parse(tc.have + tc.havePath) + require.NoError(t, err) + + object := NewObject(have, tc.method) + + assert.Equal(t, tc.expectedScheme, object.URL.Scheme) + assert.Equal(t, tc.expectedDomain, object.Domain) + assert.Equal(t, tc.expectedPath, object.URL.Path) + assert.Equal(t, tc.expectedPathClean, object.Path) + assert.Equal(t, tc.method, object.Method) + + have, err = url.Parse(tc.have) + require.NoError(t, err) + + path, err := url.ParseRequestURI(tc.havePath) + require.NoError(t, err) + + have.Path, have.RawQuery = path.Path, path.RawQuery + + object = NewObject(have, tc.method) + + assert.Equal(t, tc.expectedScheme, object.URL.Scheme) + assert.Equal(t, tc.expectedDomain, object.Domain) + assert.Equal(t, tc.expectedPath, object.URL.Path) + assert.Equal(t, tc.expectedPathClean, object.Path) + assert.Equal(t, tc.method, object.Method) + }) + } } diff --git a/internal/middlewares/authelia_context.go b/internal/middlewares/authelia_context.go index 8f73106c9..0c43083f0 100644 --- a/internal/middlewares/authelia_context.go +++ b/internal/middlewares/authelia_context.go @@ -286,9 +286,8 @@ func (ctx *AutheliaCtx) GetOriginalURL() (*url.URL, error) { var requestURI string - scheme := forwardedProto - scheme = append(scheme, protoHostSeparator...) - requestURI = string(append(scheme, + forwardedProto = append(forwardedProto, protoHostSeparator...) + requestURI = string(append(forwardedProto, append(forwardedHost, forwardedURI...)...)) parsedURL, err := url.ParseRequestURI(requestURI) diff --git a/internal/utils/url.go b/internal/utils/url.go new file mode 100644 index 000000000..9d912a1ae --- /dev/null +++ b/internal/utils/url.go @@ -0,0 +1,17 @@ +package utils + +import ( + "net/url" + "path" +) + +// 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: + return path.Clean(u.Path) + "?" + u.RawQuery + } +} diff --git a/internal/utils/url_test.go b/internal/utils/url_test.go new file mode 100644 index 000000000..5e6bce016 --- /dev/null +++ b/internal/utils/url_test.go @@ -0,0 +1,36 @@ +package utils + +import ( + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestURLPathFullClean(t *testing.T) { + testCases := []struct { + name string + have string + expected string + }{ + {"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"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + u, err := url.Parse(tc.have) + require.NoError(t, err) + + actual := URLPathFullClean(u) + + assert.Equal(t, tc.expected, actual) + }) + } +}