refactor(deps): split libkrunfw sidecar package#962
Conversation
📦 BoxLite review — couldn't completepowered by BoxLite |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (80)
💤 Files with no reviewable changes (12)
🚧 Files skipped from review as they are similar to previous changes (40)
📝 WalkthroughWalkthroughThis PR moves native runtime asset builds outside Cargo, switches boxlite to direct libkrun integration, adds gvproxy DNS filtering and TLS MITM support, and updates C SDK packaging and examples to use static archives with revised macOS library-path handling. ChangesRuntime assets and libkrun integration
gvproxy bridge networking and MITM
C SDK static linking and packaging
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/workflows/build-c.ymlTraceback (most recent call last): .github/workflows/build-runtime.ymlTraceback (most recent call last): .github/workflows/lint.ymlTraceback (most recent call last):
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors BoxLite’s dependency graph to remove the libkrun-sys forwarding facade, introducing libkrunfw as a standalone sidecar-preparation crate while having BoxLite depend directly on the local forked Rust libkrun crate. This aligns with the goal of making future libkrun-side work (VMM/virtiofs/seccomp changes) reachable without another forwarding layer.
Changes:
- Introduce new
src/deps/libkrunfwcrate to prepare/bundle libkrunfw sidecar artifacts (and optionallibkrunfw.binkernel artifact). - Replace
libkrun-sysusage by switching BoxLite’s krun integration to call the Rustlibkruncrate directly, and adjust feature plumbing (krunfw-kernel*) across CLI/shim. - Update build + packaging scripts/config for musl/static builds, runtime discovery, and Go SDK link flags.
Reviewed changes
Copilot reviewed 39 out of 43 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shim/Cargo.toml | Adds feature passthroughs for krunfw-kernel* to the shim crate. |
| src/shim/build.rs | Removes Linux linker workaround previously needed for the staticlib-based libkrun path. |
| src/shim/.cargo/config.toml | Adds shim-scoped Linux static-linking rustflags. |
| src/guest/.cargo/config.toml | Consolidates musl rustflags under cfg(target_env = "musl"). |
| src/deps/libkrunfw/src/lib.rs | New crate: documents purpose and emitted build metadata for runtime bundling. |
| src/deps/libkrunfw/Cargo.toml | New crate manifest: defines features for libkrunfw sidecars and optional kernel artifact. |
| src/deps/libkrunfw/build.rs | New build orchestration for downloading/building/fixing libkrunfw sidecar artifacts. |
| src/deps/libkrun-sys/src/lib.rs | Removes the old libkrun FFI facade crate. |
| src/deps/libkrun-sys/Cargo.toml | Removes the old libkrun-sys crate manifest. |
| src/deps/libkrun-sys/build.rs | Removes the old libkrun-sys build orchestration. |
| src/deps/libgvproxy-sys/build.rs | Tightens Linux libresolv static-path handling to glibc-only targets. |
| src/cli/Cargo.toml | Adds CLI feature passthroughs for krunfw-kernel*. |
| src/boxlite/src/vmm/krun/engine.rs | Adds runtime-configurable external-kernel selection and calls into krun_set_kernel. |
| src/boxlite/src/vmm/krun/context.rs | Switches from libkrun_sys to libkrun and adjusts constants/logging handling. |
| src/boxlite/src/vmm/krun/constants.rs | Adds missing ABI constants not re-exported by the Rust libkrun crate. |
| src/boxlite/src/vmm/controller/spawn.rs | Propagates new krunfw-kernel env vars into the shim process. |
| src/boxlite/src/util/mod.rs | Treats BOXLITE_RUNTIME_DIR as explicit override over embedded runtime and propagates it. |
| src/boxlite/src/util/binary_finder.rs | Uses platform-aware split_paths and respects explicit runtime override over embedded runtime. |
| src/boxlite/src/system_check.rs | Updates Hypervisor.framework linkage commentary to match direct libkrun dependency. |
| src/boxlite/src/net/socket_path.rs | Updates vendored libkrun source path used by a test to the new submodule location. |
| src/boxlite/src/jailer/shim_copy.rs | Copies libkrunfw.* (including optional .bin) alongside the shim for sandboxed execution. |
| src/boxlite/src/jailer/sandbox/bwrap.rs | Propagates krunfw-kernel env vars into sandboxed execution. |
| src/boxlite/src/jailer/common/rlimit.rs | Fixes getrlimit/setrlimit resource arg typing for musl vs glibc. |
| src/boxlite/src/jailer/bwrap.rs | Propagates krunfw-kernel env vars into bwrap execution path. |
| src/boxlite/deps/README.md | Updates dependency-crate docs to reflect libkrunfw replacing libkrun-sys. |
| src/boxlite/Cargo.toml | Replaces libkrun-sys with libkrunfw + libkrun, adds krunfw-kernel* features. |
| src/boxlite/build.rs | Renames/clarifies dependency bundling terminology and improves shim path discovery; removes old linker workaround. |
| sdks/go/bridge_cgo_prebuilt.go | Adds -lunwind for Linux cgo linking. |
| sdks/go/bridge_cgo_dev.go | Adds -lunwind for Linux cgo linking (dev path). |
| scripts/test/e2e/bootstrap.sh | Updates build-dependency docs to reference libkrunfw instead of libkrun-sys. |
| scripts/common.sh | Adds helper to translate legacy env var into feature args for kernel builds. |
| scripts/build/build-shim.sh | Updates Linux shim build to choose target based on host toolchain and apply feature args. |
| scripts/build/build-runtime.sh | Applies feature args when building the runtime collection artifact. |
| scripts/build/build-libseccomp.sh | Uses native CC when host toolchain matches target; improves musl toolchain handling. |
| scripts/build/build-guest.sh | Improves native-musl handling and builds from src/guest so guest-scoped cargo config is applied. |
| Cargo.toml | Removes libkrun-sys, adds libkrunfw and direct libkrun workspace dependency. |
| Cargo.lock | Updates lockfile for the new dependency graph including libkrun and its transitive deps. |
| .gitmodules | Moves libkrun submodule path and relocates libkrunfw submodule under the new crate. |
| .gitignore | Updates ignored submodule target paths to new locations. |
| .github/workflows/build-runtime.yml | Removes publishing of libkrun-sys (but needs publishing updates for libkrunfw). |
| .cargo/config.toml | Reworks musl rustflags and adds musl_v1_2_3 cfg flag. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/deps/libkrunfw/build.rs (1)
121-136: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a download timeout to
curl.
curl -fsSLhas no--max-time/--connect-timeout, so a stalled network mid-build can hang the build script (and CI) indefinitely.🕒 Proposed fix
- let output = Command::new("curl") - .args(["-fsSL", "-o", dest.to_str().unwrap(), url]) - .output()?; + let output = Command::new("curl") + .args(["-fsSL", "--connect-timeout", "10", "--max-time", "300", "-o", dest.to_str().unwrap(), url]) + .output()?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/deps/libkrunfw/build.rs` around lines 121 - 136, The download helper in build.rs uses curl without any timeout, so stalled transfers can block the build indefinitely. Update the download(url, dest) command in the download function to pass appropriate curl timeout flags (for both overall transfer and connection setup) alongside the existing -fsSL options, so libkrunfw builds fail fast instead of hanging.scripts/build/build-guest.sh (1)
81-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
rustc -vVhost parsing into a shared helper.The
rustc -vV | while read ...pattern to extract thehost:triple is duplicated incheck_prerequisites(lines 82–88),setup_rust_target(lines 103–109),build-libseccomp.sh:ensure_libseccomp_for_target(lines 121–127), andbuild-shim.sh(per the context snippet). Extracting aget_rustc_hosthelper intoscripts/common.shorscripts/build/util.shwould eliminate four copies and ensure consistent parsing.♻️ Proposed refactor
# In scripts/common.sh or scripts/build/util.sh: +get_rustc_host() { + rustc -vV | while read -r key value; do + if [ "$key" = "host:" ]; then + printf '%s\n' "$value" + break + fi + done +} # In scripts/build/build-guest.sh check_prerequisites: - local rustc_host - rustc_host=$(rustc -vV | while read -r key value; do - if [ "$key" = "host:" ]; then - printf '%s\n' "$value" - break - fi - done) + local rustc_host + rustc_host=$(get_rustc_host) # Similarly in setup_rust_target, build-libseccomp.sh, and build-shim.shAlso applies to: 103-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build/build-guest.sh` around lines 81 - 94, Extract the repeated rustc host-triple parsing into a shared helper so the same logic is not duplicated across build scripts. Move the rustc -vV host extraction currently in check_prerequisites, setup_rust_target, ensure_libseccomp_for_target, and the build-shim.sh equivalent into a reusable get_rustc_host helper in scripts/common.sh or scripts/build/util.sh, then update each caller to use that helper and keep the musl check behavior unchanged.src/boxlite/src/vmm/controller/spawn.rs (1)
151-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated KRUNFW env-var forwarding list into a shared helper.
The exact same 3-item array + forwarding loop is repeated verbatim in
jailer/bwrap.rs::build_shim_commandandjailer/sandbox/bwrap.rs::apply. Consider a shared constant (e.g.crate::vmm::krun::KRUNFW_ENV_VARS) plus a small helper (forward_env_vars(cmd, &KRUNFW_ENV_VARS)) used by all three call sites, so adding/removing a variable only needs one edit.♻️ Suggested shared helper
// e.g. in src/boxlite/src/util/mod.rs pub const KRUNFW_ENV_VARS: [&str; 3] = [ "BOXLITE_KRUNFW_EXTERNAL_KERNEL", "BOXLITE_KRUNFW_KERNEL_PATH", "BOXLITE_KRUNFW_KERNEL_FORMAT", ];Then at each call site:
for name in util::KRUNFW_ENV_VARS { ... }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/boxlite/src/vmm/controller/spawn.rs` around lines 151 - 159, Extract the repeated BOXLITE_KRUNFW env forwarding list into a shared helper so the same 3-variable array is not duplicated across spawn.rs, jailer/bwrap.rs::build_shim_command, and jailer/sandbox/bwrap.rs::apply. Add a shared constant like KRUNFW_ENV_VARS and a small forwarding helper (for example forward_env_vars) in a common module, then update the existing loop in the spawn path to use that shared source so future changes only require one edit.src/boxlite/src/vmm/krun/context.rs (1)
39-84: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUpdate
init_loggingdocs to match the current no-op behavior.
The function no longer initializes libkrun; it only resolves a level fromRUST_LOGand emits a tracing line. Rename/reword the doc comment to reflect that it’s a compatibility shim, and drop the stale “must be called before creating any context” wording.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/boxlite/src/vmm/krun/context.rs` around lines 39 - 84, The doc comment on init_logging is stale and still describes real initialization behavior that no longer happens. Reword it to explain that init_logging is now a compatibility shim that only inspects RUST_LOG and records a tracing message, and remove the “must be called before creating any context” claim. Keep the description aligned with the current behavior in init_logging and the surrounding krun context setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/build-guest.sh`:
- Around line 157-183: The static PIE check in the guest build script can
falsely report success when readelf fails, because the current command chain
treats a readelf error like “no NEEDED entries.” Update the verification logic
around the guest binary check in build-guest.sh so readelf’s exit status is
validated explicitly before interpreting its output, and only print the “static
PIE” message when readelf succeeds and no NEEDED dependencies are present. Keep
the existing guest_binary, file_output, and musl_gcc flow, but separate the
readelf invocation from the grep-based test and fail closed on any readelf
error.
In `@scripts/build/build-runtime.sh`:
- Line 241: The runtime build pipeline in build-runtime.sh currently suppresses
cargo failures in the boxlite build path and then falls back to the newest
OUT_DIR, which can hide a broken feature-enabled build. Update the runtime_src
lookup around the cargo build invocation so it fails fast when the build with
feature_args or krunfw-kernel support fails, instead of continuing to stale
output selection; keep the logic tied to the runtime build parsing flow that
populates runtime_src.
In `@sdks/go/bridge_cgo_prebuilt.go`:
- Line 16: The Linux cgo link flags in bridge_cgo_prebuilt.go add a hard
dependency on libunwind, so prebuilt consumers can fail to link on systems
without it. Update the prebuilt SDK setup to either bundle/provide libunwind
with the archive or explicitly document it as a required prerequisite, and make
sure the cgo link configuration in bridge_cgo_prebuilt.go reflects that choice.
In `@src/boxlite/src/vmm/krun/engine.rs`:
- Around line 231-256: The explicit BOXLITE_KRUNFW_KERNEL_PATH is being ignored
by find_krunfw_kernel_path because the BOXLITE_KRUNFW_EXTERNAL_KERNEL=false
check runs first; update the control flow so the explicit path lookup and
validation happen before short-circuiting on the disabled toggle. Keep the
existing metadata/file checks in find_krunfw_kernel_path, but make sure a set
ENV_KRUNFW_KERNEL_PATH is honored or at least surfaced instead of silently
returning None when krunfw_kernel_env() yields Some(false).
---
Nitpick comments:
In `@scripts/build/build-guest.sh`:
- Around line 81-94: Extract the repeated rustc host-triple parsing into a
shared helper so the same logic is not duplicated across build scripts. Move the
rustc -vV host extraction currently in check_prerequisites, setup_rust_target,
ensure_libseccomp_for_target, and the build-shim.sh equivalent into a reusable
get_rustc_host helper in scripts/common.sh or scripts/build/util.sh, then update
each caller to use that helper and keep the musl check behavior unchanged.
In `@src/boxlite/src/vmm/controller/spawn.rs`:
- Around line 151-159: Extract the repeated BOXLITE_KRUNFW env forwarding list
into a shared helper so the same 3-variable array is not duplicated across
spawn.rs, jailer/bwrap.rs::build_shim_command, and
jailer/sandbox/bwrap.rs::apply. Add a shared constant like KRUNFW_ENV_VARS and a
small forwarding helper (for example forward_env_vars) in a common module, then
update the existing loop in the spawn path to use that shared source so future
changes only require one edit.
In `@src/boxlite/src/vmm/krun/context.rs`:
- Around line 39-84: The doc comment on init_logging is stale and still
describes real initialization behavior that no longer happens. Reword it to
explain that init_logging is now a compatibility shim that only inspects
RUST_LOG and records a tracing message, and remove the “must be called before
creating any context” claim. Keep the description aligned with the current
behavior in init_logging and the surrounding krun context setup.
In `@src/deps/libkrunfw/build.rs`:
- Around line 121-136: The download helper in build.rs uses curl without any
timeout, so stalled transfers can block the build indefinitely. Update the
download(url, dest) command in the download function to pass appropriate curl
timeout flags (for both overall transfer and connection setup) alongside the
existing -fsSL options, so libkrunfw builds fail fast instead of hanging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 171e881c-aeee-4ffc-8041-e952570195d3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
.cargo/config.toml.github/workflows/build-runtime.yml.gitignore.gitmodulesCargo.tomlscripts/build/build-guest.shscripts/build/build-libseccomp.shscripts/build/build-runtime.shscripts/build/build-shim.shscripts/common.shscripts/test/e2e/bootstrap.shsdks/go/bridge_cgo_dev.gosdks/go/bridge_cgo_prebuilt.gosrc/boxlite/Cargo.tomlsrc/boxlite/build.rssrc/boxlite/deps/README.mdsrc/boxlite/src/jailer/bwrap.rssrc/boxlite/src/jailer/common/rlimit.rssrc/boxlite/src/jailer/sandbox/bwrap.rssrc/boxlite/src/jailer/shim_copy.rssrc/boxlite/src/net/socket_path.rssrc/boxlite/src/system_check.rssrc/boxlite/src/util/binary_finder.rssrc/boxlite/src/util/mod.rssrc/boxlite/src/vmm/controller/spawn.rssrc/boxlite/src/vmm/krun/constants.rssrc/boxlite/src/vmm/krun/context.rssrc/boxlite/src/vmm/krun/engine.rssrc/cli/Cargo.tomlsrc/deps/libgvproxy-sys/build.rssrc/deps/libkrunsrc/deps/libkrun-sys/Cargo.tomlsrc/deps/libkrun-sys/build.rssrc/deps/libkrun-sys/src/lib.rssrc/deps/libkrunfw/Cargo.tomlsrc/deps/libkrunfw/build.rssrc/deps/libkrunfw/src/lib.rssrc/deps/libkrunfw/vendor/libkrunfwsrc/guest/.cargo/config.tomlsrc/shim/.cargo/config.tomlsrc/shim/Cargo.tomlsrc/shim/build.rs
💤 Files with no reviewable changes (5)
- src/deps/libkrun-sys/Cargo.toml
- .github/workflows/build-runtime.yml
- src/deps/libkrun-sys/src/lib.rs
- src/shim/build.rs
- src/deps/libkrun-sys/build.rs
## Summary Fixes the static-musl shim build by linking vendored `libkrun` as a normal Cargo dependency instead of building a nested Rust `staticlib`. This removes duplicate Rust standard-library symbols and the need for `--allow-multiple-definition` while keeping the Linux shim statically linked. Aligns libkrunfw packaging with the final link mode. Dynamic builds package the shared library for libkrun's `dlopen` path; static-musl builds package the raw `libkrunfw.bin` kernel and configure it through `krun_set_kernel`. Improves native musl support across the shim, guest, runtime assembly, and local SDK development paths. Ref #927. ## Changes - Replace the nested `cargo rustc --crate-type staticlib` libkrun build with a Cargo-managed dependency on the vendored Rust crate. - Remove duplicate-symbol linker workarounds and obsolete libkrun logging initialization. The vendored crate now shares the shim's Rust runtime and process-wide logger. - Split libkrunfw artifact behavior by link mode: - dynamic Linux and macOS builds package `libkrunfw.so*` or `libkrunfw.dylib`; - static-musl builds always build and package `libkrunfw.bin`, including when only the base `krunfw` feature is enabled; - `krunfw-source` builds firmware from the vendored source, with shared libraries omitted for static-musl. - Add runtime external-kernel configuration with `BOXLITE_KRUNFW_EXTERNAL_KERNEL`, `BOXLITE_KRUNFW_KERNEL_PATH`, and `BOXLITE_KRUNFW_KERNEL_FORMAT`. - External-kernel loading defaults to off for dynamic builds and on for static-musl. - An explicit gate value overrides the platform default. - Kernel format defaults to ELF on x86_64 and RAW on other architectures; explicit formats are `raw`, `elf`, `pe-gz`, `image-bz2`, `image-gz`, and `image-zstd`. - Bundle and copy the selected libkrunfw artifact alongside the shim, and pass the external-kernel controls through the sanitized shim environment. - Scope `crt-static` flags to the shim and guest Cargo configurations so they do not leak into SDK cdylibs or host-built proc macros. - Use the native musl Rust target when available, while retaining cross-compilation support for GNU hosts. - Centralize build-script path, target-directory, and Cargo-profile handling. Build profiles now flow through `BUILD_PROFILE`; `make runtime` and `make cli` default to release, with `BUILD_PROFILE=debug` selecting debug builds. - Fix related musl portability issues: resource-limit types, GNU-only `libresolv` search paths, libseccomp cross-build handling, target/profile runtime discovery, and Go/cgo unwind linkage. - Temporarily skip publishing `libkrun-sys` from the release workflow because its vendored path dependency requires a follow-up packaging fix; #962 will address this. ## How to verify ```bash bash -n scripts/build/build-cli.sh bash -n scripts/build/build-guest.sh bash -n scripts/build/build-libseccomp.sh bash -n scripts/build/build-runtime.sh bash -n scripts/build/build-shim.sh make fmt:check:rust git diff --check BOXLITE_DEPS_STUB=1 cargo check -p boxlite --tests --features krun BOXLITE_DEPS_STUB=1 cargo check -p libkrun-sys --features krun,krunfw BOXLITE_DEPS_STUB=1 cargo test -p boxlite --lib vmm::krun::kernel::tests --features krun ``` For an end-to-end static-musl build after `make setup:build`: ```bash make runtime BUILD_PROFILE=debug make runtime ``` The assembled static-musl runtime should contain `libkrunfw.bin` and no `libkrunfw.so*`. Dynamic Linux and macOS runtimes should continue to package the shared libkrunfw artifact. `libkrun-sys` is intentionally omitted from the crates.io publish step for this PR; the packaging issue is tracked in #962.
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/boxlite/src/jailer/bwrap.rs (1)
605-615: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAlways include the shim directory in
LD_LIBRARY_PATH.Any inherited value—even empty or pointing only to paths unavailable inside the sandbox—suppresses the fallback. Prepend
shim_path.parent()to the inherited path so bundled libkrun dependencies remain resolvable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/boxlite/src/jailer/bwrap.rs` around lines 605 - 615, Update the LD_LIBRARY_PATH setup in the bwrap configuration to always prepend shim_path.parent() to any inherited LD_LIBRARY_PATH, including empty values, instead of using the inherited value as an exclusive branch. Preserve the shim directory as the fallback when no environment value exists, and retain the existing sandbox environment assignment and debug logging.
🧹 Nitpick comments (4)
.github/workflows/lint.yml (2)
119-122: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable credential persistence in checkout action.
Apply the same security best practice here by disabling credential persistence.
🛡️ Proposed fix
- name: Checkout code uses: actions/checkout@v5 with: submodules: recursive + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/lint.yml around lines 119 - 122, Update the actions/checkout step to disable credential persistence by setting its persist-credentials option to false alongside the existing recursive submodules configuration.Source: Linters/SAST tools
94-97: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable credential persistence in checkout action.
To improve security and prevent potential credential leakage in the CI environment, it's recommended to set
persist-credentials: falsewhen usingactions/checkout, unless the workflow specifically needs to push changes back to the repository.🛡️ Proposed fix
- name: Checkout code uses: actions/checkout@v5 with: submodules: recursive + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/lint.yml around lines 94 - 97, Update the actions/checkout step to disable credential persistence by setting persist-credentials to false, while preserving the existing recursive submodules configuration.Source: Linters/SAST tools
.github/workflows/test.yml (1)
57-58: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable credential persistence in
actions/checkoutto improve security posture.By default,
actions/checkoutpersists the GitHub token in the local git config, which can inadvertently leak credentials if a subsequent step executes untrusted code or creates artifacts containing the.gitdirectory. Since none of these test jobs push commits back to the repository, you should disable credential persistence across all checkout steps in this workflow.
.github/workflows/test.yml#L57-L58: Addpersist-credentials: falsealongside thesubmodulesparameter..github/workflows/test.yml#L98-L99: Addpersist-credentials: falsealongside thesubmodulesparameter..github/workflows/test.yml#L146-L147: Addpersist-credentials: falsealongside thesubmodulesparameter..github/workflows/test.yml#L196-L197: Addpersist-credentials: falsealongside thesubmodulesparameter..github/workflows/test.yml#L228-L229: Addpersist-credentials: falsealongside thesubmodulesparameter..github/workflows/test.yml#L258-L259: Addpersist-credentials: falsealongside thesubmodulesparameter.🛡️ Proposed fixes
For each instance of
actions/checkout@v5in this workflow, update the configuration like so:- uses: actions/checkout@v5 with: submodules: recursive + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 57 - 58, Disable credential persistence for every actions/checkout@v5 step by adding persist-credentials: false alongside submodules in .github/workflows/test.yml at lines 57-58, 98-99, 146-147, 196-197, 228-229, and 258-259.Source: Linters/SAST tools
src/libgvproxy-sys/gvproxy-bridge/gvproxy_create_latency_test.go (1)
23-59: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid a sub-millisecond wall-clock gate in unit-test CI.
The 500µs threshold is hardware- and scheduler-dependent, so slower shared runners can fail without a regression. Move this to a benchmark/performance job or use a substantially environment-tolerant budget.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/gvproxy_create_latency_test.go` around lines 23 - 59, The TestVirtualNetworkNewWithinLatencyBudget test uses an environment-sensitive 500µs wall-clock failure threshold. Remove the strict unit-test gate by converting this measurement to a benchmark/performance-only check, or replace it with a substantially more tolerant budget while preserving the warmup, sampling, and diagnostic logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libgvproxy-sys/Cargo.toml`:
- Line 27: Replace the public skip-native-build feature in the crate feature
definitions with a positive native-build feature enabled by default, keeping the
native-build opt-out internal to the build logic so transitive dependencies and
--all-features cannot suppress rustc-link-* directives. Update the related
clippy invocation to use the new feature name and behavior.
In `@src/libgvproxy-sys/gvproxy-bridge/dns_filter_test.go`:
- Around line 29-44: Update
TestBuildAllowNetDNSZones_PerTLDZonesHaveSinkholeDefaultIP and the
buildAllowNetDNSZones call path to accept an injectable DNS lookup/resolver,
then provide a test stub returning fixed IPv4 records for example.com. Ensure
the test no longer performs public DNS resolution while preserving its existing
zone-count and sinkhole DefaultIP assertions.
In `@src/libgvproxy-sys/gvproxy-bridge/dns_filter.go`:
- Around line 99-109: Update resolveAndAddRecords to perform the hostname lookup
with a bounded context instead of context.Background(), applying a finite
timeout per DNS rule and ensuring the context is cancelled after the lookup.
Preserve the existing warning and early-return behavior when resolution times
out or otherwise fails.
- Around line 72-83: Sort the zones generated from zoneRecords by descending
zone-name length, using the zone name as a deterministic tie-breaker, before
appending the root zone. Update the zone construction flow around the zones
slice and preserve the existing first-match-wins behavior and logging.
In `@src/libgvproxy-sys/gvproxy-bridge/forked_network.go`:
- Around line 37-45: Update TestStackFieldExists to verify that
VirtualNetwork.stack has the exact *stack.Stack type and is non-nil before any
unsafe conversion. Keep the existing missing-field validation, and ensure the
test fails fast before the stackField.Pointer cast when the upstream API
changes.
In `@src/libgvproxy-sys/gvproxy-bridge/forked_tcp_test.go`:
- Around line 77-120: Update TestMitmRouting_NonSecretHostPassthrough and
TestMitmRouting_SecretHostPort80_NoMitm to call decideTCPRoute directly and
assert non-secret traffic and secret-host port 80 return
tcpRouteStandardForward. Also assert that the secret host on port 443 returns
tcpRouteInspect, so the tests verify routing decisions rather than only matcher
behavior.
In `@src/libgvproxy-sys/gvproxy-bridge/forked_tcp.go`:
- Around line 191-219: Validate that the pre-NAT destination in the TCP bridge
belongs to the inspected hostname’s resolved addresses before the MITM branch
accesses SecretsForHost, and before any passthrough dialing. Reject and close
guestConn when the hostname-to-destination binding fails; otherwise continue
using the validated destination for MITM or net.Dial, preserving allowlist
behavior.
- Around line 134-189: Bound both connection setup and header inspection in
standardForward and inspectAndForward: replace net.Dial with a net.Dialer
configured with the project’s appropriate timeout, set a temporary read deadline
on guestConn before peekClientHelloSNI or peekHTTPHost, and clear the deadline
after inspection succeeds before handing the connection to relay. Apply the same
dial timeout to the outbound connection path later in inspectAndForward,
preserving existing error and cleanup behavior.
In `@src/libgvproxy-sys/gvproxy-bridge/main.go`:
- Around line 593-610: Update GvproxyInstance with a teardown completion
channel, close it at the end of the cleanup goroutine after removing resources,
and in gvproxy_destroy call instance.Cancel() then wait on that channel before
returning. Preserve the existing missing-instance error path and successful
destruction logging.
- Around line 354-400: Move the fallible NewBoxCAFromPEM validation block before
the platform-specific socket creation in the surrounding initialization flow,
while preserving its existing error reporting and return behavior. Ensure
CA/config validation completes before either transport.ListenUnixgram or
net.Listen is called, and retain the parsed CA for subsequent setup without
creating sockets on validation failure.
- Around line 445-490: Move the successful initErr notification until after
OverrideTCPHandler and the optional control-socket listener/server setup
complete. If OverrideTCPHandler fails, or net.Listen fails for
config.ControlSocketPath, send that error through initErr and return instead of
only logging it; send nil only once all requested security handlers and
listeners are installed.
In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go`:
- Around line 44-101: Update dialThroughMITMWithProto to register t.Cleanup that
closes the HTTP transport’s idle connections and the guest side of the net.Pipe.
Ensure cleanup covers both the HTTP/1.1 transport and the HTTP/2 transport
before returning the client.
In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go`:
- Around line 42-63: Update the reverse proxy around the `httputil.ReverseProxy`
and `secretTransport` flow to redact every configured secret from upstream
response headers and streaming response bodies before forwarding them to the
guest. Preserve response streaming while handling compressed and uncompressed
content correctly, and recalculate or remove `Content-Length` and update
`Content-Encoding` as needed after redaction.
- Around line 75-82: The HTTP/1.1 MITM path leaves singleConnListener.Accept
blocked after tlsGuest closes, preventing mitmAndForward from returning. Update
newSingleConnListener and the tlsGuest wrapper so closing the wrapped connection
also signals the listener’s closed state and causes Accept to return
net.ErrClosed; add a regression test that verifies mitmAndForward exits after
the HTTP/1.1 connection closes.
- Around line 27-32: Update the guest-facing TLS and request handling around
tlsGuest to set a temporary deadline before tlsGuest.Handshake(), then configure
guest-side HTTP/1.1 and HTTP/2 read/idle timeouts so partial handshakes or slow
requests cannot block the worker indefinitely. Preserve the existing TLS
configuration and ensure the deadline/timeout behavior applies only to
guest-side connections.
- Around line 34-40: Update the upstreamTransport initialization in the MITM
proxy setup to clone http.DefaultTransport rather than constructing a zero-value
http.Transport, then preserve the existing ForceAttemptHTTP2, TLSClientConfig,
and DialContext overrides on the clone. Add deferred cleanup with
upstreamTransport.CloseIdleConnections() for the per-session transport.
In `@src/libgvproxy-sys/gvproxy-bridge/mitm_replacer.go`:
- Around line 46-67: Filter out secrets with empty Placeholder values before
appending to pairs or updating maxPH/seen in the replacer construction loop.
Ensure strings.NewReplacer receives no empty placeholder and maxPlaceholder
remains positive for valid entries, preventing unintended insertion and
safeBoundary() panics.
In `@src/libgvproxy-sys/gvproxy-bridge/mitm.go`:
- Around line 183-185: Update the query-rewrite logic around req.URL.RawQuery to
parse the query with URL query-value decoding, apply r.Replace to the decoded
parameter values, and serialize the result with url.Values.Encode(). Preserve
query keys and repeated values while ensuring escaped placeholders are matched
and replacement secrets are correctly escaped.
---
Outside diff comments:
In `@src/boxlite/src/jailer/bwrap.rs`:
- Around line 605-615: Update the LD_LIBRARY_PATH setup in the bwrap
configuration to always prepend shim_path.parent() to any inherited
LD_LIBRARY_PATH, including empty values, instead of using the inherited value as
an exclusive branch. Preserve the shim directory as the fallback when no
environment value exists, and retain the existing sandbox environment assignment
and debug logging.
---
Nitpick comments:
In @.github/workflows/lint.yml:
- Around line 119-122: Update the actions/checkout step to disable credential
persistence by setting its persist-credentials option to false alongside the
existing recursive submodules configuration.
- Around line 94-97: Update the actions/checkout step to disable credential
persistence by setting persist-credentials to false, while preserving the
existing recursive submodules configuration.
In @.github/workflows/test.yml:
- Around line 57-58: Disable credential persistence for every
actions/checkout@v5 step by adding persist-credentials: false alongside
submodules in .github/workflows/test.yml at lines 57-58, 98-99, 146-147,
196-197, 228-229, and 258-259.
In `@src/libgvproxy-sys/gvproxy-bridge/gvproxy_create_latency_test.go`:
- Around line 23-59: The TestVirtualNetworkNewWithinLatencyBudget test uses an
environment-sensitive 500µs wall-clock failure threshold. Remove the strict
unit-test gate by converting this measurement to a benchmark/performance-only
check, or replace it with a substantially more tolerant budget while preserving
the warmup, sampling, and diagnostic logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b552e3e3-fa13-4027-a546-80a778a2e266
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locksrc/libgvproxy-sys/gvproxy-bridge/go.sumis excluded by!**/*.sum
📒 Files selected for processing (53)
.github/workflows/README.md.github/workflows/build-runtime.yml.github/workflows/lint.yml.github/workflows/test.yml.gitmodulesCargo.tomlcodecov.ymlmake/build.mkmake/quality.mkmake/vars.mkscripts/build/build-bubblewrap.shscripts/build/build-e2fsprogs.shscripts/build/build-libkrunfw.shscripts/build/build-runtime-assets.shscripts/build/build-runtime.shscripts/build/build-shim.shscripts/test/e2e/bootstrap.shsrc/boxlite/Cargo.tomlsrc/boxlite/build.rssrc/boxlite/deps/README.mdsrc/boxlite/src/jailer/bwrap.rssrc/boxlite/src/net/socket_path.rssrc/boxlite/src/system_check.rssrc/boxlite/src/vmm/krun/constants.rssrc/boxlite/src/vmm/krun/context.rssrc/boxlite/src/vmm/krun/kernel.rssrc/boxlite/tests/network_spec.rssrc/cli/tests/gvproxy_port_conflict.rssrc/deps/bubblewrap-sys/Cargo.tomlsrc/deps/bubblewrap-sys/build.rssrc/deps/bubblewrap-sys/src/lib.rssrc/deps/e2fsprogs-sys/Cargo.tomlsrc/deps/e2fsprogs-sys/build.rssrc/deps/e2fsprogs-sys/src/lib.rssrc/deps/libkrun-sys/Cargo.tomlsrc/deps/libkrun-sys/build.rssrc/deps/libkrun-sys/src/lib.rssrc/libgvproxy-sys/Cargo.tomlsrc/libgvproxy-sys/build.rssrc/libgvproxy-sys/gvproxy-bridge/config_build_test.gosrc/libgvproxy-sys/gvproxy-bridge/dns_filter.gosrc/libgvproxy-sys/gvproxy-bridge/dns_filter_test.gosrc/libgvproxy-sys/gvproxy-bridge/forked_network.gosrc/libgvproxy-sys/gvproxy-bridge/forked_network_test.gosrc/libgvproxy-sys/gvproxy-bridge/forked_tcp.gosrc/libgvproxy-sys/gvproxy-bridge/forked_tcp_test.gosrc/libgvproxy-sys/gvproxy-bridge/go.modsrc/libgvproxy-sys/gvproxy-bridge/gvproxy_create_latency_test.gosrc/libgvproxy-sys/gvproxy-bridge/main.gosrc/libgvproxy-sys/gvproxy-bridge/mitm.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_proxy.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_replacer.go
💤 Files with no reviewable changes (10)
- src/deps/bubblewrap-sys/Cargo.toml
- src/deps/bubblewrap-sys/src/lib.rs
- src/deps/e2fsprogs-sys/src/lib.rs
- src/deps/e2fsprogs-sys/Cargo.toml
- src/deps/bubblewrap-sys/build.rs
- src/deps/libkrun-sys/Cargo.toml
- src/deps/e2fsprogs-sys/build.rs
- src/boxlite/deps/README.md
- src/deps/libkrun-sys/build.rs
- src/deps/libkrun-sys/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitmodules
- src/boxlite/src/system_check.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (18)
src/libgvproxy-sys/Cargo.toml (1)
27-27: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -C3 'skip-native-build|native-build|all-features' .Repository: boxlite-ai/boxlite
Length of output: 3252
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== Cargo.toml ==\n' sed -n '1,120p' src/libgvproxy-sys/Cargo.toml printf '\n== build.rs ==\n' sed -n '1,180p' src/libgvproxy-sys/build.rs printf '\n== quality.mk excerpt ==\n' sed -n '190,210p' make/quality.mkRepository: boxlite-ai/boxlite
Length of output: 7071
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== workspace references ==\n' rg -n -C2 'libgvproxy-sys|skip-native-build|gvproxy' Cargo.toml src scripts make docs -g '!target' printf '\n== package manifests mentioning libgvproxy-sys ==\n' fd -a 'Cargo.toml' . | xargs rg -n -C2 'libgvproxy-sys|skip-native-build' --Repository: boxlite-ai/boxlite
Length of output: 50375
Use a positive
native-buildfeature instead.skip-native-buildis additive, so a transitive dependent or--all-featuresbuild can suppress therustc-link-*directives for every consumer and break final linking. Keep the skip path internal and make the crate default tonative-build; adjust the clippy invocation accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/Cargo.toml` at line 27, Replace the public skip-native-build feature in the crate feature definitions with a positive native-build feature enabled by default, keeping the native-build opt-out internal to the build logic so transitive dependencies and --all-features cannot suppress rustc-link-* directives. Update the related clippy invocation to use the new feature name and behavior.src/libgvproxy-sys/gvproxy-bridge/dns_filter_test.go (1)
29-44: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid public DNS in this unit test.
The expected
com.zone exists only ifexample.comresolves, making this test environment-dependent. Inject a lookup function/resolver and return fixed IPv4 records during tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/dns_filter_test.go` around lines 29 - 44, Update TestBuildAllowNetDNSZones_PerTLDZonesHaveSinkholeDefaultIP and the buildAllowNetDNSZones call path to accept an injectable DNS lookup/resolver, then provide a test stub returning fixed IPv4 records for example.com. Ensure the test no longer performs public DNS resolution while preserving its existing zone-count and sinkhole DefaultIP assertions.src/libgvproxy-sys/gvproxy-bridge/dns_filter.go (2)
72-83: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the file and the matching logic git ls-files | rg '^src/libgvproxy-sys/gvproxy-bridge/dns_filter\.go$|^src/libgvproxy-sys/gvproxy-bridge/' echo '--- dns_filter.go outline ---' ast-grep outline src/libgvproxy-sys/gvproxy-bridge/dns_filter.go --view expanded || true echo '--- relevant lines in dns_filter.go ---' cat -n src/libgvproxy-sys/gvproxy-bridge/dns_filter.go | sed -n '1,220p' echo '--- search for Zone consumption / matching ---' rg -n "first-match|suffix|zone|Zones|DefaultIP|AllowNet|allowNet" src/libgvproxy-sys/gvproxy-bridge -SRepository: boxlite-ai/boxlite
Length of output: 20033
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect any zone matching implementation outside the immediate file if needed. rg -n "type Zone|func .*Zone|HasSuffix|strings\.HasSuffix|suffix match|match.*zone|DefaultIP" src -SRepository: boxlite-ai/boxlite
Length of output: 3516
🏁 Script executed:
#!/bin/bash set -euo pipefail # Determine whether zone ordering is stabilized elsewhere. rg -n "sort\.Slice\(zones|zones = append|zoneRecords" src/libgvproxy-sys/gvproxy-bridge -SRepository: boxlite-ai/boxlite
Length of output: 1231
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the tests around dns zone ordering expectations. cat -n src/libgvproxy-sys/gvproxy-bridge/dns_filter_test.go | sed -n '1,220p' echo '--- buildDNSZones tests ---' cat -n src/libgvproxy-sys/gvproxy-bridge/config_build_test.go | sed -n '1,140p' echo '--- main.go zone assembly ---' cat -n src/libgvproxy-sys/gvproxy-bridge/main.go | sed -n '220,245p'Repository: boxlite-ai/boxlite
Length of output: 6090
🏁 Script executed:
#!/bin/bash set -euo pipefail # Look for any local documentation or generated config code that describes zone matching order. rg -n "first-match-wins|most-specific|suffix matching|zone matching|matches.*zone|longest suffix|specific zones" . -SRepository: boxlite-ai/boxlite
Length of output: 379
🌐 Web query:
gvisor-tap-vsock DNS zone matching first-match-wins suffix order💡 Result:
gvisor-tap-vsock does not implement a DNS zone matching system based on a "first-match-wins" suffix order for incoming requests. Instead, it functions as a pass-through DNS proxy that facilitates name resolution by forwarding queries to configured upstream nameservers [1][2][3]. The architecture operates as follows: 1. Static Zone Support: The gateway's internal DNS server can be configured to serve specific static zones [4][5]. 2. Proxying Mechanism: When a DNS query is received, the gvproxy DNS handler processes the request [1][3]. If the query is not satisfied by the locally configured static zones, the system is designed to forward the request to upstream nameservers [1][3]. 3. Upstream Resolution: As of recent updates (introduced in PR
#426), the resolver is configured to iterate through all identified IPv4 DNS services available on the host system [2][6]. It performs the DNS exchange sequentially with these configured services until a successful response is received (or all options are exhausted) [2]. There is no "suffix order" or "first-match-wins" logic applied to zone matching for general DNS queries because the system primarily acts as a forwarder for non-static queries, rather than an authoritative server with a hierarchical zone-matching priority list [1][2][3]. Configuration of DNS resolution logic is handled via platform-specific implementations that extract nameserver information from the host (e.g., /etc/resolv.conf on Unix or system-wide DNS settings on Windows) [3].Citations:
- 1: containers/gvisor-tap-vsock@58eb054
- 2: containers/gvisor-tap-vsock@d69e2ac
- 3: containers/gvisor-tap-vsock@3c875de
- 4: https://github.com/containers/gvisor-tap-vsock/blob/main/README.md
- 5: https://github.com/containers/gvisor-tap-vsock?tab=readme-ov-file
- 6: containers/gvisor-tap-vsock@2be6b3f
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the Rust-side DNS zone model and any ordering/matching comments. cat -n src/boxlite/src/net/mod.rs | sed -n '130,220p' echo '---' cat -n src/boxlite/src/net/gvproxy/config.rs | sed -n '560,640p' echo '--- search for zone lookup order / specificity ---' rg -n "zone.*order|order.*zone|longest|specificity|first-match|suffix|matches built_in_zone|DNSZone" src/boxlite/src/net -SRepository: boxlite-ai/boxlite
Length of output: 7093
Sort allow-list zones before returning them
zoneRecordscomes from a map, so zone order is nondeterministic. With first-match-wins suffix matching, a broader zone can be checked before a more specific one and sinkhole an allowed hostname. Sort by descending zone-name length (then name) before appending the root zone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/dns_filter.go` around lines 72 - 83, Sort the zones generated from zoneRecords by descending zone-name length, using the zone name as a deterministic tie-breaker, before appending the root zone. Update the zone construction flow around the zones slice and preserve the existing first-match-wins behavior and logging.
99-109: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound allowlist DNS lookups.
Each rule performs a sequential lookup with
context.Background(). An unavailable resolver can blockgvproxy_createfor every configured hostname, multiplying startup delay.Proposed fix
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() resolver := &net.Resolver{PreferGo: false}import ( "context" "net" "regexp" "strings" + "time" )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// resolveAndAddRecords resolves a hostname and adds A records to the zone. func resolveAndAddRecords(hostname, zoneName string, zoneRecords map[string][]types.Record) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() resolver := &net.Resolver{PreferGo: false} ips, err := resolver.LookupIPAddr(ctx, hostname) if err != nil { logrus.WithFields(logrus.Fields{ "hostname": hostname, "error": err, }).Warn("allowNet: DNS resolution failed for allowed host") return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/dns_filter.go` around lines 99 - 109, Update resolveAndAddRecords to perform the hostname lookup with a bounded context instead of context.Background(), applying a finite timeout per DNS rule and ensuring the context is cancelled after the lookup. Preserve the existing warning and early-return behavior when resolution times out or otherwise fails.src/libgvproxy-sys/gvproxy-bridge/forked_network.go (1)
37-45: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail git ls-files src/libgvproxy-sys/gvproxy-bridge echo '---' ast-grep outline src/libgvproxy-sys/gvproxy-bridge/forked_network.go --view expanded || true echo '---' wc -l src/libgvproxy-sys/gvproxy-bridge/forked_network.go echo '---' cat -n src/libgvproxy-sys/gvproxy-bridge/forked_network.go | sed -n '1,140p' echo '---' rg -n "TestStackFieldExists|stackField|FieldByName\\(\"stack\"\\)|type VirtualNetwork|struct .*VirtualNetwork|stack\\.Stack" src/libgvproxy-sys/gvproxy-bridge -SRepository: boxlite-ai/boxlite
Length of output: 5097
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- forked_network_test.go ---' cat -n src/libgvproxy-sys/gvproxy-bridge/forked_network_test.go | sed -n '1,220p' echo '--- go.mod ---' cat -n src/libgvproxy-sys/gvproxy-bridge/go.mod | sed -n '1,220p' echo '--- search module cache for VirtualNetwork definition ---' python3 - <<'PY' import os, glob cands = [] for base in [os.path.expanduser('~/go/pkg/mod'), '/go/pkg/mod']: if os.path.isdir(base): for path in glob.glob(base + '/**/*virtualnetwork*', recursive=True): cands.append(path) print('\n'.join(sorted(set(cands))[:200])) PY echo '--- search repository and module cache for type VirtualNetwork ---' rg -n "type VirtualNetwork struct|stack\\s+\\*stack\\.Stack|FieldByName\\(\"stack\"\\)" src/libgvproxy-sys/gvproxy-bridge ~/go/pkg/mod /go/pkg/mod -S || trueRepository: boxlite-ai/boxlite
Length of output: 8952
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,80p' /home/jailuser/go/pkg/mod/github.com/containers/[email protected]/pkg/virtualnetwork/virtualnetwork.go | cat -nRepository: boxlite-ai/boxlite
Length of output: 3082
Assert
VirtualNetwork.stackis*stack.Stackbefore the unsafe cast.TestStackFieldExistsshould check the exact type and non-nil value, not justreflect.Ptr, so an upstream API change fails fast before reaching the unsafe cast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/forked_network.go` around lines 37 - 45, Update TestStackFieldExists to verify that VirtualNetwork.stack has the exact *stack.Stack type and is non-nil before any unsafe conversion. Keep the existing missing-field validation, and ensure the test fails fast before the stackField.Pointer cast when the upstream API changes.src/libgvproxy-sys/gvproxy-bridge/forked_tcp_test.go (1)
77-120: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the routing decision, not only matcher behavior.
These tests would still pass if port 80 were incorrectly routed to MITM or non-secret traffic were blocked. Call
decideTCPRoutedirectly and assert port 80/non-secret traffic usestcpRouteStandardForward, while secret port 443 usestcpRouteInspect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/forked_tcp_test.go` around lines 77 - 120, Update TestMitmRouting_NonSecretHostPassthrough and TestMitmRouting_SecretHostPort80_NoMitm to call decideTCPRoute directly and assert non-secret traffic and secret-host port 80 return tcpRouteStandardForward. Also assert that the secret host on port 443 returns tcpRouteInspect, so the tests verify routing decisions rather than only matcher behavior.src/libgvproxy-sys/gvproxy-bridge/forked_tcp.go (2)
134-189: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add deadlines to dial and inspection operations.
net.Dialand the SNI/Host reads are unbounded. Blackholed destinations can occupy the forwarder’s limited in-flight slots, while guests that never send headers can leave accepted connections and goroutines open indefinitely.Use
net.Dialer{Timeout: ...}and set a temporary read deadline aroundpeekClientHelloSNI/peekHTTPHost, clearing it before relay.Also applies to: 218-236
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/forked_tcp.go` around lines 134 - 189, Bound both connection setup and header inspection in standardForward and inspectAndForward: replace net.Dial with a net.Dialer configured with the project’s appropriate timeout, set a temporary read deadline on guestConn before peekClientHelloSNI or peekHTTPHost, and clear the deadline after inspection succeeds before handing the connection to relay. Apply the same dial timeout to the outbound connection path later in inspectAndForward, preserving existing error and cleanup behavior.
191-219: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Bind the inspected hostname to the actual dial destination.
After an allowed Host/SNI match, the code still dials the guest-selected
destAddr. A guest can connect to an arbitrary IP and send an allowed hostname, bypassing hostname-based egress filtering. Validate that the pre-NAT destination belongs to the inspected hostname’s resolved addresses—or dial the validated hostname—before MITM or passthrough.This validation must occur before accessing secrets on Lines 192-200.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/forked_tcp.go` around lines 191 - 219, Validate that the pre-NAT destination in the TCP bridge belongs to the inspected hostname’s resolved addresses before the MITM branch accesses SecretsForHost, and before any passthrough dialing. Reject and close guestConn when the hostname-to-destination binding fails; otherwise continue using the validated destination for MITM or net.Dial, preserving allowlist behavior.src/libgvproxy-sys/gvproxy-bridge/main.go (3)
354-400: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the MITM CA before opening the platform socket.
A CA parsing failure returns after the Unix listener/connection has been created, but before cleanup is installed. Each failure leaks the descriptor and leaves the socket path behind. Move all fallible CA/config validation before Lines 359-378.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/main.go` around lines 354 - 400, Move the fallible NewBoxCAFromPEM validation block before the platform-specific socket creation in the surrounding initialization flow, while preserving its existing error reporting and return behavior. Ensure CA/config validation completes before either transport.ListenUnixgram or net.Listen is called, and retain the parsed CA for subsequent setup without creating sockets on validation failure.
445-490: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not report readiness before security handlers are installed.
initErr <- nilreturns a valid instance ID beforeOverrideTCPHandlerruns. If that override fails, the error is only logged and AllowNet silently runs with unrestricted default TCP forwarding. A requested control socket can similarly fail after creation was reported successful.Send success only after all configured handlers and listeners are installed; propagate either failure through
initErr.Proposed flow
- initErr <- nil if len(config.AllowNet) > 0 || instance.secretMatcher != nil { ... if err := OverrideTCPHandler(...); err != nil { - logrus.WithError(err).Error("TCP: failed to override handler") + initErr <- fmt.Errorf("TCP handler override failed: %w", err) + return } } ... if lErr != nil { - logrus.WithFields(...).Error("Failed to bind gvproxy services socket") + initErr <- fmt.Errorf("bind gvproxy services socket: %w", lErr) + return } ... + initErr <- nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.vn, err := virtualnetwork.New(tapConfig) if err != nil { logrus.WithFields(logrus.Fields{"error": err, "id": id}).Error("Failed to create virtual network") initErr <- err return } // Override TCP handler with AllowNet filter and/or MITM secret substitution if len(config.AllowNet) > 0 || instance.secretMatcher != nil { var tcpFilter *TCPFilter if len(config.AllowNet) > 0 { tcpFilter = NewTCPFilter(config.AllowNet, config.GatewayIP, config.GuestIP, config.HostIP) } if err := OverrideTCPHandler(vn, tapConfig, tapConfig.Ec2MetadataAccess, tcpFilter, instance.ca, instance.secretMatcher); err != nil { initErr <- fmt.Errorf("TCP handler override failed: %w", err) return } } // Store VirtualNetwork reference for stats collection instance.vnMu.Lock() instance.vn = vn instance.vnMu.Unlock() // Bind gvproxy's ServicesMux to a host unix socket so the boxlite core // can drive dynamic port forwarding / DNS / leases on the running box. // ServicesMux (not Mux) excludes the raw L2 /connect, so the VM's NIC // can never be attached through this socket. var controlListener net.Listener if config.ControlSocketPath != "" { // Remove a stale socket from a previous crash (path is unique per box). if rmErr := os.Remove(config.ControlSocketPath); rmErr != nil && !os.IsNotExist(rmErr) { logrus.WithFields(logrus.Fields{"error": rmErr, "path": config.ControlSocketPath}).Warn("Failed to remove existing services socket") } l, lErr := net.Listen("unix", config.ControlSocketPath) if lErr != nil { initErr <- fmt.Errorf("bind gvproxy services socket: %w", lErr) return } else { controlListener = l logrus.WithField("path", config.ControlSocketPath).Info("Serving gvproxy ServicesMux") go func() { if sErr := http.Serve(l, vn.ServicesMux()); sErr != nil && ctx.Err() == nil { logrus.WithError(sErr).Error("gvproxy services HTTP server exited") } }() } } initErr <- nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/main.go` around lines 445 - 490, Move the successful initErr notification until after OverrideTCPHandler and the optional control-socket listener/server setup complete. If OverrideTCPHandler fails, or net.Listen fails for config.ControlSocketPath, send that error through initErr and return instead of only logging it; send nil only once all requested security handlers and listeners are installed.
593-610: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Wait for teardown before returning from
gvproxy_destroy.Cancellation is asynchronous. If the caller immediately recreates an instance at the same path, the old cleanup goroutine can later execute
os.Remove(socketPath)and unlink the new instance’s socket.Add a completion channel to
GvproxyInstance, close it after cleanup, and wait for it afterinstance.Cancel()before returning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/main.go` around lines 593 - 610, Update GvproxyInstance with a teardown completion channel, close it at the end of the cleanup goroutine after removing resources, and in gvproxy_destroy call instance.Cancel() then wait on that channel before returning. Preserve the existing missing-instance error path and successful destruction logging.src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go (1)
44-101: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== file outline ==\n' ast-grep outline src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go --view expanded || true printf '\n== relevant lines ==\n' nl -ba src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go | sed -n '1,220p' printf '\n== cleanup/search ==\n' rg -n "CloseIdleConnections|t\.Cleanup|Cleanup\(" src/libgvproxy-sys/gvproxy-bridge -S || true printf '\n== helper usages ==\n' rg -n "dialThroughMITMWithProto\(" src/libgvproxy-sys/gvproxy-bridge -S || trueRepository: boxlite-ai/boxlite
Length of output: 1644
🏁 Script executed:
#!/bin/bash set -euo pipefail nl -ba src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go | sed -n '1,180p'Repository: boxlite-ai/boxlite
Length of output: 196
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== mitm_proxy_test.go (1-140) ==\n' awk 'NR>=1 && NR<=140 {print NR ":" $0}' src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go printf '\n== cleanup/search ==\n' rg -n "CloseIdleConnections|t\.Cleanup|Cleanup\(" src/libgvproxy-sys/gvproxy-bridge -S || true printf '\n== helper usages ==\n' rg -n "dialThroughMITMWithProto\(|dialThroughMITMH2\(|dialThroughMITM\(" src/libgvproxy-sys/gvproxy-bridge -S || trueRepository: boxlite-ai/boxlite
Length of output: 6876
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== runtime usage ==\n' rg -n "runtime\." src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go -n -S || true printf '\n== goroutine / leak related lines ==\n' rg -n "goroutine|runtime\.NumGoroutine|CloseIdleConnections|idle" src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go -n -S || true printf '\n== tail of file ==\n' awk 'NR>=140 && NR<=760 {print NR ":" $0}' src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.goRepository: boxlite-ai/boxlite
Length of output: 17066
Register cleanup for the pipe-backed client.
src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go:44-101
dialThroughMITMWithProtoleaves thenet.Pipe/transport open after each test, which can keep proxy goroutines around and skew later goroutine-leak checks. Addt.Cleanupto close idle connections and the client side of the pipe.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 49-49: TLS certificate verification is disabled by setting
InsecureSkipVerify: trueon thetls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. RemoveInsecureSkipVerify(or set it tofalse) and provide a properRootCAspool to trust custom certificates instead.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-295] Improper Certificate Validation.(tls-insecure-skip-verify-go)
[warning] 49-49: MinVersion
is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures(missing-ssl-minversion-go)
[warning] 56-61: MinVersion
is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
ServerName: hostname,
RootCAs: caPool,
NextProtos: nextProtos,
InsecureSkipVerify: caPool == nil,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures(missing-ssl-minversion-go)
🪛 OpenGrep (1.25.0)
[ERROR] 50-50: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 50-50: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go` around lines 44 - 101, Update dialThroughMITMWithProto to register t.Cleanup that closes the HTTP transport’s idle connections and the guest side of the net.Pipe. Ensure cleanup covers both the HTTP/1.1 transport and the HTTP/2 transport before returning the client.src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go (4)
27-32: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.goRepository: boxlite-ai/boxlite
Length of output: 4005
Bound the guest-facing TLS handshake and request reads. A partial handshake or slow request can pin this worker indefinitely; set a temporary deadline before
tlsGuest.Handshake()and add guest-side HTTP/1.1 and HTTP/2 read/idle timeouts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go` around lines 27 - 32, Update the guest-facing TLS and request handling around tlsGuest to set a temporary deadline before tlsGuest.Handshake(), then configure guest-side HTTP/1.1 and HTTP/2 read/idle timeouts so partial handshakes or slow requests cannot block the worker indefinitely. Preserve the existing TLS configuration and ensure the deadline/timeout behavior applies only to guest-side connections.Source: Linters/SAST tools
34-40: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ast-grep outline src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go --view expandedRepository: boxlite-ai/boxlite
Length of output: 949
🏁 Script executed:
sed -n '1,120p' src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.goRepository: boxlite-ai/boxlite
Length of output: 3499
🏁 Script executed:
sed -n '120,220p' src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.goRepository: boxlite-ai/boxlite
Length of output: 663
🌐 Web query:
Go net/http Transport TLSHandshakeTimeout IdleConnTimeout DefaultTransport Clone CloseIdleConnections documentation💡 Result:
In Go's net/http package, the Transport struct is the low-level mechanism responsible for establishing network connections and managing their lifecycle [1][2]. Key Components and Methods: DefaultTransport: This is the default implementation of the RoundTripper interface used by http.DefaultClient [1][3]. It is pre-configured with the following default values [1][4]: - MaxIdleConns: 100 - IdleConnTimeout: 90 seconds - TLSHandshakeTimeout: 10 seconds - ExpectContinueTimeout: 1 second TLSHandshakeTimeout: A field of the Transport struct that specifies the maximum amount of time to wait for a TLS handshake [5][4]. A value of zero means no timeout is applied [5][4]. IdleConnTimeout: A field of the Transport struct that determines the maximum amount of time an idle (keep-alive) connection will remain idle before being closed [4]. If set to zero, there is no limit on how long a connection can remain idle [4]. CloseIdleConnections: A method on the Transport struct that closes all connections currently sitting idle in a keep-alive state [2][4]. It does not interrupt connections that are currently in use [2][4]. Clone: A method that returns a deep copy of the Transport's exported fields [5][4]. This is useful for creating a modified version of an existing Transport (e.g., changing one setting like TLSClientConfig) without affecting the original [5][4]. These settings allow developers to fine-tune resource usage, especially when interacting with many different hosts or maintaining long-lived connections [1][3]. By default, Transport caches connections for future reuse, which can result in many open connections; this behavior can be controlled using the aforementioned timeout fields and methods, as well as MaxIdleConnsPerHost and DisableKeepAlives [1][3].
Citations:
- 1: https://go.dev/src/net/http/transport.go
- 2: https://pkg.go.dev/net/http
- 3: https://tip.golang.org/src/net/http/transport.go
- 4: https://go.googlesource.com/go/+/master/src/net/http/transport.go
- 5: https://go.dev/src/net/http/transport.go?m=text
Clone
http.DefaultTransportfor the upstream proxy transport Use a cloned default transport here so upstream requests keep the standard TLS handshake and idle-connection timeouts; the zero-valuehttp.Transportleaves both at 0.defer upstreamTransport.CloseIdleConnections()also fits this per-session transport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go` around lines 34 - 40, Update the upstreamTransport initialization in the MITM proxy setup to clone http.DefaultTransport rather than constructing a zero-value http.Transport, then preserve the existing ForceAttemptHTTP2, TLSClientConfig, and DialContext overrides on the clone. Add deferred cleanup with upstreamTransport.CloseIdleConnections() for the per-session transport.
42-63: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Prevent reflected upstream data from exposing injected secrets.
Requests are rewritten with real secret values, but responses are forwarded unchanged. The tests at
src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.goLines 117-200 demonstrate that a reflective endpoint returns the real secret to the guest, defeating secret confidentiality.Redact secret values from response headers and streaming bodies before forwarding them, with correct content-encoding and content-length handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go` around lines 42 - 63, Update the reverse proxy around the `httputil.ReverseProxy` and `secretTransport` flow to redact every configured secret from upstream response headers and streaming response bodies before forwarding them to the guest. Preserve response streaming while handling compressed and uncompressed content correctly, and recalculate or remove `Content-Length` and update `Content-Encoding` as needed after redaction.
75-82: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant file and find the listener implementation and call sites. git ls-files | rg '^src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go$|^src/libgvproxy-sys/gvproxy-bridge/' ast-grep outline src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go --view expanded || true rg -n "type singleConnListener|newSingleConnListener|func .*Accept|func .*Close|mitmAndForward|srv\.Serve|srv\.Close" src/libgvproxy-sys/gvproxy-bridge -S # Read the relevant slices with line numbers. sed -n '1,220p' src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.goRepository: boxlite-ai/boxlite
Length of output: 9499
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the listener and nearby tests for any close signaling or shutdown path. sed -n '1,180p' src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go printf '\n--- tests ---\n' rg -n "singleConnListener|mitmAndForward|HTTP/1.1|Serve\(|Close\(\)" src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go src/libgvproxy-sys/gvproxy-bridge/mitm_test.go -SRepository: boxlite-ai/boxlite
Length of output: 6379
Close the one-shot listener when the wrapped connection closes.
singleConnListener.Accept()still blocks after the first connection is consumed because nothing signalsclosedwhentlsGuestis closed. That leaveshttp.Server.Serve()stuck inAccept(), sosrv.Close()is never reached and HTTP/1.1 MITM sessions can leak a goroutine. WraptlsGuestsoClose()also closes the listener (or otherwise returnnet.ErrClosedfromAccept()), and add a regression test that waits formitmAndForwardto exit after an HTTP/1.1 connection closes.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 78-78: This
http.Serveris constructed without aReadTimeout. Without a read timeout, a slow or malicious client can hold connections open indefinitely (e.g. a Slowloris attack), exhausting server resources and causing a denial of service. SetReadTimeout(and ideallyReadHeaderTimeout,WriteTimeout, andIdleTimeout) on thehttp.Serverto bound how long the server waits while reading a request.
Context: http.Server{Handler: proxy}
Note: [CWE-400] Uncontrolled Resource Consumption.(http-server-missing-read-timeout-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go` around lines 75 - 82, The HTTP/1.1 MITM path leaves singleConnListener.Accept blocked after tlsGuest closes, preventing mitmAndForward from returning. Update newSingleConnListener and the tlsGuest wrapper so closing the wrapped connection also signals the listener’s closed state and causes Accept to return net.ErrClosed; add a regression test that verifies mitmAndForward exits after the HTTP/1.1 connection closes.src/libgvproxy-sys/gvproxy-bridge/mitm_replacer.go (1)
46-67: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' src/libgvproxy-sys/gvproxy-bridge/mitm_replacer.goRepository: boxlite-ai/boxlite
Length of output: 4926
🏁 Script executed:
rg -n "placeholder|SecretConfig|newStreamingReplacer|NewReplacer|mitm_replacer" src . --glob '!**/vendor/**'Repository: boxlite-ai/boxlite
Length of output: 50374
🏁 Script executed:
python3 - <<'PY' import os, subprocess, textwrap, tempfile, json, sys code = r''' package main import ( "fmt" "strings" ) func main() { r := strings.NewReplacer("", "X") fmt.Println(r.Replace("abc")) } ''' with open('/tmp/empty_replacer.go','w') as f: f.write(code) p = subprocess.run(['go','run','/tmp/empty_replacer.go'], capture_output=True, text=True) print('rc=', p.returncode) print(p.stdout) print(p.stderr) PYRepository: boxlite-ai/boxlite
Length of output: 172
🏁 Script executed:
sed -n '110,200p' src/boxlite/src/net/gvproxy/config.rs && printf '\n---\n' && sed -n '390,470p' src/boxlite/src/runtime/options.rs && printf '\n---\n' && sed -n '1220,1265p' src/boxlite/src/runtime/options.rs && printf '\n---\n' && sed -n '448,465p' sdks/go/options.goRepository: boxlite-ai/boxlite
Length of output: 8772
🏁 Script executed:
rg -n "placeholder\s*==\s*\"\"|placeholder:\s*String::new\(|placeholder:\s*\"\"|default.*placeholder|fn .*placeholder|Secret \{" src sdks -g '!**/vendor/**'Repository: boxlite-ai/boxlite
Length of output: 2918
🏁 Script executed:
sed -n '380,430p' sdks/node/src/options.rs && printf '\n---\n' && sed -n '320,365p' sdks/python/src/options.rs && printf '\n---\n' && sed -n '396,420p' sdks/c/src/options.rsRepository: boxlite-ai/boxlite
Length of output: 3848
Reject empty placeholders before building the replacer. Empty
placeholdervalues reach this path unchanged from config, andstrings.NewReplacer("", ...)will inject the secret throughout the body; withmaxPlaceholder == 0,safeBoundary()also panics on the next non-EOF read. Validate or drop empty placeholders up front.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm_replacer.go` around lines 46 - 67, Filter out secrets with empty Placeholder values before appending to pairs or updating maxPH/seen in the replacer construction loop. Ensure strings.NewReplacer receives no empty placeholder and maxPlaceholder remains positive for valid entries, preventing unintended insertion and safeBoundary() panics.src/libgvproxy-sys/gvproxy-bridge/mitm.go (1)
183-185: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Decode and re-encode query parameters before substitution.
RawQuerynormally contains escaped placeholders such as%3CBOXLITE_SECRET...%3E, so the literal replacer misses them. Conversely, inserting an unescaped secret containing&,+, or=changes the query semantics. Replace decoded query values and serialize them withurl.Values.Encode().Proposed fix
if req.URL != nil && req.URL.RawQuery != "" { - req.URL.RawQuery = r.Replace(req.URL.RawQuery) + query := req.URL.Query() + for key, values := range query { + for i, value := range values { + query[key][i] = r.Replace(value) + } + } + req.URL.RawQuery = query.Encode() }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if req.URL != nil && req.URL.RawQuery != "" { query := req.URL.Query() for key, values := range query { for i, value := range values { query[key][i] = r.Replace(value) } } req.URL.RawQuery = query.Encode() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm.go` around lines 183 - 185, Update the query-rewrite logic around req.URL.RawQuery to parse the query with URL query-value decoding, apply r.Replace to the decoded parameter values, and serialize the result with url.Values.Encode(). Preserve query keys and repeated values while ensuring escaped placeholders are matched and replacement secrets are correctly escaped.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/boxlite/src/jailer/sandbox/seatbelt.rs (1)
118-127: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winPreserve explicitly configured environment variables and the working directory.
Replacing
*cmd = new_cmd;without copying the existing environments and working directory causes all explicitly setCommandconfigurations to be lost. Dropping configurations likeTMPDIR(set upstream inconfigure_env()) will force the shim to fall back to the host system's temporary directories, breaking isolation guarantees.You should preserve the working directory, carry over the explicitly set environment variables, and join
DYLD_LIBRARY_PATHto prevent overwriting existing search paths.🔒️ Proposed fix
- if let Some(shim_dir) = binary_path - .parent() - .filter(|dir| !dir.as_os_str().is_empty()) - { - new_cmd.env("DYLD_LIBRARY_PATH", shim_dir); - } + if let Some(cwd) = cmd.get_current_dir() { + new_cmd.current_dir(cwd); + } + for (key, val) in cmd.get_envs() { + if let Some(val) = val { + new_cmd.env(key, val); + } else { + new_cmd.env_remove(key); + } + } + + if let Some(shim_dir) = binary_path + .parent() + .filter(|dir| !dir.as_os_str().is_empty()) + { + let mut paths = vec![shim_dir.to_path_buf()]; + if let Some(Some(existing)) = cmd.get_envs().find_map(|(k, v)| { + if k == std::ffi::OsStr::new("DYLD_LIBRARY_PATH") { + Some(v) + } else { + None + } + }) { + paths.extend(std::env::split_paths(existing)); + } + if let Ok(joined) = std::env::join_paths(paths) { + new_cmd.env("DYLD_LIBRARY_PATH", joined); + } + } new_cmd.arg(&binary); new_cmd.args(&args); *cmd = new_cmd;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/boxlite/src/jailer/sandbox/seatbelt.rs` around lines 118 - 127, Update the command replacement in the sandbox command-building function around new_cmd so it preserves the original Command working directory and explicitly configured environment variables. Carry the existing DYLD_LIBRARY_PATH forward by joining shim_dir with its prior value rather than overwriting it, while retaining the shim path behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/boxlite/src/jailer/sandbox/seatbelt.rs`:
- Around line 388-420: Update test_apply_sets_dyld_library_path_to_shim_dir to
set an existing environment variable such as TMPDIR on the original Command
before calling SeatbeltSandbox::apply, then assert the resulting command
preserves that variable while also retaining the expected merged
DYLD_LIBRARY_PATH value. Keep the existing program, arguments, and profile
assertions unchanged.
In `@src/boxlite/src/util/mod.rs`:
- Around line 124-133: Update the DYLD_LIBRARY_PATH setup around cmd.get_envs()
so existing explicitly configured paths are preserved while lib_dirs is still
added. Read and combine the existing value with the joined lib_dirs path, then
set the merged path on cmd; remove the early return while retaining the debug
logging.
---
Outside diff comments:
In `@src/boxlite/src/jailer/sandbox/seatbelt.rs`:
- Around line 118-127: Update the command replacement in the sandbox
command-building function around new_cmd so it preserves the original Command
working directory and explicitly configured environment variables. Carry the
existing DYLD_LIBRARY_PATH forward by joining shim_dir with its prior value
rather than overwriting it, while retaining the shim path behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68fb7622-c2ad-4618-b542-e3598c680aa4
📒 Files selected for processing (12)
.github/workflows/build-c.ymlapps/dashboard/src/lib/quickstart/interfaces.jsondocs/getting-started/quickstart-c.mddocs/reference/c/README.mdexamples/c/CMakeLists.txtexamples/c/README.mdmake/dev.mkmake/dist.mksdks/c/README.mdsrc/boxlite/src/jailer/sandbox/seatbelt.rssrc/boxlite/src/util/mod.rssrc/shim/build.rs
💤 Files with no reviewable changes (1)
- src/shim/build.rs
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/boxlite/src/jailer/sandbox/seatbelt.rs`:
- Around line 119-124: Preserve explicitly configured command environment
variables in SeatbeltSandbox::apply by iterating over cmd.get_envs() and
applying each entry to new_cmd after clearing inherited variables, while
retaining the existing PATH, HOME, and shim environment setup. Update the test
at src/boxlite/src/jailer/sandbox/seatbelt.rs lines 389-430 to assert
preservation of explicit TMPDIR, DYLD_LIBRARY_PATH, and SHOULD_NOT_SURVIVE
values rather than their removal.
In `@src/boxlite/src/util/mod.rs`:
- Around line 124-127: Update the environment setup near the DYLD_LIBRARY_PATH
assignment to read any existing value configured on cmd, prepend or append the
joined lib_dirs path while preserving that value, and set the merged result back
on cmd. Keep the existing library_path construction and debug logging behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e22d1f29-9143-490f-9d79-75bb323790e0
📒 Files selected for processing (12)
.github/workflows/build-c.ymlapps/dashboard/src/lib/quickstart/interfaces.jsondocs/getting-started/quickstart-c.mddocs/reference/c/README.mdexamples/c/CMakeLists.txtexamples/c/README.mdmake/dev.mkmake/dist.mksdks/c/README.mdsrc/boxlite/src/jailer/sandbox/seatbelt.rssrc/boxlite/src/util/mod.rssrc/shim/build.rs
💤 Files with no reviewable changes (1)
- src/shim/build.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/getting-started/quickstart-c.md
- make/dist.mk
- apps/dashboard/src/lib/quickstart/interfaces.json
- examples/c/CMakeLists.txt
- docs/reference/c/README.md
- .github/workflows/build-c.yml
- sdks/c/README.md
- examples/c/README.md
|
why should we change names for so many files also can we split the prs for different functions |
Only gvproxy is renamed, since it is a real dependency. All other Splitting can be done, but co-existing old/new path will be really troublesome for ai to get it right. It definitely will leave some compatibility safeguard non-sense, even if this is only building. The whole point of the refactor is splitting build into scripts, which centralize those misc builds and speed up the building on local machine a little bit by removing them from |
…target install - Pass -j to e2fsprogs and libkrunfw make invocations so they use all available cores instead of running serially. - Clear MAKEFLAGS on libkrunfw's top-level make so GNU Make's inherited -w (written as bare 'w') isn't misparsed as a target name by the kernel sub-make, which previously failed with: "No rule to make target 'w'". - Install the boxlite-guest musl cross target on every clippy runner, not just macOS. make clippy now cross-checks boxlite-guest with --target on all platforms, so Ubuntu needs the target installed too; it was previously missing, causing E0463 (can't find crate for `core`).
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/libgvproxy-sys/gvproxy-bridge/mitm_websocket.go (1)
50-55: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a dial timeout for the upstream connection.
net.Dialblocks with no timeout, so a slow or unreachable upstream ties up the handler goroutine indefinitely. Prefernet.DialTimeout(or anet.DialerwithTimeout/context) so failed connects fail fast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm_websocket.go` around lines 50 - 55, Update the upstream connection setup around net.Dial in the websocket handler to use net.DialTimeout or a net.Dialer with an explicit timeout, preserving the existing error logging and HTTP 502 response when the connection cannot be established.src/libgvproxy-sys/src/lib.rs (1)
87-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider typing the callback as
Option<LogCallbackFn>.You define
LogCallbackFnbut the registration function takes an untyped*const c_void, forcing callers to transmute a function pointer.Option<LogCallbackFn>is FFI-safe, encodes NULL asNone, and gives compile-time type checking at call sites.♻️ Proposed change
- pub fn gvproxy_set_log_callback(callback: *const c_void); + pub fn gvproxy_set_log_callback(callback: Option<LogCallbackFn>);Confirm the C/Go export signature accepts a plain function pointer so this remains ABI-compatible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/src/lib.rs` at line 87, Update gvproxy_set_log_callback to accept Option<LogCallbackFn> instead of *const c_void, using the existing LogCallbackFn type for compile-time callback validation and None for a null callback. Verify the exported C/Go signature accepts a plain function pointer so the FFI ABI remains compatible, and remove any now-unnecessary pointer casting at call sites.src/libgvproxy-sys/gvproxy-bridge/mitm.go (2)
1-1: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin
MinVersionacross the MITM TLS configs. Both TLS configs used in the MITM path omit an explicitMinVersion, relying on Go's default minimums (TLS 1.2 for client role, TLS 1.0 for server role). Since this proxy handles substituted API secrets, explicitly requiring TLS 1.2+ end-to-end is a small, consistent hardening step.
src/libgvproxy-sys/gvproxy-bridge/mitm.go#L188-206: addMinVersion: tls.VersionTLS12to thetls.Configbuilt inresolveUpstreamTLSfor upstream connections.src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go#L27-32: addMinVersion: tls.VersionTLS12to the guest-facingtls.Configpassed totls.Server.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm.go` at line 1, Set MinVersion to tls.VersionTLS12 in both MITM TLS configurations: the upstream tls.Config created by resolveUpstreamTLS and the guest-facing tls.Config passed to tls.Server in the MITM proxy. Preserve all existing TLS configuration behavior while enforcing TLS 1.2 or newer on both connections.Source: Linters/SAST tools
188-206: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet an explicit
MinVersionon the upstream TLS config.
resolveUpstreamTLSbuilds atls.Configfor connecting to real upstream hosts without pinningMinVersion. Go's client default minimum is TLS 1.2, but this MITM proxy carries substituted API secrets, so explicitly requiring TLS 1.2+ (or 1.3) is worthwhile defense-in-depth.🔒 Proposed fix
- cfg := &tls.Config{ServerName: hostname} + cfg := &tls.Config{ServerName: hostname, MinVersion: tls.VersionTLS12}Note: this shares the same root cause as the guest-facing TLS config in
mitm_proxy.go; see the consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm.go` around lines 188 - 206, Update resolveUpstreamTLS to set an explicit TLS 1.2-or-later MinVersion when constructing the default upstream tls.Config, while preserving the override behavior and existing ServerName and RootCAs handling.Source: Linters/SAST tools
src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go (1)
27-32: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winGuest-facing TLS config has no explicit
MinVersion.Same root cause as
resolveUpstreamTLSinmitm.go; see the consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go` around lines 27 - 32, Update the guest-facing TLS configuration in the tls.Server setup to explicitly set the same minimum TLS version used by resolveUpstreamTLS in mitm.go. Preserve the existing certificate callback and protocol configuration while applying the established minimum-version setting.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go`:
- Around line 675-691: Remove the stale TDD note claiming
SecretHostMatcher.Matches is a stub from TestMitmProxy_NoSecretHost_Passthrough,
leaving the existing assertions and behavior unchanged.
In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go`:
- Around line 74-84: Update the HTTP/1.1 server setup in the mitm proxy path so
the listener is closed from an http.Server ConnState callback when the
connection reaches StateClosed, also handling StateHijacked if required. This
must unblock the pending Accept() during Serve and allow the serving goroutine
to exit; keep the existing srv.Close cleanup afterward.
---
Nitpick comments:
In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go`:
- Around line 27-32: Update the guest-facing TLS configuration in the tls.Server
setup to explicitly set the same minimum TLS version used by resolveUpstreamTLS
in mitm.go. Preserve the existing certificate callback and protocol
configuration while applying the established minimum-version setting.
In `@src/libgvproxy-sys/gvproxy-bridge/mitm_websocket.go`:
- Around line 50-55: Update the upstream connection setup around net.Dial in the
websocket handler to use net.DialTimeout or a net.Dialer with an explicit
timeout, preserving the existing error logging and HTTP 502 response when the
connection cannot be established.
In `@src/libgvproxy-sys/gvproxy-bridge/mitm.go`:
- Line 1: Set MinVersion to tls.VersionTLS12 in both MITM TLS configurations:
the upstream tls.Config created by resolveUpstreamTLS and the guest-facing
tls.Config passed to tls.Server in the MITM proxy. Preserve all existing TLS
configuration behavior while enforcing TLS 1.2 or newer on both connections.
- Around line 188-206: Update resolveUpstreamTLS to set an explicit TLS
1.2-or-later MinVersion when constructing the default upstream tls.Config, while
preserving the override behavior and existing ServerName and RootCAs handling.
In `@src/libgvproxy-sys/src/lib.rs`:
- Line 87: Update gvproxy_set_log_callback to accept Option<LogCallbackFn>
instead of *const c_void, using the existing LogCallbackFn type for compile-time
callback validation and None for a null callback. Verify the exported C/Go
signature accepts a plain function pointer so the FFI ABI remains compatible,
and remove any now-unnecessary pointer casting at call sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31b39b74-7791-42bd-b7d6-daa9471a7b33
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locksrc/libgvproxy-sys/gvproxy-bridge/go.sumis excluded by!**/*.sum
📒 Files selected for processing (80)
.github/workflows/README.md.github/workflows/build-c.yml.github/workflows/build-runtime.yml.github/workflows/lint.yml.github/workflows/test.yml.gitmodulesCargo.tomlapps/dashboard/src/lib/quickstart/interfaces.jsoncodecov.ymldocs/getting-started/quickstart-c.mddocs/reference/c/README.mdexamples/c/CMakeLists.txtexamples/c/README.mdmake/build.mkmake/dev.mkmake/dist.mkmake/quality.mkmake/vars.mkscripts/build/build-bubblewrap.shscripts/build/build-e2fsprogs.shscripts/build/build-libkrunfw.shscripts/build/build-runtime-assets.shscripts/build/build-runtime.shscripts/build/build-shim.shscripts/test/e2e/bootstrap.shsdks/c/README.mdsrc/boxlite/Cargo.tomlsrc/boxlite/build.rssrc/boxlite/deps/README.mdsrc/boxlite/src/jailer/bwrap.rssrc/boxlite/src/jailer/sandbox/seatbelt.rssrc/boxlite/src/net/socket_path.rssrc/boxlite/src/system_check.rssrc/boxlite/src/util/mod.rssrc/boxlite/src/vmm/krun/constants.rssrc/boxlite/src/vmm/krun/context.rssrc/boxlite/src/vmm/krun/kernel.rssrc/boxlite/tests/network_spec.rssrc/cli/tests/gvproxy_port_conflict.rssrc/deps/bubblewrap-sys/Cargo.tomlsrc/deps/bubblewrap-sys/build.rssrc/deps/bubblewrap-sys/src/lib.rssrc/deps/e2fsprogs-sys/Cargo.tomlsrc/deps/e2fsprogs-sys/build.rssrc/deps/e2fsprogs-sys/src/lib.rssrc/deps/libkrun-sys/Cargo.tomlsrc/deps/libkrun-sys/build.rssrc/deps/libkrun-sys/src/lib.rssrc/libgvproxy-sys/Cargo.tomlsrc/libgvproxy-sys/build.rssrc/libgvproxy-sys/gvproxy-bridge/config_build_test.gosrc/libgvproxy-sys/gvproxy-bridge/dns_filter.gosrc/libgvproxy-sys/gvproxy-bridge/dns_filter_test.gosrc/libgvproxy-sys/gvproxy-bridge/forked_network.gosrc/libgvproxy-sys/gvproxy-bridge/forked_network_test.gosrc/libgvproxy-sys/gvproxy-bridge/forked_tcp.gosrc/libgvproxy-sys/gvproxy-bridge/forked_tcp_test.gosrc/libgvproxy-sys/gvproxy-bridge/go.modsrc/libgvproxy-sys/gvproxy-bridge/gvproxy_create_latency_test.gosrc/libgvproxy-sys/gvproxy-bridge/main.gosrc/libgvproxy-sys/gvproxy-bridge/mitm.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_proxy.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_replacer.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_replacer_test.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_test.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_test_helpers_test.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_websocket.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_websocket_test.gosrc/libgvproxy-sys/gvproxy-bridge/sni_peek.gosrc/libgvproxy-sys/gvproxy-bridge/sni_peek_test.gosrc/libgvproxy-sys/gvproxy-bridge/stats.gosrc/libgvproxy-sys/gvproxy-bridge/tcp_filter.gosrc/libgvproxy-sys/gvproxy-bridge/tcp_filter_test.gosrc/libgvproxy-sys/src/lib.rssrc/shim/build.rsvendor/bubblewrapvendor/e2fsprogsvendor/libkrunvendor/libkrunfw
💤 Files with no reviewable changes (12)
- src/deps/e2fsprogs-sys/src/lib.rs
- src/deps/e2fsprogs-sys/build.rs
- src/deps/bubblewrap-sys/build.rs
- src/deps/libkrun-sys/Cargo.toml
- src/deps/bubblewrap-sys/src/lib.rs
- src/deps/e2fsprogs-sys/Cargo.toml
- src/shim/build.rs
- src/deps/bubblewrap-sys/Cargo.toml
- .github/workflows/test.yml
- src/boxlite/deps/README.md
- src/deps/libkrun-sys/src/lib.rs
- src/deps/libkrun-sys/build.rs
🚧 Files skipped from review as they are similar to previous changes (40)
- make/vars.mk
- scripts/test/e2e/bootstrap.sh
- src/libgvproxy-sys/gvproxy-bridge/go.mod
- src/libgvproxy-sys/gvproxy-bridge/forked_network_test.go
- make/dist.mk
- src/libgvproxy-sys/gvproxy-bridge/config_build_test.go
- codecov.yml
- src/libgvproxy-sys/gvproxy-bridge/dns_filter_test.go
- src/boxlite/src/jailer/bwrap.rs
- src/libgvproxy-sys/gvproxy-bridge/gvproxy_create_latency_test.go
- make/dev.mk
- make/build.mk
- .github/workflows/build-c.yml
- src/boxlite/src/vmm/krun/constants.rs
- src/cli/tests/gvproxy_port_conflict.rs
- .github/workflows/lint.yml
- src/boxlite/src/vmm/krun/kernel.rs
- .github/workflows/README.md
- src/libgvproxy-sys/gvproxy-bridge/forked_network.go
- make/quality.mk
- src/libgvproxy-sys/gvproxy-bridge/dns_filter.go
- scripts/build/build-runtime.sh
- src/libgvproxy-sys/build.rs
- Cargo.toml
- src/boxlite/src/system_check.rs
- apps/dashboard/src/lib/quickstart/interfaces.json
- examples/c/CMakeLists.txt
- sdks/c/README.md
- src/boxlite/tests/network_spec.rs
- examples/c/README.md
- src/boxlite/src/jailer/sandbox/seatbelt.rs
- docs/reference/c/README.md
- .github/workflows/build-runtime.yml
- src/boxlite/src/vmm/krun/context.rs
- src/libgvproxy-sys/gvproxy-bridge/mitm_replacer.go
- src/libgvproxy-sys/gvproxy-bridge/forked_tcp.go
- src/boxlite/src/net/socket_path.rs
- src/boxlite/Cargo.toml
- src/libgvproxy-sys/gvproxy-bridge/main.go
- src/boxlite/build.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/libgvproxy-sys/gvproxy-bridge/mitm_websocket.go (1)
50-55: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a dial timeout for the upstream connection.
net.Dialblocks with no timeout, so a slow or unreachable upstream ties up the handler goroutine indefinitely. Prefernet.DialTimeout(or anet.DialerwithTimeout/context) so failed connects fail fast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm_websocket.go` around lines 50 - 55, Update the upstream connection setup around net.Dial in the websocket handler to use net.DialTimeout or a net.Dialer with an explicit timeout, preserving the existing error logging and HTTP 502 response when the connection cannot be established.src/libgvproxy-sys/src/lib.rs (1)
87-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider typing the callback as
Option<LogCallbackFn>.You define
LogCallbackFnbut the registration function takes an untyped*const c_void, forcing callers to transmute a function pointer.Option<LogCallbackFn>is FFI-safe, encodes NULL asNone, and gives compile-time type checking at call sites.♻️ Proposed change
- pub fn gvproxy_set_log_callback(callback: *const c_void); + pub fn gvproxy_set_log_callback(callback: Option<LogCallbackFn>);Confirm the C/Go export signature accepts a plain function pointer so this remains ABI-compatible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/src/lib.rs` at line 87, Update gvproxy_set_log_callback to accept Option<LogCallbackFn> instead of *const c_void, using the existing LogCallbackFn type for compile-time callback validation and None for a null callback. Verify the exported C/Go signature accepts a plain function pointer so the FFI ABI remains compatible, and remove any now-unnecessary pointer casting at call sites.src/libgvproxy-sys/gvproxy-bridge/mitm.go (2)
1-1: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin
MinVersionacross the MITM TLS configs. Both TLS configs used in the MITM path omit an explicitMinVersion, relying on Go's default minimums (TLS 1.2 for client role, TLS 1.0 for server role). Since this proxy handles substituted API secrets, explicitly requiring TLS 1.2+ end-to-end is a small, consistent hardening step.
src/libgvproxy-sys/gvproxy-bridge/mitm.go#L188-206: addMinVersion: tls.VersionTLS12to thetls.Configbuilt inresolveUpstreamTLSfor upstream connections.src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go#L27-32: addMinVersion: tls.VersionTLS12to the guest-facingtls.Configpassed totls.Server.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm.go` at line 1, Set MinVersion to tls.VersionTLS12 in both MITM TLS configurations: the upstream tls.Config created by resolveUpstreamTLS and the guest-facing tls.Config passed to tls.Server in the MITM proxy. Preserve all existing TLS configuration behavior while enforcing TLS 1.2 or newer on both connections.Source: Linters/SAST tools
188-206: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet an explicit
MinVersionon the upstream TLS config.
resolveUpstreamTLSbuilds atls.Configfor connecting to real upstream hosts without pinningMinVersion. Go's client default minimum is TLS 1.2, but this MITM proxy carries substituted API secrets, so explicitly requiring TLS 1.2+ (or 1.3) is worthwhile defense-in-depth.🔒 Proposed fix
- cfg := &tls.Config{ServerName: hostname} + cfg := &tls.Config{ServerName: hostname, MinVersion: tls.VersionTLS12}Note: this shares the same root cause as the guest-facing TLS config in
mitm_proxy.go; see the consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm.go` around lines 188 - 206, Update resolveUpstreamTLS to set an explicit TLS 1.2-or-later MinVersion when constructing the default upstream tls.Config, while preserving the override behavior and existing ServerName and RootCAs handling.Source: Linters/SAST tools
src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go (1)
27-32: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winGuest-facing TLS config has no explicit
MinVersion.Same root cause as
resolveUpstreamTLSinmitm.go; see the consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go` around lines 27 - 32, Update the guest-facing TLS configuration in the tls.Server setup to explicitly set the same minimum TLS version used by resolveUpstreamTLS in mitm.go. Preserve the existing certificate callback and protocol configuration while applying the established minimum-version setting.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go`:
- Around line 675-691: Remove the stale TDD note claiming
SecretHostMatcher.Matches is a stub from TestMitmProxy_NoSecretHost_Passthrough,
leaving the existing assertions and behavior unchanged.
In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go`:
- Around line 74-84: Update the HTTP/1.1 server setup in the mitm proxy path so
the listener is closed from an http.Server ConnState callback when the
connection reaches StateClosed, also handling StateHijacked if required. This
must unblock the pending Accept() during Serve and allow the serving goroutine
to exit; keep the existing srv.Close cleanup afterward.
---
Nitpick comments:
In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go`:
- Around line 27-32: Update the guest-facing TLS configuration in the tls.Server
setup to explicitly set the same minimum TLS version used by resolveUpstreamTLS
in mitm.go. Preserve the existing certificate callback and protocol
configuration while applying the established minimum-version setting.
In `@src/libgvproxy-sys/gvproxy-bridge/mitm_websocket.go`:
- Around line 50-55: Update the upstream connection setup around net.Dial in the
websocket handler to use net.DialTimeout or a net.Dialer with an explicit
timeout, preserving the existing error logging and HTTP 502 response when the
connection cannot be established.
In `@src/libgvproxy-sys/gvproxy-bridge/mitm.go`:
- Line 1: Set MinVersion to tls.VersionTLS12 in both MITM TLS configurations:
the upstream tls.Config created by resolveUpstreamTLS and the guest-facing
tls.Config passed to tls.Server in the MITM proxy. Preserve all existing TLS
configuration behavior while enforcing TLS 1.2 or newer on both connections.
- Around line 188-206: Update resolveUpstreamTLS to set an explicit TLS
1.2-or-later MinVersion when constructing the default upstream tls.Config, while
preserving the override behavior and existing ServerName and RootCAs handling.
In `@src/libgvproxy-sys/src/lib.rs`:
- Line 87: Update gvproxy_set_log_callback to accept Option<LogCallbackFn>
instead of *const c_void, using the existing LogCallbackFn type for compile-time
callback validation and None for a null callback. Verify the exported C/Go
signature accepts a plain function pointer so the FFI ABI remains compatible,
and remove any now-unnecessary pointer casting at call sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31b39b74-7791-42bd-b7d6-daa9471a7b33
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locksrc/libgvproxy-sys/gvproxy-bridge/go.sumis excluded by!**/*.sum
📒 Files selected for processing (80)
.github/workflows/README.md.github/workflows/build-c.yml.github/workflows/build-runtime.yml.github/workflows/lint.yml.github/workflows/test.yml.gitmodulesCargo.tomlapps/dashboard/src/lib/quickstart/interfaces.jsoncodecov.ymldocs/getting-started/quickstart-c.mddocs/reference/c/README.mdexamples/c/CMakeLists.txtexamples/c/README.mdmake/build.mkmake/dev.mkmake/dist.mkmake/quality.mkmake/vars.mkscripts/build/build-bubblewrap.shscripts/build/build-e2fsprogs.shscripts/build/build-libkrunfw.shscripts/build/build-runtime-assets.shscripts/build/build-runtime.shscripts/build/build-shim.shscripts/test/e2e/bootstrap.shsdks/c/README.mdsrc/boxlite/Cargo.tomlsrc/boxlite/build.rssrc/boxlite/deps/README.mdsrc/boxlite/src/jailer/bwrap.rssrc/boxlite/src/jailer/sandbox/seatbelt.rssrc/boxlite/src/net/socket_path.rssrc/boxlite/src/system_check.rssrc/boxlite/src/util/mod.rssrc/boxlite/src/vmm/krun/constants.rssrc/boxlite/src/vmm/krun/context.rssrc/boxlite/src/vmm/krun/kernel.rssrc/boxlite/tests/network_spec.rssrc/cli/tests/gvproxy_port_conflict.rssrc/deps/bubblewrap-sys/Cargo.tomlsrc/deps/bubblewrap-sys/build.rssrc/deps/bubblewrap-sys/src/lib.rssrc/deps/e2fsprogs-sys/Cargo.tomlsrc/deps/e2fsprogs-sys/build.rssrc/deps/e2fsprogs-sys/src/lib.rssrc/deps/libkrun-sys/Cargo.tomlsrc/deps/libkrun-sys/build.rssrc/deps/libkrun-sys/src/lib.rssrc/libgvproxy-sys/Cargo.tomlsrc/libgvproxy-sys/build.rssrc/libgvproxy-sys/gvproxy-bridge/config_build_test.gosrc/libgvproxy-sys/gvproxy-bridge/dns_filter.gosrc/libgvproxy-sys/gvproxy-bridge/dns_filter_test.gosrc/libgvproxy-sys/gvproxy-bridge/forked_network.gosrc/libgvproxy-sys/gvproxy-bridge/forked_network_test.gosrc/libgvproxy-sys/gvproxy-bridge/forked_tcp.gosrc/libgvproxy-sys/gvproxy-bridge/forked_tcp_test.gosrc/libgvproxy-sys/gvproxy-bridge/go.modsrc/libgvproxy-sys/gvproxy-bridge/gvproxy_create_latency_test.gosrc/libgvproxy-sys/gvproxy-bridge/main.gosrc/libgvproxy-sys/gvproxy-bridge/mitm.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_proxy.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_replacer.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_replacer_test.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_test.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_test_helpers_test.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_websocket.gosrc/libgvproxy-sys/gvproxy-bridge/mitm_websocket_test.gosrc/libgvproxy-sys/gvproxy-bridge/sni_peek.gosrc/libgvproxy-sys/gvproxy-bridge/sni_peek_test.gosrc/libgvproxy-sys/gvproxy-bridge/stats.gosrc/libgvproxy-sys/gvproxy-bridge/tcp_filter.gosrc/libgvproxy-sys/gvproxy-bridge/tcp_filter_test.gosrc/libgvproxy-sys/src/lib.rssrc/shim/build.rsvendor/bubblewrapvendor/e2fsprogsvendor/libkrunvendor/libkrunfw
💤 Files with no reviewable changes (12)
- src/deps/e2fsprogs-sys/src/lib.rs
- src/deps/e2fsprogs-sys/build.rs
- src/deps/bubblewrap-sys/build.rs
- src/deps/libkrun-sys/Cargo.toml
- src/deps/bubblewrap-sys/src/lib.rs
- src/deps/e2fsprogs-sys/Cargo.toml
- src/shim/build.rs
- src/deps/bubblewrap-sys/Cargo.toml
- .github/workflows/test.yml
- src/boxlite/deps/README.md
- src/deps/libkrun-sys/src/lib.rs
- src/deps/libkrun-sys/build.rs
🚧 Files skipped from review as they are similar to previous changes (40)
- make/vars.mk
- scripts/test/e2e/bootstrap.sh
- src/libgvproxy-sys/gvproxy-bridge/go.mod
- src/libgvproxy-sys/gvproxy-bridge/forked_network_test.go
- make/dist.mk
- src/libgvproxy-sys/gvproxy-bridge/config_build_test.go
- codecov.yml
- src/libgvproxy-sys/gvproxy-bridge/dns_filter_test.go
- src/boxlite/src/jailer/bwrap.rs
- src/libgvproxy-sys/gvproxy-bridge/gvproxy_create_latency_test.go
- make/dev.mk
- make/build.mk
- .github/workflows/build-c.yml
- src/boxlite/src/vmm/krun/constants.rs
- src/cli/tests/gvproxy_port_conflict.rs
- .github/workflows/lint.yml
- src/boxlite/src/vmm/krun/kernel.rs
- .github/workflows/README.md
- src/libgvproxy-sys/gvproxy-bridge/forked_network.go
- make/quality.mk
- src/libgvproxy-sys/gvproxy-bridge/dns_filter.go
- scripts/build/build-runtime.sh
- src/libgvproxy-sys/build.rs
- Cargo.toml
- src/boxlite/src/system_check.rs
- apps/dashboard/src/lib/quickstart/interfaces.json
- examples/c/CMakeLists.txt
- sdks/c/README.md
- src/boxlite/tests/network_spec.rs
- examples/c/README.md
- src/boxlite/src/jailer/sandbox/seatbelt.rs
- docs/reference/c/README.md
- .github/workflows/build-runtime.yml
- src/boxlite/src/vmm/krun/context.rs
- src/libgvproxy-sys/gvproxy-bridge/mitm_replacer.go
- src/libgvproxy-sys/gvproxy-bridge/forked_tcp.go
- src/boxlite/src/net/socket_path.rs
- src/boxlite/Cargo.toml
- src/libgvproxy-sys/gvproxy-bridge/main.go
- src/boxlite/build.rs
🛑 Comments failed to post (2)
src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go (1)
675-691: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale "stub" comment —
Matchesis already implemented.The comment claims this assertion "will fail with stub since Matches always returns false," but
SecretHostMatcher.Matchesis fully implemented inmitm.go. Leftover from earlier TDD scaffolding; misleading for future readers.🧹 Proposed fix
if !matcher.Matches("api.example.com") { - // Note: this will fail with stub since Matches always returns false. - // That's expected — will pass once implemented. t.Error("expected api.example.com to match as a secret host") }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func TestMitmProxy_NoSecretHost_Passthrough(t *testing.T) { // Verify that SecretHostMatcher correctly identifies non-secret hosts secrets := testSecrets() matcher := NewSecretHostMatcher(secrets) // api.example.com is in secrets' Hosts list if !matcher.Matches("api.example.com") { t.Error("expected api.example.com to match as a secret host") } // random.example.com is NOT in any secret's Hosts list if matcher.Matches("random.example.com") { t.Error("expected random.example.com to NOT match as a secret host") } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go` around lines 675 - 691, Remove the stale TDD note claiming SecretHostMatcher.Matches is a stub from TestMitmProxy_NoSecretHost_Passthrough, leaving the existing assertions and behavior unchanged.src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go (1)
74-84: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the target file and relevant symbols. git ls-files src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go echo '--- mitm_proxy.go outline ---' ast-grep outline src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go --view expanded || true echo '--- singleConnListener refs ---' rg -n "singleConnListener|newSingleConnListener|ConnState|Serve\\(listener\\)|srv\\.Close\\(" src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.goRepository: boxlite-ai/boxlite
Length of output: 1922
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the relevant section with line numbers. sed -n '1,180p' src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go | cat -nRepository: boxlite-ai/boxlite
Length of output: 1922
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the tests that cover the HTTP/1.1 path and connection teardown. sed -n '1,260p' src/libgvproxy-sys/gvproxy-bridge/mitm_proxy_test.go | cat -nRepository: boxlite-ai/boxlite
Length of output: 1922
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check the standard library semantics from the local Go doc if available. python3 - <<'PY' import os, subprocess, textwrap, sys for cmd in [ ["go", "doc", "net/http", "Server.Serve"], ["go", "doc", "net/http", "Server.Close"], ["go", "doc", "net/http", "Server.Serve", "|", "cat"], ]: pass PYRepository: boxlite-ai/boxlite
Length of output: 1922
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,140p' src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go | cat -nRepository: boxlite-ai/boxlite
Length of output: 1922
🌐 Web query:
Go net/http Server.Serve Accept loop returns only on listener error connstate💡 Result:
In the Go
net/httppackage, theServer.Servemethod operates an accept loop that callsListener.Accept()to handle incoming network connections [1][2]. Contrary to the premise that it only returns on a listener error, theAcceptloop is designed to handle transient errors and will only terminate and return a non-nil error when a non-recoverable error occurs or the server is closed [3][1][2]. Regarding error handling and connection state: 1. Transient Error Handling:Server.Serveincludes logic to distinguish between transient and permanentAccepterrors [4][5]. Historically, this was implemented using the now-deprecatednet.Error.Temporary()method [4][6]. If anAccepterror is considered "temporary" (e.g., resource exhaustion errors likeEMFILEorENFILE), the server typically implements a small backoff delay and continues the loop rather than returning [4][5][6]. 2. Returning from Serve:Server.Servereturns when theListeneritself returns a permanent error [3]. This includesnet.ErrClosed(returned when the listener is closed, such as during a shutdown) or other fatal network errors [3][2]. Documentation states thatServer.Servealways returns a non-nil error; if the server is shut down gracefully viaServer.ShutdownorServer.Close, it returnshttp.ErrServerClosed[1][2]. 3. ConnState: TheConnStatehook (configured viaServer.ConnState) is not involved in the logic of theAcceptloop's return or error handling [7][8]. Instead,ConnStateis triggered inside the individual connection handling goroutines (started afterAcceptsucceeds) to track the lifecycle of a connection (e.g.,StateNew,StateActive,StateIdle,StateClosed) [7][9]. It does not influence the loop's decision to continue or exit [7]. In summary, theAcceptloop does not return onConnStatechanges; it continues to run until the listener itself reports a fatal error or is closed [3][1][2]. Transient errors are caught and retried, while fatal errors causeServeto return [4][5][3].Citations:
- 1: https://pkg.go.dev/net/[email protected]
- 2: https://pkg.go.dev/net/http
- 3: golang/go#11219
- 4: golang/go#66208
- 5: golang/go#66252
- 6: https://groups.google.com/g/golang-nuts/c/-JcZzOkyqYI
- 7: https://go.dev/src/net/http/server.go?m=text
- 8: https://github.com/golang/go/blob/master/src/net/http/server.go
- 9: golang/go#4674
HTTP/1.1 path blocks forever in
Serve()
singleConnListener.Accept()returns once and then waits, sosrv.Serve(listener)never reachessrv.Close()as written. Close the listener fromConnStateonStateClosed(andStateHijackedif needed) so the pendingAccept()returns and the goroutine can exit.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 78-78: This
http.Serveris constructed without aReadTimeout. Without a read timeout, a slow or malicious client can hold connections open indefinitely (e.g. a Slowloris attack), exhausting server resources and causing a denial of service. SetReadTimeout(and ideallyReadHeaderTimeout,WriteTimeout, andIdleTimeout) on thehttp.Serverto bound how long the server waits while reading a request.
Context: http.Server{Handler: proxy}
Note: [CWE-400] Uncontrolled Resource Consumption.(http-server-missing-read-timeout-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libgvproxy-sys/gvproxy-bridge/mitm_proxy.go` around lines 74 - 84, Update the HTTP/1.1 server setup in the mitm proxy path so the listener is closed from an http.Server ConnState callback when the connection reaches StateClosed, also handling StateHijacked if required. This must unblock the pending Accept() during Serve and allow the serving goroutine to exit; keep the existing srv.Close cleanup afterward.
makes sense, thx |
Summary
Separates Rust dependencies from runtime assets.
libkrunis now a normal Cargo dependency, while e2fsprogs, bubblewrap, and libkrunfw are built explicitly before runtime assembly. This removes the*-syscrates that existed only to trigger native builds and substantially simplifiesboxlite/build.rs.This also removes all executable rpath injection from the runtime and C consumer paths. The shim resolves its copied sidecar libraries through a sandbox-scoped library search path, while C quickstarts and examples link the prepared static
libboxlite.adirectly. Dynamic C linking remains documented as an explicit alternative.Ref #848, close #927.
Changes
libkrun-syswith a direct dependency onlibkrun.e2fsprogs-sysandbubblewrap-sys.vendor/.make runtime-assets.BOXLITE_DEPS_STUBand Cargo JSON/sed-based runtime discovery.BOXLITE_RUNTIME_DIRandBOXLITE_RUNTIME_URLoverrides.-krun.sockendpoint in the existing VM network test.libgvproxy-syswith its existing excluded-source behavior and document its final-link limitation.@loader_pathand Linux$ORIGINrpaths.-Wl,-rpath,BUILD_RPATH,INSTALL_RPATH, andinstall_name_tool -add_rpathusage from C consumer paths.DYLD_LIBRARY_PATHandLD_LIBRARY_PATHto the copied shim directory inside the macOS Seatbelt and Linux bubblewrap launch paths, without inheriting host search paths.dev:canddist:centrypoints, including the Go symbol fix and bundled unwinder when required; the C workflow now callsmake dist:cinstead of patching the archive separately.libboxlite.a; keep both static and explicit dynamic linking examples in the C reference documentation.Verification
git diff --checkbash -n scripts/build/fix-go-symbols.shfix-go-symbols.shtwice is idempotent.simple_api_demo.clinks using only the preparedlibboxlite.a, without-lunwindor other added native dependency flags.Summary by CodeRabbit