From 0b4590cc11b965f7f228366241afdc8f3d21b6b4 Mon Sep 17 00:00:00 2001 From: Aditya R Date: Thu, 11 Aug 2022 10:15:52 +0530 Subject: [PATCH] add,copy: if multiple sources then dest must end with a slash When using `ADD` or `COPY` enforce the condition that destination must look like a directory by making sure that destination ends with a slash `/`. This ensures that we don't hit undefined behaviour for example with the use case `COPY some/* /oncontainer` it is not clear that `oncontainer` will be a directory or a file, users expect it to be a directory if `*` wildcard resolves to multiple files and logically if wildcard resolves to single file then it should be a directory, since this condition is undefined and not in parity with docker so lets drop support for it. Docker's defined rule: * If multiple resources are specified, either directly or due to the use of a wildcard, then must be a directory, and it must end with a slash /. Reference: https://docs.docker.com/engine/reference/builder/#add Closes: https://github.com/containers/buildah/issues/4167 Signed-off-by: Aditya R --- add.go | 35 ++++++++++++++++++++++ tests/add.bats | 34 +++++++++++++++++++-- tests/bud/copy-globs/Containerfile.missing | 2 +- tests/copy.bats | 26 ++++++++++++---- 4 files changed, 88 insertions(+), 9 deletions(-) diff --git a/add.go b/add.go index 1bb6c85009d..b8fdd031b4d 100644 --- a/add.go +++ b/add.go @@ -187,10 +187,45 @@ func includeDirectoryAnyway(path string, pm *fileutils.PatternMatcher) bool { return false } +// validateSourceAndDest performs a sanity check on the case that if more +// than one source is provided then destination must end with a slash (`/`), +// which signifies that destination is a directory and similarly function +// returns error if destination does not ends with a slash (`/`). +// Following function can contain more validation cases in future. +func validateSourceAndDest(destination string, sources []string) error { + multipleSources := false + if len(sources) > 1 { + multipleSources = true + } else { + // Check length before accessing `0` + // to avoid `index out of range`. + if len(sources) == 1 { + if strings.Contains(sources[0], "*") { + matches, err := filepath.Glob(sources[0]) + if err != nil { + return err + } + if len(matches) > 1 { + multipleSources = true + } + } + } + } + if multipleSources { + if !strings.HasSuffix(destination, "/") { + return errors.New("adding multiple sources to non-directory destination") + } + } + return nil +} + // Add copies the contents of the specified sources into the container's root // filesystem, optionally extracting contents of local files that look like // non-empty archives. func (b *Builder) Add(destination string, extract bool, options AddAndCopyOptions, sources ...string) error { + if err := validateSourceAndDest(destination, sources); err != nil { + return err + } mountPoint, err := b.Mount(b.MountLabel) if err != nil { return err diff --git a/tests/add.bats b/tests/add.bats index 100051ee1ef..d38c55625db 100644 --- a/tests/add.bats +++ b/tests/add.bats @@ -28,11 +28,13 @@ load helpers # Copy a file to a specific subdirectory run_buildah add $cid ${TEST_SCRATCH_DIR}/randomfile /subdir # Copy two files to a specific subdirectory - run_buildah add $cid ${TEST_SCRATCH_DIR}/randomfile ${TEST_SCRATCH_DIR}/other-randomfile /other-subdir + run_buildah add $cid ${TEST_SCRATCH_DIR}/randomfile ${TEST_SCRATCH_DIR}/other-randomfile /other-subdir/ # Copy two files to a specific location, which succeeds because we can create it as a directory. - run_buildah add $cid ${TEST_SCRATCH_DIR}/randomfile ${TEST_SCRATCH_DIR}/other-randomfile /notthereyet-subdir + run_buildah add $cid ${TEST_SCRATCH_DIR}/randomfile ${TEST_SCRATCH_DIR}/other-randomfile /notthereyet-subdir/ + # Add with a wildcard and destination dir does not exists + run_buildah add $cid "${TEST_SCRATCH_DIR}/*" /notthereyet2-subdir/ # Copy two files to a specific location, which fails because it's not a directory. - run_buildah 125 add $cid ${TEST_SCRATCH_DIR}/randomfile ${TEST_SCRATCH_DIR}/other-randomfile /randomfile + run_buildah 125 add $cid ${TEST_SCRATCH_DIR}/randomfile ${TEST_SCRATCH_DIR}/other-randomfile /randomfile/ # Copy a file to a different working directory run_buildah config --workingdir=/cwd $cid run_buildah add $cid ${TEST_SCRATCH_DIR}/randomfile @@ -58,6 +60,32 @@ load helpers run_buildah rm $newcid } +@test "add-multiple-files to destination not ending with slash" { + createrandom ${TEST_SCRATCH_DIR}/randomfile + createrandom ${TEST_SCRATCH_DIR}/other-randomfile + createrandom ${TEST_SCRATCH_DIR}/third-randomfile + createrandom ${TEST_SCRATCH_DIR}/hello + createrandom ${TEST_SCRATCH_DIR}/world1 + createrandom ${TEST_SCRATCH_DIR}/world2 + + run_buildah from $WITH_POLICY_JSON scratch + cid=$output + run_buildah mount $cid + root=$output + run_buildah config --workingdir / $cid + # Match buildkit parity + # This should be successful since `BASE/hello*` only resolves to one file. + run_buildah add $cid "${TEST_SCRATCH_DIR}/hello*" /test + # This should fail since `BASE/world*` resolves to more than one file. + run_buildah 125 add $cid "${TEST_SCRATCH_DIR}/world*" /test + expect_output --substring "adding multiple sources to non-directory destination" + # This should fail since `BASE/*` resolves to more than one file. + run_buildah 125 add $cid "${TEST_SCRATCH_DIR}/*" /test + expect_output --substring "adding multiple sources to non-directory destination" + run_buildah 125 add $cid ${TEST_SCRATCH_DIR}/randomefile ${TEST_SCRATCH_DIR}/other-randomfile /test + expect_output --substring "adding multiple sources to non-directory destination" +} + @test "add-local-archive" { createrandom ${TEST_SCRATCH_DIR}/randomfile createrandom ${TEST_SCRATCH_DIR}/other-randomfile diff --git a/tests/bud/copy-globs/Containerfile.missing b/tests/bud/copy-globs/Containerfile.missing index 5185cd47f0b..85af286222a 100644 --- a/tests/bud/copy-globs/Containerfile.missing +++ b/tests/bud/copy-globs/Containerfile.missing @@ -1,3 +1,3 @@ FROM scratch # No match for *foo, but *txt exists so should succeed -COPY *foo *.txt /testdir +COPY *foo *.txt /testdir/ diff --git a/tests/copy.bats b/tests/copy.bats index 0b917c9dd9d..9c1528ab0a6 100644 --- a/tests/copy.bats +++ b/tests/copy.bats @@ -26,11 +26,11 @@ load helpers # copy ${TEST_SCRATCH_DIR}/randomfile to a file of the same name in the container's working directory run_buildah copy --retry 4 --retry-delay 4s $cid ${TEST_SCRATCH_DIR}/randomfile # copy ${TEST_SCRATCH_DIR}/other-randomfile and ${TEST_SCRATCH_DIR}/third-randomfile to a new directory named ${TEST_SCRATCH_DIR}/randomfile in the container - run_buildah copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile ${TEST_SCRATCH_DIR}/randomfile + run_buildah copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile ${TEST_SCRATCH_DIR}/randomfile/ # try to copy ${TEST_SCRATCH_DIR}/other-randomfile and ${TEST_SCRATCH_DIR}/third-randomfile to a /randomfile, which already exists and is a file - run_buildah 125 copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile /randomfile + run_buildah 125 copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile /randomfile/ # copy ${TEST_SCRATCH_DIR}/other-randomfile and ${TEST_SCRATCH_DIR}/third-randomfile to previously-created directory named ${TEST_SCRATCH_DIR}/randomfile in the container - run_buildah copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile ${TEST_SCRATCH_DIR}/randomfile + run_buildah copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile ${TEST_SCRATCH_DIR}/randomfile/ run_buildah rm $cid _prefetch alpine @@ -40,7 +40,7 @@ load helpers root=$output run_buildah config --workingdir / $cid run_buildah copy $cid ${TEST_SCRATCH_DIR}/randomfile - run_buildah copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile ${TEST_SCRATCH_DIR}/randomfile /etc + run_buildah copy $cid ${TEST_SCRATCH_DIR}/other-randomfile ${TEST_SCRATCH_DIR}/third-randomfile ${TEST_SCRATCH_DIR}/randomfile /etc/ run_buildah rm $cid run_buildah from --quiet --pull=false $WITH_POLICY_JSON alpine @@ -48,7 +48,7 @@ load helpers run_buildah mount $cid root=$output run_buildah config --workingdir / $cid - run_buildah copy $cid "${TEST_SCRATCH_DIR}/*randomfile" /etc + run_buildah copy $cid "${TEST_SCRATCH_DIR}/*randomfile" /etc/ (cd ${TEST_SCRATCH_DIR}; for i in *randomfile; do cmp $i ${root}/etc/$i; done) } @@ -78,6 +78,22 @@ load helpers cmp ${TEST_SCRATCH_DIR}/other-randomfile $newroot/other-randomfile } +@test "copy-multiple-files to destination not ending with slash" { + createrandom ${TEST_SCRATCH_DIR}/randomfile + createrandom ${TEST_SCRATCH_DIR}/other-randomfile + createrandom ${TEST_SCRATCH_DIR}/third-randomfile + + run_buildah from $WITH_POLICY_JSON scratch + cid=$output + run_buildah mount $cid + root=$output + run_buildah config --workingdir / $cid + run_buildah 125 copy $cid ${TEST_SCRATCH_DIR}/* /test + expect_output --substring "adding multiple sources to non-directory destination" + run_buildah 125 copy $cid ${TEST_SCRATCH_DIR}/randomefile ${TEST_SCRATCH_DIR}/other-randomfile /test + expect_output --substring "adding multiple sources to non-directory destination" +} + @test "copy-local-subdirectory" { mkdir -p ${TEST_SCRATCH_DIR}/subdir createrandom ${TEST_SCRATCH_DIR}/subdir/randomfile