From f0cb75e1e102f95f91e9254c66c797e821857690 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Tue, 13 Apr 2021 18:38:12 +1000 Subject: [PATCH] fix(handlers): logout redirection validation (#1908) --- api/openapi.yml | 26 ++++++++++++- internal/handlers/handler_logout.go | 38 +++++++++++++++++-- internal/suites/action_logout.go | 13 +++++++ .../suites/scenario_redirection_check_test.go | 24 +++++++++++- internal/suites/suite_standalone_test.go | 4 ++ internal/suites/verify_url_is.go | 1 + web/src/services/Client.ts | 4 +- web/src/services/SignOut.ts | 15 +++++++- web/src/views/LoginPortal/SignOut/SignOut.tsx | 11 ++++-- 9 files changed, 123 insertions(+), 13 deletions(-) diff --git a/api/openapi.yml b/api/openapi.yml index c0d48be46..c6d51dcfd 100644 --- a/api/openapi.yml +++ b/api/openapi.yml @@ -187,13 +187,19 @@ paths: - Authentication summary: Logout description: The logout endpoint allows a user to logout and destroy a sesssion. + requestBody: + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/handlers.logoutRequestBody' responses: "200": description: Successful Operation content: application/json: schema: - $ref: '#/components/schemas/middlewares.OkResponse' + $ref: '#/components/schemas/handlers.logoutResponseBody' security: - authelia_auth: [] /api/reset-password/identity/start: @@ -567,6 +573,24 @@ components: totp_period: type: integer example: 30 + handlers.logoutRequestBody: + type: object + properties: + targetURL: + type: string + example: https://redirect.example.com + handlers.logoutResponseBody: + type: object + properties: + status: + type: string + example: OK + data: + type: object + properties: + safeTargetURL: + type: boolean + example: true handlers.firstFactorRequestBody: required: - username diff --git a/internal/handlers/handler_logout.go b/internal/handlers/handler_logout.go index 0f6c46554..f9b54eb29 100644 --- a/internal/handlers/handler_logout.go +++ b/internal/handlers/handler_logout.go @@ -2,18 +2,50 @@ package handlers import ( "fmt" + "net/url" "github.com/authelia/authelia/internal/middlewares" + "github.com/authelia/authelia/internal/utils" ) +type logoutBody struct { + TargetURL string `json:"targetURL"` +} + +type logoutResponseBody struct { + SafeTargetURL bool `json:"safeTargetURL"` +} + // LogoutPost is the handler logging out the user attached to the given cookie. func LogoutPost(ctx *middlewares.AutheliaCtx) { - ctx.Logger.Tracef("Destroy session") - err := ctx.Providers.SessionProvider.DestroySession(ctx.RequestCtx) + body := logoutBody{} + responseBody := logoutResponseBody{SafeTargetURL: false} + ctx.Logger.Tracef("Attempting to decode body") + + err := ctx.ParseBody(&body) + if err != nil { + ctx.Error(fmt.Errorf("Unable to parse body during logout: %s", err), operationFailedMessage) + } + + ctx.Logger.Tracef("Attempting to destroy session") + + err = ctx.Providers.SessionProvider.DestroySession(ctx.RequestCtx) if err != nil { ctx.Error(fmt.Errorf("Unable to destroy session during logout: %s", err), operationFailedMessage) } - ctx.ReplyOK() + redirectionURL, err := url.Parse(body.TargetURL) + if err == nil { + responseBody.SafeTargetURL = utils.IsRedirectionSafe(*redirectionURL, ctx.Configuration.Session.Domain) + } + + if body.TargetURL != "" { + ctx.Logger.Debugf("Logout target url is %s, safe %t", body.TargetURL, responseBody.SafeTargetURL) + } + + err = ctx.SetJSONBody(responseBody) + if err != nil { + ctx.Error(fmt.Errorf("Unable to set body during logout: %s", err), operationFailedMessage) + } } diff --git a/internal/suites/action_logout.go b/internal/suites/action_logout.go index cbac88c4b..36cac95c8 100644 --- a/internal/suites/action_logout.go +++ b/internal/suites/action_logout.go @@ -3,6 +3,7 @@ package suites import ( "context" "fmt" + "net/url" "testing" ) @@ -10,3 +11,15 @@ func (wds *WebDriverSession) doLogout(ctx context.Context, t *testing.T) { wds.doVisit(t, fmt.Sprintf("%s%s", GetLoginBaseURL(), "/logout")) wds.verifyIsFirstFactorPage(ctx, t) } + +func (wds *WebDriverSession) doLogoutWithRedirect(ctx context.Context, t *testing.T, targetURL string, firstFactor bool) { + wds.doVisit(t, fmt.Sprintf("%s%s%s", GetLoginBaseURL(), "/logout?rd=", url.QueryEscape(targetURL))) + + if firstFactor { + wds.verifyIsFirstFactorPage(ctx, t) + + return + } + + wds.verifyURLIs(ctx, t, targetURL) +} diff --git a/internal/suites/scenario_redirection_check_test.go b/internal/suites/scenario_redirection_check_test.go index 57c7e335f..fc369f65c 100644 --- a/internal/suites/scenario_redirection_check_test.go +++ b/internal/suites/scenario_redirection_check_test.go @@ -57,7 +57,7 @@ var redirectionAuthorizations = map[string]bool{ "https://secure.example.com:8080/secret.html": true, } -func (s *RedirectionCheckScenario) TestShouldRedirectOnlyWhenDomainIsHandledByAuthelia() { +func (s *RedirectionCheckScenario) TestShouldRedirectOnLoginOnlyWhenDomainIsSafe() { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() @@ -77,6 +77,28 @@ func (s *RedirectionCheckScenario) TestShouldRedirectOnlyWhenDomainIsHandledByAu } } +var logoutRedirectionURLs = map[string]bool{ + // external website + "https://www.google.fr": false, + // Not the right domain + "https://public.example-not-right.com:8080/index.html": false, + // Not https + "http://public.example.com:8080/index.html": false, + // Domain handled by Authelia + "https://public.example.com:8080/index.html": true, +} + +func (s *RedirectionCheckScenario) TestShouldRedirectOnLogoutOnlyWhenDomainIsSafe() { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + for url, success := range logoutRedirectionURLs { + s.T().Run(url, func(t *testing.T) { + s.doLogoutWithRedirect(ctx, t, url, !success) + }) + } +} + func TestRedirectionCheckScenario(t *testing.T) { if testing.Short() { t.Skip("skipping suite test in short mode") diff --git a/internal/suites/suite_standalone_test.go b/internal/suites/suite_standalone_test.go index 9d49bf61d..44e20acad 100644 --- a/internal/suites/suite_standalone_test.go +++ b/internal/suites/suite_standalone_test.go @@ -212,6 +212,10 @@ func (s *StandaloneSuite) TestRedirectionURLScenario() { suite.Run(s.T(), NewRedirectionURLScenario()) } +func (s *StandaloneSuite) TestRedirectionCheckScenario() { + suite.Run(s.T(), NewRedirectionCheckScenario()) +} + func TestStandaloneSuite(t *testing.T) { if testing.Short() { t.Skip("skipping suite test in short mode") diff --git a/internal/suites/verify_url_is.go b/internal/suites/verify_url_is.go index 009ef6565..db3848b06 100644 --- a/internal/suites/verify_url_is.go +++ b/internal/suites/verify_url_is.go @@ -15,6 +15,7 @@ func (wds *WebDriverSession) verifyURLIs(ctx context.Context, t *testing.T, url if err != nil { return false, err } + return currentURL == url, nil }) diff --git a/web/src/services/Client.ts b/web/src/services/Client.ts index 56681446c..3c65d9a18 100644 --- a/web/src/services/Client.ts +++ b/web/src/services/Client.ts @@ -2,13 +2,13 @@ import axios from "axios"; import { ServiceResponse, hasServiceError, toData } from "./Api"; -export async function PostWithOptionalResponse(path: string, body?: any) { +export async function PostWithOptionalResponse(path: string, body?: any): Promise { const res = await axios.post>(path, body); if (res.status !== 200 || hasServiceError(res).errored) { throw new Error(`Failed POST to ${path}. Code: ${res.status}. Message: ${hasServiceError(res).message}`); } - return toData(res); + return toData(res); } export async function Post(path: string, body?: any) { diff --git a/web/src/services/SignOut.ts b/web/src/services/SignOut.ts index 6209a132b..db5ea9711 100644 --- a/web/src/services/SignOut.ts +++ b/web/src/services/SignOut.ts @@ -1,6 +1,17 @@ import { LogoutPath } from "./Api"; import { PostWithOptionalResponse } from "./Client"; -export async function signOut() { - return PostWithOptionalResponse(LogoutPath); +export type SignOutResponse = { safeTargetURL: boolean } | undefined; + +export type SignOutBody = { + targetURL?: string; +}; + +export async function signOut(targetURL: string | undefined): Promise { + const body: SignOutBody = {}; + if (targetURL) { + body.targetURL = targetURL; + } + + return PostWithOptionalResponse(LogoutPath, body); } diff --git a/web/src/views/LoginPortal/SignOut/SignOut.tsx b/web/src/views/LoginPortal/SignOut/SignOut.tsx index b44a4b73a..c1a95794b 100644 --- a/web/src/views/LoginPortal/SignOut/SignOut.tsx +++ b/web/src/views/LoginPortal/SignOut/SignOut.tsx @@ -18,11 +18,14 @@ const SignOut = function (props: Props) { const { createErrorNotification } = useNotifications(); const redirectionURL = useRedirectionURL(); const [timedOut, setTimedOut] = useState(false); + const [safeRedirect, setSafeRedirect] = useState(false); const doSignOut = useCallback(async () => { try { - // TODO(c.michaud): pass redirection URL to backend for validation. - await signOut(); + const res = await signOut(redirectionURL); + if (res !== undefined && res.safeTargetURL) { + setSafeRedirect(true); + } setTimeout(() => { if (!mounted) { return; @@ -33,14 +36,14 @@ const SignOut = function (props: Props) { console.error(err); createErrorNotification("There was an issue signing out"); } - }, [createErrorNotification, setTimedOut, mounted]); + }, [createErrorNotification, redirectionURL, setSafeRedirect, setTimedOut, mounted]); useEffect(() => { doSignOut(); }, [doSignOut]); if (timedOut) { - if (redirectionURL) { + if (redirectionURL && safeRedirect) { window.location.href = redirectionURL; } else { return ;