feat(deployment): add shell-exec endpoint for synchronous command execution#3097
feat(deployment): add shell-exec endpoint for synchronous command execution#3097jobordu wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a synchronous shell-exec HTTP endpoint that validates deployment leases, authenticates with provider scope, executes commands through a WebSocket service, and returns structured output including stdout, stderr, exit code, and truncation status. ChangesShell-Exec Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Assessment against linked issues
Suggested labels: Suggested reviewers: ✨ 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/deployment/controllers/shell-exec/shell-exec.controller.spec.ts (1)
130-182:setup()mutates the global DI container — tests are not isolated.
container.register(AuthService, ...)on everysetup()call keeps stacking registrations on the root tsyringe container, and there's no teardown. Running this spec alongside other specs that resolveAuthServicewill pollute state. Use a child container (container.createChildContainer()) per test, or callcontainer.clearInstances()/container.reset()in anafterEach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.ts` around lines 130 - 182, The setup() helper is mutating the global tsyringe container by calling container.register(AuthService, ...) which leaks DI state across tests; change setup() to create a fresh child container via container.createChildContainer() and perform registrations on that child (or alternatively call container.clearInstances() / container.reset() in an afterEach) so each test uses an isolated container; update references in setup() to register AuthService on the child container (and resolve services from the child) or add an afterEach that invokes container.clearInstances()/container.reset() to remove accumulated registrations.apps/api/src/deployment/services/shell-exec/shell-exec.service.ts (2)
121-123: Silent catches + no logging hide real problems.
catch {}at Line 121 and the empty-handled error paths drop every parse/protocol anomaly on the floor. Per coding guidelines, backend code should useLoggerService(notconsole.*) — here it's using nothing at all. At minimum, log parse failures atwarn/debugwith the raw payload length (not contents, to avoid leaking secrets) so operational issues are diagnosable.As per coding guidelines: "Use
LoggerServiceinstead ofconsole.log/warn/errorfor logging".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/deployment/services/shell-exec/shell-exec.service.ts` around lines 121 - 123, The empty catch block in shell-exec.service.ts silently swallows parse/protocol errors; replace it by injecting/using the service LoggerService (or the existing logger instance) inside the catch of the parsing logic (the catch following the parse/deserialize in the ShellExecService method) and log a warning or debug-level entry that includes the event context and the raw payload length (not contents) — e.g., logger.warn or logger.debug with a message like "Failed to parse shell exec payload" plus payload.length and any non-sensitive identifiers — then rethrow or handle as appropriate per current flow; do not use console.*.
159-174: Document or fix the space-splitting limitation in shell command parsing.The
buildShellUrlmethod intentionally splits commands on spaces only (not with proper shell tokenization). This is by design based on the test suite, but causes correctness issues:"echo 'hello world'"becomes three URL parameters (cmd0=echo,cmd1='hello',cmd2=world'), losing the quoting and breaking arguments containing spaces—likely in this endpoint since it targets post-deploy secret injection (paths, JSON blobs, cert PEMs).Two options:
- Preferred: Change
commandfromstringtostring[]in the API contract, removing the need to split and matching kubectl exec semantics.- If keeping string input: Document this space-splitting behavior explicitly in the OpenAPI description so API consumers understand the limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/deployment/services/shell-exec/shell-exec.service.ts` around lines 159 - 174, The buildShellUrl currently splits ShellExecInput.command by spaces, breaking quoted/space-containing args; change the API contract so ShellExecInput.command is string[] (matching kubectl exec semantics) and update buildShellUrl to accept that array (encode each element as &cmd{i}=...), removing the string-splitting logic; update all call sites, tests, and the OpenAPI/DTO/schema for the shell exec endpoint to reflect command: string[] and adjust any validation/serialization accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/package.json`:
- Line 109: The package.json currently pins the "ws" dependency at "7.5.9" which
contains CVE-2024-37890; update the "ws" entry to a secure version (minimum
"7.5.10" or preferably "^8.20.0" / "^8") in package.json, then regenerate the
lockfile (run npm install or yarn install), commit the updated package.json and
lockfile, and run the test suite and npm/yarn audit/scan to verify no remaining
vulnerabilities; locate the "ws": "7.5.9" line in package.json to make this
change.
In `@apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.ts`:
- Around line 57-59: Replace the direct assert(false, 502, result.val) call in
the shell-exec controller with a safe mapping: inspect the result (the variable
named result returned from the shell exec service), convert known failure
strings into stable user-facing messages (e.g., "Command timed out", "Connection
lost", "Execution failed") and use assert or throw with that sanitized message
and 502, while sending the raw result.val to the LoggerService (or this.logger)
at error level; ensure any unknown/unmapped result.val is logged but replaced
with a generic message like "Remote execution error" before returning to the
client.
- Around line 29-43: The code only checks lease.state but not deployment.state
and calls toProviderAuth before confirming the provider exists; update the
handler to first assert deployment.state is not "closed" (e.g., check
deployment.state !== "closed" similar to lease.state check) and reorder logic so
you call providerService.getProvider(providerAddress) and assert(providerInfo,
404, "Provider not found") before calling providerService.toProviderAuth(...).
This prevents minting a JWT for a non-existent provider and ensures closed
deployments are rejected.
In `@apps/api/src/deployment/http-schemas/shell-exec.schema.ts`:
- Around line 3-13: Update ShellExecParamsSchema and ShellExecRequestSchema:
change gseq and oseq to z.coerce.number().int().nonnegative() so path params
coerce from strings and reject negatives; add dseq: z.string().regex(/^\d+$/) to
only allow digit sequences; tighten command to include .max(4096); make timeout
an integer by adding .int() (keep .min(1).max(120).default(60)); and tighten
service with a stricter regex (e.g.,
/^[a-zA-Z0-9][a-zA-Z0-9._-]{0,251}[a-zA-Z0-9]?$/ or at minimum reject spaces) on
ShellExecRequestSchema to prevent invalid service names.
In `@apps/api/src/deployment/services/shell-exec/shell-exec.service.spec.ts`:
- Around line 765-773: The test is dead because it only checks trivial
construction; replace it by exercising ShellExecService.execute() with a
negative timeout to assert timeout behavior: create the service and input via
createShellExecInput({timeout: -100}), switch to fake timers
(jest.useFakeTimers()), call service.execute(input) and capture the returned
promise, advance timers by 0 (or run pending timers) to trigger the immediate
timeout, then assert the promise resolves/rejects with the expected Err("Command
timed out") (or appropriate error shape). Ensure to clean up timers
(jest.useRealTimers()) and remove the two trivial expect(...) lines currently in
the test.
- Line 17: Tests are using (service as any).buildShellUrl and parseShellMessage
which violates the "no any" rule and couples tests to private helpers; either
export the pure helper functions buildShellUrl and parseShellMessage from the
shell-exec service module and update tests to import and assert those functions
directly with proper TypeScript types, or remove the private-helper unit tests
and rely on the existing execute(...) integration tests to cover behavior;
ensure no casts to any remain (replace with correct interfaces/types) and update
spec references from (service as any) to the exported helper names or to only
call service.execute when adopting the integration approach.
- Around line 874-912: Tests are asserting undesirable behaviors; update
ShellExecService.execute and related helpers to: 1) treat WebSocket "error"
events as failures by resolving Err with a sanitized error string (e.g., wrap
provider errors like ECONNREFUSED into Err("<sanitized connection error>"))
instead of Ok(exitCode=1) — change the error handler in ShellExecService (the
WebSocket "error" listener) to reject/return Err; 2) enforce a single combined
output cap (stdout+stderr) instead of per-stream caps by changing the
buffering/truncation logic in whatever collects stream chunks (look for
functions/variables handling MAX_OUTPUT_SIZE and per-stream tracking in
ShellExecService or its output buffer helpers) to drop or truncate incoming data
once the combined size reaches MAX_OUTPUT_SIZE; and 3) pick consistent semantics
for "no exit code received" (either always Err("Connection closed without exit
code") or always Ok({ exitCode: 1 })) and make the close/closed message handling
in ShellExecService (the close/message handlers that check for
exit_code/message.closed) follow that chosen behavior, then update/add one-line
comments in the corresponding tests to reflect intentional behavior if you keep
the non-obvious choices.
In `@apps/api/src/deployment/services/shell-exec/shell-exec.service.ts`:
- Around line 114-120: The handler that currently checks if (message.closed ||
message.error) and sets exitCode = 1 discards message.error; update the block in
shell-exec.service.ts (the message handling code around exitCode, timeoutId, ws)
to capture and surface server-reported errors by storing message.error (e.g.,
into a local serverError variable or appending it to the accumulated stderr
buffer) and include that text in the final rejection or stderr output instead of
silently returning exit code 1; ensure you still clearTimeout(timeoutId) and
call ws.close(), but propagate message.error with a clear marker so callers can
distinguish proxy-reported errors from normal process exits (reference symbols:
message, exitCode, timeoutId, ws).
- Around line 126-155: The ws "error" handler currently resolves an Ok with
exitCode=1 which masks auth/connection failures; change it to resolve an Err
containing the Error (or reject) instead of Ok so callers can distinguish
network/auth errors (update the ws.on("error", ...) handler to return Err(error)
using the same Result shape), and add a handler for ws.on("unexpected-response",
(req, res) => ...) that captures the HTTP statusCode and response body (or at
least statusCode) and resolves Err with that info; keep the existing close
handler logic (which uses exitCode/Err("Connection closed without exit code"))
intact but ensure you do not double-resolve by guarding with a resolved flag or
checking if exitCode/previous result was set before calling resolve.
- Around line 70-103: The truncation logic allows later chunks to be appended
after a drop, so change the handling in shell-exec.service.ts (inside the
message parsing branches that use stdout, stderr, MAX_OUTPUT_SIZE) to make the
truncated flag "sticky": before attempting any append in both the JSON branch
(where parsed.type === "data") and the binary branch (where message.message is
an array and firstByte is used), first check whether truncated is already true
and skip processing; when a chunk would overflow, set truncated = true and stop
appending any further data (do not allow subsequent smaller chunks to pass the
length checks), and optionally trigger early termination of the read/socket
(close the socket or break the read loop) to avoid buffering remaining stream
data in memory.
- Around line 186-201: parseShellMessage currently hardcodes stream: "stdout"
for JSON-framed messages; update it to honor any stream discriminator in the
parsed payload so stderr JSON messages aren't mislabelled. In parseShellMessage,
after JSON.parse and confirming parsed.message is a non-empty string, inspect
parsed.stream (and if applicable numeric codes like 2) and map it to "stderr" or
"stdout" accordingly (default to "stdout" if missing/unknown) before returning {
type: "data", data: parsed.message, stream: ... }; keep existing exit_code
handling intact.
---
Nitpick comments:
In
`@apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.ts`:
- Around line 130-182: The setup() helper is mutating the global tsyringe
container by calling container.register(AuthService, ...) which leaks DI state
across tests; change setup() to create a fresh child container via
container.createChildContainer() and perform registrations on that child (or
alternatively call container.clearInstances() / container.reset() in an
afterEach) so each test uses an isolated container; update references in setup()
to register AuthService on the child container (and resolve services from the
child) or add an afterEach that invokes
container.clearInstances()/container.reset() to remove accumulated
registrations.
In `@apps/api/src/deployment/services/shell-exec/shell-exec.service.ts`:
- Around line 121-123: The empty catch block in shell-exec.service.ts silently
swallows parse/protocol errors; replace it by injecting/using the service
LoggerService (or the existing logger instance) inside the catch of the parsing
logic (the catch following the parse/deserialize in the ShellExecService method)
and log a warning or debug-level entry that includes the event context and the
raw payload length (not contents) — e.g., logger.warn or logger.debug with a
message like "Failed to parse shell exec payload" plus payload.length and any
non-sensitive identifiers — then rethrow or handle as appropriate per current
flow; do not use console.*.
- Around line 159-174: The buildShellUrl currently splits ShellExecInput.command
by spaces, breaking quoted/space-containing args; change the API contract so
ShellExecInput.command is string[] (matching kubectl exec semantics) and update
buildShellUrl to accept that array (encode each element as &cmd{i}=...),
removing the string-splitting logic; update all call sites, tests, and the
OpenAPI/DTO/schema for the shell exec endpoint to reflect command: string[] and
adjust any validation/serialization accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6981f1f1-d270-42d7-86c4-a1fec3bd6a10
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
apps/api/package.jsonapps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.tsapps/api/src/deployment/controllers/shell-exec/shell-exec.controller.tsapps/api/src/deployment/http-schemas/shell-exec.schema.tsapps/api/src/deployment/routes/shell-exec/shell-exec.router.tsapps/api/src/deployment/services/shell-exec/shell-exec.service.spec.tsapps/api/src/deployment/services/shell-exec/shell-exec.service.tsapps/api/src/rest-app.tsapps/api/src/routes/deployment/index.ts
9eebcd2 to
805e071
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.ts (1)
2-6: Avoid mutating the global DI container in this unit test.
ShellExecControlleris constructed manually withauthService, so registeringAuthServicein the roottsyringecontainer is unnecessary and can leak state into unrelated specs. Remove the registration/import unless this test resolves the controller from the container.♻️ Proposed cleanup
-import { container } from "tsyringe"; import { describe, expect, it } from "vitest"; import { mock } from "vitest-mock-extended"; -import { AuthService } from "@src/auth/services/auth.service"; +import type { AuthService } from "@src/auth/services/auth.service"; @@ - container.register(AuthService, { useValue: authService }); - const controller = new ShellExecController(deploymentReaderService, providerService, shellExecService, authService, walletReaderService);Also applies to: 168-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.ts` around lines 2 - 6, The test is mutating the global tsyringe container by registering AuthService even though ShellExecController is constructed manually with a local mock; remove the unnecessary import/registration of AuthService and any calls to container.register/registerInstance in shell-exec.controller.spec.ts so the spec only uses the local mock (mock<AuthService>()) and the manual constructor for ShellExecController to avoid leaking DI state into other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/deployment/services/shell-exec/shell-exec.service.spec.ts`:
- Around line 970-995: The test currently asserts that a frame starting with
marker 0 carries stdout, but per protocol marker 1 = stdout and marker 0 is an
exit sentinel (or should be discarded for payloads), so update the test (or
service) to treat any incoming frame whose first byte is 0 and which also
contains payload as ignored/protocol error; specifically adjust the
ShellExecService.execute handling (and this spec) so the mock message
Buffer.from(JSON.stringify({ type: "websocket", message: [0,72,101,108,108,111]
})) is ignored and the final assertion checks that (result.val as
ShellExecOutput).stdout is "" (or that an appropriate protocol error is
surfaced), leaving other frames like marker 1 to populate stdout as before;
ensure references to ShellExecService.execute, createShellExecInput, and
ShellExecOutput are used when locating and updating the code/tests.
---
Nitpick comments:
In
`@apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.ts`:
- Around line 2-6: The test is mutating the global tsyringe container by
registering AuthService even though ShellExecController is constructed manually
with a local mock; remove the unnecessary import/registration of AuthService and
any calls to container.register/registerInstance in
shell-exec.controller.spec.ts so the spec only uses the local mock
(mock<AuthService>()) and the manual constructor for ShellExecController to
avoid leaking DI state into other tests.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a713d4b-f484-4a14-b6db-b801363558f2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
apps/api/package.jsonapps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.tsapps/api/src/deployment/controllers/shell-exec/shell-exec.controller.tsapps/api/src/deployment/http-schemas/shell-exec.schema.tsapps/api/src/deployment/routes/shell-exec/shell-exec.router.tsapps/api/src/deployment/services/shell-exec/shell-exec.service.spec.tsapps/api/src/deployment/services/shell-exec/shell-exec.service.tsapps/api/src/rest-app.tsapps/api/src/routes/deployment/index.ts
✅ Files skipped from review due to trivial changes (3)
- apps/api/package.json
- apps/api/src/routes/deployment/index.ts
- apps/api/src/deployment/http-schemas/shell-exec.schema.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api/src/deployment/routes/shell-exec/shell-exec.router.ts
- apps/api/src/deployment/services/shell-exec/shell-exec.service.ts
- apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.ts
805e071 to
d9d4d81
Compare
|
@baktun14 Ready ! btw, tu es Québécois ? |
…cution
- Add POST /v1/deployments/{dseq}/leases/{gseq}/{oseq}/shell-exec endpoint
- Use GranularAccess JWT with shell scope for authentication
- WebSocket client for provider-proxy shell endpoint communication
- CASL-based authorization with deployment ownership validation
- 129 tests including adversarial hardening coverage
Closes akash-network#3079
d9d4d81 to
63c7252
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.ts (1)
165-165: ⚡ Quick winAvoid nested overrides in
mock<AuthService>().Passing
{ currentUser: user }intomock()can wrapcurrentUserin a recursive proxy. Create the mock first, then assignauthService.currentUser = userso the test uses a plain object.Proposed change
- const authService = mock<AuthService>({ currentUser: user }); + const authService = mock<AuthService>(); + authService.currentUser = user;Based on learnings: "When using vitest-mock-extended, avoid passing nested objects as overrides to mock because nested objects can be wrapped by recursive proxies, causing unknown properties to resolve to mock functions instead of undefined. Use the workaround: after creating the mock, assign nested properties."
🤖 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/deployment/controllers/shell-exec/shell-exec.controller.spec.ts` at line 165, The test creates the AuthService mock with a nested override (mock<AuthService>({ currentUser: user })), which can produce recursive proxies; instead, construct the mock first (const authService = mock<AuthService>()), then assign the nested property directly (authService.currentUser = user) so currentUser is a plain object; update the spec to replace the inline override with separate creation and assignment for the authService mock used in the shell-exec.controller.spec.ts tests.
🤖 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/deployment/routes/shell-exec/shell-exec.router.ts`:
- Around line 27-50: The OpenAPI responses object for the shell-exec route is
missing a 500 entry while the controller can throw a 500 for runtime errors like
"Lease provider address not found"; update the responses object (in the same
shell-exec.router.ts responses block where ShellExecResponseSchema is used) to
include a 500 response that documents the server error (add a description like
"Internal server error - e.g., lease provider address not found" and a
content/schema entry consistent with other responses, or reference your generic
ErrorResponseSchema if available) so generated docs and clients match controller
behavior.
In `@apps/api/src/deployment/services/shell-exec/shell-exec.service.ts`:
- Around line 247-251: buildShellUrl currently falls back to emitting `&cmd0=`
when tokenizeCommand returns no tokens; instead validate and reject blank
commands: inside buildShellUrl (which calls tokenizeCommand) check whether
input.command.trim() (or tokens.length) is zero and throw a clear error (e.g.
"empty command") rather than building a URL with `cmd0=`; keep existing behavior
for normal tokens and preserve encodeURIComponent usage for providerBaseUrl,
dseq, service, etc.
---
Nitpick comments:
In
`@apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.ts`:
- Line 165: The test creates the AuthService mock with a nested override
(mock<AuthService>({ currentUser: user })), which can produce recursive proxies;
instead, construct the mock first (const authService = mock<AuthService>()),
then assign the nested property directly (authService.currentUser = user) so
currentUser is a plain object; update the spec to replace the inline override
with separate creation and assignment for the authService mock used in the
shell-exec.controller.spec.ts tests.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0a6709f-b379-495b-b40d-104b23f8aff2
📒 Files selected for processing (9)
apps/api/package.jsonapps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.tsapps/api/src/deployment/controllers/shell-exec/shell-exec.controller.tsapps/api/src/deployment/http-schemas/shell-exec.schema.tsapps/api/src/deployment/routes/shell-exec/shell-exec.router.tsapps/api/src/deployment/services/shell-exec/shell-exec.service.spec.tsapps/api/src/deployment/services/shell-exec/shell-exec.service.tsapps/api/src/routers/open-api-handlers.tsapps/api/src/routes/deployment/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/src/deployment/http-schemas/shell-exec.schema.ts
- apps/api/src/routes/deployment/index.ts
- apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.ts
- apps/api/src/deployment/services/shell-exec/shell-exec.service.spec.ts
|
Have you looked at the api docs? https://akash.network/docs/api-documentation/console-api/api-reference/#wss-lease-shell |
|
The intent of this PR isn’t to introduce new shell functionality—the provider WSS lease shell API already exists. The goal is to improve the developer experience by exposing a simple synchronous HTTP endpoint for API consumers, so they don’t need to implement provider JWT acquisition, WebSocket lifecycle management, stream demultiplexing, timeout handling, and output aggregation themselves. That said, I’d appreciate a closer review of the security model. Since this endpoint effectively exposes shell execution through the Console API, I want to make sure we’re not unintentionally broadening access or introducing privilege-escalation risks compared to the existing WSS flow. |
…ecret injection Address review feedback on the synchronous shell-exec endpoint: - Route the shell WebSocket through provider-proxy (PROVIDER_PROXY_URL) instead of a direct provider connection, so self-signed provider certs are validated by the proxy. Rewrite the receive parser to the real proxy frame protocol: message.message.data with LeaseShellCode markers (100 stdout / 101 stderr / 102 result / 103 failure), error frames handled before base64-decode, dual JSON / little-endian-int32 exit-code, 4001/4003 auth-expiry handling, and keep-reading-after-1MB-truncation. - Switch the command contract to argv (command: string[]); remove tokenizeCommand. - Add stdin-secret injection: optional `stdin` streamed as a 104 frame (+ EOF) so secrets never appear in the (logged) provider URL. - Residuals: dseq numeric regex, OpenAPI 500 response, provider lookup before JWT mint, typed test factories. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Thanks for the thorough review — pushed a revision ( What changed
66 unit tests pass; typecheck + lint clean. Per-point rationale (with pros/cons) is in the threads below. I review this repo asynchronously and can be slow to reply — please merge or push back as you see fit; none of the notes should block your review. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.ts (1)
242-242: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid passing nested objects as overrides to
mock<T>.Based on learnings for
vitest-mock-extended, passing nested objects (likeuser) as overrides during mock creation can cause them to be wrapped by recursive proxies, leading to unexpected behavior where properties resolve to mock functions instead ofundefined. Assign the property after creating the mock instead.♻️ Proposed refactor
- const authService = mock<AuthService>({ currentUser: user }); + const authService = mock<AuthService>(); + authService.currentUser = user;🤖 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/deployment/controllers/shell-exec/shell-exec.controller.spec.ts` at line 242, Update the authService setup in the relevant test to create the AuthService mock without the nested user override, then assign authService.currentUser = user afterward. Preserve the existing user fixture and all surrounding test behavior.Source: Learnings
🤖 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/deployment/controllers/shell-exec/shell-exec.controller.spec.ts`:
- Around line 19-55: Replace the type casts in the factory helpers createLease,
createDeployment, and createProviderInfo with vitest-mock-extended mock<Type>
calls while preserving their existing object fields and overrides; update the
resolved value at
apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.ts
lines 255-257 to use
mock<Awaited<ReturnType<WalletReaderService["getWalletByUserId"]>>> instead of
casting.
- Around line 127-137: Update the shell execution test around controller.exec
and the shellExecService.execute assertion to generate the stdin secret value
dynamically at runtime, then reuse that generated value in both places instead
of the hardcoded "SECRET=value" string. Preserve the existing assertion that the
service receives the generated stdin.
In `@apps/api/src/deployment/http-schemas/shell-exec.schema.ts`:
- Around line 13-17: The 1 MiB stdin and output limits currently count UTF-16
code units instead of UTF-8 bytes. In
apps/api/src/deployment/http-schemas/shell-exec.schema.ts:13-17, replace the
stdin length validation with a Buffer.byteLength(stdin, "utf8") check; in
apps/api/src/deployment/services/shell-exec/shell-exec.service.ts:151-163,
update output accumulation and limit checks to track UTF-8 byte counts rather
than string lengths.
---
Outside diff comments:
In
`@apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.ts`:
- Line 242: Update the authService setup in the relevant test to create the
AuthService mock without the nested user override, then assign
authService.currentUser = user afterward. Preserve the existing user fixture and
all surrounding test behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6616c43c-d4f7-443b-b4f8-3c45b7bd6e08
📒 Files selected for processing (6)
apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.spec.tsapps/api/src/deployment/controllers/shell-exec/shell-exec.controller.tsapps/api/src/deployment/http-schemas/shell-exec.schema.tsapps/api/src/deployment/routes/shell-exec/shell-exec.router.tsapps/api/src/deployment/services/shell-exec/shell-exec.service.spec.tsapps/api/src/deployment/services/shell-exec/shell-exec.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/deployment/routes/shell-exec/shell-exec.router.ts
- apps/api/src/deployment/controllers/shell-exec/shell-exec.controller.ts
…mocks - Enforce the 1 MiB stdin cap and output-truncation limit in UTF-8 bytes (Buffer.byteLength) instead of UTF-16 code units. - Use vitest-mock-extended mock<T>() in the controller spec instead of `as` casts. - Generate test secret values dynamically (no static secret strings in tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| timeout: z.number().int().min(1).max(120).default(60), | ||
| stdin: z | ||
| .string() | ||
| .refine(s => Buffer.byteLength(s, "utf8") <= 1_048_576, { message: "stdin must not exceed 1 MiB (UTF-8 bytes)" }) |
There was a problem hiding this comment.
issue(blocking): Each in-flight exec can hold up to ~1 MiB of stdout/stderr (MAX_OUTPUT_SIZE in shell-exec.service.ts) plus this stdin buffer, and nothing bounds concurrent execs — ~1000 concurrent requests ≈ 1.5 GB resident with no malicious payload.
None of the intended secret-injection payloads (env files, API keys, DB passwords, a TLS cert+key chain, service-mesh tokens) are anywhere near 1 MiB — they are kilobytes. Suggest capping stdin at 16 KiB and pulling anything larger from inside the container. Also set an explicit bodyLimit on the route to match, so the request body limit and this schema limit cannot drift.
There was a problem hiding this comment.
Done in 0157d43fc. stdin is now capped at 16 KiB (MAX_STDIN_BYTES) and the route carries an explicit bodyLimit (SHELL_EXEC_BODY_LIMIT_BYTES) that is derived arithmetically from the schema field caps (MAX_COMMAND_ARGS·MAX_COMMAND_ARG_BYTES + MAX_STDIN_BYTES + SERVICE_NAME_MAX + JSON_ENVELOPE_OVERHEAD_BYTES) — so the request-body limit and the schema caps import from one source of truth and can't drift.
command args are now bounded per-arg by UTF-8 byte length (64 × 1 KiB) rather than z.string().max() (which counts UTF-16 code units), so worst-case body size is finite. Combined with the output cap dropping to 64 KiB (see the other thread), steady-state per-exec resident memory is now ~kilobytes rather than ~1 MiB+.
| }; | ||
| } | ||
|
|
||
| function createService() { |
There was a problem hiding this comment.
suggestion: Per setup-function-instead-of-beforeeach, the object-under-test factory should be named setup(). Every other deployment service spec follows this convention.
There was a problem hiding this comment.
Done in 0157d43fc. Renamed the object-under-test factory createService() → setup() per the convention.
| const message = result.val.startsWith("Command timed out") | ||
| ? "Command execution timed out" | ||
| : result.val.startsWith("WebSocket connection failed") | ||
| ? "Failed to connect to provider" | ||
| : result.val.startsWith("Auth expired") | ||
| ? "Provider authentication expired" | ||
| : result.val.startsWith("Invalid provider host") | ||
| ? "Invalid provider host" | ||
| : result.val.startsWith("Provider error") | ||
| ? "Provider returned an error" | ||
| : "Shell execution failed"; | ||
| assert(false, 502, message); |
There was a problem hiding this comment.
suggestion: This 6-level nested ternary mapping error prefixes to messages/status is hard to read and extend. Prefer a small named helper (e.g. mapExecError(result.val) returning { status, message }) backed by a lookup table. It also reads as if all branches are 502, but the timeout case in particular should arguably be a 408/504 rather than 502 (see separate note). And auth expired should be 403. Invalid provider host = 400
There was a problem hiding this comment.
Done in 0157d43fc. The 6-level ternary is replaced with a pure mapExecError() helper backed by an ordered prefix→{ status, message } lookup table (unit-tested per prefix). Statuses:
- timeout → 504 (we're a gateway to the provider; 408 would say the client was slow to send its request, which isn't the case)
- auth expired → 403
- provider / ws-connection / closed-without-exit-code → 502, each with a distinct message so e.g. "Provider connection closed unexpectedly" is triageable separately from a generic provider error
On invalid provider host I went with 502 rather than 400: providerBaseUrl/hostUri is server-derived from on-chain provider data (providerService.getProvider(providerAddress).hostUri), not client input — so a malformed host is an upstream/config fault, and a 400 would misattribute blame to a caller who sent a perfectly valid request. It's a one-line change in the table if you'd still prefer 400 here.
The router OpenAPI now documents 403/504/413 as well.
| import { LoggerService } from "@src/core"; | ||
| import { DEPLOYMENT_CONFIG, type DeploymentConfig } from "@src/deployment/config/config.provider"; | ||
|
|
||
| const MAX_OUTPUT_SIZE = 1024 * 1024; |
There was a problem hiding this comment.
suggestion(blocking): set to 64Kb. In would be enough with a streaming solution. I'd like to avoid having big buffers in memory which can make DoS attack simple
There was a problem hiding this comment.
Done in 0157d43fc. MAX_OUTPUT_SIZE is now 64 KiB and exported so the tests assert against the constant instead of a hardcoded literal.
I kept the synchronous buffered response for this iteration since the buffer is now small enough that concurrent execs can't be used as a cheap DoS. I'm happy to follow up with an SSE/streaming variant as a separate, non-breaking change if you'd like incremental stdout/stderr for long-running commands — flagged it as a follow-up rather than folding a client-contract change into this PR.
…talniy review) Addresses @stalniy's CHANGES_REQUESTED on akash-network#3097. Plan converged via a 3-round multi-model quorum (unanimous APPROVE; full-convergence, dry improvement stream). Memory/DoS (both blocking comments): - MAX_OUTPUT_SIZE 1 MiB -> 64 KiB (exported); a synchronous buffered response is only safe while the buffer stays small. - stdin 1 MiB -> 16 KiB; command bounded per-arg by UTF-8 BYTES (not z.string().max, which counts UTF-16 code units) at 64 args x 1 KiB. - All request caps exported as named constants; SHELL_EXEC_BODY_LIMIT_BYTES is DERIVED arithmetically from them and wired into the route bodyLimit so the body limit and schema caps cannot drift. Error mapping: - Replace the 6-level nested ternary with a pure mapExecError() lookup table with correct statuses: timeout 504, auth-expired 403, invalid-provider-host 502 (hostUri is server-derived on-chain data, not client input, so 400 misattributes blame), provider/connection 502 with distinct messages. Router documents 403/504/413. Tests: - Rename service-spec factory createService() -> setup() per repo convention. - Truncation tests assert against the exported MAX_OUTPUT_SIZE. - New schema spec: stdin/command byte caps incl. multi-byte (emoji) case; body-limit derivation. - mapExecError contract test per error prefix -> status. 85 shell-exec unit tests pass; tsc (shell-exec) + eslint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@stalniy pushed a revision ( Memory / DoS (both blocking):
Error mapping:
Tests: factory Kept the synchronous buffered response this round (buffer is now small); SSE remains an easy non-breaking follow-up if you'd like incremental output. |
4 participants
See PR description at jobordu#1
Closes #3079
Summary by CodeRabbit
Release Notes
POST /v1/deployments/{dseq}/leases/{gseq}/{oseq}/shell-execendpoint to run commands on active deployment leases.wsand its TypeScript typings to the API package.