Repository: regclient/regclient
Pull request #414 (2023-04-18 00:03:18)

Fix handling of multiple separators in a reference
<!--

Commits must be signed indicating your agreement to the [DCO](https://developercertificate.org/).
See [DCO missing](https://github.com/src-d/guide/blob/master/developer-community/fix-DCO.md) for steps to fix a missing signoff.

-->

### Fixes issue

A reference with multiple consecutive separators in a path component is invalid.
<!-- If this is a bug fix, include "fixes #xxxx", or "closes #xxxx" -->

### Describe the change

The regex was adjusted to remove the `*` so that the separator cannot appear consecutively.

Unallowed:

- `example.com/path--part:tag`
- `example.com/path._part:tag`

Allowed:

- `example.com/path.and_part:tag`
<!-- Include the type of change: bug fix, new feature, breaking change, documentation update -->
<!-- Describe what was changed, why the change was made, and how it was implemented -->

### How to verify it

Invalid references return an invalid reference error without sending the request to the registry (where a 404 would typically be received).
<!-- Include steps that can be taken to verify the change -->

### Changelog text

Fix: invalid references are detected before querying the registry
<!-- If the release changelog should have an entry for this, include it here -->

### Please verify and check that the pull request fulfills the following requirements

<!-- Mark the following with an [X] to verify they are included -->

- [X] Tests have been added or not applicable
- [X] Documentation has been added, updated, or not applicable
- [X] Changes have been rebased to main
- [X] Multiple commits to the same code have been squashed



Patch:
diff --git a/types/error.go b/types/error.go
index b56a7e0..6e0811b 100644
--- a/types/error.go
+++ b/types/error.go
@@ -27,6 +27,8 @@ var (
 	ErrHTTPStatus = errors.New("unexpected http status code")
 	// ErrInvalidChallenge indicates an issue with the received challenge in the WWW-Authenticate header
 	ErrInvalidChallenge = errors.New("invalid challenge header")
+	// ErrInvalidReference indicates the reference to an image is has an invalid synax
+	ErrInvalidReference = errors.New("invalid reference")
 	// ErrManifestNotSet indicates the manifest is not set, it must be pulled with a ManifestGet first
 	ErrManifestNotSet = errors.New("manifest not set")
 	// ErrMissingAnnotation returned when a needed annotation is not found
diff --git a/types/ref/ref.go b/types/ref/ref.go
index 4b464a6..dd2cc2a 100644
--- a/types/ref/ref.go
+++ b/types/ref/ref.go
@@ -8,6 +8,8 @@ import (
 	"path"
 	"regexp"
 	"strings"
+
+	"github.com/regclient/regclient/types"
 )
 
 const (
@@ -28,7 +30,7 @@ var (
 	hostDomainS = `(?:` + hostPartS + `(?:(?:` + regexp.QuoteMeta(`.`) + hostPartS + `)+` + regexp.QuoteMeta(`.`) + `?|` + regexp.QuoteMeta(`.`) + `))`
 	hostUpperS  = `(?:[a-zA-Z0-9]*[A-Z][a-zA-Z0-9-]*[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[A-Z][a-zA-Z0-9]*)`
 	registryS   = `(?:` + hostDomainS + `|` + hostPortS + `|` + hostUpperS + `|localhost(?:` + regexp.QuoteMeta(`:`) + `[0-9]+))`
-	repoPartS   = `[a-z0-9]+(?:(?:[_.]|__|[-]*)[a-z0-9]+)*`
+	repoPartS   = `[a-z0-9]+(?:(?:[_.-]|__)[a-z0-9]+)*`
 	pathS       = `[/a-zA-Z0-9_\-. ]+`
 	tagS        = `[\w][\w.-]{0,127}`
 	digestS     = `[A-Za-z][A-Za-z0-9]*(?:[-_+.][A-Za-z][A-Za-z0-9]*)*[:][[:xdigit:]]{32,}`
@@ -76,9 +78,9 @@ func New(parse string) (Ref, error) {
 		matchRef := refRE.FindStringSubmatch(path)
 		if matchRef == nil || len(matchRef) < 5 {
 			if refRE.FindStringSubmatch(strings.ToLower(path)) != nil {
-				return Ref{}, fmt.Errorf("invalid reference \"%s\", repo must be lowercase", path)
+				return Ref{}, fmt.Errorf("%w \"%s\", repo must be lowercase", types.ErrInvalidReference, path)
 			}
-			return Ref{}, fmt.Errorf("invalid reference \"%s\"", path)
+			return Ref{}, fmt.Errorf("%w \"%s\"", types.ErrInvalidReference, path)
 		}
 		ret.Registry = matchRef[1]
 		ret.Repository = matchRef[2]
@@ -102,13 +104,13 @@ func New(parse string) (Ref, error) {
 			ret.Tag = "latest"
 		}
 		if ret.Repository == "" {
-			return Ref{}, fmt.Errorf("invalid reference \"%s\"", path)
+			return Ref{}, fmt.Errorf("%w \"%s\"", types.ErrInvalidReference, path)
 		}
 
 	case "ocidir", "ocifile":
 		matchPath := pathRE.FindStringSubmatch(path)
 		if matchPath == nil || len(matchPath) < 2 || matchPath[1] == "" {
-			return Ref{}, fmt.Errorf("invalid path for scheme \"%s\": %s", scheme, path)
+			return Ref{}, fmt.Errorf("%w, invalid path for scheme \"%s\": %s", types.ErrInvalidReference, scheme, path)
 		}
 		ret.Path = matchPath[1]
 		if len(matchPath) > 2 && matchPath[2] != "" {
@@ -119,7 +121,7 @@ func New(parse string) (Ref, error) {
 		}
 
 	default:
-		return Ref{}, fmt.Errorf("unhandled reference scheme \"%s\" in \"%s\"", scheme, parse)
+		return Ref{}, fmt.Errorf("%w, unknown scheme \"%s\" in \"%s\"", types.ErrInvalidReference, scheme, parse)
 	}
 	return ret, nil
 }


Test patch:
diff --git a/cmd/regctl/artifact_test.go b/cmd/regctl/artifact_test.go
index 2322b3e..0ce2b5e 100644
--- a/cmd/regctl/artifact_test.go
+++ b/cmd/regctl/artifact_test.go
@@ -38,7 +38,7 @@ func TestArtifactPut(t *testing.T) {
 		{
 			name:      "Invalid ref",
 			args:      []string{"artifact", "put", "invalid*ref"},
-			expectErr: fmt.Errorf("invalid reference \"%s\"", "invalid*ref"),
+			expectErr: types.ErrInvalidReference,
 		},
 		{
 			name: "Put artifact",
@@ -112,7 +112,7 @@ func TestArtifactTree(t *testing.T) {
 		{
 			name:      "Invalid ref",
 			args:      []string{"artifact", "tree", "invalid*ref"},
-			expectErr: fmt.Errorf("invalid reference \"%s\"", "invalid*ref"),
+			expectErr: types.ErrInvalidReference,
 		},
 		{
 			name:      "Missing manifest",
diff --git a/cmd/regctl/manifest_test.go b/cmd/regctl/manifest_test.go
index d40adbe..de3cad2 100644
--- a/cmd/regctl/manifest_test.go
+++ b/cmd/regctl/manifest_test.go
@@ -25,7 +25,7 @@ func TestManifestHead(t *testing.T) {
 		{
 			name:      "Invalid ref",
 			args:      []string{"manifest", "head", "invalid*ref"},
-			expectErr: fmt.Errorf("invalid reference \"%s\"", "invalid*ref"),
+			expectErr: types.ErrInvalidReference,
 		},
 		{
 			name:      "Missing manifest",
diff --git a/cmd/regsync/regsync_test.go b/cmd/regsync/regsync_test.go
index dcb738a..5257c03 100644
--- a/cmd/regsync/regsync_test.go
+++ b/cmd/regsync/regsync_test.go
@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"context"
 	"errors"
-	"fmt"
 	"io/fs"
 	"testing"
 
@@ -296,7 +295,7 @@ func TestRegsyncOnce(t *testing.T) {
 				Type:   "image",
 			},
 			desired: []string{},
-			expErr:  fmt.Errorf(`invalid reference "InvalidTestmissing:v1:garbage"`),
+			expErr:  types.ErrInvalidReference,
 		},
 		{
 			name: "InvalidTargetImage",
@@ -306,7 +305,7 @@ func TestRegsyncOnce(t *testing.T) {
 				Type:   "image",
 			},
 			desired: []string{},
-			expErr:  fmt.Errorf(`invalid reference "InvalidTestmissing:v1:garbage"`),
+			expErr:  types.ErrInvalidReference,
 		},
 		{
 			name: "InvalidSourceRepository",
@@ -316,7 +315,7 @@ func TestRegsyncOnce(t *testing.T) {
 				Type:   "repository",
 			},
 			desired: []string{},
-			expErr:  fmt.Errorf(`invalid reference "InvalidTestmissing:v1:garbage"`),
+			expErr:  types.ErrInvalidReference,
 		},
 		{
 			name: "InvalidTargetRepository",
@@ -326,7 +325,7 @@ func TestRegsyncOnce(t *testing.T) {
 				Type:   "repository",
 			},
 			desired: []string{},
-			expErr:  fmt.Errorf(`invalid reference "InvalidTestmissing:v1:garbage"`),
+			expErr:  types.ErrInvalidReference,
 		},
 		{
 			name: "InvalidType",
diff --git a/types/ref/ref_test.go b/types/ref/ref_test.go
index 8859944..47c800b 100644
--- a/types/ref/ref_test.go
+++ b/types/ref/ref_test.go
@@ -1,8 +1,10 @@
 package ref
 
 import (
-	"fmt"
+	"errors"
 	"testing"
+
+	"github.com/regclient/regclient/types"
 )
 
 func TestRef(t *testing.T) {
@@ -160,6 +162,17 @@ func TestRef(t *testing.T) {
 			path:       "",
 			wantE:      nil,
 		},
+		{
+			name:       "separators in repo",
+			ref:        "e.xample.co/g-r.o__u_p/image:a",
+			scheme:     "reg",
+			registry:   "e.xample.co",
+			repository: "g-r.o__u_p/image",
+			tag:        "a",
+			digest:     "",
+			path:       "",
+			wantE:      nil,
+		},
 		{
 			name:       "Localhost registry",
 			ref:        "localhost/group/image:v42",
@@ -240,7 +253,7 @@ func TestRef(t *testing.T) {
 		{
 			name:  "OCI file with invalid digest",
 			ref:   "ocifile://path/to/file.tgz@sha256:ZZ15f840677a5e245d9ea199eb9b026b1539208a5183621dced7b469f6aa678115ZZ",
-			wantE: fmt.Errorf("invalid path for scheme \"ocifile\": path/to/file.tgz@sha256:ZZ15f840677a5e245d9ea199eb9b026b1539208a5183621dced7b469f6aa678115ZZ"),
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:       "OCI dir",
@@ -278,85 +291,92 @@ func TestRef(t *testing.T) {
 		{
 			name:  "invalid scheme",
 			ref:   "unknown://repo:tag",
-			wantE: fmt.Errorf(`unhandled reference scheme "unknown" in "unknown://repo:tag"`),
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:  "invalid host leading dash",
 			ref:   "-docker.io/project/image:tag",
-			wantE: fmt.Errorf(`invalid reference "-docker.io/project/image:tag"`),
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:  "invalid host trailing dash",
 			ref:   "docker-.io/project/image:tag",
-			wantE: fmt.Errorf(`invalid reference "docker-.io/project/image:tag"`),
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:  "invalid repo case",
 			ref:   "docker.io/Upper/Case/Repo:tag",
-			wantE: fmt.Errorf(`invalid reference "docker.io/Upper/Case/Repo:tag", repo must be lowercase`),
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:  "invalid repo dash leading",
 			ref:   "project/-image:tag",
-			wantE: fmt.Errorf(`invalid reference "project/-image:tag"`),
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:  "invalid repo dash trailing",
 			ref:   "project/image-:tag",
-			wantE: fmt.Errorf(`invalid reference "project/image-:tag"`),
+			wantE: types.ErrInvalidReference,
+		},
+		{
+			name:  "invalid repo multiple dash",
+			ref:   "project/image--x:tag",
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:  "invalid repo chars",
 			ref:   "project/star*:tag",
-			wantE: fmt.Errorf(`invalid reference "project/star*:tag"`),
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:  "invalid tag chars",
 			ref:   "project/image:tag^1",
-			wantE: fmt.Errorf(`invalid reference "project/image:tag^1"`),
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:  "invalid short digest",
 			ref:   "project/image@sha256:12345",
-			wantE: fmt.Errorf(`invalid reference "project/image@sha256:12345"`),
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:  "invalid digest characters",
 			ref:   "project/image@sha256:gggg40677a5e245d9ea199eb9b026b1539208a5183621dced7b469f6aa678115",
-			wantE: fmt.Errorf(`invalid reference "project/image@sha256:gggg40677a5e245d9ea199eb9b026b1539208a5183621dced7b469f6aa678115"`),
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:  "invalid ocidir path",
 			ref:   "ocidir://invalid*filename:tag",
-			wantE: fmt.Errorf(`invalid path for scheme "ocidir": invalid*filename:tag`),
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:  "invalid ocidir tag",
 			ref:   "ocidir://filename:tag=fail",
-			wantE: fmt.Errorf(`invalid path for scheme "ocidir": filename:tag=fail`),
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:  "invalid ocidir digest",
 			ref:   "ocidir://filename@sha256:abcd",
-			wantE: fmt.Errorf(`invalid path for scheme "ocidir": filename@sha256:abcd`),
+			wantE: types.ErrInvalidReference,
 		},
 		{
 			name:  "localhost missing repo",
 			ref:   "localhost:5000",
-			wantE: fmt.Errorf(`invalid reference "localhost:5000"`),
+			wantE: types.ErrInvalidReference,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			ref, err := New(tt.ref)
-			if tt.wantE == nil && err != nil {
-				t.Errorf("failed creating reference, err: %v", err)
+			if tt.wantE != nil {
+				if err == nil {
+					t.Errorf("error not received, expected %v", tt.wantE)
+				} else if !errors.Is(err, tt.wantE) && err.Error() != tt.wantE.Error() {
+					t.Errorf("expected error not received, expected %v, received %v", tt.wantE, err)
+				}
 				return
-			} else if tt.wantE != nil && (err == nil || (tt.wantE != err && tt.wantE.Error() != err.Error())) {
-				t.Errorf("expected error not received, expected %v, received %v", tt.wantE, err)
-				return
-			} else if tt.wantE != nil {
+			} else if err != nil {
+				t.Errorf("failed creating reference, err: %v", err)
 				return
 			}
 			if tt.ref != ref.Reference {
@@ -380,7 +400,6 @@ func TestRef(t *testing.T) {
 			if tt.path != ref.Path {
 				t.Errorf("path mismatch for %s, expected %s, received %s", tt.ref, tt.path, ref.Path)
 			}
-
 		})
 	}
 }