From 84c36961d81c40a522be6a6bce378a729027cb9a Mon Sep 17 00:00:00 2001 From: boraberke <67373739+boraberke@users.noreply.github.com> Date: Wed, 31 Aug 2022 12:54:43 +0300 Subject: [PATCH] command/app: force `endpoint-url` to have scheme (#496) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this PR, if an URL without a scheme was given to `endpoint-url` such as `example.com`, `s5cmd` would add `http` scheme to it. After this PR, `s5cmd` will not accept `endpoint-url` without a scheme. User should give either `http` or `https` scheme explicitly. Co-authored-by: İbrahim Güngör --- CHANGELOG.md | 1 + command/app.go | 10 +++++++++ e2e/app_test.go | 54 ++++++++++++++++++++++++++++++++++++++++++++++ storage/s3.go | 6 +----- storage/s3_test.go | 15 ++++++++----- 5 files changed, 76 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 671c36b93..44e108b73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ #### Breaking changes - Adjacent slashes in key are no longer removed when uploading to remote. Before `s5cmd cp file.txt s3://bucket/a//b///c/` would copy to `s3://bucket/a/b/c/file.txt` but now to `s3://bucket/a//b///c/file.txt`.([#459](https://github.com/peak/s5cmd/pull/459)) +- `--endpoint-url` will not accept URLs without scheme such as `example.com`. Instead, it will give an error and ask for an url with a scheme; either `http://example.com` or `https://example.com` ([#496](https://github.com/peak/s5cmd/pull/496)). #### Features - Added `--content-type` and `--content-encoding` flags to `cp` command. ([#264](https://github.com/peak/s5cmd/issues/264)) diff --git a/command/app.go b/command/app.go index 6f324b842..c7a728b86 100644 --- a/command/app.go +++ b/command/app.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os" + "strings" cmpinstall "github.com/posener/complete/cmd/install" "github.com/urfave/cli/v2" @@ -96,6 +97,7 @@ var app = &cli.App{ printJSON := c.Bool("json") logLevel := c.String("log") isStat := c.Bool("stat") + endpointURL := c.String("endpoint-url") log.Init(logLevel, printJSON) parallel.Init(workerCount) @@ -120,6 +122,14 @@ var app = &cli.App{ stat.InitStat() } + if endpointURL != "" { + if !strings.HasPrefix(endpointURL, "http") { + err := fmt.Errorf(`bad value for --endpoint-url %v: scheme is missing. Must be of the form http:/// or https:///`, endpointURL) + printError(commandFromContext(c), c.Command.Name, err) + return err + } + } + return nil }, CommandNotFound: func(c *cli.Context, command string) { diff --git a/e2e/app_test.go b/e2e/app_test.go index a4eb32d11..03d912e1a 100644 --- a/e2e/app_test.go +++ b/e2e/app_test.go @@ -227,3 +227,57 @@ func TestInvalidLoglevel(t *testing.T) { 1: equals("See 's5cmd --help' for usage"), }) } + +func TestAppEndpointShouldHaveScheme(t *testing.T) { + t.Parallel() + + testcases := []struct { + name string + endpointUrl string + expectedError error + expectedExitCode int + }{ + { + name: "endpoint_with_http_scheme", + endpointUrl: "http://storage.googleapis.com", + expectedError: nil, + expectedExitCode: 0, + }, + { + name: "endpoint_with_https_scheme", + endpointUrl: "https://storage.googleapis.com", + expectedError: nil, + expectedExitCode: 0, + }, + { + name: "endpoint_with_no_scheme", + endpointUrl: "storage.googleapis.com", + expectedError: fmt.Errorf(`ERROR bad value for --endpoint-url storage.googleapis.com: scheme is missing. Must be of the form http:/// or https:///`), + expectedExitCode: 1, + }, + } + + for _, tc := range testcases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + _, s5cmd, cleanup := setup(t) + defer cleanup() + + cmd := s5cmd("--endpoint-url", tc.endpointUrl) + result := icmd.RunCmd(cmd) + + result.Assert(t, icmd.Expected{ExitCode: tc.expectedExitCode}) + + if tc.expectedError == nil && result.Stderr() == "" { + return + } + + assertLines(t, result.Stderr(), map[int]compareFunc{ + 0: equals("%v", tc.expectedError), + }) + + }) + } +} diff --git a/storage/s3.go b/storage/s3.go index 82f10a49b..ee92c0b5c 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -80,11 +80,7 @@ func parseEndpoint(endpoint string) (urlpkg.URL, error) { if endpoint == "" { return sentinelURL, nil } - // add a scheme to correctly parse the endpoint. Without a scheme, - // url.Parse will put the host information in path" - if !strings.HasPrefix(endpoint, "http") { - endpoint = "http://" + endpoint - } + u, err := urlpkg.Parse(endpoint) if err != nil { return sentinelURL, fmt.Errorf("parse endpoint %q: %v", endpoint, err) diff --git a/storage/s3_test.go b/storage/s3_test.go index 75b823f41..c32f634ef 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -53,22 +53,27 @@ func TestNewSessionPathStyle(t *testing.T) { }, { name: "expect_virtual_host_style_for_transfer_accel", - endpoint: urlpkg.URL{Host: transferAccelEndpoint}, + endpoint: urlpkg.URL{Scheme: "https", Host: transferAccelEndpoint}, expectPathStyle: false, }, { name: "expect_virtual_host_style_for_google_cloud_storage", - endpoint: urlpkg.URL{Host: gcsEndpoint}, + endpoint: urlpkg.URL{Scheme: "https", Host: gcsEndpoint}, expectPathStyle: false, }, { name: "expect_path_style_for_localhost", - endpoint: urlpkg.URL{Host: "127.0.0.1"}, + endpoint: urlpkg.URL{Scheme: "http", Host: "127.0.0.1"}, + expectPathStyle: true, + }, + { + name: "expect_path_style_for_secure_localhost", + endpoint: urlpkg.URL{Scheme: "https", Host: "127.0.0.1"}, expectPathStyle: true, }, { name: "expect_path_style_for_custom_endpoint", - endpoint: urlpkg.URL{Host: "example.com"}, + endpoint: urlpkg.URL{Scheme: "https", Host: "example.com"}, expectPathStyle: true, }, } @@ -77,7 +82,7 @@ func TestNewSessionPathStyle(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { - opts := Options{Endpoint: tc.endpoint.Hostname()} + opts := Options{Endpoint: tc.endpoint.String()} sess, err := globalSessionCache.newSession(context.Background(), opts) if err != nil { t.Fatal(err)