Skip to content

Fix policy deletion with commas in quoted fields (#1733) #1739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Jbansal2 wants to merge 1 commit into
base: master
Choose a base branch
from
+442 −17
Open

Fix policy deletion with commas in quoted fields (#1733) #1739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 26 additions & 17 deletions model/policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}

Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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]
Expand All @@ -282,15 +291,15 @@ 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
}

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
}
Expand All @@ -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
}
Expand All @@ -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
Expand All @@ -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++
}
Expand All @@ -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
Expand Down Expand Up @@ -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
}
}

Expand Down
222 changes: 222 additions & 0 deletions policy_batch_index_bug_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
Loading