yeatmanlab / roar-dashboard

A dashboard to administer ROAR assessments
https://roar.education
Other
4 stars 4 forks source link

Add Firebase App Check #743

Closed ksmontville closed 1 month ago

ksmontville commented 2 months ago

Proposed changes

Companion PR: https://github.com/yeatmanlab/roar-firekit/pull/135 https://github.com/yeatmanlab/roar-firekit/pull/146

Docs PR: https://github.com/yeatmanlab/roar-docs/pull/15

MAJOR CHANGES:

HOW TO TEST ON PROD:

  1. Open PR link
  2. Login as any account type
  3. Open the dev console and inspect the network tab for an outgoing request to https://firestore.googleapis.com/**/*
  4. Check that the headers of the request includes a field for X-Firebase-AppCheck: with a token; every request should have this token.
  5. Monitor requests here: https://console.firebase.google.com/u/0/project/gse-roar-admin/appcheck/products, https://console.firebase.google.com/u/0/project/gse-roar-assessment/appcheck/products
  6. Once ENFORCED mode is on, all requests must have this app check token or they will fail.

HOW TO TEST ON DEV:

  1. Checkout PR code
  2. npm run dev
  3. Open developer console
  4. If you have the debug token saved to your local .env file, you should see a console.log() including the debug token. You are successfully bypassing app check in development.
  5. If you do not have the debug token saved to your local .env file, all requests should fail (?)

Types of changes

What types of changes does this pull request introduce?

Checklist

Justification of missing checklist items

Further comments

github-actions[bot] commented 2 months ago

Visit the preview URL for this PR (updated for commit 00bbea3):

https://roar-staging--pr743-enh-app-check-xtv766bb.web.app

(expires Tue, 17 Sep 2024 17:15:03 GMT)

πŸ”₯ via Firebase Hosting GitHub Action 🌎

Sign: 2631e9c58fd0104ecbfddd72a62245ddac467460

cypress[bot] commented 2 months ago

roar-dashboard-e2e    Run #6113

Run Properties:  status check passed Passed #6113  •  git commit 00bbea3859: E2E Tests for PR 743 "Add Firebase App Check" from commit "00bbea385969dc9c6fd88...
Project roar-dashboard-e2e
Branch Review enh/app-check
Run status status check passed Passed #6113
Run duration 05m 05s
Commit git commit 00bbea3859: E2E Tests for PR 743 "Add Firebase App Check" from commit "00bbea385969dc9c6fd88...
Committer Kyle
View all properties for this run β†—οΈŽ

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 26
View all changes introduced in this branch β†—οΈŽ
github-actions[bot] commented 2 months ago

Coverage Report

Status Category Percentage Covered / Total
πŸ”΅ Lines 0.97% 73 / 7505
πŸ”΅ Statements 0.89% 74 / 8243
πŸ”΅ Functions 0.76% 14 / 1830
πŸ”΅ Branches 0.44% 20 / 4473
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
Changed Files
src/firebaseInit.js 0% 0% 0% 0% 5, 8-23
src/config/firebaseRoar.js 0% 0% 0% 0% 7-9, 16-18, 20-22, 21, 25-84, 26-84, 45-62, 64-72, 74-83, 86-87
src/helpers/query/utils.js 0% 0% 0% 0% 15-37, 16-36, 27-35, 28-35, 30-35, 32, 32-35, 34, 34, 39-58, 40-56, 42-52, 43-50, 49, 51, 53, 57, 57, 57, 60-65, 67-71, 68-70, 73-92, 74-76, 79, 81-86, 82-85, 88-90, 89, 91, 94-108, 95-99, 101-107, 110-145, 118-123, 119-122, 124-126, 126-144, 131-135, 134, 138-140, 139, 141-143, 147-169, 148-151, 149-150, 152-167, 155-156, 156-166, 160-164, 163, 168, 171-207, 172-175, 173-174, 176-178, 178-204, 187-203, 189-199, 190-198, 196, 200, 206, 206, 206, 206, 209-212, 214-240, 215, 217-218, 218-219, 221-239, 225-232, 227-230, 229, 235-238
Generated in workflow #265
gitguardian[bot] commented 2 months ago

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

πŸ”Ž Detected hardcoded secret in your pull request
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | | | -------------- | ------------------ | ------------------------------ | ---------------- | --------------- | -------------------- | | [13494181](https://dashboard.gitguardian.com/workspace/496782/incidents/13494181?occurrence=164797418) | Triggered | Google API Key | d135e420b03f8b80d3263db8ea523d6962dc70f0 | src/config/firebaseRoar.js | [View secret](https://github.com/yeatmanlab/roar-dashboard/commit/d135e420b03f8b80d3263db8ea523d6962dc70f0#diff-5993a91992768a05a90d53537d3836bd2e2c43fb4b3c39fb95fa63eec4b2f801L42) |
πŸ›  Guidelines to remediate hardcoded secrets
1. Understand the implications of revoking this secret by investigating where it is used in your code. 2. Replace and store your secret safely. [Learn here](https://blog.gitguardian.com/secrets-api-management?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment) the best practices. 3. Revoke and [rotate this secret](https://docs.gitguardian.com/secrets-detection/secrets-detection-engine/detectors/specifics/googleaiza#revoke-the-secret?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment). 4. If possible, [rewrite git history](https://blog.gitguardian.com/rewriting-git-history-cheatsheet?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment). Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data. To avoid such incidents in the future consider - following these [best practices](https://blog.gitguardian.com/secrets-api-management/?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment) for managing and storing secrets including API keys and other credentials - install [secret detection on pre-commit](https://docs.gitguardian.com/ggshield-docs/integrations/git-hooks/pre-commit?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment) to catch secret before it leaves your machine and ease remediation.

πŸ¦‰ GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

richford commented 2 months ago

⚠️ GitGuardian has uncovered 4 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

πŸ”Ž Detected hardcoded secrets in your pull request πŸ›  Guidelines to remediate hardcoded secrets πŸ¦‰ GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

False positive: These are not secrets and I've skipped the check.

ksmontville commented 1 month ago

I don't know why this is happening because you seem to have followed these instructions to the letter, but for some reason the debug token is being console logged. Can you please revoke the debug token until we figure out how to keep it from being console logged?

I looked into this because I noticed the token being console logged, as well.

This is a function of the SDK itself and I don't think that it can be turned off. Perhaps we could suppress all console.log() statements when in production?

maximilianoertel commented 1 month ago

@ksmontville @richford heard the conversation during standup and curious, what is the concern with the App Check debug token being logged to the console by the Firebase SDK?

As far as I understand the situation, the token is not a secret as it's designed to be used in the frontend and even if it was not logged to console, you could always extract it from the source code or network request headers.

This also shouldn't be an issue in production, since the debug token is designed to be used in non-production environments?

ksmontville commented 1 month ago

@ksmontville @richford heard the conversation during standup and curious, what is the concern with the App Check debug token being logged to the console by the Firebase SDK?

As far as I understand the situation, the token is not a secret as it's designed to be used in the frontend and even if it was not logged to console, you could always extract it from the source code or network request headers.

This also shouldn't be an issue in production, since the debug token is designed to be used in non-production environments?

The site key which identifies the App Check app to reCAPTCHA isn't a secret and we've coded that into the Firebase config itself.

The debug token itself is supposed to be kept secret, as it allows you to bypass any App Check domain checks and opens up your back end to external reequests.

The docs for debug tokens are here: https://firebase.google.com/docs/app-check/web/debug-provider#ci

"After you register the token, Firebase backend services will accept it as valid.

Because this token allows access to your Firebase resources without a valid device, it is crucial that you keep it private. Don't commit it to a public repository, and if a registered token is ever compromised, revoke it immediately in the Firebase console."

So the SDK is logging the token itself, since this is how you initially obtain it for development purposes, and then save it for use later within the Firebase web console. So we need to figure out how to suppress this particular console.log() statement.

This is the statement that is printed to the console when running in development:

"App Check debug token: D91CB063-B58B-4209-B607-776C43064D2E. You will need to add it to your app's App Check settings in the Firebase console for it to work." (I have disabled this token so it is no longer valid.)

Firebase
Use App Check with the debug provider in web apps Β |Β  Firebase App Check
maximilianoertel commented 1 month ago

Wasn't thinking straight yesterday, sorry – and thanks for the explanation @ksmontville!

However, one point remains: the debug token would only logged locally and in the CI environments, so we don't need to worry about it being logged in staging or production? Of course, it's not ideal that the token is logged in the CI environment as the logs are stored/viewable in Cypress. However, the people with access to Cypress will also have the token in their local environment for development, so the exposure here is similar to developers being able to add their own debug token to the Firebase console.

To improve security a little bit, we could have each developer generate their own debug token and add it to the Firebase console, keeping the CI debug token a separate token. Also making it simpler in case the CI or an individual token needs to be cycled.

ksmontville commented 1 month ago

Wasn't thinking straight yesterday, sorry – and thanks for the explanation @ksmontville!

However, one point remains: the debug token would only logged locally and in the CI environments, so we don't need to worry about it being logged in staging or production? Of course, it's not ideal that the token is logged in the CI environment as the logs are stored/viewable in Cypress. However, the people with access to Cypress will also have the token in their local environment for development, so the exposure here is similar to developers being able to add their own debug token to the Firebase console.

To improve security a little bit, we could have each developer generate their own debug token and add it to the Firebase console, keeping the CI debug token a separate token. Also making it simpler in case the CI or an individual token needs to be cycled.

I think the main issue is with our PR preview links (like this one https://roar-staging--pr743-enh-app-check-xiyhrhmi.web.app/) are logging the debug token, and we consider these links to be a production environment since we use them to let our partners preview any changes. The roar-dashboard is also a public repo, so anyone could theoretically open up a PR link and inspect the console for our debug token.

So it seems that we would still need a "universal" debug token that can be injected at build time and used in the PR links to bypass the App Check domain verification. Otherwise, I do think that it would be a good idea for each developer to have their own debug token.

This whole situation could all be avoided if Firebase would allow us to use a wildcard in its domain verification, then any URL of the form matching of our PR links could just use the actual App Check token rather than the debug token. But Firebas only allows explicit domains in its whitelist, so we have to use a debug token to bypass and allow our PR links (which are variable) to run.

ROAR
richford commented 1 month ago

This whole situation could all be avoided if Firebase would allow us to use a wildcard in its domain verification, then any URL of the form matching of our PR links could just use the actual App Check token rather than the debug token. But Firebas only allows explicit domains in its whitelist, so we have to use a debug token to bypass and allow our PR links (which are variable) to run.

I wonder if there is a way to use the gcloud CLI to add the PR preview link to the list of trusted domains for that token. Then we could do it in the GitHub action. While, we're at it, we could add it to the list of trusted domains for Firebase auth, then Google SSO would work by default on the PR preview links without human intervention.

ksmontville commented 1 month ago

This whole situation could all be avoided if Firebase would allow us to use a wildcard in its domain verification, then any URL of the form matching of our PR links could just use the actual App Check token rather than the debug token. But Firebas only allows explicit domains in its whitelist, so we have to use a debug token to bypass and allow our PR links (which are variable) to run.

I wonder if there is a way to use the gcloud CLI to add the PR preview link to the list of trusted domains for that token. Then we could do it in the GitHub action. While, we're at it, we could add it to the list of trusted domains for Firebase auth, then Google SSO would work by default on the PR preview links without human intervention.

This would be a great idea, since we already grab the PR preview link in the GitHub action. We'll have to look into what kinds of operations the gcloud cli can do with App Check and/or reCAPTCHA and determine if this is possible.

I'll add a subtask to the card in ClickUp.

Emily-ejag commented 1 month ago

Approved, but not sure if GitGuardian is going to follow us later