implement cmd:jwt and fix remote execution of commands#2292
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a new metadata flag "cmd:jwt" across the stack: declares MetaKey_CmdJwt, adds CmdJwt to MetaTSType and the frontend MetaType, and propagates it via blockcontroller into a new CommandOptsType.ForceJwt field. Updates shellexec to optionally inject a Wave JWT into environments for WSL (when a token exists), and for local/remote sessions when ForceJwt is true; also adds logs, adjusts command construction, and removes unused shell-type helpers. In server startup, adds cleanup to unset several WAVETERM_* environment variables after a successful grab. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/shellexec/shellexec.go (1)
267-274: Redact sensitive tokens in command logsThe only remaining critical issue is that we still emit the full combined command—complete with
WAVETERM_SWAP_TOKENandWAVETERM_JWT—to the console. This must be redacted. No changes are needed aroundForceJwtgating, as both the WSL and standard execution paths already respectcmdOpts.ForceJwt(see theif jwtToken != "" && cmdOpts.ForceJwtchecks inpkg/shellexec/shellexec.goandpkg/blockcontroller/blockcontroller.go).Please apply the following mandatory refactor:
• Replace the plain logging call:
- log.Printf("full combined command: %s", cmdCombined) + log.Printf("full combined command: %s", redactSensitive(cmdCombined))• Add a redaction helper and import:
import "regexp" var sensitiveEnvRe = regexp.MustCompile(`\b(WAVETERM_SWAP_TOKEN|WAVETERM_JWT)=\S+`) func redactSensitive(s string) string { return sensitiveEnvRe.ReplaceAllString(s, "$1=<redacted>") }Optionally, add unit tests for
redactSensitiveto ensure no tokens slip through.
🧹 Nitpick comments (6)
frontend/types/gotypes.d.ts (1)
539-539: Add MetaType key "cmd:jwt" — looks good; consider adding connection-level support.The TS typing addition is correct and consistent with surrounding MetaType keys.
Optional follow-ups:
- Allow setting this at the connection level too (ConnKeywords), for org-wide defaults.
- Briefly document semantics in your user-facing metadata docs (default false; when true, Wave injects a JWT into the command environment as per backend rules).
Additional change outside this hunk (if you want connection-level support):
--- a/frontend/types/gotypes.d.ts +++ b/frontend/types/gotypes.d.ts @@ type ConnKeywords = { @@ "cmd:env"?: {[key: string]: string}; + "cmd:jwt"?: boolean; "cmd:initscript"?: string; "cmd:initscript.sh"?: string; "cmd:initscript.bash"?: string; "cmd:initscript.zsh"?: string; "cmd:initscript.pwsh"?: string; "cmd:initscript.fish"?: string;pkg/waveobj/metaconsts.go (1)
54-54: New meta key constant MetaKey_CmdJwt — consistent and well placed.The constant fits the existing grouping/order and naming. Consider adding a short godoc-style comment in the generator so it shows up in generated places.
cmd/server/main-server.go (1)
262-271: Centralize WAVETERM env‐var cleanup and reuse canonical constantsTo avoid drift and make it easier to add or remove vars in one place:
• Create a single slice of env‐var names in cmd/server/main-server.go
• Import and use the existing pkg/wavebase constants where available (e.g. WaveJwtTokenVarName)
• For any names not yet defined in pkg/wavebase, consider adding them there so all WAVETERM_* vars live in one packageSuggested diff:
--- a/cmd/server/main-server.go +++ b/cmd/server/main-server.go @@ -261,12 +261,16 @@ func startServer(...) error { - // Remove WAVETERM env vars that leak from prod => dev - os.Unsetenv("WAVETERM_CLIENTID") - os.Unsetenv("WAVETERM_WORKSPACEID") - os.Unsetenv("WAVETERM_TABID") - os.Unsetenv("WAVETERM_BLOCKID") - os.Unsetenv("WAVETERM_CONN") - os.Unsetenv("WAVETERM_JWT") - os.Unsetenv("WAVETERM_VERSION") + // Remove WAVETERM env vars that leak from prod => dev + // TODO: move all of these into pkg/wavebase as constants to keep names in one place + leakVars := []string{ + "WAVETERM_CLIENTID", + "WAVETERM_WORKSPACEID", + "WAVETERM_TABID", + "WAVETERM_BLOCKID", + "WAVETERM_CONN", + wavebase.WaveJwtTokenVarName, // == "WAVETERM_JWT" + "WAVETERM_VERSION", + } + for _, k := range leakVars { + _ = os.Unsetenv(k) + }This ensures all cleanup targets are defined together, and reuses the verified wavebase.WaveJwtTokenVarName constant.
pkg/blockcontroller/blockcontroller.go (2)
551-561: Apply cmd:jwt to “shell” controller as well (if intended).Currently, ForceJwt is only set for “cmd” blocks. If the semantics of cmd:jwt are meant to apply to interactive “shell” blocks too (local/remote), set it here as well so shellexec can inject the JWT in non-WSH paths when requested.
Proposed change (outside the modified hunk, shown for clarity):
--- a/pkg/blockcontroller/blockcontroller.go +++ b/pkg/blockcontroller/blockcontroller.go @@ if bc.ControllerType == BlockController_Shell { cmdOpts.Interactive = true cmdOpts.Login = true cmdOpts.Cwd = blockMeta.GetString(waveobj.MetaKey_CmdCwd, "") if cmdOpts.Cwd != "" { cwdPath, err := wavebase.ExpandHomeDir(cmdOpts.Cwd) if err != nil { return nil, err } cmdOpts.Cwd = cwdPath } + // Honor cmd:jwt for shell-type controllers as well. + cmdOpts.ForceJwt = blockMeta.GetBool(waveobj.MetaKey_CmdJwt, false)If the design is “cmd:jwt only affects direct command runs,” feel free to ignore, but please confirm expected behavior in a short code comment near createCmdStrAndOpts for future readers.
372-373: Propagate cmd:jwt → cmdOpts.ForceJwt verified and explicit
- Ripgrep check confirms no other
.ForceJwt =assignments outside ofblockcontroller.go– this is the sole source of truth forForceJwt.- All other occurrences in
pkg/shellexec/shellexec.goare reads/uses ofForceJwt, not writes.- ✅ Behavior is explicit: only
createCmdStrAndOpts(inpkg/blockcontroller/blockcontroller.goat lines 372–373) drives the flag.Optional improvement:
- Add a targeted unit test for
createCmdStrAndOptsto assert that whenblockMetacontainsMetaKey_CmdJwt=true, the returnedcmdOpts.ForceJwtistrue.pkg/waveobj/wtypemeta.go (1)
53-54: MetaTSType.CmdJwt field addition — consistent with TS and consts.Matches the TS type and the new metaconst. Consider adding a brief comment in the metadata docs explaining precedence and when JWT is injected (WSL always if available; local/SSH when ForceJwt=true), to align frontend and backend expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
cmd/server/main-server.go(1 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/blockcontroller/blockcontroller.go(1 hunks)pkg/shellexec/shellexec.go(6 hunks)pkg/waveobj/metaconsts.go(1 hunks)pkg/waveobj/wtypemeta.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/blockcontroller/blockcontroller.go (2)
pkg/util/utilfn/utilfn.go (1)
GetBool(71-88)pkg/waveobj/metaconsts.go (1)
MetaKey_CmdJwt(54-54)
pkg/shellexec/shellexec.go (4)
pkg/blocklogger/blocklogger.go (1)
Debugf(86-92)pkg/wavebase/wavebase.go (1)
WaveJwtTokenVarName(36-36)pkg/wshutil/wshutil.go (1)
WaveJwtTokenVarName(50-50)pkg/util/shellutil/shellutil.go (1)
UpdateCmdEnv(237-262)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
pkg/shellexec/shellexec.go (4)
215-215: Debug log of cmdStr is fine; keep it debug-onlyLogging the raw cmdStr can include secrets. Good that this is Debugf (gated). Please keep it out of info-level logs going forward.
513-517: LGTM: Local JWT injection gated behind ForceJwtBehavior matches the new flag and uses UpdateCmdEnv rather than string-prefixing — safer and clearer. Keep the debug log at debug level only.
38-46: Add documentation forForceJwtand retain existing JSON tagTo clarify the intent of
ForceJwtand avoid breaking downstream consumers, add a comment above the field and keep theforcejwttag unchanged:pkg/shellexec/shellexec.go type CommandOptsType struct { Interactive bool `json:"interactive,omitempty"` Login bool `json:"login,omitempty"` Cwd string `json:"cwd,omitempty"` ShellPath string `json:"shellPath,omitempty"` ShellOpts []string `json:"shellOpts,omitempty"` SwapToken *shellutil.TokenSwapEntry `json:"swapToken,omitempty"` - ForceJwt bool `json:"forcejwt,omitempty"` + // ForceJwt injects WAVETERM_JWT into the spawned process environment when true. + // Note: WSL handling currently differs—see StartWslShellProc for Windows-specific logic. + ForceJwt bool `json:"forcejwt,omitempty"` }• No other references to the JSON tag exist beyond this struct, the two shell-start sites (lines 447 and 514), and the blockcontroller assignment (line 372), so renaming the tag risks breaking existing JSON contracts.
• Keeping the lowercaseforcejwttag ensures backward compatibility while the comment makes the behavior clear.
446-450: Quote environment assignments in shell commandsThe code currently embeds raw token values into a
VAR=valueprefix forsh -c. While JWTs are strictly base64url (letters, digits,-,_, and.) and won’t break a shell assignment, your swap-token payload (fromPackForClient()) may include characters (e.g. JSON delimiters, padding=signs, or other symbols) that could. To be safe, please either:
- Confirm that both
cmdOpts.SwapToken.PackForClient()cmdOpts.SwapToken.Env[wavebase.WaveJwtTokenVarName]
only ever produce characters in[A-Za-z0-9._-], or- Wrap both values in a POSIX‐quoting helper.
Suggested diff (in
pkg/shellexec/shellexec.go):- cmdCombined = fmt.Sprintf(`%s=%s %s`, wavebase.WaveSwapTokenVarName, packedToken, cmdCombined) + cmdCombined = fmt.Sprintf(`%s=%s %s`, wavebase.WaveSwapTokenVarName, shellutil.PosixQuote(packedToken), cmdCombined) …later… - cmdCombined = fmt.Sprintf(`%s=%s %s`, wavebase.WaveJwtTokenVarName, jwtToken, cmdCombined) + cmdCombined = fmt.Sprintf(`%s=%s %s`, wavebase.WaveJwtTokenVarName, shellutil.PosixQuote(jwtToken), cmdCombined)Helper in
pkg/util/shellutil:// PosixQuote wraps s in single quotes and escapes embedded single quotes. func PosixQuote(s string) string { if s == "" { return "''" } return "'" + strings.ReplaceAll(s, "'", `'"'"'`) + "'" }Please verify the character sets of your tokens if you opt not to add quoting.
| conn.Infof(logCtx, "swaptoken: %s\n", cmdOpts.SwapToken.Token) | ||
| conn.Debugf(logCtx, "cmdStr: %q\n", cmdStr) | ||
|
|
||
| if cmdStr == "" { |
There was a problem hiding this comment.
Remove token logs and redact combined command on SSH path
- conn.Infof(... "swaptoken: %s", cmdOpts.SwapToken.Token) at Line 366 logs a sensitive token at info level. This is a credential leak.
- Logging cmdCombined at info level (Line 404 below) also leaks env-injected secrets.
Switch to Debugf and redact, or drop entirely.
Apply:
- conn.Infof(logCtx, "swaptoken: %s\n", cmdOpts.SwapToken.Token)
+ conn.Debugf(logCtx, "swaptoken: <redacted>\n")- conn.Infof(logCtx, "starting shell, using command: %s\n", cmdCombined)
+ conn.Debugf(logCtx, "starting shell, using command: %s\n", redactSensitive(cmdCombined))Optional: search for similar leaks elsewhere:
#!/bin/bash
rg -nP '(Infof|Printf)\([^)]*(WAVETERM_(JWT|SWAP_TOKEN)|swaptoken|packed swaptoken|PackForClient)'1 participant
No description provided.