client: WithAPIVersion, WithAPIVersionFromEnv: validate well-formedness

Make these options more strict to not allow arbitrary values. Historically,
the `DOCKER_API_VERSION` env-var did not perform any validation as it was
intended for testing-purposes, but given the wider use of this env-var,
we should perform some amount of validation.

Both `WithAPIVersion` and `WithAPIVersionFromEnv` still allow specifying
API versions that are not supported by the client for testing purposes
(e.g. to use API versions beyond `MinAPIVersion` and `MaxAPIVersion`),
but must be well-formed.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

client/client_options.go

@@ -244,15 +244,21 @@ func WithTLSClientConfigFromEnv() Opt {
 // WithAPIVersion overrides the client's API version with the specified one,
 // and disables API version negotiation. If an empty version is provided,
 // this option is ignored to allow version negotiation. The given version
-// should be formatted "<major>.<minor>" (for example, "1.52").
+// should be formatted "<major>.<minor>" (for example, "1.52"). It returns
+// an error if the given value not in the correct format.
 //
 // WithAPIVersion does not validate if the client supports the given version,
-// and callers should verify if the version is in the correct format and
-// lower than the maximum supported version as defined by [MaxAPIVersion].
+// and callers should verify if the version lower than the maximum supported
+// version as defined by [MaxAPIVersion].
 func WithAPIVersion(version string) Opt {
 	return func(c *clientConfig) error {
-		if v := strings.TrimPrefix(version, "v"); v != "" {
-			c.version = v
+		version = strings.TrimSpace(version)
+		if val := strings.TrimPrefix(version, "v"); val != "" {
+			ver, err := parseAPIVersion(val)
+			if err != nil {
+				return fmt.Errorf("invalid API version (%s): %w", version, err)
+			}
+			c.version = ver
 			c.manualOverride = true
 		}
 		return nil
@@ -271,12 +277,21 @@ func WithVersion(version string) Opt {
 // If DOCKER_API_VERSION is not set, or set to an empty value, the version
 // is not modified.
 //
-// WithAPIVersionFromEnv does not validate if the client supports the given version,
-// and callers should verify if the version is in the correct format and
-// lower than the maximum supported version as defined by [MaxAPIVersion].
+// WithAPIVersion does not validate if the client supports the given version,
+// and callers should verify if the version lower than the maximum supported
+// version as defined by [MaxAPIVersion].
 func WithAPIVersionFromEnv() Opt {
 	return func(c *clientConfig) error {
-		return WithAPIVersion(os.Getenv(EnvOverrideAPIVersion))(c)
+		version := strings.TrimSpace(os.Getenv(EnvOverrideAPIVersion))
+		if val := strings.TrimPrefix(version, "v"); val != "" {
+			ver, err := parseAPIVersion(val)
+			if err != nil {
+				return fmt.Errorf("invalid API version (%s): %w", version, err)
+			}
+			c.version = ver
+			c.manualOverride = true
+		}
+		return nil
 	}
 }
 
@@ -285,9 +300,7 @@ func WithAPIVersionFromEnv() Opt {
 //
 // Deprecated: use [WithAPIVersionFromEnv] instead.
 func WithVersionFromEnv() Opt {
-	return func(c *clientConfig) error {
-		return WithAPIVersion(os.Getenv(EnvOverrideAPIVersion))(c)
-	}
+	return WithAPIVersionFromEnv()
 }
 
 // WithAPIVersionNegotiation enables automatic API version negotiation for the client.

client/client_options_test.go

@@ -3,6 +3,7 @@ package client
 import (
 	"net/http"
 	"runtime"
+	"strings"
 	"testing"
 	"time"
 
@@ -44,20 +45,209 @@ func TestOptionWithTimeout(t *testing.T) {
 	assert.Check(t, is.Equal(c.client.Timeout, timeout))
 }
 
-func TestOptionAPIWithVersionFromEnv(t *testing.T) {
-	c, err := New(WithAPIVersionFromEnv())
-	assert.NilError(t, err)
-	assert.Check(t, c.client != nil)
-	assert.Check(t, is.Equal(c.version, MaxAPIVersion))
-	assert.Check(t, is.Equal(c.manualOverride, false))
+func TestOptionWithAPIVersion(t *testing.T) {
+	tests := []struct {
+		doc      string
+		version  string
+		expected string
+		expError string
+	}{
+		{
+			doc:      "empty version",
+			version:  "",
+			expected: MaxAPIVersion,
+		},
+		{
+			doc:      "custom lower version with whitespace, no v-prefix",
+			version:  "   1.50   ",
+			expected: "1.50",
+		},
+		{
+			// We currently allow downgrading the client to an unsupported lower version for testing.
+			doc:      "downgrade unsupported version, no v-prefix",
+			version:  "1.0",
+			expected: "1.0",
+		},
+		{
+			doc:      "custom lower version, no v-prefix",
+			version:  "1.50",
+			expected: "1.50",
+		},
+		{
+			// We currently allow upgrading the client to an unsupported higher version for testing.
+			doc:      "upgrade version, no v-prefix",
+			version:  "9.99",
+			expected: "9.99",
+		},
+		{
+			doc:      "empty version, with v-prefix",
+			version:  "v",
+			expected: MaxAPIVersion,
+		},
+		{
+			doc:      "whitespace, with v-prefix",
+			version:  "   v1.0   ",
+			expected: "1.0",
+		},
+		{
+			doc:      "downgrade unsupported version, with v-prefix",
+			version:  "v1.0",
+			expected: "1.0",
+		},
+		{
+			doc:      "custom lower version with whitespace and v-prefix",
+			version:  "   v1.50   ",
+			expected: "1.50",
+		},
+		{
+			doc:      "custom lower version, with v-prefix",
+			version:  "v1.50",
+			expected: "1.50",
+		},
+		{
+			doc:      "upgrade version, with v-prefix",
+			version:  "v9.99",
+			expected: "9.99",
+		},
+		{
+			doc:      "malformed version",
+			version:  "something-weird",
+			expError: "invalid API version (something-weird): must be formatted <major>.<minor>",
+		},
+		{
+			doc:      "no minor",
+			version:  "1",
+			expError: "invalid API version (1): must be formatted <major>.<minor>",
+		},
+		{
+			doc:      "too many digits",
+			version:  "1.2.3",
+			expError: "invalid API version (1.2.3): invalid minor version: must be formatted <major>.<minor>",
+		},
+		{
+			doc:      "embedded whitespace",
+			version:  "v 1.0",
+			expError: "invalid API version (v 1.0): invalid major version: must be formatted <major>.<minor>",
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.doc, func(t *testing.T) {
+			client, err := New(WithAPIVersion(tc.version))
+			if tc.expError != "" {
+				assert.Check(t, is.ErrorContains(err, tc.expError))
+				assert.Check(t, client == nil)
+			} else {
+				assert.NilError(t, err)
+				assert.Check(t, client != nil)
+				assert.Check(t, is.Equal(client.ClientVersion(), tc.expected))
+				isNoOp := strings.TrimPrefix(strings.TrimSpace(tc.version), "v") == ""
+				assert.Check(t, is.Equal(client.manualOverride, !isNoOp))
+			}
+		})
+	}
+}
 
-	t.Setenv("DOCKER_API_VERSION", "2.9999")
-
-	c, err = New(WithAPIVersionFromEnv())
-	assert.NilError(t, err)
-	assert.Check(t, c.client != nil)
-	assert.Check(t, is.Equal(c.version, "2.9999"))
-	assert.Check(t, is.Equal(c.manualOverride, true))
+func TestOptionWithAPIVersionFromEnv(t *testing.T) {
+	tests := []struct {
+		doc      string
+		version  string
+		expected string
+		expError string
+	}{
+		{
+			doc:      "empty version",
+			version:  "",
+			expected: MaxAPIVersion,
+		},
+		{
+			doc:      "custom lower version with whitespace, no v-prefix",
+			version:  "   1.50   ",
+			expected: "1.50",
+		},
+		{
+			// We currently allow downgrading the client to an unsupported lower version for testing.
+			doc:      "downgrade unsupported version, no v-prefix",
+			version:  "1.0",
+			expected: "1.0",
+		},
+		{
+			doc:      "custom lower version, no v-prefix",
+			version:  "1.50",
+			expected: "1.50",
+		},
+		{
+			// We currently allow upgrading the client to an unsupported higher version for testing.
+			doc:      "upgrade version, no v-prefix",
+			version:  "9.99",
+			expected: "9.99",
+		},
+		{
+			doc:      "empty version, with v-prefix",
+			version:  "v",
+			expected: MaxAPIVersion,
+		},
+		{
+			doc:      "whitespace, with v-prefix",
+			version:  "   v1.0   ",
+			expected: "1.0",
+		},
+		{
+			doc:      "downgrade unsupported version, with v-prefix",
+			version:  "v1.0",
+			expected: "1.0",
+		},
+		{
+			doc:      "custom lower version with whitespace and v-prefix",
+			version:  "   v1.50   ",
+			expected: "1.50",
+		},
+		{
+			doc:      "custom lower version, with v-prefix",
+			version:  "v1.50",
+			expected: "1.50",
+		},
+		{
+			doc:      "upgrade version, with v-prefix",
+			version:  "v9.99",
+			expected: "9.99",
+		},
+		{
+			doc:      "malformed version",
+			version:  "something-weird",
+			expError: "invalid API version (something-weird): must be formatted <major>.<minor>",
+		},
+		{
+			doc:      "no minor",
+			version:  "1",
+			expError: "invalid API version (1): must be formatted <major>.<minor>",
+		},
+		{
+			doc:      "too many digits",
+			version:  "1.2.3",
+			expError: "invalid API version (1.2.3): invalid minor version: must be formatted <major>.<minor>",
+		},
+		{
+			doc:      "embedded whitespace",
+			version:  "v 1.0",
+			expError: "invalid API version (v 1.0): invalid major version: must be formatted <major>.<minor>",
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.doc, func(t *testing.T) {
+			t.Setenv(EnvOverrideAPIVersion, tc.version)
+			client, err := New(WithAPIVersionFromEnv())
+			if tc.expError != "" {
+				assert.Check(t, is.ErrorContains(err, tc.expError))
+				assert.Check(t, client == nil)
+			} else {
+				assert.NilError(t, err)
+				assert.Check(t, client != nil)
+				assert.Check(t, is.Equal(client.ClientVersion(), tc.expected))
+				isNoOp := strings.TrimPrefix(strings.TrimSpace(tc.version), "v") == ""
+				assert.Check(t, is.Equal(client.manualOverride, !isNoOp))
+			}
+		})
+	}
 }
 
 func TestWithUserAgent(t *testing.T) {

client/client_test.go

@@ -452,81 +452,6 @@ func TestNegotiateAPIVersionWithFixedVersion(t *testing.T) {
 	assert.Check(t, is.Equal(client.ClientVersion(), customVersion))
 }
 
-// TestCustomAPIVersion tests initializing the client with a custom
-// version.
-func TestCustomAPIVersion(t *testing.T) {
-	tests := []struct {
-		doc      string
-		version  string
-		expected string
-	}{
-		{
-			doc:      "empty version",
-			version:  "",
-			expected: MaxAPIVersion,
-		},
-		{
-			doc:      "custom lower version, no v-prefix",
-			version:  "1.50",
-			expected: "1.50",
-		},
-		{
-			// We allow upgrading the client to an unsupported higher version for testing.
-			doc:      "upgrade version, no v-prefix",
-			version:  "9.99",
-			expected: "9.99",
-		},
-		{
-			// We currently ignore malformed versions.
-			doc:      "empty version, with v-prefix",
-			version:  "v",
-			expected: MaxAPIVersion,
-		},
-		{
-			doc:      "custom lower version, with v-prefix",
-			version:  "v1.50",
-			expected: "1.50",
-		},
-		{
-			// We allow upgrading the client to an unsupported higher version for testing.
-			doc:      "upgrade version, with v-prefix",
-			version:  "v9.99",
-			expected: "9.99",
-		},
-		{
-			// We currently allow downgrading the client to an unsupported lower version for testing.
-			doc:      "downgrade unsupported version, no v-prefix",
-			version:  "1.0",
-			expected: "1.0",
-		},
-		{
-			// We currently allow downgrading the client to an unsupported lower version for testing.
-			doc:      "downgrade unsupported version, no v-prefix",
-			version:  "v1.0",
-			expected: "1.0",
-		},
-		{
-			// When manually setting a version, no validation happens.
-			// so anything is accepted.
-			doc:      "malformed version",
-			version:  "something-weird",
-			expected: "something-weird",
-		},
-	}
-	for _, tc := range tests {
-		t.Run(tc.doc, func(t *testing.T) {
-			client, err := New(WithAPIVersion(tc.version))
-			assert.NilError(t, err)
-			assert.Check(t, is.Equal(client.ClientVersion(), tc.expected))
-
-			t.Setenv(EnvOverrideAPIVersion, tc.expected)
-			client, err = New(WithAPIVersionFromEnv())
-			assert.NilError(t, err)
-			assert.Check(t, is.Equal(client.ClientVersion(), tc.expected))
-		})
-	}
-}
-
 func TestClientRedirect(t *testing.T) {
 	client := &http.Client{
 		CheckRedirect: CheckRedirect,

client/utils.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"strconv"
 	"strings"
 	"sync"
 
@@ -32,6 +33,47 @@ func trimID(objType, id string) (string, error) {
 	return id, nil
 }
 
+// parseAPIVersion checks v to be a well-formed ("<major>.<minor>")
+// API version. It returns an error if the value is empty or does not
+// have the correct format, but does not validate if the API version is
+// within the supported range ([MinAPIVersion] <= v <= [MaxAPIVersion]).
+//
+// It returns version after normalizing, or an error if validation failed.
+func parseAPIVersion(version string) (string, error) {
+	if strings.TrimPrefix(strings.TrimSpace(version), "v") == "" {
+		return "", cerrdefs.ErrInvalidArgument.WithMessage("value is empty")
+	}
+	major, minor, err := parseMajorMinor(version)
+	if err != nil {
+		return "", err
+	}
+	return fmt.Sprintf("%d.%d", major, minor), nil
+}
+
+// parseMajorMinor is a helper for parseAPIVersion.
+func parseMajorMinor(v string) (major, minor int, _ error) {
+	if strings.HasPrefix(v, "v") {
+		return 0, 0, cerrdefs.ErrInvalidArgument.WithMessage("must be formatted <major>.<minor>")
+	}
+	if strings.TrimSpace(v) == "" {
+		return 0, 0, cerrdefs.ErrInvalidArgument.WithMessage("value is empty")
+	}
+
+	majVer, minVer, ok := strings.Cut(v, ".")
+	if !ok {
+		return 0, 0, cerrdefs.ErrInvalidArgument.WithMessage("must be formatted <major>.<minor>")
+	}
+	major, err := strconv.Atoi(majVer)
+	if err != nil {
+		return 0, 0, cerrdefs.ErrInvalidArgument.WithMessage("invalid major version: must be formatted <major>.<minor>")
+	}
+	minor, err = strconv.Atoi(minVer)
+	if err != nil {
+		return 0, 0, cerrdefs.ErrInvalidArgument.WithMessage("invalid minor version: must be formatted <major>.<minor>")
+	}
+	return major, minor, nil
+}
+
 // encodePlatforms marshals the given platform(s) to JSON format, to
 // be used for query-parameters for filtering / selecting platforms.
 func encodePlatforms(platform ...ocispec.Platform) ([]string, error) {

vendor/github.com/moby/moby/client/client_options.go

@@ -244,15 +244,21 @@ func WithTLSClientConfigFromEnv() Opt {
 // WithAPIVersion overrides the client's API version with the specified one,
 // and disables API version negotiation. If an empty version is provided,
 // this option is ignored to allow version negotiation. The given version
-// should be formatted "<major>.<minor>" (for example, "1.52").
+// should be formatted "<major>.<minor>" (for example, "1.52"). It returns
+// an error if the given value not in the correct format.
 //
 // WithAPIVersion does not validate if the client supports the given version,
-// and callers should verify if the version is in the correct format and
-// lower than the maximum supported version as defined by [MaxAPIVersion].
+// and callers should verify if the version lower than the maximum supported
+// version as defined by [MaxAPIVersion].
 func WithAPIVersion(version string) Opt {
 	return func(c *clientConfig) error {
-		if v := strings.TrimPrefix(version, "v"); v != "" {
-			c.version = v
+		version = strings.TrimSpace(version)
+		if val := strings.TrimPrefix(version, "v"); val != "" {
+			ver, err := parseAPIVersion(val)
+			if err != nil {
+				return fmt.Errorf("invalid API version (%s): %w", version, err)
+			}
+			c.version = ver
 			c.manualOverride = true
 		}
 		return nil
@@ -271,12 +277,21 @@ func WithVersion(version string) Opt {
 // If DOCKER_API_VERSION is not set, or set to an empty value, the version
 // is not modified.
 //
-// WithAPIVersionFromEnv does not validate if the client supports the given version,
-// and callers should verify if the version is in the correct format and
-// lower than the maximum supported version as defined by [MaxAPIVersion].
+// WithAPIVersion does not validate if the client supports the given version,
+// and callers should verify if the version lower than the maximum supported
+// version as defined by [MaxAPIVersion].
 func WithAPIVersionFromEnv() Opt {
 	return func(c *clientConfig) error {
-		return WithAPIVersion(os.Getenv(EnvOverrideAPIVersion))(c)
+		version := strings.TrimSpace(os.Getenv(EnvOverrideAPIVersion))
+		if val := strings.TrimPrefix(version, "v"); val != "" {
+			ver, err := parseAPIVersion(val)
+			if err != nil {
+				return fmt.Errorf("invalid API version (%s): %w", version, err)
+			}
+			c.version = ver
+			c.manualOverride = true
+		}
+		return nil
 	}
 }
 
@@ -285,9 +300,7 @@ func WithAPIVersionFromEnv() Opt {
 //
 // Deprecated: use [WithAPIVersionFromEnv] instead.
 func WithVersionFromEnv() Opt {
-	return func(c *clientConfig) error {
-		return WithAPIVersion(os.Getenv(EnvOverrideAPIVersion))(c)
-	}
+	return WithAPIVersionFromEnv()
 }
 
 // WithAPIVersionNegotiation enables automatic API version negotiation for the client.

vendor/github.com/moby/moby/client/utils.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"strconv"
 	"strings"
 	"sync"
 
@@ -32,6 +33,47 @@ func trimID(objType, id string) (string, error) {
 	return id, nil
 }
 
+// parseAPIVersion checks v to be a well-formed ("<major>.<minor>")
+// API version. It returns an error if the value is empty or does not
+// have the correct format, but does not validate if the API version is
+// within the supported range ([MinAPIVersion] <= v <= [MaxAPIVersion]).
+//
+// It returns version after normalizing, or an error if validation failed.
+func parseAPIVersion(version string) (string, error) {
+	if strings.TrimPrefix(strings.TrimSpace(version), "v") == "" {
+		return "", cerrdefs.ErrInvalidArgument.WithMessage("value is empty")
+	}
+	major, minor, err := parseMajorMinor(version)
+	if err != nil {
+		return "", err
+	}
+	return fmt.Sprintf("%d.%d", major, minor), nil
+}
+
+// parseMajorMinor is a helper for parseAPIVersion.
+func parseMajorMinor(v string) (major, minor int, _ error) {
+	if strings.HasPrefix(v, "v") {
+		return 0, 0, cerrdefs.ErrInvalidArgument.WithMessage("must be formatted <major>.<minor>")
+	}
+	if strings.TrimSpace(v) == "" {
+		return 0, 0, cerrdefs.ErrInvalidArgument.WithMessage("value is empty")
+	}
+
+	majVer, minVer, ok := strings.Cut(v, ".")
+	if !ok {
+		return 0, 0, cerrdefs.ErrInvalidArgument.WithMessage("must be formatted <major>.<minor>")
+	}
+	major, err := strconv.Atoi(majVer)
+	if err != nil {
+		return 0, 0, cerrdefs.ErrInvalidArgument.WithMessage("invalid major version: must be formatted <major>.<minor>")
+	}
+	minor, err = strconv.Atoi(minVer)
+	if err != nil {
+		return 0, 0, cerrdefs.ErrInvalidArgument.WithMessage("invalid minor version: must be formatted <major>.<minor>")
+	}
+	return major, minor, nil
+}
+
 // encodePlatforms marshals the given platform(s) to JSON format, to
 // be used for query-parameters for filtering / selecting platforms.
 func encodePlatforms(platform ...ocispec.Platform) ([]string, error) {