fix(store): protect items.slug against the import path, and make restoration atomic

[P1] items.slug was unprotected, and the exclusion was true of one write path
and false of another — the lesson this cluster keeps re-teaching. The API path
derives the slug through slugify, whose [a-z0-9-] output cannot carry a NUL.
ImportWorkspace has its OWN INSERT and writes the BUNDLE's slug verbatim:
importCoercedSlug returns it unchanged whenever it is inside the length bound,
so a crafted bundle puts any bytes it likes there. That is code I wrote in S1.

[P1] Six more caller-controlled columns were unprotected: items.created_by,
items.last_modified_by, items.source, item_versions.created_by,
item_versions.source, item_links.created_by — the handlers let a request body's
value win over the server's own. Plus custom_templates.icon.

[P1] Trigger restoration was not atomic: 206 CREATE statements outside a
transaction leave a window where some tables are protected and others are not,
and a concurrent writer can commit an invalid row inside it. It runs in one
transaction now.

[P1] The restoration check compared a COUNT and matched with LIKE. A database
with the right number of triggers but one missing and one extra read as
healthy, and IF NOT EXISTS would then never repair the missing one. It compares
the SET now, and matches with GLOB — LIKE's `_` is a single-character wildcard,
so the old pattern also matched names this code never generates.

[P2] The census baseline still listed columns that had become protected, and
the test never asserted the two sets are disjoint — so losing a column's
protection would have put it back in `unaccounted`, found it still listed, and
passed. Disjointness is asserted and the baseline regenerated.

[P2] And the finding I want on the record, because my first fix for it was
worse than the gap. Codex was right that testing classifyTriggerRefusal with a
synthetic error would pass even if the wrapper stopped calling it. I added an
"integration" leg that wrote through a guarded connection and asserted the
typed error came back. It PASSED — and the refusal came from LAYER A, whose
error is the same TYPE, so errors.As succeeded while the trigger was never
involved.

There is no value that Layer A accepts and Layer B refuses: both implement the
same predicate, and the four-way differential test asserts they agree on the
whole corpus. The unreachability IS the property, so a reachable case would be
testing a disagreement we work to prevent. The leg is replaced by a structural
one asserting every wrapper error path routes through the classifier, and the
comment says why there is no end-to-end alternative rather than implying the
gap was closed.

Full Go suite green on SQLite and Postgres 17; lint 0 issues.

