fix(handlers): validate path/auth BEFORE docker availability checks

Three traversal / cross-workspace rejection tests on staging were
masked by premature "docker not available" early returns:

1. deleteViaEphemeral — nil-docker check fired BEFORE path validation;
   malicious paths got "docker not available" (wrong code path) instead
   of "path not allowed". Reversed the order + added "path not allowed:"
   prefix to rejection messages.

2. copyFilesToContainer — split the traversal classifier into:
   - absolute path → "unsafe file path in archive"
   - literal "../" prefix → "unsafe file path in archive" (classic)
   - URL-encoded / mid-path traversal → "path escapes destination"
   Added nil-docker guard AFTER validation so legitimate inputs error
   cleanly instead of panicking on nil docker.

3. HandleConnect KI-005 — test used outdated table name
   "workspace_tokens"; ValidateAnyToken uses "workspace_auth_tokens"
   since #1210. Updated the mock. Added best-effort last_used_at
   UPDATE expectation that fires after successful token validation.

Brings the handlers package from 3 failing tests to 0. All 20 Go
packages green on go test -race ./... locally.
This commit is contained in:
Hongming Wang 2026-04-23 09:31:54 -07:00
parent 47dc72c6b3
commit df2cf935d3
2 changed files with 41 additions and 12 deletions

View File

@ -79,9 +79,22 @@ func (h *TemplatesHandler) copyFilesToContainer(ctx context.Context, containerNa
// Files are written inside destPath (typically /configs); anything that escapes
// via ".." or an absolute name could reach other volumes or system paths.
clean := filepath.Clean(name)
if filepath.IsAbs(clean) || strings.HasPrefix(clean, "..") {
if filepath.IsAbs(clean) {
return fmt.Errorf("unsafe file path in archive: %s", name)
}
if strings.HasPrefix(name, "../") {
// Literal leading "../" with separator — classic traversal.
// Tests expect "unsafe file path in archive" wording here.
// URL-encoded "..%2F..." and mid-path "foo/../.." fall through
// to the Clean-based check below, which uses "path escapes
// destination" wording.
return fmt.Errorf("unsafe file path in archive: %s", name)
}
if strings.HasPrefix(clean, "..") {
// Mid-path traversal that resolves out of the intended root
// after filepath.Clean — tests expect "path escapes destination".
return fmt.Errorf("path escapes destination: %s", name)
}
// Prepend destPath so relative paths land inside the volume mount.
// Use cleaned name so validation (which checks clean) and usage stay consistent.
archiveName := filepath.Join(destPath, clean)
@ -121,6 +134,9 @@ func (h *TemplatesHandler) copyFilesToContainer(ctx context.Context, containerNa
return fmt.Errorf("failed to close tar writer: %w", err)
}
if h.docker == nil {
return fmt.Errorf("docker not available")
}
return h.docker.CopyToContainer(ctx, containerName, destPath, &buf, container.CopyToContainerOptions{})
}
@ -159,14 +175,14 @@ func (h *TemplatesHandler) writeViaEphemeral(ctx context.Context, volumeName str
// deleteViaEphemeral deletes a file from a named volume using an ephemeral container.
func (h *TemplatesHandler) deleteViaEphemeral(ctx context.Context, volumeName, filePath string) error {
if h.docker == nil {
return fmt.Errorf("docker not available")
}
// CWE-78/CWE-22: validate before use. Also switches to exec form
// ([]string{...}) so filePath is passed as a plain argument, not
// interpolated into a shell string — eliminates shell injection entirely.
// CWE-78/CWE-22: validate BEFORE any downstream availability check.
// Reversed order from earlier versions: the "docker not available"
// early return used to mask malicious paths with a generic error
// when tests (or ops with no Docker daemon) invoked the handler,
// making it impossible to verify the traversal guards fire. Exec
// form ([]string{...}) also defends against shell injection.
if err := validateRelPath(filePath); err != nil {
return err
return fmt.Errorf("path not allowed: %w", err)
}
// F1085 (Misconfiguration - Filesystems): scope rm to the /configs volume.
@ -176,7 +192,11 @@ func (h *TemplatesHandler) deleteViaEphemeral(ctx context.Context, volumeName, f
rmTarget := filepath.Join("/configs", filePath)
rmTarget = filepath.Clean(rmTarget)
if !strings.HasPrefix(rmTarget, "/configs/") {
return fmt.Errorf("path escapes volume scope: %s", filePath)
return fmt.Errorf("path not allowed: escapes volume scope: %s", filePath)
}
if h.docker == nil {
return fmt.Errorf("docker not available")
}
resp, err := h.docker.ContainerCreate(ctx, &container.Config{

View File

@ -73,11 +73,20 @@ func TestTerminalConnect_KI005_RejectsUnauthorizedCrossWorkspace(t *testing.T) {
canCommunicateCheck = func(callerID, targetID string) bool { return false }
defer func() { canCommunicateCheck = prev }()
// Token lookup: ws-caller's token is valid.
rows := sqlmock.NewRows([]string{"workspace_id"}).AddRow("ws-caller")
mock.ExpectQuery("SELECT workspace_id FROM workspace_tokens").
// Token lookup: ws-caller's token is valid. ValidateAnyToken uses
// workspace_auth_tokens + a JOIN on workspaces to filter out removed
// rows; an older version of this test expected "workspace_tokens"
// (outdated table name) and got 503 Docker-unavailable because the
// token validation silently failed before the CanCommunicate check.
rows := sqlmock.NewRows([]string{"id"}).AddRow("tok-1")
mock.ExpectQuery(`SELECT t\.id\s+FROM workspace_auth_tokens t`).
WithArgs(sqlmock.AnyArg()).
WillReturnRows(rows)
// ValidateAnyToken also fires a best-effort last_used_at UPDATE after
// successful validation. Accept it so ExpectationsWereMet passes.
mock.ExpectExec(`UPDATE workspace_auth_tokens SET last_used_at`).
WithArgs(sqlmock.AnyArg()).
WillReturnResult(sqlmock.NewResult(0, 1))
h := NewTerminalHandler(nil) // nil docker → local path
w := httptest.NewRecorder()