Skip to content
Closed
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
31 changes: 26 additions & 5 deletions src/install/PackageManager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,11 @@ pub struct PackageManager {
// `DotEnv.Loader` (leaked allocation; outlives the manager). `BackRef`
// encapsulates the liveness invariant so `env()` is a safe accessor.
pub env: Option<bun_ptr::BackRef<dot_env::Loader, bun_ptr::Mut>>,
/// The environment this process started with. `env` above also holds the
/// project's `.env*` files, which a cloned repository controls. The git
/// children the install spawns and its own proxy / TLS settings read this
/// one, so a `.env` cannot set `GIT_SSH_COMMAND` or `HTTPS_PROXY` for them.
Comment thread
robobun marked this conversation as resolved.
Resolved
pub(crate) process_env: dot_env::Loader,
pub progress: Progress,
pub(crate) downloads_node: Option<*mut ProgressNode>, // BORROW_FIELD — points into self.progress
pub scripts_node: Option<NonNull<ProgressNode>>, // points to a caller stack-local Progress node; only valid while that caller frame is live
Expand Down Expand Up @@ -892,13 +897,17 @@ impl PackageManager {
}

pub fn http_proxy(&self, url: &URL<'_>) -> Option<URL<'static>> {
// `env_mut()` yields an unbounded `&'a Loader` (process-lifetime
// singleton), so the returned `URL<'_>` borrows for `'static`.
self.env_mut().get_http_proxy_for(url)
// SAFETY: the manager is a leaked process-lifetime singleton and
// `process_env` is never written after `init()`, so the returned
// `URL<'_>`, which borrows the proxy value out of it, is valid for
// `'static`.
let process_env: &'static dot_env::Loader =
unsafe { bun_ptr::detach_lifetime_ref(&self.process_env) };
process_env.get_http_proxy_for(url)
}

pub fn tls_reject_unauthorized(&self) -> bool {
self.env().get_tls_reject_unauthorized()
self.process_env.get_tls_reject_unauthorized()
Comment thread
robobun marked this conversation as resolved.
Resolved
}

pub(crate) fn fail_root_resolution(
Expand Down Expand Up @@ -1332,6 +1341,14 @@ fn http_thread_on_init_error(err: http::InitError, opts: &http::http_thread::Ini
// allocate / get singleton
// ──────────────────────────────────────────────────────────────────────────

/// `PackageManager::process_env`: the process environment with no `.env*` file
/// merged in.
Comment thread
robobun marked this conversation as resolved.
Resolved
fn load_process_env() -> Result<dot_env::Loader, bun_alloc::AllocError> {
let mut env = dot_env::Loader::init();
env.load_process()?;
Ok(env)
}

fn allocate_package_manager() {
// Uninitialized memory, abort-on-OOM. The init() functions below write the full struct via
// `core::ptr::write` (no Drop on the uninit bytes).
Expand Down Expand Up @@ -1899,6 +1916,7 @@ pub fn init(
};

env.load_process()?;
let process_env = load_process_env()?;
// Copy the listing's basenames out under `entries_mutex`; `.data` must
// only be probed while the lock is held.
let env_probe_keys = {
Expand Down Expand Up @@ -2056,6 +2074,7 @@ pub fn init(
// reads. `BackRef` stores a raw pointer —
// ending the reborrow here does not alias the later uses.
wr!(env, Some(bun_ptr::BackRef::new_mut(&mut *env)));
wr!(process_env, process_env);
wr!(
thread_pool,
ThreadPool::init(thread_pool::Config {
Expand Down Expand Up @@ -2320,7 +2339,7 @@ pub fn init(
}

// If any HTTP proxy is set, use a diferent limit
if env.has_http_proxy() {
if mgr_ref.process_env.has_http_proxy() {
break 'brk DEFAULT_MAX_SIMULTANEOUS_REQUESTS_FOR_BUN_INSTALL_FOR_PROXIES;
}

Expand Down Expand Up @@ -2443,6 +2462,7 @@ fn init_with_runtime_once(
};

let cpu_count: u32 = u32::from(bun_core::get_thread_count());
let process_env = load_process_env()?;
Comment thread
robobun marked this conversation as resolved.
Resolved
allocate_package_manager();
// SAFETY: holder::RAW_PTR was just set by allocate_package_manager() to a
// freshly allocated, *uninitialized* PackageManager. Do NOT call `get()` /
Expand Down Expand Up @@ -2516,6 +2536,7 @@ fn init_with_runtime_once(
// reads. `BackRef` stores a raw pointer —
// ending the reborrow here does not alias the later uses.
wr!(env, Some(bun_ptr::BackRef::new_mut(&mut *env)));
wr!(process_env, process_env);
wr!(
thread_pool,
ThreadPool::init(thread_pool::Config {
Expand Down
2 changes: 1 addition & 1 deletion src/install/PackageManager/install_with_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ pub fn install_with_manager(

// Start resolving DNS for the default registry immediately.
// Unless you're behind a proxy.
if !manager.env().has_http_proxy()
if !manager.process_env.has_http_proxy()
&& manager.options.offline != crate::package_manager_real::options::OfflineMode::Offline
{
// And don't try to resolve DNS if it's an IP address.
Expand Down
2 changes: 1 addition & 1 deletion src/install/git_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ impl GitSubprocess {
/// Spawns `git <args>` with stdout and stderr captured. On `Ok` the child
/// may already have exited and freed `this`.
fn spawn(this: ThisPtr<Self>, args: &[&[u8]]) -> Result<(), Error> {
let env = GitEnv::get(this.manager().env_mut());
let env = GitEnv::get(&this.manager().process_env);
let Some(git) = &env.git else {
this.log_error(format_args!(
"\"git\" is not installed (needed for \"{}\")",
Expand Down
13 changes: 9 additions & 4 deletions src/install/repository.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,11 @@ impl SloppyGlobalGitConfig {
pub use bun_install_types::resolver_hooks::Repository;

/// The environment every `git` the install spawns runs with (see `GitEnv::get`).
///
/// Built from `PackageManager::process_env`, never from the loader that holds
/// the project's `.env*` files: git reads `GIT_SSH_COMMAND`, `GIT_EXEC_PATH`
/// and `GIT_CONFIG_*` from its environment and runs what they name, so a
/// cloned repository must not be able to set them for this install.
Comment thread
robobun marked this conversation as resolved.
Resolved
pub(crate) struct GitEnv {
/// `KEY=VALUE\0` array for spawn.
pub(crate) envp: bun_dotenv::NullDelimitedEnvMap,
Expand All @@ -147,21 +152,21 @@ pub(crate) struct GitEnv {
static GIT_ENV: bun_core::RacyCell<Option<GitEnv>> = bun_core::RacyCell::new(None);

impl GitEnv {
pub(crate) fn get(loader: &mut bun_dotenv::Loader) -> &'static GitEnv {
pub(crate) fn get(process_env: &bun_dotenv::Loader) -> &'static GitEnv {
let slot = GIT_ENV.get();
// SAFETY: only the install thread reaches this. The slot is written
// once, before any reference into it exists, and never reassigned.
unsafe {
if (*slot).is_none() {
*slot = Some(Self::init(loader));
*slot = Some(Self::init(process_env));
}
(*slot).as_ref().unwrap()
}
}

fn init(loader: &mut bun_dotenv::Loader) -> GitEnv {
fn init(process_env: &bun_dotenv::Loader) -> GitEnv {
// No prompts by default: the install's own output would hide them.
let mut map = bun_core::handle_oom(loader.map.clone_with_allocator());
let mut map = bun_core::handle_oom(process_env.map.clone_with_allocator());

if map.get(b"GIT_ASKPASS").is_none() {
let config = SloppyGlobalGitConfig::get();
Expand Down
194 changes: 192 additions & 2 deletions test/cli/install/bun-install-git-deps.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
// when an http URL is needed) or tarballs built in memory.
import { afterAll, beforeAll, expect, test } from "bun:test";
import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "fs";
import { bunEnv, bunExe, isLinux, isWindows, normalizeBunSnapshot, tempDir } from "harness";
import { bunEnv, bunExe, isLinux, isWindows, normalizeBunSnapshot, tempDir, tls } from "harness";
import { join } from "path";
import { pathToFileURL } from "url";

Expand Down Expand Up @@ -185,7 +185,13 @@ function writeProject(root: string, dependencies: Record<string, string>): strin
return project;
}

async function runInstall(cwd: string, cacheDir: string, extraEnv: Record<string, string>, ...args: string[]) {
// An `undefined` value in `extraEnv` removes that variable from the install's environment.
async function runInstall(
cwd: string,
cacheDir: string,
extraEnv: Record<string, string | undefined>,
...args: string[]
) {
const env = { ...gitEnv, ...extraEnv, BUN_INSTALL_CACHE_DIR: cacheDir };
// Set on ASAN CI lanes; it arms a subreaper around internal git spawns that
// SIGKILLs concurrent clone tasks (see #33982). This test exercises install
Expand Down Expand Up @@ -715,3 +721,187 @@ exit 1
}
},
);

// `bun install` loads the project's `.env*` files so that bunfig.toml can
// reference `$VARIABLES`. Those files come with the repository being installed,
// so they must not configure the install's own tooling: the git it spawns (git
// runs whatever `GIT_SSH_COMMAND`, `GIT_EXEC_PATH` or a `GIT_CONFIG_*` entry
// names) and the proxy and TLS settings of its HTTP client. Only the
// environment `bun install` was started with configures those.
//
// The variables a test sets through `.env` are removed from the install's real
// environment: a real one wins over `.env` in every build.
const noGitTooling = {
GIT_SSH: undefined,
GIT_SSH_COMMAND: undefined,
GIT_CONFIG_COUNT: undefined,
GIT_CONFIG_GLOBAL: undefined,
XDG_CONFIG_HOME: undefined,
};
const noProxy = {
http_proxy: undefined,
HTTP_PROXY: undefined,
https_proxy: undefined,
HTTPS_PROXY: undefined,
no_proxy: undefined,
NO_PROXY: undefined,
NODE_TLS_REJECT_UNAUTHORIZED: undefined,
};

// `<bun> <script> <args>` as git runs it: through `sh -c`. Forward slashes, so
// that the sh of Git for Windows takes the paths too.
function shCommand(script: string, ...args: string[]) {
return [bunExe(), script, ...args].map(arg => `"${arg.replaceAll("\\", "/")}"`).join(" ");
}

test.concurrent("a git config entry in the project's .env does not reach the git of the install", async () => {
using dir = tempDir("git-dep-dotenv-config", {
// git runs the `core.fsmonitor` command when it checks a commit out.
"fsmonitor.js": `require("fs").writeFileSync(process.argv[2], "");`,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
});
const root = String(dir);
const repoUrl = `git+${pathToFileURL(sharedBare)}`;
const project = writeProject(root, { [nameOf("b")]: `${repoUrl}#pkg-b` });
const { resolutions } = expectedGitPackages(repoUrl, sharedCommits, ["b"]);
const ran = join(root, "fsmonitor-ran");
writeFileSync(
join(project, ".env"),
[
"GIT_CONFIG_COUNT=1",
"GIT_CONFIG_KEY_0=core.fsmonitor",
`GIT_CONFIG_VALUE_0='${shCommand(join(root, "fsmonitor.js"), ran)}'`,
"",
].join("\n"),
);

const { stdout, stderr, exitCode } = await runInstall(project, join(root, "cache"), noGitTooling, "--ignore-scripts");
expect(existsSync(ran)).toBe(false);
expect(stderr).toContain("Saved lockfile");
expectInstalled(stdout, resolutions);
expect(await installedVersions(project, [nameOf("b")])).toEqual(markers(["b"]));
expect(exitCode).toBe(0);
});

test.concurrent("a GIT_SSH_COMMAND in the project's .env does not replace the user's ssh command", async () => {
using dir = tempDir("git-dep-dotenv-ssh", {
// Stands in for ssh: records that git ran it and fails, so that the clone ends there.
"ssh.js": `require("fs").writeFileSync(process.argv[2], ""); process.exit(255);`,
});
const root = String(dir);
const project = writeProject(root, { "pkg": "git+ssh://git@localhost/scope/pkg.git" });
const ranFromDotenv = join(root, "ran-from-dotenv");
const ranFromGitconfig = join(root, "ran-from-gitconfig");
writeFileSync(join(project, ".env"), `GIT_SSH_COMMAND='${shCommand(join(root, "ssh.js"), ranFromDotenv)}'\n`);
// The user's own ssh command, from their global git config.
const home = join(root, "home");
mkdirSync(home);
const sshCommand = shCommand(join(root, "ssh.js"), ranFromGitconfig).replaceAll('"', '\\"');
writeFileSync(join(home, ".gitconfig"), `[core]\n\tsshCommand = "${sshCommand}"\n`);

const { stderr, exitCode } = await runInstall(
project,
join(root, "cache"),
// The install tries the https form of the URL first. git refuses that
// protocol here, so nothing connects to port 443.
{ ...noGitTooling, HOME: home, USERPROFILE: home, GIT_ALLOW_PROTOCOL: "ssh" },
"--ignore-scripts",
);
expect({ ranFromDotenv: existsSync(ranFromDotenv), ranFromGitconfig: existsSync(ranFromGitconfig) }).toEqual({
ranFromDotenv: false,
ranFromGitconfig: true,
});
expect(stderr).toContain('"git clone" for "pkg" failed');
expect(exitCode).toBe(1);
});

test.concurrent(
"an HTTP_PROXY in the project's .env does not route the downloads of the install",
async () => {
const tarball = await tarballOf("package", packageFiles("leaf", "leaf"));
const direct: string[] = [];
await using server = Bun.serve({
port: 0,
fetch(req) {
direct.push(new URL(req.url).pathname);
return new Response(tarball);
},
});
// A forward proxy receives the absolute URL; this one answers the request itself.
const proxied: string[] = [];
await using proxy = Bun.serve({
port: 0,
fetch(req) {
proxied.push(req.url);
return new Response(tarball);
},
});
const proxyUrl = `http://127.0.0.1:${proxy.port}`;
const leafUrl = `http://127.0.0.1:${server.port}/leaf.tgz`;

using dir = tempDir("tarball-dep-dotenv-proxy", {});
const root = String(dir);
const project = writeProject(root, { leaf: leafUrl });
writeFileSync(join(project, ".env"), `HTTP_PROXY=${proxyUrl}\nhttp_proxy=${proxyUrl}\n`);

const fromDotenv = await runInstall(project, join(root, "cache-dotenv"), noProxy, "--ignore-scripts");
expect({ proxied, direct }).toEqual({ proxied: [], direct: ["/leaf.tgz"] });
expect(fromDotenv.stderr).toContain("Saved lockfile");
expectInstalled(fromDotenv.stdout, { leaf: leafUrl });
expect(fromDotenv.exitCode).toBe(0);

// The same value in the real environment is the user's own setting.
rmSync(join(project, "node_modules"), { recursive: true, force: true });
rmSync(join(project, "bun.lock"), { force: true });
direct.length = 0;
const fromRealEnv = await runInstall(
project,
join(root, "cache-real-env"),
{ ...noProxy, HTTP_PROXY: proxyUrl },
"--ignore-scripts",
);
expect({ proxied, direct }).toEqual({ proxied: [leafUrl], direct: [] });
expectInstalled(fromRealEnv.stdout, { leaf: leafUrl });
expect(fromRealEnv.exitCode).toBe(0);
},
30_000,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
);

test.concurrent(
"NODE_TLS_REJECT_UNAUTHORIZED=0 in the project's .env does not turn off certificate checks",
async () => {
const tarball = await tarballOf("package", packageFiles("leaf", "leaf"));
const downloads: string[] = [];
// `tls` is self-signed, so a client that checks certificates rejects it.
await using server = Bun.serve({
port: 0,
tls,
fetch(req) {
downloads.push(new URL(req.url).pathname);
return new Response(tarball);
},
});
const leafUrl = `https://localhost:${server.port}/leaf.tgz`;

using dir = tempDir("tarball-dep-dotenv-tls", {});
const root = String(dir);
const project = writeProject(root, { leaf: leafUrl });
writeFileSync(join(project, ".env"), "NODE_TLS_REJECT_UNAUTHORIZED=0\n");

const fromDotenv = await runInstall(project, join(root, "cache-dotenv"), noProxy, "--ignore-scripts");
expect(downloads).toEqual([]);
expect(fromDotenv.stderr).toContain("DEPTH_ZERO_SELF_SIGNED_CERT");
expect(fromDotenv.exitCode).toBe(1);

// The same value in the real environment is the user's own setting.
const fromRealEnv = await runInstall(
project,
join(root, "cache-real-env"),
{ ...noProxy, NODE_TLS_REJECT_UNAUTHORIZED: "0" },
"--ignore-scripts",
);
expect(downloads).toEqual(["/leaf.tgz"]);
expectInstalled(fromRealEnv.stdout, { leaf: leafUrl });
expect(fromRealEnv.exitCode).toBe(0);
},
30_000,
);
Loading