fix: Allow injection plugins to apply to files with query parameters and fragments in their name (#597)

Thristhart · Luca Forstner · web-flow · commit 7ee1f9f87239 · 2024-08-29T14:53:37.000+02:00
Co-authored-by: Luca Forstner &lt;luca.forstner@sentry.io&gt;
diff --git a/packages/bundler-plugin-core/src/debug-id-upload.ts b/packages/bundler-plugin-core/src/debug-id-upload.ts
@@ -9,6 +9,7 @@ import { Hub, NodeClient } from "@sentry/node";
 import SentryCli from "@sentry/cli";
 import { dynamicSamplingContextToSentryBaggageHeader } from "@sentry/utils";
 import { safeFlushTelemetry } from "./sentry/telemetry";
+import { stripQueryAndHashFromPath } from "./utils";
 
 interface RewriteSourcesHook {
   (source: string, map: any): string;
@@ -89,12 +90,9 @@ export function createDebugIdUploadFunction({
       });
       globSpan.finish();
 
-      const debugIdChunkFilePaths = globResult.filter(
-        (debugIdChunkFilePath) =>
-          debugIdChunkFilePath.endsWith(".js") ||
-          debugIdChunkFilePath.endsWith(".mjs") ||
-          debugIdChunkFilePath.endsWith(".cjs")
-      );
+      const debugIdChunkFilePaths = globResult.filter((debugIdChunkFilePath) => {
+        return !!stripQueryAndHashFromPath(debugIdChunkFilePath).match(/\.(js|mjs|cjs)$/);
+      });
 
       // The order of the files output by glob() is not deterministic
       // Ensure order within the files so that {debug-id}-{chunkIndex} coupling is consistent
diff --git a/packages/bundler-plugin-core/src/index.ts b/packages/bundler-plugin-core/src/index.ts
@@ -528,7 +528,10 @@ export function createRollupDebugIdInjectionHooks() {
   return {
     renderChunk(code: string, chunk: { fileName: string }) {
       if (
-        [".js", ".mjs", ".cjs"].some((ending) => chunk.fileName.endsWith(ending)) // chunks could be any file (html, md, ...)
+        // chunks could be any file (html, md, ...)
+        [".js", ".mjs", ".cjs"].some((ending) =>
+          stripQueryAndHashFromPath(chunk.fileName).endsWith(ending)
+        )
       ) {
         const debugId = stringToUUID(code); // generate a deterministic debug ID
         const codeToInject = getDebugIdSnippet(debugId);
@@ -562,7 +565,10 @@ export function createRollupModuleMetadataInjectionHooks(injectionCode: string)
   return {
     renderChunk(code: string, chunk: { fileName: string }) {
       if (
-        [".js", ".mjs", ".cjs"].some((ending) => chunk.fileName.endsWith(ending)) // chunks could be any file (html, md, ...)
+        // chunks could be any file (html, md, ...)
+        [".js", ".mjs", ".cjs"].some((ending) =>
+          stripQueryAndHashFromPath(chunk.fileName).endsWith(ending)
+        )
       ) {
         const ms = new MagicString(code, { filename: chunk.fileName });
 
@@ -600,7 +606,14 @@ export function createRollupDebugIdUploadHooks(
       if (outputOptions.dir) {
         const outputDir = outputOptions.dir;
         const buildArtifacts = await glob(
-          ["/**/*.js", "/**/*.mjs", "/**/*.cjs", "/**/*.js.map", "/**/*.mjs.map", "/**/*.cjs.map"],
+          [
+            "/**/*.js",
+            "/**/*.mjs",
+            "/**/*.cjs",
+            "/**/*.js.map",
+            "/**/*.mjs.map",
+            "/**/*.cjs.map",
+          ].map((q) => `${q}?(\\?*)?(#*)`), // We want to allow query and hashes strings at the end of files
           {
             root: outputDir,
             absolute: true,
diff --git a/packages/integration-tests/fixtures/injection-with-query-param/injection-with-query-param.test.ts b/packages/integration-tests/fixtures/injection-with-query-param/injection-with-query-param.test.ts
@@ -0,0 +1,74 @@
+/* eslint-disable jest/no-standalone-expect */
+/* eslint-disable jest/expect-expect */
+import childProcess from "child_process";
+import path from "path";
+import { testIfNodeMajorVersionIsLessThan18 } from "../../utils/testIf";
+
+function checkBundleForDebugIds(bundlePath1: string, bundlePath2: string): string[] {
+  const process1Output = childProcess.execSync(`node ${bundlePath1}`, { encoding: "utf-8" });
+  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+  const debugIdMap1 = JSON.parse(process1Output).debugIds as Record<string, string>;
+  const debugIds1 = Object.values(debugIdMap1);
+  expect(debugIds1.length).toBeGreaterThan(0);
+  expect(debugIds1).toContainEqual(
+    expect.stringMatching(/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/)
+  );
+
+  expect(Object.keys(debugIdMap1)[0]).toContain("Error");
+
+  const process2Output = childProcess.execSync(`node ${bundlePath2}`, { encoding: "utf-8" });
+  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+  const debugIdMap2 = JSON.parse(process2Output).debugIds as Record<string, string>;
+  const debugIds2 = Object.values(debugIdMap2);
+  expect(debugIds2.length).toBeGreaterThan(0);
+  expect(debugIds2).toContainEqual(
+    expect.stringMatching(/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/)
+  );
+
+  expect(Object.keys(debugIdMap2)[0]).toContain("Error");
+
+  expect(debugIds1).not.toEqual(debugIds2);
+
+  return [...debugIds1, ...debugIds2];
+}
+
+function checkBundleForRelease(bundlePath: string): void {
+  const processOutput = childProcess.execSync(`node ${bundlePath}`, { encoding: "utf-8" });
+  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+  expect(JSON.parse(processOutput).release).toBe("I AM A RELEASE!");
+}
+
+// Query params and hashes are weird on windows
+(process.platform === "win32" ? describe.skip : describe)("Injection with query params", () => {
+  test("vite bundle", () => {
+    checkBundleForDebugIds(
+      path.join(__dirname, "out", "vite", "bundle1.js?foo=bar#baz"),
+      path.join(__dirname, "out", "vite", "bundle2.js?foo=bar#baz")
+    );
+    checkBundleForRelease(path.join(__dirname, "out", "vite", "bundle1.js?foo=bar#baz"));
+  });
+
+  test("rollup bundle", () => {
+    checkBundleForDebugIds(
+      path.join(__dirname, "out", "rollup", "bundle1.js?foo=bar#baz"),
+      path.join(__dirname, "out", "rollup", "bundle2.js?foo=bar#baz")
+    );
+    checkBundleForRelease(path.join(__dirname, "out", "rollup", "bundle1.js?foo=bar#baz"));
+  });
+
+  testIfNodeMajorVersionIsLessThan18("webpack 4 bundle", () => {
+    checkBundleForDebugIds(
+      path.join(__dirname, "out", "webpack4", "bundle1.js"),
+      path.join(__dirname, "out", "webpack4", "bundle2.js")
+    );
+    checkBundleForRelease(path.join(__dirname, "out", "webpack4", "bundle1.js"));
+  });
+
+  test("webpack 5 bundle", () => {
+    checkBundleForDebugIds(
+      path.join(__dirname, "out", "webpack5", "bundle1.js"),
+      path.join(__dirname, "out", "webpack5", "bundle2.js")
+    );
+    checkBundleForRelease(path.join(__dirname, "out", "webpack5", "bundle1.js"));
+  });
+});
diff --git a/packages/integration-tests/fixtures/injection-with-query-param/input/bundle1.js b/packages/integration-tests/fixtures/injection-with-query-param/input/bundle1.js
@@ -0,0 +1,8 @@
+// eslint-disable-next-line no-console
+console.log(
+  // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access
+  JSON.stringify({ debugIds: global._sentryDebugIds, release: global.SENTRY_RELEASE.id })
+);
+
+// Just so the two bundles generate different hashes:
+global.iAmBundle1 = 1;
diff --git a/packages/integration-tests/fixtures/injection-with-query-param/input/bundle2.js b/packages/integration-tests/fixtures/injection-with-query-param/input/bundle2.js
@@ -0,0 +1,8 @@
+// eslint-disable-next-line no-console
+console.log(
+  // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access
+  JSON.stringify({ debugIds: global._sentryDebugIds, release: global.SENTRY_RELEASE.id })
+);
+
+// Just so the two bundles generate different hashes:
+global.iAmBundle2 = 2;
diff --git a/packages/integration-tests/fixtures/injection-with-query-param/setup.ts b/packages/integration-tests/fixtures/injection-with-query-param/setup.ts
@@ -0,0 +1,18 @@
+import * as path from "path";
+import { createCjsBundlesWithQueryParam } from "../../utils/create-cjs-bundles-with-query";
+
+// Query params and hashes are weird on windows
+if (process.platform !== "win32") {
+  const outputDir = path.resolve(__dirname, "out");
+  createCjsBundlesWithQueryParam(
+    {
+      bundle1: path.resolve(__dirname, "input", "bundle1.js"),
+      bundle2: path.resolve(__dirname, "input", "bundle2.js"),
+    },
+    outputDir,
+    {
+      telemetry: false,
+      release: { name: "I AM A RELEASE!", create: false },
+    }
+  );
+}
diff --git a/packages/integration-tests/utils/create-cjs-bundles-with-query.ts b/packages/integration-tests/utils/create-cjs-bundles-with-query.ts
@@ -0,0 +1,105 @@
+import * as vite from "vite";
+import * as path from "path";
+import * as rollup from "rollup";
+import { default as webpack4 } from "webpack4";
+import { webpack as webpack5 } from "webpack5";
+import { Options } from "@sentry/bundler-plugin-core";
+import { sentryVitePlugin } from "@sentry/vite-plugin";
+import { sentryWebpackPlugin } from "@sentry/webpack-plugin";
+import { sentryRollupPlugin } from "@sentry/rollup-plugin";
+
+// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+const nodejsMajorVersion = process.version.split(".")[0]!.slice(1);
+
+export function createCjsBundlesWithQueryParam(
+  entrypoints: { [name: string]: string },
+  outFolder: string,
+  sentryUnpluginOptions: Options,
+  plugins: string[] = []
+): void {
+  if (plugins.length === 0 || plugins.includes("vite")) {
+    void vite.build({
+      clearScreen: false,
+      build: {
+        sourcemap: true,
+        outDir: path.join(outFolder, "vite"),
+        rollupOptions: {
+          input: entrypoints,
+          output: {
+            format: "cjs",
+            entryFileNames: "[name].js?foo=bar#baz",
+          },
+        },
+      },
+      plugins: [sentryVitePlugin(sentryUnpluginOptions)],
+    });
+  }
+  if (plugins.length === 0 || plugins.includes("rollup")) {
+    void rollup
+      .rollup({
+        input: entrypoints,
+        plugins: [sentryRollupPlugin(sentryUnpluginOptions)],
+      })
+      .then((bundle) =>
+        bundle.write({
+          sourcemap: true,
+          dir: path.join(outFolder, "rollup"),
+          format: "cjs",
+          exports: "named",
+          entryFileNames: "[name].js?foo=bar#baz",
+        })
+      );
+  }
+
+  if (plugins.length === 0 || plugins.includes("esbuild")) {
+    // esbuild doesn't have an option to add a query param
+  }
+
+  // Webpack 4 doesn't work on Node.js versions >= 18
+  if (parseInt(nodejsMajorVersion) < 18 && (plugins.length === 0 || plugins.includes("webpack4"))) {
+    webpack4(
+      {
+        devtool: "source-map",
+        mode: "production",
+        entry: entrypoints,
+        cache: false,
+        output: {
+          path: path.join(outFolder, "webpack4"),
+          filename: "[name].js?foo=bar#baz", // For some weird reason, the query param is not actually put to disk but the "virtual" behaviour we want to test still applies
+          libraryTarget: "commonjs",
+        },
+        target: "node", // needed for webpack 4 so we can access node api
+        plugins: [sentryWebpackPlugin(sentryUnpluginOptions)],
+      },
+      (err) => {
+        if (err) {
+          throw err;
+        }
+      }
+    );
+  }
+
+  if (plugins.length === 0 || plugins.includes("webpack5")) {
+    webpack5(
+      {
+        devtool: "source-map",
+        cache: false,
+        entry: entrypoints,
+        output: {
+          path: path.join(outFolder, "webpack5"),
+          filename: "[name].js?foo=bar#baz", // For some weird reason, the query param is not actually put to disk but the "virtual" behaviour we want to test still applies
+          library: {
+            type: "commonjs",
+          },
+        },
+        mode: "production",
+        plugins: [sentryWebpackPlugin(sentryUnpluginOptions)],
+      },
+      (err) => {
+        if (err) {
+          throw err;
+        }
+      }
+    );
+  }
+}
diff --git a/packages/webpack-plugin/src/index.ts b/packages/webpack-plugin/src/index.ts
@@ -39,7 +39,7 @@ function webpackReleaseInjectionPlugin(injectionCode: string): UnpluginOptions {
         // eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-unsafe-call
         new BannerPlugin({
           raw: true,
-          include: /\.(js|ts|jsx|tsx|mjs|cjs)$/,
+          include: /\.(js|ts|jsx|tsx|mjs|cjs)(\?[^?]*)?(#[^#]*)?$/,
           banner: injectionCode,
         })
       );
@@ -105,7 +105,7 @@ function webpackDebugIdInjectionPlugin(): UnpluginOptions {
         // eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-unsafe-call
         new BannerPlugin({
           raw: true,
-          include: /\.(js|ts|jsx|tsx|mjs|cjs)$/,
+          include: /\.(js|ts|jsx|tsx|mjs|cjs)(\?[^?]*)?(#[^#]*)?$/,
           banner: (arg?: BannerPluginCallbackArg) => {
             const debugId = arg?.chunk?.hash ? stringToUUID(arg.chunk.hash) : uuidv4();
             return getDebugIdSnippet(debugId);
@@ -156,7 +156,7 @@ function webpackModuleMetadataInjectionPlugin(injectionCode: string): UnpluginOp
         // eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-unsafe-call
         new BannerPlugin({
           raw: true,
-          include: /\.(js|ts|jsx|tsx|mjs|cjs)$/,
+          include: /\.(js|ts|jsx|tsx|mjs|cjs)(\?[^?]*)?(#[^#]*)?$/,
           banner: injectionCode,
         })
       );
