From 0447fc7f9ebf3103457236633920c879380db268 Mon Sep 17 00:00:00 2001 From: lenamonj Date: Mon, 31 Aug 2026 20:47:05 -0400 Subject: [PATCH] fix: copy sets before sorting in SetEquals, SetEqualsInt and Set2DEquals --- util/util.go | 51 +++++++++++++++++++++++++--------------- util/util_test.go | 60 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 19 deletions(-) diff --git a/util/util.go b/util/util.go index 71a9ade7..54c8a82b 100644 --- a/util/util.go +++ b/util/util.go @@ -153,33 +153,41 @@ func ParamsToString(s ...string) string { } // SetEquals determines whether two string sets are identical. +// Neither argument is modified: these are usually answers the enforcer handed +// back, which alias its own state, so sorting them in place would reorder the +// caller's data as a side effect of asking a question about it. func SetEquals(a []string, b []string) bool { if len(a) != len(b) { return false } - sort.Strings(a) - sort.Strings(b) + sortedA := append([]string(nil), a...) + sortedB := append([]string(nil), b...) + sort.Strings(sortedA) + sort.Strings(sortedB) - for i, v := range a { - if v != b[i] { + for i, v := range sortedA { + if v != sortedB[i] { return false } } return true } -// SetEquals determines whether two int sets are identical. +// SetEqualsInt determines whether two int sets are identical. +// Neither argument is modified, for the same reason as SetEquals. func SetEqualsInt(a []int, b []int) bool { if len(a) != len(b) { return false } - sort.Ints(a) - sort.Ints(b) + sortedA := append([]int(nil), a...) + sortedB := append([]int(nil), b...) + sort.Ints(sortedA) + sort.Ints(sortedB) - for i, v := range a { - if v != b[i] { + for i, v := range sortedA { + if v != sortedB[i] { return false } } @@ -187,23 +195,28 @@ func SetEqualsInt(a []int, b []int) bool { } // Set2DEquals determines whether two string slice sets are identical. +// Neither argument is modified, rows included: GetPermissionsForUser and its +// siblings hand back the model's own rules, and sorting a row in place +// permuted the fields of a stored policy rule. func Set2DEquals(a [][]string, b [][]string) bool { if len(a) != len(b) { return false } - var aa []string - for _, v := range a { - sort.Strings(v) - aa = append(aa, strings.Join(v, ", ")) - } - var bb []string - for _, v := range b { - sort.Strings(v) - bb = append(bb, strings.Join(v, ", ")) + return SetEquals(sortedRows(a), sortedRows(b)) +} + +// sortedRows renders each row as a string with its fields in sorted order, +// working on a copy of the row. +func sortedRows(rows [][]string) []string { + rendered := make([]string, 0, len(rows)) + for _, row := range rows { + sorted := append([]string(nil), row...) + sort.Strings(sorted) + rendered = append(rendered, strings.Join(sorted, ", ")) } - return SetEquals(aa, bb) + return rendered } // JoinSlice joins a string and a slice into a new slice. diff --git a/util/util_test.go b/util/util_test.go index 45082b52..d99e815a 100644 --- a/util/util_test.go +++ b/util/util_test.go @@ -279,3 +279,63 @@ func TestEscapeStringLiterals(t *testing.T) { testEscapeStringLiterals(t, `'hello'`, `'hello'`) testEscapeStringLiterals(t, `"world"`, `"world"`) } + +// The Set* comparisons answer a question about their arguments and must not +// rewrite them. They used to sort in place, and since the enforcer hands back +// slices that alias its own policy, comparing an answer permuted the stored +// rule - see the enforcer level regression test TestSet2DEqualsKeepsPolicyIntact. +func TestSetComparisonsDoNotModifyArguments(t *testing.T) { + strs := []string{"c", "a", "b"} + other := []string{"b", "c", "a"} + if !SetEquals(strs, other) { + t.Error("SetEquals should hold for the same members in another order") + } + if !ArrayEquals(strs, []string{"c", "a", "b"}) { + t.Errorf("SetEquals reordered its first argument: %v", strs) + } + if !ArrayEquals(other, []string{"b", "c", "a"}) { + t.Errorf("SetEquals reordered its second argument: %v", other) + } + + ints := []int{3, 1, 2} + otherInts := []int{2, 3, 1} + if !SetEqualsInt(ints, otherInts) { + t.Error("SetEqualsInt should hold for the same members in another order") + } + if ints[0] != 3 || ints[1] != 1 || ints[2] != 2 { + t.Errorf("SetEqualsInt reordered its first argument: %v", ints) + } + if otherInts[0] != 2 || otherInts[1] != 3 || otherInts[2] != 1 { + t.Errorf("SetEqualsInt reordered its second argument: %v", otherInts) + } + + rules := [][]string{{"zoe", "data1", "read"}, {"amy", "data2", "write"}} + want := [][]string{{"amy", "data2", "write"}, {"zoe", "data1", "read"}} + if !Set2DEquals(rules, want) { + t.Error("Set2DEquals should hold for the same rules in another order") + } + if !ArrayEquals(rules[0], []string{"zoe", "data1", "read"}) { + t.Errorf("Set2DEquals reordered a row of its first argument: %v", rules) + } + if !ArrayEquals(want[1], []string{"zoe", "data1", "read"}) { + t.Errorf("Set2DEquals reordered a row of its second argument: %v", want) + } + + // The comparisons still discriminate after the change. + if SetEquals([]string{"a", "b"}, []string{"a", "c"}) { + t.Error("SetEquals should not hold for different members") + } + if SetEqualsInt([]int{1, 2}, []int{1, 3}) { + t.Error("SetEqualsInt should not hold for different members") + } + if Set2DEquals(rules, [][]string{{"zoe", "data1", "read"}, {"amy", "data2", "read"}}) { + t.Error("Set2DEquals should not hold for different rules") + } +} + +// TestLRUCachePutOverwritesAnExistingKey pins CAS-8: Put on a key already in +// the cache reuses that key's node for its links, so the new value has to be +// written onto it. Re-linking alone left the cache answering with the value the +// key was first stored with, and every later Put for that key was discarded +// without a word. Both the plain and the synchronised cache are driven, because +// the synchronised one delegates and would inherit the defect silently.