From f4d473efd455fdb78ead41e8b4b3a0c7b2527efe Mon Sep 17 00:00:00 2001 From: caihaomin Date: Tue, 8 Dec 2020 15:41:07 +0800 Subject: [PATCH] isula-build:fix problems found by code review Signed-off-by: caihaomin --- VERSION-openeuler | 2 +- git-commit | 2 +- isula-build.spec | 9 +- ...de-fix-problems-found-by-code-review.patch | 583 ++++++++++++++++++ series.conf | 1 + 5 files changed, 592 insertions(+), 5 deletions(-) create mode 100644 patch/0073-cleancode-fix-problems-found-by-code-review.patch diff --git a/VERSION-openeuler b/VERSION-openeuler index fb05724..f98c31e 100644 --- a/VERSION-openeuler +++ b/VERSION-openeuler @@ -1 +1 @@ -0.9.4-10 +0.9.4-11 diff --git a/git-commit b/git-commit index 874c71c..1285887 100644 --- a/git-commit +++ b/git-commit @@ -1 +1 @@ -80be085896f4701b8d13b28d423d3a4cd72df00d +7def8f00bd20c7be542ff86a6c3009eed8cb6149 diff --git a/isula-build.spec b/isula-build.spec index 510d762..9141ec6 100644 --- a/isula-build.spec +++ b/isula-build.spec @@ -2,7 +2,7 @@ Name: isula-build Version: 0.9.4 -Release: 10 +Release: 11 Summary: A tool to build container images License: Mulan PSL V2 URL: https://gitee.com/openeuler/isula-build @@ -85,10 +85,13 @@ fi /usr/share/bash-completion/completions/isula-build %changelog -* Fir Nov 27 2020 caihaomin - 0.9.4-10 +* Tue Dec 08 2020 caihaomin - 0.9.4-11 +- Fix problems found by code review + +* Tue Dec 08 2020 caihaomin - 0.9.4-10 - Add more fuzz tests -* Fir Nov 27 2020 caihaomin - 0.9.4-9 +* Tue Dec 08 2020 caihaomin - 0.9.4-9 - Imporve daemon push and pull unit test * Fir Nov 27 2020 lixiang - 0.9.4-8 diff --git a/patch/0073-cleancode-fix-problems-found-by-code-review.patch b/patch/0073-cleancode-fix-problems-found-by-code-review.patch new file mode 100644 index 0000000..60f22ea --- /dev/null +++ b/patch/0073-cleancode-fix-problems-found-by-code-review.patch @@ -0,0 +1,583 @@ +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 + diff --git a/series.conf b/series.conf index 2685f64..cf6d35f 100644 --- a/series.conf +++ b/series.conf @@ -10,3 +10,4 @@ patch/0069-isula-build-mask-proc-pin_memory.patch patch/0070-hack-add-compile-flag-ftrapv.patch patch/0071-imporve-daemon-push-and-pull-unit-test.patch patch/0072-fuzz-add-more-fuzz-tests.patch +patch/0073-cleancode-fix-problems-found-by-code-review.patch