Skip to content

fix(sdk): match JS and Python on malformed and null egress proxy input #1702

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mishushakov wants to merge 1 commit into from
+181 −26
Closed

fix(sdk): match JS and Python on malformed and null egress proxy input #1702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .changeset/egress-proxy-untyped-callers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
---
'e2b': patch
'@e2b/python-sdk': patch
---

Bring the JS and Python halves of `network.egressProxy` / `network["egress_proxy"]` back in line for callers that bypass the types, and stop `null` credentials from reaching the wire.

A malformed proxy now raises `InvalidArgumentError` / `InvalidArgumentException` in both SDKs. Before, only Python did; JS rebuilt the body from the known fields, so a proxy passed as a bare string sent `{}` and the caller got an API error about a field they never left out:

```ts
// Now: InvalidArgumentError, naming the option you typed.
// Before: sent `"egressProxy": {}` and failed at the API.
await Sandbox.create({
network: { egressProxy: 'proxy.example.com:1080' as never },
})
```

A `username` or `password` that is `null` / `None` is treated as "no credentials" rather than serialized as a JSON null the API rejects — the same reading both SDKs already gave `egressProxy: null` itself:

```ts
await Sandbox.create({
network: {
egressProxy: {
address: 'proxy.example.com:1080',
// Unset in the environment; the proxy takes no credentials.
username: process.env.PROXY_USER,
},
},
})
```

```python
Sandbox.create(
network={
"egress_proxy": {
"address": "proxy.example.com:1080",
"username": os.environ.get("PROXY_USER"),

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This snippet doesn't type-check under the repo's own checker, while its JS twin above does — worth fixing before it is copied verbatim into both published CHANGELOGs:

error[invalid-argument-type]: Invalid argument to key "username" with declared type `str`
  on TypedDict `SandboxEgressProxyOpts`
  --> value of type `str | None`

process.env.PROXY_USER is string | undefined, which username?: string accepts, so the TypeScript example is legal. NotRequired[str] cannot express "explicitly absent", so the Python one isn't. Please don't fix it by widening to Optional[str] — TASTE's "absence is undefined, never null" rules that out, and the runtime tolerance is deliberately a safety net for callers who bypass the types, which is what the changeset's own opening sentence says. Either frame the Python snippet as the untyped-caller case (a # type: ignore or a cast, mirroring the as never you used in the first example) or pick a form that checks, e.g. building the dict conditionally.

},
},
)
```

`getInfo` / `get_info` normalizes a `null` `username` the same way, so `SandboxEgressProxyInfo.username` is `undefined` / an absent key rather than a null that its type says cannot be there.
2 changes: 1 addition & 1 deletion packages/js-sdk/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@

```python
info = sandbox.get_info()
print(info.network["egress_proxy"])
print(info.network.get("egress_proxy"))
# {'address': 'proxy.example.com:1080', 'username': 'proxy-user'}
```

