Merge pull request #4 from pgsty/codex/policy-correctness-20260908

fix: preserve policy denies and bound wildcard matching

Author: Vonng

policy/condition/stringfunc.go

@@ -82,7 +82,8 @@ func (f stringFunc) name() name {
 func (f stringFunc) String() string {
 	valueStrings := f.values.ToSlice()
 	sort.Strings(valueStrings)
-	return fmt.Sprintf("%v:%v:%v", f.n, f.k, valueStrings)
+	// Equality and statement hashing use this representation; preserve value boundaries.
+	return fmt.Sprintf("%v:%v:%q", f.n, f.k, valueStrings)
 }
 
 func (f stringFunc) toMap() map[Key]ValueSet {

policy/condition/stringfunc_test.go

@@ -25,6 +25,32 @@ import (
 	"github.com/minio/minio-go/v7/pkg/set"
 )
 
+func TestStringFuncValueBoundaries(t *testing.T) {
+	for _, tc := range []struct {
+		left, right []string
+		equal       bool
+	}{
+		{[]string{"a b"}, []string{"a", "b"}, false},
+		{[]string{"a b", "c"}, []string{"a", "b c"}, false},
+		{[]string{"", "a"}, []string{" a"}, false},
+		{[]string{`a" "b`}, []string{"a", "b"}, false},
+		{[]string{`a\nb`}, []string{"a\nb"}, false},
+		{[]string{"a b", "c"}, []string{"c", "a b"}, true},
+	} {
+		left, err := NewStringEqualsFunc("", S3Prefix.ToKey(), tc.left...)
+		if err != nil {
+			t.Fatal(err)
+		}
+		right, err := NewStringEqualsFunc("", S3Prefix.ToKey(), tc.right...)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if got := NewFunctions(left).Equals(NewFunctions(right)); got != tc.equal {
+			t.Errorf("condition values %q and %q: Equals = %v, want %v", tc.left, tc.right, got, tc.equal)
+		}
+	}
+}
+
 func TestStringEqualsFuncEvaluate(t *testing.T) {
 	case1Function, err := newStringEqualsFunc(S3XAmzCopySource.ToKey(), NewValueSet(NewStringValue("mybucket/myobject")), "")
 	if err != nil {

policy/policy.go

@@ -127,7 +127,16 @@ type Policy struct {
 
 // HasDenyStatement returns if the policy has a deny statement.
 func (iamp *Policy) HasDenyStatement() bool {
-	return iamp.hasDeny
+	if iamp.hasDeny {
+		return true
+	}
+	// Directly constructed policies have not populated the cached flag.
+	for i := range iamp.Statements {
+		if iamp.Statements[i].Effect == Deny {
+			return true
+		}
+	}
+	return false
 }
 
 // MatchResource matches resource with match resource patterns

policy/policy_regression_test.go

@@ -0,0 +1,173 @@
+// Copyright (c) 2026 Feng Ruohang
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+package policy
+
+import (
+	"bytes"
+	"encoding/json"
+	"fmt"
+	"io"
+	"testing"
+
+	"github.com/pgsty/silo-pkg/v3/policy/condition"
+)
+
+func TestPolicyParsingPreservesNotResourceDenies(t *testing.T) {
+	for _, count := range []int{4, 10, 11, 20} {
+		p := Policy{
+			Version: DefaultVersion,
+			Statements: []Statement{
+				{Effect: Allow, Actions: NewActionSet(GetObjectAction), Resources: NewResourceSet(NewResource("*"))},
+				{Effect: Deny, Actions: NewActionSet(GetObjectAction), NotResources: NewResourceSet(NewResource("public/*"), NewResource("shared/*"))},
+				{Effect: Deny, Actions: NewActionSet(GetObjectAction), NotResources: NewResourceSet(NewResource("other/*"), NewResource("shared/*"))},
+			},
+		}
+		for len(p.Statements) < count {
+			p.Statements = append(p.Statements, Statement{
+				Effect: Allow, Actions: NewActionSet(PutObjectAction),
+				Resources: NewResourceSet(NewResource(fmt.Sprintf("filler%d/*", len(p.Statements)))),
+			})
+		}
+		data, err := json.Marshal(p)
+		if err != nil {
+			t.Fatal(err)
+		}
+		for _, parser := range []struct {
+			name  string
+			parse func(io.Reader) (*Policy, error)
+		}{
+			{"read", ParseConfig},
+			{"write", ParseConfigStrict},
+		} {
+			t.Run(fmt.Sprintf("%s/%d", parser.name, count), func(t *testing.T) {
+				parsed, err := parser.parse(bytes.NewReader(data))
+				if err != nil {
+					t.Fatal(err)
+				}
+				if got := len(parsed.Statements); got != count {
+					t.Errorf("kept %d statements, want %d", got, count)
+				}
+				for _, bucket := range []string{"public", "other", "shared"} {
+					want := bucket == "shared"
+					if got := parsed.IsAllowed(Args{Action: GetObjectAction, BucketName: bucket, ObjectName: "file"}); got != want {
+						t.Errorf("GetObject %s/file = %v, want %v", bucket, got, want)
+					}
+				}
+			})
+		}
+	}
+}
+
+func TestPolicyDeduplicationPreservesConditionValues(t *testing.T) {
+	for _, count := range []int{3, 10, 11, 20} {
+		p := Policy{Version: DefaultVersion, Statements: []Statement{{
+			Effect: Allow, Actions: NewActionSet(ListBucketAction), Resources: NewResourceSet(NewResource("bucket")),
+		}}}
+		for _, values := range [][]string{{"a b"}, {"a", "b"}} {
+			f, err := condition.NewStringEqualsFunc("", condition.S3Prefix.ToKey(), values...)
+			if err != nil {
+				t.Fatal(err)
+			}
+			p.Statements = append(p.Statements, Statement{
+				Effect: Deny, Actions: NewActionSet(ListBucketAction), Resources: NewResourceSet(NewResource("bucket")),
+				Conditions: condition.NewFunctions(f),
+			})
+		}
+		for len(p.Statements) < count {
+			p.Statements = append(p.Statements, Statement{
+				Effect: Allow, Actions: NewActionSet(GetObjectAction),
+				Resources: NewResourceSet(NewResource(fmt.Sprintf("filler%d/*", len(p.Statements)))),
+			})
+		}
+		data, err := json.Marshal(p)
+		if err != nil {
+			t.Fatal(err)
+		}
+		for _, mode := range []string{"read", "write", "merge"} {
+			t.Run(fmt.Sprintf("%s/%d", mode, count), func(t *testing.T) {
+				var parsed *Policy
+				var err error
+				switch mode {
+				case "read":
+					parsed, err = ParseConfig(bytes.NewReader(data))
+				case "write":
+					parsed, err = ParseConfigStrict(bytes.NewReader(data))
+				case "merge":
+					merged := MergePolicies(p, Policy{Version: DefaultVersion, Statements: []Statement{p.Statements[1].Clone()}})
+					parsed = &merged
+				}
+				if err != nil {
+					t.Fatal(err)
+				}
+				if got := len(parsed.Statements); got != count {
+					t.Errorf("kept %d statements, want %d", got, count)
+				}
+				for _, prefix := range []string{"a", "b", "a b", "other"} {
+					args := Args{Action: ListBucketAction, BucketName: "bucket", ConditionValues: map[string][]string{"prefix": {prefix}}}
+					if got, want := parsed.IsAllowed(args), prefix == "other"; got != want {
+						t.Errorf("ListBucket prefix %q = %v, want %v", prefix, got, want)
+					}
+				}
+			})
+		}
+	}
+}
+
+func TestMergePoliciesPreservesNotResourceDenies(t *testing.T) {
+	allow := Statement{Effect: Allow, Actions: NewActionSet(GetObjectAction), Resources: NewResourceSet(NewResource("*"))}
+	public := Statement{Effect: Deny, Actions: NewActionSet(GetObjectAction), NotResources: NewResourceSet(NewResource("public/*"), NewResource("shared/*"))}
+	other := Statement{Effect: Deny, Actions: NewActionSet(GetObjectAction), NotResources: NewResourceSet(NewResource("other/*"), NewResource("shared/*"))}
+	merged := MergePolicies(
+		Policy{Version: DefaultVersion, Statements: []Statement{allow, public}},
+		Policy{Version: DefaultVersion, Statements: []Statement{other, public.Clone()}},
+	)
+	if got := len(merged.Statements); got != 3 {
+		t.Errorf("kept %d statements, want 3 distinct statements", got)
+	}
+	for _, bucket := range []string{"public", "other", "shared"} {
+		want := bucket == "shared"
+		if got := merged.IsAllowed(Args{Action: GetObjectAction, BucketName: bucket, ObjectName: "file"}); got != want {
+			t.Errorf("GetObject %s/file = %v, want %v", bucket, got, want)
+		}
+	}
+}
+
+func TestHasDenyStatementWithoutParsing(t *testing.T) {
+	for _, tt := range []struct {
+		name    string
+		effects []Effect
+		want    bool
+	}{
+		{"empty", nil, false},
+		{"allow", []Effect{Allow}, false},
+		{"deny", []Effect{Deny}, true},
+		{"allow-then-deny", []Effect{Allow, Deny}, true},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			p := Policy{Version: DefaultVersion}
+			for _, effect := range tt.effects {
+				p.Statements = append(p.Statements, Statement{
+					Effect: effect, Actions: NewActionSet(GetObjectAction),
+					Resources: NewResourceSet(NewResource("*")),
+				})
+			}
+			if got := p.HasDenyStatement(); got != tt.want {
+				t.Errorf("HasDenyStatement() = %v, want %v", got, tt.want)
+			}
+			p.updateActionIndex()
+			if got := p.HasDenyStatement(); got != tt.want {
+				t.Errorf("indexed HasDenyStatement() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+	for _, p := range DefaultPolicies {
+		if p.Name == "readonly" {
+			if !p.Definition.HasDenyStatement() {
+				t.Error("readonly's explicit Deny must be reported before parsing")
+			}
+			return
+		}
+	}
+	t.Fatal("readonly policy not found")
+}

policy/statement.go

@@ -731,6 +731,11 @@ func (statement Statement) hash(seed uint64) [16]byte {
 		xorTo(&h, xxh3.HashString128Seed(res.Pattern+res.Type.String(), seed+6))
 	}
 
+	xorInt(&h, len(statement.NotResources), seed+9)
+	for res := range statement.NotResources {
+		xorTo(&h, xxh3.HashString128Seed(res.Pattern+res.Type.String(), seed+10))
+	}
+
 	xorInt(&h, len(statement.Conditions), seed+7)
 	for _, cond := range statement.Conditions {
 		xorTo(&h, xxh3.HashString128Seed(cond.String(), seed+8))

wildcard/match.go

@@ -17,23 +17,19 @@
 
 package wildcard
 
-import (
-	"cmp"
-	"strings"
-)
+import "strings"
 
 // 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`.
+// Unlike Match, reaching '?' after name is exhausted accepts the rest of the
+// pattern. For example, both "a?" and "a?b" match "a".
 func MatchSimple(pattern, name string) bool {
 	if pattern == "" {
 		return name == pattern
 	}
 	if pattern == "*" {
 		return true
 	}
-	// Do an extended wildcard '*' and '?' match.
 	return deepMatchRune(name, pattern, true)
 }
 
@@ -54,29 +50,73 @@ func Match(pattern, name string) (matched bool) {
 
 // Has returns true if the input pattern has a wildcard (pattern).
 func Has(pattern string) bool {
-	return cmp.Or(strings.Contains(pattern, "*"), strings.Contains(pattern, "?"))
+	return strings.ContainsAny(pattern, "*?")
 }
 
+// Keep one backtracking point instead of recursively exploring both
+// alternatives for every '*'.
 func deepMatchRune(str, pattern string, simple bool) bool {
-	for len(pattern) > 0 {
-		switch pattern[0] {
-		default:
-			if len(str) == 0 || str[0] != pattern[0] {
-				return false
-			}
-		case '?':
-			if len(str) == 0 {
-				return simple
+	var s, p int
+	// Position of the '*' to resume from, and how much of str it has consumed.
+	star, mark := -1, 0
+	for s < len(str) || p < len(pattern) {
+		if p < len(pattern) {
+			switch pattern[p] {
+			case '*':
+				star, mark = p, s
+				p++
+				if p == len(pattern) {
+					return true
+				}
+				if simple {
+					// Before a later '*' replaces this one, check whether its
+					// star-free segment can reach '?' with the name exhausted.
+					for end := p; end < len(pattern) && pattern[end] != '*' && end-p <= len(str)-s; end++ {
+						if pattern[end] == '?' && matchFixedSuffix(str[s:], pattern[p:end]) {
+							return true
+						}
+					}
+				}
+				continue
+			case '?':
+				if simple && s == len(str) {
+					return true
+				}
+				if s < len(str) {
+					s++
+					p++
+					continue
+				}
+			default:
+				if s < len(str) && pattern[p] == str[s] {
+					s++
+					p++
+					continue
+				}
 			}
-		case '*':
-			return len(pattern) == 1 || // Pattern ends with this star
-				deepMatchRune(str, pattern[1:], simple) || // Matches next part of pattern
-				(len(str) > 0 && deepMatchRune(str[1:], pattern, simple)) // Continue searching forward
 		}
-		str = str[1:]
-		pattern = pattern[1:]
+		if star < 0 {
+			return false
+		}
+		// Let the last '*' swallow one more byte and retry from there.
+		mark++
+		if mark > len(str) {
+			return false
+		}
+		s, p = mark, star+1
+	}
+	return true
+}
+
+// matchFixedSuffix checks a star-free pattern already known to fit in str.
+func matchFixedSuffix(str, pattern string) bool {
+	str = str[len(str)-len(pattern):]
+	for i := len(pattern) - 1; i >= 0; i-- {
+		if pattern[i] != '?' && pattern[i] != str[i] {
+			return false
+		}
 	}
-	return len(str) == 0 && len(pattern) == 0
+	return true
 }
 
 // MatchAsPatternPrefix matches text as a prefix of the given pattern. Examples: