diff --git a/acceptance/testdata/pr/pr-create-from-issue-develop-base.txtar b/acceptance/testdata/pr/pr-create-from-issue-develop-base.txtar new file mode 100644 index 000000000..f0619940e --- /dev/null +++ b/acceptance/testdata/pr/pr-create-from-issue-develop-base.txtar @@ -0,0 +1,37 @@ +# Set up env vars +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes ${ORG}/${REPO} + +# Clone the repo +exec gh repo clone ${ORG}/${REPO} + +# Create a branch to act as the merge base branch +cd ${REPO} +exec git checkout -b long-lived-feature-branch +exec git push -u origin long-lived-feature-branch + +# Create an issue to develop against +exec gh issue create --title 'Feature Request' --body 'Request Body' +stdout2env ISSUE_URL + +# Create a new branch using issue develop with the long lived branch as the base +exec gh issue develop --name 'feature-branch' --base 'long-lived-feature-branch' --checkout ${ISSUE_URL} + +# Prepare a PR on the develop branch +exec git commit --allow-empty -m 'Empty Commit' +exec git push -u origin feature-branch + +# Create the PR +exec gh pr create --title 'Feature Title' --body 'Feature Body' + +# Check the PR is created against the base branch we specified +exec gh pr view --json 'baseRefName' --jq '.baseRefName' +stdout 'long-lived-feature-branch' diff --git a/acceptance/testdata/pr/pr-create-from-manual-merge-base.txtar b/acceptance/testdata/pr/pr-create-from-manual-merge-base.txtar new file mode 100644 index 000000000..97ae168f5 --- /dev/null +++ b/acceptance/testdata/pr/pr-create-from-manual-merge-base.txtar @@ -0,0 +1,34 @@ +# Set up env vars +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes ${ORG}/${REPO} + +# Clone the repo +exec gh repo clone ${ORG}/${REPO} + +# Create a branch to act as the merge base branch +cd ${REPO} +exec git checkout -b long-lived-feature-branch +exec git push -u origin long-lived-feature-branch + +# Prepare a branch from the merge base to PR +exec git checkout -b feature-branch +exec git commit --allow-empty -m 'Empty Commit' +exec git push -u origin feature-branch + +# Set the merge-base branch config +exec git config 'branch.feature-branch.gh-merge-base' 'long-lived-feature-branch' + +# Create the PR +exec gh pr create --title 'Feature Title' --body 'Feature Body' + +# Check the PR is created against the merge base branch +exec gh pr view --json 'baseRefName' --jq '.baseRefName' +stdout 'long-lived-feature-branch' diff --git a/acceptance/testdata/workflow/run-download-traversal.txtar b/acceptance/testdata/workflow/run-download-traversal.txtar new file mode 100644 index 000000000..a8a644752 --- /dev/null +++ b/acceptance/testdata/workflow/run-download-traversal.txtar @@ -0,0 +1,71 @@ +# Set up env +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes ${ORG}/${REPO} + +# Clone the repo +exec gh repo clone ${ORG}/${REPO} + +# commit the workflow file +cd ${REPO} +mkdir .github/workflows +mv ../workflow.yml .github/workflows/workflow.yml +exec git add .github/workflows/workflow.yml +exec git commit -m 'Create workflow file' +exec git push -u origin main + +# Sleep because it takes a second for the workflow to register +sleep 1 + +# Check the workflow is indeed created +exec gh workflow list +stdout 'Test Workflow Name' + +# Run the workflow +exec gh workflow run 'Test Workflow Name' + +# It takes some time for a workflow run to register +sleep 10 + +# Get the run ID we want to watch +exec gh run list --json databaseId --jq '.[0].databaseId' +stdout2env RUN_ID + +# Wait for workflow to complete +exec gh run watch ${RUN_ID} --exit-status + +# Download the artifact and see there is an error +! exec gh run download ${RUN_ID} +stderr 'would result in path traversal' + +-- workflow.yml -- +# This is a basic workflow to help you get started with Actions + +name: Test Workflow Name + +# Controls when the workflow will run +on: + # Allows you to run this workflow manually from the Actions tab + workflow_dispatch: + +# A workflow run is made up of one or more jobs that can run sequentially or in parallel +jobs: + # This workflow contains a single job called "build" + build: + # The type of runner that the job will run on + runs-on: ubuntu-latest + + # Steps represent a sequence of tasks that will be executed as part of the job + steps: + - run: echo hello > world.txt + - uses: actions/upload-artifact@v4 + with: + name: .. + path: world.txt diff --git a/acceptance/testdata/workflow/run-download.txtar b/acceptance/testdata/workflow/run-download.txtar index 653fdbef5..8089cf2cd 100644 --- a/acceptance/testdata/workflow/run-download.txtar +++ b/acceptance/testdata/workflow/run-download.txtar @@ -1,17 +1,20 @@ +# Set up env +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + # Use gh as a credential helper exec gh auth setup-git # Create a repository with a file so it has a default branch -exec gh repo create $ORG/$SCRIPT_NAME-$RANDOM_STRING --add-readme --private +exec gh repo create ${ORG}/${REPO} --add-readme --private # Defer repo cleanup -defer gh repo delete --yes $ORG/$SCRIPT_NAME-$RANDOM_STRING +defer gh repo delete --yes ${ORG}/${REPO} # Clone the repo -exec gh repo clone $ORG/$SCRIPT_NAME-$RANDOM_STRING +exec gh repo clone ${ORG}/${REPO} # commit the workflow file -cd $SCRIPT_NAME-$RANDOM_STRING +cd ${REPO} mkdir .github/workflows mv ../workflow.yml .github/workflows/workflow.yml exec git add .github/workflows/workflow.yml @@ -36,13 +39,28 @@ exec gh run list --json databaseId --jq '.[0].databaseId' stdout2env RUN_ID # Wait for workflow to complete -exec gh run watch $RUN_ID --exit-status +exec gh run watch ${RUN_ID} --exit-status -# Download the artifact -exec gh run download $RUN_ID +# Download the artifact to current dir +exec gh run download ${RUN_ID} -# Check if we downloaded the artifact -exists ./my-artifact/world.txt +# Check that we downloaded the artifact and extracted into a dir with the name of the artifact +exists ./my-artifact/child/world.txt + +# Remove the artifact +rm ./my-artifact + +# Download the artifact via name to current dir +exec gh run download -n 'my-artifact' ${RUN_ID} + +# Check that we downloaded the artifact and extracted into the current dir +exists ./child/world.txt + +# Download the artifact via name to a specific dir +exec gh run download -n 'my-artifact' ${RUN_ID} --dir '..' + +# Check that we downloaded the artifact and extracted into the specified dir +exists ../child/world.txt -- workflow.yml -- # This is a basic workflow to help you get started with Actions @@ -63,8 +81,10 @@ jobs: # Steps represent a sequence of tasks that will be executed as part of the job steps: - - run: echo hello > world.txt + - run: | + mkdir -p ./parent/child + echo hello > ./parent/child/world.txt - uses: actions/upload-artifact@v4 with: name: my-artifact - path: world.txt + path: ./parent diff --git a/git/client.go b/git/client.go index 1dea7a6d6..35b9cf16c 100644 --- a/git/client.go +++ b/git/client.go @@ -20,6 +20,9 @@ import ( "github.com/cli/safeexec" ) +// MergeBaseConfig is the configuration setting to keep track of the PR target branch. +const MergeBaseConfig = "gh-merge-base" + var remoteRE = regexp.MustCompile(`(.+)\s+(.+)\s+\((push|fetch)\)`) // This regexp exists to match lines of the following form: @@ -373,10 +376,10 @@ func (c *Client) lookupCommit(ctx context.Context, sha, format string) ([]byte, return out, nil } -// ReadBranchConfig parses the `branch.BRANCH.(remote|merge)` part of git config. +// ReadBranchConfig parses the `branch.BRANCH.(remote|merge|gh-merge-base)` part of git config. func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg BranchConfig) { prefix := regexp.QuoteMeta(fmt.Sprintf("branch.%s.", branch)) - args := []string{"config", "--get-regexp", fmt.Sprintf("^%s(remote|merge)$", prefix)} + args := []string{"config", "--get-regexp", fmt.Sprintf("^%s(remote|merge|%s)$", prefix, MergeBaseConfig)} cmd, err := c.Command(ctx, args...) if err != nil { return @@ -385,6 +388,8 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg Branc if err != nil { return } + + cfg.LocalName = branch for _, line := range outputLines(out) { parts := strings.SplitN(line, " ", 2) if len(parts) < 2 { @@ -404,11 +409,26 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg Branc } case "merge": cfg.MergeRef = parts[1] + case MergeBaseConfig: + cfg.MergeBase = parts[1] } } return } +// SetBranchConfig sets the named value on the given branch. +func (c *Client) SetBranchConfig(ctx context.Context, branch, name, value string) error { + name = fmt.Sprintf("branch.%s.%s", branch, name) + args := []string{"config", name, value} + cmd, err := c.Command(ctx, args...) + if err != nil { + return err + } + // No output expected but check for any printed git error. + _, err = cmd.Output() + return err +} + func (c *Client) DeleteLocalTag(ctx context.Context, tag string) error { args := []string{"tag", "-d", tag} cmd, err := c.Command(ctx, args...) diff --git a/git/client_test.go b/git/client_test.go index 0fb7953bc..0ec4f7334 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -735,9 +735,9 @@ func TestClientReadBranchConfig(t *testing.T) { }{ { name: "read branch config", - cmdStdout: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk", - wantCmdArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge)$`, - wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/trunk"}, + cmdStdout: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk\nbranch.trunk.gh-merge-base trunk", + wantCmdArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|gh-merge-base)$`, + wantBranchConfig: BranchConfig{LocalName: "trunk", RemoteName: "origin", MergeRef: "refs/heads/trunk", MergeBase: "trunk"}, }, } for _, tt := range tests { diff --git a/git/objects.go b/git/objects.go index c33d92b7c..f371429dd 100644 --- a/git/objects.go +++ b/git/objects.go @@ -71,7 +71,11 @@ type Commit struct { } type BranchConfig struct { + // LocalName of the branch. + LocalName string RemoteName string RemoteURL *url.URL - MergeRef string + // MergeBase is the optional base branch to target in a new PR if `--base` is not specified. + MergeBase string + MergeRef string } diff --git a/internal/safepaths/absolute.go b/internal/safepaths/absolute.go new file mode 100644 index 000000000..db0551b32 --- /dev/null +++ b/internal/safepaths/absolute.go @@ -0,0 +1,74 @@ +package safepaths + +import ( + "fmt" + "path/filepath" + "strings" +) + +// Absolute must be constructed via ParseAbsolute, or other methods in this package. +// The zero value of Absolute will panic when String is called. +type Absolute struct { + path string +} + +// ParseAbsolute takes a string path that may be relative and returns +// an Absolute that is guaranteed to be absolute, or an error. +func ParseAbsolute(path string) (Absolute, error) { + path, err := filepath.Abs(path) + if err != nil { + return Absolute{}, fmt.Errorf("failed to get absolute path: %w", err) + } + + return Absolute{path: path}, nil +} + +// String returns a string representation of the absolute path, or panics +// if the absolute path is empty. This guards against programmer error. +func (a Absolute) String() string { + if a.path == "" { + panic("empty absolute path") + } + return a.path +} + +// Join an absolute path with elements to create a new Absolute path, or error. +// A PathTraversalError will be returned if the joined path would traverse outside of +// the base Absolute path. Note that this does not handle symlinks. +func (a Absolute) Join(elem ...string) (Absolute, error) { + joinedAbsolutePath, err := ParseAbsolute(filepath.Join(append([]string{a.path}, elem...)...)) + if err != nil { + return Absolute{}, fmt.Errorf("failed to parse joined path: %w", err) + } + + isSubpath, err := joinedAbsolutePath.isSubpathOf(a) + if err != nil { + return Absolute{}, err + } + + if !isSubpath { + return Absolute{}, PathTraversalError{ + Base: a, + Elems: elem, + } + } + + return joinedAbsolutePath, nil +} + +func (a Absolute) isSubpathOf(dir Absolute) (bool, error) { + relativePath, err := filepath.Rel(dir.path, a.path) + if err != nil { + return false, err + } + return !strings.HasPrefix(relativePath, ".."), nil +} + +type PathTraversalError struct { + Base Absolute + Elems []string +} + +func (e PathTraversalError) Error() string { + return fmt.Sprintf("joining %s and %s would be a traversal", e.Base, filepath.Join(e.Elems...)) +} diff --git a/internal/safepaths/absolute_test.go b/internal/safepaths/absolute_test.go new file mode 100644 index 000000000..7bda40d89 --- /dev/null +++ b/internal/safepaths/absolute_test.go @@ -0,0 +1,137 @@ +package safepaths_test + +import ( + "fmt" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/cli/cli/v2/internal/safepaths" + "github.com/stretchr/testify/require" +) + +func TestParseAbsolutePath(t *testing.T) { + t.Parallel() + + absolutePath, err := safepaths.ParseAbsolute("/base") + require.NoError(t, err) + + require.Equal(t, filepath.Join(rootDir(), "base"), absolutePath.String()) +} + +func TestAbsoluteEmptyPathStringPanic(t *testing.T) { + t.Parallel() + + absolutePath := safepaths.Absolute{} + require.Panics(t, func() { + _ = absolutePath.String() + }) +} + +func TestJoin(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + base safepaths.Absolute + elems []string + want safepaths.Absolute + wantPathTraversalError bool + }{ + { + name: "child of base", + base: mustParseAbsolute("/base"), + elems: []string{"child"}, + want: mustParseAbsolute("/base/child"), + }, + { + name: "grandchild of base", + base: mustParseAbsolute("/base"), + elems: []string{"child", "grandchild"}, + want: mustParseAbsolute("/base/child/grandchild"), + }, + { + name: "relative parent of base", + base: mustParseAbsolute("/base"), + elems: []string{".."}, + wantPathTraversalError: true, + }, + { + name: "relative grandparent of base", + base: mustParseAbsolute("/base"), + elems: []string{"..", ".."}, + wantPathTraversalError: true, + }, + { + name: "relative current dir", + base: mustParseAbsolute("/base"), + elems: []string{"."}, + want: mustParseAbsolute("/base"), + }, + { + name: "subpath via relative parent", + base: mustParseAbsolute("/child"), + elems: []string{"..", "child"}, + want: mustParseAbsolute("/child"), + }, + { + name: "empty string", + base: mustParseAbsolute("/base"), + elems: []string{""}, + want: mustParseAbsolute("/base"), + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + joinedPath, err := tt.base.Join(tt.elems...) + if tt.wantPathTraversalError { + var pathTraversalError safepaths.PathTraversalError + require.ErrorAs(t, err, &pathTraversalError) + require.Equal(t, tt.base, pathTraversalError.Base) + require.Equal(t, tt.elems, pathTraversalError.Elems) + return + } + require.NoError(t, err) + require.Equal(t, tt.want, joinedPath) + }) + } +} + +func TestPathTraversalErrorMessage(t *testing.T) { + t.Parallel() + + pathTraversalError := safepaths.PathTraversalError{ + Base: mustParseAbsolute("/base"), + Elems: []string{".."}, + } + expectedMsg := fmt.Sprintf("joining %s and %s would be a traversal", filepath.Join(rootDir(), "base"), "..") + require.EqualError(t, pathTraversalError, expectedMsg) +} + +func mustParseAbsolute(s string) safepaths.Absolute { + t, err := safepaths.ParseAbsolute(s) + if err != nil { + panic(err) + } + return t +} + +func rootDir() string { + // Get the current working directory + cwd, err := os.Getwd() + if err != nil { + panic(err) + } + + // For Windows, extract the volume and add back the root + if runtime.GOOS == "windows" { + volume := filepath.VolumeName(cwd) + return volume + "\\" + } + + // For Unix-based systems, the root is always "/" + return "/" +} diff --git a/pkg/cmd/attestation/verification/attestation.go b/pkg/cmd/attestation/verification/attestation.go index 07083a5c0..fdfd667bd 100644 --- a/pkg/cmd/attestation/verification/attestation.go +++ b/pkg/cmd/attestation/verification/attestation.go @@ -118,7 +118,7 @@ func GetRemoteAttestations(client api.Client, params FetchRemoteAttestationsPara } func GetOCIAttestations(client oci.Client, artifact artifact.DigestedArtifact) ([]*api.Attestation, error) { - attestations, err := client.GetAttestations(artifact.NameRef(), artifact.Digest()) + attestations, err := client.GetAttestations(artifact.NameRef(), artifact.DigestWithAlg()) if err != nil { return nil, fmt.Errorf("failed to fetch OCI attestations: %w", err) } diff --git a/pkg/cmd/attestation/verify/verify_integration_test.go b/pkg/cmd/attestation/verify/verify_integration_test.go index 781cb4df1..4d4c9599c 100644 --- a/pkg/cmd/attestation/verify/verify_integration_test.go +++ b/pkg/cmd/attestation/verify/verify_integration_test.go @@ -111,6 +111,25 @@ func TestVerifyIntegration(t *testing.T) { require.Error(t, err) require.ErrorContains(t, err, "verifying with issuer \"sigstore.dev\"") }) + + t.Run("with bundle from OCI registry", func(t *testing.T) { + opts := Options{ + APIClient: api.NewLiveClient(hc, host, logger), + ArtifactPath: "oci://ghcr.io/github/artifact-attestations-helm-charts/policy-controller:v0.10.0-github9", + UseBundleFromRegistry: true, + DigestAlgorithm: "sha256", + Logger: logger, + OCIClient: oci.NewLiveClient(), + OIDCIssuer: verification.GitHubOIDCIssuer, + Owner: "github", + PredicateType: verification.SLSAPredicateV1, + SANRegex: "^https://github.com/github/", + SigstoreVerifier: verification.NewLiveSigstoreVerifier(sigstoreConfig), + } + + err := runVerify(&opts) + require.NoError(t, err) + }) } func TestVerifyIntegrationCustomIssuer(t *testing.T) { diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index 87ac5826b..f4e3d8fec 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -59,10 +59,18 @@ func NewCmdBrowse(f *cmdutil.Factory, runF func(*BrowseOptions) error) *cobra.Co } cmd := &cobra.Command{ - Long: "Open the GitHub repository in the web browser.", - Short: "Open the repository in the browser", - Use: "browse [ | | ]", - Args: cobra.MaximumNArgs(1), + Short: "Open repositories, issues, pull requests, and more in the browser", + Long: heredoc.Doc(` + Transition from the terminal to the web browser to view and interact with: + + - Issues + - Pull requests + - Repository content + - Repository home page + - Repository settings + `), + Use: "browse [ | | ]", + Args: cobra.MaximumNArgs(1), Example: heredoc.Doc(` $ gh browse #=> Open the home page of the current repository diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index fc70febee..e56e6c0b8 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -198,7 +198,7 @@ func (c codespace) displayName(includeOwner bool) string { displayName = c.Name } - description := fmt.Sprintf("%s (%s): %s", c.Repository.FullName, branch, displayName) + description := fmt.Sprintf("%s [%s]: %s", c.Repository.FullName, branch, displayName) if includeOwner { description = fmt.Sprintf("%-15s %s", c.Owner.Login, description) diff --git a/pkg/cmd/codespace/common_test.go b/pkg/cmd/codespace/common_test.go index 717bbb69b..62fd02f9b 100644 --- a/pkg/cmd/codespace/common_test.go +++ b/pkg/cmd/codespace/common_test.go @@ -34,7 +34,7 @@ func Test_codespace_displayName(t *testing.T) { DisplayName: "scuba steve", }, }, - want: "cli/cli (trunk): scuba steve", + want: "cli/cli [trunk]: scuba steve", }, { name: "No included name - included gitstatus - no unsaved changes", @@ -50,7 +50,7 @@ func Test_codespace_displayName(t *testing.T) { DisplayName: "scuba steve", }, }, - want: "cli/cli (trunk): scuba steve", + want: "cli/cli [trunk]: scuba steve", }, { name: "No included name - included gitstatus - unsaved changes", @@ -67,7 +67,7 @@ func Test_codespace_displayName(t *testing.T) { DisplayName: "scuba steve", }, }, - want: "cli/cli (trunk*): scuba steve", + want: "cli/cli [trunk*]: scuba steve", }, { name: "Included name - included gitstatus - unsaved changes", @@ -84,7 +84,7 @@ func Test_codespace_displayName(t *testing.T) { DisplayName: "scuba steve", }, }, - want: "cli/cli (trunk*): scuba steve", + want: "cli/cli [trunk*]: scuba steve", }, { name: "Included name - included gitstatus - no unsaved changes", @@ -101,7 +101,7 @@ func Test_codespace_displayName(t *testing.T) { DisplayName: "scuba steve", }, }, - want: "cli/cli (trunk): scuba steve", + want: "cli/cli [trunk]: scuba steve", }, { name: "with includeOwner true, prefixes the codespace owner", @@ -123,7 +123,7 @@ func Test_codespace_displayName(t *testing.T) { DisplayName: "scuba steve", }, }, - want: "jimmy cli/cli (trunk): scuba steve", + want: "jimmy cli/cli [trunk]: scuba steve", }, } for _, tt := range tests { @@ -163,7 +163,7 @@ func Test_formatCodespacesForSelect(t *testing.T) { }, }, wantCodespacesNames: []string{ - "cli/cli (trunk): scuba steve", + "cli/cli [trunk]: scuba steve", }, }, { @@ -191,8 +191,8 @@ func Test_formatCodespacesForSelect(t *testing.T) { }, }, wantCodespacesNames: []string{ - "cli/cli (trunk): scuba steve", - "cli/cli (trunk): flappy bird", + "cli/cli [trunk]: scuba steve", + "cli/cli [trunk]: flappy bird", }, }, { @@ -220,8 +220,8 @@ func Test_formatCodespacesForSelect(t *testing.T) { }, }, wantCodespacesNames: []string{ - "cli/cli (trunk): scuba steve", - "cli/cli (feature): flappy bird", + "cli/cli [trunk]: scuba steve", + "cli/cli [feature]: flappy bird", }, }, { @@ -249,8 +249,8 @@ func Test_formatCodespacesForSelect(t *testing.T) { }, }, wantCodespacesNames: []string{ - "github/cli (trunk): scuba steve", - "cli/cli (trunk): flappy bird", + "github/cli [trunk]: scuba steve", + "cli/cli [trunk]: flappy bird", }, }, { @@ -279,8 +279,8 @@ func Test_formatCodespacesForSelect(t *testing.T) { }, }, wantCodespacesNames: []string{ - "cli/cli (trunk): scuba steve", - "cli/cli (trunk*): flappy bird", + "cli/cli [trunk]: scuba steve", + "cli/cli [trunk*]: flappy bird", }, }, } diff --git a/pkg/cmd/issue/develop/develop.go b/pkg/cmd/issue/develop/develop.go index fa01a3dac..1536800f0 100644 --- a/pkg/cmd/issue/develop/develop.go +++ b/pkg/cmd/issue/develop/develop.go @@ -44,6 +44,13 @@ func NewCmdDevelop(f *cmdutil.Factory, runF func(*DevelopOptions) error) *cobra. cmd := &cobra.Command{ Use: "develop { | }", Short: "Manage linked branches for an issue", + Long: heredoc.Docf(` + Manage linked branches for an issue. + + When using the %[1]s--base%[1]s flag, the new development branch will be created from the specified + remote branch. The new branch will be configured as the base branch for pull requests created using + %[1]sgh pr create%[1]s. + `, "`"), Example: heredoc.Doc(` # List branches for issue 123 $ gh issue develop --list 123 @@ -171,6 +178,14 @@ func developRunCreate(opts *DevelopOptions, apiClient *api.Client, issueRepo ghr return err } + // Remember which branch to target when creating a PR. + if opts.BaseBranch != "" { + err = opts.GitClient.SetBranchConfig(ctx.Background(), branchName, git.MergeBaseConfig, opts.BaseBranch) + if err != nil { + return err + } + } + fmt.Fprintf(opts.IO.Out, "%s/%s/tree/%s\n", branchRepo.RepoHost(), ghrepo.FullName(branchRepo), branchName) return checkoutBranch(opts, branchRepo, branchName) diff --git a/pkg/cmd/issue/develop/develop_test.go b/pkg/cmd/issue/develop/develop_test.go index abdebf0c8..831f03fc3 100644 --- a/pkg/cmd/issue/develop/develop_test.go +++ b/pkg/cmd/issue/develop/develop_test.go @@ -399,6 +399,7 @@ func TestDevelopRun(t *testing.T) { }, runStubs: func(cs *run.CommandStubber) { cs.Register(`git fetch origin \+refs/heads/my-branch:refs/remotes/origin/my-branch`, 0, "") + cs.Register(`git config branch\.my-branch\.gh-merge-base main`, 0, "") }, expectedOut: "github.com/OWNER/REPO/tree/my-branch\n", }, diff --git a/pkg/cmd/pr/checks/checks.go b/pkg/cmd/pr/checks/checks.go index 797c19c19..42cf8a713 100644 --- a/pkg/cmd/pr/checks/checks.go +++ b/pkg/cmd/pr/checks/checks.go @@ -77,6 +77,10 @@ func NewCmdChecks(f *cmdutil.Factory, runF func(*ChecksOptions) error) *cobra.Co RunE: func(cmd *cobra.Command, args []string) error { opts.Finder = shared.NewFinder(f) + if opts.Exporter != nil && opts.Watch { + return cmdutil.FlagErrorf("cannot use `--watch` with `--json` flag") + } + if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 { return cmdutil.FlagErrorf("argument required when using the `--repo` flag") } diff --git a/pkg/cmd/pr/checks/checks_test.go b/pkg/cmd/pr/checks/checks_test.go index 36ebf9b15..cbb8ddc7f 100644 --- a/pkg/cmd/pr/checks/checks_test.go +++ b/pkg/cmd/pr/checks/checks_test.go @@ -78,6 +78,11 @@ func TestNewCmdChecks(t *testing.T) { cli: "--fail-fast", wantsError: "cannot use `--fail-fast` flag without `--watch` flag", }, + { + name: "watch with json flag", + cli: "--watch --json workflow", + wantsError: "cannot use `--watch` with `--json` flag", + }, { name: "required flag", cli: "--required", @@ -171,7 +176,7 @@ func Test_checksRun(t *testing.T) { wantOut: heredoc.Doc(` Some checks were not successful 0 cancelled, 1 failing, 1 successful, 0 skipped, and 1 pending checks - + NAME DESCRIPTION ELAPSED URL X sad tests 1m26s sweet link ✓ cool tests 1m26s sweet link @@ -191,7 +196,7 @@ func Test_checksRun(t *testing.T) { wantOut: heredoc.Doc(` Some checks were cancelled 1 cancelled, 0 failing, 2 successful, 0 skipped, and 0 pending checks - + NAME DESCRIPTION ELAPSED URL ✓ cool tests 1m26s sweet link - sad tests 1m26s sweet link @@ -211,7 +216,7 @@ func Test_checksRun(t *testing.T) { wantOut: heredoc.Doc(` Some checks are still pending 1 cancelled, 0 failing, 2 successful, 0 skipped, and 1 pending checks - + NAME DESCRIPTION ELAPSED URL ✓ cool tests 1m26s sweet link ✓ rad tests 1m26s sweet link @@ -232,7 +237,7 @@ func Test_checksRun(t *testing.T) { wantOut: heredoc.Doc(` All checks were successful 0 cancelled, 0 failing, 3 successful, 0 skipped, and 0 pending checks - + NAME DESCRIPTION ELAPSED URL ✓ awesome tests 1m26s sweet link ✓ cool tests 1m26s sweet link @@ -253,7 +258,7 @@ func Test_checksRun(t *testing.T) { wantOut: heredoc.Docf(` %[1]s[?1049hAll checks were successful 0 cancelled, 0 failing, 3 successful, 0 skipped, and 0 pending checks - + NAME DESCRIPTION ELAPSED URL ✓ awesome tests 1m26s sweet link ✓ cool tests 1m26s sweet link @@ -281,17 +286,17 @@ func Test_checksRun(t *testing.T) { }, wantOut: heredoc.Docf(` %[1]s[?1049h%[1]s[0;0H%[1]s[JRefreshing checks status every 0 seconds. Press Ctrl+C to quit. - + Some checks were not successful 0 cancelled, 1 failing, 1 successful, 0 skipped, and 1 pending checks - + NAME DESCRIPTION ELAPSED URL X sad tests 1m26s sweet link ✓ cool tests 1m26s sweet link * slow tests 1m26s sweet link %[1]s[?1049lSome checks were not successful 0 cancelled, 1 failing, 1 successful, 0 skipped, and 1 pending checks - + NAME DESCRIPTION ELAPSED URL X sad tests 1m26s sweet link ✓ cool tests 1m26s sweet link @@ -311,7 +316,7 @@ func Test_checksRun(t *testing.T) { wantOut: heredoc.Doc(` Some checks were not successful 0 cancelled, 1 failing, 2 successful, 0 skipped, and 0 pending checks - + NAME DESCRIPTION ELAPSED URL X a status sweet link ✓ cool tests 1m26s sweet link @@ -397,7 +402,7 @@ func Test_checksRun(t *testing.T) { wantOut: heredoc.Doc(` All checks were successful 0 cancelled, 0 failing, 1 successful, 2 skipped, and 0 pending checks - + NAME DESCRIPTION ELAPSED URL ✓ cool tests 1m26s sweet link - rad tests 1m26s sweet link @@ -429,7 +434,7 @@ func Test_checksRun(t *testing.T) { wantOut: heredoc.Doc(` All checks were successful 0 cancelled, 0 failing, 1 successful, 0 skipped, and 0 pending checks - + NAME DESCRIPTION ELAPSED URL ✓ cool tests 1m26s sweet link `), @@ -484,7 +489,7 @@ func Test_checksRun(t *testing.T) { wantOut: heredoc.Doc(` All checks were successful 0 cancelled, 0 failing, 3 successful, 0 skipped, and 0 pending checks - + NAME DESCRIPTION ELAPSED URL ✓ awesome tests awesome description 1m26s sweet link ✓ cool tests cool description 1m26s sweet link @@ -515,7 +520,7 @@ func Test_checksRun(t *testing.T) { wantOut: heredoc.Doc(` All checks were successful 0 cancelled, 0 failing, 2 successful, 0 skipped, and 0 pending checks - + NAME DESCRIPTION ELAPSED URL ✓ tests/cool tests (pull_request) cool description 1m26s sweet link ✓ tests/cool tests (push) cool description 1m26s sweet link @@ -535,7 +540,7 @@ func Test_checksRun(t *testing.T) { wantOut: heredoc.Doc(` All checks were successful 0 cancelled, 0 failing, 1 successful, 0 skipped, and 0 pending checks - + NAME DESCRIPTION ELAPSED URL ✓ tests/cool tests cool description 1m26s sweet link `), diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 06f4e1e89..f3bd12870 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -119,6 +119,10 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co alongside %[1]s--fill%[1]s, the values specified by %[1]s--title%[1]s and/or %[1]s--body%[1]s will take precedence and overwrite any autofilled content. + The base branch for the created PR can be specified using the %[1]s--base%[1]s flag. If not provided, + the value of %[1]sgh-merge-base%[1]s git branch config will be used. If not configured, the repository's + default branch will be used. + Link an issue to the pull request by referencing the issue in the body of the pull request. If the body text mentions %[1]sFixes #123%[1]s or %[1]sCloses #123%[1]s, the referenced issue will automatically get closed when the pull request gets merged. @@ -513,11 +517,10 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u return nil } -func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, headBranch string) *git.TrackingRef { +func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, headBranchConfig *git.BranchConfig) *git.TrackingRef { refsForLookup := []string{"HEAD"} var trackingRefs []git.TrackingRef - headBranchConfig := gitClient.ReadBranchConfig(context.Background(), headBranch) if headBranchConfig.RemoteName != "" { tr := git.TrackingRef{ RemoteName: headBranchConfig.RemoteName, @@ -530,7 +533,7 @@ func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, h for _, remote := range remotes { tr := git.TrackingRef{ RemoteName: remote.Name, - BranchName: headBranch, + BranchName: headBranchConfig.LocalName, } trackingRefs = append(trackingRefs, tr) refsForLookup = append(refsForLookup, tr.String()) @@ -640,9 +643,10 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { var headRepo ghrepo.Interface var headRemote *ghContext.Remote + headBranchConfig := gitClient.ReadBranchConfig(context.Background(), headBranch) if isPushEnabled { // determine whether the head branch is already pushed to a remote - if pushedTo := determineTrackingBranch(gitClient, remotes, headBranch); pushedTo != nil { + if pushedTo := determineTrackingBranch(gitClient, remotes, &headBranchConfig); pushedTo != nil { isPushEnabled = false if r, err := remotes.FindByName(pushedTo.RemoteName); err == nil { headRepo = r @@ -715,6 +719,9 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { } baseBranch := opts.BaseBranch + if baseBranch == "" { + baseBranch = headBranchConfig.MergeBase + } if baseBranch == "" { baseBranch = baseRepo.DefaultBranchRef.Name } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index d31174999..27220d052 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1,6 +1,7 @@ package create import ( + ctx "context" "encoding/json" "errors" "fmt" @@ -261,6 +262,15 @@ func TestNewCmdCreate(t *testing.T) { cli: "--editor", wantsErr: true, }, + { + name: "fill and base", + cli: "--fill --base trunk", + wantsOpts: CreateOptions{ + Autofill: true, + BaseBranch: "trunk", + MaintainerCanModify: true, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -323,17 +333,18 @@ func TestNewCmdCreate(t *testing.T) { func Test_createRun(t *testing.T) { tests := []struct { - name string - setup func(*CreateOptions, *testing.T) func() - cmdStubs func(*run.CommandStubber) - promptStubs func(*prompter.PrompterMock) - httpStubs func(*httpmock.Registry, *testing.T) - expectedOutputs []string - expectedOut string - expectedErrOut string - expectedBrowse string - wantErr string - tty bool + name string + setup func(*CreateOptions, *testing.T) func() + cmdStubs func(*run.CommandStubber) + promptStubs func(*prompter.PrompterMock) + httpStubs func(*httpmock.Registry, *testing.T) + expectedOutputs []string + expectedOut string + expectedErrOut string + expectedBrowse string + wantErr string + tty bool + customBranchConfig bool }{ { name: "nontty web", @@ -626,7 +637,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, @@ -690,7 +700,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, @@ -737,7 +746,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, @@ -787,7 +795,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register("git remote rename origin upstream", 0, "") cs.Register(`git remote add origin https://github.com/monalisa/REPO.git`, 0, "") @@ -846,7 +853,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp \^branch\\\.feature\\\.`, 1, "") // determineTrackingBranch cs.Register("git show-ref --verify", 0, heredoc.Doc(` deadbeef HEAD deadb00f refs/remotes/upstream/feature @@ -878,6 +884,7 @@ func Test_createRun(t *testing.T) { assert.Equal(t, "my-feat2", input["headRefName"].(string)) })) }, + customBranchConfig: true, cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp \^branch\\\.feature\\\.`, 0, heredoc.Doc(` branch.feature.remote origin @@ -1066,7 +1073,6 @@ func Test_createRun(t *testing.T) { httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") @@ -1099,7 +1105,6 @@ func Test_createRun(t *testing.T) { mockRetrieveProjects(t, reg) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") @@ -1464,6 +1469,65 @@ func Test_createRun(t *testing.T) { }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", }, + { + name: "gh-merge-base", + tty: true, + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "my title" + opts.Body = "my body" + opts.Branch = func() (string, error) { + return "task1", nil + } + opts.Remotes = func() (context.Remotes, error) { + return context.Remotes{ + { + Remote: &git.Remote{ + Name: "upstream", + Resolved: "base", + }, + Repo: ghrepo.New("OWNER", "REPO"), + }, + { + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("monalisa", "REPO"), + }, + }, nil + } + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`mutation PullRequestCreate\b`), + httpmock.GraphQLMutation(` + { "data": { "createPullRequest": { "pullRequest": { + "URL": "https://github.com/OWNER/REPO/pull/12" + } } } } + `, func(input map[string]interface{}) { + assert.Equal(t, "REPOID", input["repositoryId"].(string)) + assert.Equal(t, "my title", input["title"].(string)) + assert.Equal(t, "my body", input["body"].(string)) + assert.Equal(t, "feature/feat2", input["baseRefName"].(string)) + assert.Equal(t, "monalisa:task1", input["headRefName"].(string)) + })) + }, + customBranchConfig: true, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config --get-regexp \^branch\\\.task1\\\.\(remote\|merge\|gh-merge-base\)\$`, 0, heredoc.Doc(` + branch.task1.remote origin + branch.task1.merge refs/heads/task1 + branch.task1.gh-merge-base feature/feat2`)) // ReadBranchConfig + cs.Register(`git show-ref --verify`, 0, heredoc.Doc(` + deadbeef HEAD + deadb00f refs/remotes/upstream/feature/feat2 + deadbeef refs/remotes/origin/task1`)) // determineTrackingBranch + }, + expectedOut: "https://github.com/OWNER/REPO/pull/12\n", + expectedErrOut: "\nCreating pull request for monalisa:task1 into feature/feat2 in OWNER/REPO\n\n", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1485,6 +1549,9 @@ func Test_createRun(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) cs.Register(`git status --porcelain`, 0, "") + if !tt.customBranchConfig { + cs.Register(`git config --get-regexp \^branch\\\..+\\\.\(remote\|merge\|gh-merge-base\)\$`, 0, "") + } if tt.cmdStubs != nil { tt.cmdStubs(cs) @@ -1651,7 +1718,8 @@ func Test_determineTrackingBranch(t *testing.T) { GhPath: "some/path/gh", GitPath: "some/path/git", } - ref := determineTrackingBranch(gitClient, tt.remotes, "feature") + headBranchConfig := gitClient.ReadBranchConfig(ctx.Background(), "feature") + ref := determineTrackingBranch(gitClient, tt.remotes, &headBranchConfig) tt.assert(ref, t) }) } diff --git a/pkg/cmd/repo/rename/rename.go b/pkg/cmd/repo/rename/rename.go index 3676f93fe..74523e2e7 100644 --- a/pkg/cmd/repo/rename/rename.go +++ b/pkg/cmd/repo/rename/rename.go @@ -49,9 +49,27 @@ func NewCmdRename(f *cmdutil.Factory, runf func(*RenameOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "rename []", Short: "Rename a repository", - Long: heredoc.Doc(`Rename a GitHub repository. + Long: heredoc.Docf(` + Rename a GitHub repository. + + %[1]s%[1]s is the desired repository name without the owner. + + By default, the current repository is renamed. Otherwise, the repository specified + with %[1]s--repo%[1]s is renamed. + + To transfer repository ownership to another user account or organization, + you must follow additional steps on GitHub.com - By default, this renames the current repository; otherwise renames the specified repository.`), + For more information on transferring repository ownership, see: + + `, "`"), + Example: heredoc.Doc(` + # Rename the current repository (foo/bar -> foo/baz) + $ gh repo rename baz + + # Rename the specified repository (qux/quux -> qux/baz) + $ gh repo rename -R qux/quux baz + `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.BaseRepo = f.BaseRepo diff --git a/pkg/cmd/run/cancel/cancel_test.go b/pkg/cmd/run/cancel/cancel_test.go index d3f580870..c894f4812 100644 --- a/pkg/cmd/run/cancel/cancel_test.go +++ b/pkg/cmd/run/cancel/cancel_test.go @@ -196,9 +196,9 @@ func TestRunCancel(t *testing.T) { }, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Select a workflow run", - []string{"* cool commit, CI (trunk) Feb 23, 2021"}, + []string{"* cool commit, CI [trunk] Feb 23, 2021"}, func(_, _ string, opts []string) (int, error) { - return prompter.IndexFor(opts, "* cool commit, CI (trunk) Feb 23, 2021") + return prompter.IndexFor(opts, "* cool commit, CI [trunk] Feb 23, 2021") }) }, wantOut: "✓ Request to cancel workflow 1234 submitted.\n", diff --git a/pkg/cmd/run/delete/delete_test.go b/pkg/cmd/run/delete/delete_test.go index 5df69aa60..b9ed2f6a0 100644 --- a/pkg/cmd/run/delete/delete_test.go +++ b/pkg/cmd/run/delete/delete_test.go @@ -132,7 +132,7 @@ func TestRunDelete(t *testing.T) { }, prompterStubs: func(pm *prompter.PrompterMock) { pm.SelectFunc = func(_, _ string, opts []string) (int, error) { - return prompter.IndexFor(opts, "✓ cool commit, CI (trunk) Feb 23, 2021") + return prompter.IndexFor(opts, "✓ cool commit, CI [trunk] Feb 23, 2021") } }, httpStubs: func(reg *httpmock.Registry) { diff --git a/pkg/cmd/run/download/download.go b/pkg/cmd/run/download/download.go index 99ec45bbe..6190325b9 100644 --- a/pkg/cmd/run/download/download.go +++ b/pkg/cmd/run/download/download.go @@ -6,6 +6,7 @@ import ( "path/filepath" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/safepaths" "github.com/cli/cli/v2/pkg/cmd/run/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" @@ -27,8 +28,9 @@ type DownloadOptions struct { type platform interface { List(runID string) ([]shared.Artifact, error) - Download(url string, dir string) error + Download(url string, dir safepaths.Absolute) error } + type iprompter interface { MultiSelect(string, []string, []string) ([]int, error) } @@ -151,8 +153,15 @@ func runDownload(opts *DownloadOptions) error { opts.IO.StartProgressIndicator() defer opts.IO.StopProgressIndicator() - // track downloaded artifacts and avoid re-downloading any of the same name + // track downloaded artifacts and avoid re-downloading any of the same name, isolate if multiple artifacts downloaded := set.NewStringSet() + isolateArtifacts := isolateArtifacts(wantNames, wantPatterns) + + absoluteDestinationDir, err := safepaths.ParseAbsolute(opts.DestinationDir) + if err != nil { + return fmt.Errorf("error parsing destination directory: %w", err) + } + for _, a := range artifacts { if a.Expired { continue @@ -165,10 +174,19 @@ func runDownload(opts *DownloadOptions) error { continue } } - destDir := opts.DestinationDir - if len(wantPatterns) != 0 || len(wantNames) != 1 { - destDir = filepath.Join(destDir, a.Name) + + destDir := absoluteDestinationDir + if isolateArtifacts { + destDir, err = absoluteDestinationDir.Join(a.Name) + if err != nil { + var pathTraversalError safepaths.PathTraversalError + if errors.As(err, &pathTraversalError) { + return fmt.Errorf("error downloading %s: would result in path traversal", a.Name) + } + return err + } } + err := opts.Platform.Download(a.DownloadURL, destDir) if err != nil { return fmt.Errorf("error downloading %s: %w", a.Name, err) @@ -183,6 +201,25 @@ func runDownload(opts *DownloadOptions) error { return nil } +func isolateArtifacts(wantNames []string, wantPatterns []string) bool { + if len(wantPatterns) > 0 { + // Patterns can match multiple artifacts + return true + } + + if len(wantNames) == 0 { + // All artifacts wanted regardless what they are named + return true + } + + if len(wantNames) > 1 { + // Multiple, specific artifacts wanted + return true + } + + return false +} + func matchAnyName(names []string, name string) bool { for _, n := range names { if name == n { diff --git a/pkg/cmd/run/download/download_test.go b/pkg/cmd/run/download/download_test.go index 3c1c8f2d8..e0f5eb781 100644 --- a/pkg/cmd/run/download/download_test.go +++ b/pkg/cmd/run/download/download_test.go @@ -2,19 +2,22 @@ package download import ( "bytes" + "errors" + "fmt" "io" "net/http" + "os" "path/filepath" "testing" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/internal/safepaths" "github.com/cli/cli/v2/pkg/cmd/run/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -143,159 +146,584 @@ func Test_NewCmdDownload(t *testing.T) { } } +type run struct { + id string + testArtifacts []testArtifact +} + +type testArtifact struct { + artifact shared.Artifact + files []string +} + +type fakePlatform struct { + runs []run +} + +func (f *fakePlatform) List(runID string) ([]shared.Artifact, error) { + runIds := map[string]struct{}{} + if runID != "" { + runIds[runID] = struct{}{} + } else { + for _, run := range f.runs { + runIds[run.id] = struct{}{} + } + } + + var artifacts []shared.Artifact + for _, run := range f.runs { + // Skip over any runs that we aren't looking for + if _, ok := runIds[run.id]; !ok { + continue + } + + // Grab the artifacts of everything else + for _, testArtifact := range run.testArtifacts { + artifacts = append(artifacts, testArtifact.artifact) + } + } + + return artifacts, nil +} + +func (f *fakePlatform) Download(url string, dir safepaths.Absolute) error { + if err := os.MkdirAll(dir.String(), 0755); err != nil { + return err + } + // Now to be consistent, we find the artifact with the provided URL. + // It's a bit janky to iterate the runs, to find the right artifact + // rather than keying directly to it, but it allows the setup of the + // fake platform to be declarative rather than imperative. + // Think fakePlatform { artifacts: ... } rather than fakePlatform.makeArtifactAvailable() + for _, run := range f.runs { + for _, testArtifact := range run.testArtifacts { + if testArtifact.artifact.DownloadURL == url { + for _, file := range testArtifact.files { + path := filepath.Join(dir.String(), file) + return os.WriteFile(path, []byte{}, 0600) + } + } + } + } + + return errors.New("no artifact matches the provided URL") +} + func Test_runDownload(t *testing.T) { tests := []struct { - name string - opts DownloadOptions - mockAPI func(*mockPlatform) - promptStubs func(*prompter.MockPrompter) - wantErr string + name string + opts DownloadOptions + platform *fakePlatform + promptStubs func(*prompter.MockPrompter) + expectedFiles []string + wantErr string }{ { - name: "download non-expired", + name: "download non-expired to relative directory", opts: DownloadOptions{ RunID: "2345", DestinationDir: "./tmp", - Names: []string(nil), }, - mockAPI: func(p *mockPlatform) { - p.On("List", "2345").Return([]shared.Artifact{ + platform: &fakePlatform{ + runs: []run{ { - Name: "artifact-1", - DownloadURL: "http://download.com/artifact1.zip", - Expired: false, + id: "2345", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: false, + }, + files: []string{ + "artifact-1-file", + }, + }, + { + artifact: shared.Artifact{ + Name: "expired-artifact", + DownloadURL: "http://download.com/expired.zip", + Expired: true, + }, + files: []string{ + "expired", + }, + }, + { + artifact: shared.Artifact{ + Name: "artifact-2", + DownloadURL: "http://download.com/artifact2.zip", + Expired: false, + }, + files: []string{ + "artifact-2-file", + }, + }, + }, }, - { - Name: "expired-artifact", - DownloadURL: "http://download.com/expired.zip", - Expired: true, - }, - { - Name: "artifact-2", - DownloadURL: "http://download.com/artifact2.zip", - Expired: false, - }, - }, nil) - p.On("Download", "http://download.com/artifact1.zip", filepath.FromSlash("tmp/artifact-1")).Return(nil) - p.On("Download", "http://download.com/artifact2.zip", filepath.FromSlash("tmp/artifact-2")).Return(nil) + }, + }, + expectedFiles: []string{ + filepath.Join("artifact-1", "artifact-1-file"), + filepath.Join("artifact-2", "artifact-2-file"), }, }, { - name: "no valid artifacts", + name: "download non-expired to absolute directory", opts: DownloadOptions{ RunID: "2345", - DestinationDir: ".", - Names: []string(nil), + DestinationDir: "/tmp", }, - mockAPI: func(p *mockPlatform) { - p.On("List", "2345").Return([]shared.Artifact{ + platform: &fakePlatform{ + runs: []run{ { - Name: "artifact-1", - DownloadURL: "http://download.com/artifact1.zip", - Expired: true, + id: "2345", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: false, + }, + files: []string{ + "artifact-1-file", + }, + }, + { + artifact: shared.Artifact{ + Name: "expired-artifact", + DownloadURL: "http://download.com/expired.zip", + Expired: true, + }, + files: []string{ + "expired", + }, + }, + { + artifact: shared.Artifact{ + Name: "artifact-2", + DownloadURL: "http://download.com/artifact2.zip", + Expired: false, + }, + files: []string{ + "artifact-2-file", + }, + }, + }, }, - { - Name: "artifact-2", - DownloadURL: "http://download.com/artifact2.zip", - Expired: true, - }, - }, nil) + }, }, - wantErr: "no valid artifacts found to download", + expectedFiles: []string{ + filepath.Join("artifact-1", "artifact-1-file"), + filepath.Join("artifact-2", "artifact-2-file"), + }, + }, + { + name: "all artifacts are expired", + opts: DownloadOptions{ + RunID: "2345", + }, + platform: &fakePlatform{ + runs: []run{ + { + id: "2345", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: true, + }, + files: []string{ + "artifact-1-file", + }, + }, + { + artifact: shared.Artifact{ + Name: "artifact-2", + DownloadURL: "http://download.com/artifact2.zip", + Expired: true, + }, + files: []string{ + "artifact-2-file", + }, + }, + }, + }, + }, + }, + expectedFiles: []string{}, + wantErr: "no valid artifacts found to download", }, { name: "no name matches", opts: DownloadOptions{ - RunID: "2345", - DestinationDir: ".", - Names: []string{"artifact-3"}, + RunID: "2345", + Names: []string{"artifact-3"}, }, - mockAPI: func(p *mockPlatform) { - p.On("List", "2345").Return([]shared.Artifact{ + platform: &fakePlatform{ + runs: []run{ { - Name: "artifact-1", - DownloadURL: "http://download.com/artifact1.zip", - Expired: false, + id: "2345", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: false, + }, + files: []string{ + "artifact-1-file", + }, + }, + { + artifact: shared.Artifact{ + Name: "artifact-2", + DownloadURL: "http://download.com/artifact2.zip", + Expired: false, + }, + files: []string{ + "artifact-2-file", + }, + }, + }, }, - { - Name: "artifact-2", - DownloadURL: "http://download.com/artifact2.zip", - Expired: false, - }, - }, nil) + }, + }, + expectedFiles: []string{}, + wantErr: "no artifact matches any of the names or patterns provided", + }, + { + name: "pattern matches", + opts: DownloadOptions{ + RunID: "2345", + FilePatterns: []string{"artifact-*"}, + }, + platform: &fakePlatform{ + runs: []run{ + { + id: "2345", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: false, + }, + files: []string{ + "artifact-1-file", + }, + }, + { + artifact: shared.Artifact{ + Name: "non-artifact-2", + DownloadURL: "http://download.com/non-artifact-2.zip", + Expired: false, + }, + files: []string{ + "non-artifact-2-file", + }, + }, + { + artifact: shared.Artifact{ + Name: "artifact-3", + DownloadURL: "http://download.com/artifact3.zip", + Expired: false, + }, + files: []string{ + "artifact-3-file", + }, + }, + }, + }, + }, + }, + expectedFiles: []string{ + filepath.Join("artifact-1", "artifact-1-file"), + filepath.Join("artifact-3", "artifact-3-file"), }, - wantErr: "no artifact matches any of the names or patterns provided", }, { name: "no pattern matches", opts: DownloadOptions{ - RunID: "2345", - DestinationDir: ".", - FilePatterns: []string{"artifiction-*"}, + RunID: "2345", + FilePatterns: []string{"artifiction-*"}, }, - mockAPI: func(p *mockPlatform) { - p.On("List", "2345").Return([]shared.Artifact{ + platform: &fakePlatform{ + runs: []run{ { - Name: "artifact-1", - DownloadURL: "http://download.com/artifact1.zip", - Expired: false, + id: "2345", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: false, + }, + files: []string{ + "artifact-1-file", + }, + }, + { + artifact: shared.Artifact{ + Name: "artifact-2", + DownloadURL: "http://download.com/artifact2.zip", + Expired: false, + }, + files: []string{ + "artifact-2-file", + }, + }, + }, }, - { - Name: "artifact-2", - DownloadURL: "http://download.com/artifact2.zip", - Expired: false, - }, - }, nil) + }, + }, + expectedFiles: []string{}, + wantErr: "no artifact matches any of the names or patterns provided", + }, + { + name: "want specific single artifact", + opts: DownloadOptions{ + RunID: "2345", + Names: []string{"non-artifact-2"}, + }, + platform: &fakePlatform{ + runs: []run{ + { + id: "2345", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: false, + }, + files: []string{ + "artifact-1-file", + }, + }, + { + artifact: shared.Artifact{ + Name: "non-artifact-2", + DownloadURL: "http://download.com/non-artifact-2.zip", + Expired: false, + }, + files: []string{ + "non-artifact-2-file", + }, + }, + { + artifact: shared.Artifact{ + Name: "artifact-3", + DownloadURL: "http://download.com/artifact3.zip", + Expired: false, + }, + files: []string{ + "artifact-3-file", + }, + }, + }, + }, + }, + }, + expectedFiles: []string{ + filepath.Join("non-artifact-2-file"), + }, + }, + { + name: "want specific multiple artifacts", + opts: DownloadOptions{ + RunID: "2345", + Names: []string{"artifact-1", "artifact-3"}, + }, + platform: &fakePlatform{ + runs: []run{ + { + id: "2345", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: false, + }, + files: []string{ + "artifact-1-file", + }, + }, + { + artifact: shared.Artifact{ + Name: "non-artifact-2", + DownloadURL: "http://download.com/non-artifact-2.zip", + Expired: false, + }, + files: []string{ + "non-artifact-2-file", + }, + }, + { + artifact: shared.Artifact{ + Name: "artifact-3", + DownloadURL: "http://download.com/artifact3.zip", + Expired: false, + }, + files: []string{ + "artifact-3-file", + }, + }, + }, + }, + }, + }, + expectedFiles: []string{ + filepath.Join("artifact-1", "artifact-1-file"), + filepath.Join("artifact-3", "artifact-3-file"), + }, + }, + { + name: "avoid redownloading files of the same name", + opts: DownloadOptions{ + RunID: "2345", + }, + platform: &fakePlatform{ + runs: []run{ + { + id: "2345", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: false, + }, + files: []string{ + "artifact-1-file", + }, + }, + { + artifact: shared.Artifact{ + Name: "artifact-1", + DownloadURL: "http://download.com/artifact2.zip", + Expired: false, + }, + files: []string{ + "artifact-2-file", + }, + }, + }, + }, + }, + }, + expectedFiles: []string{ + filepath.Join("artifact-1", "artifact-1-file"), }, - wantErr: "no artifact matches any of the names or patterns provided", }, { name: "prompt to select artifact", opts: DownloadOptions{ - RunID: "", - DoPrompt: true, - DestinationDir: ".", - Names: []string(nil), + RunID: "", + DoPrompt: true, + Names: []string(nil), }, - mockAPI: func(p *mockPlatform) { - p.On("List", "").Return([]shared.Artifact{ + platform: &fakePlatform{ + runs: []run{ { - Name: "artifact-1", - DownloadURL: "http://download.com/artifact1.zip", - Expired: false, + id: "2345", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + Expired: false, + }, + files: []string{ + "artifact-1-file", + }, + }, + { + artifact: shared.Artifact{ + Name: "expired-artifact", + DownloadURL: "http://download.com/expired.zip", + Expired: true, + }, + files: []string{ + "expired", + }, + }, + }, }, { - Name: "expired-artifact", - DownloadURL: "http://download.com/expired.zip", - Expired: true, + id: "6789", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "artifact-2", + DownloadURL: "http://download.com/artifact2.zip", + Expired: false, + }, + files: []string{ + "artifact-2-file", + }, + }, + }, }, - { - Name: "artifact-2", - DownloadURL: "http://download.com/artifact2.zip", - Expired: false, - }, - { - Name: "artifact-2", - DownloadURL: "http://download.com/artifact2.also.zip", - Expired: false, - }, - }, nil) - p.On("Download", "http://download.com/artifact2.zip", ".").Return(nil) + }, }, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterMultiSelect("Select artifacts to download:", nil, []string{"artifact-1", "artifact-2"}, func(_ string, _, opts []string) ([]int, error) { - return []int{1}, nil + for i, o := range opts { + if o == "artifact-2" { + return []int{i}, nil + } + } + return nil, fmt.Errorf("no artifact-2 found in %v", opts) }) }, + expectedFiles: []string{ + filepath.Join("artifact-2-file"), + }, + }, + { + name: "handling artifact name with path traversal exploit", + opts: DownloadOptions{ + RunID: "2345", + }, + platform: &fakePlatform{ + runs: []run{ + { + id: "2345", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "..", + DownloadURL: "http://download.com/artifact1.zip", + Expired: false, + }, + files: []string{ + "etc/passwd", + }, + }, + }, + }, + }, + }, + expectedFiles: []string{}, + wantErr: "error downloading ..: would result in path traversal", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { opts := &tt.opts + if opts.DestinationDir == "" { + opts.DestinationDir = t.TempDir() + } else { + opts.DestinationDir = filepath.Join(t.TempDir(), opts.DestinationDir) + } + ios, _, stdout, stderr := iostreams.Test() opts.IO = ios - opts.Platform = newMockPlatform(t, tt.mockAPI) + opts.Platform = tt.platform pm := prompter.NewMockPrompter(t) opts.Prompter = pm @@ -310,34 +738,31 @@ func Test_runDownload(t *testing.T) { require.NoError(t, err) } + // Check that the exact number of files exist + require.Equal(t, len(tt.expectedFiles), countFilesInDirRecursively(t, opts.DestinationDir)) + + // Then check that the exact files are correct + for _, name := range tt.expectedFiles { + require.FileExists(t, filepath.Join(opts.DestinationDir, name)) + } + assert.Equal(t, "", stdout.String()) assert.Equal(t, "", stderr.String()) }) } } -type mockPlatform struct { - mock.Mock -} +func countFilesInDirRecursively(t *testing.T, dir string) int { + t.Helper() -func newMockPlatform(t *testing.T, config func(*mockPlatform)) *mockPlatform { - m := &mockPlatform{} - m.Test(t) - t.Cleanup(func() { - m.AssertExpectations(t) - }) - if config != nil { - config(m) - } - return m -} + count := 0 + require.NoError(t, filepath.Walk(dir, func(_ string, info os.FileInfo, err error) error { + require.NoError(t, err) + if !info.IsDir() { + count++ + } + return nil + })) -func (p *mockPlatform) List(runID string) ([]shared.Artifact, error) { - args := p.Called(runID) - return args.Get(0).([]shared.Artifact), args.Error(1) -} - -func (p *mockPlatform) Download(url string, dir string) error { - args := p.Called(url, dir) - return args.Error(0) + return count } diff --git a/pkg/cmd/run/download/http.go b/pkg/cmd/run/download/http.go index 05f6ebcb9..783c8495e 100644 --- a/pkg/cmd/run/download/http.go +++ b/pkg/cmd/run/download/http.go @@ -9,6 +9,7 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/safepaths" "github.com/cli/cli/v2/pkg/cmd/run/shared" ) @@ -21,11 +22,11 @@ func (p *apiPlatform) List(runID string) ([]shared.Artifact, error) { return shared.ListArtifacts(p.client, p.repo, runID) } -func (p *apiPlatform) Download(url string, dir string) error { +func (p *apiPlatform) Download(url string, dir safepaths.Absolute) error { return downloadArtifact(p.client, url, dir) } -func downloadArtifact(httpClient *http.Client, url, destDir string) error { +func downloadArtifact(httpClient *http.Client, url string, destDir safepaths.Absolute) error { req, err := http.NewRequest("GET", url, nil) if err != nil { return err diff --git a/pkg/cmd/run/download/http_test.go b/pkg/cmd/run/download/http_test.go index 2752d88ad..75b52ae79 100644 --- a/pkg/cmd/run/download/http_test.go +++ b/pkg/cmd/run/download/http_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/safepaths" "github.com/cli/cli/v2/pkg/httpmock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -58,7 +59,8 @@ func Test_List_perRepository(t *testing.T) { func Test_Download(t *testing.T) { tmpDir := t.TempDir() - destDir := filepath.Join(tmpDir, "artifact") + destDir, err := safepaths.ParseAbsolute(filepath.Join(tmpDir, "artifact")) + require.NoError(t, err) reg := &httpmock.Registry{} defer reg.Verify(t) @@ -70,8 +72,7 @@ func Test_Download(t *testing.T) { api := &apiPlatform{ client: &http.Client{Transport: reg}, } - err := api.Download("https://api.github.com/repos/OWNER/REPO/actions/artifacts/12345/zip", destDir) - require.NoError(t, err) + require.NoError(t, api.Download("https://api.github.com/repos/OWNER/REPO/actions/artifacts/12345/zip", destDir)) var paths []string parentPrefix := tmpDir + string(filepath.Separator) diff --git a/pkg/cmd/run/download/zip.go b/pkg/cmd/run/download/zip.go index ab5723e94..bb504dde1 100644 --- a/pkg/cmd/run/download/zip.go +++ b/pkg/cmd/run/download/zip.go @@ -2,11 +2,13 @@ package download import ( "archive/zip" + "errors" "fmt" "io" "os" "path/filepath" - "strings" + + "github.com/cli/cli/v2/internal/safepaths" ) const ( @@ -15,12 +17,17 @@ const ( execMode os.FileMode = 0755 ) -func extractZip(zr *zip.Reader, destDir string) error { +func extractZip(zr *zip.Reader, destDir safepaths.Absolute) error { for _, zf := range zr.File { - fpath := filepath.Join(destDir, filepath.FromSlash(zf.Name)) - if !filepathDescendsFrom(fpath, destDir) { - continue + fpath, err := destDir.Join(zf.Name) + if err != nil { + var pathTraversalError safepaths.PathTraversalError + if errors.As(err, &pathTraversalError) { + continue + } + return err } + if err := extractZipFile(zf, fpath); err != nil { return fmt.Errorf("error extracting %q: %w", zf.Name, err) } @@ -28,10 +35,10 @@ func extractZip(zr *zip.Reader, destDir string) error { return nil } -func extractZipFile(zf *zip.File, dest string) (extractErr error) { +func extractZipFile(zf *zip.File, dest safepaths.Absolute) (extractErr error) { zm := zf.Mode() if zm.IsDir() { - extractErr = os.MkdirAll(dest, dirMode) + extractErr = os.MkdirAll(dest.String(), dirMode) return } @@ -42,14 +49,12 @@ func extractZipFile(zf *zip.File, dest string) (extractErr error) { } defer f.Close() - if dir := filepath.Dir(dest); dir != "." { - if extractErr = os.MkdirAll(dir, dirMode); extractErr != nil { - return - } + if extractErr = os.MkdirAll(filepath.Dir(dest.String()), dirMode); extractErr != nil { + return } var df *os.File - if df, extractErr = os.OpenFile(dest, os.O_WRONLY|os.O_CREATE|os.O_EXCL, getPerm(zm)); extractErr != nil { + if df, extractErr = os.OpenFile(dest.String(), os.O_WRONLY|os.O_CREATE|os.O_EXCL, getPerm(zm)); extractErr != nil { return } @@ -69,15 +74,3 @@ func getPerm(m os.FileMode) os.FileMode { } return execMode } - -func filepathDescendsFrom(p, dir string) bool { - p = filepath.Clean(p) - dir = filepath.Clean(dir) - if dir == "." && !filepath.IsAbs(p) { - return !strings.HasPrefix(p, ".."+string(filepath.Separator)) - } - if !strings.HasSuffix(dir, string(filepath.Separator)) { - dir += string(filepath.Separator) - } - return strings.HasPrefix(p, dir) -} diff --git a/pkg/cmd/run/download/zip_test.go b/pkg/cmd/run/download/zip_test.go index ca401cdb9..2584371b4 100644 --- a/pkg/cmd/run/download/zip_test.go +++ b/pkg/cmd/run/download/zip_test.go @@ -6,12 +6,14 @@ import ( "path/filepath" "testing" + "github.com/cli/cli/v2/internal/safepaths" "github.com/stretchr/testify/require" ) func Test_extractZip(t *testing.T) { tmpDir := t.TempDir() - extractPath := filepath.Join(tmpDir, "artifact") + extractPath, err := safepaths.ParseAbsolute(filepath.Join(tmpDir, "artifact")) + require.NoError(t, err) zipFile, err := zip.OpenReader("./fixtures/myproject.zip") require.NoError(t, err) @@ -20,122 +22,6 @@ func Test_extractZip(t *testing.T) { err = extractZip(&zipFile.Reader, extractPath) require.NoError(t, err) - _, err = os.Stat(filepath.Join(extractPath, "src", "main.go")) + _, err = os.Stat(filepath.Join(extractPath.String(), "src", "main.go")) require.NoError(t, err) } - -func Test_filepathDescendsFrom(t *testing.T) { - type args struct { - p string - dir string - } - tests := []struct { - name string - args args - want bool - }{ - { - name: "root child", - args: args{ - p: filepath.FromSlash("/hoi.txt"), - dir: filepath.FromSlash("/"), - }, - want: true, - }, - { - name: "abs descendant", - args: args{ - p: filepath.FromSlash("/var/logs/hoi.txt"), - dir: filepath.FromSlash("/"), - }, - want: true, - }, - { - name: "abs trailing slash", - args: args{ - p: filepath.FromSlash("/var/logs/hoi.txt"), - dir: filepath.FromSlash("/var/logs/"), - }, - want: true, - }, - { - name: "abs mismatch", - args: args{ - p: filepath.FromSlash("/var/logs/hoi.txt"), - dir: filepath.FromSlash("/var/pids"), - }, - want: false, - }, - { - name: "abs partial prefix", - args: args{ - p: filepath.FromSlash("/var/logs/hoi.txt"), - dir: filepath.FromSlash("/var/log"), - }, - want: false, - }, - { - name: "rel child", - args: args{ - p: filepath.FromSlash("hoi.txt"), - dir: filepath.FromSlash("."), - }, - want: true, - }, - { - name: "rel descendant", - args: args{ - p: filepath.FromSlash("./log/hoi.txt"), - dir: filepath.FromSlash("."), - }, - want: true, - }, - { - name: "mixed rel styles", - args: args{ - p: filepath.FromSlash("./log/hoi.txt"), - dir: filepath.FromSlash("log"), - }, - want: true, - }, - { - name: "rel clean", - args: args{ - p: filepath.FromSlash("cats/../dogs/pug.txt"), - dir: filepath.FromSlash("dogs"), - }, - want: true, - }, - { - name: "rel mismatch", - args: args{ - p: filepath.FromSlash("dogs/pug.txt"), - dir: filepath.FromSlash("dog"), - }, - want: false, - }, - { - name: "rel breakout", - args: args{ - p: filepath.FromSlash("../escape.txt"), - dir: filepath.FromSlash("."), - }, - want: false, - }, - { - name: "rel sneaky breakout", - args: args{ - p: filepath.FromSlash("dogs/../../escape.txt"), - dir: filepath.FromSlash("dogs"), - }, - want: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := filepathDescendsFrom(tt.args.p, tt.args.dir); got != tt.want { - t.Errorf("filepathDescendsFrom() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/pkg/cmd/run/rerun/rerun_test.go b/pkg/cmd/run/rerun/rerun_test.go index aac5c0797..0dc74129e 100644 --- a/pkg/cmd/run/rerun/rerun_test.go +++ b/pkg/cmd/run/rerun/rerun_test.go @@ -341,7 +341,7 @@ func TestRerun(t *testing.T) { }, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Select a workflow run", - []string{"X cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021"}, + []string{"X cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021"}, func(_, _ string, opts []string) (int, error) { return 2, nil }) diff --git a/pkg/cmd/run/shared/shared.go b/pkg/cmd/run/shared/shared.go index 90ab5a7fe..51eb547bc 100644 --- a/pkg/cmd/run/shared/shared.go +++ b/pkg/cmd/run/shared/shared.go @@ -508,7 +508,7 @@ func SelectRun(p Prompter, cs *iostreams.ColorScheme, runs []Run) (string, error symbol, _ := Symbol(cs, run.Status, run.Conclusion) candidates = append(candidates, // TODO truncate commit message, long ones look terrible - fmt.Sprintf("%s %s, %s (%s) %s", symbol, run.Title(), run.WorkflowName(), run.HeadBranch, preciseAgo(now, run.StartedTime()))) + fmt.Sprintf("%s %s, %s [%s] %s", symbol, run.Title(), run.WorkflowName(), run.HeadBranch, preciseAgo(now, run.StartedTime()))) } selected, err := p.Select("Select a workflow run", "", candidates) diff --git a/pkg/cmd/run/view/view_test.go b/pkg/cmd/run/view/view_test.go index 05eb25754..3a04fb186 100644 --- a/pkg/cmd/run/view/view_test.go +++ b/pkg/cmd/run/view/view_test.go @@ -543,9 +543,9 @@ func TestViewRun(t *testing.T) { }, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Select a workflow run", - []string{"X cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "✓ cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021"}, + []string{"X cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "✓ cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021"}, func(_, _ string, opts []string) (int, error) { - return prompter.IndexFor(opts, "✓ cool commit, CI (trunk) Feb 23, 2021") + return prompter.IndexFor(opts, "✓ cool commit, CI [trunk] Feb 23, 2021") }) }, opts: &ViewOptions{ @@ -593,9 +593,9 @@ func TestViewRun(t *testing.T) { }, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Select a workflow run", - []string{"X cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "✓ cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021"}, + []string{"X cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "✓ cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021"}, func(_, _ string, opts []string) (int, error) { - return prompter.IndexFor(opts, "✓ cool commit, CI (trunk) Feb 23, 2021") + return prompter.IndexFor(opts, "✓ cool commit, CI [trunk] Feb 23, 2021") }) pm.RegisterSelect("View a specific job in this run?", []string{"View all jobs in this run", "✓ cool job", "X sad job"}, @@ -646,9 +646,9 @@ func TestViewRun(t *testing.T) { }, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Select a workflow run", - []string{"X cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "✓ cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021"}, + []string{"X cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "✓ cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021"}, func(_, _ string, opts []string) (int, error) { - return prompter.IndexFor(opts, "✓ cool commit, CI (trunk) Feb 23, 2021") + return prompter.IndexFor(opts, "✓ cool commit, CI [trunk] Feb 23, 2021") }) pm.RegisterSelect("View a specific job in this run?", []string{"View all jobs in this run", "✓ cool job", "X sad job"}, @@ -743,9 +743,9 @@ func TestViewRun(t *testing.T) { }, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Select a workflow run", - []string{"X cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "✓ cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021"}, + []string{"X cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "✓ cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021"}, func(_, _ string, opts []string) (int, error) { - return prompter.IndexFor(opts, "✓ cool commit, CI (trunk) Feb 23, 2021") + return prompter.IndexFor(opts, "✓ cool commit, CI [trunk] Feb 23, 2021") }) pm.RegisterSelect("View a specific job in this run?", []string{"View all jobs in this run", "✓ cool job", "X sad job"}, @@ -823,7 +823,7 @@ func TestViewRun(t *testing.T) { }, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Select a workflow run", - []string{"X cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "✓ cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021"}, + []string{"X cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "✓ cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021"}, func(_, _ string, opts []string) (int, error) { return 4, nil }) @@ -876,7 +876,7 @@ func TestViewRun(t *testing.T) { }, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Select a workflow run", - []string{"X cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "✓ cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021"}, + []string{"X cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "✓ cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021"}, func(_, _ string, opts []string) (int, error) { return 4, nil }) @@ -950,7 +950,7 @@ func TestViewRun(t *testing.T) { }, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Select a workflow run", - []string{"X cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "✓ cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021"}, + []string{"X cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "✓ cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021"}, func(_, _ string, opts []string) (int, error) { return 4, nil }) @@ -1104,9 +1104,9 @@ func TestViewRun(t *testing.T) { }, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Select a workflow run", - []string{"X cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "✓ cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021"}, + []string{"X cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "✓ cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021"}, func(_, _ string, opts []string) (int, error) { - return prompter.IndexFor(opts, "✓ cool commit, CI (trunk) Feb 23, 2021") + return prompter.IndexFor(opts, "✓ cool commit, CI [trunk] Feb 23, 2021") }) pm.RegisterSelect("View a specific job in this run?", []string{"View all jobs in this run", "✓ cool job", "X sad job"}, @@ -1155,9 +1155,9 @@ func TestViewRun(t *testing.T) { }, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Select a workflow run", - []string{"X cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "✓ cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "- cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "* cool commit, CI (trunk) Feb 23, 2021", "X cool commit, CI (trunk) Feb 23, 2021"}, + []string{"X cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "✓ cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "- cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "* cool commit, CI [trunk] Feb 23, 2021", "X cool commit, CI [trunk] Feb 23, 2021"}, func(_, _ string, opts []string) (int, error) { - return prompter.IndexFor(opts, "✓ cool commit, CI (trunk) Feb 23, 2021") + return prompter.IndexFor(opts, "✓ cool commit, CI [trunk] Feb 23, 2021") }) pm.RegisterSelect("View a specific job in this run?", []string{"View all jobs in this run", "✓ cool job", "X sad job"}, diff --git a/pkg/cmd/run/watch/watch_test.go b/pkg/cmd/run/watch/watch_test.go index 442ef4252..d42e8d3d8 100644 --- a/pkg/cmd/run/watch/watch_test.go +++ b/pkg/cmd/run/watch/watch_test.go @@ -272,9 +272,9 @@ func TestWatchRun(t *testing.T) { httpStubs: successfulRunStubs, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Select a workflow run", - []string{"* commit1, CI (trunk) Feb 23, 2021", "* commit2, CI (trunk) Feb 23, 2021"}, + []string{"* commit1, CI [trunk] Feb 23, 2021", "* commit2, CI [trunk] Feb 23, 2021"}, func(_, _ string, opts []string) (int, error) { - return prompter.IndexFor(opts, "* commit2, CI (trunk) Feb 23, 2021") + return prompter.IndexFor(opts, "* commit2, CI [trunk] Feb 23, 2021") }) }, wantOut: "\x1b[?1049h\x1b[0;0H\x1b[JRefreshing run status every 0 seconds. Press Ctrl+C to quit.\n\n* trunk CI · 2\nTriggered via push about 59 minutes ago\n\nJOBS\n✓ cool job in 4m34s (ID 10)\n ✓ fob the barz\n ✓ barz the fob\n\x1b[?1049l✓ trunk CI · 2\nTriggered via push about 59 minutes ago\n\nJOBS\n✓ cool job in 4m34s (ID 10)\n ✓ fob the barz\n ✓ barz the fob\n\n✓ Run CI (2) completed with 'success'\n", @@ -290,9 +290,9 @@ func TestWatchRun(t *testing.T) { httpStubs: failedRunStubs, promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Select a workflow run", - []string{"* commit1, CI (trunk) Feb 23, 2021", "* commit2, CI (trunk) Feb 23, 2021"}, + []string{"* commit1, CI [trunk] Feb 23, 2021", "* commit2, CI [trunk] Feb 23, 2021"}, func(_, _ string, opts []string) (int, error) { - return prompter.IndexFor(opts, "* commit2, CI (trunk) Feb 23, 2021") + return prompter.IndexFor(opts, "* commit2, CI [trunk] Feb 23, 2021") }) }, wantOut: "\x1b[?1049h\x1b[0;0H\x1b[JRefreshing run status every 0 seconds. Press Ctrl+C to quit.\n\n* trunk CI · 2\nTriggered via push about 59 minutes ago\n\n\x1b[?1049lX trunk CI · 2\nTriggered via push about 59 minutes ago\n\nJOBS\nX sad job in 4m34s (ID 20)\n ✓ barf the quux\n X quux the barf\n\nANNOTATIONS\nX the job is sad\nsad job: blaze.py#420\n\n\nX Run CI (2) completed with 'failure'\n", diff --git a/test/integration/attestation-cmd/verify-oci-bundle.sh b/test/integration/attestation-cmd/verify-oci-bundle.sh new file mode 100755 index 000000000..0e9fd2281 --- /dev/null +++ b/test/integration/attestation-cmd/verify-oci-bundle.sh @@ -0,0 +1,14 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Get the root directory of the repository +rootDir="$(git rev-parse --show-toplevel)" + +ghBuildPath="$rootDir/bin/gh" + +# Verify an OCI artifact with bundles stored on the GHCR OCI registry +echo "Testing with OCI image ghcr.io/github/artifact-attestations-helm-charts/policy-controller:v0.10.0-github9 with the --bundle-from-oci flag" +if ! $ghBuildPath attestation verify oci://ghcr.io/github/artifact-attestations-helm-charts/policy-controller:v0.10.0-github9 --owner=github --bundle-from-oci; then + echo "Failed to verify oci://ghcr.io/github/artifact-attestations-helm-charts/policy-controller:v0.10.0-github9 with bundles from the GHCR OCI registry" + exit 1 +fi