From 6ca0e852283a69db231bfccb8d29a500c7b01164 Mon Sep 17 00:00:00 2001 From: Yiheng Cao <65160922+Crispy-fried-chicken@users.noreply.github.com> Date: Sat, 24 Aug 2024 04:49:23 +0000 Subject: [PATCH] Update nextUrl validation to incorporate serverBasePath (#2048) (#2050) (cherry picked from commit fc4f6a27c0c80881be9e8ed6b9259a25c3fa0e13) Signed-off-by: Yiheng Cao <65160922+Crispy-fried-chicken@users.noreply.github.com> --- server/auth/types/basic/routes.ts | 12 ++++++++++- server/auth/types/openid/routes.ts | 8 ++++++-- server/auth/types/proxy/routes.ts | 4 +++- server/auth/types/saml/routes.ts | 8 ++++++-- server/utils/next_url.test.ts | 33 +++++++++++++++++++++--------- server/utils/next_url.ts | 28 +++++++++++++++---------- 6 files changed, 66 insertions(+), 27 deletions(-) diff --git a/server/auth/types/basic/routes.ts b/server/auth/types/basic/routes.ts index bae3e338c..f0e0bc1d3 100755 --- a/server/auth/types/basic/routes.ts +++ b/server/auth/types/basic/routes.ts @@ -47,7 +47,17 @@ export class BasicAuthRoutes { this.coreSetup.http.resources.register( { path: LOGIN_PAGE_URI, - validate: false, + validate: { + query: schema.object({ + nextUrl: schema.maybe( + schema.string({ + validate: (nexturl) => { + return validateNextUrl(nexturl, this.coreSetup.http.basePath.serverBasePath); + }, + }) + ), + }), + }, options: { authRequired: false, }, diff --git a/server/auth/types/openid/routes.ts b/server/auth/types/openid/routes.ts index 8634f09e2..b9c76de7b 100644 --- a/server/auth/types/openid/routes.ts +++ b/server/auth/types/openid/routes.ts @@ -96,7 +96,9 @@ export class OpenIdAuthRoutes { code: schema.maybe(schema.string()), nextUrl: schema.maybe( schema.string({ - validate: validateNextUrl, + validate: (nexturl) => { + return validateNextUrl(nexturl, this.core.http.basePath.serverBasePath); + }, }) ), redirectHash: schema.maybe(schema.boolean()), @@ -293,7 +295,9 @@ export class OpenIdAuthRoutes { query: schema.object({ nextUrl: schema.maybe( schema.string({ - validate: validateNextUrl, + validate: (nexturl) => { + return validateNextUrl(nexturl, this.core.http.basePath.serverBasePath); + }, }) ), }), diff --git a/server/auth/types/proxy/routes.ts b/server/auth/types/proxy/routes.ts index 54a4c54f2..8da5c2169 100644 --- a/server/auth/types/proxy/routes.ts +++ b/server/auth/types/proxy/routes.ts @@ -38,7 +38,9 @@ export class ProxyAuthRoutes { query: schema.object({ nextUrl: schema.maybe( schema.string({ - validate: validateNextUrl, + validate: (nexturl) => { + return validateNextUrl(nexturl, this.coreSetup.http.basePath.serverBasePath); + }, }) ), }), diff --git a/server/auth/types/saml/routes.ts b/server/auth/types/saml/routes.ts index 52af443af..0e01803c1 100644 --- a/server/auth/types/saml/routes.ts +++ b/server/auth/types/saml/routes.ts @@ -55,7 +55,9 @@ export class SamlAuthRoutes { query: schema.object({ nextUrl: schema.maybe( schema.string({ - validate: validateNextUrl, + validate: (nexturl) => { + return validateNextUrl(nexturl, this.coreSetup.http.basePath.serverBasePath); + }, }) ), redirectHash: schema.string(), @@ -270,7 +272,9 @@ export class SamlAuthRoutes { query: schema.object({ nextUrl: schema.maybe( schema.string({ - validate: validateNextUrl, + validate: (nexturl) => { + return validateNextUrl(nexturl, this.coreSetup.http.basePath.serverBasePath); + }, }) ), }), diff --git a/server/utils/next_url.test.ts b/server/utils/next_url.test.ts index e4304192e..eee29e869 100644 --- a/server/utils/next_url.test.ts +++ b/server/utils/next_url.test.ts @@ -69,48 +69,54 @@ describe('test composeNextUrlQueryParam', () => { describe('test validateNextUrl', () => { test('accept relative url', () => { const url = '/relative/path'; - expect(validateNextUrl(url)).toEqual(undefined); + expect(validateNextUrl(url, '')).toEqual(undefined); }); test('accept relative url with # and query', () => { const url = '/relative/path#hash?a=b'; - expect(validateNextUrl(url)).toEqual(undefined); + expect(validateNextUrl(url, undefined)).toEqual(undefined); }); test('reject url not start with /', () => { const url = 'exmaple.com/relative/path'; - expect(validateNextUrl(url)).toEqual(INVALID_NEXT_URL_PARAMETER_MESSAGE); + expect(validateNextUrl(url, '')).toEqual(INVALID_NEXT_URL_PARAMETER_MESSAGE); }); test('reject absolute url', () => { const url = 'https://exmaple.com/relative/path'; - expect(validateNextUrl(url)).toEqual(INVALID_NEXT_URL_PARAMETER_MESSAGE); + expect(validateNextUrl(url, '')).toEqual(INVALID_NEXT_URL_PARAMETER_MESSAGE); }); test('reject url starts with //', () => { const url = '//exmaple.com/relative/path'; - expect(validateNextUrl(url)).toEqual(INVALID_NEXT_URL_PARAMETER_MESSAGE); + expect(validateNextUrl(url, '')).toEqual(INVALID_NEXT_URL_PARAMETER_MESSAGE); }); test('accpet url has @ in query parameters', () => { const url = '/test/path?key=a@b&k2=v'; - expect(validateNextUrl(url)).toEqual(undefined); + expect(validateNextUrl(url, '')).toEqual(undefined); }); test('allow slash', () => { const url = '/'; - expect(validateNextUrl(url)).toEqual(undefined); + expect(validateNextUrl(url, '')).toEqual(undefined); }); test('allow dashboard url', () => { const url = '/_plugin/opensearch-dashboards/app/opensearch-dashboards#dashbard/dashboard-id?_g=(param=a&p=b)'; - expect(validateNextUrl(url)).toEqual(undefined); + expect(validateNextUrl(url, '')).toEqual(undefined); + }); + + test('allow basePath with numbers', () => { + const url = '/123/app/dashboards'; + expect(validateNextUrl(url, '/123')).toEqual(undefined); }); // test cases from https://pentester.land/cheatsheets/2018/11/02/open-redirect-cheatsheet.html test('test list', () => { const urlList = [ + '/\t/example.com/', '<>//β“π¨π—°οΏ½π•β…†π“Έβ“œβ‚β„Ήβ“ƒο½‘οΌ°β“¦', '//;@β“π¨π—°οΏ½π•β…†π“Έβ“œβ‚β„Ήβ“ƒο½‘οΌ°β“¦', '/////β“π¨π—°οΏ½π•β…†π“Έβ“œβ‚β„Ήβ“ƒο½‘οΌ°β“¦/', @@ -624,10 +630,17 @@ describe('test validateNextUrl', () => { '//XY>.7d8T\\205pZM@whitelisted.com+@β“π¨π—°οΏ½π•β…†π“Έβ“œβ‚β„Ήβ“ƒο½‘οΌ°β“¦/', '//XY>.7d8T\\205pZM@whitelisted.com@google.com/', '//XY>.7d8T\\205pZM@whitelisted.com+@google.com/', + 'javascript://sub.domain.com/%0Aalert(1)', + 'javascript://%250Aalert(1)', + 'javascript://%250Aalert(1)//?1', + 'javascript://%250A1?alert(1):0', + '%09Jav%09ascript:alert(document.domain)', + 'javascript://%250Alert(document.location=document.cookie)', + '\\j\\av\\a\\s\\cr\\i\\pt\\:\\a\\l\\ert\\(1\\)', ]; - for (const url in urlList) { + for (const url of urlList) { if (url) { - expect(validateNextUrl(url)).toEqual(INVALID_NEXT_URL_PARAMETER_MESSAGE); + expect(validateNextUrl(url, '')).toEqual(INVALID_NEXT_URL_PARAMETER_MESSAGE); } } }); diff --git a/server/utils/next_url.ts b/server/utils/next_url.ts index 4014be0da..b3a5bb85f 100644 --- a/server/utils/next_url.ts +++ b/server/utils/next_url.ts @@ -45,25 +45,31 @@ export const INVALID_NEXT_URL_PARAMETER_MESSAGE = 'Invalid nextUrl parameter.'; /** * We require the nextUrl parameter to be an relative url. * - * Here we leverage the normalizeUrl function. If the library can parse the url - * parameter, which means it is an absolute url, then we reject it. Otherwise, the - * library cannot parse the url, which means it is not an absolute url, we let to - * go through. +* Here we validate the nextUrl parameter by checking if it meets the following criteria: + * - nextUrl starts with the basePath (/ if no serverBasePath is set) + * - If nextUrl is longer than 2 chars then the second character must be alphabetical or underscore + * - The following characters must be alphanumeric, dash or underscore * Note: url has been decoded by OpenSearchDashboards. * * @param url url string. * @returns error message if nextUrl is invalid, otherwise void. */ -export const validateNextUrl = (url: string | undefined): string | void => { +export function validateNextUrl( + url: string | undefined, + basePath: string | undefined +): string | void { if (url) { - const path = url.split('?')[0]; + const path = url.split(/\?|#/)[0]; + const bp = basePath || ''; + if (!path.startsWith(bp)) { + return INVALID_NEXT_URL_PARAMETER_MESSAGE; + } + const pathMinusBase = path.replace(bp, ''); if ( - !path.startsWith('/') || - path.startsWith('//') || - path.includes('\\') || - path.includes('@') + !pathMinusBase.startsWith('/') || + (pathMinusBase.length >= 2 && !/^\/[a-zA-Z_][\/a-zA-Z0-9-_]+$/.test(pathMinusBase)) ) { return INVALID_NEXT_URL_PARAMETER_MESSAGE; } } -}; +} \ No newline at end of file