Repository: peak/s5cmd
Pull request #111 (2020-03-24 01:21:21)

Improved URL Parsing
- Use `shellquote` to split lines in `commands.txt` files so filenames with spaces can be used
- Escape regex characters in `prefix`es when initializing `ObjectURL`s. These may be a part of valid S3 prefixes.

brendan-matroid: Hey @igungor, thanks for the review.

Are there any steps I need to take before running the tests locally? I'm seeing a bunch of errors similar to this:
```
--- FAIL: TestCopyMultipleFlatS3ObjectsToLocal (0.05s)
    cp_test.go:138: assertion failed: directory /tmp/TestCopyMultipleFlatS3ObjectsToLocal-726849334/workdir does not match expected:
        /another_test_file.txt
          mode: expected -rw-r--r-- got -rw-rw-r--
        /filename-with-hypen.gz
          mode: expected -rw-r--r-- got -rw-rw-r--
        /readme.md
          mode: expected -rw-r--r-- got -rw-rw-r--
        /testfile1.txt
          mode: expected -rw-r--r-- got -rw-rw-r--
```
igungor: > Are there any steps I need to take before running the tests locally? I'm seeing a bunch of errors similar to this:
> 
> ```
> --- FAIL: TestCopyMultipleFlatS3ObjectsToLocal (0.05s)
>     cp_test.go:138: assertion failed: directory /tmp/TestCopyMultipleFlatS3ObjectsToLocal-726849334/workdir does not match expected:
>         /another_test_file.txt
>           mode: expected -rw-r--r-- got -rw-rw-r--
>         /filename-with-hypen.gz
>           mode: expected -rw-r--r-- got -rw-rw-r--
>         /readme.md
>           mode: expected -rw-r--r-- got -rw-rw-r--
>         /testfile1.txt
>           mode: expected -rw-r--r-- got -rw-rw-r--
> ```

Some of the tests are failing due to the new `o.Prefix = regexp.QuoteMeta(o.Prefix)` line. The above test has an assertion that file modbits are expected to be 0644 but got 0664. 

New `run` test is failing because of the `prefix` change as well.
```
--- FAIL: TestRunSpecialCharactersInPrefix (0.20s)
    run_test.go:247: line 0: equals: (-want +got):
          string(
        - 	"cp s3://test-run-special-characters-in-prefix/special-chars_!@#$%!^(MISSING)&_()_+{[_%!C(MISSING)äè| __;'_,_._-中文 =/_!@#$%!^(MISSING)&_()_+{[_%!C(MISSING)äè| __;'_,_._-中文 =image.jpg",
        + 	"",
          )
    run_test.go:247: expected a comparison function for line "cp s3://test-run-special-characters-in-prefixspecial-chars_!@#$%^&_()_+{[_%5Cäè| __;'_,_._-中文 =/_!@#$%^&_()_+{[_%5Cäè| __;'_,_._-中文 =image.jpg" (lineno: 1)
    run_test.go:247: cp s3://test-run-special-characters-in-prefixspecial-chars_!@#$%^&_()_+{[_%5Cäè| __;'_,_._-中文 =/_!@#$%^&_()_+{[_%5Cäè| __;'_,_._-中文 =image.jpg
```

That'd be very helpful if you could provide a concrete use-case to demonstrate the problem. We can debug the issue as well. 
igungor: > That'd be very helpful if you could provide a concrete use-case to demonstrate the problem. We can debug the issue as well.

Ping @brendan-matroid
brendan-matroid: Hey @igungor, sorry for the delay.
There are two separate use cases addressed by this PR:

1) Some AWS S3 prefixes may contain spaces. E.g. `s3://some-bucket/some prefix/some file` is a valid S3 path. Splitting commands by whitespace in `s5cmd run` does not permit copying to/from such files.

2) Escaping regex metacharacters for the `prefix` portion of the wildcard is necessary in cases where the path contains regex metacharacters before the wildcard. This is also problematic in cases where there is no wildcard (since the entire path is treated as the `prefix`).

At `objurl.go:220`: `filterRegex = o.Prefix + filterRegex` which is then compiled in the next line.

If `o.Prefix` contains unescaped regex metacharacters then they will be compiled (incorrectly), which may either result in some unexpected, valid regex (and silently be used), or (in the case of the s3 prefix used in the added testcase), it will result in an invalid regex and throw an error.
igungor: > There are two separate use cases addressed by this PR:
> 
>     1. Some AWS S3 prefixes may contain spaces. E.g. `s3://some-bucket/some prefix/some file` is a valid S3 path. Splitting commands by whitespace in `s5cmd run` does not permit copying to/from such files.

