diff --git a/docs/safety-profiles.md b/docs/safety-profiles.md index 9e5a07843..128dca513 100644 --- a/docs/safety-profiles.md +++ b/docs/safety-profiles.md @@ -184,31 +184,19 @@ 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. +- setting that flag to a different value is an error, not an override. +- a lock matches the canonical flag name, so aliases are covered and it takes effect + wherever the selected command has a flag of that name. - 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. +A lock the binary cannot enforce is refused at build time rather than silently +ignored. -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: +Because a locked value reaches the command without appearing on the command line, a +usage error that names a locked flag carries a note saying where the value came from: ```text --sanitize-content cannot be used with --format raw diff --git a/internal/cmd/root.go b/internal/cmd/root.go index 12e2404bd..203b3967d 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -456,10 +456,8 @@ func reportEarlyError(w io.Writer, err error) error { 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. +// errorMessage formats err for display and appends a special locked-flag note to +// errors that mention another locked flag, as when those flags are mutually exclusive. func errorMessage(err error) string { msg := strings.TrimSpace(errfmt.Format(err)) if msg == "" { @@ -468,7 +466,11 @@ func errorMessage(err error) string { if ExitCode(err) != 2 { return msg } - note := lockedFlagsNote() + var refusal lockedFlagRefusal + if errors.As(err, &refusal) { + return msg + } + note := lockedFlagsNote(msg) if note == "" { return msg } diff --git a/internal/cmd/safety_profile.go b/internal/cmd/safety_profile.go index 9ef1d405a..230bf4754 100644 --- a/internal/cmd/safety_profile.go +++ b/internal/cmd/safety_profile.go @@ -46,24 +46,26 @@ 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. +// lockedFlagNames records the locked flags in force for this parse, so a command +// rejecting a value it never received on the command line can say where it 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 "" - } +// lockedFlagsNote names the locked flags for display beneath a usage error that +// mentions one of them: a command can reject a combination involving a value that +// never appeared on the command line, and the note is what explains where it came from. +func lockedFlagsNote(msg string) string { names := make([]string, 0, len(lockedFlagNames)) + mentioned := false for name := range lockedFlagNames { names = append(names, "--"+name) + mentioned = mentioned || strings.Contains(msg, "--"+name) + } + if !mentioned { + return "" } sort.Strings(names) return fmt.Sprintf("note: %s locked by baked safety profile %q", strings.Join(names, ", "), bakedSafetyProfileName()) @@ -110,10 +112,6 @@ func verifyLockedFlagsExist(root *kong.Node) error { // 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. @@ -128,9 +126,16 @@ func lockUnsupported(flag *kong.Flag) error { return nil } +// lockedFlagRefusal marks a rejection that already names the locked flag, so the +// display layer knows not to add the note explaining where a locked value came from. +type lockedFlagRefusal struct{ error } + +func (e lockedFlagRefusal) Unwrap() error { return e.error } + // 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. +// line that sets one of them to a different value. 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. @@ -144,17 +149,21 @@ func enforceLockedFlags(kctx *kong.Context) error { if !locked { continue } - if flagOnCommandLine(kctx, flag.Name) { - return usagef("flag --%s is locked by baked safety profile %q", flag.Name, bakedSafetyProfileName()) - } + // Recorded before anything here can fail, so the note names the profile's locks + // instead of however many the loop applied before it stopped. + lockedFlagNames[flag.Name] = true // 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) } + // Requesting the value the profile already locks is not an override: refusing it + // would break every caller that asks for the safe setting while protecting nothing. + if flagOnCommandLine(kctx, flag.Name) && !reflect.DeepEqual(flag.Target.Interface(), lockedTarget.Interface()) { + return lockedFlagRefusal{usagef("flag --%s is locked to %s by baked safety profile %q", flag.Name, value, bakedSafetyProfileName())} + } flag.Apply(lockedTarget) - lockedFlagNames[flag.Name] = true lockedPaths = append(lockedPaths, &kong.Path{Flag: flag}) } if len(lockedPaths) == 0 { diff --git a/internal/cmd/safety_profile_locked_flags_test.go b/internal/cmd/safety_profile_locked_flags_test.go index 579aed250..d0033a347 100644 --- a/internal/cmd/safety_profile_locked_flags_test.go +++ b/internal/cmd/safety_profile_locked_flags_test.go @@ -14,16 +14,15 @@ gmail: 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) { +// A lock must hold whatever spelling asks for a different value, aliases included. +func TestLockedFlag_RejectsEveryFormOfOverridingIt(t *testing.T) { for _, arg := range []string{ "--sanitize-content=false", - "--sanitize-content=true", - "--sanitize-content", + "--sanitize-content=0", + "--sanitize-content=FALSE", + "--sanitize-content=no", "--sanitize=false", - "--safe", + "--safe=false", } { t.Run(arg, func(t *testing.T) { withBakedSafetyProfile(t, lockedFlagsProfile) @@ -41,6 +40,55 @@ func TestLockedFlag_RejectsEveryFormOfSettingIt(t *testing.T) { } } +// Every spelling of the locked value counts as matching, because the comparison is +// between parsed booleans and not between the strings the caller and profile wrote. +func TestLockedFlag_AcceptsTheLockedValue(t *testing.T) { + for _, arg := range []string{ + "--sanitize-content", + "--sanitize-content=true", + "--sanitize-content=1", + "--sanitize-content=TRUE", + "--sanitize-content=yes", + "--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 && strings.Contains(result.err.Error(), "is locked") { + t.Fatalf("%s asks for the locked value and must not be refused: %v", arg, result.err) + } + }) + } +} + +func TestLockedFlag_OverrideRefusalOmitsTheNote(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("override must fail, got stdout=%q", result.stdout) + } + if strings.Contains(result.stderr, "note:") { + t.Fatalf("refusal must not append the locked-flag note: %q", result.stderr) + } +} + +func TestLockedFlag_UnrelatedErrorCarriesNoNote(t *testing.T) { + withBakedSafetyProfile(t, lockedFlagsProfile) + result := executeWithTestRuntime(t, []string{"--json", "gmail", "get", "m1"}, nil) + if result.err == nil { + t.Fatalf("missing account must fail, got stdout=%q", result.stdout) + } + if strings.Contains(result.stderr, "note:") { + t.Fatalf("unrelated error must not carry the locked-flag note: %q", result.stderr) + } +} + // 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) { @@ -299,7 +347,7 @@ version: true if result.err == nil { t.Fatalf("locking --home must fail, got stdout=%q", result.stdout) } - if !strings.Contains(result.err.Error(), "locks --home") { + if !strings.Contains(result.err.Error(), "only boolean flags can be locked") { t.Fatalf("err = %v", result.err) } }