From 7ab4388b17c4836b747d9c20134f3b57fc948210 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 | 29 ++++++++++++++++++++++ tests/add.bats | 24 +++++++++++++++--- tests/bud/copy-globs/Containerfile.missing | 2 +- tests/copy.bats | 26 +++++++++++++++---- 4 files changed, 72 insertions(+), 9 deletions(-) diff --git a/add.go b/add.go index 1f820ea551c..c1869276ee7 100644 --- a/add.go +++ b/add.go @@ -188,10 +188,39 @@ 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.HasSuffix(sources[0], "*") { + 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 a1232957199..8c9fd3490b1 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,22 @@ 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 + + run_buildah from $WITH_POLICY_JSON scratch + cid=$output + run_buildah mount $cid + root=$output + run_buildah config --workingdir / $cid + 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 2f300d2f443..112c2a2d60f 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 $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