diff --git a/policy/bucket-policy_test.go b/policy/bucket-policy_test.go index d888770..da0dd7b 100644 --- a/policy/bucket-policy_test.go +++ b/policy/bucket-policy_test.go @@ -216,6 +216,32 @@ func TestBucketPolicyIsAllowed(t *testing.T) { } } +func TestBucketPolicyIsAllowedAnonymousPrincipalPattern(t *testing.T) { + args := BucketPolicyArgs{ + Action: GetObjectAction, + BucketName: "mybucket", + ObjectName: "myobject", + ConditionValues: map[string][]string{}, + } + for _, principal := range []string{"?alice", "*?alice", "?arn:aws:iam::123456789012:user/alice"} { + p := BucketPolicy{ + Version: DefaultVersion, + Statements: []BPStatement{ + NewBPStatement("", + Allow, + NewPrincipal(principal), + NewActionSet(GetObjectAction), + NewResourceSet(NewResource("mybucket/*")), + condition.NewFunctions(), + ), + }, + } + if p.IsAllowed(args) { + t.Errorf("principal %q: anonymous GetObject allowed, want denied", principal) + } + } +} + func TestBucketPolicyIsEmpty(t *testing.T) { case1Policy := BucketPolicy{ Version: DefaultVersion, diff --git a/policy/principal_test.go b/policy/principal_test.go index eb76db6..608f40f 100644 --- a/policy/principal_test.go +++ b/policy/principal_test.go @@ -100,6 +100,10 @@ func TestPrincipalMatch(t *testing.T) { {NewPrincipal("*"), "AccountNumber", true}, {NewPrincipal("arn:aws:iam::*"), "arn:aws:iam::AccountNumber:root", true}, {NewPrincipal("arn:aws:iam::AccountNumber:*"), "arn:aws:iam::TestAccountNumber:root", false}, + {NewPrincipal("*"), "", true}, + {NewPrincipal("?alice"), "", false}, + {NewPrincipal("*?alice"), "", false}, + {NewPrincipal("?alice"), "xalice", true}, } for i, testCase := range testCases { diff --git a/wildcard/match.go b/wildcard/match.go index 1e2db3a..0b60c3f 100644 --- a/wildcard/match.go +++ b/wildcard/match.go @@ -23,8 +23,9 @@ import ( // MatchSimple - finds whether the text matches/satisfies the pattern string. // supports '*' wildcard in the pattern and ? for single characters. -// Only difference to Match is that `?` at the end is optional, -// meaning `a?` pattern will match name `a`. +// Only difference to Match is that a `?` at the end is optional, +// meaning `a?` pattern will match name `a`. A `?` followed by anything +// other than `?` or `*` still matches exactly one character. func MatchSimple(pattern, name string) bool { if pattern == "" { return name == pattern @@ -35,10 +36,11 @@ func MatchSimple(pattern, name string) bool { if deepMatchRune(name, pattern) { return true } - // Reaching a '?' with name already exhausted succeeds, and succeeds for - // the whole pattern, so the pattern also matches whenever any prefix of it - // ending at a '?' consumes name exactly. - if !strings.ContainsRune(pattern, '?') { + // An optional '?' is one that only '?' and '*' follow, so the pattern also + // matches whenever a prefix of it ending at one of those consumes name + // exactly. + tail := len(strings.TrimRight(pattern, "*?")) + if strings.IndexByte(pattern[tail:], '?') < 0 { return false } // A prefix holding no '*' consumes exactly one byte of name per byte of @@ -53,7 +55,7 @@ func MatchSimple(pattern, name string) bool { // too long to consume name exactly and the walk can stop. lits := 0 for i := range len(pattern) { - if pattern[i] == '?' && (i > firstStar || i == len(name)) && + if pattern[i] == '?' && i >= tail && (i > firstStar || i == len(name)) && deepMatchRune(name, pattern[:i]) { return true } diff --git a/wildcard/match_equivalence_test.go b/wildcard/match_equivalence_test.go index 15df988..cf5d410 100644 --- a/wildcard/match_equivalence_test.go +++ b/wildcard/match_equivalence_test.go @@ -34,7 +34,7 @@ func oldDeepMatchRune(str, pattern string, simple bool) bool { } case '?': if len(str) == 0 { - return simple + return simple && strings.Trim(pattern, "*?") == "" } case '*': return len(pattern) == 1 || diff --git a/wildcard/match_test.go b/wildcard/match_test.go index f83e7cf..3867b0d 100644 --- a/wildcard/match_test.go +++ b/wildcard/match_test.go @@ -544,6 +544,22 @@ func TestMatchSimple(t *testing.T) { text: "a", matched: true, }, + {pattern: "?alice", text: "", matched: false}, + {pattern: "?alice", text: "xalice", matched: true}, + {pattern: "*?alice", text: "", matched: false}, + {pattern: "a?b", text: "a", matched: false}, + {pattern: "a?b", text: "ab", matched: false}, + {pattern: "a?b", text: "axb", matched: true}, + {pattern: "a?*b", text: "a", matched: false}, + {pattern: "a*?b", text: "a", matched: false}, + {pattern: "?", text: "", matched: true}, + {pattern: "*?", text: "", matched: true}, + {pattern: "?*", text: "", matched: true}, + {pattern: "a??", text: "a", matched: true}, + {pattern: "a??", text: "ab", matched: true}, + {pattern: "a??", text: "abcd", matched: false}, + {pattern: "a?*?", text: "a", matched: true}, + {pattern: "a*b?", text: "axb", matched: true}, } // Iterating over the test cases, call the function under test and assert the output. for i, testCase := range testCases {