Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
28 changes: 24 additions & 4 deletions packages/bundler-plugin-core/src/build-plugin-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ function createCliInstance(options: NormalizedOptions): SentryCli {
return new SentryCli(null, {
authToken: options.authToken,
org: options.org,
project: options.project,
// Default to the first project if multiple projects are specified
project: Array.isArray(options.project) ? options.project[0] : options.project,
silent: options.silent,
url: options.url,
vcsRemote: options.release.vcsRemote,
Expand Down Expand Up @@ -360,7 +361,12 @@ export function createSentryBuildPluginManager(
if (typeof options.moduleMetadata === "function") {
const args = {
org: options.org,
project: options.project,
project: Array.isArray(options.project) ? options.project[0] : options.project,
projects: Array.isArray(options.project)
Copy link
Member

Choose a reason for hiding this comment

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

l: There is a lot of code duplication like that, which would make future development harder as we need to know to implement it like that. We could make a function for that, reuse it and write a simple unit test for it (btw this line already differs from the ones below).

E.g. getProjects and getFirstProject

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, good point. I added getProjects but opted to just use optionals for the first project.

? options.project
: options.project
? [options.project]
: undefined,
release: options.release.name,
};
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
Expand Down Expand Up @@ -444,7 +450,10 @@ export function createSentryBuildPluginManager(
getTurborepoEnvPassthroughWarning("SENTRY_ORG")
);
return;
} else if (!options.project) {
} else if (
!options.project ||
(Array.isArray(options.project) && options.project.length === 0)
) {
logger.warn(
"No project provided. Will not create release. Please set the `project` option to your Sentry project slug." +
getTurborepoEnvPassthroughWarning("SENTRY_PROJECT")
Expand Down Expand Up @@ -481,6 +490,9 @@ export function createSentryBuildPluginManager(
await cliInstance.releases.uploadSourceMaps(options.release.name, {
include: normalizedInclude,
dist: options.release.dist,
// @ts-expect-error - projects is not a valid option for uploadSourceMaps but is implemented in the CLI
// Remove once https://github.com/getsentry/sentry-cli/pull/2856 is released
projects: Array.isArray(options.project) ? options.project : [options.project],
// We want this promise to throw if the sourcemaps fail to upload so that we know about it.
// see: https://github.com/getsentry/sentry-cli/pull/2605
live: "rejectOnError",
Expand Down Expand Up @@ -625,6 +637,9 @@ export function createSentryBuildPluginManager(
},
],
ignore: ignorePaths,
// @ts-expect-error - projects is not a valid option for uploadSourceMaps but is implemented in the CLI
// Remove once https://github.com/getsentry/sentry-cli/pull/2856 is released
projects: Array.isArray(options.project) ? options.project : [options.project],
live: "rejectOnError",
});
});
Expand Down Expand Up @@ -735,6 +750,11 @@ export function createSentryBuildPluginManager(
dist: options.release.dist,
},
],
// @ts-expect-error - projects is not a valid option for uploadSourceMaps but is implemented in the CLI
// Remove once https://github.com/getsentry/sentry-cli/pull/2856 is released
projects: Array.isArray(options.project)
? options.project
: [options.project],
live: "rejectOnError",
}
);
Expand Down Expand Up @@ -843,7 +863,7 @@ function canUploadSourceMaps(
);
return false;
}
if (!options.project) {
if (!options.project || (Array.isArray(options.project) && options.project.length === 0)) {
Copy link
Member

Choose a reason for hiding this comment

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

l: Theoretically if there is a getFirstProject as recommended in another comment we could write following:

Suggested change
if (!options.project || (Array.isArray(options.project) && options.project.length === 0)) {
if (!getFirstProject(options.project)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to use !getProjects(options.project)?.[0]

logger.warn(
"No project provided. Will not upload source maps. Please set the `project` option to your Sentry project slug." +
getTurborepoEnvPassthroughWarning("SENTRY_PROJECT")
Expand Down
27 changes: 25 additions & 2 deletions packages/bundler-plugin-core/src/options-mapping.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { determineReleaseName } from "./utils";

export type NormalizedOptions = {
org: string | undefined;
project: string | undefined;
project: string | string[] | undefined;
authToken: string | undefined;
url: string;
headers: Record<string, string> | undefined;
Expand Down Expand Up @@ -89,7 +89,9 @@ export const SENTRY_SAAS_URL = "https://sentry.io";
export function normalizeUserOptions(userOptions: UserOptions): NormalizedOptions {
const options = {
org: userOptions.org ?? process.env["SENTRY_ORG"],
project: userOptions.project ?? process.env["SENTRY_PROJECT"],
project: userOptions.project ?? (process.env["SENTRY_PROJECT"]?.includes(',')
? process.env["SENTRY_PROJECT"].split(',').map(p => p.trim())
: process.env["SENTRY_PROJECT"]),
authToken: userOptions.authToken ?? process.env["SENTRY_AUTH_TOKEN"],
url: userOptions.url ?? process.env["SENTRY_URL"] ?? SENTRY_SAAS_URL,
headers: userOptions.headers,
Comment on lines 89 to 99

Choose a reason for hiding this comment

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

Potential bug: An invalid SENTRY_PROJECT env var can create an array with empty strings, bypassing guards if a custom errorHandler is used, causing CLI calls to fail.
  • Description: When the SENTRY_PROJECT environment variable is set to a value that results in empty strings after parsing (e.g., SENTRY_PROJECT=","), it creates an invalid project array like [""]. The validateOptions function detects this, but if a user has configured a custom errorHandler that doesn't throw an error, execution continues. Downstream functions like createRelease and canUploadSourceMaps contain guards that only check if the project array is undefined or has a length of zero. These guards do not validate the contents of the array, so [""] is considered valid. This leads to sentry-cli being called with an empty project name, causing the sourcemap upload process to fail during the build.

  • Suggested fix: Strengthen the guards in createRelease and canUploadSourceMaps. Instead of only checking !options.project || options.project.length === 0, also ensure that all elements within the options.project array are non-empty strings. This prevents invalid data from being passed to the Sentry CLI even if initial validation errors are suppressed.
    severity: 0.65, confidence: 0.95

Did we get this right? 👍 / 👎 to inform future reviews.

Expand Down Expand Up @@ -209,5 +211,26 @@ export function validateOptions(options: NormalizedOptions, logger: Logger): boo
return false;
}

if (options.project) {
Copy link
Member

Choose a reason for hiding this comment

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

l: you can flatten it by one if

if (options.project && Array.isArray(options.project)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

if (Array.isArray(options.project)) {
if (options.project.length === 0) {
logger.error(
"The `project` option was specified as an array but is empty.",
"Please provide at least one project slug."
);
return false;
}
// Check each project is a non-empty string
const invalidProjects = options.project.filter(p => typeof p !== 'string' || p.trim() === '');
if (invalidProjects.length > 0) {
logger.error(
"The `project` option contains invalid project slugs.",
"All projects must be non-empty strings."
);
return false;
}
}
}

return true;
}
4 changes: 2 additions & 2 deletions packages/bundler-plugin-core/src/sentry/telemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export function setTelemetryDataOnScope(

scope.setTags({
organization: org,
project,
project: Array.isArray(project) ? project.join(", ") : project ?? "undefined",
bundler: buildTool,
});

Expand All @@ -129,7 +129,7 @@ export async function allowedToSendTelemetry(options: NormalizedOptions): Promis
url,
authToken,
org,
project,
project: Array.isArray(project) ? project[0] : project,
vcsRemote: release.vcsRemote,
silent,
headers,
Expand Down
10 changes: 8 additions & 2 deletions packages/bundler-plugin-core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,13 @@ export interface Options {
/**
* The slug of the Sentry project associated with the app.
*
* When uploading source maps, you can specify multiple projects (as an array) to upload
* the same source maps to multiple projects. This is useful in monorepo environments
* where multiple projects share the same release.
*
* This value can also be specified via the `SENTRY_PROJECT` environment variable.
*/
project?: string;
project?: string | string[];

/**
* The authentication token to use for all communication with Sentry.
Expand Down Expand Up @@ -361,7 +365,8 @@ export interface Options {
* Metadata can either be passed directly or alternatively a callback can be provided that will be
* called with the following parameters:
* - `org`: The organization slug.
* - `project`: The project slug.
* - `project`: The project slug (when multiple projects are configured, this is the first project).
* - `projects`: An array of all project slugs (available when multiple projects are configured).
* - `release`: The release name.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand Down Expand Up @@ -428,6 +433,7 @@ export interface ModuleMetadata {
export interface ModuleMetadataCallbackArgs {
org?: string;
project?: string;
projects?: string[];
release?: string;
}

Expand Down
168 changes: 168 additions & 0 deletions packages/bundler-plugin-core/test/build-plugin-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,7 @@ describe("createSentryBuildPluginManager", () => {
// Should upload from temp folder
expect(mockCliUploadSourceMaps).toHaveBeenCalledWith("some-release-name", {
include: [{ paths: ["/tmp/sentry-upload-xyz"], rewrite: false, dist: "1" }],
projects: ["p"],
live: "rejectOnError",
});
});
Expand Down Expand Up @@ -463,4 +464,171 @@ describe("createSentryBuildPluginManager", () => {
);
});
});

describe("uploadSourcemaps with multiple projects", () => {
beforeEach(() => {
jest.clearAllMocks();
mockGlob.mockResolvedValue(["/path/to/bundle.js"]);
mockPrepareBundleForDebugIdUpload.mockResolvedValue(undefined);
mockCliUploadSourceMaps.mockResolvedValue(undefined);

// Mock fs operations needed for temp folder upload path
jest.spyOn(fs.promises, "mkdtemp").mockResolvedValue("/tmp/sentry-test");
jest.spyOn(fs.promises, "readdir").mockResolvedValue([]);
jest.spyOn(fs.promises, "stat").mockResolvedValue({ size: 1000 } as fs.Stats);
jest.spyOn(fs.promises, "rm").mockResolvedValue(undefined);
});

afterEach(() => {
jest.restoreAllMocks();
});

it("should pass projects array to uploadSourceMaps when multiple projects configured", async () => {
const buildPluginManager = createSentryBuildPluginManager(
{
authToken: "test-token",
org: "test-org",
project: ["proj-a", "proj-b", "proj-c"],
release: { name: "test-release" },
},
{
buildTool: "webpack",
loggerPrefix: "[sentry-webpack-plugin]",
}
);

await buildPluginManager.uploadSourcemaps(["/path/to/bundle.js"]);

expect(mockCliUploadSourceMaps).toHaveBeenCalledWith(
"test-release",
expect.objectContaining({
projects: ["proj-a", "proj-b", "proj-c"],
})
);
});

it("should pass single project as array to uploadSourceMaps", async () => {
const buildPluginManager = createSentryBuildPluginManager(
{
authToken: "test-token",
org: "test-org",
project: "single-project",
release: { name: "test-release" },
},
{
buildTool: "webpack",
loggerPrefix: "[sentry-webpack-plugin]",
}
);

await buildPluginManager.uploadSourcemaps(["/path/to/bundle.js"]);

expect(mockCliUploadSourceMaps).toHaveBeenCalledWith(
"test-release",
expect.objectContaining({
projects: ["single-project"],
})
);
});

it("should pass projects array in direct upload mode", async () => {
const buildPluginManager = createSentryBuildPluginManager(
{
authToken: "test-token",
org: "test-org",
project: ["proj-a", "proj-b"],
release: { name: "test-release" },
},
{
buildTool: "webpack",
loggerPrefix: "[sentry-webpack-plugin]",
}
);

await buildPluginManager.uploadSourcemaps(["/path/to/bundle.js"], { prepareArtifacts: false });

expect(mockCliUploadSourceMaps).toHaveBeenCalledWith(
"test-release",
expect.objectContaining({
projects: ["proj-a", "proj-b"],
})
);
});
});

describe("moduleMetadata callback with multiple projects", () => {
it("should pass project as string and projects as array when multiple projects configured", () => {
const moduleMetadataCallback = jest.fn().mockReturnValue({ custom: "metadata" });

createSentryBuildPluginManager(
{
authToken: "test-token",
org: "test-org",
project: ["proj-a", "proj-b", "proj-c"],
release: { name: "test-release" },
moduleMetadata: moduleMetadataCallback,
},
{
buildTool: "webpack",
loggerPrefix: "[sentry-webpack-plugin]",
}
);

expect(moduleMetadataCallback).toHaveBeenCalledWith({
org: "test-org",
project: "proj-a",
projects: ["proj-a", "proj-b", "proj-c"],
release: "test-release",
});
});

it("should pass project as string and projects as array with single project", () => {
const moduleMetadataCallback = jest.fn().mockReturnValue({ custom: "metadata" });

createSentryBuildPluginManager(
{
authToken: "test-token",
org: "test-org",
project: "single-project",
release: { name: "test-release" },
moduleMetadata: moduleMetadataCallback,
},
{
buildTool: "webpack",
loggerPrefix: "[sentry-webpack-plugin]",
}
);

expect(moduleMetadataCallback).toHaveBeenCalledWith({
org: "test-org",
project: "single-project",
projects: ["single-project"],
release: "test-release",
});
});

it("should pass undefined for projects when no project configured", () => {
const moduleMetadataCallback = jest.fn().mockReturnValue({ custom: "metadata" });

createSentryBuildPluginManager(
{
authToken: "test-token",
org: "test-org",
release: { name: "test-release" },
moduleMetadata: moduleMetadataCallback,
},
{
buildTool: "webpack",
loggerPrefix: "[sentry-webpack-plugin]",
}
);

expect(moduleMetadataCallback).toHaveBeenCalledWith({
org: "test-org",
project: undefined,
projects: undefined,
release: "test-release",
});
});
});
});
Loading
Loading