From 79649a1614d0d0cd01dd0a5d03fe7d837b750e47 Mon Sep 17 00:00:00 2001 From: Cole Wippern Date: Thu, 24 Oct 2019 16:30:25 -0700 Subject: [PATCH 1/3] Issue #439 double quotes in ARG value * Strip out double quotes enclosing ARG value after parsing dockerfile --- .../Dockerfile_test_arg_multi_with_quotes | 12 +++++ pkg/dockerfile/dockerfile.go | 24 +++++++++ pkg/dockerfile/dockerfile_test.go | 51 +++++++++++++++++++ 3 files changed, 87 insertions(+) create mode 100644 integration/dockerfiles/Dockerfile_test_arg_multi_with_quotes diff --git a/integration/dockerfiles/Dockerfile_test_arg_multi_with_quotes b/integration/dockerfiles/Dockerfile_test_arg_multi_with_quotes new file mode 100644 index 0000000000..8bf2524eb8 --- /dev/null +++ b/integration/dockerfiles/Dockerfile_test_arg_multi_with_quotes @@ -0,0 +1,12 @@ +ARG FILE_NAME="myFile" + +FROM busybox:latest AS builder +ARG FILE_NAME + +RUN echo $FILE_NAME && touch /$FILE_NAME.txt && stat /$FILE_NAME.txt; + +FROM busybox:latest +ARG FILE_NAME + +RUN echo $FILE_NAME && touch /$FILE_NAME.txt && stat /$FILE_NAME.txt; +COPY --from=builder /$FILE_NAME.txt / diff --git a/pkg/dockerfile/dockerfile.go b/pkg/dockerfile/dockerfile.go index 8865b17d00..1537f8c946 100644 --- a/pkg/dockerfile/dockerfile.go +++ b/pkg/dockerfile/dockerfile.go @@ -111,9 +111,33 @@ func Parse(b []byte) ([]instructions.Stage, []instructions.ArgCommand, error) { if err != nil { return nil, nil, err } + + metaArgs = stripEnclosingDoubleQuotes(metaArgs) + return stages, metaArgs, nil } +// stripEnclosingDoubleQuotes removes double quotes enclosing the value of each instructions.ArgCommand in a slice +func stripEnclosingDoubleQuotes(metaArgs []instructions.ArgCommand) []instructions.ArgCommand { + for i := range metaArgs { + arg := metaArgs[i] + v := arg.Value + if v != nil { + val := *v + if val[0] == '"' { + val = val[1:] + } + + if val[len(val)-1] == '"' { + val = val[:len(val)-1] + } + arg.Value = &val + metaArgs[i] = arg + } + } + return metaArgs +} + // targetStage returns the index of the target stage kaniko is trying to build func targetStage(stages []instructions.Stage, target string) (int, error) { if target == "" { diff --git a/pkg/dockerfile/dockerfile_test.go b/pkg/dockerfile/dockerfile_test.go index 1fa68890c6..45c26bdbb1 100644 --- a/pkg/dockerfile/dockerfile_test.go +++ b/pkg/dockerfile/dockerfile_test.go @@ -17,13 +17,64 @@ limitations under the License. package dockerfile import ( + "io/ioutil" + "os" "strconv" "testing" + "github.com/GoogleContainerTools/kaniko/pkg/config" "github.com/GoogleContainerTools/kaniko/testutil" "github.com/moby/buildkit/frontend/dockerfile/instructions" ) +func TestStagesArgValueWithQuotes(t *testing.T) { + dockerfile := ` + ARG IMAGE="ubuntu:16.04" + FROM ${IMAGE} + RUN echo hi > /hi + + FROM scratch AS second + COPY --from=0 /hi /hi2 + + FROM scratch + COPY --from=second /hi2 /hi3 + ` + tmpfile, err := ioutil.TempFile("", "Dockerfile.test") + if err != nil { + t.Fatal(err) + } + + defer os.Remove(tmpfile.Name()) + + if _, err := tmpfile.Write([]byte(dockerfile)); err != nil { + t.Fatal(err) + } + if err := tmpfile.Close(); err != nil { + t.Fatal(err) + } + + stages, err := Stages(&config.KanikoOptions{DockerfilePath: tmpfile.Name()}) + if err != nil { + t.Fatal(err) + } + + if len(stages) == 0 { + t.Fatal("length of stages expected to be greater than zero, but was zero") + + } + + if len(stages[0].MetaArgs) == 0 { + t.Fatal("length of stage[0] meta args expected to be greater than zero, but was zero") + } + + expectedVal := "ubuntu:16.04" + + arg := stages[0].MetaArgs[0] + if arg.ValueString() != expectedVal { + t.Fatalf("expected stages[0].MetaArgs[0] val to be %s but was %s", expectedVal, arg.ValueString()) + } +} + func Test_resolveStages(t *testing.T) { dockerfile := ` FROM scratch From ec2e7705c8c07a91e85531ac7a6c013e761b3c37 Mon Sep 17 00:00:00 2001 From: Cole Wippern Date: Fri, 25 Oct 2019 14:39:52 -0700 Subject: [PATCH 2/3] Issue #439 add additional tests for quotes Add additional tests to ensure that ARG values with quotes are handled properly --- .../Dockerfile_test_arg_blank_with_quotes | 12 ++ .../Dockerfile_test_arg_from_quotes | 3 + .../Dockerfile_test_arg_from_single_quotes | 3 + pkg/dockerfile/dockerfile.go | 29 +++-- pkg/dockerfile/dockerfile_test.go | 108 +++++++++++++++++- 5 files changed, 145 insertions(+), 10 deletions(-) create mode 100644 integration/dockerfiles/Dockerfile_test_arg_blank_with_quotes create mode 100644 integration/dockerfiles/Dockerfile_test_arg_from_quotes create mode 100644 integration/dockerfiles/Dockerfile_test_arg_from_single_quotes diff --git a/integration/dockerfiles/Dockerfile_test_arg_blank_with_quotes b/integration/dockerfiles/Dockerfile_test_arg_blank_with_quotes new file mode 100644 index 0000000000..a82f92601a --- /dev/null +++ b/integration/dockerfiles/Dockerfile_test_arg_blank_with_quotes @@ -0,0 +1,12 @@ +ARG FILE_NAME="" + +FROM busybox:latest AS builder +ARG FILE_NAME + +RUN echo $FILE_NAME && touch /$FILE_NAME.txt && stat /$FILE_NAME.txt; + +FROM busybox:latest +ARG FILE_NAME + +RUN echo $FILE_NAME && touch /$FILE_NAME.txt && stat /$FILE_NAME.txt; +COPY --from=builder /$FILE_NAME.txt / diff --git a/integration/dockerfiles/Dockerfile_test_arg_from_quotes b/integration/dockerfiles/Dockerfile_test_arg_from_quotes new file mode 100644 index 0000000000..80846cb92e --- /dev/null +++ b/integration/dockerfiles/Dockerfile_test_arg_from_quotes @@ -0,0 +1,3 @@ +ARG IMAGE_NAME="busybox:latest" + +FROM $IMAGE_NAME diff --git a/integration/dockerfiles/Dockerfile_test_arg_from_single_quotes b/integration/dockerfiles/Dockerfile_test_arg_from_single_quotes new file mode 100644 index 0000000000..5ce248f388 --- /dev/null +++ b/integration/dockerfiles/Dockerfile_test_arg_from_single_quotes @@ -0,0 +1,3 @@ +ARG IMAGE_NAME='busybox:latest' + +FROM $IMAGE_NAME diff --git a/pkg/dockerfile/dockerfile.go b/pkg/dockerfile/dockerfile.go index 1537f8c946..0f618529d8 100644 --- a/pkg/dockerfile/dockerfile.go +++ b/pkg/dockerfile/dockerfile.go @@ -112,30 +112,41 @@ func Parse(b []byte) ([]instructions.Stage, []instructions.ArgCommand, error) { return nil, nil, err } - metaArgs = stripEnclosingDoubleQuotes(metaArgs) + metaArgs, err = stripEnclosingQuotes(metaArgs) + if err != nil { + return nil, nil, err + } return stages, metaArgs, nil } -// stripEnclosingDoubleQuotes removes double quotes enclosing the value of each instructions.ArgCommand in a slice -func stripEnclosingDoubleQuotes(metaArgs []instructions.ArgCommand) []instructions.ArgCommand { +// stripEnclosingQuotes removes quotes enclosing the value of each instructions.ArgCommand in a slice +// if the quotes are escaped it leaves them +func stripEnclosingQuotes(metaArgs []instructions.ArgCommand) ([]instructions.ArgCommand, error) { + dbl := byte('"') + sgl := byte('\'') + for i := range metaArgs { arg := metaArgs[i] v := arg.Value if v != nil { val := *v - if val[0] == '"' { - val = val[1:] + fmt.Printf("val %s\n", val) + if (val[0] == dbl && val[len(val)-1] == dbl) || (val[0] == sgl && val[len(val)-1] == sgl) { + val = val[1 : len(val)-1] + } else if val[0:2] == `\"` && val[len(val)-2:len(val)] == `\"` { + continue + } else if val[0:2] == `\'` && val[len(val)-2:len(val)] == `\'` { + continue + } else if val[0] == dbl || val[0] == sgl || val[len(val)-1] == dbl || val[len(val)-1] == sgl { + return nil, errors.New("quotes wrapping arg values must be matched") } - if val[len(val)-1] == '"' { - val = val[:len(val)-1] - } arg.Value = &val metaArgs[i] = arg } } - return metaArgs + return metaArgs, nil } // targetStage returns the index of the target stage kaniko is trying to build diff --git a/pkg/dockerfile/dockerfile_test.go b/pkg/dockerfile/dockerfile_test.go index 45c26bdbb1..52974bd27a 100644 --- a/pkg/dockerfile/dockerfile_test.go +++ b/pkg/dockerfile/dockerfile_test.go @@ -27,7 +27,7 @@ import ( "github.com/moby/buildkit/frontend/dockerfile/instructions" ) -func TestStagesArgValueWithQuotes(t *testing.T) { +func Test_Stages_ArgValueWithQuotes(t *testing.T) { dockerfile := ` ARG IMAGE="ubuntu:16.04" FROM ${IMAGE} @@ -75,6 +75,112 @@ func TestStagesArgValueWithQuotes(t *testing.T) { } } +func Test_stripEnclosingQuotes(t *testing.T) { + type testCase struct { + name string + inArgs []instructions.ArgCommand + expected []string + success bool + } + + newArgCommand := func(key, val string) instructions.ArgCommand { + return instructions.ArgCommand{ + KeyValuePairOptional: instructions.KeyValuePairOptional{Key: key, Value: &val}, + } + } + + cases := []testCase{{ + name: "value with no enclosing quotes", + inArgs: []instructions.ArgCommand{newArgCommand("MEOW", "Purr")}, + expected: []string{"Purr"}, + success: true, + }, { + name: "value with unmatched leading double quote", + inArgs: []instructions.ArgCommand{newArgCommand("MEOW", "\"Purr")}, + }, { + name: "value with unmatched trailing double quote", + inArgs: []instructions.ArgCommand{newArgCommand("MEOW", "Purr\"")}, + }, { + name: "value with enclosing double quotes", + inArgs: []instructions.ArgCommand{newArgCommand("MEOW", "\"mrow\"")}, + expected: []string{"mrow"}, + success: true, + }, { + name: "value with unmatched leading single quote", + inArgs: []instructions.ArgCommand{newArgCommand("MEOW", "'Purr")}, + }, { + name: "value with unmatched trailing single quote", + inArgs: []instructions.ArgCommand{newArgCommand("MEOW", "Purr'")}, + }, { + name: "value with enclosing single quotes", + inArgs: []instructions.ArgCommand{newArgCommand("MEOW", "'mrow'")}, + expected: []string{"mrow"}, + success: true, + }, { + name: "blank value with enclosing double quotes", + inArgs: []instructions.ArgCommand{newArgCommand("MEOW", "\"\"")}, + expected: []string{""}, + success: true, + }, { + name: "blank value with enclosing single quotes", + inArgs: []instructions.ArgCommand{newArgCommand("MEOW", "''")}, + expected: []string{""}, + success: true, + }, { + name: "value with escaped, enclosing double quotes", + inArgs: []instructions.ArgCommand{newArgCommand("MEOW", `\"Purr\"`)}, + expected: []string{`\"Purr\"`}, + success: true, + }, { + name: "value with escaped, enclosing single quotes", + inArgs: []instructions.ArgCommand{newArgCommand("MEOW", `\'Purr\'`)}, + expected: []string{`\'Purr\'`}, + success: true, + }, { + name: "multiple values enclosed with single quotes", + inArgs: []instructions.ArgCommand{ + newArgCommand("MEOW", `'Purr'`), + newArgCommand("MEW", `'Mrow'`), + }, + expected: []string{"Purr", "Mrow"}, + success: true, + }, { + name: "no values", + success: true, + }} + + for _, test := range cases { + t.Run(test.name, func(t *testing.T) { + inArgs := test.inArgs + expected := test.expected + success := test.success + + out, err := stripEnclosingQuotes(inArgs) + if success && err != nil { + t.Fatal(err) + } + + if !success && err == nil { + t.Fatal("expected an error but none received") + } + + if len(expected) != len(out) { + t.Fatalf("Expected %d args but got %d", len(expected), len(out)) + } + + for i := range out { + if expected[i] != out[i].ValueString() { + t.Errorf( + "Expected arg at index %d to equal %v but instead equaled %v", + i, + expected[i], + out[i].ValueString()) + } + } + }) + } +} + func Test_resolveStages(t *testing.T) { dockerfile := ` FROM scratch From 0ce287d95c71abb37234c70d33114f19ba3e5dbc Mon Sep 17 00:00:00 2001 From: Cole Wippern Date: Fri, 25 Oct 2019 16:27:14 -0700 Subject: [PATCH 3/3] ISSUE #439 ci lint --- pkg/dockerfile/dockerfile.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/dockerfile/dockerfile.go b/pkg/dockerfile/dockerfile.go index 0f618529d8..ea3f5a392b 100644 --- a/pkg/dockerfile/dockerfile.go +++ b/pkg/dockerfile/dockerfile.go @@ -134,9 +134,9 @@ func stripEnclosingQuotes(metaArgs []instructions.ArgCommand) ([]instructions.Ar fmt.Printf("val %s\n", val) if (val[0] == dbl && val[len(val)-1] == dbl) || (val[0] == sgl && val[len(val)-1] == sgl) { val = val[1 : len(val)-1] - } else if val[0:2] == `\"` && val[len(val)-2:len(val)] == `\"` { + } else if val[:2] == `\"` && val[len(val)-2:] == `\"` { continue - } else if val[0:2] == `\'` && val[len(val)-2:len(val)] == `\'` { + } else if val[:2] == `\'` && val[len(val)-2:] == `\'` { continue } else if val[0] == dbl || val[0] == sgl || val[len(val)-1] == dbl || val[len(val)-1] == sgl { return nil, errors.New("quotes wrapping arg values must be matched")