Expand Down
35 changes: 22 additions & 13 deletions packages/js-sdk/src/sandbox/sandboxApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -997,20 +997,30 @@ function resolveRulesForBody(
/**
* Rebuild the proxy config from the known fields so stray properties on the
* caller's object never reach the wire and a later mutation of it cannot alter
* the in-flight request. Validation is the server's — it is the only side that
* can tell whether the address resolves, and to where.
* the in-flight request. Address reachability is the server's — it is the only
* side that can tell whether the address resolves, and to where.
*/
function buildEgressProxyBody(
egressProxy: SandboxEgressProxyOpts
): components['schemas']['SandboxEgressProxyConfig'] {
// Re-check at runtime for callers that bypass the type — rebuilding from the
// known fields drops an address that isn't there, and the API error for the
// resulting `{}` names neither the option the caller typed nor the mistake.
// Python raises `InvalidArgumentException` on the same input.

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guard is right, but this justification isn't — and unlike the PR description, a code comment outlives the diff. The API error does name both the option and the mistake. Live, with the pre-PR body:

POST /sandboxes  {"network":{"egressProxy":{}}}
400 validation error: … Error at "/network/egressProxy": property "address" is missing

and {address: 1080} gives Error at "/address": value must be a string. So network.egressProxy and the missing address are both named already.

What the guard actually buys is enough on its own: the failure is local instead of a round trip, it is an InvalidArgumentError the caller can catch by type, and it matches _build_egress_proxy's InvalidArgumentException on identical input — which is the point of the PR. The one input where the API message really is misleading is the bare string, where the rebuilt {} produces "property address is missing" for a caller who never wrote an object at all; that narrower claim would be accurate.

if (typeof egressProxy.address !== 'string') {

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional, and the precedent cuts toward what you did — flagging it only because "match JS and Python" is the title. Python's guard also requires a Mapping, so the two still disagree on one input:

class Cfg:
    address = "proxy.example.com:1080"

build_network_config({"egress_proxy": Cfg()})   # InvalidArgumentException
// accepted: sends { address: 'proxy.example.com:1080' }
await Sandbox.create('base', { network: { egressProxy: new Cfg() } as never })

This file has both styles: isPlainObject (~931) exists specifically to mirror isinstance(transform, Mapping), while buildIamBody uses bare typeof x.field !== 'string' checks — and buildIamBody is the closer neighbour, which is what you followed. Impact is low either way since the body is rebuilt from known fields, so a deliberate "JS is the permissive one here" is a fine answer; I'd just rather it be deliberate.

throw new InvalidArgumentError(
"network egressProxy must be an object with a string 'address' " +

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small vocabulary nit: the same guard is reached from the update path, where there is no network key for the caller to have typed. Verified on this branch, both SDKs:

Sandbox.updateNetwork(id, { egressProxy: 'proxy.example.com:1080' })
-> InvalidArgumentError: network egressProxy must be an object with a string 'address' (…)

build_network_update_body({"egress_proxy": "proxy.example.com:1080"})
-> InvalidArgumentException: network egress_proxy must be a dict with a string 'address' (…)

TASTE asks for the rule stated "in the option names the user typed". Dropping the leading network (`egressProxy` must be an object with a string `address`) is accurate from both call sites, and buildIamBody's message right below already names its own path exactly (iam token 'x' must have …).

"(e.g. 'proxy.example.com:1080')."
)
}

return {
address: egressProxy.address,
...(egressProxy.username !== undefined
? { username: egressProxy.username }
: {}),
...(egressProxy.password !== undefined
? { password: egressProxy.password }
: {}),
// `!= null` so a credential read out of an unset environment variable
// reads as "no credentials" rather than reaching the wire as JSON null,
// which the API rejects. Same reasoning as `egressProxy: null` itself.
...(egressProxy.username != null ? { username: egressProxy.username } : {}),

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs gap, since TASTE treats JSDoc/docstrings as part of the API. Two things this change establishes are documented only in the changeset:

  1. The new accepted spelling. SandboxEgressProxyOpts.username / .password still say only "Omit it for a proxy that takes no credentials" — but username: process.env.PROXY_USER on an unset variable is now equally supported, and that is the headline use case. Same for the Python docstrings at sandbox_api.py ~262 and ~268.
  2. The new failure mode. A malformed egressProxy now throws InvalidArgumentError / raises InvalidArgumentException, and nothing on the public surface says so. There is precedent in this very file — SandboxNetworkTransformContext.iam.tokens (~70) documents its {@link InvalidArgumentError} in prose, and iam.ts:30 uses @throws {@link InvalidArgumentError} — plus a natural home in SandboxNetworkOpts.egressProxy (~280) and the Python egress_proxy field docstring.

...(egressProxy.password != null ? { password: egressProxy.password } : {}),
}
}

Expand Down Expand Up @@ -1054,8 +1064,9 @@ function buildNetworkEgress(

/**
* Map the wire proxy config into the SDK-owned shape: `password` is dropped
* because the API never returns it, and the wire's `null` for "no proxy" is
* normalized so the union never reaches a consumer.
* because the API never returns it, and the wire's `null` — for "no proxy" and
* for an anonymous proxy's `username` alike — is normalized so it never reaches
* a consumer typed to see `undefined`.
*/
function fromApiEgressProxy(
egressProxy: components['schemas']['SandboxEgressProxyConfig'] | undefined
Expand All @@ -1066,9 +1077,7 @@ function fromApiEgressProxy(

return {
address: egressProxy.address,
...(egressProxy.username !== undefined
? { username: egressProxy.username }
: {}),
...(egressProxy.username != null ? { username: egressProxy.username } : {}),
}
}

Expand Down
74 changes: 69 additions & 5 deletions packages/js-sdk/tests/sandbox/egressProxy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { afterAll, afterEach, beforeAll, expect, test } from 'vitest'
import { http, HttpResponse } from 'msw'
import { setupServer } from 'msw/node'

import { Sandbox } from '../../src'
import { InvalidArgumentError, Sandbox } from '../../src'
import { TEST_API_KEY, apiUrl } from '../setup'

const sandboxId = 'test-sandbox-id'
Expand Down Expand Up @@ -101,14 +101,64 @@ test('Sandbox.create combines the egress proxy with allow and deny lists', async
})
})

