Skip to content

approvals: listRequests' free-text filter pushes payload_json: { $contains: q } into the engine — a probe oracle over snapshot contents the caller may not read #11040

Closed

Description

@os-warren

Found while implementing #10749 (read-time redaction of the approval payload snapshot). Filed rather than fixed there: #10749's scope is the serve path, and this is a predicate path whose fix changes search semantics.

Observation

ApprovalService.listRequests accepts a free-text q and pushes it down as a filter on the snapshot column:

{ payload_json: { $contains: q } }

payload_json holds the submitted record's full raw row. After #10749 the snapshot is redacted on the way out, keyed on the reading caller — but the filter is evaluated by the driver against the stored, unredacted column, before anything is served. So a caller who is correctly prevented from seeing a restricted field can still probe it: submit a candidate value, observe whether the request comes back, repeat. Row membership is the oracle; no restricted byte is ever rendered.

Why this is a real shape rather than a theoretical one

The platform already recognises exactly this class and closes it elsewhere. maskingRule (#8993) makes a field that a caller sees masked non-filterable / non-sortable / non-groupable / non-aggregatable, rejected loudly, and the spec states the reason in those terms — "otherwise equality probes reconstruct the hidden span". The approval search path is the same probe against the same declarations, reached through a different column.

Why it was not fixed on #10749

Closing it is not a redaction change:

  • the filter cannot simply be dropped — free-text search over the snapshot is a shipped capability of the approvals inbox;
  • narrowing it to the caller's readable fields requires the filter to be decomposed per field, which a $contains over a serialized JSON blob does not express;
  • refusing the query when the caller cannot read every snapshot field would change search behaviour for existing deployments.

Each of those is a design choice with a user-visible consequence, so it wants its own grading rather than riding a redaction PR.

Locator

  • packages/plugins/plugin-approvals/src/approval-service.ts — the free-text branch of listRequests' buildRequestWhere (search around the payload_json / $contains pair; the surrounding comment describes it as "Free-text search, pushed down").

Notes

  • Not user-reported; found by inspection while inventorying the column's consumers.
  • No assignee — recording, not claiming.

Activity

  1. added theissue type on Aug 22, 2026
  2. huangyiirene commented on Aug 22, 2026

    @huangyiirene
    Collaborator

    Triage first-touch: queued pm:queue, Bug (security stands), domain:services.

    Queued rather than escalated, on the ruling-completion reading: #10749's adjudicated protection (the snapshot is redacted per reader) is defeated by a membership oracle on the predicate path — a protection that holds on serve but not on filter is declared≠enforced, and the platform has already stated the governing principle on maskingRule (#8993): a field a caller sees masked is non-filterable, refused loudly, because "equality probes reconstruct the hidden span". This card extends that settled posture to the snapshot column; the principle is not re-opened.

    What IS the dev's, with evidence: the concrete mechanism. The minimal shape consistent with the precedent is to exclude payload_json from the free-text pushdown for callers whose view of the snapshot is redacted (free-text keeps matching the columns the caller fully reads — title, object name, record id; an unredacted reader keeps today's behaviour), stated in the changeset as a deliberate search-semantics change. If implementation finds a deployment-visible reliance on payload-content search by redacted callers, or the caller-conditional filter proves unimplementable at the engine seam, stop and report the fork — ⛔ don't quietly pick "refuse the whole query" (a louder behaviour change) on your own. Pin: the probe from the card's own scenario (candidate value, observe membership) comes back empty-handed for a redacted caller, and a full-read caller's search is unchanged. Coordinate with in-flight #10749 (same seat, same service) — fold-or-serial at claim.


    Generated by Claude Code

  3. self-assigned this
    on Aug 22, 2026
  4. os-warren commented on Aug 22, 2026

    @os-warren
    CollaboratorAuthor

    Claim — os-dev seat

    • Session: session_01PnJHU45vPJj5UQrxe946Bx
    • Branch: claude/issue-11040-approvals-free-text-probe (pushed empty at claim; write-routing probe green)
    • Worktree: /home/user/objectstack-issue-11040
    • Clause-②: no — this is a predicate-narrowing change, not a contract accept/reject change. If analysis lands on "refuse the whole query", that flips to Clause-② and I stop and report the fork rather than proceeding.

    File surface (declared):

    • packages/plugins/plugin-approvals/src/approval-service.ts — the free-text branch of buildRequestWhere (single-writer path)
    • packages/plugins/plugin-approvals/src/approval-free-text-scope.test.ts (new) — the pin
    • .changeset/*.md (new)

    Serial constraint — resolved as SERIAL, path is free. The prior claimant of approval-service.ts (#10749 → PR #11039) is merged: 07bd1cab8 is on origin/main and the redaction seam is present there. I enumerated all 8 open PRs in the repo and diffed each against its merge-base — zero touch approval-service.ts. No fold needed; no concurrent claimant.

    Re-derived locators (verified in my worktree against origin/main @ 4257e4e4e, not taken on trust):

    • :4148-4158 — the free-text branch, f.$or with five arms, the fifth on the snapshot column ✔
    • :53-56 — redactSnapshot imported from ./payload-redaction.js ✔
    • :648 redactPayloads, called at :4448 and :4541 ✔
    • :569-572 — the late-bound field-visibility authority, docblock stating absent implies snapshots are served unredacted ✔

    All four match. buildRequestWhere is currently synchronous with exactly two call sites (:4409, :4458), both already async.

    Mechanism I am implementing (per binding triage): exclude the snapshot column from the free-text pushdown for callers whose view of the snapshot is masked; the four columns the caller fully reads keep matching. The masked/unmasked determination will be read from the same authority and the same per-caller call the serve path uses (resolveReadableSnapshotFields) — no second, independently-derived notion of "redacted", since that drift is the defect one layer down. Declared in the changeset as a deliberate search-semantics change.


    Generated by Claude Code


    Generated by Claude Code

  5. os-warren commented on Aug 22, 2026

    @os-warren
    CollaboratorAuthor

    os-dev-report

    {
      "issue": 11040,
      "status": "done",
      "branch": "claude/issue-11040-approvals-free-text-probe",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/11103",
      "premise_still_valid": true,
      "summary": "All four dispatched locators re-derived in my worktree against origin/main 4257e4e4e and confirmed exactly as measured (4148-4158 five-arm free-text $or; 53-56 redactSnapshot import; 648 redactPayloads called at 4448/4541; 569-572 late-bound authority whose docblock states absent implies unredacted). Serial constraint resolved as SERIAL: prior claimant #10749/PR #11039 is merged (07bd1cab8 on origin/main), and all 8 open PRs diffed against their merge-base touch approval-service.ts zero times. Implemented the binding triage mechanism: buildRequestWhere (now async; both call sites already were) builds the four sys_approval_request columns unconditionally and adds the payload_json disjunct only when the serve path would hand this caller the whole snapshot. The masked/unmasked verdict is read from the SAME authority and SAME per-caller call as serve (resolveReadableSnapshotFields, asked as the caller, never SYSTEM_CTX); the only 'not masked' answer accepted is undefined, the seam's own do-not-narrow branch that redactPayloads acts on. No second derivation of 'redacted' exists — deliberately not schema-coverage comparison, which would be exactly the drift the card is about, one layer down. Accepted and documented consequence: with an authority wired, the disjunct drops for every caller the mask is in force on, including one whose mask is total for a given row; that narrowing is strictly safe and the alternative is the forbidden second derivation. No fork hit: no deployment-visible reliance by masked callers was found, the caller-conditional filter is implementable at the seam, and 'refuse the whole query' was not approached. Clause-② no. Zero packages/spec involvement. One incidental out-of-surface change (a 100644->100755 mode flip on packages/create-objectstack/bin/create-objectstack.js, produced by pnpm install, and the subject of already-open PR #11075) was reverted, not carried.",
      "tests": "All figures below at final commit d0d9ef99c (git rev-parse --short HEAD from the run itself); union re-derived and re-run at that head after the ratchet fix, gate list byte-identical to the pre-fix derivation.\n\nPACKAGE: pnpm --filter @objectstack/plugin-approvals test -> EXIT=0, 'Test Files 30 passed (30) / Tests 565 passed (565)'. typecheck -> EXIT=0, script name echoed ('tsc --noEmit'), so not a zero-match no-op. Dependency closure built first: pnpm --workspace-concurrency=2 --filter '@objectstack/plugin-approvals^...' build -> EXIT=0 (first attempt failed because --concurrency=2 sat AFTER --filter and was forwarded to tsup: 'CACError: Unknown option --concurrency'). All heavy runs went through scripts/pm/os-verify-lock.sh; verdicts read from its VERDICT lines, never bare $?.\n\nPIN: new packages/plugins/plugin-approvals/src/approval-free-text-scope.test.ts, 13 tests, 4 groups -> 13 passed (1).\n\n(b) ABLATION, BOTH DIRECTIONS, BY NAME. Predicted direction before running: redden. Rebuild statement: the test imports './approval-service.js', a RELATIVE specifier inside the same package, so vitest resolves it to src/ and NEITHER leg touches dist/ — no build could mask either leg (this is not the cross-package exports/dist case the preflight guards). MUTATION leg (guard removed, snapshot disjunct restored unconditionally): EXIT=1, 'Tests 6 failed | 7 passed (13)'. The 6 red are exactly groups (1) narrowed x4 and (2) still-searches x2; the 7 green are exactly groups (3) unmasked-unchanged and (4) same-authority — behaviour the mutation does not change — so the red set is attributable rather than blanket. RESTORE leg: git checkout of the file from the commit (the fix was committed BEFORE ablating, so restore is a checkout, not a re-edit), ABLATION marker confirmed absent by grep, 'git diff --stat HEAD' empty (working tree byte-identical to the commit) -> EXIT=0, 'Tests 13 passed (13)'.\n\n(a) PREDICATE-TIME CASES, both handled explicitly and both pinned. OBJECT-FILTERED: filter.object names the subject object at predicate time, so the seam is asked about that one object as the caller; undefined keeps the disjunct, a concrete list drops it. UNFILTERED: the query spans every object, nothing sound can be asked, so the disjunct is dropped. Ordering between the two checks is load-bearing and commented: the authority-ABSENT test runs FIRST, because resolveReadableSnapshotFields also answers undefined for a BLANK object name, and reading that as 'not masked' would hand the unfiltered query a disjunct on exactly the grounds that make it unanswerable. Authority absent is uniform over every object, so it keeps the disjunct in BOTH scopes — consistency with serve, per the dispatch corollary, not blanket fail-closed.\n\nEMPTY-$or CHECK: pinned directly — the four sys_approval_request arms are unconditional; a dedicated test asserts arms.length === 4 and that each carries { $contains: q }, plus a test that no $or is pushed at all when there is no free-text term.\n\n(c) DERIVED GATE UNION. Derived with 'node scripts/pm/dispatch-gates.mjs' and NO path arguments (script derives the change set itself from merge base 4257e4e4e; EXIT=0, 3 paths, three-dot semantics). Every exit code captured with redirect-then-capture, never through a pipe. 17 families + the ratchet, ALL EXIT=0 at d0d9ef99c: nul-bytes 0 | changeset-gate-self-tests 0 | objectui-changeset 0 | slot-lookup 0 | test-source-alias 0 | type-source-resolution 0 | adr-0087-registration 0 | changeset-no-major 0 | ci-filter-parity 0 | empty-changeset 0 | plugin-teardown-shape 0 | affected-docs 0 | engine-double-contract 0 | where-matcher 0 | query-options-erasure 0 | type-check-coverage 0 | i18n 0 | check:type-check-debt --re-measure 0.\n\nRATCHET REDDENED AND WAS FIXED IN THE CODE, NEVER THE BASELINE. check:type-check-coverage --re-measure first reported, in its own verdict line: '@objectstack/plugin-approvals: TEST_DEBT records 348 raw tsc error(s), tsc --noEmit now reports 349 (+1)'. Invisible to the package's own typecheck, which excludes **/*.test.ts. Isolated by re-running tsc over the package with the test exclusion lifted: exactly one error named my file — 'error TS2550: Property at does not exist on type any[]' (Array.prototype.at against this package's lib: ES2021). Fixed by indexing from the end by hand. Re-measured: 348 raw errors, ZERO naming my file — back to the frozen number. Final gate verdict line: '--re-measure: OK — 33 ledger entr(ies) re-measured in 271.2s, 1908 raw tsc error(s) total, none above its recorded number'. The ledger was not raised and lib was not widened.\n\nNOT-MEASURED CAUGHT AND NOT REPORTED AS A PASS. check:i18n first returned EXIT=1 with 'PREREQUISITE NOT MET — the workspace CLI is not built' / 'Nothing was checked'. Treated as NOT MEASURED. Built the closure (pnpm exec turbo run build --filter './packages/*' --filter './packages/*/*' -> 'Tasks: 70 successful, 70 total', VERDICT command-exit 0) and re-ran: EXIT=0, 'check-i18n-bundles: OK (9 package(s) — all bundles in sync, no undeclared authoring keys)', explicitly naming 'plugins/plugin-approvals in sync (4 bundle(s))'. Working tree clean after that build — no regenerated artifact drift, no packages/spec anchor rewrite to consider.\n\nDISCLOSURE DISCIPLINE: no reproduction recipe in the branch, the PR title or body, any commit message, any code comment, the changeset, or this report. Every artifact states the PROPERTY (the snapshot column is not a free-text predicate for a caller who reads it redacted) and never the steps. Control-byte self-scan (grep -naP over the escaped control range) run on all three changed files and on the PR body before posting: clean.",
      "open_questions": [],
      "out_of_scope_findings": []
    }

    Generated by Claude Code

  6. os-warren commented on Aug 22, 2026

    @os-warren
    CollaboratorAuthor

    PM review of PR #11103. Accepted. Reviewed against the branch by content, not against the report.

    The binding invariant holds literally, not nominally

    The dispatch's top rule was that the filter's notion of "this caller's view is redacted" must come from the same authority and the same per-caller computation as the serve path — because two derivations drift, and that drift is this card's own defect reconstituted one layer down. Verified by reading both sites:

    serve predicate
    call resolveReadableSnapshotFields(authority, object, context, this.logger) the same call, same argument shape
    asked as the caller the caller
    the branch it keys on if (readable === undefined) continue; return readable === undefined

    Same function, same arguments, same undefined-means-do-not-narrow sentinel. There is no second notion of "redacted" anywhere in the diff. The report's claim that schema-coverage comparison was deliberately not used is the right call and the reason is stated correctly: that comparison would have been the forbidden second derivation.

    ⭐ The ordering catch is the seat's, not the dispatch's

    I gave this seat the predicate-time tension (redaction is per-row; the filter is built before any row exists). I did not give it this:

    resolveReadableSnapshotFields also answers undefined for a BLANK object name, and reading that as "not masked" would hand the unfiltered query an arm on exactly the grounds that make it unanswerable.

    Had the object-scope test run first, an unfiltered query would produce a blank object name → undefined → read as "not masked" → the snapshot arm survives. The hole would come back in the broadest branch of all — the query that spans every object — while every narrow query looked correctly fixed. The authority-absent test is ordered first and the comment says why, so the next reader cannot reorder it by accident.

    This is the kind of finding that only comes from reading the seam's own behaviour rather than its name.

    The narrowing direction was taken where it was allowed, and declared where it costs something

    Dropping an arm is strictly narrowing — it never denies a query and never widens what comes back — so the fail-closed direction is cheap here, and the seat took it for the unfiltered case rather than building a per-object predicate machine to avoid it.

    It also declared the consequence rather than leaving it implied: with an authority wired, the arm drops for every caller the mask is in force on, including one whose mask happens to be empty for a given row. That is a real search-semantics narrowing for those callers, it is stated in the changeset as deliberate, and the alternative is the forbidden second derivation. Correct trade, honestly labelled.

    ⭐ And the corollary held in the other direction too: authority absent keeps the arm in both scopes, because serve narrows nothing when the authority is absent. That is consistency-with-serve, not blanket fail-closed — the seat did not "harden" the absent case into a behaviour serve itself does not have.

    The empty-$or trap is pinned, not merely avoided

    Four sys_approval_request arms are unconditional and a dedicated test asserts arms.length === 4 with each carrying { $contains: q }, plus a test that no $or is pushed at all when there is no free-text term. An empty $or: [] reads as match-none on some drivers and match-all on others; that is now a pinned property rather than a fact about today's code.

    ⛔ A ratchet reddened and the code was fixed, never the baseline

    check:type-check-coverage --re-measure reported, in its own verdict line:

    @objectstack/plugin-approvals: TEST_DEBT records 348 raw tsc error(s), tsc --noEmit now reports 349 (+1)
    

    Two things worth recording. First, the package's own typecheck could not see it — it excludes **/*.test.ts, so the new error was invisible to the check a developer runs locally and only the ledger re-measurement caught it. Second, the error was TS2550: Property at does not exist — Array.prototype.at against this package's lib (ES2021). Two evasions were available: raise the ledger by one, or widen lib. Neither was taken — the index was written from the end by hand and the number returned to 348 with zero errors naming the new file.

    check:i18n first returned PREREQUISITE NOT MET … Nothing was checked. Treated as NOT MEASURED, closure built (70/70 successful), re-run to a real verdict naming plugins/plugin-approvals in sync (4 bundle(s)). That is the correct handling of the named trap and it was not reported as a pass in between.

    Ablation attributable rather than blanket

    6 red of 13 under the mutation, and the 6 are exactly groups (1) narrowed and (2) still-searches; the 7 green are exactly groups (3) unmasked-unchanged and (4) same-authority — behaviour the mutation does not change. A blanket red would have proved much less. Restore was a git checkout from the commit (the fix was committed before ablating), marker absent by grep, git diff --stat HEAD empty, and re-run to a real verdict: 13/13.

    Incidental change correctly refused

    A 100644 → 100755 mode flip on packages/create-objectstack/bin/create-objectstack.js, produced by pnpm install, was reverted rather than carried — and the report names the already-open PR #11075 that owns it. Carrying it would have put this PR in another lane's path for no reason.

    Disclosure discipline

    This card is in the auth/authz carve-out family. Every artifact states the property — the snapshot column is not a free-text predicate for a caller who reads it redacted — and none states the steps. Confirmed across the branch, the changeset, and the commit messages. Full reproductions stay in QA session #10663.

    Scope

    Three files, no packages/spec, no content/docs/releases/**, Clause-② correctly declared no (buildRequestWhere became async but is private, and both call sites already were, so no public signature moved). Serial constraint resolved by diffing all 8 open PRs against their merge base: zero touches on approval-service.ts.

    CI convergence is mine to read; I am subscribed and will verify by job name.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions