Use safevalues to fix trusted types issues reported by tsec #8301
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dlarocque
requested review from
a team,
avolkovi,
sam-gc,
yuchenshi,
lisajian,
Xiaoshouzi-gh,
zwu52,
maneesht and
aashishpatil-g
as code owners
🦋 Changeset detectedLatest commit: c2bc27a The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Changeset File Check
|
dlarocque
force-pushed
the
dlarocque/safevalues
branch
from
24d784d
to
5606d24
Compare
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
dlarocque
commented
Jun 6, 2024
There was a problem hiding this comment.
I am confident that our usage of trustedResourceUrl works., but I feel that I need to spend much more time looking into how I can test write and safeServiceWorkerContainer.register, as those seem more complex and issues may not be caught by the tests. Any suggestions on how I can test these two would be very helpful.
dlarocque
force-pushed
the
dlarocque/safevalues
branch
from
82948f4
to
46a76f7
Compare
dlarocque
force-pushed
the
dlarocque/safevalues
branch
2 times, most recently
from
99eae21
to
7f2f930
Compare
dlarocque
force-pushed
the
dlarocque/safevalues
branch
from
96fdec2
to
b134676
Compare
hsubox76
approved these changes
Jul 11, 2024
There was a problem hiding this comment.
Looks good! I think we can make the FIXMEs into TODOs for easy context for whoever works on the fix, and set up time to work with the product teams on each of the more complicated cases. We can bring this up as an FYI in the biweekly meeting and contributors chat.
dlarocque
force-pushed
the
dlarocque/safevalues
branch
from
8c42f21
to
a073d36
Compare
Merged
|
Hi team, we are upgrading Firebase sdk to TypeError:
############################## ERROR ##############################
It looks like you are trying to call a template tag function (fn`...`)
using the normal function syntax (fn(...)), which is not supported.
The functions in the safevalues library are not designed to be called
like normal functions, and doing so invalidates the security guarantees
that safevalues provides.
If you are stuck and not sure how to proceed, please reach out to us
instead through:
- https://github.com/google/safevalues/issues
############################## ERROR ##############################
30 | const appCheckProvider = new ReCaptchaEnterpriseProvider(siteKey);
31 |
> 32 | const appCheck = initializeAppCheck(firebaseApp, {
| ^
33 | provider: appCheckProvider,
34 | isTokenAutoRefreshEnabled: true,
35 | });
at assertIsTemplateObject (../../node_modules/.pnpm/safevalues@0.6.0/node_modules/safevalues/dist/cjs/internals/string_literal.js:19:15)
at Object.trustedResourceUrl (../../node_modules/.pnpm/safevalues@0.6.0/node_modules/safevalues/dist/cjs/builders/resource_url_builders.js:162:56)
at trustedResourceUrl (../../node_modules/.pnpm/@firebase+app-check@0.8.6_@firebase+app@0.10.7/node_modules/@firebase/app-check/src/recaptcha.ts:186:5)
at initializeEnterprise (../../node_modules/.pnpm/@firebase+app-check@0.8.6_@firebase+app@0.10.7/node_modules/@firebase/app-check/src/recaptcha.ts:72:5)
at ReCaptchaEnterpriseProvider.initializeRecaptchaEnterprise [as initialize] (../../node_modules/.pnpm/@firebase+app-check@0.8.6_@firebase+app@0.10.7/node_modules/@firebase/app-check/src/providers.ts:209:5)
at _activate (../../node_modules/.pnpm/@firebase+app-check@0.8.6_@firebase+app@0.10.7/node_modules/@firebase/app-check/src/api.ts:160:18)
at initializeAppCheck (../../node_modules/.pnpm/@firebase+app-check@0.8.6_@firebase+app@0.10.7/node_modules/@firebase/app-check/src/api.ts:106:3)
at src/app-check/app-check.tsx:32:40
at apply (../utils/src/memo.tsx:41:29)
at getAppCheckToken (src/app-check/app-check-token.tsx:23:53)
at Object.<anonymous> (src/app-check/app-check-token.test.tsx:92:36) |
Hi @nicole0707, sorry that you're suddenly running into issues with this. I just tried to re-create this error message in my own Firebase web app, but was unsuccessful.
Also, if you believe this is an issue with Firebase, and not your code, please submit a new issue. |
|
Hi @dlarocque, FYI we are using the Firebase sdk with Next.js, and the testing framework is Jest. I will look into the issue and try to provide a minimal demo to reproduce it. Thanks! |
|
Hello Firebase team, my NextJS project won't build because of an exception that seem related to this update: I am using |
Downgrading to |
Hi @delmaass, thanks for reporting this. As a sanity check, I upgraded my own Next.js app (It's just the template app with basic Firebase usage) to Firebase 10.12.4, but I was not able to reproduce your issue when building or deploying. I am worried this might cause issues for a lot of Next.js users, but I haven't been able to reproduce any issues myself. |
|
@nicole0707 |
tom-andersen
pushed a commit
that referenced
this pull request
Jul 24, 2024
* Use safevalues to fix trusted types issues reported by tsec * Upgrade to safevalues 0.6.0 * Remove exemptions, and untested usages of safevalues * Add dependency that was accidentally removed * Add FIXMEs for tsec violations * Run formatting * Compare against full Gtag script in tests * Check that full reCAPTCHA script URL was assigned to script element * Replace FIXMEs with TODOs * Remove auth, rtdb, messaging from changeset
dlarocque
added a commit
that referenced
this pull request
Jul 26, 2024
Hi @nicole0707, this should be fixed in the next release which will be out by the end of the week. For more information, see #8395 |
Thanks @dlarocque, I just checked the issue has been fixed in 10.12.5. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We recently added tsec in #8285, to report errors where code is vulnerable to XSS. In this PR, we begin resolving some of the reported errors by introducing safevalues and apply it to fix a few of the simpler cases, where URLs must be wrapped in a
trustedResourceUrl. For cases that are more complex to solve, I have addedFIXME's noting that they must be fixed using the safevalues library.Testing: I have added assertions to our existing tests to ensure that the scripts' URLs are correctly assigned. For an additional sanity check, I've also created a React app using Firebase with safevalues, and verified that the
trustedResourceUrl's in App Check are actually attaching thescriptto the DOM with the correct URL using the Browser DevTools and just stepping through the code and observing state.