conn updates 5 #1755
conn updates 5 #1755
Conversation
sawka
commented
Jan 16, 2025
sawka
commented
- token swap
- setting environment variables for different local/remote shells
- bug fixes for init scripts
- more logging
- update connserver startup flow
…ing. run initrc files on connserver startup. use exec for running connserver. more logging, pass logCtx through more blockcontroller funcs. setup swaptoken variables
…ng on new authentication, write jwt directly to connserver
…SHFORCEUPDATE, improve/fix conn logging
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces a comprehensive set of changes focused on enhancing token-based authentication and shell initialization across multiple components of the system. The modifications span several packages and files, introducing new methods for JWT token management, shell environment setup, and RPC authentication. Key changes include the addition of a new The implementation adds support for generating and managing swap tokens, with new utility functions for packing, unpacking, and encoding tokens. The changes also improve logging and context management, particularly in shell execution and connection server operations. Overall, the modifications represent a significant enhancement to the system's authentication and shell initialization infrastructure, providing more robust and flexible mechanisms for managing user sessions and environment setup. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
Finishing Touches
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: 9
🔭 Outside diff range comments (2)
pkg/util/shellutil/tokenswap.go (1)
Line range hint
156-156: Avoid logging sensitive information.Logging the token value can lead to security vulnerabilities if logs are exposed. It's best to avoid logging sensitive data such as tokens.
Apply this diff to remove the sensitive log:
- log.Printf("# sending authenticate token command, token: %s\n", token.Token) + log.Println("# sending authenticate token command")pkg/util/shellutil/shellutil.go (1)
Line range hint
110-131: Improve error handling and PATH manipulation in Bash script.Similar issues to the Zsh script:
- PATH manipulation is duplicated.
- Error redirection to /dev/null might hide important errors.
Consider this improved implementation:
WAVETERM_WSHBINDIR={{.WSHBINDIR}} -# after /etc/profile which is likely to clobber the path -export PATH="$WAVETERM_WSHBINDIR:$PATH" +# add wsh to path if not already present +if [[ ":$PATH:" != *":$WAVETERM_WSHBINDIR:"* ]]; then + export PATH="$WAVETERM_WSHBINDIR:$PATH" +fi -# Source the dynamic script from wsh token -eval "$(wsh token "$WAVETERM_SWAPTOKEN" bash 2> /dev/null)" +# source dynamic script from wsh token +if ! output=$(wsh token "$WAVETERM_SWAPTOKEN" bash 2>&1); then + echo "Warning: Failed to source token script: $output" >&2 +else + eval "$output" +fi unset WAVETERM_SWAPTOKEN # Source the first of ~/.bash_profile, ~/.bash_login, or ~/.profile that exists if [ -f ~/.bash_profile ]; then . ~/.bash_profile elif [ -f ~/.bash_login ]; then . ~/.bash_login elif [ -f ~/.profile ]; then . ~/.profile fi -if [[ ":$PATH:" != *":$WAVETERM_WSHBINDIR:"* ]]; then - export PATH="$WAVETERM_WSHBINDIR:$PATH" -fi unset WAVETERM_WSHBINDIR
🧹 Nitpick comments (11)
pkg/util/ds/syncmap.go (1)
Line range hint
13-17: Consider using RWMutex for better read performance.The current implementation uses a regular
sync.Mutex, but sinceGetandGetExare read-only operations, usingsync.RWMutexcould improve performance by allowing concurrent reads.type SyncMap[T any] struct { - lock *sync.Mutex + lock *sync.RWMutex m map[string]T } func MakeSyncMap[T any]() *SyncMap[T] { return &SyncMap[T]{ - lock: &sync.Mutex{}, + lock: &sync.RWMutex{}, m: make(map[string]T), } } func (sm *SyncMap[T]) Get(key string) T { - sm.lock.Lock() - defer sm.lock.Unlock() + sm.lock.RLock() + defer sm.lock.RUnlock() return sm.m[key] } func (sm *SyncMap[T]) GetEx(key string) (T, bool) { - sm.lock.Lock() - defer sm.lock.Unlock() + sm.lock.RLock() + defer sm.lock.RUnlock() v, ok := sm.m[key] return v, ok }pkg/util/ds/syncmap_test.go (1)
8-8: Enhance test coverage with concurrent access tests.While the basic functionality is well tested, consider adding tests for:
- Concurrent access to verify thread safety
- Zero value behavior for different types
- Edge cases like nil maps
Here's a sample concurrent test to add:
func TestSyncMap_ConcurrentAccess(t *testing.T) { sm := MakeSyncMap[int]() const goroutines = 10 const iterations = 1000 var wg sync.WaitGroup wg.Add(goroutines * 2) // readers + writers // Writers for i := 0; i < goroutines; i++ { go func(id int) { defer wg.Done() for j := 0; j < iterations; j++ { key := fmt.Sprintf("key%d-%d", id, j) sm.Set(key, j) } }(i) } // Readers for i := 0; i < goroutines; i++ { go func(id int) { defer wg.Done() for j := 0; j < iterations; j++ { key := fmt.Sprintf("key%d-%d", id, j) sm.Get(key) // No panic should occur } }(i) } wg.Wait() }Also applies to: 16-16, 27-27, 40-40
pkg/util/shellutil/tokenswap.go (1)
67-76: Consider locking before removing expired tokens for thread safety.While
removeExpiredTokenshandles locking internally, calling it before acquiring the lock inAddTokenSwapEntryandGetAndRemoveTokenSwapEntrymight lead to race conditions iftokenSwapMapis accessed concurrently elsewhere.Apply this diff to call
removeExpiredTokenswithin the locked section:func AddTokenSwapEntry(entry *TokenSwapEntry) error { - removeExpiredTokens() if entry.Token == "" { return fmt.Errorf("token cannot be empty") } tokenMapLock.Lock() defer tokenMapLock.Unlock() + removeExpiredTokens() if _, ok := tokenSwapMap[entry.Token]; ok { return fmt.Errorf("token already exists: %s", entry.Token) } tokenSwapMap[entry.Token] = entry return nil } func GetAndRemoveTokenSwapEntry(token string) *TokenSwapEntry { - removeExpiredTokens() tokenMapLock.Lock() defer tokenMapLock.Unlock() + removeExpiredTokens() if entry, ok := tokenSwapMap[token]; ok { delete(tokenSwapMap, token) return entry } return nil }cmd/wsh/cmd/wshcmd-root.go (1)
86-93: Handle potential errors fromsetupRpcClient.Currently, the error returned by
setupRpcClientis checked, but additional context in the error messages can improve debugging.Enhance the error handling:
if err != nil { - return err + return fmt.Errorf("setupRpcClient failed: %w", err) }pkg/wshutil/wshproxy.go (2)
228-248: Consolidate error responses for unauthenticated connections.Repeatedly sending error responses for unauthenticated connections can be streamlined to improve code readability.
Consider refactoring the error handling:
else { - respErr := fmt.Errorf("connection not authenticated") - p.sendResponseError(msg, respErr) - continue + p.sendResponseError(msg, fmt.Errorf("connection not authenticated")) }
105-122: SimplifyvalidateRpcContextFromAuthreturn values.To enhance code clarity, return early when encountering errors and avoid redundant else statements.
Refactor the function for better readability:
func validateRpcContextFromAuth(newCtx *wshrpc.RpcContext) (string, error) { if newCtx == nil { return "", fmt.Errorf("no context found in jwt token") } if newCtx.BlockId == "" && newCtx.Conn == "" { return "", fmt.Errorf("no blockid or conn found in jwt token") } if newCtx.BlockId != "" { if _, err := uuid.Parse(newCtx.BlockId); err != nil { return "", fmt.Errorf("invalid blockId in jwt token") } } - } routeId, err := MakeRouteIdFromCtx(newCtx) if err != nil { return "", fmt.Errorf("error making routeId from context: %w", err) } return routeId, nil }pkg/blockcontroller/blockcontroller.go (2)
286-317: Handle errors more robustly inmakeSwapTokenWhile creating the swap token, errors from database lookups (e.g., finding tab, workspace, or client data) are logged but not handled. Consider propagating these errors or adding fallback logic to ensure that the token contains all necessary environment variables.
Apply this diff to handle errors:
tabId, err := wstore.DBFindTabForBlockId(ctx, bc.BlockId) if err != nil { - log.Printf("error finding tab for block: %v\n", err) + return nil, fmt.Errorf("error finding tab for block: %w", err) } else { token.Env["WAVETERM_TABID"] = tabId }
866-869: Recommend structured logging for better log managementThe
debugLogfunction logs messages usinglog.Printf, which might not include contextual details. Consider using a structured logging framework or enhancing the existing logger to include additional context for better log management and readability.pkg/remote/conncontroller/conncontroller.go (1)
236-238: UnusedlogCtxparameter inIsWshVersionUpToDateThe
logCtxparameter is added toIsWshVersionUpToDatebut isn't used within the function. Remove the unused parameter or utilize it for logging to maintain code cleanliness.Apply this diff to remove the unused parameter:
-func IsWshVersionUpToDate(logCtx context.Context, wshVersionLine string) (bool, string, string, error) { +func IsWshVersionUpToDate(wshVersionLine string) (bool, string, string, error) {cmd/wsh/cmd/wshcmd-connserver.go (1)
253-260: Consider deferring RC file installation.The RC file installation error is logged but not propagated, which might lead to silent failures. Consider moving this operation to after token validation to ensure proper authentication before making file system changes.
pkg/wshrpc/wshserver/wshserver.go (1)
Line range hint
728-728: Track TODO comments for pending work.There are two TODO items that need to be addressed:
- Add user input code for validation
- Add code for modifying configs
Would you like me to create GitHub issues to track these tasks?
Also applies to: 737-737
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
cmd/wsh/cmd/wshcmd-connserver.go(7 hunks)cmd/wsh/cmd/wshcmd-root.go(3 hunks)cmd/wsh/cmd/wshcmd-token.go(1 hunks)frontend/app/store/wshclientapi.ts(1 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/blockcontroller/blockcontroller.go(12 hunks)pkg/remote/conncontroller/conncontroller.go(7 hunks)pkg/remote/conncontroller/tokenswap.go(0 hunks)pkg/shellexec/shellexec.go(10 hunks)pkg/util/ds/syncmap.go(1 hunks)pkg/util/ds/syncmap_test.go(3 hunks)pkg/util/shellutil/shellquote.go(2 hunks)pkg/util/shellutil/shellutil.go(5 hunks)pkg/util/shellutil/tokenswap.go(1 hunks)pkg/waveapp/waveapp.go(1 hunks)pkg/wavebase/wavebase.go(1 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshremote/wshremote.go(0 hunks)pkg/wshrpc/wshrpctypes.go(3 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)pkg/wshutil/wshproxy.go(5 hunks)pkg/wshutil/wshutil.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/wshrpc/wshremote/wshremote.go
- pkg/remote/conncontroller/tokenswap.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/wshutil/wshproxy.go
202-202: SA9003: empty branch
(staticcheck)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (23)
pkg/util/ds/syncmap.go (1)
Line range hint
13-17: Verify all usages of the renamed constructor across the codebase.The renaming from
NewSyncMaptoMakeSyncMapis a breaking change that requires updates in all calling code.Run this script to find any remaining usages of the old constructor name:
Additionally, consider adding a deprecated stub for backward compatibility:
// Deprecated: Use MakeSyncMap instead func NewSyncMap[T any]() *SyncMap[T] { return MakeSyncMap[T]() }pkg/util/shellutil/tokenswap.go (1)
103-150: Ensure environment variable values are safely quoted and escaped.Verify that the
HardQuote,HardQuoteFish, andHardQuotePowerShellfunctions properly escape special characters to prevent code injection when setting environment variables.Run the following script to check for unsafe variable values:
✅ Verification successful
Environment variable values are properly escaped and safe from injection
The implementation correctly handles special characters for each shell type:
- Bash/Zsh: Escapes
",\,$,`, and\n- Fish: Escapes
",\, and$- PowerShell: Uses backtick (
`) to escape",`,$, and converts\nto`nThe comprehensive test suite validates the escaping behavior across various edge cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of HardQuote functions to verify their implementation. # Expect: HardQuote functions should properly escape special characters. # Find the implementations of HardQuote functions. rg -A 5 'func HardQuote\w*\('Length of output: 950
Script:
#!/bin/bash # Get the complete implementation and tests echo "=== shellquote.go ===" cat pkg/util/shellutil/shellquote.go echo -e "\n=== shellquote_test.go ===" cat pkg/util/shellutil/shellquote_test.go 2>/dev/null || echo "No test file found"Length of output: 5001
cmd/wsh/cmd/wshcmd-root.go (1)
139-158: Check for nil pointers when unpacking swap tokens.Ensure that all fields in the unpacked token are properly validated to prevent nil pointer dereferences.
Consider adding checks for
token.RpcContextfields:if token.RpcContext == nil { return rtn, fmt.Errorf("no rpccontext in token") } +if token.RpcContext.BlockId == "" { + return rtn, fmt.Errorf("no blockId in rpccontext") +}✅ Verification successful
The review suggestion is valid and important for consistency.
The suggested addition of BlockId validation aligns with existing validation patterns in the codebase, particularly in
pkg/waveapp/waveapp.gowhere both nil checks and empty BlockId validation are performed. This validation is crucial as BlockId is a critical field used across the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find RpcContext structure definition ast-grep --pattern 'type RpcContext struct { $$$ }' # Find UnpackSwapToken implementation ast-grep --pattern 'func UnpackSwapToken($$$) { $$$ }' # Look for similar token validation patterns rg -A 5 'RpcContext.*nil'Length of output: 2951
pkg/blockcontroller/blockcontroller.go (2)
278-279: Passing logging context enhances logging consistencyModifying
DoRunShellCommandto accept alogCtx context.Contextparameter improves logging consistency throughout the method and ensures that contextual information is included in log messages.
Line range hint
666-720: Enhanced context propagation inrunmethodUpdating the
runmethod to includelogCtx context.Contextensures that logging throughout the execution has the necessary context, improving traceability and debugging.pkg/remote/conncontroller/conncontroller.go (3)
125-127: Addition ofDebugfmethod improves debug loggingIntroducing the
Debugfmethod inSSHConnenhances debug logging by providing a consistent way to log debug messages with context, aiding in troubleshooting connection issues.
Line range hint
272-346: Improved update logic withafterUpdateparameterAdding the
afterUpdateparameter toStartConnServerrefines the control flow, allowing the function to handle different scenarios before and after WSH updates effectively.
Line range hint
684-706: Correctly handle WSH installation retryIn
tryEnableWsh, after installing WSH, the subsequent call toStartConnServerwithafterUpdateset totrueensures that the newly installed version is properly recognized, enhancing reliability.cmd/wsh/cmd/wshcmd-token.go (1)
13-48:tokencommand implementation is well-structuredThe new
tokencommand effectively exchanges a token for a shell initialization script, with appropriate argument validation and error handling. This addition enhances the utility of the CLI tool.pkg/util/shellutil/shellquote.go (1)
Line range hint
45-68: LGTM! Well-structured implementation of Fish shell quoting.The implementation correctly handles Fish shell's specific quoting requirements by only escaping
",\, and$characters, while omitting newlines and backticks. The function follows the same pattern as other quoting functions, maintaining consistency in the codebase.pkg/wavebase/wavebase.go (1)
29-37: LGTM! Well-organized constant declarations.The new constants follow the existing naming pattern and are properly grouped together. The constants enhance the configuration capabilities for JWT tokens and force updates.
Also applies to: 40-40
pkg/util/shellutil/shellutil.go (1)
155-162: LGTM! Well-implemented PowerShell script.The PowerShell implementation correctly:
- Captures and checks the output before invoking.
- Uses proper PowerShell error handling.
- Cleans up variables appropriately.
pkg/waveapp/waveapp.go (1)
184-184: Verify the authentication response handling.The removal of
NoResponse: truesuggests that the authentication command now expects a response. Please ensure that the response handling is properly implemented in the authentication flow.Run the following script to verify the authentication command usage:
✅ Verification successful
Authentication response handling is correctly implemented
The change from
{NoResponse: true}tonilis valid as different components handle authentication responses according to their specific needs. The CLI usesNoResponse: truewhen no response is needed, while the web and wave apps expect responses for proper authentication flow handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the authentication command is used consistently across the codebase. # Expect: All authentication calls should handle the response similarly. ast-grep --pattern 'AuthenticateCommand($_, $_, $_)'Length of output: 54
Script:
#!/bin/bash # Try multiple approaches to find authentication-related code # 1. Look for AuthenticateCommand with simpler pattern ast-grep --pattern 'AuthenticateCommand' # 2. Use ripgrep as fallback echo "=== Ripgrep search ===" rg "AuthenticateCommand" -A 2 # 3. Find the client package echo "=== Find client package ===" fd -t f "wshclient"Length of output: 2079
pkg/wshutil/wshutil.go (1)
49-49: LGTM! Good centralization of the JWT token variable.Moving the token variable name to
wavebasepackage promotes consistency and maintainability.frontend/app/store/wshclientapi.ts (1)
25-29: LGTM! Clean implementation of the token authentication command.The new
AuthenticateTokenCommandmethod follows the established RPC command pattern and integrates well with the token swap feature.pkg/shellexec/shellexec.go (1)
39-45: LGTM! Well-structured command options type.The
CommandOptsTypestruct is well-organized with clear field names and appropriate JSON tags. The addition ofSwapTokenfield supports the new token swap feature.pkg/wshrpc/wshclient/wshclient.go (1)
34-38: Clean implementation of token authentication!The implementation follows the established pattern, uses type-safe generics, and properly handles error cases.
pkg/wshrpc/wshrpctypes.go (3)
31-37: Well-organized command constant declaration!The new
Command_AuthenticateTokenconstant follows the established naming convention and is properly documented with the "special" comment.
269-273: Good enhancement to authentication return data!The additional fields in
CommandAuthenticateRtnDatasupport the token swap feature by including environment variables and initialization script text.
275-277: Clean and focused token data structure!The
CommandAuthenticateTokenDatastruct is minimal and serves its single purpose well.frontend/types/gotypes.d.ts (2)
112-114: Well-maintained type definitions!The optional properties in
CommandAuthenticateRtnDataare properly marked and align with the Go structure.
116-118: Clean type definition for token data!The
CommandAuthenticateTokenDatatype maintains consistency with its Go counterpart.pkg/wshrpc/wshserver/wshserver.go (1)
713-713: Improved context propagation!The context is now properly passed to
IsWshVersionUpToDate, enabling better logging and cancellation support.
cmd/wsh/cmd/wshcmd-root.go
Outdated
| if err != nil { | ||
| return rtn, fmt.Errorf("error setting up domain socket rpc client: %w", err) | ||
| } | ||
| log.Printf("# sending authenticate token command, token: %s\n", token.Token) |
There was a problem hiding this comment.
Avoid logging sensitive information.
Printing the token value can expose sensitive data in logs. It's important to prevent tokens from appearing in any logs.
Apply this diff to remove the sensitive log:
- log.Printf("# sending authenticate token command, token: %s\n", token.Token)
+ log.Println("# sending authenticate token command")📝 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.
| log.Printf("# sending authenticate token command, token: %s\n", token.Token) | |
| log.Println("# sending authenticate token command") |
pkg/wshutil/wshproxy.go
Outdated
| log.Printf("got AUTHENTICATETOKEN command %v\n", msg.Data) | ||
| if msg.Data == nil { |
There was a problem hiding this comment.
Avoid logging sensitive authentication data.
Logging the message data during authentication can expose sensitive information. It's advisable to omit or sanitize this data in logs.
Apply this diff to modify the log statement:
-func handleAuthenticateTokenCommand(msg RpcMessage) (*shellutil.TokenSwapEntry, error) {
- log.Printf("got AUTHENTICATETOKEN command %v\n", msg.Data)
+func handleAuthenticateTokenCommand(msg RpcMessage) (*shellutil.TokenSwapEntry, error) {
+ log.Println("Received AUTHENTICATETOKEN command")📝 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.
| log.Printf("got AUTHENTICATETOKEN command %v\n", msg.Data) | |
| if msg.Data == nil { | |
| log.Println("Received AUTHENTICATETOKEN command") | |
| if msg.Data == nil { |
pkg/wshutil/wshproxy.go
Outdated
| if origMsg.Command == wshrpc.Command_AuthenticateToken { | ||
|
|
||
| } | ||
| announceBytes, _ := json.Marshal(announceMsg) | ||
| router.InjectMessage(announceBytes, authRtn.RouteId) | ||
| p.sendAuthenticateResponse(origMsg, authRtn.RouteId) | ||
| return authRtn.RouteId, nil | ||
| respErr := fmt.Errorf("connection not authenticated") | ||
| p.sendResponseError(origMsg, respErr) | ||
| continue |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or handle the empty if block to prevent confusion.
An empty if block can be misleading and may indicate incomplete implementation.
Apply this diff to remove the empty if block or implement the necessary logic:
if origMsg.Command == wshrpc.Command_AuthenticateToken {
-
+ // Handle authentication token command if needed
+ // entry, err := handleAuthenticateTokenCommand(origMsg)
+ // ... (implementation)
}Alternatively, if this block is unnecessary, consider removing it:
-if origMsg.Command == wshrpc.Command_AuthenticateToken {
-
-}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
202-202: SA9003: empty branch
(staticcheck)
| @@ -325,6 +360,9 @@ func (bc *BlockController) setupAndStartShellProcess(rc *RunShellOpts, blockMeta | |||
| return nil, fmt.Errorf("unknown controller type %q", bc.ControllerType) | |||
| } | |||
| var shellProc *shellexec.ShellProc | |||
| swapToken := bc.makeSwapToken(ctx) | |||
| cmdOpts.SwapToken = swapToken | |||
| blocklogger.Infof(logCtx, "[conndebug] created swaptoken: %s\n", swapToken.Token) | |||
There was a problem hiding this comment.
Avoid logging sensitive tokens
Logging the swap token value can lead to security vulnerabilities if logs are exposed. Remove or mask the token value when logging to prevent potential leakage.
Apply this diff to fix the issue:
- blocklogger.Infof(logCtx, "[conndebug] created swaptoken: %s\n", swapToken.Token)
+ blocklogger.Infof(logCtx, "[conndebug] created swaptoken\n")📝 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.
| blocklogger.Infof(logCtx, "[conndebug] created swaptoken: %s\n", swapToken.Token) | |
| blocklogger.Infof(logCtx, "[conndebug] created swaptoken\n") |
| func askForJwtToken() (string, error) { | ||
| // if it already exists in the environment, great, use it | ||
| jwtToken := os.Getenv(wavebase.WaveJwtTokenVarName) | ||
| if jwtToken != "" { | ||
| fmt.Printf("HAVE-JWT\n") | ||
| return jwtToken, nil | ||
| } | ||
|
|
||
| // otherwise, ask for it | ||
| fmt.Printf("%s\n", wavebase.NeedJwtConst) | ||
|
|
||
| // read a single line from stdin | ||
| var line string | ||
| _, err := fmt.Fscanln(os.Stdin, &line) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read JWT token from stdin: %w", err) | ||
| } | ||
| return strings.TrimSpace(line), nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and security in askForJwtToken.
-
The error handling for
Fscanlncould be more specific:- EOF should be handled separately as it's a common case.
- Other errors should provide more context.
-
Printing "HAVE-JWT" to stdout might leak sensitive information about token presence.
Consider this improved implementation:
func askForJwtToken() (string, error) {
jwtToken := os.Getenv(wavebase.WaveJwtTokenVarName)
if jwtToken != "" {
- fmt.Printf("HAVE-JWT\n")
+ fmt.Fprintf(os.Stderr, "Using JWT token from environment\n")
return jwtToken, nil
}
fmt.Printf("%s\n", wavebase.NeedJwtConst)
var line string
_, err := fmt.Fscanln(os.Stdin, &line)
- if err != nil {
+ if err == io.EOF {
+ return "", fmt.Errorf("no JWT token provided: EOF")
+ } else if err != nil {
return "", fmt.Errorf("failed to read JWT token from stdin: %w", err)
}
return strings.TrimSpace(line), nil
}| # Source dynamic script from wsh token (the echo is to prevent fish from complaining about empty input) | ||
| wsh token "$WAVETERM_SWAPTOKEN" fish 2>/dev/null | source | ||
| set -e WAVETERM_SWAPTOKEN | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in Fish script.
The error redirection to /dev/null might hide important errors.
Consider this improved implementation:
-# Source dynamic script from wsh token (the echo is to prevent fish from complaining about empty input)
-wsh token "$WAVETERM_SWAPTOKEN" fish 2>/dev/null | source
+# Source dynamic script from wsh token
+if not set output (wsh token "$WAVETERM_SWAPTOKEN" fish 2>&1)
+ echo "Warning: Failed to source token script: $output" >&2
+else
+ echo "$output" | source
+end
set -e WAVETERM_SWAPTOKEN📝 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.
| # Source dynamic script from wsh token (the echo is to prevent fish from complaining about empty input) | |
| wsh token "$WAVETERM_SWAPTOKEN" fish 2>/dev/null | source | |
| set -e WAVETERM_SWAPTOKEN | |
| # Source dynamic script from wsh token | |
| if not set output (wsh token "$WAVETERM_SWAPTOKEN" fish 2>&1) | |
| echo "Warning: Failed to source token script: $output" >&2 | |
| else | |
| echo "$output" | source | |
| end | |
| set -e WAVETERM_SWAPTOKEN |
| # add wsh to path, source dynamic script from wsh token | ||
| WAVETERM_WSHBINDIR={{.WSHBINDIR}} | ||
| export PATH="$WAVETERM_WSHBINDIR:$PATH" | ||
| source <(wsh token "$WAVETERM_SWAPTOKEN" zsh 2>/dev/null) | ||
| unset WAVETERM_SWAPTOKEN | ||
|
|
||
| # Source the original zshrc only if ZDOTDIR has not been changed | ||
| if [ "$ZDOTDIR" = "$WAVETERM_ZDOTDIR" ]; then | ||
| [ -f ~/.zshrc ] && source ~/.zshrc | ||
| fi | ||
|
|
||
| # Custom additions | ||
| export PATH={{.WSHBINDIR}}:$PATH | ||
| if [[ ":$PATH:" != *":$WAVETERM_WSHBINDIR:"* ]]; then | ||
| export PATH="$WAVETERM_WSHBINDIR:$PATH" | ||
| fi | ||
| unset WAVETERM_WSHBINDIR | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve PATH manipulation and error handling in Zsh script.
- The PATH manipulation is duplicated (lines 59 and 69).
- Error redirection to /dev/null might hide important security-related errors.
Consider this improved implementation:
-# add wsh to path, source dynamic script from wsh token
-WAVETERM_WSHBINDIR={{.WSHBINDIR}}
-export PATH="$WAVETERM_WSHBINDIR:$PATH"
-source <(wsh token "$WAVETERM_SWAPTOKEN" zsh 2>/dev/null)
-unset WAVETERM_SWAPTOKEN
+# add wsh to path if not already present
+WAVETERM_WSHBINDIR={{.WSHBINDIR}}
+if [[ ":$PATH:" != *":$WAVETERM_WSHBINDIR:"* ]]; then
+ export PATH="$WAVETERM_WSHBINDIR:$PATH"
+fi
+
+# source dynamic script from wsh token
+if ! output=$(wsh token "$WAVETERM_SWAPTOKEN" zsh 2>&1); then
+ echo "Warning: Failed to source token script: $output" >&2
+else
+ source <(echo "$output")
+fi
+unset WAVETERM_SWAPTOKEN
# Source the original zshrc only if ZDOTDIR has not been changed
if [ "$ZDOTDIR" = "$WAVETERM_ZDOTDIR" ]; then
[ -f ~/.zshrc ] && source ~/.zshrc
fi
-if [[ ":$PATH:" != *":$WAVETERM_WSHBINDIR:"* ]]; then
- export PATH="$WAVETERM_WSHBINDIR:$PATH"
-fi
unset WAVETERM_WSHBINDIR📝 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.
| # add wsh to path, source dynamic script from wsh token | |
| WAVETERM_WSHBINDIR={{.WSHBINDIR}} | |
| export PATH="$WAVETERM_WSHBINDIR:$PATH" | |
| source <(wsh token "$WAVETERM_SWAPTOKEN" zsh 2>/dev/null) | |
| unset WAVETERM_SWAPTOKEN | |
| # Source the original zshrc only if ZDOTDIR has not been changed | |
| if [ "$ZDOTDIR" = "$WAVETERM_ZDOTDIR" ]; then | |
| [ -f ~/.zshrc ] && source ~/.zshrc | |
| fi | |
| # Custom additions | |
| export PATH={{.WSHBINDIR}}:$PATH | |
| if [[ ":$PATH:" != *":$WAVETERM_WSHBINDIR:"* ]]; then | |
| export PATH="$WAVETERM_WSHBINDIR:$PATH" | |
| fi | |
| unset WAVETERM_WSHBINDIR | |
| # add wsh to path if not already present | |
| WAVETERM_WSHBINDIR={{.WSHBINDIR}} | |
| if [[ ":$PATH:" != *":$WAVETERM_WSHBINDIR:"* ]]; then | |
| export PATH="$WAVETERM_WSHBINDIR:$PATH" | |
| fi | |
| # source dynamic script from wsh token | |
| if ! output=$(wsh token "$WAVETERM_SWAPTOKEN" zsh 2>&1); then | |
| echo "Warning: Failed to source token script: $output" >&2 | |
| else | |
| source <(echo "$output") | |
| fi | |
| unset WAVETERM_SWAPTOKEN | |
| # Source the original zshrc only if ZDOTDIR has not been changed | |
| if [ "$ZDOTDIR" = "$WAVETERM_ZDOTDIR" ]; then | |
| [ -f ~/.zshrc ] && source ~/.zshrc | |
| fi | |
| unset WAVETERM_WSHBINDIR |
| conn.Infof(logCtx, "detected shell type: %s\n", shellType) | ||
| conn.Infof(logCtx, "swaptoken: %s\n", cmdOpts.SwapToken.Token) |
There was a problem hiding this comment.
Avoid logging sensitive token information.
The token value should not be logged, even at debug level, as it could expose sensitive information in log files.
Remove or mask the token value in the log statement:
-conn.Infof(logCtx, "swaptoken: %s\n", cmdOpts.SwapToken.Token)
+conn.Infof(logCtx, "swaptoken: [redacted]\n")📝 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.
| conn.Infof(logCtx, "detected shell type: %s\n", shellType) | |
| conn.Infof(logCtx, "swaptoken: %s\n", cmdOpts.SwapToken.Token) | |
| conn.Infof(logCtx, "detected shell type: %s\n", shellType) | |
| conn.Infof(logCtx, "swaptoken: [redacted]\n") |
| packedToken, err := cmdOpts.SwapToken.PackForClient() | ||
| if err != nil { | ||
| blocklogger.Infof(logCtx, "error packing swap token: %v", err) | ||
| } else { | ||
| blocklogger.Debugf(logCtx, "packed swaptoken %s\n", packedToken) | ||
| shellutil.UpdateCmdEnv(ecmd, map[string]string{wavebase.WaveSwapTokenVarName: packedToken}) | ||
| } |
There was a problem hiding this comment.
Avoid logging sensitive token information.
Similar to the remote shell process, the token value should not be logged to prevent exposure of sensitive information.
Remove or mask the token value in the log statement:
-blocklogger.Debugf(logCtx, "packed swaptoken %s\n", packedToken)
+blocklogger.Debugf(logCtx, "packed swaptoken: [redacted]\n")📝 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.
| packedToken, err := cmdOpts.SwapToken.PackForClient() | |
| if err != nil { | |
| blocklogger.Infof(logCtx, "error packing swap token: %v", err) | |
| } else { | |
| blocklogger.Debugf(logCtx, "packed swaptoken %s\n", packedToken) | |
| shellutil.UpdateCmdEnv(ecmd, map[string]string{wavebase.WaveSwapTokenVarName: packedToken}) | |
| } | |
| packedToken, err := cmdOpts.SwapToken.PackForClient() | |
| if err != nil { | |
| blocklogger.Infof(logCtx, "error packing swap token: %v", err) | |
| } else { | |
| blocklogger.Debugf(logCtx, "packed swaptoken: [redacted]\n") | |
| shellutil.UpdateCmdEnv(ecmd, map[string]string{wavebase.WaveSwapTokenVarName: packedToken}) | |
| } |
