From fb39c38c85c4d6de1112e32e6bd115227ce56a9a Mon Sep 17 00:00:00 2001 From: Cristian Dominguez Date: Sat, 20 Mar 2021 18:46:10 -0300 Subject: [PATCH] Disable preview option in prompts if URL size is too long --- pkg/cmd/issue/create/create.go | 52 ++++++++++++++++------------- pkg/cmd/issue/create/create_test.go | 9 +++++ pkg/cmd/pr/create/create.go | 49 +++++++++++++++++---------- pkg/cmd/pr/create/create_test.go | 20 +++++++++++ pkg/cmd/pr/shared/params.go | 7 +++- utils/utils.go | 5 +++ 6 files changed, 100 insertions(+), 42 deletions(-) diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 4b3ef910d..31a9812cc 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -157,22 +157,23 @@ func createRun(opts *CreateOptions) (err error) { tpl := shared.NewTemplateManager(httpClient, baseRepo, opts.RootDirOverride, !opts.HasRepoOverride, false) + var openURL string + if opts.WebMode { - openURL := ghrepo.GenerateRepoURL(baseRepo, "issues/new") if opts.Title != "" || opts.Body != "" || tb.HasMetadata() { - if len(opts.Projects) > 0 { - var err error - tb.Projects, err = api.ProjectNamesToPaths(apiClient, baseRepo, tb.Projects) - if err != nil { - return fmt.Errorf("could not add to project: %w", err) - } - } - openURL, err = prShared.WithPrAndIssueQueryParams(openURL, tb) + openURL, err = generatePreviewURL(apiClient, baseRepo, tb) if err != nil { return } + if !utils.ValidURL(openURL) { + err = fmt.Errorf("Failed to create URL: maximum URL length exceeded") + return + } } else if ok, _ := tpl.HasTemplates(); ok { - openURL += "/choose" + openURL = ghrepo.GenerateRepoURL(baseRepo, "issues/new/choose") + if err != nil { + return + } } if isTerminal { fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", utils.DisplayURL(openURL)) @@ -240,7 +241,14 @@ func createRun(opts *CreateOptions) (err error) { } } - action, err = prShared.ConfirmSubmission(!tb.HasMetadata(), repo.ViewerCanTriage()) + openURL, err = generatePreviewURL(apiClient, baseRepo, tb) + if err != nil { + return + } + + allowPreview := !tb.HasMetadata() && utils.ValidURL(openURL) + + action, err = prShared.ConfirmSubmission(allowPreview, repo.ViewerCanTriage()) if err != nil { err = fmt.Errorf("unable to confirm: %w", err) return @@ -277,18 +285,6 @@ func createRun(opts *CreateOptions) (err error) { } if action == prShared.PreviewAction { - if len(tb.Projects) > 0 { - var err error - tb.Projects, err = api.ProjectNamesToPaths(apiClient, repo, tb.Projects) - if err != nil { - return fmt.Errorf("could not add to project: %w", err) - } - } - openURL := ghrepo.GenerateRepoURL(baseRepo, "issues/new") - openURL, err = prShared.WithPrAndIssueQueryParams(openURL, tb) - if err != nil { - return - } if isTerminal { fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", utils.DisplayURL(openURL)) } @@ -320,3 +316,13 @@ func createRun(opts *CreateOptions) (err error) { return } + +func generatePreviewURL(apiClient *api.Client, baseRepo ghrepo.Interface, tb shared.IssueMetadataState) (string, error) { + openURL := ghrepo.GenerateRepoURL(baseRepo, "issues/new") + openURL, err := prShared.WithPrAndIssueQueryParams(apiClient, baseRepo, openURL, tb) + if err != nil { + return "", err + } + + return openURL, nil +} diff --git a/pkg/cmd/issue/create/create_test.go b/pkg/cmd/issue/create/create_test.go index 5ecde4599..d222ac80b 100644 --- a/pkg/cmd/issue/create/create_test.go +++ b/pkg/cmd/issue/create/create_test.go @@ -532,6 +532,15 @@ func TestIssueCreate_web(t *testing.T) { assert.Equal(t, "https://github.com/OWNER/REPO/issues/new?assignees=MonaLisa", output.BrowsedURL) } +func TestIssueCreate_webLongURL(t *testing.T) { + longBodyFile := filepath.Join(t.TempDir(), "long-body.txt") + err := ioutil.WriteFile(longBodyFile, make([]byte, 9216), 0600) + require.NoError(t, err) + + _, err = runCommand(nil, true, fmt.Sprintf("-F '%s' --web", longBodyFile)) + require.EqualError(t, err, "Failed to create URL: maximum URL length exceeded") +} + func TestIssueCreate_webTitleBody(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 22ca05de4..9963fb1bd 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -190,6 +190,8 @@ func createRun(opts *CreateOptions) (err error) { return } + var openURL string + if opts.WebMode { if !opts.Autofill { state.Title = opts.Title @@ -199,7 +201,15 @@ func createRun(opts *CreateOptions) (err error) { if err != nil { return } - return previewPR(*opts, *ctx, *state) + openURL, err = generatePreviewURL(*ctx, *state) + if err != nil { + return + } + if !utils.ValidURL(openURL) { + err = fmt.Errorf("Failed to create URL: maximum URL length exceeded") + return + } + return previewPR(*opts, openURL) } if opts.TitleProvided { @@ -291,8 +301,15 @@ func createRun(opts *CreateOptions) (err error) { } } + openURL, err = generatePreviewURL(*ctx, *state) + if err != nil { + return + } + allowMetadata := ctx.BaseRepo.ViewerCanTriage() - action, err := shared.ConfirmSubmission(!state.HasMetadata(), allowMetadata) + allowPreview := !state.HasMetadata() && utils.ValidURL(openURL) + + action, err := shared.ConfirmSubmission(allowPreview, allowMetadata) if err != nil { return fmt.Errorf("unable to confirm: %w", err) } @@ -327,7 +344,7 @@ func createRun(opts *CreateOptions) (err error) { } if action == shared.PreviewAction { - return previewPR(*opts, *ctx, *state) + return previewPR(*opts, openURL) } if action == shared.SubmitAction { @@ -639,20 +656,7 @@ func submitPR(opts CreateOptions, ctx CreateContext, state shared.IssueMetadataS return nil } -func previewPR(opts CreateOptions, ctx CreateContext, state shared.IssueMetadataState) error { - if len(state.Projects) > 0 { - var err error - state.Projects, err = api.ProjectNamesToPaths(ctx.Client, ctx.BaseRepo, state.Projects) - if err != nil { - return fmt.Errorf("could not add to project: %w", err) - } - } - - openURL, err := generateCompareURL(ctx, state) - if err != nil { - return err - } - +func previewPR(opts CreateOptions, openURL string) error { if opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() { fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", utils.DisplayURL(openURL)) } @@ -750,7 +754,7 @@ func generateCompareURL(ctx CreateContext, state shared.IssueMetadataState) (str ctx.BaseRepo, "compare/%s...%s?expand=1", url.QueryEscape(ctx.BaseBranch), url.QueryEscape(ctx.HeadBranchLabel)) - url, err := shared.WithPrAndIssueQueryParams(u, state) + url, err := shared.WithPrAndIssueQueryParams(ctx.Client, ctx.BaseRepo, u, state) if err != nil { return "", err } @@ -758,3 +762,12 @@ func generateCompareURL(ctx CreateContext, state shared.IssueMetadataState) (str } var gitPushRegexp = regexp.MustCompile("^remote: (Create a pull request.*by visiting|[[:space:]]*https://.*/pull/new/).*\n?$") + +func generatePreviewURL(ctx CreateContext, state shared.IssueMetadataState) (string, error) { + openURL, err := generateCompareURL(ctx, state) + if err != nil { + return "", err + } + + return openURL, nil +} diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 9d338bcad..70050891c 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -835,6 +835,26 @@ func TestPRCreate_web(t *testing.T) { assert.Equal(t, "https://github.com/OWNER/REPO/compare/master...feature?expand=1", output.BrowsedURL) } +func TestPRCreate_webLongURL(t *testing.T) { + longBodyFile := filepath.Join(t.TempDir(), "long-body.txt") + err := ioutil.WriteFile(longBodyFile, make([]byte, 9216), 0600) + require.NoError(t, err) + + http := initFakeHTTP() + defer http.Verify(t) + + http.StubRepoInfoResponse("OWNER", "REPO", "master") + + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + cs.Register(`git status --porcelain`, 0, "") + cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") + + _, err = runCommand(http, nil, "feature", false, fmt.Sprintf("--body-file '%s' --web --head=feature", longBodyFile)) + require.EqualError(t, err, "Failed to create URL: maximum URL length exceeded") +} + func TestPRCreate_webProject(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 086d995a9..d994e7fc6 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -10,7 +10,7 @@ import ( "github.com/cli/cli/pkg/githubsearch" ) -func WithPrAndIssueQueryParams(baseURL string, state IssueMetadataState) (string, error) { +func WithPrAndIssueQueryParams(client *api.Client, baseRepo ghrepo.Interface, baseURL string, state IssueMetadataState) (string, error) { u, err := url.Parse(baseURL) if err != nil { return "", err @@ -29,6 +29,11 @@ func WithPrAndIssueQueryParams(baseURL string, state IssueMetadataState) (string q.Set("labels", strings.Join(state.Labels, ",")) } if len(state.Projects) > 0 { + var err error + state.Projects, err = api.ProjectNamesToPaths(client, baseRepo, state.Projects) + if err != nil { + return "", fmt.Errorf("could not add to project: %w", err) + } q.Set("projects", strings.Join(state.Projects, ",")) } if len(state.Milestones) > 0 { diff --git a/utils/utils.go b/utils/utils.go index e614d3dd0..cd423bf65 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -78,3 +78,8 @@ func DisplayURL(urlStr string) string { } return u.Hostname() + u.Path } + +// Maximum length of a URL: 8192 bytes +func ValidURL(urlStr string) bool { + return len(urlStr) < 8192 +}