diff --git a/workspace-server/internal/artifacts/client_test.go b/workspace-server/internal/artifacts/client_test.go index d386ba2c..1be79525 100644 --- a/workspace-server/internal/artifacts/client_test.go +++ b/workspace-server/internal/artifacts/client_test.go @@ -192,7 +192,7 @@ func TestForkRepo_Success(t *testing.T) { return } var req map[string]interface{} - json.NewDecoder(r.Body).Decode(&req) + _ = json.NewDecoder(r.Body).Decode(&req) if req["name"] != "forked-repo" { http.Error(w, "unexpected fork name", http.StatusBadRequest) return @@ -234,7 +234,7 @@ func TestImportRepo_Success(t *testing.T) { return } var req map[string]interface{} - json.NewDecoder(r.Body).Decode(&req) + _ = json.NewDecoder(r.Body).Decode(&req) if req["url"] == "" { http.Error(w, "url required", http.StatusBadRequest) return @@ -294,7 +294,7 @@ func TestCreateToken_Success(t *testing.T) { return } var req map[string]interface{} - json.NewDecoder(r.Body).Decode(&req) + _ = json.NewDecoder(r.Body).Decode(&req) if req["repo"] != "my-repo" { http.Error(w, "unexpected repo", http.StatusBadRequest) return diff --git a/workspace-server/internal/channels/channels_test.go b/workspace-server/internal/channels/channels_test.go index 6def5408..a308eef1 100644 --- a/workspace-server/internal/channels/channels_test.go +++ b/workspace-server/internal/channels/channels_test.go @@ -617,7 +617,7 @@ func TestDisableChannelByChatID_WiredSetsEnabledFalse(t *testing.T) { if err != nil { t.Fatalf("sqlmock: %v", err) } - t.Cleanup(func() { mockDB.Close() }) + t.Cleanup(func() { _ = mockDB.Close() }) prevDB := db.DB db.DB = mockDB t.Cleanup(func() { db.DB = prevDB }) @@ -757,7 +757,7 @@ func TestDisableChannelByChatID_NoRowsAffectedSkipsReload(t *testing.T) { // bot), the UPDATE returns RowsAffected=0 and we skip the reload. Verifies // we don't emit a spurious log or SELECT storm on unrelated kicked events. mockDB, mock, _ := sqlmock.New(sqlmock.QueryMatcherOption(sqlmock.QueryMatcherRegexp)) - t.Cleanup(func() { mockDB.Close() }) + t.Cleanup(func() { _ = mockDB.Close() }) prevDB := db.DB db.DB = mockDB t.Cleanup(func() { db.DB = prevDB }) diff --git a/workspace-server/internal/channels/lark_test.go b/workspace-server/internal/channels/lark_test.go index c90a4f66..47d04d7b 100644 --- a/workspace-server/internal/channels/lark_test.go +++ b/workspace-server/internal/channels/lark_test.go @@ -94,7 +94,7 @@ func TestLarkAdapter_SendMessage_HappyPath(t *testing.T) { gotBody = string(b) w.Header().Set("Content-Type", "application/json") w.WriteHeader(200) - w.Write([]byte(`{"code":0,"msg":"ok"}`)) + _, _ = w.Write([]byte(`{"code":0,"msg":"ok"}`)) })) defer srv.Close() @@ -115,7 +115,7 @@ func TestLarkAdapter_SendMessage_HappyPath(t *testing.T) { if err != nil { t.Fatal(err) } - resp.Body.Close() + _ = resp.Body.Close() if gotPath != "/open-apis/bot/v2/hook/test" { t.Errorf("path: got %q", gotPath) diff --git a/workspace-server/internal/channels/manager.go b/workspace-server/internal/channels/manager.go index 9c1c320e..0991d520 100644 --- a/workspace-server/internal/channels/manager.go +++ b/workspace-server/internal/channels/manager.go @@ -128,7 +128,7 @@ func (m *Manager) PausePollersForToken(workspaceID, botToken string) func() { if err != nil { return func() {} } - defer rows.Close() + defer func() { _ = rows.Close() }() var pausedIDs []string m.mu.Lock() @@ -193,7 +193,7 @@ func (m *Manager) Reload(ctx context.Context) { log.Printf("Channels: reload query error: %v", err) return } - defer rows.Close() + defer func() { _ = rows.Close() }() desired := make(map[string]ChannelRow) for rows.Next() { @@ -203,8 +203,8 @@ func (m *Manager) Reload(ctx context.Context) { log.Printf("Channels: reload scan error: %v", err) continue } - json.Unmarshal(configJSON, &ch.Config) - json.Unmarshal(allowedJSON, &ch.AllowedUsers) + _ = json.Unmarshal(configJSON, &ch.Config) + _ = json.Unmarshal(allowedJSON, &ch.AllowedUsers) // #319: decrypt at the boundary between DB (ciphertext) and the // in-memory config adapters consume. A decrypt failure logs and // skips the channel — downstream getUpdates would fail anyway diff --git a/workspace-server/internal/handlers/container_files_test.go b/workspace-server/internal/handlers/container_files_test.go new file mode 100644 index 00000000..ace31397 --- /dev/null +++ b/workspace-server/internal/handlers/container_files_test.go @@ -0,0 +1,152 @@ +package handlers + +// container_files_test.go — CWE-22 regression suite for copyFilesToContainer. +// +// Vulnerability: copyFilesToContainer validated the raw filename before +// filepath.Join(destPath, name) but placed the post-join result in the tar +// header. A mid-path traversal such as "foo/../../../etc" passes the prefix +// check (does not start with "..") yet resolves to /etc after the join, +// escaping the volume mount and writing outside the container's filesystem. +// +// Fix (PR #1434): re-validate archiveName after filepath.Join using +// filepath.Clean, then use the cleaned result in the tar header. +// A Docker client is not required for these tests — the validation rejects +// unsafe paths before any Docker call is made. + +import ( + "context" + "errors" + "testing" +) + +func TestCopyFilesToContainer_CWE22_RejectsTraversal(t *testing.T) { + // TemplatesHandler with nil docker — validation runs before any Docker call. + h := &TemplatesHandler{docker: nil} + + ctx := context.Background() + + tests := []struct { + label string + destPath string + files map[string]string + wantErr bool + errSubstr string // substring that must appear in error message + }{ + // ── Legitimate paths ─────────────────────────────────────────────────── + { + label: "simple_relative_path_ok", + destPath: "/configs", + files: map[string]string{"config.yaml": "key: value"}, + wantErr: false, + }, + { + label: "nested_relative_path_ok", + destPath: "/configs", + files: map[string]string{"subdir/script.sh": "#!/bin/sh"}, + wantErr: false, + }, + { + label: "dot_in_filename_ok", + destPath: "/configs", + files: map[string]string{"app.venv/config": "data"}, + wantErr: false, + }, + // ── CWE-22: absolute-path prefix ──────────────────────────────────────── + { + label: "absolute_path_rejected", + destPath: "/configs", + files: map[string]string{"/etc/passwd": "malicious"}, + wantErr: true, + errSubstr: "unsafe file path", + }, + // ── CWE-22: leading ".." prefix ───────────────────────────────────────── + { + label: "leading_dotdot_rejected", + destPath: "/configs", + files: map[string]string{"../etc/passwd": "malicious"}, + wantErr: true, + errSubstr: "unsafe file path", + }, + // ── CWE-22: mid-path traversal (the regression case) ──────────────────── + // "foo/../../../etc" does NOT start with ".." — passed the old check. + // After filepath.Join("/configs", "foo/../../../etc") → Clean → /etc + // (absolute), escaping the volume mount. Rejected by the post-join guard. + { + label: "mid_path_traversal_rejected", + destPath: "/configs", + files: map[string]string{"foo/../../../etc/cron.d/malicious": "* * * * * root echo pwned"}, + wantErr: true, + errSubstr: "path escapes destination", + }, + { + label: "mid_path_traversal_escapes_configs", + destPath: "/configs", + files: map[string]string{"x/y/../../../../../../../etc/shadow": "malicious"}, + wantErr: true, + errSubstr: "path escapes destination", + }, + { + label: "double_dotdot_in_subpath_rejected", + destPath: "/workspace", + files: map[string]string{"a/../../../workspace/somefile": "data"}, + wantErr: true, + errSubstr: "path escapes destination", + }, + // ── CWE-22: traversal targeting parent of destPath ─────────────────────── + { + label: "escapes_destpath_via_traversal", + destPath: "/configs", + files: map[string]string{"..%2F..%2F..%2Fsecrets": "data"}, // URL-encoded "../" — still a traversal + wantErr: true, + errSubstr: "path escapes destination", + }, + // ── Mixed: valid entry + traversal entry ──────────────────────────────── + { + label: "one_traversal_in_map_rejected", + destPath: "/configs", + files: map[string]string{"good.txt": "valid", "foo/../../../evil": "bad"}, + wantErr: true, + errSubstr: "path escapes destination", + }, + } + + for _, tc := range tests { + t.Run(tc.label, func(t *testing.T) { + err := h.copyFilesToContainer(ctx, "any-container", tc.destPath, tc.files) + if tc.wantErr { + if err == nil { + t.Errorf("want non-nil error, got nil") + return + } + if tc.errSubstr != "" && !errors.Is(err, context.DeadlineExceeded) && + !contains(err.Error(), tc.errSubstr) { + t.Errorf("error %q does not contain %q", err.Error(), tc.errSubstr) + } + } else { + // wantErr == false: we expect nil from a nil-docker call. + // With nil docker the function will panic or return a docker-err + // only if the path check is bypassed. We use a strict check: + // any error other than a docker-initialized error means the path + // was incorrectly allowed. + if err != nil && contains(err.Error(), "unsafe") { + t.Errorf("want nil (path accepted), got error: %v", err) + } + } + }) + } +} + +// contains is a simple substring check (no external imports needed in this file). +func contains(s, substr string) bool { + return len(s) >= len(substr) && (s == substr || len(substr) == 0 || + (len(s) > 0 && len(substr) > 0 && searchSubstring(s, substr))) +} + +func searchSubstring(s, substr string) bool { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} diff --git a/workspace-server/internal/handlers/terminal.go b/workspace-server/internal/handlers/terminal.go index 18b1b4cc..0735bbfd 100644 --- a/workspace-server/internal/handlers/terminal.go +++ b/workspace-server/internal/handlers/terminal.go @@ -14,10 +14,12 @@ import ( "github.com/Molecule-AI/molecule-monorepo/platform/internal/db" "github.com/Molecule-AI/molecule-monorepo/platform/internal/provisioner" - "github.com/creack/pty" + "github.com/Molecule-AI/molecule-monorepo/platform/internal/registry" + "github.com/Molecule-AI/molecule-monorepo/platform/internal/wsauth" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/container" "github.com/docker/docker/client" + "github.com/creack/pty" "github.com/gin-gonic/gin" "github.com/gorilla/websocket" ) @@ -78,12 +80,43 @@ func (h *TerminalHandler) HandleConnect(c *gin.Context) { // handleLocalConnect attaches to a Docker container running on this // tenant's Docker daemon. Original behavior preserved exactly. func (h *TerminalHandler) handleLocalConnect(c *gin.Context, workspaceID string) { +// canCommunicateCheck is the communication-authorization predicate used by +// HandleConnect to enforce the KI-005 workspace-hierarchy guard. +// Exposed as a package var so tests can stub it without DB fixtures. +var canCommunicateCheck = registry.CanCommunicate + +// HandleConnect handles WS /workspaces/:id/terminal +func (h *TerminalHandler) HandleConnect(c *gin.Context) { + targetID := c.Param("id") + ctx := c.Request.Context() + + // KI-005 fix: enforce CanCommunicate hierarchy check before granting + // terminal access. WorkspaceAuth validates the bearer's token, but the + // token is scoped to a specific workspace ID — Workspace A's token can + // reach Workspace A's terminal. Without CanCommunicate, Workspace A could + // also reach Workspace B's terminal if it knows B's UUID (enumeration + // via canvas, logs, or delegation). Shell access is more dangerous than + // A2A message-passing, so we apply the same hierarchy check here. + callerID := c.GetHeader("X-Workspace-ID") + if callerID != "" { + tok := wsauth.BearerTokenFromHeader(c.GetHeader("Authorization")) + if tok != "" { + if err := wsauth.ValidateAnyToken(ctx, db.DB, tok); err == nil { + if !canCommunicateCheck(callerID, targetID) { + c.JSON(http.StatusForbidden, gin.H{"error": "not authorized to access this workspace's terminal"}) + return + } + } + } + } + if h.docker == nil { c.JSON(http.StatusServiceUnavailable, gin.H{"error": "Docker not available"}) return } ctx := c.Request.Context() + workspaceID := targetID // Try multiple container name patterns: // 1. Provisioner naming: ws-{id[:12]} diff --git a/workspace-server/internal/handlers/terminal_test.go b/workspace-server/internal/handlers/terminal_test.go index 8664467b..6a24ea2a 100644 --- a/workspace-server/internal/handlers/terminal_test.go +++ b/workspace-server/internal/handlers/terminal_test.go @@ -57,6 +57,37 @@ func TestHandleConnect_RoutesToLocal(t *testing.T) { if w.Code != http.StatusServiceUnavailable { t.Errorf("local branch should 503 when Docker is unavailable; got %d", w.Code) +// TestTerminalConnect_KI005_RejectsUnauthorizedCrossWorkspace tests the KI-005 +// regression fix: workspace A must NOT be able to open a terminal on workspace B's +// container, even with a valid bearer token, unless they share a parent/child +// relationship. The vulnerability existed because HandleConnect only checked +// WorkspaceAuth (valid bearer → any :id) without the CanCommunicate hierarchy guard. +func TestTerminalConnect_KI005_RejectsUnauthorizedCrossWorkspace(t *testing.T) { + mock := setupTestDB(t) + // Stub CanCommunicate so it always returns false (no relationship). + // Reset after test to avoid polluting other tests. + prev := canCommunicateCheck + 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"). + WithArgs(sqlmock.AnyArg()). + WillReturnRows(rows) + + h := NewTerminalHandler(nil) // nil docker → local path + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-target"}} + c.Request = httptest.NewRequest("GET", "/workspaces/ws-target/terminal", nil) + c.Request.Header.Set("X-Workspace-ID", "ws-caller") + c.Request.Header.Set("Authorization", "Bearer valid-token-for-ws-caller") + + h.HandleConnect(c) + + if w.Code != http.StatusForbidden { + t.Errorf("cross-workspace terminal: got %d, want 403 (%s)", w.Code, w.Body.String()) } if err := mock.ExpectationsWereMet(); err != nil { t.Errorf("unmet sqlmock expectations: %v", err) @@ -113,5 +144,108 @@ func TestSSHCommandCmd_BuildsArgv(t *testing.T) { if cmd.Args[i] != want[i] { t.Errorf("argv[%d] = %q, want %q", i, cmd.Args[i], want[i]) } +// TestTerminalConnect_KI005_AllowsOwnTerminal tests the flip side of KI-005: +// a workspace must still be able to access its own terminal. The CanCommunicate +// fast-path returns true when callerID == targetID. +func TestTerminalConnect_KI005_AllowsOwnTerminal(t *testing.T) { + // CanCommunicate fast-path: callerID == targetID → returns true without DB. + prev := canCommunicateCheck + canCommunicateCheck = func(callerID, targetID string) bool { return callerID == targetID } + defer func() { canCommunicateCheck = prev }() + + h := NewTerminalHandler(nil) // nil docker → 503 if reached + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-alice"}} + c.Request = httptest.NewRequest("GET", "/workspaces/ws-alice/terminal", nil) + c.Request.Header.Set("X-Workspace-ID", "ws-alice") + c.Request.Header.Set("Authorization", "Bearer valid-token") + + h.HandleConnect(c) + + // Got 503 (nil docker) instead of 403 — means CanCommunicate passed + // and we reached the Docker path, which is correct. + if w.Code != http.StatusServiceUnavailable { + t.Errorf("own-terminal pass-through: got %d, want 503 nil-docker (%s)", w.Code, w.Body.String()) } } + +// TestTerminalConnect_KI005_SkipsCheckWithoutHeader tests the allowlist path: +// callers that don't send X-Workspace-ID (canvas/molecli with bearer-only auth) +// skip the CanCommunicate check entirely and fall through to the Docker auth path. +// We assert they get the nil-docker 503 instead of 403. +func TestTerminalConnect_KI005_SkipsCheckWithoutHeader(t *testing.T) { + h := NewTerminalHandler(nil) // nil docker → 503 if reached + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-any"}} + c.Request = httptest.NewRequest("GET", "/workspaces/ws-any/terminal", nil) + // No X-Workspace-ID header → KI-005 check is skipped + + h.HandleConnect(c) + + // Got 503 (nil docker) instead of 403 — means KI-005 check was skipped + // and we reached the Docker path, which is correct. + if w.Code != http.StatusServiceUnavailable { + t.Errorf("no X-Workspace-ID: got %d, want 503 nil-docker (%s)", w.Code, w.Body.String()) + } +} + +// TestTerminalConnect_KI005_RejectsInvalidToken tests that an invalid bearer +// token also results in a non-200 response (falls through to Docker auth). +// ValidateAnyToken returns error → CanCommunicate is never called. +func TestTerminalConnect_KI005_RejectsInvalidToken(t *testing.T) { + canCommunicateCalled := false + prev := canCommunicateCheck + canCommunicateCheck = func(callerID, targetID string) bool { + canCommunicateCalled = true + return true + } + defer func() { canCommunicateCheck = prev }() + + h := NewTerminalHandler(nil) + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-target"}} + c.Request = httptest.NewRequest("GET", "/workspaces/ws-target/terminal", nil) + c.Request.Header.Set("X-Workspace-ID", "ws-caller") + c.Request.Header.Set("Authorization", "Bearer invalid-token") + + h.HandleConnect(c) + + if canCommunicateCalled { + t.Error("CanCommunicate should not be called with an invalid token") + } + // Got 503 (nil docker) instead of 200/403 — ValidateAnyToken rejected the + // token and we fell through to Docker auth, which returned 503 (nil docker). + if w.Code != http.StatusServiceUnavailable { + t.Errorf("invalid token: got %d, want 503 nil-docker (%s)", w.Code, w.Body.String()) + } +} + +// TestTerminalConnect_KI005_AllowsSiblingWorkspace tests the sibling path: +// two workspaces with the same parent ID should be allowed to communicate. +func TestTerminalConnect_KI005_AllowsSiblingWorkspace(t *testing.T) { + prev := canCommunicateCheck + canCommunicateCheck = func(callerID, targetID string) bool { + // Simulate sibling: same parent + return callerID == "ws-pm" && targetID == "ws-dev" + } + defer func() { canCommunicateCheck = prev }() + + h := NewTerminalHandler(nil) + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-dev"}} + c.Request = httptest.NewRequest("GET", "/workspaces/ws-dev/terminal", nil) + c.Request.Header.Set("X-Workspace-ID", "ws-pm") + c.Request.Header.Set("Authorization", "Bearer valid-token") + + h.HandleConnect(c) + + // CanCommunicate returned true → reached Docker path → 503 nil-docker + if w.Code != http.StatusServiceUnavailable { + t.Errorf("sibling access: got %d, want 503 nil-docker (%s)", w.Code, w.Body.String()) + } +} +