feat(bcode-browser): allow overriding the fetch-use endpoint - #157
Open
Alezander9 wants to merge 3 commits into
Open
feat(bcode-browser): allow overriding the fetch-use endpoint#157Alezander9 wants to merge 3 commits into
Alezander9 wants to merge 3 commits into
Conversation
The default endpoint is a general-purpose URL fetcher, so BROWSER_USE_API_KEY is not bounded by whatever network allowlist the process runs under: anything holding the key can ask the fetcher to retrieve an arbitrary host, and a prompt-injected agent can put the key in that URL and read it back out of the attacker's logs. An egress allowlist does not help, because reaching the fetcher is exactly what it permits. BCODE_FETCH_USE_ENDPOINT lets the caller interpose. A sandboxed agent can be given a mediating proxy and a throwaway credential while the real key stays in the parent process, which is the arrangement the RL harness in benchmark-x-laminar needs. Unset, behaviour is unchanged. The test runs a real server on a loopback port and asserts both that the override is used and that the target url arrives in the body, since a proxy has nothing to forward otherwise. Confirmed it fails against the unmodified source rather than passing vacuously.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
The variable names the host that receives X-Browser-Use-API-Key, so a bad value leaks a credential rather than merely failing, and each rejection here is an operator mistake catchable at startup. Set-but-empty was the worst of them: `||` sent it back to the default, which is the direct fetcher -- the exact path someone setting this variable is trying to leave. A typo'd or unexpanded value silently restored the behaviour the override exists to remove, which is the failure you least want to be quiet. `??` distinguishes unset (use the default, the ordinary case) from set and empty (a mistake). Cleartext is rejected outside loopback, since a mediating proxy on the same host is the normal local arrangement and both of our own callers satisfy this already: sandboxes get an https tunnel, local runs get 127.0.0.1. Validation is synchronous and throws rather than failing the Effect, because an error channel on this layer would propagate into ToolRegistry through registry.ts:430, and a startup misconfiguration is not a recoverable condition. Note for the review suggestion this came from: its loopback test used "::1", but URL reports the IPv6 literal with brackets, so an IPv6 endpoint would have been rejected. The test covers that case and fails against the unbracketed form.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…oint Three gaps in the validation added by the previous commit. Loopback was three literal hostnames, but all of 127.0.0.0/8 is loopback and a trailing dot is the same name in rooted form, so a proxy on 127.0.0.2 was refused with a message claiming it was not loopback. The pattern is anchored and numeric so a DNS name like 127.example.com is not mistaken for the subnet. The scheme check ran only against https, so ftp://localhost passed startup on the loopback exemption and would have failed at the first webfetch instead -- the deferred failure this validation exists to pull forward. http or https is now required before the exemption is considered. Both messages echoed the raw value, which can carry userinfo or a token in its query, so a typo wrote a credential into stderr: the same log leak the override exists to close. The cleartext message names url.origin, which drops userinfo, path and query, and the parse failure names only the variable, since an operator can read back their own environment. Tested against a value carrying both a password and a query token. Not adopted: the report also expected a proxy bound on 0.0.0.0 to be accepted. That is a bind address, not a connect target, so refusing it as non-loopback is correct. All four cases fail against the previous implementation and pass against this one. 13 pass / 1 skip; typecheck 17/17.
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
1 participant
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.
Extracted so a sandboxed agent need not hold the real key.
Why
BROWSER_USE_API_KEYis not bounded by the network allowlist of the process holding it. The default fetch-use endpoint is a general-purpose URL fetcher, so any code that can reach it can ask it to retrieve an arbitrary host -- including one carrying the key as a query parameter. A prompt-injected agent then reads the key out of the attacker request log. An egress allowlist offers no protection, because reaching the fetcher is precisely what it permits.We hit this in the RL harness in
benchmark-x-laminar, which runs bcode inside a sandbox on the open web with a live cloud credential in its environment.What
BCODE_FETCH_USE_ENDPOINTlets the caller interpose:The sandboxed agent gets a proxy URL and a disposable token; the real key stays in the parent. Unset, behaviour is unchanged.
Test
Runs a real server on a loopback port and asserts both that the override is used and that the target url arrives in the body -- a mediating proxy has nothing to forward otherwise. Verified it fails against the unmodified source rather than passing vacuously.
bun test packages/bcode-browser2 pass / 1 skip (live test needs a key). Filteredbun run typecheck17/17.Summary by cubic
Allows
bcode-browserto use a configurable fetch-use endpoint with strict startup validation to prevent leakingBROWSER_USE_API_KEY. Previously it always posted to https://fetch.browser-use.com/fetch; now it usesBCODE_FETCH_USE_ENDPOINTwhen set and rejects unsafe values and schemes while avoiding echoing credentials in errors.http/https, or cleartext outside loopback; loopback includeslocalhost(optional trailing dot),[::1], and127.0.0.0/8.url.originonly.urlis in the JSON body with the API key header; tests cover accepted/rejected values (IPv6 brackets, rootedlocalhost).enabledstill reflects presence ofBROWSER_USE_API_KEY.BCODE_FETCH_USE_ENDPOINTto your mediating proxy (httpsor loopbackhttp). Give the sandbox a disposableBROWSER_USE_API_KEY; keep the real key in the proxy. No change needed if you do not interpose.Written for commit d9d571e. Summary will update on new commits.