fix: only log available bindings once in dev (#1163)

Because we were calling `printBindings` during the render phase of `<Dev/>`, we were logging the bindings multiple times (render can be called multiple times, and the interaction of Ink's stdout output intermingled with console is a bit weird). We could have put it into an effect, but I think a better solution here is to simply log it before we even start rendering `<Dev/>` (so we could see the bindings even if Dev fails to load, for example). This also adds a fix that masks any overriden values so that we don't accidentally log potential secrets into the terminal.
cloudflare · Jun 2, 2022 · 52c0bf0 · 52c0bf0
1 parent aa50f52
commit 52c0bf0
Show file tree

Hide file tree

Showing 4 changed files with 161 additions and 54 deletions.
diff --git a/.changeset/cyan-guests-grin.md b/.changeset/cyan-guests-grin.md
@@ -0,0 +1,9 @@
+---
+"wrangler": patch
+---
+
+fix: only log available bindings once in `dev`
+
+Because we were calling `printBindings` during the render phase of `<Dev/>`, we were logging the bindings multiple times (render can be called multiple times, and the interaction of Ink's stdout output intermingled with console is a bit weird). We could have put it into an effect, but I think a better solution here is to simply log it before we even start rendering `<Dev/>` (so we could see the bindings even if Dev fails to load, for example).
+
+This also adds a fix that masks any overriden values so that we don't accidentally log potential secrets into the terminal.
diff --git a/packages/wrangler/src/__tests__/dev.test.tsx b/packages/wrangler/src/__tests__/dev.test.tsx
@@ -698,7 +698,14 @@ describe("wrangler dev", () => {
       fs.writeFileSync("index.js", `export default {};`);
       await runWrangler("dev");
       expect((Dev as jest.Mock).mock.calls[0][0].ip).toEqual("localhost");
-      expect(std.out).toMatchInlineSnapshot(`""`);
+      expect(std.out).toMatchInlineSnapshot(`
+        "Your worker has access to the following bindings:
+        - Durable Objects:
+          - NAME_1: CLASS_1
+          - NAME_2: CLASS_2 (defined in SCRIPT_A)
+          - NAME_3: CLASS_3
+          - NAME_4: CLASS_4 (defined in SCRIPT_B)"
+      `);
       expect(std.warn).toMatchInlineSnapshot(`
         "[33m▲ [43;33m[[43;30mWARNING[43;33m][0m [1mProcessing wrangler.toml configuration:[0m
 
@@ -772,9 +779,18 @@ describe("wrangler dev", () => {
         EMPTY: "",
         UNQUOTED: "unquoted value", // Note that whitespace is trimmed
       });
-      expect(std.out).toMatchInlineSnapshot(
-        `"Using vars defined in .dev.vars"`
-      );
+      expect(std.out).toMatchInlineSnapshot(`
+        "Using vars defined in .dev.vars
+        Your worker has access to the following bindings:
+        - Vars:
+          - VAR_1: \\"(hidden)\\"
+          - VAR_2: \\"original value 2\\"
+          - VAR_3: \\"(hidden)\\"
+          - VAR_MULTI_LINE_1: \\"(hidden)\\"
+          - VAR_MULTI_LINE_2: \\"(hidden)\\"
+          - EMPTY: \\"(hidden)\\"
+          - UNQUOTED: \\"(hidden)\\""
+      `);
       expect(std.warn).toMatchInlineSnapshot(`""`);
       expect(std.err).toMatchInlineSnapshot(`""`);
     });
@@ -870,7 +886,10 @@ describe("wrangler dev", () => {
         Object {
           "debug": "",
           "err": "",
-          "out": "",
+          "out": "Your worker has access to the following bindings:
+        - Services:
+          - WorkerA: A
+          - WorkerB: B - staging",
           "warn": "[33m▲ [43;33m[[43;30mWARNING[43;33m][0m [1mProcessing wrangler.toml configuration:[0m
 
             - \\"services\\" fields are experimental and may change or break at any time.
@@ -883,6 +902,68 @@ describe("wrangler dev", () => {
       `);
     });
   });
+
+  describe("print bindings", () => {
+    it("should print bindings", async () => {
+      writeWranglerToml({
+        services: [
+          { binding: "WorkerA", service: "A" },
+          { binding: "WorkerB", service: "B", environment: "staging" },
+        ],
+      });
+      fs.writeFileSync("index.js", `export default {};`);
+      await runWrangler("dev index.js");
+      expect(std).toMatchInlineSnapshot(`
+        Object {
+          "debug": "",
+          "err": "",
+          "out": "Your worker has access to the following bindings:
+        - Services:
+          - WorkerA: A
+          - WorkerB: B - staging",
+          "warn": "[33m▲ [43;33m[[43;30mWARNING[43;33m][0m [1mProcessing wrangler.toml configuration:[0m
+
+            - \\"services\\" fields are experimental and may change or break at any time.
+
+
+        [33m▲ [43;33m[[43;30mWARNING[43;33m][0m [1mThis worker is bound to live services: WorkerA (A), WorkerB (B@staging)[0m
+
+        ",
+        }
+      `);
+    });
+
+    it("should mask vars that were overriden in .dev.vars", async () => {
+      writeWranglerToml({
+        vars: {
+          variable: 123,
+          overriden: "original values",
+        },
+      });
+      fs.writeFileSync(
+        ".dev.vars",
+        `
+        SECRET = "A secret"
+        overriden = "overriden value"
+      `
+      );
+      fs.writeFileSync("index.js", `export default {};`);
+      await runWrangler("dev index.js");
+      expect(std).toMatchInlineSnapshot(`
+        Object {
+          "debug": "",
+          "err": "",
+          "out": "Using vars defined in .dev.vars
+        Your worker has access to the following bindings:
+        - Vars:
+          - variable: \\"123\\"
+          - overriden: \\"(hidden)\\"
+          - SECRET: \\"(hidden)\\"",
+          "warn": "",
+        }
+      `);
+    });
+  });
 });
 
 function mockGetZones(domain: string, zones: { id: string }[] = []) {

diff --git a/packages/wrangler/src/dev/dev.tsx b/packages/wrangler/src/dev/dev.tsx
@@ -9,7 +9,6 @@ import { withErrorBoundary, useErrorHandler } from "react-error-boundary";
 import onExit from "signal-exit";
 import tmp from "tmp-promise";
 import { fetch } from "undici";
-import { printBindings } from "../config";
 import { runCustomBuild } from "../entry";
 import { openInspector } from "../inspect";
 import { logger } from "../logger";
@@ -106,8 +105,6 @@ export function DevImplementation(props: DevProps): JSX.Element {
     nodeCompat: props.nodeCompat,
   });
 
