fix: user is now redirected when authenticated (#2082)

* fix(handlers,web): user is now redirected when authenticated

Fix: #1788

* remove dead code and fix ci issues

* fix infinite loop in frontend

* fix issue with integration tests

* handle bot recommendation

* fix integration test & add dot to comment

* fix last integration test

* Update api/openapi.yml

Co-authored-by: James Elliott <james-d-elliott@users.noreply.github.com>

* Update web/src/services/SafeRedirection.ts

Co-authored-by: Amir Zarrinkafsh <nightah@me.com>

* Update web/src/services/SafeRedirection.ts

Co-authored-by: Amir Zarrinkafsh <nightah@me.com>

* Update api/openapi.yml

* Update openapi.yml

* refactor: valid -> safe

* refactor: adjust merge conflicts

* Apply suggestions from code review

Co-authored-by: James Elliott <james-d-elliott@users.noreply.github.com>

* fix: adjust test return messaging

Co-authored-by: James Elliott <james-d-elliott@users.noreply.github.com>
Co-authored-by: Amir Zarrinkafsh <nightah@me.com>

api/openapi.yml

@@ -166,13 +166,6 @@ paths:
       responses:
         "200":
           description: Successful Operation
-          headers:
-            Set-Cookie:
-              style: simple
-              explode: false
-              schema:
-                type: string
-                example: authelia_session=kTTCSLupEUirZVfLeZTijezewFQnNOgs; Path=/
           content:
             application/json:
               schema:
@@ -181,6 +174,30 @@ paths:
           description: Unauthorized
       security:
         - authelia_auth: []
+  /api/checks/safe-redirection:
+    post:
+      tags:
+        - Authentication
+      summary: Check whether URI is safe to redirect to.
+      description: >
+        End users usually needs to be redirected to a target website after authentication. This endpoint aims to check
+        if target URL is safe to redirect to. This prevents open redirect attacks.
+      requestBody:
+        content:
+          application/json:
+            schema:
+              $ref: '#/components/schemas/handlers.checkURIWithinDomainRequestBody'
+      responses:
+        "200":
+          description: Successful Operation
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/handlers.checkURIWithinDomainResponseBody'
+        "401":
+          description: Unauthorized
+      security:
+        - authelia_auth: []
   /api/logout:
     post:
       tags:

internal/handlers/handler_checks_safe_redirection.go

@@ -0,0 +1,41 @@
+package handlers
+
+import (
+	"fmt"
+
+	"github.com/authelia/authelia/internal/authentication"
+	"github.com/authelia/authelia/internal/middlewares"
+	"github.com/authelia/authelia/internal/utils"
+)
+
+// CheckSafeRedirection handler checking whether the redirection to a given URL provided in body is safe.
+func CheckSafeRedirection(ctx *middlewares.AutheliaCtx) {
+	userSession := ctx.GetSession()
+
+	if userSession.AuthenticationLevel == authentication.NotAuthenticated {
+		ctx.ReplyUnauthorized()
+		return
+	}
+
+	var reqBody checkURIWithinDomainRequestBody
+
+	err := ctx.ParseBody(&reqBody)
+	if err != nil {
+		ctx.Error(fmt.Errorf("Unable to parse request body: %w", err), messageOperationFailed)
+		return
+	}
+
+	safe, err := utils.IsRedirectionURISafe(reqBody.URI, ctx.Configuration.Session.Domain)
+	if err != nil {
+		ctx.Error(fmt.Errorf("Unable to determine if uri %s is safe to redirect to: %w", reqBody.URI, err), messageOperationFailed)
+		return
+	}
+
+	err = ctx.SetJSONBody(checkURIWithinDomainResponseBody{
+		OK: safe,
+	})
+	if err != nil {
+		ctx.Error(fmt.Errorf("Unable to create response body: %w", err), messageOperationFailed)
+		return
+	}
+}

internal/handlers/handler_checks_safe_redirection_test.go

@@ -0,0 +1,65 @@
+package handlers
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/authelia/authelia/internal/authentication"
+	"github.com/authelia/authelia/internal/mocks"
+	"github.com/authelia/authelia/internal/session"
+)
+
+var exampleDotComDomain = "example.com"
+
+func TestCheckSafeRedirection_ForbiddenCall(t *testing.T) {
+	mock := mocks.NewMockAutheliaCtxWithUserSession(t, session.UserSession{
+		Username:            "john",
+		AuthenticationLevel: authentication.NotAuthenticated,
+	})
+	defer mock.Close()
+	mock.Ctx.Configuration.Session.Domain = exampleDotComDomain
+
+	mock.SetRequestBody(t, checkURIWithinDomainRequestBody{
+		URI: "http://myapp.example.com",
+	})
+
+	CheckSafeRedirection(mock.Ctx)
+	assert.Equal(t, 401, mock.Ctx.Response.StatusCode())
+}
+
+func TestCheckSafeRedirection_UnsafeRedirection(t *testing.T) {
+	mock := mocks.NewMockAutheliaCtxWithUserSession(t, session.UserSession{
+		Username:            "john",
+		AuthenticationLevel: authentication.OneFactor,
+	})
+	defer mock.Close()
+	mock.Ctx.Configuration.Session.Domain = exampleDotComDomain
+
+	mock.SetRequestBody(t, checkURIWithinDomainRequestBody{
+		URI: "http://myapp.com",
+	})
+
+	CheckSafeRedirection(mock.Ctx)
+	mock.Assert200OK(t, checkURIWithinDomainResponseBody{
+		OK: false,
+	})
+}
+
+func TestCheckSafeRedirection_SafeRedirection(t *testing.T) {
+	mock := mocks.NewMockAutheliaCtxWithUserSession(t, session.UserSession{
+		Username:            "john",
+		AuthenticationLevel: authentication.OneFactor,
+	})
+	defer mock.Close()
+	mock.Ctx.Configuration.Session.Domain = exampleDotComDomain
+
+	mock.SetRequestBody(t, checkURIWithinDomainRequestBody{
+		URI: "https://myapp.example.com",
+	})
+
+	CheckSafeRedirection(mock.Ctx)
+	mock.Assert200OK(t, checkURIWithinDomainResponseBody{
+		OK: true,
+	})
+}

