diff --git a/server/auth/types/basic/routes.ts b/server/auth/types/basic/routes.ts index affe36120..c0ad0b095 100644 --- a/server/auth/types/basic/routes.ts +++ b/server/auth/types/basic/routes.ts @@ -25,6 +25,7 @@ import { SecurityClient } from '../../../backend/opensearch_security_client'; import { API_AUTH_LOGIN, API_AUTH_LOGOUT, LOGIN_PAGE_URI } from '../../../../common'; import { resolveTenant } from '../../../multitenancy/tenant_resolver'; import { ParsedUrlQueryParams } from '../../../utils/next_url'; +import { validateNextUrl } from '../../../utils/next_url'; export class BasicAuthRoutes { constructor( @@ -41,7 +42,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 47339d3ff..542ba7d44 100644 --- a/server/auth/types/openid/routes.ts +++ b/server/auth/types/openid/routes.ts @@ -82,7 +82,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); + }, }) ), state: schema.maybe(schema.string()), 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 0333f9bda..2c14c4da8 100644 --- a/server/auth/types/saml/routes.ts +++ b/server/auth/types/saml/routes.ts @@ -54,7 +54,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(), @@ -265,7 +267,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 b7e40219f..41eb73d16 100644 --- a/server/utils/next_url.test.ts +++ b/server/utils/next_url.test.ts @@ -19,48 +19,54 @@ import { validateNextUrl, INVALID_NEXT_URL_PARAMETER_MESSAGE } from './next_url' 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/', '<>//β“π¨π—°οΏ½π•β…†π“Έβ“œβ‚β„Ήβ“ƒο½‘οΌ°β“¦', '//;@β“π¨π—°οΏ½π•β…†π“Έβ“œβ‚β„Ήβ“ƒο½‘οΌ°β“¦', '/////β“π¨π—°οΏ½π•β…†π“Έβ“œβ‚β„Ήβ“ƒο½‘οΌ°β“¦/', @@ -574,10 +580,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 566ffb95b..708aca89a 100644 --- a/server/utils/next_url.ts +++ b/server/utils/next_url.ts @@ -37,25 +37,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; } } -}; +}