Skip to content
22 changes: 18 additions & 4 deletions src/install/lockfile/Package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1000,6 +1000,15 @@ pub struct DiffSummary {
ArrayHashMap<TruncatedPackageNameHash, bool, ArrayIdentityContext>,
pub removed_trusted_dependencies: TrustedDependenciesSet,

/// Flags a `None → Some(empty)` transition on `trusted_dependencies`
/// (i.e. the user just added `"trustedDependencies": []` to package.json
/// on an existing project). Case 2/3/case-4-with-entries already flow
/// into `added_` / `removed_trusted_dependencies` so `has_diffs()`
/// reports them; the one remaining transition that has no elements to
/// put in either set is `None → Some({})`, which still needs a save so
/// the lockfile persists the empty array.
pub trusted_dependencies_changed: bool,

pub patched_dependencies_changed: bool,
}

Expand All @@ -1020,6 +1029,7 @@ impl DiffSummary {
|| self.catalogs_changed
|| self.added_trusted_dependencies.count() > 0
|| self.removed_trusted_dependencies.count() > 0
|| self.trusted_dependencies_changed
|| self.patched_dependencies_changed
}
}
Expand Down Expand Up @@ -1310,11 +1320,15 @@ impl Diff {
summary.added_trusted_dependencies.put(to_trusted, true)?;
}

{
// removed
// none
}
// Transitioning from "use defaults" (None) to an explicit set
// — even an empty one — is itself a semantic diff: the user
// just opted out of the default allow list. With an empty
// `to`, the loop above records zero additions, so flag the
// transition here so `has_diffs()` returns true and the
// lockfile is rewritten to persist the `[]`.
summary.trusted_dependencies_changed = true;

// removed: none
break 'trusted_dependencies;
}
}
Expand Down
29 changes: 20 additions & 9 deletions src/install/lockfile/bun.lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -398,17 +398,28 @@ impl Stringifier {
tree_sort_buf.sort_by(tree_sort_is_less_than);
// PERF(port): std.sort.pdq

if found_trusted_dependencies.len() > 0 {
// Emit `trustedDependencies` whenever the user defined it in
// package.json, even as `[]` or an array of packages that aren't
// installed. The lockfile's `trusted_dependencies` is `Some(set)`
// iff the key was present; `None` means "use Bun's default allow
// list". Dropping the key when the set didn't match any installed
// dep would make a later reload silently fall back to the
// defaults, re-enabling postinstall for packages the user meant
// to block.
if lockfile.trusted_dependencies.is_some() {
Self::write_indent(writer, *indent)?;
writer.write_all(b"\"trustedDependencies\": [\n")?;
*indent += 1;
for dep_name in found_trusted_dependencies.values() {
Self::write_indent(writer, *indent)?;
writeln!(writer, "\"{}\",", bstr::BStr::new(dep_name.slice(buf)))?;
if found_trusted_dependencies.is_empty() {
writer.write_all(b"\"trustedDependencies\": [],\n")?;
} else {
writer.write_all(b"\"trustedDependencies\": [\n")?;
*indent += 1;
for dep_name in found_trusted_dependencies.values() {
Self::write_indent(writer, *indent)?;
writeln!(writer, "\"{}\",", bstr::BStr::new(dep_name.slice(buf)))?;
}
Self::dec_indent(writer, indent)?;
writer.write_all(b"],\n")?;
}

Self::dec_indent(writer, indent)?;
writer.write_all(b"],\n")?;
}

if found_patched_dependencies.len() > 0 {
Expand Down
280 changes: 280 additions & 0 deletions test/cli/install/bun-install-lifecycle-scripts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
readdirSorted,
runBunInstall,
stderrForInstall,
tempDir,
} from "harness";
import { join, sep } from "path";

Expand Down Expand Up @@ -3187,6 +3188,285 @@
});
}

