Harden git extension auth proxy#802
Conversation
ghostwriternr
requested review from
aron-cf,
scuffi and
whoiskatrin
as code owners
|
commit: |
📦 Preview BuildVersion: Install the SDK preview:
|
![devin-ai-integration[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Extension HTTP proxy leases are not cleaned up when the container stops, unlike other outbound interceptions
The in-memory proxy lease map is never cleared on container stop (onStop at packages/sandbox/src/sandbox.ts:2697), so stale entries persist if the Durable Object stays alive across restarts — unlike R2 and S3 credential proxy mounts, which are both explicitly cleaned up in the same method.
Impact: After a container restart, the next authenticated git clone would push stale lease entries into the new container's outbound configuration, wasting resources and potentially confusing diagnostics.
Mechanism: onStop cleans up R2 and S3 proxy mounts but not extension HTTP proxy leases
In packages/sandbox/src/sandbox.ts:2717-2739, the onStop method:
- Checks for R2 egress mounts and calls
configureR2EgressOutbound({ buckets: {} })(line 2731) - Checks for credential proxy mounts and calls
configureS3CredentialProxyOutbound({ mounts: {} })(line 2734) - Clears
this.activeMounts(line 2739)
But there is no equivalent cleanup for this.extensionHTTPProxyLeases (declared at packages/sandbox/src/sandbox.ts:974-978). If a withHTTPProxyLease operation is in-flight when onStop fires, the finally block in packages/sandbox/src/extensions/index.ts:178-179 tries lease.dispose(), which calls configureExtensionHTTPProxyOutbound(). That call can fail because the container is no longer running, leaving the lease entry stranded in the map. On the next container start, registerExtensionHTTPProxyLease will include those stale entries when calling configureExtensionHTTPProxyOutbound (packages/sandbox/src/sandbox.ts:5214).
The fix should add to onStop:
if (this.extensionHTTPProxyLeases.size > 0) {
this.extensionHTTPProxyLeases.clear();
await this.configureExtensionHTTPProxyOutbound().catch(() => {});
}(Refers to lines 2738-2739)
Was this helpful? React with 👍 or 👎 to provide feedback.
| function isRedirect(status: number): boolean { | ||
| return status >= 300 && status < 400; | ||
| } |
There was a problem hiding this comment.
🟡 Non-redirect 3xx responses (e.g. 304 Not Modified) are misclassified as redirects and converted to 502 errors
Any non-redirect 3xx status from the upstream (like 304 Not Modified) is treated as a redirect (isRedirect at packages/sandbox/src/extensions/http-proxy-handler.ts:17) and, because it typically lacks a Location header, the proxy returns a 502 error instead of forwarding it.
Impact: Conditional responses from an upstream server are converted into hard errors, which could break caching and cause spurious failures for any extension using this proxy.
Mechanism: isRedirect is over-broad — matches all 3xx, not just redirect codes
The function isRedirect at packages/sandbox/src/extensions/http-proxy-handler.ts:16-18 uses status >= 300 && status < 400, which matches 304 Not Modified, 300 Multiple Choices, and 305 Use Proxy. Only 301, 302, 303, 307, and 308 are actual redirects that need Location header rewriting.
When a 304 arrives:
isRedirect(304)returnstrue(packages/sandbox/src/extensions/http-proxy-handler.ts:92)upstreamResponse.headers.get('location')isnull(304 has no Location)rewriteRedirectLocation(null, ...)returnsnull(packages/sandbox/src/extensions/http-proxy-handler.ts:27)- The handler returns
502(packages/sandbox/src/extensions/http-proxy-handler.ts:104-106)
The correct set of redirect codes is [301, 302, 303, 307, 308].
| function isRedirect(status: number): boolean { | |
| return status >= 300 && status < 400; | |
| } | |
| function isRedirect(status: number): boolean { | |
| return [301, 302, 303, 307, 308].includes(status); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Harden git extension auth proxy
This keeps the git extension extraction while replacing the git-specific core auth hook with a generic extension HTTP proxy lease. Authenticated HTTPS clones are rewritten to an operation-scoped internal proxy URL, credentials stay in Worker-side proxy state, and the cloned repository origin is reset back to the sanitized public URL after checkout.
The lease lifecycle now rejects duplicate operation IDs, rolls back failed registration, and keeps disposal retryable until outbound proxy configuration is refreshed. This avoids stale credential routes and prevents public-host interception such as
github.comfrom overriding user egress policy.Also updates stale standalone-binary docs and restores the public code-interpreter example image.
Validation:
npm run checknpm test -w @cloudflare/sandboxNote: I also tried root
npm test, but this machine cannot connect to the Docker daemon needed by@repo/sandbox-execution(Cannot connect to the Docker daemon at unix:///Users/naresh/.colima/default/docker.sock).