test('Sandbox.create omits the egress proxy when not provided', async () => {
test.for([
['omitted', { allowOut: ['api.example.com'] }],
// Untyped callers spell "no proxy" as null; Python treats an explicit None
// the same way.
['null', { egressProxy: null }],
])(
'Sandbox.create omits the egress proxy when it is %s',
async ([, network]: [string, Record<string, any>]) => {
await Sandbox.create('base', {
apiKey: TEST_API_KEY,
network,
})

expect(lastCreateBody?.network).toBeDefined()
expect(lastCreateBody?.network).not.toHaveProperty('egressProxy')
}
)

test.for([
// An empty object is falsy but present — it must not silently disable
// tunneling. Match Python: fail loudly.
['empty', {}],
['missing-address', { username: 'proxy-user' }],
['non-string-address', { address: 1080 }],
['string', 'proxy.example.com:1080'],
])(
'Sandbox.create rejects a %s egress proxy',
async ([, egressProxy]: [string, unknown]) => {
// Rebuilding from the known fields drops an address that isn't there, so
// without this the caller gets an API error about a `{}` they never wrote.
await expect(
Sandbox.create('base', {
apiKey: TEST_API_KEY,
network: { egressProxy } as never,
})
).rejects.toThrow(InvalidArgumentError)

expect(lastCreateBody).toBeUndefined()
}
)

test('Sandbox.create omits credentials that are null', async () => {
// `{ username: process.env.PROXY_USER }` on an unset variable is the way
// this happens; a JSON null is rejected by the API.
await Sandbox.create('base', {
apiKey: TEST_API_KEY,
network: { allowOut: ['api.example.com'] },
network: {
egressProxy: {
address: 'proxy.example.com:1080',
username: null,
password: undefined,

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the surviving mutant: password != null in buildEgressProxyBody has no test.

password: undefined is dropped by the pre-PR password !== undefined too, so this case cannot distinguish the new clause from the old one. Reverting just that clause on head leaves all 19 tests in this file green — the username: null on the line above is doing all the work.

It can't fail either way, in fact: JSON.stringify omits undefined values, so an undefined credential could never have reached the wire on either side of this diff. The Python twin gets it right — test_create_omits_credentials_that_are_none passes "password": None, and both Python clause mutants are caught.

One character fixes it:

Suggested change
password: undefined,
password: null,

Verified both directions: with that change the password != null → !== undefined mutant fails this test, and the unmutated suite stays 19/19. Nothing is lost by giving up the undefined spelling here — absent credentials are already covered by Sandbox.create sends an address-only egress proxy without credentials above.

} as never,
},
})

expect(lastCreateBody?.network).toBeDefined()
expect(lastCreateBody?.network).not.toHaveProperty('egressProxy')
expect(lastCreateBody?.network.egressProxy).toEqual({
address: 'proxy.example.com:1080',
})
})

test('Sandbox.create strips unknown egress proxy properties', async () => {
Expand Down Expand Up @@ -199,6 +249,20 @@ test('getInfo drops a password the API unexpectedly returns', async () => {
})
})

test('getInfo drops a null username', async () => {
// `username?: string` says absence is `undefined`, so a null from the wire
// has to be normalized rather than handed to a consumer.
sandboxNetwork = {
egressProxy: { address: 'proxy.example.com:1080', username: null },
}

const info = await Sandbox.getInfo(sandboxId, { apiKey: TEST_API_KEY })

expect(info.network?.egressProxy).toEqual({
address: 'proxy.example.com:1080',
})
})

test.for([
['omitted', {}],
['null', { egressProxy: null }],
Expand Down
2 changes: 1 addition & 1 deletion packages/python-sdk/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,11 @@
console.log(info.network?.egressProxy)
// { address: 'proxy.example.com:1080', username: 'proxy-user' }
```

```python
info = sandbox.get_info()
print(info.network["egress_proxy"])
print(info.network.get("egress_proxy"))

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming @claude's note here with a live check rather than a reading: the fix trades a KeyError for an AttributeError on exactly the input it was meant to handle. SandboxInfo.network is Optional[SandboxNetworkInfo] and really is None for an ordinary sandbox —

sbx = Sandbox.create(timeout=30)
print(sbx.get_info().network)   # None

so info.network.get("egress_proxy") raises AttributeError: 'NoneType' object has no attribute 'get' for "any sandbox without a proxy", the case the PR description cites. The JS line above survives because info.network?.egressProxy guards both levels; the Python mirror needs (info.network or {}).get("egress_proxy"). The same snippet is duplicated at packages/js-sdk/CHANGELOG.md:95, since both changelogs carry the full shared changeset text.

# {'address': 'proxy.example.com:1080', 'username': 'proxy-user'}

Check warning on line 96 in packages/python-sdk/CHANGELOG.md

View check run for this annotation

Claude / Claude Code Review

Python get_info CHANGELOG example still raises when info.network is None 

The Python `get_info` CHANGELOG snippet still raises if a sandbox has no network config at all: `info.network.get("egress_proxy")` fixes the KeyError case but `info.network` can itself be `None` (`from_client_network_config()` returns `None` when the API omits `network`), and `None.get(...)` raises `AttributeError`. The JS snippet right above guards both levels with `info.network?.egressProxy`; using `(info.network or {}).get("egress_proxy")` would match it.
Comment on lines 92 to 96

@claude claude Bot Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The Python changelog fix for info.network["egress_proxy"] still crashes with AttributeError: 'NoneType' object has no attribute 'get' if info.network itself is None (a sandbox with no network config at all), since .get("egress_proxy") requires a dict to call it on. The adjacent JS example already guards this with info.network?.egressProxy; the Python snippet should use (info.network or {}).get("egress_proxy") for true parity.

Extended reasoning...

What the bug is: This PR's stated goal for the get_info changelog snippet is to fix a crash: info.network["egress_proxy"] raised KeyError whenever a sandbox had a network config but no proxy configured on it. The fix changes this to info.network.get("egress_proxy"), which correctly handles that case. But .get() is a dict method — it still assumes info.network itself is non-None. SandboxDetail.network is not in the required list in spec/openapi.yml, so the wire field can be entirely absent. from_client_network_config in packages/python-sdk/e2b/sandbox/sandbox_api.py returns None in that case, making SandboxInfo.network: Optional[SandboxNetworkInfo] = None a real, reachable value — not just a type annotation. Calling .get(...) on that None raises AttributeError: 'NoneType' object has no attribute 'get'.

Code path: sandbox.get_info() → SandboxInfo._from_sandbox_detail() → from_client_network_config(sandbox_detail.network) → returns None when the API response's network key is unset. The published snippet then does info.network.get("egress_proxy") on that None.

Why existing code doesn't prevent it: The PR only patched the inner lookup (missing key inside an existing dict), not the outer optionality (the dict itself being absent). These are two independent failure modes stacked on the same line, and the fix addressed only the one that was reported. The adjacent JS half of the same changelog block already handles both: info.network?.egressProxy short-circuits to undefined whether network is null/undefined or the key inside it is simply not there. That's a direct, avoidable JS/Python divergence in a PR whose explicit purpose is JS/Python parity.

Impact: This is scoped to a documentation code sample in packages/python-sdk/CHANGELOG.md (and the same block copied into packages/js-sdk/CHANGELOG.md). It is never executed as part of the SDK or its test suite. A user who copies the snippet verbatim and later runs it against a sandbox with no network configuration at all (not just "no proxy" — no network config whatsoever) would hit an uncaught AttributeError.

Step-by-step proof:

  1. A sandbox is created without any network option, and the API's SandboxDetail response consequently omits the network field entirely (it's optional per spec).
  2. _from_sandbox_detail calls from_client_network_config(sandbox_detail.network), where sandbox_detail.network is Unset.
  3. from_client_network_config sees isinstance(network, Unset) is True and returns None.
  4. SandboxInfo.network is now None.
  5. The changelog example runs info.network.get("egress_proxy").
  6. Python evaluates None.get("egress_proxy") → AttributeError: 'NoneType' object has no attribute 'get'.
  7. Compare to the JS version on the same lines: info.network?.egressProxy — if info.network were undefined there, the optional-chain operator short-circuits to undefined with no error.

Fix: Change the Python snippet to (info.network or {}).get("egress_proxy"), matching the JS example's null-safety and the PR's own stated goal of parity between the two SDKs.

Severity: This is a nit. It's a changelog documentation example, never executed by any test or shipped code path, and the scenario the snippet is illustrating (a proxy already configured, from the printed sample output) always has network populated. The gap only matters if a reader copies the snippet as-is and runs it against a sandbox with no network configuration at all — a real but narrow corner case for documentation text.

```

Egress fails closed: when the proxy is unreachable or does not speak SOCKS5, outbound connections fail rather than falling back to a direct connection. The address is validated server-side when the sandbox is created — a rejected create leaves nothing behind. Available on E2B Cloud and in BYOC deployments; a sandbox that names a proxy on a deployment built from the open source `e2b-dev/infra` repository is rejected as unsupported.
Expand Down
16 changes: 10 additions & 6 deletions packages/python-sdk/e2b/sandbox/sandbox_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,9 +671,12 @@ def _build_egress_proxy(
)

body = ClientSandboxEgressProxyConfig(address=egress_proxy["address"])
if "username" in egress_proxy:
# `is not None` so a credential read out of an unset environment variable
# reads as "no credentials" rather than reaching the wire as JSON null,
# which the API rejects. Same reasoning as ``"egress_proxy": None`` itself.
if egress_proxy.get("username") is not None:
body.username = egress_proxy["username"]
if "password" in egress_proxy:
if egress_proxy.get("password") is not None:
body.password = egress_proxy["password"]

return body
Expand Down Expand Up @@ -899,15 +902,16 @@ def _from_client_egress_proxy(
) -> Optional[SandboxEgressProxyInfo]:
"""
Map the wire proxy config into the SDK-owned shape: ``password`` is dropped
because the API never returns it, and the wire's ``None`` for "no proxy"
becomes an absent key.
because the API never returns it, and the wire's ``None`` — for "no proxy"
and for an anonymous proxy's ``username`` alike — becomes an absent key.
"""
if not isinstance(egress_proxy, ClientSandboxEgressProxyConfig):
return None

result: SandboxEgressProxyInfo = {"address": egress_proxy.address}
if not isinstance(egress_proxy.username, Unset):
result["username"] = egress_proxy.username
username = egress_proxy.username
if not isinstance(username, Unset) and username is not None:

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is where the PR's strongest coverage evidence lives. Branch arc 913 → 916 (the false side of this guard) goes from never-taken on base to taken on head — no test in the suite had ever driven a None username through the read path before test_get_info_drops_a_none_username. That is the read-path half of the fix, demonstrated rather than asserted, and after it every branch in all three touched functions (_build_egress_proxy, _build_network_egress, _from_client_egress_proxy) is covered.

Two caveats worth recording so the numbers aren't over-read:

  1. The second newly-taken arc, 927 → 929 in from_client_network_config, is incidental — the new test builds a minimal SandboxNetworkConfig with only egress_proxy set, so allow_out stays Unset and that false branch gets taken for the first time. It's an artifact of the fixture, not of the fix.
  2. Immediately next door, arc 929 → 930 stays uncovered: the true side of if not isinstance(network.deny_out, Unset). No test makes get_info return a deny_out at all. Pre-existing and out of scope here, but it's the same read path this PR is tightening, and it's the natural place to close the gap if you add a deny_out case while you're here.

result["username"] = username

return result

Expand Down
35 changes: 35 additions & 0 deletions packages/python-sdk/tests/shared/sandbox/test_egress_proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,25 @@ def test_create_rejects_a_malformed_egress_proxy(egress_proxy):
build_network_config(cast(Any, {"egress_proxy": egress_proxy}))


def test_create_omits_credentials_that_are_none():
# ``{"username": os.environ.get("PROXY_USER")}`` on an unset variable is the
# way this happens; a JSON null is rejected by the API.
body = build_network_config(
cast(
Any,
{
"egress_proxy": {
"address": "proxy.example.com:1080",
"username": None,
"password": None,
},
},
)
)
assert body is not None
assert body["egress_proxy"].to_dict() == {"address": "proxy.example.com:1080"}


def test_create_strips_unknown_egress_proxy_keys():
# An untyped caller can copy an extra key out of a config file; the API
# rejects unknown properties.
Expand Down Expand Up @@ -145,6 +164,22 @@ def test_get_info_reports_the_active_egress_proxy_without_the_password():
}


def test_get_info_drops_a_none_username():
# ``username`` is ``NotRequired[str]``, so absence is a missing key — a None
# from the wire has to be normalized rather than handed to a caller.
info = from_client_network_config(
SandboxNetworkConfig(
egress_proxy=ClientSandboxEgressProxyConfig(
address="proxy.example.com:1080",
username=cast(Any, None),
)
)
)

assert info is not None
assert info["egress_proxy"] == {"address": "proxy.example.com:1080"}


@pytest.mark.parametrize(
"egress_proxy",
[
Expand Down
Loading