mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 23:15:40 +00:00
main
11 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Matt Faltyn
|
43d94a35d5 | fix(cli): pass PostgreSQL connection strings to backup clients (#1289) | ||
|
Matt Faltyn
|
a3a1d5862b | fix(store): preserve valid item references across workspace import (#1271) | ||
|
xarmian
|
0363c139a9 |
fix(store,cli): the oracle failed open, and it over-refuses one column (BUG-2810)
Three findings from codex round 5, all real; the third corrected a claim I had
made about the design.
**The suspect path could leave data unrepaired and exit 0.** The CLI printed
SuspectsFailed and then returned nil, checking only the violation bucket. A
script sees success; an operator who trusts the status moves on. Both buckets
now decide the exit code, and the decision is extracted into
nulRepairExitError so it is testable without a database — the bug was in the
decision, not in the repair, and a test that needs a fixture to reach it is a
test nobody writes.
**The destination oracle failed open.** Connection failures, timeouts and
read-back errors were bucketed with "the destination answered, about something
else" — reported and not refused on. So an UNVERIFIED suspect passed the
preflight, which is the defect the suspect class was added to correct arriving
by a different route.
There are now three outcomes rather than two: the server answered with a NUL
code (refuse), the server answered with another complaint about the value
(report, because a NUL preflight that quietly grew into a general one would
block migrations unrelated to this bug), and the server never answered
(REFUSE). ErrDestinationCheckUnavailable carries the third, and
TestDestinationOracleFailsClosedOnAnUnusableConnection pins it against a real
closed pool — with an open-pool control first, since a classifier that answered
"unavailable" for everything would satisfy the assertion and refuse every
migration.
**The oracle is not a perfect model of the migration, and I said it was.**
Codex claimed workspaces.settings is normalised on import, so the cast
over-refuses there. Measured rather than argued, by importing the same
shadowed-duplicate value into three columns against a real server:
workspaces.settings -> import SUCCEEDS, stored as {"a": "clean"}
items.fields -> import FAILS, SQLSTATE 22P05
collections.schema -> import FAILS, SQLSTATE 22P05
CreateWorkspace runs models.NormalizeWorkspaceSettings, a map round-trip that
drops the shadowed member. So the claim was right, and my own runtime demo
earlier on this branch — which used workspaces.settings — was showing a
spurious refusal.
The cast STAYS. That row is a value Layer B refuses on every write today and
exists only because it predates enforcement, so surviving the migration is an
accident of one column's normaliser rather than a property worth preserving,
and repair-nul clears it in one command. Deriving "would this column's writer
normalise it" is a per-column enumeration, which is the shape this cluster
keeps proving unmaintainable.
What changed is the CLAIM. The file header no longer says the oracle is "exact
in both directions" — it is exact about the VALUE and is not a model of the
MIGRATION; the refusal no longer tells an operator PostgreSQL would reject the
row, only that the value carries a NUL jsonb refuses; and the measurement and
the over-refusal are written into CheckJSONBAcceptable's doc comment and
docs/backup.md, which also now states that the check errs toward refusing.
The disposition is flagged to the lead rather than settled here: skipping
normalised columns is a scope call, not mine.
|
||
|
xarmian
|
57b7ca5f48 |
feat(store,cli): ask the destination about suspects instead of dropping them (BUG-2810)
Day-54 lead ruling on PR #1233, and the ruling names the defect precisely: the scan's own SQL pre-filter already surfaces the shadowed-duplicate row as a candidate, and `ParameterRefused` then drops it. So the preflight was discarding information it was holding and going on to promise the migration would go through. I had recorded that as an accepted residual on the grounds that closing it would violate DOC-2823's one-layer rule — but that rule is about what the enforcement layers REFUSE. It says nothing about a preflight throwing away a candidate it had in hand. **The SUSPECT class.** A pre-filter hit the predicate does not refuse. Most are doubled-backslash literals — text that writes ABOUT the escape, which is the false positive this whole predicate family exists to avoid. One member is not: a NUL in a value shadowed by a LITERAL duplicate key, which a map-model decode drops and PostgreSQL refuses. Nothing here can tell them apart, so nothing here tries: `pad db scan-nul` lists them under their own heading, apart from the violations, with what resolves them. **The destination is the oracle.** `pad db migrate-to-pg` casts each suspect on the TARGET connection — `SELECT $1::jsonb`, side-effect-free, and the very cast an INSERT performs — and refuses on 22P05 / 22021. That is exact in both directions precisely because it is not a fourth opinion of ours. Measured against a real server: the literal is ACCEPTED, the shadowed duplicate is REFUSED with 22P05, and a non-JSON value fails for a reason that is reported rather than refused on, because a NUL preflight that quietly grew into a general one would block migrations unrelated to this bug. **The repair had to be measured, not assumed, and the answer changed the design.** `textguard.Repair` leaves the shadowed value completely untouched: its scanner is gated on DocumentDecodesNULAnyShape, a map-model question that answers false for exactly this shape, so it never runs. A preflight that refused the row and printed `pad db repair-nul` would have been printing a command that does nothing to it — a remedy nobody ran (PATTE-135). So the repair reaches the class through the token-level scanner, exported for this, which rewrites the shadowed escape and still leaves the literal byte-identical because it consumes escapes in order. Suspects get their own buckets in the repair report rather than being folded into Repaired, so the dry run's promise and the run's result stay the same number. **Nothing about what any layer REFUSES changed.** textguard.KnownGaps and its pin are untouched, and TestScanNULInheritsTheRecordedKnownGaps still asserts the scan does NOT detect the shape. TestSuspectsCollapseWhenBUG2812Lands fails when the token-walk makes that false, and names every file to delete — the suspect path is a second mechanism that exists only while the predicate is blind. **One defect this found that no test did.** Running the real command against a real Postgres, the refusal announced "0 stored value(s) carry a NUL; nothing was migrated" while listing one — the count used the violations only, and the tests asserted the message CONTAINED "nothing was migrated" without reading the number. Fixed, and the assertion now reads the count. The whole loop is now verified end to end: preflight refuses, `repair-nul` fixes, the migration completes. My own prose from earlier on this branch is corrected with it. ScanNUL's doc comment, the preflight's, and docs/backup.md all said this shape passes the preflight and fails mid-copy, which the same commit makes false. |
||
|
xarmian
|
178b6b5010 |
fix(server,store): two more from codex rounds 3 and 4 (BUG-2810)
**The import repair could silently change what gets imported.** It decodes into map[string]any, where a repeated object member keeps only the LAST value. The TYPED decode that runs next does not agree: encoding/json unmarshals members in order into the same struct field, so two `"workspace"` objects MERGE there and collapse here. A body with duplicate members would therefore import differently with --repair-nul than without, which is outside what a flag by that name may do. It now DECLINES such a body: returns it untouched, lets the gate judge it exactly as it would without the flag, and says why in the refusal — "the payload repeats the member X, and repairing it would change which value is imported". Detection is a token walk, because a decode is what loses the information: by the time there is a map the duplicate is gone. The detector's own test carries the false positive that matters — the same member name in SIBLING objects is not a duplicate, and a single shared set of names would decline every real export, since items all carry `id`, `title`, `slug`. Rewriting such a body faithfully wants a token-preserving pass, which is BUG-2812's token-walk and not a rider on this. A real export cannot contain duplicate members (json.Marshal does not emit them), so declining costs nothing an operator meets by accident. The tally now owns the repair — decodeJSONRepairingNUL takes it and calls Apply — so the count and the declined reason come back through one object instead of a return value a caller has to remember to record. That is the same mistake this branch already made once, when the JSON path dropped the count and the header reported 0 for an import that had rewritten a value. **A row the repair could not address was reported as a failure.** A NUL in a key column the list does not protect, on a row whose violation is elsewhere, makes the address unbindable: Layer A inspects every bound parameter, including a WHERE clause's, so the lookup is refused before SQLite is asked to find the row. It landed in Failed carrying "invalid text parameter: parameter 2" — the same information phrased as a fault in the repair rather than a property of the row. Now detected up front and reported as a skip with the reason, alongside the two skips that already existed. **One finding NOT fixed, deliberately, and recorded instead.** Round 3 raised that the scan misses a NUL in a value shadowed by a LITERAL duplicate key, so such a row passes the migrate-to-pg preflight and then fails during the copy — the exact failure the preflight replaces, surviving for one shape. That is textguard.KnownGaps: a blind spot every layer shares on purpose, which DOC-2823 forbids closing in one layer alone, because layers disagreeing about one value is the defect this cluster is made of. So it is named in ScanNUL's doc comment, in the preflight's, and in docs/backup.md for the operator, and TestScanNULInheritsTheRecordedKnownGaps pins the miss and FAILS when it stops being one — the notification that BUG-2812 has landed and those three prose sites need updating. The consequence is recorded on BUG-2812's trail. Round 2's single finding was refuted rather than fixed: it predicted TestRepairFlagReachesTheNestedAndObliqueForms would fail, on a mechanism that describes the raw-byte scanner this branch had already replaced. The test passes; the outer decode resolves the oblique spelling before the walk sees it. |
||
|
xarmian
|
49bd342e4c |
fix(store,server,cli): three defects from codex round 1 (BUG-2810)
**The import flag could not repair the column it exists for.** `--repair-nul` scanned the RAW body for a live escape, which is right for a value the gate reads at the top level and wrong for the one that actually matters. An item's `fields` blob travels through an export as a STRING: a NUL escape in the stored blob marshals into the body with a DOUBLED backslash, which a raw scan must leave alone because at that layer it is literal text — while the gate refuses it anyway, since it decodes the body and re-parses that string as the document it is. So the repair now walks the DECODED body with the same classing bodyDecodesNUL uses, one verb changed: where the gate asks textguard whether a value decodes to a NUL, this asks textguard to repair it. Two walks of one shape in one package is a real risk, and the mitigation is that they are measured against the same corpus in both directions rather than reviewed for similarity — TestBodyRepairMirrorsTheGateOverTheCorpus drives every case through the body shape and asserts refused-becomes-accepted and accepted-stays-byte-identical. Two consequences worth stating. The walk also reaches the OBLIQUE spelling — the backslash written as its own escape, so the six characters never appear in the raw bytes at all — which the scanner could not, so the test that pinned that limit is replaced by one asserting the capability. And re-encoding is now possible, so it is bounded: UseNumber, so an integer wider than float64 is not silently re-emitted in scientific notation; SetEscapeHTML(false); and a body with nothing to repair is returned byte-identical rather than round-tripped. The mutation that removes UseNumber turns 9007199254740993 into ...992, and a test says so. The header is now X-Pad-Repaired-NUL-Values, because at the decoded layer an escape is not a thing that exists any more and one nested document may have carried several. **The scan could not run on the databases it exists for.** Several protected tables carry a NULLABLE workspace_id — activities, api_tokens, mcp_audit_log — and the scan selected it into a plain *string, which fails with "converting NULL to string is unsupported" and takes the scan, the repair and the migrate-to-pg preflight down with it. Every column is now scanned as sql.NullString: SQLite also permits NULL in a declared PRIMARY KEY that is neither INTEGER PRIMARY KEY nor NOT NULL, which no other engine does, and a NULL key cannot address a row for an UPDATE — such rows are reported and skipped with the reason rather than handed a WHERE that matches nothing. Verified against the unfixed code: the scan returned `scan activities.actor row: sql: Scan error ... converting NULL to string`. It needed a VIOLATING row in such a table, which is why every fixture that planted its rows in `items` missed it. **--force by accident.** The repair skipped the running-server check whenever --from was given — and the most natural --from an operator types is the path `pad db scan-nul` just printed, which IS the live database. The check is now on the resolved path (Abs + EvalSymlinks, so a symlinked data directory or a relative path still matches), and a --from naming an unrelated backup stays unguarded, which is correct: nothing is writing it. The ordering moved with it. `store.New` runs pending migrations, so the refusal now happens BEFORE the database is opened; opening first and refusing second made the guard arrive after the thing it guards against. |
||
|
xarmian
|
63da2f4f5f |
feat(store,server,cli): count and repair the legacy NUL population (BUG-2810)
Layers A and B stop the value being written. Neither makes a row that
already carries one go away, and BUG-2810's filing is what that costs: an
affected workspace exports with a 200 and re-imports with a 400, so a
self-hoster restoring their own backup is blocked with no path forward in
the product, and `pad db migrate-to-pg` fails partway through the copy
against PostgreSQL's jsonb parser rather than up front.
This is DOC-2823's S3, on Dave's day-54 rulings: U+FFFD as the replacement,
repair standalone only with a migrate-to-pg preflight that refuses and
prints the command, `--repair-nul` on import shipping default-strict.
ONE REPAIR, beside the one predicate. textguard.Repair lives next to
ParameterRefused because four layers that agree about what is REFUSED and
disagree about what a repair PRODUCES is this bug family arriving one step
later. Its contract is a property over the same corpus, in both directions:
every refused value becomes one all four layers accept, and every accepted
value comes back IDENTICAL. The second half is the load-bearing one — a
repair that tidies values nobody complained about rewrites
`{"a":"x\\u0000y"}`, six literal characters after a doubled backslash, and
corrupts it.
The JSON arm is a string-literal SCANNER, not decode-walk-remarshal, which
is what the recon write-up proposed before it was written. Re-marshalling
changes four things nobody asked to change — object key order, insignificant
whitespace, integers wider than float64, HTML-ish characters — and silently
drops one of a document's LITERAL duplicate keys, which is a gap BUG-2812
owns and the last thing a repair should do. Scanning copies every byte it
does not deliberately rewrite, so an untouched document is byte-identical
without that having to be argued. A substring replace is not equivalent and
the test that proves it took a mutation to find: a doubled-backslash literal
ALONE never reaches the scanner, so the discriminating fixture is one
document carrying a live escape AND a literal.
THE COUNT IS COMPUTED IN GO. Measured on the read path in this worktree: a
row planted with `bad<NUL>name` 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 and concluded no DB-side REPAIR could be
trusted; the same measurement on the read path says no DB-side COUNT can be
either. SQL narrows — `instr(col, char(0))`, plus the escape prefix on
JSON-classed columns, which is textguard's own pre-filter — and never
decides. The decision stays ParameterRefused with isJSON from the shared
86-column list, i.e. Layer B's classing.
Row addressing is read from the live schema rather than a hand-kept map:
39 tables carry protected columns, one (item_wiki_links) declares no primary
key and is addressed by rowid, two have composite keys, and five have a
single key that is not `id`. The repair checks RowsAffected because an
address that stopped selecting its row would otherwise commit an UPDATE that
touched nothing and report it as repaired — the one failure an operator
cannot see in the output.
`email_optouts(email)` is both a protected column and its own primary key.
Repairing it changes the row's identity and can collide with an existing
row, which in that table means somebody starts receiving mail again. It is
reported and skipped, with the reason.
The import flag is NOT an exemption from the gate. `--repair-nul` buys the
body one repair attempt and then runs the same `bodyDecodesNUL` on the
repaired bytes, which still decides — a decode path that skipped the check
is the door BUG-2803 spent thirty rounds closing, on the endpoint carrying
the largest attacker-controlled body in the product. Only the ESCAPE form is
repaired: a raw NUL byte makes the document invalid JSON, and widening what
parses is not this flag's job. Both doors are covered, JSON and tar.gz,
because giving them different answers is how one of them keeps being
forgotten.
Postgres is settled with evidence rather than sent up as a ruling: it cannot
hold either defect (22021, 22P05) and the four-way differential test already
pins that, so the scan reports not-applicable WITH the reason rather than
returning a zero a reader could mistake for a clean database.
Spellings settled here, per the dispatch: `pad db scan-nul` and
`pad db repair-nul` as siblings rather than `repair --nul`, matching
`migrate-to-pg`'s hyphenated compound — a repair verb that errors when given
no flag is a worse shape, and there is no second repair to share it with.
scan-nul IS the dry run, so repair-nul grows no --dry-run. It refuses while
the server is running unless --force, on the `pad db restore` precedent: the
report is a claim about a database, and one somebody else is concurrently
writing makes it a claim about a moment that has passed.
docs/backup.md's section on this is rewritten. It still said the rule lives
in the binary and not the database, which S2 made false, and it pointed at
this item for a preflight and a repair that now exist. Its import examples
also showed `pad workspace import < file`, which has never worked — the file
is an argument.
Closes BUG-2810.
|
||
|
xarmian
|
ba1255881d |
fix(server): refuse a decoded NUL in a JSON request body (BUG-2803) (#1220)
* fix(server): refuse a decoded NUL in a JSON request body (BUG-2803)
The body half of BUG-2782 (path) and BUG-2784 (query). A caller-supplied
string reached a Postgres text parameter, Postgres refused it, and the
handler answered 500 — the honest answer is 400.
WHY THE TRANSPORT RULE CANNOT BE EXTENDED, which is the whole reason this
is a different fix rather than a wider middleware. ValidateQuery works
because a decoded query value is a substring of the raw query with ASCII
substitutions: the bad byte in the raw text IS the bad byte in the value.
That property fails for a JSON body — the reachable NUL arrives as the
six-character escape, all ordinary ASCII — so no request middleware can
find it without decoding the body, which is the handler's job.
MECHANISM, each premise measured against encoding/json rather than
reasoned about:
raw NUL inside a string -> decode ERR (invalid character in string literal)
raw NUL after the value -> decode ERR
the escape in a value -> decodes to a string CONTAINING a NUL
the escape in a KEY -> same
a DOUBLED backslash -> decodes to literal text, NO NUL
the uppercase spelling -> not a JSON escape at all
So the escape is the only vector and its substring is a sound FAST PATH
(absent -> no NUL possible), but not a sufficient test: a doubled
backslash carries the same six characters and decodes to text. In this
product that is not hypothetical — items and documents store markdown,
and a document about JSON escapes is an ordinary thing to write. The
exact step is json.Decoder.Token(), which returns DECODED strings, covers
object keys and arbitrary nesting (an item's fields blob), and needs no
knowledge of the destination type.
NOT REFLECTION over the decoded value, the other obvious design: it sees
[]byte fields AFTER base64 decoding, so a body carrying legitimate binary
({"b":"AQAC"} -> bytes 01 00 02) would be refused for a NUL that is not
text. A token walk sees the base64 characters. No request struct has such
a field today (searched: []byte with a json tag in internal/server and
internal/models, non-test — only models.YjsUpdate.UpdateData, which no
handler decodes from a body); the token walk is chosen so adding one
later cannot silently start rejecting valid requests.
BUFFERING IS NOT A COST. json.Decoder.Decode already holds the whole
top-level value in memory — refill accumulates into dec.buf and grows it
by doubling (encoding/json/stream.go) — so streaming never avoided the
copy. Measured on the 64 MiB workspace-import shape, total allocation:
stream+Decode 354.7 MiB, ReadAll+Unmarshal 256.5 MiB, ReadAll+Decode
512.5 MiB. Peak heap is order-dependent and does not discriminate; the
first run of that measurement showed a 0.77x peak win that vanished when
the legs were swapped, so only the allocation figure is claimed.
POPULATION, measured on Postgres 17 through the real router with a
control leg on every endpoint (92 mutating routes enumerated via
chi.Walk; 13 probed):
before: 12 of 13 DOOR (control 201 / NUL 500, SQLSTATE 22021)
after: 0 of 13 — every NUL leg 400, every control leg unchanged
Confirmed doors: workspace name, collection name, item title, item
content, item fields value, item title via PATCH, comment body, agent
role name, view name, document title, webhook secret, workspace import.
workspace-token name is UNMEASURED, not clean — its control leg 500s on
an unrelated FK in this fixture. The other 79 routes are unprobed, not
claimed clean; the completeness argument is structural instead, and
enforced by a test rather than asserted.
SECOND DEFECT, named rather than slipped in: the six handlers that
decoded straight off r.Body had no http.MaxBytesReader either — the cap
decodeJSON has always applied — so each was an unbounded body read.
Routing them through decodeJSON closes that too.
COMPATIBILITY: json.Unmarshal refuses trailing non-whitespace after the
JSON value where Decode ignored it. Deliberate, same direction as this
fix, and the only behaviour change beyond the refusal. Trailing
whitespace still passes. An EMPTY body still returns a wrapped io.EOF,
because handlers_playbooks.go reads errors.Is(err, io.EOF) as "no
arguments supplied" — caught by TestPlaybookRunAcceptsEmptyBody, which is
exactly the wiring a helper-level change is blind to.
No call site changed for the refusal itself: all 65 decodeJSON callers
already turn a decode error into a 400 carrying err.Error().
Release note: a NUL character in a JSON request body now returns 400
instead of 500 on Postgres deployments.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): follow the NUL refusal into JSON-encoded string fields (BUG-2803)
Codex round 1 on #1220: the check scanned ONE JSON layer, and several
fields cross the wire as JSON-ENCODED STRINGS rather than nested objects
— an item's fields, a collection's schema, a workspace's settings. The
OUTER decode of {"fields":"{...}"} yields the inner document as literal
text, in which the escape is still six ordinary characters and no NUL
exists, so the single-layer token walk passed it.
MEASURED on Postgres 17 with a control leg on each, after the
single-layer check was already in place:
item.fields as a JSON-encoded string 500 control 201
collection.schema as a string 500 control 201
workspace.settings as a string 500 control 201
The error is DIFFERENT from the rest of this family, which is why it is
worth reading rather than assuming:
insert collection: ERROR: unsupported Unicode escape sequence (SQLSTATE 22P05)
22P05, not the 22021 the path and query halves produce. The outer string
is pure ASCII so it never trips the text-encoding check; this is
Postgres's own JSON parser refusing the escape inside a document bound
for jsonb, which cannot represent a NUL. After this change all three
answer 400 with their control legs unchanged.
THE FIX: when a decoded string is itself a complete JSON object or array
— the class this API re-parses downstream — walk it too, to a depth
bound of 8. Recursion terminates on its own (each level is a strict
substring of the one above); the bound keeps a hostile body from buying
many full re-parses, and AT the bound the body is refused rather than
passed uninspected, since the escape is known to be present and the walk
has stopped looking.
WHAT THIS OVER-REFUSES, by design and pinned by a test: the rule is
structural, not destination-typed, so a plain TEXT field whose ENTIRE
value is a valid JSON document carrying the escape is refused too, even
though its column would have stored it. Prose ABOUT a JSON escape does
not parse as a bare document, so the case is narrow, and a value of that
shape breaks any consumer that parses it. The destination-typed
alternative — an allow-list of the fields that arrive JSON-encoded — is
exactly correct and goes stale in silence, which is the failure mode
ValidateQuery's comment rejects when it explains why per-site query
validators could not be written.
Tests: nested documents (fields/schema/settings/array/twice-encoded),
with controls for ordinary content, a doubled backslash INSIDE the
nested document, a string that starts like JSON but does not parse, and
prose that merely mentions the escape; the over-refusal pinned as a
decision rather than left as an accident; the depth bound; and a wiring
leg through the real router on SQLite, where the write would otherwise
SUCCEED so a green cannot be the database doing the work. Fixtures build
their JSON-encoded strings with encoding/json rather than hand-written
backslashes, since the escaping rules are the subject under test.
All four new tests fail with the recursion removed.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): build the NUL-bearing timeline fixture through the store (BUG-2803)
TestTimeline_NeverEmitsACursorItWouldRefuse built its fixture through the
API on a premise its own comment stated: "a structured id comes from the
item's fields blob, which nothing validates on write". BUG-2803 made that
false — decodeJSON now refuses a body whose strings decode to a NUL,
including one nested inside a JSON-encoded `fields` string — so the API
can no longer produce the row and the test 400'd on its fixture.
Repaired rather than deleted, because the DEFENCE it covers is still
live: rows in this shape can predate the rule, and the store has no such
check of its own, so a migration, an import or any future non-HTTP writer
can still produce one. The timeline must keep refusing to hand out a
cursor it would then reject.
The fixture now writes the blob directly, injecting the six-character
JSON escape rather than a raw NUL — the blob is JSON text and both
backends reject a raw NUL in it; the NUL comes into existence when Go
DECODES the blob, which is exactly how the timeline ends up with one
inside an entry id. The test is not vacuous under the change: it asserts
the NUL-bearing id took the positional fallback, so an injection that
failed to produce a NUL fails the test rather than passing quietly.
This is the CONVE-23 case — a change that falsifies existing prose owes a
sweep for that prose. The stale sentence was found by the test failing,
not by the sweep, which is the weaker of the two ways to find it.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* docs(server): correct the timeline comment BUG-2803 falsified (CONVE-23)
The entryID fallback's comment said note and decision ids "come from the
item's fields blob and nothing validates them on write". BUG-2803 made
that half false: the HTTP API now refuses a request body whose strings
decode to a NUL, including one nested inside a JSON-encoded `fields`
string. The sentence was true when written and nothing in this branch's
diff pointed at it.
The fallback still has to exist, and the corrected comment says why:
the STORE has no such check, so rows predating the rule — and anything
writing a blob by another path, a migration, an import, a future
non-HTTP writer — can still carry one.
SWEPT AND DELIBERATELY LEFT: two nearby comments
(handlers_timeline_id_collision_test.go, handlers_timeline_structured_test.go)
also say "nothing validates them on write". Both are about id FORMAT and
DUPLICATION — an imported artifact carrying a UUID-shaped id, a
hand-written blob repeating one — and this change validates neither. In
context those sentences remain true, so they are left alone rather than
edited into noise.
Sweep command: grep -rniE "nothing validates|not validated on write|no
validation on write|unvalidated" --include=*.go internal/ cmd/ — six
further hits, all about other subjects (github_pr raw writes, terminal
schema keys, push payload format, decodeJSON's size bound).
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): scope the nested-NUL walk to JSON-encoded fields (BUG-2803)
Codex round 2 on #1220. The nesting check from the previous commit
recursed into ANY string that parsed as a JSON document, on the argument
that a structural test beats a destination-typed one. That argument was
wrong in a way I had written down as an accepted trade and should have
weighed as a defect: a plain-text `content` value holding a JSON snippet
that merely MENTIONS the escape was accepted before this branch, is
stored in a text column that has no problem with it, and was newly
refused — including on RE-IMPORT of an export carrying it.
Refusing input the server itself produced is a worse failure than the
door the unscoped recursion was closing. Measured before the fix: a
workspace whose item content held such a snippet exported 200 and
re-imported 400.
The walk now descends only under keys whose STRING value is a JSON
document something downstream re-parses: config, events, fields,
metadata, phase_data, plan_overrides, schema, settings, tags, traits.
WHY A LIST IS SAFE HERE, when ValidateQuery's comment rejects exactly
this shape for query parameters: there the set of names is unbounded by
design (parseItemListParams turns any unrecognised parameter into a field
filter), so no list could be complete. Here the set is a closed property
of the wire model — a field is JSON-encoded because a Go struct declares
it as a string holding JSON — and
TestJSONEncodedFieldKeysCoversTheModels derives it from internal/models
and fails when a new one appears. The list cannot go stale in silence.
Over-inclusion is the safe direction and the list takes it: a listed key
that is not really JSON-encoded costs one parse attempt and can only
refuse a complete JSON document carrying the escape, while a missing key
reopens a door. `traits` is listed for that reason — it carries JSON but
its declaration has no comment saying so, which is exactly how the
derivation test would have missed it, so the test asserts coverage in one
direction only and the list is allowed to be a superset.
The walk also changed shape: decoding into `any` and walking the value,
rather than a token stream, because key context is needed to know which
subtree is JSON-encoded. The []byte reasoning is unchanged and still
holds — decoding into `any` never produces a []byte, so a base64 field is
seen as its ASCII text rather than as decoded bytes that might contain a
legitimate 0x00.
Tests: text fields carrying a JSON document are ACCEPTED (five keys),
with a leg proving the same document under a JSON-encoded key is still
refused, so the pair differs only in the key; the derivation test; and
the depth-bound fixture now nests under a JSON-encoded key at every
level, since nesting under an ordinary key would never start the
recursion and would have passed for the wrong reason.
STILL OPEN, and the lead holds it: a LEGACY row whose stored fields blob
already carries the escape still exports 200 and re-imports 400. That is
data this fix cannot make importable without weakening the write-side
refusal, and the disposition (repair sweep, flagged import, or documented
acceptance) is a product ruling. Recorded on the item.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): close the three body doors codex round 3 found (BUG-2803)
All three verified before fixing, none taken on the reviewer's word.
1. BUNDLE IMPORT BYPASSED THE REFUSAL (P1). handlers_import_bundle.go
parses pad-export.json itself rather than through decodeJSON, so the
SAME workspace import — reached with Content-Type application/gzip
instead of application/json — walked straight past the NUL check into
Postgres. The bundle's export blob is now checked with bodyDecodesNUL
before ImportWorkspace, answering the same 400. Test drives a real
tar.gz through the router with a clean-bundle control leg, because this
path answers 400 for a dozen unrelated reasons (bad gzip, out-of-order
tar, duplicate entries) and a bare 400 would prove nothing.
2. ONE CALLER SWALLOWED THE NEW ERROR (P2). handlers_admin.go's
test-email endpoint read `if err := decodeJSON(...); err != nil ||
input.To == ""` and fell back to the admin's own address, so a body
carrying a NUL answered 200. An ABSENT body legitimately means "send it
to me"; a body that is present and REFUSED is a different thing, and
collapsing the two turns a validation error into a success. The two
cases are now separated on errors.Is(err, io.EOF).
3. THE COMPLETENESS TEST COULD NOT SEE PAST TWO CALL SHAPES (P2). It
scanned for json.NewDecoder(r.Body) and io.ReadAll(r.Body), so it was
blind to io.ReadAll(io.LimitReader(r.Body, n)) — a shape ALREADY in the
package — and to any alias or helper. A completeness test that misses a
live example is worse than none, because it reads as coverage. It now
scans for the thing that cannot be spelled around, a reference to the
request body at all, and requires every FILE touching one to be
accounted for with a written reason. Both directions are asserted: an
unaccounted file fails because a door may have opened, and an accounted
file that no longer touches a body ALSO fails, so the list cannot rot
into stale excuses that quietly cover a future reader. Verified with a
positive control (an added body reference in an unlisted file fails) and
a negative one (a stale entry fails).
FOUND BY THAT WIDENED SWEEP, and fixed here rather than filed: the raw
artifact import (POST /workspaces/{ws}/import-artifact) takes TEXT, not
JSON, so it never went through decodeJSON and inherited neither the NUL
refusal nor the path/query rule — a body is neither. A raw NUL or
invalid UTF-8 reached the store and Postgres answered 22021, which the
handler turned into a 500 for what is a client error. It now applies
bindableText, the same predicate ValidatePath and ValidateQuery use, and
answers 400 invalid_body. Note the shape difference from the JSON half:
there the ESCAPE is the vector because a decoder rejects a raw NUL;
here the RAW BYTE is, because nothing is in the way.
Each fix has a mutation run against it: disabling the bundle guard fails
the bundle test, disabling the artifact guard fails the artifact test,
and both controls still pass.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): the escape gate was unsound, and YAML has its own (BUG-2803)
Codex round 4, two P1s, both reproduced before fixing.
1. THE FAST PATH LET A REAL NUL THROUGH. bodyDecodesNUL gated on "does
the raw body contain the six-character escape". That is unsound: the
BACKSLASH itself can be written as an escape, so a body carrying
\u0000 contains no literal six-character sequence anywhere in its
raw bytes, while the OUTER decode manufactures one inside the string —
and if that string is re-parsed as a JSON document (jsonEncodedFieldKeys)
the second parse turns it into a real NUL.
Measured through the real router before the fix: the oblique spelling
answered 201 where the direct one answered 400.
The mistake was applying a fact about how a NUL is spelled INSIDE a
decoded string to the RAW BYTES, where the backslash can itself be an
escape. That is the same layer-confusion this whole bug is made of, for
the third round running.
The gate is now a BACKSLASH. Every JSON escape mechanism requires one, so
a body with no backslash has decoded strings byte-identical to its raw
bytes, and a raw NUL cannot survive the decoder — no backslash therefore
means no NUL, at any depth, however spelled. Bodies WITH one pay for an
exact answer, a larger set than before (any nested JSON carries a
backslash-quote), which is the cost of being correct. The same
correction applies to the per-string pre-filter one level down.
2. YAML HAS ITS OWN ESCAPE VOCABULARY. The raw bindableText check added
last commit passes a double-quoted scalar `title: "a\0b"` — no NUL in
the request bytes — and the YAML decode manufactures one. Measured
before the fix: that artifact imported 201 with a NUL in the item title.
The decoded artifact is now checked too: title, body, and every
frontmatter field value, walked because a playbook's `arguments` is a
nested structure rather than a scalar. Keys are checked as well as
values, on the same precautionary grounds ValidateQuery states for
query parameter names.
Same shape as the JSON half in both cases: a value that is harmless
until a SECOND parse, checked at the layer that can see it.
Tests: the oblique spelling joins the nested-document table, and the
YAML escape joins the artifact table. Each is mutation-verified —
reverting the gate to the substring fails the oblique case only, and
disabling the post-decode artifact check fails the YAML case only, with
the raw-byte cases still killed by the raw check. That per-leg
discrimination is the point: it shows each check earns its own keep
rather than being covered by its neighbour.
Prose corrected where this falsified it: jsonNULEscape's "it is the ONLY
spelling" is true of the escape and was being used to justify a filter on
the raw bytes, which is a different claim. Both now say so explicitly.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): multipart text fields and the bundle manifest (BUG-2803)
Codex round 4's two P2s. Both are the same shape as the rest: a
caller-supplied string reaching a text comparison through a door the
earlier fixes did not cover.
1. MULTIPART TEXT FIELDS. The multipart body is deliberately exempt from
the JSON rule — its payload is binary blob content and must not be
scanned for text validity — but its TEXT fields are a different thing.
`item_id` goes to ResolveItem and into a database comparison exactly as
the query-string channel does, and that channel has been validated at
the transport since BUG-2784; the form channel was not. multipartValues
now drops values that are not bindable text, which makes an unusable
value indistinguishable from an absent one — the disposition
resolveUploadItemID already applies to empty values.
The uploaded FILENAME gets the same predicate, with a fallback to a
generic name rather than a refusal: the bytes are fine, only the label
is unusable.
A NEGATIVE RESULT worth recording, because it changed the test: a RAW
NUL in the multipart header is NOT the vector. Go's multipart reader
refuses it as a malformed MIME header line before any handler sees it
(measured: 400, "malformed MIME header line"). The reachable spelling is
the RFC 5987 encoded form, filename*=UTF-8''sh%00ot.png, which the
header parser accepts and percent-decodes afterwards. The first version
of this test used the raw form and was testing a vector that does not
exist.
2. THE BUNDLE ATTACHMENT MANIFEST. A second JSON document inside the
tar.gz, parsed directly like pad-export.json was, so it needed the same
check. Without it a NUL in a manifest string reached
rehydrateAttachment, whose failure is logged and SKIPPED — so the import
reported success while silently dropping the attachment. The
skip-on-failure behaviour is pre-existing and deliberate (a partial
restore beats none); refusing the bad INPUT is what stops it being
reached this way. Left as it is, and named rather than quietly changed.
A VACUOUS ASSERTION THE MUTATION CAUGHT, recorded because the test would
otherwise have shipped as coverage: the filename leg first asserted
`!strings.ContainsRune(body, 0)` on the RESPONSE, which is JSON — a NUL
in the filename comes back as the six-character escape, not as a 0x00,
so the check passed whether or not the fix was present. It did pass with
the fallback disabled. Now it decodes the response and asserts the
replacement name. The item_id leg had the mirror-image weakness: it
asserted "not a 500", which is the Postgres-only symptom, so on SQLite it
would have passed either way; it now asserts the request behaves exactly
like the no-value control.
Every fix in this commit has a mutation against it, and each kills only
its own leg.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* refactor(server): drop the now-unused escape constant (BUG-2803)
The gate became a backslash check, which was the last production use of
jsonNULEscape; golangci-lint's unused check failed on the next run. Its
documentation was load-bearing, so the explanation moved into
bodyDecodesNUL's comment rather than being deleted with the variable —
including the distinction that made the old gate wrong (the escape has
one spelling INSIDE a decoded string, which is not a claim about the raw
bytes).
Caught by re-running lint on the tip after the previous commit rather
than trusting the run from the tip before it.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): rune-safe truncation and User-Agent sanitising (BUG-2803)
Codex round 5 was asked for the POPULATION rather than a confirmation —
"enumerate every remaining way a caller-supplied string can reach a
database text or jsonb parameter without passing a validity check" — and
returned three residual classes with their sinks. Two are fixed here;
the third is filed, because measuring it needs a fixture this unit
should not grow.
1. TRUNCATION CAN UNDO THE VALIDATION. Four sites cut a caller string
with a plain byte slice (name[:120], input.Name[:200]). If the boundary
lands inside a multi-byte rune the result ends in a partial sequence and
is no longer valid UTF-8 — so a value that PASSED the body check a few
frames earlier arrives at the store unbindable, and Postgres answers
22021 for a request the server already accepted.
This is the interesting one, because no input-side round could have
found it: the defect is downstream of validation, and it is invisible
with ASCII fixtures, which is what every test in that area used.
truncateBindableText walks back off continuation bytes and drops the
straddling rune. Tested with 2-, 3- and 4-byte runes so an off-by-one
walk-back cannot pass them all, and with a counterfactual leg asserting
the naive slice really does produce unbindable output for the same
input — without it the cases would pass against an implementation that
did nothing.
2. USER-AGENT REACHES TEXT COLUMNS. It lands in activities.user_agent
(three document paths, the connected-apps revoke) and
sessions.user_agent (three login paths), and no rule here sees a header.
The disposition is SANITISE, not refuse, and that is deliberate: a
header is metadata this server chose to record, not something the caller
asked for, so a malformed one must not turn an otherwise fine request
into a 400. The two sites that HASH the header are left alone — sha256
over arbitrary bytes is well defined, and changing what is hashed would
invalidate every stored UAHash.
The filing's own earlier probe had recorded User-Agent as NOT
reproducing on the item-create path. That was true and did not
generalise; these are different sinks.
3. NOT FIXED, FILED: the OAuth form-encoded bodies
(/oauth/token, /oauth/authorize/decide, /oauth/revoke,
/oauth/introspect) parse url-encoded form data outside the shared body
validator, with connection_name reaching oauth_connections.name and
client_id reaching the oauth_clients.id lookup. This was the ORIGINAL
subject of BUG-2803 before the filing was re-scoped, and it was recorded
then as unreachable without a fosite-backed fixture. That is still true,
and round 5's sink list is far more than the filing had. Filed rather
than guessed at.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): narrow the gate, stop refusing natural-shape fields (BUG-2803)
Codex round 6 plus one measurement of my own. Three changes, one of them
a revert of something I got wrong in the previous commit.
1. THE GATE COST TOO MUCH, so it is narrower and still sound. The
previous commit gated the walk on "does the raw body contain a
backslash", which is correct but catches every body carrying nested JSON
(each `\"` is a backslash). Measured on a ~377 KB import-shaped body:
60106 allocs/op with that gate versus 30073 with the walk disabled — the
walk was running on ordinary traffic.
The gate is now the four bytes that begin any \u escape for a character
below U+0100. The argument: to manufacture the six-character NUL escape
inside a decoded string, each of its characters arrives either literally
from the raw bytes — in which case the raw contains the escape, which
begins with that prefix — or from a \u escape of its own, and the three
characters involved (backslash U+005C, 'u' U+0075, '0' U+0030) all sit
below U+0100, so those escapes begin with it too. Back to 30073
allocs/op, identical to the walk-disabled build.
That argument is the same KIND of reasoning that was wrong two rounds
ago, so it does not stand on its own: a differential test runs the gated
function against an UNGATED walk over a corpus built to attack it —
oblique backslash, upper-case hex, an escaped 'u', an escaped '0', a
doubled backslash — and fails on any disagreement. It also asserts the
corpus contains both answers, since agreement over a one-sided corpus
would be vacuous. Reverting the gate to the old substring fails it.
2. THE CHECK REFUSED THE NATURAL SHAPE OF ITS OWN FIELDS. `tags` and
`fields` accept both a JSON-encoded STRING and their natural array/object
form, and the walk propagated "this subtree is JSON-encoded" into
containers — so a free-form tag whose whole value happened to be a JSON
document was refused, though nothing re-parses it. Measured: refused
before, accepted now, while the JSON-encoded spelling of the same field
is still refused. The flag now marks only a direct STRING child of a
listed key.
3. REVERTED: I wired the three LOGIN paths to the User-Agent sanitiser
last commit, before reading store.CreateSession. It HASHES the header
and stores no text — the round-5 enumeration named "sessions.user_agent"
and I took the name for a column. The change would have been actively
harmful: login would store sha256(sanitised) while middleware_auth still
compares sha256(RAW), so every session from a client with a non-UTF-8
User-Agent would fail validation. A sink named in a review is a pointer
to verify, not a finding. The real sink is activities.user_agent, from
three document paths and the connected-apps revoke.
4. And the wiring leg codex asked for, on that real sink: a request
through the router with a malformed header, reading the STORED value out
of the activities row, with a control asserting an ordinary header is
kept VERBATIM. Unwiring the production call site fails it; the helper's
unit test does not notice.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): apply the key rule at every level, not once (BUG-2803)
Codex round 7, both findings, the first confirmed by measurement.
1. THE RECURSION WENT ONE LEVEL TOO DEEP. Once the walk descended into
a JSON-encoded string it treated the WHOLE subtree below as
JSON-encoded, so a value nested two levels down — an ordinary string
inside a `fields` blob that happens to hold JSON text — was refused.
That is a false rejection, and the measurement says so plainly. With the
depth-2 check disabled, on Postgres 17:
depth 1 (the fields blob itself) -> 400 (correct: Postgres parses it)
depth 2 (a string INSIDE the blob) -> 201 (accepted, no error)
control -> 201
The handler parses `fields` ONCE. The inner text is re-escaped when the
blob is written, so what Postgres receives has a doubled backslash and no
escape at all. Only the document Postgres itself parses can carry a fatal
one.
The nested call now passes false rather than true, which makes this a KEY
RULE APPLIED AT EVERY LEVEL rather than a depth limit: a JSON-encoded key
INSIDE a document still recurses (pinned by a test), an ordinary one does
not. Same correction as round 6's natural-shape fix, one level further in
— I fixed the sibling case and left this one, which is CONVE-18's lesson
about my own enumeration being a sample too.
I checked whether anything re-parses a value inside the blob before
loosening this, rather than assuming: `arguments` was the candidate, and
parsePlaybookArguments asserts it is a native ARRAY (raw.([]any)) rather
than a JSON string, so it is covered by the natural-shape rule and needs
no second parse.
2. AN ERROR MESSAGE THAT SENT CLIENTS THE WRONG WAY. The OAuth dynamic
client registration handler prefixed every decode failure with "Request
body must be JSON". A body carrying a NUL is valid JSON, so that message
sends a client hunting a syntax error it does not have. The two failures
are now distinguished.
Round 7 also reports no break in normal CLI, MCP or web-client request
generation — they marshal JSON and encode paths and query parameters —
which is the first thing any round has said about the client surface.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): complete the artifact check, make the walk path-aware (BUG-2803)
Codex round 8. It confirmed round 7's two fixes, then found two real
defects and two inaccurate comments — the comment half being the angle
the round was asked for.
1. THE ARTIFACT CHECK MISSED TWO REACHABLE FIELDS. artifactIsBindableText
walked the decoded artifact by TYPE, so it never covered Provenance —
whose strings are rendered into a Markdown footer appended to the stored
content — and never matched Arguments, declared []map[string]any, a
concrete slice type the walk's []any case does not match. A YAML NUL
escape in either reached storage.
It now MARSHALS the artifact and searches the output for the escape
encoding/json produces. A type switch over a struct that grows is a list
that goes stale in silence; marshalling covers every exported field,
including ones added later. The one thing it cannot see is invalid UTF-8
(which marshals to U+FFFD), and it does not need to: step 2 rejects that
in the request bytes, and YAML cannot manufacture it from valid input —
its escapes name code points, where \0 names a NUL.
Both new cases fail with the check disabled; the raw-byte cases still
pass, killed by the raw check, so each leg is discriminating.
2. THE WALK WAS NOT PATH-AWARE. A collection may declare a user field
literally named `schema` or `tags`. The walk consulted the wire-key list
at every level, so `{"fields":{"schema":"..."}}` treated a user field
name as a wire key and refused valid text holding a JSON example.
The key list is now consulted only OUTSIDE caller data — not under a
natural `fields` object, not inside an element of a `tags` array, not
inside a re-parsed document. Combined with round 7's fix that makes the
descent exactly one level deep BY CONSTRUCTION, which is why the depth
counter is gone: with the flag no longer inherited, a bound could never
fire, and dead protection reads as protection. The depth-bound test is
replaced by one that pins the property directly — an escape IN the
parsed document is refused, one BELOW it is accepted, and a
wire-key-shaped user field does not restart the descent.
3. THREE COMMENTS CORRECTED, all mine, all of the kind a reader would
believe without checking:
- MaxBytesReader: Close FORWARDS to the underlying body rather than
being a no-op, and with a nil writer there is no automatic 413 — the
cap surfaces as a read error the callers turn into 400. Behaviour
unchanged; only the claim was wrong.
- parseArtifactRequest said "three checks" while implementing five, and
its returns list omitted ErrArtifactUnbindableText. Both added by this
branch, which is exactly the prose a change is most likely to falsify
(CONVE-23).
- errJSONBodyNUL claimed all 65 callers surface its message. The STATUS
is uniform; the wording is not — several substitute a generic string.
4. And one in a test: the timeline fixture said both backends hold a
CHECK constraint a raw NUL violates. items.fields is a plain TEXT column
with no CHECK on SQLite. What was OBSERVED is "SQL logic error:
malformed JSON"; the likely source is an expression index over
json_extract, and that attribution is recorded as NOT verified rather
than asserted.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): a regression this branch introduced, and the same trap again (BUG-2803)
Codex round 9. Both findings are mine, one of them a regression from the
round-8 restructure two commits ago.
1. THE ROUND-8 RESTRUCTURE REOPENED THE ORIGINAL DOOR. Taking the
JSON-encoded branch for a listed key skipped the plain "does this string
contain a NUL" check and asked only "does the document this string
carries hold an escape". Those are different questions. So
{"fields":"a<NUL escape>b"} — a direct NUL in the fields value, the very
first case this whole change closed — was accepted again.
Both checks now run. The test pins all three legs: a direct NUL in the
fields string, an escape inside the fields document, and an ordinary
fields string that must still be accepted, so the first two cannot pass
merely because everything under a listed key is refused.
2. THE ARTIFACT CHECK FELL INTO THE TRAP IT WAS WRITTEN AGAINST. It
searched the MARSHALLED bytes for the escape sequence, and a value
holding the six LITERAL characters marshals to a doubled backslash which
still contains that sequence as a substring — so valid content was
refused. Artifacts are documentation; text about a JSON escape is
exactly what one carries.
Worse than the bug: the comment I wrote asserted the ambiguity "cannot
arise here". It was the same doubled-backslash case bodyDecodesNUL exists
to resolve, one function away, and I wrote a sentence explaining why it
did not apply instead of checking. The marshalled form is now decoded
again and walked with the same machinery — the round trip is what makes
every field reachable without a type switch, the walk is what makes the
answer exact.
Its test asserts literal escape TEXT is accepted in title, body and a
field value, with a counterfactual leg asserting a real NUL in each of
those places is still refused, so acceptance cannot come from the check
doing nothing.
Reverting either fix fails its test and only its test.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* docs(backup): the one case where an export is not importable (BUG-2803)
Codex round 12, an operational pass. It found no migration or config
requirement, and two documentation gaps.
docs/backup.md promises that application-level export/import is portable
across SQLite and PostgreSQL. Since BUG-2803 that has one exception: a
workspace whose stored data contains a NUL exports fine and is refused on
import. It can only affect data written before the rule existed and only
on SQLite, which accepted it — a PostgreSQL instance never stored one.
`pad db migrate-to-pg` has the SAME problem and reports it worse: it
copies rows directly and never passes through the import guard, so a
legacy row fails against PostgreSQL's JSONB parser partway through the
copy rather than being refused up front. That is the likelier way an
operator meets this, since it is the operation that puts an entire old
SQLite database in front of PostgreSQL for the first time. Recorded on
BUG-2810, which owns the preflight and repair.
Round 12's other finding — that the PR's stated release note covered the
JSON 500-to-400 change and none of the rest — is fixed in the PR body
rather than in the tree.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): close two blind spots the tests themselves had (BUG-2803)
Codex round 13, asked whether the new TESTS are sound. Five findings;
these are the two that were self-contained. The other three are recorded
on the item with what each needs.
1. THE COMPLETENESS SCAN WAS BLIND TO FORM BODIES. It matched only
`.Body`, so FormValue / ParseForm / MultipartForm — which read the
request body just as surely — were invisible. It therefore reported full
coverage while the OAuth form-encoded handlers were entirely outside its
view. Widened, and it immediately failed on handlers_oauth.go, which is
the instrument working.
That file is now ACCOUNTED FOR AS A KNOWN GAP rather than as safe: the
OAuth handlers read form-encoded bodies that no rule in this family
covers (the transport rules see the query half of r.Form, not the body
half), tracked as BUG-2811 and needing a fosite-backed fixture to
measure. The test now STATES the gap instead of being blind to it, which
is the difference between a completeness claim and a completeness
appearance.
2. THE TRUNCATION TEST ADMITTED AN IMPLEMENTATION THAT RETURNED "". Its
assertions were: within the limit, bindable text, a prefix of the input.
An empty string satisfies all three. It now also asserts that an input
fitting the limit comes back UNCHANGED, and that no more than one rune
(4 bytes) is lost to the boundary — so a truncator that drops too much
fails, not just one that keeps too much.
Both were found by asking whether a broken implementation would pass,
which is the question CONVE-12 is about and which I had applied to the
production code and not to these two tests.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): the three remaining round-13 gaps (BUG-2803)
Codex round 13's other three findings, all of the same shape: a test
that would stay green with the production change reverted.
1. THE MANIFEST CHECK WAS UNTESTED. The bundle test built archives
containing only pad-export.json, so disabling the INDEPENDENT attachment-
manifest check left the suite green. The new test builds a bundle with
both entries, differing only in the manifest, so a refusal cannot come
from the export half. Verified by disabling each check separately: only
the matching test fails, so the two are independently covered.
2. THE TEST-EMAIL CHANGE HAD NO HANDLER-LEVEL TEST. Every existing leg
exercised decodeJSON, so reverting handlers_admin.go to default EVERY
decode failure to the admin's own address passed them all. The new test
drives the real endpoint with a wired mock sender and pins the
distinction that used to collapse: an ABSENT body still means "send it
to me" (control), an ordinary body still sends (control), and a body that
is present and refused answers 400 rather than being reinterpreted as
the default recipient.
3. THE MULTIPART LEG CHECKED ONE BYTE CLASS. A filter rejecting NULs
while letting malformed UTF-8 through would have passed it. It now drives
both, which matters because invalid UTF-8 is the class that reaches
Postgres as 22021 on a UTF8 database.
Round 13 was asked whether the new TESTS are sound — deterministic,
order-independent, and failing on broken code. It reported the fixtures
isolated and found five ways they were not discriminating. Two were
fixed in the previous commit; these are the rest.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* test(server): pin the wiring at every call site, not one (BUG-2803)
Codex round 14 confirmed round 13's five, then found the same shape one
level out: reverting a SINGLE call site back to the unsafe form left the
whole suite green, because the surviving fixtures are ASCII and a
helper's unit test does not care who calls it.
TestTextSafeHelpersAreUsedAtEveryCallSite asserts the wiring STATICALLY
rather than adding a fixture per site (an OAuth connection, a cloud
login, four audit paths). A byte-slice truncation of a caller string
fails it, and so does a raw User-Agent read outside the exempt set. Both
directions are checked: finding none of the SAFE form also fails, so a
scan that silently matched nothing cannot pass forever.
The User-Agent exemptions carry counts rather than being blanket, so a
NEW raw read in an exempt file still fails. All four reads in
handlers_auth.go are exempt because they feed a HASH — CreateSession
hashes the header and stores no text — and sanitising before hashing
would be actively harmful: login would store sha256(sanitised) while the
session check still hashes the RAW header, failing validation for every
client with a non-UTF-8 User-Agent. middleware_request_text.go's one raw
read is requestUserAgent itself.
Verified by reverting one truncation call site and one User-Agent call
site independently; each fails the test.
Round 14's third finding is fixed behaviourally rather than statically,
because the static scan cannot see it — handlers_oauth.go is already
listed for its form-body reads. TestOAuthRegisterRefusesNULBody drives
the real dynamic-registration endpoint with cloud mode and an OAuth
server wired, with a control leg registering successfully, and pins both
the refusal and the message split: the body IS valid JSON, so the answer
must not send a client hunting a syntax error.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): match wire keys the way the decoder does (BUG-2803)
Codex round 16, asked whether this change is consistent with its siblings
in the same file and extensible by someone who did not write it. It found
a live bypass instead.
encoding/json matches an incoming key to a struct field by an exact match
first and a CASE-INSENSITIVE one otherwise, so {"Fields":...} and
{"FIELDS":...} land in ItemCreate.Fields exactly as {"fields":...} does.
The walk looked the key up case-SENSITIVELY, so it skipped the nested
document for a body the handler went on to accept, and the database
answered the original 500.
Measured before the fix: `fields` refused, `Fields` and `FIELDS`
accepted.
This is the same defect shape as everything else in this unit — a check
that agrees with one layer's rules while the layer that actually consumes
the value uses different ones — which is why the fix is a PREDICATE
rather than a wider map: the map is the vocabulary, and the matching RULE
belongs to the consumer. Someone adding a key should not also have to
remember to add its spellings.
The test drives six spellings including mixed case, with a control
asserting an unlisted key stays caller data in any casing, so this is
case-insensitive matching rather than matching everything.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): fold keys the way encoding/json folds them (BUG-2803)
Codex round 17, first of five findings. The previous commit fixed the
ASCII half of key matching and left the Unicode half, which is this
bug's own pattern one more time.
encoding/json matches with Unicode SIMPLE FOLDING, not lower-casing.
U+017F LATIN SMALL LETTER LONG S folds to 's', so "ſchema" reaches the
`schema` struct field while strings.ToLower("ſchema") is unchanged and
missed the allowlist — a nested NUL under that spelling reached the
handler undetected.
Matching is now strings.EqualFold against each canonical key. The test
carries both a lower-case fold spelling and an upper-case one alongside
the ASCII cases, and keeps its control asserting an unlisted key stays
caller data in any casing.
The other four round-17 findings are recorded on the item rather than
patched here: they are genuine layer disagreements (duplicate keys
merging differently in a typed decode than in a map, a scan-failure
disposition on inputs the typed decode tolerates, and unknown-field
policy) whose fixes are design decisions rather than corrections, and
this seat is near its context bar. Each is written up with the
measurement it needs.
Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
* fix(server): pin that a NUL-bearing manifest refusal keeps the partial workspace (BUG-2803)
Codex round 18. The comment on the manifest NUL branch said refusing the
input "stops it from being reached this way" and stopped there, which
reads as though the refusal undoes the import. It does not.
A plain error with a non-nil workspace keeps the partial workspace,
exactly as every other manifest failure in this loop does — the rollback
branch fires only for *importStatusError, and mid-stream manifest
failures intentionally keep what was imported (TASK-896). Returning a
rollback-shaped error here would give NUL-bearing manifests different
semantics from malformed ones, which is a change to the bundle-import
contract rather than a fix to this bug.
So the behaviour is unchanged and now DELIBERATE: the comment states it,
and the test asserts the persisted state rather than only the HTTP
answer. Mutation: routing the branch through *importStatusError makes
the refusal roll back, and the new assertion fails naming the release
note it would falsify. The pre-existing status/body assertions do not
notice.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(server): record the four map-model disagreements as dispositions, and pin them (BUG-2803)
Lead ruling day-68 is land-and-follow: this branch lands on its measured
commits, and the token-stream rewrite is the BUG-2812 unit's spec rather
than a late restructure of an 18-commit branch under review pressure.
That makes the four open findings from rounds 16-17 something to WRITE
DOWN precisely, not something to leave in a trail comment.
The doc comment on bodyDecodesNUL now carries all four, with the one
root cause named: this scan decodes into map[string]any and the typed
decode does not agree with that model about keys. Two under-refuse
(duplicate-key merge; scan-failure passthrough) and are BUG-2812's spec
- both dissolve under a walk that never builds values. Two over-refuse
(unknown fields; case-variant duplicates) and are ACCEPTED, because
refusing is the safe direction. The asymmetry is stated rather than
smoothed over: within the map model, (1) and (4) are one defect seen
from two sides and only one of them fails safe.
Finding (3) is an observable compatibility change - a forward-compatible
field carrying a NUL escape now gets a 400 where it got a 200 - so it
goes in the release note as well as here. A qualification only protects
where the actor meets it.
All four are pinned by a test, measured on this tip rather than carried
over from the round-16/17 write-up. The two known-gap legs assert the
WRONG answer on purpose: when BUG-2812 lands they FAIL, naming the doc
comment and the release note as what to update. Both gap legs carry a
premise assertion - the same bodies with the disagreement mechanism
removed ARE detected - without which they would pass against a check
that detected nothing.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* test(server): wire release-note item 10 to the router, with its before-state measured (BUG-2803)
The disposition test proves bodyDecodesNUL RETURNS true for an unknown
field carrying a NUL escape. The release note claims the API answers
400. Those are different claims and only the second one is what an
operator or client author reads - CONVE-19, my own convention: a
direct-call test vouches for the component, not its binding.
Two legs, and the control is the load-bearing one. An unknown field with
an ordinary value must still be ACCEPTED, so this pins "refused for the
NUL" rather than "refused for being unknown". The handler does not
reject unknown fields; if it ever started to, the note's explanation
would be wrong while its status code stayed right, and no
status-code-only assertion could see that.
The before-state is measured rather than asserted from memory. Disabling
the check makes the same request answer 201 - which is main's behaviour,
since decodeJSONWithLimit there unmarshals straight into the typed value
and the key is dropped. So "answers 400 where it answered 200" is a
measurement in both directions, not a recollection of one.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(backup): the NUL rule lives in the binary, not the database (BUG-2803, BUG-2813)
Codex round 19, the fresh-angle deploy/rollback/mixed-version pass.
docs/backup.md said a NUL-bearing row "can only affect data written
before that rule existed, and only on SQLite". The second half is true.
The first half is false, and the reason is the interesting part: the
guard is in decodeJSONWithLimit, so the invariant is a property of the
running BINARY, not of the database.
On SQLite any window where an older binary serves the same database can
still write one - a rollback after upgrading, a staged rollout with an
old and a new instance sharing a database, a second older instance on
the same file. The window closes, the guard returns, and the rows are
already stored, behaving exactly like genuinely old ones. A rollback is
an ordinary operational move, so this is not an exotic path.
The doc now states the binary-version dependence, says which dialect is
affected and why PostgreSQL is not (it refuses a NUL itself, at every
version), and gives the operational answer: drain writes from older
binaries before the new one serves, or roll forward rather than back.
Store-layer enforcement - so the running build stops mattering - is
filed as BUG-2813 rather than added here. It is a dialect-level change
and the day-68 ruling on this unit is land-and-follow.
The same false implication was carried by the PR's release note calling
such a workspace "legacy"; corrected there too.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(server): cite the ruling in house style, not the team-room day counter (BUG-2803)
"lead ruling day-68" is the internal day counter, which means nothing to
anyone reading this repo and is inconsistent with every other citation
in it - the codebase cites a lead ruling by DATE or by BUG ref, never by
day-N. Replaced with the bug ref, which is the part a reader can
actually follow.
Claude-Session: https://claude.ai/code/session_01AUvLoXsKdS5sdpYju6rj4p
* docs(server): drop a commit count I had already measured as wrong, and stop asserting a cause I borrowed (BUG-2803)
Two defects in a comment I wrote an hour ago, both of the kind this
unit's trail keeps recording.
"an 18-commit branch" - the branch was 20 commits at
|
||
|
xarmian
|
9be8e96cfd |
fix(cli): safe SQLite db backup/restore (config path + VACUUM INTO) (#837)
* fix(cli): safe SQLite db backup/restore (config path + VACUUM INTO)
pad db backup/restore hardcoded ~/.pad/pad.db, so `docker exec pad db
backup` (container sets PAD_DATA_DIR=/data) and Windows layouts broke,
and the SQLite path did a torn io.Copy of pad.db + separate -wal/-shm
copy that could lose or tear in-flight WAL writes.
- Resolve the SQLite path via the server's config loader (PAD_DB_PATH >
PAD_DATA_DIR/pad.db > ~/.pad/pad.db) instead of os.Getenv("HOME").
Covers backup, restore, and migrate-to-pg's --from default.
- Replace the file copy with an online-safe `VACUUM INTO` through the
embedded modernc.org/sqlite driver: one self-contained file, no
-wal/-shm juggling, safe while the server is live.
- Restore refuses when a live server is detected (a running WAL
checkpoint could clobber the restored file); --force overrides.
- docs/backup.md: `pad db backup -o <file>` is the canonical SQLite
path (+ the `docker exec <container> pad db backup -o /data/backup.db`
form); dropped the "PostgreSQL-only" mislabel.
PostgreSQL pg_dump/psql paths are unchanged.
Fixes BUG-1996.
Claude-Session: https://claude.ai/code/session_01BoPkYhKqMiWPYmxQigeWsA
* fix(cli): fail restore on stale sidecar removal + drop unsafe backup doc
Address Codex review P2s:
- Restore: treat a failure to remove a stale -wal/-shm at the target as
fatal (was silently ignored). With single-file VACUUM INTO backups a
leftover sidecar would replay old WAL state over the restored DB.
- docs/backup.md: the SQLite strategy block still recommended a raw
`cp pad.db` daily; point it at `pad db backup --cron` instead.
Claude-Session: https://claude.ai/code/session_01BoPkYhKqMiWPYmxQigeWsA
|
||
|
xarmian
|
ac744fce2b |
fix(docs): replace 'pad serve' with 'pad server start' (TASK-675) (#199)
The 'pad serve' command does not exist in this binary — its canonical name has been 'pad server start' for some time. Users following the systemd example in docs/deployment.md would get a non-starting service today. Six real references fixed: - cmd/pad/main.go:5714 — migrate-to-pg help text - cmd/pad/main.go:5795 — 'Next steps' instruction - docs/backup.md:81,94 — Postgres migration walkthrough - docs/deployment.md:116 — binary launch example - docs/deployment.md:164 — systemd ExecStart Repo-wide grep is now clean of 'pad serve' outside gitignored v1-archive/ and .pad/ (local workspace data). README.md was already correct. Parent: PLAN-644. |
||
|
xarmian
|
1d26283752 |
feat: add PostgreSQL backup, restore, and migration CLI commands
- pad db backup: wraps pg_dump with --output and --cron flags - pad db restore: wraps psql with confirmation prompt and --force - pad db migrate-to-pg: one-time SQLite→PostgreSQL migration using application-level export/import for all workspace data - docs/backup.md: comprehensive backup strategy guide covering SQLite, PostgreSQL, cloud snapshots, and disaster recovery |