fix(server): refuse a decoded NUL in a JSON request body (BUG-2803)

docs/backup.md

@@ -160,6 +160,43 @@ pad workspace import < my-workspace.json
 pad workspace import --name "imported-workspace" < my-workspace.json
 ```
 
+### One case where an export is not importable
+
+A workspace whose stored data contains a **NUL character** exports fine and is
+refused on import, with a 400 naming the cause. This is not a corruption of
+your backup — it is the import applying a rule the write path now applies too
+(BUG-2803): Pad does not accept a NUL in a text or JSON value. That is an
+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.
+
+Only SQLite is affected. 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.
+
+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.
+
+If you hit either, the affected value has to be repaired at the source before
+the export or migration will go through. A preflight check and a repair path
+are tracked as BUG-2810; until then the failing row is named in the error.
+
+
 This format is database-agnostic and can be used to:
 - Transfer workspaces between Pad instances
 - Create workspace templates

internal/server/artifact_import.go

@@ -1,6 +1,7 @@
 package server
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -49,19 +50,41 @@ var ErrArtifactTooLarge = errors.New("artifact import: body exceeds size limit")
 // the YAML-bomb guard limits (node count, nesting depth, or anchors/aliases).
 var ErrArtifactUnsafeYAML = errors.New("artifact import: frontmatter rejected by safety limits")
 
+// ErrArtifactUnbindableText is returned when the artifact body is not text the
+// database can be asked to store — invalid UTF-8, or carrying a NUL. It is a
+// client error (400), not a 500, for the reason BUG-2782 gives: this is a
+// value Pad refuses to store, so the caller sent something that cannot mean
+// anything here.
+//
+// "Refuses", not "cannot" — the distinction matters and the earlier wording
+// blurred it (codex round 20). PostgreSQL rejects a NUL in a text or JSON
+// column outright; SQLite would accept one in TEXT. So this is an application
+// rule Pad applies on both dialects, not a storage limit it inherits from
+// either. Stating it as a capability would tell the next reader that SQLite
+// enforces something it does not.
+var ErrArtifactUnbindableText = errors.New("artifact import: body contains invalid UTF-8 or a NUL byte")
+
 // parseArtifactRequest is the guarded HTTP-boundary parse used by the import
-// handler. It applies three checks IN ORDER:
+// handler. It applies five steps IN ORDER:
 //
 //  1. Byte cap on the raw body (http.MaxBytesReader), so an oversized body is
 //     rejected before full materialization.
-//  2. YAML-bomb guard: the frontmatter region is parsed into a yaml.Node tree
+//  2. Raw text validity (bindableText), so invalid UTF-8 or a NUL BYTE is
+//     refused before anything parses it.
+//  3. YAML-bomb guard: the frontmatter region is parsed into a yaml.Node tree
 //     and walked, enforcing maxFrontmatterNodes / maxFrontmatterDepth /
 //     maxFrontmatterAliases. This runs BEFORE the struct decode so an alias-
 //     storm or deep-nesting document never reaches the expanding unmarshaler.
-//  3. artifact.Decode, which produces the typed Artifact.
+//  4. artifact.Decode, which produces the typed Artifact.
+//  5. Decoded text validity, because YAML manufactures a NUL from \0 that
+//     step 2 cannot see in the request bytes.
+//
+// Steps 2 and 5 arrived with BUG-2803; this list said "three checks" until
+// codex round 8 pointed out it was describing the version before them.
 //
 // Returns the decoded Artifact or a typed error: ErrArtifactTooLarge,
-// ErrArtifactUnsafeYAML, or an artifact.* sentinel (ErrMalformed /
+// ErrArtifactUnbindableText, ErrArtifactUnsafeYAML, or an artifact.*
+// sentinel (ErrMalformed /
 // ErrUnknownKind / ErrUnsupportedVersion) wrapped for context. The import
 // handler maps these to HTTP statuses.
 func parseArtifactRequest(w http.ResponseWriter, r *http.Request, maxBytes int64) (artifact.Artifact, error) {
@@ -83,16 +106,39 @@ func parseArtifactRequest(w http.ResponseWriter, r *http.Request, maxBytes int64
 		return artifact.Artifact{}, fmt.Errorf("artifact import: read body: %w", err)
 	}
 
-	// (2) YAML-bomb guard on the frontmatter region only.
+	// (2) The artifact body is TEXT bound for text columns, and this
+	// handler reads it directly rather than through decodeJSON, so it
+	// inherits neither BUG-2803's refusal nor the path/query rule (a body is
+	// neither). A raw NUL or invalid UTF-8 here reaches the store and
+	// Postgres answers 22021, which the handler turns into a 500 for what is
+	// a client error. Same predicate as ValidatePath and ValidateQuery.
+	// Found by the codex round 3 sweep over body readers (BUG-2803).
+	if !bindableText(string(data)) {
+		return artifact.Artifact{}, ErrArtifactUnbindableText
+	}
+
+	// (3) YAML-bomb guard on the frontmatter region only.
 	if err := guardArtifactFrontmatter(data); err != nil {
 		return artifact.Artifact{}, err
 	}
 
-	// (3) Typed decode.
+	// (4) Typed decode.
 	art, err := artifact.Decode(data)
 	if err != nil {
 		return artifact.Artifact{}, err
 	}
+
+	// (5) The DECODED artifact must be bindable text too — the raw check in
+	// (2) is not sufficient on its own. YAML has its own escape vocabulary:
+	// a double-quoted scalar `title: "a\0b"` carries no NUL in the request
+	// bytes, passes (2a), and manufactures one during the YAML decode.
+	// Measured before this check: such an artifact imported 201 with a NUL in
+	// the item title (codex round 4, BUG-2803). Same shape as the JSON half —
+	// a value that only becomes dangerous after a SECOND parse — so it gets
+	// the same answer, at the layer that can see it.
+	if !artifactIsBindableText(art) {
+		return artifact.Artifact{}, ErrArtifactUnbindableText
+	}
 	return art, nil
 }
 
@@ -194,3 +240,57 @@ func extractFrontmatterRegion(s string) (string, bool) {
 		offset += nl + 1
 	}
 }
+
+// artifactIsBindableText reports whether a decoded artifact carries a NUL in
+// any string it would take into the store.
+//
+// It works by MARSHALLING the artifact and searching the result, rather than
+// walking its fields by type. The hand-written walk this replaces missed two
+// things a reviewer found immediately (codex round 8): Provenance, whose
+// strings are rendered into a Markdown footer appended to the stored content,
+// and Arguments, whose declared type is []map[string]any — a concrete slice
+// type the walk's `[]any` case never matched. Both were reachable. A type
+// switch over a struct that grows is a list that goes stale in silence, which
+// is the same objection this file's jsonEncodedFieldKeys has to answer for
+// and can only answer with a derivation test. Marshalling has no such gap:
+// every exported field is covered, including ones added later.
+//
+// encoding/json escapes a NUL as the six-character sequence, so a decoded NUL
+// anywhere in the artifact appears in the output. The search is for that
+// sequence in bytes the MARSHALLER produced, not in caller-supplied text, so
+// the ambiguity bodyDecodesNUL has to resolve — a doubled backslash meaning
+// literal text — cannot arise here: a literal backslash in a value marshals
+// to a doubled one.
+//
+// SCOPE, stated because marshalling hides one thing: invalid UTF-8 in a Go
+// string marshals to U+FFFD rather than surviving, so this cannot detect it.
+// It does not need to. Step (2a) rejects invalid UTF-8 in the request bytes
+// before the decode, and YAML cannot manufacture it from valid input — its
+// escapes name code points (\xff is U+00FF, a valid rune), where \0 names a
+// NUL. NUL is the class that survives the decode, and it is the class this
+// checks.
+func artifactIsBindableText(art artifact.Artifact) bool {
+	encoded, err := json.Marshal(art)
+	if err != nil {
+		// An artifact that cannot be marshalled cannot be reasoned about;
+		// refuse rather than pass it on unexamined.
+		return false
+	}
+	// Searching the marshalled BYTES for the escape is wrong, and this
+	// function did it until codex round 9: a value holding the six LITERAL
+	// characters marshals to a doubled backslash, which still contains the
+	// six-character sequence as a substring, so valid content was refused.
+	// That is the same doubled-backslash trap bodyDecodesNUL exists to
+	// resolve — and an earlier version of this comment asserted it "cannot
+	// arise here", which was simply wrong.
+	//
+	// So the marshalled form is decoded again and walked with the same
+	// machinery, as caller data (no wire-key list applies to an artifact).
+	// The round trip is what makes every field reachable without a type
+	// switch; the walk is what makes the answer exact.
+	var v any
+	if err := json.Unmarshal(encoded, &v); err != nil {
+		return false
+	}
+	return !valueDecodesNUL(v, true)
+}