Gate reference-creation on file/read access in ScopedStack (#51)#98
Merged
Conversation
#51) ScopedStack.create() forwarded parentId, associations, and file-ref content fields from the caller unchecked, so a bare create grant let a requester attach a private file (and read it back through the reference) or plant relationship/parentId links to records they can't read. associate() had the same hole for attachment associations. Both now require what possessing the reference would grant: file access for attachment associations and file-ref fields, read access to the target for relationship associations and parentId. tag associations and _group roster associations (gated more strictly by #58's admin check) are exempt. A missing target and an existing-but-forbidden one always produce the same StackPermissionError, so the check can't become an existence oracle. Extends the fix to file-ref content fields (#63) since they convey attachment access identically to attachment associations — the same vulnerability class #51 describes, just added after #51 was filed. getAttachment() now shares its access predicate with the new gate instead of duplicating the logic. permissions and appId are intentionally left ungated at create time, per the issue's resolution: permissions is already owner/creator territory via setPermissions(), and appId is self-reported metadata everywhere, not a permission input. Refs #58 (_group roster exemption), #63 (file-ref reference parity), #50 (uploader-check pagination this depends on, already landed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDmUW3oNxPSCLf2ftqaF8q
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ScopedStack.create()forcedentityIdto the requester but forwardedparentId,associations, and permissions-adjacent options straight through unchecked. The sharpest hole: attachment access is reference-implies-access (getAttachment()succeeds if the requester can read any record referencing afileId), but nothing gated creating such a reference — a requester holding only a barecreategrant could attach a private (or guessed-hash) file'sfileIdto their own record and read it back, including using success/failure as a confirmation oracle for guessed content hashes.canAccessFile(fileId)— the dual ofgetAttachment()'s access rule: owner, uploader, or already-readable-referencer.getAttachment()now calls this instead of duplicating the logic.canReadReferent(recordId)— read access to aparentIdorrelationshipassociation target.create()andassociate()now gateattachmentassociations and file-ref content fields oncanAccessFile, andrelationshipassociations /parentIdoncanReadReferent.tagassociations carry no reference and are never gated.StackPermissionError— indistinguishable, so the check itself can't leak existence._grouproster associations (member/adminrelationship links) are exempt from the relationship-target check: theirrecordIdnames an entity, not a readable record, and roster mutation is already gated far more strictly by Group roles are decorative: admin should mean "can manage the group" #58's admin-or-owner rule.create()/update()): file-ref field kind: content-held file references are invisible to the reference checks the spec says exist #63 landed after ScopedStack.create() passes caller options through unchecked — attachment-association escalation, parent/app spoofing #51 was filed and made a file-ref field convey attachment access identically to anattachmentassociation — same vulnerability class, same fix.permissionsandappIdare deliberately left ungated at create time, per the issue's own resolution:permissionsis already owner/creator territory viasetPermissions(), andappIdis self-reported, untrusted metadata everywhere, not a permission input. Both documented in the spec rather than code-gated.docs/spec.mdupdated: new "Reference-creation gating" paragraph under Permissions/Enforcement, a pointer from the Associations section, and theappId/permissionspolicy statements.Depends on #50 (uploader-check pagination fix — already merged).
Test plan
packages/coretypecheck cleanpackages/corelint cleanpackages/coretest suite passing (408/408), including new coverage:create(): rejects inaccessible files, allows uploader/owner/already-readable-referencer, tag associations ungated, owner exemptparentIdgating oncreate(): same allow/reject/anti-oracle/owner-exempt coverage_grouproster associations remain exempt from the relationship-target checkassociate()gating for attachment and relationship associations on an already-owned recordcreate()andupdate(), including the untouched-field-in-a-patch carve-outGenerated by Claude Code