Update nextUrl validation to incorporate serverBasePath (#2048) (#2049)

* Fix the nextUrl validation test and test url path using regex Signed-off-by: Craig Perkins <cwperx@amazon.com> * Update nextUrl validation regex Signed-off-by: Craig Perkins <cwperx@amazon.com> * Add missing tests Signed-off-by: Craig Perkins <cwperx@amazon.com> * Combine url split to single line Signed-off-by: Craig Perkins <cwperx@amazon.com> * Update docstring Signed-off-by: Craig Perkins <cwperx@amazon.com> * Simplify condition Signed-off-by: Craig Perkins <cwperx@amazon.com> * Add origin on redirect Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove absolute url on redirect Signed-off-by: Craig Perkins <cwperx@amazon.com> * Update login-page tests Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove url.origin Signed-off-by: Craig Perkins <cwperx@amazon.com> * Account for server base path that can be numeric Signed-off-by: Craig Perkins <cwperx@amazon.com> * Update docstring Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Craig Perkins <cwperx@amazon.com> (cherry picked from commit a44e265) Co-authored-by: Craig Perkins <cwperx@amazon.com>
opensearch-project · Jul 23, 2024 · 53d418f · 53d418f
1 parent 6e89c3c
commit 53d418f
Show file tree

Hide file tree

Showing 6 changed files with 67 additions and 27 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;
     }
   }
-};
+}