Unify some of the appid code, improve script tag detection and add va…

…lidation (#2519) * Some unification * Put a few things back * Put back * Remove unused import * changefile * More test fixes * More test fixes * Tests passing * Update domains * Update changelog
OfficeDev · Sep 25, 2024 · 60eb045 · 60eb045
1 parent 0f935e3
commit 60eb045
Show file tree

Hide file tree

Showing 12 changed files with 234 additions and 208 deletions.
diff --git a/change/@microsoft-teams-js-81b620f2-8af1-4450-89b0-20600bac843e.json b/change/@microsoft-teams-js-81b620f2-8af1-4450-89b0-20600bac843e.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Updated internal app id validation",
+  "packageName": "@microsoft/teams-js",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/packages/teams-js/src/internal/appIdValidation.ts b/packages/teams-js/src/internal/appIdValidation.ts
@@ -1,3 +1,5 @@
+import { hasScriptTags } from './utils';
+
 /**
  * This function can be used to validate if a string is a "valid" app id.
  * Valid is a relative term, in this case. Truly valid app ids are UUIDs as documented in the schema:
@@ -10,7 +12,7 @@
  * @throws Error with a message describing the exact validation violation
  */
 export function validateStringAsAppId(potentialAppId: string): void {
-  if (doesStringContainScriptTags(potentialAppId)) {
+  if (hasScriptTags(potentialAppId)) {
     throw new Error(`Potential app id (${potentialAppId}) is invalid; it contains script tags.`);
   }
 
@@ -25,11 +27,6 @@ export function validateStringAsAppId(potentialAppId: string): void {
   }
 }
 
-export function doesStringContainScriptTags(str: string): boolean {
-  const scriptRegex = /<script[^>]*>[\s\S]*?<\/script[^>]*>/gi;
-  return scriptRegex.test(str);
-}
-
 export const minimumValidAppIdLength = 4;
 export const maximumValidAppIdLength = 256;
 

diff --git a/packages/teams-js/src/internal/utils.ts b/packages/teams-js/src/internal/utils.ts
@@ -460,43 +460,19 @@ export function fullyQualifyUrlString(fullOrRelativePath: string): URL {
 }
 
 /**
- * The hasScriptTags function first decodes any HTML entities in the input string using the decodeHTMLEntities function.
- * It then tries to decode the result as a URI component. If the URI decoding fails (which would throw an error), it assumes that the input was not encoded and uses the original input.
- * Next, it defines a regular expression scriptRegex that matches any string that starts with <script (followed by any characters), then has any characters (including newlines),
- * and ends with </script> (preceded by any characters).
- * Finally, it uses the test method to check if the decoded input matches this regular expression. The function returns true if a match is found and false otherwise.
- * @param input URL converted to string to pattern match
+ * Detects if there are any script tags in a given string, even if they are Uri encoded or encoded as HTML entities.
+ * @param input string to test for script tags
  * @returns true if the input string contains a script tag, false otherwise
  */
-function hasScriptTags(input: string): boolean {
-  let decodedInput;
-  try {
-    const decodedHTMLInput = decodeHTMLEntities(input);
-    decodedInput = decodeURIComponent(decodedHTMLInput);
-  } catch (e) {
-    // input was not encoded, use it as is
-    decodedInput = input;
-  }
-  const scriptRegex = /<script[^>]*>[\s\S]*?<\/script[^>]*>/gi;
-  return scriptRegex.test(decodedInput);
-}
+export function hasScriptTags(input: string): boolean {
+  const openingScriptTagRegex = /<script[^>]*>|&lt;script[^&]*&gt;|%3Cscript[^%]*%3E/gi;
+  const closingScriptTagRegex = /<\/script[^>]*>|&lt;\/script[^&]*&gt;|%3C\/script[^%]*%3E/gi;
 
-/**
- * The decodeHTMLEntities function replaces HTML entities in the input string with their corresponding characters.
- */
-function decodeHTMLEntities(input: string): string {
-  const entityMap = new Map<string, string>([
-    ['&lt;', '<'],
-    ['&gt;', '>'],
-    ['&amp;', '&'],
-    ['&quot;', '"'],
-    ['&#39;', "'"],
-    ['&#x2F;', '/'],
-  ]);
-  entityMap.forEach((value, key) => {
-    input = input.replace(new RegExp(key, 'gi'), value);
-  });
-  return input;
+  const openingOrClosingScriptTagRegex = new RegExp(
+    `${openingScriptTagRegex.source}|${closingScriptTagRegex.source}`,
+    'gi',
+  );
+  return openingOrClosingScriptTagRegex.test(input);
 }
 
 function isIdLengthValid(id: string): boolean {

diff --git a/packages/teams-js/src/private/externalAppAuthentication.ts b/packages/teams-js/src/private/externalAppAuthentication.ts
@@ -2,6 +2,7 @@ import { sendMessageToParentAsync } from '../internal/communication';
 import { ensureInitialized } from '../internal/internalAPIs';
 import { ApiName, ApiVersionNumber, getApiVersionTag } from '../internal/telemetry';
 import { validateId, validateUrl } from '../internal/utils';
+import { AppId } from '../public';
 import { errorNotSupportedOnPlatform, FrameContexts } from '../public/constants';
 import { runtime } from '../public/runtime';
 
@@ -351,7 +352,7 @@ export namespace externalAppAuthentication {
     if (!isSupported()) {
       throw errorNotSupportedOnPlatform;
     }
-    validateId(appId, new Error('App id is not valid.'));
+    const typeSafeAppId: AppId = new AppId(appId);
     validateOriginalRequestInfo(originalRequestInfo);
 
     // Ask the parent window to open an authentication window with the parameters provided by the caller.
@@ -362,7 +363,7 @@ export namespace externalAppAuthentication {
       ),
       'externalAppAuthentication.authenticateAndResendRequest',
       [
-        appId,
+        typeSafeAppId.toString(),
         originalRequestInfo,
         authenticateParameters.url.href,
         authenticateParameters.width,
@@ -395,14 +396,15 @@ export namespace externalAppAuthentication {
     if (!isSupported()) {
       throw errorNotSupportedOnPlatform;
     }
-    validateId(appId, new Error('App id is not valid.'));
+    const typeSafeAppId: AppId = new AppId(appId);
+
     return sendMessageToParentAsync(
       getApiVersionTag(
         externalAppAuthenticationTelemetryVersionNumber,
         ApiName.ExternalAppAuthentication_AuthenticateWithSSO,
       ),
       'externalAppAuthentication.authenticateWithSSO',
-      [appId, authTokenRequest.claims, authTokenRequest.silent],
+      [typeSafeAppId.toString(), authTokenRequest.claims, authTokenRequest.silent],
     ).then(([wasSuccessful, error]: [boolean, InvokeError]) => {
       if (!wasSuccessful) {
         throw error;
@@ -431,7 +433,7 @@ export namespace externalAppAuthentication {
     if (!isSupported()) {
       throw errorNotSupportedOnPlatform;
     }
-    validateId(appId, new Error('App id is not valid.'));
+    const typeSafeAppId: AppId = new AppId(appId);
 
     validateOriginalRequestInfo(originalRequestInfo);
 
@@ -441,7 +443,7 @@ export namespace externalAppAuthentication {
         ApiName.ExternalAppAuthentication_AuthenticateWithSSOAndResendRequest,
       ),
       'externalAppAuthentication.authenticateWithSSOAndResendRequest',
-      [appId, originalRequestInfo, authTokenRequest.claims, authTokenRequest.silent],
+      [typeSafeAppId.toString(), originalRequestInfo, authTokenRequest.claims, authTokenRequest.silent],
     ).then(([wasSuccessful, response]: [boolean, IInvokeResponse | InvokeErrorWrapper]) => {
       if (wasSuccessful && response.responseType != null) {
         return response as IInvokeResponse;

diff --git a/packages/teams-js/src/private/externalAppCardActions.ts b/packages/teams-js/src/private/externalAppCardActions.ts
@@ -1,7 +1,7 @@
 import { sendMessageToParentAsync } from '../internal/communication';
 import { ensureInitialized } from '../internal/internalAPIs';
 import { ApiName, ApiVersionNumber, getApiVersionTag } from '../internal/telemetry';
-import { validateId } from '../internal/utils';
+import { AppId } from '../public';
 import { errorNotSupportedOnPlatform, FrameContexts } from '../public/constants';
 import { runtime } from '../public/runtime';
 import { ExternalAppErrorCode } from './constants';
@@ -96,15 +96,15 @@ export namespace externalAppCardActions {
     if (!isSupported()) {
       throw errorNotSupportedOnPlatform;
     }
-    validateId(appId, new Error('App id is not valid.'));
+    const typeSafeAppId: AppId = new AppId(appId);
 
     return sendMessageToParentAsync<[boolean, ActionSubmitError]>(
       getApiVersionTag(
         externalAppCardActionsTelemetryVersionNumber,
         ApiName.ExternalAppCardActions_ProcessActionSubmit,
       ),
       'externalAppCardActions.processActionSubmit',
-      [appId, actionSubmitPayload],
+      [typeSafeAppId.toString(), actionSubmitPayload],
     ).then(([wasSuccessful, error]: [boolean, ActionSubmitError]) => {
       if (!wasSuccessful) {
         throw error;
@@ -135,14 +135,14 @@ export namespace externalAppCardActions {
     if (!isSupported()) {
       throw errorNotSupportedOnPlatform;
     }
-    validateId(appId, new Error('App id is not valid.'));
+    const typeSafeAppId: AppId = new AppId(appId);
     return sendMessageToParentAsync<[ActionOpenUrlError, ActionOpenUrlType]>(
       getApiVersionTag(
         externalAppCardActionsTelemetryVersionNumber,
         ApiName.ExternalAppCardActions_ProcessActionOpenUrl,
       ),
       'externalAppCardActions.processActionOpenUrl',
-      [appId, url.href, fromElement],
+      [typeSafeAppId.toString(), url.href, fromElement],
     ).then(([error, response]: [ActionOpenUrlError, ActionOpenUrlType]) => {
       if (error) {
         throw error;

diff --git a/packages/teams-js/src/private/externalAppCommands.ts b/packages/teams-js/src/private/externalAppCommands.ts
@@ -1,7 +1,7 @@
 import { sendMessageToParentAsync } from '../internal/communication';
 import { ensureInitialized } from '../internal/internalAPIs';
 import { ApiName, ApiVersionNumber, getApiVersionTag } from '../internal/telemetry';
-import { validateId } from '../internal/utils';
+import { AppId } from '../public';
 import { errorNotSupportedOnPlatform, FrameContexts } from '../public/constants';
 import { runtime } from '../public/runtime';
 import { ExternalAppErrorCode } from './constants';
@@ -135,12 +135,12 @@ export namespace externalAppCommands {
     if (!isSupported()) {
       throw errorNotSupportedOnPlatform;
     }
-    validateId(appId, new Error('App id is not valid.'));
+    const typeSafeAppId: AppId = new AppId(appId);
 
     const [error, response] = await sendMessageToParentAsync<[ActionCommandError, IActionCommandResponse]>(
       getApiVersionTag(externalAppCommandsTelemetryVersionNumber, ApiName.ExternalAppCommands_ProcessActionCommands),
       ApiName.ExternalAppCommands_ProcessActionCommands,
-      [appId, commandId, extractedParameters],
+      [typeSafeAppId.toString(), commandId, extractedParameters],
     );
     if (error) {
       throw error;

diff --git a/packages/teams-js/test/internal/appIdValidation.spec.ts b/packages/teams-js/test/internal/appIdValidation.spec.ts
@@ -1,6 +1,5 @@
 import {
   doesStringContainNonPrintableCharacters,
-  doesStringContainScriptTags,
   isStringWithinAppIdLengthLimits,
   maximumValidAppIdLength,
   minimumValidAppIdLength,
@@ -58,37 +57,6 @@ describe('isStringWithinAppIdLengthLimits', () => {
   });
 });
 
-describe('doesStringContainScriptTags', () => {
-  test('should return true for strings containing script tags', () => {
-    expect(doesStringContainScriptTags('<script>alert("Hello")</script>')).toBe(true);
-    expect(doesStringContainScriptTags('<script src="example.js"></script>')).toBe(true);
-    expect(doesStringContainScriptTags('<script type="text/javascript">console.log("test")</script>')).toBe(true);
-  });
-
-  test('should return false for strings without script tags', () => {
-    expect(doesStringContainScriptTags('This is a test string')).toBe(false);
-    expect(doesStringContainScriptTags('8e6523aa-97f9-49ad-8614-75cae22f6597')).toBe(false);
-    expect(doesStringContainScriptTags('com.microsoft.teamspace.tab.youtube')).toBe(false);
-    expect(doesStringContainScriptTags('<div>This is a div</div>')).toBe(false);
-    expect(doesStringContainScriptTags('<a href="example.com">Link</a>')).toBe(false);
-  });
-
-  test('should return true for strings with script tags containing newlines and spaces', () => {
-    expect(doesStringContainScriptTags('<script>\nalert("Hello")\n</script>')).toBe(true);
-    expect(doesStringContainScriptTags('<script> \n console.log("test") \n </script>')).toBe(true);
-  });
-
-  test('should return false for empty string', () => {
-    expect(doesStringContainScriptTags('')).toBe(false);
-  });
-
-  test('should return true for strings with multiple script tags', () => {
-    expect(doesStringContainScriptTags('<script>alert("Hello")</script><script>console.log("test")</script>')).toBe(
-      true,
-    );
-  });
-});
-
 // Since there are plenty of tests validating the individual validation functions, these tests are intentionally not as
 // comprehensive as those. It executes a few basic tests and also validates that the error messages thrown are as expected.
 describe('validateStringAsAppId', () => {
@@ -98,15 +66,15 @@ describe('validateStringAsAppId', () => {
   });
 
   test('should throw error with "script" in message for app id containing script tag', () => {
-    expect(() => validateStringAsAppId('<script>alert("Hello")</script>')).toThrowError(/script/);
+    expect(() => validateStringAsAppId('<script>alert("Hello")</script>')).toThrowError(/script/i);
   });
 
   test('should throw error with "length" in message for app id too long or too short', () => {
-    expect(() => validateStringAsAppId('a')).toThrowError(/length/);
-    expect(() => validateStringAsAppId('a'.repeat(maximumValidAppIdLength))).toThrowError(/length/);
+    expect(() => validateStringAsAppId('a')).toThrowError(/length/i);
+    expect(() => validateStringAsAppId('a'.repeat(maximumValidAppIdLength))).toThrowError(/length/i);
   });
 
   test('should throw error with "printable" in message for app id containing non-printable characters', () => {
-    expect(() => validateStringAsAppId('hello\u0080world')).toThrowError(/printable/);
+    expect(() => validateStringAsAppId('hello\u0080world')).toThrowError(/printable/i);
   });
 });
diff --git a/packages/teams-js/test/internal/utils.spec.ts b/packages/teams-js/test/internal/utils.spec.ts
@@ -3,6 +3,7 @@ import {
   compareSDKVersions,
   createTeamsAppLink,
   getBase64StringFromBlob,
+  hasScriptTags,
   validateId,
   validateUrl,
   validateUuid,
@@ -367,6 +368,105 @@ describe('utils', () => {
     });
   });
 
+  describe('hasScriptTags', () => {
+    test('detects plain opening <script> tag', () => {
+      expect(hasScriptTags('<script>alert("XSS")</script>')).toBe(true);
+    });
+
+    test('detects HTML entity encoded opening <script> tag', () => {
+      expect(hasScriptTags('&lt;script&gt;alert("XSS")&lt;/script&gt;')).toBe(true);
+    });
+
+    test('detects URI encoded opening <script> tag', () => {
+      expect(hasScriptTags('%3Cscript%3Ealert("XSS")%3C/script%3E')).toBe(true);
+    });
+
+    test('detects plain closing </script> tag', () => {
+      expect(hasScriptTags('</script>')).toBe(true);
+    });
+
+    test('detects HTML entity encoded closing </script> tag', () => {
+      expect(hasScriptTags('&lt;/script&gt;')).toBe(true);
+    });
+
+    test('detects URI encoded closing </script> tag', () => {
+      expect(hasScriptTags('%3C/script%3E')).toBe(true);
+    });
+
+    test('returns false for strings without <script> tags', () => {
+      expect(hasScriptTags('<div>no script here</div>')).toBe(false);
+    });
+
+    test('detects mixed content with <script> tags', () => {
+      expect(hasScriptTags('<div><script>alert("XSS")</script></div>')).toBe(true);
+    });
+
+    test('returns false for empty string', () => {
+      expect(hasScriptTags('')).toBe(false);
+    });
+
+    test('detects multiple <script> tags', () => {
+      expect(hasScriptTags('<script>alert("XSS")</script><script>alert("XSS2")</script>')).toBe(true);
+    });
+
+    test('detects <script> tags with attributes', () => {
+      expect(hasScriptTags('<script type="text/javascript">alert("XSS")</script>')).toBe(true);
+      expect(hasScriptTags('<script src="example.js"></script>')).toBe(true);
+      expect(hasScriptTags('<script async defer>alert("XSS")</script>')).toBe(true);
+    });
+
+    test('detects HTML entity encoded <script> tag with attributes', () => {
+      expect(hasScriptTags('&lt;script type="text/javascript"&gt;alert("XSS")&lt;/script&gt;')).toBe(true);
+      expect(hasScriptTags('&lt;script src="example.js"&gt;&lt;/script&gt;')).toBe(true);
+    });
+
+    test('detects URI encoded <script> tag with attributes', () => {
+      expect(hasScriptTags('%3Cscript%20type=%22text/javascript%22%3Ealert("XSS")%3C/script%3E')).toBe(true);
+      expect(hasScriptTags('%3Cscript%20src=%22example.js%22%3E%3C/script%3E')).toBe(true);
+    });
+
+    test('detects <script> tags with spaces', () => {
+      expect(hasScriptTags('<script >alert("XSS")</script >')).toBe(true);
+    });
+
+    test('detects plain opening <script> tag with URI encoded closing tag', () => {
+      expect(hasScriptTags('<script>alert("XSS")%3C/script%3E')).toBe(true);
+    });
+
+    test('detects URI encoded opening <script> tag with plain closing tag', () => {
+      expect(hasScriptTags('%3Cscript%3Ealert("XSS")</script>')).toBe(true);
+    });
+
+    test('detects plain opening <script> tag with HTML entity encoded closing tag', () => {
+      expect(hasScriptTags('<script>alert("XSS")&lt;/script&gt;')).toBe(true);
+    });
+
+    test('detects HTML entity encoded opening <script> tag with plain closing tag', () => {
+      expect(hasScriptTags('&lt;script&gt;alert("XSS")</script>')).toBe(true);
+    });
+
+    test('detects nested <script> tags', () => {
+      expect(hasScriptTags('<script><script>alert("nested")</script></script>')).toBe(true);
+    });
+
+    test('detects <script> tags with unusual but valid attributes', () => {
+      expect(hasScriptTags('<script data-custom="value">alert("XSS")</script>')).toBe(true);
+      expect(hasScriptTags('<script nonce="random">alert("XSS")</script>')).toBe(true);
+    });
+
+    test('detects <script> tags with different casing', () => {
+      expect(hasScriptTags('<SCRIPT>alert("XSS")</SCRIPT>')).toBe(true);
+      expect(hasScriptTags('&lt;SCRIPT&gt;alert("XSS")&lt;/SCRIPT&gt;')).toBe(true);
+      expect(hasScriptTags('%3CSCRIPT%3Ealert("XSS")%3C/SCRIPT%3E')).toBe(true);
+    });
+
+    test('detects mixed casing <script> tags', () => {
+      expect(hasScriptTags('<sCRipT>alert("XSS")</sCRipT>')).toBe(true);
+      expect(hasScriptTags('&lt;sCRipT&gt;alert("XSS")&lt;/sCRipT&gt;')).toBe(true);
+      expect(hasScriptTags('%3CsCRipT%3Ealert("XSS")%3C/sCRipT%3E')).toBe(true);
+    });
+  });
+
   describe('UUID class tests', () => {
     describe('validateUuid', () => {
       it('should throw error when id is undefined', async () => {