From 6ee8705ae63ec9918f2cc19b1d903c5cdb0d5487 Mon Sep 17 00:00:00 2001
From: DCCooper <1866858@gmail.com>
Date: Mon, 30 Nov 2020 19:40:14 +0800
Subject: [PATCH 3/4] cleancode:fix problems found by code review

Signed-off-by: DCCooper <1866858@gmail.com>
---
 Makefile                       |  1 +
 builder/dockerfile/add_copy.go |  5 ++++-
 builder/dockerfile/builder.go  |  7 +++++--
 cmd/cli/build.go               | 18 ++++++++++--------
 cmd/cli/build_test.go          |  6 ++----
 cmd/daemon/main.go             | 34 +++++++++++++++++++++++++++-------
 daemon/daemon.go               | 15 +++++++++++++--
 daemon/import.go               |  8 ++++++--
 daemon/load.go                 |  8 +++++---
 daemon/login.go                | 10 +++++++---
 daemon/logout.go               | 13 ++++++++-----
 exporter/common.go             | 25 +++++++++++++++----------
 store/store.go                 |  8 ++++----
 util/common.go                 | 12 ++++++++++++
 util/common_test.go            | 18 +++++++++++++++---
 15 files changed, 134 insertions(+), 54 deletions(-)

diff --git a/Makefile b/Makefile
index f40941b4..cbace592 100644
--- a/Makefile
+++ b/Makefile
@@ -97,6 +97,7 @@ proto:
 install:
 	install -D -m0551 bin/isula-build $(BINDIR)
 	install -D -m0550 bin/isula-builder $(BINDIR)
+	@( getent group isula > /dev/null ) || ( groupadd --system isula )
 	@[ ! -d ${CONFIG_DIR}/${CONFIG_FILE} ] && install -dm0650 ${CONFIG_DIR}
 	@( [ -f ${CONFIG_DIR}/${CONFIG_FILE} ] && printf "%-20s %s\n" "${CONFIG_FILE}" "already exist in ${CONFIG_DIR}, please replace it manually." ) || install -D -m0600 ${LOCAL_CONF_PREFIX}/${CONFIG_FILE} ${CONFIG_DIR}/${CONFIG_FILE}
 	@( [ -f ${CONFIG_DIR}/${POLICY_FILE} ] && printf "%-20s %s\n" "${POLICY_FILE}" "already exist in ${CONFIG_DIR}, please replace it manually." ) || install -D -m0600 ${LOCAL_CONF_PREFIX}/${POLICY_FILE} ${CONFIG_DIR}/${POLICY_FILE}
