Expose inbound network state in NetworkInfo (read side) - #1198
Closed
G4614 wants to merge 30 commits into
Closed
Conversation
This reverts commit 2cd1e45.
Rebase auto-merged new UDP allow_net tests (added upstream on main)
that still constructed the pre-refactor NetworkSpec::Enabled { .. }
enum variant. NetworkSpec is now a struct with outbound/inbound
fields; switch these call sites to the NetworkSpec::enabled() helper
already used elsewhere in this file.
Replace InboundNetworkSpec{service_access: Option<ServiceAccess>} with
Enabled{allow_net}|Disabled, mirroring OutboundNetworkSpec exactly:
- Disabled = private (unreachable from outside)
- Enabled with empty allow_net = fully open (any caller may reach in)
- Enabled with non-empty allow_net = restrict inbound reachability to
those hosts/IPs
Propagated across all surfaces: core Rust types, REST wire types, CLI
(new --inbound / --inbound-allow-net flags), serve command, C/Node/Python
FFI bindings, Go SDK, REST DTO + OpenAPI schema, and e2e tests.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
inbound.allow_net is accepted and stored, but no runtime sink (apps/proxy, port_publish.rs, gvproxy) restricts inbound reachability by it yet — only inbound.mode is enforced. Validate allow_net in the same place outbound already validates it (NetworkSpec::try_from), and warn on a non-empty value under mode=enabled so a caller doesn't assume it's already restricting who can connect in. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
NetworkInfo only ever surfaced outbound's mode/allow_net; inbound's
current state (set via --inbound/--inbound-allow-net) had no output
field at all. Reshape NetworkInfo into {outbound, inbound,
published_ports}, mirroring NetworkSpec's shape, so box.info()/list()
can report both directions.
Before:
NetworkInfo{mode, allow_net} ← BUG: only ever outbound; inbound
configured via NetworkSpec but unreadable back out
After:
NetworkInfo{outbound: NetworkDirectionInfo, inbound: NetworkDirectionInfo,
published_ports}
Propagated through the C/Go/Node/Python SDK bindings and the C header
(regenerated by cbindgen).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📦 BoxLite review — couldn't completepowered by BoxLite |
📝 WalkthroughWalkthroughThe PR replaces flat network configuration with separate outbound and inbound policies across the runtime, REST API, CLI, C SDK, Go SDK, Node SDK, and Python SDK. It adds directional validation, metadata conversion, inbound access controls, compatibility handling, documentation, and tests. ChangesDirectional network runtime
REST network API
CLI network configuration
C and Go SDK bindings
Node and Python SDK bindings
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
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/runtime/options.rs (1)
584-589: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the error text to name the nested field.
The check now reads
self.network.outbound, but the message still namesnetwork.mode. The wire shape isnetwork.outbound.mode. A caller who follows this message edits the wrong field.🔧 Proposed fix
- "ports require network.mode=\"enabled\"".to_string(), + "ports require network.outbound.mode=\"enabled\"".to_string(),Check whether any test asserts on the old string before you change it.
🤖 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/runtime/options.rs` around lines 584 - 589, Update the Config error message in the outbound-network validation condition to reference the correct nested field, network.outbound.mode, instead of network.mode. Search tests for assertions on the old error string and update them consistently if present.
🧹 Nitpick comments (3)
sdks/c/src/event_queue.rs (1)
1198-1201: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExercise inbound allowlist ownership.
Use at least one inbound
allow_netvalue in this fixture. Update the expected freed-string count. The test must cover destruction of the new inbound string allocation path.🤖 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 `@sdks/c/src/event_queue.rs` around lines 1198 - 1201, Update the fixture’s inbound NetworkDirectionInfo to include at least one allow_net value, then adjust the expected freed-string count to account for that allocation. Ensure the test exercises destruction of the inbound allowlist string through the cleanup path.src/boxlite/tests/network_spec.rs (1)
24-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the inbound side in the round-trip tests.
Both round-trip tests only inspect
rt.outbound. A regression ininboundserialization or in the customDeserializepasses these tests.💚 Proposed test addition
fn serde_disabled_roundtrip() { let spec = NetworkSpec::disabled(); let json = serde_json::to_string(&spec).unwrap(); let rt: NetworkSpec = serde_json::from_str(&json).unwrap(); assert!(matches!(rt.outbound, OutboundNetworkSpec::Disabled)); + assert!( + matches!(rt.inbound, InboundNetworkSpec::Enabled { ref allow_net } if allow_net.is_empty()), + "disabled() keeps the default public inbound policy across a round trip" + ); }Add
InboundNetworkSpecto the import at Line 6.🤖 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/tests/network_spec.rs` around lines 24 - 41, Update serde_enabled_roundtrip and serde_disabled_roundtrip to assert the deserialized rt.inbound value as well as rt.outbound, covering the expected inbound variant for each NetworkSpec configuration. Add InboundNetworkSpec to the existing imports and preserve the current outbound assertions.src/boxlite/src/rest/types.rs (1)
211-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the two direction structs into one.
CreateBoxOutboundNetworkSpecandCreateBoxInboundNetworkSpecdeclare identical fields and identical serde attributes. The core crate already uses one sharedNetworkDirectionInfotype for both directions. One shared struct keeps the two directions from drifting when a field is added later.♻️ Proposed refactor
pub(crate) struct CreateBoxNetworkSpec { - pub outbound: CreateBoxOutboundNetworkSpec, - pub inbound: CreateBoxInboundNetworkSpec, + pub outbound: CreateBoxNetworkDirectionSpec, + pub inbound: CreateBoxNetworkDirectionSpec, } #[derive(Debug, Serialize)] -pub(crate) struct CreateBoxOutboundNetworkSpec { - pub mode: String, - #[serde(skip_serializing_if = "Vec::is_empty")] - pub allow_net: Vec<String>, -} - -#[derive(Debug, Serialize)] -pub(crate) struct CreateBoxInboundNetworkSpec { +pub(crate) struct CreateBoxNetworkDirectionSpec { pub mode: String, #[serde(skip_serializing_if = "Vec::is_empty")] pub allow_net: Vec<String>, }The serialized wire shape does not change.
🤖 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/rest/types.rs` around lines 211 - 230, Replace the duplicate CreateBoxOutboundNetworkSpec and CreateBoxInboundNetworkSpec declarations with one shared network specification struct, reusing the existing NetworkDirectionInfo type where appropriate. Update both direction-specific call sites and type references to use the shared struct while preserving the mode field, allow_net serde behavior, and serialized wire shape.
🤖 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 `@apps/api/src/boxlite-rest/mappers/box-to-box.mapper.ts`:
- Around line 53-58: The mapper must not convert an enabled inbound policy with
a non-empty allow_net into unrestricted public: true; either add and enforce a
downstream allowlist field or reject such requests before mapping, with
rejection being the smallest supported change. In
apps/api/src/boxlite-rest/mappers/box-to-box.mapper.ts lines 53-58, implement
that validation before assigning createDto.public. Update
apps/api/src/boxlite-rest/dto/create-box.dto.spec.ts lines 179-193 to test the
enforced rejection or supported behavior, and update openapi/box.openapi.yaml
lines 1779-1786 to remove claims that inbound allowlists provide access
restrictions unless enforcement is implemented.
In `@sdks/c/include/boxlite.h`:
- Around line 781-785: Update the comment for
boxlite_options_set_auto_stop_interval so it states that an idle box may remain
idle before the runtime pauses it, replacing the incorrect “paused” wording
while preserving the rest of the description.
In `@sdks/c/src/info.rs`:
- Around line 60-70: Update the C SDK ABI/version metadata associated with
CNetworkInfo to increment the ABI version or SONAME beyond 0.9.7, ensuring
consumers can distinguish the incompatible struct layout and avoid using stale
field offsets.
In `@sdks/c/src/options.rs`:
- Around line 442-456: Update options_set_network_enabled and
options_set_network_disabled to mutate only the outbound/network-enabled state
while preserving the existing inbound policy, matching the behavior of the
inbound setters at Lines 472-488. Add a test that configures inbound as disabled
before setting outbound, then verifies inbound remains Disabled regardless of
setter order.
- Around line 180-183: Update the documentation comment for the idle auto-stop
option near the relevant options API so it describes the idle duration before
the runtime stops or auto-stops the box, rather than saying it pauses an already
paused box. Preserve the existing seconds, zero-default, and null-pointer
behavior descriptions.
In `@sdks/go/options.go`:
- Around line 497-510: Reject any non-empty cfg.network.Inbound.AllowNet in the
inbound network configuration flow of sdks/go/options.go (lines 497-510) until
runtime enforcement exists, returning the existing cleanup/error pattern rather
than silently accepting it. Update the public documentation in
sdks/c/include/boxlite.h (lines 766-769) and sdks/go/options.go (lines 82-90) to
remove claims that AllowNet restricts inbound callers and accurately describe
its unsupported or unenforced behavior.
- Around line 303-313: Add a comprehensive Go doc comment for the public
WithNetwork function, explicitly documenting its behavior and stating that it
copies both outbound and inbound allowlists. Keep the implementation unchanged.
In `@sdks/node/src/options.rs`:
- Around line 347-359: Correct the inbound allowlist contract across all three
sites: in sdks/node/src/options.rs lines 347-359, update JsInboundNetworkSpec
documentation to state that allowNet is metadata only and does not restrict
callers; in sdks/python/README.md lines 257-258, document outbound and inbound
semantics separately and clarify that inbound allowlists are not access control;
in sdks/python/src/info.rs lines 70-102, add comprehensive Python-visible
documentation stating that allow_net reports configured metadata without
enforcing caller restrictions.
In `@sdks/node/tests/network-secrets.integration.test.ts`:
- Around line 32-35: Add an integration test alongside “disabled network removes
eth0” that creates a SimpleBox with an inbound policy (disabled or an enabled
allowlist), calls await box.getInfo(), and asserts NetworkInfo reports the
configured inbound state, covering Node-to-native conversion and readback.
In `@sdks/python/src/options.rs`:
- Around line 319-322: Update the NetworkSpec conversion logic returning
Ok(Self) to reject inputs where both outbound and inbound are absent, before
applying legacy_outbound or defaults. Preserve existing handling when either
nested policy is supplied, matching the Node SDK’s explicit-network validation
contract.
In `@src/boxlite/src/runtime/options.rs`:
- Around line 714-729: Update the documentation comments for InboundNetworkSpec
and InboundNetworkConfig to state that a non-empty allow_net is currently
accepted but not enforced, so callers must not assume traffic is restricted;
mirror the existing caveat in NetworkDirectionInfo and leave the TryFrom
behavior unchanged.
---
Outside diff comments:
In `@src/boxlite/src/runtime/options.rs`:
- Around line 584-589: Update the Config error message in the outbound-network
validation condition to reference the correct nested field,
network.outbound.mode, instead of network.mode. Search tests for assertions on
the old error string and update them consistently if present.
---
Nitpick comments:
In `@sdks/c/src/event_queue.rs`:
- Around line 1198-1201: Update the fixture’s inbound NetworkDirectionInfo to
include at least one allow_net value, then adjust the expected freed-string
count to account for that allocation. Ensure the test exercises destruction of
the inbound allowlist string through the cleanup path.
In `@src/boxlite/src/rest/types.rs`:
- Around line 211-230: Replace the duplicate CreateBoxOutboundNetworkSpec and
CreateBoxInboundNetworkSpec declarations with one shared network specification
struct, reusing the existing NetworkDirectionInfo type where appropriate. Update
both direction-specific call sites and type references to use the shared struct
while preserving the mode field, allow_net serde behavior, and serialized wire
shape.
In `@src/boxlite/tests/network_spec.rs`:
- Around line 24-41: Update serde_enabled_roundtrip and serde_disabled_roundtrip
to assert the deserialized rt.inbound value as well as rt.outbound, covering the
expected inbound variant for each NetworkSpec configuration. Add
InboundNetworkSpec to the existing imports and preserve the current outbound
assertions.
🪄 Autofix
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: e6c4b8d4-20aa-4444-a96d-fe952fc362fb
📒 Files selected for processing (46)
apps/api/src/boxlite-rest/boxlite-box.controller.tsapps/api/src/boxlite-rest/dto/create-box.dto.spec.tsapps/api/src/boxlite-rest/dto/create-box.dto.tsapps/api/src/boxlite-rest/mappers/box-to-box.mapper.spec.tsapps/api/src/boxlite-rest/mappers/box-to-box.mapper.tsapps/e2e/cases/test_box_management.pyopenapi/box.openapi.yamlsdks/c/README.mdsdks/c/include/boxlite.hsdks/c/src/event_queue.rssdks/c/src/info.rssdks/c/src/options.rssdks/go/boxlite_test.gosdks/go/info.gosdks/go/info_cgo_dev_test.gosdks/go/info_cgo_test_support_dev.gosdks/go/network_secrets_integration_test.gosdks/go/options.gosdks/node/README.mdsdks/node/lib/native-contracts.tssdks/node/lib/simplebox.tssdks/node/src/info.rssdks/node/src/options.rssdks/node/tests/network-secrets.integration.test.tssdks/node/tests/options.test.tssdks/node/tests/skillbox.integration.test.tssdks/python/README.mdsdks/python/boxlite/__init__.pysdks/python/src/info.rssdks/python/src/lib.rssdks/python/src/options.rssdks/python/tests/test_network_spec.pysdks/python/tests/test_secret_substitution.pysdks/python/tests/test_tcp_filter.pysrc/boxlite/src/lib.rssrc/boxlite/src/litebox/init/tasks/guest_init.rssrc/boxlite/src/litebox/init/tasks/vmm_attach.rssrc/boxlite/src/litebox/init/tasks/vmm_spawn.rssrc/boxlite/src/rest/types.rssrc/boxlite/src/runtime/options.rssrc/boxlite/src/runtime/types.rssrc/boxlite/tests/network_spec.rssrc/boxlite/tests/security_enforcement.rssrc/cli/src/cli.rssrc/cli/src/commands/serve/mod.rssrc/cli/src/commands/serve/types.rs
Comment on lines
+53
to
+58
| // The runner DTO only has a public/private boolean — inbound.allow_net | ||
| // has no downstream sink yet (no runner-side host-based inbound | ||
| // restriction), so it isn't mapped here. | ||
| createDto.public = dto.network.inbound?.mode | ||
| ? dto.network.inbound.mode === 'enabled' | ||
| : undefined |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Preserve or reject inbound.allow_net.
An enabled inbound policy with allow_net is mapped to public: true and loses its restriction. This can expose a box to all callers instead of the requested allowlist.
apps/api/src/boxlite-rest/mappers/box-to-box.mapper.ts#L53-L58: Add a downstream field and enforcement forinbound.allow_net, or reject non-empty inbound allowlists before mapping.apps/api/src/boxlite-rest/dto/create-box.dto.spec.ts#L179-L193: Replace the downstream-validation assumption with coverage for the enforced behavior.openapi/box.openapi.yaml#L1779-L1786: Do not document inbound allowlists as access restrictions until the REST creation path enforces them.
📍 Affects 3 files
apps/api/src/boxlite-rest/mappers/box-to-box.mapper.ts#L53-L58(this comment)apps/api/src/boxlite-rest/dto/create-box.dto.spec.ts#L179-L193openapi/box.openapi.yaml#L1779-L1786
🤖 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 `@apps/api/src/boxlite-rest/mappers/box-to-box.mapper.ts` around lines 53 - 58,
The mapper must not convert an enabled inbound policy with a non-empty allow_net
into unrestricted public: true; either add and enforce a downstream allowlist
field or reject such requests before mapping, with rejection being the smallest
supported change. In apps/api/src/boxlite-rest/mappers/box-to-box.mapper.ts
lines 53-58, implement that validation before assigning createDto.public. Update
apps/api/src/boxlite-rest/dto/create-box.dto.spec.ts lines 179-193 to test the
enforced rejection or supported behavior, and update openapi/box.openapi.yaml
lines 1779-1786 to remove claims that inbound allowlists provide access
restrictions unless enforcement is implemented.
Comment on lines
+781
to
785
| // Set how long an idle box may remain paused before the runtime pauses it. | ||
| // | ||
| // The value is expressed in seconds. `0` preserves the runtime/control-plane | ||
| // default. A null options pointer is treated as a no-op. | ||
| void boxlite_options_set_auto_stop_interval(CBoxliteOptions *opts, uint32_t seconds); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the auto-stop description.
Line 781 says that a box remains paused before the runtime pauses it. Replace “paused” with “idle”.
Proposed fix
-// Set how long an idle box may remain paused before the runtime pauses it.
+// Set how long an idle box may remain idle before the runtime pauses it.📝 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.
Suggested change
| // Set how long an idle box may remain paused before the runtime pauses it. | |
| // | |
| // The value is expressed in seconds. `0` preserves the runtime/control-plane | |
| // default. A null options pointer is treated as a no-op. | |
| void boxlite_options_set_auto_stop_interval(CBoxliteOptions *opts, uint32_t seconds); | |
| // Set how long an idle box may remain idle before the runtime pauses it. | |
| // | |
| // The value is expressed in seconds. `0` preserves the runtime/control-plane | |
| // default. A null options pointer is treated as a no-op. | |
| void boxlite_options_set_auto_stop_interval(CBoxliteOptions *opts, uint32_t seconds); |
🤖 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 `@sdks/c/include/boxlite.h` around lines 781 - 785, Update the comment for
boxlite_options_set_auto_stop_interval so it states that an idle box may remain
idle before the runtime pauses it, replacing the incorrect “paused” wording
while preserving the rest of the description.
Comment on lines
+60
to
70
| /// Typed network metadata owned by an enclosing [`CBoxInfo`]. | ||
| /// | ||
| /// `published_ports` is null when the current handle does not know the | ||
| /// bindings, non-null and empty when there are no active publications, and | ||
| /// otherwise contains concrete bindings. | ||
| #[repr(C)] | ||
| pub struct CNetworkInfo { | ||
| pub outbound: CNetworkDirectionInfo, | ||
| pub inbound: CNetworkDirectionInfo, | ||
| pub published_ports: *mut CPublishedPortList, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare the generated C header network structs with the Rust repr(C) definitions.
set -euo pipefail
fd -t f 'boxlite.h' sdks/c --exec rg -n -C6 'CNetworkDirectionInfo|CNetworkInfo' {}
# Version markers for the C SDK.
fd -t f -e toml -e md . sdks/c --exec rg -n -i 'version|abi|soname' {} \; | head -50Repository: boxlite-ai/boxlite
Length of output: 3373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Rust definitions ---'
rg -n -C8 'pub struct CNetwork(DirectionInfo|Info)|struct CNetwork(DirectionInfo|Info)' sdks/c
echo '--- Header definitions ---'
rg -n -C8 'typedef struct CNetwork(DirectionInfo|Info)|struct CNetwork(DirectionInfo|Info)' sdks/c
echo '--- C SDK manifests and build files ---'
fd -t f -e toml -e cmake -e mk -e h -e c -e cc -e cpp -e md . sdks/c --exec rg -n -i -C2 'version|abi|soname|SOVERSION|VERSION|breaking|breaks|incompatible' {} \;
echo '--- Workspace/package version context ---'
fd -t f -e toml . --exec rg -n -C3 '^\[workspace\.package\]|^version\s*=|^name\s*=' {} \; | head -200Repository: boxlite-ai/boxlite
Length of output: 14297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Workspace and C SDK package versions ---'
rg -n -C5 '^\[workspace\]|^\[workspace\.package\]|^version\s*=|^name\s*=\s*"boxlite-c"|^name\s*=\s*"boxlite"' --glob 'Cargo.toml' --glob '**/Cargo.toml' .
echo '--- SONAME and shared-library version metadata ---'
rg -n -i -C3 'soname|soversion|install_name|shared_library|cdylib|dylib|\.so|abi' \
--glob '!target/**' --glob '!node_modules/**' .
echo '--- C SDK breaking-change documentation ---'
sed -n '890,955p' sdks/c/README.md
echo '--- C SDK build and release files ---'
fd -t f . sdks/c | sortRepository: boxlite-ai/boxlite
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Root Cargo.toml ---'
sed -n '1,80p' Cargo.toml
echo '--- C SDK Cargo.toml ---'
cat sdks/c/Cargo.toml
echo '--- Tracked C SDK files with ABI/version markers ---'
git ls-files sdks/c | while IFS= read -r file; do
case "$file" in
*.toml|*.cmake|CMakeLists.txt|*.mk|*.md|*.h|*.c|*.cc|*.cpp|*.rs)
rg -n -i -C2 'soname|soversion|install_name|cdylib|dylib|\.so|abi|breaking|breaks|version' "$file" || true
;;
esac
done
echo '--- Relevant README section ---'
sed -n '895,950p' sdks/c/README.mdRepository: boxlite-ai/boxlite
Length of output: 18445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
rust = Path("sdks/c/src/info.rs").read_text()
header = Path("sdks/c/include/boxlite.h").read_text()
cargo = Path("sdks/c/Cargo.toml").read_text()
workspace = Path("Cargo.toml").read_text()
lib = Path("sdks/c/src/lib.rs").read_text()
build = Path("sdks/c/build.rs").read_text()
rust_block = re.search(
r"pub struct CNetworkInfo\s*\{(?P<body>.*?)\}", rust, re.S
).group("body")
header_block = re.search(
r"typedef struct CNetworkInfo\s*\{(?P<body>.*?)\}\s*CNetworkInfo;",
header, re.S
).group("body")
rust_fields = re.findall(r"pub\s+(\w+)\s*:", rust_block)
header_fields = re.findall(r"\b(?:struct\s+\w+|\w[\w\s*]+?)\s+(\w+)\s*;", header_block)
workspace_version = re.search(
r"\[workspace\.package\].*?^version\s*=\s*\"([^\"]+)\"",
workspace, re.M | re.S
).group(1)
print("rust CNetworkInfo fields:", rust_fields)
print("header CNetworkInfo fields:", header_fields)
print("layout field order matches:", rust_fields == header_fields)
print("C SDK inherits workspace version:", "version.workspace = true" in cargo)
print("workspace version:", workspace_version)
print("version export uses CARGO_PKG_VERSION:", "CARGO_PKG_VERSION" in lib)
print("macOS install_name metadata:", re.findall(r"install_name[^\\n]*", build))
print("SONAME metadata present:", bool(re.search(r"soname|SOVERSION", build, re.I)))
PYRepository: boxlite-ai/boxlite
Length of output: 550
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
rust = Path("sdks/c/src/info.rs").read_text()
header = Path("sdks/c/include/boxlite.h").read_text()
rust_body = re.search(r"pub struct CNetworkInfo\s*\{(.*?)\}", rust, re.S).group(1)
header_body = re.search(
r"typedef struct CNetworkInfo\s*\{(.*?)\}\s*CNetworkInfo;",
header, re.S
).group(1)
rust_fields = re.findall(r"^\s*pub\s+([A-Za-z_]\w*)\s*:", rust_body, re.M)
header_fields = []
for line in header_body.splitlines():
line = line.split("//", 1)[0].strip()
match = re.match(r".*\b([A-Za-z_]\w*)\s*;\s*$", line)
if match:
header_fields.append(match.group(1))
print("rust CNetworkInfo fields:", rust_fields)
print("header CNetworkInfo fields:", header_fields)
print("layout field order matches:", rust_fields == header_fields)
PY
echo '--- Version implementation ---'
rg -n -C4 'CARGO_PKG_VERSION|pub extern "C" fn boxlite_version|fn boxlite_version|fn version' sdks/c/srcRepository: boxlite-ai/boxlite
Length of output: 1238
Increment the C SDK ABI version or SONAME.
The generated header matches the Rust layout. The C SDK still reports version 0.9.7, and no SONAME distinguishes the incompatible layout. Existing consumers can read invalid data with the old field offsets.
🤖 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 `@sdks/c/src/info.rs` around lines 60 - 70, Update the C SDK ABI/version
metadata associated with CNetworkInfo to increment the ABI version or SONAME
beyond 0.9.7, ensuring consumers can distinguish the incompatible struct layout
and avoid using stale field offsets.
Comment on lines
+180
to
+183
| /// Set how long an idle box may remain paused before the runtime pauses it. | ||
| /// | ||
| /// The value is expressed in seconds. `0` preserves the runtime/control-plane | ||
| /// default. A null options pointer is treated as a no-op. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the contradictory auto-stop description.
The sentence states that the value controls how long a box "may remain paused before the runtime pauses it". The condition and the action name the same state. This text is generated into the public C header.
📝 Proposed fix
-/// Set how long an idle box may remain paused before the runtime pauses it.
+/// Set how long a box may stay idle before the runtime pauses it.📝 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.
Suggested change
| /// Set how long an idle box may remain paused before the runtime pauses it. | |
| /// | |
| /// The value is expressed in seconds. `0` preserves the runtime/control-plane | |
| /// default. A null options pointer is treated as a no-op. | |
| /// Set how long a box may stay idle before the runtime pauses it. | |
| /// | |
| /// The value is expressed in seconds. `0` preserves the runtime/control-plane | |
| /// default. A null options pointer is treated as a no-op. |
🤖 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 `@sdks/c/src/options.rs` around lines 180 - 183, Update the documentation
comment for the idle auto-stop option near the relevant options API so it
describes the idle duration before the runtime stops or auto-stops the box,
rather than saying it pauses an already paused box. Preserve the existing
seconds, zero-default, and null-pointer behavior descriptions.
Comment on lines
442
to
456
| pub unsafe fn options_set_network_enabled(handle: *mut OptionsHandle) { | ||
| unsafe { | ||
| if !handle.is_null() { | ||
| (*handle).options.network = NetworkSpec::Enabled { | ||
| allow_net: Vec::new(), | ||
| }; | ||
| (*handle).options.network = NetworkSpec::enabled(Vec::new()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub unsafe fn options_set_network_disabled(handle: *mut OptionsHandle) { | ||
| unsafe { | ||
| if !handle.is_null() { | ||
| (*handle).options.network = NetworkSpec::Disabled; | ||
| (*handle).options.network = NetworkSpec::disabled(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The outbound mode setters silently reset the inbound policy.
NetworkSpec::enabled() and NetworkSpec::disabled() both build a whole NetworkSpec and set inbound to InboundNetworkSpec::default(), which is Enabled with an empty allowlist. Assigning the result to (*handle).options.network therefore discards any inbound configuration that the caller already applied.
This sequence makes the box public again without an error:
boxlite_options_set_network_inbound_disabled(opts);
boxlite_options_set_network_disabled(opts); // inbound silently returns to EnabledThe setters must mutate only their own direction, so that call order does not change the result.
🐛 Proposed fix
pub unsafe fn options_set_network_enabled(handle: *mut OptionsHandle) {
unsafe {
if !handle.is_null() {
- (*handle).options.network = NetworkSpec::enabled(Vec::new());
+ (*handle).options.network.outbound = OutboundNetworkSpec::Enabled {
+ allow_net: Vec::new(),
+ };
}
}
}
pub unsafe fn options_set_network_disabled(handle: *mut OptionsHandle) {
unsafe {
if !handle.is_null() {
- (*handle).options.network = NetworkSpec::disabled();
+ (*handle).options.network.outbound = OutboundNetworkSpec::Disabled;
}
}
}The inbound setters at Lines 472-488 already follow this pattern. Add a test that sets inbound first and outbound second, then asserts that inbound stays Disabled.
🤖 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 `@sdks/c/src/options.rs` around lines 442 - 456, Update
options_set_network_enabled and options_set_network_disabled to mutate only the
outbound/network-enabled state while preserving the existing inbound policy,
matching the behavior of the inbound setters at Lines 472-488. Add a test that
configures inbound as disabled before setting outbound, then verifies inbound
remains Disabled regardless of setter order.
Comment on lines
+497
to
+510
| switch cfg.network.Inbound.Mode { | ||
| case "", NetworkModeEnabled: | ||
| C.boxlite_options_set_network_inbound_enabled(cOpts) | ||
| for _, host := range cfg.network.Inbound.AllowNet { | ||
| cHost := toCString(host) | ||
| C.boxlite_options_add_network_inbound_allow(cOpts, cHost) | ||
| C.free(unsafe.Pointer(cHost)) | ||
| } | ||
| case NetworkModeDisabled: | ||
| if len(cfg.network.Inbound.AllowNet) > 0 { | ||
| C.boxlite_options_free(cOpts) | ||
| return nil, fmt.Errorf("inbound.mode=%q is incompatible with allow_net", NetworkModeDisabled) | ||
| } | ||
| C.boxlite_options_set_network_inbound_disabled(cOpts) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Do not present inbound allowlists as enforced access controls.
src/boxlite/src/runtime/types.rs documents inbound allow_net as accepted but not yet enforced. The Go SDK silently accepts this configuration, while the Go and C public API documentation states that it restricts incoming access. A caller can configure an inbound allowlist and expose services publicly.
Until runtime enforcement exists, reject non-empty inbound allowlists or report them as unsupported. Update the public documentation to state the actual behavior.
sdks/go/options.go#L497-L510: reject non-emptycfg.network.Inbound.AllowNetuntil enforcement exists.sdks/c/include/boxlite.h#L766-L769: remove the claim that listed hosts/IPs are the only permitted callers.sdks/go/options.go#L82-L90: remove the claim thatAllowNetrestricts inbound callers.
📍 Affects 2 files
sdks/go/options.go#L497-L510(this comment)sdks/c/include/boxlite.h#L766-L769sdks/go/options.go#L82-L90
🤖 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 `@sdks/go/options.go` around lines 497 - 510, Reject any non-empty
cfg.network.Inbound.AllowNet in the inbound network configuration flow of
sdks/go/options.go (lines 497-510) until runtime enforcement exists, returning
the existing cleanup/error pattern rather than silently accepting it. Update the
public documentation in sdks/c/include/boxlite.h (lines 766-769) and
sdks/go/options.go (lines 82-90) to remove claims that AllowNet restricts
inbound callers and accurately describe its unsupported or unenforced behavior.
Comment on lines
+347
to
+359
| /// Aligned field-for-field with `JsOutboundNetworkSpec`: `mode="enabled"` | ||
| /// means services the box exposes are publicly reachable (optionally | ||
| /// restricted to `allowNet`); `mode="disabled"` means private. | ||
| #[napi(object)] | ||
| #[derive(Clone, Debug)] | ||
| pub struct JsInboundNetworkSpec { | ||
| /// Inbound mode: "enabled" or "disabled". | ||
| pub mode: String, | ||
|
|
||
| /// Inbound allowlist when mode is "enabled". Empty/omitted means any | ||
| /// caller may reach the box's exposed services. | ||
| #[napi(js_name = "allowNet")] | ||
| pub allow_net: Option<Vec<String>>, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Correct the inbound allowlist security contract.
InboundNetworkSpec.allowNet is accepted and returned in metadata, but the runtime contract states that it is not yet enforced. Line 347 currently says it restricts callers. A caller can therefore expose a service publicly while believing the inbound allowlist protects it.
sdks/node/src/options.rs#L347-L359: Remove the claim thatallowNetrestricts inbound callers, or enforce the allowlist before documenting that behavior.sdks/python/README.md#L257-L258: Document outbound and inbound semantics separately. State that inbound allowlists are not access control until enforcement exists.sdks/python/src/info.rs#L70-L102: Add a comprehensive Python-visible docstring. State that inboundallow_netreports configured metadata and does not currently enforce caller restrictions.
As per coding guidelines, “Write comprehensive docstrings for all public functions and classes.”
📍 Affects 3 files
sdks/node/src/options.rs#L347-L359(this comment)sdks/python/README.md#L257-L258sdks/python/src/info.rs#L70-L102
🤖 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 `@sdks/node/src/options.rs` around lines 347 - 359, Correct the inbound
allowlist contract across all three sites: in sdks/node/src/options.rs lines
347-359, update JsInboundNetworkSpec documentation to state that allowNet is
metadata only and does not restrict callers; in sdks/python/README.md lines
257-258, document outbound and inbound semantics separately and clarify that
inbound allowlists are not access control; in sdks/python/src/info.rs lines
70-102, add comprehensive Python-visible documentation stating that allow_net
reports configured metadata without enforcing caller restrictions.
Source: Coding guidelines
Comment on lines
32
to
+35
| test("disabled network removes eth0", async () => { | ||
| const box = new SimpleBox({ | ||
| image: "alpine:latest", | ||
| network: { mode: "disabled" }, | ||
| network: { outbound: { mode: "disabled" } }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add an inbound policy integration test.
The new inbound policy is not exercised through SimpleBox creation. Add a test that creates a box with inbound: { mode: "disabled" } or an enabled allowlist, then asserts await box.getInfo() reports the configured inbound state. This verifies the Node-to-native conversion and the NetworkInfo readback contract.
As per coding guidelines: sdks/node/**/*.test.{js,ts,jsx,tsx} requires unit tests for all public functions and critical business logic.
Also applies to: 77-84
🤖 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 `@sdks/node/tests/network-secrets.integration.test.ts` around lines 32 - 35,
Add an integration test alongside “disabled network removes eth0” that creates a
SimpleBox with an inbound policy (disabled or an enabled allowlist), calls await
box.getInfo(), and asserts NetworkInfo reports the configured inbound state,
covering Node-to-native conversion and readback.
Source: Coding guidelines
Comment on lines
+319
to
+322
| Ok(Self { | ||
| outbound: outbound.or(legacy_outbound), | ||
| inbound, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject an empty NetworkSpec.
NetworkSpec() currently succeeds and Lines 365-372 silently replace both omitted policies with defaults. The Node SDK rejects a supplied network object with neither outbound nor inbound. Require at least one nested policy here too, so the Python and Node SDKs have the same explicit-network validation contract.
Proposed fix
+ if outbound.is_none() && inbound.is_none() && legacy_outbound.is_none() {
+ return Err(PyValueError::new_err(
+ "NetworkSpec must include outbound or inbound",
+ ));
+ }
+
Ok(Self {
outbound: outbound.or(legacy_outbound),
inbound,
})📝 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.
Suggested change
| Ok(Self { | |
| outbound: outbound.or(legacy_outbound), | |
| inbound, | |
| }) | |
| if outbound.is_none() && inbound.is_none() && legacy_outbound.is_none() { | |
| return Err(PyValueError::new_err( | |
| "NetworkSpec must include outbound or inbound", | |
| )); | |
| } | |
| Ok(Self { | |
| outbound: outbound.or(legacy_outbound), | |
| inbound, | |
| }) |
🤖 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 `@sdks/python/src/options.rs` around lines 319 - 322, Update the NetworkSpec
conversion logic returning Ok(Self) to reject inputs where both outbound and
inbound are absent, before applying legacy_outbound or defaults. Preserve
existing handling when either nested policy is supplied, matching the Node SDK’s
explicit-network validation contract.
Comment on lines
+714
to
+729
| let inbound = match config.inbound.mode { | ||
| NetworkMode::Enabled => { | ||
| if !config.inbound.allow_net.is_empty() { | ||
| // No runtime sink enforces this yet (apps/proxy gates only on | ||
| // inbound.mode) — warn rather than silently accepting a | ||
| // no-op allowlist. Remove once enforcement lands. | ||
| tracing::warn!( | ||
| allow_net = ?config.inbound.allow_net, | ||
| "inbound.allow_net is accepted but not yet enforced; \ | ||
| all callers can still reach this box's exposed ports" | ||
| ); | ||
| } | ||
| InboundNetworkSpec::Enabled { | ||
| allow_net: config.inbound.allow_net, | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Align the InboundNetworkSpec doc with the unenforced allowlist.
TryFrom accepts a non-empty inbound.allow_net and only logs a warning, because no sink enforces it. The doc comment on InboundNetworkSpec states that a non-empty allow_net means "only listed hosts/IPs". That statement describes behavior that does not exist yet. A caller who reads the type doc can assume the box is restricted while every caller can still reach the exposed ports.
NetworkDirectionInfo in src/boxlite/src/runtime/types.rs already carries the "not yet enforced" caveat. Add the same caveat here and on InboundNetworkConfig.
📝 Proposed doc fix
/// Whether services the box exposes are reachable from outside it. Mirrors
/// [`OutboundNetworkSpec`]'s shape: `Enabled` = publicly reachable (empty
/// `allow_net` = any caller; non-empty = only listed hosts/IPs), `Disabled`
/// = private, unreachable from outside the box.
+///
+/// `allow_net` is accepted and persisted but NOT yet enforced: with
+/// `Enabled`, every caller can reach the exposed ports regardless of the
+/// allowlist contents.Also applies to: 847-858
🤖 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/runtime/options.rs` around lines 714 - 729, Update the
documentation comments for InboundNetworkSpec and InboundNetworkConfig to state
that a non-empty allow_net is currently accepted but not enforced, so callers
must not assume traffic is restricted; mirror the existing caveat in
NetworkDirectionInfo and leave the TryFrom behavior unchanged.
DorianZheng
reviewed
Aug 11, 2026
| const errors = await validate( | ||
| plainToInstance(CreateBoxDto, { | ||
| network: { | ||
| mode: 'enabled', |
Member
There was a problem hiding this comment.
keep supporting it, but mark deprecated
Contributor
Author
There was a problem hiding this comment.
Adopted — pushed to #1199: legacy flat network.{mode,allow_net,service_access} is now accepted and normalized into network.{outbound,inbound}, with a deprecation warning logged instead of a 400. Legacy fields mixed with the nested shape in the same request are still rejected (no sane precedence to guess between them).
DorianZheng
reviewed
Aug 11, 2026
Contributor
Author
There was a problem hiding this comment.
Same fix — see reply on the sibling comment: legacy flat network fields are now accepted (deprecated + normalized) rather than rejected, landed in #1199.
Contributor
Author
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NetworkInfo(returned bybox.info()/list()) only ever exposed outbound'smode/allow_net. Inbound's current state — configurable since #996 via--inbound/--inbound-allow-net— had no output field at all: a caller could set it but never read it back.Before/after
Mirrors
NetworkSpec{outbound, inbound}'s shape one-for-one.published_portsstays at the top level — it's the actual host:port bindings, direction-agnostic.Test plan
cargo test -p boxlite -p boxlite-cli -p boxlite-c -p boxlite-node -p boxlite-python— all green (core 1044/1045; the one unrelated failure,ws_watchdog_fires_when_idle, is a pre-existing timing-flaky test inrest/litebox.rs, untouched by this diff)make fmt:check:rustcleansdks/c/include/boxlite.h)Follow-up to #996; addresses Dorian's review comment ("How do the user know allow_net means outbound allow_net only?").
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation