Skip to content

installAttachmentAccessHooks does not authorize an UNSCOPED multi-delete: no id + no where reads as "nothing to authorize" and deleteMany runs over the whole table #4757

Closed
#4780
Closed
installAttachmentAccessHooks does not authorize an UNSCOPED multi-delete: no id + no where reads as "nothing to authorize" and deleteMany runs over the whole table#4757
#4780
Assignees
os-zhuang
Labels
bugSomething isn't workingpm:dispatchedpm:queuesecurity

Description

@os-zhuang
os-zhuang
opened on Aug 3, 2026

Found while implementing #4630 (the symmetric record-level gates for sys_comment). Filed unassigned — the fix belongs to service-storage, not to that PR.

What

packages/services/service-storage/src/attachment-access-hooks.ts, the beforeDelete gate, resolves the rows a delete matches in two ways and then short-circuits when it found none:

const ids = asIdList(ctx?.input?.id);
if (ids) { /* resolve each by id */ }
else if (ctx?.input?.options?.where) { /* resolve the match set, bounded */ }
if (!rows.length) return;   // nothing matched — nothing to authorize

A delete with no id and no where takes neither branch, so rows stays empty and the gate returns allow. That is not "nothing matched" — nothing was ever queried.

The engine then treats the same call as a bulk delete over everything (packages/objectql/src/engine.ts, the delete path):

if (!id) {
  opCtx.ast = { object, ...(options?.where !== undefined ? { where: options.where } : {}) };
}
…
} else if (options?.multi && driver.deleteMany) {
  const ast = opCtx.ast;               // { object }  — no `where`
  result = await driver.deleteMany(object, ast, …);
}

