diff --git a/README.md b/README.md index 064008e3b..60ab7d6d9 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ For more information, see [Linux & BSD installation](./docs/install_linux.md). | ------------------- | --------------------| | `winget install --id GitHub.cli` | `winget upgrade --id GitHub.cli` | -> **Note** +> [!NOTE] > The Windows installer modifies your PATH. When using Windows Terminal, you will need to **open a new window** for the changes to take effect. (Simply opening a new tab will _not_ be sufficient.) #### scoop @@ -125,6 +125,38 @@ GitHub CLI comes pre-installed in all [GitHub-Hosted Runners](https://docs.githu Download packaged binaries from the [releases page][]. +#### Verification of binaries + +Since version 2.50.0 `gh` has been producing [Build Provenance Attestation](https://github.blog/changelog/2024-06-25-artifact-attestations-is-generally-available/) enabling a cryptographically verifiable paper-trail back to the origin GitHub repository, git revision and build instructions used. The build provenance attestations are signed and relies on Public Good [Sigstore](https://www.sigstore.dev/) for PKI. + +There are two common ways to verify a downloaded release, depending if `gh` is aready installed or not. If `gh` is installed, it's trivial to verify a new release: + +- **Option 1: Using `gh` if already installed:** + + ```shell + $ % gh at verify -R cli/cli gh_2.62.0_macOS_arm64.zip + Loaded digest sha256:fdb77f31b8a6dd23c3fd858758d692a45f7fc76383e37d475bdcae038df92afc for file://gh_2.62.0_macOS_arm64.zip + Loaded 1 attestation from GitHub API + ✓ Verification succeeded! + + sha256:fdb77f31b8a6dd23c3fd858758d692a45f7fc76383e37d475bdcae038df92afc was attested by: + REPO PREDICATE_TYPE WORKFLOW + cli/cli https://slsa.dev/provenance/v1 .github/workflows/deployment.yml@refs/heads/trunk + ``` + +- **Option 2: Using Sigstore [`cosign`](https://github.com/sigstore/cosign):** + + To perform this, download the [attestation](https://github.com/cli/cli/attestations) for the downloaded release and use cosign to verify the authenticity of the downloaded release: + + ```shell + $ cosign verify-blob-attestation --bundle cli-cli-attestation-3120304.sigstore.json \ + --new-bundle-format \ + --certificate-oidc-issuer="https://token.actions.githubusercontent.com" \ + --certificate-identity-regexp="^https://github.com/cli/cli/.github/workflows/deployment.yml@refs/heads/trunk$" \ + gh_2.62.0_macOS_arm64.zip + Verified OK + ``` + ### Build from source See here on how to [build GitHub CLI from source][build from source]. diff --git a/api/queries_pr.go b/api/queries_pr.go index 370f2db24..aa493b5e9 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -33,6 +33,7 @@ type PullRequest struct { Closed bool URL string BaseRefName string + BaseRefOid string HeadRefName string HeadRefOid string Body string diff --git a/api/query_builder.go b/api/query_builder.go index dbf889273..2112367e3 100644 --- a/api/query_builder.go +++ b/api/query_builder.go @@ -285,6 +285,7 @@ var PullRequestFields = append(sharedIssuePRFields, "additions", "autoMergeRequest", "baseRefName", + "baseRefOid", "changedFiles", "commits", "deletions", diff --git a/git/client.go b/git/client.go index 560eccfdd..b2cfbce45 100644 --- a/git/client.go +++ b/git/client.go @@ -16,6 +16,7 @@ import ( "strings" "sync" + "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/safeexec" ) @@ -94,16 +95,65 @@ func (c *Client) Command(ctx context.Context, args ...string) (*Command, error) return &Command{cmd}, nil } +// CredentialPattern is used to inform AuthenticatedCommand which patterns Git should match +// against when trying to find credentials. It is a little over-engineered as a type because we +// want AuthenticatedCommand to have a clear compilation error when this is not provided, +// as opposed to using a string which might compile with `client.AuthenticatedCommand(ctx, "fetch")`. +// +// It is only usable when constructed by another function in the package because the empty pattern, +// without allMatching set to true, will result in an error in AuthenticatedCommand. +// +// Callers can currently opt-in to an slightly less secure mode for backwards compatibility by using +// AllMatchingCredentialsPattern. +type CredentialPattern struct { + allMatching bool // should only be constructable via AllMatchingCredentialsPattern + pattern string +} + +// AllMatchingCredentialsPattern allows for setting gh as credential helper for all hosts. +// However, we should endeavour to remove it as it's less secure. +var AllMatchingCredentialsPattern = CredentialPattern{allMatching: true, pattern: ""} +var disallowedCredentialPattern = CredentialPattern{allMatching: false, pattern: ""} + +// WM-TODO: Are there any funny remotes that might not resolve to a URL? +func CredentialPatternFromRemote(ctx context.Context, c *Client, remote string) (CredentialPattern, error) { + gitURL, err := c.GetRemoteURL(ctx, remote) + if err != nil { + return CredentialPattern{}, err + } + return CredentialPatternFromGitURL(gitURL) +} + +func CredentialPatternFromGitURL(gitURL string) (CredentialPattern, error) { + normalizedURL, err := ParseURL(gitURL) + if err != nil { + return CredentialPattern{}, fmt.Errorf("failed to parse remote URL: %w", err) + } + return CredentialPattern{ + pattern: strings.TrimSuffix(ghinstance.HostPrefix(normalizedURL.Host), "/"), + }, nil +} + // AuthenticatedCommand is a wrapper around Command that included configuration to use gh // as the credential helper for git. -func (c *Client) AuthenticatedCommand(ctx context.Context, args ...string) (*Command, error) { - preArgs := []string{"-c", "credential.helper="} +func (c *Client) AuthenticatedCommand(ctx context.Context, credentialPattern CredentialPattern, args ...string) (*Command, error) { if c.GhPath == "" { // Assumes that gh is in PATH. c.GhPath = "gh" } credHelper := fmt.Sprintf("!%q auth git-credential", c.GhPath) - preArgs = append(preArgs, "-c", fmt.Sprintf("credential.helper=%s", credHelper)) + + var preArgs []string + if credentialPattern == disallowedCredentialPattern { + return nil, fmt.Errorf("empty credential pattern is not allowed unless provided explicitly") + } else if credentialPattern == AllMatchingCredentialsPattern { + preArgs = []string{"-c", "credential.helper="} + preArgs = append(preArgs, "-c", fmt.Sprintf("credential.helper=%s", credHelper)) + } else { + preArgs = []string{"-c", fmt.Sprintf("credential.%s.helper=", credentialPattern.pattern)} + preArgs = append(preArgs, "-c", fmt.Sprintf("credential.%s.helper=%s", credentialPattern.pattern, credHelper)) + } + args = append(preArgs, args...) return c.Command(ctx, args...) } @@ -152,6 +202,19 @@ func (c *Client) UpdateRemoteURL(ctx context.Context, name, url string) error { return nil } +func (c *Client) GetRemoteURL(ctx context.Context, name string) (string, error) { + args := []string{"remote", "get-url", name} + cmd, err := c.Command(ctx, args...) + if err != nil { + return "", err + } + out, err := cmd.Output() + if err != nil { + return "", err + } + return strings.TrimSpace(string(out)), nil +} + func (c *Client) SetRemoteResolution(ctx context.Context, name, resolution string) error { args := []string{"config", "--add", fmt.Sprintf("remote.%s.gh-resolved", name), resolution} cmd, err := c.Command(ctx, args...) @@ -549,7 +612,7 @@ func (c *Client) Fetch(ctx context.Context, remote string, refspec string, mods if refspec != "" { args = append(args, refspec) } - cmd, err := c.AuthenticatedCommand(ctx, args...) + cmd, err := c.AuthenticatedCommand(ctx, AllMatchingCredentialsPattern, args...) if err != nil { return err } @@ -564,7 +627,7 @@ func (c *Client) Pull(ctx context.Context, remote, branch string, mods ...Comman if remote != "" && branch != "" { args = append(args, remote, branch) } - cmd, err := c.AuthenticatedCommand(ctx, args...) + cmd, err := c.AuthenticatedCommand(ctx, AllMatchingCredentialsPattern, args...) if err != nil { return err } @@ -576,7 +639,7 @@ func (c *Client) Pull(ctx context.Context, remote, branch string, mods ...Comman func (c *Client) Push(ctx context.Context, remote string, ref string, mods ...CommandModifier) error { args := []string{"push", "--set-upstream", remote, ref} - cmd, err := c.AuthenticatedCommand(ctx, args...) + cmd, err := c.AuthenticatedCommand(ctx, AllMatchingCredentialsPattern, args...) if err != nil { return err } @@ -587,6 +650,13 @@ func (c *Client) Push(ctx context.Context, remote string, ref string, mods ...Co } func (c *Client) Clone(ctx context.Context, cloneURL string, args []string, mods ...CommandModifier) (string, error) { + // Note that even if this is an SSH clone URL, we are setting the pattern anyway. + // We could write some code to prevent this, but it also doesn't seem harmful. + pattern, err := CredentialPatternFromGitURL(cloneURL) + if err != nil { + return "", err + } + cloneArgs, target := parseCloneArgs(args) cloneArgs = append(cloneArgs, cloneURL) // If the args contain an explicit target, pass it to clone otherwise, @@ -601,7 +671,7 @@ func (c *Client) Clone(ctx context.Context, cloneURL string, args []string, mods } } cloneArgs = append([]string{"clone"}, cloneArgs...) - cmd, err := c.AuthenticatedCommand(ctx, cloneArgs...) + cmd, err := c.AuthenticatedCommand(ctx, pattern, cloneArgs...) if err != nil { return "", err } diff --git a/git/client_test.go b/git/client_test.go index 1e3032317..41b651d0a 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -64,17 +64,32 @@ func TestClientAuthenticatedCommand(t *testing.T) { tests := []struct { name string path string + pattern CredentialPattern wantArgs []string + wantErr error }{ { - name: "adds credential helper config options", + name: "when credential pattern allows for anything, credential helper matches everything", path: "path/to/gh", + pattern: AllMatchingCredentialsPattern, wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"path/to/gh" auth git-credential`, "fetch"}, }, + { + name: "when credential pattern is set, credential helper only matches that pattern", + path: "path/to/gh", + pattern: CredentialPattern{pattern: "https://github.com"}, + wantArgs: []string{"path/to/git", "-c", "credential.https://github.com.helper=", "-c", `credential.https://github.com.helper=!"path/to/gh" auth git-credential`, "fetch"}, + }, { name: "fallback when GhPath is not set", + pattern: AllMatchingCredentialsPattern, wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"gh" auth git-credential`, "fetch"}, }, + { + name: "errors when attempting to use an empty pattern that isn't marked all matching", + pattern: CredentialPattern{allMatching: false, pattern: ""}, + wantErr: fmt.Errorf("empty credential pattern is not allowed unless provided explicitly"), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -82,9 +97,12 @@ func TestClientAuthenticatedCommand(t *testing.T) { GhPath: tt.path, GitPath: "path/to/git", } - cmd, err := client.AuthenticatedCommand(context.Background(), "fetch") - assert.NoError(t, err) - assert.Equal(t, tt.wantArgs, cmd.Args) + cmd, err := client.AuthenticatedCommand(context.Background(), tt.pattern, "fetch") + if tt.wantErr != nil { + require.Equal(t, tt.wantErr, err) + return + } + require.Equal(t, tt.wantArgs, cmd.Args) }) } } @@ -1131,44 +1149,52 @@ func TestClientSetRemoteBranches(t *testing.T) { func TestClientFetch(t *testing.T) { tests := []struct { - name string - mods []CommandModifier - cmdExitStatus int - cmdStdout string - cmdStderr string - wantCmdArgs string - wantErrorMsg string + name string + mods []CommandModifier + commands mockedCommands + wantErrorMsg string }{ { - name: "fetch", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`, + name: "fetch", + commands: map[args]commandResult{ + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "accepts command modifiers", - mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`, + name: "accepts command modifiers", + mods: []CommandModifier{WithRepoDir("/path/to/repo")}, + commands: map[args]commandResult{ + `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "git error", - cmdExitStatus: 1, - cmdStderr: "git error message", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`, - wantErrorMsg: "failed to run git: git error message", + name: "git error on fetch", + commands: map[args]commandResult{ + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`: { + ExitStatus: 1, + Stderr: "fetch error message", + }, + }, + wantErrorMsg: "failed to run git: fetch error message", }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr) + cmdCtx := createMockedCommandContext(t, tt.commands) client := Client{ GitPath: "path/to/git", commandContext: cmdCtx, } err := client.Fetch(context.Background(), "origin", "trunk", tt.mods...) - assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) if tt.wantErrorMsg == "" { - assert.NoError(t, err) + require.NoError(t, err) } else { - assert.EqualError(t, err, tt.wantErrorMsg) + require.EqualError(t, err, tt.wantErrorMsg) } }) } @@ -1176,44 +1202,52 @@ func TestClientFetch(t *testing.T) { func TestClientPull(t *testing.T) { tests := []struct { - name string - mods []CommandModifier - cmdExitStatus int - cmdStdout string - cmdStderr string - wantCmdArgs string - wantErrorMsg string + name string + mods []CommandModifier + commands mockedCommands + wantErrorMsg string }{ { - name: "pull", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`, + name: "pull", + commands: map[args]commandResult{ + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "accepts command modifiers", - mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`, + name: "accepts command modifiers", + mods: []CommandModifier{WithRepoDir("/path/to/repo")}, + commands: map[args]commandResult{ + `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "git error", - cmdExitStatus: 1, - cmdStderr: "git error message", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`, - wantErrorMsg: "failed to run git: git error message", + name: "git error on pull", + commands: map[args]commandResult{ + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { + ExitStatus: 1, + Stderr: "pull error message", + }, + }, + wantErrorMsg: "failed to run git: pull error message", }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr) + cmdCtx := createMockedCommandContext(t, tt.commands) client := Client{ GitPath: "path/to/git", commandContext: cmdCtx, } err := client.Pull(context.Background(), "origin", "trunk", tt.mods...) - assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) if tt.wantErrorMsg == "" { - assert.NoError(t, err) + require.NoError(t, err) } else { - assert.EqualError(t, err, tt.wantErrorMsg) + require.EqualError(t, err, tt.wantErrorMsg) } }) } @@ -1221,44 +1255,52 @@ func TestClientPull(t *testing.T) { func TestClientPush(t *testing.T) { tests := []struct { - name string - mods []CommandModifier - cmdExitStatus int - cmdStdout string - cmdStderr string - wantCmdArgs string - wantErrorMsg string + name string + mods []CommandModifier + commands mockedCommands + wantErrorMsg string }{ { - name: "push", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`, + name: "push", + commands: map[args]commandResult{ + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "accepts command modifiers", - mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`, + name: "accepts command modifiers", + mods: []CommandModifier{WithRepoDir("/path/to/repo")}, + commands: map[args]commandResult{ + `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "git error", - cmdExitStatus: 1, - cmdStderr: "git error message", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`, - wantErrorMsg: "failed to run git: git error message", + name: "git error on push", + commands: map[args]commandResult{ + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { + ExitStatus: 1, + Stderr: "push error message", + }, + }, + wantErrorMsg: "failed to run git: push error message", }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr) + cmdCtx := createMockedCommandContext(t, tt.commands) client := Client{ GitPath: "path/to/git", commandContext: cmdCtx, } err := client.Push(context.Background(), "origin", "trunk", tt.mods...) - assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) if tt.wantErrorMsg == "" { - assert.NoError(t, err) + require.NoError(t, err) } else { - assert.EqualError(t, err, tt.wantErrorMsg) + require.EqualError(t, err, tt.wantErrorMsg) } }) } @@ -1279,14 +1321,14 @@ func TestClientClone(t *testing.T) { { name: "clone", args: []string{}, - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone github.com/cli/cli`, + wantCmdArgs: `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone https://github.com/cli/cli`, wantTarget: "cli", }, { name: "accepts command modifiers", args: []string{}, mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential clone github.com/cli/cli`, + wantCmdArgs: `path/to/git -C /path/to/repo -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone https://github.com/cli/cli`, wantTarget: "cli", }, { @@ -1294,19 +1336,19 @@ func TestClientClone(t *testing.T) { args: []string{}, cmdExitStatus: 1, cmdStderr: "git error message", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone github.com/cli/cli`, + wantCmdArgs: `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone https://github.com/cli/cli`, wantErrorMsg: "failed to run git: git error message", }, { name: "bare clone", args: []string{"--bare"}, - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone --bare github.com/cli/cli`, + wantCmdArgs: `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone --bare https://github.com/cli/cli`, wantTarget: "cli.git", }, { name: "bare clone with explicit target", args: []string{"cli-bare", "--bare"}, - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone --bare github.com/cli/cli cli-bare`, + wantCmdArgs: `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone --bare https://github.com/cli/cli cli-bare`, wantTarget: "cli-bare", }, } @@ -1317,7 +1359,7 @@ func TestClientClone(t *testing.T) { GitPath: "path/to/git", commandContext: cmdCtx, } - target, err := client.Clone(context.Background(), "github.com/cli/cli", tt.args, tt.mods...) + target, err := client.Clone(context.Background(), "https://github.com/cli/cli", tt.args, tt.mods...) assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) if tt.wantErrorMsg == "" { assert.NoError(t, err) @@ -1442,6 +1484,54 @@ func initRepo(t *testing.T, dir string) { assert.NoError(t, err) } +type args string + +type commandResult struct { + ExitStatus int `json:"exitStatus"` + Stdout string `json:"out"` + Stderr string `json:"err"` +} + +type mockedCommands map[args]commandResult + +// TestCommandMocking is an invoked test helper that emulates expected behavior for predefined shell commands, erroring when unexpected conditions are encountered. +func TestCommandMocking(t *testing.T) { + if os.Getenv("GH_WANT_HELPER_PROCESS_RICH") != "1" { + return + } + + jsonVar, ok := os.LookupEnv("GH_HELPER_PROCESS_RICH_COMMANDS") + if !ok { + fmt.Fprint(os.Stderr, "missing GH_HELPER_PROCESS_RICH_COMMANDS") + os.Exit(1) + } + + var commands mockedCommands + if err := json.Unmarshal([]byte(jsonVar), &commands); err != nil { + fmt.Fprint(os.Stderr, "failed to unmarshal GH_HELPER_PROCESS_RICH_COMMANDS") + os.Exit(1) + } + + // The discarded args are those for the go test binary itself, e.g. `-test.run=TestHelperProcessRich` + realArgs := os.Args[3:] + + commandResult, ok := commands[args(strings.Join(realArgs, " "))] + if !ok { + fmt.Fprintf(os.Stderr, "unexpected command: %s\n", strings.Join(realArgs, " ")) + os.Exit(1) + } + + if commandResult.Stdout != "" { + fmt.Fprint(os.Stdout, commandResult.Stdout) + } + + if commandResult.Stderr != "" { + fmt.Fprint(os.Stderr, commandResult.Stderr) + } + + os.Exit(commandResult.ExitStatus) +} + func TestHelperProcess(t *testing.T) { if os.Getenv("GH_WANT_HELPER_PROCESS") != "1" { return @@ -1465,6 +1555,87 @@ func TestHelperProcess(t *testing.T) { os.Exit(0) } +func TestCredentialPatternFromGitURL(t *testing.T) { + tests := []struct { + name string + gitURL string + wantErr bool + wantCredentialPattern CredentialPattern + }{ + { + name: "Given a well formed gitURL, it returns the corresponding CredentialPattern", + gitURL: "https://github.com/OWNER/REPO", + wantCredentialPattern: CredentialPattern{ + pattern: "https://github.com", + allMatching: false, + }, + }, + { + name: "Given a malformed gitURL, it returns an error", + // This pattern is copied from the tests in ParseURL + // Unexpectedly, a non URL-like string did not error in ParseURL + gitURL: "ssh://git@[/tmp/git-repo", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + credentialPattern, err := CredentialPatternFromGitURL(tt.gitURL) + if tt.wantErr { + assert.ErrorContains(t, err, "failed to parse remote URL") + } else { + assert.NoError(t, err) + assert.Equal(t, tt.wantCredentialPattern, credentialPattern) + } + }) + } +} + +func TestCredentialPatternFromRemote(t *testing.T) { + tests := []struct { + name string + remote string + wantCredentialPattern CredentialPattern + wantErr bool + }{ + { + name: "Given a well formed remote, it returns the corresponding CredentialPattern", + remote: "https://github.com/OWNER/REPO", + wantCredentialPattern: CredentialPattern{ + pattern: "https://github.com", + allMatching: false, + }, + }, + { + name: "Given an error from GetRemoteURL, it returns that error", + remote: "foo remote", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var cmdCtx func(ctx context.Context, name string, args ...string) *exec.Cmd + if tt.wantErr { + _, cmdCtx = createCommandContext(t, 1, tt.remote, "GetRemoteURL error") + } else { + _, cmdCtx = createCommandContext(t, 0, tt.remote, "") + } + + client := Client{ + GitPath: "path/to/git", + commandContext: cmdCtx, + } + credentialPattern, err := CredentialPatternFromRemote(context.Background(), &client, tt.remote) + if tt.wantErr { + assert.ErrorContains(t, err, "GetRemoteURL error") + } else { + assert.NoError(t, err) + assert.Equal(t, tt.wantCredentialPattern, credentialPattern) + } + }) + } +} + func createCommandContext(t *testing.T, exitStatus int, stdout, stderr string) (*exec.Cmd, commandCtx) { cmd := exec.CommandContext(context.Background(), os.Args[0], "-test.run=TestHelperProcess", "--") cmd.Env = []string{ @@ -1479,3 +1650,21 @@ func createCommandContext(t *testing.T, exitStatus int, stdout, stderr string) ( return cmd } } + +func createMockedCommandContext(t *testing.T, commands mockedCommands) commandCtx { + marshaledCommands, err := json.Marshal(commands) + require.NoError(t, err) + + // invokes helper within current test binary, emulating desired behavior + return func(ctx context.Context, exe string, args ...string) *exec.Cmd { + cmd := exec.CommandContext(context.Background(), os.Args[0], "-test.run=TestCommandMocking", "--") + cmd.Env = []string{ + "GH_WANT_HELPER_PROCESS_RICH=1", + fmt.Sprintf("GH_HELPER_PROCESS_RICH_COMMANDS=%s", string(marshaledCommands)), + } + + cmd.Args = append(cmd.Args, exe) + cmd.Args = append(cmd.Args, args...) + return cmd + } +} diff --git a/go.mod b/go.mod index 40193c4f8..a95ccacc0 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,7 @@ require ( github.com/cenkalti/backoff/v4 v4.3.0 github.com/charmbracelet/glamour v0.7.0 github.com/charmbracelet/lipgloss v0.10.1-0.20240413172830-d0be07ea6b9c - github.com/cli/go-gh/v2 v2.11.0 + github.com/cli/go-gh/v2 v2.11.1 github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24 github.com/cli/oauth v1.1.1 github.com/cli/safeexec v1.0.1 diff --git a/go.sum b/go.sum index d068d9d11..a7d87de7d 100644 --- a/go.sum +++ b/go.sum @@ -95,8 +95,8 @@ github.com/charmbracelet/x/exp/term v0.0.0-20240425164147-ba2a9512b05f/go.mod h1 github.com/cli/browser v1.0.0/go.mod h1:IEWkHYbLjkhtjwwWlwTHW2lGxeS5gezEQBMLTwDHf5Q= github.com/cli/browser v1.3.0 h1:LejqCrpWr+1pRqmEPDGnTZOjsMe7sehifLynZJuqJpo= github.com/cli/browser v1.3.0/go.mod h1:HH8s+fOAxjhQoBUAsKuPCbqUuxZDhQ2/aD+SzsEfBTk= -github.com/cli/go-gh/v2 v2.11.0 h1:TERLYMMWderKBO3lBff/JIu2+eSly2oFRgN2WvO+3eA= -github.com/cli/go-gh/v2 v2.11.0/go.mod h1:MeRoKzXff3ygHu7zP+NVTT+imcHW6p3tpuxHAzRM2xE= +github.com/cli/go-gh/v2 v2.11.1 h1:amAyfqMWQTBdue8iTmDUegGZK7c8kk6WCxD9l/wLtGI= +github.com/cli/go-gh/v2 v2.11.1/go.mod h1:MeRoKzXff3ygHu7zP+NVTT+imcHW6p3tpuxHAzRM2xE= github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24 h1:QDrhR4JA2n3ij9YQN0u5ZeuvRIIvsUGmf5yPlTS0w8E= github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24/go.mod h1:rr9GNING0onuVw8MnracQHn7PcchnFlP882Y0II2KZk= github.com/cli/oauth v1.1.1 h1:459gD3hSjlKX9B1uXBuiAMdpXBUQ9QGf/NDcCpoQxPs= diff --git a/internal/run/stub.go b/internal/run/stub.go index 49bf62d29..5cd3c6de5 100644 --- a/internal/run/stub.go +++ b/internal/run/stub.go @@ -9,7 +9,7 @@ import ( ) const ( - gitAuthRE = `-c credential.helper= -c credential.helper=!"[^"]+" auth git-credential ` + gitAuthRE = `-c credential(?:\..+)?\.helper= -c credential(?:\..+)?\.helper=!"[^"]+" auth git-credential ` ) type T interface { diff --git a/pkg/cmd/attestation/download/download.go b/pkg/cmd/attestation/download/download.go index 77c928093..143912308 100644 --- a/pkg/cmd/attestation/download/download.go +++ b/pkg/cmd/attestation/download/download.go @@ -122,14 +122,13 @@ func runDownload(opts *Options) error { opts.Logger.VerbosePrintf("Downloading trusted metadata for artifact %s\n\n", opts.ArtifactPath) - c := verification.FetchAttestationsConfig{ - APIClient: opts.APIClient, - Digest: artifact.DigestWithAlg(), - Limit: opts.Limit, - Owner: opts.Owner, - Repo: opts.Repo, + params := verification.FetchRemoteAttestationsParams{ + Digest: artifact.DigestWithAlg(), + Limit: opts.Limit, + Owner: opts.Owner, + Repo: opts.Repo, } - attestations, err := verification.GetRemoteAttestations(c) + attestations, err := verification.GetRemoteAttestations(opts.APIClient, params) if err != nil { if errors.Is(err, api.ErrNoAttestations{}) { fmt.Fprintf(opts.Logger.IO.Out, "No attestations found for %s\n", opts.ArtifactPath) diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index 0ea91c2f7..07083a5c0 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -9,8 +9,8 @@ import ( "path/filepath" "github.com/cli/cli/v2/pkg/cmd/attestation/api" + "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" "github.com/cli/cli/v2/pkg/cmd/attestation/artifact/oci" - "github.com/google/go-containerregistry/pkg/name" protobundle "github.com/sigstore/protobuf-specs/gen/pb-go/bundle/v1" "github.com/sigstore/sigstore-go/pkg/bundle" ) @@ -20,32 +20,11 @@ const SLSAPredicateV1 = "https://slsa.dev/provenance/v1" var ErrUnrecognisedBundleExtension = errors.New("bundle file extension not supported, must be json or jsonl") var ErrEmptyBundleFile = errors.New("provided bundle file is empty") -type FetchAttestationsConfig struct { - APIClient api.Client - BundlePath string - Digest string - Limit int - Owner string - Repo string - OCIClient oci.Client - UseBundleFromRegistry bool - NameRef name.Reference -} - -func (c *FetchAttestationsConfig) IsBundleProvided() bool { - return c.BundlePath != "" -} - -func GetAttestations(c FetchAttestationsConfig) ([]*api.Attestation, error) { - if c.IsBundleProvided() { - return GetLocalAttestations(c.BundlePath) - } - - if c.UseBundleFromRegistry { - return GetOCIAttestations(c) - } - - return GetRemoteAttestations(c) +type FetchRemoteAttestationsParams struct { + Digest string + Limit int + Owner string + Repo string } // GetLocalAttestations returns a slice of attestations read from a local bundle file. @@ -116,30 +95,30 @@ func loadBundlesFromJSONLinesFile(path string) ([]*api.Attestation, error) { return attestations, nil } -func GetRemoteAttestations(c FetchAttestationsConfig) ([]*api.Attestation, error) { - if c.APIClient == nil { +func GetRemoteAttestations(client api.Client, params FetchRemoteAttestationsParams) ([]*api.Attestation, error) { + if client == nil { return nil, fmt.Errorf("api client must be provided") } // check if Repo is set first because if Repo has been set, Owner will be set using the value of Repo. // If Repo is not set, the field will remain empty. It will not be populated using the value of Owner. - if c.Repo != "" { - attestations, err := c.APIClient.GetByRepoAndDigest(c.Repo, c.Digest, c.Limit) + if params.Repo != "" { + attestations, err := client.GetByRepoAndDigest(params.Repo, params.Digest, params.Limit) if err != nil { - return nil, fmt.Errorf("failed to fetch attestations from %s: %w", c.Repo, err) + return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Repo, err) } return attestations, nil - } else if c.Owner != "" { - attestations, err := c.APIClient.GetByOwnerAndDigest(c.Owner, c.Digest, c.Limit) + } else if params.Owner != "" { + attestations, err := client.GetByOwnerAndDigest(params.Owner, params.Digest, params.Limit) if err != nil { - return nil, fmt.Errorf("failed to fetch attestations from %s: %w", c.Owner, err) + return nil, fmt.Errorf("failed to fetch attestations from %s: %w", params.Owner, err) } return attestations, nil } return nil, fmt.Errorf("owner or repo must be provided") } -func GetOCIAttestations(c FetchAttestationsConfig) ([]*api.Attestation, error) { - attestations, err := c.OCIClient.GetAttestations(c.NameRef, c.Digest) +func GetOCIAttestations(client oci.Client, artifact artifact.DigestedArtifact) ([]*api.Attestation, error) { + attestations, err := client.GetAttestations(artifact.NameRef(), artifact.Digest()) if err != nil { return nil, fmt.Errorf("failed to fetch OCI attestations: %w", err) } diff --git a/pkg/cmd/attestation/verification/extensions.go b/pkg/cmd/attestation/verification/extensions.go index e302d89c9..2958408d0 100644 --- a/pkg/cmd/attestation/verification/extensions.go +++ b/pkg/cmd/attestation/verification/extensions.go @@ -13,49 +13,50 @@ var ( GitHubTenantOIDCIssuer = "https://token.actions.%s.ghe.com" ) -func VerifyCertExtensions(results []*AttestationProcessingResult, ec EnforcementCriteria) error { +// VerifyCertExtensions allows us to perform case insensitive comparisons of certificate extensions +func VerifyCertExtensions(results []*AttestationProcessingResult, ec EnforcementCriteria) ([]*AttestationProcessingResult, error) { if len(results) == 0 { - return errors.New("no attestations proccessing results") + return nil, errors.New("no attestations processing results") } + verified := make([]*AttestationProcessingResult, 0, len(results)) var lastErr error for _, attestation := range results { - err := verifyCertExtensions(*attestation.VerificationResult.Signature.Certificate, ec) - if err == nil { - // if at least one attestation is verified, we're good as verification - // is defined as successful if at least one attestation is verified - return nil + if err := verifyCertExtensions(*attestation.VerificationResult.Signature.Certificate, ec.Certificate); err != nil { + lastErr = err + // move onto the next attestation in the for loop if verification fails + continue } - lastErr = err + // otherwise, add the result to the results slice and increment verifyCount + verified = append(verified, attestation) } - // if we have exited the for loop without returning early due to successful - // verification, we need to return an error - return lastErr + // if we have exited the for loop without verifying any attestations, + // return the last error found + if len(verified) == 0 { + return nil, lastErr + } + + return verified, nil } -func verifyCertExtensions(verifiedCert certificate.Summary, criteria EnforcementCriteria) error { - sourceRepositoryOwnerURI := verifiedCert.Extensions.SourceRepositoryOwnerURI - if !strings.EqualFold(criteria.Certificate.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) { - return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", criteria.Certificate.SourceRepositoryOwnerURI, sourceRepositoryOwnerURI) +func verifyCertExtensions(given, expected certificate.Summary) error { + if !strings.EqualFold(expected.SourceRepositoryOwnerURI, given.SourceRepositoryOwnerURI) { + return fmt.Errorf("expected SourceRepositoryOwnerURI to be %s, got %s", expected.SourceRepositoryOwnerURI, given.SourceRepositoryOwnerURI) } - // if repo is set, check the SourceRepositoryURI field - if criteria.Certificate.SourceRepositoryURI != "" { - sourceRepositoryURI := verifiedCert.Extensions.SourceRepositoryURI - if !strings.EqualFold(criteria.Certificate.SourceRepositoryURI, sourceRepositoryURI) { - return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", criteria.Certificate.SourceRepositoryURI, sourceRepositoryURI) - } + // if repo is set, compare the SourceRepositoryURI fields + if expected.SourceRepositoryURI != "" && !strings.EqualFold(expected.SourceRepositoryURI, given.SourceRepositoryURI) { + return fmt.Errorf("expected SourceRepositoryURI to be %s, got %s", expected.SourceRepositoryURI, given.SourceRepositoryURI) } - // if issuer is anything other than the default, use the user-provided value; - // otherwise, select the appropriate default based on the tenant - certIssuer := verifiedCert.Extensions.Issuer - if !strings.EqualFold(criteria.Certificate.Issuer, certIssuer) { - if strings.Index(certIssuer, criteria.Certificate.Issuer+"/") == 0 { - return fmt.Errorf("expected Issuer to be %s, got %s -- if you have a custom OIDC issuer policy for your enterprise, use the --cert-oidc-issuer flag with your expected issuer", criteria.Certificate.Issuer, certIssuer) + // compare the OIDC issuers. If not equal, return an error depending + // on if there is a partial match + if !strings.EqualFold(expected.Issuer, given.Issuer) { + if strings.Index(given.Issuer, expected.Issuer+"/") == 0 { + return fmt.Errorf("expected Issuer to be %s, got %s -- if you have a custom OIDC issuer policy for your enterprise, use the --cert-oidc-issuer flag with your expected issuer", expected.Issuer, given.Issuer) } - return fmt.Errorf("expected Issuer to be %s, got %s", criteria.Certificate.Issuer, certIssuer) + return fmt.Errorf("expected Issuer to be %s, got %s", expected.Issuer, given.Issuer) } return nil diff --git a/pkg/cmd/attestation/verification/extensions_test.go b/pkg/cmd/attestation/verification/extensions_test.go index b8ef2875f..73d808119 100644 --- a/pkg/cmd/attestation/verification/extensions_test.go +++ b/pkg/cmd/attestation/verification/extensions_test.go @@ -37,8 +37,9 @@ func TestVerifyCertExtensions(t *testing.T) { } t.Run("passes with one result", func(t *testing.T) { - err := VerifyCertExtensions(results, c) + verified, err := VerifyCertExtensions(results, c) require.NoError(t, err) + require.Len(t, verified, 1) }) t.Run("passes with 1/2 valid results", func(t *testing.T) { @@ -46,8 +47,9 @@ func TestVerifyCertExtensions(t *testing.T) { require.Len(t, twoResults, 2) twoResults[1].VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong" - err := VerifyCertExtensions(twoResults, c) + verified, err := VerifyCertExtensions(twoResults, c) require.NoError(t, err) + require.Len(t, verified, 1) }) t.Run("fails when all results fail verification", func(t *testing.T) { @@ -56,35 +58,40 @@ func TestVerifyCertExtensions(t *testing.T) { twoResults[0].VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong" twoResults[1].VerificationResult.Signature.Certificate.Extensions.SourceRepositoryOwnerURI = "https://github.com/wrong" - err := VerifyCertExtensions(twoResults, c) + verified, err := VerifyCertExtensions(twoResults, c) require.Error(t, err) + require.Nil(t, verified) }) t.Run("with wrong SourceRepositoryOwnerURI", func(t *testing.T) { expectedCriteria := c expectedCriteria.Certificate.SourceRepositoryOwnerURI = "https://github.com/wrong" - err := VerifyCertExtensions(results, expectedCriteria) + verified, err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected SourceRepositoryOwnerURI to be https://github.com/wrong, got https://github.com/owner") + require.Nil(t, verified) }) t.Run("with wrong SourceRepositoryURI", func(t *testing.T) { expectedCriteria := c expectedCriteria.Certificate.SourceRepositoryURI = "https://github.com/foo/wrong" - err := VerifyCertExtensions(results, expectedCriteria) + verified, err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/foo/wrong, got https://github.com/owner/repo") + require.Nil(t, verified) }) t.Run("with wrong OIDCIssuer", func(t *testing.T) { expectedCriteria := c expectedCriteria.Certificate.Issuer = "wrong" - err := VerifyCertExtensions(results, expectedCriteria) + verified, err := VerifyCertExtensions(results, expectedCriteria) require.ErrorContains(t, err, "expected Issuer to be wrong, got https://token.actions.githubusercontent.com") + require.Nil(t, verified) }) t.Run("with partial OIDCIssuer match", func(t *testing.T) { expectedResults := results expectedResults[0].VerificationResult.Signature.Certificate.Extensions.Issuer = "https://token.actions.githubusercontent.com/foo-bar" - err := VerifyCertExtensions(expectedResults, c) + verified, err := VerifyCertExtensions(expectedResults, c) require.ErrorContains(t, err, "expected Issuer to be https://token.actions.githubusercontent.com, got https://token.actions.githubusercontent.com/foo-bar -- if you have a custom OIDC issuer") + require.Nil(t, verified) }) } diff --git a/pkg/cmd/attestation/verification/mock_verifier.go b/pkg/cmd/attestation/verification/mock_verifier.go index 41332dc62..be828e66f 100644 --- a/pkg/cmd/attestation/verification/mock_verifier.go +++ b/pkg/cmd/attestation/verification/mock_verifier.go @@ -6,6 +6,7 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/api" "github.com/cli/cli/v2/pkg/cmd/attestation/test/data" + "github.com/sigstore/sigstore-go/pkg/bundle" "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" in_toto "github.com/in-toto/attestation/go/v1" @@ -13,10 +14,15 @@ import ( ) type MockSigstoreVerifier struct { - t *testing.T + t *testing.T + mockResults []*AttestationProcessingResult } -func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { +func (v *MockSigstoreVerifier) Verify([]*api.Attestation, verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { + if v.mockResults != nil { + return v.mockResults, nil + } + statement := &in_toto.Statement{} statement.PredicateType = SLSAPredicateV1 @@ -45,11 +51,51 @@ func (v *MockSigstoreVerifier) Verify(attestations []*api.Attestation, policy ve } func NewMockSigstoreVerifier(t *testing.T) *MockSigstoreVerifier { - return &MockSigstoreVerifier{t} + result := BuildSigstoreJsMockResult(t) + results := []*AttestationProcessingResult{&result} + + return &MockSigstoreVerifier{t, results} +} + +func NewMockSigstoreVerifierWithMockResults(t *testing.T, mockResults []*AttestationProcessingResult) *MockSigstoreVerifier { + return &MockSigstoreVerifier{t, mockResults} } type FailSigstoreVerifier struct{} -func (v *FailSigstoreVerifier) Verify(attestations []*api.Attestation, policy verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { +func (v *FailSigstoreVerifier) Verify([]*api.Attestation, verify.PolicyBuilder) ([]*AttestationProcessingResult, error) { return nil, fmt.Errorf("failed to verify attestations") } + +func BuildMockResult(b *bundle.Bundle, buildSignerURI, sourceRepoOwnerURI, sourceRepoURI, issuer string) AttestationProcessingResult { + statement := &in_toto.Statement{} + statement.PredicateType = SLSAPredicateV1 + + return AttestationProcessingResult{ + Attestation: &api.Attestation{ + Bundle: b, + }, + VerificationResult: &verify.VerificationResult{ + Statement: statement, + Signature: &verify.SignatureVerificationResult{ + Certificate: &certificate.Summary{ + Extensions: certificate.Extensions{ + BuildSignerURI: buildSignerURI, + SourceRepositoryOwnerURI: sourceRepoOwnerURI, + SourceRepositoryURI: sourceRepoURI, + Issuer: issuer, + }, + }, + }, + }, + } +} + +func BuildSigstoreJsMockResult(t *testing.T) AttestationProcessingResult { + bundle := data.SigstoreBundle(t) + buildSignerURI := "https://github.com/github/example/.github/workflows/release.yml@refs/heads/main" + sourceRepoOwnerURI := "https://github.com/sigstore" + sourceRepoURI := "https://github.com/sigstore/sigstore-js" + issuer := "https://token.actions.githubusercontent.com" + return BuildMockResult(bundle, buildSignerURI, sourceRepoOwnerURI, sourceRepoURI, issuer) +} diff --git a/pkg/cmd/attestation/verify/attestation.go b/pkg/cmd/attestation/verify/attestation.go new file mode 100644 index 000000000..bb96c9526 --- /dev/null +++ b/pkg/cmd/attestation/verify/attestation.go @@ -0,0 +1,73 @@ +package verify + +import ( + "fmt" + + "github.com/cli/cli/v2/internal/text" + "github.com/cli/cli/v2/pkg/cmd/attestation/api" + "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" + "github.com/cli/cli/v2/pkg/cmd/attestation/verification" +) + +func getAttestations(o *Options, a artifact.DigestedArtifact) ([]*api.Attestation, string, error) { + if o.BundlePath != "" { + attestations, err := verification.GetLocalAttestations(o.BundlePath) + if err != nil { + msg := fmt.Sprintf("✗ Loading attestations from %s failed", a.URL) + return nil, msg, err + } + pluralAttestation := text.Pluralize(len(attestations), "attestation") + msg := fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.BundlePath) + return attestations, msg, nil + } + + if o.UseBundleFromRegistry { + attestations, err := verification.GetOCIAttestations(o.OCIClient, a) + if err != nil { + msg := "✗ Loading attestations from OCI registry failed" + return nil, msg, err + } + pluralAttestation := text.Pluralize(len(attestations), "attestation") + msg := fmt.Sprintf("Loaded %s from %s", pluralAttestation, o.ArtifactPath) + return attestations, msg, nil + } + + params := verification.FetchRemoteAttestationsParams{ + Digest: a.DigestWithAlg(), + Limit: o.Limit, + Owner: o.Owner, + Repo: o.Repo, + } + + attestations, err := verification.GetRemoteAttestations(o.APIClient, params) + if err != nil { + msg := "✗ Loading attestations from GitHub API failed" + return nil, msg, err + } + pluralAttestation := text.Pluralize(len(attestations), "attestation") + msg := fmt.Sprintf("Loaded %s from GitHub API", pluralAttestation) + return attestations, msg, nil +} + +func verifyAttestations(art artifact.DigestedArtifact, att []*api.Attestation, sgVerifier verification.SigstoreVerifier, ec verification.EnforcementCriteria) ([]*verification.AttestationProcessingResult, string, error) { + sgPolicy, err := buildSigstoreVerifyPolicy(ec, art) + if err != nil { + logMsg := "✗ Failed to build Sigstore verification policy" + return nil, logMsg, err + } + + sigstoreVerified, err := sgVerifier.Verify(att, sgPolicy) + if err != nil { + logMsg := "✗ Sigstore verification failed" + return nil, logMsg, err + } + + // Verify extensions + certExtVerified, err := verification.VerifyCertExtensions(sigstoreVerified, ec) + if err != nil { + logMsg := "✗ Policy verification failed" + return nil, logMsg, err + } + + return certExtVerified, "", nil +} diff --git a/pkg/cmd/attestation/verify/attestation_integration_test.go b/pkg/cmd/attestation/verify/attestation_integration_test.go new file mode 100644 index 000000000..caa02281f --- /dev/null +++ b/pkg/cmd/attestation/verify/attestation_integration_test.go @@ -0,0 +1,117 @@ +//go:build integration + +package verify + +import ( + "testing" + + "github.com/cli/cli/v2/pkg/cmd/attestation/api" + "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" + "github.com/cli/cli/v2/pkg/cmd/attestation/io" + "github.com/cli/cli/v2/pkg/cmd/attestation/test" + "github.com/cli/cli/v2/pkg/cmd/attestation/verification" + "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" + "github.com/stretchr/testify/require" +) + +func getAttestationsFor(t *testing.T, bundlePath string) []*api.Attestation { + t.Helper() + + attestations, err := verification.GetLocalAttestations(bundlePath) + require.NoError(t, err) + + return attestations +} + +func TestVerifyAttestations(t *testing.T) { + sgVerifier := verification.NewLiveSigstoreVerifier(verification.SigstoreConfig{ + Logger: io.NewTestHandler(), + }) + + certSummary := certificate.Summary{} + certSummary.SourceRepositoryOwnerURI = "https://github.com/sigstore" + certSummary.SourceRepositoryURI = "https://github.com/sigstore/sigstore-js" + certSummary.Issuer = verification.GitHubOIDCIssuer + + ec := verification.EnforcementCriteria{ + Certificate: certSummary, + PredicateType: verification.SLSAPredicateV1, + SANRegex: "^https://github.com/sigstore/", + } + require.NoError(t, ec.Valid()) + + artifactPath := test.NormalizeRelativePath("../test/data/sigstore-js-2.1.0.tgz") + a, err := artifact.NewDigestedArtifact(nil, artifactPath, "sha512") + require.NoError(t, err) + + t.Run("all attestations pass verification", func(t *testing.T) { + attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") + require.Len(t, attestations, 2) + results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, ec) + require.NoError(t, err) + require.Zero(t, errMsg) + require.Len(t, results, 2) + }) + + t.Run("passes verification with 2/3 attestations passing Sigstore verification", func(t *testing.T) { + invalidBundle := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json") + attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") + attestations = append(attestations, invalidBundle[0]) + require.Len(t, attestations, 3) + + results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, ec) + require.NoError(t, err) + require.Zero(t, errMsg) + require.Len(t, results, 2) + }) + + t.Run("fails verification when Sigstore verification fails", func(t *testing.T) { + invalidBundle := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json") + invalidBundle2 := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json") + attestations := append(invalidBundle, invalidBundle2...) + require.Len(t, attestations, 2) + + results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, ec) + require.Error(t, err) + require.Contains(t, errMsg, "✗ Sigstore verification failed") + require.Nil(t, results) + }) + + t.Run("attestations fail to verify when cert extensions don't match enforcement criteria", func(t *testing.T) { + sgjAttestation := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") + reusableWorkflowAttestations := getAttestationsFor(t, "../test/data/reusable-workflow-attestation.sigstore.json") + attestations := []*api.Attestation{sgjAttestation[0], reusableWorkflowAttestations[0], sgjAttestation[1]} + require.Len(t, attestations, 3) + + rwfResult := verification.BuildMockResult(reusableWorkflowAttestations[0].Bundle, "", "https://github.com/malancas", "", verification.GitHubOIDCIssuer) + sgjResult := verification.BuildSigstoreJsMockResult(t) + mockResults := []*verification.AttestationProcessingResult{&sgjResult, &rwfResult, &sgjResult} + mockSgVerifier := verification.NewMockSigstoreVerifierWithMockResults(t, mockResults) + + // we want to test that attestations that pass Sigstore verification but fail + // cert extension verification are filtered out properly in the second step + // in verifyAttestations. By using a mock Sigstore verifier, we can ensure + // that the call to verification.VerifyCertExtensions in verifyAttestations + // is filtering out attestations as expected + results, errMsg, err := verifyAttestations(*a, attestations, mockSgVerifier, ec) + require.NoError(t, err) + require.Zero(t, errMsg) + require.Len(t, results, 2) + for _, result := range results { + require.NotEqual(t, result.Attestation.Bundle, reusableWorkflowAttestations[0].Bundle) + } + }) + + t.Run("fails verification when cert extension verification fails", func(t *testing.T) { + attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") + require.Len(t, attestations, 2) + + expectedCriteria := ec + expectedCriteria.Certificate.SourceRepositoryOwnerURI = "https://github.com/wrong" + + results, errMsg, err := verifyAttestations(*a, attestations, sgVerifier, expectedCriteria) + require.Error(t, err) + require.Contains(t, errMsg, "✗ Policy verification failed") + require.Nil(t, results) + }) +} diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 47b52bb30..5b31371ff 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -6,7 +6,6 @@ import ( "regexp" "github.com/cli/cli/v2/internal/ghinstance" - "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmd/attestation/api" "github.com/cli/cli/v2/pkg/cmd/attestation/artifact" "github.com/cli/cli/v2/pkg/cmd/attestation/artifact/oci" @@ -222,42 +221,18 @@ func runVerify(opts *Options) error { opts.Logger.Printf("Loaded digest %s for %s\n", artifact.DigestWithAlg(), artifact.URL) - c := verification.FetchAttestationsConfig{ - APIClient: opts.APIClient, - BundlePath: opts.BundlePath, - Digest: artifact.DigestWithAlg(), - Limit: opts.Limit, - Owner: opts.Owner, - Repo: opts.Repo, - OCIClient: opts.OCIClient, - UseBundleFromRegistry: opts.UseBundleFromRegistry, - NameRef: artifact.NameRef(), - } - attestations, err := verification.GetAttestations(c) + attestations, logMsg, err := getAttestations(opts, *artifact) if err != nil { if ok := errors.Is(err, api.ErrNoAttestations{}); ok { opts.Logger.Printf(opts.Logger.ColorScheme.Red("✗ No attestations found for subject %s\n"), artifact.DigestWithAlg()) return err } - - if c.IsBundleProvided() { - opts.Logger.Printf(opts.Logger.ColorScheme.Red("✗ Loading attestations from %s failed\n"), artifact.URL) - } else if c.UseBundleFromRegistry { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Loading attestations from OCI registry failed")) - } else { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Loading attestations from GitHub API failed")) - } + // Print the message signifying failure fetching attestations + opts.Logger.Println(opts.Logger.ColorScheme.Red(logMsg)) return err } - - pluralAttestation := text.Pluralize(len(attestations), "attestation") - if c.IsBundleProvided() { - opts.Logger.Printf("Loaded %s from %s\n", pluralAttestation, opts.BundlePath) - } else if c.UseBundleFromRegistry { - opts.Logger.Printf("Loaded %s from %s\n", pluralAttestation, opts.ArtifactPath) - } else { - opts.Logger.Printf("Loaded %s from GitHub API\n", pluralAttestation) - } + // Print the message signifying success fetching attestations + opts.Logger.Println(logMsg) // Apply predicate type filter to returned attestations filteredAttestations := verification.FilterAttestations(ec.PredicateType, attestations) @@ -269,21 +244,9 @@ func runVerify(opts *Options) error { opts.Logger.VerbosePrintf("Verifying attestations with predicate type: %s\n", ec.PredicateType) - sp, err := buildSigstoreVerifyPolicy(ec, *artifact) + verified, errMsg, err := verifyAttestations(*artifact, attestations, opts.SigstoreVerifier, ec) if err != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to build Sigstore verification policy")) - return err - } - - verifyResults, err := opts.SigstoreVerifier.Verify(attestations, sp) - if err != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Sigstore verification failed")) - return err - } - - // Verify extensions - if err := verification.VerifyCertExtensions(verifyResults, ec); err != nil { - opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Policy verification failed")) + opts.Logger.Println(opts.Logger.ColorScheme.Red(errMsg)) return err } @@ -292,7 +255,7 @@ func runVerify(opts *Options) error { // If an exporter is provided with the --json flag, write the results to the terminal in JSON format if opts.exporter != nil { // print the results to the terminal as an array of JSON objects - if err = opts.exporter.Write(opts.Logger.IO, verifyResults); err != nil { + if err = opts.exporter.Write(opts.Logger.IO, verified); err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("✗ Failed to write JSON output")) return err } @@ -302,7 +265,7 @@ func runVerify(opts *Options) error { opts.Logger.Printf("%s was attested by:\n", artifact.DigestWithAlg()) // Otherwise print the results to the terminal in a table - tableContent, err := buildTableVerifyContent(opts.Tenant, verifyResults) + tableContent, err := buildTableVerifyContent(opts.Tenant, verified) if err != nil { opts.Logger.Println(opts.Logger.ColorScheme.Red("failed to parse results")) return err diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index 4794d1fbe..65a9d696a 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -35,23 +35,23 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "delete [| | --all]", Short: "Delete GitHub Actions caches", - Long: ` - Delete GitHub Actions caches. + Long: heredoc.Docf(` + Delete GitHub Actions caches. - Deletion requires authorization with the "repo" scope. -`, + Deletion requires authorization with the %[1]srepo%[1]s scope. + `, "`"), Example: heredoc.Doc(` - # Delete a cache by id - $ gh cache delete 1234 + # Delete a cache by id + $ gh cache delete 1234 - # Delete a cache by key - $ gh cache delete cache-key + # Delete a cache by key + $ gh cache delete cache-key - # Delete a cache by id in a specific repo - $ gh cache delete 1234 --repo cli/cli + # Delete a cache by id in a specific repo + $ gh cache delete 1234 --repo cli/cli - # Delete all caches - $ gh cache delete --all + # Delete all caches + $ gh cache delete --all `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { diff --git a/pkg/cmd/codespace/rebuild.go b/pkg/cmd/codespace/rebuild.go index 93d43bf29..565406edc 100644 --- a/pkg/cmd/codespace/rebuild.go +++ b/pkg/cmd/codespace/rebuild.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/codespaces" "github.com/cli/cli/v2/internal/codespaces/api" "github.com/cli/cli/v2/internal/codespaces/portforwarder" @@ -20,9 +21,12 @@ func newRebuildCmd(app *App) *cobra.Command { rebuildCmd := &cobra.Command{ Use: "rebuild", Short: "Rebuild a codespace", - Long: `Rebuilding recreates your codespace. Your code and any current changes will be -preserved. Your codespace will be rebuilt using your working directory's -dev container. A full rebuild also removes cached Docker images.`, + Long: heredoc.Doc(` + Rebuilding recreates your codespace. + + Your code and any current changes will be preserved. Your codespace will be rebuilt using + your working directory's dev container. A full rebuild also removes cached Docker images. + `), Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { return app.Rebuild(cmd.Context(), selector, fullRebuild) diff --git a/pkg/cmd/issue/develop/develop_test.go b/pkg/cmd/issue/develop/develop_test.go index c35bcb845..abdebf0c8 100644 --- a/pkg/cmd/issue/develop/develop_test.go +++ b/pkg/cmd/issue/develop/develop_test.go @@ -249,7 +249,7 @@ func TestDevelopRun(t *testing.T) { expectedOut: heredoc.Doc(` Showing linked branches for OWNER/REPO#42 - + BRANCH URL foo https://github.com/OWNER/REPO/tree/foo bar https://github.com/OWNER/OTHER-REPO/tree/bar diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index cd9da809c..54ecf17f6 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -174,7 +174,14 @@ func checkoutRun(opts *CheckoutOptions) error { cmdQueue = append(cmdQueue, []string{"submodule", "update", "--init", "--recursive"}) } - err = executeCmds(opts.GitClient, cmdQueue) + // Note that although we will probably be fetching from the headRemote, in practice, PR checkout can only + // ever point to one host, and we know baseRemote must be populated, where headRemote might be nil (e.g. when + // it was deleted). + credentialPattern, err := git.CredentialPatternFromRemote(context.Background(), opts.GitClient, baseRemote.Name) + if err != nil { + return err + } + err = executeCmds(opts.GitClient, credentialPattern, cmdQueue) if err != nil { return err } @@ -284,13 +291,16 @@ func localBranchExists(client *git.Client, b string) bool { return err == nil } -func executeCmds(client *git.Client, cmdQueue [][]string) error { +func executeCmds(client *git.Client, credentialPattern git.CredentialPattern, cmdQueue [][]string) error { for _, args := range cmdQueue { var err error var cmd *git.Command - if args[0] == "fetch" || args[0] == "submodule" { - cmd, err = client.AuthenticatedCommand(context.Background(), args...) - } else { + switch args[0] { + case "submodule": + cmd, err = client.AuthenticatedCommand(context.Background(), credentialPattern, args...) + case "fetch": + cmd, err = client.AuthenticatedCommand(context.Background(), git.AllMatchingCredentialsPattern, args...) + default: cmd, err = client.Command(context.Background(), args...) } if err != nil { diff --git a/pkg/cmd/pr/checkout/checkout_test.go b/pkg/cmd/pr/checkout/checkout_test.go index 138e1afdf..0d6b12bc4 100644 --- a/pkg/cmd/pr/checkout/checkout_test.go +++ b/pkg/cmd/pr/checkout/checkout_test.go @@ -75,6 +75,32 @@ func Test_checkoutRun(t *testing.T) { wantStderr string wantErr bool }{ + { + name: "checkout with ssh remote URL", + opts: &CheckoutOptions{ + SelectorArg: "123", + Finder: func() shared.PRFinder { + baseRepo, pr := stubPR("OWNER/REPO:master", "OWNER/REPO:feature") + finder := shared.NewMockFinder("123", pr, baseRepo) + return finder + }(), + Config: func() (gh.Config, error) { + return config.NewBlankConfig(), nil + }, + Branch: func() (string, error) { + return "main", nil + }, + }, + remotes: map[string]string{ + "origin": "OWNER/REPO", + }, + runStubs: func(cs *run.CommandStubber) { + cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") + cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") + cs.Register(`git checkout -b feature --track origin/feature`, 0, "") + }, + }, { name: "fork repo was deleted", opts: &CheckoutOptions{ @@ -97,6 +123,7 @@ func Test_checkoutRun(t *testing.T) { "origin": "OWNER/REPO", }, runStubs: func(cs *run.CommandStubber) { + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 1, "") cs.Register(`git checkout feature`, 0, "") @@ -127,6 +154,7 @@ func Test_checkoutRun(t *testing.T) { }, runStubs: func(cs *run.CommandStubber) { cs.Register(`git show-ref --verify -- refs/heads/foobar`, 1, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git checkout -b foobar --track origin/feature`, 0, "") }, @@ -154,6 +182,7 @@ func Test_checkoutRun(t *testing.T) { }, runStubs: func(cs *run.CommandStubber) { cs.Register(`git config branch\.foobar\.merge`, 1, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/hubot/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:foobar`, 0, "") cs.Register(`git checkout foobar`, 0, "") cs.Register(`git config branch\.foobar\.remote https://github.com/hubot/REPO.git`, 0, "") @@ -320,6 +349,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "") cs.Register(`git checkout -b feature --track origin/feature`, 0, "") @@ -340,6 +370,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "") cs.Register(`git checkout feature`, 0, "") @@ -373,6 +404,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch robot-fork \+refs/heads/feature:refs/remotes/robot-fork/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "") cs.Register(`git checkout -b feature --track robot-fork/feature`, 0, "") @@ -394,6 +426,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 1, "") cs.Register(`git checkout feature`, 0, "") @@ -417,6 +450,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 0, "refs/heads/feature\n") cs.Register(`git checkout feature`, 0, "") @@ -437,6 +471,7 @@ func TestPRCheckout_detachedHead(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 0, "refs/heads/feature\n") cs.Register(`git checkout feature`, 0, "") @@ -457,6 +492,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head`, 0, "") cs.Register(`git config branch\.feature\.merge`, 0, "refs/heads/feature\n") cs.Register(`git merge --ff-only FETCH_HEAD`, 0, "") @@ -494,6 +530,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 1, "") cs.Register(`git checkout feature`, 0, "") @@ -516,6 +553,7 @@ func TestPRCheckout_recurseSubmodules(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "") cs.Register(`git checkout feature`, 0, "") @@ -538,6 +576,7 @@ func TestPRCheckout_force(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "") cs.Register(`git checkout feature`, 0, "") @@ -561,6 +600,7 @@ func TestPRCheckout_detach(t *testing.T) { defer cmdTeardown(t) cs.Register(`git checkout --detach FETCH_HEAD`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/hubot/REPO.git") cs.Register(`git fetch origin refs/pull/123/head`, 0, "") output, err := runCommand(http, nil, "", `123 --detach`) diff --git a/pkg/cmd/pr/view/view_test.go b/pkg/cmd/pr/view/view_test.go index 470e3bd27..e7f572c76 100644 --- a/pkg/cmd/pr/view/view_test.go +++ b/pkg/cmd/pr/view/view_test.go @@ -32,6 +32,7 @@ func TestJSONFields(t *testing.T) { "author", "autoMergeRequest", "baseRefName", + "baseRefOid", "body", "changedFiles", "closed", diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index 0b7c99b23..8e77546d0 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -477,6 +477,21 @@ func createRun(opts *CreateOptions) error { } newRelease, err := createRelease(httpClient, baseRepo, params) + + var errMissingRequiredWorkflowScope *errMissingRequiredWorkflowScope + if errors.As(err, &errMissingRequiredWorkflowScope) { + host := errMissingRequiredWorkflowScope.Hostname + refreshInstructions := fmt.Sprintf("gh auth refresh -h %[1]s -s workflow", host) + cs := opts.IO.ColorScheme() + + var sb strings.Builder + sb.WriteString(fmt.Sprintf("%s Failed to create release, \"workflow\" scope may be required.\n", cs.WarningIcon())) + sb.WriteString(fmt.Sprintf("To request it, run:\n%s\n", cs.Bold(refreshInstructions))) + fmt.Fprint(opts.IO.ErrOut, sb.String()) + + return cmdutil.SilentError + } + if err != nil { return err } diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index 85c1c3f3f..c9e9a8c8a 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "testing" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" @@ -1082,6 +1083,74 @@ func Test_createRun(t *testing.T) { runStubs: defaultRunStubs, wantErr: "cannot generate release notes from tag v1.2.3 as it does not exist locally", }, + { + name: "API returns 404, OAuth token has no workflow scope", + isTTY: false, + opts: CreateOptions{ + TagName: "Does not matter", + }, + runStubs: func(rs *run.CommandStubber) { + rs.Register(contentCmd, 0, "some tag message") + rs.Register(signatureCmd, 0, "") + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("RepositoryFindRef"), + httpmock.StringResponse(`{"data":{"repository":{"ref": {"id": "tag id"}}}}`), + ) + reg.Register( + httpmock.REST("POST", "repos/OWNER/REPO/releases"), + httpmock.StatusScopesResponder(404, `repo,read:org`)) + }, + wantStderr: heredoc.Doc(` + ! Failed to create release, "workflow" scope may be required. + To request it, run: + gh auth refresh -h github.com -s workflow + `), + wantErr: cmdutil.SilentError.Error(), + }, + { + name: "API returns 404, OAuth token has workflow scope", + isTTY: false, + opts: CreateOptions{ + TagName: "Does not matter", + }, + runStubs: func(rs *run.CommandStubber) { + rs.Register(contentCmd, 0, "some tag message") + rs.Register(signatureCmd, 0, "") + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("RepositoryFindRef"), + httpmock.StringResponse(`{"data":{"repository":{"ref": {"id": "tag id"}}}}`), + ) + reg.Register( + httpmock.REST("POST", "repos/OWNER/REPO/releases"), + httpmock.StatusScopesResponder(404, `repo,read:org,workflow`)) + }, + wantErr: "HTTP 404 (https://api.github.com/repos/OWNER/REPO/releases)", + }, + { + name: "API returns 404, not an OAuth token", + isTTY: false, + opts: CreateOptions{ + TagName: "Does not matter", + }, + runStubs: func(rs *run.CommandStubber) { + rs.Register(contentCmd, 0, "some tag message") + rs.Register(signatureCmd, 0, "") + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("RepositoryFindRef"), + httpmock.StringResponse(`{"data":{"repository":{"ref": {"id": "tag id"}}}}`), + ) + reg.Register( + httpmock.REST("POST", "repos/OWNER/REPO/releases"), + httpmock.StatusStringResponse(404, `HTTP 404 (https://api.github.com/repos/OWNER/REPO/releases)`)) + }, + wantErr: "HTTP 404 (https://api.github.com/repos/OWNER/REPO/releases)", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1115,7 +1184,6 @@ func Test_createRun(t *testing.T) { err := createRun(&tt.opts) if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) - return } else { require.NoError(t, err) } diff --git a/pkg/cmd/release/create/http.go b/pkg/cmd/release/create/http.go index 84e95710f..3bb55f39e 100644 --- a/pkg/cmd/release/create/http.go +++ b/pkg/cmd/release/create/http.go @@ -8,12 +8,16 @@ import ( "io" "net/http" "net/url" + "slices" + "strings" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/shurcooL/githubv4" + + ghauth "github.com/cli/go-gh/v2/pkg/auth" ) type tag struct { @@ -27,6 +31,14 @@ type releaseNotes struct { var notImplementedError = errors.New("not implemented") +type errMissingRequiredWorkflowScope struct { + Hostname string +} + +func (e errMissingRequiredWorkflowScope) Error() string { + return "workflow scope may be required" +} + func remoteTagExists(httpClient *http.Client, repo ghrepo.Interface, tagName string) (bool, error) { gql := api.NewClientFromHTTP(httpClient) qualifiedTagName := fmt.Sprintf("refs/tags/%s", tagName) @@ -174,6 +186,24 @@ func createRelease(httpClient *http.Client, repo ghrepo.Interface, params map[st } defer resp.Body.Close() + // Check if we received a 404 while attempting to create a release without + // the workflow scope, and if so, return an error message that explains a possible + // solution to the user. + // + // If the same file (with both the same path and contents) exists + // on another branch in the repo, releases with workflow file changes can be + // created without the workflow scope. Otherwise, the workflow scope is + // required to create the release, but the API does not indicate this criteria + // beyond returning a 404. + // + // https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/scopes-for-oauth-apps#available-scopes + if resp.StatusCode == http.StatusNotFound && !tokenHasWorkflowScope(resp) { + normalizedHostname := ghauth.NormalizeHostname(resp.Request.URL.Hostname()) + return nil, &errMissingRequiredWorkflowScope{ + Hostname: normalizedHostname, + } + } + success := resp.StatusCode >= 200 && resp.StatusCode < 300 if !success { return nil, api.HandleHTTPError(resp) @@ -254,3 +284,18 @@ func deleteRelease(httpClient *http.Client, release *shared.Release) error { } return nil } + +// tokenHasWorkflowScope checks if the given http.Response's token has the workflow scope. +// Tokens that do not have OAuth scopes are assumed to have the workflow scope. +func tokenHasWorkflowScope(resp *http.Response) bool { + scopes := resp.Header.Get("X-Oauth-Scopes") + + // Return true when no scopes are present - no scopes in this header + // means that the user is probably authenticating with a token type other + // than an OAuth token, and we don't know what this token's scopes actually are. + if scopes == "" { + return true + } + + return slices.Contains(strings.Split(scopes, ","), "workflow") +} diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index 79c349aa4..ec7026759 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -676,7 +676,7 @@ func createFromLocal(opts *CreateOptions) error { } if opts.Push && repoType == bare { - cmd, err := opts.GitClient.AuthenticatedCommand(context.Background(), "push", baseRemote, "--mirror") + cmd, err := opts.GitClient.AuthenticatedCommand(context.Background(), git.AllMatchingCredentialsPattern, "push", baseRemote, "--mirror") if err != nil { return err } diff --git a/pkg/cmd/repo/delete/delete.go b/pkg/cmd/repo/delete/delete.go index 722d748b9..7c6476f1d 100644 --- a/pkg/cmd/repo/delete/delete.go +++ b/pkg/cmd/repo/delete/delete.go @@ -6,6 +6,7 @@ import ( "net/http" "strings" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" @@ -38,12 +39,14 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "delete []", Short: "Delete a repository", - Long: `Delete a GitHub repository. + Long: heredoc.Docf(` + Delete a GitHub repository. + + With no argument, deletes the current repository. Otherwise, deletes the specified repository. -With no argument, deletes the current repository. Otherwise, deletes the specified repository. - -Deletion requires authorization with the "delete_repo" scope. -To authorize, run "gh auth refresh -s delete_repo"`, + Deletion requires authorization with the %[1]sdelete_repo%[1]s scope. + To authorize, run %[1]sgh auth refresh -s delete_repo%[1]s + `, "`"), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { if len(args) > 0 { diff --git a/pkg/cmd/repo/sync/git.go b/pkg/cmd/repo/sync/git.go index 1f18d0fdb..0a0be1ed6 100644 --- a/pkg/cmd/repo/sync/git.go +++ b/pkg/cmd/repo/sync/git.go @@ -55,7 +55,7 @@ func (g *gitExecuter) CurrentBranch() (string, error) { func (g *gitExecuter) Fetch(remote, ref string) error { args := []string{"fetch", "-q", remote, ref} - cmd, err := g.client.AuthenticatedCommand(context.Background(), args...) + cmd, err := g.client.AuthenticatedCommand(context.Background(), git.AllMatchingCredentialsPattern, args...) if err != nil { return err } diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index 787cdcf9d..196a047d8 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -225,10 +225,16 @@ func GraphQLQuery(body string, cb func(string, map[string]interface{})) Responde } } +// ScopesResponder returns a response with a 200 status code and the given OAuth scopes. func ScopesResponder(scopes string) func(*http.Request) (*http.Response, error) { + return StatusScopesResponder(http.StatusOK, scopes) +} + +// StatusScopesResponder returns a response with the given status code and OAuth scopes. +func StatusScopesResponder(status int, scopes string) func(*http.Request) (*http.Response, error) { return func(req *http.Request) (*http.Response, error) { return &http.Response{ - StatusCode: 200, + StatusCode: status, Request: req, Header: map[string][]string{ "X-Oauth-Scopes": {scopes},