internal/handlers/response.go

@@ -101,10 +101,8 @@ func Handle1FAResponse(ctx *middlewares.AutheliaCtx, targetURI, requestMethod st
 	}
 
 	ctx.Logger.Debugf("Redirection URL %s is safe", targetURI)
+	err = ctx.SetJSONBody(redirectResponse{Redirect: targetURI})
 
-	response := redirectResponse{Redirect: targetURI}
-
-	err = ctx.SetJSONBody(response)
 	if err != nil {
 		ctx.Logger.Errorf("Unable to set redirection URL in body: %s", err)
 	}
@@ -125,15 +123,17 @@ func Handle2FAResponse(ctx *middlewares.AutheliaCtx, targetURI string) {
 		return
 	}
 
-	targetURL, err := url.ParseRequestURI(targetURI)
+	safe, err := utils.IsRedirectionURISafe(targetURI, ctx.Configuration.Session.Domain)
 
 	if err != nil {
-		ctx.Error(fmt.Errorf("Unable to parse target URL: %s", err), messageMFAValidationFailed)
+		ctx.Error(fmt.Errorf("Unable to check target URL: %s", err), messageMFAValidationFailed)
 		return
 	}
 
-	if targetURL != nil && utils.IsRedirectionSafe(*targetURL, ctx.Configuration.Session.Domain) {
+	if safe {
+		ctx.Logger.Debugf("Redirection URL %s is safe", targetURI)
 		err := ctx.SetJSONBody(redirectResponse{Redirect: targetURI})
+
 		if err != nil {
 			ctx.Logger.Errorf("Unable to set redirection URL in body: %s", err)
 		}

internal/handlers/types.go

@@ -53,6 +53,16 @@ type firstFactorRequestBody struct {
 	// TODO(c.michaud): add required validation once the above PR is merged.
 }
 
+// checkURIWithinDomainRequestBody represents the JSON body received by the endpoint checking if an URI is within
+// the configured domain.
+type checkURIWithinDomainRequestBody struct {
+	URI string `json:"uri"`
+}
+
+type checkURIWithinDomainResponseBody struct {
+	OK bool `json:"ok"`
+}
+
 // redirectResponse represent the response sent by the first factor endpoint
 // when a redirection URL has been provided.
 type redirectResponse struct {

internal/mocks/mock_authelia_ctx.go

@@ -127,12 +127,28 @@ func NewMockAutheliaCtx(t *testing.T) *MockAutheliaCtx {
 	return mockAuthelia
 }
 
+// NewMockAutheliaCtxWithUserSession create an instance of AutheliaCtx mock with predefined user session.
+func NewMockAutheliaCtxWithUserSession(t *testing.T, userSession session.UserSession) *MockAutheliaCtx {
+	mock := NewMockAutheliaCtx(t)
+	err := mock.Ctx.SaveSession(userSession)
+	require.NoError(t, err)
+
+	return mock
+}
+
 // Close close the mock.
 func (m *MockAutheliaCtx) Close() {
 	m.Hook.Reset()
 	m.Ctrl.Finish()
 }
 
+// SetRequestBody set the request body from a struct with json tags.
+func (m *MockAutheliaCtx) SetRequestBody(t *testing.T, body interface{}) {
+	bodyBytes, err := json.Marshal(body)
+	require.NoError(t, err)
+	m.Ctx.Request.SetBody(bodyBytes)
+}
+
 // Assert401KO assert an error response from the service.
 func (m *MockAutheliaCtx) Assert401KO(t *testing.T, message string) {
 	assert.Equal(t, 401, m.Ctx.Response.StatusCode())

internal/server/server.go

@@ -64,6 +64,8 @@ func registerRoutes(configuration schema.Configuration, providers middlewares.Pr
 	r.GET("/api/verify", autheliaMiddleware(handlers.VerifyGet(configuration.AuthenticationBackend)))
 	r.HEAD("/api/verify", autheliaMiddleware(handlers.VerifyGet(configuration.AuthenticationBackend)))
 
+	r.POST("/api/checks/safe-redirection", autheliaMiddleware(handlers.CheckSafeRedirection))
+
 	r.POST("/api/firstfactor", autheliaMiddleware(handlers.FirstFactorPost(1000, true)))
 	r.POST("/api/logout", autheliaMiddleware(handlers.LogoutPost))
 

internal/suites/suite_one_factor_only_test.go

@@ -2,6 +2,7 @@ package suites
 
 import (
 	"context"
+	"fmt"
 	"log"
 	"testing"
 	"time"
@@ -69,12 +70,35 @@ func (s *OneFactorOnlyWebSuite) TestShouldDisplayAuthenticatedView() {
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 	defer cancel()
 
-	s.doLoginOneFactor(ctx, s.T(), "john", "password", false, "http://unsafe.local")
+	s.doLoginOneFactor(ctx, s.T(), "john", "password", false, "")
 	s.verifyURLIs(ctx, s.T(), HomeBaseURL+"/")
 	s.doVisit(s.T(), GetLoginBaseURL())
 	s.verifyIsAuthenticatedPage(ctx, s.T())
 }
 
+func (s *OneFactorOnlyWebSuite) TestShouldRedirectAlreadyAuthenticatedUser() {
+	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second)
+	defer cancel()
+
+	s.doLoginOneFactor(ctx, s.T(), "john", "password", false, "")
+	s.verifyURLIs(ctx, s.T(), HomeBaseURL+"/")
+
+	s.doVisit(s.T(), fmt.Sprintf("%s?rd=https://singlefactor.example.com:8080/secret.html", GetLoginBaseURL()))
+	s.verifyURLIs(ctx, s.T(), "https://singlefactor.example.com:8080/secret.html")
+}
+
+func (s *OneFactorOnlyWebSuite) TestShouldNotRedirectAlreadyAuthenticatedUserToUnsafeURL() {
+	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second)
+	defer cancel()
+
+	s.doLoginOneFactor(ctx, s.T(), "john", "password", false, "")
+	s.verifyURLIs(ctx, s.T(), HomeBaseURL+"/")
+
+	// Visit the login page and wait for redirection to 2FA page with success icon displayed.
+	s.doVisit(s.T(), fmt.Sprintf("%s?rd=https://secure.example.local:8080", GetLoginBaseURL()))
+	s.verifyNotificationDisplayed(ctx, s.T(), "Redirection was determined to be unsafe and aborted. Ensure the redirection URL is correct.")
+}
+
 func (s *OneFactorOnlySuite) TestWeb() {
 	suite.Run(s.T(), NewOneFactorOnlyWebSuite())
 }

internal/suites/suite_standalone_test.go

@@ -67,6 +67,36 @@ func (s *StandaloneWebDriverSuite) TestShouldLetUserKnowHeIsAlreadyAuthenticated
 	s.verifyIsAuthenticatedPage(ctx, s.T())
 }
 
+func (s *StandaloneWebDriverSuite) TestShouldRedirectAlreadyAuthenticatedUser() {
+	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second)
+	defer cancel()
+
+	_ = s.doRegisterAndLogin2FA(ctx, s.T(), "john", "password", false, "")
+
+	// Visit home page to change context.
+	s.doVisit(s.T(), HomeBaseURL)
+	s.verifyIsHome(ctx, s.T())
+
+	// Visit the login page and wait for redirection to 2FA page with success icon displayed.
+	s.doVisit(s.T(), fmt.Sprintf("%s?rd=https://secure.example.com:8080", GetLoginBaseURL()))
+	s.verifyURLIs(ctx, s.T(), "https://secure.example.com:8080/")
+}
+
+func (s *StandaloneWebDriverSuite) TestShouldNotRedirectAlreadyAuthenticatedUserToUnsafeURL() {
+	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second)
+	defer cancel()
+
+	_ = s.doRegisterAndLogin2FA(ctx, s.T(), "john", "password", false, "")
+
+	// Visit home page to change context.
+	s.doVisit(s.T(), HomeBaseURL)
+	s.verifyIsHome(ctx, s.T())
+
+	// Visit the login page and wait for redirection to 2FA page with success icon displayed.
+	s.doVisit(s.T(), fmt.Sprintf("%s?rd=https://secure.example.local:8080", GetLoginBaseURL()))
+	s.verifyNotificationDisplayed(ctx, s.T(), "Redirection was determined to be unsafe and aborted. Ensure the redirection URL is correct.")
+}
+
 func (s *StandaloneWebDriverSuite) TestShouldCheckUserIsAskedToRegisterDevice() {
 	ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
 	defer cancel()

internal/utils/safe_redirection.go

@@ -1,11 +1,12 @@
 package utils
 
 import (
+	"fmt"
 	"net/url"
 	"strings"
 )
 
-// IsRedirectionSafe determines if a redirection URL is secured.
+// 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
@@ -17,3 +18,14 @@ func IsRedirectionSafe(url url.URL, protectedDomain string) bool {
 
 	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

@@ -12,14 +12,31 @@ func isURLSafe(requestURI string, domain string) bool { //nolint:unparam
 	return IsRedirectionSafe(*url, domain)
 }
 
-func TestShouldReturnFalseOnBadScheme(t *testing.T) {
+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 TestShouldReturnFalseOnBadDomain(t *testing.T) {
+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)
+}

web/src/hooks/Redirector.ts

@@ -1,5 +1,7 @@
+import { useCallback } from "react";
+
 export function useRedirector() {
-    return (url: string) => {
+    return useCallback((url: string) => {
         window.location.href = url;
-    };
+    }, []);
 }

web/src/index.tsx

@@ -1,9 +1,8 @@
-import "@utils/AssetPath";
-
 import React from "react";
 
 import ReactDOM from "react-dom";
 
+import "@utils/AssetPath";
 import "@root/index.css";
 import App from "@root/App";
 import * as serviceWorker from "@root/serviceWorker";

web/src/services/Api.ts

@@ -25,6 +25,7 @@ export const InitiateResetPasswordPath = basePath + "/api/reset-password/identit
 export const CompleteResetPasswordPath = basePath + "/api/reset-password/identity/finish";
 // Do the password reset during completion.
 export const ResetPasswordPath = basePath + "/api/reset-password";
+export const ChecksSafeRedirectionPath = basePath + "/api/checks/safe-redirection";
 
 export const LogoutPath = basePath + "/api/logout";
 export const StatePath = basePath + "/api/state";

web/src/services/SafeRedirection.ts

@@ -0,0 +1,10 @@
+import { ChecksSafeRedirectionPath } from "@services/Api";
+import { PostWithOptionalResponse } from "@services/Client";
+
+interface SafeRedirectionResponse {
+    ok: boolean;
+}
+
+export async function checkSafeRedirection(uri: string) {
+    return PostWithOptionalResponse<SafeRedirectionResponse>(ChecksSafeRedirectionPath, { uri });
+}

web/src/views/DeviceRegistration/RegisterOneTimePassword.tsx

@@ -56,6 +56,7 @@ const RegisterOneTimePassword = function () {
     useEffect(() => {
         completeRegistrationProcess();
     }, [completeRegistrationProcess]);
+
     function SecretButton(text: string | undefined, action: string, icon: IconDefinition) {
         return (
             <IconButton

web/src/views/LoginPortal/LoginPortal.tsx

@@ -1,14 +1,14 @@
-import React, { useEffect, Fragment, ReactNode, useState, useCallback } from "react";
+import React, { Fragment, ReactNode, useCallback, useEffect, useState } from "react";
 
-import { Switch, Route, Redirect, useHistory, useLocation } from "react-router";
+import { Redirect, Route, Switch, useHistory, useLocation } from "react-router";
 
 import {
+    AuthenticatedRoute,
     FirstFactorRoute,
+    SecondFactorPushRoute,
     SecondFactorRoute,
     SecondFactorTOTPRoute,
-    SecondFactorPushRoute,
     SecondFactorU2FRoute,
-    AuthenticatedRoute,
 } from "@constants/Routes";
 import { useConfiguration } from "@hooks/Configuration";
 import { useNotifications } from "@hooks/NotificationsContext";
@@ -18,6 +18,7 @@ import { useRequestMethod } from "@hooks/RequestMethod";
 import { useAutheliaState } from "@hooks/State";
 import { useUserPreferences as userUserInfo } from "@hooks/UserInfo";
 import { SecondFactorMethod } from "@models/Methods";
+import { checkSafeRedirection } from "@services/SafeRedirection";
 import { AuthenticationLevel } from "@services/State";
 import LoadingPage from "@views/LoadingPage/LoadingPage";
 import AuthenticatedView from "@views/LoginPortal/AuthenticatedView/AuthenticatedView";
@@ -29,6 +30,9 @@ export interface Props {
     resetPassword: boolean;
 }
 
+const RedirectionErrorMessage =
+    "Redirection was determined to be unsafe and aborted. Ensure the redirection URL is correct.";
+
 const LoginPortal = function (props: Props) {
     const history = useHistory();
     const location = useLocation();
@@ -87,7 +91,31 @@ const LoginPortal = function (props: Props) {
 
     // Redirect to the correct stage if not enough authenticated
     useEffect(() => {
-        if (state) {
+        (async function () {
+            if (!state) {
+                return;
+            }
+
+            if (
+                redirectionURL &&
+                ((configuration &&
+                    !configuration.second_factor_enabled &&
+                    state.authentication_level >= AuthenticationLevel.OneFactor) ||
+                    state.authentication_level === AuthenticationLevel.TwoFactor)
+            ) {
+                try {
+                    const res = await checkSafeRedirection(redirectionURL);
+                    if (res && res.ok) {
+                        redirector(redirectionURL);
+                    } else {
+                        createErrorNotification(RedirectionErrorMessage);
+                    }
+                } catch (err) {
+                    createErrorNotification(RedirectionErrorMessage);
+                }
+                return;
+            }
+
             const redirectionSuffix = redirectionURL
                 ? `?rd=${encodeURIComponent(redirectionURL)}${requestMethod ? `&rm=${requestMethod}` : ""}`
                 : "";
@@ -108,8 +136,18 @@ const LoginPortal = function (props: Props) {
                     }
                 }
             }
-        }
-    }, [state, redirectionURL, requestMethod, redirect, userInfo, setFirstFactorDisabled, configuration]);
+        })();
+    }, [
+        state,
+        redirectionURL,
+        requestMethod,
+        redirect,
+        userInfo,
+        setFirstFactorDisabled,
+        configuration,
+        createErrorNotification,
+        redirector,
+    ]);
 
     const handleAuthSuccess = async (redirectionURL: string | undefined) => {
         if (redirectionURL) {

web/src/views/LoginPortal/SecondFactor/OneTimePasswordMethod.tsx

@@ -38,7 +38,7 @@ const OneTimePasswordMethod = function (props: Props) {
     /* eslint-enable react-hooks/exhaustive-deps */
 
     const signInFunc = useCallback(async () => {
-        if (props.authenticationLevel === AuthenticationLevel.TwoFactor) {
+        if (!props.registered || props.authenticationLevel === AuthenticationLevel.TwoFactor) {
             return;
         }
 
@@ -59,7 +59,14 @@ const OneTimePasswordMethod = function (props: Props) {
             setState(State.Failure);
         }
         setPasscode("");
-    }, [passcode, onSignInErrorCallback, onSignInSuccessCallback, redirectionURL, props.authenticationLevel]);
+    }, [
+        passcode,
+        onSignInErrorCallback,
+        onSignInSuccessCallback,
+        redirectionURL,
+        props.authenticationLevel,
+        props.registered,
+    ]);
 
     // Set successful state if user is already authenticated.
     useEffect(() => {

web/src/views/LoginPortal/SecondFactor/SecurityKeyMethod.tsx

@@ -47,7 +47,7 @@ const SecurityKeyMethod = function (props: Props) {
 
     const doInitiateSignIn = useCallback(async () => {
         // If user is already authenticated, we don't initiate sign in process.
-        if (!props.registered || props.authenticationLevel >= AuthenticationLevel.TwoFactor) {
+        if (!props.registered || props.authenticationLevel === AuthenticationLevel.TwoFactor) {
             return;
         }