Skip to content

Core code treats one query() page as the complete result set (grants, attachment metadata, uploader checks) #50

Description

@cuibonobo

Problem

query() is paginated: when no limit is passed, adapters return a default page (50 in record-adapter-sqljs and MemoryAdapter) plus a cursor. But several internal call sites in core treat a single query() call as exhaustive — they never follow the cursor. Everything works in small test stacks and silently breaks past the page-size threshold.

This is a design-level trap, not a one-off: the internal code assumes "query = all matches" while the adapter contract says "query = one page." Fixing the individual sites without naming the rule invites the next occurrence.

Affected sites

1. Grant prefetch in ScopedStack.query() (stack.tsprefetchedGrants)

const prefetchedGrants = this.requesterEntityId
  ? (await this.stack.query({ filter: { typeId: `${SYSTEM_TYPES.GRANT}@1` } })).records
  : undefined;

No limit, no cursor walk → only the first 50 _grant@1 records are considered. A stack with more than 50 grants silently denies permissions that exist beyond page one. Fails closed (no escalation), but legitimate access disappears with no error — and which grants land in the first page depends on sort order, so behavior shifts as grants are added.

2. hasGrant() non-prefetched path — same shape: one unpaginated query. Mitigated when contentFieldQuery narrows to the target type, but on adapters without it, all grants are fetched (first page only) and filtered in memory.

3. Attachment metadata collection in Stack.deleteAttachment()

The _attachment@1 metadata query takes the default page. With more than 50 metadata records (or on a non-contentFieldQuery adapter, more than 50 attachment records total), some metadata records are missed → the bytes are deleted while orphaned _attachment@1 records survive, pointing at a file that no longer exists.

4. Uploader check in ScopedStack.getAttachment() — worst instance, breaks below 50 too

const uploadResult = await this.stack.query({
  filter: { typeId: `_attachment@1`, entityId: this.requesterEntityId, /* content filter only if capable */ },
  limit: 1,
});

On adapters without contentFieldQuery, this fetches one of the requester's attachment records and then checks in memory whether it matches the fileId. A requester who has uploaded more than one file gets denied access to any upload that isn't first in sort order. False denial with as few as two uploads.

Why tests didn't catch this

MemoryAdapter (the ScopedStack test double) ignores most filters — content, attachmentFileId, tags, relatedTo, date ranges — returning unfiltered results. The permission tests pass without exercising the real predicates, and no test crosses the 50-record threshold.

Proposed fix

1. Name the rule in the spec

Add to the Queries section: query() always paginates. An absent limit means one adapter-default page (spec the default: 50), not "everything." Any caller that needs the complete result set MUST follow cursor to exhaustion. Currently the spec doesn't state what an absent limit means, which is how the ambiguity crept in.

2. Internal queryAll helper in core

/** Follow cursors to exhaustion. Internal — for correctness-critical scans. */
private async queryAllPages(query: StackQuery, opts?: { max?: number }): Promise<StackRecord[]>

Cursor-walks with a generous safety cap (throw or warn loudly on hitting it — never silently truncate, that's the current bug with extra steps). migrateAll() already does this correctly and can share the helper.

Use it at sites 1–3. For site 4, drop limit: 1 on the incapable-adapter path and cursor-walk the requester's attachment records (or better: filter by attachmentFileId-equivalent semantics in memory across all pages).

3. Keep the capability fast-path

Where contentFieldQuery is available, the narrowed query stays (grants for one typeId, metadata for one fileId) — those result sets are small and the walk terminates immediately. The cursor walk is the correctness backstop, not a perf regression.

4. Test fidelity

  • MemoryAdapter implements the full filter set (content, attachmentFileId, tags, relatedTo, appId, date ranges, and payload-aware dissociate) so permission logic is tested against real predicates. Its "fully functional" doc comment currently overpromises.
  • Add threshold tests: >50 grants (grantee's access must survive), >50 attachment metadata records (deleteAttachment leaves no orphans), multi-upload uploader-access check (site 4, catches the bug at n=2).

Non-goals

  • Changing the public query() contract — pagination stays; apps that want everything follow cursors like today. This issue is about core's own internal consumers honoring the contract core defines.
  • The ScopedStack.query() refill loop itself — it already walks cursors correctly; only its grant prefetch is affected.

Refs #48 (adapter atomicity work touches the same code paths), and the design-review finding that MemoryAdapter's filter gaps mask permission-logic bugs.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions