firebase / firebase-tools

The Firebase Command Line Tools
MIT License
4.02k stars 937 forks source link

Reserved env vars from `.env` should not crash the emulator #5833

Open gustavopch opened 1 year ago

gustavopch commented 1 year ago

[REQUIRED] Environment info

firebase-tools: 12.0.0

Platform: macOS

[REQUIRED] Test case

No test case, but the behavior is defined here:

https://github.com/firebase/firebase-tools/blob/56fb8a34201ab903f92178f696bc3b28acfb79bb/src/functions/env.ts#L163-L180

Edit: And I wrote a possible fix in the next comment below this one.

[REQUIRED] Steps to reproduce

Create a project using the Functions emulator, create a .env file with a reserved env var like FIREBASE_PROJECT_ID and run firebase emulators:start.

[REQUIRED] Expected behavior

It should log a warning but still be able to run the emulator. Other tools in the project may depend on that env var, it's only disallowed for Firebase, so crashing creates a compatibility issue with basically all other tools that need to access Firebase.

[REQUIRED] Actual behavior

It crashes the emulator with the following error:

Failed to load function definition from source: FirebaseError: Failed to load environment variables from .env.
gustavopch commented 1 year ago

Suggested fix

-export function validateKey(key: string): void {
+export function validateKey(key: string, isEmulator: boolean): void {
   if (RESERVED_KEYS.includes(key)) {
+    if (isEmulator) {
+      logWarning(`Key ${key} is reserved for internal use.`);
+    } else {
       throw new KeyValidationError(key, `Key ${key} is reserved for internal use.`);
+    }
   }
   if (!/^[A-Z_][A-Z0-9_]*$/.test(key)) {
     throw new KeyValidationError(
       key,
       `Key ${key} must start with an uppercase ASCII letter or underscore` +
         ", and then consist of uppercase ASCII letters, digits, and underscores."
     );
   }
   if (RESERVED_PREFIXES.some((prefix) => key.startsWith(prefix))) {
+    if (isEmulator) {
+      logWarning(
+        `Key ${key} starts with a reserved prefix (${RESERVED_PREFIXES.join(" ")})`
+      );
+    } else {
       throw new KeyValidationError(
         key,
         `Key ${key} starts with a reserved prefix (${RESERVED_PREFIXES.join(" ")})`
       );
+    }
   }
 }

In addition to that, if possible, consider removing RESERVED_PREFIXES altogether and explicitly declaring each reserved env var in RESERVED_KEYS so it doesn't generate warnings for env vars that are not going to cause any conflict.

aalej commented 1 year ago

Hi @gustavopch. Based on our documentation, it seems like this is working as intended. Some environment variable keys are reserved for internal use, such as keys starting with the prefix FIREBASE_. That said, I'll be tagging this issue as a feature request, and you may refer to this thread for updates.

gustavopch commented 1 year ago

@aalej You're right, it's a compatibility issue, but not a bug per se. I hope you can see how important it is to be able to have a .env with those env vars for tools other than Firebase itself though, as .env is not a file used exclusively by Firebase. Thanks for your comment.

christhompsongoogle commented 1 year ago

Hi gustavopch, can you help us understand why using a secondary .env file with FIREBASE_ prefixed keys is helpful for you?

gustavopch commented 1 year ago

@christhompsongoogle Sure. It's a single .env at the root of the repo, next to package.json. That .env contains all the env vars for the whole project. Any tool that needs to read env vars will read from that single file. So I have some env vars like FIREBASE_PROJECT_ID, FIREBASE_DATABASE_URL, etc. that are used to initialize Firebase. While they're not needed in the context of Firebase Functions, they're needed in other contexts (e.g. to initialize firebase-admin in a migration script). To illustrate:

functions/
  some-function.js
scripts/
  some-script.js
.env
.firebaserc
firebase.json
package.json

Both functions/some-function.js and scripts/some-script.js will read .env, and scripts/some-script.js will need to read FIREBASE_ prefixed keys to initialize firebase-admin. Let me know if it's not clear or if you need more information.

gustavopch commented 1 year ago

@christhompsongoogle Another use case: I'm using Remix with Firebase framework-aware hosting (through Express). The recommended way to use env vars in Remix is: the server gives them to the client during rendering. So my loader, which runs in a Firebase Function, must read the env vars that will be passed to the client.

The code that I'd like to use looks like this:

export const loader = () => {
  return {
    env: {
      // Remember this loader runs in the context of a Firebase Function and
      // I need to access these FIREBASE_ keys to pass them to the client.
      FIREBASE_API_KEY: process.env.FIREBASE_API_KEY,
      FIREBASE_APP_ID: process.env.FIREBASE_APP_ID_ADMIN,
      FIREBASE_AUTH_DOMAIN: process.env.FIREBASE_AUTH_DOMAIN,
      FIREBASE_DATABASE_URL: process.env.FIREBASE_DATABASE_URL,
      FIREBASE_MESSAGING_SENDER_ID: process.env.FIREBASE_MESSAGING_SENDER_ID,
      FIREBASE_PROJECT_ID: process.env.FIREBASE_PROJECT_ID,
      FIREBASE_STORAGE_BUCKET: process.env.FIREBASE_STORAGE_BUCKET,
    },
  }
}

function Root() {
  const loaderData = useLoaderData<typeof loader>()

  return (
    <html lang="pt-BR">
      <head>
        <Meta />
        <Links />
      </head>
      <body>
        <Outlet />
        <ScrollRestoration />
        <script
          dangerouslySetInnerHTML={{
            // This <script> runs on the client and makes the env vars returned
            // by the loader available in the Window object.
            __html: `window.env = ${JSON.stringify(loaderData.env)}`,
          }}
        />
        <Scripts />
        <LiveReload />
      </body>
    </html>
  )
}

Then in entry.client.ts, I'll be able to read the values from window.env:

firebase.initializeApp({
  apiKey: window.env.FIREBASE_API_KEY,
  appId: window.env.FIREBASE_APP_ID,
  authDomain: window.env.FIREBASE_AUTH_DOMAIN,
  databaseURL: window.env.FIREBASE_DATABASE_URL,
  messagingSenderId: window.env.FIREBASE_MESSAGING_SENDER_ID,
  projectId: window.env.FIREBASE_PROJECT_ID,
  storageBucket: window.env.FIREBASE_STORAGE_BUCKET,
})

startTransition(() => {
  hydrateRoot(document, <RemixBrowser />)
})

In this use case, I'm not talking only about being able to use FIREBASE_ prefixed keys in the emulator, but also in production. So I believe the best solution would be: allow me to write whatever keys I want in my .env, then overwrite the reserved keys as necessary.

I know I could use different names, like X_FIREBASE_PROJECT_ID, but it's just ugly.