diff --git a/CHANGELOG.md b/CHANGELOG.md index e0c1c1b6d..811ebf66f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## 0.35.1 - Unreleased +- Safety: allow custom baked profiles to lock boolean CLI flags against command-line, environment, and config overrides without echoing the locked value in override errors. (#976) — thanks @ronny-rentner. - Auth: offer one-time re-authorization for expired or revoked stored OAuth refresh tokens after interactive confirmation, while preserving non-interactive recovery guidance. (#973) — thanks @inamiy. - Dependencies: update Kong, Cloudflare Workers types, and pnpm to their latest releases. diff --git a/README.md b/README.md index d7a78d350..41a1452b8 100644 --- a/README.md +++ b/README.md @@ -101,8 +101,8 @@ gog --account you@gmail.com \ ``` See [Automation](docs/automation.md) for output and exit-code contracts, and -[Safety Profiles](docs/safety-profiles.md) for binaries with command policy -baked in at build time. +[Safety Profiles](docs/safety-profiles.md) for binaries with command policy and +locked flag values baked in at build time. ## Accounts and authentication diff --git a/cmd/bake-safety-profile/locked_flags_test.go b/cmd/bake-safety-profile/locked_flags_test.go new file mode 100644 index 000000000..44fbee0c5 --- /dev/null +++ b/cmd/bake-safety-profile/locked_flags_test.go @@ -0,0 +1,54 @@ +package main + +import ( + "bytes" + "fmt" + "go/parser" + "go/token" + "testing" + + "github.com/openclaw/gogcli/internal/safetyprofile" +) + +func TestGenerateLockedFlagsEmitsHashedLookup(t *testing.T) { + profile := &safetyprofile.Profile{ + Name: "test", + AllowRules: []string{"gmail.get"}, + LockedFlags: []safetyprofile.LockedFlag{ + {Name: "sanitize-content", Value: "true"}, + {Name: "inline-max-bytes", Value: "8388608"}, + }, + } + + out := generate(profile) + + if _, err := parser.ParseFile(token.NewFileSet(), "gen.go", out, parser.AllErrors); err != nil { + t.Fatalf("generated code does not parse as Go:\n%s\n\nerror: %v", out, err) + } + + // The flag name must be hashed like a command rule, and the value emitted as the + // literal the flag parser consumes. + for _, flag := range profile.LockedFlags { + want := fmt.Sprintf("\tcase 0x%016x:\n\t\treturn %q, true\n", safetyprofile.HashRule(flag.Name), flag.Value) + if !bytes.Contains(out, []byte(want)) { + t.Fatalf("generated output missing case for %q:\n%s\n\nfull output:\n%s", flag.Name, want, out) + } + if bytes.Contains(out, []byte(`"`+flag.Name+`"`)) { + t.Fatalf("locked flag name %q appears verbatim; it should only be hashed\n\nfull output:\n%s", flag.Name, out) + } + } +} + +// A profile with no locked flags still needs the lookup, so safety_profile builds +// compile whether or not the profile uses the feature. +func TestGenerateWithoutLockedFlagsStillDefinesLookup(t *testing.T) { + out := generate(&safetyprofile.Profile{Name: "test", AllowRules: []string{"gmail.get"}}) + + if _, err := parser.ParseFile(token.NewFileSet(), "gen.go", out, parser.AllErrors); err != nil { + t.Fatalf("generated code does not parse as Go:\n%s\n\nerror: %v", out, err) + } + want := "func bakedSafetyLockedFlag(name string) (string, bool) {\n\treturn \"\", false\n}\n" + if !bytes.Contains(out, []byte(want)) { + t.Fatalf("generated output missing the empty lookup:\n%s", out) + } +} diff --git a/cmd/bake-safety-profile/main.go b/cmd/bake-safety-profile/main.go index c4194ff62..4b152bead 100644 --- a/cmd/bake-safety-profile/main.go +++ b/cmd/bake-safety-profile/main.go @@ -64,10 +64,38 @@ func generate(profile *safetyprofile.Profile) []byte { writeMatcher(&out, "bakedSafetyAllowMatch", profile.AllowRules, profile.AllowAll) out.WriteString("\n") writeMatcher(&out, "bakedSafetyDenyMatch", profile.DenyRules, false) + out.WriteString("\n") + writeLockedFlags(&out, profile.LockedFlags) return out.Bytes() } +// writeLockedFlags emits the locked flag lookup. Names are hashed like the command +// rules; the locked boolean values are literals because the flag parser consumes them. The +// count travels alongside so the runtime can tell that every lock matched a real +// flag without the names being written down. +func writeLockedFlags(out *bytes.Buffer, flags []safetyprofile.LockedFlag) { + out.WriteString("func bakedSafetyLockedFlag(name string) (string, bool) {\n") + if len(flags) == 0 { + out.WriteString("\treturn \"\", false\n}\n") + out.WriteString("\nfunc bakedSafetyLockedFlagCount() int { return 0 }\n") + return + } + out.WriteString("\tswitch bakedSafetyHashPath([]string{name}) {\n") + seen := make(map[uint64]string, len(flags)) + for _, flag := range flags { + h := safetyprofile.HashRule(flag.Name) + if existing, dup := seen[h]; dup { + fmt.Fprintf(os.Stderr, "bake-safety-profile: hash collision between locked flags %q and %q (FNV-64a=%#x)\n", existing, flag.Name, h) + os.Exit(1) + } + seen[h] = flag.Name + fmt.Fprintf(out, "\tcase 0x%016x:\n\t\treturn %s, true\n", h, strconv.Quote(flag.Value)) + } + out.WriteString("\t}\n\treturn \"\", false\n}\n") + fmt.Fprintf(out, "\nfunc bakedSafetyLockedFlagCount() int { return %d }\n", len(flags)) +} + func writeMatcher(out *bytes.Buffer, name string, rules []string, matchAll bool) { fmt.Fprintf(out, "func %s(path []string) bool {\n", name) if matchAll { diff --git a/docs/safety-profiles.md b/docs/safety-profiles.md index 0e8981436..9e5a07843 100644 --- a/docs/safety-profiles.md +++ b/docs/safety-profiles.md @@ -9,6 +9,10 @@ Runtime guards such as `--enable-commands`, `--disable-commands`, and stronger: the policy is compiled into the binary and cannot be changed with flags, environment variables, config files, or shell arguments. +A profile controls which commands may run. Through `locked-flags` it can also fix +the value of individual flags, so settings such as sanitized output do not depend +on the caller passing them. See [Locked Flags](#locked-flags). + ## Quick Start Build an agent-safe binary: @@ -142,6 +146,7 @@ Rules: - unlisted commands are blocked when the profile has any allow rules. - command names are written as dot paths internally, such as `gmail.drafts.create`. - `aliases:` controls root shortcuts such as `send`, `ls`, `search`, and `upload`. +- `locked-flags:` is not a command path; it locks flag values, see [Locked Flags](#locked-flags). Parent rules are prefix matches. For example, `drive: true` allows every `drive` subcommand unless a child is explicitly blocked. For restrictive profiles, prefer @@ -155,6 +160,87 @@ gmail: modify: false ``` +## Locked Flags + +Command rules decide what may run. They do not decide how it runs. Sanitized +output, for example, happens only when the caller passes `--sanitize-content`, +and a flag that does take its value from the environment is still overridden by +the command line. When the command line is written by a model rather than a +person, neither is a setting you can rely on. + +`locked-flags` fixes a flag's value in the profile: + +```yaml +locked-flags: + sanitize-content: true + wrap-untrusted: true + no-input: true +``` + +Locked values and the flags they target must be boolean. This intentionally keeps +the mechanism focused on safety policy rather than compiling account names, paths, +tokens, or other arbitrary configuration into a binary. + +A locked flag behaves as follows: + +- the value is applied before the command runs, so the caller need not pass it. +- setting that flag on the command line is an error, not an override. +- aliases are covered, since the check is on the canonical flag name. +- a locked flag that the selected command does not define is ignored rather than + an error, so per-command flags can be locked without breaking other commands. +- a locked output mode wins over the competing one. With `json` locked, `GOG_PLAIN` + gives way silently, since an environment default is a preference rather than a + request, while an explicit `--plain` is refused for asking output the profile + forbids. +- commands that build partial requests from which flags were given treat a locked + flag as given, so the locked value reaches the request. +- override errors name the flag and profile without printing the locked value. + +Non-boolean flags are refused. `--help` and `--version`, which Kong executes before +normal parsing, are refused too, as are `--home`, required flags, and names matching +no flag at all. In each case the binary declines to run instead of reporting a +guarantee it does not have. + +Setting a locked flag fails before the command handler runs: + +```text +flag --sanitize-content is locked by baked safety profile "agent-safe-locked" +``` + +Because a locked value can make a command reject a combination the caller never +asked for, usage errors name the locked flags: + +```text +--sanitize-content cannot be used with --format raw +note: --no-input, --sanitize-content, --wrap-untrusted locked by baked safety profile "agent-safe-locked" +``` + +Lock deliberately. A flag that participates in a conflict or a requirement removes +the command paths that contradict it. Locking `sanitize-content` makes +`gmail get --format raw` unavailable, which is an acceptable trade for an agent +profile because raw is the unsanitized dump. + +Locking a boolean flag that requires another one is the case to avoid. `--reply-all` needs +a message or thread to reply to, so `locked-flags: {reply-all: true}` breaks an +ordinary draft: + +```bash +bin/gog-my-agent gmail drafts create --to you@example.com --subject Hi --body Hi +``` + +The command is allowed by the profile and the caller passed nothing wrong, yet it +fails because the locked flag demands an argument they had no reason to supply: + +```text +--reply-all requires --reply-to-message-id or --thread-id +``` + +Flags that only shape output are the safe candidates. + +To make an existing preset stricter without changing its behavior for everyone, +copy it and add the locks you need. For example, an agent-oriented copy commonly +locks `sanitize-content`, `wrap-untrusted`, and `no-input` to `true`. + ## Choosing A Profile Use `readonly` when the caller should never change Google or local `gog` state. diff --git a/internal/cmd/kong_helpers.go b/internal/cmd/kong_helpers.go index 77c238510..c074e3c6f 100644 --- a/internal/cmd/kong_helpers.go +++ b/internal/cmd/kong_helpers.go @@ -2,7 +2,18 @@ package cmd import "github.com/alecthomas/kong" +// flagProvided reports whether a command should treat the flag as supplied. A value +// a baked profile locked counts: commands that build partial requests from which +// flags were given would otherwise drop a locked value on the floor, leaving the +// lock set on the struct but absent from the request. func flagProvided(kctx *kong.Context, name string) bool { + return flagOnCommandLine(kctx, name) || lockedFlagNames[name] +} + +// flagOnCommandLine reports only what the caller typed. Lock enforcement and +// output-mode precedence need that narrower question: one to detect an override +// attempt, the other to rank an explicit flag against an environment default. +func flagOnCommandLine(kctx *kong.Context, name string) bool { if kctx == nil { return false } diff --git a/internal/cmd/root.go b/internal/cmd/root.go index 60a8a2f11..12e2404bd 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -119,6 +119,7 @@ func Execute(args []string) (err error) { } func executeWithRuntime(args []string, runtime *app.Runtime) (err error) { + resetLockedFlagState() runtime = normalizedRuntime(runtime) runtimeIO := runtime.IO @@ -141,6 +142,9 @@ func executeWithRuntime(args []string, runtime *app.Runtime) (err error) { if err != nil { return reportEarlyError(runtimeIO.Err, err) } + if err = verifyLockedFlagsExist(parser.Model.Node); err != nil { + return reportEarlyError(runtimeIO.Err, err) + } args = rewriteDocsCellUpdateContentArgs(parser.Model, args) args = rewriteDesirePathArgs(parser.Model, args) @@ -165,7 +169,6 @@ func executeWithRuntime(args []string, runtime *app.Runtime) (err error) { cli.diagnostics = runtimeIO.Err cli.authOperations = runtime.Auth cli.authMode = googleapi.ParseAuthMode(os.Getenv("GOG_AUTH_MODE")) - applyExplicitOutputModePrecedence(kctx, &cli.RootFlags) // Make config-backed account and alias resolution available to the // pre-Run enforcement hooks below (enforceGmailNoSend resolves the @@ -179,9 +182,24 @@ func executeWithRuntime(args []string, runtime *app.Runtime) (err error) { return runtime.Config, nil } + // Treat automatic JSON as an ambient default, like the parser's environment + // and config-backed defaults. Locked flags run afterwards and therefore remain + // authoritative when a profile fixes json to false or plain to true. + if envBool("GOG_AUTO_JSON") && !cli.JSON && !cli.Plain && !isTerminalWriter(runtimeIO.Out) { + cli.JSON = true + } + if err = enforceBakedSafetyProfile(kctx); err != nil { return reportEarlyError(runtimeIO.Err, err) } + if err = enforceLockedFlags(kctx); err != nil { + return reportEarlyError(runtimeIO.Err, err) + } + // After the locks, so a locked output mode is what precedence resolves around + // rather than something a competing mode can leave in conflict. + if err = applyExplicitOutputModePrecedence(kctx, &cli.RootFlags); err != nil { + return reportEarlyError(runtimeIO.Err, err) + } if err = enforceEnabledCommands(kctx, cli.EnableCommands, cli.EnableCommandsExact); err != nil { return reportEarlyError(runtimeIO.Err, err) } @@ -202,12 +220,6 @@ func executeWithRuntime(args []string, runtime *app.Runtime) (err error) { }))) defer slog.SetDefault(previousLogger) - // Optional automatic JSON output when stdout is piped/non-TTY. - // We intentionally do this after parsing so `--plain` can override it. - if envBool("GOG_AUTO_JSON") && !cli.JSON && !cli.Plain && !isTerminalWriter(runtimeIO.Out) { - cli.JSON = true - } - mode, err := outfmt.FromFlags(cli.JSON, cli.Plain) if err != nil { return reportEarlyError(runtimeIO.Err, newUsageError(err)) @@ -334,13 +346,13 @@ func executeWithRuntime(args []string, runtime *app.Runtime) (err error) { err = stableExitCode(err) if u := ui.FromContext(ctx); u != nil { - msg := strings.TrimSpace(errfmt.Format(err)) + msg := errorMessage(err) if msg != "" { u.Err().Error(msg) } return err } - msg := strings.TrimSpace(errfmt.Format(err)) + msg := errorMessage(err) if msg != "" { _, _ = fmt.Fprintln(runtimeIO.Err, msg) } @@ -400,32 +412,69 @@ func validateJSONTransformFlags(mode outfmt.Mode, flags *RootFlags) error { } } -func applyExplicitOutputModePrecedence(kctx *kong.Context, flags *RootFlags) { +// applyExplicitOutputModePrecedence settles --json against --plain, which cannot +// both be set. A locked mode outranks the competing one: typing that competing flag +// is refused the way setting the locked flag itself is, since the caller asked for +// output the profile forbids, while an environment default gives way silently +// because it is an ambient setting rather than a request about this invocation. +func applyExplicitOutputModePrecedence(kctx *kong.Context, flags *RootFlags) error { if flags == nil { - return + return nil } - jsonSet := flagProvided(kctx, "json") - plainSet := flagProvided(kctx, "plain") + jsonLocked := lockedFlagNames["json"] && flags.JSON + plainLocked := lockedFlagNames["plain"] && flags.Plain + jsonSet := flagOnCommandLine(kctx, "json") + plainSet := flagOnCommandLine(kctx, "plain") switch { + case jsonLocked && !plainLocked: + if plainSet { + return usagef("flag --plain conflicts with --json, locked by baked safety profile %q", bakedSafetyProfileName()) + } + flags.Plain = false + case plainLocked && !jsonLocked: + if jsonSet { + return usagef("flag --json conflicts with --plain, locked by baked safety profile %q", bakedSafetyProfileName()) + } + flags.JSON = false case jsonSet && !plainSet: flags.Plain = false case plainSet && !jsonSet: flags.JSON = false } + return nil } func reportEarlyError(w io.Writer, err error) error { if err == nil { return nil } - msg := strings.TrimSpace(errfmt.Format(err)) + msg := errorMessage(err) if msg != "" { _, _ = fmt.Fprintln(w, msg) } return err } +// errorMessage renders a command's error for display. Usage errors carry the +// locked-flag note, because a baked profile can supply a value the caller never +// passed and the rejection then names a flag absent from their command line. The +// pre-run enforcement errors skip it: those name the locked flag themselves. +func errorMessage(err error) string { + msg := strings.TrimSpace(errfmt.Format(err)) + if msg == "" { + return msg + } + if ExitCode(err) != 2 { + return msg + } + note := lockedFlagsNote() + if note == "" { + return msg + } + return msg + "\n" + note +} + func isTerminalWriter(w io.Writer) bool { file, ok := w.(*os.File) return ok && termutil.IsTerminal(file) diff --git a/internal/cmd/safety_profile.go b/internal/cmd/safety_profile.go index 64132bd80..9ef1d405a 100644 --- a/internal/cmd/safety_profile.go +++ b/internal/cmd/safety_profile.go @@ -1,7 +1,10 @@ package cmd import ( + "fmt" "hash/fnv" + "reflect" + "sort" "strings" "github.com/alecthomas/kong" @@ -43,6 +46,135 @@ func enforceBakedSafetyProfile(kctx *kong.Context) error { return nil } +// lockedFlagNames records the flags enforceLockedFlags set, so a command rejecting a +// value it never received on the command line can say where that value came from. +var lockedFlagNames = map[string]bool{} + +func resetLockedFlagState() { + lockedFlagNames = map[string]bool{} +} + +// lockedFlagsNote names the locked flags for display beneath a usage error. A command +// can reject a combination involving a value the caller never passed, so the note is +// what explains where that value came from. Empty when nothing is locked. +func lockedFlagsNote() string { + if len(lockedFlagNames) == 0 { + return "" + } + names := make([]string, 0, len(lockedFlagNames)) + for name := range lockedFlagNames { + names = append(names, "--"+name) + } + sort.Strings(names) + return fmt.Sprintf("note: %s locked by baked safety profile %q", strings.Join(names, ", "), bakedSafetyProfileName()) +} + +// verifyLockedFlagsExist refuses to run when a locked name matches no flag in the +// CLI. Enforcement only ever asks whether a given flag is locked, so a misspelled +// name would lock nothing at all and the profile would claim a guarantee it does not +// have. Counting the matches catches that without the names appearing in the binary. +func verifyLockedFlagsExist(root *kong.Node) error { + want := bakedSafetyLockedFlagCount() + if want == 0 { + return nil + } + seen := map[string]bool{} + var unsupported error + var walk func(node *kong.Node) + walk = func(node *kong.Node) { + if node == nil { + return + } + for _, flag := range node.Flags { + if _, locked := bakedSafetyLockedFlag(flag.Name); !locked { + continue + } + seen[flag.Name] = true + if err := lockUnsupported(flag); err != nil && unsupported == nil { + unsupported = err + } + } + for _, child := range node.Children { + walk(child) + } + } + walk(root) + if len(seen) < want { + return usagef("baked safety profile %q locks %d flag(s) but only %d exist; check the locked-flags names", bakedSafetyProfileName(), want, len(seen)) + } + return unsupported +} + +// lockUnsupported names the flags a lock cannot reach, because their value is +// consumed before locks are applied. Refusing them is honest: the alternative is a +// profile that reports a guarantee the run never had. +func lockUnsupported(flag *kong.Flag) error { + switch { + case flag.Name == "home": + // The layout resolver reads --home straight from argv before Kong parses, so + // config and credential roots are already chosen by the time locks run. + return usagef("baked safety profile %q locks --home, which is read before flags are parsed and cannot be locked", bakedSafetyProfileName()) + case flag.Name == "help" || flag.Name == "version": + // Kong executes these flags from a BeforeReset hook and exits before normal + // defaults, validation, and locked-flag enforcement run. + return usagef("baked safety profile %q locks --%s, which runs before flags are parsed and cannot be locked", bakedSafetyProfileName(), flag.Name) + case flag.Required: + // Kong rejects a missing required flag during parsing, so a locked one fails + // when omitted and is refused as an override when supplied. + return usagef("baked safety profile %q locks required flag --%s, which must be supplied on the command line", bakedSafetyProfileName(), flag.Name) + case !flag.IsBool(): + return usagef("baked safety profile %q locks --%s, but only boolean flags can be locked", bakedSafetyProfileName(), flag.Name) + } + return nil +} + +// enforceLockedFlags applies the profile's locked flag values and refuses a command +// line that sets one of them. The value is locked rather than merely defaulted so it +// holds without help from the environment, which the caller may not control. +func enforceLockedFlags(kctx *kong.Context) error { + // Rebuilt per parse: carrying names over would let one run's note describe a + // profile that is not in force. + resetLockedFlagState() + if !bakedSafetyEnabled() { + return nil + } + lockedPaths := make([]*kong.Path, 0, bakedSafetyLockedFlagCount()) + for _, flag := range kctx.Flags() { + value, locked := bakedSafetyLockedFlag(flag.Name) + if !locked { + continue + } + if flagOnCommandLine(kctx, flag.Name) { + return usagef("flag --%s is locked by baked safety profile %q", flag.Name, bakedSafetyProfileName()) + } + // Decode into a zero target so the locked boolean replaces any environment or + // default value already resolved by Kong. + lockedTarget := reflect.New(flag.Target.Type()).Elem() + if err := flag.Parse(kong.ScanFromTokens(kong.Token{Type: kong.FlagValueToken, Value: value}), lockedTarget); err != nil { + return usagef("locked boolean flag --%s could not be applied", flag.Name) + } + flag.Apply(lockedTarget) + lockedFlagNames[flag.Name] = true + lockedPaths = append(lockedPaths, &kong.Path{Flag: flag}) + } + if len(lockedPaths) == 0 { + return nil + } + + // Parsing a mapper is not the whole Kong contract. Revalidate enums, flag and + // command validators, required groups, and xor/and relationships with the locks + // represented as supplied flags. Restore the real command-line trace afterwards + // so override detection continues to mean "typed by the caller". + pathLen := len(kctx.Path) + kctx.Path = append(kctx.Path, lockedPaths...) + validationErr := kctx.Validate() + kctx.Path = kctx.Path[:pathLen] + if validationErr != nil { + return usagef("baked safety profile %q has locked flags that are invalid for the selected command", bakedSafetyProfileName()) + } + return nil +} + func bakedSafetyProfileError(path []string, profileName string, included bool) error { command := strings.Join(path, " ") if included { diff --git a/internal/cmd/safety_profile_default.go b/internal/cmd/safety_profile_default.go index 5cf14ef66..78eaea1fc 100644 --- a/internal/cmd/safety_profile_default.go +++ b/internal/cmd/safety_profile_default.go @@ -16,6 +16,7 @@ var bakedSafetyTestProfile struct { allowAll bool allow map[string]bool deny map[string]bool + lockedFlags map[string]string } func bakedSafetyEnabled() bool { return bakedSafetyTestProfile.enabled } @@ -32,3 +33,12 @@ func bakedSafetyAllowMatch(path []string) bool { func bakedSafetyDenyMatch(path []string) bool { return commandPathMatches(bakedSafetyTestProfile.deny, path) } + +func bakedSafetyLockedFlag(name string) (string, bool) { + value, ok := bakedSafetyTestProfile.lockedFlags[name] + return value, ok +} + +func bakedSafetyLockedFlagCount() int { + return len(bakedSafetyTestProfile.lockedFlags) +} diff --git a/internal/cmd/safety_profile_locked_flags_test.go b/internal/cmd/safety_profile_locked_flags_test.go new file mode 100644 index 000000000..579aed250 --- /dev/null +++ b/internal/cmd/safety_profile_locked_flags_test.go @@ -0,0 +1,351 @@ +package cmd + +import ( + "strings" + "testing" +) + +const lockedFlagsProfile = ` +name: locked +locked-flags: + sanitize-content: true +gmail: + get: true + search: true +` + +// A lock must hold whatever spelling reaches it, including the value it already +// has: accepting a matching value would make the lock depend on what the caller +// asked for. +func TestLockedFlag_RejectsEveryFormOfSettingIt(t *testing.T) { + for _, arg := range []string{ + "--sanitize-content=false", + "--sanitize-content=true", + "--sanitize-content", + "--sanitize=false", + "--safe", + } { + t.Run(arg, func(t *testing.T) { + withBakedSafetyProfile(t, lockedFlagsProfile) + result := executeWithTestRuntime(t, []string{ + "--json", "--account", "a@b.com", + "gmail", "get", "m1", arg, + }, nil) + if result.err == nil { + t.Fatalf("%s must be rejected, got stdout=%q", arg, result.stdout) + } + if !strings.Contains(result.err.Error(), "--sanitize-content is locked") { + t.Fatalf("%s: err = %v", arg, result.err) + } + }) + } +} + +// Locking a flag one command declares must not disturb commands that do not, or +// locking a per-command flag would break the rest of the CLI. +func TestLockedFlag_InertOnCommandsWithoutTheFlag(t *testing.T) { + withBakedSafetyProfile(t, lockedFlagsProfile) + result := executeWithTestRuntime(t, []string{ + "--json", "--account", "a@b.com", + "gmail", "search", "newer_than:1d", + }, nil) + if result.err != nil && strings.Contains(result.err.Error(), "is locked") { + t.Fatalf("lock leaked into a command without the flag: %v", result.err) + } +} + +// Stock builds compile the no-profile stub, so locks must not apply at all. +func TestLockedFlag_IgnoredWithoutBakedProfile(t *testing.T) { + result := executeWithTestRuntime(t, []string{ + "--json", "--account", "a@b.com", + "gmail", "get", "m1", "--sanitize-content=false", + }, nil) + if result.err != nil && strings.Contains(result.err.Error(), "is locked") { + t.Fatalf("lock applied without a baked profile: %v", result.err) + } +} + +func TestLockedFlag_RejectsCommandLineOverride(t *testing.T) { + withBakedSafetyProfile(t, lockedFlagsProfile) + result := executeWithTestRuntime(t, []string{ + "--json", "--account", "a@b.com", + "gmail", "get", "m1", "--sanitize-content=false", + }, nil) + if result.err == nil { + t.Fatalf("locked flag override must fail, got stdout=%q", result.stdout) + } + if !strings.Contains(result.err.Error(), "--sanitize-content is locked") { + t.Fatalf("err = %v", result.err) + } +} + +func TestLockedFlag_RejectsAliasOverride(t *testing.T) { + withBakedSafetyProfile(t, lockedFlagsProfile) + result := executeWithTestRuntime(t, []string{ + "--json", "--account", "a@b.com", + "gmail", "get", "m1", "--safe=false", + }, nil) + if result.err == nil { + t.Fatalf("alias override must fail, got stdout=%q", result.stdout) + } + if !strings.Contains(result.err.Error(), "--sanitize-content is locked") { + t.Fatalf("err = %v", result.err) + } +} + +// The locked value must reach the command, not merely block overrides. gmail get +// refuses --format raw when sanitize is on, so that refusal proves the profile set +// the flag even though the command line never mentioned it. +func TestLockedFlag_LockedValueReachesCommand(t *testing.T) { + withBakedSafetyProfile(t, lockedFlagsProfile) + result := executeWithTestRuntime(t, []string{ + "--json", "--account", "a@b.com", + "gmail", "get", "m1", "--format", "raw", + }, nil) + if result.err == nil { + t.Fatalf("locked sanitize-content must reject --format raw, got stdout=%q", result.stdout) + } + if !strings.Contains(result.err.Error(), "cannot be used with --format raw") { + t.Fatalf("err = %v", result.err) + } + // The caller never passed the flag, so the printed message has to say where the + // value came from. The note is added at display time, not to the error itself. + if !strings.Contains(result.stderr, `note: --sanitize-content locked by baked safety profile "locked"`) { + t.Fatalf("stderr lacks the locked-flag note: %q", result.stderr) + } +} + +// Without the profile the same command line is accepted, so the rejection above +// comes from the lock rather than from the flag being unusable. +func TestLockedFlag_UnlockedProfileAllowsOverride(t *testing.T) { + withBakedSafetyProfile(t, "name: unlocked\ngmail:\n get: true\n") + result := executeWithTestRuntime(t, []string{ + "--json", "--account", "a@b.com", + "gmail", "get", "m1", "--sanitize-content=false", + }, nil) + if result.err != nil && strings.Contains(result.err.Error(), "is locked") { + t.Fatalf("unlocked profile must not reject the flag: %v", result.err) + } +} + +const lockedJSONProfile = ` +name: locked +locked-flags: + json: true +version: true +` + +// --json and --plain are mutually exclusive, so asking for the competing mode is +// asking for output the profile forbids. Refusing it rather than quietly producing +// the locked mode keeps typing --plain as visible as typing the locked flag itself. +func TestLockedFlag_LockedJSONRejectsExplicitPlain(t *testing.T) { + withBakedSafetyProfile(t, lockedJSONProfile) + result := executeWithTestRuntime(t, []string{"--plain", "version"}, nil) + if result.err == nil { + t.Fatalf("--plain against a locked json must fail, got stdout=%q", result.stdout) + } + if !strings.Contains(result.err.Error(), `--plain conflicts with --json, locked by baked safety profile "locked"`) { + t.Fatalf("err = %v", result.err) + } +} + +func TestLockedFlag_LockedJSONBeatsEnvironmentPlain(t *testing.T) { + t.Setenv("GOG_PLAIN", "1") + withBakedSafetyProfile(t, lockedJSONProfile) + result := executeWithTestRuntime(t, []string{"version"}, nil) + if result.err != nil { + t.Fatalf("locked json with GOG_PLAIN must not fail: %v (stderr=%q)", result.err, result.stderr) + } + if !strings.HasPrefix(strings.TrimSpace(result.stdout), "{") { + t.Fatalf("expected JSON output, got %q", result.stdout) + } +} + +func TestLockedFlag_LockedPlainRejectsExplicitJSON(t *testing.T) { + withBakedSafetyProfile(t, ` +name: locked +locked-flags: + plain: true +version: true +`) + result := executeWithTestRuntime(t, []string{"--json", "version"}, nil) + if result.err == nil { + t.Fatalf("--json against a locked plain must fail, got stdout=%q", result.stdout) + } + if !strings.Contains(result.err.Error(), `--json conflicts with --plain, locked by baked safety profile "locked"`) { + t.Fatalf("err = %v", result.err) + } +} + +// The locked mode still applies when nothing competes on the command line. +func TestLockedFlag_LockedPlainAppliesWithoutCompetingFlag(t *testing.T) { + withBakedSafetyProfile(t, ` +name: locked +locked-flags: + plain: true +version: true +`) + result := executeWithTestRuntime(t, []string{"version"}, nil) + if result.err != nil { + t.Fatalf("locked plain must apply: %v (stderr=%q)", result.err, result.stderr) + } + if strings.HasPrefix(strings.TrimSpace(result.stdout), "{") { + t.Fatalf("expected plain output, got %q", result.stdout) + } +} + +func TestLockedFlag_FalseJSONAllowsPlain(t *testing.T) { + withBakedSafetyProfile(t, ` +name: locked +locked-flags: + json: false +version: true +`) + result := executeWithTestRuntime(t, []string{"--plain", "version"}, nil) + if result.err != nil { + t.Fatalf("false json lock must allow plain: %v (stderr=%q)", result.err, result.stderr) + } + if strings.HasPrefix(strings.TrimSpace(result.stdout), "{") { + t.Fatalf("expected plain output, got %q", result.stdout) + } +} + +func TestLockedFlag_FalsePlainAllowsJSON(t *testing.T) { + withBakedSafetyProfile(t, ` +name: locked +locked-flags: + plain: false +version: true +`) + result := executeWithTestRuntime(t, []string{"--json", "version"}, nil) + if result.err != nil { + t.Fatalf("false plain lock must allow json: %v (stderr=%q)", result.err, result.stderr) + } + if !strings.HasPrefix(strings.TrimSpace(result.stdout), "{") { + t.Fatalf("expected JSON output, got %q", result.stdout) + } +} + +func TestLockedFlag_FalseJSONOverridesEnvironmentDefaults(t *testing.T) { + for _, key := range []string{"GOG_JSON", "GOG_AUTO_JSON"} { + t.Run(key, func(t *testing.T) { + t.Setenv(key, "1") + withBakedSafetyProfile(t, ` +name: locked +locked-flags: + json: false +version: true +`) + result := executeWithTestRuntime(t, []string{"version"}, nil) + if result.err != nil { + t.Fatalf("false json lock with %s must not fail: %v (stderr=%q)", key, result.err, result.stderr) + } + if strings.HasPrefix(strings.TrimSpace(result.stdout), "{") { + t.Fatalf("%s overrode locked json=false: %q", key, result.stdout) + } + }) + } +} + +// A locked name that matches no flag locks nothing, so the binary must refuse to run +// rather than report a guarantee it cannot keep. +func TestLockedFlag_NonexistentNameRefusesToRun(t *testing.T) { + withBakedSafetyProfile(t, ` +name: locked +locked-flags: + sanitize-contnet: true +gmail: + get: true +`) + result := executeWithTestRuntime(t, []string{ + "--json", "--account", "a@b.com", + "gmail", "get", "m1", + }, nil) + if result.err == nil { + t.Fatalf("a misspelled locked flag must fail, got stdout=%q", result.stdout) + } + if !strings.Contains(result.err.Error(), "locks 1 flag(s) but only 0 exist") { + t.Fatalf("err = %v", result.err) + } +} + +// Commands that build partial requests ask which flags were given, so a locked value +// has to count as given or it never reaches the request. Lock enforcement asks the +// narrower question and must not see itself as an override. +func TestLockedFlag_CountsAsProvidedButNotAsTyped(t *testing.T) { + previous := lockedFlagNames + lockedFlagNames = map[string]bool{"summary": true} + t.Cleanup(func() { lockedFlagNames = previous }) + + if !flagProvided(nil, "summary") { + t.Fatal("a locked flag must count as provided") + } + if flagOnCommandLine(nil, "summary") { + t.Fatal("a locked flag must not count as typed on the command line") + } +} + +// --home is read from argv before Kong parses, so a lock on it would leave the +// caller's own config and credential roots in place while the profile claimed +// otherwise. Refusing is the honest outcome. +func TestLockedFlag_HomeIsRefused(t *testing.T) { + withBakedSafetyProfile(t, ` +name: locked +locked-flags: + home: false +version: true +`) + result := executeWithTestRuntime(t, []string{"version"}, nil) + if result.err == nil { + t.Fatalf("locking --home must fail, got stdout=%q", result.stdout) + } + if !strings.Contains(result.err.Error(), "locks --home") { + t.Fatalf("err = %v", result.err) + } +} + +func TestLockedFlag_NonBooleanFlagIsRefused(t *testing.T) { + withBakedSafetyProfile(t, ` +name: locked +locked-flags: + format: true +gmail: + get: true +`) + result := executeWithTestRuntime(t, []string{ + "--account", "nobody@example.invalid", + "gmail", "get", "m1", + }, nil) + if result.err == nil { + t.Fatal("locking non-boolean --format must fail") + } + if !strings.Contains(result.err.Error(), "only boolean flags can be locked") { + t.Fatalf("err = %v", result.err) + } +} + +func TestLockedFlag_PreParseFlagsAreRefusedBeforeTheyCanExit(t *testing.T) { + for _, tc := range []struct { + name string + args []string + }{ + {name: "help", args: []string{"--help"}}, + {name: "version", args: []string{"--version"}}, + } { + t.Run(tc.name, func(t *testing.T) { + withBakedSafetyProfile(t, ` +name: locked +locked-flags: + `+tc.name+`: false +version: true +`) + result := executeWithTestRuntime(t, tc.args, nil) + if result.err == nil { + t.Fatalf("locking --%s must fail before Kong exits, got stdout=%q", tc.name, result.stdout) + } + if !strings.Contains(result.err.Error(), "runs before flags are parsed") { + t.Fatalf("err = %v", result.err) + } + }) + } +} diff --git a/internal/cmd/safety_profile_test.go b/internal/cmd/safety_profile_test.go index d3ba212c7..db63709a7 100644 --- a/internal/cmd/safety_profile_test.go +++ b/internal/cmd/safety_profile_test.go @@ -33,6 +33,11 @@ func withBakedSafetyProfile(t *testing.T, raw string) { bakedSafetyTestProfile.hasAllowRules = profile.AllowAll || len(profile.AllowRules) > 0 bakedSafetyTestProfile.allow = allow bakedSafetyTestProfile.deny = deny + locked := make(map[string]string, len(profile.LockedFlags)) + for _, f := range profile.LockedFlags { + locked[f.Name] = f.Value + } + bakedSafetyTestProfile.lockedFlags = locked t.Cleanup(func() { bakedSafetyTestProfile = prev }) } diff --git a/internal/safetyprofile/locked_flags_test.go b/internal/safetyprofile/locked_flags_test.go new file mode 100644 index 000000000..9d4206272 --- /dev/null +++ b/internal/safetyprofile/locked_flags_test.go @@ -0,0 +1,80 @@ +package safetyprofile + +import "testing" + +func TestParse_LockedFlagsValues(t *testing.T) { + profile, err := Parse(` +name: locked +locked-flags: + sanitize-content: true + wrap-untrusted: false +gmail: + get: true +`) + if err != nil { + t.Fatalf("Parse: %v", err) + } + want := []LockedFlag{ + {Name: "sanitize-content", Value: "true"}, + {Name: "wrap-untrusted", Value: "false"}, + } + if len(profile.LockedFlags) != len(want) { + t.Fatalf("locked flags = %#v", profile.LockedFlags) + } + for i, w := range want { + if profile.LockedFlags[i] != w { + t.Fatalf("locked flag %d = %#v, want %#v", i, profile.LockedFlags[i], w) + } + } +} + +func TestParse_LockedFlagsRejectsNonBooleanValues(t *testing.T) { + for _, value := range []string{"1", "metadata"} { + t.Run(value, func(t *testing.T) { + _, err := Parse(` +name: locked +locked-flags: + format: ` + value + ` +gmail: + get: true +`) + if err == nil { + t.Fatal("non-boolean locked value must be rejected") + } + }) + } +} + +func TestParse_LockedFlagsRejectsNonScalar(t *testing.T) { + _, err := Parse(` +name: locked +locked-flags: + sanitize-content: + nested: true +gmail: + get: true +`) + if err == nil { + t.Fatal("nested locked-flags value must be rejected") + } +} + +// locked-flags must not be flattened into command rules the way other top-level +// keys are. +func TestParse_LockedFlagsAreNotCommandRules(t *testing.T) { + profile, err := Parse(` +name: locked +locked-flags: + sanitize-content: true +gmail: + get: true +`) + if err != nil { + t.Fatalf("Parse: %v", err) + } + for _, rule := range append(profile.AllowRules, profile.DenyRules...) { + if rule == "locked-flags.sanitize-content" || rule == "sanitize-content" { + t.Fatalf("locked flag leaked into command rules: %v", rule) + } + } +} diff --git a/internal/safetyprofile/parse.go b/internal/safetyprofile/parse.go index c431805e0..6b2c5f905 100644 --- a/internal/safetyprofile/parse.go +++ b/internal/safetyprofile/parse.go @@ -25,6 +25,10 @@ func Parse(raw string) (*Profile, error) { Name: parsed.name, AllowAll: parsed.allow[literalAll] || parsed.allow["*"], } + for name, value := range parsed.lockedFlags { + out.LockedFlags = append(out.LockedFlags, LockedFlag{Name: name, Value: value}) + } + sort.Slice(out.LockedFlags, func(i, j int) bool { return out.LockedFlags[i].Name < out.LockedFlags[j].Name }) for k := range parsed.allow { if k == literalAll || k == "*" { continue @@ -40,9 +44,10 @@ func Parse(raw string) (*Profile, error) { } type rawProfile struct { - name string - allow map[string]bool - deny map[string]bool + name string + allow map[string]bool + deny map[string]bool + lockedFlags map[string]string } func parseRaw(raw string) (*rawProfile, error) { @@ -52,9 +57,10 @@ func parseRaw(raw string) (*rawProfile, error) { } profile := &rawProfile{ - name: "unnamed", - allow: map[string]bool{}, - deny: map[string]bool{}, + name: "unnamed", + allow: map[string]bool{}, + deny: map[string]bool{}, + lockedFlags: map[string]string{}, } if rawName, present := root["name"]; present { @@ -70,10 +76,13 @@ func parseRaw(raw string) (*rawProfile, error) { if err := addList(profile.deny, root["deny"]); err != nil { return nil, fmt.Errorf("deny: %w", err) } + if err := addLockedFlags(profile.lockedFlags, root["locked-flags"]); err != nil { + return nil, fmt.Errorf("locked-flags: %w", err) + } for key, value := range root { switch key { - case "name", "description", "allow", "deny": + case "name", "description", "allow", "deny", "locked-flags": continue } prefix := []string{key} @@ -116,6 +125,30 @@ func addList(out map[string]bool, value any) error { return nil } +// addLockedFlags reads the locked-flags mapping of flag name to locked value. Values +// are rendered as the literal a flag parses, matching how Kong reads an envar. +func addLockedFlags(out map[string]string, value any) error { + if value == nil { + return nil + } + entries, ok := value.(map[string]any) + if !ok { + return fmt.Errorf("expected mapping of flag name to value") + } + for name, raw := range entries { + flag := strings.TrimSpace(strings.ToLower(name)) + if flag == "" { + return fmt.Errorf("empty flag name") + } + value, ok := raw.(bool) + if !ok { + return fmt.Errorf("%s: expected boolean value, got %T", flag, raw) + } + out[flag] = fmt.Sprintf("%t", value) + } + return nil +} + func flatten(profile *rawProfile, prefix []string, value any) error { switch typed := value.(type) { case bool: diff --git a/internal/safetyprofile/profile.go b/internal/safetyprofile/profile.go index f8b770290..f314c912c 100644 --- a/internal/safetyprofile/profile.go +++ b/internal/safetyprofile/profile.go @@ -14,4 +14,14 @@ type Profile struct { AllowAll bool AllowRules []string DenyRules []string + // LockedFlags fix a flag to a value the command line cannot change. Sorted by + // name so the generated switch is stable across builds. + LockedFlags []LockedFlag +} + +// LockedFlag is one locked boolean flag. Value is "true" or "false" in the form +// the flag parser consumes. +type LockedFlag struct { + Name string + Value string }