diff --git a/cmd/pad/cmd_db.go b/cmd/pad/cmd_db.go index 1320a008d..8be9d1800 100644 --- a/cmd/pad/cmd_db.go +++ b/cmd/pad/cmd_db.go @@ -1106,7 +1106,7 @@ func preflightNULForMigration(src *store.Store, dst *store.Store, fromPath strin if n := len(elsewhere); n > 0 { fmt.Fprintf(os.Stderr, - "\nNOTE: %d value(s) carrying a NUL are in tables this migration does not copy\n"+ + "\nNOTE: %d value(s) carrying a NUL or invalid UTF-8 are in tables this migration does not copy\n"+ "(users, platform settings, auth data). They do not block it, and\n"+ "'%s' will repair them:\n\n", n, repairNULCommandHint) for _, v := range elsewhere { @@ -1131,8 +1131,8 @@ func preflightNULForMigration(src *store.Store, dst *store.Store, fromPath strin // two differ. fmt.Fprintf(os.Stderr, " %s (destination refused it; no layer of ours sees this one)\n", sus) } - fmt.Fprintf(os.Stderr, "\nEach carries a NUL, which PostgreSQL refuses in a text or jsonb value —\n"+ - "SQLSTATE 22021 and 22P05. Migrating risks failing partway through the copy, after\n"+ + fmt.Fprintf(os.Stderr, "\nEach carries a NUL or invalid UTF-8. PostgreSQL refuses a NUL in text (SQLSTATE 22021),\n"+ + "a NUL escape in jsonb (22P05) and invalid UTF-8 (22021). Migrating risks failing partway through the copy, after\n"+ "some workspaces have already moved, so it is refused up front.\n\n"+ "Nothing has been migrated. Repair them first:\n\n %s\n\n"+ "then re-run this command. To see the same list without migrating: pad db scan-nul\n", @@ -1146,7 +1146,7 @@ func preflightNULForMigration(src *store.Store, dst *store.Store, fromPath strin // running the command against a real Postgres, not by a test: the tests // asserted the message CONTAINED "nothing was migrated" and never read the // number. - return fmt.Errorf("%d stored value(s) carry a NUL; nothing was migrated", total) + return fmt.Errorf("%d stored value(s) carry a NUL or invalid UTF-8; nothing was migrated", total) } // suspectFailure pairs a suspect with the destination's complaint. diff --git a/cmd/pad/cmd_db_nul.go b/cmd/pad/cmd_db_nul.go index 6bb957449..d3e781b4b 100644 --- a/cmd/pad/cmd_db_nul.go +++ b/cmd/pad/cmd_db_nul.go @@ -42,15 +42,23 @@ func dbScanNULCmd() *cobra.Command { cmd := &cobra.Command{ Use: "scan-nul", - Short: "Report stored values carrying a NUL (read-only)", + Short: "Report stored values PostgreSQL refuses: a NUL, or invalid UTF-8 (read-only)", Long: `Counts and locates every stored value that violates Pad's NUL invariant: a real NUL byte in any protected column, or a JSON escape in a JSON column -that a JSON parser would decode to one. +that a JSON parser would decode to one. It also reports every value that is +not valid UTF-8. That is not a NUL, but PostgreSQL refuses it too (SQLSTATE +22021 under a UTF8 database), so it breaks a migration in the same way. + +Finding invalid UTF-8 means reading every stored value in full, since SQLite +has no way to test for it in a query; on a large database that takes about as +long as reading the database once. Such rows can only have been written by a binary older than the enforcement that now refuses them. They are not cosmetic: their workspace exports fine and re-imports with a 400, and 'pad db migrate-to-pg' fails partway through the -copy against PostgreSQL's jsonb parser. +copy against PostgreSQL. A SQL_ASCII PostgreSQL would accept the invalid-UTF-8 +bytes; they are reported anyway, because nothing on the Pad side can tell which +encoding a later migration will target. The scan itself only reads. Opening the database does apply any pending schema migrations, exactly as starting the server does — pass --from to inspect a @@ -90,10 +98,13 @@ func dbRepairNULCmd() *cobra.Command { cmd := &cobra.Command{ Use: "repair-nul", - Short: "Replace stored NULs with U+FFFD (rewrites user content)", - Long: `Rewrites every stored value 'pad db scan-nul' reports, replacing each NUL -with U+FFFD (the Unicode replacement character) and leaving the rest of the -value byte for byte as it was. + Short: "Replace stored NULs and invalid UTF-8 with U+FFFD (rewrites user content)", + Long: `Rewrites every stored value 'pad db scan-nul' reports, replacing each NUL, +and each invalid UTF-8 byte sequence, with U+FFFD (the Unicode replacement +character) and leaving the rest of the value byte for byte as it was. In a valid +JSON document an invalid byte can only sit inside a string, so the document +stays valid JSON with that string's bad bytes replaced; a value that was not +valid JSON to begin with is not made valid by this. THIS CHANGES USER CONTENT. It is a separate command, and never part of a migration, for that reason: a migration that rewrote stored text would decide @@ -151,8 +162,8 @@ row.`, } if !force { - fmt.Fprintf(os.Stderr, "\nThis will rewrite the %d value(s) above, replacing each NUL with "+ - "U+FFFD, and inspect %d suspect value(s) — rewriting only those that hide a NUL behind "+ + fmt.Fprintf(os.Stderr, "\nThis will rewrite the %d value(s) above, replacing each NUL and each "+ + "invalid UTF-8 byte sequence with U+FFFD, and inspect %d suspect value(s) — rewriting only those that hide a NUL behind "+ "a repeated key.\n", scan.Total(), len(scan.Suspects)) fmt.Fprintf(os.Stderr, "Run with --force to skip this confirmation, or press Ctrl+C to abort.\n") fmt.Fprintf(os.Stderr, "Continue? [y/N] ") @@ -245,7 +256,8 @@ func resolveNULToolsTarget(fromPath *string) (proceed bool, err error) { if os.Getenv("PAD_DB_DRIVER") == "postgres" { fmt.Fprintln(os.Stderr, "This deployment is PostgreSQL, which refuses these values natively (SQLSTATE 22021 for a\n"+ - "NUL in text, 22P05 for the escape reaching jsonb), so no stored row can carry one.\n"+ + "NUL in text or invalid UTF-8, 22P05 for the escape reaching jsonb), so no stored row can\n"+ + "carry one.\n"+ "Nothing to scan or repair.") return false, nil } @@ -294,12 +306,12 @@ func printNULScanReport(w io.Writer, report *store.NULScanReport, dbPath string) } if report.Total() == 0 { - fmt.Fprintln(w, "No values carrying a NUL were found.") + fmt.Fprintln(w, "No values carrying a NUL or invalid UTF-8 were found.") printNULSuspects(w, report) return } - fmt.Fprintf(w, "\nFound %d value(s) carrying a NUL:\n\n", report.Total()) + fmt.Fprintf(w, "\nFound %d value(s) carrying a NUL or invalid UTF-8:\n\n", report.Total()) byColumn := report.ByColumn() for _, key := range sortedCountKeys(byColumn) { diff --git a/cmd/pad/cmd_db_nul_test.go b/cmd/pad/cmd_db_nul_test.go index 844a7e854..5e991a313 100644 --- a/cmd/pad/cmd_db_nul_test.go +++ b/cmd/pad/cmd_db_nul_test.go @@ -656,3 +656,79 @@ func plantActivitySuspect(t *testing.T, dbPath, value string) { t.Fatalf("plant: %v", err) } } + +// TestPreflightRefusesInvalidUTF8 is BUG-3222: PostgreSQL refuses a value that +// is not valid UTF-8 (SQLSTATE 22021), so the preflight refuses on one in a +// table the migration copies, and reports without blocking one in a table it +// does not. +// +// The last leg is the case BUG-3220 must keep refused after deleting the NUL +// suspect path: an invalid byte in a JSON value that ALSO carries the escape +// text as a harmless literal. Before BUG-3222 only the destination cast on +// suspects caught it, by coincidence. +func TestPreflightRefusesInvalidUTF8(t *testing.T) { + bad := string([]byte{0x80}) + backslash := textguard.EscNUL[:1] + + newSource := func(t *testing.T) (*store.Store, string, string) { + t.Helper() + dbPath := filepath.Join(t.TempDir(), "src.db") + s, err := store.New(dbPath) + if err != nil { + t.Fatalf("open source: %v", err) + } + t.Cleanup(func() { s.Close() }) + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "UTF8"}) + if err != nil { + t.Fatalf("create workspace: %v", err) + } + return s, dbPath, ws.ID + } + + t.Run("control: valid multibyte text passes", func(t *testing.T) { + s, path, wsID := newSource(t) + plantNULInWorkspaceName(t, path, wsID, "é"+string(rune(0x4E2D))+string(rune(0x1F600))) + if err := preflightNULForMigration(s, nil, path); err != nil { + t.Fatalf("the preflight refused valid UTF-8: %v", err) + } + }) + + t.Run("invalid UTF-8 in a migrated text column refuses", func(t *testing.T) { + s, path, wsID := newSource(t) + plantNULInWorkspaceName(t, path, wsID, "bad"+bad+"name") + err := preflightNULForMigration(s, nil, path) + if err == nil || !strings.HasPrefix(err.Error(), "1 stored value") || !strings.Contains(err.Error(), "nothing was migrated") { + t.Fatalf("want a refusal of exactly one value, got %v", err) + } + }) + + t.Run("invalid UTF-8 in a table the migration does not copy is not blocking", func(t *testing.T) { + s, path, _ := newSource(t) + plantPlatformSetting(t, path, "branding", "site"+bad+"name") + scan, err := s.ScanNUL() + if err != nil { + t.Fatalf("scan: %v", err) + } + if scan.Total() != 1 { + t.Fatalf("the scan should report the row; got %v", scan.Violations) + } + if err := preflightNULForMigration(s, nil, path); err != nil { + t.Errorf("the preflight blocked on a table it does not copy: %v", err) + } + }) + + t.Run("the BUG-3220 sliver: invalid byte plus the escape as a literal, in JSON", func(t *testing.T) { + s, path, wsID := newSource(t) + plantWorkspaceSettings(t, path, wsID, `{"a":"`+bad+`","b":"x`+backslash+textguard.EscNUL+`y"}`) + scan, err := s.ScanNUL() + if err != nil { + t.Fatalf("scan: %v", err) + } + if scan.Total() != 1 || !scan.Violations[0].InvalidUTF8 { + t.Fatalf("want one invalid-UTF-8 violation, got %v", scan.Violations) + } + if err := preflightNULForMigration(s, nil, path); err == nil { + t.Fatal("the preflight let the sliver through") + } + }) +} diff --git a/docs/backup.md b/docs/backup.md index d18bf5d0b..8bfd844d9 100644 --- a/docs/backup.md +++ b/docs/backup.md @@ -349,17 +349,21 @@ commands below are for. ### Finding and repairing affected rows -`pad db scan-nul` reports every stored value carrying a NUL — which table and -column, which row, and which workspace — and changes nothing: +`pad db scan-nul` reports every stored value carrying a NUL, and every value +that is not valid UTF-8 — which table and column, which row, and which +workspace — and changes nothing. Invalid UTF-8 is not a NUL, but PostgreSQL +refuses it too (SQLSTATE 22021 under a UTF8 database) and SQLite stores it +silently, so it breaks a migration in the same way. Finding it means reading +every stored value in full; on a 306 MB database that added about a second. ```bash pad db scan-nul # the live database pad db scan-nul --from /backups/pad-20260901.db # or a backup file ``` -`pad db repair-nul` rewrites those values, replacing each NUL with U+FFFD (the -Unicode replacement character) and leaving the rest of the value byte for byte -as it was. **It changes stored content**, which is why it is a separate command +`pad db repair-nul` rewrites those values, replacing each NUL and each invalid +UTF-8 byte sequence with U+FFFD (the Unicode replacement character) and leaving +the rest of the value byte for byte as it was. **It changes stored content**, which is why it is a separate command and never part of a migration — running a schema upgrade should not rewrite your text on your behalf. Run the scan first; it is the dry run. Running the repair twice is safe. diff --git a/internal/store/nulrepair.go b/internal/store/nulrepair.go index ad4c60951..f93f43fc0 100644 --- a/internal/store/nulrepair.go +++ b/internal/store/nulrepair.go @@ -4,6 +4,8 @@ import ( "database/sql" "errors" "fmt" + "strings" + "unicode/utf8" "github.com/PerpetualSoftware/pad/internal/textguard" ) @@ -208,11 +210,24 @@ func (s *Store) repairOneNUL(v NULViolation) (bool, error) { } isJSON := nulColumnIsJSON(v.Table, v.Column) - if !textguard.ParameterRefused(value, isJSON) { + invalidUTF8 := !utf8.ValidString(value) + if !invalidUTF8 && !textguard.ParameterRefused(value, isJSON) { return false, nil } - repaired := textguard.Repair(value, isJSON) + // Invalid UTF-8 first (BUG-3222): each invalid byte sequence becomes + // U+FFFD, the same replacement the NUL repair uses. In a VALID JSON + // document an invalid byte can only sit inside a string, so the result is + // the same document with that string's bad bytes replaced + // (TestRepairInvalidUTF8KeepsJSONValid). A value that was already MALFORMED + // JSON stays malformed: PostgreSQL refuses it for that reason too (22P02), + // which is outside this census and not made worse by the repair (codex r1). + // Then the NUL repair, on the now-valid text. + repaired := value + if invalidUTF8 { + repaired = strings.ToValidUTF8(repaired, textguard.Replacement) + } + repaired = textguard.Repair(repaired, isJSON) if repaired == value { // textguard.Repair is required to change any refused value; a no-op // here would mean the predicate and the repair disagree, and looping diff --git a/internal/store/nulscan.go b/internal/store/nulscan.go index 8dd391e60..6de7f2a33 100644 --- a/internal/store/nulscan.go +++ b/internal/store/nulscan.go @@ -55,6 +55,16 @@ type NULViolation struct { // pass for each. RawNUL bool EscapedNUL bool + // InvalidUTF8 marks a value that is not valid UTF-8 (BUG-3222). It is + // not a NUL defect, but PostgreSQL refuses it too (SQLSTATE 22021 under + // a UTF8 database), and a migration that meets one fails + // partway through the copy exactly as a NUL does. So it rides the same + // census, repair and preflight rather than a second command pair. + InvalidUTF8 bool + // rowid is the row's SQLite rowid, internal to the scan: it identifies the + // row even when KeyIncomplete does not let Key do so, which is what lets + // the NUL and invalid-UTF-8 findings for one value merge (BUG-3222). + rowid int64 // KeyIncomplete marks a row one of whose key columns is NULL, so Key does // not address it. SQLite permits NULL in a declared PRIMARY KEY that is // neither INTEGER PRIMARY KEY nor NOT NULL, which no other engine does. @@ -69,13 +79,17 @@ func (v NULViolation) String() string { for _, k := range sortedKeys(v.Key) { parts = append(parts, k+"="+v.Key[k]) } - kind := "raw NUL" - switch { - case v.RawNUL && v.EscapedNUL: - kind = "raw NUL + escaped NUL" - case v.EscapedNUL: - kind = "escaped NUL" + var kinds []string + if v.RawNUL { + kinds = append(kinds, "raw NUL") } + if v.EscapedNUL { + kinds = append(kinds, "escaped NUL") + } + if v.InvalidUTF8 { + kinds = append(kinds, "invalid UTF-8") + } + kind := strings.Join(kinds, " + ") out := fmt.Sprintf("%s.%s [%s] (%s)", v.Table, v.Column, strings.Join(parts, ", "), kind) if v.WorkspaceID != "" { out += " workspace=" + v.WorkspaceID @@ -206,7 +220,7 @@ func (s *Store) ScanNUL() (*NULScanReport, error) { if s.dialect.Driver() != DriverSQLite { return &NULScanReport{ Applicable: false, - Reason: "PostgreSQL refuses these values natively (SQLSTATE 22021 for a NUL in text, " + + Reason: "PostgreSQL refuses these values natively (SQLSTATE 22021 for a NUL in text or invalid UTF-8, " + "22P05 for the escape reaching jsonb), so no stored row can carry one", }, nil } @@ -246,8 +260,15 @@ func (s *Store) ScanNUL() (*NULScanReport, error) { if err != nil { return nil, err } - report.Violations = append(report.Violations, found...) report.Suspects = append(report.Suspects, suspects...) + + // BUG-3222: invalid UTF-8, folded into the same violation when a + // row carries both defects so the census counts each value once. + invalid, err := s.scanColumnUTF8(c, addr) + if err != nil { + return nil, err + } + report.Violations = append(report.Violations, mergeUTF8Violations(found, invalid)...) } sort.Strings(report.ColumnsAbsent) return report, nil @@ -346,7 +367,8 @@ func (s *Store) scanColumn(c nulColumn, addr tableAddressing) ([]NULViolation, [ qt := quoteIdent(c.Table) qc := quoteIdent(c.Column) - sel := make([]string, 0, len(addr.KeyColumns)+2) + sel := make([]string, 0, len(addr.KeyColumns)+3) + sel = append(sel, "rowid") for _, k := range addr.KeyColumns { sel = append(sel, quoteIdent(k)) } @@ -383,6 +405,8 @@ func (s *Store) scanColumn(c nulColumn, addr tableAddressing) ([]NULViolation, [ // that is not INTEGER PRIMARY KEY or explicitly NOT NULL. Only the // value column is guaranteed non-NULL, by the query's own WHERE. dest := make([]any, 0, len(sel)) + var rowid int64 + dest = append(dest, &rowid) keyVals := make([]sql.NullString, len(addr.KeyColumns)) for i := range keyVals { dest = append(dest, &keyVals[i]) @@ -436,6 +460,7 @@ func (s *Store) scanColumn(c nulColumn, addr tableAddressing) ([]NULViolation, [ Key: map[string]string{}, WorkspaceID: wsID.String, RawNUL: textguard.ContainsNUL(value), + rowid: rowid, } v.EscapedNUL = isJSON && textguard.DocumentDecodesNULAnyShape(value) for i, k := range addr.KeyColumns { diff --git a/internal/store/nulutf8.go b/internal/store/nulutf8.go new file mode 100644 index 000000000..9a7074bc5 --- /dev/null +++ b/internal/store/nulutf8.go @@ -0,0 +1,114 @@ +package store + +import ( + "database/sql" + "fmt" + "strings" + "unicode/utf8" +) + +// scanColumnUTF8 finds every value in one protected column that is not valid +// UTF-8 (BUG-3222). +// +// PostgreSQL refuses such a value in text or jsonb (SQLSTATE 22021 under a +// UTF8 database), and SQLite stores it without complaint, so a legacy row of +// this class breaks `pad db migrate-to-pg` partway through the copy, leaving +// the destination half-populated, which is the failure the NUL preflight +// exists to prevent. No HTTP door stores one today (measured on BUG-3220's +// trail: JSON bodies decode through Go strings, multipart filenames are +// substituted), so this is a guard for rows an earlier release or a direct +// database edit left behind. +// +// NO SQL PRE-FILTER, unlike scanColumn: SQLite has no UTF-8 validity test, so +// every non-NULL value is read as a BLOB and checked in Go. That is one full +// read of each protected column, the same order of work as the copy it +// guards, which reads every row anyway. +// +// CAST AS BLOB, not a plain read: the bytes are what is judged, and a BLOB is +// returned exactly as stored. +func (s *Store) scanColumnUTF8(c nulColumn, addr tableAddressing) ([]NULViolation, error) { + sel := make([]string, 0, len(addr.KeyColumns)+3) + sel = append(sel, "rowid") + for _, k := range addr.KeyColumns { + sel = append(sel, quoteIdent(k)) + } + if addr.HasWorkspace { + sel = append(sel, `"workspace_id"`) + } + qc := quoteIdent(c.Column) + sel = append(sel, "CAST("+qc+" AS BLOB)") + + q := fmt.Sprintf(`SELECT %s FROM %s WHERE %s IS NOT NULL`, strings.Join(sel, ", "), quoteIdent(c.Table), qc) + rows, err := s.db.Query(q) + if err != nil { + return nil, fmt.Errorf("scan %s.%s for invalid UTF-8: %w", c.Table, c.Column, err) + } + defer rows.Close() + + var out []NULViolation + for rows.Next() { + // Nullable key and workspace columns, for the reason scanColumn + // gives: SQLite permits NULL in some PRIMARY KEY columns. + dest := make([]any, 0, len(sel)) + var rowid int64 + dest = append(dest, &rowid) + keyVals := make([]sql.NullString, len(addr.KeyColumns)) + for i := range keyVals { + dest = append(dest, &keyVals[i]) + } + var wsID sql.NullString + if addr.HasWorkspace { + dest = append(dest, &wsID) + } + var value []byte + dest = append(dest, &value) + if err := rows.Scan(dest...); err != nil { + return nil, fmt.Errorf("scan %s.%s row for invalid UTF-8: %w", c.Table, c.Column, err) + } + if utf8.Valid(value) { + continue + } + v := NULViolation{ + Table: c.Table, + Column: c.Column, + Key: map[string]string{}, + WorkspaceID: wsID.String, + InvalidUTF8: true, + rowid: rowid, + } + for i, k := range addr.KeyColumns { + if !keyVals[i].Valid { + v.KeyIncomplete = true + continue + } + v.Key[k] = keyVals[i].String + } + out = append(out, v) + } + return out, rows.Err() +} + +// mergeUTF8Violations folds one column's invalid-UTF-8 findings into its NUL +// findings, so a value carrying both defects is ONE violation with both kinds +// rather than two rows the census would count twice. Matched on the rowid +// both scans read, not on Key, which cannot identify a KeyIncomplete row +// (codex r1). Order: the NUL findings in their order, then the UTF-8-only +// ones in theirs. +func mergeUTF8Violations(nul, invalid []NULViolation) []NULViolation { + if len(invalid) == 0 { + return nul + } + index := make(map[int64]int, len(nul)) + for i, v := range nul { + index[v.rowid] = i + } + out := append([]NULViolation{}, nul...) + for _, v := range invalid { + if i, ok := index[v.rowid]; ok { + out[i].InvalidUTF8 = true + continue + } + out = append(out, v) + } + return out +} diff --git a/internal/store/nulutf8_test.go b/internal/store/nulutf8_test.go new file mode 100644 index 000000000..4dffa100f --- /dev/null +++ b/internal/store/nulutf8_test.go @@ -0,0 +1,220 @@ +package store + +import ( + "database/sql" + "encoding/json" + "strings" + "testing" + "unicode/utf8" + + "github.com/PerpetualSoftware/pad/internal/textguard" +) + +// BUG-3222: invalid UTF-8 rides the NUL census. PostgreSQL refuses it +// (SQLSTATE 22021), SQLite stores it, and a migration meeting one fails +// partway through the copy. + +// invalidByte is one byte that can never start or continue valid UTF-8 on its +// own. Built from its value so no escape text passes through an editor. +var invalidByte = string([]byte{0x80}) + +func TestScanAndRepairInvalidUTF8(t *testing.T) { + t.Parallel() + s := testStore(t) + if s.dialect.Driver() != DriverSQLite { + t.Skip("SQLite only") + } + ws := createTestWorkspace(t, s, "UTF8WS") + col := createTestCollection(t, s, ws.ID, "Tasks") + textRow := createTestItem(t, s, ws.ID, col.ID, "text", "body") + jsonRow := createTestItem(t, s, ws.ID, col.ID, "json", "body") + bothRow := createTestItem(t, s, ws.ID, col.ID, "both", "body") + control := createTestItem(t, s, ws.ID, col.ID, "control", "body") + + multibyte := "é" + string(rune(0x4E2D)) + string(rune(0x1F600)) + plantLegacyRows(t, s, func(raw *sql.DB) { + mustExec(t, raw, `UPDATE items SET title = ? WHERE id = ?`, "ok"+invalidByte+"x", textRow.ID) + mustExec(t, raw, `UPDATE items SET fields = ? WHERE id = ?`, `{"note":"x`+invalidByte+`y"}`, jsonRow.ID) + mustExec(t, raw, `UPDATE items SET title = ? WHERE id = ?`, "a"+textguard.NUL+"b"+invalidByte, bothRow.ID) + mustExec(t, raw, `UPDATE items SET title = ?, fields = ? WHERE id = ?`, + multibyte, `{"note":"`+multibyte+`"}`, control.ID) + }) + + report, err := s.ScanNUL() + if err != nil { + t.Fatalf("scan: %v", err) + } + got := map[string]NULViolation{} + for _, v := range report.Violations { + got[v.Key["id"]+"/"+v.Column] = v + } + for _, c := range []struct { + key string + wantInvalid, rawNUL bool + }{ + {textRow.ID + "/title", true, false}, + {jsonRow.ID + "/fields", true, false}, + {bothRow.ID + "/title", true, true}, + } { + v, ok := got[c.key] + if !ok { + t.Errorf("%s: not reported", c.key) + continue + } + if v.InvalidUTF8 != c.wantInvalid || v.RawNUL != c.rawNUL { + t.Errorf("%s: InvalidUTF8=%v RawNUL=%v, want %v/%v", c.key, v.InvalidUTF8, v.RawNUL, c.wantInvalid, c.rawNUL) + } + if v.WorkspaceID != ws.ID { + t.Errorf("%s: workspace %q, want %q", c.key, v.WorkspaceID, ws.ID) + } + } + for k := range got { + if strings.HasPrefix(k, control.ID) { + t.Errorf("valid multibyte text reported as a violation: %s", k) + } + } + if report.Total() != 3 { + t.Errorf("violations = %d, want 3 (a row with both defects counts once): %v", report.Total(), report.Violations) + } + if !strings.Contains(got[bothRow.ID+"/title"].String(), "raw NUL + invalid UTF-8") { + t.Errorf("both kinds not named: %s", got[bothRow.ID+"/title"]) + } + + rep, err := s.RepairNUL() + if err != nil { + t.Fatalf("repair: %v", err) + } + if len(rep.Failed) != 0 || len(rep.Repaired) != 3 { + t.Fatalf("repaired %d, failed %v; want 3 and none", len(rep.Repaired), rep.Failed) + } + read := func(col, id string) string { + var v string + if err := s.db.QueryRow(`SELECT `+col+` FROM items WHERE id = ?`, id).Scan(&v); err != nil { + t.Fatal(err) + } + return v + } + if v := read("title", textRow.ID); v != "ok"+textguard.Replacement+"x" { + t.Errorf("text row repaired to %q", v) + } + if v := read("title", bothRow.ID); v != "a"+textguard.Replacement+"b"+textguard.Replacement { + t.Errorf("both row repaired to %q", v) + } + if v := read("title", control.ID); v != multibyte { + t.Errorf("the control was changed: %q", v) + } + after, err := s.ScanNUL() + if err != nil { + t.Fatalf("rescan: %v", err) + } + if after.Total() != 0 { + t.Errorf("violations after repair: %v", after.Violations) + } +} + +// In a VALID JSON document an invalid byte can only sit inside a string, so +// replacing it leaves a valid document with the same shape. +func TestRepairInvalidUTF8KeepsJSONValid(t *testing.T) { + t.Parallel() + s := testStore(t) + if s.dialect.Driver() != DriverSQLite { + t.Skip("SQLite only") + } + ws := createTestWorkspace(t, s, "UTF8JSON") + col := createTestCollection(t, s, ws.ID, "Tasks") + it := createTestItem(t, s, ws.ID, col.ID, "json", "body") + doc := `{"note":"x` + invalidByte + `y","n":1e3,"arr":["a` + invalidByte + `"]}` + plantLegacyRows(t, s, func(raw *sql.DB) { + mustExec(t, raw, `UPDATE items SET fields = ? WHERE id = ?`, doc, it.ID) + }) + if _, err := s.RepairNUL(); err != nil { + t.Fatalf("repair: %v", err) + } + var got string + if err := s.db.QueryRow(`SELECT fields FROM items WHERE id = ?`, it.ID).Scan(&got); err != nil { + t.Fatal(err) + } + want := `{"note":"x` + textguard.Replacement + `y","n":1e3,"arr":["a` + textguard.Replacement + `"]}` + if got != want || !json.Valid([]byte(got)) || !utf8.ValidString(got) { + t.Errorf("repaired to %q (valid JSON %v), want %q", got, json.Valid([]byte(got)), want) + } +} + +// A value whose row has an INCOMPLETE key (a NULL in its primary key, which +// SQLite permits) and carries both defects is still ONE violation (codex r1). +// Two such rows stay two: the merge is by rowid, which both scans read, not by +// the partial key they share. +func TestInvalidUTF8MergesOnKeyIncompleteRows(t *testing.T) { + t.Parallel() + s := testStore(t) + if s.dialect.Driver() != DriverSQLite { + t.Skip("SQLite only") + } + value := `{"note":"a` + textguard.NUL + `b` + invalidByte + `"}` + plantLegacyRows(t, s, func(raw *sql.DB) { + for i := 0; i < 2; i++ { + mustExec(t, raw, `INSERT INTO activities (id, action, actor, source, metadata, created_at) + VALUES (NULL, 'created', 'agent', 'cli', ?, datetime('now'))`, value) + } + }) + report, err := s.ScanNUL() + if err != nil { + t.Fatalf("scan: %v", err) + } + var got []NULViolation + for _, v := range report.Violations { + if v.Table == "activities" && v.Column == "metadata" { + got = append(got, v) + } + } + if len(got) != 2 { + t.Fatalf("want 2 violations (two rows, each counted once), got %d: %v", len(got), got) + } + for _, v := range got { + if !v.KeyIncomplete || !v.RawNUL || !v.InvalidUTF8 { + t.Errorf("want one incomplete-key violation carrying both kinds, got %+v", v) + } + } +} + +// The rowid merge (and the rowid address item_wiki_links already uses) reads +// SQLite's hidden rowid. A table that DECLARED a column named rowid, _rowid_ +// or oid would shadow it, and the scan would read that column instead (codex +// r2). None does; this makes adding one fail here rather than miscount. +func TestNoProtectedTableShadowsRowid(t *testing.T) { + t.Parallel() + s := testStore(t) + if s.dialect.Driver() != DriverSQLite { + t.Skip("SQLite only") + } + seen := map[string]bool{} + for _, c := range NULProtectedColumns() { + if seen[c.Table] { + continue + } + seen[c.Table] = true + rows, err := s.db.Query(`SELECT name FROM pragma_table_info(?)`, c.Table) + if err != nil { + t.Fatalf("table_info %s: %v", c.Table, err) + } + n := 0 + for rows.Next() { + var name string + if err := rows.Scan(&name); err != nil { + t.Fatal(err) + } + n++ + switch strings.ToLower(name) { + case "rowid", "_rowid_", "oid": + t.Errorf("%s declares a column %q, which shadows SQLite's rowid the NUL scan merges on", c.Table, name) + } + } + rows.Close() + if n == 0 { + t.Errorf("%s: no columns read; the check would pass vacuously", c.Table) + } + } + if len(seen) == 0 { + t.Fatal("no protected tables") + } +}