From f1ddad331879a497fcc2054c3dc9089ece6e04aa Mon Sep 17 00:00:00 2001 From: mleku Date: Tue, 25 Nov 2025 20:46:46 +0000 Subject: [PATCH] fix policy logic error caused by interface breach --- CLAUDE.md | 8 + app/handle-event.go | 2 +- app/handle-req.go | 57 +----- pkg/policy/benchmark_test.go | 14 +- pkg/policy/bug_reproduction_test.go | 18 +- pkg/policy/comprehensive_test.go | 2 +- pkg/policy/kind_whitelist_test.go | 22 +-- pkg/policy/policy.go | 67 +++++-- pkg/policy/policy_integration_test.go | 4 +- pkg/policy/policy_test.go | 84 ++++----- pkg/policy/precedence_test.go | 16 +- pkg/policy/privileged_only_test.go | 243 ++++++++++++++++++++++++++ pkg/policy/read_access_test.go | 10 +- pkg/version/version | 2 +- 14 files changed, 390 insertions(+), 159 deletions(-) create mode 100644 pkg/policy/privileged_only_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 559bf54..eaeb1dc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -302,6 +302,14 @@ export ORLY_DB_INDEX_CACHE_MB=256 # Index cache size - Embedded via `//go:embed` directive in `app/web.go` - Served at root path `/` with API at `/api/*` +**Domain Boundaries & Encapsulation:** +- Library packages (e.g., `pkg/policy`) should NOT export internal state variables +- Use unexported fields (lowercase) for internal state to enforce encapsulation at compile time +- Provide public API methods (e.g., `IsEnabled()`, `CheckPolicy()`) instead of exposing internals +- When JSON unmarshalling is needed for unexported fields, use a shadow struct with custom `UnmarshalJSON` +- External packages (e.g., `app/`) should ONLY use public API methods, never access internal fields +- **DO NOT** change unexported fields to exported when fixing bugs - this breaks the domain boundary + ## Development Workflow ### Making Changes to Web UI diff --git a/app/handle-event.go b/app/handle-event.go index d601f5a..f4c3416 100644 --- a/app/handle-event.go +++ b/app/handle-event.go @@ -111,7 +111,7 @@ func (l *Listener) HandleEvent(msg []byte) (err error) { } // Check if policy is enabled and process event through it - if l.policyManager != nil && l.policyManager.Manager != nil && l.policyManager.Manager.IsEnabled() { + if l.policyManager.IsEnabled() { // Check policy for write access allowed, policyErr := l.policyManager.CheckPolicy("write", env.E, l.authedPubkey.Load(), l.remote) diff --git a/app/handle-req.go b/app/handle-req.go index d22c94e..6bb7d04 100644 --- a/app/handle-req.go +++ b/app/handle-req.go @@ -26,7 +26,6 @@ import ( "git.mleku.dev/mleku/nostr/encoders/tag" "next.orly.dev/pkg/policy" "next.orly.dev/pkg/protocol/nip43" - "next.orly.dev/pkg/utils" "git.mleku.dev/mleku/nostr/utils/normalize" "git.mleku.dev/mleku/nostr/utils/pointers" ) @@ -388,64 +387,16 @@ func (l *Listener) HandleReq(msg []byte) (err error) { ) } } else { - // Check if policy defines this event as privileged (even if not in hardcoded list) - // Policy check will handle this later, but we can skip it here if not authenticated - // to avoid unnecessary processing - if l.policyManager != nil && l.policyManager.Manager != nil && l.policyManager.Manager.IsEnabled() { - rule, hasRule := l.policyManager.Rules[int(ev.Kind)] - if hasRule && rule.Privileged && accessLevel != "admin" { - pk := l.authedPubkey.Load() - if pk == nil { - // Not authenticated - cannot see policy-privileged events - log.T.C( - func() string { - return fmt.Sprintf( - "policy-privileged event %s denied - not authenticated", - ev.ID, - ) - }, - ) - continue - } - // Policy check will verify authorization later, but we need to check - // if user is party to the event here - authorized := false - if utils.FastEqual(ev.Pubkey, pk) { - authorized = true - } else { - // Check p tags - pTags := ev.Tags.GetAll([]byte("p")) - for _, pTag := range pTags { - var pt []byte - if pt, err = hexenc.Dec(string(pTag.Value())); chk.E(err) { - continue - } - if utils.FastEqual(pt, pk) { - authorized = true - break - } - } - } - if !authorized { - log.T.C( - func() string { - return fmt.Sprintf( - "policy-privileged event %s does not contain the logged in pubkey %0x", - ev.ID, pk, - ) - }, - ) - continue - } - } - } + // Policy-defined privileged events are handled by the policy engine + // at line 455+. No early filtering needed here - delegate entirely to + // the policy engine to avoid duplicate logic. tmp = append(tmp, ev) } } events = tmp // Apply policy filtering for read access if policy is enabled - if l.policyManager != nil && l.policyManager.Manager != nil && l.policyManager.Manager.IsEnabled() { + if l.policyManager.IsEnabled() { var policyFilteredEvents event.S for _, ev := range events { allowed, policyErr := l.policyManager.CheckPolicy("read", ev, l.authedPubkey.Load(), l.remote) diff --git a/pkg/policy/benchmark_test.go b/pkg/policy/benchmark_test.go index b423576..02586dd 100644 --- a/pkg/policy/benchmark_test.go +++ b/pkg/policy/benchmark_test.go @@ -74,7 +74,7 @@ func BenchmarkCheckPolicy(b *testing.B) { Kind: Kinds{ Whitelist: []int{1, 3, 5}, }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: { Description: "test rule", WriteAllow: []string{hex.Enc(pubkey)}, @@ -132,9 +132,9 @@ done testEvent := createTestEventBench(b, signer, "test content", 1) policy := &P{ - Manager: manager, + manager: manager, Kind: Kinds{}, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: { Description: "test rule with script", Script: "policy.sh", @@ -201,7 +201,7 @@ func BenchmarkCheckPolicyMultipleKinds(b *testing.B) { policy := &P{ Kind: Kinds{}, - Rules: rules, + rules: rules, } // Generate keypair once for all events @@ -231,7 +231,7 @@ func BenchmarkCheckPolicyLargeWhitelist(b *testing.B) { Kind: Kinds{ Whitelist: whitelist, }, - Rules: map[int]Rule{}, + rules: map[int]Rule{}, } // Generate keypair once for all events @@ -255,7 +255,7 @@ func BenchmarkCheckPolicyLargeBlacklist(b *testing.B) { Kind: Kinds{ Blacklist: blacklist, }, - Rules: map[int]Rule{}, + rules: map[int]Rule{}, } // Generate keypair once for all events @@ -307,7 +307,7 @@ func BenchmarkCheckPolicyLargeEvent(b *testing.B) { policy := &P{ Kind: Kinds{}, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: { Description: "size limit test", SizeLimit: int64Ptr(200000), // 200KB limit diff --git a/pkg/policy/bug_reproduction_test.go b/pkg/policy/bug_reproduction_test.go index 0ba832e..4545c9c 100644 --- a/pkg/policy/bug_reproduction_test.go +++ b/pkg/policy/bug_reproduction_test.go @@ -42,7 +42,7 @@ func TestBugReproduction_Kind1AllowedWithWhitelist4678(t *testing.T) { if allowed { t.Errorf("BUG REPRODUCED: Kind 1 event was ALLOWED but should be REJECTED (only kind 4678 is whitelisted)") t.Logf("Policy whitelist: %v", policy.Kind.Whitelist) - t.Logf("Policy rules: %v", policy.Rules) + t.Logf("Policy rules: %v", policy.rules) t.Logf("Default policy: %s", policy.DefaultPolicy) } }) @@ -112,9 +112,9 @@ func TestBugReproduction_WithPolicyManager(t *testing.T) { if allowed { t.Errorf("BUG REPRODUCED: Kind 1 event was ALLOWED but should be REJECTED") t.Logf("Policy whitelist: %v", policy.Kind.Whitelist) - t.Logf("Policy rules: %v", policy.Rules) + t.Logf("Policy rules: %v", policy.rules) t.Logf("Default policy: %s", policy.DefaultPolicy) - t.Logf("Manager enabled: %v", policy.Manager.IsEnabled()) + t.Logf("Manager enabled: %v", policy.manager.IsEnabled()) } }) @@ -130,8 +130,8 @@ func TestBugReproduction_WithPolicyManager(t *testing.T) { }) // Clean up - if policy.Manager != nil { - policy.Manager.Shutdown() + if policy.manager != nil { + policy.manager.Shutdown() } } @@ -159,7 +159,7 @@ func TestBugReproduction_DebugPolicyFlow(t *testing.T) { t.Logf("=== Policy Configuration ===") t.Logf("Whitelist: %v", policy.Kind.Whitelist) t.Logf("Blacklist: %v", policy.Kind.Blacklist) - t.Logf("Rules: %v", policy.Rules) + t.Logf("rules: %v", policy.rules) t.Logf("Default policy: %s", policy.DefaultPolicy) t.Logf("") t.Logf("=== Event Details ===") @@ -223,7 +223,7 @@ func TestBugFix_FailSafeWhenConfigMissing(t *testing.T) { policy := &P{ DefaultPolicy: "allow", - Manager: manager, + manager: manager, } // Try to load from nonexistent file - this should trigger the panic @@ -249,7 +249,7 @@ func TestBugFix_FailSafeWhenConfigMissing(t *testing.T) { Kind: Kinds{ Whitelist: []int{}, // Empty }, - Rules: make(map[int]Rule), // No rules + rules: make(map[int]Rule), // No rules } event := createTestEvent(t, testSigner, "Hello Nostr!", 1) @@ -269,7 +269,7 @@ func TestBugFix_FailSafeWhenConfigMissing(t *testing.T) { Kind: Kinds{ Whitelist: []int{}, // Empty }, - Rules: make(map[int]Rule), // No rules + rules: make(map[int]Rule), // No rules } event := createTestEvent(t, testSigner, "Hello Nostr!", 1) diff --git a/pkg/policy/comprehensive_test.go b/pkg/policy/comprehensive_test.go index bb167ca..9b1fe3e 100644 --- a/pkg/policy/comprehensive_test.go +++ b/pkg/policy/comprehensive_test.go @@ -332,7 +332,7 @@ done // Initialize policy manager ctx, cancel := context.WithCancel(context.Background()) defer cancel() - policy.Manager = &PolicyManager{ + policy.manager = &PolicyManager{ ctx: ctx, cancel: cancel, configDir: tempDir, diff --git a/pkg/policy/kind_whitelist_test.go b/pkg/policy/kind_whitelist_test.go index a48a1f8..c3e0482 100644 --- a/pkg/policy/kind_whitelist_test.go +++ b/pkg/policy/kind_whitelist_test.go @@ -17,7 +17,7 @@ func TestKindWhitelistComprehensive(t *testing.T) { Kind: Kinds{ Whitelist: []int{1, 3, 5}, // Explicit whitelist }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: {Description: "Rule for kind 1"}, 3: {Description: "Rule for kind 3"}, 5: {Description: "Rule for kind 5"}, @@ -40,7 +40,7 @@ func TestKindWhitelistComprehensive(t *testing.T) { Kind: Kinds{ Whitelist: []int{1, 3, 5}, // Explicit whitelist }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: {Description: "Rule for kind 1"}, // Kind 3 has no rule }, @@ -62,7 +62,7 @@ func TestKindWhitelistComprehensive(t *testing.T) { Kind: Kinds{ Whitelist: []int{1, 3, 5}, // Explicit whitelist - kind 10 NOT included }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: {Description: "Rule for kind 1"}, 10: {Description: "Rule for kind 10"}, // Has rule but not in whitelist! }, @@ -84,7 +84,7 @@ func TestKindWhitelistComprehensive(t *testing.T) { Kind: Kinds{ Whitelist: []int{1, 3, 5}, // Explicit whitelist }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: {Description: "Rule for kind 1"}, }, } @@ -103,7 +103,7 @@ func TestKindWhitelistComprehensive(t *testing.T) { policy := &P{ DefaultPolicy: "allow", // Changed to allow so rules without constraints allow by default // No explicit whitelist - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: {Description: "Rule for kind 1"}, 3: {Description: "Rule for kind 3"}, }, @@ -123,7 +123,7 @@ func TestKindWhitelistComprehensive(t *testing.T) { policy := &P{ DefaultPolicy: "allow", // No explicit whitelist - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: {Description: "Rule for kind 1"}, 3: {Description: "Rule for kind 3"}, }, @@ -149,7 +149,7 @@ func TestKindWhitelistComprehensive(t *testing.T) { Description: "Global rule applies to all kinds", WriteAllow: []string{hex.Enc(testPubkey)}, }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: {Description: "Rule for kind 1"}, }, } @@ -171,7 +171,7 @@ func TestKindWhitelistComprehensive(t *testing.T) { Kind: Kinds{ Blacklist: []int{10, 20}, // Blacklist }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: {Description: "Rule for kind 1"}, 10: {Description: "Rule for kind 10"}, // Has rule but blacklisted! }, @@ -194,7 +194,7 @@ func TestKindWhitelistComprehensive(t *testing.T) { Kind: Kinds{ Blacklist: []int{10, 20}, // Blacklist }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: {Description: "Rule for kind 1"}, 3: {Description: "Rule for kind 3"}, }, @@ -219,7 +219,7 @@ func TestKindWhitelistComprehensive(t *testing.T) { Whitelist: []int{1, 3, 5, 10}, // Whitelist includes 10 Blacklist: []int{10, 20}, // Blacklist also includes 10 }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: {Description: "Rule for kind 1"}, 10: {Description: "Rule for kind 10"}, }, @@ -248,7 +248,7 @@ func TestKindWhitelistRealWorld(t *testing.T) { Kind: Kinds{ Whitelist: []int{1, 3, 30023}, }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: { Description: "Text notes", // No WriteAllow = anyone authenticated can write diff --git a/pkg/policy/policy.go b/pkg/policy/policy.go index 97dd818..d55cdc4 100644 --- a/pkg/policy/policy.go +++ b/pkg/policy/policy.go @@ -246,14 +246,37 @@ type PolicyManager struct { type P struct { // Kind is policies for accepting or rejecting events by kind number. Kind Kinds `json:"kind"` - // Rules is a map of rules for criteria that must be met for the event to be allowed to be written to the relay. - Rules map[int]Rule `json:"rules"` + // rules is a map of rules for criteria that must be met for the event to be allowed to be written to the relay. + // Unexported to enforce use of public API methods (CheckPolicy, IsEnabled). + rules map[int]Rule // Global is a rule set that applies to all events. Global Rule `json:"global"` // DefaultPolicy determines the default behavior when no rules deny an event ("allow" or "deny", defaults to "allow") DefaultPolicy string `json:"default_policy"` - // Manager handles policy script execution - Manager *PolicyManager `json:"-"` + // manager handles policy script execution. + // Unexported to enforce use of public API methods (CheckPolicy, IsEnabled). + manager *PolicyManager +} + +// pJSON is a shadow struct for JSON unmarshalling with exported fields. +type pJSON struct { + Kind Kinds `json:"kind"` + Rules map[int]Rule `json:"rules"` + Global Rule `json:"global"` + DefaultPolicy string `json:"default_policy"` +} + +// UnmarshalJSON implements custom JSON unmarshalling to handle unexported fields. +func (p *P) UnmarshalJSON(data []byte) error { + var shadow pJSON + if err := json.Unmarshal(data, &shadow); err != nil { + return err + } + p.Kind = shadow.Kind + p.rules = shadow.Rules + p.Global = shadow.Global + p.DefaultPolicy = shadow.DefaultPolicy + return nil } // New creates a new policy from JSON configuration. @@ -275,10 +298,10 @@ func New(policyJSON []byte) (p *P, err error) { // Populate binary caches for all rules (including global rule) p.Global.populateBinaryCache() - for kind := range p.Rules { - rule := p.Rules[kind] // Get a copy + for kind := range p.rules { + rule := p.rules[kind] // Get a copy rule.populateBinaryCache() - p.Rules[kind] = rule // Store the modified copy back + p.rules[kind] = rule // Store the modified copy back } return @@ -321,6 +344,12 @@ func IsPartyInvolved(ev *event.E, userPubkey []byte) bool { return false } +// IsEnabled returns whether the policy system is enabled and ready to process events. +// This is the public API for checking if policy filtering should be applied. +func (p *P) IsEnabled() bool { + return p != nil && p.manager != nil && p.manager.IsEnabled() +} + // getDefaultPolicyAction returns true if the default policy is "allow", false if "deny" func (p *P) getDefaultPolicyAction() (allowed bool) { switch p.DefaultPolicy { @@ -356,7 +385,7 @@ func NewWithManager(ctx context.Context, appName string, enabled bool) *P { // Load policy configuration from JSON file policy := &P{ DefaultPolicy: "allow", // Set default value - Manager: manager, + manager: manager, } if enabled { @@ -881,9 +910,9 @@ func (p *P) LoadFromFile(configPath string) error { // Populate binary caches for all rules (including global rule) p.Global.populateBinaryCache() - for kind, rule := range p.Rules { + for kind, rule := range p.rules { rule.populateBinaryCache() - p.Rules[kind] = rule // Update the map with the modified rule + p.rules[kind] = rule // Update the map with the modified rule } return nil @@ -912,15 +941,15 @@ func (p *P) CheckPolicy( } // Get rule for this kind - rule, hasRule := p.Rules[int(ev.Kind)] + rule, hasRule := p.rules[int(ev.Kind)] if !hasRule { // No specific rule for this kind, use default policy return p.getDefaultPolicyAction(), nil } // Check if script is present and enabled - if rule.Script != "" && p.Manager != nil { - if p.Manager.IsEnabled() { + if rule.Script != "" && p.manager != nil { + if p.manager.IsEnabled() { // Check if script file exists before trying to use it if _, err := os.Stat(rule.Script); err == nil { // Script exists, try to use it @@ -985,7 +1014,7 @@ func (p *P) checkKindsPolicy(kind uint16) bool { } } // Not in blacklist - check if rule exists for implicit whitelist - _, hasRule := p.Rules[int(kind)] + _, hasRule := p.rules[int(kind)] return hasRule // Only allow if there's a rule defined } @@ -993,9 +1022,9 @@ func (p *P) checkKindsPolicy(kind uint16) bool { // If there are specific rules defined, use implicit whitelist // If there's only a global rule (no specific rules), fall back to default policy // If there are NO rules at all, fall back to default policy - if len(p.Rules) > 0 { + if len(p.rules) > 0 { // Implicit whitelist mode - only allow kinds with specific rules - _, hasRule := p.Rules[int(kind)] + _, hasRule := p.rules[int(kind)] return hasRule } // No specific rules (maybe global rule exists) - fall back to default policy @@ -1272,12 +1301,12 @@ func (p *P) checkScriptPolicy( access string, ev *event.E, scriptPath string, loggedInPubkey []byte, ipAddress string, ) (allowed bool, err error) { - if p.Manager == nil { + if p.manager == nil { return false, fmt.Errorf("policy manager is not initialized") } // If policy is disabled, fall back to default policy immediately - if !p.Manager.IsEnabled() { + if !p.manager.IsEnabled() { log.W.F( "policy rule for kind %d is inactive (policy disabled), falling back to default policy (%s)", ev.Kind, p.DefaultPolicy, @@ -1294,7 +1323,7 @@ func (p *P) checkScriptPolicy( } // Get or create a runner for this specific script path - runner := p.Manager.getOrCreateRunner(scriptPath) + runner := p.manager.getOrCreateRunner(scriptPath) // Policy is enabled, check if this runner is running if !runner.IsRunning() { diff --git a/pkg/policy/policy_integration_test.go b/pkg/policy/policy_integration_test.go index f1b5215..7622dad 100644 --- a/pkg/policy/policy_integration_test.go +++ b/pkg/policy/policy_integration_test.go @@ -131,8 +131,8 @@ func TestPolicyIntegration(t *testing.T) { } // Verify policy loaded correctly - if len(policy.Rules) != 4 { - t.Errorf("Expected 4 rules, got %d", len(policy.Rules)) + if len(policy.rules) != 4 { + t.Errorf("Expected 4 rules, got %d", len(policy.rules)) } // Test policy checks directly diff --git a/pkg/policy/policy_test.go b/pkg/policy/policy_test.go index a81b006..e70a67e 100644 --- a/pkg/policy/policy_test.go +++ b/pkg/policy/policy_test.go @@ -135,8 +135,8 @@ func TestNew(t *testing.T) { t.Errorf("Expected policy but got nil") return } - if len(policy.Rules) != tt.expectRules { - t.Errorf("Expected %d rules, got %d", tt.expectRules, len(policy.Rules)) + if len(policy.rules) != tt.expectRules { + t.Errorf("Expected %d rules, got %d", tt.expectRules, len(policy.rules)) } }) } @@ -153,7 +153,7 @@ func TestCheckKindsPolicy(t *testing.T) { name: "no whitelist or blacklist - allow (no rules at all)", policy: &P{ Kind: Kinds{}, - Rules: map[int]Rule{}, // No rules defined + rules: map[int]Rule{}, // No rules defined }, kind: 1, expected: true, // Should be allowed (no rules = allow all kinds) @@ -162,7 +162,7 @@ func TestCheckKindsPolicy(t *testing.T) { name: "no whitelist or blacklist - deny (has other rules)", policy: &P{ Kind: Kinds{}, - Rules: map[int]Rule{ + rules: map[int]Rule{ 2: {Description: "Rule for kind 2"}, }, }, @@ -173,7 +173,7 @@ func TestCheckKindsPolicy(t *testing.T) { name: "no whitelist or blacklist - allow (has rule)", policy: &P{ Kind: Kinds{}, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: {Description: "Rule for kind 1"}, }, }, @@ -187,7 +187,7 @@ func TestCheckKindsPolicy(t *testing.T) { Global: Rule{ WriteAllow: []string{"test"}, // Global rule exists }, - Rules: map[int]Rule{}, // No specific rules + rules: map[int]Rule{}, // No specific rules }, kind: 1, expected: true, // Should be allowed (global rule exists) @@ -218,7 +218,7 @@ func TestCheckKindsPolicy(t *testing.T) { Kind: Kinds{ Blacklist: []int{2, 4, 6}, }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 3: {Description: "Rule for kind 3"}, // Has at least one rule }, }, @@ -231,7 +231,7 @@ func TestCheckKindsPolicy(t *testing.T) { Kind: Kinds{ Blacklist: []int{2, 4, 6}, }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: {Description: "Rule for kind 1"}, }, }, @@ -517,7 +517,7 @@ func TestCheckPolicy(t *testing.T) { event: testEvent, policy: &P{ Kind: Kinds{}, - Rules: map[int]Rule{}, + rules: map[int]Rule{}, }, loggedInPubkey: eventPubkey, ipAddress: "127.0.0.1", @@ -532,7 +532,7 @@ func TestCheckPolicy(t *testing.T) { Kind: Kinds{ Whitelist: []int{3, 5}, }, - Rules: map[int]Rule{}, + rules: map[int]Rule{}, }, loggedInPubkey: eventPubkey, ipAddress: "127.0.0.1", @@ -545,7 +545,7 @@ func TestCheckPolicy(t *testing.T) { event: testEvent, policy: &P{ Kind: Kinds{}, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: { Description: "block test", WriteDeny: []string{hex.Enc(testEvent.Pubkey)}, @@ -632,8 +632,8 @@ func TestLoadFromFile(t *testing.T) { t.Errorf("Unexpected error: %v", err) return } - if len(policy.Rules) != tt.expectRules { - t.Errorf("Expected %d rules, got %d", tt.expectRules, len(policy.Rules)) + if len(policy.rules) != tt.expectRules { + t.Errorf("Expected %d rules, got %d", tt.expectRules, len(policy.rules)) } }) } @@ -748,15 +748,15 @@ func TestNewWithManager(t *testing.T) { t.Fatal("Expected policy but got nil") } - if policy.Manager == nil { + if policy.manager == nil { t.Fatal("Expected policy manager but got nil") } - if policy.Manager.IsEnabled() { + if policy.manager.IsEnabled() { t.Error("Expected policy manager to be disabled") } - if policy.Manager.IsRunning() { + if policy.manager.IsRunning() { t.Error("Expected policy manager to not be running") } @@ -766,7 +766,7 @@ func TestNewWithManager(t *testing.T) { } // Clean up - policy.Manager.Shutdown() + policy.manager.Shutdown() }) // Test with enabled policy and valid config file @@ -810,7 +810,7 @@ func TestNewWithManager(t *testing.T) { policy := &P{ DefaultPolicy: "allow", - Manager: manager, + manager: manager, } // Load policy from our test file @@ -818,11 +818,11 @@ func TestNewWithManager(t *testing.T) { t.Fatalf("Failed to load policy: %v", err) } - if policy.Manager == nil { + if policy.manager == nil { t.Fatal("Expected policy manager but got nil") } - if !policy.Manager.IsEnabled() { + if !policy.manager.IsEnabled() { t.Error("Expected policy manager to be enabled") } @@ -836,7 +836,7 @@ func TestNewWithManager(t *testing.T) { } // Clean up - policy.Manager.Shutdown() + policy.manager.Shutdown() }) } @@ -962,7 +962,7 @@ func TestEdgeCasesLargeEvent(t *testing.T) { policy := &P{ Kind: Kinds{}, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: { Description: "size limit test", SizeLimit: int64Ptr(50000), // 50KB limit @@ -1170,7 +1170,7 @@ func TestCheckPolicyWithGlobalRule(t *testing.T) { Kind: Kinds{ Whitelist: []int{1}, // Allow kind 1 }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: { WriteAllow: []string{hex.Enc(eventPubkey)}, // Allow event pubkey for kind 1 }, @@ -1293,13 +1293,13 @@ func TestScriptPolicyDisabledFallsBackToDefault(t *testing.T) { // Create a policy with a script rule but policy is disabled, default policy is "allow" policy := &P{ DefaultPolicy: "allow", - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: { Description: "script rule", Script: "policy.sh", }, }, - Manager: &PolicyManager{ + manager: &PolicyManager{ enabled: false, // Policy is disabled runners: make(map[string]*ScriptRunner), }, @@ -1336,7 +1336,7 @@ func TestDefaultPolicyAllow(t *testing.T) { policy := &P{ DefaultPolicy: "allow", Kind: Kinds{}, - Rules: map[int]Rule{}, // No specific rules + rules: map[int]Rule{}, // No specific rules } // Create real test event with proper signing @@ -1360,7 +1360,7 @@ func TestDefaultPolicyDeny(t *testing.T) { policy := &P{ DefaultPolicy: "deny", Kind: Kinds{}, - Rules: map[int]Rule{}, // No specific rules + rules: map[int]Rule{}, // No specific rules } // Create real test event with proper signing @@ -1384,7 +1384,7 @@ func TestDefaultPolicyEmpty(t *testing.T) { policy := &P{ DefaultPolicy: "", Kind: Kinds{}, - Rules: map[int]Rule{}, // No specific rules + rules: map[int]Rule{}, // No specific rules } // Create real test event with proper signing @@ -1408,7 +1408,7 @@ func TestDefaultPolicyInvalid(t *testing.T) { policy := &P{ DefaultPolicy: "invalid", Kind: Kinds{}, - Rules: map[int]Rule{}, // No specific rules + rules: map[int]Rule{}, // No specific rules } // Create real test event with proper signing @@ -1432,7 +1432,7 @@ func TestDefaultPolicyWithSpecificRule(t *testing.T) { policy := &P{ DefaultPolicy: "deny", // Default is deny Kind: Kinds{}, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: { Description: "allow kind 1", WriteAllow: []string{}, // Allow all for kind 1 @@ -1497,13 +1497,13 @@ func TestScriptProcessingDisabledFallsBackToDefault(t *testing.T) { // Test that when policy is disabled, it falls back to default policy policy := &P{ DefaultPolicy: "allow", - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: { Description: "script rule", Script: "policy.sh", }, }, - Manager: &PolicyManager{ + manager: &PolicyManager{ enabled: false, // Policy is disabled runners: make(map[string]*ScriptRunner), }, @@ -1546,7 +1546,7 @@ func TestDefaultPolicyLogicWithRules(t *testing.T) { Kind: Kinds{ Whitelist: []int{1, 2, 3}, // Allow kinds 1, 2, 3 }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: { Description: "allow all for kind 1", WriteAllow: []string{}, // Empty means allow all @@ -1605,7 +1605,7 @@ func TestDefaultPolicyLogicWithRules(t *testing.T) { Kind: Kinds{ Whitelist: []int{1, 2, 3}, // Allow kinds 1, 2, 3 }, - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: { Description: "deny specific pubkey for kind 1", WriteDeny: []string{hex.Enc(deniedPubkey)}, @@ -1684,8 +1684,8 @@ done // Create policy with a rule that uses the script policy := &P{ DefaultPolicy: "deny", - Manager: manager, - Rules: map[int]Rule{ + manager: manager, + rules: map[int]Rule{ 4678: { Description: "Test rule with custom script", Script: scriptPath, // Rule-specific script path @@ -1906,12 +1906,12 @@ func TestPolicyFilterProcessing(t *testing.T) { } // Verify rules are loaded correctly - if len(policy.Rules) != 4 { - t.Errorf("Expected 4 rules, got %d", len(policy.Rules)) + if len(policy.rules) != 4 { + t.Errorf("Expected 4 rules, got %d", len(policy.rules)) } // Verify rule 4678 (script-based) - rule4678, ok := policy.Rules[4678] + rule4678, ok := policy.rules[4678] if !ok { t.Fatal("Rule 4678 not found") } @@ -1926,7 +1926,7 @@ func TestPolicyFilterProcessing(t *testing.T) { } // Verify rule 10306 (read_allow) - rule10306, ok := policy.Rules[10306] + rule10306, ok := policy.rules[10306] if !ok { t.Fatal("Rule 10306 not found") } @@ -1944,7 +1944,7 @@ func TestPolicyFilterProcessing(t *testing.T) { } // Verify rule 30520 (write_allow) - rule30520, ok := policy.Rules[30520] + rule30520, ok := policy.rules[30520] if !ok { t.Fatal("Rule 30520 not found") } @@ -1962,7 +1962,7 @@ func TestPolicyFilterProcessing(t *testing.T) { } // Verify rule 30919 (write_allow) - rule30919, ok := policy.Rules[30919] + rule30919, ok := policy.rules[30919] if !ok { t.Fatal("Rule 30919 not found") } diff --git a/pkg/policy/precedence_test.go b/pkg/policy/precedence_test.go index 4adf3fc..df5c998 100644 --- a/pkg/policy/precedence_test.go +++ b/pkg/policy/precedence_test.go @@ -44,7 +44,7 @@ func TestPolicyPrecedenceRules(t *testing.T) { t.Run("Deny List Overrides Everything", func(t *testing.T) { policy := &P{ DefaultPolicy: "allow", - Rules: map[int]Rule{ + rules: map[int]Rule{ 100: { Description: "Deny overrides allow and privileged", WriteAllow: []string{hex.Enc(alicePubkey)}, // Alice in allow list @@ -75,7 +75,7 @@ func TestPolicyPrecedenceRules(t *testing.T) { t.Run("Allow List OR Privileged Access", func(t *testing.T) { policy := &P{ DefaultPolicy: "allow", - Rules: map[int]Rule{ + rules: map[int]Rule{ 200: { Description: "Privileged with allow list", ReadAllow: []string{hex.Enc(bobPubkey)}, // Only Bob in allow list @@ -128,7 +128,7 @@ func TestPolicyPrecedenceRules(t *testing.T) { t.Run("Privileged Grants Access When No Allow List", func(t *testing.T) { policy := &P{ DefaultPolicy: "deny", // Default deny to make test clearer - Rules: map[int]Rule{ + rules: map[int]Rule{ 300: { Description: "Privileged without allow list", Privileged: true, @@ -183,7 +183,7 @@ func TestPolicyPrecedenceRules(t *testing.T) { t.Run("Allow List Exclusive Without Privileged", func(t *testing.T) { policy := &P{ DefaultPolicy: "allow", // Even with allow default - Rules: map[int]Rule{ + rules: map[int]Rule{ 400: { Description: "Allow list only", WriteAllow: []string{hex.Enc(alicePubkey)}, // Only Alice @@ -223,7 +223,7 @@ func TestPolicyPrecedenceRules(t *testing.T) { t.Run("Complex Precedence Chain", func(t *testing.T) { policy := &P{ DefaultPolicy: "allow", - Rules: map[int]Rule{ + rules: map[int]Rule{ 500: { Description: "Complex rules", WriteAllow: []string{hex.Enc(alicePubkey), hex.Enc(bobPubkey)}, @@ -277,7 +277,7 @@ func TestPolicyPrecedenceRules(t *testing.T) { // Test 6a: With allow default and no rules policyAllow := &P{ DefaultPolicy: "allow", - Rules: map[int]Rule{ + rules: map[int]Rule{ // No rule for kind 600 }, } @@ -296,7 +296,7 @@ func TestPolicyPrecedenceRules(t *testing.T) { // Test 6b: With deny default and no rules policyDeny := &P{ DefaultPolicy: "deny", - Rules: map[int]Rule{ + rules: map[int]Rule{ // No rule for kind 600 }, } @@ -314,7 +314,7 @@ func TestPolicyPrecedenceRules(t *testing.T) { // Test 6c: Default does NOT apply when allow list exists policyWithRule := &P{ DefaultPolicy: "allow", // Allow default - Rules: map[int]Rule{ + rules: map[int]Rule{ 700: { WriteAllow: []string{hex.Enc(bobPubkey)}, // Only Bob }, diff --git a/pkg/policy/privileged_only_test.go b/pkg/policy/privileged_only_test.go new file mode 100644 index 0000000..51f7ed5 --- /dev/null +++ b/pkg/policy/privileged_only_test.go @@ -0,0 +1,243 @@ +package policy + +import ( + "encoding/json" + "testing" + + "git.mleku.dev/mleku/nostr/encoders/hex" +) + +// TestPrivilegedOnlyBug tests the reported bug where privileged flag +// doesn't work when read_allow is missing +func TestPrivilegedOnlyBug(t *testing.T) { + aliceSigner, alicePubkey := generateTestKeypair(t) + _, bobPubkey := generateTestKeypair(t) + _, charliePubkey := generateTestKeypair(t) + + // Create policy with ONLY privileged, no read_allow + policyJSON := map[string]interface{}{ + "rules": map[string]interface{}{ + "4": map[string]interface{}{ + "description": "DM - privileged only", + "privileged": true, + }, + }, + } + + policyBytes, err := json.Marshal(policyJSON) + if err != nil { + t.Fatalf("Failed to marshal policy: %v", err) + } + t.Logf("Policy JSON: %s", policyBytes) + + policy, err := New(policyBytes) + if err != nil { + t.Fatalf("Failed to create policy: %v", err) + } + + // Verify the rule was loaded correctly + rule, hasRule := policy.rules[4] + if !hasRule { + t.Fatal("Rule for kind 4 was not loaded") + } + t.Logf("Loaded rule: %+v", rule) + t.Logf("Privileged flag: %v", rule.Privileged) + t.Logf("ReadAllow: %v", rule.ReadAllow) + t.Logf("readAllowBin: %v", rule.readAllowBin) + + if !rule.Privileged { + t.Fatal("BUG: Privileged flag was not set to true!") + } + + // Create a kind 4 (DM) event from Alice to Bob + ev := createTestEvent(t, aliceSigner, "Secret message from Alice to Bob", 4) + addPTag(ev, bobPubkey) // Bob is recipient + + t.Logf("Event author: %s", hex.Enc(ev.Pubkey)) + t.Logf("Event p-tags: %v", ev.Tags.GetAll([]byte("p"))) + + // Test 1: Alice (author) should be able to read + t.Run("alice_author_can_read", func(t *testing.T) { + allowed, err := policy.CheckPolicy("read", ev, alicePubkey, "127.0.0.1") + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if !allowed { + t.Error("BUG! Author should be able to read their own privileged event") + } + }) + + // Test 2: Bob (in p-tag) should be able to read + t.Run("bob_recipient_can_read", func(t *testing.T) { + allowed, err := policy.CheckPolicy("read", ev, bobPubkey, "127.0.0.1") + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if !allowed { + t.Error("BUG! Recipient (in p-tag) should be able to read privileged event") + } + }) + + // Test 3: Charlie (third party) should NOT be able to read + t.Run("charlie_third_party_denied", func(t *testing.T) { + allowed, err := policy.CheckPolicy("read", ev, charliePubkey, "127.0.0.1") + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if allowed { + t.Error("BUG! Third party should NOT be able to read privileged event") + } + }) + + // Test 4: Unauthenticated user should NOT be able to read + t.Run("unauthenticated_denied", func(t *testing.T) { + allowed, err := policy.CheckPolicy("read", ev, nil, "127.0.0.1") + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if allowed { + t.Error("BUG! Unauthenticated user should NOT be able to read privileged event") + } + }) +} + +// TestPrivilegedWithReadAllowBug tests the scenario where read_allow is present +// and checks if the OR logic works correctly +func TestPrivilegedWithReadAllowBug(t *testing.T) { + aliceSigner, alicePubkey := generateTestKeypair(t) + _, bobPubkey := generateTestKeypair(t) + _, charliePubkey := generateTestKeypair(t) + _, davePubkey := generateTestKeypair(t) + + // Create policy with privileged AND read_allow + // Expected: OR logic - user can read if in read_allow OR party to event + policyJSON := map[string]interface{}{ + "rules": map[string]interface{}{ + "4": map[string]interface{}{ + "description": "DM - privileged with read_allow", + "privileged": true, + "read_allow": []string{hex.Enc(davePubkey)}, // Dave is in read_allow + }, + }, + } + + policyBytes, err := json.Marshal(policyJSON) + if err != nil { + t.Fatalf("Failed to marshal policy: %v", err) + } + t.Logf("Policy JSON: %s", policyBytes) + + policy, err := New(policyBytes) + if err != nil { + t.Fatalf("Failed to create policy: %v", err) + } + + // Create a kind 4 (DM) event from Alice to Bob (Dave is NOT in p-tag) + ev := createTestEvent(t, aliceSigner, "Secret message from Alice to Bob", 4) + addPTag(ev, bobPubkey) // Bob is recipient, Dave is NOT in p-tag + + t.Logf("Event author: %s", hex.Enc(ev.Pubkey)) + t.Logf("Event p-tags: %v", ev.Tags.GetAll([]byte("p"))) + t.Logf("Dave (in read_allow, NOT in p-tag): %s", hex.Enc(davePubkey)) + + // Test 1: Alice (author, party) should be able to read via privileged + t.Run("alice_party_can_read", func(t *testing.T) { + allowed, err := policy.CheckPolicy("read", ev, alicePubkey, "127.0.0.1") + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if !allowed { + t.Error("BUG! Author should be able to read (privileged)") + } + }) + + // Test 2: Bob (in p-tag, party) should be able to read via privileged + t.Run("bob_party_can_read", func(t *testing.T) { + allowed, err := policy.CheckPolicy("read", ev, bobPubkey, "127.0.0.1") + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if !allowed { + t.Error("BUG! Recipient (in p-tag) should be able to read (privileged)") + } + }) + + // Test 3: Dave (in read_allow, NOT party) should be able to read via OR logic + t.Run("dave_read_allow_can_read", func(t *testing.T) { + allowed, err := policy.CheckPolicy("read", ev, davePubkey, "127.0.0.1") + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if !allowed { + t.Error("BUG! User in read_allow should be able to read (OR logic)") + } + }) + + // Test 4: Charlie (NOT in read_allow, NOT party) should NOT be able to read + t.Run("charlie_denied", func(t *testing.T) { + allowed, err := policy.CheckPolicy("read", ev, charliePubkey, "127.0.0.1") + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if allowed { + t.Error("BUG! Third party should NOT be able to read") + } + }) +} + +// TestNoReadAllowNoPrivileged tests what happens when a rule has neither +// read_allow nor privileged - should default to allow +func TestNoReadAllowNoPrivileged(t *testing.T) { + aliceSigner, _ := generateTestKeypair(t) + _, charliePubkey := generateTestKeypair(t) + + // Create policy with a rule that has no read_allow and no privileged + policyJSON := map[string]interface{}{ + "default_policy": "allow", + "rules": map[string]interface{}{ + "1": map[string]interface{}{ + "description": "Regular text note - no restrictions", + }, + }, + } + + policyBytes, err := json.Marshal(policyJSON) + if err != nil { + t.Fatalf("Failed to marshal policy: %v", err) + } + t.Logf("Policy JSON: %s", policyBytes) + + policy, err := New(policyBytes) + if err != nil { + t.Fatalf("Failed to create policy: %v", err) + } + + // Check that privileged is false for this rule + rule := policy.rules[1] + t.Logf("Privileged: %v, ReadAllow: %v", rule.Privileged, rule.ReadAllow) + + // Create a kind 1 event + ev := createTestEvent(t, aliceSigner, "Hello world", 1) + + // Test: Third party should be able to read (no restrictions) + t.Run("charlie_can_read_unrestricted", func(t *testing.T) { + allowed, err := policy.CheckPolicy("read", ev, charliePubkey, "127.0.0.1") + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if !allowed { + t.Error("Third party should be able to read unrestricted events") + } + }) + + // Test: Unauthenticated should also be able to read + t.Run("unauthenticated_can_read_unrestricted", func(t *testing.T) { + allowed, err := policy.CheckPolicy("read", ev, nil, "127.0.0.1") + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if !allowed { + t.Error("Unauthenticated user should be able to read unrestricted events") + } + }) +} diff --git a/pkg/policy/read_access_test.go b/pkg/policy/read_access_test.go index 58ec462..6a092e1 100644 --- a/pkg/policy/read_access_test.go +++ b/pkg/policy/read_access_test.go @@ -26,7 +26,7 @@ func TestReadAllowLogic(t *testing.T) { // Create policy: Only Bob can READ kind 30166 events policy := &P{ DefaultPolicy: "allow", - Rules: map[int]Rule{ + rules: map[int]Rule{ 30166: { Description: "Private server heartbeat events", ReadAllow: []string{hex.Enc(bobPubkey)}, // Only Bob can read @@ -94,7 +94,7 @@ func TestReadDenyLogic(t *testing.T) { // Create policy: Charlie cannot READ kind 1 events (but others can) policy := &P{ DefaultPolicy: "allow", - Rules: map[int]Rule{ + rules: map[int]Rule{ 1: { Description: "Test events", ReadDeny: []string{hex.Enc(charliePubkey)}, // Charlie cannot read @@ -305,7 +305,7 @@ func TestReadAllowWithPrivileged(t *testing.T) { // Create policy: Kind 100 is privileged AND has read_allow policy := &P{ DefaultPolicy: "allow", - Rules: map[int]Rule{ + rules: map[int]Rule{ 100: { Description: "Privileged with read_allow", Privileged: true, @@ -379,7 +379,7 @@ func TestReadAllowWriteAllowIndependent(t *testing.T) { // - Charlie can do neither policy := &P{ DefaultPolicy: "allow", - Rules: map[int]Rule{ + rules: map[int]Rule{ 200: { Description: "Write/Read separation test", WriteAllow: []string{hex.Enc(alicePubkey)}, // Only Alice can write @@ -472,7 +472,7 @@ func TestReadAccessEdgeCases(t *testing.T) { policy := &P{ DefaultPolicy: "allow", - Rules: map[int]Rule{ + rules: map[int]Rule{ 300: { Description: "Test edge cases", ReadAllow: []string{"somepubkey"}, // Non-empty ReadAllow diff --git a/pkg/version/version b/pkg/version/version index 29e939c..5db15ea 100644 --- a/pkg/version/version +++ b/pkg/version/version @@ -1 +1 @@ -v0.30.0 \ No newline at end of file +v0.30.1 \ No newline at end of file