fix: runCommand returns correct success status and remove credentials…#156
Open
SARAMALI15792 wants to merge 1 commit intoFullAgent:mainfrom
Open
fix: runCommand returns correct success status and remove credentials…#156SARAMALI15792 wants to merge 1 commit intoFullAgent:mainfrom
SARAMALI15792 wants to merge 1 commit intoFullAgent:mainfrom
Conversation
… from WebSocket URL
- execCommand now returns { output, exitCode } instead of just output string
- runCommand checks exitCode and returns success:false when command fails
- Remove authorization credentials from WebSocket URL query params
- Credentials are already sent securely in WebSocket init message
❌ PR Check Results: FailedBuild Checks
|
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
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.
Bug Fixes — Command Failure Detection & Terminal Credential Security
This PR fixes two bugs discovered during a code review of the sandbox
execution and terminal connection layer.
Bug 1 — Commands Were Always Reported as Successful Even on Failure
How to Reproduce
exit 1success: truesuccess: false| Actual:success: trueRoot Cause
execCommandinlib/util/ttyd-exec.tswas silently discardingthe shell exit code and returning only the raw output text.
As a result,
runCommandinlib/actions/sandbox.tshad no wayto distinguish a successful command from a failed one — it blindly
returned
{ success: true }every single time.Fix
execCommandto return{ output, exitCode }insteadof just a plain string
runCommandexitCode !== 0→ returns{ success: false, error: "Command failed with exit code X" }exitCode === 0→ returns{ success: true, output }as intendedOutcome
Failed commands are now correctly reported as
success: false.AI agents and other callers can reliably detect failures and handle
them properly instead of silently building on a broken state.
Future Recommendation
Consider exposing the raw
exitCodevalue inside theExecResulttype so callers have full visibility into what the command returned,
not just a boolean success flag.
Bug 2 — Terminal Credentials Were Being Leaked Into Server Logs
How to Reproduce
ws?authorization=dXNlcjpwYXNzof the terminal session, visible in plain sight
Root Cause
The
buildWsUrlfunction inlib/util/ttyd-exec.tswas appendingthe
authorizationcredential as a URL query parameter on everyWebSocket connection. This meant the credential was being written
into every Kubernetes, nginx, and server access log entry on each
terminal session — a significant security exposure.
Fix
authorizationfrom the URL query params insidebuildWsUrlauthorizationfrom the final WebSocket URL insidexterm-terminal.tsxbefore the connection is establishedthe WebSocket init message — removing them from the URL does not
affect authentication or terminal functionality in any way
Outcome
Terminal credentials no longer appear in any server access logs.
The terminal connects and operates exactly as before — the only
difference is that sensitive credentials are no longer exposed.
Future Recommendation
Consider storing the ttyd URL in the database without the
authorizationquery param embedded inside it. Keeping credentialscompletely separate from URLs at the storage level would eliminate
this class of issue entirely going forward.