fix(authorization): object path not normalized (#3661)

This fixes an issue where the object path is not normalized.

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.

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{
+	return Object{
-		Scheme: targetURL.Scheme,
+		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.

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)
+		})
+	}
 }

internal/middlewares/authelia_context.go

@@ -286,9 +286,8 @@ func (ctx *AutheliaCtx) GetOriginalURL() (*url.URL, error) {
 
 	var requestURI string
 
-	scheme := forwardedProto
+	forwardedProto = append(forwardedProto, protoHostSeparator...)
-	scheme = append(scheme, protoHostSeparator...)
+	requestURI = string(append(forwardedProto,
-	requestURI = string(append(scheme,
 		append(forwardedHost, forwardedURI...)...))
 
 	parsedURL, err := url.ParseRequestURI(requestURI)

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
+	}
+}

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)
+		})
+	}
+}