fix(policy): restore parsing of star-prefixed resources - #251
Conversation
Skipping the "*" entry while looping the ARN prefixes made every string
that merely starts with "*" fail to parse. "**", "***" and "*foo" all
returned `invalid resource`, which broke LoadPolicy for any stored
policy using one and surfaced during IAM authorization as
Error: invalid resource '**' (*grid.RemoteErr)
internal/grid/fanout.go:127:cmd.(*NotificationSys).LoadPolicy
The skip was added so a bare ARN prefix would stop parsing as the
wildcard. That still holds without it: the exact string "*" is
special-cased ahead of the loop so its pattern stays "*" rather than the
empty remainder, and a bare prefix like "arn:aws:s3:::" matches its own
entry rather than the wildcard.
Regression test covers the parse and a whole policy carrying "**"
loading and matching.
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthrough
ChangesWildcard resource parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@policy/resource.go`:
- Around line 319-325: In policy/resource.go lines 319-325, replace the detailed
parsing-history comment with a brief explanation that the exact "*" branch is
handled separately so its pattern remains "*". In policy/memory-resource_test.go
lines 730-736, remove the failure-history comment or reduce it to a concise
regression rationale; make no other changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb43dcb4-2bd2-458f-b79e-8effab8e4d58
📒 Files selected for processing (2)
policy/memory-resource_test.gopolicy/resource.go
A Memory list names a query, not a record, so the prefix cannot live in the resource path. One ARN would mean a single record under memory:GetAgent and every sibling sharing that prefix under memory:ListAgents, and an operator writing both actions against one resource would leak names with no cue that the string had changed meaning. Both keys are accepted only on the list actions. A read or a write has no query to constrain, so accepting them there would let a policy look scoped while constraining nothing -- the mistake now surfaces when the policy is written rather than as a grant that silently does nothing. Also add a corpus of every resource form the parser accepts. ParseResource is shared by every ARN type, so a change made for one narrows what the others take -- which is how the star-prefix regression reached main through a green suite. Adding a form is a deliberate widening; removing one breaks stored policies and now has to be argued rather than discovered downstream. The corpus found a pre-existing quirk on its first run: String() renders the wildcard as its prefix plus its pattern, so "*" serializes to "**". That is where the doubled form in stored policies comes from, and why rejecting it broke real deployments rather than an exotic hand-written value. The round-trip assertion checks stability rather than byte identity and records the reason.
1 participant
Regression from #249.
Skipping the
"*"entry while looping the ARN prefixes made every string thatmerely starts with
*fail to parse:That breaks
LoadPolicyfor any stored policy using one. It surfaced inaistor CI as an IAM authorization failure:
The skip existed so a bare ARN prefix would stop parsing as the wildcard. That
property is unaffected: the exact string
"*"is special-cased ahead of theloop so its pattern stays
"*"rather than the empty remainder, and a bareprefix such as
arn:aws:s3:::matches its own entry rather than the wildcard.Verified by mutation — restoring the skip fails the new test on all three
inputs.
make lintclean, full suite green.Summary by CodeRabbit
*,**,***, and*foo.