Repository navigation
feat(s3): carry an optional body on the forwarded request - #87
Merged
Merged
Conversation
Hilt decides some requests on their body: the bucket policy PUT already travels with its document, and DeleteObjects needs the version permission when an entry names a version. The shared request type gains an optional body for the operations Hilt names, omitted from the encoding otherwise, so a request without one encodes as before. The codec caps it at 2 MiB; a DeleteObjects body is at most about 1.2 MiB. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
alanshaw
added a commit
to fil-forge/ingot
that referenced
this pull request
Oct 21, 2026
…ath on it Hilt now reads a DeleteObjects body and needs s3:DeleteObjectVersion when an entry names a version. The body travels only for the operations Hilt's classifier names, so a streaming write's body is never read here; the fast path checks the version permission from the same classification and leaves a body the signature does not bind to Hilt, which refuses it as UnboundBody. Pins libforge to fil-forge/libforge#87 and hilt to fil-forge/hilt#106. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The size limit rejects valid XML-escaped requests, and both decoders lose explicitly empty bodies.
3 open issues
2 files not reviewed
commands/s3/cbor_gen.go: Generated filecommands/s3/json_gen.go: Generated file
What changed in this PR
Adds an optional body to forwarded S3 requests so Hilt can authorize operations whose permissions depend on request content.
Changes:
- Adds
Request.Bodywith omission when unset. - Updates CBOR and DAG-JSON codecs with a 2 MiB body limit.
- Tests body round trips and omission.
| File | Description |
|---|---|
| commands/s3/types.go | Defines the optional body field. |
| commands/s3/json_gen.go | Adds DAG-JSON body encoding and decoding. |
| commands/s3/codec_test.go | Tests body serialization and omission. |
| commands/s3/cbor_gen.go | Adds CBOR body encoding and decoding. |
Review effort: Balanced
XML escaping expands a key up to sixfold, and an entry carries a version id and conditional fields beside it, so a thousand entries can run to about 7.3 MB. The field gets its own 8 MiB limit; everything else keeps the codecs' 2 MiB. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The previously reported empty-body round-trip issue remains unresolved in both decoders.
2 open issues
1 resolved since last review
2 files not reviewed
commands/s3/cbor_gen.go: Generated filecommands/s3/json_gen.go: Generated file
Review effort: Balanced
alanshaw
added a commit
to fil-forge/hilt
that referenced
this pull request
Oct 22, 2026
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
alanshaw
added a commit
to fil-forge/hilt
that referenced
this pull request
Oct 22, 2026
A DeleteObjects entry naming a version needs `s3:DeleteObjectVersion` in S3, and only the request body says so. Hilt classified the request as `s3:DeleteObject` on the bucket, and Ingot removed the named versions with no further check, so a key or principal holding the delete permission alone could permanently remove any version it could name, a thousand per request. The single-object DELETE with a versionId was already classified correctly. ### What changes The body now travels to Hilt for the operations whose body decides a permission, and for no other. `Operation.ReadsBody` names them, and today that is DeleteObjects alone; the bucket policy PUT keeps its document on its own command. Ingot attaches the body for those operations only, so PutObject and UploadPart never have theirs forwarded, and Hilt refuses a body on any other operation. The field is capped at 8 MiB by the codec and here, which holds the largest valid DeleteObjects document once XML escaping and the per-entry fields are counted, about 7.3 MB. Hilt binds the body to the signature before reading it, the same rule as for headers. A signed payload hash covers it directly. Under an unsigned payload, a signed `x-amz-checksum-*` or `Content-MD5` header covers it, which S3 requires on a DeleteObjects and the SDKs send signed. A body that is missing, unparseable, oversized or bound by neither is refused as `InvalidBody`. An entry naming a version, the null version included, adds `s3:DeleteObjectVersion` to the request's permissions on the bucket, checked for a service key and for a principal's effective set, and reported by `RequirementsFor` for Ingot's fast path. ### Behaviour changes to know about - A key or principal holding `s3:DeleteObject` without `s3:DeleteObjectVersion` can no longer delete named versions through DeleteObjects. Plain entries are unaffected. - Hilt decides the whole request: a batch mixing plain and versioned entries is refused outright when the version permission is missing, where S3 would delete the plain entries and refuse the rest inside one response. This fails closed and keeps the decision in one place; a client wanting the S3 shape splits its batch. - Ingot must forward the body for DeleteObjects; until it does, Hilt refuses every DeleteObjects as `InvalidBody`. The Ingot change lands with this. Pins libforge main, which carries fil-forge/libforge#87. The IAM itest scenario `DeleteObjectsVersionNeedsTheVersionAction` is added but was not run; neither were the checksum-header fallbacks exercised against a real SDK, only the payload-hash path that the CLI and SDKs use for this operation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
alanshaw
added a commit
to fil-forge/ingot
that referenced
this pull request
Oct 22, 2026
…ath on it (#299) Companion to fil-forge/hilt#106 and fil-forge/libforge#87, both merged; pins Hilt and libforge main. Hilt now decides a DeleteObjects on its body: an entry naming a version needs `s3:DeleteObjectVersion` beside `s3:DeleteObject`. Until now Ingot removed the named versions with no such check, so a key holding the delete permission alone could remove any version it could name. ### What changes The request forwarded to Hilt carries its body only for the operations Hilt's classifier names as deciding on one, which today is DeleteObjects. Every other request goes without, so PutObject and UploadPart never have their body read here. Hilt binds the body to the signature before reading it, and the fast path decides on the same classification: a version entry is refused locally when the key's cached set lacks the version permission, and a body the signature does not bind is left to Hilt, which refuses it as `InvalidBody`. That name joins `UnsignedHeader` in the error mapping as AccessDenied. ### Behaviour changes to know about - A DeleteObjects naming a version by a key or principal without `s3:DeleteObjectVersion` is refused as a whole, where S3 would refuse only those entries. Plain batches are unaffected. - A DeleteObjects whose body is not covered by the signature, as the payload hash or a signed checksum header, is refused. The SDKs and CLI sign the payload for this operation. The IAM itest scenario for this lives in Hilt and was not run. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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.
Hilt decides some requests on their body. The bucket policy PUT already travels to Hilt with its document on the policy command. DeleteObjects is the other one: an entry naming a version needs
s3:DeleteObjectVersion, and only the body says so. This gives the shared forwarded request an optional body for that.What changes
s3.Requestgains aBodyfield, omitted from both encodings when unset, so a request without one encodes exactly as it did before. The rule for when it is set lives in Hilt, which names the operations whose body decides a permission; Ingot attaches the body for those and no others, so the bulk streams of PutObject and UploadPart never reach it. The field carries its own 8 MiB limit, which holds the largest valid DeleteObjects document: a thousand entries of a kilobyte key expanded up to sixfold by XML escaping, a version id as long, and the conditional fields, about 7.3 MB. Everything else keeps the codecs' 2 MiB.The follow-ups in Hilt and Ingot pin this.
🤖 Generated with Claude Code