Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: "go get" fails on import path patterns with wildcards ("...") #29241

Closed
FiloSottile opened this issue Dec 13, 2018 · 18 comments

Comments

Projects
None yet
9 participants
@FiloSottile
Copy link
Member

commented Dec 13, 2018

The fix for cmd/go: remote command execution during "go get -u" (#29230) as released in Go 1.10.6 and 1.11.3 introduced a regression causing commands like go get example.com/... to fail with invalid import path: malformed import path "example.com/...": double dot when fetching packages not already present in GOPATH.

See #29236 (comment)

@jadr2ddude

This comment has been minimized.

Copy link

commented Dec 13, 2018

Looking through the code, I saw the following mistakes that could have caused this error:

Error 1 - strings.Contains(path, "..") creates false positives with something/...
8954add#diff-5b11f198c631a3d489e1be171853ab7bR40

Error 2 - the ... path would cause this to error too, since it is all .
8954add#diff-5b11f198c631a3d489e1be171853ab7bR70

Suggested Fix

Remove the check listed in error 1. In addition to causing false positives, it is rendered unnecessary by the line mentioned in error 2. Leave the line in error 2 unchanged, but add a check that skips this validation for the last element if it exactly matches ....

@FiloSottile FiloSottile pinned this issue Dec 13, 2018

@josharian josharian unpinned this issue Dec 13, 2018

@cespare cespare pinned this issue Dec 13, 2018

@cespare cespare unpinned this issue Dec 13, 2018

@josharian josharian pinned this issue Dec 13, 2018

@jadr2ddude

This comment has been minimized.

Copy link

commented Dec 13, 2018

Is someone currently working on this? If not, I can try to create a fix.

@jadr2ddude

This comment has been minimized.

Copy link

commented Dec 14, 2018

In absence of a response, I have started fixing this.

@FiloSottile

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2018

Thank you, but I believe @dmitshur and @bcmills are already moving on a fix.

@jadr2ddude

This comment has been minimized.

Copy link

commented Dec 14, 2018

Ok. Thank you for letting me know.

@willbuckner

This comment has been minimized.

Copy link

commented Dec 14, 2018

diff --git a/src/cmd/go/internal/get/path.go b/src/cmd/go/internal/get/path.go
index d443bd28a9..2a109d6638 100644
--- a/src/cmd/go/internal/get/path.go
+++ b/src/cmd/go/internal/get/path.go
@@ -41,9 +41,6 @@ func checkPath(path string, fileName bool) error {
        if path == "" {
                return fmt.Errorf("empty string")
        }
-       if strings.Contains(path, "..") {
-               return fmt.Errorf("double dot")
-       }
        if strings.Contains(path, "//") {
                return fmt.Errorf("double slash")
        }
@@ -59,8 +56,10 @@ func checkPath(path string, fileName bool) error {
                        elemStart = i + 1
                }
        }
-       if err := checkElem(path[elemStart:], fileName); err != nil {
-               return err
+       if path[elemStart:] != "..." {
+               if err := checkElem(path[elemStart:], fileName); err != nil {
+                       return err
+               }
        }
        return nil
 }

I'd submit this, but I'm sure someone else can do it faster as I haven't yet made a Go contribution :)

@gopherbot

This comment has been minimized.

Copy link

commented Dec 14, 2018

Change https://golang.org/cl/154121 mentions this issue: cmd/go: fix regression allowing '...' wildcard as last element in go get

@FiloSottile

This comment has been minimized.

@bcmills

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

@gopherbot, please backport to 1.11 and 1.10 immediately: this is a major regression.

@gopherbot

This comment has been minimized.

Copy link

commented Dec 14, 2018

Backport issue(s) opened: #29247 (for 1.10), #29248 (for 1.11).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot

This comment has been minimized.

Copy link

commented Dec 14, 2018

Change https://golang.org/cl/154108 mentions this issue: cmd/go/internal/get: move wildcard-trimming to before CheckImportPath

thaJeztah added a commit to thaJeztah/containerd that referenced this issue Dec 14, 2018

Fix CI due to Golang 1.10.6 / 1.11.3 regressions (workaround)
Attempt to fix CI is failing due to a regression in Go 1.10.6 / 1.11.3 (see golang/go#29241)

```
package github.com/containernetworking/plugins/...: github.com/containernetworking/plugins/...: invalid import path: malformed import path "github.com/containernetworking/plugins/...": double dot
```

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 52de371)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

@FiloSottile I noticed that go list ./... still works on Go 1.11.3 (and Go 1.10.6). While it's usable as a workaround for this regression (containerd/containerd#2879), wondering if that's actually something that was missed in the patch release (or won't go list have the same security issue? I didn't dive deep into the fix that went into those patch releases 😅)

@bcmills

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

@thaJeztah, go list does not exhibit the same issue because it does not run VCS commands.

@thaJeztah

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

@bcmills ah, makes sense, thanks for confirming it's nothing to worry about 👍

@myitcv myitcv unpinned this issue Dec 14, 2018

@FiloSottile FiloSottile pinned this issue Dec 14, 2018

shentubot added a commit to google/gvisor that referenced this issue Dec 15, 2018

Use containerd at HEAD until better tagged version is available.
Go 1.11.3 has a bug:
golang/go#29241

This is fixed/workarounded in containerd:
containerd/containerd@52de371

Until that commit has made it into a tagged version, we will use containerd at
head.

PiperOrigin-RevId: 225636987
Change-Id: I7e32beb7751f566f5b41682a29a14442c1aa56c2

@myitcv myitcv unpinned this issue Dec 15, 2018

jgillich referenced this issue in docker-library/official-images Dec 16, 2018

@johan-lejdung

This comment has been minimized.

Copy link

commented Dec 16, 2018

Is there any workaround or does anyone know when the planned fix for this is released?

@ldez

This comment has been minimized.

Copy link

commented Dec 16, 2018

The release is already done since 2 days: go1.11.4 and go1.10.7

Note: the docker images are not up-to-date https://hub.docker.com/_/golang?tab=tags

@johan-lejdung

This comment has been minimized.

Copy link

commented Dec 16, 2018

Ah thanks! I was looking at the docker images. Do those follow a different release schedule?

aswild added a commit to aswild/gitea that referenced this issue Dec 16, 2018

Makefile: fix "go get" for go-bindata in go 1.11.3
Go 1.11.3's CVE fix for 'remote command execution during "go get -u"'
broke getting wildcard paths, which Gitea's makefile uses to fetch
go-bindata and swagger.

The error is:
'package github.com/jteeuwen/go-bindata/...: github.com/jteeuwen/go-bindata/...:
invalid import path: malformed import path
"github.com/jteeuwen/go-bindata/...": double dot'

If the source already exists (get without the ...) then it works.

This should be fixed in go 1.11.4.
See golang/go#29241
@jadr2ddude

This comment has been minimized.

Copy link

commented Dec 16, 2018

@johan-lejdung docker-library/golang#255 - the docker tags are currently in progress

tonistiigi pushed a commit to tonistiigi/gvisor that referenced this issue Jan 30, 2019

Use containerd at HEAD until better tagged version is available.
Go 1.11.3 has a bug:
golang/go#29241

This is fixed/workarounded in containerd:
containerd/containerd@52de371

Until that commit has made it into a tagged version, we will use containerd at
head.

PiperOrigin-RevId: 225636987
Change-Id: I7e32beb7751f566f5b41682a29a14442c1aa56c2
Upstream-commit: 13e4930
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.