From 0efee931a8845b4bff0f6f9e34fbdb2a82626698 Mon Sep 17 00:00:00 2001 From: shanglei Date: Sat, 16 May 2026 14:21:22 +0800 Subject: [PATCH] fix(extension/platform): address PR review must-fix items - cmdpolicy: skip AnnotationPureGroup commands in EvaluateAll, aggregateParents, and hasRunnableDescendant so user-layer policy no longer blocks ` --help` after the unknown-subcommand guard attaches RunE to every parent - cmd/root: tag guarded parent groups with AnnotationPureGroup - extension/platform: drop `//go:build testing` from register_testing.go so `go test ./...` works without an extra build tag - extension/platform/README: inline reason_code reference, fix plugin lifecycle diagram order (init/Register precede RegisteredPlugins) - cmd/platform_bootstrap: route userPolicyPath through core.GetBaseConfigDir so LARKSUITE_CLI_CONFIG_DIR is honoured - cmdpolicy: add RedactHomeDir helper, fold base config dir and $HOME prefixes for config policy show + resolver errors - internal/platform: reject unrecognised FailurePolicy values with invalid_capability instead of silently fail-open - cmd/config: surface diagnostic policy/plugins commands in `config --help` Long text - CHANGELOG: document command_denied error.type rename and unknown_subcommand exit-2 behavior change --- CHANGELOG.md | 41 ++++++++++ cmd/config/config.go | 5 ++ cmd/config/policy.go | 14 +++- cmd/platform_bootstrap.go | 22 +++-- cmd/root.go | 12 +++ extension/platform/README.md | 97 +++++++++++++++++++++- extension/platform/register_test.go | 1 + extension/platform/register_testing.go | 16 ++-- internal/cmdpolicy/aggregation_test.go | 107 +++++++++++++++++++++++++ internal/cmdpolicy/apply.go | 24 ++++++ internal/cmdpolicy/engine.go | 17 +++- internal/cmdpolicy/path.go | 68 ++++++++++++++++ internal/cmdpolicy/path_test.go | 57 +++++++++++++ internal/cmdpolicy/resolver.go | 13 ++- internal/platform/host.go | 16 ++++ 15 files changed, 485 insertions(+), 25 deletions(-) create mode 100644 internal/cmdpolicy/path_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 8af91bad8..0c295f27c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,47 @@ All notable changes to this project will be documented in this file. +## [Unreleased] + +### Features + +- **extension/platform**: Public plugin SDK with policy engine, hooks, + and the `NewPlugin` Builder. Plugins ship as a fork of `lark-cli` that + blank-imports the plugin package; the host installs hooks (Observe / + Wrap / Lifecycle) and a single optional user-layer `Restrict` rule. + Diagnostic commands `lark-cli config plugins show` and + `lark-cli config policy show` enumerate the resulting inventory (#910). + +### Breaking Changes + +- **error.type rename — `strict_mode` / `pruning` → `command_denied`**. + Both the strict-mode pruning pass and the new user-layer policy + engine now emit a single envelope `error.type == "command_denied"`. + The original layer is preserved on `detail.layer` (`strict_mode` or + `policy`), so agents that need to distinguish can still do so. + + - Old: `error.type == "strict_mode"` and `error.type == "pruning"`. + - New: `error.type == "command_denied" && detail.layer == "strict_mode"` + or `detail.layer == "policy"`. + - Migration: any agent / monitor / script that matches + `error.type == "strict_mode"` will silently break — update the + match to the new shape. + +- **Unknown subcommand returns exit 2 + structured error**. Previously + invoking a group with an unknown subcommand (e.g. + `lark-cli drive nosuchcommand`) silently printed help and exited 0. + It now emits an `error.type == "unknown_subcommand"` envelope and + exits with code 2. + - Migration: any script / CI step that relied on exit-0 for unknown + subcommands must be updated. `lark-cli --help` still + returns exit 0 + help as before. + +### Documentation + +- **extension/platform**: Embed the full `reason_code` reference table + inside `extension/platform/README.md` so the public SDK landing page + no longer links to documents that live outside the repository (#910). + ## [v1.0.31] - 2026-05-14 ### Features diff --git a/cmd/config/config.go b/cmd/config/config.go index c99f6b482..ddcb10723 100644 --- a/cmd/config/config.go +++ b/cmd/config/config.go @@ -14,6 +14,11 @@ func NewCmdConfig(f *cmdutil.Factory) *cobra.Command { cmd := &cobra.Command{ Use: "config", Short: "Global CLI configuration management", + Long: `Global CLI configuration management. + +Diagnostic (hidden) commands — runnable but omitted from --help: + lark-cli config policy show Inspect active user-layer policy + lark-cli config plugins show Inspect installed plugins and hooks`, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { // Replicate rootCmd's PersistentPreRun behaviour: cobra stops at the first // PersistentPreRun[E] found walking up the chain, so the root-level diff --git a/cmd/config/policy.go b/cmd/config/policy.go index a150eeaa7..bb1e38a3a 100644 --- a/cmd/config/policy.go +++ b/cmd/config/policy.go @@ -74,10 +74,20 @@ func runConfigPolicyShow(f *cmdutil.Factory) error { return nil } + // `yaml_path` and the yaml-source `source_name` leak the user's home + // directory in their raw form (e.g. /Users/alice/.lark-cli/policy.yml). + // `config policy show` is read by AI agents and CI logs, so we redact + // the prefix before emitting -- same rule as policySourceLabel for + // envelopes. For plugin sources, Source.Name is the plugin name (no + // path) and is surfaced verbatim. + sourceName := active.Source.Name + if active.Source.Kind == cmdpolicy.SourceYAML { + sourceName = cmdpolicy.RedactHomeDir(sourceName) + } out := map[string]any{ "source": string(active.Source.Kind), - "source_name": active.Source.Name, - "yaml_path": active.YAMLPath, + "source_name": sourceName, + "yaml_path": cmdpolicy.RedactHomeDir(active.YAMLPath), "denied_paths": active.DeniedPaths, } if active.Rule != nil { diff --git a/cmd/platform_bootstrap.go b/cmd/platform_bootstrap.go index 3f21f74fc..c4e65f229 100644 --- a/cmd/platform_bootstrap.go +++ b/cmd/platform_bootstrap.go @@ -13,9 +13,9 @@ import ( "github.com/larksuite/cli/extension/platform" "github.com/larksuite/cli/internal/cmdpolicy" + "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/hook" internalplatform "github.com/larksuite/cli/internal/platform" - "github.com/larksuite/cli/internal/vfs" ) // userPolicyFileName is the conventional filename for the user-layer Rule. @@ -221,14 +221,20 @@ func splitCSV(s string) []string { return out } -// userPolicyPath returns the absolute path of ~/.lark-cli/policy.yml, -// or an error if the user's home directory cannot be determined. +// userPolicyPath returns the path of /policy.yml. +// +// The base directory honours LARKSUITE_CLI_CONFIG_DIR (via +// core.GetBaseConfigDir) so that test isolation, container deployments +// and per-Agent config overrides all see a consistent policy location. +// Using vfs.UserHomeDir directly here would silently bypass the env +// override and route every test through the real ~/.lark-cli. +// +// The error return is retained for caller compatibility but is always +// nil today: GetBaseConfigDir falls back to a relative ".lark-cli" when +// the home dir can't be resolved, and the resolver already treats a +// missing file as "no policy". func userPolicyPath() (string, error) { - home, err := vfs.UserHomeDir() - if err != nil { - return "", err - } - return filepath.Join(home, ".lark-cli", userPolicyFileName), nil + return filepath.Join(core.GetBaseConfigDir(), userPolicyFileName), nil } // warnPolicyError writes a one-line stderr warning when the user policy diff --git a/cmd/root.go b/cmd/root.go index 0e799578e..00d9a24bc 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -19,6 +19,7 @@ import ( "github.com/larksuite/cli/extension/platform" internalauth "github.com/larksuite/cli/internal/auth" "github.com/larksuite/cli/internal/build" + "github.com/larksuite/cli/internal/cmdpolicy" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/hook" @@ -286,9 +287,20 @@ func writeSecurityPolicyError(w io.Writer, spErr *internalauth.SecurityPolicyErr // installUnknownSubcommandGuard replaces cobra's silent help fallback on // group commands (no Run/RunE) with an unknown_subcommand error. +// +// IMPORTANT: every command modified here is also tagged with +// cmdpolicy.AnnotationPureGroup so the user-layer policy engine +// continues to treat the command as a pure parent group. Without the +// tag, the RunE injection here would flip Runnable()=true and a user +// rule like `max_risk: read` would deny every ` --help` call +// with reason_code=risk_not_annotated. func installUnknownSubcommandGuard(cmd *cobra.Command) { if cmd.HasSubCommands() && cmd.Run == nil && cmd.RunE == nil { cmd.RunE = unknownSubcommandRunE + if cmd.Annotations == nil { + cmd.Annotations = map[string]string{} + } + cmd.Annotations[cmdpolicy.AnnotationPureGroup] = "true" } for _, c := range cmd.Commands() { installUnknownSubcommandGuard(c) diff --git a/extension/platform/README.md b/extension/platform/README.md index 1c3f168e2..d2834ddd7 100644 --- a/extension/platform/README.md +++ b/extension/platform/README.md @@ -61,6 +61,42 @@ You should see `audit` in the plugin list. | `On(Startup/Shutdown)` | Process lifecycle | N/A | | `Restrict(Rule)` | Bootstrap-time, single per binary | Denies whole subtrees | +### Plugin lifecycle + +```mermaid +sequenceDiagram + participant Host as lark-cli (host) + participant SDK as platform (SDK) + participant Plugin as your plugin + + Note over Host,Plugin: Process start (before main) + Plugin->>Plugin: init() (via blank import) + Plugin->>SDK: Register(plugin) + + Note over Host,Plugin: Bootstrap (host main) + Host->>SDK: RegisteredPlugins() + SDK-->>Host: snapshot in registration order + Host->>SDK: InstallAll() + SDK->>Plugin: Capabilities() + SDK->>Plugin: Install(Registrar) + Plugin->>SDK: Observe / Wrap / Restrict / On(Startup,Shutdown) + SDK->>Plugin: On(Startup) fire + + Note over Host,Plugin: Each command dispatch + Host->>SDK: hook chain (in registration order) + SDK->>Plugin: Observer Before + SDK->>Plugin: Wrap (around RunE) + SDK->>Plugin: Observer After + + Note over Host,Plugin: Process exit + Host->>SDK: Emit(Shutdown) + SDK->>Plugin: On(Shutdown) fire +``` + +A `command_denied` decision (from `Restrict` or strict-mode) bypasses +the `Wrap` chain entirely — observers still fire so audit plugins see +the rejected dispatch. + ## Safety contract (read this) - A plugin calling `Restrict()` MUST declare `FailClosed`. The Builder @@ -85,9 +121,66 @@ You should see `audit` in the plugin list. does NOT bypass this — typo is a code bug, not a missing annotation. +## reason_code reference + +Every install / dispatch failure emits a `command_denied` or +`plugin_install` envelope carrying a `detail.reason_code` from the +closed enum below. Use the code (not the human-readable message) when +matching errors in agents, CI scripts, or downstream tools — the +messages are localised and may change between releases. + +### Plugin install (`error.type = plugin_install`) + +| reason_code | When it fires | Honours FailurePolicy? | +| --------------------------- | ------------------------------------------------------------------------------ | ---------------------- | +| `invalid_plugin_name` | `Plugin.Name()` doesn't match `^[a-z0-9][a-z0-9-]*$` | No — always aborts | +| `plugin_name_panic` | `Plugin.Name()` panicked | No — always aborts | +| `duplicate_plugin_name` | Two plugins return the same `Name()` | No — always aborts | +| `capabilities_panic` | `Plugin.Capabilities()` panicked | Yes | +| `invalid_capability` | `Capabilities` malformed: bad `RequiredCLIVersion`, unknown `FailurePolicy` | No — always aborts | +| `capability_unmet` | Current CLI version doesn't satisfy `RequiredCLIVersion` | Yes | +| `restricts_mismatch` | `Restricts=true` without `FailClosed`, or `Restricts` flag inconsistent w/ Install | No — always aborts | +| `invalid_hook_name` | Hook name contains `.` or doesn't match the plugin namespace | Yes | +| `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 | +| `install_failed` | `Plugin.Install` returned a non-nil error | Yes | +| `install_panic` | `Plugin.Install` panicked | Yes | + +"No — always aborts" entries are treated as **untrusted-config errors**: +the host can't honour the plugin's declared `FailurePolicy` because the +declaration itself is suspect (e.g. an `invalid_capability` plugin +might also be lying about being `FailOpen`). + +### Command dispatch (`error.type = command_denied`) + +| reason_code | Meaning | +| ----------------------- | ---------------------------------------------------------------------------------------------------------------- | +| `risk_not_annotated` | Command has no `risk_level` annotation, and the active Rule does not set `allow_unannotated: true` | +| `risk_invalid` | Command's `risk_level` is a typo / not in the `read | write | high-risk-write` taxonomy (always fail-closed) | +| `command_denylisted` | Command path matched the active Rule's `deny` glob | +| `domain_not_allowed` | Active Rule has a non-empty `allow` list and the command path did not match any glob | +| `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` | +| `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: +`policy` (this SDK's user-layer engine) vs. `strict_mode` +(`cmd/prune.go`'s credential-hardening pass). Agents that want to +dispatch on "any denial" should match `error.type == "command_denied"` +and ignore the layer; agents that only care about user-policy denials +should additionally check `detail.layer == "policy"`. + ## Where to go next - [Runnable example: audit observer](./examples/audit-observer/) - [Runnable example: read-only policy](./examples/readonly-policy/) -- [Plugin author guide](../../docs/extension/plugin-author-guide.md) -- [reason_code reference](../../docs/extension/reason-codes.md) +- Builder API: see [`builder.go`](./builder.go) for the full DSL + (`NewPlugin`, `Observer`, `Wrap`, `Restrict`, `FailOpen`/`FailClosed`, + `MustBuild`). +- Inventory diagnostic: run `lark-cli config plugins show` after + installing your plugin to see hooks/rules attributed to your plugin + name. diff --git a/extension/platform/register_test.go b/extension/platform/register_test.go index a64ac2a55..80425e701 100644 --- a/extension/platform/register_test.go +++ b/extension/platform/register_test.go @@ -40,6 +40,7 @@ func TestRegister_preservesInsertionOrder(t *testing.T) { func TestRegister_resetClears(t *testing.T) { platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) platform.Register(stubPlugin{name: "a"}) if len(platform.RegisteredPlugins()) != 1 { t.Fatalf("expected 1 plugin") diff --git a/extension/platform/register_testing.go b/extension/platform/register_testing.go index 878d6d66b..8d32f67f0 100644 --- a/extension/platform/register_testing.go +++ b/extension/platform/register_testing.go @@ -1,14 +1,16 @@ // Copyright (c) 2026 Lark Technologies Pte. Ltd. // SPDX-License-Identifier: MIT -//go:build testing - package platform -// ResetForTesting clears the global plugin registry. Available only -// under `-tags testing`; not part of the public API. +// ResetForTesting clears the global plugin registry. Exposed for test +// isolation only — plugin authors and SDK consumers must NOT call this +// from production code. The function is exported (rather than placed in +// an internal test-only file) so that `go test ./...` works for every +// downstream package without an extra build tag. // -// Tests that exercise plugin registration should defer -// `t.Cleanup(platform.ResetForTesting)` so subsequent tests start -// from a clean slate. +// Tests that exercise plugin registration must defer +// `t.Cleanup(platform.ResetForTesting)` so subsequent tests start from a +// clean slate. The helper is NOT goroutine-safe across concurrent +// `t.Parallel()` tests — the global registry is shared process state. func ResetForTesting() { pluginRegistry.reset() } diff --git a/internal/cmdpolicy/aggregation_test.go b/internal/cmdpolicy/aggregation_test.go index f0d36e527..59384952a 100644 --- a/internal/cmdpolicy/aggregation_test.go +++ b/internal/cmdpolicy/aggregation_test.go @@ -240,6 +240,113 @@ func TestApply_runEReturnsExitErrorAndCommandDeniedError(t *testing.T) { } } +// Regression: a pure parent group carrying AnnotationPureGroup must be +// skipped by both EvaluateAll and aggregateParents. Without the skip, +// the cmd.installUnknownSubcommandGuard pass (which attaches a RunE to +// every group for cobra's silent-help fallback) would flip Runnable() +// to true for `docs`, `drive`, etc., and a yaml rule like +// `max_risk: read` would deny every ` --help` invocation with +// reason_code = risk_not_annotated. +func TestEvaluateAll_skipsAnnotatedPureGroup(t *testing.T) { + root := &cobra.Command{Use: "lark-cli"} + drive := &cobra.Command{ + Use: "drive", + RunE: func(*cobra.Command, []string) error { return nil }, // emulate guard injection + Annotations: map[string]string{ + cmdpolicy.AnnotationPureGroup: "true", + }, + } + root.AddCommand(drive) + pull := &cobra.Command{Use: "+pull", RunE: noop} + cmdutil.SetRisk(pull, "read") + drive.AddCommand(pull) + + e := cmdpolicy.New(&platform.Rule{MaxRisk: "read"}) + got := e.EvaluateAll(root) + + if d, present := got["drive"]; present { + t.Errorf("annotated pure group should not appear in Decisions; got %+v", d) + } + if !got["drive/+pull"].Allowed { + t.Errorf("leaf under pure group must still be evaluated; got %+v", got["drive/+pull"]) + } +} + +// Regression: hasRunnableDescendant must also treat +// AnnotationPureGroup-tagged commands as non-runnable. Without the +// skip, an entire branch consisting of a pure-group placeholder + a +// single pure-group leaf would advertise itself as a "live" subtree +// and the parent aggregation pass would refuse to install a deny stub +// (allLiveChildrenDenied flips to false because the pure group is +// neither runnable nor in `denied`). +func TestHasRunnableDescendant_ignoresAnnotatedPureGroup(t *testing.T) { + root := &cobra.Command{Use: "lark-cli"} + docs := &cobra.Command{Use: "docs"} + root.AddCommand(docs) + + // A pure-group sibling of a real leaf. The parent must still + // aggregate based on the real leaf alone. + placeholder := &cobra.Command{ + Use: "placeholder", + RunE: func(*cobra.Command, []string) error { return nil }, + Annotations: map[string]string{ + cmdpolicy.AnnotationPureGroup: "true", + }, + } + docs.AddCommand(placeholder) + noChild := &cobra.Command{ + Use: "+ghost", + RunE: func(*cobra.Command, []string) error { return nil }, + Annotations: map[string]string{ + cmdpolicy.AnnotationPureGroup: "true", + }, + } + placeholder.AddCommand(noChild) + + fetch := &cobra.Command{Use: "+fetch", RunE: noop} + cmdutil.SetRisk(fetch, "write") + docs.AddCommand(fetch) + + e := cmdpolicy.New(&platform.Rule{MaxRisk: "read"}) + decisions := e.EvaluateAll(root) + denied := cmdpolicy.BuildDeniedByPath(root, decisions, cmdpolicy.ResolveSource{Kind: cmdpolicy.SourceYAML}, "") + + if _, ok := denied["docs"]; !ok { + t.Fatalf("docs should be aggregated as fully denied (pure-group children excluded from live count); map=%+v", denied) + } +} + +// Regression: aggregateParents must treat an AnnotationPureGroup-tagged +// command exactly like a parent-only group. With cmdRunnable accidentally +// true (RunE attached by the guard), the aggregator would otherwise look +// for an own-RunE denial entry and skip aggregation, leaving ` +// --help` reachable even when every live child is denied. +func TestBuildDeniedByPath_aggregatesAnnotatedPureGroup(t *testing.T) { + root := &cobra.Command{Use: "lark-cli"} + drive := &cobra.Command{ + Use: "drive", + RunE: func(*cobra.Command, []string) error { return nil }, + Annotations: map[string]string{ + cmdpolicy.AnnotationPureGroup: "true", + }, + } + root.AddCommand(drive) + push := &cobra.Command{Use: "+push", RunE: noop} + cmdutil.SetRisk(push, "write") + drive.AddCommand(push) + pull := &cobra.Command{Use: "+pull", RunE: noop} + cmdutil.SetRisk(pull, "write") + drive.AddCommand(pull) + + e := cmdpolicy.New(&platform.Rule{MaxRisk: "read"}) + decisions := e.EvaluateAll(root) + denied := cmdpolicy.BuildDeniedByPath(root, decisions, cmdpolicy.ResolveSource{Kind: cmdpolicy.SourceYAML}, "") + + if _, ok := denied["drive"]; !ok { + t.Fatalf("aggregator must install drive denial when all children denied; map=%+v", denied) + } +} + // The binary root must never receive a denyStub even if every descendant // is denied. cobra still needs root to dispatch help / completion. func TestApply_neverInstallsOnRoot(t *testing.T) { diff --git a/internal/cmdpolicy/apply.go b/internal/cmdpolicy/apply.go index 6b28993d8..9843ad859 100644 --- a/internal/cmdpolicy/apply.go +++ b/internal/cmdpolicy/apply.go @@ -72,8 +72,32 @@ func Apply(root *cobra.Command, deniedByPath map[string]Denial) int { const ( AnnotationDenialLayer = "lark:policy_denied_layer" AnnotationDenialSource = "lark:policy_denied_source" + + // AnnotationPureGroup marks a cobra.Command that is logically a + // parent-only group but had a RunE attached by the bootstrap-time + // unknown-subcommand guard. The engine treats annotated commands + // the same as un-annotated parent groups (no RunE): they are not + // evaluated against the Rule, and aggregateParents does not treat + // them as hybrids. + // + // Without this signal, a user enabling a policy.yml with + // max_risk: read would see every group (`lark-cli drive --help`, + // `lark-cli docs --help`) return exit 2 + risk_not_annotated, + // because the guard's RunE flips Runnable()=true and the engine + // then demands a risk_level annotation on the group itself. + AnnotationPureGroup = "lark:cmd_pure_group" ) +// IsPureGroup reports whether cmd carries the AnnotationPureGroup marker. +// Used by the engine to skip evaluation and by the aggregator to treat the +// command as a parent-only group regardless of cobra's Runnable() answer. +func IsPureGroup(cmd *cobra.Command) bool { + if cmd == nil || cmd.Annotations == nil { + return false + } + return cmd.Annotations[AnnotationPureGroup] == "true" +} + // CommandDeniedFromDenial materialises the wrapped error type carried // on ExitError.Err so errors.As works for in-process consumers. func CommandDeniedFromDenial(path string, d Denial) *platform.CommandDeniedError { diff --git a/internal/cmdpolicy/engine.go b/internal/cmdpolicy/engine.go index b5ad70e40..71386f3f1 100644 --- a/internal/cmdpolicy/engine.go +++ b/internal/cmdpolicy/engine.go @@ -63,6 +63,14 @@ func (e *Engine) EvaluateAll(root *cobra.Command) map[string]Decision { if !c.Runnable() { return } + // Pure parent groups carrying the AnnotationPureGroup marker + // (installed by cmd.installUnknownSubcommandGuard) look + // Runnable to cobra but are not a real leaf: skip them just + // like cobra-native parent groups, so a user-level Rule does + // not block ` --help` discovery. + if IsPureGroup(c) { + return + } path := CanonicalPath(c) if path == "" { return @@ -211,7 +219,12 @@ func aggregateParents(cmd *cobra.Command, denied map[string]Denial) bool { } children := cmd.Commands() - cmdRunnable := cmd.Runnable() + // A pure parent group decorated with the unknown-subcommand guard + // looks Runnable() to cobra but is not a true hybrid: treat it + // exactly like cobra-native parent groups so the aggregation pass + // can still install an aggregate deny stub when every live child + // is denied. + cmdRunnable := cmd.Runnable() && !IsPureGroup(cmd) cmdPath := CanonicalPath(cmd) // Pure leaf @@ -277,7 +290,7 @@ func hasRunnableDescendant(cmd *cobra.Command) bool { if cmd == nil { return false } - if cmd.Runnable() { + if cmd.Runnable() && !IsPureGroup(cmd) { return true } for _, c := range cmd.Commands() { diff --git a/internal/cmdpolicy/path.go b/internal/cmdpolicy/path.go index 6ce4f1985..0db0486e0 100644 --- a/internal/cmdpolicy/path.go +++ b/internal/cmdpolicy/path.go @@ -4,9 +4,13 @@ package cmdpolicy import ( + "path/filepath" "strings" "github.com/spf13/cobra" + + "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/vfs" ) // CanonicalPath returns the rootless slash-separated path used everywhere in @@ -50,3 +54,67 @@ func useName(cmd *cobra.Command) string { } return name } + +// RedactHomeDir collapses environment-rooted prefixes so path strings +// can be safely surfaced through `config policy show` and resolver +// error messages without leaking the user's filesystem layout to AI +// agents / CI logs. +// +// It folds, in priority order: +// 1. core.GetBaseConfigDir() (typically ~/.lark-cli, or a custom +// directory under LARKSUITE_CLI_CONFIG_DIR — e.g. +// "/private/tmp/sandbox/.lark-cli" in a sandboxed run) → "" +// 2. The user's home directory → "~" +// +// (1) runs first so a `LARKSUITE_CLI_CONFIG_DIR` pointing outside `$HOME` +// still produces a stable, non-identifying label. When neither prefix +// matches, the input is returned unchanged — those cases don't leak +// anything that wasn't already passed in by the caller. +func RedactHomeDir(path string) string { + if path == "" { + return "" + } + abs, err := filepath.Abs(path) + if err != nil { + abs = path + } + + if rel, ok := foldPrefix(abs, core.GetBaseConfigDir()); ok { + if rel == "" { + return "" + } + return "/" + rel + } + + home, err := vfs.UserHomeDir() + if err != nil || home == "" { + return path + } + if rel, ok := foldPrefix(abs, home); ok { + if rel == "" { + return "~" + } + return "~/" + rel + } + return path +} + +// foldPrefix reports whether abs lives at or beneath prefix; on hit it +// returns the slash-form relative tail (empty when abs == prefix). +func foldPrefix(abs, prefix string) (string, bool) { + if prefix == "" { + return "", false + } + absPrefix, err := filepath.Abs(prefix) + if err != nil { + absPrefix = prefix + } + rel, err := filepath.Rel(absPrefix, abs) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", false + } + if rel == "." { + return "", true + } + return filepath.ToSlash(rel), true +} diff --git a/internal/cmdpolicy/path_test.go b/internal/cmdpolicy/path_test.go new file mode 100644 index 000000000..4cc88e883 --- /dev/null +++ b/internal/cmdpolicy/path_test.go @@ -0,0 +1,57 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package cmdpolicy_test + +import ( + "path/filepath" + "testing" + + "github.com/larksuite/cli/internal/cmdpolicy" +) + +// RedactHomeDir folds two prefixes: +// +// 1. core.GetBaseConfigDir() → "" (covers the +// LARKSUITE_CLI_CONFIG_DIR override, which is the only way a real +// deployment writes the policy file outside $HOME). +// 2. The user's home dir → "~" (catches the conventional +// ~/.lark-cli/policy.yml path when no override is set). +// +// Both folds run in path-prefix space (not string-prefix), so a path +// like "/Usersfoo" never gets folded against "/Users". +func TestRedactHomeDir_foldsConfigDirOverride(t *testing.T) { + tmp := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", tmp) + + policyPath := filepath.Join(tmp, "policy.yml") + got := cmdpolicy.RedactHomeDir(policyPath) + if got != "/policy.yml" { + t.Errorf("override path = %q, want /policy.yml", got) + } + + // A path that equals the config dir itself collapses to "". + if got := cmdpolicy.RedactHomeDir(tmp); got != "" { + t.Errorf("exact-prefix path = %q, want ", got) + } +} + +// A path outside both the config dir and $HOME stays absolute. This is +// the "no leak introduced" property: redaction never invents a label +// for something it doesn't recognise. +func TestRedactHomeDir_unrelatedPathUnchanged(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", "/var/lib/lark-cli") + + path := "/etc/random/file.yml" + if got := cmdpolicy.RedactHomeDir(path); got != path { + t.Errorf("unrelated path = %q, want %q (unchanged)", got, path) + } +} + +// Empty input round-trips. Callers (e.g. `config policy show` with +// no yaml configured) rely on this. +func TestRedactHomeDir_emptyStays(t *testing.T) { + if got := cmdpolicy.RedactHomeDir(""); got != "" { + t.Errorf("empty input = %q, want empty string", got) + } +} diff --git a/internal/cmdpolicy/resolver.go b/internal/cmdpolicy/resolver.go index 293097092..dea23b62c 100644 --- a/internal/cmdpolicy/resolver.go +++ b/internal/cmdpolicy/resolver.go @@ -82,22 +82,27 @@ func Resolve(pluginRules []PluginRule, yamlPath string) (*platform.Rule, Resolve // vfs.Stat lets callers swap in an in-memory FS for tests. The // errors here surface as typed os.ErrNotExist when the file is // absent, just like a direct os.ReadFile call would. + // + // Error messages use the home-dir-redacted form so the user's + // absolute path doesn't reach agents / CI logs through the + // warnPolicyError stderr line. + display := RedactHomeDir(yamlPath) if _, err := vfs.Stat(yamlPath); err != nil { if errors.Is(err, os.ErrNotExist) { return nil, ResolveSource{Kind: SourceNone}, nil } - return nil, ResolveSource{}, fmt.Errorf("stat policy yaml %q: %w", yamlPath, err) + return nil, ResolveSource{}, fmt.Errorf("stat policy yaml %q: %w", display, err) } data, err := vfs.ReadFile(yamlPath) if err != nil { - return nil, ResolveSource{}, fmt.Errorf("read policy yaml %q: %w", yamlPath, err) + return nil, ResolveSource{}, fmt.Errorf("read policy yaml %q: %w", display, err) } rule, err := pyaml.Parse(data) if err != nil { - return nil, ResolveSource{}, fmt.Errorf("policy yaml %q: %w", yamlPath, err) + return nil, ResolveSource{}, fmt.Errorf("policy yaml %q: %w", display, err) } if err := ValidateRule(rule); err != nil { - return nil, ResolveSource{}, fmt.Errorf("policy yaml %q: %w", yamlPath, err) + return nil, ResolveSource{}, fmt.Errorf("policy yaml %q: %w", display, err) } return rule, ResolveSource{Kind: SourceYAML, Name: yamlPath}, nil } diff --git a/internal/platform/host.go b/internal/platform/host.go index f7944db8e..2f13cf59d 100644 --- a/internal/platform/host.go +++ b/internal/platform/host.go @@ -116,6 +116,22 @@ func installOne(name string, p platform.Plugin, result *InstallResult) error { return capsErr } + // FailurePolicy is a closed enum. An out-of-range value almost + // always means the plugin author shipped FailurePolicy(2)/etc. by + // mistake, and the host's switch on caps.FailurePolicy below would + // silently treat the unknown value as FailOpen — defeating the + // security boundary the policy was meant to express. Reject up + // front with ReasonInvalidCapability (classified as + // untrusted-config, so the abort is unconditional). + if caps.FailurePolicy != platform.FailOpen && caps.FailurePolicy != platform.FailClosed { + return &PluginInstallError{ + PluginName: name, + ReasonCode: ReasonInvalidCapability, + Reason: fmt.Sprintf("FailurePolicy=%d is not a recognised value (expected FailOpen or FailClosed)", + caps.FailurePolicy), + } + } + // Strict consistency check: Restricts=true must pair with // FailClosed (design hard-constraint #6). if caps.Restricts && caps.FailurePolicy != platform.FailClosed {