From a4d827ece015ad273b9687cba54f6cc155968d00 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Wed, 5 Jun 2024 11:27:55 -0500 Subject: [PATCH 01/10] Use safevalues to fix trusted types issues reported by tsec --- .changeset/happy-trees-battle.md | 9 +++ packages/analytics/package.json | 5 +- packages/analytics/src/helpers.test.ts | 73 +------------------ packages/analytics/src/helpers.ts | 60 +++------------ packages/app-check/package.json | 1 + packages/app-check/src/recaptcha.ts | 15 +++- packages/auth/package.json | 3 +- packages/auth/tsconfig.json | 11 ++- packages/auth/tsec-exemptions.json | 3 + packages/database-compat/tsconfig.json | 11 ++- packages/database-compat/tsec-exemptions.json | 3 + packages/database/package.json | 1 + .../src/realtime/BrowserPollConnection.ts | 4 +- packages/database/tsconfig.json | 11 ++- packages/database/tsec-exemptions.json | 3 + packages/messaging/package.json | 2 +- .../src/helpers/registerDefaultSw.ts | 11 ++- yarn.lock | 5 ++ 18 files changed, 94 insertions(+), 137 deletions(-) create mode 100644 .changeset/happy-trees-battle.md create mode 100644 packages/auth/tsec-exemptions.json create mode 100644 packages/database-compat/tsec-exemptions.json create mode 100644 packages/database/tsec-exemptions.json diff --git a/.changeset/happy-trees-battle.md b/.changeset/happy-trees-battle.md new file mode 100644 index 00000000000..b5e727d5092 --- /dev/null +++ b/.changeset/happy-trees-battle.md @@ -0,0 +1,9 @@ +--- +'@firebase/analytics': patch +'@firebase/app-check': patch +'@firebase/messaging': patch +'@firebase/database': patch +'@firebase/auth': patch +--- + +Begin using [safevalues](https://github.com/google/safevalues) to sanitize HTML vulnerable to XSS. diff --git a/packages/analytics/package.json b/packages/analytics/package.json index 5e200228a69..fa1ae0275c3 100644 --- a/packages/analytics/package.json +++ b/packages/analytics/package.json @@ -45,15 +45,16 @@ "@firebase/logger": "0.4.2", "@firebase/util": "1.9.7", "@firebase/component": "0.6.8", - "tslib": "^2.1.0" + "tslib": "^2.1.0", + "safevalues": "0.5.2" }, "license": "Apache-2.0", "devDependencies": { "@firebase/app": "0.10.6", - "rollup": "2.79.1", "@rollup/plugin-commonjs": "21.1.0", "@rollup/plugin-json": "4.1.0", "@rollup/plugin-node-resolve": "13.3.0", + "rollup": "2.79.1", "rollup-plugin-typescript2": "0.31.2", "typescript": "4.7.4" }, diff --git a/packages/analytics/src/helpers.test.ts b/packages/analytics/src/helpers.test.ts index ff06ba3ea6c..d2dd9094aba 100644 --- a/packages/analytics/src/helpers.test.ts +++ b/packages/analytics/src/helpers.test.ts @@ -24,16 +24,12 @@ import { insertScriptTag, wrapOrCreateGtag, findGtagScriptOnPage, - promiseAllSettled, - createGtagTrustedTypesScriptURL, - createTrustedTypesPolicy + promiseAllSettled } from './helpers'; -import { GtagCommand, GTAG_URL } from './constants'; +import { GtagCommand } from './constants'; import { Deferred } from '@firebase/util'; import { ConsentSettings } from './public-types'; import { removeGtagScripts } from '../testing/gtag-script-util'; -import { logger } from './logger'; -import { AnalyticsError, ERROR_FACTORY } from './errors'; const fakeMeasurementId = 'abcd-efgh-ijkl'; const fakeAppId = 'my-test-app-1234'; @@ -50,71 +46,6 @@ const fakeDynamicConfig: DynamicConfig = { }; const fakeDynamicConfigPromises = [Promise.resolve(fakeDynamicConfig)]; -describe('Trusted Types policies and functions', () => { - if (window.trustedTypes) { - describe('Trusted types exists', () => { - let ttStub: SinonStub; - - beforeEach(() => { - ttStub = stub( - window.trustedTypes as TrustedTypePolicyFactory, - 'createPolicy' - ).returns({ - createScriptURL: (s: string) => s - } as any); - }); - - afterEach(() => { - removeGtagScripts(); - ttStub.restore(); - }); - - it('Verify trustedTypes is called if the API is available', () => { - const trustedTypesPolicy = createTrustedTypesPolicy( - 'firebase-js-sdk-policy', - { - createScriptURL: createGtagTrustedTypesScriptURL - } - ); - - expect(ttStub).to.be.called; - expect(trustedTypesPolicy).not.to.be.undefined; - }); - - it('createGtagTrustedTypesScriptURL verifies gtag URL base exists when a URL is provided', () => { - expect(createGtagTrustedTypesScriptURL(GTAG_URL)).to.equal(GTAG_URL); - }); - - it('createGtagTrustedTypesScriptURL rejects URLs with non-gtag base', () => { - const NON_GTAG_URL = 'http://iamnotgtag.com'; - const loggerWarnStub = stub(logger, 'warn'); - const errorMessage = ERROR_FACTORY.create( - AnalyticsError.INVALID_GTAG_RESOURCE, - { - gtagURL: NON_GTAG_URL - } - ).message; - - expect(createGtagTrustedTypesScriptURL(NON_GTAG_URL)).to.equal(''); - expect(loggerWarnStub).to.be.calledWith(errorMessage); - }); - }); - } - describe('Trusted types does not exist', () => { - it('Verify trustedTypes functions are not called if the API is not available', () => { - delete window.trustedTypes; - const trustedTypesPolicy = createTrustedTypesPolicy( - 'firebase-js-sdk-policy', - { - createScriptURL: createGtagTrustedTypesScriptURL - } - ); - - expect(trustedTypesPolicy).to.be.undefined; - }); - }); -}); - describe('Gtag wrapping functions', () => { afterEach(() => { removeGtagScripts(); diff --git a/packages/analytics/src/helpers.ts b/packages/analytics/src/helpers.ts index 2e9a46e03b2..a35ee0b01f4 100644 --- a/packages/analytics/src/helpers.ts +++ b/packages/analytics/src/helpers.ts @@ -24,25 +24,12 @@ import { import { DynamicConfig, DataLayer, Gtag, MinimalDynamicConfig } from './types'; import { GtagCommand, GTAG_URL } from './constants'; import { logger } from './logger'; -import { AnalyticsError, ERROR_FACTORY } from './errors'; +import { trustedResourceUrl } from 'safevalues'; +import { safeScriptEl } from 'safevalues/dom'; // Possible parameter types for gtag 'event' and 'config' commands type GtagConfigOrEventParams = ControlParams & EventParams & CustomParams; -/** - * Verifies and creates a TrustedScriptURL. - */ -export function createGtagTrustedTypesScriptURL(url: string): string { - if (!url.startsWith(GTAG_URL)) { - const err = ERROR_FACTORY.create(AnalyticsError.INVALID_GTAG_RESOURCE, { - gtagURL: url - }); - logger.warn(err.message); - return ''; - } - return url; -} - /** * Makeshift polyfill for Promise.allSettled(). Resolves when all promises * have either resolved or rejected. @@ -55,29 +42,6 @@ export function promiseAllSettled( return Promise.all(promises.map(promise => promise.catch(e => e))); } -/** - * Creates a TrustedTypePolicy object that implements the rules passed as policyOptions. - * - * @param policyName A string containing the name of the policy - * @param policyOptions Object containing implementations of instance methods for TrustedTypesPolicy, see {@link https://developer.mozilla.org/en-US/docs/Web/API/TrustedTypePolicy#instance_methods - * | the TrustedTypePolicy reference documentation}. - */ -export function createTrustedTypesPolicy( - policyName: string, - policyOptions: Partial -): Partial | undefined { - // Create a TrustedTypes policy that we can use for updating src - // properties - let trustedTypesPolicy: Partial | undefined; - if (window.trustedTypes) { - trustedTypesPolicy = window.trustedTypes.createPolicy( - policyName, - policyOptions - ); - } - return trustedTypesPolicy; -} - /** * Inserts gtag script tag into the page to asynchronously download gtag. * @param dataLayerName Name of datalayer (most often the default, "_dataLayer"). @@ -86,21 +50,17 @@ export function insertScriptTag( dataLayerName: string, measurementId: string ): void { - const trustedTypesPolicy = createTrustedTypesPolicy( - 'firebase-js-sdk-policy', - { - createScriptURL: createGtagTrustedTypesScriptURL - } - ); - const script = document.createElement('script'); + // We are not providing an analyticsId in the URL because it would trigger a `page_view` // without fid. We will initialize ga-id using gtag (config) command together with fid. - - const gtagScriptURL = `${GTAG_URL}?l=${dataLayerName}&id=${measurementId}`; - (script.src as string | TrustedScriptURL) = trustedTypesPolicy - ? (trustedTypesPolicy as TrustedTypePolicy)?.createScriptURL(gtagScriptURL) - : gtagScriptURL; + // + // We also have to ensure the template string before the first expression constitutes a valid URL + // start, as this is what the initial validation focuses on. If the template literal begins + // directly with an expression (e.g. `${GTAG_SCRIPT_URL}`), the validation fails due to an + // empty initial string. + const url = trustedResourceUrl`https://www.googletagmanager.com/gtag/js?l=${dataLayerName}&id=${measurementId}`; + safeScriptEl.setSrc(script, url); script.async = true; document.head.appendChild(script); diff --git a/packages/app-check/package.json b/packages/app-check/package.json index 07468c8738c..a26c2937c2f 100644 --- a/packages/app-check/package.json +++ b/packages/app-check/package.json @@ -42,6 +42,7 @@ "@firebase/util": "1.9.7", "@firebase/component": "0.6.8", "@firebase/logger": "0.4.2", + "safevalues": "^0.5.2", "tslib": "^2.1.0" }, "license": "Apache-2.0", diff --git a/packages/app-check/src/recaptcha.ts b/packages/app-check/src/recaptcha.ts index 8eb72e2add7..0f0650296af 100644 --- a/packages/app-check/src/recaptcha.ts +++ b/packages/app-check/src/recaptcha.ts @@ -19,7 +19,12 @@ import { FirebaseApp } from '@firebase/app'; import { getStateReference } from './state'; import { Deferred } from '@firebase/util'; import { getRecaptcha, ensureActivated } from './util'; +import { trustedResourceUrl } from 'safevalues'; +import { safeScriptEl } from 'safevalues/dom'; +// Note that these are used for testing. If they are changed, the URLs used in loadReCAPTCHAV3Script +// and loadReCAPTCHAEnterpriseScript must also be changed. They aren't used to create the URLs +// since trusted resource URLs must be created using template string literals. export const RECAPTCHA_URL = 'https://www.google.com/recaptcha/api.js'; export const RECAPTCHA_ENTERPRISE_URL = 'https://www.google.com/recaptcha/enterprise.js'; @@ -166,14 +171,20 @@ function renderInvisibleWidget( function loadReCAPTCHAV3Script(onload: () => void): void { const script = document.createElement('script'); - script.src = RECAPTCHA_URL; + safeScriptEl.setSrc( + script, + trustedResourceUrl`https://www.google.com/recaptcha/api.js` + ); script.onload = onload; document.head.appendChild(script); } function loadReCAPTCHAEnterpriseScript(onload: () => void): void { const script = document.createElement('script'); - script.src = RECAPTCHA_ENTERPRISE_URL; + safeScriptEl.setSrc( + script, + trustedResourceUrl`https://www.google.com/recaptcha/enterprise.js` + ); script.onload = onload; document.head.appendChild(script); } diff --git a/packages/auth/package.json b/packages/auth/package.json index 4d074795fef..730a86dc409 100644 --- a/packages/auth/package.json +++ b/packages/auth/package.json @@ -131,7 +131,8 @@ "@firebase/logger": "0.4.2", "@firebase/util": "1.9.7", "undici": "5.28.4", - "tslib": "^2.1.0" + "tslib": "^2.1.0", + "safevalues": "^0.5.2" }, "license": "Apache-2.0", "devDependencies": { diff --git a/packages/auth/tsconfig.json b/packages/auth/tsconfig.json index 03897eed09c..9264e1461f0 100644 --- a/packages/auth/tsconfig.json +++ b/packages/auth/tsconfig.json @@ -1,10 +1,17 @@ { "extends": "../../config/tsconfig.base.json", "compilerOptions": { - "outDir": "dist" + "outDir": "dist", + "plugins": [ + { + "name": "tsec", + "reportTsecDiagnosticsOnly": true, + "exemptionConfig": "./tsec-exemptions.json" + } + ] }, "exclude": [ "dist/**/*", "demo/**/*" ] -} \ No newline at end of file +} diff --git a/packages/auth/tsec-exemptions.json b/packages/auth/tsec-exemptions.json new file mode 100644 index 00000000000..e366beec368 --- /dev/null +++ b/packages/auth/tsec-exemptions.json @@ -0,0 +1,3 @@ +{ + "ban-element-setattribute": ["src/platform_browser/index.ts", "src/platform_browser/load_js.test.ts"] +} diff --git a/packages/database-compat/tsconfig.json b/packages/database-compat/tsconfig.json index ce12ac3c5dc..891c6ae01f5 100644 --- a/packages/database-compat/tsconfig.json +++ b/packages/database-compat/tsconfig.json @@ -3,9 +3,16 @@ "compilerOptions": { "outDir": "dist", "strict": false, - "downlevelIteration": true + "downlevelIteration": true, + "plugins": [ + { + "name": "tsec", + "reportTsecDiagnosticsOnly": true, + "exemptionConfig": "./tsec-exemptions.json" + } + ] }, "exclude": [ "dist/**/*" ] -} \ No newline at end of file +} diff --git a/packages/database-compat/tsec-exemptions.json b/packages/database-compat/tsec-exemptions.json new file mode 100644 index 00000000000..126a0ce25f9 --- /dev/null +++ b/packages/database-compat/tsec-exemptions.json @@ -0,0 +1,3 @@ +{ + "ban-script-src-assignments": ["../database/src/realtime/BrowserPollConnection.ts"] +} diff --git a/packages/database/package.json b/packages/database/package.json index bb47fe500e8..1ef91452f76 100644 --- a/packages/database/package.json +++ b/packages/database/package.json @@ -56,6 +56,7 @@ "@firebase/app-check-interop-types": "0.3.2", "@firebase/auth-interop-types": "0.2.3", "faye-websocket": "0.11.4", + "safevalues": "^0.5.2", "tslib": "^2.1.0" }, "devDependencies": { diff --git a/packages/database/src/realtime/BrowserPollConnection.ts b/packages/database/src/realtime/BrowserPollConnection.ts index 21396cecc8e..4dcb7dd641d 100644 --- a/packages/database/src/realtime/BrowserPollConnection.ts +++ b/packages/database/src/realtime/BrowserPollConnection.ts @@ -16,6 +16,8 @@ */ import { base64Encode, isNodeSdk, stringify } from '@firebase/util'; +import { sanitizeHtml } from 'safevalues'; +import { safeDocument } from 'safevalues/dom'; import { RepoInfo, repoInfoConnectionURL } from '../core/RepoInfo'; import { StatsCollection } from '../core/stats/StatsCollection'; @@ -475,7 +477,7 @@ export class FirebaseIFrameScriptHolder { const iframeContents = '' + script + ''; try { this.myIFrame.doc.open(); - this.myIFrame.doc.write(iframeContents); + safeDocument.write(this.myIFrame.doc, sanitizeHtml(iframeContents)); this.myIFrame.doc.close(); } catch (e) { log('frame writing exception'); diff --git a/packages/database/tsconfig.json b/packages/database/tsconfig.json index ce12ac3c5dc..891c6ae01f5 100644 --- a/packages/database/tsconfig.json +++ b/packages/database/tsconfig.json @@ -3,9 +3,16 @@ "compilerOptions": { "outDir": "dist", "strict": false, - "downlevelIteration": true + "downlevelIteration": true, + "plugins": [ + { + "name": "tsec", + "reportTsecDiagnosticsOnly": true, + "exemptionConfig": "./tsec-exemptions.json" + } + ] }, "exclude": [ "dist/**/*" ] -} \ No newline at end of file +} diff --git a/packages/database/tsec-exemptions.json b/packages/database/tsec-exemptions.json new file mode 100644 index 00000000000..39e700cb0cd --- /dev/null +++ b/packages/database/tsec-exemptions.json @@ -0,0 +1,3 @@ +{ + "ban-script-src-assignments": ["src/realtime/BrowserPollConnection.ts"] +} diff --git a/packages/messaging/package.json b/packages/messaging/package.json index a1d61f92129..f9d3c8c8ac4 100644 --- a/packages/messaging/package.json +++ b/packages/messaging/package.json @@ -59,13 +59,13 @@ "@firebase/util": "1.9.7", "@firebase/component": "0.6.8", "idb": "7.1.1", + "safevalues": "^0.5.2", "tslib": "^2.1.0" }, "devDependencies": { "@firebase/app": "0.10.6", "rollup": "2.79.1", "rollup-plugin-typescript2": "0.31.2", - "@rollup/plugin-json": "4.1.0", "ts-essentials": "9.3.0", "typescript": "4.7.4" }, diff --git a/packages/messaging/src/helpers/registerDefaultSw.ts b/packages/messaging/src/helpers/registerDefaultSw.ts index 239e6ed8244..dd27943eb1c 100644 --- a/packages/messaging/src/helpers/registerDefaultSw.ts +++ b/packages/messaging/src/helpers/registerDefaultSw.ts @@ -15,7 +15,10 @@ * limitations under the License. */ -import { DEFAULT_SW_PATH, DEFAULT_SW_SCOPE } from '../util/constants'; +import { trustedResourceUrl } from 'safevalues'; +import { safeServiceWorkerContainer } from 'safevalues/dom'; + +import { DEFAULT_SW_SCOPE } from '../util/constants'; import { ERROR_FACTORY, ErrorCode } from '../util/errors'; import { MessagingService } from '../messaging-service'; @@ -24,8 +27,10 @@ export async function registerDefaultSw( messaging: MessagingService ): Promise { try { - messaging.swRegistration = await navigator.serviceWorker.register( - DEFAULT_SW_PATH, + const container = navigator.serviceWorker; + messaging.swRegistration = await safeServiceWorkerContainer.register( + container, + trustedResourceUrl`/firebase-messaging-sw.js`, { scope: DEFAULT_SW_SCOPE } diff --git a/yarn.lock b/yarn.lock index 9d943a928b0..1f53c4f3833 100644 --- a/yarn.lock +++ b/yarn.lock @@ -15405,6 +15405,11 @@ safe-stable-stringify@^1.1.0: resolved "https://registry.npmjs.org/safer-buffer/-/safer-buffer-2.1.2.tgz" integrity sha512-YZo3K82SD7Riyi0E1EQPojLz7kpepnSQI9IyPbHHg1XXXevb5dJI7tpyN2ADxGcQbHG7vcyRHk0cbwqcQriUtg== +safevalues@^0.5.2: + version "0.5.2" + resolved "https://registry.npmjs.org/safevalues/-/safevalues-0.5.2.tgz#f0ec859de1cdf95d237d059429916b63a1dc3213" + integrity sha512-8MeVqP6q2UompeFEUVphKZyLzeenFrSATIAAeSE5+6aGiozwVe3mK61TCODNrBFHocBJdpGL/lCmdYWiocdMng== + sauce-connect-launcher@^1.2.7: version "1.3.2" resolved "https://registry.npmjs.org/sauce-connect-launcher/-/sauce-connect-launcher-1.3.2.tgz" From b482ecac65d5d220cfb98d9c49005fabfdf04c58 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Thu, 4 Jul 2024 10:40:29 -0400 Subject: [PATCH 02/10] Upgrade to safevalues 0.6.0 --- packages/analytics/package.json | 2 +- packages/app-check/package.json | 2 +- packages/auth/package.json | 2 +- packages/database/package.json | 2 +- packages/messaging/package.json | 2 +- yarn.lock | 8 ++++---- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/analytics/package.json b/packages/analytics/package.json index fa1ae0275c3..a967e04ce73 100644 --- a/packages/analytics/package.json +++ b/packages/analytics/package.json @@ -46,7 +46,7 @@ "@firebase/util": "1.9.7", "@firebase/component": "0.6.8", "tslib": "^2.1.0", - "safevalues": "0.5.2" + "safevalues": "0.6.0" }, "license": "Apache-2.0", "devDependencies": { diff --git a/packages/app-check/package.json b/packages/app-check/package.json index a26c2937c2f..0a200953c0a 100644 --- a/packages/app-check/package.json +++ b/packages/app-check/package.json @@ -42,7 +42,7 @@ "@firebase/util": "1.9.7", "@firebase/component": "0.6.8", "@firebase/logger": "0.4.2", - "safevalues": "^0.5.2", + "safevalues": "0.6.0", "tslib": "^2.1.0" }, "license": "Apache-2.0", diff --git a/packages/auth/package.json b/packages/auth/package.json index 730a86dc409..c2007896e91 100644 --- a/packages/auth/package.json +++ b/packages/auth/package.json @@ -132,7 +132,7 @@ "@firebase/util": "1.9.7", "undici": "5.28.4", "tslib": "^2.1.0", - "safevalues": "^0.5.2" + "safevalues": "0.6.0" }, "license": "Apache-2.0", "devDependencies": { diff --git a/packages/database/package.json b/packages/database/package.json index 1ef91452f76..6486c0d9fdf 100644 --- a/packages/database/package.json +++ b/packages/database/package.json @@ -56,7 +56,7 @@ "@firebase/app-check-interop-types": "0.3.2", "@firebase/auth-interop-types": "0.2.3", "faye-websocket": "0.11.4", - "safevalues": "^0.5.2", + "safevalues": "0.6.0", "tslib": "^2.1.0" }, "devDependencies": { diff --git a/packages/messaging/package.json b/packages/messaging/package.json index f9d3c8c8ac4..0a4bf97d9a3 100644 --- a/packages/messaging/package.json +++ b/packages/messaging/package.json @@ -59,7 +59,7 @@ "@firebase/util": "1.9.7", "@firebase/component": "0.6.8", "idb": "7.1.1", - "safevalues": "^0.5.2", + "safevalues": "0.6.0", "tslib": "^2.1.0" }, "devDependencies": { diff --git a/yarn.lock b/yarn.lock index 1f53c4f3833..4f1fccdbdc3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -15405,10 +15405,10 @@ safe-stable-stringify@^1.1.0: resolved "https://registry.npmjs.org/safer-buffer/-/safer-buffer-2.1.2.tgz" integrity sha512-YZo3K82SD7Riyi0E1EQPojLz7kpepnSQI9IyPbHHg1XXXevb5dJI7tpyN2ADxGcQbHG7vcyRHk0cbwqcQriUtg== -safevalues@^0.5.2: - version "0.5.2" - resolved "https://registry.npmjs.org/safevalues/-/safevalues-0.5.2.tgz#f0ec859de1cdf95d237d059429916b63a1dc3213" - integrity sha512-8MeVqP6q2UompeFEUVphKZyLzeenFrSATIAAeSE5+6aGiozwVe3mK61TCODNrBFHocBJdpGL/lCmdYWiocdMng== +safevalues@0.6.0: + version "0.6.0" + resolved "https://registry.npmjs.org/safevalues/-/safevalues-0.6.0.tgz#425eadbdb699c13d8cfd932485983f858222362a" + integrity sha512-MZ7DcTOcIoPXN36/UONVE9BT0pmwlCr9WcS7Pj/q4FxOwr33FkWC0CUWj/THQXYWxf/F7urbhaHaOeFPSqGqHA== sauce-connect-launcher@^1.2.7: version "1.3.2" From 0c2ebad8cc20a7c226e9ed403b217fe7f3cb5a4d Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Thu, 4 Jul 2024 11:15:19 -0400 Subject: [PATCH 03/10] Remove exemptions, and untested usages of safevalues --- packages/auth/tsconfig.json | 1 - packages/auth/tsec-exemptions.json | 3 --- packages/database-compat/tsconfig.json | 1 - packages/database-compat/tsec-exemptions.json | 3 --- .../database/src/realtime/BrowserPollConnection.ts | 5 ++--- packages/database/tsconfig.json | 1 - packages/database/tsec-exemptions.json | 3 --- packages/messaging/src/helpers/registerDefaultSw.ts | 12 ++++-------- 8 files changed, 6 insertions(+), 23 deletions(-) delete mode 100644 packages/auth/tsec-exemptions.json delete mode 100644 packages/database-compat/tsec-exemptions.json delete mode 100644 packages/database/tsec-exemptions.json diff --git a/packages/auth/tsconfig.json b/packages/auth/tsconfig.json index 9264e1461f0..f100c88aea0 100644 --- a/packages/auth/tsconfig.json +++ b/packages/auth/tsconfig.json @@ -6,7 +6,6 @@ { "name": "tsec", "reportTsecDiagnosticsOnly": true, - "exemptionConfig": "./tsec-exemptions.json" } ] }, diff --git a/packages/auth/tsec-exemptions.json b/packages/auth/tsec-exemptions.json deleted file mode 100644 index e366beec368..00000000000 --- a/packages/auth/tsec-exemptions.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "ban-element-setattribute": ["src/platform_browser/index.ts", "src/platform_browser/load_js.test.ts"] -} diff --git a/packages/database-compat/tsconfig.json b/packages/database-compat/tsconfig.json index 891c6ae01f5..e2f493565c5 100644 --- a/packages/database-compat/tsconfig.json +++ b/packages/database-compat/tsconfig.json @@ -8,7 +8,6 @@ { "name": "tsec", "reportTsecDiagnosticsOnly": true, - "exemptionConfig": "./tsec-exemptions.json" } ] }, diff --git a/packages/database-compat/tsec-exemptions.json b/packages/database-compat/tsec-exemptions.json deleted file mode 100644 index 126a0ce25f9..00000000000 --- a/packages/database-compat/tsec-exemptions.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "ban-script-src-assignments": ["../database/src/realtime/BrowserPollConnection.ts"] -} diff --git a/packages/database/src/realtime/BrowserPollConnection.ts b/packages/database/src/realtime/BrowserPollConnection.ts index 4dcb7dd641d..d8c32665721 100644 --- a/packages/database/src/realtime/BrowserPollConnection.ts +++ b/packages/database/src/realtime/BrowserPollConnection.ts @@ -16,8 +16,6 @@ */ import { base64Encode, isNodeSdk, stringify } from '@firebase/util'; -import { sanitizeHtml } from 'safevalues'; -import { safeDocument } from 'safevalues/dom'; import { RepoInfo, repoInfoConnectionURL } from '../core/RepoInfo'; import { StatsCollection } from '../core/stats/StatsCollection'; @@ -477,7 +475,8 @@ export class FirebaseIFrameScriptHolder { const iframeContents = '' + script + ''; try { this.myIFrame.doc.open(); - safeDocument.write(this.myIFrame.doc, sanitizeHtml(iframeContents)); + // FIXME: Use the safevalues library to sanitize this + this.myIFrame.doc.write(iframeContents); this.myIFrame.doc.close(); } catch (e) { log('frame writing exception'); diff --git a/packages/database/tsconfig.json b/packages/database/tsconfig.json index 891c6ae01f5..e2f493565c5 100644 --- a/packages/database/tsconfig.json +++ b/packages/database/tsconfig.json @@ -8,7 +8,6 @@ { "name": "tsec", "reportTsecDiagnosticsOnly": true, - "exemptionConfig": "./tsec-exemptions.json" } ] }, diff --git a/packages/database/tsec-exemptions.json b/packages/database/tsec-exemptions.json deleted file mode 100644 index 39e700cb0cd..00000000000 --- a/packages/database/tsec-exemptions.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "ban-script-src-assignments": ["src/realtime/BrowserPollConnection.ts"] -} diff --git a/packages/messaging/src/helpers/registerDefaultSw.ts b/packages/messaging/src/helpers/registerDefaultSw.ts index dd27943eb1c..3ab4dd870f5 100644 --- a/packages/messaging/src/helpers/registerDefaultSw.ts +++ b/packages/messaging/src/helpers/registerDefaultSw.ts @@ -15,10 +15,7 @@ * limitations under the License. */ -import { trustedResourceUrl } from 'safevalues'; -import { safeServiceWorkerContainer } from 'safevalues/dom'; - -import { DEFAULT_SW_SCOPE } from '../util/constants'; +import { DEFAULT_SW_PATH, DEFAULT_SW_SCOPE } from '../util/constants'; import { ERROR_FACTORY, ErrorCode } from '../util/errors'; import { MessagingService } from '../messaging-service'; @@ -27,10 +24,9 @@ export async function registerDefaultSw( messaging: MessagingService ): Promise { try { - const container = navigator.serviceWorker; - messaging.swRegistration = await safeServiceWorkerContainer.register( - container, - trustedResourceUrl`/firebase-messaging-sw.js`, + // FIXME: Use safevalues to register the service worker with a sanitized URL. + messaging.swRegistration = await navigator.serviceWorker.register( + DEFAULT_SW_PATH, { scope: DEFAULT_SW_SCOPE } From 749de146608c69edc3e572ee6f807b88b8e66662 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Thu, 4 Jul 2024 11:17:13 -0400 Subject: [PATCH 04/10] Add dependency that was accidentally removed --- packages/messaging/package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/messaging/package.json b/packages/messaging/package.json index 0a4bf97d9a3..2fb2c2d2372 100644 --- a/packages/messaging/package.json +++ b/packages/messaging/package.json @@ -66,6 +66,7 @@ "@firebase/app": "0.10.6", "rollup": "2.79.1", "rollup-plugin-typescript2": "0.31.2", + "@rollup/plugin-json": "4.1.0", "ts-essentials": "9.3.0", "typescript": "4.7.4" }, From 5d16cbf66055f9c60140b41f8e37d49e091f637a Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Thu, 4 Jul 2024 11:45:35 -0400 Subject: [PATCH 05/10] Add FIXMEs for tsec violations --- packages/auth/src/platform_browser/index.ts | 4 ++++ packages/auth/src/platform_browser/load_js.test.ts | 4 ++++ packages/database/src/realtime/BrowserPollConnection.ts | 7 ++++++- packages/messaging/src/helpers/registerDefaultSw.ts | 2 +- 4 files changed, 15 insertions(+), 2 deletions(-) diff --git a/packages/auth/src/platform_browser/index.ts b/packages/auth/src/platform_browser/index.ts index f94525bfeb7..872cd442767 100644 --- a/packages/auth/src/platform_browser/index.ts +++ b/packages/auth/src/platform_browser/index.ts @@ -124,6 +124,10 @@ _setExternalJSProvider({ // TODO: consider adding timeout support & cancellation return new Promise((resolve, reject) => { const el = document.createElement('script'); + // FIXME: Do not use setAttribute, since it can lead to XSS. Instead, use the safevalues library to + // safely set an attribute for a sanitized trustedResourceUrl. Since the trustedResourceUrl + // must be initialized from a template string literal, this could involve some heavy + // refactoring. el.setAttribute('src', url); el.onload = resolve; el.onerror = e => { diff --git a/packages/auth/src/platform_browser/load_js.test.ts b/packages/auth/src/platform_browser/load_js.test.ts index 972fb292065..d7535b8eb82 100644 --- a/packages/auth/src/platform_browser/load_js.test.ts +++ b/packages/auth/src/platform_browser/load_js.test.ts @@ -44,6 +44,8 @@ describe('platform-browser/load_js', () => { loadJS(url: string): Promise { return new Promise((resolve, reject) => { const el = document.createElement('script'); + // FIXME: Do not use setAttribute, as this can lead to XSS. Instead, use the safevalues + // library, or get an exception for tests. el.setAttribute('src', url); el.onload = resolve; el.onerror = e => { @@ -65,6 +67,8 @@ describe('platform-browser/load_js', () => { // eslint-disable-next-line @typescript-eslint/no-floating-promises _loadJS('http://localhost/url'); + // FIXME: Do not use setAttribute, as this can lead to XSS. Instead, use the safevalues + // library, or get an exception for tests. expect(el.setAttribute).to.have.been.calledWith( 'src', 'http://localhost/url' diff --git a/packages/database/src/realtime/BrowserPollConnection.ts b/packages/database/src/realtime/BrowserPollConnection.ts index d8c32665721..da56c414246 100644 --- a/packages/database/src/realtime/BrowserPollConnection.ts +++ b/packages/database/src/realtime/BrowserPollConnection.ts @@ -475,7 +475,8 @@ export class FirebaseIFrameScriptHolder { const iframeContents = '' + script + ''; try { this.myIFrame.doc.open(); - // FIXME: Use the safevalues library to sanitize this + // FIXME: Do not use document.write, since it can lead to XSS. Instead, use the safevalues + // library to sanitize the HTML in the iframeContents. this.myIFrame.doc.write(iframeContents); this.myIFrame.doc.close(); } catch (e) { @@ -718,6 +719,10 @@ export class FirebaseIFrameScriptHolder { const newScript = this.myIFrame.doc.createElement('script'); newScript.type = 'text/javascript'; newScript.async = true; + // FIXME: We cannot assign an arbitrary URL to a script attached to the DOM, since it is + // at risk of XSS. We should use the safevalues library to create a safeScriptEl, and + // assign a sanitized trustedResourceURL to it. Since the URL must be a template string + // literal, this could require some heavy refactoring. newScript.src = url; // eslint-disable-next-line @typescript-eslint/no-explicit-any newScript.onload = (newScript as any).onreadystatechange = diff --git a/packages/messaging/src/helpers/registerDefaultSw.ts b/packages/messaging/src/helpers/registerDefaultSw.ts index 3ab4dd870f5..63bccc7acb2 100644 --- a/packages/messaging/src/helpers/registerDefaultSw.ts +++ b/packages/messaging/src/helpers/registerDefaultSw.ts @@ -24,7 +24,7 @@ export async function registerDefaultSw( messaging: MessagingService ): Promise { try { - // FIXME: Use safevalues to register the service worker with a sanitized URL. + // FIXME: Use safevalues to register the service worker with a sanitized trustedResourceUrl. messaging.swRegistration = await navigator.serviceWorker.register( DEFAULT_SW_PATH, { From 6c7fd0ed19f1328a976e8a723259e3338d9a068e Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Thu, 4 Jul 2024 11:47:27 -0400 Subject: [PATCH 06/10] Run formatting --- packages/auth/src/platform_browser/index.ts | 2 +- packages/auth/src/platform_browser/load_js.test.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/auth/src/platform_browser/index.ts b/packages/auth/src/platform_browser/index.ts index 872cd442767..fe06490c95c 100644 --- a/packages/auth/src/platform_browser/index.ts +++ b/packages/auth/src/platform_browser/index.ts @@ -126,7 +126,7 @@ _setExternalJSProvider({ const el = document.createElement('script'); // FIXME: Do not use setAttribute, since it can lead to XSS. Instead, use the safevalues library to // safely set an attribute for a sanitized trustedResourceUrl. Since the trustedResourceUrl - // must be initialized from a template string literal, this could involve some heavy + // must be initialized from a template string literal, this could involve some heavy // refactoring. el.setAttribute('src', url); el.onload = resolve; diff --git a/packages/auth/src/platform_browser/load_js.test.ts b/packages/auth/src/platform_browser/load_js.test.ts index d7535b8eb82..eef9e913d99 100644 --- a/packages/auth/src/platform_browser/load_js.test.ts +++ b/packages/auth/src/platform_browser/load_js.test.ts @@ -44,7 +44,7 @@ describe('platform-browser/load_js', () => { loadJS(url: string): Promise { return new Promise((resolve, reject) => { const el = document.createElement('script'); - // FIXME: Do not use setAttribute, as this can lead to XSS. Instead, use the safevalues + // FIXME: Do not use setAttribute, as this can lead to XSS. Instead, use the safevalues // library, or get an exception for tests. el.setAttribute('src', url); el.onload = resolve; @@ -67,7 +67,7 @@ describe('platform-browser/load_js', () => { // eslint-disable-next-line @typescript-eslint/no-floating-promises _loadJS('http://localhost/url'); - // FIXME: Do not use setAttribute, as this can lead to XSS. Instead, use the safevalues + // FIXME: Do not use setAttribute, as this can lead to XSS. Instead, use the safevalues // library, or get an exception for tests. expect(el.setAttribute).to.have.been.calledWith( 'src', From 1483f418ba86921241f3f40f5f538000c29ef862 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Fri, 5 Jul 2024 10:01:45 -0400 Subject: [PATCH 07/10] Compare against full Gtag script in tests --- packages/analytics/src/helpers.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/analytics/src/helpers.test.ts b/packages/analytics/src/helpers.test.ts index d2dd9094aba..3df561be986 100644 --- a/packages/analytics/src/helpers.test.ts +++ b/packages/analytics/src/helpers.test.ts @@ -67,8 +67,7 @@ describe('Gtag wrapping functions', () => { insertScriptTag(customDataLayerName, fakeMeasurementId); const scriptTag = findGtagScriptOnPage(customDataLayerName); expect(scriptTag).to.not.be.null; - expect(scriptTag!.src).to.contain(`l=customDataLayerName`); - expect(scriptTag!.src).to.contain(`id=${fakeMeasurementId}`); + expect(scriptTag!.src).to.equal(`https://www.googletagmanager.com/gtag/js?l=${customDataLayerName}&id=${fakeMeasurementId}`); }); // The test above essentially already touches this functionality but it is still valuable From b1346761b6f6cdb2174555f4ce9ac32414bcda54 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Fri, 5 Jul 2024 10:12:28 -0400 Subject: [PATCH 08/10] Check that full reCAPTCHA script URL was assigned to script element --- packages/analytics/src/helpers.test.ts | 4 +++- packages/app-check/src/recaptcha.test.ts | 12 +++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/analytics/src/helpers.test.ts b/packages/analytics/src/helpers.test.ts index 3df561be986..9145b625469 100644 --- a/packages/analytics/src/helpers.test.ts +++ b/packages/analytics/src/helpers.test.ts @@ -67,7 +67,9 @@ describe('Gtag wrapping functions', () => { insertScriptTag(customDataLayerName, fakeMeasurementId); const scriptTag = findGtagScriptOnPage(customDataLayerName); expect(scriptTag).to.not.be.null; - expect(scriptTag!.src).to.equal(`https://www.googletagmanager.com/gtag/js?l=${customDataLayerName}&id=${fakeMeasurementId}`); + expect(scriptTag!.src).to.equal( + `https://www.googletagmanager.com/gtag/js?l=${customDataLayerName}&id=${fakeMeasurementId}` + ); }); // The test above essentially already touches this functionality but it is still valuable diff --git a/packages/app-check/src/recaptcha.test.ts b/packages/app-check/src/recaptcha.test.ts index 7f028cca1a5..03c72dcab73 100644 --- a/packages/app-check/src/recaptcha.test.ts +++ b/packages/app-check/src/recaptcha.test.ts @@ -30,7 +30,9 @@ import { initializeV3, initializeEnterprise, getToken, - GreCAPTCHATopLevel + GreCAPTCHATopLevel, + RECAPTCHA_ENTERPRISE_URL, + RECAPTCHA_URL } from './recaptcha'; import * as utils from './util'; import { @@ -81,7 +83,9 @@ describe('recaptcha', () => { expect(findgreCAPTCHAScriptsOnPage().length).to.equal(0); await initializeV3(app, FAKE_SITE_KEY); - expect(findgreCAPTCHAScriptsOnPage().length).to.equal(1); + const greCATPCHAScripts = findgreCAPTCHAScriptsOnPage(); + expect(greCATPCHAScripts.length).to.equal(1); + expect(greCATPCHAScripts[0].src).to.equal(RECAPTCHA_URL); }); it('creates invisible widget', async () => { @@ -128,7 +132,9 @@ describe('recaptcha', () => { expect(findgreCAPTCHAScriptsOnPage().length).to.equal(0); await initializeEnterprise(app, FAKE_SITE_KEY); - expect(findgreCAPTCHAScriptsOnPage().length).to.equal(1); + const greCAPTCHAScripts = findgreCAPTCHAScriptsOnPage(); + expect(greCAPTCHAScripts.length).to.equal(1); + expect(greCAPTCHAScripts[0].src).to.equal(RECAPTCHA_ENTERPRISE_URL); }); it('creates invisible widget', async () => { From a073d36dff7fccb1c1e3ded3edf58147ac2e2b57 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Mon, 15 Jul 2024 10:32:32 -0400 Subject: [PATCH 09/10] Replace FIXMEs with TODOs --- packages/auth/src/platform_browser/index.ts | 2 +- packages/auth/src/platform_browser/load_js.test.ts | 4 ++-- packages/database/src/realtime/BrowserPollConnection.ts | 4 ++-- packages/messaging/src/helpers/registerDefaultSw.ts | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/auth/src/platform_browser/index.ts b/packages/auth/src/platform_browser/index.ts index fe06490c95c..f2682da38ec 100644 --- a/packages/auth/src/platform_browser/index.ts +++ b/packages/auth/src/platform_browser/index.ts @@ -124,7 +124,7 @@ _setExternalJSProvider({ // TODO: consider adding timeout support & cancellation return new Promise((resolve, reject) => { const el = document.createElement('script'); - // FIXME: Do not use setAttribute, since it can lead to XSS. Instead, use the safevalues library to + // TODO: Do not use setAttribute, since it can lead to XSS. Instead, use the safevalues library to // safely set an attribute for a sanitized trustedResourceUrl. Since the trustedResourceUrl // must be initialized from a template string literal, this could involve some heavy // refactoring. diff --git a/packages/auth/src/platform_browser/load_js.test.ts b/packages/auth/src/platform_browser/load_js.test.ts index eef9e913d99..0fda2a6d016 100644 --- a/packages/auth/src/platform_browser/load_js.test.ts +++ b/packages/auth/src/platform_browser/load_js.test.ts @@ -44,7 +44,7 @@ describe('platform-browser/load_js', () => { loadJS(url: string): Promise { return new Promise((resolve, reject) => { const el = document.createElement('script'); - // FIXME: Do not use setAttribute, as this can lead to XSS. Instead, use the safevalues + // TODO: Do not use setAttribute, as this can lead to XSS. Instead, use the safevalues // library, or get an exception for tests. el.setAttribute('src', url); el.onload = resolve; @@ -67,7 +67,7 @@ describe('platform-browser/load_js', () => { // eslint-disable-next-line @typescript-eslint/no-floating-promises _loadJS('http://localhost/url'); - // FIXME: Do not use setAttribute, as this can lead to XSS. Instead, use the safevalues + // TODO: Do not use setAttribute, as this can lead to XSS. Instead, use the safevalues // library, or get an exception for tests. expect(el.setAttribute).to.have.been.calledWith( 'src', diff --git a/packages/database/src/realtime/BrowserPollConnection.ts b/packages/database/src/realtime/BrowserPollConnection.ts index da56c414246..bcfd8bba09d 100644 --- a/packages/database/src/realtime/BrowserPollConnection.ts +++ b/packages/database/src/realtime/BrowserPollConnection.ts @@ -475,7 +475,7 @@ export class FirebaseIFrameScriptHolder { const iframeContents = '' + script + ''; try { this.myIFrame.doc.open(); - // FIXME: Do not use document.write, since it can lead to XSS. Instead, use the safevalues + // TODO: Do not use document.write, since it can lead to XSS. Instead, use the safevalues // library to sanitize the HTML in the iframeContents. this.myIFrame.doc.write(iframeContents); this.myIFrame.doc.close(); @@ -719,7 +719,7 @@ export class FirebaseIFrameScriptHolder { const newScript = this.myIFrame.doc.createElement('script'); newScript.type = 'text/javascript'; newScript.async = true; - // FIXME: We cannot assign an arbitrary URL to a script attached to the DOM, since it is + // TODO: We cannot assign an arbitrary URL to a script attached to the DOM, since it is // at risk of XSS. We should use the safevalues library to create a safeScriptEl, and // assign a sanitized trustedResourceURL to it. Since the URL must be a template string // literal, this could require some heavy refactoring. diff --git a/packages/messaging/src/helpers/registerDefaultSw.ts b/packages/messaging/src/helpers/registerDefaultSw.ts index 63bccc7acb2..b0be350bf72 100644 --- a/packages/messaging/src/helpers/registerDefaultSw.ts +++ b/packages/messaging/src/helpers/registerDefaultSw.ts @@ -24,7 +24,7 @@ export async function registerDefaultSw( messaging: MessagingService ): Promise { try { - // FIXME: Use safevalues to register the service worker with a sanitized trustedResourceUrl. + // TODO: Use safevalues to register the service worker with a sanitized trustedResourceUrl. messaging.swRegistration = await navigator.serviceWorker.register( DEFAULT_SW_PATH, { From c2bc27a12e19edb4371df34b74adc237499501e7 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Tue, 16 Jul 2024 10:08:51 -0400 Subject: [PATCH 10/10] Remove auth, rtdb, messaging from changeset --- .changeset/happy-trees-battle.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/.changeset/happy-trees-battle.md b/.changeset/happy-trees-battle.md index b5e727d5092..bf4a10a3bfe 100644 --- a/.changeset/happy-trees-battle.md +++ b/.changeset/happy-trees-battle.md @@ -1,9 +1,6 @@ --- '@firebase/analytics': patch '@firebase/app-check': patch -'@firebase/messaging': patch -'@firebase/database': patch -'@firebase/auth': patch --- Begin using [safevalues](https://github.com/google/safevalues) to sanitize HTML vulnerable to XSS.