Skip to content

Commit 4049482

Browse files
AkihiroSudaAkihiroSuda
committed
dockerfile2llb: check GitURLOpts error
Prior to this commit, `ADD https://github.com/moby/example.git?invalid=42` was fetching a single HTML file without causing an error. Now it returns an error `unexpected query "invalid"`. Hint: use `git show --ignore-all-space` to review this commit. Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
1 parent 79c8bd0 commit 4049482

2 files changed

Lines changed: 114 additions & 83 deletions

File tree

‎frontend/dockerfile/dockerfile2llb/convert.go‎

Lines changed: 102 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -1465,7 +1465,15 @@ func dispatchCopy(d *dispatchState, cfg copyConfig) error {
14651465
if len(cfg.params.SourcePaths) != 1 {
14661466
return errors.New("checksum can't be specified for multiple sources")
14671467
}
1468-
if !isHTTPSource(cfg.params.SourcePaths[0]) && !isGitSource(cfg.params.SourcePaths[0]) {
1468+
isHTTPSourceV, err := isHTTPSource(cfg.params.SourcePaths[0])
1469+
if err != nil {
1470+
return err
1471+
}
1472+
isGitSourceV, err := isGitSource(cfg.params.SourcePaths[0])
1473+
if err != nil {
1474+
return err
1475+
}
1476+
if !isHTTPSourceV && !isGitSourceV {
14691477
return errors.New("checksum requires HTTP(S) or Git sources")
14701478
}
14711479
}
@@ -1530,96 +1538,102 @@ func dispatchCopy(d *dispatchState, cfg copyConfig) error {
15301538
} else {
15311539
a = a.Copy(st, "/", dest, opts...)
15321540
}
1533-
} else if isHTTPSource(src) {
1534-
if !cfg.isAddCommand {
1535-
return errors.New("source can't be a URL for COPY")
1541+
} else {
1542+
isHTTPSourceV, err := isHTTPSource(src)
1543+
if err != nil {
1544+
return err
15361545
}
1546+
if isHTTPSourceV {
1547+
if !cfg.isAddCommand {
1548+
return errors.New("source can't be a URL for COPY")
1549+
}
15371550

1538-
// Resources from remote URLs are not decompressed.
1539-
// https://docs.docker.com/engine/reference/builder/#add
1540-
//
1541-
// Note: mixing up remote archives and local archives in a single ADD instruction
1542-
// would result in undefined behavior: https://github.com/moby/buildkit/pull/387#discussion_r189494717
1543-
u, err := url.Parse(src)
1544-
f := "__unnamed__"
1545-
if err == nil {
1546-
if base := path.Base(u.Path); base != "." && base != "/" {
1547-
f = base
1551+
// Resources from remote URLs are not decompressed.
1552+
// https://docs.docker.com/engine/reference/builder/#add
1553+
//
1554+
// Note: mixing up remote archives and local archives in a single ADD instruction
1555+
// would result in undefined behavior: https://github.com/moby/buildkit/pull/387#discussion_r189494717
1556+
u, err := url.Parse(src)
1557+
f := "__unnamed__"
1558+
if err == nil {
1559+
if base := path.Base(u.Path); base != "." && base != "/" {
1560+
f = base
1561+
}
15481562
}
1549-
}
15501563

1551-
var checksum digest.Digest
1552-
if cfg.checksum != "" {
1553-
checksum, err = digest.Parse(cfg.checksum)
1554-
if err != nil {
1555-
return err
1564+
var checksum digest.Digest
1565+
if cfg.checksum != "" {
1566+
checksum, err = digest.Parse(cfg.checksum)
1567+
if err != nil {
1568+
return err
1569+
}
15561570
}
1557-
}
15581571

1559-
st := llb.HTTP(src, llb.Filename(f), llb.WithCustomName(pgName), llb.Checksum(checksum), dfCmd(cfg.params))
1572+
st := llb.HTTP(src, llb.Filename(f), llb.WithCustomName(pgName), llb.Checksum(checksum), dfCmd(cfg.params))
15601573

1561-
var unpack bool
1562-
if cfg.unpack != nil {
1563-
unpack = *cfg.unpack
1564-
}
1574+
var unpack bool
1575+
if cfg.unpack != nil {
1576+
unpack = *cfg.unpack
1577+
}
15651578

1566-
opts := append([]llb.CopyOption{&llb.CopyInfo{
1567-
Mode: chopt,
1568-
CreateDestPath: true,
1569-
AttemptUnpack: unpack,
1570-
}}, copyOpt...)
1579+
opts := append([]llb.CopyOption{&llb.CopyInfo{
1580+
Mode: chopt,
1581+
CreateDestPath: true,
1582+
AttemptUnpack: unpack,
1583+
}}, copyOpt...)
15711584

1572-
if a == nil {
1573-
a = llb.Copy(st, f, dest, opts...)
1574-
} else {
1575-
a = a.Copy(st, f, dest, opts...)
1576-
}
1577-
} else {
1578-
validateCopySourcePath(src, &cfg)
1579-
var patterns []string
1580-
if cfg.parents {
1581-
// detect optional pivot point
1582-
parent, pattern, ok := strings.Cut(src, "/./")
1583-
if !ok {
1584-
pattern = src
1585-
src = "/"
1585+
if a == nil {
1586+
a = llb.Copy(st, f, dest, opts...)
15861587
} else {
1587-
src = parent
1588+
a = a.Copy(st, f, dest, opts...)
15881589
}
1590+
} else {
1591+
validateCopySourcePath(src, &cfg)
1592+
var patterns []string
1593+
if cfg.parents {
1594+
// detect optional pivot point
1595+
parent, pattern, ok := strings.Cut(src, "/./")
1596+
if !ok {
1597+
pattern = src
1598+
src = "/"
1599+
} else {
1600+
src = parent
1601+
}
15891602

1590-
pattern, err = system.NormalizePath("/", pattern, d.platform.OS, false)
1603+
pattern, err = system.NormalizePath("/", pattern, d.platform.OS, false)
1604+
if err != nil {
1605+
return errors.Wrap(err, "removing drive letter")
1606+
}
1607+
1608+
patterns = []string{strings.TrimPrefix(pattern, "/")}
1609+
}
1610+
1611+
src, err = system.NormalizePath("/", src, d.platform.OS, false)
15911612
if err != nil {
15921613
return errors.Wrap(err, "removing drive letter")
15931614
}
15941615

1595-
patterns = []string{strings.TrimPrefix(pattern, "/")}
1596-
}
1597-
1598-
src, err = system.NormalizePath("/", src, d.platform.OS, false)
1599-
if err != nil {
1600-
return errors.Wrap(err, "removing drive letter")
1601-
}
1602-
1603-
unpack := cfg.isAddCommand
1604-
if cfg.unpack != nil {
1605-
unpack = *cfg.unpack
1606-
}
1607-
1608-
opts := append([]llb.CopyOption{&llb.CopyInfo{
1609-
Mode: chopt,
1610-
FollowSymlinks: true,
1611-
CopyDirContentsOnly: true,
1612-
IncludePatterns: patterns,
1613-
AttemptUnpack: unpack,
1614-
CreateDestPath: true,
1615-
AllowWildcard: true,
1616-
AllowEmptyWildcard: true,
1617-
}}, copyOpt...)
1616+
unpack := cfg.isAddCommand
1617+
if cfg.unpack != nil {
1618+
unpack = *cfg.unpack
1619+
}
16181620

1619-
if a == nil {
1620-
a = llb.Copy(cfg.source, src, dest, opts...)
1621-
} else {
1622-
a = a.Copy(cfg.source, src, dest, opts...)
1621+
opts := append([]llb.CopyOption{&llb.CopyInfo{
1622+
Mode: chopt,
1623+
FollowSymlinks: true,
1624+
CopyDirContentsOnly: true,
1625+
IncludePatterns: patterns,
1626+
AttemptUnpack: unpack,
1627+
CreateDestPath: true,
1628+
AllowWildcard: true,
1629+
AllowEmptyWildcard: true,
1630+
}}, copyOpt...)
1631+
1632+
if a == nil {
1633+
a = llb.Copy(cfg.source, src, dest, opts...)
1634+
} else {
1635+
a = a.Copy(cfg.source, src, dest, opts...)
1636+
}
16231637
}
16241638
}
16251639
}
@@ -2255,19 +2269,26 @@ func commonImageNames() []string {
22552269
return out
22562270
}
22572271

2258-
func isHTTPSource(src string) bool {
2272+
func isHTTPSource(src string) (bool, error) {
22592273
if !strings.HasPrefix(src, "http://") && !strings.HasPrefix(src, "https://") {
2260-
return false
2274+
return false, nil
22612275
}
2262-
return !isGitSource(src)
2276+
b, err := isGitSource(src)
2277+
return !b, err
22632278
}
22642279

2265-
func isGitSource(src string) bool {
2280+
func isGitSource(src string) (bool, error) {
22662281
// https://github.com/ORG/REPO.git is a git source, not an http source
2267-
if gitRef, gitErr := gitutil.ParseGitRef(src); gitRef != nil && gitErr == nil {
2268-
return true
2282+
gitRef, gitErr := gitutil.ParseGitRef(src)
2283+
var guoe *gitutil.GitURLOptsError
2284+
if errors.As(gitErr, &guoe) {
2285+
// this is a git source, and it has invalid URL opts
2286+
return true, gitErr
2287+
}
2288+
if gitRef != nil && gitErr == nil {
2289+
return true, nil
22692290
}
2270-
return false
2291+
return false, nil
22712292
}
22722293

22732294
func isEnabledForStage(stage string, value string) bool {

‎util/gitutil/git_url.go‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,11 @@ type GitURLOpts struct {
6666
Subdir string
6767
}
6868

69+
// GitURLOptsError is returned for invalid GitURLOpts.
70+
type GitURLOptsError struct {
71+
error
72+
}
73+
6974
// parseOpts splits a git URL fragment into its respective git
7075
// reference and subdirectory components.
7176
func parseOpts(fragment string, query url.Values) (*GitURLOpts, error) {
@@ -169,7 +174,9 @@ func fromURL(url *url.URL) (*GitURL, error) {
169174
withoutOpts.RawQuery = ""
170175
opts, err := parseOpts(url.Fragment, url.Query())
171176
if err != nil {
172-
return nil, err
177+
return nil, &GitURLOptsError{
178+
error: errors.Wrapf(err, "failed to parse git URL opts %q", url.Redacted()),
179+
}
173180
}
174181
return &GitURL{
175182
Scheme: url.Scheme,
@@ -187,7 +194,10 @@ func fromSCPStyleURL(url *sshutil.SCPStyleURL) (*GitURL, error) {
187194
withoutOpts.Query = nil
188195
opts, err := parseOpts(url.Fragment, url.Query)
189196
if err != nil {
190-
return nil, err
197+
return nil, &GitURLOptsError{
198+
// *sshutil.SCPStyleURL.String() does not contain password
199+
error: errors.Wrapf(err, "failed to parse git URL opts %q", url.String()),
200+
}
191201
}
192202
return &GitURL{
193203
Scheme: SSHProtocol,

0 commit comments

Comments
 (0)