I'm OK with this change. I think it is necessary.

>     2. Escaping regex metacharacters for the `prefix` portion of the wildcard is necessary in cases where the path contains regex metacharacters before the wildcard. This is also problematic in cases where there is no wildcard (since the entire path is treated as the `prefix`).
> 
> 
> At `objurl.go:220`: `filterRegex = o.Prefix + filterRegex` which is then compiled in the next line.
> 
> If `o.Prefix` contains unescaped regex metacharacters then they will be compiled (incorrectly), which may either result in some unexpected, valid regex (and silently be used in addition to any regexes resulting from wildcard expressions), or (in the case of the s3 prefix used in the added testcase), it will result in an invalid regex and compilation of said regex will throw an error.

I understand the problem now, thanks for the detailed answer.

The problem with this change is that `u.Prefix` needs to stay as is since it's the canonical prefix information. The change should be, as you said earlier, `at objurl.go:220: filterRegex = o.Prefix + filterRegex`. It should be `filterRegex = regexp.QuoteMeta(o.Prefix) + filterRegex`.

Could you try and check if it's working for your use case? If so, and if you're still interested, could you rebase your branch please, so that we can progress.


ilkinulas: will re-open to trigger circle-ci
igungor: > will re-open to trigger circle-ci

let's try again now.

Patch:
diff --git a/command/run.go b/command/run.go
index f2d5ddf..ab6e4cb 100644
--- a/command/run.go
+++ b/command/run.go
@@ -10,6 +10,7 @@ import (
 	"strings"
 
 	"github.com/urfave/cli/v2"
+	"github.com/kballard/go-shellquote"
 
 	"github.com/peak/s5cmd/parallel"
 )
@@ -86,7 +87,11 @@ var runCommand = &cli.Command{
 				continue
 			}
 
-			fields := strings.Fields(line)
+			fields, err := shellquote.Split(line)
+			if err != nil {
+				return err
+			}
+
 			if len(fields) == 0 {
 				continue
 			}