So ql.delete('sys_attachment', { multi: true, context: <a real caller's context> }) reaches deleteMany with an unscoped AST.

Why the other layers do not catch it

  • plugin-sharing — buildWriteFilter returns null for an object with no owner field, and sys_attachment's provenance column is uploaded_by, not owner_id, so no row-scoping predicate is composed onto the AST.
  • plugin-security — the member_default baseline carries no allowDelete (ADR-0090 D5), so a rank-and-file member is refused by RBAC first. But an app that ships a domain grant with the delete bit on sys_attachment — which is exactly what the attachments panel requires, and what attachmentsFixture's att_attachment_manager models — passes RBAC and lands on the ungated path.

Scope

I have not checked whether the REST layer can produce this shape (its delete routes look id-bound); the reachable surfaces are the SDK / ObjectQL / flow delete_record-style callers that pass multi: true without a predicate.

Suggested fix

Fail closed instead of falling through: when there is neither an id nor a where, refuse rather than returning. #4630's sys_comment gate does this — see resolveTargetRows in packages/plugins/plugin-audit/src/comment-access-hooks.ts:

forbid(`Refusing an unscoped multi-${verb} of comments — scope the write to the rows you mean`);

sys_attachment should get the same posture (403 ATTACHMENT_DELETE_DENIED), plus a unit test alongside the existing attachment-access-hooks.test.ts cases. Worth a look at whether any other beforeDelete gate in the repo shares the "no rows resolved ⇒ allow" shape.

Activity

  1. os-zhuang
    added
    bugSomething isn't working
    security
    pm:queue
    pm:dispatched
    on Aug 3, 2026
  2. os-zhuang
    self-assigned this
    on Aug 3, 2026

  3. os-zhuang commented on Aug 3, 2026

    @os-zhuang
    os-zhuang
    on Aug 3, 2026 – with Claude
    ContributorAuthor

    认领:PM 循环第 3 轮
    会话:session_015Br2xsJsczFsTR9bvbh2Ny
    分支:claude/issue-4757-attachment-unscoped-delete
    Worktree:objectstack-issue-4757

    分诊:落地 objectstack(后端,packages/services/service-storage);按 issue 的建议修法(fail closed),无需额外裁定。

    这条是 #4630 的镜像面:那边给 sys_comment 立的 resolveTargetRows 已经是正确姿势(无 id 且无 where ⇒ forbid),这边照抄即可 —— 「无行可授权」与「从未查询过」是两件事,把后者当前者是典型的 fail open。

    范围约束(本轮并行批的隔离要求,请严格遵守):

    1. 修改限定在 packages/services/service-storage/。issue 正文引用了 packages/objectql/src/engine.ts 的 delete 路径,那是背景说明,不是改动目标 —— 本轮 bug(objectql): ADR-0104「空库即已迁移」自证写在首启 seed 之前 —— 部署证明了一个它同一次启动就违反的契约，第二次 pnpm dev 起永久 10 条 ERROR #4769 正在改 objectql,任何对 engine.ts 的改动都会撞车。若你判断确实必须动 engine,停下来在报告里说明,不要自行改。
    2. issue 末尾那句「Worth a look at whether any other beforeDelete gate in the repo shares the 『no rows resolved ⇒ allow』shape」——请做这个排查,但发现的其它 gate 另立 issue(Prime Directive chore: version packages #10),不在本 PR 修。这是本 issue 最有价值的部分:同一个 fail-open 形状很可能不止两处。

    维护者可否决本裁定;不阻塞派发。

    Generated by Claude Code

  4. os-zhuang commented on Aug 3, 2026

    @os-zhuang
    os-zhuang
    on Aug 3, 2026 – with Claude
    ContributorAuthor

    PM 复核:ACCEPT —— PR #4780(draft,targets main,Fixes 行正确)。

    落地内容:beforeDelete 对「无 id 且无 where」失败关闭,403 ATTACHMENT_DELETE_DENIED,不再穿过 if (!rows.length) return 短路。姿势与 #4630 给 sys_comment 立的 resolveTargetRows 一致。

    复核要点:

    • 范围干净:3 个文件(源码 / 测试 / changeset),全在 packages/services/service-storage/。我下的隔离约束守住了 —— objectql/engine.ts 只读未改(本轮 bug(objectql): ADR-0104「空库即已迁移」自证写在首启 seed 之前 —— 部署证明了一个它同一次启动就违反的契约，第二次 pnpm dev 起永久 10 条 ERROR #4769 正在改它),plugin-auth、service-automation、plugin-approvals 同样未碰。无 content/docs/releases/ 改动。
    • 测试证据是真的:dev 先回退源码、只留新测试跑了一遍,得到 5 failed | 16 passed,失败的正是 5 条新用例;打上修复后 21/21 全绿,全包 260/260。这一步是我最看重的 —— 它证明测试确实在测东西,而不是碰巧和实现一起绿。
    • 边界覆盖到位:除了拒绝路径,还正面锁住了四条合法路径不被误伤(按 id、带 where、where: {}、谓词跑了但零匹配)。其中 where: {} 的处理是这次做对的关键判断 —— 它匹配全表,但是一次真实查询,所以仍走逐行授权而非直接拒绝;把它错判成「无作用域」会把一个合法能力误杀。
    • changeset 里写了迁移说明(确需清表:传 { multi: true, where: {} } 逐行授权,或在 system context 下做)。

    超范围产出已核实存在,且质量超出预期 —— issue 末尾那句「看看还有没有别的守卫共享这个形状」的排查确实做了,检索面覆盖全仓 beforeDelete|beforeUpdate 命中的 48 个文件:

    CI 仍在跑(Test Core / Dogfood Regression / Temporal Conformance / 类型检查在飞;Build Core、Check Changeset、PR Size、同 issue 唯一 PR 检查均已绿)。全绿后我会标 ready 并送进合并队列,红则回报。

    Generated by Claude Code

  5. os-zhuang
    closed this as completedin #4780on Aug 3, 2026
  6. akarma-synetal
    added 3 commits that reference this issue on Aug 3, 2026

    fix(service-storage): refuse an unscoped multi-delete of sys_attachme…

    941dec4

    fix(approvals): hold the record lock for predicate (multi) updates (o…

    0b795da

    fix(plugin-sharing): recompute sharing rules for predicate (multi) wr…

    c272e48
  7. os-zhuang
    added a commit that references this issue on Aug 9, 2026

    fix(plugin-sharing): an empty per-row union reads as unbounded, not a…

    39f78f9
  8. akarma-synetal
    added 2 commits that reference this issue on Aug 17, 2026

    fix(spec,objectql,sharing,storage): state per-row vs record dispatch …

    fc3a36a

    test(service-storage): resolve `@objectstack/core` from source so a s…

    93be029
  9. os-zhuang
    added a commit that references this issue on Aug 23, 2026

    tests(attachments): pin clause C3 end to end and correct the ledger t…

    e3da4cd
  10. akarma-synetal
    added 2 commits that reference this issue on Aug 23, 2026

    fix(objectql): restore the objectstack-ai#4757 unscoped multi-delete …

    1258dca

    tests(attachments): pin clause C3 end to end and correct the ledger t…

    9ca1618
  11. akarma-synetal
    added a commit that references this issue on Oct 7, 2026

    docs(dogfood): re-anchor the dead tracker citations in packages/qa/do…

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

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingpm:dispatchedpm:queuesecurity

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions