diff --git a/internal/secrets/client.go b/internal/secrets/client.go index 6557b53..3d2365b 100644 --- a/internal/secrets/client.go +++ b/internal/secrets/client.go @@ -9,6 +9,7 @@ import ( "fmt" "net/http" "net/url" + "strings" "github.com/tenseleyFlow/shithub-cli/internal/api" ) @@ -225,22 +226,27 @@ func (c *Client) DeleteOrgVariable(ctx context.Context, org, name string) error return c.api.REST(ctx, http.MethodDelete, path, nil, nil) } -// validateName rejects names containing path separators / wildcards so -// a malformed name can't escape the secret namespace via traversal. -// Match GitHub's spec: alphanumeric + underscore, can't start with a -// digit, can't start with GITHUB_. +// validateName enforces GitHub Actions secret-name rules. Match gh / +// GitHub: ^[A-Z_][A-Z0-9_]*$, can't start with GITHUB_, no +// lowercase (C-audit C25). The pre-D3d implementation accepted +// lowercase silently which then mismatched gh-compat scripts. func validateName(name string) error { if name == "" { return errors.New("secrets: name is required") } - for _, r := range name { + if strings.HasPrefix(name, "GITHUB_") { + return fmt.Errorf("secrets: name %q is reserved (cannot start with GITHUB_)", name) + } + for i, r := range name { switch { case r >= 'A' && r <= 'Z': - case r >= 'a' && r <= 'z': - case r >= '0' && r <= '9': case r == '_': + case r >= '0' && r <= '9': + if i == 0 { + return fmt.Errorf("secrets: name %q cannot start with a digit", name) + } default: - return fmt.Errorf("secrets: invalid character %q in name %q (allowed: A-Z, a-z, 0-9, _)", r, name) + return fmt.Errorf("secrets: invalid character %q in name %q (allowed: A-Z, 0-9, _)", r, name) } } return nil diff --git a/internal/secrets/client_test.go b/internal/secrets/client_test.go new file mode 100644 index 0000000..d5ed051 --- /dev/null +++ b/internal/secrets/client_test.go @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package secrets + +import ( + "strings" + "testing" +) + +// TestValidateNameRejectsLowercase is the C-audit C25 regression: +// gh-compat secret-name rules forbid lowercase. The pre-D3d code +// accepted them silently. +func TestValidateNameRejectsLowercase(t *testing.T) { + for _, bad := range []string{"lowercase", "Mixed_Case", "mostlyUpper"} { + err := validateName(bad) + if err == nil { + t.Errorf("validateName(%q) should reject lowercase", bad) + } + } +} + +// TestValidateNameRejectsLeadingDigit catches the "name can't start +// with a digit" rule documented in gh. +func TestValidateNameRejectsLeadingDigit(t *testing.T) { + for _, bad := range []string{"1FOO", "9", "0_X"} { + err := validateName(bad) + if err == nil { + t.Errorf("validateName(%q) should reject leading digit", bad) + } + } +} + +// TestValidateNameRejectsGithubPrefix: GITHUB_* names are reserved. +func TestValidateNameRejectsGithubPrefix(t *testing.T) { + err := validateName("GITHUB_TOKEN") + if err == nil { + t.Fatal("validateName should reject GITHUB_ prefix") + } + if !strings.Contains(err.Error(), "reserved") { + t.Errorf("error should mention reserved; got: %v", err) + } +} + +// TestValidateNameAccepts: confirm the legal shapes still pass. +func TestValidateNameAccepts(t *testing.T) { + for _, good := range []string{"FOO", "A", "_PRIVATE", "DEPLOY_KEY_42", "X1"} { + if err := validateName(good); err != nil { + t.Errorf("validateName(%q) should accept; got: %v", good, err) + } + } +} diff --git a/pkg/cmd/alias/set.go b/pkg/cmd/alias/set.go index 158141c..10bc97d 100644 --- a/pkg/cmd/alias/set.go +++ b/pkg/cmd/alias/set.go @@ -63,6 +63,11 @@ func setRun(_ context.Context, opts *setOptions) error { if cfg.Aliases == nil { cfg.Aliases = map[string]string{} } + // C26: warn on overwrite so users notice when they typo over an + // existing alias. gh prints `! Changing alias X from Y to Z`. + if prev, ok := cfg.Aliases[opts.Name]; ok && prev != expansion { + fmt.Fprintf(opts.IO.ErrOut, "! Changing alias %s from %s to %s\n", opts.Name, prev, expansion) + } cfg.Aliases[opts.Name] = expansion if err := cfg.Save(); err != nil { return fmt.Errorf("alias: save: %w", err) diff --git a/pkg/cmd/cache/cache.go b/pkg/cmd/cache/cache.go index 2185777..8b35eb7 100644 --- a/pkg/cmd/cache/cache.go +++ b/pkg/cmd/cache/cache.go @@ -27,6 +27,20 @@ func NewCmd(f *cmdutil.Factory) *cobra.Command { cmd := &cobra.Command{ Use: "cache ", Short: "Manage GitHub-Actions-compatible cache entries", + // C-audit C18: without an explicit Args/RunE on the parent, + // `shithub cache totally-not-a-real-subcmd` printed the + // parent's help and exited 0. CI scripts running an unknown + // subcommand against an older binary silently no-op'd. Reject + // any positional that didn't match a registered subcommand. + Args: func(c *cobra.Command, args []string) error { + if len(args) > 0 { + return fmt.Errorf("unknown subcommand %q for %q", args[0], c.CommandPath()) + } + return nil + }, + RunE: func(c *cobra.Command, _ []string) error { + return c.Help() + }, } cmd.AddCommand(cachelist.NewCmd(f)) cmd.AddCommand(newDeferredStub("delete", "Delete a cache entry (deferred until shithub-S41g)")) diff --git a/pkg/cmd/completion/completion.go b/pkg/cmd/completion/completion.go index 94402bc..7abefdb 100644 --- a/pkg/cmd/completion/completion.go +++ b/pkg/cmd/completion/completion.go @@ -70,7 +70,11 @@ The shell may be passed positionally (gh-style) or via -s/--shell. // Run emits the completion script for opts.Shell to opts.IO.Out. func Run(_ context.Context, root *cobra.Command, opts *options) error { if opts.Shell == "" { - return errors.New("completion: --shell is required (one of: " + listShells() + ")") + // C-audit C22: message text used to say "--shell is required", + // which conflicts with the help shape (`completion `). + // Match the help: positional is preferred, --shell is a script + // alias that lands at the same destination. + return errors.New("completion: shell is required (one of: " + listShells() + ")") } return generate(root, opts.IO.Out, opts.Shell, opts.NoDesc) } diff --git a/pkg/cmd/repo/list/list.go b/pkg/cmd/repo/list/list.go index cad9acb..807d5e0 100644 --- a/pkg/cmd/repo/list/list.go +++ b/pkg/cmd/repo/list/list.go @@ -47,6 +47,10 @@ type options struct { Sort string Direction string Limit int + Web bool + + // Opener is overridden in tests. + Opener func(url string) error Exporter output.Options } @@ -58,6 +62,7 @@ func NewCmd(f *cmdutil.Factory) *cobra.Command { HTTPClient: f.HTTPClient, DefaultHost: f.DefaultHost, Limit: DefaultLimit, + Opener: defaultOpener, } cmd := &cobra.Command{ Use: "list []", @@ -81,7 +86,10 @@ func NewCmd(f *cmdutil.Factory) *cobra.Command { cmd.Flags().StringVar(&opts.Sort, "sort", "updated", "sort field: {created|updated|pushed|name}") cmd.Flags().StringVar(&opts.Direction, "order", "desc", "sort order: {asc|desc}") cmd.Flags().IntVarP(&opts.Limit, "limit", "L", DefaultLimit, "maximum number of repositories to list (max 1000)") + cmd.Flags().BoolVarP(&opts.Web, "web", "w", false, "open the repository list in a browser") output.AddFlags(cmd, &opts.Exporter) + // TODO(D3a-followup): add output.MarkWebMutuallyExclusive(cmd) + // once D3a (output-web-mutex helper) lands on trunk. return cmd } @@ -104,6 +112,33 @@ func Run(ctx context.Context, opts *options) error { if host == "" && opts.DefaultHost != nil { host = opts.DefaultHost() } + + // C23: --web sends the user to the host's repo-list page for the + // supplied (or authenticated) user/org. gh has this flag; we + // were the odd ones out. + if opts.Web { + who := opts.Owner + if who == "" { + // Best-effort: defer to the host's "your repos" page; + // the server's UI handles the auth redirect. + who = "?tab=repositories" + } + base := "https://" + if host != "" { + base += host + } + var url string + if strings.HasPrefix(who, "?") { + url = base + "/" + who + } else { + url = base + "/" + who + "?tab=repositories" + } + fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", url) + if opts.Opener == nil { + return nil + } + return opts.Opener(url) + } client, err := opts.HTTPClient(host) if err != nil { return err @@ -248,3 +283,7 @@ func truncate(s string, n int) string { } return string(runes[:n-1]) + "…" } + +// defaultOpener is the prod-time no-op; iostreams will plumb a real +// browser launcher in a later sprint. Tests override. +var defaultOpener = func(_ string) error { return nil }