From fc4f6a27c0c80881be9e8ed6b9259a25c3fa0e13 Mon Sep 17 00:00:00 2001 From: "opensearch-trigger-bot[bot]" <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Date: Tue, 23 Jul 2024 15:27:51 -0400 Subject: [PATCH] Update nextUrl validation to incorporate serverBasePath (#2048) (#2050) * Fix the nextUrl validation test and test url path using regex Signed-off-by: Craig Perkins * Update nextUrl validation regex Signed-off-by: Craig Perkins * Add missing tests Signed-off-by: Craig Perkins * Combine url split to single line Signed-off-by: Craig Perkins * Update docstring Signed-off-by: Craig Perkins * Simplify condition Signed-off-by: Craig Perkins * Add origin on redirect Signed-off-by: Craig Perkins * Remove absolute url on redirect Signed-off-by: Craig Perkins * Update login-page tests Signed-off-by: Craig Perkins * Remove url.origin Signed-off-by: Craig Perkins * Account for server base path that can be numeric Signed-off-by: Craig Perkins * Update docstring Signed-off-by: Craig Perkins --------- Signed-off-by: Craig Perkins (cherry picked from commit a44e26539345c17e066bc3ceb9ff1e5c07d50428) Co-authored-by: Craig Perkins --- server/auth/types/basic/routes.ts | 13 +++++++++++- 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, 67 insertions(+), 27 deletions(-) diff --git a/server/auth/types/basic/routes.ts b/server/auth/types/basic/routes.ts index bae3e338c..e6a3e3ce4 100755 --- a/server/auth/types/basic/routes.ts +++ b/server/auth/types/basic/routes.ts @@ -31,6 +31,7 @@ import { import { resolveTenant } from '../../../multitenancy/tenant_resolver'; import { encodeUriQuery } from '../../../../../../src/plugins/opensearch_dashboards_utils/common/url/encode_uri_query'; import { AuthType } from '../../../../common'; +import { validateNextUrl } from '../../../utils/next_url'; export class BasicAuthRoutes { constructor( @@ -47,7 +48,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..83cb0e7d5 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..6b439f872 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; } } -}; +}