// https://github.com/oven-sh/bun/issues/31026
//
// `trustedDependencies: []` is supposed to block lifecycle scripts for every
// package, including the ones in Bun's built-in default allow list. On the
// first install this worked, but the text lockfile writer dropped the empty
// array (it only emitted `"trustedDependencies"` when at least one installed
// dep matched the set). On reload, `lockfile.trusted_dependencies = None`
// made `has_trusted_dependency` fall through to the default list, silently
// re-trusting packages like `bcrypt` / `electron` / `esbuild`.
test.concurrent("trustedDependencies: [] survives a lockfile round-trip", async () => {
// Use a `file:` dep so the test is self-contained (no registry needed).
// The bug we're guarding against is in the lockfile writer/reader, not in
// npm-specific install logic — the writer handles `trusted_dependencies`
// identically regardless of how deps were resolved.
using dir = tempDir("trusted-deps-empty-roundtrip", {
"local-dep/package.json": JSON.stringify({ name: "local-dep", version: "1.0.0" }),
"package.json": JSON.stringify({
name: "roundtrip",
version: "1.0.0",
trustedDependencies: [],
dependencies: { "local-dep": "file:./local-dep" },
}),
});

Check warning on line 3213 in test/cli/install/bun-install-lifecycle-scripts.test.ts

View check run for this annotation

Claude / Claude Code Review

Concurrent tests bypass the file-level semaphore

nit: these two `tempDir`-based tests are declared `test.concurrent` but don't go through `setupTest()` / `acquireSlot()`, so they bypass the file-level `MAX_CONCURRENT=12` semaphore documented at lines 34-39 — the only `test.concurrent` in the file that do (the other two tests added in this PR at lines 3334/3407 correctly use `using ctx = await setupTest()`). Practical impact is negligible (single `file:` dep, no lifecycle scripts, no registry), but for consistency consider wrapping them in `awa
Comment thread
robobun marked this conversation as resolved.
Outdated

const env = { ...baseEnv, BUN_INSTALL_CACHE_DIR: join(String(dir), ".bun-cache") };

// First install writes the lockfile. The critical assertion is that the
// empty array survives — without the fix, the key would be dropped.
{
await using proc = Bun.spawn({
cmd: [bunExe(), "install"],
cwd: String(dir),
env,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).not.toContain("error:");
expect(stdout).toContain("1 package installed");
expect(exitCode).toBe(0);
}

expect(await file(join(String(dir), "bun.lock")).text()).toContain('"trustedDependencies": []');

// Round-trip: reading the lockfile back must yield `Some(empty_set)` and
// not `None`. We can't inspect that directly from userspace, but a second
// install must succeed and the lockfile must still have the empty array
// (proving reader + writer are symmetric).
await rm(join(String(dir), "node_modules"), { recursive: true, force: true });
{
await using proc = Bun.spawn({
cmd: [bunExe(), "install"],
cwd: String(dir),
env,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).not.toContain("error:");
expect(stdout).toContain("1 package installed");
expect(exitCode).toBe(0);
}
expect(await file(join(String(dir), "bun.lock")).text()).toContain('"trustedDependencies": []');
});

// Follow-up to the above: on an existing project that never had
// `trustedDependencies` in its lockfile, adding `"trustedDependencies": []`
// to package.json alone (no dep changes) must be treated as a diff — the
// `None → Some({})` transition records zero adds/removes, so without an
// explicit flag the diff walker considered it "no changes" and the lockfile
// stayed on disk without the empty array. `bun pm untrusted` (which reads
// only the lockfile) then silently fell back to the default allow list.
test.concurrent("trustedDependencies: [] added to an existing project triggers a lockfile save", async () => {
using dir = tempDir("trusted-deps-added-later", {
"local-dep/package.json": JSON.stringify({ name: "local-dep", version: "1.0.0" }),
"package.json": JSON.stringify({
name: "added-later",
version: "1.0.0",
dependencies: { "local-dep": "file:./local-dep" },
}),
});

const env = { ...baseEnv, BUN_INSTALL_CACHE_DIR: join(String(dir), ".bun-cache") };

// Baseline install: no `trustedDependencies` key. Lockfile is written
// without the key.
{
await using proc = Bun.spawn({
cmd: [bunExe(), "install"],
cwd: String(dir),
env,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).not.toContain("error:");
expect(stdout).toContain("1 package installed");
expect(exitCode).toBe(0);
}
expect(await file(join(String(dir), "bun.lock")).text()).not.toContain('"trustedDependencies"');

// Add `"trustedDependencies": []` — no dep changes. Without the fix the
// diff summary records zero adds/removes, `had_any_diffs` stays false,
// `should_save_lockfile` is false, and the `[]` never reaches disk.
await Bun.write(
join(String(dir), "package.json"),
JSON.stringify({
name: "added-later",
version: "1.0.0",
trustedDependencies: [],
dependencies: { "local-dep": "file:./local-dep" },
}),
);

{
await using proc = Bun.spawn({
cmd: [bunExe(), "install"],
cwd: String(dir),
env,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).not.toContain("error:");
expect(stdout).not.toContain("error");
expect(exitCode).toBe(0);
}

expect(await file(join(String(dir), "bun.lock")).text()).toContain('"trustedDependencies": []');
});

// End-to-end version of the round-trip test above, exercising the actual
// security property: `electron` is on Bun's built-in default allow list and
// its registry fixture has a `preinstall` script that writes `preinstall.txt`.
// With `trustedDependencies: []` the postinstall must stay blocked on *every*
// install. On `main` the text lockfile dropped the empty array (electron
// didn't match the empty set, so the writer emitted no key), so reloading
// yielded `trusted_dependencies = None`, the default allow list re-applied,
// and electron's preinstall ran on the reinstall. The `file:` tests above
// can't catch this: `file:` deps carry a non-Npm tag, and the default-list
// fallback only applies to Npm-tag packages, so they're "not trusted" under
// both `Some(empty)` and `None`.
test.concurrent("trustedDependencies: [] keeps a default-trusted package blocked across reinstall", async () => {
using ctx = await setupTest();
const { packageDir, packageJson, env } = ctx;

await writeFile(
packageJson,
JSON.stringify({
name: "foo",
version: "1.2.3",
trustedDependencies: [],
dependencies: {
electron: "1.0.0",
},
}),
);

// First install: electron is default-trusted, but `trustedDependencies: []`
// opts out of the default list, so its preinstall must be blocked.
{
const { stdout, stderr, exited } = spawn({
cmd: [bunExe(), "install"],
cwd: packageDir,
stdout: "pipe",
stdin: "ignore",
stderr: "pipe",
env,
});
const err = await stderr.text();
expect(err).toContain("Saved lockfile");
expect(err).not.toContain("not found");
expect(err).not.toContain("error:");
const out = await stdout.text();
expect(out).toContain("+ electron@1.0.0");
expect(out).toContain("Blocked 1 postinstall");
expect(await exited).toBe(0);
assertManifestsPopulated(join(packageDir, ".bun-cache"), verdaccio.registryUrl());
}

// The empty array must be persisted in the text lockfile, otherwise the
// reload below sees `None` and falls back to the default allow list.
expect(await file(join(packageDir, "bun.lock")).text()).toContain('"trustedDependencies": []');
expect(await exists(join(packageDir, "node_modules", "electron", "preinstall.txt"))).toBeFalse();

// Reinstall from the persisted lockfile. This is where `main` regresses:
// the dropped key makes the defaults re-apply and electron's preinstall
// runs. With the fix, `[]` round-trips and the postinstall stays blocked.
await rm(join(packageDir, "node_modules"), { recursive: true, force: true });
{
const { stdout, stderr, exited } = spawn({
cmd: [bunExe(), "install"],
cwd: packageDir,
stdout: "pipe",
stdin: "ignore",
stderr: "pipe",
env,
});
const err = await stderr.text();
expect(err).not.toContain("not found");
expect(err).not.toContain("error:");
const out = await stdout.text();
expect(out).toContain("Blocked 1 postinstall");
expect(await exited).toBe(0);
}

expect(await file(join(packageDir, "bun.lock")).text()).toContain('"trustedDependencies": []');
expect(await exists(join(packageDir, "node_modules", "electron", "preinstall.txt"))).toBeFalse();
});

// Bonus transition from the review: deleting the `trustedDependencies` key
// from package.json (`Some → None`) must drop it from the text lockfile too,
// so a reload correctly returns to the default allow list. Start from `[]`
// (which blocks electron), then remove the key and confirm electron becomes
// trusted again and the lockfile no longer carries the array.
test.concurrent("removing trustedDependencies drops the key from the lockfile", async () => {
using ctx = await setupTest();
const { packageDir, packageJson, env } = ctx;

await writeFile(
packageJson,
JSON.stringify({
name: "foo",
version: "1.2.3",
trustedDependencies: [],
dependencies: {
electron: "1.0.0",
},
}),
);

{
const { stderr, exited } = spawn({
cmd: [bunExe(), "install"],
cwd: packageDir,
stdout: "pipe",
stdin: "ignore",
stderr: "pipe",
env,
});
const err = await stderr.text();
expect(err).not.toContain("error:");
expect(await exited).toBe(0);
}
Comment thread
robobun marked this conversation as resolved.
Outdated
expect(await file(join(packageDir, "bun.lock")).text()).toContain('"trustedDependencies": []');
expect(await exists(join(packageDir, "node_modules", "electron", "preinstall.txt"))).toBeFalse();

// Remove the key entirely → back to the default allow list.
await writeFile(
packageJson,
JSON.stringify({
name: "foo",
version: "1.2.3",
dependencies: {
electron: "1.0.0",
},
}),
);
await rm(join(packageDir, "node_modules"), { recursive: true, force: true });

{
const { stderr, exited } = spawn({
cmd: [bunExe(), "install"],
cwd: packageDir,
stdout: "pipe",
stdin: "ignore",
stderr: "pipe",
env,
});
const err = await stderr.text();
expect(err).not.toContain("error:");
expect(await exited).toBe(0);
}

const lockAfter = await file(join(packageDir, "bun.lock")).text();
expect(lockAfter).not.toContain("trustedDependencies");
expect(await exists(join(packageDir, "node_modules", "electron", "preinstall.txt"))).toBeTrue();
});

test.concurrent("ignore-scripts is read from npmrc", async () => {
using ctx = await setupTest();
const { packageDir, packageJson, env } = ctx;
Expand Down
Loading