Skip to content

Commit a9f8440

Browse files
dlarocquedlarocque
authored
Revert introduction of safevalues (#8395)
* Revert "Use safevalues to fix trusted types issues reported by tsec (#8301)" This reverts commit f58d48c. * Add Changeset
1 parent e542f1d commit a9f8440

File tree

18 files changed

+143
-89
lines changed

18 files changed

+143
-89
lines changed

‎.changeset/tender-apes-clap.md

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@firebase/analytics': patch
3+
'@firebase/app-check': patch
4+
---
5+
6+
Revert introduction of safevalues to prevent issues from arising in Browser CommonJS environments due to ES5 incompatibility. For more information, see [GitHub PR #8395](https://github.com/firebase/firebase-js-sdk/pull/8395)

‎packages/analytics/package.json

+2-3
Original file line numberDiff line numberDiff line change
@@ -45,16 +45,15 @@
4545
"@firebase/logger": "0.4.2",
4646
"@firebase/util": "1.9.7",
4747
"@firebase/component": "0.6.8",
48-
"tslib": "^2.1.0",
49-
"safevalues": "0.6.0"
48+
"tslib": "^2.1.0"
5049
},
5150
"license": "Apache-2.0",
5251
"devDependencies": {
5352
"@firebase/app": "0.10.7",
53+
"rollup": "2.79.1",
5454
"@rollup/plugin-commonjs": "21.1.0",
5555
"@rollup/plugin-json": "4.1.0",
5656
"@rollup/plugin-node-resolve": "13.3.0",
57-
"rollup": "2.79.1",
5857
"rollup-plugin-typescript2": "0.31.2",
5958
"typescript": "4.7.4"
6059
},

‎packages/analytics/src/helpers.test.ts

+73-5
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,16 @@ import {
2424
insertScriptTag,
2525
wrapOrCreateGtag,
2626
findGtagScriptOnPage,
27-
promiseAllSettled
27+
promiseAllSettled,
28+
createGtagTrustedTypesScriptURL,
29+
createTrustedTypesPolicy
2830
} from './helpers';
29-
import { GtagCommand } from './constants';
31+
import { GtagCommand, GTAG_URL } from './constants';
3032
import { Deferred } from '@firebase/util';
3133
import { ConsentSettings } from './public-types';
3234
import { removeGtagScripts } from '../testing/gtag-script-util';
35+
import { logger } from './logger';
36+
import { AnalyticsError, ERROR_FACTORY } from './errors';
3337

3438
const fakeMeasurementId = 'abcd-efgh-ijkl';
3539
const fakeAppId = 'my-test-app-1234';
@@ -46,6 +50,71 @@ const fakeDynamicConfig: DynamicConfig = {
4650
};
4751
const fakeDynamicConfigPromises = [Promise.resolve(fakeDynamicConfig)];
4852

53+
describe('Trusted Types policies and functions', () => {
54+
if (window.trustedTypes) {
55+
describe('Trusted types exists', () => {
56+
let ttStub: SinonStub;
57+
58+
beforeEach(() => {
59+
ttStub = stub(
60+
window.trustedTypes as TrustedTypePolicyFactory,
61+
'createPolicy'
62+
).returns({
63+
createScriptURL: (s: string) => s
64+
} as any);
65+
});
66+
67+
afterEach(() => {
68+
removeGtagScripts();
69+
ttStub.restore();
70+
});
71+
72+
it('Verify trustedTypes is called if the API is available', () => {
73+
const trustedTypesPolicy = createTrustedTypesPolicy(
74+
'firebase-js-sdk-policy',
75+
{
76+
createScriptURL: createGtagTrustedTypesScriptURL
77+
}
78+
);
79+
80+
expect(ttStub).to.be.called;
81+
expect(trustedTypesPolicy).not.to.be.undefined;
82+
});
83+
84+
it('createGtagTrustedTypesScriptURL verifies gtag URL base exists when a URL is provided', () => {
85+
expect(createGtagTrustedTypesScriptURL(GTAG_URL)).to.equal(GTAG_URL);
86+
});
87+
88+
it('createGtagTrustedTypesScriptURL rejects URLs with non-gtag base', () => {
89+
const NON_GTAG_URL = 'http://iamnotgtag.com';
90+
const loggerWarnStub = stub(logger, 'warn');
91+
const errorMessage = ERROR_FACTORY.create(
92+
AnalyticsError.INVALID_GTAG_RESOURCE,
93+
{
94+
gtagURL: NON_GTAG_URL
95+
}
96+
).message;
97+
98+
expect(createGtagTrustedTypesScriptURL(NON_GTAG_URL)).to.equal('');
99+
expect(loggerWarnStub).to.be.calledWith(errorMessage);
100+
});
101+
});
102+
}
103+
describe('Trusted types does not exist', () => {
104+
it('Verify trustedTypes functions are not called if the API is not available', () => {
105+
delete window.trustedTypes;
106+
const trustedTypesPolicy = createTrustedTypesPolicy(
107+
'firebase-js-sdk-policy',
108+
{
109+
createScriptURL: createGtagTrustedTypesScriptURL
110+
}
111+
);
112+
113+
expect(trustedTypesPolicy).to.be.undefined;
114+
});
115+
});
116+
});
117+
49118
describe('Gtag wrapping functions', () => {
50119
afterEach(() => {
51120
removeGtagScripts();
@@ -67,9 +136,8 @@ describe('Gtag wrapping functions', () => {
67136
insertScriptTag(customDataLayerName, fakeMeasurementId);
68137
const scriptTag = findGtagScriptOnPage(customDataLayerName);
69138
expect(scriptTag).to.not.be.null;
70-
expect(scriptTag!.src).to.equal(
71-
`https://www.googletagmanager.com/gtag/js?l=${customDataLayerName}&id=${fakeMeasurementId}`
72-
);
139+
expect(scriptTag!.src).to.contain(`l=customDataLayerName`);
140+
expect(scriptTag!.src).to.contain(`id=${fakeMeasurementId}`);
73141
});
74142

75143
// The test above essentially already touches this functionality but it is still valuable

‎packages/analytics/src/helpers.ts

+50-10
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,25 @@ import {
2424
import { DynamicConfig, DataLayer, Gtag, MinimalDynamicConfig } from './types';
2525
import { GtagCommand, GTAG_URL } from './constants';
2626
import { logger } from './logger';
27-
import { trustedResourceUrl } from 'safevalues';
28-
import { safeScriptEl } from 'safevalues/dom';
27+
import { AnalyticsError, ERROR_FACTORY } from './errors';
2928

3029
// Possible parameter types for gtag 'event' and 'config' commands
3130
type GtagConfigOrEventParams = ControlParams & EventParams & CustomParams;
3231

32+
/**
33+
* Verifies and creates a TrustedScriptURL.
34+
*/
35+
export function createGtagTrustedTypesScriptURL(url: string): string {
36+
if (!url.startsWith(GTAG_URL)) {
37+
const err = ERROR_FACTORY.create(AnalyticsError.INVALID_GTAG_RESOURCE, {
38+
gtagURL: url
39+
});
40+
logger.warn(err.message);
41+
return '';
42+
}
43+
return url;
44+
}
45+
3346
/**
3447
* Makeshift polyfill for Promise.allSettled(). Resolves when all promises
3548
* have either resolved or rejected.
@@ -42,6 +55,29 @@ export function promiseAllSettled<T>(
4255
return Promise.all(promises.map(promise => promise.catch(e => e)));
4356
}
4457

58+
/**
59+
* Creates a TrustedTypePolicy object that implements the rules passed as policyOptions.
60+
*
61+
* @param policyName A string containing the name of the policy
62+
* @param policyOptions Object containing implementations of instance methods for TrustedTypesPolicy, see {@link https://developer.mozilla.org/en-US/docs/Web/API/TrustedTypePolicy#instance_methods
63+
* | the TrustedTypePolicy reference documentation}.
64+
*/
65+
export function createTrustedTypesPolicy(
66+
policyName: string,
67+
policyOptions: Partial<TrustedTypePolicyOptions>
68+
): Partial<TrustedTypePolicy> | undefined {
69+
// Create a TrustedTypes policy that we can use for updating src
70+
// properties
71+
let trustedTypesPolicy: Partial<TrustedTypePolicy> | undefined;
72+
if (window.trustedTypes) {
73+
trustedTypesPolicy = window.trustedTypes.createPolicy(
74+
policyName,
75+
policyOptions
76+
);
77+
}
78+
return trustedTypesPolicy;
79+
}
80+
4581
/**
4682
* Inserts gtag script tag into the page to asynchronously download gtag.
4783
* @param dataLayerName Name of datalayer (most often the default, "_dataLayer").
@@ -50,17 +86,21 @@ export function insertScriptTag(
5086
dataLayerName: string,
5187
measurementId: string
5288
): void {
53-
const script = document.createElement('script');
89+
const trustedTypesPolicy = createTrustedTypesPolicy(
90+
'firebase-js-sdk-policy',
91+
{
92+
createScriptURL: createGtagTrustedTypesScriptURL
93+
}
94+
);
5495

96+
const script = document.createElement('script');
5597
// We are not providing an analyticsId in the URL because it would trigger a `page_view`
5698
// without fid. We will initialize ga-id using gtag (config) command together with fid.
57-
//
58-
// We also have to ensure the template string before the first expression constitutes a valid URL
59-
// start, as this is what the initial validation focuses on. If the template literal begins
60-
// directly with an expression (e.g. `${GTAG_SCRIPT_URL}`), the validation fails due to an
61-
// empty initial string.
62-
const url = trustedResourceUrl`https://www.googletagmanager.com/gtag/js?l=${dataLayerName}&id=${measurementId}`;
63-
safeScriptEl.setSrc(script, url);
99+
100+
const gtagScriptURL = `${GTAG_URL}?l=${dataLayerName}&id=${measurementId}`;
101+
(script.src as string | TrustedScriptURL) = trustedTypesPolicy
102+
? (trustedTypesPolicy as TrustedTypePolicy)?.createScriptURL(gtagScriptURL)
103+
: gtagScriptURL;
64104

65105
script.async = true;
66106
document.head.appendChild(script);

‎packages/app-check/package.json

-1
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@
4242
"@firebase/util": "1.9.7",
4343
"@firebase/component": "0.6.8",
4444
"@firebase/logger": "0.4.2",
45-
"safevalues": "0.6.0",
4645
"tslib": "^2.1.0"
4746
},
4847
"license": "Apache-2.0",

‎packages/app-check/src/recaptcha.test.ts

+3-9
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,7 @@ import {
3030
initializeV3,
3131
initializeEnterprise,
3232
getToken,
33-
GreCAPTCHATopLevel,
34-
RECAPTCHA_ENTERPRISE_URL,
35-
RECAPTCHA_URL
33+
GreCAPTCHATopLevel
3634
} from './recaptcha';
3735
import * as utils from './util';
3836
import {
@@ -83,9 +81,7 @@ describe('recaptcha', () => {
8381

8482
expect(findgreCAPTCHAScriptsOnPage().length).to.equal(0);
8583
await initializeV3(app, FAKE_SITE_KEY);
86-
const greCATPCHAScripts = findgreCAPTCHAScriptsOnPage();
87-
expect(greCATPCHAScripts.length).to.equal(1);
88-
expect(greCATPCHAScripts[0].src).to.equal(RECAPTCHA_URL);
84+
expect(findgreCAPTCHAScriptsOnPage().length).to.equal(1);
8985
});
9086

9187
it('creates invisible widget', async () => {
@@ -132,9 +128,7 @@ describe('recaptcha', () => {
132128

133129
expect(findgreCAPTCHAScriptsOnPage().length).to.equal(0);
134130
await initializeEnterprise(app, FAKE_SITE_KEY);
135-
const greCAPTCHAScripts = findgreCAPTCHAScriptsOnPage();
136-
expect(greCAPTCHAScripts.length).to.equal(1);
137-
expect(greCAPTCHAScripts[0].src).to.equal(RECAPTCHA_ENTERPRISE_URL);
131+
expect(findgreCAPTCHAScriptsOnPage().length).to.equal(1);
138132
});
139133

140134
it('creates invisible widget', async () => {

‎packages/app-check/src/recaptcha.ts

+2-13
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,7 @@ import { FirebaseApp } from '@firebase/app';
1919
import { getStateReference } from './state';
2020
import { Deferred } from '@firebase/util';
2121
import { getRecaptcha, ensureActivated } from './util';
22-
import { trustedResourceUrl } from 'safevalues';
23-
import { safeScriptEl } from 'safevalues/dom';
2422

25-
// Note that these are used for testing. If they are changed, the URLs used in loadReCAPTCHAV3Script
26-
// and loadReCAPTCHAEnterpriseScript must also be changed. They aren't used to create the URLs
27-
// since trusted resource URLs must be created using template string literals.
2823
export const RECAPTCHA_URL = 'https://www.google.com/recaptcha/api.js';
2924
export const RECAPTCHA_ENTERPRISE_URL =
3025
'https://www.google.com/recaptcha/enterprise.js';
@@ -171,20 +166,14 @@ function renderInvisibleWidget(
171166

172167
function loadReCAPTCHAV3Script(onload: () => void): void {
173168
const script = document.createElement('script');
174-
safeScriptEl.setSrc(
175-
script,
176-
trustedResourceUrl`https://www.google.com/recaptcha/api.js`
177-
);
169+
script.src = RECAPTCHA_URL;
178170
script.onload = onload;
179171
document.head.appendChild(script);
180172
}
181173

182174
function loadReCAPTCHAEnterpriseScript(onload: () => void): void {
183175
const script = document.createElement('script');
184-
safeScriptEl.setSrc(
185-
script,
186-
trustedResourceUrl`https://www.google.com/recaptcha/enterprise.js`
187-
);
176+
script.src = RECAPTCHA_ENTERPRISE_URL;
188177
script.onload = onload;
189178
document.head.appendChild(script);
190179
}

‎packages/auth/package.json

+1-2
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,7 @@
131131
"@firebase/logger": "0.4.2",
132132
"@firebase/util": "1.9.7",
133133
"undici": "5.28.4",
134-
"tslib": "^2.1.0",
135-
"safevalues": "0.6.0"
134+
"tslib": "^2.1.0"
136135
},
137136
"license": "Apache-2.0",
138137
"devDependencies": {

‎packages/auth/src/platform_browser/index.ts

-4
Original file line numberDiff line numberDiff line change
@@ -124,10 +124,6 @@ _setExternalJSProvider({
124124
// TODO: consider adding timeout support & cancellation
125125
return new Promise((resolve, reject) => {
126126
const el = document.createElement('script');
127-
// TODO: Do not use setAttribute, since it can lead to XSS. Instead, use the safevalues library to
128-
// safely set an attribute for a sanitized trustedResourceUrl. Since the trustedResourceUrl
129-
// must be initialized from a template string literal, this could involve some heavy
130-
// refactoring.
131127
el.setAttribute('src', url);
132128
el.onload = resolve;
133129
el.onerror = e => {

‎packages/auth/src/platform_browser/load_js.test.ts

-4
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,6 @@ describe('platform-browser/load_js', () => {
4444
loadJS(url: string): Promise<Event> {
4545
return new Promise((resolve, reject) => {
4646
const el = document.createElement('script');
47-
// TODO: Do not use setAttribute, as this can lead to XSS. Instead, use the safevalues
48-
// library, or get an exception for tests.
4947
el.setAttribute('src', url);
5048
el.onload = resolve;
5149
el.onerror = e => {
@@ -67,8 +65,6 @@ describe('platform-browser/load_js', () => {
6765

6866
// eslint-disable-next-line @typescript-eslint/no-floating-promises
6967
_loadJS('http://localhost/url');
70-
// TODO: Do not use setAttribute, as this can lead to XSS. Instead, use the safevalues
71-
// library, or get an exception for tests.
7268
expect(el.setAttribute).to.have.been.calledWith(
7369
'src',
7470
'http://localhost/url'

‎packages/auth/tsconfig.json

+2-8
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,10 @@
11
{
22
"extends": "../../config/tsconfig.base.json",
33
"compilerOptions": {
4-
"outDir": "dist",
5-
"plugins": [
6-
{
7-
"name": "tsec",
8-
"reportTsecDiagnosticsOnly": true,
9-
}
10-
]
4+
"outDir": "dist"
115
},
126
"exclude": [
137
"dist/**/*",
148
"demo/**/*"
159
]
16-
}
10+
}

‎packages/database-compat/tsconfig.json

+2-8
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,9 @@
33
"compilerOptions": {
44
"outDir": "dist",
55
"strict": false,
6-
"downlevelIteration": true,
7-
"plugins": [
8-
{
9-
"name": "tsec",
10-
"reportTsecDiagnosticsOnly": true,
11-
}
12-
]
6+
"downlevelIteration": true
137
},
148
"exclude": [
159
"dist/**/*"
1610
]
17-
}
11+
}

‎packages/database/package.json

-1
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@
5656
"@firebase/app-check-interop-types": "0.3.2",
5757
"@firebase/auth-interop-types": "0.2.3",
5858
"faye-websocket": "0.11.4",
59-
"safevalues": "0.6.0",
6059
"tslib": "^2.1.0"
6160
},
6261
"devDependencies": {

‎packages/database/src/realtime/BrowserPollConnection.ts

-6
Original file line numberDiff line numberDiff line change
@@ -475,8 +475,6 @@ export class FirebaseIFrameScriptHolder {
475475
const iframeContents = '<html><body>' + script + '</body></html>';
476476
try {
477477
this.myIFrame.doc.open();
478-
// TODO: Do not use document.write, since it can lead to XSS. Instead, use the safevalues
479-
// library to sanitize the HTML in the iframeContents.
480478
this.myIFrame.doc.write(iframeContents);
481479
this.myIFrame.doc.close();
482480
} catch (e) {
@@ -719,10 +717,6 @@ export class FirebaseIFrameScriptHolder {
719717
const newScript = this.myIFrame.doc.createElement('script');
720718
newScript.type = 'text/javascript';
721719
newScript.async = true;
722-
// TODO: We cannot assign an arbitrary URL to a script attached to the DOM, since it is
723-
// at risk of XSS. We should use the safevalues library to create a safeScriptEl, and
724-
// assign a sanitized trustedResourceURL to it. Since the URL must be a template string
725-
// literal, this could require some heavy refactoring.
726720
newScript.src = url;
727721
// eslint-disable-next-line @typescript-eslint/no-explicit-any
728722
newScript.onload = (newScript as any).onreadystatechange =

0 commit comments

Comments
 (0)