Conversation
d4024b1 to
f797e0f
Compare
f797e0f to
75af1ea
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Exclude * from queried principals in Effective and add the corresponding test.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a pure bucket-policy package for validation, canonical ETags, permission evaluation, and change detection.
Changes:
- Defines policy documents, statements, effects, and validation rules.
- Implements canonical JSON, SHA-256 ETags, effective permissions, and change tracking.
- Adds table-driven tests.
File summaries
| File | Summary |
|---|---|
pkg/policy/policy.go |
Implements policy validation, canonicalization, evaluation, and change detection. Moderate issue (2 votes): Effective treats * as a queryable principal. |
pkg/policy/policy_test.go |
Tests policy validation, canonicalization, ETags, evaluation, and change detection. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
75af1ea to
8c144d5
Compare
pyropy
changed the title
2821817 to
7ecf429
Compare
7ecf429 to
1133cac
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1133cac89e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1133cac to
6c425fc
Compare
a981176 to
fac3ea6
Compare
fac3ea6 to
6dc5b91
Compare
6dc5b91 to
fac3ea6
Compare
fac3ea6 to
48cfce0
Compare
48cfce0 to
aa5a4df
Compare
816e5d7 to
385868a
Compare
385868a to
a038ddb
Compare
a038ddb to
d2aa4d1
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2aa4d164b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dec := json.NewDecoder(bytes.NewReader(data)) | ||
| dec.DisallowUnknownFields() | ||
| var d Policy | ||
| if err := dec.Decode(&d); err != nil { |
There was a problem hiding this comment.
When an otherwise valid policy contains "sid": null, encoding/json silently leaves the string field empty rather than reporting a type error. Validate does not inspect Sid, so Decode accepts this schema-invalid value and Canonical then omits it entirely instead of producing the documented 422 error; explicitly reject null for sid while still allowing the property to be absent.
Useful? React with 👍 / 👎.
Adds pkg/bucketpolicy, a pure package with no store or transport dependencies. Validate applies the RFC's 422 rules: at least one statement, a recognized effect, non-empty principals and actions, actions from the policy vocabulary (s3perm.PolicyAction), and named principals that exist for the tenant. Canonical produces compact JSON in a fixed field order with input order kept, and ETag is the quoted hex SHA-256 of that encoding, so stored tags compare with If-Match values. Effective computes a principal's actions on a bucket: the union of Allow statements naming it or the wildcard, minus the union of Deny statements naming it or the wildcard. Named lists the principals a policy names and whether it uses the wildcard. Changed lists the principals whose effective set differs between two policies, expanding the wildcard to the tenant's principals, which is the set a policy write must invalidate. The wildcard is not itself a principal: Effective is nil for "*", so only Changed expands it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A principal list naming an id twice decoded as given and was stored that way. Decoding now keeps the first occurrence of each id, so every write path that goes through Decode, the policy API and the CreateBucket header alike, stores and tags the filtered document. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…r CID The statement, principal and action lists are sets: Decode and Canonical deduplicate and sort them, and Hilt stores and returns the sorted document. The ETag is the CID (v1, dag-cbor, sha2-256) of the DAG-CBOR encoding of that document, so two documents that differ only in order or in duplicates carry the same tag. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
d2aa4d1 to
98dde85
Compare
Adds
pkg/bucketpolicy, the pure policy document package: validation, evaluation, and a canonical form whose lists are sets. The ETag is the CID of the document's DAG-CBOR encoding.Decode,Validate,Canonical,ETagEffective(allow minus deny),Named,Changed🤖 Generated with Claude Code