diff --git a/cmd/pad/cmd_db.go b/cmd/pad/cmd_db.go index c1dcae7e..80f6e58c 100644 --- a/cmd/pad/cmd_db.go +++ b/cmd/pad/cmd_db.go @@ -408,6 +408,23 @@ Steps: } defer dstStore.Close() + // PREFLIGHT: refuse BEFORE moving anything (DOC-2823 S3). + // + // The reason this is a preflight and not an error mid-copy is the + // shape of the failure it replaces. A legacy row carrying a NUL + // reaches PostgreSQL's jsonb parser during ImportWorkspace and + // fails there — at a point where earlier workspaces have already + // been written, so the operator is left with a half-moved + // database and an error naming a driver, not a cause. The scan is + // read-only and cheap next to the copy it guards. + // + // It does NOT repair. Dave's day-54 ruling: a migration that + // rewrites user content decides consent for the operator, so this + // prints the exact command that asks for it. + if err := preflightNULForMigration(srcStore, fromPath); err != nil { + return err + } + // List workspaces from source workspaces, err := srcStore.ListWorkspaces() if err != nil { @@ -482,3 +499,34 @@ func maskPassword(pgURL string) string { } // --- audit-log --- + +// preflightNULForMigration refuses a SQLite-to-PostgreSQL migration whose +// source carries values PostgreSQL will not accept, listing them and naming the +// repair command. +// +// Nothing has been written to the destination when this runs — it sits above +// the workspace loop, which is the whole point: the failure it replaces +// happened partway through the copy. +func preflightNULForMigration(src *store.Store, fromPath string) error { + report, err := src.ScanNUL() + if err != nil { + return fmt.Errorf("NUL preflight: %w", err) + } + if !report.Applicable || report.Total() == 0 { + return nil + } + + fmt.Fprintf(os.Stderr, "\nPreflight found %d stored value(s) in %s that PostgreSQL will not accept:\n\n", + report.Total(), fromPath) + for _, v := range report.Violations { + fmt.Fprintf(os.Stderr, " %s\n", v) + } + fmt.Fprintf(os.Stderr, "\nEach carries a NUL, which PostgreSQL refuses natively — SQLSTATE 22021 in a text\n"+ + "column, 22P05 for an escape reaching jsonb. Migrating would fail partway through\n"+ + "the copy, after some workspaces had already moved.\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", + repairNULCommandHint) + + return fmt.Errorf("%d stored value(s) carry a NUL; nothing was migrated", report.Total()) +} diff --git a/cmd/pad/cmd_db_nul.go b/cmd/pad/cmd_db_nul.go new file mode 100644 index 00000000..9ea47f94 --- /dev/null +++ b/cmd/pad/cmd_db_nul.go @@ -0,0 +1,267 @@ +package main + +import ( + "fmt" + "os" + "sort" + + "github.com/spf13/cobra" + + "github.com/PerpetualSoftware/pad/internal/cli" + "github.com/PerpetualSoftware/pad/internal/config" + "github.com/PerpetualSoftware/pad/internal/store" +) + +// The operator-facing half of DOC-2823 S3 (BUG-2810): find the legacy rows the +// two enforcement layers cannot retroactively fix, and repair them on an +// explicit instruction. +// +// TWO SIBLING COMMANDS RATHER THAN `repair --nul`, which is how DOC-2823 first +// spelled it. A `repair` verb that errors when given no flag is a worse shape +// than two honest siblings, and there is no second kind of repair for it to +// share a namespace with today. `scan-nul` also matches `migrate-to-pg`, the +// existing hyphenated compound in this group. +// +// `scan-nul` IS the dry run, so `repair-nul` grows no --dry-run of its own. + +// repairNULCommandHint is the exact command the migrate-to-pg preflight prints +// and this command's own help names. It is the STORE's constant rather than a +// second spelling of the same words: the server quotes that same string in the +// import's strict refusal, and a remedy naming a command that has been renamed +// is worse than no remedy at all. +// +// TestRepairNULHintNamesARealCommand pins it against the cobra command tree, so +// a rename that misses one of the three call sites fails a test rather than an +// operator. +const repairNULCommandHint = store.RepairNULCommand + +func dbScanNULCmd() *cobra.Command { + var fromPath string + + cmd := &cobra.Command{ + Use: "scan-nul", + Short: "Report stored values carrying a NUL (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. + +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. + +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 +backup file instead of the live database. + +Nothing is repaired. Run '` + repairNULCommandHint + `' for that.`, + RunE: func(cmd *cobra.Command, args []string) error { + s, err := openSQLiteForNULTools(&fromPath) + if err != nil { + return err + } + if s == nil { + return nil // Postgres: reported and nothing to do. + } + defer s.Close() + + report, err := s.ScanNUL() + if err != nil { + return fmt.Errorf("scan: %w", err) + } + printNULScanReport(report, fromPath) + return nil + }, + } + + cmd.Flags().StringVar(&fromPath, "from", "", "SQLite database path (default: server-resolved — PAD_DB_PATH > PAD_DATA_DIR/pad.db > ~/.pad/pad.db)") + return cmd +} + +func dbRepairNULCmd() *cobra.Command { + var fromPath string + var force bool + + 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. + +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 +on the operator's behalf. Every value it changes is listed. + +Run 'pad db scan-nul' first to see what would change — that is the dry run, so +this command has no --dry-run of its own. Running it twice is safe: the second +pass finds nothing to do. + +A row whose PRIMARY KEY carries the NUL is reported and left alone, because +repairing it would change the row's identity and could collide with another +row.`, + RunE: func(cmd *cobra.Command, args []string) error { + // Refuse while the server is up, on the same reasoning as + // 'pad db restore': the report is a claim about a database, and a + // database somebody else is concurrently writing makes it a claim + // about a moment that has passed. + if fromPath == "" { + cfg, err := config.Load() + if err != nil { + return fmt.Errorf("load config: %w", err) + } + if cli.IsServerRunning(cfg) && !force { + return fmt.Errorf("the Pad server appears to be running at %s:%d — stop it first ('pad server stop') so the rows cannot change under the repair, or re-run with --force to override", cfg.Host, cfg.Port) + } + } + + s, err := openSQLiteForNULTools(&fromPath) + if err != nil { + return err + } + if s == nil { + return nil + } + defer s.Close() + + // Show the operator what is about to change BEFORE asking. A + // confirmation prompt for an unnamed set of rows is not consent. + scan, err := s.ScanNUL() + if err != nil { + return fmt.Errorf("scan: %w", err) + } + printNULScanReport(scan, fromPath) + if scan.Total() == 0 { + return nil + } + + if !force { + fmt.Fprintf(os.Stderr, "\nThis will rewrite %d value(s) above, replacing each NUL with U+FFFD.\n", scan.Total()) + 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] ") + var confirm string + fmt.Scanln(&confirm) + if confirm != "y" && confirm != "Y" { + fmt.Fprintln(os.Stderr, "Aborted. Nothing was changed.") + return nil + } + } + + report, err := s.RepairNUL() + if err != nil { + return fmt.Errorf("repair: %w", err) + } + + fmt.Fprintf(os.Stderr, "\nRepaired %d value(s).\n", len(report.Repaired)) + for _, v := range report.Repaired { + fmt.Fprintf(os.Stderr, " %s\n", v) + } + if len(report.Skipped) > 0 { + fmt.Fprintf(os.Stderr, "\nSkipped %d value(s):\n", len(report.Skipped)) + for _, sk := range report.Skipped { + fmt.Fprintf(os.Stderr, " %s\n %s\n", sk.Violation, sk.Reason) + } + } + if len(report.Failed) > 0 { + fmt.Fprintf(os.Stderr, "\nFailed on %d value(s):\n", len(report.Failed)) + for _, f := range report.Failed { + fmt.Fprintf(os.Stderr, " %s\n %v\n", f.Violation, f.Err) + } + return fmt.Errorf("%d value(s) could not be repaired", len(report.Failed)) + } + return nil + }, + } + + cmd.Flags().StringVar(&fromPath, "from", "", "SQLite database path (default: server-resolved)") + cmd.Flags().BoolVar(&force, "force", false, "skip the confirmation prompt and the running-server check") + return cmd +} + +// openSQLiteForNULTools resolves the database both commands work on, and +// returns (nil, nil) when the deployment is PostgreSQL — where the state these +// commands exist for cannot be stored at all. +// +// It writes the resolved path back through fromPath so the caller can report +// which database it looked at. A report that does not name its subject is the +// kind an operator can act on against the wrong instance. +func openSQLiteForNULTools(fromPath *string) (*store.Store, error) { + if *fromPath == "" { + if os.Getenv("PAD_DB_DRIVER") == "postgres" || os.Getenv("PAD_DATABASE_URL") != "" { + 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"+ + "Nothing to scan or repair.") + return nil, nil + } + resolved, err := resolveSQLiteDBPath() + if err != nil { + return nil, err + } + *fromPath = resolved + } + if _, err := os.Stat(*fromPath); os.IsNotExist(err) { + return nil, fmt.Errorf("SQLite database not found: %s", *fromPath) + } + s, err := store.New(*fromPath) + if err != nil { + return nil, fmt.Errorf("open SQLite: %w", err) + } + return s, nil +} + +// printNULScanReport renders a scan for a human, on stderr so a caller piping +// stdout is unaffected. +func printNULScanReport(report *store.NULScanReport, dbPath string) { + if !report.Applicable { + fmt.Fprintf(os.Stderr, "Not applicable: %s.\n", report.Reason) + return + } + + fmt.Fprintf(os.Stderr, "Scanned %d protected column(s) in %s.\n", report.ColumnsScanned, dbPath) + if len(report.ColumnsAbsent) > 0 { + // Not an error — an older schema legitimately lacks later columns — + // but a census that quietly skipped part of its population is not one. + fmt.Fprintf(os.Stderr, " (%d listed column(s) absent from this schema: %v)\n", + len(report.ColumnsAbsent), report.ColumnsAbsent) + } + + if report.Total() == 0 { + fmt.Fprintln(os.Stderr, "No values carrying a NUL were found.") + return + } + + fmt.Fprintf(os.Stderr, "\nFound %d value(s) carrying a NUL:\n\n", report.Total()) + + byColumn := report.ByColumn() + for _, key := range sortedCountKeys(byColumn) { + fmt.Fprintf(os.Stderr, " %-44s %d\n", key, byColumn[key]) + } + + byWorkspace := report.ByWorkspace() + fmt.Fprintln(os.Stderr, "\nBy workspace:") + for _, id := range sortedCountKeys(byWorkspace) { + label := id + if label == "" { + // Sixteen of the protected tables carry no workspace_id — users, + // sessions, the oauth tables, platform settings. Saying so beats + // an empty column. + label = "(instance-wide tables, no workspace)" + } + fmt.Fprintf(os.Stderr, " %-44s %d\n", label, byWorkspace[id]) + } + + fmt.Fprintln(os.Stderr, "\nRows:") + for _, v := range report.Violations { + fmt.Fprintf(os.Stderr, " %s\n", v) + } +} + +func sortedCountKeys(m map[string]int) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} diff --git a/cmd/pad/cmd_db_nul_test.go b/cmd/pad/cmd_db_nul_test.go new file mode 100644 index 00000000..8041c273 --- /dev/null +++ b/cmd/pad/cmd_db_nul_test.go @@ -0,0 +1,171 @@ +package main + +import ( + "database/sql" + "path/filepath" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/store" + "github.com/PerpetualSoftware/pad/internal/textguard" +) + +// TestRepairNULHintNamesARealCommand is the guard the hint's own comment +// promises. +// +// Three surfaces quote this command at an operator — the migrate-to-pg +// preflight, the workspace import's strict refusal, and scan-nul's help — and +// each of them is a claim that typing it does something. Walking the real cobra +// tree is what makes a rename fail here rather than in front of a user, which +// is the difference between a cited convention and a consulted one. +func TestRepairNULHintNamesARealCommand(t *testing.T) { + root := newRootCmd() + + // The hint is a full command line ("pad db repair-nul"); resolve it as a + // path through the tree rather than by string comparison against a second + // spelling, which would only prove two constants agree. + fields := strings.Fields(repairNULCommandHint) + if len(fields) < 2 || fields[0] != "pad" { + t.Fatalf("the hint is not a 'pad ...' command line: %q", repairNULCommandHint) + } + cmd, _, err := root.Find(fields[1:]) + if err != nil { + t.Fatalf("the hint names a command that does not exist: %q (%v)", repairNULCommandHint, err) + } + // Find falls back to the closest ancestor rather than failing, so the + // resolved command must actually BE the leaf named — otherwise "pad db + // repair-nonsense" resolves to "db" and passes. + if cmd.Name() != fields[len(fields)-1] { + t.Fatalf("the hint %q resolves to %q, not to a command of its own name", + repairNULCommandHint, cmd.CommandPath()) + } + if cmd.RunE == nil && cmd.Run == nil { + t.Errorf("%q exists but does nothing when run", repairNULCommandHint) + } + + // And the store's constant — which the SERVER quotes in the import + // refusal — is the same string, so all three surfaces move together. + if repairNULCommandHint != store.RepairNULCommand { + t.Errorf("the CLI hint (%q) and the string the server quotes (%q) have drifted apart", + repairNULCommandHint, store.RepairNULCommand) + } +} + +// TestMigrateToPgPreflightRefusesAndNamesTheRepair covers the preflight's whole +// contract: it refuses, it names the rows, it names the command, and — the part +// that matters most — it does so BEFORE anything has moved. +func TestMigrateToPgPreflightRefusesAndNamesTheRepair(t *testing.T) { + dbPath := filepath.Join(t.TempDir(), "preflight.db") + s, err := store.New(dbPath) + if err != nil { + t.Fatalf("open store: %v", err) + } + defer s.Close() + + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "Preflight"}) + if err != nil { + t.Fatalf("create workspace: %v", err) + } + + // CONTROL FIRST: a clean database passes the preflight. Without this, a + // preflight that refused everything would satisfy the assertion below. + if err := preflightNULForMigration(s, dbPath); err != nil { + t.Fatalf("preflight refused a clean database: %v", err) + } + + plantNULInWorkspaceName(t, dbPath, ws.ID, "bad"+textguard.NUL+"name") + + err = preflightNULForMigration(s, dbPath) + if err == nil { + t.Fatal("the preflight accepted a database carrying a value PostgreSQL will refuse — the migration " + + "would fail partway through the copy, which is the failure this replaces") + } + if !strings.Contains(err.Error(), "nothing was migrated") { + t.Errorf("the refusal does not say the migration did not start: %v", err) + } +} + +// TestScanAndRepairAgreeThroughTheCommandPath drives the store API the two +// commands call, so the CLI's promise — scan-nul is the dry run for +// repair-nul — is measured rather than asserted in help text. +func TestScanAndRepairAgreeThroughTheCommandPath(t *testing.T) { + dbPath := filepath.Join(t.TempDir(), "scanrepair.db") + s, err := store.New(dbPath) + if err != nil { + t.Fatalf("open store: %v", err) + } + defer s.Close() + + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "ScanRepair"}) + if err != nil { + t.Fatalf("create workspace: %v", err) + } + plantNULInWorkspaceName(t, dbPath, ws.ID, "bad"+textguard.NUL+"name") + + scan, err := s.ScanNUL() + if err != nil { + t.Fatalf("scan: %v", err) + } + if scan.Total() != 1 { + t.Fatalf("scan found %d violations, want 1: %v", scan.Total(), scan.Violations) + } + + report, err := s.RepairNUL() + if err != nil { + t.Fatalf("repair: %v", err) + } + // The dry run's count and the repair's count are the same number, which is + // the entire reason scan-nul is offered instead of a --dry-run flag. + if len(report.Repaired) != scan.Total() { + t.Errorf("scan promised %d change(s), repair made %d", scan.Total(), len(report.Repaired)) + } + + var name string + if err := s.DB().QueryRow(`SELECT name FROM workspaces WHERE id = ?`, ws.ID).Scan(&name); err != nil { + t.Fatalf("read back: %v", err) + } + if want := "bad" + textguard.Replacement + "name"; name != want { + t.Errorf("repaired name = %q, want %q", name, want) + } +} + +// plantNULInWorkspaceName writes the legacy state through a raw handle with the +// NUL triggers dropped — which is what a pre-enforcement binary was. +func plantNULInWorkspaceName(t *testing.T, dbPath, wsID, value string) { + t.Helper() + + raw, err := sql.Open("sqlite", dbPath+"?_pragma=busy_timeout(30000)") + if err != nil { + t.Fatalf("open raw: %v", err) + } + defer raw.Close() + + rows, err := raw.Query( + `SELECT name FROM sqlite_master WHERE type = 'trigger' AND name GLOB 'pad_nul_workspaces_name_*'`) + if err != nil { + t.Fatalf("list triggers: %v", err) + } + var names []string + for rows.Next() { + var n string + if err := rows.Scan(&n); err != nil { + rows.Close() + t.Fatalf("scan: %v", err) + } + names = append(names, n) + } + rows.Close() + if len(names) == 0 { + t.Fatal("no workspaces.name triggers found — the fixture would plant nothing and the test " + + "would pass for the wrong reason") + } + for _, n := range names { + if _, err := raw.Exec(`DROP TRIGGER IF EXISTS "` + n + `"`); err != nil { + t.Fatalf("drop %s: %v", n, err) + } + } + if _, err := raw.Exec(`UPDATE workspaces SET name = ? WHERE id = ?`, value, wsID); err != nil { + t.Fatalf("plant: %v", err) + } +} diff --git a/cmd/pad/cmd_workspace.go b/cmd/pad/cmd_workspace.go index 4b05b820..6d974c2d 100644 --- a/cmd/pad/cmd_workspace.go +++ b/cmd/pad/cmd_workspace.go @@ -4,6 +4,8 @@ import ( "encoding/json" "fmt" "io" + "net/http" + "net/url" "os" "path/filepath" "strings" @@ -17,6 +19,7 @@ import ( "github.com/PerpetualSoftware/pad/internal/cli" "github.com/PerpetualSoftware/pad/internal/collections" "github.com/PerpetualSoftware/pad/internal/config" + "github.com/PerpetualSoftware/pad/internal/server" "github.com/PerpetualSoftware/pad/internal/models" "golang.org/x/term" @@ -1032,6 +1035,7 @@ Both formats can be re-imported via 'pad workspace import'.`, func importCmd() *cobra.Command { var nameFlag string + var repairNUL bool cmd := &cobra.Command{ Use: "import ", Short: "Import workspace from JSON export or tar.gz bundle", @@ -1048,9 +1052,16 @@ Format is detected by file extension. Override workspace name with --name.`, client, _ := getClient() filePath := args[0] - path := "/workspaces/import" + q := url.Values{} if nameFlag != "" { - path += "?name=" + nameFlag + q.Set("name", nameFlag) + } + if repairNUL { + q.Set(server.NULRepairQueryParam, "true") + } + path := "/workspaces/import" + if len(q) > 0 { + path += "?" + q.Encode() } // Detect bundle by extension. .tar.gz / .tgz route through @@ -1074,7 +1085,8 @@ Format is detected by file extension. Override workspace name with --name.`, defer f.Close() var ws models.Workspace - if err := client.PostStreamWithContentType(path, f, contentType, &ws); err != nil { + header, err := client.PostStreamWithContentTypeHeaders(path, f, contentType, &ws) + if err != nil { return fmt.Errorf("import: %w", err) } @@ -1085,10 +1097,20 @@ Format is detected by file extension. Override workspace name with --name.`, fmt.Printf(" Attachments: rehydrated from bundle\n") } fmt.Printf(" All IDs regenerated\n") + if repairNUL { + // Report what the consent flag actually did. "Repaired" with no + // number is the kind of reassurance an operator cannot check, + // and zero is a genuinely useful answer: it means the export was + // clean and the flag was not needed. + fmt.Printf(" NUL escapes repaired (replaced with U+FFFD): %s\n", + repairedNULCount(header)) + } return nil }, } cmd.Flags().StringVar(&nameFlag, "name", "", "override workspace name") + cmd.Flags().BoolVar(&repairNUL, "repair-nul", false, + "replace NUL escapes in the export with U+FFFD instead of refusing it (rewrites content; default is strict)") return cmd } @@ -1185,3 +1207,21 @@ func auditLogCmd() *cobra.Command { return cmd } + +// repairedNULCount reads the count the server reports for a --repair-nul +// import. +// +// An ABSENT header is reported as unknown rather than as zero. A server older +// than this flag ignores the query parameter entirely and imports strictly, so +// printing "0" there would tell the operator the export was clean when in fact +// nothing was even asked. +func repairedNULCount(header http.Header) string { + if header == nil { + return "unknown (no response headers)" + } + v := header.Get(server.NULRepairHeader) + if v == "" { + return "unknown (this server does not report the count; it may predate --repair-nul)" + } + return v +} diff --git a/cmd/pad/groups.go b/cmd/pad/groups.go index 9ae153e1..9dd9907d 100644 --- a/cmd/pad/groups.go +++ b/cmd/pad/groups.go @@ -192,13 +192,15 @@ func agentCmd() *cobra.Command { func dbCmd() *cobra.Command { cmd := &cobra.Command{ Use: "db", - Short: "Database backup, restore, and migration tools", + Short: "Database backup, restore, repair, and migration tools", RunE: unknownSubcommandRun, } cmd.AddCommand( dbBackupCmd(), dbRestoreCmd(), dbMigrateToPgCmd(), + dbScanNULCmd(), + dbRepairNULCmd(), ) return cmd } diff --git a/docs/backup.md b/docs/backup.md index bf5549d3..a150479b 100644 --- a/docs/backup.md +++ b/docs/backup.md @@ -153,11 +153,13 @@ For portable workspace backups that work across SQLite and PostgreSQL: # Export a workspace to JSON pad workspace export > my-workspace.json -# Import into any Pad instance (SQLite or PostgreSQL) -pad workspace import < my-workspace.json +# Import into any Pad instance (SQLite or PostgreSQL). +# The file is an ARGUMENT, not stdin — `pad workspace import < file` fails +# with "accepts 1 arg(s), received 0". +pad workspace import my-workspace.json # Import with a new name -pad workspace import --name "imported-workspace" < my-workspace.json +pad workspace import --name "imported-workspace" my-workspace.json ``` ### One case where an export is not importable @@ -170,35 +172,72 @@ application rule, not a universal storage fact — PostgreSQL does refuse a NUL outright, but SQLite accepts one in a TEXT column, which is why the rule has to be enforced rather than assumed, and why the paragraphs below matter. -**The rule lives in the binary, not in the database**, so "before the rule -existed" is a statement about which build served the write, not about a date. -On SQLite, any window in which an older binary serves the same database can -still create such rows: a rollback to the previous version, a staged rollout -where an old and a new instance share a database, or a second older instance -pointed at the same file. Once that window closes the guard is back, but the -rows are already stored, and they behave exactly like genuinely old ones. +**The rule is now enforced by the database as well as by the binary.** It used +to live only in the running build, which meant any window where an older binary +served the same SQLite database could still create such rows — a rollback, a +staged rollout, a second old instance pointed at the same file. A schema +migration now installs triggers that refuse the write in the database itself, +so an older binary writing to an upgraded file is refused too (BUG-2813). The +window that remains is a SQLite database an upgraded binary has never opened: +until its migrations run, it has no triggers. -Only SQLite is affected. PostgreSQL refuses a NUL in a text or JSON column +Only SQLite ever needed this. PostgreSQL refuses a NUL in a text or JSON column itself, at every binary version, so a PostgreSQL instance never stored such a value regardless of which build wrote it. -If you want the guarantee rather than the guard, drain writes from older -binaries before the new one starts serving, or roll forward rather than back. -Enforcing the invariant below the HTTP layer, so the running build stops -mattering, is tracked as BUG-2813. +None of that helps a row that was **already** stored, which is what the two +commands below are for. -The same limitation applies to `pad db migrate-to-pg`, which copies rows -directly and does not go through the import guard: a row carrying a NUL will -fail against PostgreSQL's JSONB parser during the copy rather than being -reported up front. +### Finding and repairing affected rows -If you hit either, the affected value has to be repaired at the source before -the import or migration will go through — the export itself succeeds either -way, as described above. A preflight check and a repair path are tracked as -BUG-2810. Until then, neither error names the exact row: the import answers -400 naming the rule it refused on, and `pad db migrate-to-pg` reports which -workspace's copy failed — locating the offending value inside it is manual -today. +`pad db scan-nul` reports every stored value carrying a NUL — which table and +column, which row, and which workspace — and changes nothing: + +```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 +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. + +```bash +pad server stop +pad db repair-nul # lists what it will change, then asks +pad server start +``` + +A row whose **primary key** is the value carrying the NUL is reported and left +alone: repairing it would change the row's identity and could collide with +another row. `email_optouts` is the only table where that can happen today. + +### Migrating to PostgreSQL + +`pad db migrate-to-pg` now runs the same scan as a **preflight**. If the source +database carries any affected rows it lists them, prints the repair command, +and exits without moving anything — rather than failing partway through the +copy against PostgreSQL's JSONB parser, which is what it used to do. + +### Importing an export that predates the rule + +If you have an export file taken from an affected database, the import still +refuses it by default and the 400 names the remedy. Passing `--repair-nul` +applies the same U+FFFD substitution to the payload as it is imported: + +```bash +pad workspace import --repair-nul my-workspace.json +``` + +The default stays strict, and the flag is your consent to the rewrite; the +command reports how many values it changed. It repairs escaped NULs, which is +what an export from an affected database contains. Repairing the source +database with `pad db repair-nul` and re-exporting gives the same result +without a rewrite at import time, and is the better option when you still have +the source instance. This format is database-agnostic and can be used to: diff --git a/internal/cli/client.go b/internal/cli/client.go index a8cf8474..b4fb8f61 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -1180,17 +1180,34 @@ func (c *Client) PostRawWithContentType(path string, data []byte, contentType st // import path — together they keep import memory bounded by the // largest single blob (~25 MiB) rather than the full bundle size. func (c *Client) PostStreamWithContentType(path string, body io.Reader, contentType string, result interface{}) error { + _, err := c.PostStreamWithContentTypeHeaders(path, body, contentType, result) + return err +} + +// PostStreamWithContentTypeHeaders is PostStreamWithContentType, also returning +// the response headers. +// +// The workspace import reports what its --repair-nul flag changed in a response +// header rather than in the body, because the success body is the created +// workspace and its shape is a public contract. A caller that does not need the +// count keeps using the wrapper above. +// +// Headers are captured BEFORE handleResponse, which reads and closes the body; +// on an error path they are returned alongside the error rather than dropped, +// so a caller can still read a diagnostic header from a failed request. +func (c *Client) PostStreamWithContentTypeHeaders(path string, body io.Reader, contentType string, result interface{}) (http.Header, error) { req, err := c.newRequest("POST", path, body) if err != nil { - return err + return nil, err } req.Header.Set("Content-Type", contentType) resp, err := c.streamClient.Do(req) if err != nil { - return fmt.Errorf("request failed: %w", err) + return nil, fmt.Errorf("request failed: %w", err) } defer resp.Body.Close() - return c.handleResponse(resp, result) + header := resp.Header + return header, c.handleResponse(resp, result) } // --- Auth API --- diff --git a/internal/server/handlers_import_bundle.go b/internal/server/handlers_import_bundle.go index c32c063b..f0b91a24 100644 --- a/internal/server/handlers_import_bundle.go +++ b/internal/server/handlers_import_bundle.go @@ -124,7 +124,13 @@ func (s *Server) handleImportWorkspaceBundle(w http.ResponseWriter, r *http.Requ newName := r.URL.Query().Get("name") userID := currentUserID(r) - ws, err := s.importBundle(r.Context(), gz, newName, userID) + // The bundle door gets the same --repair-nul treatment as the JSON one: + // a gzip import is the same import reached by a different Content-Type, + // and BUG-2803's round 3 already learned that giving the two doors + // different answers is how one of them keeps being forgotten. + repair := &nulRepairTally{Enabled: wantsNULRepair(r)} + + ws, err := s.importBundle(r.Context(), gz, newName, userID, repair) if err != nil { // Errors from importBundle are already shaped with status hints — // surface as 400 unless the underlying error wraps an http hint. @@ -184,6 +190,7 @@ func (s *Server) handleImportWorkspaceBundle(w http.ResponseWriter, r *http.Requ "workspace_id", ws.ID, "user_id", userID, "error", err) } } + repair.SetHeader(w) writeJSON(w, http.StatusCreated, ws) } @@ -206,7 +213,7 @@ func (s *Server) handleImportWorkspaceBundle(w http.ResponseWriter, r *http.Requ // Split out from the handler so tests can drive it with a tar.Reader // over an in-memory bundle and assert on the resulting state without // a live HTTP server. -func (s *Server) importBundle(ctx context.Context, r io.Reader, newName, ownerID string) (*models.Workspace, error) { +func (s *Server) importBundle(ctx context.Context, r io.Reader, newName, ownerID string, repair *nulRepairTally) (*models.Workspace, error) { tr := tar.NewReader(r) blobCap := s.effectiveBlobMaxBytes() @@ -280,10 +287,12 @@ func (s *Server) importBundle(ctx context.Context, r io.Reader, newName, ownerID // the same door as the JSON one, reached by a different // Content-Type, so it gets the same answer rather than a 500 // from Postgres further down (codex round 3). + buf = repair.Apply(buf) if bodyDecodesNUL(buf) { return nil, &importStatusError{ status: http.StatusBadRequest, code: "bad_bundle", - message: "Bundle pad-export.json could not be decoded: " + errJSONBodyNUL.Error(), + message: "Bundle pad-export.json could not be decoded: " + errJSONBodyNUL.Error() + + nulRepairRemedy(repair), } } var export models.WorkspaceExport @@ -358,8 +367,10 @@ func (s *Server) importBundle(ctx context.Context, r io.Reader, newName, ownerID // the bundle-import contract rather than a fix to this bug. The // resulting state is pinned by a test and stated in the release // note instead of being left incidental. + buf = repair.Apply(buf) if bodyDecodesNUL(buf) { - return ws, fmt.Errorf("manifest decode: %w (workspace created but attachments not restored)", errJSONBodyNUL) + return ws, fmt.Errorf("manifest decode: %w (workspace created but attachments not restored)%s", + errJSONBodyNUL, nulRepairRemedy(repair)) } var manifest models.AttachmentManifest if err := json.Unmarshal(buf, &manifest); err != nil { diff --git a/internal/server/handlers_workspace_import_nul_test.go b/internal/server/handlers_workspace_import_nul_test.go new file mode 100644 index 00000000..02981a18 --- /dev/null +++ b/internal/server/handlers_workspace_import_nul_test.go @@ -0,0 +1,218 @@ +package server + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/textguard" +) + +// The workspace import's --repair-nul flag, measured AT THE DOOR (DOC-2823 S3, +// BUG-2810). +// +// The unit-level tests in nul_repair_differential_test.go measure the repair +// and the remedy string. These drive the handler, because the defect BUG-2810 +// describes is a round trip — "the server emits a payload it will then refuse" +// — and the flag's whole job is to close it. A door-level test is also what +// catches the wiring mistake this file's first version had: the JSON path +// repaired the body and then discarded the count, so the header reported 0 +// while the import had rewritten a value. + +// nulExportBody builds the payload an export of an AFFECTED database produces. +// +// The escape is not typed: a real NUL is put in a Go string and json.Marshal +// writes it out as the six-character escape, which is exactly how the export +// endpoint produces one. Typing the escape here would make the fixture a +// statement about this file rather than about the export. +func nulExportBody(t *testing.T, content string) []byte { + t.Helper() + b, err := json.Marshal(models.WorkspaceExport{ + Version: 1, + Workspace: models.WorkspaceExportMeta{Name: "Affected", Slug: "affected"}, + Collections: []models.CollectionExport{{ + ID: "col-1", Name: "Tasks", Slug: "tasks", Schema: "{}", Settings: "{}", + }}, + Items: []models.ItemExport{{ + ID: "item-1", CollectionID: "col-1", Title: "Subject", Slug: "subject", + Content: content, Fields: "{}", Tags: "[]", ItemNumber: 1, + }}, + }) + if err != nil { + t.Fatalf("marshal export: %v", err) + } + return b +} + +func importWithQuery(t *testing.T, srv *Server, query string, body []byte) *httptest.ResponseRecorder { + t.Helper() + r := httptest.NewRequest("POST", "/api/v1/workspaces/import"+query, bytes.NewReader(body)) + r.Header.Set("Content-Type", "application/json") + r.RemoteAddr = "192.0.2.1:1234" + rr := httptest.NewRecorder() + srv.handleImportWorkspace(rr, r) + return rr +} + +// TestImportOfAnAffectedExportIsRefusedAndTheRemedyWorks is BUG-2810's filed +// symptom and its fix, in one test, in that order. +func TestImportOfAnAffectedExportIsRefusedAndTheRemedyWorks(t *testing.T) { + affected := nulExportBody(t, "before"+textguard.NUL+"after") + + // The fixture must actually carry the escape, or everything below passes + // while measuring nothing. + if !bytes.Contains(affected, []byte(textguard.EscNUL)) { + t.Fatalf("fixture does not carry the escape: %s", affected) + } + + t.Run("strict refuses it and names the flag", func(t *testing.T) { + srv := testServer(t) + rr := importWithQuery(t, srv, "", affected) + + if rr.Code != http.StatusBadRequest { + t.Fatalf("strict import returned %d, want 400: %s", rr.Code, rr.Body.String()) + } + body := rr.Body.String() + if !strings.Contains(body, "--repair-nul") { + t.Errorf("the refusal does not name the remedy: %s", body) + } + if !strings.Contains(body, "pad db repair-nul") { + t.Errorf("the refusal does not name the database-side repair: %s", body) + } + }) + + t.Run("the named flag accepts the same body and reports the count", func(t *testing.T) { + srv := testServer(t) + rr := importWithQuery(t, srv, "?repair_nul=true", affected) + + if rr.Code != http.StatusCreated { + t.Fatalf("import with --repair-nul returned %d, want 201: %s", rr.Code, rr.Body.String()) + } + if got := rr.Header().Get(NULRepairHeader); got != "1" { + t.Errorf("%s = %q, want \"1\" — the operator is told what was rewritten, and a count that "+ + "is always 0 is how the repair silently stops being reported", NULRepairHeader, got) + } + + // And the stored value is the repaired one, not the original and not + // something blanked. This is the assertion that makes the 201 mean + // something. + var ws models.Workspace + if err := json.Unmarshal(rr.Body.Bytes(), &ws); err != nil { + t.Fatalf("decode response: %v", err) + } + items, err := srv.store.ListItems(ws.ID, models.ItemListParams{}) + if err != nil { + t.Fatalf("list items: %v", err) + } + if len(items) != 1 { + t.Fatalf("imported %d items, want 1", len(items)) + } + if want := "before" + textguard.Replacement + "after"; items[0].Content != want { + t.Errorf("imported content = %q, want %q", items[0].Content, want) + } + }) + + t.Run("a clean export reports zero rather than nothing", func(t *testing.T) { + srv := testServer(t) + rr := importWithQuery(t, srv, "?repair_nul=true", nulExportBody(t, "ordinary content")) + + if rr.Code != http.StatusCreated { + t.Fatalf("clean import with the flag returned %d, want 201: %s", rr.Code, rr.Body.String()) + } + // Zero is a real answer: it tells the operator the export did not need + // the flag. An absent header would be indistinguishable from an old + // server that ignored it. + if got := rr.Header().Get(NULRepairHeader); got != "0" { + t.Errorf("%s = %q, want \"0\"", NULRepairHeader, got) + } + }) + + t.Run("the flag does not repair without being asked", func(t *testing.T) { + // The control for the whole feature: an import that repaired by + // default would pass every assertion above and break the strict + // posture Dave's ruling keeps. + srv := testServer(t) + rr := importWithQuery(t, srv, "?repair_nul=false", affected) + if rr.Code != http.StatusBadRequest { + t.Fatalf("repair_nul=false returned %d, want 400 — the default must stay strict: %s", + rr.Code, rr.Body.String()) + } + }) +} + +// TestImportBundle_RepairFlagCoversTheBundleDoorToo is the parity leg. +// +// handleImportWorkspaceBundle is reachable only through the Content-Type +// dispatch in handleImportWorkspace, so a flag honoured below that dispatch +// would work on the JSON path and silently do nothing on the tar.gz one — which +// is the path a real `pad workspace export --bundle` produces. BUG-2803's round +// 3 found the strict check missing on exactly this door for the same reason, +// and the plan-limit unit found its gate missing there too. +func TestImportBundle_RepairFlagCoversTheBundleDoorToo(t *testing.T) { + src, srcSlug := testServerWithAttachments(t) + rr := doRequest(src, "GET", "/api/v1/workspaces/"+srcSlug+"/export", nil) + if rr.Code != http.StatusOK { + t.Fatalf("export src: %d %s", rr.Code, rr.Body.String()) + } + clean := rr.Body.String() + + // Same fixture shape as TestImportBundle_RefusesNULInExport: the escape + // goes into the exported workspace NAME, because a bundle is bytes on the + // wire and that is the shape an affected export carries. + withNUL := strings.Replace(clean, `"name":"`, `"name":"a`+textguard.EscNUL+`b `, 1) + if withNUL == clean { + t.Fatal("fixture did not modify the export; the probe would be vacuous") + } + + bundle := func(exportJSON string) []byte { + var buf bytes.Buffer + gzw := gzip.NewWriter(&buf) + tw := tar.NewWriter(gzw) + if err := tw.WriteHeader(&tar.Header{ + Name: "pad-export.json", Mode: 0o644, Size: int64(len(exportJSON)), + }); err != nil { + t.Fatalf("write header: %v", err) + } + if _, err := tw.Write([]byte(exportJSON)); err != nil { + t.Fatalf("write export: %v", err) + } + tw.Close() + gzw.Close() + return buf.Bytes() + } + + post := func(query string, body []byte) *httptest.ResponseRecorder { + dest, _ := testServerWithAttachments(t) + req := httptest.NewRequest("POST", "/api/v1/workspaces/import"+query, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/gzip") + req.RemoteAddr = "127.0.0.1:1234" + rec := httptest.NewRecorder() + dest.ServeHTTP(rec, req) + return rec + } + + // Strict still refuses, and names the remedy. + strict := post("?name=Strict", bundle(withNUL)) + if strict.Code != http.StatusBadRequest { + t.Fatalf("strict bundle import returned %d, want 400: %s", strict.Code, strict.Body.String()) + } + if !strings.Contains(strict.Body.String(), "--repair-nul") { + t.Errorf("the bundle refusal does not name the remedy: %s", strict.Body.String()) + } + + // And the named flag accepts the same bytes. + repaired := post("?name=Repaired&repair_nul=true", bundle(withNUL)) + if repaired.Code != http.StatusOK && repaired.Code != http.StatusCreated { + t.Fatalf("bundle import with --repair-nul returned %d, want 201 — the flag is honoured on the "+ + "JSON door and not this one: %s", repaired.Code, repaired.Body.String()) + } + if got := repaired.Header().Get(NULRepairHeader); got != "1" { + t.Errorf("%s = %q, want \"1\"", NULRepairHeader, got) + } +} diff --git a/internal/server/handlers_workspaces.go b/internal/server/handlers_workspaces.go index 9491c51f..262d61f2 100644 --- a/internal/server/handlers_workspaces.go +++ b/internal/server/handlers_workspaces.go @@ -880,8 +880,29 @@ func (s *Server) handleImportWorkspace(w http.ResponseWriter, r *http.Request) { // the default 2 MiB decodeJSON cap. 64 MiB is well above any realistic // single-workspace backup while still far from the heap-exhaustion // range the default cap protects against. - if err := decodeJSONWithLimit(r, &data, 64<<20); err != nil { - writeError(w, http.StatusBadRequest, "bad_request", "invalid export data: "+err.Error()) + // + // --repair-nul (DOC-2823 S3 / BUG-2810). The default is strict; the flag + // buys the body ONE repair attempt and then runs the same gate on the + // repaired bytes, so this is not a decode path that skips the check. + repair := &nulRepairTally{Enabled: wantsNULRepair(r)} + var decodeErr error + if repair.Enabled { + var replaced int + replaced, decodeErr = decodeJSONRepairingNUL(r, &data, 64<<20) + repair.Replaced += replaced + } else { + decodeErr = decodeJSONWithLimit(r, &data, 64<<20) + } + if decodeErr != nil { + msg := "invalid export data: " + decodeErr.Error() + if errors.Is(decodeErr, errJSONBodyNUL) { + // The strict refusal NAMES the remedy, per Dave's day-54 ruling — + // and TestImportStrictRefusalNamesTheWorkingRemedy drives the named + // flag against this exact failing body, because a suggested remedy + // is an untested contract claim until it has been run (PATTE-135). + msg += nulRepairRemedy(repair) + } + writeError(w, http.StatusBadRequest, "bad_request", msg) return } @@ -908,5 +929,10 @@ func (s *Server) handleImportWorkspace(w http.ResponseWriter, r *http.Request) { } } + if repair.Enabled && repair.Replaced > 0 { + slog.Info("workspace import repaired NUL escapes on the operator's instruction", + "workspace_id", ws.ID, "replaced", repair.Replaced) + } + repair.SetHeader(w) writeJSON(w, http.StatusCreated, ws) } diff --git a/internal/server/import_nul_repair.go b/internal/server/import_nul_repair.go new file mode 100644 index 00000000..22bbc75b --- /dev/null +++ b/internal/server/import_nul_repair.go @@ -0,0 +1,107 @@ +package server + +import ( + "net/http" + "strconv" + + "github.com/PerpetualSoftware/pad/internal/store" +) + +// The workspace import's --repair-nul consent flag (DOC-2823 S3, BUG-2810). +// +// WHY IT EXISTS. A self-hoster whose database predates the NUL enforcement can +// EXPORT a workspace and then not import it back: the server emits a payload it +// will refuse. That is the sentence in BUG-2810 that made it an item rather +// than a note — the product has no path forward for somebody restoring their +// own backup. +// +// WHAT IT IS NOT. It is not an exemption from the gate. The gate still runs, on +// the repaired bytes, and still decides; the flag buys the body one repair +// attempt first. BUG-2803's filing named this endpoint as carrying the largest +// attacker-controlled body in the product and explicitly ruled out exempting +// it, so nothing here may become a decode path that skips the check. +// +// Dave's day-54 ruling: the flag ships, the default stays strict, the strict +// refusal names the flag, and a test drives that named remedy against the exact +// failing fixture. + +// NULRepairQueryParam is how the flag reaches the server. The CLI's +// `pad workspace import --repair-nul` sets it. +// +// EXPORTED so the CLI sends the name this package reads, rather than a second +// spelling of it. A query parameter and a response header are a two-sided +// contract, and the failure mode of the two sides drifting is silent: the +// import simply stops repairing, or stops reporting, with nothing to see. +const NULRepairQueryParam = "repair_nul" + +// NULRepairHeader reports how many escapes were replaced, so the operator is +// told what changed rather than only that something did. A header rather than a +// response field because the success body is the created workspace and its +// shape is a public contract. +const NULRepairHeader = "X-Pad-Repaired-NUL-Escapes" + +// nulRepairTally carries the flag through an import and counts what it changed. +type nulRepairTally struct { + Enabled bool + Replaced int +} + +// wantsNULRepair reports whether the request asked for the repair. +func wantsNULRepair(r *http.Request) bool { + switch r.URL.Query().Get(NULRepairQueryParam) { + case "1", "true", "yes": + return true + } + return false +} + +// Apply repairs a JSON document's live NUL escapes when the flag is set, and +// returns the bytes the gate should judge. +// +// A nil tally, or one with the flag unset, returns the input untouched — so a +// caller that forgets to thread the flag gets the strict behaviour, which is +// the safe direction for this particular forgetting. +func (t *nulRepairTally) Apply(raw []byte) []byte { + if t == nil || !t.Enabled { + return raw + } + repaired, n := repairBodyNULEscapes(raw) + t.Replaced += n + return repaired +} + +// SetHeader reports the count on a successful import. +func (t *nulRepairTally) SetHeader(w http.ResponseWriter) { + if t == nil || !t.Enabled { + return + } + w.Header().Set(NULRepairHeader, strconv.Itoa(t.Replaced)) +} + +// nulRepairRemedy is the sentence appended to a strict refusal, naming the flag +// that would have accepted it. +// +// It says something DIFFERENT when the flag was already given, because at that +// point the value is one the repair could not fix, and telling an operator to +// re-run with a flag they just used is worse than saying nothing (PATTE-135 in +// the other direction: a remedy that does not work is still a contract claim). +func nulRepairRemedy(t *nulRepairTally) string { + if t != nil && t.Enabled { + // DELIBERATELY DOES NOT NAME THE CAUSE. An earlier wording said "the + // value carries a NUL byte rather than an escape", which is one of at + // least two possibilities and so is a claim this code cannot make: the + // other is an escape spelled indirectly (`\u005cu0000` decodes to the + // escape TEXT, which a nested document then re-parses into a NUL) — + // the oblique form BUG-2803's round 4 found, which the gate catches and + // a document-level rewrite does not reach. Naming the wrong one sends + // the operator to the wrong fix. + return ". --repair-nul could not repair this value: it is not a plain NUL escape in the document." + + " Repair the source database with '" + store.RepairNULCommand + "' and export again" + } + // Worded for BOTH doors. The web UI posts to this same endpoint, and a + // browser user has no command line to add a flag to — so the message names + // the option and then where to find it, rather than assuming a terminal. + return ". This export was written by a Pad older than the check that now refuses it. Re-run the import" + + " with the NUL repair option (--repair-nul on the CLI) to replace each NUL with U+FFFD, or repair" + + " the source database first with '" + store.RepairNULCommand + "'" +} diff --git a/internal/server/nul_repair_differential_test.go b/internal/server/nul_repair_differential_test.go new file mode 100644 index 00000000..7e4fa80c --- /dev/null +++ b/internal/server/nul_repair_differential_test.go @@ -0,0 +1,222 @@ +package server + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/textguard" +) + +// The HTTP gate's REPAIR leg (DOC-2823 S3). The other three are in +// internal/store, beside their own corpus legs. + +// TestHTTPGateAcceptsEveryRepairedCorpusValue drives each repaired value +// through the gate exactly as the refusal leg drives the original, with the +// same key-derived classing. +func TestHTTPGateAcceptsEveryRepairedCorpusValue(t *testing.T) { + for _, c := range textguard.Corpus { + t.Run(c.Name, func(t *testing.T) { + repaired := textguard.Repair(c.Value, c.IsJSON) + + key := "content" + if c.IsJSON { + key = "fields" + } + body, err := json.Marshal(map[string]any{key: repaired}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if bodyDecodesNUL(body) { + t.Errorf("the gate refuses a REPAIRED value\n original: %q\n repaired: %q\n body: %s\n"+ + " why this case exists: %s", c.Value, repaired, body, c.Why) + } + }) + } +} + +// TestImportRepairFlagRepairsExactlyTheEscape pins what the flag does to a +// body, at the level the handler consumes. +// +// The two negative halves are the load-bearing ones. A raw NUL BYTE makes the +// document invalid JSON, so repairing one would turn a body the decoder rejects +// into one it accepts — and widening what parses is not this flag's job. And a +// doubled-backslash literal decodes to no NUL at all, so rewriting it would +// corrupt a legitimate value. +func TestImportRepairFlagRepairsExactlyTheEscape(t *testing.T) { + esc := textguard.EscNUL + backslash := esc[:1] + + cases := []struct { + name string + body string + wantChanged bool + wantCount int + why string + }{ + { + name: "a live escape is replaced", + body: `{"content":"x` + esc + `y"}`, + wantChanged: true, wantCount: 1, + why: "the shape a pre-enforcement export actually carries.", + }, + { + name: "two live escapes are both replaced and counted", + body: `{"content":"x` + esc + `y","title":"a` + esc + `b"}`, + wantChanged: true, wantCount: 2, + why: "the count is reported to the operator, so it has to be a count and not a flag.", + }, + { + name: "a doubled-backslash literal is untouched", + body: `{"content":"x` + backslash + esc + `y"}`, + wantChanged: false, wantCount: 0, + why: "literal text after an escaped backslash. Rewriting it would corrupt a valid value.", + }, + { + name: "a raw NUL byte is left to fail the decode", + body: `{"content":"x` + textguard.NUL + `y"}`, + wantChanged: false, wantCount: 0, + why: "invalid JSON. Repairing it would make a body parse that previously did not.", + }, + { + name: "a clean body is untouched", + body: `{"content":"ordinary"}`, + wantChanged: false, wantCount: 0, + why: "the control. Without it a repair that rewrites everything passes every other case.", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, n := repairBodyNULEscapes([]byte(tc.body)) + changed := string(got) != tc.body + if changed != tc.wantChanged { + t.Errorf("%s\n body: %q\n repaired: %q\n changed=%t, want %t", + tc.why, tc.body, got, changed, tc.wantChanged) + } + if n != tc.wantCount { + t.Errorf("replaced %d, want %d (%s)", n, tc.wantCount, tc.why) + } + // Whatever came back, the gate must accept it when the repair + // claims to have fixed something — that is the whole contract. + if tc.wantChanged && bodyDecodesNUL(got) { + t.Errorf("the repaired body is still refused by the gate: %q", got) + } + }) + } +} + +// TestImportStrictRefusalNamesTheWorkingRemedy is Dave's day-54 ruling 2 in +// test form, and PATTE-135's rule: a suggested remedy is an untested contract +// claim until something runs it. +// +// So this does not merely assert that the refusal message mentions a flag. It +// takes the EXACT body that produced the refusal, applies the named remedy, and +// asserts the gate then accepts it — which is the only version of this test +// that would fail if the message named a flag that did not work. +func TestImportStrictRefusalNamesTheWorkingRemedy(t *testing.T) { + failing := []byte(`{"items":[{"content":"x` + textguard.EscNUL + `y"}]}`) + + // Precondition: strict really does refuse this body. Without it the test + // could pass against a fixture that was never refused in the first place. + if !bodyDecodesNUL(failing) { + t.Fatalf("fixture does not reproduce the refusal: %s", failing) + } + + strict := &nulRepairTally{Enabled: false} + msg := nulRepairRemedy(strict) + if !strings.Contains(msg, "--repair-nul") { + t.Fatalf("the strict refusal does not name the flag: %q", msg) + } + if !strings.Contains(msg, "pad db repair-nul") { + t.Errorf("the refusal does not name the database-side repair either: %q", msg) + } + + // Now RUN the named remedy against the same body. + withFlag := &nulRepairTally{Enabled: true} + repaired := withFlag.Apply(failing) + if bodyDecodesNUL(repaired) { + t.Fatalf("the remedy the refusal names does not accept the body it was suggested for\n"+ + " body: %s\n repaired: %s", failing, repaired) + } + if withFlag.Replaced != 1 { + t.Errorf("replaced %d escapes, want 1", withFlag.Replaced) + } + + // And the message an operator sees AFTER using the flag must not tell them + // to use it again — at that point the repair has already been tried and did + // not fix the value. + afterMsg := nulRepairRemedy(withFlag) + if strings.Contains(afterMsg, "Re-run the import with --repair-nul") { + t.Errorf("the post-flag message repeats a remedy the caller already used: %q", afterMsg) + } + if !strings.Contains(afterMsg, "pad db repair-nul") { + t.Errorf("the post-flag message gives no remaining course of action: %q", afterMsg) + } +} + +// TestNULRepairTallyDefaultsToStrict pins the direction a mistake falls in. +// +// A nil tally, or one nobody set the flag on, must leave the body alone — so a +// call site that forgets to thread the flag gets the strict behaviour rather +// than a silently repairing import. +func TestNULRepairTallyDefaultsToStrict(t *testing.T) { + body := []byte(`{"content":"x` + textguard.EscNUL + `y"}`) + + var nilTally *nulRepairTally + if got := nilTally.Apply(body); string(got) != string(body) { + t.Errorf("a nil tally repaired the body: %q", got) + } + + off := &nulRepairTally{} + if got := off.Apply(body); string(got) != string(body) { + t.Errorf("a disabled tally repaired the body: %q", got) + } + if off.Replaced != 0 { + t.Errorf("a disabled tally counted %d replacements", off.Replaced) + } +} + +// TestRepairFlagDoesNotReachTheObliqueEscape pins the limit the post-flag +// message now claims, rather than leaving it as prose. +// +// A backslash spelled as its OWN escape (u005c) followed by the text u0000 +// decodes to the six-character NUL escape, which a nested document then +// re-parses into a real NUL — the oblique spelling BUG-2803's round 4 found, +// and the reason the gate walks the DECODED body rather than searching raw +// bytes. A document-level rewrite cannot reach it: at the layer the repair +// scans, those bytes are an escaped backslash followed by ordinary text. +// +// So the flag must NOT accept this body, and the message it produces must not +// tell the operator the cause is a raw NUL byte, which it is not. +func TestRepairFlagDoesNotReachTheObliqueEscape(t *testing.T) { + backslash := textguard.EscNUL[:1] + oblique := backslash + "u005c" + "u0000" + body := []byte(`{"fields":"{\"a\":\"x` + oblique + `y\"}"}`) + + // Precondition: the gate refuses it. If it does not, this test is measuring + // a body that was never a problem. + if !bodyDecodesNUL(body) { + t.Fatalf("the gate does not refuse the oblique fixture; it proves nothing: %s", body) + } + + tally := &nulRepairTally{Enabled: true} + repaired := tally.Apply(body) + if tally.Replaced != 0 { + t.Errorf("the repair claims to have rewritten %d escape(s) in a body that carries none at its "+ + "own layer: %s", tally.Replaced, repaired) + } + if !bodyDecodesNUL(repaired) { + t.Fatalf("the repair made the oblique body acceptable — it must not, since the value it would " + + "have to rewrite lives one decode deeper than the document it scans") + } + + msg := nulRepairRemedy(tally) + if strings.Contains(msg, "NUL byte") { + t.Errorf("the post-flag message names a cause it cannot know — this value carries an escape, "+ + "not a raw byte: %q", msg) + } + if !strings.Contains(msg, "pad db repair-nul") { + t.Errorf("the post-flag message leaves the operator with no course of action: %q", msg) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 8181679a..6d848c98 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -37,6 +37,7 @@ import ( "github.com/PerpetualSoftware/pad/internal/models" "github.com/PerpetualSoftware/pad/internal/oauth" "github.com/PerpetualSoftware/pad/internal/store" + "github.com/PerpetualSoftware/pad/internal/textguard" "github.com/PerpetualSoftware/pad/internal/watchevents" "github.com/PerpetualSoftware/pad/internal/webhooks" ) @@ -2274,6 +2275,60 @@ func decodeJSONWithLimit(r *http.Request, v interface{}, maxBytes int64) error { if err != nil { return fmt.Errorf("invalid JSON: %w", err) } + return decodeJSONBytes(raw, v) +} + +// decodeJSONRepairingNUL is decodeJSONWithLimit with exactly one difference, +// and the difference is deliberately NOT a bypass (DOC-2823 S3, BUG-2810). +// +// The workspace import's --repair-nul flag exists because a self-hoster whose +// database predates the enforcement can EXPORT a workspace and then not import +// it back: the server emits a payload it will refuse. Dave's day-54 ruling +// ships the flag with the default staying strict. +// +// What the flag does is repair the body and then run the SAME gate on the +// repaired bytes. It does not skip the gate, and it must not: a decode path +// that does is precisely the door BUG-2803 spent thirty rounds closing, and it +// would be reachable from the endpoint carrying the largest attacker-controlled +// body in the product. So a value the repair cannot fix is still refused, by +// the same function, with the same error. +// +// Only the ESCAPE form is repaired, via textguard.RepairJSONDocument. A raw NUL +// BYTE inside a JSON string makes the document invalid, so replacing one would +// turn a body the decoder rejects into one it accepts — widening what parses is +// not this flag's job. Those bodies keep failing where they failed. +// +// Returns how many escapes were replaced, which the handler reports. +func decodeJSONRepairingNUL(r *http.Request, v interface{}, maxBytes int64) (int, error) { + raw, err := readBodyForDecode(r, maxBytes) + if err != nil { + return 0, fmt.Errorf("invalid JSON: %w", err) + } + repaired, n := repairBodyNULEscapes(raw) + return n, decodeJSONBytes(repaired, v) +} + +// repairBodyNULEscapes replaces live NUL escapes in a JSON body, leaving a body +// that is not valid JSON untouched so its own decode error is what the caller +// reports. +func repairBodyNULEscapes(raw []byte) ([]byte, int) { + if !json.Valid(raw) { + return raw, 0 + } + repaired, n := textguard.RepairJSONDocument(string(raw)) + if n == 0 { + return raw, 0 + } + return []byte(repaired), n +} + +// decodeJSONBytes is everything decodeJSONWithLimit does once the body has been +// read: the empty-body contract, the NUL gate, and the unmarshal. +// +// Extracted so the --repair-nul path can insert a repair between the read and +// the gate WITHOUT reimplementing any of the three, which is what keeps the +// gate the single decider. +func decodeJSONBytes(raw []byte, v interface{}) error { // An EMPTY (or whitespace-only) body must keep returning a wrapped // io.EOF. json.Decoder.Decode answered io.EOF there and at least one // caller depends on it — handlers_playbooks.go treats diff --git a/internal/store/nul_repair_differential_test.go b/internal/store/nul_repair_differential_test.go new file mode 100644 index 00000000..ccfab799 --- /dev/null +++ b/internal/store/nul_repair_differential_test.go @@ -0,0 +1,161 @@ +package store + +import ( + "database/sql" + "encoding/json" + "os" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/textguard" +) + +// The REPAIR legs of the four-way differential test (DOC-2823 S3). +// +// The four existing legs measure what each layer REFUSES. These measure that +// every value the repair produces is one all four layers ACCEPT — which is the +// property `pad db repair-nul` exists to deliver, and the one a repair tested +// only against its own package could satisfy while still leaving rows that the +// database, or Postgres, will not have. +// +// Three of the four live here (Layer A, Layer B, native Postgres); the HTTP +// gate's is in internal/server, beside its own corpus leg. +// +// EACH LEG DRIVES THE SAME textguard.Repair the command calls. A leg that +// repaired values with a local helper would be measuring a repair nobody ships. + +// TestLayerAAcceptsEveryRepairedCorpusValue — the driver guard. +func TestLayerAAcceptsEveryRepairedCorpusValue(t *testing.T) { + s := testStore(t) + ws := createTestWorkspace(t, s, "RepairLayerA") + col := createTestCollection(t, s, ws.ID, "Tasks") + item := createTestItem(t, s, ws.ID, col.ID, "Repair subject", "") + + for _, c := range textguard.Corpus { + t.Run(c.Name, func(t *testing.T) { + repaired := textguard.Repair(c.Value, c.IsJSON) + + // Same routing rule the refusal legs use: a JSON-classed value that + // is not valid JSON goes to the text column, or the fields column + // rejects it as malformed before the guard is consulted and the + // case measures SQLite's JSON parser instead. + var err error + if c.IsJSON && json.Valid([]byte(strings.TrimSpace(repaired))) { + _, err = s.UpdateItem(item.ID, models_ItemUpdateFields(repaired)) + } else { + _, err = s.UpdateItem(item.ID, models_ItemUpdateContent(repaired)) + } + if err != nil { + t.Fatalf("Layer A refused a REPAIRED value — the repair does not satisfy the guard it is "+ + "meant to satisfy\n original: %q\n repaired: %q\n err: %v\n why this case exists: %s", + c.Value, repaired, err, c.Why) + } + }) + } +} + +// TestLayerBAcceptsEveryRepairedCorpusValue — the SQLite triggers, through a +// raw handle so what is measured is the DATABASE's verdict rather than Layer +// A's reflected back. +func TestLayerBAcceptsEveryRepairedCorpusValue(t *testing.T) { + s := testStore(t) + if s.dialect.Driver() != DriverSQLite { + t.Skip("Layer B is SQLite-only") + } + ws := createTestWorkspace(t, s, "RepairLayerB") + col := createTestCollection(t, s, ws.ID, "Tasks") + item := createTestItem(t, s, ws.ID, col.ID, "Repair subject", "") + + raw, err := sql.Open("sqlite", s.dbPath+"?_pragma=busy_timeout(30000)") + if err != nil { + t.Fatalf("open raw: %v", err) + } + defer raw.Close() + + for _, c := range textguard.Corpus { + t.Run(c.Name, func(t *testing.T) { + repaired := textguard.Repair(c.Value, c.IsJSON) + + var werr error + if c.IsJSON && json.Valid([]byte(strings.TrimSpace(repaired))) { + werr = execRaw(raw, `UPDATE workspaces SET settings = ? WHERE id = ?`, repaired, ws.ID) + } else { + werr = execRaw(raw, `UPDATE items SET content = ? WHERE id = ?`, repaired, item.ID) + } + if werr != nil { + t.Fatalf("Layer B refused a REPAIRED value\n original: %q\n repaired: %q\n err: %v", + c.Value, repaired, werr) + } + + // And it PERSISTED intact, for the reason the refusal leg reads its + // values back: a trigger that discarded the row would also produce + // no error. + var stored string + var rerr error + if c.IsJSON && json.Valid([]byte(strings.TrimSpace(repaired))) { + rerr = raw.QueryRow(`SELECT settings FROM workspaces WHERE id = ?`, ws.ID).Scan(&stored) + } else { + rerr = raw.QueryRow(`SELECT content FROM items WHERE id = ?`, item.ID).Scan(&stored) + } + if rerr != nil { + t.Fatalf("read back: %v", rerr) + } + if stored != repaired { + t.Errorf("the repaired value did not survive the round trip\n wrote: %q\n read: %q", + repaired, stored) + } + }) + } +} + +// TestNativePostgresAcceptsEveryRepairedCorpusValue — the leg that makes the +// repair worth running. +// +// BUG-2810's filing is that an affected workspace exports and will not import, +// and that `pad db migrate-to-pg` fails partway through the copy against +// PostgreSQL's own parser. That claim is only discharged by putting the +// repaired values in front of a real Postgres. +func TestNativePostgresAcceptsEveryRepairedCorpusValue(t *testing.T) { + dsn := os.Getenv("PAD_TEST_POSTGRES_URL") + if dsn == "" { + t.Skip("PAD_TEST_POSTGRES_URL not set; the native-Postgres leg needs a real server") + } + + // The RAW pgx driver, not the guarded name — otherwise this measures the + // guard agreeing with itself. + db, err := sql.Open("pgx", dsn) + if err != nil { + t.Fatalf("open raw pgx: %v", err) + } + defer db.Close() + + if _, err := db.Exec(`DROP TABLE IF EXISTS nul_repair_differential`); err != nil { + t.Fatalf("drop: %v", err) + } + if _, err := db.Exec( + `CREATE TABLE nul_repair_differential (id TEXT PRIMARY KEY, txt TEXT, doc JSONB)`, + ); err != nil { + t.Fatalf("create: %v", err) + } + defer func() { _, _ = db.Exec(`DROP TABLE IF EXISTS nul_repair_differential`) }() + + for i, c := range textguard.Corpus { + t.Run(c.Name, func(t *testing.T) { + repaired := textguard.Repair(c.Value, c.IsJSON) + id := "repaired-" + strings.ReplaceAll(c.Name, " ", "-") + "-" + string(rune('a'+i%26)) + + var err error + if c.IsJSON && json.Valid([]byte(strings.TrimSpace(repaired))) { + _, err = db.Exec( + `INSERT INTO nul_repair_differential (id, doc) VALUES ($1, $2)`, id, repaired) + } else { + _, err = db.Exec( + `INSERT INTO nul_repair_differential (id, txt) VALUES ($1, $2)`, id, repaired) + } + if err != nil { + t.Fatalf("PostgreSQL refused a REPAIRED value — the repair does not unblock the migration "+ + "it exists to unblock\n original: %q\n repaired: %q\n err: %v", c.Value, repaired, err) + } + }) + } +} diff --git a/internal/store/nulrepair.go b/internal/store/nulrepair.go new file mode 100644 index 00000000..b87bed7c --- /dev/null +++ b/internal/store/nulrepair.go @@ -0,0 +1,204 @@ +package store + +import ( + "database/sql" + "errors" + "fmt" + "strings" + + "github.com/PerpetualSoftware/pad/internal/textguard" +) + +// The explicit operator repair for the legacy NUL population (DOC-2823 S3). +// +// NEVER A MIGRATION, on Dave's day-54 ruling: a migration that rewrites user +// content decides consent for the operator. This runs only when somebody types +// `pad db repair-nul`, and `pad db migrate-to-pg` refuses and prints that +// command rather than repairing on the operator's behalf. +// +// IN GO, NOT IN SQL, for the reason TASK-2824 measured and this unit measured +// again on the read path: SQLite's own string functions disagree about a +// NUL-bearing value (`length()` answers 3 for an 8-byte value), so no SQL-side +// transform can be trusted to leave the rest of the value intact. The rewrite +// is textguard.Repair, which is also what the four enforcement layers are +// measured against. + +// RepairNULCommand is the exact command an operator runs to fix the rows this +// file repairs. +// +// It lives here, in the package that implements the repair, because three +// places quote it at an operator — the migrate-to-pg preflight, the import's +// strict refusal, and the CLI's own help — and a remedy naming a command that +// has been renamed is worse than no remedy at all. +const RepairNULCommand = "pad db repair-nul" + +// NULRepairSkip is a violation the repair deliberately did not touch. +type NULRepairSkip struct { + Violation NULViolation + Reason string +} + +// NULRepairFailure is a violation the repair tried and could not complete. +type NULRepairFailure struct { + Violation NULViolation + Err error +} + +// NULRepairReport is what the repair returns. Every violation the scan found +// ends up in exactly one of the three buckets, which is what lets the CLI +// report a total that adds up. +type NULRepairReport struct { + Scan *NULScanReport + Repaired []NULViolation + Skipped []NULRepairSkip + Failed []NULRepairFailure +} + +// RepairNUL rewrites every offending stored value, replacing each NUL with +// U+FFFD, and reports what it changed. +// +// PER-ROW TRANSACTIONS, not one big one. The repair is idempotent — running it +// twice changes nothing the second time, pinned in textguard — so a partial run +// is a resumable state rather than a corrupt one, and that is worth more here +// than atomicity across an unbounded number of rows: a single transaction over +// a large database holds SQLite's write lock for the whole sweep, which on the +// one deployment shape this exists for (a self-hoster's live instance) is the +// difference between a repair and an outage. +// +// Each row IS read and written inside its own transaction, so the value cannot +// change between the read and the rewrite. +func (s *Store) RepairNUL() (*NULRepairReport, error) { + scan, err := s.ScanNUL() + if err != nil { + return nil, err + } + report := &NULRepairReport{Scan: scan} + if !scan.Applicable { + return report, nil + } + + for _, v := range scan.Violations { + // THE PRIMARY KEY IS NOT REWRITTEN. Repairing a column that is part of + // its own row's key changes the row's identity, and U+FFFD substitution + // can land it on top of an existing row — for email_optouts(email), the + // only such column today, that would silently merge two opt-out records + // and could un-suppress mail to somebody. Refusing to guess is the + // posture the cross-workspace copy takes when a destination field needs + // a value; the operator is told which rows and why. + // + // It is also mechanically impossible through this handle: Layer A + // inspects every bound parameter, so a WHERE clause carrying the + // NUL-bearing key would be refused along with the write. + if _, isKey := v.Key[v.Column]; isKey { + report.Skipped = append(report.Skipped, NULRepairSkip{ + Violation: v, + Reason: "the column is part of the row's primary key; repairing it would change the row's " + + "identity and could collide with an existing row", + }) + continue + } + + repaired, err := s.repairOneNUL(v) + switch { + case err != nil: + report.Failed = append(report.Failed, NULRepairFailure{Violation: v, Err: err}) + case repaired: + report.Repaired = append(report.Repaired, v) + default: + // The value was clean by the time the transaction read it — + // somebody else repaired it, or the row changed. Not a failure and + // not a repair. + report.Skipped = append(report.Skipped, NULRepairSkip{ + Violation: v, + Reason: "the value no longer violates the invariant; nothing to do", + }) + } + } + return report, nil +} + +// repairOneNUL reads, rewrites and writes one value inside one transaction. +// It reports whether it actually changed anything. +func (s *Store) repairOneNUL(v NULViolation) (bool, error) { + tx, err := s.db.Begin() + if err != nil { + return false, fmt.Errorf("begin: %w", err) + } + defer func() { _ = tx.Rollback() }() + + where, args := nulRowPredicate(v) + qc := quoteIdent(v.Column) + qt := quoteIdent(v.Table) + + var value string + err = tx.QueryRow(fmt.Sprintf(`SELECT %s FROM %s WHERE %s`, qc, qt, where), args...).Scan(&value) + if errors.Is(err, sql.ErrNoRows) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("read %s.%s: %w", v.Table, v.Column, err) + } + + isJSON := nulColumnIsJSON(v.Table, v.Column) + if !textguard.ParameterRefused(value, isJSON) { + return false, nil + } + + repaired := textguard.Repair(value, 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 + // on it or reporting success would both be lies. + return false, fmt.Errorf("repair produced no change for a refused value in %s.%s", v.Table, v.Column) + } + + updateArgs := append([]any{repaired}, args...) + res, err := tx.Exec(fmt.Sprintf(`UPDATE %s SET %s = ? WHERE %s`, qt, qc, where), updateArgs...) + if err != nil { + return false, fmt.Errorf("update %s.%s: %w", v.Table, v.Column, err) + } + + // THE ROW COUNT IS CHECKED, and it is not defensive padding. The WHERE + // clause is built from values the scan read back out of the database, and + // one of them is a `rowid` for the single protected table that declares no + // primary key — an INTEGER column addressed with the TEXT the scan scanned + // it into. If any such binding ever stopped matching, this function would + // commit an UPDATE that touched nothing and report the value as repaired. + // A repair that reports success for a row it did not change is the one + // failure mode an operator cannot detect from the output. + n, err := res.RowsAffected() + if err != nil { + return false, fmt.Errorf("rows affected for %s.%s: %w", v.Table, v.Column, err) + } + if n != 1 { + return false, fmt.Errorf("repairing %s.%s matched %d rows, want exactly 1 — the row address the "+ + "scan produced does not select it", v.Table, v.Column, n) + } + if err := tx.Commit(); err != nil { + return false, fmt.Errorf("commit: %w", err) + } + return true, nil +} + +// nulRowPredicate renders the WHERE clause addressing one row, plus its args. +func nulRowPredicate(v NULViolation) (string, []any) { + keys := sortedKeys(v.Key) + clauses := make([]string, 0, len(keys)) + args := make([]any, 0, len(keys)) + for _, k := range keys { + clauses = append(clauses, quoteIdent(k)+" = ?") + args = append(args, v.Key[k]) + } + return strings.Join(clauses, " AND "), args +} + +// nulColumnIsJSON answers the classing question from the shared list, so the +// repair and the predicate cannot disagree about a column. +func nulColumnIsJSON(table, column string) bool { + for _, c := range NULProtectedColumns() { + if c.Table == table && c.Column == column { + return c.Class == classJSON + } + } + return false +} diff --git a/internal/store/nulscan.go b/internal/store/nulscan.go new file mode 100644 index 00000000..5704221c --- /dev/null +++ b/internal/store/nulscan.go @@ -0,0 +1,389 @@ +package store + +import ( + "fmt" + "sort" + "strings" + + "github.com/PerpetualSoftware/pad/internal/textguard" +) + +// The counter and the repair for the NUL invariant's LEGACY population +// (DOC-2823 S3, closing BUG-2810). +// +// Layers A and B stop the value being WRITTEN. Neither of them makes a row that +// already carries one go away, and such a row is not merely untidy: its +// workspace exports fine and re-imports 400, and `pad db migrate-to-pg` fails +// partway through the copy against PostgreSQL's jsonb parser. This file is the +// read-only census of that population and the explicit operator repair. +// +// THREE PROPERTIES, and each of them was a decision: +// +// ONE PREDICATE. The decision about a value is textguard.ParameterRefused with +// isJSON taken from nulColumns — the same core the HTTP gate, Layer A and Layer +// B share, with Layer B's column-derived classing. The SQL below narrows, it +// never decides. A fourth implementation of "decodes to a NUL" is the thing +// this whole cluster exists because of. +// +// THE COUNT IS COMPUTED IN GO, NOT IN SQL. Measured on the read path in this +// worktree: a row planted with `badname` reads back into a Go string with +// all 8 bytes and the NUL intact, while `length(name)` in the same database +// answers 3. TASK-2824 found that C-truncation in SQLite's string functions and +// concluded no DB-side REPAIR could be trusted; the same measurement on the +// read path says no DB-side COUNT can be either. +// +// SQLITE ONLY, and not by omission. PostgreSQL cannot hold either defect — +// SQLSTATE 22021 for a raw NUL in text, 22P05 for the escape reaching jsonb — +// which is why Layer B was ruled not to apply there, and the four-way +// differential test pins that native refusal. A scan of a Postgres database +// would be a full table scan of every protected column to prove a theorem. +// ScanNUL says so and returns an empty report rather than pretending it looked. + +// NULViolation is one stored value that violates the invariant. +type NULViolation struct { + Table string + Column string + // Key addresses the row: the declared primary-key columns and their + // values, or `rowid` for the one table that declares no primary key. + Key map[string]string + // WorkspaceID is the owning workspace, or "" for the 16 protected tables + // that carry no workspace_id column — see the census in nulscan_test.go. + WorkspaceID string + // RawNUL and EscapedNUL record WHICH defect the value carries. They are + // not exclusive: a JSON blob can hold both, and the repair has a separate + // pass for each. + RawNUL bool + EscapedNUL bool +} + +// String renders a violation the way the CLI reports it. +func (v NULViolation) String() string { + parts := make([]string, 0, len(v.Key)) + 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" + } + out := fmt.Sprintf("%s.%s [%s] (%s)", v.Table, v.Column, strings.Join(parts, ", "), kind) + if v.WorkspaceID != "" { + out += " workspace=" + v.WorkspaceID + } + return out +} + +// NULScanReport is what the counter returns. +type NULScanReport struct { + // Applicable is false on PostgreSQL, where the state cannot exist. Reason + // says why nothing was scanned, so a zero report is never mistaken for a + // scan that found nothing. + Applicable bool + Reason string + + // Violations is every offending value, in table/column order. The + // population this exists for is legacy rows on a single self-hosted + // database; it is not bounded, because an operator deciding whether to + // repair needs the whole list and a truncated one would understate it. + Violations []NULViolation + + // ColumnsScanned is how many of the protected columns actually exist in + // this database's schema, and ColumnsAbsent lists any that do not. + // A database at an older migration is not an error, but a scan that + // silently skipped columns is not a census. + ColumnsScanned int + ColumnsAbsent []string +} + +// Total is the number of offending values. +func (r *NULScanReport) Total() int { return len(r.Violations) } + +// ByWorkspace groups the violation count by workspace id, with "" collecting +// the tables that have no workspace column. +func (r *NULScanReport) ByWorkspace() map[string]int { + out := map[string]int{} + for _, v := range r.Violations { + out[v.WorkspaceID]++ + } + return out +} + +// ByColumn groups the violation count by "table.column". +func (r *NULScanReport) ByColumn() map[string]int { + out := map[string]int{} + for _, v := range r.Violations { + out[v.Table+"."+v.Column]++ + } + return out +} + +// ScanNUL counts and locates every stored value violating the NUL invariant. +// +// Read-only: it issues SELECTs and nothing else, so it is safe to run against a +// live server and safe to run repeatedly. `pad db migrate-to-pg` calls it as a +// preflight for exactly that reason. +// +// COST, stated because an operator should not be surprised by it: one +// unindexed scan per protected column — 131 of them today (24 JSON-classed, +// 107 text), measured from NULProtectedColumns rather than counted by hand. +// There is no index that would help, since the predicate is a substring search +// over the value. That is cheap next to the migration it guards, which reads +// every row of every table anyway. +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, " + + "22P05 for the escape reaching jsonb), so no stored row can carry one", + }, nil + } + + live, err := s.liveColumnTypes() + if err != nil { + return nil, err + } + + report := &NULScanReport{Applicable: true} + cols := NULProtectedColumns() + sort.Slice(cols, func(i, j int) bool { + if cols[i].Table != cols[j].Table { + return cols[i].Table < cols[j].Table + } + return cols[i].Column < cols[j].Column + }) + + addressing := map[string]tableAddressing{} + for _, c := range cols { + if !live[c.Table+"."+c.Column] { + report.ColumnsAbsent = append(report.ColumnsAbsent, c.Table+"."+c.Column) + continue + } + report.ColumnsScanned++ + + addr, ok := addressing[c.Table] + if !ok { + addr, err = s.addressingFor(c.Table) + if err != nil { + return nil, err + } + addressing[c.Table] = addr + } + + found, err := s.scanColumn(c, addr) + if err != nil { + return nil, err + } + report.Violations = append(report.Violations, found...) + } + sort.Strings(report.ColumnsAbsent) + return report, nil +} + +// tableAddressing is how a row in one table is named and attributed. +type tableAddressing struct { + // KeyColumns are the declared primary-key columns, or {"rowid"} for a + // table that declares none. item_wiki_links is the only such table today + // (measured); rowid is a correct address for it because it is not + // declared WITHOUT ROWID. + KeyColumns []string + // HasWorkspace says whether the table carries a workspace_id. + HasWorkspace bool + // KeyIsProtected marks a table whose PRIMARY KEY is itself a protected + // column. Repairing such a row rewrites its identity — and can collide + // with an existing row — so the repair refuses it and says so. + // email_optouts(email) is the only one today. + KeyIsProtected bool +} + +// addressingFor reads a table's primary key and workspace column from the live +// schema rather than from a list, because a hand-kept table→key mapping is the +// enumeration this cluster keeps proving unmaintainable. +func (s *Store) addressingFor(table string) (tableAddressing, error) { + rows, err := s.db.Query(`SELECT name, pk FROM pragma_table_info(?)`, table) + if err != nil { + return tableAddressing{}, fmt.Errorf("table_info %s: %w", table, err) + } + defer rows.Close() + + var addr tableAddressing + type pkCol struct { + name string + pos int + } + var pks []pkCol + for rows.Next() { + var name string + var pos int + if err := rows.Scan(&name, &pos); err != nil { + return tableAddressing{}, err + } + if name == "workspace_id" { + addr.HasWorkspace = true + } + if pos > 0 { + pks = append(pks, pkCol{name, pos}) + } + } + if err := rows.Err(); err != nil { + return tableAddressing{}, err + } + + // pragma_table_info's `pk` is the 1-based position WITHIN the key, so a + // composite key must be ordered by it rather than by column order. + sort.Slice(pks, func(i, j int) bool { return pks[i].pos < pks[j].pos }) + for _, p := range pks { + addr.KeyColumns = append(addr.KeyColumns, p.name) + } + if len(addr.KeyColumns) == 0 { + addr.KeyColumns = []string{"rowid"} + } + + protected := map[string]bool{} + for _, c := range NULProtectedColumns() { + if c.Table == table { + protected[c.Column] = true + } + } + for _, k := range addr.KeyColumns { + if protected[k] { + addr.KeyIsProtected = true + } + } + return addr, nil +} + +// scanColumn narrows in SQL and decides in Go. +// +// The WHERE clause is a PRE-FILTER on the two byte patterns a violating value +// must contain, and it is the same pre-filter textguard applies internally +// before it pays for a decode. It can only ever return a superset: +// +// - `instr(col, char(0))` finds a raw NUL. Measured (TASK-2824, and again on +// the read path here): instr searches the whole stored value, unlike +// length(), which stops at the NUL. +// - `instr(col, '\u00')` finds the four characters every NUL escape starts +// with. Only JSON-classed columns get it: in a text column those six +// characters are six characters, and refusing them there is the false +// positive that made BUG-2803's parity pre-filter unsound. +// +// Whether the match MEANS anything is then textguard's call, on the value read +// into Go — which is what keeps `{"a":"x\\u0000y"}` out of the report. +func (s *Store) scanColumn(c nulColumn, addr tableAddressing) ([]NULViolation, error) { + qt := quoteIdent(c.Table) + qc := quoteIdent(c.Column) + + sel := make([]string, 0, len(addr.KeyColumns)+2) + for _, k := range addr.KeyColumns { + sel = append(sel, quoteIdent(k)) + } + if addr.HasWorkspace { + sel = append(sel, `"workspace_id"`) + } + sel = append(sel, qc) + + where := fmt.Sprintf(`%s IS NOT NULL AND (instr(%s, char(0)) > 0`, qc, qc) + args := []any{} + if c.Class == classJSON { + where += fmt.Sprintf(` OR instr(%s, ?) > 0`, qc) + args = append(args, nulEscapePrefix) + } + where += `)` + + q := fmt.Sprintf(`SELECT %s FROM %s WHERE %s`, strings.Join(sel, ", "), qt, where) + rows, err := s.db.Query(q, args...) + if err != nil { + return nil, fmt.Errorf("scan %s.%s: %w", c.Table, c.Column, err) + } + defer rows.Close() + + var out []NULViolation + for rows.Next() { + dest := make([]any, 0, len(sel)) + keyVals := make([]string, len(addr.KeyColumns)) + for i := range keyVals { + dest = append(dest, &keyVals[i]) + } + var wsID string + if addr.HasWorkspace { + dest = append(dest, &wsID) + } + var value string + dest = append(dest, &value) + + if err := rows.Scan(dest...); err != nil { + return nil, fmt.Errorf("scan %s.%s row: %w", c.Table, c.Column, err) + } + + isJSON := c.Class == classJSON + if !textguard.ParameterRefused(value, isJSON) { + // The pre-filter matched and the predicate did not. This is the + // doubled-backslash case and it is EXPECTED, not an anomaly — the + // pre-filter is allowed to be a superset and is worthless if it is + // not. + continue + } + + v := NULViolation{ + Table: c.Table, + Column: c.Column, + Key: map[string]string{}, + WorkspaceID: wsID, + RawNUL: textguard.ContainsNUL(value), + } + v.EscapedNUL = isJSON && textguard.DocumentDecodesNULAnyShape(value) + for i, k := range addr.KeyColumns { + v.Key[k] = keyVals[i] + } + out = append(out, v) + } + return out, rows.Err() +} + +// nulEscapePrefix is the four characters every NUL escape begins with, built +// rather than typed: typing them produces the CHARACTER in a Go source file, +// which is the decay corpus.go records and which happened twice while writing +// this unit. +var nulEscapePrefix = textguard.EscNUL[:4] + +// liveColumnTypes reports which table.column pairs actually exist, so a +// database at an older migration is scanned for what it has rather than +// erroring on a column the list knows about and the schema does not. +func (s *Store) liveColumnTypes() (map[string]bool, error) { + rows, err := s.db.Query(` + SELECT m.name, ti.name + FROM sqlite_master m, pragma_table_info(m.name) ti + WHERE m.type = 'table' + `) + if err != nil { + return nil, fmt.Errorf("read live schema: %w", err) + } + defer rows.Close() + out := map[string]bool{} + for rows.Next() { + var table, col string + if err := rows.Scan(&table, &col); err != nil { + return nil, err + } + out[table+"."+col] = true + } + return out, rows.Err() +} + +// quoteIdent quotes a SQL identifier. The names come from this package's own +// list rather than from user input, but the trigger restoration learned that +// quoting is worth having anyway when identifiers are interpolated at all. +func quoteIdent(name string) string { + return `"` + strings.ReplaceAll(name, `"`, `""`) + `"` +} + +func sortedKeys(m map[string]string) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} diff --git a/internal/store/nulscan_test.go b/internal/store/nulscan_test.go new file mode 100644 index 00000000..291f305a --- /dev/null +++ b/internal/store/nulscan_test.go @@ -0,0 +1,415 @@ +package store + +import ( + "database/sql" + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/textguard" +) + +// dropAllNULTriggers turns the store's database into a pre-S2 one for the +// duration of a test, so a raw handle can plant the LEGACY rows this unit +// exists to find. Restores them before returning to the caller's control. +// +// A raw *sql.DB opened on the same file is what an old binary is: no Layer A +// wrapper. With the triggers gone too, neither enforcement layer is present, +// which is exactly the window BUG-2813 describes and the state BUG-2810's rows +// were written in. +func plantLegacyRows(t *testing.T, s *Store, plant func(raw *sql.DB)) { + t.Helper() + + raw, err := sql.Open("sqlite", s.dbPath+"?_pragma=busy_timeout(30000)") + if err != nil { + t.Fatalf("open raw: %v", err) + } + defer raw.Close() + + names, err := nulTriggersIn(raw) + if err != nil { + t.Fatalf("list triggers: %v", err) + } + for name := range names { + if _, err := raw.Exec(`DROP TRIGGER IF EXISTS "` + name + `"`); err != nil { + t.Fatalf("drop %s: %v", name, err) + } + } + + plant(raw) + + // Put the database back the way a real one is BEFORE the scan runs. A scan + // measured against a database with no triggers would not be measuring the + // deployment it exists for, and the repair's writes have to pass Layer B. + if _, err := s.ensureNULTriggersReporting(); err != nil { + t.Fatalf("restore triggers: %v", err) + } +} + +// TestScanNULFindsThePlantedPopulation drives every shape the counter has to +// tell apart, including the two it must NOT report. +func TestScanNULFindsThePlantedPopulation(t *testing.T) { + s := testStore(t) + if s.dialect.Driver() != DriverSQLite { + t.Skip("the scan is SQLite-only; Postgres cannot hold the state") + } + + ws := createTestWorkspace(t, s, "ScanWS") + col := createTestCollection(t, s, ws.ID, "Tasks") + dirtyText := createTestItem(t, s, ws.ID, col.ID, "clean title", "clean body") + dirtyJSON := createTestItem(t, s, ws.ID, col.ID, "second", "body") + literalOnly := createTestItem(t, s, ws.ID, col.ID, "third", "body") + cleanItem := createTestItem(t, s, ws.ID, col.ID, "fourth", "body") + + esc := textguard.EscNUL + backslash := esc[:1] + + plantLegacyRows(t, s, func(raw *sql.DB) { + // (1) raw NUL in a TEXT column. + mustExec(t, raw, `UPDATE items SET title = ? WHERE id = ?`, + "bad"+textguard.NUL+"title", dirtyText.ID) + // (2) live escape in a JSON column. + mustExec(t, raw, `UPDATE items SET fields = ? WHERE id = ?`, + `{"note":"x`+esc+`y"}`, dirtyJSON.ID) + // (3) raw NUL inside a JSON column — a defect of the stored bytes + // rather than of the document, and a different repair pass. + mustExec(t, raw, `UPDATE items SET tags = ? WHERE id = ?`, + `["a`+textguard.NUL+`b"]`, dirtyJSON.ID) + // (4) THE NEGATIVE CONTROL. A doubled backslash makes the six + // characters literal text: the SQL pre-filter matches it and the + // predicate must not. A scan that reports this row has a false + // positive of exactly the kind BUG-2803's parity filter had. + mustExec(t, raw, `UPDATE items SET fields = ? WHERE id = ?`, + `{"note":"x`+backslash+esc+`y"}`, literalOnly.ID) + // (5) THE OTHER NEGATIVE CONTROL. The escape in a TEXT column is six + // ordinary characters and is not a violation anywhere. + mustExec(t, raw, `UPDATE items SET content = ? WHERE id = ?`, + "writing about "+esc+" in a doc", literalOnly.ID) + // (6) A table with NO declared primary key, addressed by rowid. + mustExec(t, raw, + `INSERT INTO item_wiki_links (source_item_id, target_kind, target_title, position) + VALUES (?, 'title', ?, 0)`, + cleanItem.ID, "link"+textguard.NUL+"title") + }) + + report, err := s.ScanNUL() + if err != nil { + t.Fatalf("scan: %v", err) + } + if !report.Applicable { + t.Fatalf("scan reported not applicable on SQLite: %s", report.Reason) + } + + // Every protected column must exist in a freshly migrated database. A + // non-empty ColumnsAbsent here means the list names something the schema + // does not have, which the scan would silently skip. + if len(report.ColumnsAbsent) != 0 { + t.Errorf("protected columns missing from the live schema: %v", report.ColumnsAbsent) + } + if want := len(NULProtectedColumns()); report.ColumnsScanned != want { + t.Errorf("scanned %d columns, list has %d", report.ColumnsScanned, want) + } + + found := map[string]NULViolation{} + for _, v := range report.Violations { + found[v.Table+"."+v.Column] = v + } + + for _, want := range []struct { + key string + rawNUL bool + escapedNUL bool + workspace string + }{ + {"items.title", true, false, ws.ID}, + {"items.fields", false, true, ws.ID}, + {"items.tags", true, false, ws.ID}, + {"item_wiki_links.target_title", true, false, ""}, + } { + v, ok := found[want.key] + if !ok { + t.Errorf("%s: not reported", want.key) + continue + } + if v.RawNUL != want.rawNUL || v.EscapedNUL != want.escapedNUL { + t.Errorf("%s: kind mismatch — raw=%v escaped=%v, want raw=%v escaped=%v", + want.key, v.RawNUL, v.EscapedNUL, want.rawNUL, want.escapedNUL) + } + if v.WorkspaceID != want.workspace { + t.Errorf("%s: workspace %q, want %q", want.key, v.WorkspaceID, want.workspace) + } + } + + // items.content carried the escape as ordinary text and must not appear; + // nor may the doubled-backslash document, which lives in items.fields and + // would show up as a SECOND items.fields violation. + if _, reported := found["items.content"]; reported { + t.Error("items.content reported: the escape in a TEXT column is six ordinary characters") + } + fieldsCount := report.ByColumn()["items.fields"] + if fieldsCount != 1 { + t.Errorf("items.fields reported %d times, want exactly 1 — the doubled-backslash document "+ + "matches the SQL pre-filter and must be dropped by the predicate", fieldsCount) + } + + // The rowid-addressed row must carry a usable address. + if v, ok := found["item_wiki_links.target_title"]; ok { + if _, hasRowid := v.Key["rowid"]; !hasRowid { + t.Errorf("item_wiki_links declares no primary key; expected a rowid address, got %v", v.Key) + } + } + + if got := report.ByWorkspace()[ws.ID]; got != 3 { + t.Errorf("workspace %s has %d violations, want 3", ws.ID, got) + } +} + +// TestRepairNULRepairsThePopulationAndIsIdempotent is the other half: after the +// repair the database satisfies the invariant, and the values that were never +// violating are byte-identical. +func TestRepairNULRepairsThePopulationAndIsIdempotent(t *testing.T) { + s := testStore(t) + if s.dialect.Driver() != DriverSQLite { + t.Skip("SQLite only") + } + + ws := createTestWorkspace(t, s, "RepairWS") + col := createTestCollection(t, s, ws.ID, "Tasks") + dirty := createTestItem(t, s, ws.ID, col.ID, "clean", "body") + literalOnly := createTestItem(t, s, ws.ID, col.ID, "second", "body") + + esc := textguard.EscNUL + backslash := esc[:1] + literalDoc := `{"note":"x` + backslash + esc + `y"}` + + plantLegacyRows(t, s, func(raw *sql.DB) { + mustExec(t, raw, `UPDATE items SET title = ? WHERE id = ?`, "bad"+textguard.NUL+"title", dirty.ID) + mustExec(t, raw, `UPDATE items SET fields = ? WHERE id = ?`, `{"note":"x`+esc+`y"}`, dirty.ID) + mustExec(t, raw, `UPDATE items SET fields = ? WHERE id = ?`, literalDoc, literalOnly.ID) + }) + + report, err := s.RepairNUL() + if err != nil { + t.Fatalf("repair: %v", err) + } + if len(report.Failed) != 0 { + t.Fatalf("repair failures: %+v", report.Failed) + } + if len(report.Repaired) != 2 { + t.Errorf("repaired %d values, want 2 (%+v)", len(report.Repaired), report.Repaired) + } + + // The database now satisfies the invariant. + after, err := s.ScanNUL() + if err != nil { + t.Fatalf("re-scan: %v", err) + } + if after.Total() != 0 { + t.Errorf("scan after repair still reports %d violations: %v", after.Total(), after.Violations) + } + + // The repaired values carry U+FFFD where the NUL was, and nothing else + // changed. Asserting the CONTENT rather than only the count is what stops a + // repair that blanks the column from passing. + var title, fields string + if err := s.db.QueryRow(`SELECT title, fields FROM items WHERE id = ?`, dirty.ID). + Scan(&title, &fields); err != nil { + t.Fatalf("read repaired row: %v", err) + } + if want := "bad" + textguard.Replacement + "title"; title != want { + t.Errorf("title = %q, want %q", title, want) + } + if want := `{"note":"x` + textguard.ReplacementEscape + `y"}`; fields != want { + t.Errorf("fields = %q, want %q", fields, want) + } + + // The row that only ever held literal text is untouched, BYTE FOR BYTE. + var untouched string + if err := s.db.QueryRow(`SELECT fields FROM items WHERE id = ?`, literalOnly.ID).Scan(&untouched); err != nil { + t.Fatalf("read literal row: %v", err) + } + if untouched != literalDoc { + t.Errorf("a value nobody complained about was rewritten\n before: %q\n after: %q", literalDoc, untouched) + } + + // Running it again does nothing, which is what makes a partial run + // resumable rather than a state somebody has to reason about. + second, err := s.RepairNUL() + if err != nil { + t.Fatalf("second repair: %v", err) + } + if len(second.Repaired) != 0 || len(second.Failed) != 0 { + t.Errorf("second pass was not a no-op: repaired=%d failed=%+v", len(second.Repaired), second.Failed) + } +} + +// TestRepairNULRefusesToRewriteAPrimaryKey pins the one shape the repair +// deliberately leaves alone. +// +// email_optouts(email) is both the primary key and a protected column. A +// U+FFFD substitution there rewrites the row's identity and can land on top of +// an existing row — silently merging two opt-out records, which in this table +// means somebody starts receiving mail again. +func TestRepairNULRefusesToRewriteAPrimaryKey(t *testing.T) { + s := testStore(t) + if s.dialect.Driver() != DriverSQLite { + t.Skip("SQLite only") + } + + bad := "someone" + textguard.NUL + "@example.com" + plantLegacyRows(t, s, func(raw *sql.DB) { + mustExec(t, raw, `INSERT INTO email_optouts (email) VALUES (?)`, bad) + }) + + report, err := s.RepairNUL() + if err != nil { + t.Fatalf("repair: %v", err) + } + + var skipped *NULRepairSkip + for i := range report.Skipped { + if report.Skipped[i].Violation.Table == "email_optouts" { + skipped = &report.Skipped[i] + } + } + if skipped == nil { + t.Fatalf("email_optouts.email was not skipped; buckets: repaired=%+v skipped=%+v failed=%+v", + report.Repaired, report.Skipped, report.Failed) + } + if !strings.Contains(skipped.Reason, "primary key") { + t.Errorf("skip reason does not name the cause: %q", skipped.Reason) + } + + // And the row is genuinely untouched, rather than reported as skipped + // while having been written anyway. + var count int + if err := s.db.QueryRow( + `SELECT COUNT(*) FROM email_optouts WHERE instr(email, char(0)) > 0`, + ).Scan(&count); err != nil { + t.Fatalf("verify: %v", err) + } + if count != 1 { + t.Errorf("the skipped row was modified: %d rows still carry the NUL, want 1", count) + } +} + +// TestScanNULOnPostgresSaysWhyItDidNotLook guards the one thing a zero report +// must never be mistaken for. +func TestScanNULOnPostgresSaysWhyItDidNotLook(t *testing.T) { + s := testStore(t) + if s.dialect.Driver() == DriverSQLite { + t.Skip("this leg is about the Postgres arm") + } + report, err := s.ScanNUL() + if err != nil { + t.Fatalf("scan: %v", err) + } + if report.Applicable { + t.Error("the scan claims to have scanned a Postgres database") + } + if report.Reason == "" { + t.Error("a not-applicable report with no reason is indistinguishable from a clean one") + } + if report.Total() != 0 { + t.Errorf("not-applicable report carries %d violations", report.Total()) + } +} + +func mustExec(t *testing.T, db *sql.DB, query string, args ...any) { + t.Helper() + if _, err := db.Exec(query, args...); err != nil { + t.Fatalf("exec %s: %v", query, err) + } +} + +// TestScanCostFiguresMatchTheList keeps ScanNUL's doc comment honest. +// +// That comment tells an operator how much work the scan is — one unindexed +// scan per protected column — and quotes the number. A figure in prose that +// nothing checks is a figure that silently stops being true the next time a +// column joins the list, which in this cluster has happened in almost every +// round. If this fails, update the comment rather than the numbers here: the +// list is the source and the comment is the copy. +func TestScanCostFiguresMatchTheList(t *testing.T) { + const ( + wantTotal = 131 + wantJSON = 24 + ) + + cols := NULProtectedColumns() + gotJSON := 0 + for _, c := range cols { + if c.Class == classJSON { + gotJSON++ + } + } + + if len(cols) != wantTotal || gotJSON != wantJSON { + t.Errorf("ScanNUL's doc comment says %d protected columns (%d JSON, %d text); the list now has "+ + "%d (%d JSON, %d text). Update the comment.", + wantTotal, wantJSON, wantTotal-wantJSON, len(cols), gotJSON, len(cols)-gotJSON) + } +} + +// TestRepairNULAddressesARowidOnlyTable covers the one row shape whose ADDRESS +// is not an id. +// +// item_wiki_links declares no primary key, so the scan addresses it by `rowid` +// — an INTEGER column, scanned into a Go string and bound back as one. That +// round trip is the only place in this unit where the address could silently +// stop selecting the row, and an UPDATE matching nothing would otherwise commit +// and be reported as a repair. +// +// Driven separately from the main repair test because the main one plants only +// id-addressed rows, and a leg that never exercises the rowid path would let +// that binding break without a failure. +func TestRepairNULAddressesARowidOnlyTable(t *testing.T) { + s := testStore(t) + if s.dialect.Driver() != DriverSQLite { + t.Skip("SQLite only") + } + + ws := createTestWorkspace(t, s, "RowidWS") + col := createTestCollection(t, s, ws.ID, "Tasks") + item := createTestItem(t, s, ws.ID, col.ID, "linked", "body") + + plantLegacyRows(t, s, func(raw *sql.DB) { + // Two rows, so a repair that matched the WRONG one — or all of them — + // is distinguishable from a repair that matched its own. + mustExec(t, raw, + `INSERT INTO item_wiki_links (source_item_id, target_kind, target_title, position) + VALUES (?, 'title', ?, 0)`, item.ID, "bad"+textguard.NUL+"link") + mustExec(t, raw, + `INSERT INTO item_wiki_links (source_item_id, target_kind, target_title, position) + VALUES (?, 'title', ?, 1)`, item.ID, "innocent bystander") + }) + + report, err := s.RepairNUL() + if err != nil { + t.Fatalf("repair: %v", err) + } + if len(report.Failed) != 0 { + t.Fatalf("repair failures: %+v", report.Failed) + } + if len(report.Repaired) != 1 { + t.Fatalf("repaired %d values, want 1: %+v", len(report.Repaired), report.Repaired) + } + if _, addressed := report.Repaired[0].Key["rowid"]; !addressed { + t.Errorf("the row was addressed by %v, not by rowid", report.Repaired[0].Key) + } + + var repaired, bystander string + if err := s.db.QueryRow( + `SELECT target_title FROM item_wiki_links WHERE position = 0`).Scan(&repaired); err != nil { + t.Fatalf("read repaired: %v", err) + } + if err := s.db.QueryRow( + `SELECT target_title FROM item_wiki_links WHERE position = 1`).Scan(&bystander); err != nil { + t.Fatalf("read bystander: %v", err) + } + if want := "bad" + textguard.Replacement + "link"; repaired != want { + t.Errorf("repaired title = %q, want %q", repaired, want) + } + if bystander != "innocent bystander" { + t.Errorf("the neighbouring row was rewritten too: %q", bystander) + } +} diff --git a/internal/textguard/repair.go b/internal/textguard/repair.go new file mode 100644 index 00000000..d69989cb --- /dev/null +++ b/internal/textguard/repair.go @@ -0,0 +1,165 @@ +package textguard + +import "strings" + +// Repair is the ONE implementation of "make this value satisfy the invariant", +// and it lives beside the predicate for the same reason the predicate is +// shared: four layers that agree about what is REFUSED but disagree about what +// a repair PRODUCES is this bug family arriving one step later. +// +// DOC-2823 S3, on Dave's day-54 ruling: the replacement character is U+FFFD. +// Visible, greppable, and — because the JSON arm emits it as an escape — the +// same six characters wide as the escape it replaces. +// +// isJSON is the caller's classification, exactly as in ParameterRefused, and +// the two must be given the SAME value for the same column. The scan derives it +// from the column list in internal/store/nulcolumns.go, which is Layer B's +// classing. +// +// THE PROPERTY THIS MUST HAVE, in both directions, is what corpus_test.go pins: +// +// - For every value the layers REFUSE, Repair produces one they ACCEPT. +// - For every value the layers ACCEPT, Repair is the IDENTITY. +// +// The second half is not politeness. A repair that "tidies" values nobody +// complained about is a repair that rewrites `{"a":"x\\u0000y"}` — six literal +// characters after a doubled backslash, an accepted value — and corrupts it. +// That case is in the corpus precisely because every cheap approach fails it. +func Repair(value string, isJSON bool) string { + // Raw NULs first, as TEXT. This is byte-preserving everywhere else and is + // the whole repair for a text-classed column; it is also the right repair + // for a raw NUL sitting inside a JSON-classed blob, which is a defect of + // the stored bytes rather than of the document. + out := strings.ReplaceAll(value, NUL, Replacement) + + // The escape form only matters where something parses the value as a JSON + // document. In a text column the six characters are six characters, and + // rewriting them there is the false positive the corpus's third case + // exists to catch. + if isJSON && DocumentDecodesNULAnyShape(out) { + out, _ = RepairJSONDocument(out) + } + return out +} + +// RepairJSONDocument rewrites every NUL escape a JSON parser would DECODE in +// an already-valid JSON document, and reports how many it replaced. +// +// Exported for the workspace import's --repair-nul path, which needs the count +// to tell the operator what it changed and cannot use Repair: Repair also +// rewrites RAW NUL bytes, and a raw NUL inside a JSON string makes the document +// invalid, so repairing one there would turn a body the decoder rejects into +// one it accepts. Widening what parses is not this flag's job. +func RepairJSONDocument(s string) (string, int) { + return repairJSONNULEscapes(s) +} + +const ( + // Replacement is U+FFFD as text, for a raw NUL in a stored value. + Replacement = "�" + + // ReplacementEscape is U+FFFD spelled as a JSON escape, for a live NUL + // escape inside a JSON document. Six characters replacing six, so the + // document's byte length is unchanged and nothing around it shifts. + ReplacementEscape = "\\u" + "fffd" + + // nulEscapeLen is the length of a \uXXXX escape. + nulEscapeLen = 6 +) + +// repairJSONNULEscapes rewrites every NUL escape that a JSON parser would +// DECODE, and nothing else. +// +// WHY THIS IS A SCANNER AND NOT decode-walk-remarshal, which was the shape the +// S3 recon write-up proposed before this was written. Re-marshalling a document +// changes four things nobody asked to change: object key order (Go sorts map +// keys), insignificant whitespace, integers wider than float64 (unless the +// decoder is told to use json.Number), and HTML-ish characters (unless the +// encoder is told not to escape them). Worse, a document with LITERAL duplicate +// keys silently loses one on the way through a map — which is one of the two +// gaps BUG-2812 owns, and a repair is the last place that should quietly drop +// user data. +// +// Scanning the raw text has none of those failure modes: every byte the repair +// does not deliberately rewrite is copied verbatim, so a document with no live +// escape comes out byte-identical without that having to be argued. +// +// WHY IT IS NOT A SUBSTRING REPLACE, which is the version that looks equivalent +// and is not. `{"a":"x\\u0000y"}` contains the six characters and decodes to no +// NUL at all, because the doubled backslash makes them literal. Only a scanner +// that consumes escapes IN ORDER can tell the two apart — which is the same +// layer-relativity that made BUG-2803's parity pre-filter unsound, met again in +// the write direction. +// +// The input is a value DocumentDecodesNULAnyShape has already accepted as +// valid JSON, so the scanner has no malformed-input arm: an unterminated string +// or a stray backslash cannot occur. It still copies anything it does not +// recognise rather than assuming, so a future caller passing something else +// gets its bytes back unchanged instead of a mangled document. +func repairJSONNULEscapes(s string) (string, int) { + var b strings.Builder + b.Grow(len(s)) + + replaced := 0 + inString := false + for i := 0; i < len(s); { + c := s[i] + + if !inString { + if c == '"' { + inString = true + } + b.WriteByte(c) + i++ + continue + } + + switch c { + case '"': + inString = false + b.WriteByte(c) + i++ + case '\\': + // An escape. Consume it WHOLE, so its second character can never + // be read as the start of another one — that is the entire + // difference between this and a substring replace. + if i+1 >= len(s) { + b.WriteByte(c) + i++ + continue + } + if s[i+1] == 'u' && i+nulEscapeLen <= len(s) && isNULEscapeHex(s[i+2:i+nulEscapeLen]) { + b.WriteString(ReplacementEscape) + i += nulEscapeLen + replaced++ + continue + } + // Any other escape, including `\\`: copy both bytes and move on. + b.WriteString(s[i : i+2]) + i += 2 + default: + b.WriteByte(c) + i++ + } + } + return b.String(), replaced +} + +// isNULEscapeHex reports whether the four hex digits of a \uXXXX escape name +// U+0000. +// +// No case folding, and that is not an oversight: the only spelling of this +// code point is four ASCII zeros, which have no case. Every OTHER code point +// would need it, so a caller widening this function to a general hex compare +// must add it then. +func isNULEscapeHex(hex string) bool { + if len(hex) != 4 { + return false + } + for i := 0; i < 4; i++ { + if hex[i] != '0' { + return false + } + } + return true +} diff --git a/internal/textguard/repair_test.go b/internal/textguard/repair_test.go new file mode 100644 index 00000000..fe9c9aa1 --- /dev/null +++ b/internal/textguard/repair_test.go @@ -0,0 +1,242 @@ +package textguard + +import ( + "encoding/json" + "strings" + "testing" +) + +// The repair's contract is a property over the SAME corpus the four +// enforcement layers are measured against (DOC-2823 S3). Driving the repair +// through the corpus rather than through examples of its own is what stops it +// from being correct about a set of values nobody enforces. + +// TestRepairSatisfiesTheCorpusBothWays is the whole contract in one test. +// +// Both directions matter and they fail differently. A repair that is not +// ACCEPTED afterwards has not repaired anything; a repair that is not the +// IDENTITY on accepted values has corrupted something nobody complained about, +// which is the harder failure to notice because every layer stays green. +func TestRepairSatisfiesTheCorpusBothWays(t *testing.T) { + for _, c := range Corpus { + t.Run(c.Name, func(t *testing.T) { + got := Repair(c.Value, c.IsJSON) + + if ParameterRefused(got, c.IsJSON) { + t.Errorf("repaired value is still refused\n in: %q\n out: %q", c.Value, got) + } + + if c.Refused { + // A repair that returned its input unchanged would satisfy the + // check above only by accident on the accepted cases, and + // would silently do nothing here. Asserting the change is what + // makes this leg fail for a no-op implementation. + if got == c.Value { + t.Errorf("refused value came back unchanged: %q", c.Value) + } + } else if got != c.Value { + t.Errorf("accepted value was rewritten\n in: %q\n out: %q", c.Value, got) + } + }) + } +} + +// TestRepairAcceptsTheStoreOverRefusals covers the values only Layer A refuses. +// +// They are outside Corpus because the layers disagree about them, but the +// repair still has to produce something the store will accept — otherwise +// `pad db repair-nul` leaves behind exactly the rows the store's own classing +// would refuse to rewrite. +func TestRepairAcceptsTheStoreOverRefusals(t *testing.T) { + for _, c := range StoreOverRefusals { + t.Run(c.Name, func(t *testing.T) { + // The store classes by COLUMN; these are text columns whose value + // happens to parse as a document, so the store checks them as JSON. + got := Repair(c.Value, true) + if ParameterRefused(got, true) { + t.Errorf("still refused by the store's classing\n in: %q\n out: %q", c.Value, got) + } + }) + } +} + +// TestRepairIsIdempotent pins that a second pass changes nothing. +// +// The scan and the repair are separate commands, so an operator re-running the +// repair after a partial run is expected, not exceptional; and a repair whose +// output it would itself flag is a repair that never terminates. +func TestRepairIsIdempotent(t *testing.T) { + for _, c := range Corpus { + once := Repair(c.Value, c.IsJSON) + twice := Repair(once, c.IsJSON) + if once != twice { + t.Errorf("%s: not idempotent\n once: %q\n twice: %q", c.Name, once, twice) + } + } +} + +// TestRepairPreservesTheDocumentModuloNUL is the faithfulness half. +// +// "Accepted afterwards" is satisfied by a repair that returns `{}` for every +// document. This asserts the repaired document decodes to the SAME structure as +// the original, with every NUL — in a value or in a key — replaced by U+FFFD +// and nothing else touched. +func TestRepairPreservesTheDocumentModuloNUL(t *testing.T) { + for _, c := range Corpus { + if !c.IsJSON || !json.Valid([]byte(strings.TrimSpace(c.Value))) { + continue + } + t.Run(c.Name, func(t *testing.T) { + var before, after any + if err := json.Unmarshal([]byte(c.Value), &before); err != nil { + t.Fatalf("decode original: %v", err) + } + repaired := Repair(c.Value, true) + if err := json.Unmarshal([]byte(repaired), &after); err != nil { + t.Fatalf("repaired value is not valid JSON: %v (%q)", err, repaired) + } + if !equalModuloNUL(before, after) { + t.Errorf("repair changed more than the NULs\n before: %#v\n after: %#v", before, after) + } + }) + } +} + +// equalModuloNUL compares two decoded JSON values, treating a NUL on the left +// as equal to U+FFFD on the right and requiring everything else to match. +func equalModuloNUL(before, after any) bool { + switch b := before.(type) { + case string: + a, ok := after.(string) + return ok && strings.ReplaceAll(b, NUL, Replacement) == a + case map[string]any: + a, ok := after.(map[string]any) + if !ok || len(a) != len(b) { + return false + } + for k, bv := range b { + av, present := a[strings.ReplaceAll(k, NUL, Replacement)] + if !present || !equalModuloNUL(bv, av) { + return false + } + } + return true + case []any: + a, ok := after.([]any) + if !ok || len(a) != len(b) { + return false + } + for i := range b { + if !equalModuloNUL(b[i], a[i]) { + return false + } + } + return true + default: + return before == after + } +} + +// TestRepairScannerEdges covers the shapes the corpus does not carry, because +// the corpus is about what the LAYERS disagree on and these are about what the +// SCANNER can misread. +func TestRepairScannerEdges(t *testing.T) { + // Built, never typed — see corpus.go. A case that means the escape while + // carrying the character is vacuous. + esc := EscNUL + backslash := esc[:1] + + cases := []struct { + name string + in string + isJSON bool + want string + why string + }{ + { + name: "escape in a key AND a value", isJSON: true, + in: `{"k` + esc + `":"v` + esc + `"}`, + want: `{"k` + ReplacementEscape + `":"v` + ReplacementEscape + `"}`, + why: "keys and values take the same path; a value-only rewrite leaves the document refused.", + }, + { + name: "doubled backslash then the escape text", isJSON: true, + in: `{"a":"x` + backslash + esc + `y"}`, + want: `{"a":"x` + backslash + esc + `y"}`, + why: "literal text after an escaped backslash. This is the case a substring replace corrupts.", + }, + { + name: "escape immediately after another escape", isJSON: true, + in: `{"a":"` + backslash + `n` + esc + `"}`, + want: `{"a":"` + backslash + `n` + ReplacementEscape + `"}`, + why: "the scanner must resume at the right offset after consuming a two-byte escape.", + }, + { + // THE DISCRIMINATING CASE, and the one the first draft of this test + // did not have. A doubled-backslash literal ALONE never reaches the + // scanner: Repair's guard skips a document that decodes to no NUL, + // so a naive substring replace survived every leg here. It is only + // when the same document ALSO carries a live escape that the + // scanner runs over the literal — and that is the row an operator + // actually has, since the literal is what a document about this bug + // contains. Found by mutating the scanner to strings.ReplaceAll and + // watching the suite stay green. + name: "a live escape and a doubled-backslash literal in ONE document", isJSON: true, + in: `{"a":"x` + esc + `y","b":"lit` + backslash + esc + `eral"}`, + want: `{"a":"x` + ReplacementEscape + `y","b":"lit` + backslash + esc + `eral"}`, + why: "the live escape is rewritten and the literal is not. A substring replace corrupts the second.", + }, + { + name: "the escape text OUTSIDE any string", isJSON: true, + in: `{"a":1}`, + want: `{"a":1}`, + why: "control: structural bytes are copied verbatim and no rewrite fires.", + }, + { + name: "raw NUL and a live escape in one JSON value", isJSON: true, + in: `{"a":"x` + NUL + `y` + esc + `z"}`, + want: `{"a":"x` + Replacement + `y` + ReplacementEscape + `z"}`, + why: "both defects in one value: the raw pass and the document pass must BOTH fire.", + }, + { + name: "escape inside a string that is not classed as JSON", isJSON: false, + in: `{"a":"x` + esc + `y"}`, + want: `{"a":"x` + esc + `y"}`, + why: "classing decides. In a text column these are six characters and rewriting them is the false positive.", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := Repair(tc.in, tc.isJSON) + if got != tc.want { + t.Errorf("%s\n in: %q\n got: %q\n want: %q", tc.why, tc.in, got, tc.want) + } + }) + } +} + +// TestRepairConstantsDidNotDecay is the same guard corpus_test.go keeps over +// NUL and EscNUL, extended to the two this file adds. +// +// It compares against values it BUILDS rather than ones it types, because +// typing the six characters is what produces the character (measured: it +// happened twice while writing this unit). +func TestRepairConstantsDidNotDecay(t *testing.T) { + if []rune(Replacement)[0] != 0xFFFD || len([]rune(Replacement)) != 1 { + t.Errorf("Replacement is not a single U+FFFD: %q", Replacement) + } + if len(ReplacementEscape) != 6 || + ReplacementEscape[0] != '\\' || ReplacementEscape[1] != 'u' || + ReplacementEscape[2:] != "fffd" { + t.Errorf("ReplacementEscape decayed: %q", ReplacementEscape) + } + // And the two must mean the same thing to a JSON parser. + var viaEscape string + if err := json.Unmarshal([]byte(`"`+ReplacementEscape+`"`), &viaEscape); err != nil { + t.Fatalf("ReplacementEscape is not a valid JSON escape: %v", err) + } + if viaEscape != Replacement { + t.Errorf("the escape and the character disagree: %q vs %q", viaEscape, Replacement) + } +}