feat(proxy): route public box service traffic#995
Merged
DorianZheng merged 11 commits intoJul 23, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesNetwork tunnel and preview routing
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant EdgeProxy
participant Runner
participant Guest
Client->>API: Request network tunnel URI
API-->>Client: Encoded preview URI
Client->>EdgeProxy: CONNECT preview host:port
EdgeProxy->>Runner: Authenticated CONNECT tunnel
Runner->>Guest: Open guest port stream
Guest-->>Runner: Connected stream
Runner-->>EdgeProxy: Tunnel established
EdgeProxy-->>Client: 200 Connection Established
Client<<->>Guest: Bidirectional traffic
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
G4614
force-pushed
the
codex/shared-forwarded-proxy
branch
from
July 21, 2026 13:51
bb55fdc to
0454697
Compare
|
|
|
|
G4614
changed the title
1 task
G4614
marked this pull request as ready for review
July 22, 2026 03:46
📦 BoxLite review — couldn't completepowered by BoxLite |
G4614
enabled auto-merge
July 22, 2026 03:46
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/box/services/box.service.ts (1)
223-283: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBox preview visibility now defaults to public. Both sites reflect the same root-cause change:
box.publicdefaults totrueinstead offalsewhen the caller omits it, so every new/warm-pool-assigned box becomes previewable by default rather than opt-in.
apps/api/src/box/services/box.service.ts#L223-L283: confirm this default flip (in bothcreateandassignWarmPoolBox) is intentional for this PR and not an inadvertent side-effect of wiring up public box-service routing; if intentional, call it out in release/migration notes since it changes exposure for callers that don't explicitly setpublic.apps/api/src/box/dto/create-box.dto.ts#L58-L65: this is just the Swagger-doc mirror of the service-layer default; no separate code change needed once the service-layer default is confirmed intentional.🤖 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/box/services/box.service.ts` around lines 223 - 283, Confirm the box.public default flip to true in both create and assignWarmPoolBox is intentional rather than an accidental routing side effect; if intentional, document the changed default exposure in release or migration notes. The Swagger default in apps/api/src/box/dto/create-box.dto.ts lines 58-65 requires no separate code change once the service-layer behavior is confirmed.
🧹 Nitpick comments (2)
apps/runner/pkg/api/controllers/proxy.go (1)
188-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTerminal port
22222is a magic number duplicated across services.
isTerminalToolboxPathhardcodes/proxy/22222, matchingTERMINAL_PREVIEW_PORT = 22222inapps/api/src/box/services/box.service.ts, but there's no shared constant between the Go runner and the TypeScript API. A future change to one without the other would silently break terminal routing.🤖 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/runner/pkg/api/controllers/proxy.go` around lines 188 - 191, Remove the hardcoded 22222 value from isTerminalToolboxPath and replace it with a shared terminal preview port configuration or constant used by both the Go runner and TypeScript API. Ensure the generated terminal paths and the TERMINAL_PREVIEW_PORT value remain synchronized when matching the root and nested /proxy paths.apps/proxy/pkg/proxy/proxy.go (1)
195-198: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding
ReadHeaderTimeoutto mitigate slow-header DoS.Static analysis flags the missing server timeouts (Slowloris risk, CWE-400). Since this server now also carries long-lived CONNECT tunnels, avoid a blanket
ReadTimeout/WriteTimeout(which would bound the entire tunnel duration) — butReadHeaderTimeoutonly bounds header-reading and is safe to add without affecting active tunnels.🛡️ Proposed fix
httpServer := &http.Server{ - Addr: fmt.Sprintf(":%d", config.ProxyPort), + Addr: fmt.Sprintf(":%d", config.ProxyPort), + ReadHeaderTimeout: 10 * time.Second, Handler: connectAwareHandler(http.HandlerFunc(proxy.handleTunnelConnect), router, shutdownWg), }🤖 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/proxy/pkg/proxy/proxy.go` around lines 195 - 198, Update the http.Server initialization in the proxy handler to set ReadHeaderTimeout, limiting header-reading time while leaving long-lived CONNECT tunnel activity unaffected. Keep the existing Addr and Handler configuration unchanged.Source: Linters/SAST tools
🤖 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/box/services/box.service.ts`:
- Line 223: Update the box creation logic around the public assignment in the
box service to preserve the existing default of false when createBoxDto.public
is omitted. Keep explicit public: true and public: false values unchanged, and
apply the same correction to the corresponding second assignment.
In `@apps/proxy/pkg/proxy/tunnel.go`:
- Around line 24-65: Update handleTunnelConnect to call the existing
getBoxPublic/Authenticate authorization path after resolving boxID and before
getBoxRunnerInfo or dialRunnerTunnel. Reject boxes that are not public with HTTP
403, ensuring private-box CONNECT requests never dial the runner.
In `@apps/runner/pkg/api/controllers/proxy.go`:
- Around line 230-251: Update handleGuestPortProxy and the surrounding proxy
lifecycle to reuse a *http.Transport per (boxId, port) instead of creating one
for every request. Add bounded, lifecycle-aware caching with idle/TTL eviction,
ensure concurrent requests safely share transports, and close/remove transports
when they expire or the box is deleted; retain the existing ReverseProxy
behavior and error handling.
- Around line 252-265: Update guestPortProxyErrorMessage to avoid returning raw
err.Error() text for unrecognized failures; preserve the existing friendly
message for known connection errors, but return a generic client-facing guest
port proxy failure message while keeping detailed diagnostics exclusively in the
existing logger.WarnContext path.
---
Outside diff comments:
In `@apps/api/src/box/services/box.service.ts`:
- Around line 223-283: Confirm the box.public default flip to true in both
create and assignWarmPoolBox is intentional rather than an accidental routing
side effect; if intentional, document the changed default exposure in release or
migration notes. The Swagger default in apps/api/src/box/dto/create-box.dto.ts
lines 58-65 requires no separate code change once the service-layer behavior is
confirmed.
---
Nitpick comments:
In `@apps/proxy/pkg/proxy/proxy.go`:
- Around line 195-198: Update the http.Server initialization in the proxy
handler to set ReadHeaderTimeout, limiting header-reading time while leaving
long-lived CONNECT tunnel activity unaffected. Keep the existing Addr and
Handler configuration unchanged.
In `@apps/runner/pkg/api/controllers/proxy.go`:
- Around line 188-191: Remove the hardcoded 22222 value from
isTerminalToolboxPath and replace it with a shared terminal preview port
configuration or constant used by both the Go runner and TypeScript API. Ensure
the generated terminal paths and the TERMINAL_PREVIEW_PORT value remain
synchronized when matching the root and nested /proxy paths.
🪄 Autofix (Beta)
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: 0431ead1-f290-45dc-a6ef-15c4e1e2ad2f
📒 Files selected for processing (21)
apps/api-client-go/api/openapi.yamlapps/api/src/box/dto/create-box.dto.tsapps/api/src/box/dto/port-preview-url.dto.tsapps/api/src/box/services/box.service.spec.tsapps/api/src/box/services/box.service.tsapps/api/src/boxlite-rest/boxlite-proxy.controller.spec.tsapps/api/src/boxlite-rest/boxlite-proxy.controller.tsapps/api/src/boxlite-rest/boxlite-rest-routing.spec.tsapps/common-go/pkg/proxy/proxy.goapps/common-go/pkg/proxy/proxy_test.goapps/infra/sst.config.tsapps/proxy/pkg/proxy/get_box_target.goapps/proxy/pkg/proxy/get_box_target_test.goapps/proxy/pkg/proxy/proxy.goapps/proxy/pkg/proxy/tunnel.goapps/proxy/pkg/proxy/tunnel_test.goapps/runner/pkg/api/controllers/proxy.goapps/runner/pkg/api/controllers/proxy_integration_test.goapps/runner/pkg/api/controllers/proxy_test.goapps/runner/pkg/api/server.goapps/runner/pkg/boxlite/guest_port_tunnel.go
G4614
force-pushed
the
codex/shared-forwarded-proxy
branch
from
July 22, 2026 08:40
bceb2b7 to
a14e11a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/api/src/boxlite-rest/boxlite-proxy.controller.spec.ts (1)
56-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the port-validation and non-public-box branches too.
The new test only exercises the happy path. Given
proxyNetworkTunnelalso returns 400 for out-of-range ports (per the controller contract), a quick additional case would guard that branch from regressing.🤖 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/boxlite-proxy.controller.spec.ts` around lines 56 - 64, Add tests alongside the existing proxyNetworkTunnel happy-path case to cover out-of-range port validation and the non-public-box branch, asserting each returns the controller’s expected 400 response and does not invoke the preview URL service. Use the existing makeHarness setup and response mock.
🤖 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/proxy/pkg/proxy/proxy.go`:
- Line 178: Configure a ReadHeaderTimeout on the HTTP server that serves the
CONNECT proxy, using an appropriate bounded duration so slow clients cannot
delay proxy.handleTunnelConnect indefinitely. Do not add a normal WriteTimeout,
since the tunnel handled by connectAwareHandler is intentionally long-lived.
In `@apps/runner/pkg/api/controllers/proxy.go`:
- Around line 141-154: Update the hijacked client connection flow before
ProxyBidirectionalStream to preserve bytes already buffered by buffered.Reader.
Wrap clientConn with a reader/connection that drains buffered.Reader before
reading from the underlying socket, then pass that wrapper to
ProxyBidirectionalStream while retaining the existing response write, flush, and
cleanup behavior.
- Around line 128-132: Update the error response in the DialGuestPort failure
path to return a generic 502 message instead of err.Error(). Preserve the
existing detailed logger.WarnContext call and its error context, while keeping
the current status code and early return behavior unchanged.
---
Nitpick comments:
In `@apps/api/src/boxlite-rest/boxlite-proxy.controller.spec.ts`:
- Around line 56-64: Add tests alongside the existing proxyNetworkTunnel
happy-path case to cover out-of-range port validation and the non-public-box
branch, asserting each returns the controller’s expected 400 response and does
not invoke the preview URL service. Use the existing makeHarness setup and
response mock.
🪄 Autofix (Beta)
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: 0d23c080-98d1-4804-8dc8-77d56ad6a23b
📒 Files selected for processing (23)
apps/api-client-go/api/openapi.yamlapps/api/src/box/dto/create-box.dto.tsapps/api/src/box/dto/port-preview-url.dto.tsapps/api/src/box/services/box.service.spec.tsapps/api/src/box/services/box.service.tsapps/api/src/boxlite-rest/boxlite-proxy.controller.spec.tsapps/api/src/boxlite-rest/boxlite-proxy.controller.tsapps/api/src/boxlite-rest/boxlite-rest-routing.spec.tsapps/common-go/pkg/proxy/proxy.goapps/common-go/pkg/proxy/proxy_test.goapps/infra/sst.config.tsapps/proxy/pkg/proxy/get_box_target.goapps/proxy/pkg/proxy/get_box_target_test.goapps/proxy/pkg/proxy/proxy.goapps/proxy/pkg/proxy/tunnel.goapps/proxy/pkg/proxy/tunnel_test.goapps/runner/pkg/api/controllers/proxy.goapps/runner/pkg/api/controllers/proxy_integration_test.goapps/runner/pkg/api/controllers/proxy_test.goapps/runner/pkg/api/server.goapps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/guest_port_tunnel.goapps/runner/pkg/boxlite/guest_port_tunnel_test.go
🚧 Files skipped from review as they are similar to previous changes (18)
- apps/api/src/box/dto/port-preview-url.dto.ts
- apps/api-client-go/api/openapi.yaml
- apps/api/src/box/services/box.service.ts
- apps/api/src/box/dto/create-box.dto.ts
- apps/runner/pkg/api/server.go
- apps/api/src/boxlite-rest/boxlite-proxy.controller.ts
- apps/runner/pkg/boxlite/guest_port_tunnel_test.go
- apps/common-go/pkg/proxy/proxy_test.go
- apps/proxy/pkg/proxy/tunnel_test.go
- apps/common-go/pkg/proxy/proxy.go
- apps/proxy/pkg/proxy/get_box_target_test.go
- apps/runner/pkg/boxlite/client.go
- apps/runner/pkg/api/controllers/proxy_integration_test.go
- apps/infra/sst.config.ts
- apps/api/src/box/services/box.service.spec.ts
- apps/runner/pkg/boxlite/guest_port_tunnel.go
- apps/runner/pkg/api/controllers/proxy_test.go
- apps/proxy/pkg/proxy/get_box_target.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/proxy/pkg/proxy/proxy.go (1)
236-246: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftClose active CONNECT tunnels before waiting on shutdown. A live CONNECT tunnel stays tracked in
shutdownWgafter hijack, so shutdown can sit until the timeout becausehttp.Server.Shutdownwon’t close it. Add a tunnel-close signal beforeshutdownWg.Wait()and cover the active-tunnel shutdown path in tests.🤖 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/proxy/pkg/proxy/proxy.go` around lines 236 - 246, Update connectAwareHandler to register each hijacked CONNECT tunnel with the existing shutdown mechanism and ensure shutdown signals active tunnels to close before shutdownWg.Wait() runs. Preserve normal CONNECT handling and completion bookkeeping, and add coverage for an active tunnel being closed during shutdown.
🤖 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.
Outside diff comments:
In `@apps/proxy/pkg/proxy/proxy.go`:
- Around line 236-246: Update connectAwareHandler to register each hijacked
CONNECT tunnel with the existing shutdown mechanism and ensure shutdown signals
active tunnels to close before shutdownWg.Wait() runs. Preserve normal CONNECT
handling and completion bookkeeping, and add coverage for an active tunnel being
closed during shutdown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 54d6732f-0515-4a94-badb-78b9bd5754b4
📒 Files selected for processing (6)
apps/common-go/pkg/proxy/proxy.goapps/common-go/pkg/proxy/proxy_test.goapps/proxy/pkg/proxy/proxy.goapps/proxy/pkg/proxy/tunnel.goapps/proxy/pkg/proxy/tunnel_test.goapps/runner/pkg/api/controllers/proxy.go
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/common-go/pkg/proxy/proxy_test.go
- apps/proxy/pkg/proxy/tunnel.go
- apps/common-go/pkg/proxy/proxy.go
- apps/proxy/pkg/proxy/tunnel_test.go
- apps/runner/pkg/api/controllers/proxy.go
G4614
force-pushed
the
codex/shared-forwarded-proxy
branch
from
July 22, 2026 11:12
ae1b144 to
a618d5a
Compare
5 tasks
G4614
force-pushed
the
codex/shared-forwarded-proxy
branch
from
July 22, 2026 12:33
d53b877 to
9af9041
Compare
DorianZheng
disabled auto-merge
July 23, 2026 03:21
DorianZheng
pushed a commit
that referenced
this pull request
Jul 23, 2026
Route browser HTTP previews through the edge proxy over the runner CONNECT tunnel merged in PR #995. Test plan: - [x] `cd apps/common-go && go test -race ./pkg/proxy` - [x] `cd apps/proxy && go test -race ./pkg/proxy` - [x] `cd apps/api && yarn jest --passWithNoTests=true --runInBand src/box/services/box.service.spec.ts --testNamePattern='^BoxService preview URLs'` - [x] `cd apps/api && yarn prettier --check src/box/dto/port-preview-url.dto.ts src/box/services/box.service.ts src/box/services/box.service.spec.ts` - [ ] Pre-push full matrix: local libkrunfw/e2fsprogs native build environment failed. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Preview URLs now use DNS-safe encoded box identifiers for service ports and network tunnels. * Proxy routing improves guest-port handling and supports robust bidirectional stream proxying. * Box activity refresh is more consistently applied during active proxy sessions. * **Bug Fixes** * Non-terminal port preview URLs are now generated instead of being rejected. * Forwarded header handling is tightened so untrusted client values are not propagated. * Box creation defaults to public when omitted (and respects an explicit `false`); legacy preview formats remain supported. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
5 tasks
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.
Route public box service traffic through the API, proxy, runner, and guest-port tunnel path.
Test plan:
Summary by CodeRabbit