feat: SSH-equivalent sandbox access over HTTPS (WebSocket exec + MCP) - #3
Merged
Conversation
Closes #2 Daemon (toolbox): - GET /process/exec/connect: WebSocket exec channel with SSH channel semantics. Client frames start/stdin/signal/resize/stdin_eof; server frames stdout/stderr/exit/error. With a command it streams output and reports the exit code (SIGINT on sleep 60 -> 130); without one it spawns an interactive login shell (bash -l, fallback sh) on a PTY with control-char signal delivery and resize support. The exit frame is always sent before close. Built on SessionService + cmdWrapperFormat (no parallel command runner); PTY support is additive-only. - POST /mcp: stateless streamable-HTTP MCP endpoint with tools exec_command, fs_read_file, fs_write_file, fs_list_files. Adds exactly one new dependency: github.com/modelcontextprotocol/go-sdk. Proxy: - SSH access tokens (Authorization: Bearer or ?token= query) are accepted for /{sandboxId}/process/exec/connect and /{sandboxId}/mcp. Tokens are validated per connection via /sandbox/ssh-access/validate (no caching, so revocation blocks new connections immediately) and non-started sandboxes are rejected with an explicit state message, matching the SSH gateway. Sandbox activity keepalive piggybacks on the existing last-activity polling (on connect + interval), same as ssh-gateway. API: - validateSshAccess now allows the proxy via OrGuard (same pattern as the other proxy-reachable endpoints); auth spec updated. Generated/docs: - swag init regenerated toolbox swagger docs; toolbox API clients (go, ts, java, python, python-async) regenerated with the new routes; api clients regenerated with zero diff. New docs page "SSH over HTTPS" documents the wire protocol. Verification: - go build ./apps/daemon/... ./apps/proxy/... - go test ./apps/daemon/... (incl. new unit tests for the WS frame protocol start/stdin/signal/exit and the MCP tool handlers) - golangci-lint run: 0 issues for daemon and proxy - npx nx test api: 54 suites / 562 tests pass - Live smoke over real TCP: exec stdout/stderr/exit, SIGINT -> 130, interactive shell state, concurrent connections, MCP initialize/tools/list/stateless tools/call, fs roundtrip
📝 WalkthroughWalkthroughAdds authenticated SSH-equivalent access over HTTPS through WebSocket exec and stateless MCP endpoints, with session/PTY support, proxy token validation, generated client APIs, Swagger contracts, documentation, and tests. ChangesSSH over HTTPS access
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy
participant Toolbox
participant Session
Client->>Proxy: Connect with SSH access token
Proxy->>Toolbox: Forward authenticated request
Toolbox->>Session: Start exec session or MCP command
Session-->>Toolbox: Stream output and exit status
Toolbox-->>Client: Return WebSocket or MCP response
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/toolbox-api-client/src/api/process-api.ts (1)
272-298: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse native WebSocket clients for
execConnect.
/process/exec/connectupgrades to WebSocket and exchanges start/stdin/stdout/exit frames;AxiosPromise<void>andOkHttpClient.call()only produce an ordinary HTTP request/response.
libs/toolbox-api-client/src/api/process-api.ts: expose a WebSocket/session API or URL builder for callers’ WebSocket implementation instead of resolving this as an Axios request.libs/toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/api/ProcessApi.java: useOkHttpClient.newWebSocketand return a WebSocket/session abstraction with listener callbacks.libs/toolbox-api-client-java/src/test/java/io/daytona/toolbox/client/api/ProcessApiTest.java: add an enabled integration test covering WebSocket upgrade and start/stdin/output/exit frames.🤖 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 `@libs/toolbox-api-client/src/api/process-api.ts` around lines 272 - 298, The execConnect implementation must use native WebSocket semantics rather than resolving an Axios HTTP request. In libs/toolbox-api-client/src/api/process-api.ts:272-298, expose a WebSocket/session API or URL builder for callers’ WebSocket implementation; in libs/toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/api/ProcessApi.java:850-955, replace OkHttpClient.call() with newWebSocket and return a listener-backed WebSocket/session abstraction; in libs/toolbox-api-client-java/src/test/java/io/daytona/toolbox/client/api/ProcessApiTest.java:140-145, add an enabled integration test covering upgrade and start, stdin, output, and exit frames.
🧹 Nitpick comments (2)
apps/daemon/pkg/toolbox/process/exec/controller.go (1)
195-223: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStdin/signal/resize failures are swallowed instead of surfaced to the client.
Unlike the unknown-signal (Line 207) and unknown-frame-type (Line 227) cases, which emit an
ErrorFrame, failures fromWriteStdin,CloseStdin,Signal, andResize(Lines 197-223) are only logged atDebuglevel. A client whose signal or stdin write silently failed (e.g. command already exited, stdin already closed) has no way to know from the protocol and may wait indefinitely for output that will never come.♻️ Suggested fix
case FrameTypeStdin: if err := sess.WriteStdin([]byte(frame.Data)); err != nil { logger.Debug("stdin write failed", "error", err) + emit(ErrorFrame{Type: FrameTypeError, Message: fmt.Sprintf("stdin write failed: %v", err)}, false) } case FrameTypeStdinEOF: if err := sess.CloseStdin(); err != nil { logger.Debug("stdin close failed", "error", err) + emit(ErrorFrame{Type: FrameTypeError, Message: fmt.Sprintf("stdin close failed: %v", err)}, false) } case FrameTypeSignal: ... if err := sess.Signal(sig); err != nil { logger.Debug("signal failed", "signal", frame.Signal, "error", err) + emit(ErrorFrame{Type: FrameTypeError, Message: fmt.Sprintf("signal failed: %v", err)}, false) } case FrameTypeResize: ... if err := sess.Resize(frame.Cols, frame.Rows); err != nil { logger.Debug("resize failed", "error", err) + emit(ErrorFrame{Type: FrameTypeError, Message: fmt.Sprintf("resize failed: %v", err)}, false) }🤖 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/daemon/pkg/toolbox/process/exec/controller.go` around lines 195 - 223, Update the FrameTypeStdin, FrameTypeStdinEOF, FrameTypeSignal, and FrameTypeResize branches in the frame-processing switch to emit an ErrorFrame to the client whenever the corresponding sess operation fails, including the operation error in the message. Preserve the existing debug logging if useful, and keep successful operations and validation behavior unchanged.apps/daemon/pkg/toolbox/process/exec/demux_test.go (1)
49-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover an incomplete marker at EOF.
Lines 55-59 only test markers that eventually complete. Add a case that writes a proper marker prefix, calls
Flush, and asserts those bytes are emitted as normal stream content.🤖 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/daemon/pkg/toolbox/process/exec/demux_test.go` around lines 49 - 67, Add a test case alongside TestStreamDemuxSplitMarkerAcrossChunks that writes only a valid marker prefix to the stream, calls d.Flush(), and verifies the prefix is emitted through the normal stream-content path rather than discarded or treated as a marker.
🤖 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/daemon/pkg/session/exec_support.go`:
- Around line 45-58: Update WriteInput to guard session.cmd against nil before
accessing ProcessState, matching the existing safety check in SignalDescendants.
Preserve the current GoneError behavior for an assigned command whose process
has exited, while avoiding a panic when the shell process has not yet been
assigned.
- Around line 64-83: Update the stdin write flow around syscall.Write to ensure
every byte in data is written, looping over the remaining slice until complete
and advancing by the number of bytes written; preserve the existing blocking
configuration from syscall.SetNonblock(fd, false) and return the same internal
error response if a write fails.
In `@apps/daemon/pkg/session/execute.go`:
- Around line 219-220: Update the async FIFO writer setup around the
input-holder PID recording so input_holder.pid identifies a single terminable
holder process or process group encompassing both the while/sleep loop and FIFO
writer. Ensure CloseInput terminates the entire holder, preventing inherited
sleep children from delaying EOF, and remove input_holder.pid during cleanup.
In `@apps/daemon/pkg/toolbox/docs/docs.go`:
- Around line 2455-2476: Model the complete MCP transport contract across
apps/daemon/pkg/toolbox/docs/docs.go (lines 2455-2476),
apps/daemon/pkg/toolbox/docs/swagger.json (lines 2140-2154), and
apps/daemon/pkg/toolbox/docs/swagger.yaml (lines 2774-2792): define JSON-RPC
POST payloads, GET/DELETE operations, valid streaming media types without the
leading space, and SSE responses. Regenerate or manually update
libs/toolbox-api-client-go/api_mcp.go (lines 41-113) so MCP requests accept
payloads, advertise streaming formats, and read streaming responses.
In `@apps/daemon/pkg/toolbox/mcp/server.go`:
- Around line 74-80: Update the MCP Swagger annotations near the endpoint
documentation to define separate method-specific operations for POST, GET, and
DELETE, each with a unique operation ID and the appropriate response/transport
details. Ensure the generated Swagger spec is regenerated so GET SSE support and
DELETE are discoverable.
In `@apps/daemon/pkg/toolbox/mcp/tools.go`:
- Around line 171-187: Update the read-file flow around the existing os.Stat,
os.ReadFile, and readFileResult logic to open the path first, validate the
opened descriptor is a regular file, and read through a limit of
maxReadFileBytes+1 so growth or special files cannot cause unbounded allocation;
return the existing tool error format when the limit is exceeded, close the
file, and add a regression test covering an oversized or special file.
In `@apps/daemon/pkg/toolbox/process/exec/types.go`:
- Around line 52-64: The process stream payloads in ClientFrame.Data and
OutputFrame.Data must preserve arbitrary bytes instead of JSON/text coercing
invalid UTF-8. Update the frame encoding and WebSocket transport used by the
process execution path to use base64 or binary WebSocket frames consistently for
stdin, stdout, and stderr, while keeping the existing frame metadata and
ensuring decoding restores the original bytes.
In `@apps/proxy/pkg/proxy/agent_access.go`:
- Around line 40-41: Update the token validation call inside the
RetryWithExponentialBackoff callback to pass the caller’s ctx instead of
context.Background(), preserving cancellation and request lifetime propagation
through ValidateSshAccess.
In `@apps/proxy/pkg/proxy/auth.go`:
- Around line 35-59: Update the successful regular Bearer-token authentication
path before its early return, as well as the valid SSH-token path in the
allowSshAccessToken flow, to call ensureSandboxStarted. When the check fails,
return its error immediately; preserve credential stripping and existing return
behavior after a successful state check.
In `@libs/toolbox-api-client-go/api_process.go`:
- Around line 944-1007: The ExecConnectExecute method currently uses the
generated REST request flow, which cannot establish the WebSocket connection.
Replace its HTTP response return type and callAPI-based execution with the SDK’s
dedicated WebSocket dial entrypoint, preserving the existing context, server
URL, token, and endpoint path while returning the bidirectional WebSocket
connection and dial error.
In `@libs/toolbox-api-client-go/api/openapi.yaml`:
- Around line 1727-1738: Update the /mcp OpenAPI path: add a typed requestBody
for POST containing the JSON-RPC message schema, declare its SSE response media
type and event payload schema, and add a GET operation for the SSE stream if
supported by the implementation. Reuse existing JSON-RPC and SSE schema symbols
where available, and keep the MCP operation’s authentication and endpoint
behavior consistent.
- Around line 1800-1820: The OpenAPI definition for ExecConnect currently causes
REST SDKs to generate a plain GET method for the bidirectional WebSocket
endpoint. Exclude ExecConnect from REST-client generation and document its
WebSocket URL separately, or provide a dedicated WebSocket client/session
abstraction that sends the start frame and supports streaming frames instead of
returning an HTTP response.
In
`@libs/toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/api/McpApi.java`:
- Around line 100-126: The MCP client methods currently send empty POST bodies,
preventing JSON-RPC requests. Update McpApi in
libs/toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/api/McpApi.java#L100-L126
to accept a JSON-RPC request body, pass it as localVarPostBody, and use the MCP
JSON content type; update the corresponding request method in
libs/toolbox-api-client-python-async/daytona_toolbox_api_client_async/api/mcp_api.py#L222-L270
and
libs/toolbox-api-client-python/daytona_toolbox_api_client/api/mcp_api.py#L222-L270
to expose and serialize the same body contract instead of passing None.
---
Outside diff comments:
In `@libs/toolbox-api-client/src/api/process-api.ts`:
- Around line 272-298: The execConnect implementation must use native WebSocket
semantics rather than resolving an Axios HTTP request. In
libs/toolbox-api-client/src/api/process-api.ts:272-298, expose a
WebSocket/session API or URL builder for callers’ WebSocket implementation; in
libs/toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/api/ProcessApi.java:850-955,
replace OkHttpClient.call() with newWebSocket and return a listener-backed
WebSocket/session abstraction; in
libs/toolbox-api-client-java/src/test/java/io/daytona/toolbox/client/api/ProcessApiTest.java:140-145,
add an enabled integration test covering upgrade and start, stdin, output, and
exit frames.
---
Nitpick comments:
In `@apps/daemon/pkg/toolbox/process/exec/controller.go`:
- Around line 195-223: Update the FrameTypeStdin, FrameTypeStdinEOF,
FrameTypeSignal, and FrameTypeResize branches in the frame-processing switch to
emit an ErrorFrame to the client whenever the corresponding sess operation
fails, including the operation error in the message. Preserve the existing debug
logging if useful, and keep successful operations and validation behavior
unchanged.
In `@apps/daemon/pkg/toolbox/process/exec/demux_test.go`:
- Around line 49-67: Add a test case alongside
TestStreamDemuxSplitMarkerAcrossChunks that writes only a valid marker prefix to
the stream, calls d.Flush(), and verifies the prefix is emitted through the
normal stream-content path rather than discarded or treated as a marker.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bd1b718-3ca5-4227-8f7e-5b7dc9a8e9ca
⛔ Files ignored due to path filters (1)
apps/daemon/go.sumis excluded by!**/*.sum
📒 Files selected for processing (55)
apps/api/src/sandbox/controllers/sandbox.controller.auth.spec.tsapps/api/src/sandbox/controllers/sandbox.controller.tsapps/daemon/go.modapps/daemon/pkg/session/exec_support.goapps/daemon/pkg/session/execute.goapps/daemon/pkg/toolbox/docs/docs.goapps/daemon/pkg/toolbox/docs/swagger.jsonapps/daemon/pkg/toolbox/docs/swagger.yamlapps/daemon/pkg/toolbox/mcp/server.goapps/daemon/pkg/toolbox/mcp/tools.goapps/daemon/pkg/toolbox/mcp/tools_test.goapps/daemon/pkg/toolbox/process/exec/controller.goapps/daemon/pkg/toolbox/process/exec/controller_test.goapps/daemon/pkg/toolbox/process/exec/demux.goapps/daemon/pkg/toolbox/process/exec/demux_test.goapps/daemon/pkg/toolbox/process/exec/session_exec.goapps/daemon/pkg/toolbox/process/exec/shell_exec.goapps/daemon/pkg/toolbox/process/exec/types.goapps/daemon/pkg/toolbox/process/pty/controller.goapps/daemon/pkg/toolbox/process/pty/ephemeral.goapps/daemon/pkg/toolbox/process/pty/session.goapps/daemon/pkg/toolbox/process/pty/types.goapps/daemon/pkg/toolbox/process/pty/websocket.goapps/daemon/pkg/toolbox/server.goapps/docs/src/content/docs/en/ssh-over-https.mdxapps/docs/src/content/i18n/en.jsonapps/docs/src/content/i18n/ja.jsonapps/docs/src/sidebar-config.tsapps/proxy/pkg/proxy/agent_access.goapps/proxy/pkg/proxy/auth.goapps/proxy/pkg/proxy/get_sandbox_target.golibs/toolbox-api-client-go/.openapi-generator/FILESlibs/toolbox-api-client-go/api/openapi.yamllibs/toolbox-api-client-go/api_mcp.golibs/toolbox-api-client-go/api_process.golibs/toolbox-api-client-go/client.golibs/toolbox-api-client-java/.openapi-generator/FILESlibs/toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/api/McpApi.javalibs/toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/api/ProcessApi.javalibs/toolbox-api-client-java/src/test/java/io/daytona/toolbox/client/api/McpApiTest.javalibs/toolbox-api-client-java/src/test/java/io/daytona/toolbox/client/api/ProcessApiTest.javalibs/toolbox-api-client-python-async/.openapi-generator/FILESlibs/toolbox-api-client-python-async/daytona_toolbox_api_client_async/__init__.pylibs/toolbox-api-client-python-async/daytona_toolbox_api_client_async/api/__init__.pylibs/toolbox-api-client-python-async/daytona_toolbox_api_client_async/api/mcp_api.pylibs/toolbox-api-client-python-async/daytona_toolbox_api_client_async/api/process_api.pylibs/toolbox-api-client-python/.openapi-generator/FILESlibs/toolbox-api-client-python/daytona_toolbox_api_client/__init__.pylibs/toolbox-api-client-python/daytona_toolbox_api_client/api/__init__.pylibs/toolbox-api-client-python/daytona_toolbox_api_client/api/mcp_api.pylibs/toolbox-api-client-python/daytona_toolbox_api_client/api/process_api.pylibs/toolbox-api-client/src/.openapi-generator/FILESlibs/toolbox-api-client/src/api.tslibs/toolbox-api-client/src/api/mcp-api.tslibs/toolbox-api-client/src/api/process-api.ts
Comment on lines
+35
to
+59
| // Agent-access endpoints additionally accept SSH access tokens (Bearer or | ||
| // ?token= query for WebSocket clients that cannot set headers). | ||
| if allowSshAccessToken { | ||
| sshToken, fromQuery := bearerToken, false | ||
| if sshToken == "" { | ||
| sshToken, fromQuery = ctx.Query(SSH_ACCESS_TOKEN_QUERY_PARAM), true | ||
| } | ||
| if sshToken != "" { | ||
| isValid, err := p.getSshAccessTokenValid(ctx.Request.Context(), sandboxIdOrSignedToken, sshToken) | ||
| if err != nil { | ||
| authErrors = append(authErrors, fmt.Sprintf("SSH access token validation error: %v", err)) | ||
| } else if isValid != nil && *isValid { | ||
| // A valid token was presented: enforce the same started-state | ||
| // check as the SSH gateway and fail fast on its result. | ||
| if err := p.ensureSandboxStarted(ctx.Request.Context(), sandboxIdOrSignedToken); err != nil { | ||
| return sandboxIdOrSignedToken, false, err | ||
| } | ||
| // Do not forward the credential to the sandbox | ||
| ctx.Request.Header.Del("Authorization") | ||
| if fromQuery { | ||
| newQuery := ctx.Request.URL.Query() | ||
| newQuery.Del(SSH_ACCESS_TOKEN_QUERY_PARAM) | ||
| ctx.Request.URL.RawQuery = newQuery.Encode() | ||
| } | ||
| return sandboxIdOrSignedToken, false, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce the started-state check for every agent credential.
A valid regular Bearer token returns at Lines 22-29 before this SSH-only check, so /mcp and /process/exec/connect can still forward requests for stopped sandboxes and fail upstream rather than returning the required explicit state error. Apply ensureSandboxStarted on every successful authentication path when allowSshAccessToken is true.
🤖 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/auth.go` around lines 35 - 59, Update the successful
regular Bearer-token authentication path before its early return, as well as the
valid SSH-token path in the allowSshAccessToken flow, to call
ensureSandboxStarted. When the check fails, return its error immediately;
preserve credential stripping and existing return behavior after a successful
state check.
Comment on lines
+944
to
+1007
| func (a *ProcessAPIService) ExecConnectExecute(r ProcessAPIExecConnectRequest) (*http.Response, error) { | ||
| var ( | ||
| localVarHTTPMethod = http.MethodGet | ||
| localVarPostBody interface{} | ||
| formFiles []formFile | ||
| ) | ||
|
|
||
| localBasePath, err := a.client.cfg.ServerURLWithContext(r.ctx, "ProcessAPIService.ExecConnect") | ||
| if err != nil { | ||
| return nil, &GenericOpenAPIError{error: err.Error()} | ||
| } | ||
|
|
||
| localVarPath := localBasePath + "/process/exec/connect" | ||
|
|
||
| localVarHeaderParams := make(map[string]string) | ||
| localVarQueryParams := url.Values{} | ||
| localVarFormParams := url.Values{} | ||
|
|
||
| if r.token != nil { | ||
| parameterAddToHeaderOrQuery(localVarQueryParams, "token", r.token, "", "") | ||
| } | ||
| // to determine the Content-Type header | ||
| localVarHTTPContentTypes := []string{} | ||
|
|
||
| // set Content-Type header | ||
| localVarHTTPContentType := selectHeaderContentType(localVarHTTPContentTypes) | ||
| if localVarHTTPContentType != "" { | ||
| localVarHeaderParams["Content-Type"] = localVarHTTPContentType | ||
| } | ||
|
|
||
| // to determine the Accept header | ||
| localVarHTTPHeaderAccepts := []string{} | ||
|
|
||
| // set Accept header | ||
| localVarHTTPHeaderAccept := selectHeaderAccept(localVarHTTPHeaderAccepts) | ||
| if localVarHTTPHeaderAccept != "" { | ||
| localVarHeaderParams["Accept"] = localVarHTTPHeaderAccept | ||
| } | ||
| req, err := a.client.prepareRequest(r.ctx, localVarPath, localVarHTTPMethod, localVarPostBody, localVarHeaderParams, localVarQueryParams, localVarFormParams, formFiles) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| localVarHTTPResponse, err := a.client.callAPI(req) | ||
| if err != nil || localVarHTTPResponse == nil { | ||
| return localVarHTTPResponse, err | ||
| } | ||
|
|
||
| localVarBody, err := io.ReadAll(localVarHTTPResponse.Body) | ||
| localVarHTTPResponse.Body.Close() | ||
| localVarHTTPResponse.Body = io.NopCloser(bytes.NewBuffer(localVarBody)) | ||
| if err != nil { | ||
| return localVarHTTPResponse, err | ||
| } | ||
|
|
||
| if localVarHTTPResponse.StatusCode >= 300 { | ||
| newErr := &GenericOpenAPIError{ | ||
| body: localVarBody, | ||
| error: localVarHTTPResponse.Status, | ||
| } | ||
| return localVarHTTPResponse, newErr | ||
| } | ||
|
|
||
| return localVarHTTPResponse, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 \
'ExecConnectExecute|prepareRequest|callAPI|UpgradeToWebSocket' \
libs/toolbox-api-client-go/api_process.go \
apps/daemon/pkg/toolbox/process/exec/controller.goRepository: arrrrny/daytona
Length of output: 25457
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate websocket-related files and symbols =="
rg -n 'websocket|WebSocket|UpgradeToWebSocket|DialContext|Dialer|UpgradeToWebSocket\(' --glob '!**/vendor/**' .
echo
echo "== controller Connect implementation =="
sed -n '1,130p' apps/daemon/pkg/toolbox/process/exec/controller.go
echo
echo "== websocket upgrade implementation candidate =="
rg -n -C 8 'func UpgradeToWebSocket|type .*Util|websocket\.Dial|Upgrade\(' --glob '!**/vendor/**' .Repository: arrrrny/daytona
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== toolbox client-go API interface =="
sed -n '100,135p' libs/toolbox-api-client-go/api_process.go
sed -n '900,945p' libs/toolbox-api-client-go/api_process.go
echo
echo "== toolbox client-go prepareRequest / callAPI implementations =="
fd -a 'api_client.go|.*client.*\.go' libs/toolbox-api-client-go | sed -n '1,20p'
rg -n -C 12 'func .*prepareRequest|func .*callAPI' libs/toolbox-api-client-go
echo
echo "== toolbox client-go dependency files and websocket imports =="
fd -a 'go.mod|go.sum' libs/toolbox-api-client-go -a | sed -n '1,20p'
for f in $(fd 'go.mod|go.sum' libs/toolbox-api-client-go); do
echo "--- $f"
sed -n '/toolbox-api-client-go/,/)/p' "$f" | head -80
done
echo
echo "== compare other SDK exec-connect docs/imports if available =="
rg -n -C 3 'Exec Connect|ExecConnect|/process/exec/connect|websocket.Dial|DialContext|101|Switching Protocols' libs/toolbox-api-client-java libs/toolbox-api-client-python-async libs/toolbox-api-client/src apps/daemon/internal/util apps/daemon/pkg/toolbox/server.goRepository: arrrrny/daytona
Length of output: 42811
🌐 Web query:
Go net/http DefaultTransport DialTLSContext TLS handshake upgrades to HTTP/2 no WebSocket 101 behavior
💡 Result:
In Go's net/http package, the DefaultTransport does not perform HTTP/2 upgrades for WebSocket 101 Switching Protocols because the HTTP/1.1 Upgrade mechanism is fundamentally incompatible with HTTP/2 [1]. Key technical points regarding this behavior: 1. Protocol Incompatibility: The HTTP Upgrade mechanism and the 101 Switching Protocols status code are specific to HTTP/1.1 [1]. HTTP/2 and HTTP/3 do not support the Upgrade header or 101 status code [1]. Instead, WebSocket connections over HTTP/2 and HTTP/3 are established using the Extended CONNECT method (RFC 8441), which uses a CONNECT request with a:protocol pseudo-header set to websocket [1]. 2. DefaultTransport and HTTP/2: While the DefaultTransport automatically supports HTTP/2 for HTTPS connections [2][3], it does not attempt to "upgrade" an existing HTTP/2 connection to a WebSocket connection using the legacy 101/Upgrade flow [1]. If a client attempts to use the HTTP/1.1 Upgrade mechanism over an HTTP/2 connection, it will fail because the protocol does not support it. 3. Extended CONNECT in Go: Go has added support for the Extended CONNECT protocol (RFC 8441) to enable WebSockets-over-HTTP/2 [4][5]. However, this is not enabled by default for clients as of recent versions due to compatibility concerns [4][5]. It can be controlled via the GODEBUG=http2xconnect=1 setting or through future configurations in HTTP2Config [4][5]. 4. DialTLSContext and Connections: When you provide a custom DialTLSContext to a Transport, the Transport assumes the returned net.Conn is already past the TLS handshake [6][7]. If the connection is HTTP/2, it remains HTTP/2; if you need to perform an HTTP/1.1-style WebSocket handshake, you must ensure the underlying connection is HTTP/1.1. The Transport's automatic handling of HTTP/2 does not automatically intercept or bridge these disparate protocol upgrade paths [2][3].
Citations:
- 1: https://http.dev/protocol-upgrade
- 2: https://go.dev/src/net/http/transport.go
- 3: https://pkg.go.dev/net/http
- 4: net/http: add support for SETTINGS_ENABLE_CONNECT_PROTOCOL golang/go#53208
- 5: https://go.googlesource.com/net/+/master/http2/http2.go
- 6: https://github.com/golang/go/blob/master/src/net/http/transport.go
- 7: https://github.com/golang/go/blob/go1.16.3/src/net/http/transport.go
🌐 Web query:
github.com/gorilla/websocket Go client client.Dial returns Conn ReadMessage WriteMessage bidirectional API
💡 Result:
The gorilla/websocket package provides a fully bidirectional WebSocket communication API [1][2]. The Dial method initiates a connection to a server and returns a *Conn object [3][4], which is the same type used on the server side after an Upgrade [1][2]. Key points regarding the API and bidirectional usage: 1. Bidirectional Communication: Once a connection is established, the *Conn object allows both sending and receiving messages independently [1][2]. Applications typically handle these tasks concurrently [5][4]. 2. Concurrency Requirements: The library enforces strict concurrency rules for the *Conn object: - Connections support exactly one concurrent reader and one concurrent writer [1][2]. - Applications must ensure that no more than one goroutine calls the read methods (e.g., ReadMessage, NextReader) and no more than one goroutine calls the write methods (e.g., WriteMessage, NextWriter) at the same time [1][5]. 3. Recommended Pattern: The standard way to implement bidirectional communication is to use separate goroutines for reading and writing [5][4]: - A read-dedicated goroutine calls ReadMessage in a loop to handle inbound traffic [6][4]. - A write-dedicated goroutine (or the main goroutine) calls WriteMessage to handle outbound traffic [6][4]. - Channels are commonly used to coordinate these goroutines and pass data between them [5][7]. The ReadMessage method reads the next message from the connection and returns the message type and the message data [1][8], while the WriteMessage method writes a message of a specified type to the connection [1][8]. These methods handle the framing logic, allowing developers to work with messages rather than raw stream data [1][2].
Citations:
- 1: https://github.com/gorilla/websocket/blob/main/doc.go
- 2: https://github.com/gorilla/websocket/blob/v1.5.3/doc.go
- 3: https://github.com/gorilla/websocket/blob/78cf1bc733a927f673fd1988a25256b425552a8a/client.go
- 4: https://tests.ws/guides/golang-gorilla-websocket
- 5: https://github.com/gorilla/websocket/tree/main/examples/chat
- 6: https://github.com/gorilla/websocket/blob/master/examples/echo/client.go
- 7: https://github.com/gorilla/websocket/blob/master/examples/chat/client.go
- 8: https://github.com/gorilla/websocket/blob/c3dd95aea9779669bb3daafbd84ee0530c8ce1c1/conn.go
Use a WebSocket dialer instead of the generated REST executor.
ExecConnectExecute() builds a normal HTTP GET and callAPI() dispatches it through http.Client.Do(), so it cannot perform the /process/exec/connect WebSocket handshake. ProcessAPIService.ExecConnectExecute() should return a bidirectional WebSocket connection instead of *http.Response, using a dedicated dial entrypoint like other Go SDK WebSocket paths.
🤖 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 `@libs/toolbox-api-client-go/api_process.go` around lines 944 - 1007, The
ExecConnectExecute method currently uses the generated REST request flow, which
cannot establish the WebSocket connection. Replace its HTTP response return type
and callAPI-based execution with the SDK’s dedicated WebSocket dial entrypoint,
preserving the existing context, server URL, token, and endpoint path while
returning the bidirectional WebSocket connection and dial error.
Comment on lines
+1727
to
+1738
| /mcp: | ||
| post: | ||
| description: "Model Context Protocol endpoint (streamable-HTTP transport) exposing\ | ||
| \ sandbox tools: exec_command, fs_read_file, fs_write_file, fs_list_files.\ | ||
| \ POST sends JSON-RPC messages (responses are SSE events per the transport);\ | ||
| \ GET opens the SSE stream. Authenticate with a scoped SSH access token (Authorization:\ | ||
| \ Bearer <token>) exactly like /process/exec/connect." | ||
| operationId: MCP | ||
| responses: | ||
| "200": | ||
| content: {} | ||
| description: OK |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant OpenAPI excerpts =="
sed -n '1680,1765p' libs/toolbox-api-client-go/api/openapi.yaml
echo
echo "== WebSocket/connect section =="
rg -n -A 40 -B 10 '/process/exec/connect|Model Context Protocol|JSON-RPC|SSE|mcp' libs/toolbox-api-client-go/api/openapi.yaml
echo
echo "== OpenAPI request/response/requestBody occurrences around mcp/connect =="
python3 - <<'PY'
import re
from pathlib import Path
p=Path('libs/toolbox-api-client-go/api/openapi.yaml')
s=p.read_text()
lines=s.splitlines()
for label in ['/mcp','/process/exec/connect','initialize','json-rpc','SSE','GET']:
print(f'-- label {label!r} --')
for i,l in enumerate(lines,1):
if label.lower() in l.lower() or label in l:
lo=max(1,i-8); hi=min(len(lines),i+10)
print(f'\nline {i}: {l}')
for j in range(lo,hi+1):
print(f'{j}: {lines[j-1]}')
print()
PYRepository: arrrrny/daytona
Length of output: 50372
Define the MCP message and streaming contracts.
POST /mcp is described as accepting JSON-RPC, but has no requestBody; generated clients therefore expose no typed way to send initialize, tool calls, or other MCP messages. It also describes SSE responses inside POST and mentions GET opens the SSE stream without declaring a GET operation. Add typed JSON-RPC request content, SSE response content, and the GET operation if the endpoint supports it.
🤖 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 `@libs/toolbox-api-client-go/api/openapi.yaml` around lines 1727 - 1738, Update
the /mcp OpenAPI path: add a typed requestBody for POST containing the JSON-RPC
message schema, declare its SSE response media type and event payload schema,
and add a GET operation for the SSE stream if supported by the implementation.
Reuse existing JSON-RPC and SSE schema symbols where available, and keep the MCP
operation’s authentication and endpoint behavior consistent.
Comment on lines
+1800
to
+1820
| /process/exec/connect: | ||
| get: | ||
| description: "SSH-equivalent exec channel over HTTPS. After the upgrade the\ | ||
| \ client sends a start frame: {\"type\":\"start\",\"command\":\"...\",\"cwd\"\ | ||
| :\"...\",\"env\":{...},\"cols\":...,\"rows\":...}. When command is omitted,\ | ||
| \ an interactive login shell is started (like bare `ssh host`). Subsequent\ | ||
| \ client frames: stdin, signal, resize, stdin_eof. Server frames: stdout,\ | ||
| \ stderr, exit (always last, before close), error. One connection = one exec;\ | ||
| \ shell state persists for the lifetime of the connection." | ||
| operationId: ExecConnect | ||
| parameters: | ||
| - description: SSH access token (alternative to the Authorization header for | ||
| WS clients that cannot set headers) | ||
| in: query | ||
| name: token | ||
| schema: | ||
| type: string | ||
| responses: | ||
| "101": | ||
| content: {} | ||
| description: Switching Protocols - WebSocket connection established |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file context =="
sed -n '1760,1845p' libs/toolbox-api-client-go/api/openapi.yaml
echo
echo "== all websocket/upgrade references =="
rg -n "101|WebSocket|Sec-WebSocket|Upgrade|exec/exec/connect|ExecConnect|operationId: ExecConnect" libs/toolbox-api-client-go/api/openapi.yaml
echo
echo "== SDK/client generation config files =="
git ls-files | rg '(^|/)(openapi-?-generate\.ya?ml|openapi.*\.ya?ml|generate.*openapi|.*openapi.*generator.*|Makefile|mk\.ya?ml|.*\.sh$)' || trueRepository: arrrrny/daytona
Length of output: 8749
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files mentioning /process/exec/connect or operationId ==="
rg -n "process/exec/connect|ExecConnect|ExecConnect" . || true
echo
echo "== generator config snippets =="
for f in $(git ls-files | rg 'openapi.*config|openapi.*generator|generate.*yaml|generate.*yml|gen.*yaml|Makefile' || true); do
echo "--- $f"
sed -n '1,220p' "$f"
doneRepository: arrrrny/daytona
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== build scripts and invocation =="
for f in hack/go-client/postprocess.sh hack/*.sh libs/toolbox-api-client-go/.openapi-generator/VERSION libs/toolbox-api-client-go/api_process.go; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,220p' "$f"
fi
done
echo
echo "== relevant generated Go operation =="
sed -n '912,972p' libs/toolbox-api-client-go/api_process.go
echo
echo "== relevant generated Python operation =="
sed -n '1848,1915p' libs/toolbox-api-client-python/daytona_toolbox_api_client/api/process_api.py
sed -n '1825,1885p' libs/toolbox-api-client-python-async/daytona_toolbox_api_client_async/api/process_api.py
echo
echo "== relevant generated TypeScript operation =="
sed -n '258,285p' libs/toolbox-api-client/src/api/process-api.ts
echo
echo "== websocket exec docs =="
sed -n '1,110p' apps/docs/src/content/docs/en/ssh-over-https.mdxRepository: arrrrny/daytona
Length of output: 25191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== OpenAPI spec operations that return 101 =="
python3 - <<'PY'
import pathlib, re
p = pathlib.Path('libs/toolbox-api-client-go/api/openapi.yaml')
text = p.read_text()
paths = re.split(r'^\s{0,2}/[^\n]+:\n', text, flags=re.M)
for sec in paths:
m = re.match(r'^(?! )', sec)
if not m: continue
path = m.group(0).rstrip()
for method in ('get', 'post', 'put', 'patch', 'delete'):
if f'\n{method}:' in sec and '\n "101"' in sec:
oid = re.search(r'operationId:\s*(.+)$', sec, re.M)
print(path, method, oid.group(1) if oid else '<no operationId>')
PY
echo
echo "== check whether generated clients expose plain HTTP execute methods =="
python3 - <<'PY'
from pathlib import Path
files = {
'go': 'libs/toolbox-api-client-go/api_process.go',
'py': 'libs/toolbox-api-client-python/daytona_toolbox_api_client/api/process_api.py',
'pysync': 'libs/toolbox-api-client-python.daytona_toolbox_api_client.api.process_api.py',
'ts': 'libs/toolbox-api-client/src/api/process-api.ts',
'java': 'libs/toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/api/ProcessApi.java',
}
for name, path in files.items():
p = Path(path)
if p.exists():
txt = p.read_text()
hits = []
if name == 'go':
for needle in ['ExecConnect(', 'ExecConnectExecute(', 'ExecConnect(url', 'return r.client.Do']:
if needle in txt: hits.append(needle)
elif name in ('py', 'pysync'):
txt = p.read_text()
for needle in ['ExecConnect', 'ExecConnect_execute', 'ApiException', 'None']:
hits = []
if 'ExecConnect' in txt: hits.append('has ExecConnect symbol')
elif name == 'ts':
txt = p.read_text()
for needle in ['execute(): Promise', 'get', 'resource_path']:
if needle in txt: hits.append(needle)
elif name == 'java':
txt = p.read_text()
hits.append('has ExecConnect' if 'Execconnect' in txt or 'exec_Connect' in txt else 'no direct symbol')
print(name, p, ';'.join(hits))
PYRepository: arrrrny/daytona
Length of output: 596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== full generated Go ExecConnectExecute =="
sed -n '937,1045p' libs/toolbox-api-client-go/api_process.go
echo
echo "== full spec /process/exec/connect =="
python3 - <<'PY'
from pathlib import Path
import re
text = Path('libs/toolbox-api-client-go/api/openapi.yaml').read_text()
m = re.search(r'^ (/process/exec/connect):(?:\n.*)+?(?=\n /|\\Z)', text, re.M | re.S)
if m:
print(m.group(0).split('\n', 80)[0])
for line in m.group(0).splitlines()[:120]:
print(line)
else:
print('not found')
PY
echo
echo "== nearby full spec websocket PTY operation =="
sed -n '2030,2075p' libs/toolbox-api-client-go/api/openapi.yamlRepository: arrrrny/daytona
Length of output: 145
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== generated Go method tails =="
wc -l libs/toolbox-api-client-go/api_process.go
sed -n '937,990p' libs/toolbox-api-client-go/api_process.go
sed -n '990,1045p' libs/toolbox-api-client-go/api_process.go || true
echo
echo "== exec connect spec tail =="
sed -n '1845,1860p' libs/toolbox-api-client-go/api/openapi.yaml
echo
echo "== comparable websocket spec tail =="
sed -n '2075,2100p' libs/toolbox-api-client-go/api/openapi.yamlRepository: arrrrny/daytona
Length of output: 4812
Do not expose /process/exec/connect from REST SDKs.
The endpoint is documented as a live bidirectional WebSocket that requires sending a start frame and streaming frames, but the generated REST SDK methods issue a plain GET and return an http.Response/HTTP error. Generate a dedicated WebSocket client/session abstraction for this operation, or omit it from REST-client generation and document the WebSocket URL separately.
🤖 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 `@libs/toolbox-api-client-go/api/openapi.yaml` around lines 1800 - 1820, The
OpenAPI definition for ExecConnect currently causes REST SDKs to generate a
plain GET method for the bidirectional WebSocket endpoint. Exclude ExecConnect
from REST-client generation and document its WebSocket URL separately, or
provide a dedicated WebSocket client/session abstraction that sends the start
frame and supports streaming frames instead of returning an HTTP response.
Daemon: - session: guard nil session.cmd in WriteInput (panic window) - session: loop syscall.Write until all stdin bytes are written (single write(2) can short-write, truncating large frames) - session: make the async stdin holder a single process (exec tail -f /dev/null) so killing the recorded PID drops the FIFO's last writer and CloseInput delivers EOF immediately; previously the sleep child kept stdin open for up to an hour - mcp: fs_read_file opens the file first, rejects non-regular files (e.g. /dev/zero) and reads through LimitReader so growth or special files cannot cause unbounded allocation Proxy: - pass the request context to ValidateSshAccess instead of context.Background() so disconnected clients cancel validation - enforce ensureSandboxStarted for regular Bearer tokens on agent-access paths, matching the SSH-token path Exec WS protocol: - emit error frames on stdin/stdin_eof/signal/resize failures instead of only debug-logging them MCP/swagger contract: - split HandleMCP into per-method handlers so the spec models POST (JSON-RPC body, json+SSE), GET (SSE stream) and DELETE with unique operation IDs; fix malformed ' text/event-stream' media type; regenerate swagger + Go/TS/Java/Python/Ruby clients (MCP clients now accept a message body) Tests: daemon (session/mcp/exec) go test green incl. new /dev/zero and demux-EOF regression tests, nx test api green, alpine/dash smoke for FIFO EOF on holder kill. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/daemon/pkg/session/exec_support.go (1)
120-123: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not acknowledge EOF before the holder exists.
Line 120 returns success when the PID file is absent.
Executeonly queues the async wrapper and immediately returns (apps/daemon/pkg/session/execute.goLines 83-92), so an immediatestdin_eofcan win this race. The holder then starts afterward and keeps stdin open indefinitely. Coordinate holder readiness before accepting EOF, or wait/retry for the PID file and return an error if it never appears.🤖 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/daemon/pkg/session/exec_support.go` around lines 120 - 123, Update the PID-file readiness logic used by Execute so a missing PID file is not treated as successful holder startup. Coordinate readiness before accepting stdin_eof, or retry until the PID file appears and return an error when it does not; ensure the async wrapper cannot leave stdin held indefinitely.
🧹 Nitpick comments (1)
libs/toolbox-api-client-ruby/lib/daytona_toolbox_api_client/api/mcp_api.rb (1)
26-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename MCP operationIds to avoid split method names.
MCPGet,MCPPost, andMCPDeletegeneratem_cp_get,m_cp_post, andm_cp_deletein both Ruby and Python. UseMcpGet,McpPost, andMcpDeletein the daemon@idannotations/swagger docs to getmcp_get,mcp_post, andmcp_delete.🤖 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 `@libs/toolbox-api-client-ruby/lib/daytona_toolbox_api_client/api/mcp_api.rb` around lines 26 - 29, Update the daemon MCP operationId annotations and Swagger documentation from MCPGet, MCPPost, and MCPDelete to McpGet, McpPost, and McpDelete so generated Ruby and Python methods consistently use mcp_get, mcp_post, and mcp_delete instead of m_cp_get, m_cp_post, and m_cp_delete; regenerate the affected clients if applicable.
🤖 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/auth.go`:
- Around line 27-36: Remove the SSH query-token parameter before the successful
regular Bearer-token return in the authentication flow surrounding
ensureSandboxStarted. Ensure requests containing both credentials cannot retain
SSH_ACCESS_TOKEN_QUERY_PARAM for downstream forwarding or logging, while
preserving the existing Authorization-header removal and return behavior.
---
Outside diff comments:
In `@apps/daemon/pkg/session/exec_support.go`:
- Around line 120-123: Update the PID-file readiness logic used by Execute so a
missing PID file is not treated as successful holder startup. Coordinate
readiness before accepting stdin_eof, or retry until the PID file appears and
return an error when it does not; ensure the async wrapper cannot leave stdin
held indefinitely.
---
Nitpick comments:
In `@libs/toolbox-api-client-ruby/lib/daytona_toolbox_api_client/api/mcp_api.rb`:
- Around line 26-29: Update the daemon MCP operationId annotations and Swagger
documentation from MCPGet, MCPPost, and MCPDelete to McpGet, McpPost, and
McpDelete so generated Ruby and Python methods consistently use mcp_get,
mcp_post, and mcp_delete instead of m_cp_get, m_cp_post, and m_cp_delete;
regenerate the affected clients if applicable.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a615b833-8ce7-44ed-b521-822862b5acb9
📒 Files selected for processing (24)
apps/daemon/pkg/session/exec_support.goapps/daemon/pkg/session/execute.goapps/daemon/pkg/toolbox/docs/docs.goapps/daemon/pkg/toolbox/docs/swagger.jsonapps/daemon/pkg/toolbox/docs/swagger.yamlapps/daemon/pkg/toolbox/mcp/server.goapps/daemon/pkg/toolbox/mcp/tools.goapps/daemon/pkg/toolbox/mcp/tools_test.goapps/daemon/pkg/toolbox/process/exec/controller.goapps/daemon/pkg/toolbox/process/exec/demux_test.goapps/daemon/pkg/toolbox/server.goapps/proxy/pkg/proxy/agent_access.goapps/proxy/pkg/proxy/auth.golibs/toolbox-api-client-go/api/openapi.yamllibs/toolbox-api-client-go/api_mcp.golibs/toolbox-api-client-java/src/main/java/io/daytona/toolbox/client/api/McpApi.javalibs/toolbox-api-client-java/src/test/java/io/daytona/toolbox/client/api/McpApiTest.javalibs/toolbox-api-client-python-async/daytona_toolbox_api_client_async/api/mcp_api.pylibs/toolbox-api-client-python/daytona_toolbox_api_client/api/mcp_api.pylibs/toolbox-api-client-ruby/.openapi-generator/FILESlibs/toolbox-api-client-ruby/lib/daytona_toolbox_api_client.rblibs/toolbox-api-client-ruby/lib/daytona_toolbox_api_client/api/mcp_api.rblibs/toolbox-api-client-ruby/lib/daytona_toolbox_api_client/api/process_api.rblibs/toolbox-api-client/src/api/mcp-api.ts
Comment on lines
+27
to
36
| // Agent-access endpoints enforce the same started-state check as | ||
| // the SSH gateway, regardless of which credential was presented. | ||
| if allowSshAccessToken { | ||
| if err := p.ensureSandboxStarted(ctx.Request.Context(), sandboxIdOrSignedToken); err != nil { | ||
| return sandboxIdOrSignedToken, false, err | ||
| } | ||
| } | ||
| // If authentication successful, remove the Authorization header to prevent it from being forwarded to the sandbox | ||
| ctx.Request.Header.Del("Authorization") | ||
| return sandboxIdOrSignedToken, false, nil |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Strip ?token= before the regular Bearer early return.
When a request contains both a valid regular Bearer token and an SSH query token, this branch returns at Line 36 without removing SSH_ACCESS_TOKEN_QUERY_PARAM; the unused credential remains available for downstream forwarding or logging.
Proposed fix
if allowSshAccessToken {
if err := p.ensureSandboxStarted(ctx.Request.Context(), sandboxIdOrSignedToken); err != nil {
return sandboxIdOrSignedToken, false, err
}
+ newQuery := ctx.Request.URL.Query()
+ newQuery.Del(SSH_ACCESS_TOKEN_QUERY_PARAM)
+ ctx.Request.URL.RawQuery = newQuery.Encode()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Agent-access endpoints enforce the same started-state check as | |
| // the SSH gateway, regardless of which credential was presented. | |
| if allowSshAccessToken { | |
| if err := p.ensureSandboxStarted(ctx.Request.Context(), sandboxIdOrSignedToken); err != nil { | |
| return sandboxIdOrSignedToken, false, err | |
| } | |
| } | |
| // If authentication successful, remove the Authorization header to prevent it from being forwarded to the sandbox | |
| ctx.Request.Header.Del("Authorization") | |
| return sandboxIdOrSignedToken, false, nil | |
| // Agent-access endpoints enforce the same started-state check as | |
| // the SSH gateway, regardless of which credential was presented. | |
| if allowSshAccessToken { | |
| if err := p.ensureSandboxStarted(ctx.Request.Context(), sandboxIdOrSignedToken); err != nil { | |
| return sandboxIdOrSignedToken, false, err | |
| } | |
| newQuery := ctx.Request.URL.Query() | |
| newQuery.Del(SSH_ACCESS_TOKEN_QUERY_PARAM) | |
| ctx.Request.URL.RawQuery = newQuery.Encode() | |
| } | |
| // If authentication successful, remove the Authorization header to prevent it from being forwarded to the sandbox | |
| ctx.Request.Header.Del("Authorization") | |
| return sandboxIdOrSignedToken, false, nil |
🤖 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/auth.go` around lines 27 - 36, Remove the SSH
query-token parameter before the successful regular Bearer-token return in the
authentication flow surrounding ensureSandboxStarted. Ensure requests containing
both credentials cannot retain SSH_ACCESS_TOKEN_QUERY_PARAM for downstream
forwarding or logging, while preserving the existing Authorization-header
removal and return behavior.
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.
Closes #2
Daemon (toolbox)
GET /process/exec/connect— WebSocket exec channel with SSH channel semantics. Client framesstart/stdin/signal/resize/stdin_eof; server framesstdout/stderr/exit/error. With acommandit streams output and reports the exit code (SIGINT onsleep 60→130); without one it spawns an interactive login shell (bash -l, fallbacksh) on a PTY with control-character signal delivery (^C/^\\/^Z) and workingresize. Theexitframe is always sent before the connection closes. Built on the existingSessionService+cmdWrapperFormat(no parallel command runner); PTY changes are additive-only (output subscribers, ephemeral sessions,WaitExit).POST /mcp— stateless streamable-HTTP MCP endpoint with toolsexec_command,fs_read_file,fs_write_file,fs_list_files. Adds exactly one new dependency:github.com/modelcontextprotocol/go-sdk.Proxy
/{sandboxId}/process/exec/connectand/{sandboxId}/mcpviaAuthorization: Beareror?token=query (for WS clients that cannot set headers). Tokens are validated per connection via/sandbox/ssh-access/validate— deliberately no caching, so revocation blocks new connections immediately. Non-started sandboxes are rejected with an explicit state message (Sandbox is not started (state: ...). Please start the sandbox before attempting to connect.), matching the SSH gateway. Keepalive piggybacks on the existing last-activity polling (on connect + periodic while open), same as ssh-gateway. Zero behavioral change to existing endpoints.API
validateSshAccessguard widened toOrGuard([SshGatewayAuthContextGuard, ProxyAuthContextGuard])(same pattern as the other proxy-reachable endpoints); auth spec updated accordingly.Generated / docs
swag initregenerated toolbox swagger docs (minimal diff: only the two new routes); toolbox API clients (Go, TS, Java, Python, Python-async) regenerated — newExecConnect/ MCP APIs; API clients regenerated with zero diff.apps/docs/src/content/docs/en/ssh-over-https.mdx) documenting the wire protocol, with sidebar + i18n wiring.Verification
go build ./apps/daemon/... ./apps/proxy/...✅go test ./apps/daemon/...✅ — incl. new unit tests covering the WS frame protocol (start/stdin/signal/exit, SIGINT→130, shell mode, concurrency) and the MCP tool handlers + streamable-HTTP transportgolangci-lint run— 0 issues for daemon and proxy ✅npx nx test api— 54 suites / 562 tests pass ✅api-client-rubyregeneration verified byte-identical (zero drift); itsbundle installvalidation step is unavailable in this environment (no Ruby toolchain) — output is unaffected.Summary by CodeRabbit
/process/exec/connect) and streaming stdout/stderr, stdin/EOF, signals, resize, and exit status./mcpwithGET(SSE),POST(JSON-RPC message), andDELETE(terminate).