diff --git a/cmd/config/plugins.go b/cmd/config/plugins.go index a50d47075..fc1738337 100644 --- a/cmd/config/plugins.go +++ b/cmd/config/plugins.go @@ -82,8 +82,8 @@ func runConfigPluginsShow(f *cmdutil.Factory) error { "version": p.Version, "capabilities": p.Capabilities, } - if p.Rule != nil { - entry["rule"] = p.Rule + if len(p.Rules) > 0 { + entry["rules"] = p.Rules } entry["hooks"] = map[string]any{ "observers": p.Observers, diff --git a/cmd/config/policy.go b/cmd/config/policy.go index 78f2b10a7..16217c4e1 100644 --- a/cmd/config/policy.go +++ b/cmd/config/policy.go @@ -59,16 +59,20 @@ func runConfigPolicyShow(f *cmdutil.Factory) error { "source_name": sourceName, "denied_paths": active.DeniedPaths, } - if active.Rule != nil { - out["rule"] = map[string]any{ - "name": active.Rule.Name, - "description": active.Rule.Description, - "allow": active.Rule.Allow, - "deny": active.Rule.Deny, - "max_risk": active.Rule.MaxRisk, - "identities": active.Rule.Identities, - "allow_unannotated": active.Rule.AllowUnannotated, + if len(active.Rules) > 0 { + rules := make([]map[string]any, 0, len(active.Rules)) + for _, r := range active.Rules { + rules = append(rules, map[string]any{ + "name": r.Name, + "description": r.Description, + "allow": r.Allow, + "deny": r.Deny, + "max_risk": r.MaxRisk, + "identities": r.Identities, + "allow_unannotated": r.AllowUnannotated, + }) } + out["rules"] = rules } output.PrintJson(f.IOStreams.Out, out) return nil diff --git a/cmd/config/policy_test.go b/cmd/config/policy_test.go index 05d8a180b..502d6b2a6 100644 --- a/cmd/config/policy_test.go +++ b/cmd/config/policy_test.go @@ -57,7 +57,7 @@ func TestConfigPolicyShow_PluginActive(t *testing.T) { MaxRisk: "read", } cmdpolicy.SetActive(&cmdpolicy.ActivePolicy{ - Rule: rule, + Rules: []*platform.Rule{rule}, Source: cmdpolicy.ResolveSource{ Kind: cmdpolicy.SourcePlugin, Name: "secaudit", @@ -83,12 +83,16 @@ func TestConfigPolicyShow_PluginActive(t *testing.T) { if got["denied_paths"] != float64(42) { t.Errorf("denied_paths = %v, want 42", got["denied_paths"]) } - ruleMap, ok := got["rule"].(map[string]any) + rulesAny, ok := got["rules"].([]any) + if !ok || len(rulesAny) != 1 { + t.Fatalf("rules field missing or wrong shape: %v", got["rules"]) + } + ruleMap, ok := rulesAny[0].(map[string]any) if !ok { - t.Fatalf("rule field missing or wrong type") + t.Fatalf("rules[0] wrong type") } if ruleMap["name"] != "secaudit" { - t.Errorf("rule.name = %v", ruleMap["name"]) + t.Errorf("rules[0].name = %v", ruleMap["name"]) } } @@ -101,7 +105,7 @@ func TestConfigPolicyShow_YamlSourceNameIsEmpty(t *testing.T) { t.Cleanup(cmdpolicy.ResetActiveForTesting) cmdpolicy.SetActive(&cmdpolicy.ActivePolicy{ - Rule: &platform.Rule{Name: "my-yaml-rule"}, + Rules: []*platform.Rule{{Name: "my-yaml-rule"}}, Source: cmdpolicy.ResolveSource{ Kind: cmdpolicy.SourceYAML, Name: "/Users/alice/.lark-cli/policy.yml", diff --git a/cmd/platform_bootstrap.go b/cmd/platform_bootstrap.go index ef2ac6b73..9b1b57ac3 100644 --- a/cmd/platform_bootstrap.go +++ b/cmd/platform_bootstrap.go @@ -36,47 +36,71 @@ const userPolicyFileName = "policy.yml" // pluginRules carries Plugin.Restrict() contributions collected from // the InstallAll phase; nil/empty is fine. func applyUserPolicyPruning(rootCmd *cobra.Command, pluginRules []cmdpolicy.PluginRule) error { - yamlPath, err := userPolicyPath() - if err != nil { - // No user home dir means we cannot locate the policy. Treat - // the same as "file missing": no pruning, no error. This keeps - // non-interactive CI environments (no HOME set) running. - yamlPath = "" + // Plugin rules shadow the yaml source entirely (Resolve: plugin > + // yaml). When a plugin contributed rules we therefore do NOT even + // read ~/.lark-cli/policy.yml: build.go fail-CLOSES on any policy + // error once a plugin is present, so reading a malformed yaml here + // would let an unrelated broken file on the user's machine abort a + // plugin-governed binary -- exactly the file the plugin is supposed + // to shadow. Skipping the read keeps the shadow contract honest. + var ( + yamlRules []*platform.Rule + yamlPath string + ) + if len(pluginRules) == 0 { + p, perr := userPolicyPath() + if perr != nil { + // No user home dir means we cannot locate the policy. Treat + // the same as "file missing": no pruning, no error. This keeps + // non-interactive CI environments (no HOME set) running. + p = "" + } + yamlPath = p + loaded, lerr := cmdpolicy.LoadYAMLPolicy(yamlPath) + if lerr != nil { + // Yaml-only failures are fail-OPEN at the caller (warn and + // continue), but the active-policy snapshot is process-global + // and may still carry data from a previous build in long-lived + // embedders / tests. Clear it explicitly so `config policy + // show` reports "no policy" instead of a stale rule that + // doesn't reflect the current command tree. + cmdpolicy.SetActive(nil) + return lerr + } + yamlRules = loaded } - yamlRule, err := cmdpolicy.LoadYAMLPolicy(yamlPath) - if err != nil { - // Yaml-only failures are fail-OPEN at the caller (warn and - // continue), but the active-policy snapshot is process-global - // and may still carry data from a previous build in long-lived - // embedders / tests. Clear it explicitly so `config policy - // show` reports "no policy" instead of a stale rule that - // doesn't reflect the current command tree. - cmdpolicy.SetActive(nil) - return err - } - - rule, source, err := cmdpolicy.Resolve(cmdpolicy.Sources{ + rules, source, err := cmdpolicy.Resolve(cmdpolicy.Sources{ PluginRules: pluginRules, - YAMLRule: yamlRule, + YAMLRules: yamlRules, YAMLPath: yamlPath, }) if err != nil { cmdpolicy.SetActive(nil) return err } - if rule == nil { + if len(rules) == 0 { cmdpolicy.SetActive(&cmdpolicy.ActivePolicy{Source: source}) return nil } - engine := cmdpolicy.New(rule) + // RuleName attributes a denial to a specific rule in the envelope. + // With a single rule that is unambiguous and preserves the legacy + // envelope verbatim; with several rules a denial means "no rule + // granted it", which has no single owner, so the field is left empty + // and reason_code=no_matching_rule carries the meaning instead. + ruleName := "" + if len(rules) == 1 { + ruleName = rules[0].Name + } + + engine := cmdpolicy.NewSet(rules) decisions := engine.EvaluateAll(rootCmd) - denied := cmdpolicy.BuildDeniedByPath(rootCmd, decisions, source, rule.Name) + denied := cmdpolicy.BuildDeniedByPath(rootCmd, decisions, source, ruleName) cmdpolicy.Apply(rootCmd, denied) cmdpolicy.SetActive(&cmdpolicy.ActivePolicy{ - Rule: rule, + Rules: rules, Source: source, DeniedPaths: len(denied), }) diff --git a/cmd/platform_bootstrap_test.go b/cmd/platform_bootstrap_test.go index 4fe814453..308eb1636 100644 --- a/cmd/platform_bootstrap_test.go +++ b/cmd/platform_bootstrap_test.go @@ -13,6 +13,8 @@ import ( "github.com/spf13/cobra" + "github.com/larksuite/cli/extension/platform" + "github.com/larksuite/cli/internal/cmdpolicy" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/output" ) @@ -184,6 +186,39 @@ func TestApplyUserPolicyPruning_malformedYamlReturnsError(t *testing.T) { } } +// When a plugin contributed rules, a malformed user policy.yml must NOT +// abort: plugin rules shadow yaml entirely, so the broken file is never +// read. Regression -- previously LoadYAMLPolicy ran first and an +// unrelated broken yaml on the user's machine could fatal a +// plugin-governed binary (build.go fail-CLOSES on policy errors when a +// plugin is present). +func TestApplyUserPolicyPruning_pluginRulesSkipBrokenYaml(t *testing.T) { + cfgDir := tmpHome(t) + t.Cleanup(cmdpolicy.ResetActiveForTesting) + writePolicy(t, cfgDir, "::: not yaml :::") // broken on purpose + + pluginRules := []cmdpolicy.PluginRule{ + {PluginName: "secaudit", Rule: &platform.Rule{ + Name: "docs-only", + Allow: []string{"docs/**"}, + MaxRisk: "write", + }}, + } + root := fakeTree(t) + if err := applyUserPolicyPruning(root, pluginRules); err != nil { + t.Fatalf("plugin rules must shadow (and skip reading) yaml; broken yaml should not error, got %v", err) + } + + // Plugin rule actually applied: im/+send is outside docs/** -> hidden. + if send := findLeaf(t, root, "im", "+send"); !send.Hidden { + t.Errorf("im/+send should be hidden by plugin rule (not in docs/** allow)") + } + // docs/+update is within allow and at/below max_risk -> stays visible. + if update := findLeaf(t, root, "docs", "+update"); update.Hidden { + t.Errorf("docs/+update should remain visible under plugin rule") + } +} + // Semantically-invalid Rule (bad MaxRisk) reaches ValidateRule inside // Resolve and produces an error. This is the safety contract: a typo in // the rule must not silently lower the pruning bar. diff --git a/extension/platform/README.md b/extension/platform/README.md index d2834ddd7..9f9eb56da 100644 --- a/extension/platform/README.md +++ b/extension/platform/README.md @@ -59,7 +59,7 @@ You should see `audit` in the plugin list. | `Observer` | Before / After each command | No (fire-and-forget audit) | | `Wrap` | Around each command's RunE | Yes (return `*AbortError`) | | `On(Startup/Shutdown)` | Process lifecycle | N/A | -| `Restrict(Rule)` | Bootstrap-time, single per binary | Denies whole subtrees | +| `Restrict(Rule)` | Bootstrap-time, ≥1 per plugin | Denies whole subtrees | ### Plugin lifecycle @@ -102,10 +102,17 @@ the rejected dispatch. - A plugin calling `Restrict()` MUST declare `FailClosed`. The Builder flips it automatically; the lower-level `Plugin` interface rejects the mismatch with `restricts_mismatch`. -- Only ONE plugin per binary can call `Restrict()`. Multi-plugin - Restrict is a deliberate `plugin_conflict` error (single-rule - ecosystem assumption). YAML policy at `~/.lark-cli/policy.yml` is - shadowed by any plugin Restrict. +- A plugin may call `Restrict()` more than once; each call adds one + scoped Rule and the engine combines them with **OR** — a command is + allowed when it satisfies every axis (allow / deny / max_risk / + identities) of at least one rule. Note a rule's `deny` is scoped to + that rule only and cannot veto another rule's allow. Only ONE plugin + per binary may contribute rules, though: two DISTINCT plugins each + calling `Restrict()` is a deliberate `multiple_restrict_plugins` error + (single-owner assumption — an independent plugin must not be able to + widen another's policy). YAML policy at `~/.lark-cli/policy.yml` (which + may itself list several rules under `rules:`) is shadowed by any plugin + Restrict. - The `Wrap` factory runs **once per command dispatch**, not at install time. Long-lived state (clients, caches, metrics counters) must live on the Plugin struct or in package-level variables. @@ -115,7 +122,8 @@ the rejected dispatch. - Commands missing a `risk_level` annotation are denied by default when a Rule is active. Set `Rule.AllowUnannotated = true` (or `allow_unannotated: true` in yaml) to opt out during gradual - adoption. + adoption. With several rules this is per-rule: an unannotated command + is allowed as long as one rule that opts in also grants it. - Risk annotation typos (e.g. `"wrtie"`) are always denied with `risk_invalid` plus a "did you mean" suggestion. `AllowUnannotated` does NOT bypass this — typo is a code bug, not a missing @@ -144,8 +152,7 @@ messages are localised and may change between releases. | `duplicate_hook_name` | Same hook name registered twice within a plugin | Yes | | `invalid_hook_registration` | Hook factory returns nil / Wrap chain re-entry / etc. | Yes | | `invalid_rule` | Rule fails ValidateRule (malformed glob, bad MaxRisk, unknown Identity) | Yes | -| `double_restrict` | Plugin called `r.Restrict()` more than once in one Install | Yes | -| `multiple_restrict_plugins` | Two or more plugins each contributed Restrict | Yes | +| `multiple_restrict_plugins` | Two or more DISTINCT plugins each contributed Restrict (one plugin may contribute several rules) | Yes | | `install_failed` | `Plugin.Install` returned a non-nil error | Yes | | `install_panic` | `Plugin.Install` panicked | Yes | @@ -165,6 +172,7 @@ might also be lying about being `FailOpen`). | `write_not_allowed` | Command risk is `write` / `high-risk-write` and exceeds Rule `max_risk` | | `risk_too_high` | Command risk exceeds Rule `max_risk` but is not a write (reserved for future risk levels) | | `identity_mismatch` | Command's `supportedIdentities` does not intersect Rule `identities` | +| `no_matching_rule` | Several rules are active and the command satisfied none of them (the message summarises each rule's own rejection). Single-rule policies keep their specific reason_code instead | | `aggregate_all_denied` | Aggregate stub installed on a parent group because every live child was denied | The `detail.layer` field distinguishes who rejected the call: diff --git a/extension/platform/builder.go b/extension/platform/builder.go index 1bcba749f..479fea824 100644 --- a/extension/platform/builder.go +++ b/extension/platform/builder.go @@ -37,7 +37,7 @@ type Builder struct { caps Capabilities actions []func(Registrar) - rule *Rule + rules []*Rule hookNames map[string]bool errs []error @@ -125,7 +125,8 @@ func (b *Builder) On(event LifecycleEvent, hookName string, fn LifecycleHandler) // sets Restricts=true and FailurePolicy=FailClosed (the framework // requires both to coexist; the builder enforces the pairing so the // plugin author cannot accidentally ship a policy plugin under -// FailOpen). +// FailOpen). It may be called more than once; each call adds one scoped +// Rule and the engine OR-combines them. func (b *Builder) Restrict(rule *Rule) *Builder { if rule == nil { b.errs = append(b.errs, errors.New("Restrict(nil): rule must not be nil")) @@ -133,7 +134,14 @@ func (b *Builder) Restrict(rule *Rule) *Builder { } b.caps.Restricts = true b.caps.FailurePolicy = FailClosed - b.rule = rule + // Defensive clone: capture an independent snapshot so a caller that + // reuses and mutates the same *Rule across multiple Restrict calls + // gets distinct entries (mirrors the staging registrar's clone). + cp := *rule + cp.Allow = append([]string(nil), rule.Allow...) + cp.Deny = append([]string(nil), rule.Deny...) + cp.Identities = append([]Identity(nil), rule.Identities...) + b.rules = append(b.rules, &cp) return b } @@ -143,7 +151,7 @@ func (b *Builder) Restrict(rule *Rule) *Builder { // The Restrict + FailOpen mismatch is checked here, not in the chained // setters, because the two methods may be called in either order. func (b *Builder) Build() (Plugin, error) { - if b.rule != nil && b.caps.FailurePolicy == FailOpen { + if len(b.rules) > 0 && b.caps.FailurePolicy == FailOpen { b.errs = append(b.errs, errors.New( "Restrict() requires FailClosed; do not call FailOpen() after Restrict()")) } @@ -155,7 +163,7 @@ func (b *Builder) Build() (Plugin, error) { version: b.version, caps: b.caps, actions: b.actions, - rule: b.rule, + rules: b.rules, }, nil } @@ -198,15 +206,15 @@ type builtPlugin struct { version string caps Capabilities actions []func(Registrar) - rule *Rule + rules []*Rule } func (p *builtPlugin) Name() string { return p.name } func (p *builtPlugin) Version() string { return p.version } func (p *builtPlugin) Capabilities() Capabilities { return p.caps } func (p *builtPlugin) Install(r Registrar) error { - if p.rule != nil { - r.Restrict(p.rule) + for _, rule := range p.rules { + r.Restrict(rule) } for _, action := range p.actions { action(r) diff --git a/extension/platform/builder_test.go b/extension/platform/builder_test.go index 541271a1b..7a813665f 100644 --- a/extension/platform/builder_test.go +++ b/extension/platform/builder_test.go @@ -17,7 +17,8 @@ type recorder struct { observers int wrappers int lifecycles int - rule *platform.Rule + rule *platform.Rule // last rule (existing single-rule assertions) + rules []*platform.Rule // every rule, in Restrict order } func (r *recorder) Observe(platform.When, string, platform.Selector, platform.Observer) { @@ -25,7 +26,39 @@ func (r *recorder) Observe(platform.When, string, platform.Selector, platform.Ob } func (r *recorder) Wrap(string, platform.Selector, platform.Wrapper) { r.wrappers++ } func (r *recorder) On(platform.LifecycleEvent, string, platform.LifecycleHandler) { r.lifecycles++ } -func (r *recorder) Restrict(rule *platform.Rule) { r.rule = rule } +func (r *recorder) Restrict(rule *platform.Rule) { + r.rule = rule + r.rules = append(r.rules, rule) +} + +// Restrict must snapshot each rule: a caller that reuses and mutates the +// same *Rule object across two Restrict calls must still get two distinct +// rules at Install time, not two pointers to the last mutation. +func TestBuilder_restrictClonesEachRule(t *testing.T) { + shared := &platform.Rule{Name: "docs-ro", Allow: []string{"docs/**"}, MaxRisk: platform.RiskRead} + b := platform.NewPlugin("p", "0").Restrict(shared) + // Reuse and mutate the same object, then register it again. + shared.Name = "im-rw" + shared.Allow[0] = "im/**" + shared.MaxRisk = platform.RiskWrite + p, err := b.Restrict(shared).Build() + if err != nil { + t.Fatalf("Build: %v", err) + } + r := &recorder{} + if err := p.Install(r); err != nil { + t.Fatalf("Install: %v", err) + } + if len(r.rules) != 2 { + t.Fatalf("got %d rules, want 2", len(r.rules)) + } + if r.rules[0].Name != "docs-ro" || r.rules[0].Allow[0] != "docs/**" || r.rules[0].MaxRisk != platform.RiskRead { + t.Errorf("rule[0] leaked later mutation: %+v", r.rules[0]) + } + if r.rules[1].Name != "im-rw" || r.rules[1].Allow[0] != "im/**" { + t.Errorf("rule[1] = %+v, want im-rw / im/**", r.rules[1]) + } +} func TestBuilder_basicAssembly(t *testing.T) { p, err := platform.NewPlugin("audit", "0.1.0"). diff --git a/extension/platform/registrar.go b/extension/platform/registrar.go index 8774050bf..36df85f45 100644 --- a/extension/platform/registrar.go +++ b/extension/platform/registrar.go @@ -13,9 +13,10 @@ package platform // identifier is "{plugin}.{hook}". A plugin cannot register two hooks // with the same name in the same Install call. // -// Restrict may be called at most once per plugin; multiple plugins -// contributing Restrict() is a configuration error (the resolver -// aborts startup). +// Restrict may be called multiple times per plugin; each call adds one +// scoped Rule (OR-combined by the engine). Two or more DISTINCT plugins +// contributing Restrict() is a configuration error (the resolver aborts +// startup). type Registrar interface { // Observe registers a side-effect-only command hook at the given // When stage. The selector decides which commands it fires on. @@ -29,8 +30,9 @@ type Registrar interface { // On registers a lifecycle handler for the given event. On(event LifecycleEvent, hookName string, fn LifecycleHandler) - // Restrict contributes a pruning Rule. The framework merges it - // with the yaml-sourced Rule using single-rule semantics: plugin - // rule wins, but two plugins both calling Restrict abort startup. + // Restrict contributes a pruning Rule. May be called more than once + // to declare several scoped grants (OR-combined by the engine). + // Plugin rules take precedence over the yaml source; two distinct + // plugins both calling Restrict abort startup. Restrict(r *Rule) } diff --git a/internal/cmdpolicy/active.go b/internal/cmdpolicy/active.go index 488d641c1..7a766e29d 100644 --- a/internal/cmdpolicy/active.go +++ b/internal/cmdpolicy/active.go @@ -15,8 +15,12 @@ import ( // it hide?". // // Set once at bootstrap time; consumed read-only thereafter. +// +// Rules is the full set the winning source contributed (one rule for the +// common single-rule case, several when a plugin or yaml declares scoped +// grants). nil/empty means "no rule applied". type ActivePolicy struct { - Rule *platform.Rule + Rules []*platform.Rule Source ResolveSource DeniedPaths int // number of commands the engine marked as denied (post-aggregation) } @@ -56,20 +60,26 @@ func GetActive() *ActivePolicy { return cloneActivePolicy(activePolicy) } -// cloneActivePolicy deep-copies the top-level struct plus the embedded -// Rule's slice fields. Other fields (Source, DeniedPaths) are value -// types so the struct copy already disjoints them. +// cloneActivePolicy deep-copies the top-level struct, the Rules slice, and +// each Rule's own slice fields. Other fields (Source, DeniedPaths) are +// value types so the struct copy already disjoints them. func cloneActivePolicy(in *ActivePolicy) *ActivePolicy { if in == nil { return nil } cp := *in - if in.Rule != nil { - rule := *in.Rule - rule.Allow = append([]string(nil), in.Rule.Allow...) - rule.Deny = append([]string(nil), in.Rule.Deny...) - rule.Identities = append([]platform.Identity(nil), in.Rule.Identities...) - cp.Rule = &rule + if in.Rules != nil { + cp.Rules = make([]*platform.Rule, len(in.Rules)) + for i, r := range in.Rules { + if r == nil { + continue + } + rule := *r + rule.Allow = append([]string(nil), r.Allow...) + rule.Deny = append([]string(nil), r.Deny...) + rule.Identities = append([]platform.Identity(nil), r.Identities...) + cp.Rules[i] = &rule + } } return &cp } diff --git a/internal/cmdpolicy/engine.go b/internal/cmdpolicy/engine.go index c2e7e0162..3b0e2c1eb 100644 --- a/internal/cmdpolicy/engine.go +++ b/internal/cmdpolicy/engine.go @@ -17,6 +17,7 @@ package cmdpolicy import ( "fmt" + "strings" "github.com/bmatcuk/doublestar/v4" "github.com/spf13/cobra" @@ -36,16 +37,45 @@ type Decision struct { Reason string // human-readable } -// Engine evaluates a Rule against the command tree. It is stateless except -// for the Rule snapshot it was constructed with. +// Engine evaluates a set of Rules against the command tree with OR +// semantics: a command is allowed when it satisfies every axis of AT +// LEAST ONE rule. It is stateless except for the Rule snapshot it was +// constructed with. type Engine struct { - rule *platform.Rule + rules []*platform.Rule } -// New returns an Engine bound to a Rule. A nil Rule means "no user-layer -// restriction" -- EvaluateOne always returns Allowed=true. +// New returns an Engine bound to a single Rule. A nil Rule means "no +// user-layer restriction" -- EvaluateOne always returns Allowed=true. +// It is the ergonomic single-rule constructor, kept so existing callers +// (and the single-rule decision path) stay byte-for-byte unchanged. func New(rule *platform.Rule) *Engine { - return &Engine{rule: rule} + if rule == nil { + return &Engine{} + } + return &Engine{rules: []*platform.Rule{rule}} +} + +// NewSet returns an Engine bound to a set of Rules evaluated with OR +// semantics. An empty/nil slice means "no user-layer restriction". nil +// entries are dropped so callers may pass a slice with gaps without a +// separate filter step. +// +// With exactly one rule the behaviour is identical to New(rule): the +// rejection Decision is returned verbatim. With multiple rules a command +// rejected by all of them gets the aggregate reason_code +// "no_matching_rule" (see mergeDenials). +func NewSet(rules []*platform.Rule) *Engine { + cleaned := make([]*platform.Rule, 0, len(rules)) + for _, r := range rules { + if r != nil { + cleaned = append(cleaned, r) + } + } + if len(cleaned) == 0 { + return &Engine{} + } + return &Engine{rules: cleaned} } // EvaluateAll walks the command tree and evaluates every **runnable** @@ -81,27 +111,29 @@ func (e *Engine) EvaluateAll(root *cobra.Command) map[string]Decision { } // EvaluateOne returns the user-layer decision for a single command. Always -// Allowed=true when the engine has no Rule. +// Allowed=true when the engine has no Rule. With multiple rules the +// decision is the OR over per-rule evaluations: the command is allowed as +// soon as one rule grants it; if every rule rejects it, the rejections are +// merged (see mergeDenials). func (e *Engine) EvaluateOne(cmd *cobra.Command) Decision { - if e.rule == nil { + if len(e.rules) == 0 { return Decision{Allowed: true} } - r := e.rule path := CanonicalPath(cmd) if IsDiagnosticPath(path) { return Decision{Allowed: true} } - // A registered Rule expresses intent over the closed risk taxonomy - // (read / write / high-risk-write). Two ways a command can fall - // outside that taxonomy: + // risk_invalid is a property of the COMMAND's own annotation (the + // annotation exists but is a typo / not in the closed taxonomy + // read / write / high-risk-write). It is independent of any Rule and + // is always fail-closed regardless of AllowUnannotated -- a typo is a + // code bug, not a migration phase. So it is checked once up front, + // before the per-rule OR loop, and short-circuits to deny. // - // - "absent" (no risk_level annotation) — fail-closed by default, - // but Rule.AllowUnannotated=true opts out for gradual adoption. - // - "invalid" (annotation exists but is a typo / not in the - // closed enum) — always fail-closed regardless of - // AllowUnannotated. Typo is a code bug, not a migration phase. + // The "absent" case (no risk_level annotation at all) is per-rule: + // each rule's AllowUnannotated decides, so it lives inside evalRule. cmdRiskStr, hasRisk := cmdmeta.Risk(cmd) cmdRisk := platform.Risk(cmdRiskStr) var ( @@ -117,7 +149,31 @@ func (e *Engine) EvaluateOne(cmd *cobra.Command) Decision { Reason: fmt.Sprintf("invalid risk %q; did you mean %q?", cmdRiskStr, suggestRisk(cmdRiskStr)), } } - } else if !r.AllowUnannotated { + } + + // OR across rules: the first rule that fully grants the command wins. + denials := make([]Decision, 0, len(e.rules)) + for _, r := range e.rules { + d := evalRule(r, path, cmd, hasRisk, cmdRisk, cmdRank, cmdRankOk) + if d.Allowed { + return Decision{Allowed: true} + } + denials = append(denials, d) + } + return mergeDenials(e.rules, denials) +} + +// evalRule applies one Rule's four-axis AND filter to a command whose +// risk annotation has already been parsed by EvaluateOne (risk_invalid is +// handled there). cmdRankOk is false only when the command is unannotated +// (hasRisk=false); a present-but-invalid risk never reaches here. Returns +// Allowed=true only when the command clears every axis of this rule. +func evalRule(r *platform.Rule, path string, cmd *cobra.Command, hasRisk bool, cmdRisk platform.Risk, cmdRank int, cmdRankOk bool) Decision { + // Unannotated gate: fail-closed unless THIS rule opts out. A command + // with no risk_level annotation can still be granted by a rule that + // sets AllowUnannotated=true (gradual-adoption opt-in); other rules in + // the set reject it here and the OR moves on. + if !hasRisk && !r.AllowUnannotated { return Decision{ Allowed: false, ReasonCode: "risk_not_annotated", @@ -125,7 +181,9 @@ func (e *Engine) EvaluateOne(cmd *cobra.Command) Decision { } } - // Axis 1: Deny has priority. + // Axis 1: Deny has priority. Note OR semantics scope a rule's Deny to + // that rule only -- it cannot veto another rule's Allow. A command to + // block everywhere must be denied (or simply not allowed) by every rule. if matched, ok := firstMatch(r.Deny, path); ok { return Decision{ Allowed: false, @@ -171,6 +229,34 @@ func (e *Engine) EvaluateOne(cmd *cobra.Command) Decision { return Decision{Allowed: true} } +// mergeDenials collapses the per-rule rejections into a single Decision +// for a command that no rule granted. denials is parallel to rules (same +// order, one entry per rule, all Allowed=false). +// +// With exactly one rule the original rejection is returned verbatim, so +// single-rule envelopes are byte-for-byte identical to the pre-multi-rule +// behaviour (reason_code / reason unchanged). With multiple rules the +// rejection is the aggregate reason_code "no_matching_rule"; its Reason +// enumerates each rule's own rejection for debugging. +func mergeDenials(rules []*platform.Rule, denials []Decision) Decision { + if len(denials) == 1 { + return denials[0] + } + parts := make([]string, len(denials)) + for i, d := range denials { + name := rules[i].Name + if name == "" { + name = fmt.Sprintf("#%d", i) + } + parts[i] = fmt.Sprintf("%s: %s", name, d.ReasonCode) + } + return Decision{ + Allowed: false, + ReasonCode: "no_matching_rule", + Reason: fmt.Sprintf("no rule grants this command (%s)", strings.Join(parts, "; ")), + } +} + // BuildDeniedByPath converts engine Decisions to a deniedByPath map keyed // by canonical path. It performs the parent-group aggregation defined in // the tech doc: a non-runnable parent whose every runnable descendant is diff --git a/internal/cmdpolicy/engine_test.go b/internal/cmdpolicy/engine_test.go index e2e9ca559..992b599b4 100644 --- a/internal/cmdpolicy/engine_test.go +++ b/internal/cmdpolicy/engine_test.go @@ -398,6 +398,93 @@ func TestEvaluate_unknownIdentitiesIsAllow(t *testing.T) { } } +// --- Multi-rule (OR) semantics --- + +// Two scoped rules (docs read-only, im writable) are OR-combined: a +// command is allowed when it satisfies ANY rule. This is the headline +// multi-rule use case -- different command groups need different risk +// ceilings within one policy. +func TestEvaluate_multiRuleOR(t *testing.T) { + root := buildTree() + e := cmdpolicy.NewSet([]*platform.Rule{ + {Name: "docs-ro", Allow: []string{"docs/**"}, MaxRisk: "read"}, + {Name: "im-rw", Allow: []string{"im/**"}, MaxRisk: "write"}, + }) + got := e.EvaluateAll(root) + + // docs/+fetch (read) clears docs-ro. + if !got["docs/+fetch"].Allowed { + t.Errorf("docs/+fetch should be allowed by docs-ro") + } + // im/+send (write) clears im-rw even though docs-ro rejects it. + if !got["im/+send"].Allowed { + t.Errorf("im/+send (write) should be allowed by im-rw") + } + // docs/+update (write) exceeds docs-ro's read ceiling AND is outside + // im-rw's allow list -> rejected by both -> no_matching_rule. + if got["docs/+update"].Allowed { + t.Fatalf("docs/+update should be denied: read-only in docs, not allowed in im") + } + if rc := got["docs/+update"].ReasonCode; rc != "no_matching_rule" { + t.Errorf("docs/+update ReasonCode = %q, want no_matching_rule", rc) + } +} + +// Identity can differ per rule: docs limited to user, im open to bot. +// This is the second half of the requirement -- some commands restrict +// identity, others allow the bot identity. +func TestEvaluate_multiRulePerRuleIdentity(t *testing.T) { + root := buildTree() + e := cmdpolicy.NewSet([]*platform.Rule{ + {Name: "docs-user", Allow: []string{"docs/**"}, MaxRisk: "write", Identities: []platform.Identity{"user"}}, + {Name: "im-bot", Allow: []string{"im/**"}, MaxRisk: "write", Identities: []platform.Identity{"bot"}}, + }) + got := e.EvaluateAll(root) + + // docs/+update identities=[user] -> docs-user grants. + if !got["docs/+update"].Allowed { + t.Errorf("docs/+update (user) should be allowed by docs-user") + } + // im/+send identities=[bot] -> im-bot grants. + if !got["im/+send"].Allowed { + t.Errorf("im/+send (bot) should be allowed by im-bot") + } + // docs/+delete-doc is high-risk-write -> exceeds both ceilings -> denied. + if got["docs/+delete-doc"].Allowed { + t.Errorf("docs/+delete-doc (high-risk-write) should be denied by both rules") + } +} + +// NewSet with a single rule must behave exactly like New: the per-rule +// rejection (not the aggregate no_matching_rule) is preserved so the +// single-rule envelope is unchanged. +func TestEvaluate_newSetSingleRuleKeepsReason(t *testing.T) { + root := buildTree() + e := cmdpolicy.NewSet([]*platform.Rule{ + {Allow: []string{"docs/**"}}, + }) + got := e.EvaluateAll(root) + if got["im/+send"].Allowed { + t.Fatalf("im/+send should be denied by docs-only rule") + } + if rc := got["im/+send"].ReasonCode; rc != "domain_not_allowed" { + t.Errorf("single-rule reason must be preserved verbatim, got %q want domain_not_allowed", rc) + } +} + +// NewSet drops nil entries; an all-nil/empty set means "no restriction". +func TestNewSet_emptyAndNilMeansNoRestriction(t *testing.T) { + root := buildTree() + for _, rules := range [][]*platform.Rule{nil, {}, {nil}} { + got := cmdpolicy.NewSet(rules).EvaluateAll(root) + for path, d := range got { + if !d.Allowed { + t.Fatalf("empty/nil rule set must allow all, got deny for %s", path) + } + } + } +} + // Apply must install denyStubs only on Layer="policy" entries. A // "strict_mode" denial in the same map must be left for // applyStrictModeDenials in cmd/. diff --git a/internal/cmdpolicy/resolver.go b/internal/cmdpolicy/resolver.go index d70335f58..3afd6fcf3 100644 --- a/internal/cmdpolicy/resolver.go +++ b/internal/cmdpolicy/resolver.go @@ -33,44 +33,69 @@ type PluginRule struct { type Sources struct { PluginRules []PluginRule - YAMLRule *platform.Rule + YAMLRules []*platform.Rule YAMLPath string } -var ErrMultipleRestricts = errors.New("multiple plugins called Restrict; only one is permitted") +var ErrMultipleRestricts = errors.New("multiple plugins called Restrict; only one plugin may own the policy") -// Resolve picks by precedence: plugin > yaml > none. Pure function; load -// yaml via LoadYAMLPolicy first. Winner is validated. -func Resolve(s Sources) (*platform.Rule, ResolveSource, error) { - if len(s.PluginRules) > 1 { - names := make([]string, len(s.PluginRules)) - for i, pr := range s.PluginRules { - names[i] = pr.PluginName - } - return nil, ResolveSource{}, fmt.Errorf("%w: %v", ErrMultipleRestricts, names) +// Resolve picks by precedence: plugin > yaml > none, returning the full +// rule set the winning source contributes. Pure function; load yaml via +// LoadYAMLPolicy first. Every returned rule is validated. +// +// Multi-rule semantics (single owner): one plugin may contribute several +// rules (each a scoped grant, OR-combined by the engine), but two or more +// DISTINCT plugins contributing rules is still a configuration error -- +// the resolver aborts so independent plugins cannot silently widen each +// other's policy. yaml may likewise carry several rules under "rules:". +func Resolve(s Sources) ([]*platform.Rule, ResolveSource, error) { + owners := distinctOwners(s.PluginRules) + if len(owners) > 1 { + return nil, ResolveSource{}, fmt.Errorf("%w: %v", ErrMultipleRestricts, owners) } - if len(s.PluginRules) == 1 { - pr := s.PluginRules[0] - if err := ValidateRule(pr.Rule); err != nil { - return nil, ResolveSource{}, fmt.Errorf("plugin %q rule invalid: %w", pr.PluginName, err) + if len(s.PluginRules) > 0 { + rules := make([]*platform.Rule, 0, len(s.PluginRules)) + for _, pr := range s.PluginRules { + if err := ValidateRule(pr.Rule); err != nil { + return nil, ResolveSource{}, fmt.Errorf("plugin %q rule invalid: %w", pr.PluginName, err) + } + rules = append(rules, pr.Rule) } - return pr.Rule, ResolveSource{Kind: SourcePlugin, Name: pr.PluginName}, nil + return rules, ResolveSource{Kind: SourcePlugin, Name: owners[0]}, nil } - if s.YAMLRule != nil { - if err := ValidateRule(s.YAMLRule); err != nil { - return nil, ResolveSource{}, fmt.Errorf("policy yaml %q: %w", s.YAMLPath, err) + if len(s.YAMLRules) > 0 { + for _, r := range s.YAMLRules { + if err := ValidateRule(r); err != nil { + return nil, ResolveSource{}, fmt.Errorf("policy yaml %q: %w", s.YAMLPath, err) + } } - return s.YAMLRule, ResolveSource{Kind: SourceYAML, Name: s.YAMLPath}, nil + return s.YAMLRules, ResolveSource{Kind: SourceYAML, Name: s.YAMLPath}, nil } return nil, ResolveSource{Kind: SourceNone}, nil } +// distinctOwners returns the unique plugin names contributing a rule, in +// first-seen order. A single plugin contributing N rules collapses to one +// owner; that is the case the single-owner check below permits. +func distinctOwners(prs []PluginRule) []string { + seen := map[string]bool{} + owners := make([]string, 0, len(prs)) + for _, pr := range prs { + if !seen[pr.PluginName] { + seen[pr.PluginName] = true + owners = append(owners, pr.PluginName) + } + } + return owners +} + // LoadYAMLPolicy returns (nil, nil) when path is empty or file is absent, -// so callers can pass the result straight into Sources.YAMLRule. -func LoadYAMLPolicy(path string) (*platform.Rule, error) { +// so callers can pass the result straight into Sources.YAMLRules. A +// present file yields one or more rules (see yaml.Parse). +func LoadYAMLPolicy(path string) ([]*platform.Rule, error) { if path == "" { return nil, nil } @@ -84,9 +109,9 @@ func LoadYAMLPolicy(path string) (*platform.Rule, error) { if err != nil { return nil, fmt.Errorf("read policy yaml %q: %w", path, err) } - rule, err := pyaml.Parse(data) + rules, err := pyaml.Parse(data) if err != nil { return nil, fmt.Errorf("policy yaml %q: %w", path, err) } - return rule, nil + return rules, nil } diff --git a/internal/cmdpolicy/resolver_test.go b/internal/cmdpolicy/resolver_test.go index 1631cb6c7..e687574db 100644 --- a/internal/cmdpolicy/resolver_test.go +++ b/internal/cmdpolicy/resolver_test.go @@ -21,23 +21,45 @@ func TestResolve_singlePluginWins(t *testing.T) { if err != nil { t.Fatalf("Resolve err: %v", err) } - if got != rule || src.Kind != cmdpolicy.SourcePlugin || src.Name != "secaudit" { + if len(got) != 1 || got[0] != rule || src.Kind != cmdpolicy.SourcePlugin || src.Name != "secaudit" { t.Fatalf("Resolve = (%v, %+v)", got, src) } } +// A single plugin may contribute several rules (each a scoped grant). They +// are all returned, in registration order, under one plugin source. +func TestResolve_singlePluginMultipleRules(t *testing.T) { + r1 := &platform.Rule{Name: "docs-ro", Allow: []string{"docs/**"}, MaxRisk: "read"} + r2 := &platform.Rule{Name: "im-rw", Allow: []string{"im/**"}, MaxRisk: "write"} + got, src, err := cmdpolicy.Resolve(cmdpolicy.Sources{ + PluginRules: []cmdpolicy.PluginRule{ + {PluginName: "secaudit", Rule: r1}, + {PluginName: "secaudit", Rule: r2}, + }, + }) + if err != nil { + t.Fatalf("Resolve err: %v", err) + } + if len(got) != 2 || got[0] != r1 || got[1] != r2 { + t.Fatalf("expected both rules in order, got %v", got) + } + if src.Kind != cmdpolicy.SourcePlugin || src.Name != "secaudit" { + t.Fatalf("source = %+v, want plugin:secaudit", src) + } +} + func TestResolve_pluginShadowsYaml(t *testing.T) { pluginRule := &platform.Rule{Name: "from-plugin"} yamlRule := &platform.Rule{Name: "from-yaml"} got, src, err := cmdpolicy.Resolve(cmdpolicy.Sources{ PluginRules: []cmdpolicy.PluginRule{{PluginName: "secaudit", Rule: pluginRule}}, - YAMLRule: yamlRule, + YAMLRules: []*platform.Rule{yamlRule}, YAMLPath: "/some/policy.yml", }) if err != nil { t.Fatalf("Resolve err: %v", err) } - if got.Name != "from-plugin" || src.Kind != cmdpolicy.SourcePlugin { + if len(got) != 1 || got[0].Name != "from-plugin" || src.Kind != cmdpolicy.SourcePlugin { t.Fatalf("plugin should shadow yaml, got %+v / %+v", got, src) } } @@ -45,13 +67,13 @@ func TestResolve_pluginShadowsYaml(t *testing.T) { func TestResolve_yamlWhenNoPlugin(t *testing.T) { yamlRule := &platform.Rule{Name: "from-yaml", MaxRisk: "read"} got, src, err := cmdpolicy.Resolve(cmdpolicy.Sources{ - YAMLRule: yamlRule, - YAMLPath: "/some/policy.yml", + YAMLRules: []*platform.Rule{yamlRule}, + YAMLPath: "/some/policy.yml", }) if err != nil { t.Fatalf("Resolve err: %v", err) } - if got.Name != "from-yaml" || src.Kind != cmdpolicy.SourceYAML { + if len(got) != 1 || got[0].Name != "from-yaml" || src.Kind != cmdpolicy.SourceYAML { t.Fatalf("yaml should win when no plugin, got %+v / %+v", got, src) } if src.Name != "/some/policy.yml" { @@ -59,19 +81,36 @@ func TestResolve_yamlWhenNoPlugin(t *testing.T) { } } +// yaml may also carry several rules under "rules:"; all are returned. +func TestResolve_yamlMultipleRules(t *testing.T) { + r1 := &platform.Rule{Name: "a", MaxRisk: "read"} + r2 := &platform.Rule{Name: "b", MaxRisk: "write"} + got, src, err := cmdpolicy.Resolve(cmdpolicy.Sources{ + YAMLRules: []*platform.Rule{r1, r2}, + YAMLPath: "/some/policy.yml", + }) + if err != nil { + t.Fatalf("Resolve err: %v", err) + } + if len(got) != 2 || src.Kind != cmdpolicy.SourceYAML { + t.Fatalf("expected both yaml rules, got %v / %+v", got, src) + } +} + func TestResolve_emptyEverythingIsNone(t *testing.T) { got, src, err := cmdpolicy.Resolve(cmdpolicy.Sources{}) if err != nil { t.Fatalf("Resolve err: %v", err) } - if got != nil || src.Kind != cmdpolicy.SourceNone { - t.Fatalf("expected (nil, SourceNone), got (%v, %+v)", got, src) + if len(got) != 0 || src.Kind != cmdpolicy.SourceNone { + t.Fatalf("expected (empty, SourceNone), got (%v, %+v)", got, src) } } -// Two plugins both contributing a Rule must produce the typed error so -// the bootstrap pipeline aborts (hard-constraint #7). -func TestResolve_multipleRestrictIsError(t *testing.T) { +// Two DISTINCT plugins both contributing a Rule must produce the typed +// error so the bootstrap pipeline aborts (single-owner invariant): one +// plugin cannot silently widen another plugin's policy. +func TestResolve_multipleRestrictPluginsIsError(t *testing.T) { _, _, err := cmdpolicy.Resolve(cmdpolicy.Sources{ PluginRules: []cmdpolicy.PluginRule{ {PluginName: "a", Rule: &platform.Rule{Name: "a"}}, @@ -84,26 +123,26 @@ func TestResolve_multipleRestrictIsError(t *testing.T) { } // LoadYAMLPolicy: missing file returns (nil, nil) silently so callers -// can pass the result straight into Sources.YAMLRule without special- +// can pass the result straight into Sources.YAMLRules without special- // casing not-exist. func TestLoadYAMLPolicy_missingIsSilent(t *testing.T) { missing := filepath.Join(t.TempDir(), "absent-policy.yml") - rule, err := cmdpolicy.LoadYAMLPolicy(missing) + rules, err := cmdpolicy.LoadYAMLPolicy(missing) if err != nil { t.Fatalf("missing yaml should not error, got %v", err) } - if rule != nil { - t.Fatalf("missing yaml should return nil rule, got %+v", rule) + if rules != nil { + t.Fatalf("missing yaml should return nil rules, got %+v", rules) } } func TestLoadYAMLPolicy_emptyPathIsNoop(t *testing.T) { - rule, err := cmdpolicy.LoadYAMLPolicy("") + rules, err := cmdpolicy.LoadYAMLPolicy("") if err != nil { t.Fatalf("empty path should not error, got %v", err) } - if rule != nil { - t.Fatalf("empty path should return nil rule, got %+v", rule) + if rules != nil { + t.Fatalf("empty path should return nil rules, got %+v", rules) } } @@ -113,11 +152,11 @@ func TestLoadYAMLPolicy_parsesValid(t *testing.T) { if err := os.WriteFile(yamlPath, []byte("name: from-yaml\nmax_risk: read\n"), 0o644); err != nil { t.Fatalf("write yaml: %v", err) } - rule, err := cmdpolicy.LoadYAMLPolicy(yamlPath) + rules, err := cmdpolicy.LoadYAMLPolicy(yamlPath) if err != nil { t.Fatalf("LoadYAMLPolicy err: %v", err) } - if rule == nil || rule.Name != "from-yaml" { - t.Fatalf("expected rule with name=from-yaml, got %+v", rule) + if len(rules) != 1 || rules[0].Name != "from-yaml" { + t.Fatalf("expected one rule with name=from-yaml, got %+v", rules) } } diff --git a/internal/cmdpolicy/yaml/schema.go b/internal/cmdpolicy/yaml/schema.go index 718d2a8bd..65432c085 100644 --- a/internal/cmdpolicy/yaml/schema.go +++ b/internal/cmdpolicy/yaml/schema.go @@ -1,10 +1,10 @@ // Copyright (c) 2026 Lark Technologies Pte. Ltd. // SPDX-License-Identifier: MIT -// Package yaml parses a Rule from yaml bytes. It is kept separate from the -// public extension/platform package so that platform stays free of yaml -// library dependencies -- plugins constructing a Rule in Go code never -// import yaml, only the file loader does. +// Package yaml parses one or more Rules from yaml bytes. It is kept +// separate from the public extension/platform package so that platform +// stays free of yaml library dependencies -- plugins constructing a Rule +// in Go code never import yaml, only the file loader does. // // This package does **structural** parsing only (yaml syntax + unknown-field // rejection). Semantic validation (valid MaxRisk enum, valid identity @@ -23,9 +23,9 @@ import ( "github.com/larksuite/cli/extension/platform" ) -// schema is the internal yaml-tagged shape. Mirrors platform.Rule but lives -// here so the public Rule has no yaml tag baggage. -type schema struct { +// ruleSchema is the internal yaml-tagged shape of one rule. Mirrors +// platform.Rule but lives here so the public Rule has no yaml tag baggage. +type ruleSchema struct { Name string `yaml:"name"` Description string `yaml:"description,omitempty"` Allow []string `yaml:"allow,omitempty"` @@ -35,35 +35,45 @@ type schema struct { AllowUnannotated bool `yaml:"allow_unannotated,omitempty"` } -// Parse decodes yaml bytes into a *platform.Rule. Unknown fields are -// rejected so an old binary cannot silently ignore new schema additions -// (forward-compat safeguard). +// fileSchema is the top-level document shape. Two mutually-exclusive +// layouts are accepted: // -// Semantic validation (MaxRisk taxonomy, identity values, glob syntax) is -// the caller's responsibility -- run the result through -// internal/cmdpolicy.ValidateRule before handing it to the engine. -func Parse(data []byte) (*platform.Rule, error) { - var s schema - dec := gopkgyaml.NewDecoder(bytesReader(data)) - dec.KnownFields(true) - if err := dec.Decode(&s); err != nil { - return nil, fmt.Errorf("parse policy yaml: %w", err) - } +// - a single rule written with flat top-level fields (the historical +// layout; the inlined ruleSchema), or +// - a "rules:" list of rule objects (multi-rule layout). +// +// Mixing the two (flat fields AND a rules: list in the same file) is a +// configuration error -- Parse rejects it rather than guessing intent. +// +// Rules is a pointer so Parse can tell "rules: key absent" (nil) apart +// from "rules: present but empty" (non-nil, len 0). The latter is a +// foot-gun -- a config generator that renders an empty list would +// otherwise yield a single all-zero Rule that lets every annotated +// command through -- so Parse rejects it outright. +type fileSchema struct { + ruleSchema `yaml:",inline"` + Rules *[]ruleSchema `yaml:"rules,omitempty"` +} - // Reject multi-document input: yaml.v3 only decodes one document - // per call, so a stray "---" followed by another document would - // silently drop the trailing rule. - var extra schema - if err := dec.Decode(&extra); !errors.Is(err, io.EOF) { - if err == nil { - return nil, fmt.Errorf("parse policy yaml: multiple YAML documents are not allowed") +// isZero reports whether every field is its zero value. Used to detect +// the flat-fields-plus-rules: mixing error. +func (s ruleSchema) isZero() bool { + return s.Name == "" && s.Description == "" && + len(s.Allow) == 0 && len(s.Deny) == 0 && + s.MaxRisk == "" && len(s.Identities) == 0 && !s.AllowUnannotated +} + +func (s ruleSchema) toRule() *platform.Rule { + // Leave Identities nil when absent (omitempty-style), matching how the + // Allow/Deny slices arrive nil from yaml. A zero-length but non-nil + // slice is behaviourally identical to the engine but trips + // reflect.DeepEqual in tests and reads as "explicitly empty". + var idents []platform.Identity + if len(s.Identities) > 0 { + idents = make([]platform.Identity, len(s.Identities)) + for i, id := range s.Identities { + idents[i] = platform.Identity(id) } - return nil, fmt.Errorf("parse policy yaml: %w", err) - } - - idents := make([]platform.Identity, len(s.Identities)) - for i, id := range s.Identities { - idents[i] = platform.Identity(id) } return &platform.Rule{ Name: s.Name, @@ -73,5 +83,53 @@ func Parse(data []byte) (*platform.Rule, error) { MaxRisk: platform.Risk(s.MaxRisk), Identities: idents, AllowUnannotated: s.AllowUnannotated, - }, nil + } +} + +// Parse decodes yaml bytes into one or more *platform.Rule. Unknown fields +// are rejected so an old binary cannot silently ignore new schema additions +// (forward-compat safeguard). +// +// The result always has at least one element: a flat-fields document +// yields a single rule (possibly an all-zero "no restriction" rule), and a +// "rules:" list yields one rule per entry. +// +// Semantic validation (MaxRisk taxonomy, identity values, glob syntax) is +// the caller's responsibility -- run each result through +// internal/cmdpolicy.ValidateRule before handing it to the engine. +func Parse(data []byte) ([]*platform.Rule, error) { + var s fileSchema + dec := gopkgyaml.NewDecoder(bytesReader(data)) + dec.KnownFields(true) + if err := dec.Decode(&s); err != nil { + return nil, fmt.Errorf("parse policy yaml: %w", err) + } + + // Reject multi-document input: yaml.v3 only decodes one document + // per call, so a stray "---" followed by another document would + // silently drop the trailing rule. + var extra fileSchema + if err := dec.Decode(&extra); !errors.Is(err, io.EOF) { + if err == nil { + return nil, fmt.Errorf("parse policy yaml: multiple YAML documents are not allowed") + } + return nil, fmt.Errorf("parse policy yaml: %w", err) + } + + if s.Rules != nil { + if len(*s.Rules) == 0 { + return nil, fmt.Errorf("parse policy yaml: 'rules:' is present but empty; remove the key, or list at least one rule") + } + if !s.ruleSchema.isZero() { + return nil, fmt.Errorf("parse policy yaml: top-level rule fields cannot be combined with a 'rules:' list; move every rule under 'rules:'") + } + out := make([]*platform.Rule, 0, len(*s.Rules)) + for _, rs := range *s.Rules { + out = append(out, rs.toRule()) + } + return out, nil + } + + // Backward-compatible single top-level rule (flat fields). + return []*platform.Rule{s.ruleSchema.toRule()}, nil } diff --git a/internal/cmdpolicy/yaml/schema_test.go b/internal/cmdpolicy/yaml/schema_test.go index 912c8b2a5..4b77e99f8 100644 --- a/internal/cmdpolicy/yaml/schema_test.go +++ b/internal/cmdpolicy/yaml/schema_test.go @@ -24,7 +24,7 @@ max_risk: read identities: - user `) - rule, err := pyaml.Parse(data) + rules, err := pyaml.Parse(data) if err != nil { t.Fatalf("Parse failed: %v", err) } @@ -36,8 +36,59 @@ identities: MaxRisk: "read", Identities: []platform.Identity{"user"}, } - if !reflect.DeepEqual(rule, want) { - t.Fatalf("rule = %+v, want %+v", rule, want) + // A flat top-level rule yields exactly one element (backward compat). + if !reflect.DeepEqual(rules, []*platform.Rule{want}) { + t.Fatalf("rules = %+v, want single %+v", rules, want) + } +} + +// A "rules:" list yields one platform.Rule per entry, in order. This is +// the multi-rule layout: each rule is a scoped grant the engine +// OR-combines. +func TestParse_rulesList(t *testing.T) { + data := []byte(` +rules: + - name: docs-ro + allow: [docs/**] + max_risk: read + - name: im-rw + allow: [im/**] + max_risk: write + identities: [user, bot] +`) + rules, err := pyaml.Parse(data) + if err != nil { + t.Fatalf("Parse failed: %v", err) + } + want := []*platform.Rule{ + {Name: "docs-ro", Allow: []string{"docs/**"}, MaxRisk: "read"}, + {Name: "im-rw", Allow: []string{"im/**"}, MaxRisk: "write", Identities: []platform.Identity{"user", "bot"}}, + } + if !reflect.DeepEqual(rules, want) { + t.Fatalf("rules = %+v, want %+v", rules, want) + } +} + +// A "rules:" key that is present but empty is a foot-gun: an empty list +// would otherwise fall through to a single all-zero Rule that allows +// every annotated command ("looks like a policy, enforces almost +// nothing"). Parse must reject it outright instead. +func TestParse_rejectsEmptyRulesList(t *testing.T) { + if _, err := pyaml.Parse([]byte("rules: []\n")); err == nil { + t.Fatalf("Parse should reject a present-but-empty 'rules:' list") + } +} + +// Mixing top-level flat rule fields with a rules: list is ambiguous and +// must be rejected rather than silently picking one. +func TestParse_rejectsFlatPlusRulesMix(t *testing.T) { + data := []byte(` +name: top-level +rules: + - name: nested +`) + if _, err := pyaml.Parse(data); err == nil { + t.Fatalf("Parse should reject mixing top-level fields with a rules: list") } } @@ -52,15 +103,15 @@ name: agent-readonly max_risk: read allow_unannotated: true `) - rule, err := pyaml.Parse(data) + rules, err := pyaml.Parse(data) if err != nil { t.Fatalf("Parse failed: %v", err) } - if !rule.AllowUnannotated { + if !rules[0].AllowUnannotated { t.Fatalf("AllowUnannotated = false, want true (yaml field must propagate)") } - if rule.MaxRisk != "read" || rule.Name != "agent-readonly" { - t.Errorf("other fields lost: %+v", rule) + if rules[0].MaxRisk != "read" || rules[0].Name != "agent-readonly" { + t.Errorf("other fields lost: %+v", rules[0]) } } @@ -71,11 +122,11 @@ func TestParse_allowUnannotatedDefaultsFalse(t *testing.T) { name: x max_risk: read `) - rule, err := pyaml.Parse(data) + rules, err := pyaml.Parse(data) if err != nil { t.Fatalf("Parse failed: %v", err) } - if rule.AllowUnannotated { + if rules[0].AllowUnannotated { t.Fatalf("AllowUnannotated must default to false when key is absent") } } @@ -96,12 +147,12 @@ mystery_field: oh no // structural yaml; an invalid max_risk passes through (validation happens // downstream). func TestParse_doesNotValidateSemantics(t *testing.T) { - rule, err := pyaml.Parse([]byte("max_risk: nuclear\n")) + rules, err := pyaml.Parse([]byte("max_risk: nuclear\n")) if err != nil { t.Fatalf("structural parse should succeed, got %v", err) } - if rule.MaxRisk != "nuclear" { - t.Fatalf("MaxRisk = %q, want passed through as-is", rule.MaxRisk) + if rules[0].MaxRisk != "nuclear" { + t.Fatalf("MaxRisk = %q, want passed through as-is", rules[0].MaxRisk) } } diff --git a/internal/platform/error.go b/internal/platform/error.go index 8ee037aa6..0934a6bed 100644 --- a/internal/platform/error.go +++ b/internal/platform/error.go @@ -39,7 +39,6 @@ const ( ReasonDuplicateHookName = "duplicate_hook_name" ReasonInvalidHookRegister = "invalid_hook_registration" ReasonInvalidRule = "invalid_rule" - ReasonDoubleRestrict = "double_restrict" ReasonRestrictsMismatch = "restricts_mismatch" ReasonCapabilityUnmet = "capability_unmet" ReasonCapabilitiesPanic = "capabilities_panic" diff --git a/internal/platform/host.go b/internal/platform/host.go index 2f13cf59d..b8265ce60 100644 --- a/internal/platform/host.go +++ b/internal/platform/host.go @@ -201,10 +201,10 @@ func installOne(name string, p platform.Plugin, result *InstallResult) error { for _, e := range staging.stagedLifecycles { result.Registry.AddLifecycle(e) } - if staging.rule != nil { + for _, rule := range staging.rules { result.PluginRules = append(result.PluginRules, cmdpolicy.PluginRule{ PluginName: name, - Rule: staging.rule, + Rule: rule, }) } diff --git a/internal/platform/host_test.go b/internal/platform/host_test.go index 13b1f574e..c00a7868a 100644 --- a/internal/platform/host_test.go +++ b/internal/platform/host_test.go @@ -389,3 +389,45 @@ func TestInstallAll_atomicRollback(t *testing.T) { t.Fatalf("error must be *PluginInstallError, got %T", err) } } + +// multiRestrictPlugin calls r.Restrict twice -- the multi-rule case. A +// single plugin may declare several scoped grants; both must be collected +// into PluginRules under the same plugin name, in registration order. +type multiRestrictPlugin struct{} + +func (multiRestrictPlugin) Name() string { return "secaudit" } +func (multiRestrictPlugin) Version() string { return "1.0.0" } +func (multiRestrictPlugin) Capabilities() platform.Capabilities { + return platform.Capabilities{ + Restricts: true, + FailurePolicy: platform.FailClosed, + } +} +func (multiRestrictPlugin) Install(r platform.Registrar) error { + r.Restrict(&platform.Rule{Name: "docs-ro", Allow: []string{"docs/**"}, MaxRisk: platform.RiskRead}) + r.Restrict(&platform.Rule{Name: "im-rw", Allow: []string{"im/**"}, MaxRisk: platform.RiskWrite}) + return nil +} + +// A single plugin calling Restrict more than once is valid (multi-rule +// support): both rules are collected, in order, under the one plugin name. +// This pins the behaviour change from the old "Restrict at most once" +// double_restrict error. +func TestInstallAll_multipleRestrictPerPlugin(t *testing.T) { + result, err := internalplatform.InstallAll([]platform.Plugin{multiRestrictPlugin{}}, nil) + if err != nil { + t.Fatalf("multiple Restrict per plugin must succeed, got %v", err) + } + if len(result.PluginRules) != 2 { + t.Fatalf("PluginRules = %d, want 2", len(result.PluginRules)) + } + for _, pr := range result.PluginRules { + if pr.PluginName != "secaudit" { + t.Errorf("PluginName = %q, want secaudit", pr.PluginName) + } + } + if result.PluginRules[0].Rule.Name != "docs-ro" || result.PluginRules[1].Rule.Name != "im-rw" { + t.Errorf("rules out of order: %q, %q", + result.PluginRules[0].Rule.Name, result.PluginRules[1].Rule.Name) + } +} diff --git a/internal/platform/inventory.go b/internal/platform/inventory.go index 1127f9f46..063cb3747 100644 --- a/internal/platform/inventory.go +++ b/internal/platform/inventory.go @@ -24,8 +24,10 @@ type PluginEntry struct { Version string Capabilities CapabilitiesView - // Rule is non-nil only when the plugin called r.Restrict. - Rule *RuleView + // Rules holds the plugin's Restrict contributions, one per r.Restrict + // call (a plugin may declare several scoped rules). Empty when the + // plugin did not call r.Restrict. + Rules []*RuleView Observers []HookEntry Wrappers []HookEntry @@ -149,15 +151,15 @@ func BuildInventory(plugins []PluginInventorySource, registry *hook.Registry, ru for _, r := range rules { if entry := byPlugin[r.PluginName]; entry != nil { - entry.Rule = &RuleView{ + entry.Rules = append(entry.Rules, &RuleView{ Name: r.RuleName, Description: r.Desc, - Allow: r.Allow, - Deny: r.Deny, + Allow: append([]string(nil), r.Allow...), + Deny: append([]string(nil), r.Deny...), MaxRisk: r.MaxRisk, - Identities: r.Identities, + Identities: append([]string(nil), r.Identities...), AllowUnannotated: r.AllowUnannotated, - } + }) } } return out @@ -248,12 +250,18 @@ func cloneInventory(in *Inventory) *Inventory { Version: p.Version, Capabilities: p.Capabilities, } - if p.Rule != nil { - rv := *p.Rule - rv.Allow = append([]string(nil), p.Rule.Allow...) - rv.Deny = append([]string(nil), p.Rule.Deny...) - rv.Identities = append([]string(nil), p.Rule.Identities...) - entry.Rule = &rv + if p.Rules != nil { + entry.Rules = make([]*RuleView, len(p.Rules)) + for j, r := range p.Rules { + if r == nil { + continue + } + rv := *r + rv.Allow = append([]string(nil), r.Allow...) + rv.Deny = append([]string(nil), r.Deny...) + rv.Identities = append([]string(nil), r.Identities...) + entry.Rules[j] = &rv + } } entry.Observers = append([]HookEntry(nil), p.Observers...) entry.Wrappers = append([]HookEntry(nil), p.Wrappers...) diff --git a/internal/platform/inventory_test.go b/internal/platform/inventory_test.go index a9d8d8b51..992722cff 100644 --- a/internal/platform/inventory_test.go +++ b/internal/platform/inventory_test.go @@ -56,8 +56,8 @@ func TestBuildInventory_groupsByPluginName(t *testing.T) { if got := len(a.Lifecycles); got != 1 { t.Errorf("a.Lifecycles = %d, want 1", got) } - if a.Rule == nil || a.Rule.Name != "a-rule" { - t.Errorf("a.Rule = %+v, want name a-rule", a.Rule) + if len(a.Rules) != 1 || a.Rules[0].Name != "a-rule" { + t.Errorf("a.Rules = %+v, want single rule name a-rule", a.Rules) } if a.Capabilities.FailurePolicy != "FailClosed" { t.Errorf("a.Capabilities.FailurePolicy = %q, want FailClosed", a.Capabilities.FailurePolicy) @@ -66,14 +66,42 @@ func TestBuildInventory_groupsByPluginName(t *testing.T) { if got := len(b.Observers); got != 1 { t.Errorf("b.Observers = %d, want 1 (only b.audit)", got) } - if b.Rule != nil { - t.Errorf("b.Rule = %+v, want nil (b did not call Restrict)", b.Rule) + if len(b.Rules) != 0 { + t.Errorf("b.Rules = %+v, want empty (b did not call Restrict)", b.Rules) } if b.Capabilities.FailurePolicy != "FailOpen" { t.Errorf("b.Capabilities.FailurePolicy = %q, want FailOpen (zero value)", b.Capabilities.FailurePolicy) } } +// A plugin contributing several rules (same PluginName, multiple +// RuleInventorySource entries) must surface ALL of them under Rules, in +// order -- not silently overwrite down to the last one. Pins the +// multi-rule inventory fix. +func TestBuildInventory_multipleRulesPerPlugin(t *testing.T) { + plugins := []internalplatform.PluginInventorySource{ + {Name: "a", Version: "1.0", Capabilities: platform.Capabilities{ + Restricts: true, FailurePolicy: platform.FailClosed, + }}, + } + rules := []internalplatform.RuleInventorySource{ + {PluginName: "a", RuleName: "docs-ro", Allow: []string{"docs/**"}, MaxRisk: "read"}, + {PluginName: "a", RuleName: "im-rw", Allow: []string{"im/**"}, MaxRisk: "write"}, + } + + inv := internalplatform.BuildInventory(plugins, nil, rules) + a := findPlugin(inv, "a") + if a == nil { + t.Fatalf("missing entry a") + } + if len(a.Rules) != 2 { + t.Fatalf("a.Rules = %d, want 2 (both rules preserved, no overwrite)", len(a.Rules)) + } + if a.Rules[0].Name != "docs-ro" || a.Rules[1].Name != "im-rw" { + t.Errorf("rules out of order: %q, %q", a.Rules[0].Name, a.Rules[1].Name) + } +} + func TestBuildInventory_empty(t *testing.T) { inv := internalplatform.BuildInventory(nil, nil, nil) if got := len(inv.Plugins); got != 0 { diff --git a/internal/platform/staging.go b/internal/platform/staging.go index 1b0b7668a..e3bb48295 100644 --- a/internal/platform/staging.go +++ b/internal/platform/staging.go @@ -30,14 +30,15 @@ type stagingRegistrar struct { stagedWrappers []hook.WrapperEntry stagedLifecycles []hook.LifecycleEntry - // rule is the staged Restrict contribution, captured for the host - // to merge with the yaml side later. nil means the plugin did not - // call r.Restrict. - rule *platform.Rule + // rules holds the staged Restrict contributions, captured for the + // host to feed into the resolver later. Empty means the plugin did + // not call r.Restrict. A plugin may call Restrict more than once; + // each call adds one scoped Rule (OR-combined by the engine). + rules []*platform.Rule - // actuallyRestricted records whether r.Restrict was called at all. - // Even a Restrict(nil) flips this to true so the - // Restricts-vs-actual consistency check can detect the call. + // actuallyRestricted records whether r.Restrict was called at all + // (even Restrict(nil)), so the Restricts-vs-actual consistency check + // can detect the call. actuallyRestricted bool // seenHookNames detects duplicate hookName within this plugin's @@ -126,10 +127,6 @@ func (r *stagingRegistrar) On(event platform.LifecycleEvent, name string, fn pla } func (r *stagingRegistrar) Restrict(rule *platform.Rule) { - if r.actuallyRestricted { - r.bufferErr(ReasonDoubleRestrict, "Restrict called more than once") - return - } r.actuallyRestricted = true if rule == nil { r.bufferErr(ReasonInvalidRule, "Restrict(nil)") @@ -144,7 +141,7 @@ func (r *stagingRegistrar) Restrict(rule *platform.Rule) { cp.Allow = append([]string(nil), rule.Allow...) cp.Deny = append([]string(nil), rule.Deny...) cp.Identities = append([]platform.Identity(nil), rule.Identities...) - r.rule = &cp + r.rules = append(r.rules, &cp) } // --- helpers ---