-  printBindings(props.bindings);
-
   // only load the UI if we're running in a supported environment
   const { isRawModeSupported } = useStdin();
   return isRawModeSupported ? (

diff --git a/packages/wrangler/src/index.tsx b/packages/wrangler/src/index.tsx
@@ -15,7 +15,7 @@ import { setGlobalDispatcher, ProxyAgent } from "undici";
 import makeCLI from "yargs";
 import { version as wranglerVersion } from "../package.json";
 import { fetchResult } from "./cfetch";
-import { findWranglerToml, readConfig } from "./config";
+import { findWranglerToml, printBindings, readConfig } from "./config";
 import { createWorkerUploadForm } from "./create-worker-upload-form";
 import Dev from "./dev/dev";
 import { getVarsForDev } from "./dev/dev-vars";
@@ -1119,6 +1119,70 @@ function createCLIParser(argv: string[]) {
         );
       }
 
+      const bindings = {
+        kv_namespaces: config.kv_namespaces?.map(
+          ({ binding, preview_id, id: _id }) => {
+            // In `dev`, we make folks use a separate kv namespace called
+            // `preview_id` instead of `id` so that they don't
+            // break production data. So here we check that a `preview_id`
+            // has actually been configured.
+            // This whole block of code will be obsoleted in the future
+            // when we have copy-on-write for previews on edge workers.
+            if (!preview_id) {
+              // TODO: This error has to be a _lot_ better, ideally just asking
+              // to create a preview namespace for the user automatically
+              throw new Error(
+                `In development, you should use a separate kv namespace than the one you'd use in production. Please create a new kv namespace with "wrangler kv:namespace create <name> --preview" and add its id as preview_id to the kv_namespace "${binding}" in your wrangler.toml`
+              ); // Ugh, I really don't like this message very much
+            }
+            return {
+              binding,
+              id: preview_id,
+            };
+          }
+        ),
+        // Use a copy of combinedVars since we're modifying it later
+        vars: getVarsForDev(config),
+        wasm_modules: config.wasm_modules,
+        text_blobs: config.text_blobs,
+        data_blobs: config.data_blobs,
+        durable_objects: config.durable_objects,
+        r2_buckets: config.r2_buckets?.map(
+          ({ binding, preview_bucket_name, bucket_name: _bucket_name }) => {
+            // same idea as kv namespace preview id,
+            // same copy-on-write TODO
+            if (!preview_bucket_name) {
+              throw new Error(
+                `In development, you should use a separate r2 bucket than the one you'd use in production. Please create a new r2 bucket with "wrangler r2 bucket create <name>" and add its name as preview_bucket_name to the r2_buckets "${binding}" in your wrangler.toml`
+              );
+            }
+            return {
+              binding,
+              bucket_name: preview_bucket_name,
+            };
+          }
+        ),
+        services: config.services,
+        unsafe: config.unsafe?.bindings,
+      };
+
+      // mask anything that was overridden in .dev.vars
+      // so that we don't log potential secrets into the terminal
+      const maskedVars = { ...bindings.vars };
+      for (const key of Object.keys(maskedVars)) {
+        if (maskedVars[key] !== config.vars[key]) {
+          // This means it was overridden in .dev.vars
+          // so let's mask it
+          maskedVars[key] = "(hidden)";
+        }
+      }
+
+      // now log all available bindings into the terminal
+      printBindings({
+        ...bindings,
+        vars: maskedVars,
+      });
+
       const { waitUntilExit } = render(
         <Dev
           name={getScriptName(args, config)}
@@ -1164,51 +1228,7 @@ function createCLIParser(argv: string[]) {
             args["compatibility-flags"] || config.compatibility_flags
           }
           usageModel={config.usage_model}
-          bindings={{
-            kv_namespaces: config.kv_namespaces?.map(
-              ({ binding, preview_id, id: _id }) => {
-                // In `dev`, we make folks use a separate kv namespace called
-                // `preview_id` instead of `id` so that they don't
-                // break production data. So here we check that a `preview_id`
-                // has actually been configured.
-                // This whole block of code will be obsoleted in the future
-                // when we have copy-on-write for previews on edge workers.
-                if (!preview_id) {
-                  // TODO: This error has to be a _lot_ better, ideally just asking
-                  // to create a preview namespace for the user automatically
-                  throw new Error(
-                    `In development, you should use a separate kv namespace than the one you'd use in production. Please create a new kv namespace with "wrangler kv:namespace create <name> --preview" and add its id as preview_id to the kv_namespace "${binding}" in your wrangler.toml`
-                  ); // Ugh, I really don't like this message very much
-                }
-                return {
-                  binding,
-                  id: preview_id,
-                };
-              }
-            ),
-            vars: getVarsForDev(config),
-            wasm_modules: config.wasm_modules,
-            text_blobs: config.text_blobs,
-            data_blobs: config.data_blobs,
-            durable_objects: config.durable_objects,
-            r2_buckets: config.r2_buckets?.map(
-              ({ binding, preview_bucket_name, bucket_name: _bucket_name }) => {
-                // same idea as kv namespace preview id,
-                // same copy-on-write TODO
-                if (!preview_bucket_name) {
-                  throw new Error(
-                    `In development, you should use a separate r2 bucket than the one you'd use in production. Please create a new r2 bucket with "wrangler r2 bucket create <name>" and add its name as preview_bucket_name to the r2_buckets "${binding}" in your wrangler.toml`
-                  );
-                }
-                return {
-                  binding,
-                  bucket_name: preview_bucket_name,
-                };
-              }
-            ),
-            services: config.services,
-            unsafe: config.unsafe?.bindings,
-          }}
+          bindings={bindings}
           crons={config.triggers.crons}
         />
       );