Uh oh!
There was an error while loading. Please reload this page.
fix(objectql): refuse undeclared insert fields at the schema, and keep bound values out of the write-path logs (#8682) - #8737
Conversation
…p bound values out of the write-path logs (#8682) Half A — an undeclared write key is refused by the object's field map before `applyFieldDefaults`, so no default, no summary seeding, no beforeInsert hook, no secret write, no validation and no AUTONUMBER runs for a request that was already going to be refused. Measured on origin/main, one mistyped key made a valid create numbered 0001 be followed by 0003; it is now followed by 0002. The wire answer is unchanged: 400 INVALID_FIELD with the same message. Half B — the insert/update/delete loggers no longer write the driver's bound statement. `Logger` serializes exactly `message` and `stack`, and the statement opened both, so a single mistyped field name wrote an entire row's values to disk at ERROR. The level, the message, the object and the database's own diagnostic (which names the failing column) are all kept; the rethrown error is untouched so the caller's 400 does not move. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 1 package(s): 15 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also reference the affected code. These are read-only:
|
Uh oh!
There was an error while loading. Please reload this page.
Fixes#8682
Both halves of the card land. Verified at
d99df41.Half A — validation ordering
An undeclared write key is now refused by the object's field map as the first act inside
insert()'s middleware body — after middleware (which may legitimately rewritedata), beforeapplyFieldDefaults, and therefore before the defaults, the summary seeding, thebeforeInserthooks, the secret writes, validation, the autonumber and the statement.Measured on
origin/main@3508678with a real engine and a recording driver, one mistyped key on a single insert:0001→ refused →00030001→ refused →0002beforeInserthook runsok-1,bad,ok-2ok-1,ok-2create()callsSQLITE_ERROR, no statusINVALID_FIELD+400The hook did not merely run — its derived value reached the statement, which is the card's
period_label = 'Q3 2026'shape: a hook is not a pure function, so "it ran and we threw the result away" is a side effect of a request the server had already decided to refuse.The wire answer is deliberately unchanged. The refusal carries
code: 'INVALID_FIELD',status: 400,fieldandobject, and its message is byte-identical to the onemapDataError's driver-string branch produced.@objectstack/restre-emits that verbatim, so the caller sees exactly what it saw before — the refusal moved, the answer did not.Where the door deliberately has no opinion (each measured, not assumed — all three surfaced as test failures in the first full run and are now pinned as controls):
schema.fieldsabsent or empty — an empty map is indistinguishable from an unpopulated one, so refusing everything on it would be a verdict made from an absence.id/created_at/updated_at— tolerated even when a declaration omits them, mirroring the three namesfind()/findOne()already add to their known set, so a key accepted by a read is not refused by a write.In all three the driver remains the backstop it has always been; nothing is widened.
insertManyculls such a row per row instead of failing the batch around it — before this, an undeclared key defeated partial-success mode completely, because the row travelled tobulkCreatewith the rest and took the whole batch down with it.Half B — value logging
Logger.error(msg, error, meta)serializes exactlyerror.messageanderror.stack, and a knex-shaped driver error opens both with the fully bound statement. Confirmed with the card's canaries onorigin/main:messageandstackeach carriedSENSITIVE-CANARY-9f3a2band the row's other values — so redacting one and not the other would have moved the leak rather than closed it. Both are rebuilt.What is kept, deliberately: the level (
error), the message (Insert operation failed), theobjectmeta, the database's own diagnostic — which names the failing column — and every stack frame. The card's triage is explicit that the failing column and object must still be logged, and this narrows only WHAT is written.The cut is structural rather than lexical:
@objectstack/types'looksLikeInternalErrorLeakalready owns "is this a driver dump?" for both HTTP boundaries, so this asks it the verdict and adds only the one thing it does not answer — where the statement ends. Every inlined value sits in the statement and the statement always comes first, so the last-separates the part that may carry values from the part that may not. Last rather than first because a bound value may itself contain-; cutting early would leave a fragment of that value standing in what we then log as "the diagnostic". A dump with no separator carries no statement and is returned untouched, same reference.The rethrown error is untouched.
mapDataErrorreads the driver's raw message to answer400 INVALID_FIELDwith the failing field, so the redaction applies to the log slot only — one argument at one call site.One scope call worth a reviewer's eye
The card measured the INSERT logger. The
Update operation failedandDelete operation failedloggers three screens away are the same one-argument call, the same knex-shaped error and the samesecurityconsequence — an UPDATE inlines the caller's values in itssetclause exactly as an INSERT does in itsvalueslist. I applied the same redaction to all three and pinned the update and delete phrasings, rather than leave two thirds of a security-labelled hole open for a later round. Happy to trim it to insert-only if you would rather keep the diff on the card's exact letter.The first sub-claim was already fixed, and I confirmed it
The card reports the false
[REST] Unhandled errorlabel as FIXED upstream. Confirmed on currentmain:packages/rest/src/rest-expected-error-logging.test.tspins exactly that ("[#4886] Expected 4xx must not be logged as[REST] Unhandled error"). Nothing was needed for it, so this PR implements less than the card describes for that one row.Fixture triage
The first full run of the objectql suite went from 3591 green to 49 failures, every one an accept-set narrowing. Triaged individually rather than batch-re-spelled:
titleontask, 1 ×customeronsys_license— fixtures whose registry stub isfields: {}. Fixed in the DOOR, not the fixtures: an empty field map now yields no verdict, which is the correct rule and not a test accommodation.idonbf_leave_request— the fixture declares neitheridnor the audit columns. Fixed in the door too, mirroring the read path's existing tolerance.package_idonsys_metadata— four minimal stubs omitted a column the REAL object declares (metadata-core/src/objects/sys-metadata.object.ts:65, part of the row's uniqueness key) and thatSysMetadataRepositorywrites. The production writer is correct; the stubs were never spec-valid, and nothing noticed while an undeclared key simply travelled on to the driver. Declarations added.engine-insert-many— the bad row was spelled{ slug: 'no-name' }to mean "a row missing the requiredname", butslugis itself undeclared, so the door refused it before the hooks and the assertion would have measured the wrong refusal. Re-spelled to{}, which says the one thing intended.No production code outside
engine.tsneeded a change, and no undeclared-write finding survived triage as a real defect.Verification — at
d99df41pnpm --filter @objectstack/objectql test— 205 files, 3617 tests, 0 failurespnpm --filter @objectstack/objectql typecheck— cleanpnpm --workspace-concurrency=2 --no-bail --filter '...@objectstack/objectql' test(prefix form = the 43 packages that DEPEND on objectql), after a fullpnpm build: 43/43 green, zeroUnknown fieldrefusals anywhere —packages/qa/dogfoodincluded, which boots real example apps end to end. That sweep is the evidence that the narrowed accept set costs no live caller anything.engine.tsreverted toorigin/main, pins re-run, then restored and proved byte-identical (git hash-object== the committed blob). The direction is deliberately not uniform, and that is the point: 8 of 10 half-A cases go red (expected '0003' to be '0002'; hooks['ok-1','bad','ok-2']; 1 driver create), while the two half-A controls stay green — they assert the door has NO opinion, so they must pass onorigin/maintoo. On half B exactly one engine-level case flips (the canary case); "the entry survives" and "the failing column is still named" stay green, becauseorigin/mainalso logged at ERROR with the object and the column — it just also logged the values.scripts/pm/dispatch-gates.mjs, all green:check:durability-log-level,check:stack-collection-maps,check-engine-split-ratio, plus seven the dispatch list did not name —check:nul-bytes,check:changeset-gate-self-tests,check:objectui-changeset,check:query-options-erasure,check:type-check-coverage,check:type-check-debt(the--re-measureratchet, 33 entries, none above ceiling), and the three changeset scripts.The regression pin is the log-independent observable the triage asked for — the autonumber gap across the rejected request. No test in this PR greps log contents for half A, precisely because half B rewrites the logger in the same commit.
Generated by Claude Code