From 7128970a5341625f1cde924907c8f7fffd78c7e0 Mon Sep 17 00:00:00 2001 From: Clement Michaud Date: Fri, 22 Sep 2017 17:53:18 +0200 Subject: [PATCH] Add redirection URL as a query parameter during authentication Before this fix, the redirection URL was stored in the user session, but this has a big drawback since user could open several pages in browser and thus override the redirection URL leading the user to be incorrectly redirected. --- example/nginx/nginx.conf | 14 +++++----- scripts/integration-tests.sh | 4 +-- src/client/lib/QueryParametersRetriever.ts | 12 +++++++++ src/client/lib/firstfactor/index.ts | 9 +++++-- src/client/lib/secondfactor/index.ts | 7 ++++- src/server/lib/routes/firstfactor/post.ts | 4 +-- .../lib/routes/secondfactor/redirect.ts | 2 +- src/server/lib/routes/verify/get.ts | 2 +- test/features/authentication.feature | 6 ++--- test/features/redirection.feature | 26 ++++++++++++++++++- test/features/resilience.feature | 2 +- test/features/support/world.ts | 2 +- .../server/routes/firstfactor/post.test.ts | 5 ++-- 13 files changed, 72 insertions(+), 23 deletions(-) create mode 100644 src/client/lib/QueryParametersRetriever.ts diff --git a/example/nginx/nginx.conf b/example/nginx/nginx.conf index a91df4ce6..65c8d501b 100644 --- a/example/nginx/nginx.conf +++ b/example/nginx/nginx.conf @@ -60,12 +60,6 @@ http { ssl_certificate /etc/ssl/server.crt; ssl_certificate_key /etc/ssl/server.key; - error_page 401 = @error401; - error_page 403 = https://auth.test.local:8080/error/403; - location @error401 { - return 302 https://auth.test.local:8080; - } - location /auth_verify { internal; proxy_set_header X-Original-URI $request_uri; @@ -78,12 +72,20 @@ http { location = /secret.html { auth_request /auth_verify; + auth_request_set $redirect $upstream_http_redirect; + proxy_set_header Redirect $redirect; + auth_request_set $user $upstream_http_x_remote_user; proxy_set_header X-Forwarded-User $user; + auth_request_set $groups $upstream_http_remote_groups; proxy_set_header Remote-Groups $groups; + auth_request_set $expiry $upstream_http_remote_expiry; proxy_set_header Remote-Expiry $expiry; + + error_page 401 =302 https://auth.test.local:8080?redirect=$redirect; + error_page 403 = https://auth.test.local:8080/error/403; } } } diff --git a/scripts/integration-tests.sh b/scripts/integration-tests.sh index 84675b53e..12ef19ed9 100755 --- a/scripts/integration-tests.sh +++ b/scripts/integration-tests.sh @@ -9,7 +9,7 @@ start_services() { } shut_services() { - $DC_SCRIPT down + $DC_SCRIPT down --remove-orphans } expect_services_count() { @@ -66,4 +66,4 @@ run_integration_tests run_other_tests # Test example with precompiled container -run_other_tests_docker \ No newline at end of file +run_other_tests_docker diff --git a/src/client/lib/QueryParametersRetriever.ts b/src/client/lib/QueryParametersRetriever.ts new file mode 100644 index 000000000..a529adb6e --- /dev/null +++ b/src/client/lib/QueryParametersRetriever.ts @@ -0,0 +1,12 @@ + +export class QueryParametersRetriever { + static get(name: string, url?: string): string { + if (!url) url = window.location.href; + name = name.replace(/[\[\]]/g, "\\$&"); + const regex = new RegExp("[?&]" + name + "(=([^&#]*)|&|#|$)"), + results = regex.exec(url); + if (!results) return undefined; + if (!results[2]) return ""; + return decodeURIComponent(results[2].replace(/\+/g, " ")); + } +} \ No newline at end of file diff --git a/src/client/lib/firstfactor/index.ts b/src/client/lib/firstfactor/index.ts index 9725c2bb0..1dfb03154 100644 --- a/src/client/lib/firstfactor/index.ts +++ b/src/client/lib/firstfactor/index.ts @@ -2,6 +2,7 @@ import FirstFactorValidator = require("./FirstFactorValidator"); import JSLogger = require("js-logger"); import UISelectors = require("./UISelectors"); import { Notifier } from "../Notifier"; +import { QueryParametersRetriever } from "../QueryParametersRetriever"; import Endpoints = require("../../../server/endpoints"); @@ -22,8 +23,12 @@ export default function (window: Window, $: JQueryStatic, function onFirstFactorSuccess() { jslogger.debug("First factor validated."); - // Redirect to second factor - window.location.href = Endpoints.SECOND_FACTOR_GET; + const redirectUrl = QueryParametersRetriever.get("redirect"); + if (redirectUrl) + window.location.href = Endpoints.SECOND_FACTOR_GET + "?redirect=" + + encodeURIComponent(redirectUrl); + else + window.location.href = Endpoints.SECOND_FACTOR_GET; } function onFirstFactorFailure(err: Error) { diff --git a/src/client/lib/secondfactor/index.ts b/src/client/lib/secondfactor/index.ts index 022c4a95a..4a60e101d 100644 --- a/src/client/lib/secondfactor/index.ts +++ b/src/client/lib/secondfactor/index.ts @@ -7,6 +7,7 @@ import U2FValidator = require("./U2FValidator"); import Endpoints = require("../../../server/endpoints"); import Constants = require("./constants"); import { Notifier } from "../Notifier"; +import { QueryParametersRetriever } from "../QueryParametersRetriever"; export default function (window: Window, $: JQueryStatic, u2fApi: typeof U2fApi) { @@ -14,7 +15,11 @@ export default function (window: Window, $: JQueryStatic, u2fApi: typeof U2fApi) const notifierU2f = new Notifier(".notification-u2f", $); function onAuthenticationSuccess(data: any) { - window.location.href = data.redirection_url; + const redirectUrl = QueryParametersRetriever.get("redirect"); + if (redirectUrl) + window.location.href = redirectUrl; + else + window.location.href = Endpoints.FIRST_FACTOR_GET; } function onSecondFactorTotpSuccess(data: any) { diff --git a/src/server/lib/routes/firstfactor/post.ts b/src/server/lib/routes/firstfactor/post.ts index b5fa2df8b..0cb48c401 100644 --- a/src/server/lib/routes/firstfactor/post.ts +++ b/src/server/lib/routes/firstfactor/post.ts @@ -59,8 +59,8 @@ export default function (req: express.Request, res: express.Response): BluebirdP logger.debug("1st factor: Mark successful authentication to regulator."); regulator.mark(username, true); - logger.debug("1st factor: Redirect to %s", Endpoint.SECOND_FACTOR_GET); - res.redirect(Endpoint.SECOND_FACTOR_GET); + res.status(204); + res.send(); return BluebirdPromise.resolve(); }) .catch(exceptions.LdapSearchError, ErrorReplies.replyWithError500(res, logger)) diff --git a/src/server/lib/routes/secondfactor/redirect.ts b/src/server/lib/routes/secondfactor/redirect.ts index 7ae69a3ad..2e6f5558c 100644 --- a/src/server/lib/routes/secondfactor/redirect.ts +++ b/src/server/lib/routes/secondfactor/redirect.ts @@ -8,7 +8,7 @@ import AuthenticationSession = require("../../AuthenticationSession"); export default function (req: express.Request, res: express.Response) { const authSession = AuthenticationSession.get(req); - const redirectUrl = authSession.redirect || Endpoints.FIRST_FACTOR_GET; + const redirectUrl = req.query.redirect || Endpoints.FIRST_FACTOR_GET; res.json({ redirection_url: redirectUrl }); diff --git a/src/server/lib/routes/verify/get.ts b/src/server/lib/routes/verify/get.ts index 59d06387b..57485074b 100644 --- a/src/server/lib/routes/verify/get.ts +++ b/src/server/lib/routes/verify/get.ts @@ -17,7 +17,7 @@ function verify_filter(req: express.Request, res: express.Response): BluebirdPro const authSession = AuthenticationSession.get(req); logger.debug("Verify: headers are %s", JSON.stringify(req.headers)); - authSession.redirect = "https://" + req.headers["host"] + req.headers["x-original-uri"]; + res.set("Redirect", encodeURIComponent("https://" + req.headers["host"] + req.headers["x-original-uri"])); return AuthenticationValidator.validate(req) .then(function () { diff --git a/test/features/authentication.feature b/test/features/authentication.feature index cf5a531e5..5625f6fea 100644 --- a/test/features/authentication.feature +++ b/test/features/authentication.feature @@ -18,14 +18,14 @@ Feature: User validate first factor Given I visit "https://auth.test.local:8080/" And I login with user "john" and password "password" And I register a TOTP secret called "Sec0" - When I visit "https://secret.test.local:8080/secret.html" and get redirected "https://auth.test.local:8080/" + When I visit "https://secret.test.local:8080/secret.html" and get redirected "https://auth.test.local:8080/?redirect=https%3A%2F%2Fsecret.test.local%3A8080%2Fsecret.html" And I login with user "john" and password "password" And I use "Sec0" as TOTP token handle And I click on "TOTP" Then I'm redirected to "https://secret.test.local:8080/secret.html" Scenario: User fails TOTP second factor - When I visit "https://secret.test.local:8080/secret.html" and get redirected "https://auth.test.local:8080/" + When I visit "https://secret.test.local:8080/secret.html" and get redirected "https://auth.test.local:8080/?redirect=https%3A%2F%2Fsecret.test.local%3A8080%2Fsecret.html" And I login with user "john" and password "password" And I use "BADTOKEN" as TOTP token And I click on "TOTP" @@ -50,4 +50,4 @@ Feature: User validate first factor And I login with user "john" and password "password" And I use "Sec0" as TOTP token handle When I visit "https://auth.test.local:8080/logout?redirect=https://www.google.fr" - Then I'm redirected to "https://www.google.fr" \ No newline at end of file + Then I'm redirected to "https://www.google.fr" diff --git a/test/features/redirection.feature b/test/features/redirection.feature index 760f23436..6ce54c648 100644 --- a/test/features/redirection.feature +++ b/test/features/redirection.feature @@ -20,4 +20,28 @@ Feature: User is correctly redirected Scenario: User Harry does not have access to https://secret.test.local:8080/secret.html and thus he must get an error 401 When I register TOTP and login with user "harry" and password "password" And I visit "https://secret.test.local:8080/secret.html" - Then I get an error 403 \ No newline at end of file + Then I get an error 403 + + + + Scenario: Redirection URL is propagated from restricted page to first factor + When I visit "https://public.test.local:8080/secret.html" + Then I'm redirected to "https://auth.test.local:8080/?redirect=https%3A%2F%2Fpublic.test.local%3A8080%2Fsecret.html" + + Scenario: Redirection URL is propagated from first factor to second factor + Given I visit "https://auth.test.local:8080/" + And I login with user "john" and password "password" + And I register a TOTP secret called "Sec0" + When I visit "https://public.test.local:8080/secret.html" + And I login with user "john" and password "password" + Then I'm redirected to "https://auth.test.local:8080/secondfactor?redirect=https%3A%2F%2Fpublic.test.local%3A8080%2Fsecret.html" + + Scenario: Redirection URL is used to send user from second factor to target page + Given I visit "https://auth.test.local:8080/" + And I login with user "john" and password "password" + And I register a TOTP secret called "Sec0" + When I visit "https://public.test.local:8080/secret.html" + And I login with user "john" and password "password" + And I use "Sec0" as TOTP token handle + And I click on "TOTP" + Then I'm redirected to "https://public.test.local:8080/secret.html" \ No newline at end of file diff --git a/test/features/resilience.feature b/test/features/resilience.feature index 800ad0154..9a63dfd28 100644 --- a/test/features/resilience.feature +++ b/test/features/resilience.feature @@ -12,7 +12,7 @@ Feature: Authelia keeps user sessions despite the application restart And I login with user "john" and password "password" And I register a TOTP secret called "Sec0" When the application restarts - And I visit "https://secret.test.local:8080/secret.html" and get redirected "https://auth.test.local:8080/" + And I visit "https://secret.test.local:8080/secret.html" and get redirected "https://auth.test.local:8080/?redirect=https%3A%2F%2Fsecret.test.local%3A8080%2Fsecret.html" And I login with user "john" and password "password" And I use "Sec0" as TOTP token handle And I click on "TOTP" diff --git a/test/features/support/world.ts b/test/features/support/world.ts index 698d76cbb..fe5b5db48 100644 --- a/test/features/support/world.ts +++ b/test/features/support/world.ts @@ -99,7 +99,7 @@ function CustomWorld() { }; this.useTotpToken = function (totpSecret: string) { - return that.driver.wait(seleniumWebdriver.until.elementLocated(seleniumWebdriver.By.className("register-totp")), 5000) + return that.driver.wait(seleniumWebdriver.until.elementLocated(seleniumWebdriver.By.id("token")), 5000) .then(function () { return that.driver.findElement(seleniumWebdriver.By.id("token")) .sendKeys(totpSecret); diff --git a/test/unit/server/routes/firstfactor/post.test.ts b/test/unit/server/routes/firstfactor/post.test.ts index aa04fc2a0..91ee1b746 100644 --- a/test/unit/server/routes/firstfactor/post.test.ts +++ b/test/unit/server/routes/firstfactor/post.test.ts @@ -71,7 +71,7 @@ describe("test the first factor validation route", function () { res = ExpressMock.ResponseMock(); }); - it("should redirect client to second factor page", function () { + it("should reply with 204 if success", function () { (serverVariables.ldapAuthenticator as any).authenticate.withArgs("username", "password") .returns(BluebirdPromise.resolve({ emails: emails, @@ -81,7 +81,8 @@ describe("test the first factor validation route", function () { return FirstFactorPost.default(req as any, res as any) .then(function () { assert.equal("username", authSession.userid); - assert.equal(Endpoints.SECOND_FACTOR_GET, res.redirect.getCall(0).args[0]); + assert(res.send.calledOnce); + assert(res.status.calledWith(204)); }); });