Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions packages/bundler-plugin-core/src/debug-id-upload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,7 @@ export function createDebugIdUploadFunction({
globSpan.finish();

const debugIdChunkFilePaths = globResult.filter(
(debugIdChunkFilePath) =>
debugIdChunkFilePath.endsWith(".js") ||
debugIdChunkFilePath.endsWith(".mjs") ||
debugIdChunkFilePath.endsWith(".cjs")
(debugIdChunkFilePath) => !!debugIdChunkFilePath.match(/\.(js|mjs|cjs)(\?.*)?(#.*)?$/)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regex can lead to polynomial buildup. While I don't think that this is likely to produce a real ReDos situation we can change the regex a bit to avoid it:

Suggested change
(debugIdChunkFilePath) => !!debugIdChunkFilePath.match(/\.(js|mjs|cjs)(\?.*)?(#.*)?$/)
(debugIdChunkFilePath) => !!debugIdChunkFilePath.match(/\.(js|mjs|cjs)(\?[^\?]*)?(#[^#]*)?$/)

I might have been burned by this a while ago, so I just wanna be extra careful 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, could we not also just use stripQueryAndHashFromPath here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

damn, good catch - this realistically should never receive unsanitized user input but it's definitely good to change in any case. I think I'll do stripQueryAndHash here.

);

// The order of the files output by glob() is not deterministic
Expand Down
19 changes: 16 additions & 3 deletions packages/bundler-plugin-core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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 });

Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -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"));
});
});
Original file line number Diff line number Diff line change
@@ -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;
Original file line number Diff line number Diff line change
@@ -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;
Original file line number Diff line number Diff line change
@@ -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 },
}
);
}
105 changes: 105 additions & 0 deletions packages/integration-tests/utils/create-cjs-bundles-with-query.ts
Original file line number Diff line number Diff line change
@@ -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;
}
}
);
}
}
6 changes: 3 additions & 3 deletions packages/webpack-plugin/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)(\?.*)?(#.*)?$/,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also change these regexes here as shown above

banner: injectionCode,
})
);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
})
);
Expand Down
Loading