From 4da4b5371c2f5f9503d9cbc471df6cf7b556a6ca Mon Sep 17 00:00:00 2001 From: Osamah Bukraa Date: Thu, 15 Feb 2024 11:29:44 +0100 Subject: [PATCH 1/4] Add configurable fromUrl domain check --- readme.md | 4 ++++ src/controller.js | 7 ++++--- src/middleware/sso.js | 16 ++++++++++++---- src/util/isValidCallbackUrl.js | 10 ++++++++++ 4 files changed, 30 insertions(+), 7 deletions(-) create mode 100644 src/util/isValidCallbackUrl.js diff --git a/readme.md b/readme.md index b53f0aa..9562208 100644 --- a/readme.md +++ b/readme.md @@ -120,6 +120,10 @@ The login router & the SSO middleware use the same configuration. - **url**: *String* Url where the user will be retrieved with after login has succeeded (e.g.: https://api-gw-a.antwerpen.be/acpaas/shared-identity-data/v1) +- **allowedDomains**: *String[]* + List of domains allowed to redirect to after successful login + (e.g.: ['antwerpen.be']) + (e.g.: ['antwerpen.be', 'digipolis.be, 'gate15.be']) ### API Store configuration diff --git a/src/controller.js b/src/controller.js index c9f96e8..a2cca7d 100644 --- a/src/controller.js +++ b/src/controller.js @@ -11,6 +11,7 @@ import { getHost, logoutEncrypt, runHooks } from './helpers'; import createDeleteSessionsHook from './hooks/deleteSessions'; import createAssuranceLevelAndAuthMethodHook from './hooks/assuranceLevelAndAuthMethod'; import createDetermineLoginTypeHook from './hooks/determineLoginType'; +import { isValidCallbackUrl } from './util/isValidCallbackUrl'; const EXPIRY_MARGIN = 5 * 60 * 1000; export default function createController(config) { @@ -26,6 +27,7 @@ export default function createController(config) { errorRedirect = '/', key: objectKey = 'user', logLevel = 'error', + allowedDomains, } = config; const logger = pino({ @@ -162,7 +164,7 @@ export default function createController(config) { const stateKey = uuid.v4(); const url = createLoginUrl(host, stateKey, req.query); req.session.loginKey = stateKey; - req.session.fromUrl = req.query.fromUrl || '/'; + req.session.fromUrl = isValidCallbackUrl(req.query.fromUrl, allowedDomains) ? req.query.fromUrl : '/'; runHooks(preLoginHooks, req, res, () => req.session.save(() => res.redirect(url))); } @@ -215,8 +217,7 @@ export default function createController(config) { const username = getProp(user, 'dataSources.aprofiel.username'); logger.debug( - `finished hooks, redirecting ${username} to ${ - req.session.fromUrl || '/' + `finished hooks, redirecting ${username} to ${req.session.fromUrl || '/' }`, ); return req.session.save(() => res.redirect(req.session.fromUrl || '/')); diff --git a/src/middleware/sso.js b/src/middleware/sso.js index 4d04f3a..a2b4fcd 100644 --- a/src/middleware/sso.js +++ b/src/middleware/sso.js @@ -3,14 +3,21 @@ import pino from 'pino'; import { getSessions } from '../sessionStore'; import { getAccessToken } from '../accessToken'; +import { isValidCallbackUrl } from '../util/isValidCallbackUrl'; function getFallbackFromUrl(req, port) { return `${req.protocol}://${req.hostname}${port ? `:${port}` : ''}${req.originalUrl}`; } -function getFromUrl(req, port) { - const rawFromUrl = req.query.fromUrl || req.query.fromurl || getFallbackFromUrl(req, port); - return encodeURIComponent(rawFromUrl); +function getFromUrl(req, port, allowedDomains) { + if ( + (!req.query.fromUrl && !req.query.fromurl) || + !isValidCallbackUrl(req.query.fromUrl || req.query.fromurl, allowedDomains) + ) { + return encodeURIComponent(getFallbackFromUrl(req, port)); + } + + return encodeURIComponent(req.query.fromUrl || req.query.fromurl); } function getSessionWithAssuranceLevel(sessions, assuranceLevel) { @@ -28,6 +35,7 @@ export default function sso(options) { port = false, ssoCookieName = 'dgp.auth.ssokey', shouldUpgradeAssuranceLevel = true, + allowedDomains } = options; const loginPath = `${basePath}/login`; @@ -67,7 +75,7 @@ export default function sso(options) { return next(); } - const baseRedirectUrl = `${loginPath}?fromUrl=${getFromUrl(req, port)}`; + const baseRedirectUrl = `${loginPath}?fromUrl=${getFromUrl(req, port, allowedDomains)}`; const highSession = getSessionWithAssuranceLevel(sessions, 'high'); if (highSession) { diff --git a/src/util/isValidCallbackUrl.js b/src/util/isValidCallbackUrl.js new file mode 100644 index 0000000..a3ccbff --- /dev/null +++ b/src/util/isValidCallbackUrl.js @@ -0,0 +1,10 @@ +export function isValidCallbackUrl(url, allowedDomains = ['antwerpen.be']) { + const callbackUrl = new URL(url); + + if (callbackUrl.protocol !== 'https:') { + return false; + } + + const regex = new RegExp(`(${allowedDomains.map(allowedDomain => `${allowedDomain.replace('.', '\\.')}$`).join('|')})`); + return regex.test(callbackUrl.host); +} \ No newline at end of file From b950b418d6e7da7d5b72dcc45e132c38330a074e Mon Sep 17 00:00:00 2001 From: Osamah Bukraa Date: Thu, 15 Feb 2024 13:19:00 +0100 Subject: [PATCH 2/4] Fix syntax error in readme docs --- readme.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readme.md b/readme.md index 9562208..80b997f 100644 --- a/readme.md +++ b/readme.md @@ -123,7 +123,7 @@ The login router & the SSO middleware use the same configuration. - **allowedDomains**: *String[]* List of domains allowed to redirect to after successful login (e.g.: ['antwerpen.be']) - (e.g.: ['antwerpen.be', 'digipolis.be, 'gate15.be']) + (e.g.: ['antwerpen.be', 'digipolis.be', 'gate15.be']) ### API Store configuration From 5a5704b20f6b54481dfa2a355d4fefeb79d178d7 Mon Sep 17 00:00:00 2001 From: Osamah Bukraa Date: Thu, 15 Feb 2024 13:29:14 +0100 Subject: [PATCH 3/4] Fix linting issues --- package.json | 1 + src/middleware/sso.js | 6 +++--- src/util/isValidCallbackUrl.js | 14 +++++++------- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/package.json b/package.json index 7c5bc6e..af1c62f 100644 --- a/package.json +++ b/package.json @@ -9,6 +9,7 @@ "build": "npx rollup -c --exports default", "prepublish": "npm run build", "lint": "eslint . --ext .js", + "lint:fix": "eslint . --ext .js --fix", "test": "cross-env NODE_ENV=test nyc --reporter=lcov --reporter=text mocha" }, "publishConfig": { diff --git a/src/middleware/sso.js b/src/middleware/sso.js index a2b4fcd..4c2b00f 100644 --- a/src/middleware/sso.js +++ b/src/middleware/sso.js @@ -11,8 +11,8 @@ function getFallbackFromUrl(req, port) { function getFromUrl(req, port, allowedDomains) { if ( - (!req.query.fromUrl && !req.query.fromurl) || - !isValidCallbackUrl(req.query.fromUrl || req.query.fromurl, allowedDomains) + (!req.query.fromUrl && !req.query.fromurl) + || !isValidCallbackUrl(req.query.fromUrl || req.query.fromurl, allowedDomains) ) { return encodeURIComponent(getFallbackFromUrl(req, port)); } @@ -35,7 +35,7 @@ export default function sso(options) { port = false, ssoCookieName = 'dgp.auth.ssokey', shouldUpgradeAssuranceLevel = true, - allowedDomains + allowedDomains, } = options; const loginPath = `${basePath}/login`; diff --git a/src/util/isValidCallbackUrl.js b/src/util/isValidCallbackUrl.js index a3ccbff..5c9337f 100644 --- a/src/util/isValidCallbackUrl.js +++ b/src/util/isValidCallbackUrl.js @@ -1,10 +1,10 @@ export function isValidCallbackUrl(url, allowedDomains = ['antwerpen.be']) { - const callbackUrl = new URL(url); + const callbackUrl = new URL(url); - if (callbackUrl.protocol !== 'https:') { - return false; - } + if (callbackUrl.protocol !== 'https:') { + return false; + } - const regex = new RegExp(`(${allowedDomains.map(allowedDomain => `${allowedDomain.replace('.', '\\.')}$`).join('|')})`); - return regex.test(callbackUrl.host); -} \ No newline at end of file + const regex = new RegExp(`(${allowedDomains.map((allowedDomain) => `${allowedDomain.replace('.', '\\.')}$`).join('|')})`); + return regex.test(callbackUrl.host); +} From b97e9065b611711ef8d4e31c249671a1ad064553 Mon Sep 17 00:00:00 2001 From: Osamah Bukraa Date: Thu, 15 Feb 2024 13:49:22 +0100 Subject: [PATCH 4/4] Add test checking fromUrl logic, fix existing tests, add invalid url fallback --- src/util/isValidCallbackUrl.js | 14 +++++++---- test/login.js | 43 +++++++++++++++++++++++++++++++++- test/mocks/correctConfig.js | 1 + 3 files changed, 52 insertions(+), 6 deletions(-) diff --git a/src/util/isValidCallbackUrl.js b/src/util/isValidCallbackUrl.js index 5c9337f..2adc359 100644 --- a/src/util/isValidCallbackUrl.js +++ b/src/util/isValidCallbackUrl.js @@ -1,10 +1,14 @@ export function isValidCallbackUrl(url, allowedDomains = ['antwerpen.be']) { - const callbackUrl = new URL(url); + try { + const callbackUrl = new URL(url); - if (callbackUrl.protocol !== 'https:') { + if (callbackUrl.protocol !== 'https:') { + return false; + } + + const regex = new RegExp(`(${allowedDomains.map((allowedDomain) => `${allowedDomain.replace('.', '\\.')}$`).join('|')})`); + return regex.test(callbackUrl.host); + } catch (error) { return false; } - - const regex = new RegExp(`(${allowedDomains.map((allowedDomain) => `${allowedDomain.replace('.', '\\.')}$`).join('|')})`); - return regex.test(callbackUrl.host); } diff --git a/test/login.js b/test/login.js index 9380e33..e77bd1e 100644 --- a/test/login.js +++ b/test/login.js @@ -9,7 +9,7 @@ describe('GET /login', () => { it('should redirect to login', (done) => { const router = createRouter(mockExpress, correctConfig); const host = 'http://www.app.com'; - const fromUrl = 'test.com/d'; + const fromUrl = 'https://test.com/d'; let redirectUrl = false; const req = reqres.req({ url: '/auth/login', @@ -47,6 +47,47 @@ describe('GET /login', () => { router.handle(req, res); }); + it('should not redirect if fromUrl is invalid', (done) => { + const router = createRouter(mockExpress, correctConfig); + const host = 'http://www.app.com'; + const fromUrl = 'https://invalid.com/d'; + let redirectUrl = false; + const req = reqres.req({ + url: '/auth/login', + query: { + fromUrl, + }, + get: () => host, + session: { + save: (cb) => { + cb(); + }, + }, + }); + const res = reqres.res({ + header: () => { }, + redirect(val) { + redirectUrl = val; + this.emit('end'); + }, + }); + + res.on('end', () => { + assert(redirectUrl); + assert(req.session.fromUrl === '/'); + assert(redirectUrl.includes(encodeURIComponent(host))); + assert(redirectUrl.includes(encodeURIComponent(correctConfig.clientId))); + const scopes = correctConfig.defaultScopes.join(' '); + assert( + redirectUrl + .includes(encodeURIComponent(scopes)), + ); + return done(); + }); + + router.handle(req, res); + }); + it('should redirect to login with language if supplied', (done) => { const router = createRouter(mockExpress, correctConfig); const host = 'http://www.app.com'; diff --git a/test/mocks/correctConfig.js b/test/mocks/correctConfig.js index 8a33295..db2c1d6 100644 --- a/test/mocks/correctConfig.js +++ b/test/mocks/correctConfig.js @@ -15,6 +15,7 @@ const config = { url: 'https://api-gw-a.antwerpen.be/acpaas/shared-identity-data/v1', consentUrl: 'https://api-gw-a.antwerpen.be/acpaas/consent/v1', refresh: true, + allowedDomains: ['test.com'] }; export default config;