command/app: force endpoint-url to have scheme (#496)

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 <[email protected]>
peak · Aug 31, 2022 · 84c3696 · 84c3696
1 parent 914e701
commit 84c3696
Show file tree

Hide file tree

Showing 5 changed files with 76 additions and 10 deletions.
diff --git 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
@@ -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://<hostname>/ or https://<hostname>/`, 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
@@ -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://<hostname>/ or https://<hostname>/`),
+			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
@@ -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
@@ -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)