mirror of
https://github.com/oven-sh/bun
synced 2026-02-17 06:12:08 +00:00
Address CodeRabbit review comments for publishConfig.directory
- Fixed --dry-run crash by passing edited_package.json.len instead of 0 when publishConfig.directory is set (addresses comment #2427922588) - Refactored pack_dir_path to be computed once and reused throughout the function, eliminating duplicate computation (addresses comment #2428128412) - Added 3 additional test cases (addresses comment #2428128423): 1. Verifies lifecycle scripts run from workspace root, not subdirectory 2. Rejects parent directory traversal (../) 3. Rejects absolute paths - Added validation to reject invalid publishConfig.directory values: * Absolute paths are rejected with clear error message * Parent directory traversal (..) is rejected with clear error message * Added Output.flush() calls to ensure error messages are displayed All 9 publishConfig.directory tests now passing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -1176,6 +1176,20 @@ pub const PackCommand = struct {
|
||||
|
||||
// If publishConfig.directory is set, re-read package.json from that directory
|
||||
const actual_package_json_path: stringZ = if (publish_config_directory) |directory| path_with_dir: {
|
||||
// Reject absolute paths before normalization
|
||||
if (std.fs.path.isAbsolute(directory)) {
|
||||
Output.errGeneric("publishConfig.directory cannot be an absolute path: {s}", .{directory});
|
||||
Output.flush();
|
||||
Global.crash();
|
||||
}
|
||||
|
||||
// Reject parent directory traversal before normalization
|
||||
if (strings.contains(directory, "..")) {
|
||||
Output.errGeneric("publishConfig.directory cannot contain '..': {s}", .{directory});
|
||||
Output.flush();
|
||||
Global.crash();
|
||||
}
|
||||
|
||||
var path_buf: PathBuffer = undefined;
|
||||
const normalized_dir = strings.withoutTrailingSlash(strings.withoutPrefixComptime(
|
||||
bun.path.normalizeBuf(directory, &path_buf, .posix),
|
||||
@@ -1198,6 +1212,11 @@ pub const PackCommand = struct {
|
||||
);
|
||||
} else abs_package_json_path;
|
||||
|
||||
// Directory we actually pack from (directory containing actual package.json)
|
||||
const pack_dir_path: string = strings.withoutTrailingSlash(
|
||||
strings.withoutSuffixComptime(actual_package_json_path, "package.json"),
|
||||
);
|
||||
|
||||
// Re-read package.json from the actual directory
|
||||
const actual_json = if (publish_config_directory != null) switch (manager.workspace_package_json_cache.getWithPath(manager.allocator, manager.log, actual_package_json_path, .{
|
||||
.guess_indentation = true,
|
||||
@@ -1368,7 +1387,6 @@ pub const PackCommand = struct {
|
||||
};
|
||||
|
||||
// Open the directory where files will be packed from (and where package.json is)
|
||||
const pack_dir_path: string = strings.withoutTrailingSlash(strings.withoutSuffixComptime(actual_package_json_path, "package.json"));
|
||||
var root_dir = root_dir: {
|
||||
var path_buf: PathBuffer = undefined;
|
||||
@memcpy(path_buf[0..pack_dir_path.len], pack_dir_path);
|
||||
|
||||
@@ -1546,4 +1546,71 @@ describe("publishConfig.directory", () => {
|
||||
// Should not create the tarball
|
||||
expect(await exists(join(packageDir, "pack-publishconfig-dryrun-1.0.0.tgz"))).toBeFalse();
|
||||
});
|
||||
|
||||
test("scripts run from workspace root", async () => {
|
||||
await mkdir(join(packageDir, "dist"), { recursive: true });
|
||||
await Promise.all([
|
||||
write(
|
||||
join(packageDir, "package.json"),
|
||||
JSON.stringify({
|
||||
name: "pack-publishconfig-scripts",
|
||||
version: "1.0.0",
|
||||
scripts: {
|
||||
prepack: "touch .prepack-touched",
|
||||
},
|
||||
publishConfig: {
|
||||
directory: "dist",
|
||||
},
|
||||
}),
|
||||
),
|
||||
write(
|
||||
join(packageDir, "dist", "package.json"),
|
||||
JSON.stringify({
|
||||
name: "pack-publishconfig-scripts",
|
||||
version: "1.0.0",
|
||||
}),
|
||||
),
|
||||
write(join(packageDir, "dist", "index.js"), "console.log('dist');"),
|
||||
]);
|
||||
|
||||
await pack(packageDir, bunEnv);
|
||||
|
||||
// Script should run from workspace root, not from dist/
|
||||
expect(await exists(join(packageDir, ".prepack-touched"))).toBeTrue();
|
||||
expect(await exists(join(packageDir, "dist", ".prepack-touched"))).toBeFalse();
|
||||
});
|
||||
|
||||
test("rejects parent directory traversal", async () => {
|
||||
await write(
|
||||
join(packageDir, "package.json"),
|
||||
JSON.stringify({
|
||||
name: "pack-publishconfig-parent",
|
||||
version: "1.0.0",
|
||||
publishConfig: {
|
||||
directory: "../outside",
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
const { err } = await packExpectError(packageDir, bunEnv);
|
||||
// Should reject parent directory traversal
|
||||
expect(err).toContain("publishConfig.directory cannot contain '..'");
|
||||
});
|
||||
|
||||
test("rejects absolute paths", async () => {
|
||||
await write(
|
||||
join(packageDir, "package.json"),
|
||||
JSON.stringify({
|
||||
name: "pack-publishconfig-absolute",
|
||||
version: "1.0.0",
|
||||
publishConfig: {
|
||||
directory: "/tmp/absolute",
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
const { err } = await packExpectError(packageDir, bunEnv);
|
||||
// Should reject absolute paths
|
||||
expect(err).toContain("publishConfig.directory cannot be an absolute path");
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user