Claude-Session: https://claude.ai/code/session_01XLtX4dbjBpApbAv3SuBcTm
This commit is contained in:
xarmian
2026-09-01 17:35:36 +00:00
parent 46c55e32f3
commit f789eaac21
5 changed files with 299 additions and 29 deletions
@@ -555,6 +555,24 @@ BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: custom_templates.doc_type must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_custom_templates_icon_ins
BEFORE INSERT ON custom_templates
FOR EACH ROW WHEN NEW.icon IS NOT NULL AND (
instr(NEW.icon, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: custom_templates.icon must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_custom_templates_icon_upd
BEFORE UPDATE OF icon ON custom_templates
FOR EACH ROW WHEN NEW.icon IS NOT NULL AND (
instr(NEW.icon, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: custom_templates.icon must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_custom_templates_name_ins
BEFORE INSERT ON custom_templates
FOR EACH ROW WHEN NEW.name IS NOT NULL AND (
@@ -697,6 +715,24 @@ BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: event_outbox.payload must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_item_links_created_by_ins
BEFORE INSERT ON item_links
FOR EACH ROW WHEN NEW.created_by IS NOT NULL AND (
instr(NEW.created_by, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: item_links.created_by must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_item_links_created_by_upd
BEFORE UPDATE OF created_by ON item_links
FOR EACH ROW WHEN NEW.created_by IS NOT NULL AND (
instr(NEW.created_by, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: item_links.created_by must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_item_versions_change_summary_ins
BEFORE INSERT ON item_versions
FOR EACH ROW WHEN NEW.change_summary IS NOT NULL AND (
@@ -733,6 +769,42 @@ BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: item_versions.content must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_item_versions_created_by_ins
BEFORE INSERT ON item_versions
FOR EACH ROW WHEN NEW.created_by IS NOT NULL AND (
instr(NEW.created_by, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: item_versions.created_by must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_item_versions_created_by_upd
BEFORE UPDATE OF created_by ON item_versions
FOR EACH ROW WHEN NEW.created_by IS NOT NULL AND (
instr(NEW.created_by, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: item_versions.created_by must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_item_versions_source_ins
BEFORE INSERT ON item_versions
FOR EACH ROW WHEN NEW.source IS NOT NULL AND (
instr(NEW.source, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: item_versions.source must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_item_versions_source_upd
BEFORE UPDATE OF source ON item_versions
FOR EACH ROW WHEN NEW.source IS NOT NULL AND (
instr(NEW.source, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: item_versions.source must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_item_wiki_links_display_text_ins
BEFORE INSERT ON item_wiki_links
FOR EACH ROW WHEN NEW.display_text IS NOT NULL AND (
@@ -805,6 +877,24 @@ BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: items.content must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_items_created_by_ins
BEFORE INSERT ON items
FOR EACH ROW WHEN NEW.created_by IS NOT NULL AND (
instr(NEW.created_by, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: items.created_by must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_items_created_by_upd
BEFORE UPDATE OF created_by ON items
FOR EACH ROW WHEN NEW.created_by IS NOT NULL AND (
instr(NEW.created_by, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: items.created_by must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_items_fields_ins
BEFORE INSERT ON items
FOR EACH ROW WHEN NEW.fields IS NOT NULL AND (
@@ -831,6 +921,60 @@ BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: items.fields must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_items_last_modified_by_ins
BEFORE INSERT ON items
FOR EACH ROW WHEN NEW.last_modified_by IS NOT NULL AND (
instr(NEW.last_modified_by, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: items.last_modified_by must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_items_last_modified_by_upd
BEFORE UPDATE OF last_modified_by ON items
FOR EACH ROW WHEN NEW.last_modified_by IS NOT NULL AND (
instr(NEW.last_modified_by, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: items.last_modified_by must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_items_slug_ins
BEFORE INSERT ON items
FOR EACH ROW WHEN NEW.slug IS NOT NULL AND (
instr(NEW.slug, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: items.slug must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_items_slug_upd
BEFORE UPDATE OF slug ON items
FOR EACH ROW WHEN NEW.slug IS NOT NULL AND (
instr(NEW.slug, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: items.slug must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_items_source_ins
BEFORE INSERT ON items
FOR EACH ROW WHEN NEW.source IS NOT NULL AND (
instr(NEW.source, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: items.source must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_items_source_upd
BEFORE UPDATE OF source ON items
FOR EACH ROW WHEN NEW.source IS NOT NULL AND (
instr(NEW.source, char(0)) > 0
)
BEGIN
SELECT RAISE(ABORT, 'pad_nul_invariant: items.source must not contain a NUL');
END;
CREATE TRIGGER IF NOT EXISTS pad_nul_items_tags_ins
BEFORE INSERT ON items
FOR EACH ROW WHEN NEW.tags IS NOT NULL AND (
@@ -6,7 +6,6 @@ activities.user_id
activities.workspace_id
agent_roles.created_at
agent_roles.id
agent_roles.slug
agent_roles.updated_at
agent_roles.workspace_id
api_tokens.created_at
@@ -44,13 +43,11 @@ collection_grants.workspace_id
collections.created_at
collections.deleted_at
collections.id
collections.slug
collections.updated_at
collections.workspace_id
comment_reactions.actor
comment_reactions.comment_id
comment_reactions.created_at
comment_reactions.emoji
comment_reactions.id
comment_reactions.user_id
comments.activity_id
@@ -64,7 +61,6 @@ comments.updated_at
comments.user_id
comments.workspace_id
custom_templates.created_at
custom_templates.icon
custom_templates.id
custom_templates.updated_at
custom_templates.workspace_id
@@ -109,7 +105,6 @@ item_grants.permission
item_grants.user_id
item_grants.workspace_id
item_links.created_at
item_links.created_by
item_links.id
item_links.link_type
item_links.source_id
@@ -120,10 +115,8 @@ item_stars.created_at
item_stars.item_id
item_stars.user_id
item_versions.created_at
item_versions.created_by
item_versions.id
item_versions.item_id
item_versions.source
item_versions.user_id
item_wiki_links.source_item_id
item_wiki_links.target_item_id
@@ -144,14 +137,11 @@ items.assigned_user_id
items.collection_id
items.content_flushed_at
items.created_at
items.created_by
items.created_by_user_id
items.deleted_at
items.id
items.last_modified_by
items.last_modified_by_user_id
items.parent_id
items.source
items.updated_at
items.workspace_id
mcp_audit_log.args_hash
@@ -177,7 +167,6 @@ oauth_authorization_codes.requested_at
oauth_authorization_codes.signature
oauth_clients.created_at
oauth_clients.id
oauth_clients.logo_url
oauth_clients.token_endpoint_auth_method
oauth_connection_workspaces.added_at
oauth_connection_workspaces.added_by
@@ -265,7 +254,6 @@ versions.source
views.collection_id
views.created_at
views.id
views.slug
views.updated_at
views.view_type
views.workspace_id
@@ -296,6 +284,5 @@ workspaces.created_at
workspaces.deleted_at
workspaces.id
workspaces.owner_id
workspaces.slug
workspaces.source
workspaces.updated_at
+83 -16
View File
@@ -59,8 +59,20 @@ var nulColumns = []nulColumn{
{"items", "tags", classJSON},
{"items", "title", classText},
{"items", "content", classText},
// items.slug is deliberately absent: slugify emits only [a-z0-9-], so a
// NUL cannot survive into it.
// items.slug IS protected (codex round 2), and the reasoning that excluded
// it was true of one write path and false of another — the lesson this
// cluster keeps re-teaching. The API path derives the slug through slugify,
// whose [a-z0-9-] output cannot carry a NUL. ImportWorkspace has its OWN
// INSERT and writes the BUNDLE's slug verbatim: importCoercedSlug returns
// it unchanged whenever it is inside the length bound, so a crafted bundle
// puts any bytes it likes in this column.
{"items", "slug", classText},
// Attribution columns. The handlers let a request body's value win over the
// server's own, so these carry caller text.
{"items", "created_by", classText},
{"items", "last_modified_by", classText},
{"items", "source", classText},
// collections
{"collections", "schema", classJSON},
@@ -85,6 +97,9 @@ var nulColumns = []nulColumn{
// versions
{"item_versions", "content", classText},
{"item_versions", "change_summary", classText},
{"item_versions", "created_by", classText},
{"item_versions", "source", classText},
{"item_links", "created_by", classText},
{"versions", "content", classText},
{"versions", "change_summary", classText},
@@ -127,6 +142,7 @@ var nulColumns = []nulColumn{
{"custom_templates", "content", classText},
{"custom_templates", "name", classText},
{"custom_templates", "description", classText},
{"custom_templates", "icon", classText},
{"custom_templates", "doc_type", classText}, // second ring
// webhooks
@@ -168,9 +184,9 @@ var nulColumns = []nulColumn{
// CALLER-SUPPLIED SLUGS (codex round 1).
//
// items.slug is excluded because it is DERIVED — ItemCreate has no Slug
// field, so slugify's [a-z0-9-] output is the only thing that reaches it.
// That reasoning does NOT transfer to these: CreateWorkspace and
// These are caller-supplied, and were missed because items.slug's
// derived-only reasoning was read as covering slugs generally.
// CreateWorkspace and
// CreateCollection both start with `slug := input.Slug` and only fall back
// to slugify when the caller supplied none. The census's exclusion note was
// right about items and was read as covering slugs generally.
@@ -256,7 +272,6 @@ var nulExcluded = map[string]string{
"mcp_audit_log.error_kind": "server enum, mcp_audit.go",
"users.recovery_codes": "newline-joined bcrypt hashes of server-generated codes; looks like JSON, is not",
"workspace_members.collection_access": "validated enum all/selected",
"items.slug": "DERIVED: ItemCreate has no Slug field, so slugify's [a-z0-9-] output is the only thing that reaches it. NOTE this reasoning is specific to items — workspaces, collections, views and agent_roles all accept a caller-supplied slug and ARE protected.",
"item_yjs_updates.update_data": "BINARY (BLOB/BYTEA), the only such column in either schema. Raw Yjs updates legitimately contain NUL bytes; Layer A exempts it for the same reason and TestBinaryColumnCensus pins that. Surfaced here when the census's type filter was widened to include BLOB affinity, which is correct — the decision to exclude it is a judgement, not an oversight.",
}
@@ -277,14 +292,32 @@ func (s *Store) ensureNULTriggers() error {
return nil
}
var have int
if err := s.db.QueryRow(
`SELECT COUNT(*) FROM sqlite_master WHERE type='trigger' AND name LIKE 'pad_nul_%'`,
).Scan(&have); err != nil {
return fmt.Errorf("count NUL triggers: %w", err)
want := map[string]bool{}
for _, c := range NULProtectedColumns() {
want["pad_nul_"+c.Table+"_"+c.Column+"_ins"] = true
want["pad_nul_"+c.Table+"_"+c.Column+"_upd"] = true
}
want := len(NULProtectedColumns()) * 2 // one BEFORE INSERT + one BEFORE UPDATE each
if have == want {
// The SET, not the count (codex round 2). A database with the right NUMBER
// of triggers but a missing one and an extra one read as healthy, and
// CREATE TRIGGER IF NOT EXISTS would then never repair the missing one.
//
// Matched with GLOB rather than LIKE because LIKE's `_` is a single-
// character wildcard, so 'pad_nul_%' also matches names this code never
// generates — a loose pattern in a health check is a health check that can
// be satisfied by the wrong thing.
have, err := s.currentNULTriggers()
if err != nil {
return err
}
missing := false
for name := range want {
if !have[name] {
missing = true
break
}
}
if !missing {
return nil
}
@@ -292,15 +325,49 @@ func (s *Store) ensureNULTriggers() error {
if err != nil {
return fmt.Errorf("read %s: %w", nulTriggerMigration, err)
}
if err := execMulti(s.db, string(data)); err != nil {
// IN ONE TRANSACTION (codex round 2). Running 206 CREATE statements outside
// a transaction leaves a window in which some tables are protected and
// others are not, and a concurrent writer can commit an invalid row inside
// it. SQLite's DDL is transactional, so the restoration is all-or-nothing.
tx, err := s.db.Begin()
if err != nil {
return fmt.Errorf("begin trigger restore: %w", err)
}
defer func() { _ = tx.Rollback() }()
if err := execMulti(tx, string(data)); err != nil {
return fmt.Errorf("restore NUL triggers: %w", err)
}
if err := tx.Commit(); err != nil {
return fmt.Errorf("commit trigger restore: %w", err)
}
slog.Warn("NUL invariant triggers were missing and have been restored — a table rebuild most likely "+
"dropped them; the rows written while they were absent are NOT checked",
"had", have, "want", want)
"dropped them; rows written while they were absent are NOT retroactively checked",
"had", len(have), "want", len(want))
return nil
}
// currentNULTriggers reads the trigger names actually present.
func (s *Store) currentNULTriggers() (map[string]bool, error) {
rows, err := s.db.Query(
`SELECT name FROM sqlite_master WHERE type = 'trigger' AND name GLOB 'pad_nul_*'`,
)
if err != nil {
return nil, fmt.Errorf("list NUL triggers: %w", err)
}
defer rows.Close()
out := map[string]bool{}
for rows.Next() {
var n string
if err := rows.Scan(&n); err != nil {
return nil, err
}
out[n] = true
}
return out, rows.Err()
}
// nulTriggerMigration is the generated file, named once so the generator, the
// re-assertion and the pin test all refer to the same artifact.
const nulTriggerMigration = "084_nul_invariant_triggers.sql"
+19
View File
@@ -146,6 +146,25 @@ func TestNULColumnCensus(t *testing.T) {
"baseline with GEN_NUL_BASELINE=1 — but decide first.",
len(newlyUnaccounted), strings.Join(newlyUnaccounted, "\n "))
}
// A baseline entry that has become PROTECTED is a stale record, and leaving
// it there is what lets a protection REGRESSION pass silently: the column
// would drop out of the protected set, land back in `unaccounted`, find
// itself still listed in the baseline, and read as expected (codex round
// 2). The two sets must stay disjoint.
var protectedButBaselined []string
for key := range known {
if protected[key] {
protectedButBaselined = append(protectedButBaselined, key)
}
}
sort.Strings(protectedButBaselined)
if len(protectedButBaselined) > 0 {
t.Errorf("%d column(s) are BOTH protected and recorded as unprotected:\n %s\n\n"+
"Remove them from nul_unprotected_baseline.txt. While they are listed there, losing their "+
"protection would not fail this test.",
len(protectedButBaselined), strings.Join(protectedButBaselined, "\n "))
}
if len(goneFromBaseline) > 0 {
t.Errorf("%d baseline column(s) are no longer in the schema:\n %s\n\n"+
"A removed or renamed column is worth a look — a RENAME means the old name's exemption now "+
+53
View File
@@ -175,6 +175,59 @@ func TestNULTriggersMatchTheList(t *testing.T) {
// exercise this is to classify the driver error directly and, separately, to
// prove an unguarded write produces a message the classifier recognises.
func TestTriggerRefusalIsIndistinguishableFromLayerA(t *testing.T) {
// WHY THERE IS NO END-TO-END LEG HERE, stated rather than faked.
//
// Codex round 2 was right that testing classifyTriggerRefusal with a
// synthetic error would pass even if the wrapper stopped calling it. My
// first answer was an "integration" leg that wrote a nested-document value
// through a guarded connection and asserted the typed error came back. It
// PASSED — and for the wrong reason: the refusal came from LAYER A, whose
// message ("parameter 1: value is a JSON document...") is the same TYPE, so
// errors.As succeeded while the trigger was never involved.
//
// Reaching a trigger through a guarded connection needs a value Layer A
// ACCEPTS and Layer B REFUSES. By construction there is none: both
// implement the same predicate, and the four-way differential test asserts
// exactly that they agree on the whole corpus. The unreachability IS the
// property, so a test that manufactured a reachable case would be testing a
// disagreement we have gone to some trouble to prevent.
//
// What is testable, and is: (a) the classifier's behaviour, below; (b) that
// the marker it parses is the one the migration emits, below; and (c) that
// every error path in the wrapper passes through it, structurally — the
// arm-parity shape from the item-title unit, which exists because
// "somebody will add a fifth return and forget" is the failure that
// actually happens.
t.Run("every wrapper error path routes through the classifier", func(t *testing.T) {
src, err := os.ReadFile("nulguard.go")
if err != nil {
t.Fatalf("read nulguard.go: %v", err)
}
body := string(src)
// The four driver entry points that can surface a database error.
for _, fn := range []string{
"func (c guardConn) ExecContext(",
"func (c guardConn) QueryContext(",
"func (s guardStmt) ExecContext(",
"func (s guardStmt) QueryContext(",
} {
i := strings.Index(body, fn)
if i < 0 {
t.Errorf("%s not found — the instrument is out of step with the code, so its silence "+
"means nothing", fn)
continue
}
end := strings.Index(body[i:], "\n}\n")
if end < 0 {
t.Fatalf("could not find the end of %s", fn)
}
if !strings.Contains(body[i:i+end], "classifyTriggerRefusal") {
t.Errorf("%s returns a driver error without classifying it — a Layer B refusal down that "+
"path reaches the handler as a 500 instead of a 400", fn)
}
}
})
t.Run("a trigger abort classifies as the Layer A error", func(t *testing.T) {
raw := errors.New("SQL logic error: pad_nul_invariant: activities.user_agent must not contain a NUL (1)")
got := classifyTriggerRefusal(raw)