diff --git a/internal/cmd/flags_difc.go b/internal/cmd/flags_difc.go index 4bd00cb8..80d7c0ff 100644 --- a/internal/cmd/flags_difc.go +++ b/internal/cmd/flags_difc.go @@ -42,15 +42,27 @@ func registerGuardsModeCompletion(cmd *cobra.Command) { } } +func registerAllowOnlyScopeFlags(cmd *cobra.Command, public *bool, owner, repo, minIntegrity *string) { + cmd.Flags().BoolVar(public, "allowonly-scope-public", envutil.GetEnvBool(config.EnvAllowOnlyScopePublic, false), "Use public AllowOnly scope") + cmd.Flags().StringVar(owner, "allowonly-scope-owner", envutil.GetEnvString(config.EnvAllowOnlyScopeOwner, ""), "AllowOnly owner scope value") + cmd.Flags().StringVar(repo, "allowonly-scope-repo", envutil.GetEnvString(config.EnvAllowOnlyScopeRepo, ""), "AllowOnly repo name (requires owner)") + cmd.Flags().StringVar(minIntegrity, "allowonly-min-integrity", envutil.GetEnvString(config.EnvAllowOnlyMinIntegrity, ""), "AllowOnly integrity: none|unapproved|approved|merged") + + // Cobra validates flags that were explicitly changed, rather than their + // resolved values. When either value has an environment-derived default, + // leave value-based validation to BuildAllowOnlyPolicy so CLI flags can + // clear that default while selecting the other scope. + if !envutil.GetEnvBool(config.EnvAllowOnlyScopePublic, false) && envutil.GetEnvString(config.EnvAllowOnlyScopeOwner, "") == "" { + cmd.MarkFlagsMutuallyExclusive("allowonly-scope-public", "allowonly-scope-owner") + } +} + func init() { RegisterFlag(func(cmd *cobra.Command) { registerGuardsModeFlag(cmd, &difcMode) cmd.Flags().StringVar(&difcSinkServerIDs, "guards-sink-server-ids", envutil.GetEnvString("MCP_GATEWAY_GUARDS_SINK_SERVER_IDS", ""), "Comma-separated server IDs whose RPC JSONL logs should include agent secrecy/integrity tag snapshots") cmd.Flags().StringVar(&guardPolicyJSON, "guard-policy-json", envutil.GetEnvString(config.EnvGuardPolicyJSON, ""), "Guard policy JSON (e.g. {\"allow-only\":{\"repos\":\"public\",\"min-integrity\":\"none\"}})") - cmd.Flags().BoolVar(&allowOnlyPublic, "allowonly-scope-public", envutil.GetEnvBool(config.EnvAllowOnlyScopePublic, false), "Use public AllowOnly scope") - cmd.Flags().StringVar(&allowOnlyOwner, "allowonly-scope-owner", envutil.GetEnvString(config.EnvAllowOnlyScopeOwner, ""), "AllowOnly owner scope value") - cmd.Flags().StringVar(&allowOnlyRepo, "allowonly-scope-repo", envutil.GetEnvString(config.EnvAllowOnlyScopeRepo, ""), "AllowOnly repo name (requires owner)") - cmd.Flags().StringVar(&allowOnlyMinInt, "allowonly-min-integrity", envutil.GetEnvString(config.EnvAllowOnlyMinIntegrity, ""), "AllowOnly integrity: none|unapproved|approved|merged") + registerAllowOnlyScopeFlags(cmd, &allowOnlyPublic, &allowOnlyOwner, &allowOnlyRepo, &allowOnlyMinInt) }) } diff --git a/internal/cmd/flags_difc_test.go b/internal/cmd/flags_difc_test.go index 7086ca7e..ddd8e704 100644 --- a/internal/cmd/flags_difc_test.go +++ b/internal/cmd/flags_difc_test.go @@ -1,6 +1,7 @@ package cmd import ( + "os" "testing" "github.com/github/gh-aw-mcpg/internal/config" @@ -198,6 +199,110 @@ func TestParseDIFCSinkServerIDs(t *testing.T) { } } +func setupAllowOnlyScopeCmd(t *testing.T) (*cobra.Command, *bool, *string) { + t.Helper() + cmd := &cobra.Command{ + Use: "test", + SilenceUsage: true, + RunE: func(cmd *cobra.Command, args []string) error { + return nil + }, + } + public := new(bool) + owner := new(string) + registerAllowOnlyScopeFlags(cmd, public, owner, new(string), new(string)) + return cmd, public, owner +} + +func TestAllowOnlyScopeFlagsMutuallyExclusive(t *testing.T) { + clearEnv := func(t *testing.T) { + t.Helper() + for _, name := range []string{config.EnvAllowOnlyScopePublic, config.EnvAllowOnlyScopeOwner} { + value, wasSet := os.LookupEnv(name) + require.NoError(t, os.Unsetenv(name)) + t.Cleanup(func() { + if wasSet { + require.NoError(t, os.Setenv(name, value)) + return + } + require.NoError(t, os.Unsetenv(name)) + }) + } + } + + t.Run("public and owner together are rejected by cobra", func(t *testing.T) { + clearEnv(t) + cmd, _, _ := setupAllowOnlyScopeCmd(t) + cmd.SetArgs([]string{"--allowonly-scope-public", "--allowonly-scope-owner", "octocat"}) + err := cmd.Execute() + require.Error(t, err, "should fail when both --allowonly-scope-public and --allowonly-scope-owner are provided") + assert.Contains(t, err.Error(), "allowonly-scope-public", "error should mention allowonly-scope-public") + assert.Contains(t, err.Error(), "allowonly-scope-owner", "error should mention allowonly-scope-owner") + }) + + t.Run("empty owner environment value does not disable cobra validation", func(t *testing.T) { + clearEnv(t) + t.Setenv(config.EnvAllowOnlyScopeOwner, "") + cmd, _, _ := setupAllowOnlyScopeCmd(t) + cmd.SetArgs([]string{"--allowonly-scope-public", "--allowonly-scope-owner", "octocat"}) + + require.Error(t, cmd.Execute()) + }) + + t.Run("public alone is accepted", func(t *testing.T) { + clearEnv(t) + cmd, _, _ := setupAllowOnlyScopeCmd(t) + cmd.SetArgs([]string{"--allowonly-scope-public"}) + err := cmd.Execute() + require.NoError(t, err, "should succeed when only --allowonly-scope-public is provided") + }) + + t.Run("owner alone is accepted", func(t *testing.T) { + clearEnv(t) + cmd, _, _ := setupAllowOnlyScopeCmd(t) + cmd.SetArgs([]string{"--allowonly-scope-owner", "octocat"}) + err := cmd.Execute() + require.NoError(t, err, "should succeed when only --allowonly-scope-owner is provided") + }) + + t.Run("neither flag is accepted", func(t *testing.T) { + clearEnv(t) + cmd, _, _ := setupAllowOnlyScopeCmd(t) + cmd.SetArgs([]string{}) + err := cmd.Execute() + require.NoError(t, err, "should succeed when neither AllowOnly scope flag is provided") + }) + + t.Run("public environment default can be cleared for owner scope", func(t *testing.T) { + clearEnv(t) + t.Setenv(config.EnvAllowOnlyScopePublic, "true") + cmd, public, owner := setupAllowOnlyScopeCmd(t) + cmd.SetArgs([]string{"--allowonly-scope-public=false", "--allowonly-scope-owner", "octocat"}) + + require.NoError(t, cmd.Execute()) + assert.False(t, *public) + assert.Equal(t, "octocat", *owner) + }) + + t.Run("owner environment default can be cleared for public scope", func(t *testing.T) { + clearEnv(t) + t.Setenv(config.EnvAllowOnlyScopeOwner, "octocat") + cmd, public, owner := setupAllowOnlyScopeCmd(t) + cmd.SetArgs([]string{"--allowonly-scope-owner=", "--allowonly-scope-public"}) + + require.NoError(t, cmd.Execute()) + assert.True(t, *public) + assert.Empty(t, *owner) + }) +} + +// TestAllowOnlyScopeFlagsRegistered verifies that the AllowOnly scope flags +// exist on the root command. +func TestAllowOnlyScopeFlagsRegistered(t *testing.T) { + assert.NotNil(t, rootCmd.Flags().Lookup("allowonly-scope-public"), "allowonly-scope-public flag should be registered on rootCmd") + assert.NotNil(t, rootCmd.Flags().Lookup("allowonly-scope-owner"), "allowonly-scope-owner flag should be registered on rootCmd") +} + func TestBuildAllowOnlyPolicy(t *testing.T) { t.Run("public scope valid", func(t *testing.T) { policy, err := config.BuildAllowOnlyPolicy(true, "", "", "none")