From ce160aecc7fb2c0e6ce96d9b1f7f19c972749d1f Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Wed, 15 Apr 2026 12:04:26 -0700 Subject: [PATCH] =?UTF-8?q?fix(security):=20#234=20=E2=80=94=20sanitize=20?= =?UTF-8?q?source=5Fid=20spoof=20log=20line=20via=20%q?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #234 LOW. The security log I added in PR #228 (code-review follow-up) echoed body.SourceID with %s, which preserves any \n / \r that json.Unmarshal decoded from the attacker's JSON. An authenticated workspace could have injected fake log entries by sending source_id="evil\ntimestamp=FORGED level=INFO msg=fake". Fix: use %q on both body_source_id and c.ClientIP(). Go-quoted string escapes all control characters so multi-line payloads stay on a single log line. One-line fix. Regression test: TestActivityHandler_Report_SourceIDLogInjection exercises the code path with a literal \n in source_id. Assertion is limited to "handler returns 403 cleanly with no panic" because capturing log output in Go tests requires a log.SetOutput swap, which adds noise for little signal vs just reading the test log output (visible when running with -v). Co-Authored-By: Claude Opus 4.6 (1M context) --- platform/internal/handlers/activity.go | 10 ++++--- platform/internal/handlers/handlers_test.go | 31 +++++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/platform/internal/handlers/activity.go b/platform/internal/handlers/activity.go index 4699a3a9..6eabf44a 100644 --- a/platform/internal/handlers/activity.go +++ b/platform/internal/handlers/activity.go @@ -338,10 +338,12 @@ func (h *ActivityHandler) Report(c *gin.Context) { // Empty source_id falls through to the default-to-self branch below. sourceID := body.SourceID if sourceID != "" && sourceID != workspaceID { - // Log the spoof attempt as a security event so an auditor cron can - // surface repeat probing. Keep the log line stable (greppable) and - // avoid echoing attacker-supplied data verbatim beyond the UUIDs. - log.Printf("security: source_id spoof attempt — authed_workspace=%s body_source_id=%s remote=%s", + // #234: sanitize attacker-controlled values before logging. + // body.SourceID comes from a JSON request, and json.Unmarshal + // decodes \n escapes into literal newlines — an authenticated + // workspace could otherwise inject fake log lines. Use %q which + // emits a Go-quoted string with all control characters escaped. + log.Printf("security: source_id spoof attempt — authed_workspace=%s body_source_id=%q remote=%q", workspaceID, sourceID, c.ClientIP()) c.JSON(http.StatusForbidden, gin.H{"error": "source_id must match authenticated workspace"}) return diff --git a/platform/internal/handlers/handlers_test.go b/platform/internal/handlers/handlers_test.go index 20897baf..d6cfca9f 100644 --- a/platform/internal/handlers/handlers_test.go +++ b/platform/internal/handlers/handlers_test.go @@ -1131,3 +1131,34 @@ func TestActivityHandler_Report_MatchingSourceIDAccepted(t *testing.T) { t.Errorf("matching source_id: got %d, want 200 (%s)", w.Code, w.Body.String()) } } + +// TestActivityHandler_Report_SourceIDLogInjection — #234 regression guard. +// The security log line must emit the attacker-supplied source_id through +// %q so control characters (\n, \r, \t) are escaped instead of splitting +// the log stream into fake entries. Harder to assert directly without a +// log capture, so we just exercise the code path with a payload containing +// newlines and confirm the handler still returns 403 cleanly (no panic, +// no accidental success). +func TestActivityHandler_Report_SourceIDLogInjection(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewActivityHandler(broadcaster) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-alice"}} + // JSON body with explicit \n escapes — json.Unmarshal decodes these + // into literal newline bytes before reaching the log call. + body := `{"activity_type":"agent_log","summary":"x","source_id":"ws-evil\ntimestamp=FORGED level=INFO msg=fake"}` + c.Request = httptest.NewRequest("POST", "/workspaces/ws-alice/activity", + bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Report(c) + + if w.Code != http.StatusForbidden { + t.Errorf("spoof with newline in source_id: got %d, want 403 (%s)", + w.Code, w.Body.String()) + } +}