feat(ssh): real-SSH access via in-VM sshd + gvproxy port forwarding#541
Closed
nieyy wants to merge 1 commit into
Closed
feat(ssh): real-SSH access via in-VM sshd + gvproxy port forwarding#541nieyy wants to merge 1 commit into
nieyy wants to merge 1 commit into
Conversation
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
2 times, most recently
from
9c6c763 to
74f929f
Compare
nieyy
changed the title
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
from
74f929f to
6504ffd
Compare
law-chain-hot
left a comment
law-chain-hot
left a comment
Contributor
There was a problem hiding this comment.
Left one comment on the OpenSSH download step.
Contributor
Code reviewFound 4 issues:
boxlite/src/guest/scripts/boxlite-disable-ssh Lines 5 to 10 in 11a15a3 Compare with the enable-side branch: boxlite/src/guest/scripts/boxlite-enable-ssh Lines 6 to 12 in 11a15a3
boxlite/apps/runner/pkg/sshgateway/service.go Lines 192 to 201 in 11a15a3
boxlite/apps/runner/pkg/boxlite/client_ssh.go Lines 810 to 836 in 11a15a3
boxlite/apps/ssh-gateway/main.go Lines 258 to 264 in 11a15a3 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
lilongen
suggested changes
May 19, 2026
lilongen
left a comment
lilongen
left a comment
Contributor
There was a problem hiding this comment.
See detailed review: #541 (comment)
Blocking on issue #1 (boxlite-disable-ssh root-home asymmetry) — when the controller's default unix_user=root is used, disable-ssh deletes a non-existent path and the surviving /root/.ssh/.ssh_enabled marker causes boxlite-ensure-ssh to silently re-enable sshd on the next VM restart, defeating the API-level revoke. This is a security-relevant revoke bypass on the default code path.
Issues #2 (sshUserOrDefault comment/impl mismatch), #3 (boxSSHMu unbounded growth — CLAUDE.md "No unbounded queues/concurrency/memory"), and #4 (raw SSH bearer token logged in cleartext — CLAUDE.md "Mask secrets in errors and logs") should also be addressed before merge.
🤖 Generated with Claude Code
DorianZheng
reviewed
May 20, 2026
Member
|
Why does the runner compute the gvproxy admin socket path itself?
filepath.Join(homeDir, "boxes", boxId, "sockets", "gvproxy-admin.sock")This makes |
Member
|
Local builds silently skip sshd injection, so real-SSH can't be exercised end-to-end on dev machines. The static
Each layer fails open, so the gap doesn't surface until someone enables SSH on a locally-built sandbox and gets |
Member
|
should update |
Author
Fixed all 4, thanks for the thorough review: boxlite-disable-ssh root-home asymmetry — added the root branch so disable targets /root/.ssh/ consistently with enable. Revoke now works on the default code path. |
Author
Good suggestion — added make sshd that calls scripts/build/build-static-sshd.sh (requires Docker). CI now uses the same script so the build logic isn't duplicated. Devs can run make sshd to get a fully SSH-capable guest locally. Added make sshd target that builds static boxlite-sshd / boxlite-ssh-keygen via Alpine Docker (same as CI). Tested on the dev machine — output is statically linked OpenSSH 9.7p1. Requires Docker Desktop locally. |
Author
Thanks — added the SSH endpoints ( |
Author
Added
One related note for the future: if a VM is ever migrated to a different runner machine, the gvproxy port forward will be lost along with the socket. |
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
2 times, most recently
from
eaf0ff8 to
5e40c87
Compare
Member
I think it's too broad to expose the admin socket. The user will not know how to use it. A set of abstraction APIs is required |
Author
Moved |
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
4 times, most recently
from
18b493d to
2e8bc74
Compare
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
2 times, most recently
from
b926046 to
7197edc
Compare
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
from
7197edc to
095df7c
Compare
|
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:
📝 WalkthroughWalkthroughThis PR introduces real-SSH access for runner sandboxes end-to-end: API/DTO/migration contracts for a nullable ChangesReal SSH Access Feature
Audit target-id string cast cleanup
Workspace and Jest config housekeeping
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant RunnerAPI
participant BoxliteClient
participant SSHPortAllocator
participant Gvproxy
participant GuestSSHD
RunnerAPI->>BoxliteClient: EnableSSHAccess(boxId, gatewayKey, unixUser)
BoxliteClient->>SSHPortAllocator: Allocate(boxId)
BoxliteClient->>Gvproxy: addGvproxyPortForward(hostPort)
BoxliteClient->>GuestSSHD: EnableSSH(authorizedKeys, unixUser)
GuestSSHD-->>BoxliteClient: sshd started
BoxliteClient-->>RunnerAPI: hostPort, unixUser, enabled=true
sequenceDiagram
participant Client
participant SSHGateway
participant RunnerAPI
participant RealSSHD
participant ExecBridge
Client->>SSHGateway: SSH connect with token
SSHGateway->>SSHGateway: validate token, extract unix_user
SSHGateway->>RunnerAPI: getRunnerSSHAccess(sandboxId)
RunnerAPI-->>SSHGateway: enabled, degraded, hostPort, unixUser
alt real-SSH available and matches
SSHGateway->>RealSSHD: connectToRunner(unixUser, hostPort)
else fail-closed or mismatch
SSHGateway->>ExecBridge: connectToRunner(execUser, execPort)
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
apps/api/src/sandbox/dto/ssh-access.dto.ts (1)
122-140: 💤 Low valueDocument field precedence for dual naming convention.
The DTO accepts both
unixUser(camelCase) andunix_user(snake_case), which creates potential ambiguity. The controller resolves this withbody?.unixUser?.trim() || body?.unix_user?.trim(), meaning camelCase wins if both are provided.Consider documenting this precedence in the
@ApiPropertydescriptions to clarify the resolution order for API consumers.📝 Suggested documentation improvement
`@ApiProperty`({ - description: 'Unix user for SSH access (camelCase form)', + description: 'Unix user for SSH access (camelCase form; takes precedence over unix_user if both provided)', example: 'boxlite', required: false, }) unixUser?: string `@ApiProperty`({ - description: 'Unix user for SSH access (snake_case wire form)', + description: 'Unix user for SSH access (snake_case form; for backward compatibility, use unixUser if possible)', example: 'boxlite', required: false, }) // eslint-disable-next-line `@typescript-eslint/naming-convention` unix_user?: string🤖 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/sandbox/dto/ssh-access.dto.ts` around lines 122 - 140, The ApiProperty descriptions for CreateSshAccessBodyDto's unixUser and unix_user are ambiguous about which is used when both are provided; update the description strings for both properties to state the precedence (camelCase unixUser takes precedence over snake_case unix_user) and optionally reference the controller resolution used (e.g., body?.unixUser?.trim() || body?.unix_user?.trim()) so API consumers know camelCase wins; modify the descriptions on the unixUser and unix_user decorators accordingly.sdks/c/src/box_handle.rs (1)
104-131: 💤 Low valueRedundant unsafe block.
The function is already declared as
unsafe extern "C", so the innerunsafeblock at line 112 is redundant. You can remove it and access the raw pointer directly in the function body.♻️ Proposed simplification
#[unsafe(no_mangle)] pub unsafe extern "C" fn boxlite_box_admin_sock_path(handle: *mut CBoxHandle) -> *mut c_char { - unsafe { - if handle.is_null() { - return ptr::null_mut(); - } - let handle_ref = &*handle; - if handle_ref.home_dir.as_os_str().is_empty() { - return ptr::null_mut(); - } - let path = handle_ref - .home_dir - .join("boxes") - .join(handle_ref.box_id.as_str()) - .join("sockets") - .join("gvproxy-admin.sock"); - match std::ffi::CString::new(path.to_string_lossy().as_ref()) { - Ok(s) => s.into_raw(), - Err(_) => ptr::null_mut(), - } + if handle.is_null() { + return ptr::null_mut(); + } + let handle_ref = &*handle; + if handle_ref.home_dir.as_os_str().is_empty() { + return ptr::null_mut(); + } + let path = handle_ref + .home_dir + .join("boxes") + .join(handle_ref.box_id.as_str()) + .join("sockets") + .join("gvproxy-admin.sock"); + match std::ffi::CString::new(path.to_string_lossy().as_ref()) { + Ok(s) => s.into_raw(), + Err(_) => ptr::null_mut(), } }🤖 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 `@sdks/c/src/box_handle.rs` around lines 104 - 131, The inner redundant "unsafe" block inside the exported function boxlite_box_admin_sock_path should be removed since the function is already declared unsafe extern "C"; simply use handle, dereference to &*handle (CBoxHandle) and perform the same null checks and path construction (using handle_ref.home_dir, handle_ref.box_id) directly in the function body, preserving the CString::new(...) match and returning s.into_raw() or ptr::null_mut() as before.apps/runner/pkg/api/controllers/ssh_access_test.go (1)
36-54: 💤 Low valueTest 2 validates the function signature, not runtime behavior.
The
callerKeysvariable is declared but never passed togatewayOnlyKeysbecause the function's signature deliberately doesn't accept caller keys. The test documents the security invariant but the assertion at lines 47-53 is redundant — ifgatewayOnlyKeysonly takesgwKeyand returns[]string{gwKey}, caller keys can never appear in the result by construction.Consider simplifying to just verify the return value equals
[]string{gwKey}, which already proves caller keys cannot be injected.🤖 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/ssh_access_test.go` around lines 36 - 54, TestGatewayOnlyKeysDoesNotIncludeCallerKey currently declares callerKeys but never passes them to gatewayOnlyKeys, so the loop asserting they aren't present is redundant; change the test to simply assert that gatewayOnlyKeys(gwKey) returns exactly []string{gwKey} (i.e., compare length and element or use reflect.DeepEqual) and remove the unused callerKeys variable and the nested loop, keeping the test focused on the gatewayOnlyKeys function's return value.
🤖 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/migrations/1778751201300-migration.ts`:
- Around line 18-19: Update the comment that describes NULL semantics for the
unixUser column in the migration to reflect the current legacy-token behavior
(not "'boxlite'"): find the comment adjacent to the unixUser definition in the
migration (symbol: unixUser) and replace the misleading text so it clearly
states that existing NULL rows represent the legacy-token/previous behavior for
backward compatibility; keep note that NULL has no default and preserves
existing rows as legacy-token semantics for application logic and operational
clarity.
In `@apps/api/src/sandbox/common/redis-lock.provider.ts`:
- Around line 56-62: The waitForLockOwned loop currently can hang indefinitely;
update the waitForLockOwned method to accept an optional timeoutMs (or use a
configurable default), track elapsed time while polling lock(key, ttl, code) at
the current interval, and throw a clear error (e.g., LockAcquisitionTimeout) if
elapsed time exceeds timeoutMs; ensure you still return the LockCode on success,
keep the existing sleep interval (or make it configurable), and update any
callers of waitForLockOwned to pass the timeout or rely on the new default.
In `@apps/api/src/sandbox/services/sandbox.service.ts`:
- Around line 2317-2341: If revokedIsRealSsh is true but
runnerService.findOne(sandbox.runnerId) returns null, change the flow to "fail
closed" by aborting and throwing an error before any sshAccessRepository.delete
calls; specifically, inside the block handling remainingRealSsh === 0 and
redisLockProvider.isLockOwned(...), after calling runnerService.findOne(...)
check if runner is null and if revokedIsRealSsh throw or propagate an error (so
the DB token deletion is skipped), otherwise continue with adapter creation and
disableSSHAccess as currently implemented.
In `@apps/libs/api-client/src/api/sandbox-api.ts`:
- Around line 1824-1825: The public API createSshAccess now takes
createSshAccessBodyDto before options which breaks callers passing options as
the 4th arg; change the parameter handling to detect whether the 4th argument is
an Axios options object (implement a looksLikeAxiosOptions type-guard checking
for headers/timeout/params/baseURL) and then set body = undefined/requestOptions
= createSshAccessBodyDtoOrOptions or body =
createSshAccessBodyDtoOrOptions/requestOptions = options accordingly, then pass
the derived body and requestOptions into
localVarAxiosParamCreator.createSshAccess; apply the same backward-compatible
detection/fix to the other occurrences of this pattern in the file for the
related createSshAccess overloads.
In `@apps/runner/cmd/runner/config/config.go`:
- Around line 68-70: Validate SSH port settings immediately after environment
parsing: check SSHPortBase is within valid TCP port range (e.g., 1024–65535),
SSHPortPoolSize is >0, and that SSHPortBase+SSHPortPoolSize-1 does not exceed
65535. Perform these checks in the config parsing/constructor (where env is
decoded, e.g., the function that builds the config struct) and return a
descriptive error if any validation fails so the process fails fast; reference
the SSHPortBase and SSHPortPoolSize fields when adding the validation and error
messages.
In `@apps/runner/README.md`:
- Around line 472-475: Update the README text that currently states "Sessions
always run as the `boxlite` unix user" to reflect the implemented default SSH
user `root`; locate the sentence mentioning the SSH gateway and the `boxlite`
account (the phrase "Sessions always run as the `boxlite` unix user — the
unprivileged account...") and replace or reword it to state that real-SSH
sessions default to `root`, and keep the note that images without a `boxlite`
account remain unsupported via the SSH gateway and recommend the WebSocket
terminal or SDK exec API as alternatives.
In `@apps/ssh-gateway/main_test.go`:
- Around line 169-174: The test mutates and reads execBridgeCalled from multiple
goroutines (set in the PublicKeyCallback and read in the test), causing data
races; replace the plain bool with a concurrency-safe mechanism (e.g., an int32
using sync/atomic or protect with a sync.Mutex) and update all occurrences where
execBridgeCalled is set and checked (including the other instances noted around
lines referencing PublicKeyCallback and execBridgeCalled) to use
atomic.StoreInt32/atomic.LoadInt32 or lock/unlock so the flag is updated and
observed safely from server goroutines and the test goroutine.
- Around line 730-779: The test
TestSSHAccessTokenRejectedWhenRunnerAPITokenEmpty currently simulates routing
with local booleans; replace those asserts with behavior tests that exercise the
real routing by invoking SSHGateway.handleChannel (or the exported routing entry
used by tests) using a crafted channel/session carrying an SSH-access token and
the test signer (newTestSigner) and verifying the observed outcome (reject vs.
exec bridge) against g.runnerAPIToken == "" cases; locate the SSHGateway struct
and handleChannel method and update this test (and the similar blocks around
lines 809–865 and 867–928) to create a mock channel/connection, inject
tokenIsSSHAccess vs legacy token payloads, call the real routing function, and
assert on the actual path taken instead of local boolean math.
In `@openapi/box.openapi.yaml`:
- Around line 2140-2206: The YAML has an orphaned description under
CreateSshAccessRequest.unix_user: remove the stray `description: Opaque token
for next page` entry so the unix_user property only has its intended
nullable/description/example fields; locate the CreateSshAccessRequest schema
and delete that misplaced description line (it does not belong to `unix_user`).
In `@scripts/build/build-static-sshd.sh`:
- Around line 12-15: Add pre-flight validation at the top of the
build-static-sshd.sh script: verify Docker is available (e.g., check command -v
docker or docker --version) and exit with a clear error if not, and validate the
OUTPUT directory (the OUTPUT variable and the target created by mkdir -p
"$OUTPUT") is writable before proceeding (check directory exists and is writable
or that parent is writable), failing fast with descriptive messages if either
check fails.
In `@src/boxlite/src/rootfs/guest.rs`:
- Around line 469-474: The rootfs cache key currently omits SSH artifacts so
updates to boxlite-sshd/ssh-keygen/SSH scripts can be skipped; modify the cache
key generation to include an ssh_assets_hash() that deterministically
incorporates the contents (or a stable placeholder when missing) of binaries
discovered by util::find_binary("boxlite-sshd") and related SSH scripts/keys,
and use that hash when constructing the guest/rootfs version key (update the
code paths around the match util::find_binary(...) and the cache-key
construction spanning the blocks referenced 469 and 487-531 to include
ssh_assets_hash()); ensure ssh_assets_hash() returns deterministic values in
local-dev environments by inserting explicit placeholders for missing binaries.
In `@src/deps/libgvproxy-sys/gvproxy-bridge/main.go`:
- Around line 466-489: The admin UNIX socket listener failure is currently only
logged (adminErr from net.Listen on adminSockPath) which lets gvproxy_create
succeed with a broken SSH-forwarding control plane; change gvproxy_create (or
the function that sets up adminSockPath/net.Listen) to treat adminErr as a fatal
startup error: when net.Listen returns a non-nil adminErr, propagate/return that
error (or wrap it with context) instead of continuing, and ensure any prior
cleanup (removed stale socket) is left intact; keep the existing
adminServer/Serve and ctx shutdown logic only in the successful branch where
adminListener is non-nil.
---
Nitpick comments:
In `@apps/api/src/sandbox/dto/ssh-access.dto.ts`:
- Around line 122-140: The ApiProperty descriptions for CreateSshAccessBodyDto's
unixUser and unix_user are ambiguous about which is used when both are provided;
update the description strings for both properties to state the precedence
(camelCase unixUser takes precedence over snake_case unix_user) and optionally
reference the controller resolution used (e.g., body?.unixUser?.trim() ||
body?.unix_user?.trim()) so API consumers know camelCase wins; modify the
descriptions on the unixUser and unix_user decorators accordingly.
In `@apps/runner/pkg/api/controllers/ssh_access_test.go`:
- Around line 36-54: TestGatewayOnlyKeysDoesNotIncludeCallerKey currently
declares callerKeys but never passes them to gatewayOnlyKeys, so the loop
asserting they aren't present is redundant; change the test to simply assert
that gatewayOnlyKeys(gwKey) returns exactly []string{gwKey} (i.e., compare
length and element or use reflect.DeepEqual) and remove the unused callerKeys
variable and the nested loop, keeping the test focused on the gatewayOnlyKeys
function's return value.
In `@sdks/c/src/box_handle.rs`:
- Around line 104-131: The inner redundant "unsafe" block inside the exported
function boxlite_box_admin_sock_path should be removed since the function is
already declared unsafe extern "C"; simply use handle, dereference to &*handle
(CBoxHandle) and perform the same null checks and path construction (using
handle_ref.home_dir, handle_ref.box_id) directly in the function body,
preserving the CString::new(...) match and returning s.into_raw() or
ptr::null_mut() as before.
🪄 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: 708c37cf-9044-491f-a98c-25e3229f885a
⛔ Files ignored due to path filters (1)
apps/ssh-gateway/go.sumis excluded by!**/*.sum
📒 Files selected for processing (90)
.github/workflows/build-runtime.yml.gitignoreapps/api-client-go/model_ssh_access_validation_dto.goapps/api-client-go/model_ssh_access_validation_dto_test.goapps/api/jest.config.tsapps/api/src/admin/controllers/runner.controller.tsapps/api/src/admin/controllers/sandbox.controller.tsapps/api/src/api-key/api-key.controller.tsapps/api/src/boxlite-rest/boxlite-box.controller.tsapps/api/src/docker-registry/controllers/docker-registry.controller.tsapps/api/src/interceptors/metrics.interceptor.tsapps/api/src/migrations/1778751201300-migration.tsapps/api/src/organization/controllers/organization-invitation.controller.tsapps/api/src/organization/controllers/organization-region.controller.tsapps/api/src/organization/controllers/organization-role.controller.tsapps/api/src/organization/controllers/organization-user.controller.tsapps/api/src/organization/controllers/organization.controller.tsapps/api/src/organization/guards/organization-access.guard.tsapps/api/src/sandbox/common/redis-lock.provider.tsapps/api/src/sandbox/controllers/runner.controller.tsapps/api/src/sandbox/controllers/sandbox.controller.tsapps/api/src/sandbox/controllers/snapshot.controller.tsapps/api/src/sandbox/controllers/toolbox.deprecated.controller.tsapps/api/src/sandbox/controllers/volume.controller.tsapps/api/src/sandbox/controllers/workspace.deprecated.controller.tsapps/api/src/sandbox/dto/ssh-access.dto.tsapps/api/src/sandbox/entities/ssh-access.entity.tsapps/api/src/sandbox/runner-adapter/runnerAdapter.tsapps/api/src/sandbox/runner-adapter/runnerAdapter.v0.tsapps/api/src/sandbox/runner-adapter/runnerAdapter.v2.tsapps/api/src/sandbox/services/sandbox-ssh-access.spec.tsapps/api/src/sandbox/services/sandbox.service.tsapps/api/src/user/user.controller.tsapps/api/src/webhook/controllers/webhook.controller.tsapps/api/tsconfig.spec.jsonapps/apps/tsconfig.base.jsonapps/dashboard/src/components/ErrorBoundaryFallback.tsxapps/dashboard/src/hooks/mutations/useCreateSandboxMutation.tsxapps/dashboard/src/hooks/mutations/useCreateSshAccessMutation.tsapps/dashboard/src/hooks/useSandboxSession.tsapps/dashboard/src/pages/Sandboxes.tsxapps/infra/sst.config.tsapps/libs/api-client/src/api/sandbox-api.tsapps/package.jsonapps/runner/README.mdapps/runner/cmd/runner/config/config.goapps/runner/cmd/runner/main.goapps/runner/pkg/api/controllers/proxy.goapps/runner/pkg/api/controllers/ssh_access.goapps/runner/pkg/api/controllers/ssh_access_test.goapps/runner/pkg/api/server.goapps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/client_ssh.goapps/runner/pkg/boxlite/client_ssh_test.goapps/runner/pkg/boxlite/exec_manager.goapps/runner/pkg/runner/runner.goapps/runner/pkg/sshgateway/service.goapps/runner/pkg/sshgateway/ssh_integration_test.goapps/runner/pkg/sshgateway/stdin_cancel_test.goapps/runner/pkg/sshport/allocator.goapps/runner/pkg/sshport/allocator_test.goapps/ssh-gateway/main.goapps/ssh-gateway/main_test.gojest.preset.jsmake/build.mkmake/test.mkopenapi/box.openapi.yamlscripts/build/build-static-sshd.shscripts/deploy/runner-update-binary.shsdks/c/include/boxlite.hsdks/c/include/boxlite_internal.hsdks/c/src/box_handle.rssdks/c/src/event_queue.rssdks/c/src/rest.rssdks/c/src/runtime.rssdks/c/src/tests.rssdks/go/box_handle.gosdks/go/boxlite_internal.hsdks/go/bridge.hsdks/go/exec.gosrc/boxlite/build.rssrc/boxlite/src/net/gvproxy/config.rssrc/boxlite/src/rootfs/guest.rssrc/boxlite/src/runtime/layout.rssrc/deps/libgvproxy-sys/gvproxy-bridge/main.gosrc/guest/scripts/boxlite-disable-sshsrc/guest/scripts/boxlite-enable-sshsrc/guest/scripts/boxlite-ensure-sshsrc/guest/src/container/spec.rstsconfig.base.json
G4614
pushed a commit
to G4614/boxlite
that referenced
this pull request
Jun 15, 2026
## Summary - Remove the stale `box_public_request: request.public` create-box metric after boxlite-ai#762 moved metrics to the REST create DTO. - Replace the Tailwind v4-only toggle group gap class with valid Tailwind 3/CSS-var output. - Make `dashboard:build` depend on `api-client:build` so clean Docker builds have `dist/libs/api-client` before dashboard typecheck. ## Root cause `sst deploy --stage dev` failed while building the API image. The first blocker was `api:build:production` typechecking `request.public` against the REST `CreateBoxDto`, which intentionally has no `public` field. After that was fixed, the same clean Docker image path exposed two dashboard production blockers: invalid generated CSS from `gap-[--spacing(var(--gap))]`, and a missing `api-client` build dependency. ## Validation - `git diff --check` - Remote dev machine: `cd apps && NX_DAEMON=false yarn nx build api --configuration=production --nxBail=true --output-style=stream` - Remote dev machine: `docker build -f apps/api/Dockerfile --target boxlite .` ## Notes I checked current open PRs before opening this. boxlite-ai#719 and boxlite-ai#541 both touch `metrics.interceptor.ts`, but neither contains this stale `request.public` fix; boxlite-ai#719 is also a broad draft for older API production webpack issues. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Style** * Enhanced toggle group component spacing behavior for improved visual consistency * **Chores** * Updated build dependency configuration for dashboard production builds * Refined analytics metric collection payload structure <!-- end of auto-generated comment: release notes by coderabbit.ai -->
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
from
095df7c to
df13edd
Compare
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
from
df13edd to
6fb3f75
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/api/src/box/services/box.service.ts (1)
1665-1669: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated SSH gateway URL resolution.
This region-lookup + config fallback block is duplicated verbatim at Lines 1616-1620 (second fencing return). Two copies must stay in sync; extract a small private helper (e.g.
resolveSshGatewayUrl(box)) and reuse it in both return sites.♻️ Suggested helper
private async resolveSshGatewayUrl(box: Box): Promise<string> { const region = await this.regionService.findOne(box.region, true) return region?.sshGatewayUrl ?? this.configService.getOrThrow('sshGateway.url') }Then both return sites become:
return SshAccessDto.fromSshAccess(sshAccess, await this.resolveSshGatewayUrl(box))🤖 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 1665 - 1669, Extract the duplicated SSH gateway URL lookup logic into a small private helper on BoxService, such as resolveSshGatewayUrl(box), so both fencing return sites share the same regionService.findOne plus configService fallback behavior. Have the helper return region?.sshGatewayUrl ?? this.configService.getOrThrow('sshGateway.url'), and update the two SshAccessDto.fromSshAccess call sites to await the helper instead of repeating the inline block.apps/api/src/box/runner-adapter/runnerAdapter.v2.ts (1)
250-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDiscards backend error detail on failure.
Non-2xx responses are converted to a bare
HTTP {status}message, dropping the runner's{"error": "..."}payload (e.g. specific validation or state errors fromEnableSSHAccess). This makes failures harder to diagnose from the API layer.♻️ Suggested fix: surface backend error message
if (response.status < 200 || response.status >= 300) { + const detail = typeof response.data === 'object' && response.data?.error ? response.data.error : `HTTP ${response.status}` throw new Error( - `enableSSHAccess failed for sandbox ${sandboxId} on runner ${this.runner.id}: HTTP ${response.status}`, + `enableSSHAccess failed for sandbox ${sandboxId} on runner ${this.runner.id}: ${detail}`, ) }🤖 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/runner-adapter/runnerAdapter.v2.ts` around lines 250 - 270, The enableSSHAccess failure path in runnerAdapter.v2.ts is dropping the backend’s error payload and only reporting the HTTP status. Update the enableSSHAccess method to inspect the non-2xx axios response body and include the returned error message (for example the runner’s error field) in the thrown error, while keeping the existing 503 special-case handling intact. Use the enableSSHAccess and axiosInstance request logic to locate the change, and make the generic failure branch preserve backend detail instead of emitting only HTTP {status}.
🤖 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/runner-adapter/runnerAdapter.v2.ts`:
- Around line 32-34: RunnerAdapterV2’s direct runner HTTP client is created
without retry support, so transient failures in enableSSHAccess and
disableSSHAccess are not retried. Update the axiosInstance setup in
RunnerAdapterV2 to mirror the V0 retry configuration by applying axiosRetry when
the instance is created for runner.apiUrl, and ensure the direct SSH runner
calls continue to use that retried client.
In `@apps/infra/sst.config.ts`:
- Line 720: The SSH gateway is using the default runner token instead of the
per-runner token, so boxes on extra runners can fail authentication. Update the
token wiring around RUNNER_API_TOKEN in the infra config and the
getRunnerSSHAccess call path so it uses the same BOXLITE_RUNNER_TOKEN value
assigned to each runner (rather than defaultRunnerApiKey.result), keeping the
runner-specific token aligned end to end.
---
Nitpick comments:
In `@apps/api/src/box/runner-adapter/runnerAdapter.v2.ts`:
- Around line 250-270: The enableSSHAccess failure path in runnerAdapter.v2.ts
is dropping the backend’s error payload and only reporting the HTTP status.
Update the enableSSHAccess method to inspect the non-2xx axios response body and
include the returned error message (for example the runner’s error field) in the
thrown error, while keeping the existing 503 special-case handling intact. Use
the enableSSHAccess and axiosInstance request logic to locate the change, and
make the generic failure branch preserve backend detail instead of emitting only
HTTP {status}.
In `@apps/api/src/box/services/box.service.ts`:
- Around line 1665-1669: Extract the duplicated SSH gateway URL lookup logic
into a small private helper on BoxService, such as resolveSshGatewayUrl(box), so
both fencing return sites share the same regionService.findOne plus
configService fallback behavior. Have the helper return region?.sshGatewayUrl ??
this.configService.getOrThrow('sshGateway.url'), and update the two
SshAccessDto.fromSshAccess call sites to await the helper instead of repeating
the inline block.
🪄 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: e003c7ca-8127-4ad9-830c-0fd98d4e58c5
⛔ Files ignored due to path filters (1)
apps/ssh-gateway/go.sumis excluded by!**/*.sum
📒 Files selected for processing (79)
.github/workflows/build-runtime.yml.gitignoreapps/api-client-go/model_ssh_access_validation_dto.goapps/api-client-go/model_ssh_access_validation_dto_test.goapps/api/jest.config.tsapps/api/src/admin/controllers/box.controller.tsapps/api/src/admin/controllers/runner.controller.tsapps/api/src/api-key/api-key.controller.tsapps/api/src/box/common/redis-lock.provider.tsapps/api/src/box/controllers/box.controller.tsapps/api/src/box/controllers/runner.controller.tsapps/api/src/box/controllers/volume.controller.tsapps/api/src/box/dto/ssh-access.dto.tsapps/api/src/box/entities/ssh-access.entity.tsapps/api/src/box/runner-adapter/runnerAdapter.tsapps/api/src/box/runner-adapter/runnerAdapter.v0.tsapps/api/src/box/runner-adapter/runnerAdapter.v2.tsapps/api/src/box/services/box.service.tsapps/api/src/box/services/sandbox-ssh-access.spec.tsapps/api/src/boxlite-rest/boxlite-box.controller.tsapps/api/src/migrations/1778751201300-migration.tsapps/api/src/organization/controllers/organization-invitation.controller.tsapps/api/src/organization/controllers/organization-region.controller.tsapps/api/src/organization/controllers/organization-role.controller.tsapps/api/src/organization/controllers/organization-user.controller.tsapps/api/src/organization/controllers/organization.controller.tsapps/api/src/organization/guards/organization-access.guard.tsapps/api/src/user/user.controller.tsapps/api/src/webhook/controllers/webhook.controller.tsapps/api/tsconfig.spec.jsonapps/apps/tsconfig.base.jsonapps/dashboard/src/hooks/mutations/useCreateSshAccessMutation.tsapps/infra/sst.config.tsapps/libs/api-client/src/api/box-api.tsapps/runner/README.mdapps/runner/cmd/runner/config/config.goapps/runner/cmd/runner/main.goapps/runner/pkg/api/controllers/proxy.goapps/runner/pkg/api/controllers/ssh_access.goapps/runner/pkg/api/controllers/ssh_access_test.goapps/runner/pkg/api/server.goapps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/client_ssh.goapps/runner/pkg/boxlite/client_ssh_test.goapps/runner/pkg/boxlite/exec_manager.goapps/runner/pkg/runner/runner.goapps/runner/pkg/sshgateway/service.goapps/runner/pkg/sshgateway/ssh_integration_test.goapps/runner/pkg/sshgateway/stdin_cancel_test.goapps/runner/pkg/sshport/allocator.goapps/runner/pkg/sshport/allocator_test.goapps/ssh-gateway/main.goapps/ssh-gateway/main_test.gojest.preset.jsmake/build.mkmake/test.mkopenapi/box.openapi.yamlscripts/build/build-static-sshd.shscripts/deploy/runner-update-binary.shsdks/c/include/boxlite.hsdks/c/include/boxlite_internal.hsdks/c/src/box_handle.rssdks/c/src/event_queue.rssdks/c/src/rest.rssdks/c/src/runtime.rssdks/c/src/tests.rssdks/go/box_handle.gosdks/go/boxlite_internal.hsdks/go/bridge.hsdks/go/exec.gosrc/boxlite/build.rssrc/boxlite/src/net/gvproxy/config.rssrc/boxlite/src/rootfs/guest.rssrc/boxlite/src/runtime/layout.rssrc/deps/libgvproxy-sys/gvproxy-bridge/main.gosrc/guest/scripts/boxlite-disable-sshsrc/guest/scripts/boxlite-enable-sshsrc/guest/scripts/boxlite-ensure-sshsrc/guest/src/container/spec.rs
✅ Files skipped from review due to trivial changes (13)
- apps/api/src/box/controllers/volume.controller.ts
- sdks/c/src/tests.rs
- jest.preset.js
- apps/api/src/admin/controllers/runner.controller.ts
- apps/api/src/admin/controllers/box.controller.ts
- apps/api/src/box/controllers/runner.controller.ts
- apps/api/src/organization/controllers/organization-role.controller.ts
- .gitignore
- apps/apps/tsconfig.base.json
- apps/api/src/organization/controllers/organization-user.controller.ts
- apps/api/src/user/user.controller.ts
- sdks/c/include/boxlite_internal.h
- sdks/c/src/event_queue.rs
🚧 Files skipped from review as they are similar to previous changes (47)
- sdks/go/bridge.h
- .github/workflows/build-runtime.yml
- sdks/c/src/rest.rs
- apps/api/src/migrations/1778751201300-migration.ts
- apps/runner/pkg/api/controllers/proxy.go
- apps/dashboard/src/hooks/mutations/useCreateSshAccessMutation.ts
- sdks/go/boxlite_internal.h
- apps/runner/pkg/boxlite/exec_manager.go
- apps/runner/pkg/sshport/allocator_test.go
- src/guest/src/container/spec.rs
- sdks/c/src/runtime.rs
- src/guest/scripts/boxlite-ensure-ssh
- make/test.mk
- src/boxlite/src/net/gvproxy/config.rs
- src/boxlite/src/rootfs/guest.rs
- sdks/c/include/boxlite.h
- scripts/build/build-static-sshd.sh
- apps/api/src/boxlite-rest/boxlite-box.controller.ts
- apps/api/src/organization/guards/organization-access.guard.ts
- make/build.mk
- apps/runner/cmd/runner/main.go
- apps/api/src/api-key/api-key.controller.ts
- apps/api/src/organization/controllers/organization-region.controller.ts
- src/boxlite/build.rs
- apps/api-client-go/model_ssh_access_validation_dto_test.go
- src/guest/scripts/boxlite-disable-ssh
- apps/api/tsconfig.spec.json
- apps/api-client-go/model_ssh_access_validation_dto.go
- scripts/deploy/runner-update-binary.sh
- apps/runner/README.md
- apps/runner/pkg/sshport/allocator.go
- apps/api/src/organization/controllers/organization-invitation.controller.ts
- src/boxlite/src/runtime/layout.rs
- openapi/box.openapi.yaml
- apps/runner/pkg/api/controllers/ssh_access_test.go
- src/guest/scripts/boxlite-enable-ssh
- apps/runner/cmd/runner/config/config.go
- sdks/c/src/box_handle.rs
- apps/runner/pkg/api/controllers/ssh_access.go
- apps/api/src/webhook/controllers/webhook.controller.ts
- sdks/go/box_handle.go
- sdks/go/exec.go
- apps/runner/pkg/runner/runner.go
- apps/runner/pkg/boxlite/client_ssh.go
- apps/runner/pkg/boxlite/client.go
- apps/ssh-gateway/main.go
- apps/runner/pkg/sshgateway/stdin_cancel_test.go
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
from
6fb3f75 to
9c765c0
Compare
nieyy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/src/box/common/redis-lock.provider.ts (1)
65-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
as anycast foreval; usedefineCommandor typedeval.ioredis already exposes
evalon the typedRedisinstance, and for reusable Lua scripts it providesdefineCommand, which caches the script and auto-selectsEVALSHA. Theas anycast here is unnecessary and suppresses type-checking on the call arguments.♻️ Proposed refactor using `defineCommand`
+ constructor(`@InjectRedis`() private readonly redis: Redis) { + this.redis.defineCommand('unlockOwned', { + numberOfKeys: 1, + lua: ` + if redis.call('get',KEYS[1]) == ARGV[1] then + return redis.call('del',KEYS[1]) + else + return 0 + end`, + }) + } ... async unlockOwned(key: string, code: LockCode): Promise<void> { - const script = ` - if redis.call('get',KEYS[1]) == ARGV[1] then - return redis.call('del',KEYS[1]) - else - return 0 - end` - await (this.redis as any).eval(script, 1, key, code.getCode()) + await (this.redis as unknown as { unlockOwned(key: string, code: string): Promise<number> }).unlockOwned(key, code.getCode()) }🤖 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/common/redis-lock.provider.ts` around lines 65 - 75, The unlockOwned method in RedisLockProvider uses an unnecessary as any cast around this.redis.eval, which bypasses type checking. Replace the cast by calling the typed ioredis API directly, or refactor the Lua script into a reusable defineCommand on RedisLockProvider so the command is strongly typed and cached; keep the compare-and-delete behavior intact while removing the unsafe cast.apps/infra/sst.config.ts (1)
1027-1046: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHarden the private-key temp file path.
deriveGatewayPublicKeydecodesSSH_PRIVATE_KEY_B64into a predictable path (/tmp/sst-ssh-gw-${process.pid}).writeFileSyncfollows an existing symlink at that path, so on a shared build host a pre-planted symlink could redirect the private-key write. Prefer a fresh unpredictable directory.🔒 Suggested hardening
- const { writeFileSync, unlinkSync } = require("fs") as typeof import("fs"); - const tmp = `/tmp/sst-ssh-gw-${process.pid}`; + const { writeFileSync, unlinkSync, mkdtempSync, rmSync } = require("fs") as typeof import("fs"); + const { tmpdir } = require("os") as typeof import("os"); + const { join } = require("path") as typeof import("path"); + const dir = mkdtempSync(join(tmpdir(), "sst-ssh-gw-")); + const tmp = join(dir, "key"); try { writeFileSync(tmp, Buffer.from(privB64, "base64").toString("utf8"), { mode: 0o600 }); return execSync(`ssh-keygen -y -f ${tmp}`, { encoding: "utf8" }).trim(); } catch (err) { const reason = err instanceof Error ? err.message : String(err); throw new Error(`SSH_GATEWAY_PUBLIC_KEY cannot be derived from SSH_PRIVATE_KEY_B64: ${reason}`); } finally { - try { unlinkSync(tmp); } catch { /* best-effort cleanup */ } + try { rmSync(dir, { recursive: true, force: true }); } catch { /* best-effort cleanup */ } }🤖 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/infra/sst.config.ts` around lines 1027 - 1046, deriveGatewayPublicKey currently writes the decoded private key to a predictable tmp path, which can be exploited via a pre-existing symlink. Update the temp-file handling in deriveGatewayPublicKey to use a freshly created unpredictable location instead of `/tmp/sst-ssh-gw-${process.pid}`, and keep the existing ssh-keygen and cleanup flow intact. Reference the deriveGatewayPublicKey helper and its writeFileSync/unlinkSync usage when making the change.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/runner/pkg/sshgateway/service.go`:
- Around line 155-201: Update the stale docs around startExecFn, the sshUser
field, and sshUserOrDefault() so they all describe the actual default behavior:
empty sshUser resolves to "root", not "boxlite". Adjust the comments on
Service.startExec, Service.sshUser, and sshUserOrDefault() to match the current
implementation and remove the misleading unprivileged-user claim unless the
default behavior itself is being changed.
---
Nitpick comments:
In `@apps/api/src/box/common/redis-lock.provider.ts`:
- Around line 65-75: The unlockOwned method in RedisLockProvider uses an
unnecessary as any cast around this.redis.eval, which bypasses type checking.
Replace the cast by calling the typed ioredis API directly, or refactor the Lua
script into a reusable defineCommand on RedisLockProvider so the command is
strongly typed and cached; keep the compare-and-delete behavior intact while
removing the unsafe cast.
In `@apps/infra/sst.config.ts`:
- Around line 1027-1046: deriveGatewayPublicKey currently writes the decoded
private key to a predictable tmp path, which can be exploited via a pre-existing
symlink. Update the temp-file handling in deriveGatewayPublicKey to use a
freshly created unpredictable location instead of
`/tmp/sst-ssh-gw-${process.pid}`, and keep the existing ssh-keygen and cleanup
flow intact. Reference the deriveGatewayPublicKey helper and its
writeFileSync/unlinkSync usage when making the change.
🪄 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: bf7dabac-5de3-4265-bd8a-ba65b161d042
⛔ Files ignored due to path filters (1)
apps/ssh-gateway/go.sumis excluded by!**/*.sum
📒 Files selected for processing (79)
.github/workflows/build-runtime.yml.gitignoreapps/api-client-go/model_ssh_access_validation_dto.goapps/api-client-go/model_ssh_access_validation_dto_test.goapps/api/jest.config.tsapps/api/src/admin/controllers/box.controller.tsapps/api/src/admin/controllers/runner.controller.tsapps/api/src/api-key/api-key.controller.tsapps/api/src/box/common/redis-lock.provider.tsapps/api/src/box/controllers/box.controller.tsapps/api/src/box/controllers/runner.controller.tsapps/api/src/box/controllers/volume.controller.tsapps/api/src/box/dto/ssh-access.dto.tsapps/api/src/box/entities/ssh-access.entity.tsapps/api/src/box/runner-adapter/runnerAdapter.tsapps/api/src/box/runner-adapter/runnerAdapter.v0.tsapps/api/src/box/runner-adapter/runnerAdapter.v2.tsapps/api/src/box/services/box.service.tsapps/api/src/box/services/sandbox-ssh-access.spec.tsapps/api/src/boxlite-rest/boxlite-box.controller.tsapps/api/src/migrations/1778751201300-migration.tsapps/api/src/organization/controllers/organization-invitation.controller.tsapps/api/src/organization/controllers/organization-region.controller.tsapps/api/src/organization/controllers/organization-role.controller.tsapps/api/src/organization/controllers/organization-user.controller.tsapps/api/src/organization/controllers/organization.controller.tsapps/api/src/organization/guards/organization-access.guard.tsapps/api/src/user/user.controller.tsapps/api/src/webhook/controllers/webhook.controller.tsapps/api/tsconfig.spec.jsonapps/apps/tsconfig.base.jsonapps/dashboard/src/hooks/mutations/useCreateSshAccessMutation.tsapps/infra/sst.config.tsapps/libs/api-client/src/api/box-api.tsapps/runner/README.mdapps/runner/cmd/runner/config/config.goapps/runner/cmd/runner/main.goapps/runner/pkg/api/controllers/proxy.goapps/runner/pkg/api/controllers/ssh_access.goapps/runner/pkg/api/controllers/ssh_access_test.goapps/runner/pkg/api/server.goapps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/client_ssh.goapps/runner/pkg/boxlite/client_ssh_test.goapps/runner/pkg/boxlite/exec_manager.goapps/runner/pkg/runner/runner.goapps/runner/pkg/sshgateway/service.goapps/runner/pkg/sshgateway/ssh_integration_test.goapps/runner/pkg/sshgateway/stdin_cancel_test.goapps/runner/pkg/sshport/allocator.goapps/runner/pkg/sshport/allocator_test.goapps/ssh-gateway/main.goapps/ssh-gateway/main_test.gojest.preset.jsmake/build.mkmake/test.mkopenapi/box.openapi.yamlscripts/build/build-static-sshd.shscripts/deploy/runner-update-binary.shsdks/c/include/boxlite.hsdks/c/include/boxlite_internal.hsdks/c/src/box_handle.rssdks/c/src/event_queue.rssdks/c/src/rest.rssdks/c/src/runtime.rssdks/c/src/tests.rssdks/go/box_handle.gosdks/go/boxlite_internal.hsdks/go/bridge.hsdks/go/exec.gosrc/boxlite/build.rssrc/boxlite/src/net/gvproxy/config.rssrc/boxlite/src/rootfs/guest.rssrc/boxlite/src/runtime/layout.rssrc/deps/libgvproxy-sys/gvproxy-bridge/main.gosrc/guest/scripts/boxlite-disable-sshsrc/guest/scripts/boxlite-enable-sshsrc/guest/scripts/boxlite-ensure-sshsrc/guest/src/container/spec.rs
✅ Files skipped from review due to trivial changes (11)
- apps/api/src/admin/controllers/box.controller.ts
- apps/apps/tsconfig.base.json
- sdks/c/src/tests.rs
- apps/dashboard/src/hooks/mutations/useCreateSshAccessMutation.ts
- apps/api/src/organization/controllers/organization-user.controller.ts
- apps/api/src/user/user.controller.ts
- apps/api/src/boxlite-rest/boxlite-box.controller.ts
- .gitignore
- apps/api/src/api-key/api-key.controller.ts
- apps/api/src/admin/controllers/runner.controller.ts
- apps/runner/README.md
🚧 Files skipped from review as they are similar to previous changes (61)
- sdks/go/bridge.h
- jest.preset.js
- sdks/go/boxlite_internal.h
- apps/api/src/box/entities/ssh-access.entity.ts
- sdks/c/include/boxlite_internal.h
- sdks/go/box_handle.go
- sdks/c/src/event_queue.rs
- sdks/c/include/boxlite.h
- apps/runner/cmd/runner/main.go
- apps/api/src/box/controllers/runner.controller.ts
- .github/workflows/build-runtime.yml
- make/test.mk
- apps/api/src/webhook/controllers/webhook.controller.ts
- apps/runner/pkg/boxlite/exec_manager.go
- src/boxlite/src/rootfs/guest.rs
- apps/runner/pkg/api/controllers/proxy.go
- apps/api/src/organization/guards/organization-access.guard.ts
- apps/api/src/organization/controllers/organization-region.controller.ts
- apps/runner/pkg/api/server.go
- apps/api/src/organization/controllers/organization-role.controller.ts
- apps/runner/pkg/api/controllers/ssh_access_test.go
- apps/runner/pkg/runner/runner.go
- apps/api-client-go/model_ssh_access_validation_dto.go
- apps/api/tsconfig.spec.json
- apps/runner/pkg/sshport/allocator.go
- apps/api/src/box/controllers/volume.controller.ts
- apps/api/src/organization/controllers/organization.controller.ts
- sdks/c/src/rest.rs
- scripts/build/build-static-sshd.sh
- src/boxlite/src/net/gvproxy/config.rs
- apps/api/src/box/dto/ssh-access.dto.ts
- apps/api/jest.config.ts
- apps/api/src/organization/controllers/organization-invitation.controller.ts
- apps/api-client-go/model_ssh_access_validation_dto_test.go
- apps/runner/pkg/api/controllers/ssh_access.go
- src/guest/src/container/spec.rs
- make/build.mk
- apps/runner/pkg/sshport/allocator_test.go
- sdks/c/src/runtime.rs
- apps/runner/cmd/runner/config/config.go
- src/guest/scripts/boxlite-enable-ssh
- apps/api/src/box/runner-adapter/runnerAdapter.ts
- apps/api/src/migrations/1778751201300-migration.ts
- scripts/deploy/runner-update-binary.sh
- src/guest/scripts/boxlite-ensure-ssh
- apps/api/src/box/runner-adapter/runnerAdapter.v0.ts
- src/guest/scripts/boxlite-disable-ssh
- src/boxlite/build.rs
- openapi/box.openapi.yaml
- src/boxlite/src/runtime/layout.rs
- sdks/go/exec.go
- sdks/c/src/box_handle.rs
- apps/runner/pkg/boxlite/client_ssh.go
- apps/api/src/box/controllers/box.controller.ts
- apps/runner/pkg/boxlite/client.go
- apps/api/src/box/services/box.service.ts
- apps/libs/api-client/src/api/box-api.ts
- apps/api/src/box/runner-adapter/runnerAdapter.v2.ts
- apps/runner/pkg/sshgateway/stdin_cancel_test.go
- apps/ssh-gateway/main.go
- apps/ssh-gateway/main_test.go
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
from
9c765c0 to
4fd484d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/runner/pkg/sshgateway/ssh_integration_test.go (1)
294-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
filepathhelper shadows the stdlibpath/filepathpackageNaming this test helper
filepathshadows the standardpath/filepathpackage name within this file. If a future test needsfilepath.Join/filepath.Base, this will silently resolve to the local function instead, causing a confusing compile error or (if never imported) simply blocking that import.♻️ Proposed rename
-// filepath returns a path inside t.TempDir() with the given base name. -func filepath(t *testing.T, name string) string { +// tempFilePath returns a path inside t.TempDir() with the given base name. +func tempFilePath(t *testing.T, name string) string { t.Helper() return t.TempDir() + "/" + name }Update the two call sites (
dst := filepath(t, "dst.bin")) accordingly.🤖 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/sshgateway/ssh_integration_test.go` around lines 294 - 298, The test helper named filepath in ssh_integration_test.go shadows the standard path/filepath package name, which can cause future imports or calls like filepath.Join/Base to resolve incorrectly. Rename the local helper to a less conflicting name, and update its call sites in the SSH integration tests (including the dst := filepath(t, "dst.bin") usages) to match the new helper name.
🤖 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-client-go/api/openapi.yaml`:
- Around line 2461-2466: The SSH access endpoint request body was made required
in the OpenAPI spec, which forces SDK callers to always send a body even though
the server accepts it as optional. Update the requestBody definition for
CreateSshAccessBodyDto in the OpenAPI schema to be optional again so it matches
BoxController’s `@Body`() body?: CreateSshAccessBodyDto behavior and avoids the
generated CreateSshAccessExecute hard-fail in the Go client. Keep the change
scoped to the SSH access operation and ensure the schema still allows an empty
or omitted body.
In `@apps/api/src/box/dto/ssh-access.dto.ts`:
- Around line 132-143: The CreateSshAccessBodyDto fields unixUser and unix_user
need a POSIX username allowlist before guest provisioning, since unsafe values
can break interpolation in the guest script and inject config into /etc/passwd,
/etc/sudoers.d, HOME_DIR, and the AllowUsers heredoc. Add validation directly on
these DTO properties so only safe username characters are accepted, and ensure
both camelCase and snake_case inputs are covered by the same rule in
CreateSshAccessBodyDto.
In `@apps/infra/sst.config.ts`:
- Around line 1024-1046: The SSH gateway public key derivation is reading from
process.env, so it misses the SST secret value and can produce an empty key in
the normal deploy path. Update deriveGatewayPublicKey() to accept the secret
value directly, pass sshPrivateKey.value into it, and reuse the derived result
at both runner user-data call sites so the same key is used consistently.
In `@apps/runner/pkg/boxlite/client_ssh.go`:
- Around line 416-433: Persist the degraded SSH state in the rollback path of
client_ssh.go before returning from enableSSHAccess, so the failure case is
recoverable after restart. Update the same SSH state bookkeeping used by
reconcileSSHState and the existing c.sshStates/c.sshBoxes tracking so the
removed gvproxy forward is recorded in durable state, not only in memory, and
ensure the reserved port remains associated with the box across crashes.
---
Nitpick comments:
In `@apps/runner/pkg/sshgateway/ssh_integration_test.go`:
- Around line 294-298: The test helper named filepath in ssh_integration_test.go
shadows the standard path/filepath package name, which can cause future imports
or calls like filepath.Join/Base to resolve incorrectly. Rename the local helper
to a less conflicting name, and update its call sites in the SSH integration
tests (including the dst := filepath(t, "dst.bin") usages) to match the new
helper name.
🪄 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: 381464b9-cae8-4c7f-8a55-64d66dd0ca5a
⛔ Files ignored due to path filters (1)
apps/ssh-gateway/go.sumis excluded by!**/*.sum
📒 Files selected for processing (87)
.github/workflows/build-runtime.yml.gitignoreapps/api-client-go/.openapi-generator/FILESapps/api-client-go/api/openapi.yamlapps/api-client-go/api_box.goapps/api-client-go/model_create_ssh_access_body_dto.goapps/api-client-go/model_ssh_access_validation_dto.goapps/api/jest.config.tsapps/api/src/admin/controllers/box.controller.tsapps/api/src/admin/controllers/runner.controller.tsapps/api/src/api-key/api-key.controller.tsapps/api/src/box/common/redis-lock.provider.tsapps/api/src/box/controllers/box.controller.tsapps/api/src/box/controllers/runner.controller.tsapps/api/src/box/controllers/volume.controller.tsapps/api/src/box/dto/ssh-access.dto.tsapps/api/src/box/entities/ssh-access.entity.tsapps/api/src/box/runner-adapter/runnerAdapter.tsapps/api/src/box/runner-adapter/runnerAdapter.v0.tsapps/api/src/box/runner-adapter/runnerAdapter.v2.tsapps/api/src/box/services/box.service.tsapps/api/src/box/services/sandbox-ssh-access.spec.tsapps/api/src/boxlite-rest/boxlite-box.controller.tsapps/api/src/migrations/1778751201300-migration.tsapps/api/src/organization/controllers/organization-invitation.controller.tsapps/api/src/organization/controllers/organization-region.controller.tsapps/api/src/organization/controllers/organization-role.controller.tsapps/api/src/organization/controllers/organization-user.controller.tsapps/api/src/organization/controllers/organization.controller.tsapps/api/src/organization/guards/organization-access.guard.tsapps/api/src/user/user.controller.tsapps/api/src/webhook/controllers/webhook.controller.tsapps/api/tsconfig.spec.jsonapps/dashboard/src/hooks/mutations/useCreateSshAccessMutation.tsapps/infra/sst.config.tsapps/libs/api-client/src/.openapi-generator/FILESapps/libs/api-client/src/api/box-api.tsapps/libs/api-client/src/docs/BoxApi.mdapps/libs/api-client/src/docs/CreateSshAccessBodyDto.mdapps/libs/api-client/src/docs/SshAccessValidationDto.mdapps/libs/api-client/src/models/create-ssh-access-body-dto.tsapps/libs/api-client/src/models/index.tsapps/libs/api-client/src/models/ssh-access-validation-dto.tsapps/runner/README.mdapps/runner/cmd/runner/config/config.goapps/runner/cmd/runner/main.goapps/runner/pkg/api/controllers/proxy.goapps/runner/pkg/api/controllers/ssh_access.goapps/runner/pkg/api/controllers/ssh_access_test.goapps/runner/pkg/api/server.goapps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/client_ssh.goapps/runner/pkg/boxlite/client_ssh_test.goapps/runner/pkg/boxlite/exec_manager.goapps/runner/pkg/runner/runner.goapps/runner/pkg/sshgateway/service.goapps/runner/pkg/sshgateway/ssh_integration_test.goapps/runner/pkg/sshgateway/stdin_cancel_test.goapps/runner/pkg/sshport/allocator.goapps/runner/pkg/sshport/allocator_test.goapps/ssh-gateway/main.goapps/ssh-gateway/main_test.gomake/build.mkmake/test.mkopenapi/box.openapi.yamlscripts/build/build-static-sshd.shscripts/deploy/runner-update-binary.shsdks/c/include/boxlite.hsdks/c/include/boxlite_internal.hsdks/c/src/box_handle.rssdks/c/src/event_queue.rssdks/c/src/rest.rssdks/c/src/runtime.rssdks/c/src/tests.rssdks/go/box_handle.gosdks/go/boxlite_internal.hsdks/go/bridge.hsdks/go/exec.gosrc/boxlite/build.rssrc/boxlite/src/net/gvproxy/config.rssrc/boxlite/src/rootfs/guest.rssrc/boxlite/src/runtime/layout.rssrc/deps/libgvproxy-sys/gvproxy-bridge/main.gosrc/guest/scripts/boxlite-disable-sshsrc/guest/scripts/boxlite-enable-sshsrc/guest/scripts/boxlite-ensure-sshsrc/guest/src/container/spec.rs
✅ Files skipped from review due to trivial changes (19)
- apps/libs/api-client/src/models/create-ssh-access-body-dto.ts
- apps/api/src/organization/controllers/organization-user.controller.ts
- apps/api/src/box/controllers/volume.controller.ts
- apps/libs/api-client/src/.openapi-generator/FILES
- apps/api/src/admin/controllers/box.controller.ts
- apps/api/src/organization/controllers/organization-role.controller.ts
- apps/api/src/organization/controllers/organization-invitation.controller.ts
- apps/api/src/user/user.controller.ts
- apps/runner/README.md
- sdks/c/src/rest.rs
- apps/api/src/organization/controllers/organization-region.controller.ts
- apps/api/src/box/controllers/runner.controller.ts
- apps/api/src/organization/guards/organization-access.guard.ts
- apps/api/src/webhook/controllers/webhook.controller.ts
- apps/api/src/admin/controllers/runner.controller.ts
- apps/api-client-go/model_create_ssh_access_body_dto.go
- .gitignore
- sdks/c/src/event_queue.rs
- apps/api/src/organization/controllers/organization.controller.ts
🚧 Files skipped from review as they are similar to previous changes (49)
- sdks/go/boxlite_internal.h
- sdks/c/src/tests.rs
- apps/runner/pkg/api/server.go
- sdks/go/bridge.h
- apps/api/src/box/entities/ssh-access.entity.ts
- sdks/go/box_handle.go
- sdks/c/include/boxlite.h
- apps/api/src/api-key/api-key.controller.ts
- sdks/c/include/boxlite_internal.h
- src/boxlite/src/runtime/layout.rs
- apps/runner/pkg/boxlite/exec_manager.go
- make/test.mk
- scripts/build/build-static-sshd.sh
- apps/runner/cmd/runner/main.go
- apps/api/jest.config.ts
- apps/api/src/boxlite-rest/boxlite-box.controller.ts
- .github/workflows/build-runtime.yml
- apps/api/src/migrations/1778751201300-migration.ts
- apps/api/tsconfig.spec.json
- apps/runner/cmd/runner/config/config.go
- src/guest/scripts/boxlite-ensure-ssh
- src/guest/src/container/spec.rs
- apps/runner/pkg/api/controllers/proxy.go
- make/build.mk
- apps/runner/pkg/sshport/allocator_test.go
- sdks/go/exec.go
- apps/api/src/box/runner-adapter/runnerAdapter.ts
- apps/runner/pkg/sshport/allocator.go
- src/guest/scripts/boxlite-disable-ssh
- scripts/deploy/runner-update-binary.sh
- src/boxlite/src/rootfs/guest.rs
- apps/runner/pkg/api/controllers/ssh_access_test.go
- sdks/c/src/runtime.rs
- apps/api/src/box/runner-adapter/runnerAdapter.v2.ts
- src/boxlite/build.rs
- src/boxlite/src/net/gvproxy/config.rs
- apps/runner/pkg/runner/runner.go
- sdks/c/src/box_handle.rs
- apps/api/src/box/common/redis-lock.provider.ts
- apps/api/src/box/runner-adapter/runnerAdapter.v0.ts
- openapi/box.openapi.yaml
- apps/runner/pkg/api/controllers/ssh_access.go
- apps/runner/pkg/boxlite/client.go
- apps/api/src/box/controllers/box.controller.ts
- apps/ssh-gateway/main_test.go
- apps/api/src/box/services/box.service.ts
- apps/runner/pkg/sshgateway/stdin_cancel_test.go
- apps/ssh-gateway/main.go
- src/guest/scripts/boxlite-enable-ssh
nieyy
added a commit
to nieyy/boxlite
that referenced
this pull request
Jul 7, 2026
Fixes 11 issues flagged during review of the real-SSH feature: - openapi/box.openapi.yaml: remove orphaned duplicate description key under CreateSshAccessRequest.unix_user - box.service.ts: revokeSshAccess fails closed (instead of silently deleting the DB token) when the runner record is missing but real-SSH state exists - redis-lock.provider.ts: waitForLock/waitForLockOwned take an explicit timeout instead of polling forever when a lock is never released - runner config.go: validate SSH_PORT_BASE/SSH_PORT_POOL_SIZE bounds so an invalid range fails at startup instead of allocating out-of-range ports - ssh-gateway main_test.go: fix data races on plain bools read/written from goroutines by switching to atomic.Bool - guest.rs: fold an SSH-assets hash (sshd/ssh-keygen binaries + management scripts) into the rootfs version key, so rebuilding them invalidates the cache instead of being silently served from a stale rootfs - gvproxy-bridge main.go: gvproxy_create now fails (instead of only logging) when the admin HTTP server can't bind its Unix socket, since a broken admin server otherwise surfaces later as an unexplained SSH forwarding failure - runnerAdapter.v2.ts: retry enableSSHAccess/disableSSHAccess on transient network errors (ECONNRESET/ETIMEDOUT), matching RunnerAdapterV0 - migration comment: correct NULL semantics (legacy exec-bridge token, not "treated as boxlite") - runner README + sshgateway service.go comments: correct the documented default SSH exec user from stale "boxlite" to the actual "root" default - build-static-sshd.sh: validate Docker availability/reachability and output directory writability before running, instead of failing deep inside the Docker build with a confusing error Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
2 times, most recently
from
1c4300c to
88f2209
Compare
Member
|
a design note on the sshd choice: since the guest agent is rust, the tempting alternative was embedding an ssh library in it — russh is mature (warpgate, a production ssh gateway, is built on it), and that route would delete the whole openssh supply line: the alpine/musl ci build, dist/ bundling, ext4 injection, bind mounts, even ssh-keygen. why real openssh is still the right call here: with an embedded library, our own code becomes the security guard — "run this session as alice, with alice's permissions, nobody else's" turns into hand-rolled fork/setuid with no AllowUsers and no privilege separation. that's the known weak spot of the embedded approach (daytona's in-sandbox daemon takes it: gliderlabs server accepting any pubkey, a hardcoded password, one shared uid, exit codes flattened to 127). since unix_user enforcement is this pr's core security contract, a 25-year-audited sshd at the login boundary beats a library plus homemade login logic. rule of thumb: embed the library where we're a broker (the external gateway could be russh one day), exec the real sshd where we're the login target. this pr lands on the right side of that line. |
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
3 times, most recently
from
c300265 to
abc52ff
Compare
Author
Yeah, I hadn't looked into russh before — but I was already convinced this had to be built on real sshd from the start. Appreciate the validation! |
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
from
abc52ff to
52f6bb0
Compare
📦 BoxLite review — 3 issues ·
|
Author
|
Thanks for the detailed review — all 4 are fixed now:
Appreciate the catches — #3 and #4 in particular would've been nasty to track down in prod. |
Adds full SSH (SFTP, scp, port forwarding, non-PTY exec) to BoxLite sandboxes by
routing SSH-access tokens through a per-sandbox port on the Runner EC2 directly to
a real sshd running inside the container namespace, bypassing the Rust exec bridge
that blocks binary streams. The external SSH Gateway detects real-SSH tokens (unix_user
non-null) and TCP-proxies to the allocated port; legacy exec-bridge tokens fall through
unchanged.
Security contract:
- Only the gateway public key is installed in guest sshd authorized_keys
- Real-SSH tokens require unix_user; empty/absent → null (exec-bridge) at controller boundary
- Gateway is fail-closed on every error path: lookup failure, degraded state, unix_user
mismatch, missing RUNNER_API_TOKEN, empty unix_user string
- Per-sandbox Redis lock (90s TTL, compare-and-delete) guards concurrent enable/disable
- Expired row cleanup covers all three cases: runner-attached, runner-less, null sandbox
- Non-expired token with null sandbox (concurrent delete) returns invalid cleanly
- Revocation is fail-closed (disable-before-delete): runner unreachable → error propagated,
DB row preserved for retry; legacy exec-bridge tokens use best-effort runner cleanup
- PermitRootLogin set to prohibit-password; default SSH user is root
- OpenSSH tarball SHA256 verified in CI before build (supply-chain)
- SSH bearer tokens redacted in gateway logs (8-char prefix kept for correlation)
- Real-SSH classification derives from GetUnixUser() != "" rather than the generated
client's HasUnixUser(), which reports key-presence (true even for explicit null) and
would misclassify every legacy exec-bridge token as real-SSH
- unixUser/unix_user validated against a POSIX username allowlist before guest
provisioning: the value is interpolated unquoted into /etc/passwd,
/etc/sudoers.d/$UNIX_USER, and the sshd AllowUsers config by boxlite-enable-ssh, so
an unvalidated value could corrupt those files or inject extra sshd directives
Implementation:
- CI statically compiles OpenSSH 9.7p1 (sshd + ssh-keygen) via Alpine musl Docker;
`make sshd` runs the same script locally; guest.rs injects binaries + management
scripts into the guest ext4 rootfs at build time
- SSH infra files bind-mounted read-only into container from guest rootfs, gated on
/boxlite/bin/sshd presence with per-file checks
- boxlite-enable-ssh: UID collision check covers /etc/group; sudoers written before sshd
starts; passwordless sudo scoped to default 'boxlite' user only; root home dir handled;
creates sshd privsep user+group when absent (required by non-Alpine images)
- boxlite-disable-ssh: root-home asymmetry fixed (targets /root/.ssh for root user);
mirrors enable-side sudoers guard
- gvproxy admin socket path carried explicitly in GvproxyConfig (no path arithmetic in
Go); exposed via C ABI as boxlite_box_admin_sock_path() and cached in SSHState so
Disable/Reapply/Cleanup do not re-derive it; gvproxy_create fails (instead of only
logging) when the admin HTTP server can't bind its Unix socket
- gvproxy admin HTTP server handles /services/forwarder/expose for dynamic per-sandbox
port forwarding to guest sshd (port 22222)
- sst.config.ts: auto-derive SSH_GATEWAY_PUBLIC_KEY from SSH_PRIVATE_KEY_B64 (threaded
through as the sst.Secret's resolved value, not read from process.env, which never
contains sst.Secret values inside sst.config.ts); quote key in systemd Environment=
(spaces); restrict port 22100–22199 ingress to ECS gateway SG
- runner-update-binary.sh: guard SSH_GATEWAY_PUBLIC_KEY injection so running without the
secret does not erase the live value
- runnerAdapter.v2.ts: implement enableSSHAccess / disableSSHAccess natively with retry
on transient network errors (ECONNRESET/ETIMEDOUT); 503 surfaces as error, 404 is
no-op on disable; remove stale apiVersion gate
- box.service.ts: fix remainingRealSsh to count only unixUser≠null rows; add lock-lost
fencing before runner disable; rollback on create failure skips if prior real-SSH
session existed; real→legacy rotation calls disableSSHAccess fail-closed; revokeSshAccess
fails closed instead of silently deleting the DB token when the runner record is
missing but real-SSH state exists
- client_ssh.go: internalBoxID() with three-level fallback; hostPort=0 handled in
forward recovery; per-box mutex deleted in cleanupSSHOnDestroy
- ssh-gateway: track pty-req per channel; PTY sessions use Close() on exit so the
client exits immediately; non-PTY sessions keep CloseWrite() for scp/sftp teardown
- redis-lock.provider.ts: waitForLock/waitForLockOwned take an explicit timeout instead
of polling forever when a lock is never released
- runner config.go: validate SSH_PORT_BASE/SSH_PORT_POOL_SIZE bounds so an invalid range
fails at startup instead of allocating out-of-range ports
- guest.rs: fold an SSH-assets hash (sshd/ssh-keygen binaries + management scripts) into
the rootfs version key, so rebuilding them invalidates the cache instead of being
silently served from a stale rootfs
- box.controller.ts: @ApiBody({required: false}) on createSshAccess so the generated
OpenAPI spec (and Go SDK) stop requiring a body the server never required
- OpenAPI: SSH access endpoints + schemas added to box.openapi.yaml; generated
api-client-go/libs-api-client clients regenerated to match (unix_user hidden from
schema via @ApiHideProperty to avoid a Go PascalCase field collision with unixUser)
- build-static-sshd.sh: validate Docker availability/reachability and output directory
writability before running, instead of failing deep inside the Docker build
- corrected stale docs: migration comment (NULL means legacy exec-bridge token, not
"treated as boxlite"), runner README + sshgateway service.go default SSH exec user
("root", not "boxlite")
- 62 regression tests from the initial implementation, plus additional tests added
for each fix above (DTO validation, port-range bounds, lock timeout, retry behavior,
rootfs cache-key hashing, admin-socket failure, data-race fixes)
nieyy
force-pushed
the
claude/orchestrator/c944e67f
branch
from
52f6bb0 to
c76a6de
Compare
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
Comment on lines
+819
to
+829
| func (c *Client) cleanupSSHOnDestroy(ctx context.Context, boxId string, alloc *sshport.Allocator) { | ||
| mu := c.boxSSHMutex(boxId) | ||
| mu.Lock() | ||
| defer mu.Unlock() | ||
|
|
||
| c.mu.RLock() | ||
| state, ok := c.sshStates[boxId] | ||
| c.mu.RUnlock() | ||
| if !ok { | ||
| return | ||
| } |
There was a problem hiding this comment.
boxSSHMutex(boxId) inserts an entry into c.boxSSHMu before the if !ok { return } guard, which fires for every box that never enabled SSH, so delete(c.boxSSHMu, boxId) at line 851 is never reached — unbounded map growth per Destroy() call over the runner's lifetime.
Suggested change
| func (c *Client) cleanupSSHOnDestroy(ctx context.Context, boxId string, alloc *sshport.Allocator) { | |
| mu := c.boxSSHMutex(boxId) | |
| mu.Lock() | |
| defer mu.Unlock() | |
| c.mu.RLock() | |
| state, ok := c.sshStates[boxId] | |
| c.mu.RUnlock() | |
| if !ok { | |
| return | |
| } | |
| func (c *Client) cleanupSSHOnDestroy(ctx context.Context, boxId string, alloc *sshport.Allocator) { | |
| mu := c.boxSSHMutex(boxId) | |
| mu.Lock() | |
| defer mu.Unlock() | |
| defer func() { | |
| c.boxSSHMuMu.Lock() | |
| delete(c.boxSSHMu, boxId) | |
| c.boxSSHMuMu.Unlock() | |
| }() | |
| c.mu.RLock() | |
| state, ok := c.sshStates[boxId] | |
| c.mu.RUnlock() | |
| if !ok { | |
| return | |
| } |
Comment on lines
+377
to
+378
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() |
There was a problem hiding this comment.
handleChannel builds its context from context.Background() instead of the Start(ctx) passed in (previously execCtx derived from ctx); cancelling the gateway's ctx on shutdown/SIGTERM no longer terminates in-flight exec sessions or their guest processes — they run until the client disconnects.
Author
|
Need to reimplement via russh, will open another PR. |
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
4 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.
Adds full SSH (SFTP, scp, port forwarding, non-PTY exec) to BoxLite sandboxes by routing SSH-access tokens through a per-sandbox port on the Runner EC2 directly to a real sshd running inside the container namespace, bypassing the Rust exec bridge that blocks binary streams. The external SSH Gateway detects real-SSH tokens (unix_user non-null) and TCP-proxies to the allocated port; legacy exec-bridge tokens fall through unchanged.
Security contract:
Implementation:
guest.rs injects binaries + management scripts into the guest ext4 rootfs at build time
container from guest rootfs, gated on /boxlite/bin/sshd presence with per-file checks
for dynamic per-sandbox port forwarding to guest sshd (port 22222)
starts; passwordless sudo scoped to default 'boxlite' user only; root home dir handled
in systemd Environment= (spaces); restrict port 22100–22199 ingress to ECS gateway SG
secret does not erase the live value
503 surfaces as error, 404 is no-op on disable; remove stale apiVersion gate
lock-lost fencing before runner disable; rollback on create failure skips if prior
real-SSH session existed; real→legacy rotation calls disableSSHAccess fail-closed
query, external UUID) for correct gvproxy socket path resolution; hostPort=0 case
handled in forward recovery (degraded mode when gvproxy was unavailable at enable time)
server EOF exit cleanly instead of hanging
Summary by CodeRabbit
New Features
Bug Fixes