[PR #160] [CLOSED] fix(api): accept allowInternetAccess as an alias so isolation requests can't silently fail open #217

New issue

Closed

opened 2026-10-29 14:21:07 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:21:07 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/160
Author: @sunkencity999
Created: 8/12/2026
Status: ❌ Closed

Base: main ← Head: fix/newsandbox-allow-internet-access

📝 Commits (1)

5bb536d fix(api): accept allowInternetAccess as an alias so it can't silently fail open

📊 Changes

2 files changed (+48 additions, -2 deletions)

View changed files

📝 src/api/generated/src/models.rs (+10 -2)
📝 src/api/impls/sandbox.rs (+38 -0)

📄 Description

Follows the direction you suggested in #156: allow_internet_access stays canonical on NewSandbox for E2B compatibility, and allowInternetAccess is accepted as a serde alias. Nothing is renamed and no existing client changes behaviour.

You were right that the field name is intentional — I checked the E2B spec you linked and it has the same split (NewSandbox takes snake_case, SandboxDetail returns camelCase), so AgentENV is mirroring the contract correctly.

What this fixes

The residual problem is what happens when a caller sends the other spelling. serde drops the unknown key, so the field arrives as None; base_policy_from_allow_internet_access maps None to BaseSandboxNetworkPolicy::Default rather than Deny, and the sandbox is created with unrestricted egress after isolation was explicitly requested. Nothing rejects the request and nothing logs it.

GET /sandboxes/{id} then reports allowInternetAccess: null, which reads as "never set" rather than "your setting was discarded" — so it isn't obvious from the readback either.

Callers land on the wrong spelling because the contract is internally inconsistent: allowInternetAccess is what the response hands back and what NewColdSandbox accepts. The alias closes that trap without touching the canonical name.

There's a second-order effect worth noting: allowOut domain rules only arm under a Deny base policy, so a create combining allowInternetAccess: false with domain rules produced a sandbox with no egress restriction at all — neither the deny nor the allowlist.

Changes

NewSandbox: #[serde(rename = "allow_internet_access", alias = "allowInternetAccess")]
SandboxNetworkUpdateConfig: same alias, so both endpoints behave alike
Three regression tests: camelCase and snake_case on create, both spellings on update, and Deny base-policy propagation from a camelCase create body

Both model edits are in a generated file and carry comments so a regeneration doesn't silently revert them. If you'd rather express this in openapi.yml and regenerate — which is the more durable fix, and what you hinted at — I'm happy to close this in favour of that; the tests would still be worth keeping either way.

Not addressed here

Whether an unrecognised field in a network-policy body should be rejected outright rather than ignored. deny_unknown_fields on these two models would turn this whole class of mistake into a 4xx instead of an open sandbox, and stays E2B-compatible since conforming clients only send known fields. It's a behaviour change, so it seemed wrong to bundle it — raised separately in #156 if you think it's worth pursuing.

Closes #156 if you take this approach; happy to adjust or withdraw.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/kvcache-ai/AgentENV/pull/160 **Author:** [@sunkencity999](https://github.com/sunkencity999) **Created:** 8/12/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/newsandbox-allow-internet-access` --- ### 📝 Commits (1) - [`5bb536d`](https://github.com/kvcache-ai/AgentENV/commit/5bb536dfc3fce4f3db8ffca8633049f6e48ee9b1) fix(api): accept allowInternetAccess as an alias so it can't silently fail open ### 📊 Changes **2 files changed** (+48 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/api/generated/src/models.rs` (+10 -2) 📝 `src/api/impls/sandbox.rs` (+38 -0) </details> ### 📄 Description Follows the direction you suggested in #156: `allow_internet_access` stays canonical on `NewSandbox` for E2B compatibility, and `allowInternetAccess` is accepted as a serde alias. Nothing is renamed and no existing client changes behaviour. You were right that the field name is intentional — I checked the E2B spec you linked and it has the same split (`NewSandbox` takes snake_case, `SandboxDetail` returns camelCase), so AgentENV is mirroring the contract correctly. ### What this fixes The residual problem is what happens when a caller sends the *other* spelling. serde drops the unknown key, so the field arrives as `None`; `base_policy_from_allow_internet_access` maps `None` to `BaseSandboxNetworkPolicy::Default` rather than `Deny`, and the sandbox is created with unrestricted egress after isolation was explicitly requested. Nothing rejects the request and nothing logs it. `GET /sandboxes/{id}` then reports `allowInternetAccess: null`, which reads as "never set" rather than "your setting was discarded" — so it isn't obvious from the readback either. Callers land on the wrong spelling because the contract is internally inconsistent: `allowInternetAccess` is what the response hands back and what `NewColdSandbox` accepts. The alias closes that trap without touching the canonical name. There's a second-order effect worth noting: `allowOut` domain rules only arm under a `Deny` base policy, so a create combining `allowInternetAccess: false` with domain rules produced a sandbox with no egress restriction at all — neither the deny nor the allowlist. ### Changes - `NewSandbox`: `#[serde(rename = "allow_internet_access", alias = "allowInternetAccess")]` - `SandboxNetworkUpdateConfig`: same alias, so both endpoints behave alike - Three regression tests: camelCase and snake_case on create, both spellings on update, and `Deny` base-policy propagation from a camelCase create body Both model edits are in a generated file and carry comments so a regeneration doesn't silently revert them. If you'd rather express this in `openapi.yml` and regenerate — which is the more durable fix, and what you hinted at — I'm happy to close this in favour of that; the tests would still be worth keeping either way. ### Not addressed here Whether an unrecognised field in a network-policy body should be rejected outright rather than ignored. `deny_unknown_fields` on these two models would turn this whole class of mistake into a 4xx instead of an open sandbox, and stays E2B-compatible since conforming clients only send known fields. It's a behaviour change, so it seemed wrong to bundle it — raised separately in #156 if you think it's worth pursuing. Closes #156 if you take this approach; happy to adjust or withdraw. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-29 14:21:07 +00:00