Skip to content

Commit 3ec2a25

Browse files
Honny1Honny1
committed
Fix .dockerignore wildcard negation directory descent
Buildah skipped excluded directories when negation patterns contained wildcards (For example: !**/*.go), because the descent check only matched literal prefixes. Extract the literal prefix before the first wildcard and descend when the directory is at or under it; when the prefix is empty, always descend. Same bug as Docker's classic builder (moby/moby#30018, moby/moby#45608). Skip Docker conformance comparison for affected tests: client-side filtering has the same bug, and BuildKit's server-side filtering does not do parent-prefix matching so it excludes fewer files than buildah (moby/moby#42788, moby/moby#40319). Fixes: #6615 Signed-off-by: Jan Rodák <hony.com@seznam.cz>
1 parent bb54d94 commit 3ec2a25

12 files changed

Lines changed: 341 additions & 77 deletions

File tree

‎add.go‎

Lines changed: 3 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
"github.com/tonistiigi/dchapes-mode"
3030
"go.podman.io/buildah/copier"
3131
"go.podman.io/buildah/define"
32+
"go.podman.io/buildah/internal/excludes"
3233
"go.podman.io/buildah/internal/tmpdir"
3334
"go.podman.io/buildah/internal/urlsource"
3435
"go.podman.io/buildah/pkg/chrootuser"
@@ -253,29 +254,6 @@ func getURL(src string, chown *idtools.IDPair, mountpoint, renameTarget string,
253254
return nil
254255
}
255256

256-
// includeDirectoryAnyway returns true if "path" is a prefix for an exception
257-
// known to "pm". If "path" is a directory that "pm" claims matches its list
258-
// of patterns, but "pm"'s list of exclusions contains a pattern for which
259-
// "path" is a prefix, then IncludeDirectoryAnyway() will return true.
260-
// This is not always correct, because it relies on the directory part of any
261-
// exception paths to be specified without wildcards.
262-
func includeDirectoryAnyway(path string, pm *fileutils.PatternMatcher) bool {
263-
if !pm.Exclusions() {
264-
return false
265-
}
266-
prefix := strings.TrimPrefix(path, string(os.PathSeparator)) + string(os.PathSeparator)
267-
for _, pattern := range pm.Patterns() {
268-
if !pattern.Exclusion() {
269-
continue
270-
}
271-
spec := strings.TrimPrefix(pattern.String(), string(os.PathSeparator))
272-
if strings.HasPrefix(spec, prefix) {
273-
return true
274-
}
275-
}
276-
return false
277-
}
278-
279257
// globbedToGlobbable takes a pathname which might include the '[', *, or ?
280258
// characters, and converts it into a glob pattern that matches itself by
281259
// marking the '[' characters as _not_ the beginning of match ranges and
@@ -707,7 +685,7 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
707685
}
708686
// Check for dockerignore-style exclusion of this item.
709687
if rel != "." {
710-
excluded, err := pm.Matches(filepath.ToSlash(rel)) //nolint:staticcheck
688+
excluded, err := pm.IsMatch(filepath.ToSlash(rel))
711689
if err != nil {
712690
return fmt.Errorf("checking if %q(%q) is excluded: %w", globbed, rel, err)
713691
}
@@ -716,7 +694,7 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
716694
// directories can only be skipped if we don't have to allow for the
717695
// possibility of finding things to include under them
718696
globInfo := localSourceStat.Results[globbed]
719-
if !globInfo.IsDir || !includeDirectoryAnyway(rel, pm) {
697+
if !globInfo.IsDir || !excludes.ShouldDescendExcludedDir(rel, pm) {
720698
continue
721699
}
722700
} else {

‎copier/copier.go‎

Lines changed: 7 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ import (
2323
"unicode"
2424

2525
"github.com/sirupsen/logrus"
26-
"github.com/tonistiigi/dchapes-mode"
26+
mode "github.com/tonistiigi/dchapes-mode"
27+
"go.podman.io/buildah/internal/excludes"
2728
"go.podman.io/image/v5/pkg/compression"
2829
"go.podman.io/image/v5/types"
2930
"go.podman.io/storage/pkg/archive"
@@ -1527,32 +1528,12 @@ func copierHandlerGet(bulkWriter io.Writer, req request, pm *fileutils.PatternMa
15271528
}
15281529
if skip {
15291530
if d.IsDir() {
1530-
// if there are no "include
1531-
// this anyway" patterns at
1532-
// all, we don't need to
1533-
// descend into this particular
1534-
// directory if it's a directory
1535-
if !pm.Exclusions() {
1536-
return filepath.SkipDir
1537-
}
1538-
// if there are exclusion
1539-
// patterns for which this
1540-
// path is a prefix, we
1541-
// need to keep descending
1542-
for _, pattern := range pm.Patterns() {
1543-
if !pattern.Exclusion() {
1544-
continue
1545-
}
1546-
spec := strings.Trim(pattern.String(), string(os.PathSeparator))
1547-
trimmedPath := strings.Trim(skippedPath, string(os.PathSeparator))
1548-
if strings.HasPrefix(spec+string(os.PathSeparator), trimmedPath) {
1549-
// we can't just skip over
1550-
// this directory
1551-
return nil
1552-
}
1531+
// check if a negation pattern
1532+
// means we should descend into
1533+
// this excluded directory
1534+
if excludes.ShouldDescendExcludedDir(skippedPath, pm) {
1535+
return nil
15531536
}
1554-
// there are exclusions, but
1555-
// none of them apply here
15561537
return filepath.SkipDir
15571538
}
15581539
// skip this item, but if we're

‎copier/copier_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1029,7 +1029,7 @@ func testGetMultiple(t *testing.T) {
10291029
"file-b",
10301030
"link-c",
10311031
"hlink-0",
1032-
// "subdir-a/file-c", // strings.HasPrefix("**/*-c", "subdir-a/") is false
1032+
"subdir-a/file-c",
10331033
"subdir-b/",
10341034
"subdir-b/file-n",
10351035
"subdir-b/file-o",
@@ -1155,8 +1155,8 @@ func testGetMultiple(t *testing.T) {
11551155
pattern: ".",
11561156
exclude: []string{"*", "!**/*-c"},
11571157
items: []string{
1158-
// "subdir-a/file-c", // strings.HasPrefix("**/*-c", "subdir-a/") is false
11591158
"link-c",
1159+
"subdir-a/file-c",
11601160
"subdir-c/",
11611161
"subdir-c/file-p",
11621162
"subdir-c/file-q",

‎internal/excludes/excludes.go‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
package excludes
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"strings"
7+
8+
"go.podman.io/storage/pkg/fileutils"
9+
)
10+
11+
// ShouldDescendExcludedDir checks whether an excluded directory should still be
12+
// descended into because a negation pattern in pm might match files under it.
13+
// It handles literal prefix matches (e.g. !cmd/main.go for dir "cmd") and
14+
// wildcard negations (e.g. !**/*.go, !*/*.go). The wildcard check extracts
15+
// the literal prefix before the first wildcard and may intentionally
16+
// overmatch (descend into directories that won't ultimately contain matches),
17+
// which is safe because actual file-level matching happens later.
18+
func ShouldDescendExcludedDir(dirPath string, pm *fileutils.PatternMatcher) bool {
19+
if pm == nil || !pm.Exclusions() {
20+
return false
21+
}
22+
dir := filepath.ToSlash(strings.Trim(dirPath, string(os.PathSeparator)))
23+
for _, pattern := range pm.Patterns() {
24+
if !pattern.Exclusion() {
25+
continue
26+
}
27+
slashPattern := filepath.ToSlash(strings.Trim(pattern.String(), string(os.PathSeparator)))
28+
29+
// Literal-prefix check: the negation spec starts with this
30+
// directory path, for example: !cmd/main.go matches dir "cmd"
31+
if strings.HasPrefix(slashPattern, dir+"/") {
32+
return true
33+
}
34+
35+
// Wildcard-aware check: extract the literal prefix before
36+
// the first wildcard character (*, ?, [), for example: !cmd/**/*.go matches dir "cmd"
37+
// if the directory is at or under that literal prefix, a file beneath this
38+
// directory could match the negation, so keep descending.
39+
if firstWild := strings.IndexAny(slashPattern, "*?["); firstWild >= 0 {
40+
var literalPrefix string
41+
if idx := strings.LastIndex(slashPattern[:firstWild], "/"); idx >= 0 {
42+
literalPrefix = slashPattern[:idx]
43+
}
44+
if literalPrefix == "" {
45+
return true
46+
}
47+
if dir == literalPrefix || strings.HasPrefix(dir, literalPrefix+"/") {
48+
return true
49+
}
50+
}
51+
}
52+
return false
53+
}

‎internal/excludes/excludes_test.go‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
package excludes
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
"github.com/stretchr/testify/require"
8+
"go.podman.io/storage/pkg/fileutils"
9+
)
10+
11+
func TestShouldDescendExcludedDir(t *testing.T) {
12+
tests := []struct {
13+
name string
14+
path string
15+
patterns []string
16+
want bool
17+
}{
18+
{
19+
name: "nil matcher",
20+
path: "cmd",
21+
patterns: nil,
22+
want: false,
23+
},
24+
{
25+
name: "no exclusions",
26+
path: "cmd",
27+
patterns: []string{"*"},
28+
want: false,
29+
},
30+
{
31+
name: "literal prefix match",
32+
path: "cmd",
33+
patterns: []string{"*", "!cmd/main.go"},
34+
want: true,
35+
},
36+
{
37+
name: "literal prefix no match",
38+
path: "other",
39+
patterns: []string{"*", "!cmd/main.go"},
40+
want: false,
41+
},
42+
{
43+
name: "double star at start matches any dir",
44+
path: "cmd",
45+
patterns: []string{"**", "!**/*.go"},
46+
want: true,
47+
},
48+
{
49+
name: "double star at start matches nested dir",
50+
path: "cmd/sub",
51+
patterns: []string{"**", "!**/*.go"},
52+
want: true,
53+
},
54+
{
55+
name: "double star with prefix matches dir under prefix",
56+
path: "cmd/sub",
57+
patterns: []string{"**", "!cmd/**/*.go"},
58+
want: true,
59+
},
60+
{
61+
name: "double star with prefix no match for other dir",
62+
path: "other",
63+
patterns: []string{"**", "!cmd/**/*.go"},
64+
want: false,
65+
},
66+
{
67+
name: "single star at start matches any dir",
68+
path: "cmd",
69+
patterns: []string{"*", "!*/*.go"},
70+
want: true,
71+
},
72+
{
73+
name: "single star at start matches nested dir",
74+
path: "cmd/sub",
75+
patterns: []string{"*", "!*/*.go"},
76+
want: true,
77+
},
78+
{
79+
name: "single star with prefix matches dir under prefix",
80+
path: "src/pkg",
81+
patterns: []string{"**", "!src/*/*.go"},
82+
want: true,
83+
},
84+
{
85+
name: "single star with prefix no match for other dir",
86+
path: "other",
87+
patterns: []string{"**", "!src/*/*.go"},
88+
want: false,
89+
},
90+
{
91+
name: "leading slash is stripped",
92+
path: "/cmd",
93+
patterns: []string{"*", "!cmd/main.go"},
94+
want: true,
95+
},
96+
{
97+
name: "deep nested with double star prefix",
98+
path: "src/internal/pkg",
99+
patterns: []string{"**", "!src/**/*.go"},
100+
want: true,
101+
},
102+
{
103+
name: "dir prefix match is not a partial match",
104+
path: "cmds",
105+
patterns: []string{"*", "!cmd/main.go"},
106+
want: false,
107+
},
108+
{
109+
name: "wildcard mid-segment descends parent dir",
110+
path: "cmd/images",
111+
patterns: []string{"**", "!cmd/image*/main.go"},
112+
want: true,
113+
},
114+
{
115+
name: "wildcard mid-segment matches parent",
116+
path: "cmd",
117+
patterns: []string{"**", "!cmd/image*"},
118+
want: true,
119+
},
120+
{
121+
name: "question mark wildcard matches any dir",
122+
path: "cmd",
123+
patterns: []string{"**", "!cm?/*.go"},
124+
want: true,
125+
},
126+
{
127+
name: "question mark wildcard no literal prefix matches any dir",
128+
path: "other",
129+
patterns: []string{"**", "!?md/*.go"},
130+
want: true,
131+
},
132+
{
133+
name: "bracket wildcard matches dir",
134+
path: "cmd",
135+
patterns: []string{"**", "!cm[d]/*.go"},
136+
want: true,
137+
},
138+
{
139+
name: "bracket wildcard no literal prefix matches any dir",
140+
path: "other",
141+
patterns: []string{"**", "![c]md/*.go"},
142+
want: true,
143+
},
144+
{
145+
name: "bracket wildcard with prefix matches dir under prefix",
146+
path: "src/cmd",
147+
patterns: []string{"**", "!src/cm[d]/*.go"},
148+
want: true,
149+
},
150+
{
151+
name: "bracket wildcard with prefix no match for other dir",
152+
path: "other",
153+
patterns: []string{"**", "!src/cm[d]/*.go"},
154+
want: false,
155+
},
156+
{
157+
name: "non-exclusion patterns are ignored",
158+
path: "cmd",
159+
patterns: []string{"cmd/**/*.go"},
160+
want: false,
161+
},
162+
}
163+
for _, tt := range tests {
164+
t.Run(tt.name, func(t *testing.T) {
165+
var pm *fileutils.PatternMatcher
166+
if tt.patterns != nil {
167+
var err error
168+
pm, err = fileutils.NewPatternMatcher(tt.patterns)
169+
require.NoError(t, err)
170+
}
171+
got := ShouldDescendExcludedDir(tt.path, pm)
172+
assert.Equal(t, tt.want, got, "ShouldDescendExcludedDir(%q)", tt.path)
173+
})
174+
}
175+
}

0 commit comments

Comments
 (0)