fix(core): write results respect operation-level query access by borisno2 · Pull Request #1770 · OpenSaasAU/stack

borisno2 · 2026-10-07T08:12:32Z

Summary

A create/update/delete result now consults the list's operation-level query access. If the session cannot query the written row (rule is false, or a filter the row doesn't match), only the list's system fields are returned; the write still persists.
update is checked against the row as written; delete against the row before deletion. sudo() is unchanged, and no change to whether a write is allowed.
The filter case is evaluated as a scoped read of the row's id in the same transaction, reusing the read path's plan resolution. A true rule issues no extra statement.

Test plan

New tests: query: () => false create returns { id } and persists; filter rule own vs. other's row; update moving out of scope; delete of an unqueryable row; sudo unchanged
packages/core context and secured suites pass
pnpm format / lint
Admin UI and MCP flows were not exercised against a query-denied list; they only need id from the result, but I haven't verified that.

Closes #1686

🤖 Generated with Claude Code

https://claude.ai/code/session_01PNc1LCUFSBwEKyd7EcbupM

Generated by Claude Code

Closes #1686 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PNc1LCUFSBwEKyd7EcbupM

changeset-bot · 2026-10-07T08:12:37Z

🦋 Changeset detected

Latest commit: 8b38761

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages

Name	Type
@opensaas/stack-core	Patch
@opensaas/stack-auth	Patch
@opensaas/stack-cli	Patch
@opensaas/stack-rag	Patch
@opensaas/stack-storage	Patch
@opensaas/stack-tiptap	Patch
@opensaas/stack-ui	Patch
@opensaas/stack-storage-s3	Patch
@opensaas/stack-storage-vercel	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2026-10-07T08:12:39Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
stack-docs	Ready	Preview	Oct 7, 2026 10:51am UTC

chatgpt-codex-connector · 2026-10-07T08:12:42Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-07T08:19:27.846956Z	`a521384`	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

borisno2 · 2026-10-07T08:13:14Z

Code review for PR #1770 (write results respect operation-level query access, #1686)

Findings, most important first.

Partial object returned instead of null (packages/core/src/secured/read.ts ~L1303). When the session has write access but cannot query the row, the write resolves to an object holding only system fields, such as { id }. The static return type is still the full item. The documented contract is that a denied operation returns null. create(...) returns { id }, so post === null is false and post.title is undefined at runtime while the type says string. visibleWrittenRow does not model this partial shape.
Update and delete evaluate queryability at different times (packages/core/src/context/write-pipeline.ts ~L605). Delete checks before the delete. Update checks after the write. With query: { published: { equals: true } } and an update that sets published: false, the caller gets { id } back instead of the row they just changed. Update should arguably use the pre-write row, as delete does. Please decide which is intended and document it.
Extra query on every write (write-pipeline.ts ~L532). writtenRowQueryable runs on every create, update and delete, even when query access is plain true. Whenever a filter rule exists it adds a SELECT id ... first() inside the transaction. That roughly doubles round trips for bulk writes and holds row locks longer. Please skip it when query access is unconditional.
Strict id type check fails closed (read.ts ~L1264). Any id that is not a string or number counts as not queryable. A list whose id codec yields another type (bigint, for example) would have every write return system fields only, with no error. Please base the check on the contract's id codec.
Fragile coupling to resolvePlan (read.ts ~L1281). plan.predicates.length === 1 stands in for "no access filter was added". If resolvePlan later adds another engine predicate, the shortcut breaks. It would then either skip the scoped read and leak fields, or never short-circuit. Please make the plan expose this explicitly.
Duplicated read path (read.ts ~L1259). writtenRowQueryable builds its own QueryState and scoped collection, partly copying first(). Changes to QueryState or FIRST_DISPOSITIONS must now be mirrored by hand, and the two can drift. Please reuse the first() terminal.
Delete result diverges from hooks, and delete lacks test coverage (write-pipeline.ts ~L631). When the row is not queryable, delete returns only { id }. afterOperation hooks still receive the full item. MCP delete tools and admin UI callers may echo undefined fields. The changeset says this applies to delete, but I could not find a test for the delete path. Please add one.

Overall: the direction is right, but findings 1 and 2 change the public contract (null versus partial object) and should be resolved before merge.

Generated by Claude Code

borisno2 · 2026-10-07T08:13:30Z

Response to the review, no code change:

1 (partial object, not null): by design. Issue A write's returned row ignores operation-level query access, leaking rows the session cannot read #1686's triage decision specifies system fields only, because null reads as "denied" and the write succeeded. The TSDoc on visibleWrittenRow notes the narrowed case.
2 (update checked after write): by design. The spec says an update is checked against the row as written, so one that moves a row out of scope returns system fields only. Delete is checked before because the row is gone afterwards.
3 (extra query): a true rule adds no statement (plan.predicates.length === 1 short-circuits). Only filter rules add one scoped SELECT, which the spec requires.
4 (id type): ids are uuid7, cuid2 or integer, so string or number covers all of them.
5, 6 (coupling to resolvePlan, duplicated read path): not worth reworking here. The helper reuses resolvePlan, scope and the engine origin, so it follows the read path's access resolution.
7 (delete coverage): write-result-query-access.test.ts has a delete case. On the afterOperation point, hooks see the real row by design, since only the caller's return value is narrowed.

Generated by Claude Code

borisno2 · 2026-10-07T08:14:16Z

The test job fails at "Check for duplicate ADR numbers", and every later step is skipped. This PR doesn't touch docs/adr. main at de2bd7a already has two ADR 0074 files, from #1767 and #1768:

0074-a-throwing-after-transaction-hook-is-reported-never-propagated.md
0074-field-write-access-gates-caller-input-not-hook-output.md

Fixing it means renumbering one of them and updating its references, which is outside this PR's scope. I haven't found a fix PR yet. Once main is fixed I'll merge it in and re-run. Locally, the packages/core context and secured suites pass on this branch.

Generated by Claude Code

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a521384c74

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-10-07T08:19:25Z

+  const queryable = await writtenRowQueryable(binding, item.id)
+
  // ── Phase 8: DB delete ──────────────────────────────────────────────────────
  const deleted = await strategy.persist(collection, ops, scope, {})


Bind query access to the row actually deleted

Under PostgreSQL's default Read Committed isolation, this queryability read and the subsequent delete are separate statements without a row lock. If the row initially matches a query filter such as ownerId = session.userId, a concurrent transaction can change both the owner and sensitive fields before the delete acquires its lock; delete() then returns the newly modified row, but the stale queryable = true causes all of those fields to be exposed. Lock the target across the check and delete, or otherwise derive visibility atomically from the row actually deleted.

AGENTS.md reference: AGENTS.md:L123-L127

Useful? React with 👍 / 👎.

The race is real but narrow, and I'm leaving it as is. A session reaches it only if it holds delete access to the row, the row matches its query filter at the check, and another transaction moves it out of scope before the delete locks it. In that case delete() returns the row as the other transaction left it. Before this PR a delete returned the full row unconditionally, so this PR only narrows what a caller can see, and update and create are checked after the write under the transaction's own row lock. Closing the gap for delete would mean taking a forUpdate lock on the target before the check, in every delete. I'd rather not add that without the maintainer asking for it.

Generated by Claude Code

github-actions · 2026-10-07T11:07:13Z