forked from molecule-ai/molecule-core
fix(security): #234 — sanitize source_id spoof log line via %q
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) <noreply@anthropic.com>
This commit is contained in:
parent
3735068de7
commit
ce160aecc7
@ -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
|
||||
|
||||
@ -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())
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user