Repository navigation
install: make the security scanner child follow --no-env-file, --env-file and bunfig env = false #44845
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
install: make the security scanner child follow --no-env-file, --env-file and bunfig env = false #44845
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,9 @@ use bstr::BStr; | |
| // `bun_io::Loop` is the trait's nominal: `us_loop_t` on POSIX, `uv_loop_t` | ||
| // on Windows. The inherent `loop_()` projects `.uv_loop` from the uws wrapper | ||
| // on Windows so `BufferedReaderParent::loop_` returns the libuv loop directly. | ||
| use core::ffi::CStr; | ||
| use std::ffi::CString; | ||
|
|
||
| use crate::Error; | ||
| use crate::bun_fs::FileSystem; | ||
| use crate::bun_json::{Expr, ExprData}; | ||
|
|
@@ -29,14 +32,14 @@ use bun_ptr::{RefCount, RefPtr}; | |
| use bun_spawn::SpawnResultExt as _; | ||
| use bun_spawn::subprocess::{self, StdioResult}; | ||
| use bun_spawn::{ | ||
| self as spawn, Exited, Process, ProcessExit, ProcessExitKind, ProcessHandle, Rusage, | ||
| self as spawn, Exited, Process, ProcessExit, ProcessExitKind, ProcessHandle, Rusage, SpawnEnv, | ||
| SpawnOptions, Status, Stdio, | ||
| }; | ||
| use bun_sys::{self, Fd, FdExt as _}; | ||
|
|
||
| use crate::hoisted_install as HoistedInstall; | ||
| use crate::isolated_install as IsolatedInstall; | ||
| use crate::package_manager_real::package_manager_options::Do; | ||
| use crate::package_manager_real::package_manager_options::{Do, EnvFiles}; | ||
|
|
||
| #[inline] | ||
| fn signal_name(raw: u8) -> &'static str { | ||
|
|
@@ -255,6 +258,33 @@ pub fn perform_security_scan_for_all( | |
| ) | ||
| } | ||
|
|
||
| /// What the scanner child, a `bun -e` in the project root, is told about `.env` files. | ||
| enum ScannerEnv { | ||
| /// Nothing: it loads the default files itself. | ||
| Default, | ||
| /// `--no-env-file`. | ||
| NoEnvFile, | ||
| /// `--no-env-file`, and the environment with the `--env-file` values. | ||
| Resolved(bun_dotenv::NullDelimitedEnvMap), | ||
| } | ||
|
|
||
| impl ScannerEnv { | ||
| fn new(manager: &PackageManager) -> Result<Self, Error> { | ||
| Ok(match manager.options.env_files { | ||
| EnvFiles::Default => Self::Default, | ||
| EnvFiles::Disabled => Self::NoEnvFile, | ||
| EnvFiles::Explicit => { | ||
| let env = manager.env_mut(); | ||
| // `--env-file` copies the environment before bunfig `[run] noOrphans` sets this. | ||
| if bun_io::parent_death_watchdog::is_enabled() { | ||
| env.map.put(b"BUN_FEATURE_FLAG_NO_ORPHANS", b"1")?; | ||
| } | ||
| Self::Resolved(env.map.create_null_delimited_env_map()?) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| fn scan_installing_scanner_if_needed( | ||
| manager: &mut PackageManager, | ||
| security_scanner: &[u8], | ||
|
|
@@ -263,13 +293,17 @@ fn scan_installing_scanner_if_needed( | |
| command_ctx: CommandContext, | ||
| original_cwd: &[u8], | ||
| ) -> Result<Option<SecurityScanResults>, Error> { | ||
| // Built once: the partial install below can add script variables to the loader. | ||
| let env = ScannerEnv::new(manager)?; | ||
|
|
||
| let result = attempt_security_scan( | ||
| manager, | ||
| security_scanner, | ||
| scan_all, | ||
| seeds, | ||
| command_ctx, | ||
| original_cwd, | ||
| &env, | ||
| )?; | ||
|
|
||
| match result { | ||
|
|
@@ -288,6 +322,7 @@ fn scan_installing_scanner_if_needed( | |
| command_ctx, | ||
| original_cwd, | ||
| true, | ||
| &env, | ||
| )?; | ||
| match retry_result { | ||
| ScanAttemptResult::Success(scan_results) => Ok(Some(scan_results)), | ||
|
|
@@ -793,6 +828,7 @@ fn attempt_security_scan( | |
| seeds: &[PackageID], | ||
| command_ctx: CommandContext, | ||
| original_cwd: &[u8], | ||
| env: &ScannerEnv, | ||
| ) -> Result<ScanAttemptResult, Error> { | ||
| attempt_security_scan_with_retry( | ||
| manager, | ||
|
|
@@ -802,6 +838,7 @@ fn attempt_security_scan( | |
| command_ctx, | ||
| original_cwd, | ||
| false, | ||
| env, | ||
| ) | ||
| } | ||
|
|
||
|
|
@@ -813,6 +850,7 @@ fn attempt_security_scan_with_retry( | |
| command_ctx: CommandContext, | ||
| original_cwd: &[u8], | ||
| is_retry: bool, | ||
| env: &ScannerEnv, | ||
| ) -> Result<ScanAttemptResult, Error> { | ||
| if manager.options.log_level == crate::package_manager::Options::LogLevel::Verbose { | ||
| bun_core::pretty_errorln!( | ||
|
|
@@ -910,7 +948,7 @@ fn attempt_security_scan_with_retry( | |
| }); | ||
| // Cleanup of code/json_data/process handled by `Drop for SecurityScanSubprocess` when Box drops. | ||
|
|
||
| scanner.spawn()?; | ||
| scanner.spawn(env)?; | ||
|
|
||
| // `sleep_until` takes `*mut PackageManager` + `fn(&mut C) -> bool`; pass the | ||
| // boxed scanner as the closure context and a fn pointer that probes `is_done`. | ||
|
|
@@ -1004,7 +1042,7 @@ bun_io::impl_buffered_reader_parent! { | |
| } | ||
|
|
||
| impl<'a> SecurityScanSubprocess<'a> { | ||
| pub(crate) fn spawn(&mut self) -> Result<(), Error> { | ||
| fn spawn(&mut self, env: &ScannerEnv) -> Result<(), Error> { | ||
| self.ipc_data = Vec::new(); | ||
| let parent: *mut Self = self; | ||
| self.ipc_reader.set_parent(parent.cast()); | ||
|
|
@@ -1025,36 +1063,32 @@ impl<'a> SecurityScanSubprocess<'a> { | |
|
|
||
| let exec_path = bun_core::self_exe_path()?; | ||
|
|
||
| // Build | ||
| // owned NUL-terminated buffers so the pointers stay valid across the | ||
| // `spawn_process` FFI boundary. | ||
| let mut argv0_buf: Vec<u8> = exec_path.as_bytes().to_vec(); | ||
| argv0_buf.push(0); | ||
| let mut argv3_buf: Vec<u8> = self.code.to_vec(); | ||
| argv3_buf.push(0); | ||
| // Element type MUST be bare `*const c_char` (null sentinel), never | ||
| // `Option<*const c_char>`: raw pointers are already nullable, and | ||
| // `Option<*const T>` is a 2-word (tag, ptr) pair — casting that to | ||
| // `Argv` interleaves discriminant words and EFAULTs in the kernel. | ||
| let mut argv: [*const core::ffi::c_char; 5] = [ | ||
| argv0_buf.as_ptr().cast(), | ||
| c"--no-install".as_ptr(), | ||
| c"-e".as_ptr(), | ||
| argv3_buf.as_ptr().cast(), | ||
| core::ptr::null(), | ||
| ]; | ||
| const _: () = assert!( | ||
| core::mem::size_of::<[*const core::ffi::c_char; 5]>() | ||
| == 5 * core::mem::size_of::<usize>() | ||
| ); | ||
| let code = CString::new(&*self.code).map_err(|_| crate::Error::InvalidCharacter)?; | ||
| let mut argv: Vec<&CStr> = Vec::with_capacity(5); | ||
| argv.push(exec_path.as_cstr()); | ||
| argv.push(c"--no-install"); | ||
| if !matches!(env, ScannerEnv::Default) { | ||
| argv.push(c"--no-env-file"); | ||
|
Contributor
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. 🟣 pre-existing, not blocking: CI users who steer bun with Why this was flaggedTrigger: Verification: Pre-existing; acknowledged in diff: the |
||
| } | ||
| argv.push(c"-e"); | ||
| argv.push(&code); | ||
|
|
||
| let resolved: Vec<&CStr>; | ||
| let env = match env { | ||
| ScannerEnv::Default | ScannerEnv::NoEnvFile => SpawnEnv::Inherit, | ||
| ScannerEnv::Resolved(envp) => { | ||
| resolved = envp.iter().collect(); | ||
| SpawnEnv::Strings(&resolved) | ||
| } | ||
| }; | ||
|
|
||
| #[cfg(windows)] | ||
| { | ||
| self.spawn_windows(&mut argv, ipc_output_fds)?; | ||
| self.spawn_windows(&argv, env, ipc_output_fds)?; | ||
| } | ||
| #[cfg(not(windows))] | ||
| { | ||
| self.spawn_posix(&mut argv, ipc_output_fds)?; | ||
| self.spawn_posix(&argv, env, ipc_output_fds)?; | ||
| } | ||
|
|
||
| Ok(()) | ||
|
|
@@ -1067,7 +1101,8 @@ impl<'a> SecurityScanSubprocess<'a> { | |
| #[cfg(unix)] | ||
| fn spawn_posix( | ||
| &mut self, | ||
| argv: &mut [*const core::ffi::c_char; 5], | ||
| argv: &[&CStr], | ||
| env: SpawnEnv<'_>, | ||
| ipc_output_fds: [Fd; 2], | ||
| ) -> Result<(), Error> { | ||
| let extra_fds: Box<[Stdio]> = Box::new([ | ||
|
|
@@ -1084,16 +1119,8 @@ impl<'a> SecurityScanSubprocess<'a> { | |
| ..Default::default() | ||
| }; | ||
|
|
||
| // SAFETY: `argv` is a local null-terminated C-string array with a | ||
| // non-null argv[0]; `environ_ptr()` is the process environ block. | ||
| let mut spawned = unsafe { | ||
| spawn::spawn_process( | ||
| &spawn_options, | ||
| argv.as_mut_ptr().cast(), | ||
| bun_sys::environ_ptr(), | ||
| ) | ||
| }? | ||
| .map_err(|e| e.to_zig_err())?; | ||
| let mut spawned = | ||
| spawn::spawn_process_cstr(&spawn_options, argv, env)?.map_err(|e| e.to_zig_err())?; | ||
| // `defer spawned.extra_pipes.deinit()` — drops at scope exit. | ||
|
|
||
| ipc_output_fds[1].close(); | ||
|
|
@@ -1117,7 +1144,8 @@ impl<'a> SecurityScanSubprocess<'a> { | |
| #[cfg(windows)] | ||
| fn spawn_windows( | ||
| &mut self, | ||
| argv: &mut [*const core::ffi::c_char; 5], | ||
| argv: &[&CStr], | ||
| env: SpawnEnv<'_>, | ||
| ipc_output_fds: [Fd; 2], | ||
| ) -> Result<(), Error> { | ||
| use bun_sys::ReturnCodeExt as _; | ||
|
|
@@ -1196,16 +1224,8 @@ impl<'a> SecurityScanSubprocess<'a> { | |
| ..Default::default() | ||
| }; | ||
|
|
||
| // SAFETY: `argv` is a local null-terminated C-string array with a | ||
| // non-null argv[0]; `environ_ptr()` is the process environ block. | ||
| let mut spawned = unsafe { | ||
| spawn::spawn_process( | ||
| &spawn_options, | ||
| argv.as_mut_ptr().cast(), | ||
| bun_sys::environ_ptr(), | ||
| ) | ||
| }? | ||
| .map_err(|e| e.to_zig_err())?; | ||
| let mut spawned = | ||
| spawn::spawn_process_cstr(&spawn_options, argv, env)?.map_err(|e| e.to_zig_err())?; | ||
| // `defer spawned.extra_pipes.deinit()` — drops at scope exit. | ||
|
|
||
| ipc_output_fds[1].close(); | ||
|
|
||
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.
🟡 (optional) Users of
bun install --env-filewith a scanner and one large env value now see the whole install abort withsecurity scanner failed: E2BIGwhere the base install completed. security_scanner.rs:282 snapshots the entire loader map into envp and :1081 passes it as SpawnEnv::Strings, so a single KEY=VALUE over Linux MAX_ARG_STRLEN (128 KiB) makes execve fail. The base child inherited environ and loaded .env itself, so the value never reached execve. Fix: do not turn the scan into a hard install failure on an oversized env entry — skip or truncate the oversized value with a warning, or fall back to Inherit plus--env-fileon the child argv.Why this was flagged
Trigger:
bun install --env-file=ci.env(or add/update/remove/pm scan) with[install.security] scannerconfigured and ci.env containing a value longer than MAX_ARG_STRLEN (128 KiB on Linux), e.g. a base64 blob or inlined key material; the author's own measurement shows a 200,000-byte value fails. ScannerEnv::new at security_scanner.rs:276-282 calls create_null_delimited_env_map() over the full loader map and spawn at :1079-1081 hands it to spawn_process_cstr as SpawnEnv::Strings, so execve returns E2BIG and attempt_security_scan_with_retry propagates it as an install error. On the base the scanner was spawned with the inherited environ and the oversized value stayed inside the parent's loader, so the install succeeded. The dismissal argued lifecycle scripts already fail on the same map (PackageManagerLifecycle.rs:399), but that only applies to projects that run lifecycle scripts; projects with--ignore-scriptsor no scripts newly fail. Remedy: degrade quietly (warn and drop the oversized entry) instead of aborting the install.Verification: Trigger:
bun installrun with--env-file=<file>where the file holds one KEY=VALUE over 128 KiB and[install.security] scanneris configured. security_scanner.rs:276-282 snapshots every loader entry; :1077-1083 passes it asSpawnEnv::Stringsto execve, which fails with E2BIG, and install_with_manager.rs:2135-2143 callsGlobal::exit(1), aborting the whole install. The base passedbun_sys::environ_ptr(), so the install completed. No test pins it.