diff --git a/go.mod b/go.mod
index 549de66..505a1be 100644
--- a/go.mod
+++ b/go.mod
@@ -10,6 +10,7 @@ require (
 	github.com/iancoleman/strcase v0.0.0-20191112232945-16388991a334
 	github.com/johannesboyne/gofakes3 v0.0.0-20191228161223-9aee1c78a252
 	github.com/karrick/godirwalk v1.15.3
+	github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51
 	github.com/kr/pretty v0.2.0 // indirect
 	github.com/posener/complete v1.2.3
 	github.com/stretchr/testify v1.4.0
diff --git a/go.sum b/go.sum
index 7a9c4f7..322d2d5 100644
--- a/go.sum
+++ b/go.sum
@@ -26,6 +26,8 @@ github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af h1:pmfjZENx5i
 github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af/go.mod h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k=
 github.com/karrick/godirwalk v1.15.3 h1:0a2pXOgtB16CqIqXTiT7+K9L73f74n/aNQUnH6Ortew=
 github.com/karrick/godirwalk v1.15.3/go.mod h1:j4mkqPuvaLI8mp1DroR3P6ad7cyYd4c1qeJ3RV7ULlk=
+github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 h1:Z9n2FFNUXsshfwJMBgNA0RU6/i7WVaAegv3PtuIHPMs=
+github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51/go.mod h1:CzGEWj7cYgsdH8dAjBGEr58BoE7ScuLd+fwFZ44+/x8=
 github.com/kr/pretty v0.2.0 h1:s5hAObm+yFO5uHYt5dYjxi2rXrsnmRpJx4OYvIWUaQs=
 github.com/kr/pretty v0.2.0/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI=
 github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
diff --git a/storage/url/url.go b/storage/url/url.go
index 1c145ac..b24a511 100644
--- a/storage/url/url.go
+++ b/storage/url/url.go
@@ -225,7 +225,7 @@ func (u *URL) setPrefixAndFilter() error {
 		filterRegex = strings.Replace(filterRegex, "\\?", ".", -1)
 		filterRegex = strings.Replace(filterRegex, "\\*", ".*?", -1)
 	}
-	filterRegex = u.Prefix + filterRegex
+	filterRegex = regexp.QuoteMeta(u.Prefix) + filterRegex
 	r, err := regexp.Compile("^" + filterRegex + "$")
 	if err != nil {
 		return err
diff --git a/vendor/github.com/kballard/go-shellquote/LICENSE b/vendor/github.com/kballard/go-shellquote/LICENSE
new file mode 100644
index 0000000..a6d7731
--- /dev/null
+++ b/vendor/github.com/kballard/go-shellquote/LICENSE
@@ -0,0 +1,19 @@
+Copyright (C) 2014 Kevin Ballard
+
+Permission is hereby granted, free of charge, to any person obtaining
+a copy of this software and associated documentation files (the "Software"),
+to deal in the Software without restriction, including without limitation
+the rights to use, copy, modify, merge, publish, distribute, sublicense,
+and/or sell copies of the Software, and to permit persons to whom the
+Software is furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included
+in all copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
+IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
+TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE
+OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
diff --git a/vendor/github.com/kballard/go-shellquote/README b/vendor/github.com/kballard/go-shellquote/README
new file mode 100644
index 0000000..4d34e87
--- /dev/null
+++ b/vendor/github.com/kballard/go-shellquote/README
@@ -0,0 +1,36 @@
+PACKAGE
+
+package shellquote
+    import "github.com/kballard/go-shellquote"
+
+    Shellquote provides utilities for joining/splitting strings using sh's
+    word-splitting rules.
+
+VARIABLES
+
+var (
+    UnterminatedSingleQuoteError = errors.New("Unterminated single-quoted string")
+    UnterminatedDoubleQuoteError = errors.New("Unterminated double-quoted string")
+    UnterminatedEscapeError      = errors.New("Unterminated backslash-escape")
+)
+
+
+FUNCTIONS
+
+func Join(args ...string) string
+    Join quotes each argument and joins them with a space. If passed to
+    /bin/sh, the resulting string will be split back into the original
+    arguments.
+
+func Split(input string) (words []string, err error)
+    Split splits a string according to /bin/sh's word-splitting rules. It
+    supports backslash-escapes, single-quotes, and double-quotes. Notably it
+    does not support the $'' style of quoting. It also doesn't attempt to
+    perform any other sort of expansion, including brace expansion, shell
+    expansion, or pathname expansion.
+
+    If the given input has an unterminated quoted string or ends in a
+    backslash-escape, one of UnterminatedSingleQuoteError,
+    UnterminatedDoubleQuoteError, or UnterminatedEscapeError is returned.
+
+
diff --git a/vendor/github.com/kballard/go-shellquote/doc.go b/vendor/github.com/kballard/go-shellquote/doc.go
new file mode 100644
index 0000000..9445fa4
--- /dev/null
+++ b/vendor/github.com/kballard/go-shellquote/doc.go
@@ -0,0 +1,3 @@
+// Shellquote provides utilities for joining/splitting strings using sh's
+// word-splitting rules.
+package shellquote
diff --git a/vendor/github.com/kballard/go-shellquote/quote.go b/vendor/github.com/kballard/go-shellquote/quote.go
new file mode 100644
index 0000000..72a8cb3
--- /dev/null
+++ b/vendor/github.com/kballard/go-shellquote/quote.go
@@ -0,0 +1,102 @@
+package shellquote
+
+import (
+	"bytes"
+	"strings"
+	"unicode/utf8"
+)
+
+// Join quotes each argument and joins them with a space.
+// If passed to /bin/sh, the resulting string will be split back into the
+// original arguments.
+func Join(args ...string) string {
+	var buf bytes.Buffer
+	for i, arg := range args {
+		if i != 0 {
+			buf.WriteByte(' ')
+		}
+		quote(arg, &buf)
+	}
+	return buf.String()
+}
+
+const (
+	specialChars      = "\\'\"`${[|&;<>()*?!"
+	extraSpecialChars = " \t\n"
+	prefixChars       = "~"
+)
+
+func quote(word string, buf *bytes.Buffer) {
+	// We want to try to produce a "nice" output. As such, we will
+	// backslash-escape most characters, but if we encounter a space, or if we
+	// encounter an extra-special char (which doesn't work with
+	// backslash-escaping) we switch over to quoting the whole word. We do this
+	// with a space because it's typically easier for people to read multi-word
+	// arguments when quoted with a space rather than with ugly backslashes
+	// everywhere.
+	origLen := buf.Len()
+
+	if len(word) == 0 {
+		// oops, no content
+		buf.WriteString("''")
+		return
+	}
+
+	cur, prev := word, word
+	atStart := true
+	for len(cur) > 0 {
+		c, l := utf8.DecodeRuneInString(cur)
+		cur = cur[l:]
+		if strings.ContainsRune(specialChars, c) || (atStart && strings.ContainsRune(prefixChars, c)) {
+			// copy the non-special chars up to this point
+			if len(cur) < len(prev) {
+				buf.WriteString(prev[0 : len(prev)-len(cur)-l])
+			}
+			buf.WriteByte('\\')
+			buf.WriteRune(c)
+			prev = cur
+		} else if strings.ContainsRune(extraSpecialChars, c) {
+			// start over in quote mode
+			buf.Truncate(origLen)
+			goto quote
+		}
+		atStart = false
+	}
+	if len(prev) > 0 {
+		buf.WriteString(prev)
+	}
+	return
+
+quote:
+	// quote mode
+	// Use single-quotes, but if we find a single-quote in the word, we need
+	// to terminate the string, emit an escaped quote, and start the string up
+	// again
+	inQuote := false
+	for len(word) > 0 {
+		i := strings.IndexRune(word, '\'')
+		if i == -1 {
+			break
+		}
+		if i > 0 {
+			if !inQuote {
+				buf.WriteByte('\'')
+				inQuote = true
+			}
+			buf.WriteString(word[0:i])
+		}
+		word = word[i+1:]
+		if inQuote {
+			buf.WriteByte('\'')
+			inQuote = false
+		}
+		buf.WriteString("\\'")
+	}
+	if len(word) > 0 {
+		if !inQuote {
+			buf.WriteByte('\'')
+		}
+		buf.WriteString(word)
+		buf.WriteByte('\'')
+	}
+}
diff --git a/vendor/github.com/kballard/go-shellquote/unquote.go b/vendor/github.com/kballard/go-shellquote/unquote.go
new file mode 100644
index 0000000..b1b13da
--- /dev/null
+++ b/vendor/github.com/kballard/go-shellquote/unquote.go
@@ -0,0 +1,156 @@
+package shellquote
+
+import (
+	"bytes"
+	"errors"
+	"strings"
+	"unicode/utf8"
+)
+
+var (
+	UnterminatedSingleQuoteError = errors.New("Unterminated single-quoted string")
+	UnterminatedDoubleQuoteError = errors.New("Unterminated double-quoted string")
+	UnterminatedEscapeError      = errors.New("Unterminated backslash-escape")
+)
+
+var (
+	splitChars        = " \n\t"
+	singleChar        = '\''
+	doubleChar        = '"'
+	escapeChar        = '\\'
+	doubleEscapeChars = "$`\"\n\\"
+)
+
+// Split splits a string according to /bin/sh's word-splitting rules. It
+// supports backslash-escapes, single-quotes, and double-quotes. Notably it does
+// not support the $'' style of quoting. It also doesn't attempt to perform any
+// other sort of expansion, including brace expansion, shell expansion, or
+// pathname expansion.
+//
+// If the given input has an unterminated quoted string or ends in a
+// backslash-escape, one of UnterminatedSingleQuoteError,
+// UnterminatedDoubleQuoteError, or UnterminatedEscapeError is returned.
+func Split(input string) (words []string, err error) {
+	var buf bytes.Buffer
+	words = make([]string, 0)
+
+	for len(input) > 0 {
+		// skip any splitChars at the start
+		c, l := utf8.DecodeRuneInString(input)
+		if strings.ContainsRune(splitChars, c) {
+			input = input[l:]
+			continue
+		} else if c == escapeChar {
+			// Look ahead for escaped newline so we can skip over it
+			next := input[l:]
+			if len(next) == 0 {
+				err = UnterminatedEscapeError
+				return
+			}
+			c2, l2 := utf8.DecodeRuneInString(next)
+			if c2 == '\n' {
+				input = next[l2:]
+				continue
+			}
+		}
+
+		var word string
+		word, input, err = splitWord(input, &buf)
+		if err != nil {
+			return
+		}
+		words = append(words, word)
+	}
+	return
+}
+
+func splitWord(input string, buf *bytes.Buffer) (word string, remainder string, err error) {
+	buf.Reset()
+
+raw:
+	{
+		cur := input
+		for len(cur) > 0 {
+			c, l := utf8.DecodeRuneInString(cur)
+			cur = cur[l:]
+			if c == singleChar {
+				buf.WriteString(input[0 : len(input)-len(cur)-l])
+				input = cur
+				goto single
+			} else if c == doubleChar {
+				buf.WriteString(input[0 : len(input)-len(cur)-l])
+				input = cur
+				goto double
+			} else if c == escapeChar {
+				buf.WriteString(input[0 : len(input)-len(cur)-l])
+				input = cur
+				goto escape
+			} else if strings.ContainsRune(splitChars, c) {
+				buf.WriteString(input[0 : len(input)-len(cur)-l])
+				return buf.String(), cur, nil
+			}
+		}
+		if len(input) > 0 {
+			buf.WriteString(input)
+			input = ""
+		}
+		goto done
+	}
+
+escape:
+	{
+		if len(input) == 0 {
+			return "", "", UnterminatedEscapeError
+		}
+		c, l := utf8.DecodeRuneInString(input)
+		if c == '\n' {
+			// a backslash-escaped newline is elided from the output entirely
+		} else {
+			buf.WriteString(input[:l])
+		}
+		input = input[l:]
+	}
+	goto raw
+
+single:
+	{
+		i := strings.IndexRune(input, singleChar)
+		if i == -1 {
+			return "", "", UnterminatedSingleQuoteError
+		}
+		buf.WriteString(input[0:i])
+		input = input[i+1:]
+		goto raw
+	}
+
+double:
+	{
+		cur := input
+		for len(cur) > 0 {
+			c, l := utf8.DecodeRuneInString(cur)
+			cur = cur[l:]
+			if c == doubleChar {
+				buf.WriteString(input[0 : len(input)-len(cur)-l])
+				input = cur
+				goto raw
+			} else if c == escapeChar {
+				// bash only supports certain escapes in double-quoted strings
+				c2, l2 := utf8.DecodeRuneInString(cur)
+				cur = cur[l2:]
+				if strings.ContainsRune(doubleEscapeChars, c2) {
+					buf.WriteString(input[0 : len(input)-len(cur)-l-l2])
+					if c2 == '\n' {
+						// newline is special, skip the backslash entirely
+					} else {
+						buf.WriteRune(c2)
+					}
+					input = cur
+				}
+			}
+		}
+		return "", "", UnterminatedDoubleQuoteError
+	}
+
+done:
+	return buf.String(), input, nil
+}
diff --git a/vendor/modules.txt b/vendor/modules.txt
index ed3dc06..f41a5b6 100644
--- a/vendor/modules.txt
+++ b/vendor/modules.txt
@@ -74,6 +74,8 @@ github.com/johannesboyne/gofakes3/internal/goskipiter
 github.com/johannesboyne/gofakes3/internal/s3io
 # github.com/karrick/godirwalk v1.15.3
 github.com/karrick/godirwalk
+# github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51
+github.com/kballard/go-shellquote
 # github.com/pkg/errors v0.9.1
 github.com/pkg/errors
 # github.com/pmezard/go-difflib v1.0.0


Test patch:
diff --git a/e2e/run_test.go b/e2e/run_test.go
index d3c1237..b6f3699 100644
--- a/e2e/run_test.go
+++ b/e2e/run_test.go
@@ -201,3 +201,34 @@ func TestRunWildcardCountGreaterEqualThanWorkerCount(t *testing.T) {
 
 	assertLines(t, result.Stderr(), map[int]compareFunc{})
 }
+
+func TestRunSpecialCharactersInPrefix(t *testing.T) {
+	t.Parallel()
+
+	bucket := s3BucketFromTestName(t)
+	sourceFileName := `special-chars_!@#$%^&_()_+{[_%5Cäè| __;'_,_._-中文 =/_!@#$%^&_()_+{[_%5Cäè| __;'_,_._-中文 =image.jpg`
+	targetFilePath := `./image.jpg`
+
+	s3client, s5cmd, cleanup := setup(t)
+	defer cleanup()
+
+	createBucket(t, s3client, bucket)
+	putFile(t, s3client, bucket, sourceFileName, "content")
+
+	content := []string{
+		`cp "s3://` + bucket + `/` + sourceFileName + `" ` + targetFilePath,
+	}
+	file := fs.NewFile(t, "prefix", fs.WithContent(strings.Join(content, "\n")))
+	defer file.Remove()
+
+	cmd := s5cmd("run", file.Path())
+	cmd.Timeout = time.Second
+	result := icmd.RunCmd(cmd)
+	result.Assert(t, icmd.Success)
+
+	assertLines(t, result.Stdout(), map[int]compareFunc{
+		0: equals(`cp s3://%v/%v %v`, bucket, sourceFileName, targetFilePath),
+	}, sortInput(true))
+
+	assertLines(t, result.Stderr(), map[int]compareFunc{})
+}