diff --git a/api/openapi.yml b/api/openapi.yml index c6d51dcfd..9ef9354dd 100644 --- a/api/openapi.yml +++ b/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: diff --git a/internal/handlers/handler_checks_safe_redirection.go b/internal/handlers/handler_checks_safe_redirection.go new file mode 100644 index 000000000..a621a9523 --- /dev/null +++ b/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 + } +} diff --git a/internal/handlers/handler_checks_safe_redirection_test.go b/internal/handlers/handler_checks_safe_redirection_test.go new file mode 100644 index 000000000..26b3360b1 --- /dev/null +++ b/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, + }) +} diff --git a/internal/handlers/response.go b/internal/handlers/response.go index d2d4072f9..c4b94058e 100644 --- a/internal/handlers/response.go +++ b/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) } diff --git a/internal/handlers/types.go b/internal/handlers/types.go index 85283d269..7cdff2463 100644 --- a/internal/handlers/types.go +++ b/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 { diff --git a/internal/mocks/mock_authelia_ctx.go b/internal/mocks/mock_authelia_ctx.go index b308a155a..461d69b67 100644 --- a/internal/mocks/mock_authelia_ctx.go +++ b/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()) diff --git a/internal/server/server.go b/internal/server/server.go index a602c78c2..d5f78c688 100644 --- a/internal/server/server.go +++ b/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)) diff --git a/internal/suites/suite_one_factor_only_test.go b/internal/suites/suite_one_factor_only_test.go index 02c382049..4b1971fd2 100644 --- a/internal/suites/suite_one_factor_only_test.go +++ b/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()) } diff --git a/internal/suites/suite_standalone_test.go b/internal/suites/suite_standalone_test.go index 08e3807b3..4c722777b 100644 --- a/internal/suites/suite_standalone_test.go +++ b/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() diff --git a/internal/utils/safe_redirection.go b/internal/utils/safe_redirection.go index d161c8b36..b7a95b9d0 100644 --- a/internal/utils/safe_redirection.go +++ b/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 +} diff --git a/internal/utils/safe_redirection_test.go b/internal/utils/safe_redirection_test.go index 642e11925..87c58a7db 100644 --- a/internal/utils/safe_redirection_test.go +++ b/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) +} diff --git a/web/src/hooks/Redirector.ts b/web/src/hooks/Redirector.ts index 18197a127..659b8603b 100644 --- a/web/src/hooks/Redirector.ts +++ b/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; - }; + }, []); } diff --git a/web/src/index.tsx b/web/src/index.tsx index 04d408f30..d6c04578b 100644 --- a/web/src/index.tsx +++ b/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"; diff --git a/web/src/services/Api.ts b/web/src/services/Api.ts index 8c90d1666..652c85601 100644 --- a/web/src/services/Api.ts +++ b/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"; diff --git a/web/src/services/SafeRedirection.ts b/web/src/services/SafeRedirection.ts new file mode 100644 index 000000000..f94bd1fee --- /dev/null +++ b/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(ChecksSafeRedirectionPath, { uri }); +} diff --git a/web/src/views/DeviceRegistration/RegisterOneTimePassword.tsx b/web/src/views/DeviceRegistration/RegisterOneTimePassword.tsx index 31c47cd62..c9cbbe6b6 100644 --- a/web/src/views/DeviceRegistration/RegisterOneTimePassword.tsx +++ b/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 ( { - 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) { diff --git a/web/src/views/LoginPortal/SecondFactor/OneTimePasswordMethod.tsx b/web/src/views/LoginPortal/SecondFactor/OneTimePasswordMethod.tsx index 6a75d3876..30a7ef566 100644 --- a/web/src/views/LoginPortal/SecondFactor/OneTimePasswordMethod.tsx +++ b/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(() => { diff --git a/web/src/views/LoginPortal/SecondFactor/SecurityKeyMethod.tsx b/web/src/views/LoginPortal/SecondFactor/SecurityKeyMethod.tsx index dad2284ec..9e6221749 100644 --- a/web/src/views/LoginPortal/SecondFactor/SecurityKeyMethod.tsx +++ b/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; }