Single Session Mode #1634
Single Session Mode #1634
Conversation
This adds a shellexec command for launching connections that only require one remote session.
This adds the code that will call the shellexec code. It also disables the existing connserver/install code for single shell mode.
WalkthroughThe pull request introduces a new configuration option for managing connection sessions across multiple files in the project. An optional boolean property Additionally, a new boolean variable Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/shellexec/shellexec.go (1)
10-10: Consider using text/template if HTML-specific features are unnecessary
Since there's no HTML sanitization or escaping in use,text/templatemight suffice and be clearer.pkg/blockcontroller/blockcontroller.go (2)
319-325: Variable naming clarity
singleSessionis fine, but consider renaming it to something likeuseSingleSessionto be more self-descriptive.
350-379: Follow up on TODO for domain socket approach
The comment indicates future domain socket signaling. Either implement the plan or remove the comment if it’s no longer necessary.pkg/remote/conncontroller/conncontroller.go (1)
514-514: Add a quick doc comment
Explain whysingleSessionis needed so future maintainers can easily see its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/types/gotypes.d.ts(1 hunks)pkg/blockcontroller/blockcontroller.go(2 hunks)pkg/remote/conncontroller/conncontroller.go(2 hunks)pkg/shellexec/shellexec.go(2 hunks)pkg/wshrpc/wshrpctypes.go(1 hunks)
🔇 Additional comments (4)
pkg/shellexec/shellexec.go (1)
241-253: Sanitize any user-supplied inputs in singleSessionCmdTemplate
If external input is inserted into this template, ensure it’s properly escaped or validated to avoid command injection.
pkg/remote/conncontroller/conncontroller.go (1)
524-526: Check thread-safety on connSettings usage
If other goroutines can modify connSettings, ensure consistent locking or concurrency control.
pkg/wshrpc/wshrpctypes.go (1)
465-465: Clear naming
ConnSingleSession accurately conveys the single-session concept. This addition looks consistent with the rest of the code.
frontend/types/gotypes.d.ts (1)
294-294: Consistent typing with backend
Adding "conn:singlesession"?: boolean; ensures the front-end matches the new backend field. This looks correct.
| func StartSingleSessionRemoteShellProc(termSize waveobj.TermSize, conn *conncontroller.SSHConn) (*ShellProc, error) { | ||
| client := conn.GetClient() | ||
| session, err := client.NewSession() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| remoteStdinRead, remoteStdinWriteOurs, err := os.Pipe() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| remoteStdoutReadOurs, remoteStdoutWrite, err := os.Pipe() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| pipePty := &PipePty{ | ||
| remoteStdinWrite: remoteStdinWriteOurs, | ||
| remoteStdoutRead: remoteStdoutReadOurs, | ||
| } | ||
| if termSize.Rows == 0 || termSize.Cols == 0 { | ||
| termSize.Rows = shellutil.DefaultTermRows | ||
| termSize.Cols = shellutil.DefaultTermCols | ||
| } | ||
| if termSize.Rows <= 0 || termSize.Cols <= 0 { | ||
| return nil, fmt.Errorf("invalid term size: %v", termSize) | ||
| } | ||
| session.Stdin = remoteStdinRead | ||
| session.Stdout = remoteStdoutWrite | ||
| session.Stderr = remoteStdoutWrite | ||
|
|
||
| session.RequestPty("xterm-256color", termSize.Rows, termSize.Cols, nil) | ||
|
|
||
| wshDir := "~/.waveterm/bin" | ||
| wshPath := wshDir + "/wsh" | ||
| var installWords = map[string]string{ | ||
| "wshPath": wshPath, | ||
| "tempPath": wshPath + ".temp", | ||
| } | ||
|
|
||
| //todo add code that allows streaming base64 for download | ||
|
|
||
| singleSessionCmd := &bytes.Buffer{} | ||
| cmdTemplate := template.Must(template.New("").Parse(singleSessionCmdTemplate)) | ||
| cmdTemplate.Execute(singleSessionCmd, installWords) | ||
|
|
||
| log.Printf("full single session command is: %s", singleSessionCmd) | ||
| sessionWrap := MakeSessionWrap(session, singleSessionCmd.String(), pipePty) | ||
|
|
||
| err = sessionWrap.Start() | ||
| if err != nil { | ||
| pipePty.Close() | ||
| return nil, err | ||
| } | ||
| return &ShellProc{Cmd: sessionWrap, ConnName: conn.GetName(), CloseOnce: &sync.Once{}, DoneCh: make(chan any)}, nil | ||
| } |
There was a problem hiding this comment.
Check the error from session.RequestPty
The call to session.RequestPty might fail, leaving the session in a partial state. Handling this error helps identify pty allocation issues.
- session.RequestPty("xterm-256color", termSize.Rows, termSize.Cols, nil)
+if errPty := session.RequestPty("xterm-256color", termSize.Rows, termSize.Cols, nil); errPty != nil {
+ pipePty.Close()
+ session.Close()
+ return nil, fmt.Errorf("failed to request pty: %w", errPty)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func StartSingleSessionRemoteShellProc(termSize waveobj.TermSize, conn *conncontroller.SSHConn) (*ShellProc, error) { | |
| client := conn.GetClient() | |
| session, err := client.NewSession() | |
| if err != nil { | |
| return nil, err | |
| } | |
| remoteStdinRead, remoteStdinWriteOurs, err := os.Pipe() | |
| if err != nil { | |
| return nil, err | |
| } | |
| remoteStdoutReadOurs, remoteStdoutWrite, err := os.Pipe() | |
| if err != nil { | |
| return nil, err | |
| } | |
| pipePty := &PipePty{ | |
| remoteStdinWrite: remoteStdinWriteOurs, | |
| remoteStdoutRead: remoteStdoutReadOurs, | |
| } | |
| if termSize.Rows == 0 || termSize.Cols == 0 { | |
| termSize.Rows = shellutil.DefaultTermRows | |
| termSize.Cols = shellutil.DefaultTermCols | |
| } | |
| if termSize.Rows <= 0 || termSize.Cols <= 0 { | |
| return nil, fmt.Errorf("invalid term size: %v", termSize) | |
| } | |
| session.Stdin = remoteStdinRead | |
| session.Stdout = remoteStdoutWrite | |
| session.Stderr = remoteStdoutWrite | |
| session.RequestPty("xterm-256color", termSize.Rows, termSize.Cols, nil) | |
| wshDir := "~/.waveterm/bin" | |
| wshPath := wshDir + "/wsh" | |
| var installWords = map[string]string{ | |
| "wshPath": wshPath, | |
| "tempPath": wshPath + ".temp", | |
| } | |
| //todo add code that allows streaming base64 for download | |
| singleSessionCmd := &bytes.Buffer{} | |
| cmdTemplate := template.Must(template.New("").Parse(singleSessionCmdTemplate)) | |
| cmdTemplate.Execute(singleSessionCmd, installWords) | |
| log.Printf("full single session command is: %s", singleSessionCmd) | |
| sessionWrap := MakeSessionWrap(session, singleSessionCmd.String(), pipePty) | |
| err = sessionWrap.Start() | |
| if err != nil { | |
| pipePty.Close() | |
| return nil, err | |
| } | |
| return &ShellProc{Cmd: sessionWrap, ConnName: conn.GetName(), CloseOnce: &sync.Once{}, DoneCh: make(chan any)}, nil | |
| } | |
| func StartSingleSessionRemoteShellProc(termSize waveobj.TermSize, conn *conncontroller.SSHConn) (*ShellProc, error) { | |
| client := conn.GetClient() | |
| session, err := client.NewSession() | |
| if err != nil { | |
| return nil, err | |
| } | |
| remoteStdinRead, remoteStdinWriteOurs, err := os.Pipe() | |
| if err != nil { | |
| return nil, err | |
| } | |
| remoteStdoutReadOurs, remoteStdoutWrite, err := os.Pipe() | |
| if err != nil { | |
| return nil, err | |
| } | |
| pipePty := &PipePty{ | |
| remoteStdinWrite: remoteStdinWriteOurs, | |
| remoteStdoutRead: remoteStdoutReadOurs, | |
| } | |
| if termSize.Rows == 0 || termSize.Cols == 0 { | |
| termSize.Rows = shellutil.DefaultTermRows | |
| termSize.Cols = shellutil.DefaultTermCols | |
| } | |
| if termSize.Rows <= 0 || termSize.Cols <= 0 { | |
| return nil, fmt.Errorf("invalid term size: %v", termSize) | |
| } | |
| session.Stdin = remoteStdinRead | |
| session.Stdout = remoteStdoutWrite | |
| session.Stderr = remoteStdoutWrite | |
| if errPty := session.RequestPty("xterm-256color", termSize.Rows, termSize.Cols, nil); errPty != nil { | |
| pipePty.Close() | |
| session.Close() | |
| return nil, fmt.Errorf("failed to request pty: %w", errPty) | |
| } | |
| wshDir := "~/.waveterm/bin" | |
| wshPath := wshDir + "/wsh" | |
| var installWords = map[string]string{ | |
| "wshPath": wshPath, | |
| "tempPath": wshPath + ".temp", | |
| } | |
| //todo add code that allows streaming base64 for download | |
| singleSessionCmd := &bytes.Buffer{} | |
| cmdTemplate := template.Must(template.New("").Parse(singleSessionCmdTemplate)) | |
| cmdTemplate.Execute(singleSessionCmd, installWords) | |
| log.Printf("full single session command is: %s", singleSessionCmd) | |
| sessionWrap := MakeSessionWrap(session, singleSessionCmd.String(), pipePty) | |
| err = sessionWrap.Start() | |
| if err != nil { | |
| pipePty.Close() | |
| return nil, err | |
| } | |
| return &ShellProc{Cmd: sessionWrap, ConnName: conn.GetName(), CloseOnce: &sync.Once{}, DoneCh: make(chan any)}, nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/shellexec/shellexec.go (1)
299-299: Implement the base64 streaming functionality.The TODO comment indicates missing implementation for streaming base64 data.
Would you like me to help implement the base64 streaming functionality for downloading the binary?
pkg/blockcontroller/blockcontroller.go (1)
374-379: Implement connection server and domain socket handling.The TODO comments indicate missing functionality for:
- Receiving signals from the process when domain socket is opened
- Forwarding the unix domain socket
- Setting the wsh boolean flag
Consider implementing a simple protocol for domain socket readiness:
- Process writes a special marker to stdout when socket is ready
- Parent process monitors stdout for this marker
- Upon marker detection, proceed with socket forwarding
Would you like me to provide a detailed implementation plan?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/types/gotypes.d.ts(1 hunks)pkg/blockcontroller/blockcontroller.go(2 hunks)pkg/remote/conncontroller/conncontroller.go(2 hunks)pkg/shellexec/shellexec.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/types/gotypes.d.ts
🔇 Additional comments (3)
pkg/shellexec/shellexec.go (1)
290-290: Handle errors from session.RequestPty.The call to
session.RequestPtymight fail, leaving the session in a partial state.Apply this fix:
-session.RequestPty("xterm-256color", termSize.Rows, termSize.Cols, nil) +if err := session.RequestPty("xterm-256color", termSize.Rows, termSize.Cols, nil); err != nil { + pipePty.Close() + session.Close() + return nil, fmt.Errorf("failed to request pty: %w", err) +}pkg/remote/conncontroller/conncontroller.go (1)
Line range hint
520-532: LGTM! Clean integration of single session mode.The implementation correctly:
- Reads singleSession from connection settings
- Integrates with existing wsh installation logic
- Maintains backward compatibility
pkg/blockcontroller/blockcontroller.go (1)
350-373: LGTM! Clean implementation of single session shell process.The code properly:
- Handles credentials and connection setup
- Validates connection status
- Sets up JWT token for authentication
- Launches single session shell process
| var singleSessionCmdTemplate = `bash -c ' \ | ||
| {{.wshPath}} connserver --single || \ | ||
| ( uname -m && \ | ||
| read -r count && | ||
| for ((i=0; i<count; i++)); do \ | ||
| read -r line || break; \ | ||
| printf "%%b" "$line"; \ | ||
| done > {{.tempPath}} && \ | ||
| chmod a+x {{.tempPath}} && \ | ||
| mv {{.tempPath}} {{.wshPath}} && | ||
| {{.wshPath}} connserver --single \ | ||
| ) | ||
| ` |
There was a problem hiding this comment.
Add error handling and secure file permissions.
The command template has several potential issues:
- No error handling for file operations (read/write/chmod/mv)
- Using
chmod a+xgrants execute permission to all users, which is overly permissive - No cleanup of temporary file on failure
Consider these improvements:
var singleSessionCmdTemplate = `bash -c ' \
{{.wshPath}} connserver --single || \
( uname -m && \
read -r count &&
for ((i=0; i<count; i++)); do \
read -r line || break; \
printf "%%b" "$line"; \
done > {{.tempPath}} && \
- chmod a+x {{.tempPath}} && \
+ chmod 0700 {{.tempPath}} && \
mv {{.tempPath}} {{.wshPath}} &&
{{.wshPath}} connserver --single \
) || { rm -f {{.tempPath}}; exit 1; }
'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var singleSessionCmdTemplate = `bash -c ' \ | |
| {{.wshPath}} connserver --single || \ | |
| ( uname -m && \ | |
| read -r count && | |
| for ((i=0; i<count; i++)); do \ | |
| read -r line || break; \ | |
| printf "%%b" "$line"; \ | |
| done > {{.tempPath}} && \ | |
| chmod a+x {{.tempPath}} && \ | |
| mv {{.tempPath}} {{.wshPath}} && | |
| {{.wshPath}} connserver --single \ | |
| ) | |
| ` | |
| var singleSessionCmdTemplate = `bash -c ' \ | |
| {{.wshPath}} connserver --single || \ | |
| ( uname -m && \ | |
| read -r count && | |
| for ((i=0; i<count; i++)); do \ | |
| read -r line || break; \ | |
| printf "%%b" "$line"; \ | |
| done > {{.tempPath}} && \ | |
| chmod 0700 {{.tempPath}} && \ | |
| mv {{.tempPath}} {{.wshPath}} && | |
| {{.wshPath}} connserver --single \ | |
| ) || { rm -f {{.tempPath}}; exit 1; } | |
| ' |
| wshDir := "~/.waveterm/bin" | ||
| wshPath := wshDir + "/wsh" | ||
| var installWords = map[string]string{ | ||
| "wshPath": wshPath, | ||
| "tempPath": wshPath + ".temp", | ||
| } |
There was a problem hiding this comment.
Validate directory existence before file operations.
The code assumes the ~/.waveterm/bin directory exists without validation.
Add directory validation and creation:
wshDir := "~/.waveterm/bin"
+if err := os.MkdirAll(wshDir, 0700); err != nil {
+ return nil, fmt.Errorf("failed to create wsh directory: %w", err)
+}
wshPath := wshDir + "/wsh"Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/remote/conncontroller/conncontroller.go (1)
Line range hint
515-527: LGTM! Consider adding documentation.The implementation correctly reads the single session mode configuration and modifies the WSH installation behavior accordingly. This change aligns with the PR objective of enabling connections that can spawn multiple sessions from a single remote session.
Consider adding a comment explaining the purpose of the
singleSessionflag and its implications for WSH installation, particularly in ForceCommand scenarios.pkg/blockcontroller/blockcontroller.go (1)
381-386: Implementation needed for domain socket handling.The TODO comments indicate several critical pieces of functionality are missing:
- Signal handling for domain socket creation
- Unix domain socket forwarding
- WSH boolean state management
These are essential for proper single session mode operation.
Would you like me to help implement these missing components or create GitHub issues to track them?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/blockcontroller/blockcontroller.go(2 hunks)pkg/remote/conncontroller/conncontroller.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Analyze (javascript-typescript)
pkg/blockcontroller/blockcontroller.go
[failure] 363-363363:
not enough return values
[failure] 368-368368:
not enough return values
[failure] 373-373373:
not enough return values
[failure] 379-379379:
not enough return values
🪛 GitHub Check: Analyze (go)
pkg/blockcontroller/blockcontroller.go
[failure] 363-363363:
not enough return values
[failure] 368-368368:
not enough return values
[failure] 373-373373:
not enough return values
[failure] 379-379379:
not enough return values
🪛 GitHub Actions: CodeQL
pkg/blockcontroller/blockcontroller.go
[error] 363-363363: not enough return values
🔇 Additional comments (2)
pkg/remote/conncontroller/conncontroller.go (1)
527-527: LGTM! Condition handles WSH installation correctly.The condition
enableWsh && !singleSessionensures WSH is only installed when appropriate, preventing interference with ForceCommand scenarios.pkg/blockcontroller/blockcontroller.go (1)
327-332: LGTM! Configuration retrieval is implemented correctly.The code properly retrieves the single session mode configuration from connection settings.
| } else if remoteName != "" && singleSession { | ||
| credentialCtx, cancelFunc := context.WithTimeout(context.Background(), 60*time.Second) | ||
| defer cancelFunc() | ||
|
|
||
| opts, err := remote.ParseOpts(remoteName) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| conn := conncontroller.GetConn(credentialCtx, opts, false, &wshrpc.ConnKeywords{}) | ||
| connStatus := conn.DeriveConnStatus() | ||
| if connStatus.Status != conncontroller.Status_Connected { | ||
| return fmt.Errorf("not connected, cannot start shellproc") | ||
| } | ||
| if !blockMeta.GetBool(waveobj.MetaKey_CmdNoWsh, false) { | ||
| jwtStr, err := wshutil.MakeClientJWTToken(wshrpc.RpcContext{TabId: bc.TabId, BlockId: bc.BlockId, Conn: conn.Opts.String()}, conn.GetDomainSocketName()) | ||
| if err != nil { | ||
| return fmt.Errorf("error making jwt token: %w", err) | ||
| } | ||
| cmdOpts.Env[wshutil.WaveJwtTokenVarName] = jwtStr | ||
| } | ||
| shellProc, err = shellexec.StartSingleSessionRemoteShellProc(rc.TermSize, conn) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Fix return values in single session handling.
The function setupAndStartShellProcess expects a return type of (*shellexec.ShellProc, error), but several return statements only return error. This will cause runtime issues.
Apply this fix to the return statements:
- return err
+ return nil, err
- return fmt.Errorf("not connected, cannot start shellproc")
+ return nil, fmt.Errorf("not connected, cannot start shellproc")
- return fmt.Errorf("error making jwt token: %w", err)
+ return nil, fmt.Errorf("error making jwt token: %w", err)
- return err
+ return nil, err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if remoteName != "" && singleSession { | |
| credentialCtx, cancelFunc := context.WithTimeout(context.Background(), 60*time.Second) | |
| defer cancelFunc() | |
| opts, err := remote.ParseOpts(remoteName) | |
| if err != nil { | |
| return err | |
| } | |
| conn := conncontroller.GetConn(credentialCtx, opts, false, &wshrpc.ConnKeywords{}) | |
| connStatus := conn.DeriveConnStatus() | |
| if connStatus.Status != conncontroller.Status_Connected { | |
| return fmt.Errorf("not connected, cannot start shellproc") | |
| } | |
| if !blockMeta.GetBool(waveobj.MetaKey_CmdNoWsh, false) { | |
| jwtStr, err := wshutil.MakeClientJWTToken(wshrpc.RpcContext{TabId: bc.TabId, BlockId: bc.BlockId, Conn: conn.Opts.String()}, conn.GetDomainSocketName()) | |
| if err != nil { | |
| return fmt.Errorf("error making jwt token: %w", err) | |
| } | |
| cmdOpts.Env[wshutil.WaveJwtTokenVarName] = jwtStr | |
| } | |
| shellProc, err = shellexec.StartSingleSessionRemoteShellProc(rc.TermSize, conn) | |
| if err != nil { | |
| return err | |
| } | |
| } else if remoteName != "" && singleSession { | |
| credentialCtx, cancelFunc := context.WithTimeout(context.Background(), 60*time.Second) | |
| defer cancelFunc() | |
| opts, err := remote.ParseOpts(remoteName) | |
| if err != nil { | |
| return nil, err | |
| } | |
| conn := conncontroller.GetConn(credentialCtx, opts, false, &wshrpc.ConnKeywords{}) | |
| connStatus := conn.DeriveConnStatus() | |
| if connStatus.Status != conncontroller.Status_Connected { | |
| return nil, fmt.Errorf("not connected, cannot start shellproc") | |
| } | |
| if !blockMeta.GetBool(waveobj.MetaKey_CmdNoWsh, false) { | |
| jwtStr, err := wshutil.MakeClientJWTToken(wshrpc.RpcContext{TabId: bc.TabId, BlockId: bc.BlockId, Conn: conn.Opts.String()}, conn.GetDomainSocketName()) | |
| if err != nil { | |
| return nil, fmt.Errorf("error making jwt token: %w", err) | |
| } | |
| cmdOpts.Env[wshutil.WaveJwtTokenVarName] = jwtStr | |
| } | |
| shellProc, err = shellexec.StartSingleSessionRemoteShellProc(rc.TermSize, conn) | |
| if err != nil { | |
| return nil, err | |
| } |
🧰 Tools
🪛 GitHub Check: Analyze (javascript-typescript)
[failure] 363-363363:
not enough return values
[failure] 368-368368:
not enough return values
[failure] 373-373373:
not enough return values
[failure] 379-379379:
not enough return values
🪛 GitHub Check: Analyze (go)
[failure] 363-363363:
not enough return values
[failure] 368-368368:
not enough return values
[failure] 373-373373:
not enough return values
[failure] 379-379379:
not enough return values
🪛 GitHub Actions: CodeQL
[error] 363-363363: not enough return values
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/blockcontroller/blockcontroller.go(2 hunks)
🔇 Additional comments (2)
pkg/blockcontroller/blockcontroller.go (2)
327-332: LGTM! Clean implementation of single session configuration.The code properly retrieves and handles the single session mode configuration from the connection settings.
381-386: Verify WSH functionality in single session mode.The comments indicate that the connection server is disabled for single session connections, which might affect WSH (Wave Shell) functionality. This needs verification to ensure it doesn't break existing features.
Run the following script to check WSH usage in single session mode:
✅ Verification successful
Based on the search results, I can now provide a comprehensive verification of the WSH functionality in single session mode:
WSH functionality is properly handled in single session mode
The codebase shows that WSH (Wave Shell) functionality is correctly managed for single session connections through several key mechanisms:
- The connection's WSH state is properly tracked via atomic boolean flags and respected throughout the codebase
- The system gracefully falls back to non-WSH mode when needed:
if !conn.WshEnabled.Load() { shellProc, err = shellexec.StartRemoteShellProcNoWsh(rc.TermSize, cmdStr, cmdOpts, conn) }- WSH enablement is configurable both globally and per-connection:
enableWsh := config.Settings.ConnWshEnabled if connSettings.ConnWshEnabled != nil { enableWsh = *connSettings.ConnWshEnabled }- The JWT token and domain socket handling required for WSH communication is properly implemented for both single session and regular modes
The commented-out code in the review is part of a planned enhancement but doesn't affect the current functionality, as the system already handles both WSH and non-WSH modes correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check WSH functionality usage in single session mode # Test: Search for WSH-related function calls and verify their compatibility # Expect: Functions that might need adaptation for single session mode # Check WSH-related function calls rg -A 5 "WshEnabled|StartRemoteShellProc.*wsh.*|wshutil\." # Check for potential WSH dependencies ast-grep --pattern 'if $_.WshEnabled.Load() { $$$ }'Length of output: 68367
| } else if remoteName != "" && singleSession { | ||
| credentialCtx, cancelFunc := context.WithTimeout(context.Background(), 60*time.Second) | ||
| defer cancelFunc() | ||
|
|
||
| opts, err := remote.ParseOpts(remoteName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| conn := conncontroller.GetConn(credentialCtx, opts, false, &wshrpc.ConnKeywords{}) | ||
| connStatus := conn.DeriveConnStatus() | ||
| if connStatus.Status != conncontroller.Status_Connected { | ||
| return nil, fmt.Errorf("not connected, cannot start shellproc") | ||
| } | ||
| if !blockMeta.GetBool(waveobj.MetaKey_CmdNoWsh, false) { | ||
| jwtStr, err := wshutil.MakeClientJWTToken(wshrpc.RpcContext{TabId: bc.TabId, BlockId: bc.BlockId, Conn: conn.Opts.String()}, conn.GetDomainSocketName()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error making jwt token: %w", err) | ||
| } | ||
| cmdOpts.Env[wshutil.WaveJwtTokenVarName] = jwtStr | ||
| } | ||
| shellProc, err = shellexec.StartSingleSessionRemoteShellProc(rc.TermSize, conn) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // todo | ||
| // i have disabled the conn server for this type of connection | ||
| // this means we need to receive a signal from the process once it has | ||
| // opened a domain socket. then, once that is done, we can forward the | ||
| // unix domain socket here. also, we need to set the wsh boolean true as | ||
| // is done in the current connserver implementation |
There was a problem hiding this comment.
Add error handling for domain socket setup.
The current implementation lacks proper error handling for the case where StartSingleSessionRemoteShellProc succeeds but the subsequent domain socket setup fails. This could lead to resource leaks or hanging connections.
Consider adding cleanup code and proper error propagation:
shellProc, err = shellexec.StartSingleSessionRemoteShellProc(rc.TermSize, conn)
if err != nil {
return nil, err
}
+ // Wait for domain socket setup with timeout
+ socketSetupCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ select {
+ case <-socketSetupCtx.Done():
+ shellProc.Close()
+ return nil, fmt.Errorf("timeout waiting for domain socket setup")
+ case err := <-shellProc.SocketSetupErrCh:
+ if err != nil {
+ shellProc.Close()
+ return nil, fmt.Errorf("domain socket setup failed: %w", err)
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
Allows connections to spawn multiple sessions from only one remote session. Useful specifically in situations where sshd
ForceCommandis being used for 2-factor authentication. The main idea is that a single connection session is made with a command containing the wsh connserver. Then, the connserver is able to spawn local sessions that can be routed through the connserver instead of through individual shells.