fix(utils): domain suffix improperly checked (#3799)

internal/handlers/handler_logout.go

@@ -33,7 +33,7 @@ func LogoutPOST(ctx *middlewares.AutheliaCtx) {
 
 	redirectionURL, err := url.Parse(body.TargetURL)
 	if err == nil {
-		responseBody.SafeTargetURL = utils.IsRedirectionSafe(*redirectionURL, ctx.Configuration.Session.Domain)
+		responseBody.SafeTargetURL = utils.URLDomainHasSuffix(*redirectionURL, ctx.Configuration.Session.Domain)
 	}
 
 	if body.TargetURL != "" {

internal/handlers/handler_sign_duo_test.go

@@ -614,14 +614,14 @@ func (s *SecondFactorDuoPostSuite) TestShouldRedirectUserToSafeTargetURL() {
 	duoMock.EXPECT().AuthCall(s.mock.Ctx, gomock.Any()).Return(&response, nil)
 
 	bodyBytes, err := json.Marshal(signDuoRequestBody{
-		TargetURL: "https://mydomain.local",
+		TargetURL: "https://example.com",
 	})
 	s.Require().NoError(err)
 	s.mock.Ctx.Request.SetBody(bodyBytes)
 
 	DuoPOST(duoMock)(s.mock.Ctx)
 	s.mock.Assert200OK(s.T(), redirectResponse{
-		Redirect: "https://mydomain.local",
+		Redirect: "https://example.com",
 	})
 }
 
@@ -663,7 +663,7 @@ func (s *SecondFactorDuoPostSuite) TestShouldNotRedirectToUnsafeURL() {
 	duoMock.EXPECT().AuthCall(s.mock.Ctx, gomock.Any()).Return(&response, nil)
 
 	bodyBytes, err := json.Marshal(signDuoRequestBody{
-		TargetURL: "http://mydomain.local",
+		TargetURL: "http://example.com",
 	})
 	s.Require().NoError(err)
 	s.mock.Ctx.Request.SetBody(bodyBytes)
@@ -710,7 +710,7 @@ func (s *SecondFactorDuoPostSuite) TestShouldRegenerateSessionForPreventingSessi
 	duoMock.EXPECT().AuthCall(s.mock.Ctx, gomock.Any()).Return(&response, nil)
 
 	bodyBytes, err := json.Marshal(signDuoRequestBody{
-		TargetURL: "http://mydomain.local",
+		TargetURL: "http://example.com",
 	})
 	s.Require().NoError(err)
 	s.mock.Ctx.Request.SetBody(bodyBytes)

internal/handlers/handler_sign_totp_test.go

@@ -167,7 +167,7 @@ func (s *HandlerSignTOTPSuite) TestShouldRedirectUserToSafeTargetURL() {
 
 	bodyBytes, err := json.Marshal(signTOTPRequestBody{
 		Token:     "abc",
-		TargetURL: "https://mydomain.local",
+		TargetURL: "https://example.com",
 	})
 
 	s.Require().NoError(err)
@@ -175,7 +175,7 @@ func (s *HandlerSignTOTPSuite) TestShouldRedirectUserToSafeTargetURL() {
 
 	TimeBasedOneTimePasswordPOST(s.mock.Ctx)
 	s.mock.Assert200OK(s.T(), redirectResponse{
-		Redirect: "https://mydomain.local",
+		Redirect: "https://example.com",
 	})
 }
 
@@ -205,7 +205,7 @@ func (s *HandlerSignTOTPSuite) TestShouldNotRedirectToUnsafeURL() {
 
 	bodyBytes, err := json.Marshal(signTOTPRequestBody{
 		Token:     "abc",
-		TargetURL: "http://mydomain.local",
+		TargetURL: "http://example.com",
 	})
 
 	s.Require().NoError(err)

internal/handlers/response.go

@@ -104,7 +104,7 @@ func Handle1FAResponse(ctx *middlewares.AutheliaCtx, targetURI, requestMethod st
 		return
 	}
 
-	if !utils.IsRedirectionSafe(*targetURL, ctx.Configuration.Session.Domain) {
+	if !utils.URLDomainHasSuffix(*targetURL, ctx.Configuration.Session.Domain) {
 		ctx.Logger.Debugf("Redirection URL %s is not safe", targetURI)
 
 		if !ctx.Providers.Authorizer.IsSecondFactorEnabled() && ctx.Configuration.DefaultRedirectionURL != "" {

internal/mocks/authelia_ctx.go

@@ -67,11 +67,12 @@ func NewMockAutheliaCtx(t *testing.T) *MockAutheliaCtx {
 	datetime, _ := time.Parse("2006-Jan-02", "2013-Feb-03")
 	mockAuthelia.Clock.Set(datetime)
 
-	configuration := schema.Configuration{}
+	config := schema.Configuration{}
-	configuration.Session.RememberMeDuration = schema.DefaultSessionConfiguration.RememberMeDuration
+	config.Session.RememberMeDuration = schema.DefaultSessionConfiguration.RememberMeDuration
-	configuration.Session.Name = "authelia_session"
+	config.Session.Name = "authelia_session"
-	configuration.AccessControl.DefaultPolicy = "deny"
+	config.Session.Domain = "example.com"
-	configuration.AccessControl.Rules = []schema.ACLRule{{
+	config.AccessControl.DefaultPolicy = "deny"
+	config.AccessControl.Rules = []schema.ACLRule{{
 		Domains: []string{"bypass.example.com"},
 		Policy:  "bypass",
 	}, {
@@ -106,12 +107,12 @@ func NewMockAutheliaCtx(t *testing.T) *MockAutheliaCtx {
 	providers.Notifier = mockAuthelia.NotifierMock
 
 	providers.Authorizer = authorization.NewAuthorizer(
-		&configuration)
+		&config)
 
 	providers.SessionProvider = session.NewProvider(
-		configuration.Session, nil)
+		config.Session, nil)
 
-	providers.Regulator = regulation.NewRegulator(configuration.Regulation, providers.StorageProvider, &mockAuthelia.Clock)
+	providers.Regulator = regulation.NewRegulator(config.Regulation, providers.StorageProvider, &mockAuthelia.Clock)
 
 	mockAuthelia.TOTPMock = NewMockTOTP(mockAuthelia.Ctrl)
 	providers.TOTP = mockAuthelia.TOTPMock
@@ -126,7 +127,7 @@ func NewMockAutheliaCtx(t *testing.T) *MockAutheliaCtx {
 	// Set a cookie to identify this client throughout the test.
 	// request.Request.Header.SetCookie("authelia_session", "client_cookie").
 
-	ctx := middlewares.NewAutheliaCtx(request, configuration, providers)
+	ctx := middlewares.NewAutheliaCtx(request, config, providers)
 	mockAuthelia.Ctx = ctx
 
 	logger, hook := test.NewNullLogger()

internal/utils/const.go

@@ -31,6 +31,11 @@ const (
 	unknown = "unknown"
 )
 
+const (
+	period = "."
+	https  = "https"
+)
+
 // X.509 consts.
 const (
 	BlockTypeRSAPrivateKey      = "RSA PRIVATE KEY"

internal/utils/safe_redirection.go

@@ -1,31 +0,0 @@
-package utils
-
-import (
-	"fmt"
-	"net/url"
-	"strings"
-)
-
-// IsRedirectionSafe determines whether the URL is safe to be redirected to.
-func IsRedirectionSafe(url url.URL, protectedDomain string) bool {
-	if url.Scheme != "https" {
-		return false
-	}
-
-	if !strings.HasSuffix(url.Hostname(), protectedDomain) {
-		return false
-	}
-
-	return true
-}
-
-// IsRedirectionURISafe determines whether the URI is safe to be redirected to.
-func IsRedirectionURISafe(uri, protectedDomain string) (bool, error) {
-	targetURL, err := url.ParseRequestURI(uri)
-
-	if err != nil {
-		return false, fmt.Errorf("Unable to parse redirection URI %s: %w", uri, err)
-	}
-
-	return targetURL != nil && IsRedirectionSafe(*targetURL, protectedDomain), nil
-}

internal/utils/safe_redirection_test.go

@@ -1,42 +0,0 @@
-package utils
-
-import (
-	"net/url"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-)
-
-func isURLSafe(requestURI string, domain string) bool { //nolint:unparam
-	url, _ := url.ParseRequestURI(requestURI)
-	return IsRedirectionSafe(*url, domain)
-}
-
-func TestIsRedirectionSafe_ShouldReturnFalseOnBadScheme(t *testing.T) {
-	assert.False(t, isURLSafe("http://secure.example.com", "example.com"))
-	assert.False(t, isURLSafe("ftp://secure.example.com", "example.com"))
-	assert.True(t, isURLSafe("https://secure.example.com", "example.com"))
-}
-
-func TestIsRedirectionSafe_ShouldReturnFalseOnBadDomain(t *testing.T) {
-	assert.False(t, isURLSafe("https://secure.example.com.c", "example.com"))
-	assert.False(t, isURLSafe("https://secure.example.comc", "example.com"))
-	assert.False(t, isURLSafe("https://secure.example.co", "example.com"))
-}
-
-func TestIsRedirectionURISafe_CannotParseURI(t *testing.T) {
-	_, err := IsRedirectionURISafe("http//invalid", "example.com")
-	assert.EqualError(t, err, "Unable to parse redirection URI http//invalid: parse \"http//invalid\": invalid URI for request")
-}
-
-func TestIsRedirectionURISafe_InvalidRedirectionURI(t *testing.T) {
-	valid, err := IsRedirectionURISafe("http://myurl.com/myresource", "example.com")
-	assert.NoError(t, err)
-	assert.False(t, valid)
-}
-
-func TestIsRedirectionURISafe_ValidRedirectionURI(t *testing.T) {
-	valid, err := IsRedirectionURISafe("http://myurl.example.com/myresource", "example.com")
-	assert.NoError(t, err)
-	assert.False(t, valid)
-}

internal/utils/url.go

@@ -1,8 +1,10 @@
 package utils
 
 import (
+	"fmt"
 	"net/url"
 	"path"
+	"strings"
 )
 
 // URLPathFullClean returns a URL path with the query parameters appended (full path) with the path portion parsed
@@ -27,3 +29,31 @@ func URLPathFullClean(u *url.URL) (output string) {
 		return path.Clean(u.Path)
 	}
 }
+
+// URLDomainHasSuffix determines whether the uri has a suffix of the domain value.
+func URLDomainHasSuffix(uri url.URL, domain string) bool {
+	if uri.Scheme != https {
+		return false
+	}
+
+	if uri.Hostname() == domain {
+		return true
+	}
+
+	if strings.HasSuffix(uri.Hostname(), period+domain) {
+		return true
+	}
+
+	return false
+}
+
+// IsRedirectionURISafe determines whether the URI is safe to be redirected to.
+func IsRedirectionURISafe(uri, protectedDomain string) (safe bool, err error) {
+	var parsedURI *url.URL
+
+	if parsedURI, err = url.ParseRequestURI(uri); err != nil {
+		return false, fmt.Errorf("failed to parse URI '%s': %w", uri, err)
+	}
+
+	return parsedURI != nil && URLDomainHasSuffix(*parsedURI, protectedDomain), nil
+}

internal/utils/url_test.go

@@ -38,3 +38,46 @@ func TestURLPathFullClean(t *testing.T) {
 		})
 	}
 }
+
+func isURLSafe(requestURI string, domain string) bool { //nolint:unparam
+	u, _ := url.ParseRequestURI(requestURI)
+	return URLDomainHasSuffix(*u, domain)
+}
+
+func TestIsRedirectionSafe_ShouldReturnTrueOnExactDomain(t *testing.T) {
+	assert.True(t, isURLSafe("https://example.com", "example.com"))
+}
+
+func TestIsRedirectionSafe_ShouldReturnFalseOnBadScheme(t *testing.T) {
+	assert.False(t, isURLSafe("http://secure.example.com", "example.com"))
+	assert.False(t, isURLSafe("ftp://secure.example.com", "example.com"))
+	assert.True(t, isURLSafe("https://secure.example.com", "example.com"))
+}
+
+func TestIsRedirectionSafe_ShouldReturnFalseOnBadDomain(t *testing.T) {
+	assert.False(t, isURLSafe("https://secure.example.com.c", "example.com"))
+	assert.False(t, isURLSafe("https://secure.example.comc", "example.com"))
+	assert.False(t, isURLSafe("https://secure.example.co", "example.com"))
+	assert.False(t, isURLSafe("https://secure.notexample.com", "example.com"))
+}
+
+func TestIsRedirectionURISafe_CannotParseURI(t *testing.T) {
+	_, err := IsRedirectionURISafe("http//invalid", "example.com")
+	assert.EqualError(t, err, "failed to parse URI 'http//invalid': parse \"http//invalid\": invalid URI for request")
+}
+
+func TestIsRedirectionURISafe_InvalidRedirectionURI(t *testing.T) {
+	valid, err := IsRedirectionURISafe("http://myurl.com/myresource", "example.com")
+	assert.NoError(t, err)
+	assert.False(t, valid)
+}
+
+func TestIsRedirectionURISafe_ValidRedirectionURI(t *testing.T) {
+	valid, err := IsRedirectionURISafe("http://myurl.example.com/myresource", "example.com")
+	assert.NoError(t, err)
+	assert.False(t, valid)
+
+	valid, err = IsRedirectionURISafe("http://example.com/myresource", "example.com")
+	assert.NoError(t, err)
+	assert.False(t, valid)
+}