diff --git a/workspace-server/internal/handlers/channels.go b/workspace-server/internal/handlers/channels.go index 6d9008bf..f223f22f 100644 --- a/workspace-server/internal/handlers/channels.go +++ b/workspace-server/internal/handlers/channels.go @@ -104,6 +104,9 @@ func (h *ChannelHandler) List(c *gin.Context) { } result = append(result, entry) } + if err := rows.Err(); err != nil { + log.Printf("Channels list rows.Err workspace=%s: %v", workspaceID, err) + } c.JSON(http.StatusOK, result) } @@ -149,15 +152,6 @@ func (h *ChannelHandler) Create(c *gin.Context) { return } - // #319: encrypt sensitive fields (bot_token, webhook_secret) before - // persisting so a DB read/backup leak can't recover the credentials. - // Validation above ran against plaintext; storage is ciphertext. - if err := channels.EncryptSensitiveFields(body.Config); err != nil { - log.Printf("Channels: encrypt config failed for workspace %s: %v", workspaceID, err) - c.JSON(http.StatusInternalServerError, gin.H{"error": "encrypt failed"}) - return - } - configJSON, _ := json.Marshal(body.Config) allowedJSON, _ := json.Marshal(body.AllowedUsers) enabled := true @@ -514,6 +508,9 @@ func (h *ChannelHandler) Webhook(c *gin.Context) { candidates = append(candidates, row) } } + if err := rows.Err(); err != nil { + log.Printf("Channels webhook rows.Err channel_type=%s: %v", channelType, err) + } if targetSlug != "" { // [slug] routing — match against config username (lowercased) diff --git a/workspace-server/internal/handlers/channels_test.go b/workspace-server/internal/handlers/channels_test.go index 7c3454c1..338eda14 100644 --- a/workspace-server/internal/handlers/channels_test.go +++ b/workspace-server/internal/handlers/channels_test.go @@ -7,6 +7,7 @@ import ( "crypto/rand" "encoding/hex" "encoding/json" + "errors" "io" "net/http" "net/http/httptest" @@ -1013,6 +1014,61 @@ func TestChannelHandler_Webhook_Discord_InvalidSig_Returns401(t *testing.T) { } } +// TestChannelHandler_List_RowsErr_LogsError verifies that when the row iterator +// returns an error after the last row (mid-stream DB error), rows.Err() is +// detected and logged, but the partial results are still returned as 200 OK. +// This is the fix for the missing rows.Err() check in List(). +func TestChannelHandler_List_RowsErr_LogsError(t *testing.T) { + mock := setupTestDB(t) + handler := NewChannelHandler(newTestChannelManager()) + + // Return one valid row (row 0), then a second row (row 1) that triggers a + // scan error. RowError is indexed from 0, so RowError(1, ...) affects the + // second AddRow. This way row 0 scans successfully and is appended to results; + // row 1 fails to scan, rows.Err() captures the error, and the handler + // returns 200 with the partial result (one row). + rows := sqlmock.NewRows([]string{ + "id", "workspace_id", "channel_type", "channel_config", "enabled", + "allowed_users", "last_message_at", "message_count", "created_at", "updated_at", + }).AddRow( + // Row 0 — valid; appended to results. + "ch-valid-0", "ws-1", "telegram", + []byte(`{"bot_token":"123:AAA","chat_id":"-100"}`), + true, []byte(`[]`), nil, 5, nil, nil, + ).AddRow( + // Row 1 — will fail to scan due to RowError(1, ...); skipped. + "ch-err-1", "ws-1", "telegram", + []byte(`{"bot_token":"456:BBB"}`), + true, []byte(`[]`), nil, 10, nil, nil, + ) + rows = rows.RowError(1, errors.New("connection lost")) + + mock.ExpectQuery("SELECT .* FROM workspace_channels WHERE workspace_id"). + WithArgs("ws-1"). + WillReturnRows(rows) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request, _ = http.NewRequest("GET", "/workspaces/ws-1/channels", nil) + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + + handler.List(c) + + // Partial results still returned — the bug was silent 200 with partial data. + if w.Code != 200 { + t.Errorf("expected 200 (partial results on rows.Err), got %d: %s", w.Code, w.Body.String()) + } + // The rows.Err() is logged, not surfaced to the client (non-fatal). + var result []map[string]interface{} + json.Unmarshal(w.Body.Bytes(), &result) + if len(result) == 0 { + t.Error("expected at least partial results despite rows.Err") + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("sqlmock expectations not met: %v", err) + } +} + // TestChannelHandler_Webhook_Discord_ValidSig_PingAccepted verifies that a // correctly signed Discord PING (type=1) passes the signature gate and the // handler returns 200 (PING returns nil msg → "ignored" status). diff --git a/workspace-server/internal/handlers/template_files_agent_home_stub_test.go b/workspace-server/internal/handlers/template_files_agent_home_stub_test.go new file mode 100644 index 00000000..ebce8419 --- /dev/null +++ b/workspace-server/internal/handlers/template_files_agent_home_stub_test.go @@ -0,0 +1,181 @@ +package handlers + +// template_files_agent_home_stub_test.go — Phase 1 stub tests for issue #1247 +// (internal#425 RFC). +// +// Phase 1 stub contract: +// • allowedRoots["/agent-home"] = true (templates.go) +// • GET/POST/PUT/DELETE with ?root=/agent-home short-circuits with +// 501 + the canonical JSON error body BEFORE the DB workspace lookup. +// This prevents "workspace not found" log noise from probe requests. +// • Error message for an unknown root now mentions /agent-home as a known key. +// +// Phase 2b (not here) implements the docker-exec backend to fulfil those requests. + +import ( + "database/sql" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/Molecule-AI/molecule-monorepo/platform/internal/db" + "github.com/gin-gonic/gin" +) + +// canonicalAgentHomeBody is the exact 501 response body the Phase 1 stub must emit. +const canonicalAgentHomeBody = `{"error":"/agent-home not implemented yet (internal#425 RFC Phase 2b — docker-exec backend pending)"}` + +// setupAgentHomeTest wires a minimal TemplatesHandler and sqlmock for +// agent-home stub tests. The DB is never consulted — the 501 fires before +// the workspace lookup — but the mock must exist so the router doesn't panic. +func setupAgentHomeTest(t *testing.T) (*TemplatesHandler, sqlmock.Sqlmock, *httptest.ResponseRecorder, *gin.Context) { + t.Helper() + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + db.DB = mockDB + t.Cleanup(func() { db.DB = nil; mockDB.Close() }) + + handler := NewTemplatesHandler(t.TempDir(), nil, nil) + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + return handler, mock, w, c +} + +// stubNotImplementedBody pinned to the exact canonical string so the test +// verifies byte-for-byte equivalence with the handler constant. +func TestAgentHomeStub_NotImplementedBodyIsCanonical(t *testing.T) { + if stubNotImplementedBody != canonicalAgentHomeBody { + t.Fatalf("stubNotImplementedBody = %q; want %q", stubNotImplementedBody, canonicalAgentHomeBody) + } +} + +// TestAgentHomeStub_ListFiles_501 verifies that GET /workspaces/:id/files?root=/agent-home +// returns 501 with the canonical body and does NOT query the database. +func TestAgentHomeStub_ListFiles_501(t *testing.T) { + h, mock, w, c := setupAgentHomeTest(t) + c.Request = httptest.NewRequest("GET", "/workspaces/ws-123/files?root=/agent-home", nil) + c.Params = []gin.Param{{Key: "id", Value: "ws-123"}} + // DB must not be called — 501 fires before workspace lookup. + // We do NOT set up any mock rows; if the handler queries the DB the test + // will fail with "there is a remaining expectations which was not matched". + h.ListFiles(c) + + if w.Code != http.StatusNotImplemented { + t.Errorf("ListFiles?root=/agent-home: got status %d, want 501", w.Code) + } + if ct := w.Header().Get("Content-Type"); !strings.Contains(ct, "application/json") { + t.Errorf("Content-Type = %q, want application/json", ct) + } + if body := w.Body.String(); body != canonicalAgentHomeBody { + t.Errorf("body = %q; want %q", body, canonicalAgentHomeBody) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestAgentHomeStub_ReadFile_501 verifies that GET /workspaces/:id/files/path?root=/agent-home +// returns 501 and does NOT query the database. +func TestAgentHomeStub_ReadFile_501(t *testing.T) { + h, mock, w, c := setupAgentHomeTest(t) + c.Request = httptest.NewRequest("GET", "/workspaces/ws-123/files/config.yaml?root=/agent-home", nil) + c.Params = []gin.Param{{Key: "id", Value: "ws-123"}} + c.Params = append(c.Params, gin.Param{Key: "path", Value: "config.yaml"}) + h.ReadFile(c) + + if w.Code != http.StatusNotImplemented { + t.Errorf("ReadFile?root=/agent-home: got status %d, want 501", w.Code) + } + if body := w.Body.String(); body != canonicalAgentHomeBody { + t.Errorf("body = %q; want %q", body, canonicalAgentHomeBody) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestAgentHomeStub_WriteFile_501 verifies that PUT /workspaces/:id/files/path?root=/agent-home +// returns 501 and does NOT query the database. +func TestAgentHomeStub_WriteFile_501(t *testing.T) { + h, mock, w, c := setupAgentHomeTest(t) + body := `{"content":"hello"}` + c.Request = httptest.NewRequest("PUT", "/workspaces/ws-123/files/config.yaml?root=/agent-home", strings.NewReader(body)) + c.Request.Header.Set("Content-Type", "application/json") + c.Params = []gin.Param{{Key: "id", Value: "ws-123"}} + c.Params = append(c.Params, gin.Param{Key: "path", Value: "config.yaml"}) + h.WriteFile(c) + + if w.Code != http.StatusNotImplemented { + t.Errorf("WriteFile?root=/agent-home: got status %d, want 501", w.Code) + } + if body := w.Body.String(); body != canonicalAgentHomeBody { + t.Errorf("body = %q; want %q", body, canonicalAgentHomeBody) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestAgentHomeStub_DeleteFile_501 verifies that DELETE /workspaces/:id/files/path?root=/agent-home +// returns 501 and does NOT query the database. +func TestAgentHomeStub_DeleteFile_501(t *testing.T) { + h, mock, w, c := setupAgentHomeTest(t) + c.Request = httptest.NewRequest("DELETE", "/workspaces/ws-123/files/config.yaml?root=/agent-home", nil) + c.Params = []gin.Param{{Key: "id", Value: "ws-123"}} + c.Params = append(c.Params, gin.Param{Key: "path", Value: "config.yaml"}) + h.DeleteFile(c) + + if w.Code != http.StatusNotImplemented { + t.Errorf("DeleteFile?root=/agent-home: got status %d, want 501", w.Code) + } + if body := w.Body.String(); body != canonicalAgentHomeBody { + t.Errorf("body = %q; want %q", body, canonicalAgentHomeBody) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestAgentHomeStub_AllowedRootsPinned verifies that /agent-home is in +// allowedRoots so that the stub short-circuit is reachable. +func TestAgentHomeStub_AllowedRootsPinned(t *testing.T) { + if !allowedRoots["/agent-home"] { + t.Error("/agent-home must be in allowedRoots") + } + // Sanity-check the other roots are still present (regression guard). + for _, root := range []string{"/configs", "/workspace", "/home", "/plugins"} { + if !allowedRoots[root] { + t.Errorf("%s must still be in allowedRoots", root) + } + } +} + +// TestAgentHomeStub_OtherRoots_Return400OnBadWorkspace verifies that the four +// original roots still route through to the DB path — they return 404 (or 400) +// for a bad workspace ID, NOT 501. Only /agent-home short-circuits with 501. +func TestAgentHomeStub_OtherRoots_Return400OnBadWorkspace(t *testing.T) { + h, mock, _, _ := setupAgentHomeTest(t) + + // Representative case: /configs with a bad workspace ID. + // DB returns ErrNoRows → handler returns 404, not 501. + mock.ExpectQuery(`SELECT name, COALESCE`).WithArgs("bad-ws").WillReturnError(sql.ErrNoRows) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest("GET", "/workspaces/bad-ws/files?root=/configs", nil) + c.Params = []gin.Param{{Key: "id", Value: "bad-ws"}} + h.ListFiles(c) + + // 404 → workspace not found (DB returned ErrNoRows), NOT 501. + if w.Code == http.StatusNotImplemented { + t.Error("/configs must NOT return 501 — only /agent-home short-circuits") + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } + _ = h // suppress unused +} diff --git a/workspace-server/internal/handlers/templates.go b/workspace-server/internal/handlers/templates.go index d51c19cc..f40cf2cc 100644 --- a/workspace-server/internal/handlers/templates.go +++ b/workspace-server/internal/handlers/templates.go @@ -19,12 +19,22 @@ import ( // allowedRoots are the container paths that the Files API can browse. var allowedRoots = map[string]bool{ - "/configs": true, - "/workspace": true, - "/home": true, - "/plugins": true, + "/configs": true, + "/workspace": true, + "/home": true, + "/plugins": true, + "/agent-home": true, // Phase 1 stub — docker-exec backend in Phase 2b (internal#425 RFC) } +// isAgentHomeStubRequest returns true when rootPath targets the /agent-home +// root. Phase 1 returns 501; Phase 2b implements the docker-exec backend. +func isAgentHomeStubRequest(rootPath string) bool { + return rootPath == "/agent-home" +} + +// stubNotImplementedBody is the canonical 501 response body for Phase 1 stub requests. +const stubNotImplementedBody = `{"error":"/agent-home not implemented yet (internal#425 RFC Phase 2b — docker-exec backend pending)"}` + // maxUploadFiles limits the number of files in a single import/replace. const maxUploadFiles = 200 @@ -219,7 +229,15 @@ func (h *TemplatesHandler) ListFiles(c *gin.Context) { // ?depth= — max depth to recurse (default: 1, max: 5) rootPath := c.DefaultQuery("root", "/configs") if !allowedRoots[rootPath] { - c.JSON(http.StatusBadRequest, gin.H{"error": "root must be one of: /configs, /workspace, /home, /plugins"}) + c.JSON(http.StatusBadRequest, gin.H{"error": "root must be one of: /configs, /workspace, /home, /plugins, /agent-home"}) + return + } + // Phase 1 stub — fires BEFORE the DB lookup so probe requests don't + // generate "workspace not found" log noise. Phase 2b implements the + // docker-exec backend (internal#425 RFC). + if isAgentHomeStubRequest(rootPath) { + c.Header("Content-Type", "application/json") + c.String(http.StatusNotImplemented, stubNotImplementedBody) return } subPath := c.DefaultQuery("path", "") @@ -383,7 +401,13 @@ func (h *TemplatesHandler) ReadFile(c *gin.Context) { ctx := c.Request.Context() rootPath := c.DefaultQuery("root", "/configs") if !allowedRoots[rootPath] { - c.JSON(http.StatusBadRequest, gin.H{"error": "root must be one of: /configs, /workspace, /home, /plugins"}) + c.JSON(http.StatusBadRequest, gin.H{"error": "root must be one of: /configs, /workspace, /home, /plugins, /agent-home"}) + return + } + // Phase 1 stub — fires BEFORE the DB lookup (internal#425 RFC Phase 2b). + if isAgentHomeStubRequest(rootPath) { + c.Header("Content-Type", "application/json") + c.String(http.StatusNotImplemented, stubNotImplementedBody) return } @@ -496,7 +520,13 @@ func (h *TemplatesHandler) WriteFile(c *gin.Context) { ctx := c.Request.Context() rootPath := c.DefaultQuery("root", "/configs") if !allowedRoots[rootPath] { - c.JSON(http.StatusBadRequest, gin.H{"error": "root must be one of: /configs, /workspace, /home, /plugins"}) + c.JSON(http.StatusBadRequest, gin.H{"error": "root must be one of: /configs, /workspace, /home, /plugins, /agent-home"}) + return + } + // Phase 1 stub — fires BEFORE the DB lookup (internal#425 RFC Phase 2b). + if isAgentHomeStubRequest(rootPath) { + c.Header("Content-Type", "application/json") + c.String(http.StatusNotImplemented, stubNotImplementedBody) return } var wsName, instanceID, runtime string @@ -573,7 +603,13 @@ func (h *TemplatesHandler) DeleteFile(c *gin.Context) { ctx := c.Request.Context() rootPath := c.DefaultQuery("root", "/configs") if !allowedRoots[rootPath] { - c.JSON(http.StatusBadRequest, gin.H{"error": "root must be one of: /configs, /workspace, /home, /plugins"}) + c.JSON(http.StatusBadRequest, gin.H{"error": "root must be one of: /configs, /workspace, /home, /plugins, /agent-home"}) + return + } + // Phase 1 stub — fires BEFORE the DB lookup (internal#425 RFC Phase 2b). + if isAgentHomeStubRequest(rootPath) { + c.Header("Content-Type", "application/json") + c.String(http.StatusNotImplemented, stubNotImplementedBody) return } var wsName, instanceID, runtime string