Repository: containers/buildah (language: go)

Issue:
Cannot escape square brackets in build of Dockerfile with COPY instruction
<!--
If you are reporting a new issue, make sure that we do not have any duplicates
already open. You can ensure this by searching the issue list for this
repository. If there is a duplicate, please close your issue and add a comment
to the existing issue instead.

If you suspect your issue is a bug, please edit your issue description to
include the BUG REPORT INFORMATION shown below. If you fail to provide this
information within 7 days, we cannot debug your issue and will close it. We
will, however, reopen it if you later provide the information.

---------------------------------------------------
BUG REPORT INFORMATION
---------------------------------------------------
Use the commands below to provide key information from your environment:
You do NOT have to include this information if this is a FEATURE REQUEST
-->

**Description**

I am trying to build a nextjs app Dockerfile.  The Dockerfile contains a COPY instruction with a source path that contains square brackets.  I quickly found the escape sequence on the Dockerfile reference docs: https://docs.docker.com/reference/dockerfile/#copy, and from the docs:

"When adding files or directories that contain special characters (such as [ and ]), you need to escape those paths following the Golang rules to prevent them from being treated as a matching pattern. For example, to add a file named arr[0].txt, use the following;"

Applying the escape sequence allowed me to build the container image using the docker runtime, however I was not able to do the same using the buildah runtime.

<!--
Briefly describe the problem you are having in a few paragraphs.
-->

