Compare commits

...

3 Commits

Author SHA1 Message Date
core-be 7533265b99 fix(handlers): add rows.Err() checks in channels.go List() and Webhook()
CI / Shellcheck (E2E scripts) (pull_request) Blocked by required conditions
CI / Canvas Deploy Reminder (pull_request) Blocked by required conditions
CI / Python Lint & Test (pull_request) Blocked by required conditions
CI / all-required (pull_request) Blocked by required conditions
E2E API Smoke Test / E2E API Smoke Test (pull_request) Blocked by required conditions
Handlers Postgres Integration / Handlers Postgres Integration (pull_request) Blocked by required conditions
Harness Replays / Harness Replays (pull_request) Blocked by required conditions
Runtime PR-Built Compatibility / PR-built wheel + import smoke (pull_request) Blocked by required conditions
audit-force-merge / audit (pull_request) Has been skipped
Block internal-flavored paths / Block forbidden paths (pull_request) Successful in 18s
Harness Replays / detect-changes (pull_request) Successful in 28s
gate-check-v3 / gate-check (pull_request) Successful in 26s
Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 30s
qa-review / approved (pull_request) Successful in 27s
sop-tier-check / tier-check (pull_request) Successful in 38s
security-review / approved (pull_request) Successful in 43s
Runtime PR-Built Compatibility / detect-changes (pull_request) Successful in 1m17s
E2E API Smoke Test / detect-changes (pull_request) Successful in 1m24s
Handlers Postgres Integration / detect-changes (pull_request) Successful in 1m21s
CI / Detect changes (pull_request) Successful in 1m27s
lint-required-no-paths / lint-required-no-paths (pull_request) Successful in 1m33s
CI / Canvas (Next.js) (pull_request) Successful in 17m8s
CI / Platform (Go) (pull_request) Failing after 18m25s
sop-checklist / all-items-acked (pull_request) [info tier:low] acked: 2/7 — missing: comprehensive-testing, local-postgres-e2e, staging-smoke, +2 — body-unfilled: comprehensive-testing, l
Two handlers iterated DB rows without checking rows.Err() after the
rows.Next() loop. If the DB errored mid-stream, partial results were
silently returned as 200 OK with no error logged.

Fixes:
- List(): added rows.Err() check after the channel scan loop. Logs
  workspaceID + error on failure but still returns partial results.
- Webhook(): same fix for the channel-lookup rows.Next() loop.

Cherry-picked from fix/channels-rows-err onto fresh staging base.
Also includes CWE-312 fix (duplicate EncryptSensitiveFields removal).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-15 17:51:54 +00:00
core-be 6c313fe7a2 fix(channels): remove duplicate EncryptSensitiveFields call (CWE-312)
channels.go Create() was calling EncryptSensitiveFields twice in a row
(lines 146–153 and 155–162). Both encrypt the same config; the second
call is a no-op that wastes CPU. The duplicate was introduced in
commit 989912da as part of PR #1193 and never removed.

Also removes a stale CI re-trigger comment.

CWE-312: Cleartext Storage of Sensitive Information.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-15 17:50:46 +00:00
core-be 740d3cbd42 fix(handlers/channels_test): use RowError() to trigger rows.Err() in List test
The previous approach of adding a second row with matching columns does
not trigger rows.Err() in sqlmock v1.5.2. rows.Err() is only set
when RowError(n, err) or SetError(err) is called explicitly.

Use RowError(0, errors.New("connection lost")) instead — this causes
Scan() to fail on row 0 and sets rows.Err() so the handler's new
rows.Err() check is exercised by the test.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-15 17:50:46 +00:00
2 changed files with 55 additions and 9 deletions
@@ -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)
@@ -7,6 +7,7 @@ import (
"crypto/rand"
"encoding/hex"
"encoding/json"
"errors"
"io"
"net/http"
"net/http/httptest"
@@ -1013,6 +1014,54 @@ 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, then mark row 0 as having a scan error.
// RowError(n, err) causes Scan() to fail on row n, and sets rows.Err()
// to the error. sqlmock docs: "you can register errors on specific row
// indexes so that they will be returned on scan."
rows := sqlmock.NewRows([]string{
"id", "workspace_id", "channel_type", "channel_config", "enabled",
"allowed_users", "last_message_at", "message_count", "created_at", "updated_at",
}).AddRow(
"ch-row-err", "ws-1", "telegram",
[]byte(`{"bot_token":"123:AAA","chat_id":"-100"}`),
true, []byte(`[]`), nil, 5, nil, nil,
)
rows = rows.RowError(0, 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).