diff --git a/model/policy.go b/model/policy.go index e55bf410..a7ff8719 100644 --- a/model/policy.go +++ b/model/policy.go @@ -35,6 +35,15 @@ const ( const DefaultSep = "," +// policyKey generates a unique key for a policy rule that handles fields containing the separator. +// It uses ASCII unit separator (US, 0x1F) which is explicitly designed for this purpose +// and is unlikely to appear in policy data. +const policySeparator = "\x1f" + +func policyKey(rule []string) string { + return strings.Join(rule, policySeparator) +} + // BuildIncrementalRoleLinks provides incremental build the role inheritance relations. func (model Model) BuildIncrementalRoleLinks(rmMap map[string]rbac.RoleManager, op PolicyOp, sec string, ptype string, rules [][]string) error { if sec == "g" && rmMap[ptype] != nil { @@ -174,7 +183,7 @@ func (model Model) HasPolicy(sec string, ptype string, rule []string) (bool, err if err != nil { return false, err } - _, ok := model[sec][ptype].PolicyMap[strings.Join(rule, DefaultSep)] + _, ok := model[sec][ptype].PolicyMap[policyKey(rule)] return ok, nil } @@ -200,7 +209,7 @@ func (model Model) AddPolicy(sec string, ptype string, rule []string) error { return err } assertion.Policy = append(assertion.Policy, rule) - assertion.PolicyMap[strings.Join(rule, DefaultSep)] = len(model[sec][ptype].Policy) - 1 + assertion.PolicyMap[policyKey(rule)] = len(model[sec][ptype].Policy) - 1 hasPriority := false if _, ok := assertion.FieldIndexMap[constant.PriorityIndex]; ok { @@ -215,10 +224,10 @@ func (model Model) AddPolicy(sec string, ptype string, rule []string) error { break } assertion.Policy[i] = assertion.Policy[i-1] - assertion.PolicyMap[strings.Join(assertion.Policy[i-1], DefaultSep)]++ + assertion.PolicyMap[policyKey(assertion.Policy[i-1])]++ } assertion.Policy[i] = rule - assertion.PolicyMap[strings.Join(rule, DefaultSep)] = i + assertion.PolicyMap[policyKey(rule)] = i } } return nil @@ -238,7 +247,7 @@ func (model Model) AddPoliciesWithAffected(sec string, ptype string, rules [][]s } var affected [][]string for _, rule := range rules { - hashKey := strings.Join(rule, DefaultSep) + hashKey := policyKey(rule) _, ok := model[sec][ptype].PolicyMap[hashKey] if ok { continue @@ -259,7 +268,7 @@ func (model Model) RemovePolicy(sec string, ptype string, rule []string) (bool, if err != nil { return false, err } - key := strings.Join(rule, DefaultSep) + key := policyKey(rule) index, ok := ast.PolicyMap[key] if !ok { return false, nil @@ -268,7 +277,7 @@ func (model Model) RemovePolicy(sec string, ptype string, rule []string) (bool, lastIdx := len(ast.Policy) - 1 if index != lastIdx { ast.Policy[index] = ast.Policy[lastIdx] - lastPolicyKey := strings.Join(ast.Policy[index], DefaultSep) + lastPolicyKey := policyKey(ast.Policy[index]) ast.PolicyMap[lastPolicyKey] = index } ast.Policy = ast.Policy[:lastIdx] @@ -282,7 +291,7 @@ func (model Model) UpdatePolicy(sec string, ptype string, oldRule []string, newR if err != nil { return false, err } - oldPolicy := strings.Join(oldRule, DefaultSep) + oldPolicy := policyKey(oldRule) index, ok := model[sec][ptype].PolicyMap[oldPolicy] if !ok { return false, nil @@ -290,7 +299,7 @@ func (model Model) UpdatePolicy(sec string, ptype string, oldRule []string, newR model[sec][ptype].Policy[index] = newRule delete(model[sec][ptype].PolicyMap, oldPolicy) - model[sec][ptype].PolicyMap[strings.Join(newRule, DefaultSep)] = index + model[sec][ptype].PolicyMap[policyKey(newRule)] = index return true, nil } @@ -309,8 +318,8 @@ func (model Model) UpdatePolicies(sec string, ptype string, oldRules, newRules [ if rollbackFlag { for index, oldNewIndex := range modifiedRuleIndex { model[sec][ptype].Policy[index] = oldRules[oldNewIndex[0]] - oldPolicy := strings.Join(oldRules[oldNewIndex[0]], DefaultSep) - newPolicy := strings.Join(newRules[oldNewIndex[1]], DefaultSep) + oldPolicy := policyKey(oldRules[oldNewIndex[0]]) + newPolicy := policyKey(newRules[oldNewIndex[1]]) delete(model[sec][ptype].PolicyMap, newPolicy) model[sec][ptype].PolicyMap[oldPolicy] = index } @@ -319,7 +328,7 @@ func (model Model) UpdatePolicies(sec string, ptype string, oldRules, newRules [ newIndex := 0 for oldIndex, oldRule := range oldRules { - oldPolicy := strings.Join(oldRule, DefaultSep) + oldPolicy := policyKey(oldRule) index, ok := model[sec][ptype].PolicyMap[oldPolicy] if !ok { rollbackFlag = true @@ -328,7 +337,7 @@ func (model Model) UpdatePolicies(sec string, ptype string, oldRules, newRules [ model[sec][ptype].Policy[index] = newRules[newIndex] delete(model[sec][ptype].PolicyMap, oldPolicy) - model[sec][ptype].PolicyMap[strings.Join(newRules[newIndex], DefaultSep)] = index + model[sec][ptype].PolicyMap[policyKey(newRules[newIndex])] = index modifiedRuleIndex[index] = []int{oldIndex, newIndex} newIndex++ } @@ -350,16 +359,16 @@ func (model Model) RemovePoliciesWithAffected(sec string, ptype string, rules [] } var affected [][]string for _, rule := range rules { - index, ok := model[sec][ptype].PolicyMap[strings.Join(rule, DefaultSep)] + index, ok := model[sec][ptype].PolicyMap[policyKey(rule)] if !ok { continue } affected = append(affected, rule) model[sec][ptype].Policy = append(model[sec][ptype].Policy[:index], model[sec][ptype].Policy[index+1:]...) - delete(model[sec][ptype].PolicyMap, strings.Join(rule, DefaultSep)) + delete(model[sec][ptype].PolicyMap, policyKey(rule)) for i := index; i < len(model[sec][ptype].Policy); i++ { - model[sec][ptype].PolicyMap[strings.Join(model[sec][ptype].Policy[i], DefaultSep)] = i + model[sec][ptype].PolicyMap[policyKey(model[sec][ptype].Policy[i])] = i } } return affected, nil @@ -389,7 +398,7 @@ func (model Model) RemoveFilteredPolicy(sec string, ptype string, fieldIndex int effects = append(effects, rule) } else { tmp = append(tmp, rule) - model[sec][ptype].PolicyMap[strings.Join(rule, DefaultSep)] = len(tmp) - 1 + model[sec][ptype].PolicyMap[policyKey(rule)] = len(tmp) - 1 } } diff --git a/policy_batch_index_bug_test.go b/policy_batch_index_bug_test.go new file mode 100644 index 00000000..d38f7b42 --- /dev/null +++ b/policy_batch_index_bug_test.go @@ -0,0 +1,222 @@ +// Copyright 2017 The casbin Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package casbin + +import ( + "testing" + + "github.com/casbin/casbin/v3/model" +) + +// Test for issue #1733: Index corruption in batch delete +// When removing multiple policies, the PolicyMap indices become stale after the first removal +func TestBatchRemoveIndexCorruption(t *testing.T) { + modelText := ` +[request_definition] +r = sub, obj, act + +[policy_definition] +p = sub, obj, act + +[policy_effect] +e = some(where (p.eft == allow)) + +[matchers] +m = r.sub == p.sub && r.obj == p.obj && r.act == p.act +` + + m, err := model.NewModelFromString(modelText) + if err != nil { + t.Fatalf("Failed to create model: %v", err) + } + + e, err := NewEnforcer(m) + if err != nil { + t.Fatalf("Failed to create enforcer: %v", err) + } + + // Add multiple policies with commas in fields + // The order matters for reproducing the bug + policies := [][]string{ + {"alice", "data1", "read"}, // index 0 + {"bob", "data,with,comma", "write"}, // index 1 - will be deleted + {"charlie", "data3", "read"}, // index 2 + {"dave", "another,comma,field", "write"}, // index 3 - will be deleted + {"eve", "data5", "read"}, // index 4 + {"frank", "yet,another,comma", "write"}, // index 5 - will be deleted + {"grace", "data7", "read"}, // index 6 + } + + for _, p := range policies { + _, err := e.AddPolicy(p) + if err != nil { + t.Fatalf("Failed to add policy: %v", err) + } + } + + allPolicies, err := e.GetPolicy() + if err != nil { + t.Fatalf("Failed to get policies: %v", err) + } + t.Logf("Added %d policies", len(allPolicies)) + + // Now try to remove policies at indices 1, 3, 5 in a single batch + // This simulates the issue where multiple non-consecutive policies with commas need to be removed + toRemove := [][]string{ + {"bob", "data,with,comma", "write"}, + {"dave", "another,comma,field", "write"}, + {"frank", "yet,another,comma", "write"}, + } + + t.Log("Attempting to remove 3 policies with commas in batch...") + + // Verify they all exist before removal + for i, p := range toRemove { + has, err := e.HasPolicy(p) + if err != nil { + t.Fatalf("HasPolicy failed: %v", err) + } + if !has { + t.Errorf("Policy %d not found before removal: %v", i, p) + } + } + + removed, err := e.RemovePolicies(toRemove) + if err != nil { + t.Fatalf("RemovePolicies failed: %v", err) + } + + if !removed { + t.Error("RemovePolicies returned false") + } + + remaining, err := e.GetPolicy() + if err != nil { + t.Fatalf("Failed to get remaining policies: %v", err) + } + + t.Logf("After removal: %d policies remaining (expected 4)", len(remaining)) + + if len(remaining) != 4 { + t.Errorf("Expected 4 policies remaining, got %d", len(remaining)) + t.Log("Remaining policies:") + for _, p := range remaining { + t.Logf(" %v", p) + } + } + + // Verify each removed policy is actually gone + for i, p := range toRemove { + has, err := e.HasPolicy(p) + if err != nil { + t.Fatalf("HasPolicy check failed: %v", err) + } + if has { + t.Errorf("Policy %d still exists after batch removal: %v", i, p) + } + } + + // Verify the policies we expected to keep are still there + expectedRemaining := [][]string{ + {"alice", "data1", "read"}, + {"charlie", "data3", "read"}, + {"eve", "data5", "read"}, + {"grace", "data7", "read"}, + } + + for _, p := range expectedRemaining { + has, err := e.HasPolicy(p) + if err != nil { + t.Fatalf("HasPolicy check failed: %v", err) + } + if !has { + t.Errorf("Expected policy missing: %v", p) + } + } +} + +// Simpler test to demonstrate the index shifting bug +func TestBatchRemoveAdjacentPolicies(t *testing.T) { + modelText := ` +[request_definition] +r = sub, obj, act + +[policy_definition] +p = sub, obj, act + +[policy_effect] +e = some(where (p.eft == allow)) + +[matchers] +m = r.sub == p.sub && r.obj == p.obj && r.act == p.act +` + + m, err := model.NewModelFromString(modelText) + if err != nil { + t.Fatalf("Failed to create model: %v", err) + } + + e, err := NewEnforcer(m) + if err != nil { + t.Fatalf("Failed to create enforcer: %v", err) + } + + // Add 5 policies + policies := [][]string{ + {"user1", "data1", "read"}, + {"user2", "data2", "read"}, + {"user3", "data3", "read"}, + {"user4", "data4", "read"}, + {"user5", "data5", "read"}, + } + + for _, p := range policies { + _, err := e.AddPolicy(p) + if err != nil { + t.Fatalf("Failed to add policy: %v", err) + } + } + + // Try to remove policies at indices 1, 2, 3 in one batch + // After removing index 1, what was at index 2 is now at index 1 + // After removing (new) index 1, what was at index 3 is now at index 1 + toRemove := [][]string{ + policies[1], + policies[2], + policies[3], + } + + removed, err := e.RemovePolicies(toRemove) + if err != nil { + t.Fatalf("RemovePolicies failed: %v", err) + } + + if !removed { + t.Error("RemovePolicies returned false") + } + + remaining, err := e.GetPolicy() + if err != nil { + t.Fatalf("Failed to get remaining policies: %v", err) + } + + if len(remaining) != 2 { + t.Errorf("Expected 2 policies remaining, got %d", len(remaining)) + t.Log("Remaining:") + for _, p := range remaining { + t.Logf(" %v", p) + } + } +} diff --git a/policy_hash_collision_test.go b/policy_hash_collision_test.go new file mode 100644 index 00000000..6fd7c78e --- /dev/null +++ b/policy_hash_collision_test.go @@ -0,0 +1,194 @@ +// Copyright 2017 The casbin Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package casbin + +import ( + "strings" + "testing" + + "github.com/casbin/casbin/v3/model" +) + +// Test for potential hash key collision with comma separator +// This demonstrates the theoretical issue with using comma as separator +func TestPolicyHashCollisionPotential(t *testing.T) { + // Two different policies that would have identical hash keys if using comma separator + policy1 := []string{"alice", "data,file", "read"} // 3 fields + policy2 := []string{"alice", "data", "file", "read"} // 4 fields + + // OLD approach (with comma) would cause collision + oldHash1 := strings.Join(policy1, ",") + oldHash2 := strings.Join(policy2, ",") + + t.Logf("Policy 1: %v -> old hash: %s", policy1, oldHash1) + t.Logf("Policy 2: %v -> old hash: %s", policy2, oldHash2) + + if oldHash1 == oldHash2 { + t.Logf("OLD APPROACH: Collision detected with comma separator") + } + + // NEW approach (with unit separator) prevents collision + // Note: we can't call policyKey directly as it's not exported, but the internal implementation uses \x1f + newHash1 := strings.Join(policy1, "\x1f") + newHash2 := strings.Join(policy2, "\x1f") + + t.Logf("NEW approach - Policy 1: %v -> new hash: %q", policy1, newHash1) + t.Logf("NEW approach - Policy 2: %v -> new hash: %q", policy2, newHash2) + + if newHash1 != newHash2 { + t.Logf("NEW APPROACH: No collision with unit separator - fix working!") + } else { + t.Error("NEW APPROACH: Unexpected collision!") + } + + // Verify with actual Casbin behavior + modelText := ` +[request_definition] +r = sub, obj, act + +[policy_definition] +p = sub, obj, act + +[policy_effect] +e = some(where (p.eft == allow)) + +[matchers] +m = r.sub == p.sub && r.obj == p.obj && r.act == p.act +` + + m, err := model.NewModelFromString(modelText) + if err != nil { + t.Fatalf("Failed to create model: %v", err) + } + + e, err := NewEnforcer(m) + if err != nil { + t.Fatalf("Failed to create enforcer: %v", err) + } + + // Try to add policy1 (3 fields - should succeed) + added1, err := e.AddPolicy(policy1) + if err != nil { + t.Logf("AddPolicy for policy1 failed (expected): %v", err) + } else if added1 { + t.Logf("Policy1 added successfully") + } + + // Try to add policy2 (4 fields - should fail due to field count mismatch) + added2, err := e.AddPolicy(policy2) + if err != nil { + t.Logf("AddPolicy for policy2 failed as expected due to field count: %v", err) + } else if !added2 { + t.Logf("Policy2 not added (field count validation)") + } else { + // This is unexpected but not an error - model validation should prevent it + t.Logf("Note: Policy2 was added despite field count difference") + } +} + +// More realistic collision test: same field count +func TestPolicyHashCollisionSameFieldCount(t *testing.T) { + // Can we create a collision with the same field count? + // Yes, if we have nested commas + + policy1 := []string{"alice", "data", "read"} + policy2 := []string{"alice", "da,ta", "read"} // This has different content but could theoretically collide if parsed incorrectly + + hash1 := strings.Join(policy1, ",") + hash2 := strings.Join(policy2, ",") + + t.Logf("Policy 1: %#v -> hash: %s", policy1, hash1) + t.Logf("Policy 2: %#v -> hash: %s", policy2, hash2) + + // These should have DIFFERENT hashes + // policy1: "alice,data,read" + // policy2: "alice,da,ta,read" <- wait, this has 4 commas but []string has 3 elements + + // Actually strings.Join is safe here because it works on []string + // The issue would only occur if we parse FROM a string + + if hash1 != hash2 { + t.Logf("Good: No collision (hashes are different)") + } else { + t.Errorf("COLLISION: Two policies with same field count have identical hash!") + } + + modelText := ` +[request_definition] +r = sub, obj, act + +[policy_definition] +p = sub, obj, act + +[policy_effect] +e = some(where (p.eft == allow)) + +[matchers] +m = r.sub == p.sub && r.obj == p.obj && r.act == p.act +` + + m, err := model.NewModelFromString(modelText) + if err != nil { + t.Fatalf("Failed to create model: %v", err) + } + + e, err := NewEnforcer(m) + if err != nil { + t.Fatalf("Failed to create enforcer: %v", err) + } + + // Add both policies + _, err = e.AddPolicy(policy1) + if err != nil { + t.Fatalf("Failed to add policy1: %v", err) + } + + _, err = e.AddPolicy(policy2) + if err != nil { + t.Fatalf("Failed to add policy2: %v", err) + } + + // Check both exist + has1, _ := e.HasPolicy(policy1) + has2, _ := e.HasPolicy(policy2) + + t.Logf("Policy1 exists: %v", has1) + t.Logf("Policy2 exists: %v", has2) + + if !has1 || !has2 { + t.Error("One or both policies missing - possible hash collision") + } + + // Try to remove policy1 + removed, err := e.RemovePolicy(policy1) + if err != nil { + t.Fatalf("RemovePolicy failed: %v", err) + } + if !removed { + t.Error("Failed to remove policy1") + } + + // Check policy2 is still there + has2After, _ := e.HasPolicy(policy2) + if !has2After { + t.Error("Policy2 was incorrectly removed (hash collision!)") + } + + // Check policy1 is gone + has1After, _ := e.HasPolicy(policy1) + if has1After { + t.Error("Policy1 still exists after removal") + } +}