Update nextUrl validation to incorporate serverBasePath (opensearch-p…

…roject#2048) * Fix the nextUrl validation test and test url path using regex Signed-off-by: Craig Perkins <[email protected]> * Update nextUrl validation regex Signed-off-by: Craig Perkins <[email protected]> * Add missing tests Signed-off-by: Craig Perkins <[email protected]> * Combine url split to single line Signed-off-by: Craig Perkins <[email protected]> * Update docstring Signed-off-by: Craig Perkins <[email protected]> * Simplify condition Signed-off-by: Craig Perkins <[email protected]> * Add origin on redirect Signed-off-by: Craig Perkins <[email protected]> * Remove absolute url on redirect Signed-off-by: Craig Perkins <[email protected]> * Update login-page tests Signed-off-by: Craig Perkins <[email protected]> * Remove url.origin Signed-off-by: Craig Perkins <[email protected]> * Account for server base path that can be numeric Signed-off-by: Craig Perkins <[email protected]> * Update docstring Signed-off-by: Craig Perkins <[email protected]> --------- Signed-off-by: Craig Perkins <[email protected]> (cherry picked from commit a44e265)
cwperks · Jul 25, 2024 · eaf85b6 · eaf85b6
1 parent 7d471b1
commit eaf85b6
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 26 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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\\[email protected]+@Ⓛ𝐨𝗰�𝕝ⅆ𝓸ⓜₐℹⓃ｡Ｐⓦ/',
       '//XY>.7d8T\\[email protected]@google.com/',
       '//XY>.7d8T\\[email protected][email protected]/',
+      '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
@@ -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;
     }
   }
-};
+}