Implement atomic attachment upload with metadata (#106) - #122
Closed
cuibonobo wants to merge 5 commits into
Closed
Conversation
#106) ScopedStack.create() applied no special check for _attachment@1, so a non-owner holding nothing but a bare create grant on the type — every uploader, by design — could name an arbitrary guessed fileId and, via canAccessFile()'s uploader clause, turn a correct guess into a read. putAttachment() was always the safe primitive (it derives fileId from bytes it just hashed, so possession is proven by construction); this closes the other path. - ScopedStack.create() refuses _attachment@1 for non-owners, with a carve-out: a readable record already referencing the fileId may get a second metadata record (e.g. a second filename) without re-uploading — never via the uploader clause, which would reintroduce the same circularity. - ScopedStack.putAttachment() now omits entityId for owner uploads, matching the normalization create() already applies (E1). - The mimeType-conflict validation error no longer names the established mimeType, closing a secondary anti-oracle leak. - Updates docs/spec.md (§Attachments, §API Adapter Wire Format) and @haverstack/conformance-fixtures to match: POST /attachments is documented as creating the _attachment@1 record atomically (the non-owner-safe combined primitive, not an efficiency optimization), and generic POST /records for _attachment@1 is owner-only. Scope note: the atomicity portion of #106 (folding bytes+metadata into one local-adapter transaction, closing the bare-bytes orphan window and the F3 mimeType race) depends on #112 and is deliberately not included here, per the issue's own sequencing. @haverstack/adapter-api is unchanged in this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LSNFMXRWseS34w8U2rt1u5
…T /attachments endpoint (#106) Follow-up to the ScopedStack.create() fix: POST /attachments now creates the _attachment@1 record atomically, but Stack.putAttachment() (client-side, wrapping APIAdapter) still made two separate wire calls under the hood — a bytes-only POST /attachments followed by a generic POST /records, which is now refused for non-owners. That left non-owner grantees using the SDK remotely with no working upload-with-metadata path. - StackBlobAdapter gains a required (not optional/capability-flagged) putAttachmentWithMetadata(data, mimeType, filename?) returning { fileId, record? }. Local storage adapters (disk, sqljs, memory) can't create a record themselves — a different backend — so they return no record; Stack.putAttachment() falls back to its existing create() call, unchanged. APIAdapter is the one implementation that populates record, via a single POST /attachments request. - Stack.putAttachment() calls the new method first and skips its own create() call whenever a record comes back, avoiding a redundant, potentially conflicting second write. - APIAdapter.putAttachment() (bytes-only) and putAttachmentWithMetadata() both parse the record POST /attachments now returns. - Wires @haverstack/conformance-fixtures' attachmentUploadFixtures into adapter-api's conformance test suite. - Updates docs/spec.md to describe the SDK's use of the endpoint and the resulting putAttachmentBytes()-over-HTTP quirk. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LSNFMXRWseS34w8U2rt1u5
…adapters The trivial local-adapter implementations from the previous commit declared narrower types than the StackBlobAdapter interface: return types omitted the optional `record` field, and mimeType/filename params were dropped entirely from the disk and in-memory adapters. Both were structurally assignable to the interface (fewer required params, a return subtype), so they built and passed vitest — which transforms test files without full type-checking — but failed `tsc --noEmit` on the concrete class types: mocking a record in a test, or calling with the full argument list, doesn't type-check against a narrower concrete signature. Also fixes an adapter-api test-only cast where WireRecord | WireError didn't overlap enough with the narrower shape being asserted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LSNFMXRWseS34w8U2rt1u5
…achmentWithMetadata() Both methods hit the same POST /attachments endpoint and got back a full record; putAttachment() had its own separate uploadBinary()+ parseRecord() call instead of reusing putAttachmentWithMetadata()'s, discarding the record for no reason other than that it was written before the second method existed. Now it's a two-line wrapper: call the richer method with the default mimeType, keep the fileId. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LSNFMXRWseS34w8U2rt1u5
…ithMetadata The method's name promised metadata gets applied, but that's only ever true for the API adapter — every local storage adapter (disk, sqljs, memory) silently ignores mimeType/filename, since it has no access to record creation (a different backend). The `try` prefix makes that explicit: callers must check `record` in the result, not the method name, to know whether metadata was actually used. Pure rename, no behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LSNFMXRWseS34w8U2rt1u5
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
2 participants
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
This PR implements issue #106, which adds a new
putAttachmentWithMetadata()method to the blob adapter interface and refactors attachment handling to support atomic record creation during upload. The changes close a security vulnerability where non-owners could guess fileIds and create metadata records without proving possession of the bytes.Key Changes
New adapter method: Added
putAttachmentWithMetadata(data, mimeType, filename?)toStackBlobAdapterinterface. This method is required on all adapters but has different implementations:{ fileId }with no record, allowingStack.putAttachment()to fall back to its owncreate()call{ fileId, record }from a single atomicPOST /attachmentsrequest, preventing the security issueNon-owner _attachment@1 refusal: Added strict permission checks in
ScopedStack.create()to refuse non-owner attempts to create_attachment@1records via generic create(), with one carve-out:Anti-oracle protection: Modified mimeType conflict error messages to never reveal the established mimeType, preventing confirmation oracles that could leak information about existing fileIds
API adapter updates:
POST /attachmentsnow returns the full created_attachment@1record instead of just{ fileId }putAttachmentWithMetadata()implementation with proper Content-Type and Content-Disposition header handlingputAttachment()to extract fileId from the returned recordStack.putAttachment() refactoring: Now delegates to
adapter.putAttachmentWithMetadata()first, skipping its owncreate()call when the adapter returns a record (atomic path), otherwise falling back to the pre-existing create() behaviorTest coverage: Added comprehensive tests for:
Notable Implementation Details
POST /attachments) rather than asserted by the callerhasReadableReference()helper method explicitly excludes the uploader clause to prevent circularityputAttachmentWithMetadata()but return no record, maintaining backward compatibility with the existingStack.putAttachment()fallbackPOST /attachmentsincluding the new record response formathttps://claude.ai/code/session_01LSNFMXRWseS34w8U2rt1u5