diff --git a/builder/dockerfile/add_copy.go b/builder/dockerfile/add_copy.go
index c5d966e2..42cda7f9 100644
--- a/builder/dockerfile/add_copy.go
+++ b/builder/dockerfile/add_copy.go
@@ -143,7 +143,10 @@ func (c *cmdBuilder) getCopyContextDir(from string) (string, func(), error) {
 	if err != nil {
 		return "", nil, err
 	}
-	c.stage.buildOpt.systemContext.DockerCertPath = filepath.Join(constant.DefaultCertRoot, server)
+	c.stage.buildOpt.systemContext.DockerCertPath, err = securejoin.SecureJoin(constant.DefaultCertRoot, server)
+	if err != nil {
+		return "", nil, err
+	}
 
 	// "from" is neither name nor index of stage, consider that "from" is image description
 	imgDesc, err := prepareImage(&image.PrepareImageOptions{
diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go
index 757f4cd5..7e954f0d 100644
--- a/builder/dockerfile/builder.go
+++ b/builder/dockerfile/builder.go
@@ -22,13 +22,13 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"path/filepath"
 	"regexp"
 	"sort"
 	"strings"
 	"time"
 
 	"github.com/containers/image/v5/docker/reference"
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/opencontainers/go-digest"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -257,7 +257,10 @@ func (b *Builder) newStageBuilders() error {
 		if err != nil {
 			return err
 		}
-		sb.buildOpt.systemContext.DockerCertPath = filepath.Join(constant.DefaultCertRoot, server)
+		sb.buildOpt.systemContext.DockerCertPath, err = securejoin.SecureJoin(constant.DefaultCertRoot, server)
+		if err != nil {
+			return err
+		}
 
 		b.stageBuilders = append(b.stageBuilders, sb)
 	}
diff --git a/cmd/cli/build.go b/cmd/cli/build.go
index f2a49e02..3ebbb90f 100644
--- a/cmd/cli/build.go
+++ b/cmd/cli/build.go
@@ -171,6 +171,13 @@ func newBuildOptions(args []string) error {
 		return nil
 	}
 
+	// check cap list
+	for _, c := range buildOpts.capAddList {
+		if !util.CheckCap(c) {
+			return errors.Errorf("cap %v is invalid", c)
+		}
+	}
+
 	// the path may be a symbol link
 	contextDir, err := filepath.Abs(args[0])
 	if err != nil {
@@ -236,8 +243,9 @@ func modifyLocalTransporter(transport string, absPath string, segments []string)
 	const validIsuladFieldsLen = 3
 	switch transport {
 	case "docker-archive":
-		segments[1] = absPath
-		buildOpts.output = strings.Join(segments, ":")
+		newSeg := util.CopyStrings(segments)
+		newSeg[1] = absPath
+		buildOpts.output = strings.Join(newSeg, ":")
 		return nil
 	case "isulad":
 		if len(segments) != validIsuladFieldsLen {
@@ -311,12 +319,6 @@ func runBuild(ctx context.Context, cli Cli) (string, error) {
 		digest          string
 	)
 
-	for _, c := range buildOpts.capAddList {
-		if !util.CheckCap(c) {
-			return "", errors.Errorf("cap %v is invalid", c)
-		}
-	}
-
 	if err = checkAndProcessOutput(); err != nil {
 		return "", err
 	}
diff --git a/cmd/cli/build_test.go b/cmd/cli/build_test.go
index 1fa8ecc4..b4c3a61f 100644
--- a/cmd/cli/build_test.go
+++ b/cmd/cli/build_test.go
@@ -602,17 +602,15 @@ func TestRunBuildWithCap(t *testing.T) {
 	defer tmpDir.Remove()
 	buildOpts.file = tmpDir.Join("Dockerfile")
 	buildOpts.output = "docker-daemon:cap:latest"
-	mockBuild := newMockDaemon()
-	ctx := context.Background()
-	cli := newMockClient(&mockGrpcClient{imageBuildFunc: mockBuild.build})
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			buildOpts.capAddList = tt.caps
-			_, err := runBuild(ctx, &cli)
+			err := newBuildOptions([]string{tmpDir.Path()})
 			if tt.isErr {
 				assert.ErrorContains(t, err, "is invalid")
 			}
+			buildOpts.capAddList = nil
 		})
 	}
 }
diff --git a/cmd/daemon/main.go b/cmd/daemon/main.go
index 9fcde5cd..d6f1d080 100644
--- a/cmd/daemon/main.go
+++ b/cmd/daemon/main.go
@@ -22,6 +22,7 @@ import (
 
 	"github.com/BurntSushi/toml"
 	"github.com/containers/storage/pkg/reexec"
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
@@ -157,9 +158,17 @@ func before(cmd *cobra.Command) error {
 	logrus.SetOutput(os.Stdout)
 	logrus.SetFormatter(&logrus.TextFormatter{FullTimestamp: true})
 
+	runRoot, err := securejoin.SecureJoin(daemonOpts.RunRoot, "storage")
+	if err != nil {
+		return err
+	}
+	dataRoot, err := securejoin.SecureJoin(daemonOpts.DataRoot, "storage")
+	if err != nil {
+		return err
+	}
 	store.SetDefaultStoreOptions(store.DaemonStoreOptions{
-		RunRoot:      filepath.Join(daemonOpts.RunRoot, "storage"),
-		DataRoot:     filepath.Join(daemonOpts.DataRoot, "storage"),
+		RunRoot:      runRoot,
+		DataRoot:     dataRoot,
 		Driver:       daemonOpts.StorageDriver,
 		DriverOption: util.CopyStrings(daemonOpts.StorageOpts),
 	})
@@ -204,7 +213,7 @@ func loadConfig(path string) (config.TomlConfig, error) {
 	return conf, err
 }
 
-func mergeStorageConfig(cmd *cobra.Command) {
+func mergeStorageConfig(cmd *cobra.Command) error {
 	store.SetDefaultConfigFilePath(constant.StorageConfigPath)
 	option, err := store.GetDefaultStoreOptions(true)
 	if err == nil {
@@ -218,10 +227,16 @@ func mergeStorageConfig(cmd *cobra.Command) {
 
 	var storeOpt store.DaemonStoreOptions
 	if option.RunRoot == "" {
-		storeOpt.RunRoot = filepath.Join(daemonOpts.RunRoot, "storage")
+		storeOpt.RunRoot, err = securejoin.SecureJoin(daemonOpts.RunRoot, "storage")
+		if err != nil {
+			return err
+		}
 	}
 	if option.GraphRoot == "" {
-		storeOpt.DataRoot = filepath.Join(daemonOpts.DataRoot, "storage")
+		storeOpt.DataRoot, err = securejoin.SecureJoin(daemonOpts.DataRoot, "storage")
+		if err != nil {
+			return err
+		}
 	}
 	if daemonOpts.StorageDriver != "" {
 		storeOpt.Driver = daemonOpts.StorageDriver
@@ -230,6 +245,8 @@ func mergeStorageConfig(cmd *cobra.Command) {
 		storeOpt.DriverOption = util.CopyStrings(daemonOpts.StorageOpts)
 	}
 	store.SetDefaultStoreOptions(storeOpt)
+
+	return nil
 }
 
 func mergeConfig(conf config.TomlConfig, cmd *cobra.Command) {
@@ -258,7 +275,10 @@ func setupWorkingDirectories() error {
 		return errors.Errorf("runroot(%q) and dataroot(%q) must be different paths", daemonOpts.RunRoot, daemonOpts.DataRoot)
 	}
 
-	buildTmpDir := filepath.Join(daemonOpts.DataRoot, dataRootTmpDirPrefix)
+	buildTmpDir, err := securejoin.SecureJoin(daemonOpts.DataRoot, dataRootTmpDirPrefix)
+	if err != nil {
+		return err
+	}
 	dirs := []string{daemonOpts.DataRoot, daemonOpts.RunRoot, buildTmpDir}
 	for _, dir := range dirs {
 		if !filepath.IsAbs(dir) {
@@ -325,7 +345,7 @@ func checkAndValidateConfig(cmd *cobra.Command) error {
 
 	// if storage config file exists, merge storage config
 	if util.IsExist(constant.StorageConfigPath) {
-		mergeStorageConfig(cmd)
+		return mergeStorageConfig(cmd)
 	}
 
 	return nil
diff --git a/daemon/daemon.go b/daemon/daemon.go
index 1237f35d..90ccf648 100644
--- a/daemon/daemon.go
+++ b/daemon/daemon.go
@@ -23,6 +23,7 @@ import (
 	"time"
 
 	"github.com/containerd/containerd/sys/reaper"
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
@@ -130,10 +131,20 @@ func (d *Daemon) Run() (err error) {
 
 // NewBuilder returns the builder with request sent from GRPC service
 func (d *Daemon) NewBuilder(ctx context.Context, req *pb.BuildRequest) (b builder.Builder, err error) {
+	var (
+		buildDir string
+		runDir   string
+	)
 	// buildDir is used to set directory which is used to store tmp data
-	buildDir := filepath.Join(d.opts.DataRoot, dataRootTmpDirPrefix, req.BuildID)
+	buildDir, err = securejoin.SecureJoin(d.opts.DataRoot, filepath.Join(dataRootTmpDirPrefix, req.BuildID))
+	if err != nil {
+		return nil, err
+	}
 	// runDir is used to store such as container bundle directories
-	runDir := filepath.Join(d.opts.RunRoot, req.BuildID)
+	runDir, err = securejoin.SecureJoin(d.opts.RunRoot, req.BuildID)
+	if err != nil {
+		return nil, err
+	}
 
 	// this key with BuildDir will be used by exporter to save blob temporary
 	// NOTE: keep it be updated before NewBuilder. ctx will be taken by Builder
diff --git a/daemon/import.go b/daemon/import.go
index a72d732e..21ffeaa3 100644
--- a/daemon/import.go
+++ b/daemon/import.go
@@ -22,6 +22,7 @@ import (
 	"github.com/containers/image/v5/tarball"
 	"github.com/containers/image/v5/transports"
 	"github.com/containers/image/v5/types"
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sync/errgroup"
@@ -43,6 +44,7 @@ func (b *Backend) Import(req *pb.ImportRequest, stream pb.Control_ImportServer)
 		source     = req.Source
 		reference  = req.Reference
 		importID   = req.ImportID
+		tmpDir     string
 	)
 	logEntry := logrus.WithFields(logrus.Fields{"ImportID": importID})
 	logEntry.Info("ImportRequest received")
@@ -78,8 +80,10 @@ func (b *Backend) Import(req *pb.ImportRequest, stream pb.Control_ImportServer)
 
 	log := logger.NewCliLogger(constant.CliLogBufferLen)
 	imageCopyOptions := image.NewImageCopyOptions(log)
-
-	tmpDir := filepath.Join(b.daemon.opts.DataRoot, dataRootTmpDirPrefix, importID)
+	tmpDir, err = securejoin.SecureJoin(b.daemon.opts.DataRoot, filepath.Join(dataRootTmpDirPrefix, importID))
+	if err != nil {
+		return err
+	}
 	if err = os.MkdirAll(tmpDir, constant.DefaultRootDirMode); err != nil {
 		logEntry.Error(err)
 		return err
diff --git a/daemon/load.go b/daemon/load.go
index d3f9bf6e..1f4c2f87 100644
--- a/daemon/load.go
+++ b/daemon/load.go
@@ -14,10 +14,9 @@
 package daemon
 
 import (
-	"path/filepath"
-
 	"github.com/containers/image/v5/docker/tarfile"
 	"github.com/containers/storage"
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sync/errgroup"
@@ -85,7 +84,10 @@ func (b *Backend) Load(req *pb.LoadRequest, stream pb.Control_LoadServer) error
 
 func getRepoTagFromImageTar(dataRoot, path string) ([]string, error) {
 	// tmp dir will be removed after NewSourceFromFileWithContext
-	tmpDir := filepath.Join(dataRoot, dataRootTmpDirPrefix)
+	tmpDir, err := securejoin.SecureJoin(dataRoot, dataRootTmpDirPrefix)
+	if err != nil {
+		return nil, err
+	}
 	systemContext := image.GetSystemContext()
 	systemContext.BigFilesTemporaryDir = tmpDir
 
diff --git a/daemon/login.go b/daemon/login.go
index 012816a4..e3399983 100644
--- a/daemon/login.go
+++ b/daemon/login.go
@@ -16,11 +16,11 @@ package daemon
 import (
 	"context"
 	"crypto"
-	"path/filepath"
 
 	"github.com/containers/image/v5/docker"
 	"github.com/containers/image/v5/pkg/docker/config"
 	"github.com/containers/image/v5/types"
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 
@@ -48,12 +48,16 @@ func (b *Backend) Login(ctx context.Context, req *pb.LoginRequest) (*pb.LoginRes
 		"Username": req.GetUsername(),
 	}).Info("LoginRequest received")
 
-	if err := validLoginOpts(req); err != nil {
+	err := validLoginOpts(req)
+	if err != nil {
 		return &pb.LoginResponse{Content: loginFailed}, err
 	}
 
 	sysCtx := image.GetSystemContext()
-	sysCtx.DockerCertPath = filepath.Join(constant.DefaultCertRoot, req.Server)
+	sysCtx.DockerCertPath, err = securejoin.SecureJoin(constant.DefaultCertRoot, req.Server)
+	if err != nil {
+		return &pb.LoginResponse{Content: loginFailed}, err
+	}
 
 	if loginWithAuthFile(req) {
 		auth, err := config.GetCredentials(sysCtx, req.Server)
diff --git a/daemon/logout.go b/daemon/logout.go
index 82c5beac..355b1f7a 100644
--- a/daemon/logout.go
+++ b/daemon/logout.go
@@ -16,10 +16,10 @@ package daemon
 import (
 	"context"
 	"fmt"
-	"path/filepath"
 	"strings"
 
 	"github.com/containers/image/v5/pkg/docker/config"
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 
@@ -35,12 +35,16 @@ func (b *Backend) Logout(ctx context.Context, req *pb.LogoutRequest) (*pb.Logout
 		"All":    req.GetAll(),
 	}).Info("LogoutRequest received")
 
-	if err := validLogoutOpts(req); err != nil {
+	err := validLogoutOpts(req)
+	if err != nil {
 		return &pb.LogoutResponse{Result: "Logout Failed"}, err
 	}
 
 	sysCtx := image.GetSystemContext()
-	sysCtx.DockerCertPath = filepath.Join(constant.DefaultCertRoot, req.Server)
+	sysCtx.DockerCertPath, err = securejoin.SecureJoin(constant.DefaultCertRoot, req.Server)
+	if err != nil {
+		return &pb.LogoutResponse{Result: "Logout Failed"}, err
+	}
 
 	if req.All {
 		if err := config.RemoveAllAuthentication(sysCtx); err != nil {
@@ -51,8 +55,7 @@ func (b *Backend) Logout(ctx context.Context, req *pb.LogoutRequest) (*pb.Logout
 		return &pb.LogoutResponse{Result: "Removed authentications"}, nil
 	}
 
-	err := config.RemoveAuthentication(sysCtx, req.Server)
-	if err == nil {
+	if err = config.RemoveAuthentication(sysCtx, req.Server); err == nil {
 		msg := fmt.Sprintf("Removed authentication for %s", req.Server)
 		logrus.Infof("Success logout from server: %q", req.Server)
 		return &pb.LogoutResponse{Result: msg}, nil
diff --git a/exporter/common.go b/exporter/common.go
index 1953b4e1..6e70a38e 100644
--- a/exporter/common.go
+++ b/exporter/common.go
@@ -20,7 +20,6 @@ import (
 	"io"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"strings"
 
 	cp "github.com/containers/image/v5/copy"
@@ -30,6 +29,7 @@ import (
 	"github.com/containers/image/v5/types"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/stringid"
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/docker/distribution/reference"
 	"github.com/opencontainers/go-digest"
 	"github.com/pkg/errors"
@@ -72,15 +72,12 @@ func Export(src, destSpec string, opts ExportOptions, localStore *store.Store) e
 	if err != nil {
 		return err
 	}
-	opts.SystemContext.DockerCertPath = filepath.Join(constant.DefaultCertRoot, registry)
-
-	options := NewCopyOptions(opts)
-
-	policyContext, err := NewPolicyContext(opts.SystemContext)
+	opts.SystemContext.DockerCertPath, err = securejoin.SecureJoin(constant.DefaultCertRoot, registry)
 	if err != nil {
 		return err
 	}
-	ref, digest, err := export(opts, epter, policyContext, options)
+
+	ref, digest, err := export(epter, opts)
 	if err != nil {
 		return errors.Errorf("export image from %s to %s failed, got error: %s", src, destSpec, err)
 	}
@@ -117,13 +114,18 @@ func exportToIsulad(ctx context.Context, tarPath string) error {
 	return nil
 }
 
-func export(exOpts ExportOptions, e Exporter, policyContext *signature.PolicyContext, cpOpts *cp.Options) (reference.Canonical, digest.Digest, error) {
+func export(e Exporter, exOpts ExportOptions) (reference.Canonical, digest.Digest, error) {
 	var (
-		err            error
 		ref            reference.Canonical
 		manifestBytes  []byte
 		manifestDigest digest.Digest
 	)
+
+	cpOpts := NewCopyOptions(exOpts)
+	policyContext, err := NewPolicyContext(exOpts.SystemContext)
+	if err != nil {
+		return nil, "", err
+	}
 	defer func() {
 		destroyErr := policyContext.Destroy()
 		if err == nil {
@@ -176,7 +178,10 @@ func parseExporter(opts ExportOptions, src, destSpec string, localStore *store.S
 	// 3. get dest reference
 	if parts[0] == "isulad" {
 		randomID := stringid.GenerateNonCryptoID()[:constant.DefaultIDLen]
-		isuladTarPath = filepath.Join(opts.DataDir, fmt.Sprintf("isula-build-tmp-%s.tar", randomID))
+		isuladTarPath, err = securejoin.SecureJoin(opts.DataDir, fmt.Sprintf("isula-build-tmp-%s.tar", randomID))
+		if err != nil {
+			return nil, "", err
+		}
 		// construct format: transport:path:image:tag
 		// parts[1] here could not be empty cause client-end already processed it
 		destSpec = fmt.Sprintf("docker-archive:%s:%s", isuladTarPath, parts[1])
diff --git a/store/store.go b/store/store.go
index 263d69e8..410eef11 100644
--- a/store/store.go
+++ b/store/store.go
@@ -131,15 +131,15 @@ func (s *Store) CleanContainer(id string) error {
 
 	// Do not care about all the errors whiling cleaning the container,
 	// just return one if the error occurs.
-	var err error
+	var finalErr error
 	if _, uerr := s.Unmount(id, false); uerr != nil {
-		err = uerr
+		finalErr = uerr
 		logrus.Warnf("Unmount container store failed while cleaning %q", id)
 	}
 	if derr := s.DeleteContainer(id); derr != nil {
-		err = derr
+		finalErr = derr
 		logrus.Warnf("Delete container store failed while cleaning %q", id)
 	}
 
-	return err
+	return finalErr
 }
diff --git a/util/common.go b/util/common.go
index 9e2e2537..5cd4bb28 100644
--- a/util/common.go
+++ b/util/common.go
@@ -20,6 +20,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 	"golang.org/x/sys/unix"
@@ -152,6 +153,17 @@ func ParseServer(server string) (string, error) {
 		return "", errors.Errorf("invalid registry address %s", server)
 	}
 
+	// to prevent directory traversal
+	fakePrefix := "/fakePrefix"
+	origAddr := fmt.Sprintf("%s/%s", fakePrefix, fields[0])
+	cleanAddr, err := securejoin.SecureJoin(fakePrefix, fields[0])
+	if err != nil {
+		return "", err
+	}
+	if cleanAddr != origAddr {
+		return "", errors.Errorf("invalid relative path detected")
+	}
+
 	return fields[0], nil
 }
 
diff --git a/util/common_test.go b/util/common_test.go
index b48c508f..e9b6ad85 100644
--- a/util/common_test.go
+++ b/util/common_test.go
@@ -153,17 +153,29 @@ func TestParseServer(t *testing.T) {
 			want:    "",
 			wantErr: true,
 		},
+		{
+			name:    "TC10 - abnormal server address with relative filepath",
+			args:    args{server: "https://mydockerhub/../../../"},
+			want:    "mydockerhub",
+			wantErr: false,
+		},
+		{
+			name:    "TC11 - abnormal server address with relative filepath 2",
+			args:    args{server: "https://../../../../mydockerhub"},
+			want:    "",
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			got, err := ParseServer(tt.args.server)
+			if got != tt.want {
+				t.Errorf("ParseServer() got = %v, want %v", got, tt.want)
+			}
 			if (err != nil) != tt.wantErr {
 				t.Errorf("ParseServer() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
-			if got != tt.want {
-				t.Errorf("ParseServer() got = %v, want %v", got, tt.want)
-			}
 		})
 	}
 }
-- 
2.27.0

