Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add jwt token back to wsl connections #1841

Merged
merged 7 commits into from
Jan 24, 2025
Merged

add jwt token back to wsl connections #1841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b383ad2
Fix connparse for wsl connections
esimkowitz Jan 24, 2025
0035b6c
add jwt token back to wsl connections
sawka Jan 24, 2025
d5fa489
implement authenticatetokencommand in wshserver.go
sawka Jan 24, 2025
cee9251
allow windows paths for bg
sawka Jan 24, 2025
d4d23d8
save
esimkowitz Jan 24, 2025
fd962ae
Merge branch 'evan/fix-setbg' into sawka/wsl-fix
esimkowitz Jan 24, 2025
bf7d820
Merge branch 'evan/wsl-connparse' into sawka/wsl-fix
esimkowitz Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/wsh/cmd/wshcmd-setbg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func setBgRun(cmd *cobra.Command, args []string) (rtnErr error) {
}

// Create URL-safe path
escapedPath := strings.ReplaceAll(absPath, "\\", "\\\\")
escapedPath := filepath.ToSlash(absPath)
escapedPath = strings.ReplaceAll(escapedPath, "'", "\\'")
bgStyle = fmt.Sprintf("url('%s')", escapedPath)

Expand Down
2 changes: 1 addition & 1 deletion frontend/app/app-bg.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function processBackgroundUrls(cssText: string): string {
return;
}
// allow absolute paths
if (originalUrl.startsWith("/") || originalUrl.startsWith("~/")) {
if (originalUrl.startsWith("/") || originalUrl.startsWith("~/") || /^[a-zA-Z]:(\/|\\)/.test(originalUrl)) {
const newUrl = encodeFileURL(originalUrl);
node.value = newUrl;
return;
Expand Down
16 changes: 11 additions & 5 deletions pkg/remote/connparse/connparse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const (
)

var windowsDriveRegex = regexp.MustCompile(`^[a-zA-Z]:`)
var wslConnRegex = regexp.MustCompile(`^wsl://[^/]+`)

type Connection struct {
Scheme string
Expand Down Expand Up @@ -117,12 +118,17 @@ func ParseURI(uri string) (*Connection, error) {
remotePath = rest
}
} else {
split = strings.SplitN(rest, "/", 2)
host = split[0]
if len(split) > 1 {
remotePath = split[1]
if strings.HasPrefix(rest, "wsl://") {
host = wslConnRegex.FindString(rest)
remotePath = strings.TrimPrefix(rest, host)
Comment on lines +121 to +123
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

} else {
remotePath = "/"
split = strings.SplitN(rest, "/", 2)
host = split[0]
if len(split) > 1 {
remotePath = split[1]
} else {
remotePath = "/"
}
}
}

Expand Down
4 changes: 4 additions & 0 deletions pkg/shellexec/shellexec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,10 @@ func StartWslShellProc(ctx context.Context, termSize waveobj.TermSize, cmdStr st
conn.Debugf(ctx, "packed swaptoken %s\n", packedToken)
cmdCombined = fmt.Sprintf(`%s=%s %s`, wavebase.WaveSwapTokenVarName, packedToken, cmdCombined)
}
jwtToken := cmdOpts.SwapToken.Env[wavebase.WaveJwtTokenVarName]
if jwtToken != "" {
cmdCombined = fmt.Sprintf(`%s=%s %s`, wavebase.WaveJwtTokenVarName, jwtToken, cmdCombined)
}
Comment on lines +265 to +268
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Prevent nil-pointer dereference and sensitive token leakage

  1. Potential nil-pointer dereference
    cmdOpts.SwapToken can be nil, leading to a panic when accessing cmdOpts.SwapToken.Env. Ensure SwapToken and its Env map are non-nil before referencing them.

  2. Avoid leaking JWT tokens in logs
    Immediately after appending the JWT token to cmdCombined, 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 leak

And 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.

log.Printf("full combined command: %s", cmdCombined)
ecmd := exec.Command("wsl.exe", "~", "-d", client.Name(), "--", "sh", "-c", cmdCombined)
if termSize.Rows == 0 || termSize.Cols == 0 {
Expand Down
1 change: 0 additions & 1 deletion pkg/util/shellutil/tokenswap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ type TokenSwapEntry struct {
Token string `json:"token"`
SockName string `json:"sockname,omitempty"`
RpcContext *wshrpc.RpcContext `json:"rpccontext,omitempty"`
JwtToken string `json:"jwttoken,omitempty"`
Env map[string]string `json:"env,omitempty"`
ScriptText string `json:"scripttext,omitempty"`
Exp time.Time `json:"-"`
Expand Down
14 changes: 14 additions & 0 deletions pkg/wshrpc/wshserver/wshserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/wavetermdev/waveterm/pkg/remote/fileshare"
"github.com/wavetermdev/waveterm/pkg/telemetry"
"github.com/wavetermdev/waveterm/pkg/util/envutil"
"github.com/wavetermdev/waveterm/pkg/util/shellutil"
"github.com/wavetermdev/waveterm/pkg/util/utilfn"
"github.com/wavetermdev/waveterm/pkg/util/wavefileutil"
"github.com/wavetermdev/waveterm/pkg/waveai"
Expand All @@ -51,6 +52,19 @@ func (*WshServer) WshServerImpl() {}

var WshServerImpl = WshServer{}

// TODO remove this after implementing in multiproxy, just for wsl
func (ws *WshServer) AuthenticateTokenCommand(ctx context.Context, data wshrpc.CommandAuthenticateTokenData) (wshrpc.CommandAuthenticateRtnData, error) {
entry := shellutil.GetAndRemoveTokenSwapEntry(data.Token)
if entry == nil {
return wshrpc.CommandAuthenticateRtnData{}, fmt.Errorf("invalid token")
}
rtn := wshrpc.CommandAuthenticateRtnData{
Env: entry.Env,
InitScriptText: entry.ScriptText,
}
return rtn, nil
}

func (ws *WshServer) TestCommand(ctx context.Context, data string) error {
defer func() {
panichandler.PanicHandler("TestCommand", recover())
Expand Down
Loading