**Steps to reproduce the issue:**
1. In a fresh directory, create an empty file at the path app/[xyz]/file.  Screenshot of my directory:
![image](https://github.com/user-attachments/assets/18867f33-b9d9-4dcd-94c3-da4bcb94edb2)

2. Create a Dockerfile with the following contents:
```
FROM node
WORKDIR /app
COPY ./app/[[]xyz]/file ./app/[xyz]/file
```

Screenshot of Dockerfile:
![image](https://github.com/user-attachments/assets/9eadef6d-f2c5-41db-8c82-64eb5dbe4c98)


3. run `buildah build -t my-image .`

**Describe the results you received:**

Output of step 3 command:
```
STEP 1/3: FROM node
STEP 2/3: WORKDIR /app
STEP 3/3: COPY app/[[]xyz]/file /app/[xyz]file
Error: building at STEP "COPY app/[[]xyz]/file /app/[xyz]file": reading "/app/app/[[]xyz]/file": copier: get: globs [/app/[xyz]/file] matched nothing (0 filtered out): no such file or directory
```


**Describe the results you expected:**

Expected container image to build successfully.


**Output of `rpm -q buildah` or `apt list buildah`:**

```
buildah-1.36.0-1.fc40.aarch64
```

**Output of `buildah version`:**

```
Version:         1.36.0
Go Version:      go1.22.3
Image Spec:      1.1.0
Runtime Spec:    1.2.0
CNI Spec:        1.0.0
libcni Version:  
image Version:   5.31.0
Git Commit:      
Built:           Mon May 27 13:11:32 2024
OS/Arch:         linux/arm64
BuildPlatform:   linux/arm64/v8
```

**Output of `podman version` if reporting a `podman build` issue:**

```
N/A
```

**Output of `cat /etc/*release`:**

```
Fedora release 40 (Forty)
NAME="Fedora Linux"
VERSION="40 (Container Image)"
ID=fedora
VERSION_ID=40
VERSION_CODENAME=""
PLATFORM_ID="platform:f40"
PRETTY_NAME="Fedora Linux 40 (Container Image)"
ANSI_COLOR="0;38;2;60;110;180"
LOGO=fedora-logo-icon
CPE_NAME="cpe:/o:fedoraproject:fedora:40"
DEFAULT_HOSTNAME="fedora"
HOME_URL="https://fedoraproject.org/"
DOCUMENTATION_URL="https://docs.fedoraproject.org/en-US/fedora/f40/system-administrators-guide/"
SUPPORT_URL="https://ask.fedoraproject.org/"
BUG_REPORT_URL="https://bugzilla.redhat.com/"
REDHAT_BUGZILLA_PRODUCT="Fedora"
REDHAT_BUGZILLA_PRODUCT_VERSION=40
REDHAT_SUPPORT_PRODUCT="Fedora"
REDHAT_SUPPORT_PRODUCT_VERSION=40
SUPPORT_END=2025-05-13
VARIANT="Container Image"
VARIANT_ID=container
Fedora release 40 (Forty)
Fedora release 40 (Forty)
```

**Output of `uname -a`:**

```
Linux 2a09f66d6d28 6.8.0-31-generic #31-Ubuntu SMP PREEMPT_DYNAMIC Sat Apr 20 02:32:42 UTC 2024 aarch64 GNU/Linux
```

**Output of `cat /etc/containers/storage.conf`:**

```
# This file is the configuration file for all tools
# that use the containers/storage library. The storage.conf file
# overrides all other storage.conf files. Container engines using the
# container/storage library do not inherit fields from other storage.conf
# files.
#
#  Note: The storage.conf file overrides other storage.conf files based on this precedence:
#      /usr/containers/storage.conf
#      /etc/containers/storage.conf
#      $HOME/.config/containers/storage.conf
#      $XDG_CONFIG_HOME/containers/storage.conf (If XDG_CONFIG_HOME is set)
# See man 5 containers-storage.conf for more information
# The "container storage" table contains all of the server options.
[storage]

# Default Storage Driver, Must be set for proper operation.
driver = "overlay"

# Temporary storage location
runroot = "/run/containers/storage"

# Primary Read/Write location of container storage
# When changing the graphroot location on an SELINUX system, you must
# ensure  the labeling matches the default locations labels with the
# following commands:
# semanage fcontext -a -e /var/lib/containers/storage /NEWSTORAGEPATH
# restorecon -R -v /NEWSTORAGEPATH
graphroot = "/var/lib/containers/storage"

# Optional alternate location of image store if a location separate from the
# container store is required. If set, it must be different than graphroot.
# imagestore = ""


# Storage path for rootless users
#
# rootless_storage_path = "$HOME/.local/share/containers/storage"

# Transient store mode makes all container metadata be saved in temporary storage
# (i.e. runroot above). This is faster, but doesn't persist across reboots.
# Additional garbage collection must also be performed at boot-time, so this
# option should remain disabled in most configurations.
# transient_store = true

[storage.options]
# Storage options to be passed to underlying storage drivers

# AdditionalImageStores is used to pass paths to additional Read/Only image stores
# Must be comma separated list.
additionalimagestores = [
"/var/lib/shared",
"/usr/lib/containers/storage",
]

# Allows specification of how storage is populated when pulling images. This
# option can speed the pulling process of images compressed with format
# zstd:chunked. Containers/storage looks for files within images that are being
# pulled from a container registry that were previously pulled to the host.  It
# can copy or create a hard link to the existing file when it finds them,
# eliminating the need to pull them from the container registry. These options
# can deduplicate pulling of content, disk storage of content and can allow the
# kernel to use less memory when running containers.

# containers/storage supports four keys
#   * enable_partial_images="true" | "false"
#     Tells containers/storage to look for files previously pulled in storage
#     rather then always pulling them from the container registry.
#   * use_hard_links = "false" | "true"
#     Tells containers/storage to use hard links rather then create new files in
#     the image, if an identical file already existed in storage.
#   * ostree_repos = ""
#     Tells containers/storage where an ostree repository exists that might have
#     previously pulled content which can be used when attempting to avoid
#     pulling content from the container registry
#   * convert_images = "false" | "true"
#     If set to true, containers/storage will convert images to a
#     format compatible with partial pulls in order to take advantage
#     of local deduplication and hard linking.  It is an expensive
#     operation so it is not enabled by default.
pull_options = {enable_partial_images = "true", use_hard_links = "false", ostree_repos=""}

# Remap-UIDs/GIDs is the mapping from UIDs/GIDs as they should appear inside of
# a container, to the UIDs/GIDs as they should appear outside of the container,
# and the length of the range of UIDs/GIDs.  Additional mapped sets can be
# listed and will be heeded by libraries, but there are limits to the number of
# mappings which the kernel will allow when you later attempt to run a
# container.
#
# remap-uids = "0:1668442479:65536"
# remap-gids = "0:1668442479:65536"

# Remap-User/Group is a user name which can be used to look up one or more UID/GID
# ranges in the /etc/subuid or /etc/subgid file.  Mappings are set up starting
# with an in-container ID of 0 and then a host-level ID taken from the lowest
# range that matches the specified name, and using the length of that range.
# Additional ranges are then assigned, using the ranges which specify the
# lowest host-level IDs first, to the lowest not-yet-mapped in-container ID,
# until all of the entries have been used for maps. This setting overrides the
# Remap-UIDs/GIDs setting.
#
# remap-user = "containers"
# remap-group = "containers"

# Root-auto-userns-user is a user name which can be used to look up one or more UID/GID
# ranges in the /etc/subuid and /etc/subgid file.  These ranges will be partitioned
# to containers configured to create automatically a user namespace.  Containers
# configured to automatically create a user namespace can still overlap with containers
# having an explicit mapping set.
# This setting is ignored when running as rootless.
# root-auto-userns-user = "storage"
#
# Auto-userns-min-size is the minimum size for a user namespace created automatically.
# auto-userns-min-size=1024
#
# Auto-userns-max-size is the maximum size for a user namespace created automatically.
# auto-userns-max-size=65536

[storage.options.overlay]
# ignore_chown_errors can be set to allow a non privileged user running with
# a single UID within a user namespace to run containers. The user can pull
# and use any image even those with multiple uids.  Note multiple UIDs will be
# squashed down to the default uid in the container.  These images will have no
# separation between the users in the container. Only supported for the overlay
# and vfs drivers.
#ignore_chown_errors = "false"

# Inodes is used to set a maximum inodes of the container image.
# inodes = ""

# Path to an helper program to use for mounting the file system instead of mounting it
# directly.
mount_program = "/usr/bin/fuse-overlayfs"

# mountopt specifies comma separated list of extra mount options
mountopt = "nodev,fsync=0"

# Set to skip a PRIVATE bind mount on the storage home directory.
# skip_mount_home = "false"

# Set to use composefs to mount data layers with overlay.
# use_composefs = "false"

# Size is used to set a maximum size of the container image.
# size = ""

# ForceMask specifies the permissions mask that is used for new files and
# directories.
#
# The values "shared" and "private" are accepted.
# Octal permission masks are also accepted.
#
#  "": No value specified.
#     All files/directories, get set with the permissions identified within the
#     image.
#  "private": it is equivalent to 0700.
#     All files/directories get set with 0700 permissions.  The owner has rwx
#     access to the files. No other users on the system can access the files.
#     This setting could be used with networked based homedirs.
#  "shared": it is equivalent to 0755.
#     The owner has rwx access to the files and everyone else can read, access
#     and execute them. This setting is useful for sharing containers storage
#     with other users.  For instance have a storage owned by root but shared
#     to rootless users as an additional store.
#     NOTE:  All files within the image are made readable and executable by any
#     user on the system. Even /etc/shadow within your image is now readable by
#     any user.
#
#   OCTAL: Users can experiment with other OCTAL Permissions.
#
#  Note: The force_mask Flag is an experimental feature, it could change in the
#  future.  When "force_mask" is set the original permission mask is stored in
#  the "user.containers.override_stat" xattr and the "mount_program" option must
#  be specified. Mount programs like "/usr/bin/fuse-overlayfs" present the
#  extended attribute permissions to processes within containers rather than the
#  "force_mask"  permissions.
#
# force_mask = ""
```



Discussion / hints:

Fix patch:
diff --git a/add.go b/add.go
index 233fa2b73f5..1aa0feaa948 100644
--- a/add.go
+++ b/add.go
@@ -230,6 +230,18 @@ func includeDirectoryAnyway(path string, pm *fileutils.PatternMatcher) bool {
 	return false
 }
 
+// globbedToGlobbable takes a pathname which might include the '[', *, or ?
+// characters, and converts it into a glob pattern that matches itself by
+// marking the '[' characters as _not_ the beginning of match ranges and
+// escaping the * and ? characters.
+func globbedToGlobbable(glob string) string {
+	result := glob
+	result = strings.ReplaceAll(result, "[", "[[]")
+	result = strings.ReplaceAll(result, "?", "\\?")
+	result = strings.ReplaceAll(result, "*", "\\*")
+	return result
+}
+
 // 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.
@@ -517,27 +529,27 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
 
 		// Iterate through every item that matched the glob.
 		itemsCopied := 0
-		for _, glob := range localSourceStat.Globbed {
-			rel := glob
-			if filepath.IsAbs(glob) {
-				if rel, err = filepath.Rel(contextDir, glob); err != nil {
-					return fmt.Errorf("computing path of %q relative to %q: %w", glob, contextDir, err)
+		for _, globbed := range localSourceStat.Globbed {
+			rel := globbed
+			if filepath.IsAbs(globbed) {
+				if rel, err = filepath.Rel(contextDir, globbed); err != nil {
+					return fmt.Errorf("computing path of %q relative to %q: %w", globbed, contextDir, err)
 				}
 			}
 			if strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
-				return fmt.Errorf("possible escaping context directory error: %q is outside of %q", glob, contextDir)
+				return fmt.Errorf("possible escaping context directory error: %q is outside of %q", globbed, contextDir)
 			}
 			// Check for dockerignore-style exclusion of this item.
 			if rel != "." {
 				excluded, err := pm.Matches(filepath.ToSlash(rel)) // nolint:staticcheck
 				if err != nil {
-					return fmt.Errorf("checking if %q(%q) is excluded: %w", glob, rel, err)
+					return fmt.Errorf("checking if %q(%q) is excluded: %w", globbed, rel, err)
 				}
 				if excluded {
 					// non-directories that are excluded are excluded, no question, but
 					// directories can only be skipped if we don't have to allow for the
 					// possibility of finding things to include under them
-					globInfo := localSourceStat.Results[glob]
+					globInfo := localSourceStat.Results[globbed]
 					if !globInfo.IsDir || !includeDirectoryAnyway(rel, pm) {
 						continue
 					}
@@ -554,7 +566,7 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
 				// due to potentially not having anything in the tarstream that we passed.
 				itemsCopied++
 			}
-			st := localSourceStat.Results[glob]
+			st := localSourceStat.Results[globbed]
 			pipeReader, pipeWriter := io.Pipe()
 			wg.Add(1)
 			go func() {
@@ -584,7 +596,7 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
 					StripSetgidBit: options.StripSetgidBit,
 					StripStickyBit: options.StripStickyBit,
 				}
-				getErr = copier.Get(contextDir, contextDir, getOptions, []string{glob}, writer)
+				getErr = copier.Get(contextDir, contextDir, getOptions, []string{globbedToGlobbable(globbed)}, writer)
 				closeErr = writer.Close()
 				if renameTarget != "" && renamedItems > 1 {
 					renameErr = fmt.Errorf("internal error: renamed %d items when we expected to only rename 1", renamedItems)


Test patch:
diff --git a/tests/conformance/conformance_test.go b/tests/conformance/conformance_test.go
index 04da60b5b68..e4bcb46780a 100644
--- a/tests/conformance/conformance_test.go
+++ b/tests/conformance/conformance_test.go
@@ -1421,6 +1421,13 @@ var internalTestCases = []testCase{
 		dockerRegex:  "(?s)RUN env.*?Running in [0-9a-z]+(.*?)---",
 	},
 
+	{
+		name:              "copy-escape-glob",
+		contextDir:        "copy-escape-glob",
+		fsSkip:            []string{"(dir):app:mtime", "(dir):app2:mtime", "(dir):app3:mtime", "(dir):app4:mtime", "(dir):app5:mtime"},
+		dockerUseBuildKit: true,
+	},
+
 	{
 		name:         "copy file to root",
 		dockerfile:   "Dockerfile.copyfrom_1",
diff --git a/tests/conformance/testdata/copy-escape-glob/Dockerfile b/tests/conformance/testdata/copy-escape-glob/Dockerfile
new file mode 100644
index 00000000000..a34f1746490
--- /dev/null
+++ b/tests/conformance/testdata/copy-escape-glob/Dockerfile
@@ -0,0 +1,12 @@
+FROM scratch
+WORKDIR /app
+# The "[]" sequence immediately after the "[" signals to filepath.Glob() that
+# it is _not_ the beginning of a "class" chunk in the globbing pattern.  N.B.
+# this is a golang-specific way to do it, different from what glob(7) suggests.
+COPY ./app/[[]xyz]/file.txt /app/
+COPY ./app/[[]xyz]/[[]abc]/file.txt /app2/
+COPY ./app/* /app3/
+# This should only match one file.
+COPY ./app/n\?pe/file.txt /app4/file.txt
+# This should only match one file.
+COPY ./app/st\*uv/file.txt /app5/file.txt
diff --git a/tests/conformance/testdata/copy-escape-glob/app/[xyz]/[abc]/file.txt b/tests/conformance/testdata/copy-escape-glob/app/[xyz]/[abc]/file.txt
new file mode 100644
index 00000000000..27bc7aa5174
--- /dev/null
+++ b/tests/conformance/testdata/copy-escape-glob/app/[xyz]/[abc]/file.txt
@@ -0,0 +1,1 @@
+also a file
diff --git a/tests/conformance/testdata/copy-escape-glob/app/[xyz]/file.txt b/tests/conformance/testdata/copy-escape-glob/app/[xyz]/file.txt
new file mode 100644
index 00000000000..02f6335fc4f
--- /dev/null
+++ b/tests/conformance/testdata/copy-escape-glob/app/[xyz]/file.txt
@@ -0,0 +1,1 @@
+a file
diff --git a/tests/conformance/testdata/copy-escape-glob/app/jkl?/file.txt b/tests/conformance/testdata/copy-escape-glob/app/jkl?/file.txt
new file mode 100644
index 00000000000..9b24da92a91
--- /dev/null
+++ b/tests/conformance/testdata/copy-escape-glob/app/jkl?/file.txt
@@ -0,0 +1,1 @@
+another
diff --git a/tests/conformance/testdata/copy-escape-glob/app/n?pe/file.txt b/tests/conformance/testdata/copy-escape-glob/app/n?pe/file.txt
new file mode 100644
index 00000000000..68776e3ff16
--- /dev/null
+++ b/tests/conformance/testdata/copy-escape-glob/app/n?pe/file.txt
@@ -0,0 +1,1 @@
+and also
diff --git a/tests/conformance/testdata/copy-escape-glob/app/nope/file.txt b/tests/conformance/testdata/copy-escape-glob/app/nope/file.txt
new file mode 100644
index 00000000000..c08f21e6746
--- /dev/null
+++ b/tests/conformance/testdata/copy-escape-glob/app/nope/file.txt
@@ -0,0 +1,1 @@
+but not
diff --git a/tests/conformance/testdata/copy-escape-glob/app/st*uv/file.txt b/tests/conformance/testdata/copy-escape-glob/app/st*uv/file.txt
new file mode 100644
index 00000000000..0f14a3308ca
--- /dev/null
+++ b/tests/conformance/testdata/copy-escape-glob/app/st*uv/file.txt
@@ -0,0 +1,1 @@
+and yet
diff --git a/tests/conformance/testdata/copy-escape-glob/app/strauv/file.txt b/tests/conformance/testdata/copy-escape-glob/app/strauv/file.txt
new file mode 100644
index 00000000000..4ac6b8a6f2a
--- /dev/null
+++ b/tests/conformance/testdata/copy-escape-glob/app/strauv/file.txt
@@ -0,0 +1,1 @@
+not this one