-
Notifications
You must be signed in to change notification settings - Fork 36
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
lforst
merged 7 commits into
getsentry:main
from
Thristhart:webpack-banner-include-query-params
Aug 29, 2024
Merged
fix: Allow injection plugins to apply to files with query parameters and fragments in their name #597
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
c819c78
fix(webpack-plugin): Allow injection plugins to apply to files with q…
Thristhart 721944e
Add other bundlers, fragment and tests
lforst f1699bc
fix for win
lforst b6baee5
fix for win?
lforst 36c4067
god this is cumbersome
lforst e0734cd
ok that one is on me
lforst 0eb3970
Fix redos
lforst File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
74 changes: 74 additions & 0 deletions
74
.../integration-tests/fixtures/injection-with-query-param/injection-with-query-param.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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")); | ||
}); | ||
}); |
8 changes: 8 additions & 0 deletions
8
packages/integration-tests/fixtures/injection-with-query-param/input/bundle1.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; |
8 changes: 8 additions & 0 deletions
8
packages/integration-tests/fixtures/injection-with-query-param/input/bundle2.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; |
18 changes: 18 additions & 0 deletions
18
packages/integration-tests/fixtures/injection-with-query-param/setup.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
105
packages/integration-tests/utils/create-cjs-bundles-with-query.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; | ||
} | ||
} | ||
); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)(\?.*)?(#.*)?$/, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd also change these regexes here as shown above |
||
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, | ||
}) | ||
); | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:
I might have been burned by this a while ago, so I just wanna be extra careful 😅
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.