mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
test: add regression guard for chat title masking (#24584)
Follow-up to #24564 addressing unresolved review findings. - **DEREM-1**: Add `Test_diff/Chat/TitleMasked` to `enterprise/audit/diff_internal_test.go` so flipping `title` back to `ActionTrack` fails loudly. Verified: the case passes today, fails with a clear diff after flipping to `ActionTrack`, passes again after reverting. - **DEREM-4**: Inline comment at `coderd/audit/request.go:138` explaining why `ResourceTarget` for `database.Chat` returns a UUID prefix instead of the title. - **DEREM-5**: Trailing comment on `enterprise/audit/table.go` `title` entry, matching the surrounding `ActionSecret` comment style. Won't-fix, with rationale (per user): - **DEREM-2** (8-char prefix collision risk): `resource_target` is a display hint, not an identifier; the full UUID lives in `resource_id`. - **DEREM-3** (named constant for `[:8]`): single call site; extracting would be ceremony. - **DEREM-6** (PR title misleading): merged PR title is immutable. - **DEREM-7** (historical log redaction): the offending version only shipped to dogfood for a couple of hours and not to customers. > 🤖
This commit is contained in:
@@ -135,6 +135,12 @@ func ResourceTarget[T Auditable](tgt T) string {
|
||||
case database.AiSeatState:
|
||||
return "AI Seat"
|
||||
case database.Chat:
|
||||
// Chat titles can contain sensitive content (secrets, internal
|
||||
// project names), so we use a short UUID prefix as a display
|
||||
// hint instead. The full UUID is still recorded in resource_id,
|
||||
// which is what the audit UI links on. An 8-char prefix is fine
|
||||
// for display; collisions affect the display label and search
|
||||
// filter but not the primary resource identifier.
|
||||
return typed.ID.String()[:8]
|
||||
default:
|
||||
panic(fmt.Sprintf("unknown resource %T for ResourceTarget", tgt))
|
||||
|
||||
@@ -4,10 +4,12 @@ import (
|
||||
"context"
|
||||
"testing"
|
||||
|
||||
"github.com/google/uuid"
|
||||
"github.com/stretchr/testify/require"
|
||||
"go.opentelemetry.io/otel/propagation"
|
||||
|
||||
"github.com/coder/coder/v2/coderd/audit"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
)
|
||||
|
||||
func TestBaggage(t *testing.T) {
|
||||
@@ -31,3 +33,15 @@ func TestBaggage(t *testing.T) {
|
||||
|
||||
require.Equal(t, expected, got)
|
||||
}
|
||||
|
||||
func TestResourceTarget_ChatTitleNotLeaked(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
chat := database.Chat{
|
||||
ID: uuid.UUID{1},
|
||||
Title: "sensitive-project-name",
|
||||
}
|
||||
target := audit.ResourceTarget(chat)
|
||||
require.NotContains(t, target, chat.Title,
|
||||
"ResourceTarget for Chat must not contain the title; it should use a UUID prefix")
|
||||
}
|
||||
|
||||
@@ -367,6 +367,29 @@ func Test_diff(t *testing.T) {
|
||||
},
|
||||
})
|
||||
|
||||
runDiffTests(t, []diffTest{
|
||||
{
|
||||
// Chat titles can contain sensitive content, so they must be
|
||||
// masked in audit diffs via ActionSecret. This case guards
|
||||
// against a regression where title is flipped back to
|
||||
// ActionTrack in enterprise/audit/table.go.
|
||||
name: "TitleMasked",
|
||||
left: audit.Empty[database.Chat](),
|
||||
right: database.Chat{
|
||||
ID: uuid.UUID{1},
|
||||
OwnerID: uuid.UUID{2},
|
||||
WorkspaceID: uuid.NullUUID{UUID: uuid.UUID{3}, Valid: true},
|
||||
Title: "a very secret chat title",
|
||||
},
|
||||
exp: audit.Map{
|
||||
"id": audit.OldNew{Old: "", New: uuid.UUID{1}.String()},
|
||||
"owner_id": audit.OldNew{Old: "", New: uuid.UUID{2}.String()},
|
||||
"workspace_id": audit.OldNew{Old: "null", New: uuid.UUID{3}.String()},
|
||||
"title": audit.OldNew{Old: "", New: "", Secret: true},
|
||||
},
|
||||
},
|
||||
})
|
||||
|
||||
runDiffTests(t, []diffTest{
|
||||
{
|
||||
name: "Create",
|
||||
|
||||
@@ -386,18 +386,18 @@ var auditableResourcesTypes = map[any]map[string]Action{
|
||||
"workspace_id": ActionTrack,
|
||||
"build_id": ActionIgnore, // Internal lifecycle.
|
||||
"agent_id": ActionIgnore, // Internal lifecycle.
|
||||
"title": ActionSecret,
|
||||
"title": ActionSecret, // May contain sensitive content.
|
||||
"status": ActionIgnore, // Churns every message.
|
||||
"worker_id": ActionIgnore, // Internal.
|
||||
"started_at": ActionIgnore,
|
||||
"heartbeat_at": ActionIgnore,
|
||||
"heartbeat_at": ActionIgnore, // Internal.
|
||||
"created_at": ActionIgnore, // Never changes.
|
||||
"updated_at": ActionIgnore, // Bumped on every mutation.
|
||||
"parent_chat_id": ActionIgnore, // Immutable after creation.
|
||||
"root_chat_id": ActionIgnore, // Immutable after creation.
|
||||
"last_model_config_id": ActionIgnore, // Churns every message.
|
||||
"archived": ActionTrack,
|
||||
"last_error": ActionIgnore,
|
||||
"last_error": ActionIgnore, // Internal.
|
||||
"mode": ActionTrack,
|
||||
"mcp_server_ids": ActionTrack,
|
||||
"labels": ActionTrack,
|
||||
|
||||
Reference in New Issue
Block a user