add jwt token back to wsl connections #1841
Conversation
WalkthroughThe pull request introduces changes across several files: ✨ 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/shellexec/shellexec.go(1 hunks)pkg/util/shellutil/tokenswap.go(0 hunks)
💤 Files with no reviewable changes (1)
- pkg/util/shellutil/tokenswap.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
| jwtToken := cmdOpts.SwapToken.Env[wavebase.WaveJwtTokenVarName] | ||
| if jwtToken != "" { | ||
| cmdCombined = fmt.Sprintf(`%s=%s %s`, wavebase.WaveJwtTokenVarName, jwtToken, cmdCombined) | ||
| } |
There was a problem hiding this comment.
Prevent nil-pointer dereference and sensitive token leakage
-
Potential nil-pointer dereference
cmdOpts.SwapTokencan benil, leading to a panic when accessingcmdOpts.SwapToken.Env. EnsureSwapTokenand itsEnvmap are non-nil before referencing them. -
Avoid leaking JWT tokens in logs
Immediately after appending the JWT token tocmdCombined, the full command is logged:log.Printf("full combined command: %s", cmdCombined)This exposes the token in logs, creating a significant security risk. Mask or omit the token to protect sensitive credentials.
Below are sample diffs to mitigate both issues:
--- a/pkg/shellexec/shellexec.go
+++ b/pkg/shellexec/shellexec.go
@@ -264,7 +264,11 @@ func StartWslShellProc(ctx context.Context, termSize waveobj.TermSize, cmdStr st
}
- jwtToken := cmdOpts.SwapToken.Env[wavebase.WaveJwtTokenVarName]
+ if cmdOpts.SwapToken == nil || cmdOpts.SwapToken.Env == nil {
+ conn.Infof(ctx, "warning: SwapToken or SwapToken.Env is nil; skipping JWT token injection")
+ } else {
+ jwtToken := cmdOpts.SwapToken.Env[wavebase.WaveJwtTokenVarName]
+ if jwtToken != "" {
+ cmdCombined = fmt.Sprintf(`%s=%s %s`, wavebase.WaveJwtTokenVarName, jwtToken, cmdCombined)
+ }
+ }
log.Printf("full combined command: %s", cmdCombined) // Potential token leakAnd to mask the leaked token in logs (outside the annotated lines):
- log.Printf("full combined command: %s", cmdCombined)
+ // Avoid logging sensitive token
+ maskedCommand := strings.ReplaceAll(
+ cmdCombined,
+ fmt.Sprintf("%s=%s", wavebase.WaveJwtTokenVarName, jwtToken),
+ fmt.Sprintf("%s=***REDACTED***", wavebase.WaveJwtTokenVarName),
+ )
+ log.Printf("full combined command: %s", maskedCommand)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/wshrpc/wshserver/wshserver.go (1)
55-66: Add validation & error handling for token-based authentication.
The function checks if the token entry is nil and returns an error if it is invalid. Consider adding additional safeguards:
- Verify the authenticity of the token or incorporate an expiration mechanism.
- Log or track invalid attempts if needed.
- Remove the TODO comment once multiproxy integration is complete.
Additionally, ensure proper unit tests cover valid tokens, invalid tokens, and concurrency edge cases for
shellutil.GetAndRemoveTokenSwapEntry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/wshrpc/wshserver/wshserver.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
pkg/wshrpc/wshserver/wshserver.go (1)
31-31: Confirm usage of newly introduced import.
Theshellutilimport is introduced here. Make sure it is only used for retrieving and removing token entries—and confirm no overlapping or redundant imports exist within the file or codebase.Use this script to locate other possible references to
shellutil:✅ Verification successful
Import usage verified - properly scoped to token operations
Theshellutilimport in wshserver.go is used exclusively for token management viaGetAndRemoveTokenSwapEntry(). No overlapping or redundant imports exist within the file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for 'shellutil' references throughout the codebase rg -A 5 'shellutil'Length of output: 27960
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/remote/connparse/connparse.go (1)
26-26: Consider validating the presence of a host segment in the compiled regex.The
regexp.MustCompile("^wsl://[^/]+")pattern allows for scenarios likewsl://without any subsequent characters. This returns an empty match on.FindString()if there's no host. Ensure your design either expects this or gracefully handles this edge case.frontend/app/app-bg.tsx (1)
57-57: Expand or constrain Windows path handling as appropriate.By allowing Windows-style paths (e.g.,
C:\), you open the door for local file inclusion as backgrounds. Consider adding checks or warnings to ensure users do not unintentionally expose sensitive files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/wsh/cmd/wshcmd-setbg.go(1 hunks)frontend/app/app-bg.tsx(1 hunks)pkg/remote/connparse/connparse.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- cmd/wsh/cmd/wshcmd-setbg.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
pkg/remote/connparse/connparse.go (1)
125-131: Approved fallback logic for non-WSL URIs.The current logic correctly splits the string by the first slash to separate host and path for other schemes. No concerns found.
| if strings.HasPrefix(rest, "wsl://") { | ||
| host = wslConnRegex.FindString(rest) | ||
| remotePath = strings.TrimPrefix(rest, host) |
There was a problem hiding this comment.
💡 Codebase verification
Empty WSL host validation is required
The review is correct. The regex pattern ^wsl://[^/]+ would match an empty string after "wsl://" prefix, and the codebase expects a valid distro name in multiple places (wslconn, wshserver, blockcontroller). The suggested fix to validate the host is appropriate to prevent potential issues downstream.
🔗 Analysis chain
Handle empty host scenario for WSL URIs.
If the pattern fails to match (e.g., wsl:// with no host), .FindString(rest) will be "", passing an empty host downstream unintentionally. Consider returning an error in that scenario.
Apply the following diff snippet to validate the host:
host = wslConnRegex.FindString(rest)
+if host == "" {
+ return nil, fmt.Errorf("invalid wsl:// URI: missing host segment")
+}
remotePath = strings.TrimPrefix(rest, host)🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find wslConnRegex definition and surrounding context
rg "wslConnRegex" pkg/remote/connparse/connparse.go -B 2 -A 2
# Look for test files and WSL-related tests
fd ".*_test\.go" pkg/remote/connparse/ --exec cat {}
# Search for WSL URI patterns in the codebase
rg "wsl://" -A 2 -B 2
Length of output: 16431
No description provided.