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.

Sign up for GitHub

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

Jump to bottom

Use CleanPath instead of path.Clean #23371

Merged
merged 2 commits into from
Mar 8, 2023
Merged

Use CleanPath instead of path.Clean #23371

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
46aaa2e
Use CleanPath instead of path.Clean
lunny Mar 8, 2023
c9371bc
merge main branch
lunny Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions models/git/lfs_lock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package git
import (
"context"
"fmt"
"path"
"strings"
"time"

Expand All @@ -17,6 +16,7 @@ import (
"code.gitea.io/gitea/models/unit"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
)

// LFSLock represents a git lfs lock of repository.
Expand All @@ -34,11 +34,7 @@ func init() {

// BeforeInsert is invoked from XORM before inserting an object of this type.
func (l *LFSLock) BeforeInsert() {
l.Path = cleanPath(l.Path)
}

func cleanPath(p string) string {
return path.Clean("/" + p)[1:]
l.Path = util.CleanPath(l.Path)
}

// CreateLFSLock creates a new lock.
Expand All @@ -53,7 +49,7 @@ func CreateLFSLock(ctx context.Context, repo *repo_model.Repository, lock *LFSLo
return nil, err
}

lock.Path = cleanPath(lock.Path)
lock.Path = util.CleanPath(lock.Path)
lock.RepoID = repo.ID

l, err := GetLFSLock(dbCtx, repo, lock.Path)
Expand All @@ -73,7 +69,7 @@ func CreateLFSLock(ctx context.Context, repo *repo_model.Repository, lock *LFSLo

// GetLFSLock returns release by given path.
func GetLFSLock(ctx context.Context, repo *repo_model.Repository, path string) (*LFSLock, error) {
path = cleanPath(path)
path = util.CleanPath(path)
rel := &LFSLock{RepoID: repo.ID}
has, err := db.GetEngine(ctx).Where("lower(path) = ?", strings.ToLower(path)).Get(rel)
if err != nil {
Expand Down
3 changes: 1 addition & 2 deletions modules/options/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"fmt"
"os"
"path"
"strings"

"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
Expand All @@ -16,7 +15,7 @@ import (

// GetRepoInitFile returns repository init files
func GetRepoInitFile(tp, name string) ([]byte, error) {
cleanedName := strings.TrimLeft(path.Clean("/"+name), "/")
lunny marked this conversation as resolved.
Show resolved Hide resolved
cleanedName := util.CleanPath(name)
relPath := path.Join("options", tp, cleanedName)

// Use custom file when available.
Expand Down
4 changes: 2 additions & 2 deletions modules/public/public.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ package public
import (
"net/http"
"os"
"path"
"path/filepath"
"strings"

"code.gitea.io/gitea/modules/container"
"code.gitea.io/gitea/modules/httpcache"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
)

// Options represents the available options to configure the handler.
Expand Down Expand Up @@ -103,7 +103,7 @@ func setWellKnownContentType(w http.ResponseWriter, file string) {

func (opts *Options) handle(w http.ResponseWriter, req *http.Request, fs http.FileSystem, file string) bool {
// use clean to keep the file is a valid path with no . or ..
f, err := fs.Open(path.Clean(file))
f, err := fs.Open(util.CleanPath(file))
if err != nil {
if os.IsNotExist(err) {
return false
Expand Down
3 changes: 1 addition & 2 deletions modules/storage/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"io"
"net/url"
"os"
"path"
"path/filepath"
"strings"

Expand Down Expand Up @@ -59,7 +58,7 @@ func NewLocalStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
}

func (l *LocalStorage) buildLocalPath(p string) string {
return filepath.Join(l.dir, path.Clean("/" + strings.ReplaceAll(p, "\\", "/"))[1:])
return filepath.Join(l.dir, util.CleanPath(strings.ReplaceAll(p, "\\", "/")))
}

// Open a file
Expand Down
3 changes: 2 additions & 1 deletion modules/storage/minio.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"time"

"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/util"

"github.com/minio/minio-go/v7"
"github.com/minio/minio-go/v7/pkg/credentials"
Expand Down Expand Up @@ -120,7 +121,7 @@ func NewMinioStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
}

func (m *MinioStorage) buildMinioPath(p string) string {
return strings.TrimPrefix(path.Join(m.basePath, path.Clean("/" + strings.ReplaceAll(p, "\\", "/"))[1:]), "/")
return strings.TrimPrefix(path.Join(m.basePath, util.CleanPath(strings.ReplaceAll(p, "\\", "/"))), "/")
}

// Open open a file
Expand Down
8 changes: 8 additions & 0 deletions modules/util/path.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ import (
"strings"
)

// CleanPath ensure to clean the path
func CleanPath(p string) string {
if strings.HasPrefix(p, "/") {
return path.Clean(p)
}
return path.Clean("/" + p)[1:]
Comment on lines +19 to +22
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just read through the documentation of the method, this whole block is unnecessary and can be replaced with return path.Clean("/" + p)[1:].
That way, we ensure that the path is cleaned and that it is a relative path.
If we always want relative paths, we can just use that instead.
So the question is rather: Is there any use case where we need to clean an absolute path?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just read through the documentation of the method, this whole block is unnecessary and can be replaced with return path.Clean("/" + p)[1:]. That way, we ensure that the path is cleaned and that it is a relative path. If we always want relative paths, we can just use that instead. So the question is rather: Is there any use case where we need to clean an absolute path?

I don't think so except we rename the function to CleanAndEnsureRelativePath.

}

// EnsureAbsolutePath ensure that a path is absolute, making it
// relative to absoluteBase if necessary
func EnsureAbsolutePath(path, absoluteBase string) string {
Expand Down
12 changes: 12 additions & 0 deletions modules/util/path_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,3 +136,15 @@ func TestMisc_IsReadmeFileName(t *testing.T) {
assert.Equal(t, testCase.idx, idx)
}
}

func TestCleanPath(t *testing.T) {
cases := map[string]string{
"../../test": "test",
"/test": "/test",
"/../test": "/test",
}

for k, v := range cases {
assert.Equal(t, v, CleanPath(k))
}
}
5 changes: 3 additions & 2 deletions routers/web/base.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/storage"
"code.gitea.io/gitea/modules/templates"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web/middleware"
"code.gitea.io/gitea/modules/web/routing"
"code.gitea.io/gitea/services/auth"
Expand All @@ -44,7 +45,7 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
routing.UpdateFuncInfo(req.Context(), funcInfo)

rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
rPath = path.Clean("/" + strings.ReplaceAll(rPath, "\\", "/"))[1:]
rPath = util.CleanPath(strings.ReplaceAll(rPath, "\\", "/"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong and is going to result in it being possible to have rPath have a preceding "/" whereas previously it was impossible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If rPath has a / prefix, it will be invoked with path.Clean directly. I think it should be better than the current logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

util.CleanPath will result in rPath still having the / whereas previous it would always be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if util.CleanPath here will not convert absolute path to relative path, but line 47 will ensure rPath is a relative path.

Copy link
Contributor

@wxiaoguang wxiaoguang Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need clear definitions for every case, instead of using unpredictable CleanPath behavior.

The unstable part is :

  • CleanPath("/path") => /path
  • CleanPath("path") => path

But in many cases, we the caller only wants path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that's not CleanPath's problem. I sent #23446 to fix possible unclear places

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem is that CleanPath's behavior is not that stable in many cases.

Callers should always know what they need - absolute or relative - no matter what path has been passed in the util function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolute or relative should not be the responsibility of CleanPath. It should be decided out of the function. Or we can rename CleanPath to CleanAndEnsureRelativePath.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm .. that's a disagreement: I do not think so about "absolute or relative should not be the responsibility of CleanPath. It should be decided out of the function."

IMO maybe it could be (like #23441)

rel := util.PathRelJoin(....)   //  `/path` => `path`,  `path` => `path`
abs := path.Join("/", util.PathRelJoin(....))  // if necessary


u, err := objStore.URL(rPath, path.Base(rPath))
if err != nil {
Expand Down Expand Up @@ -80,7 +81,7 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
routing.UpdateFuncInfo(req.Context(), funcInfo)

rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
rPath = path.Clean("/" + strings.ReplaceAll(rPath, "\\", "/"))[1:]
rPath = util.CleanPath(strings.ReplaceAll(rPath, "\\", "/"))
if rPath == "" {
http.Error(w, "file not found", http.StatusNotFound)
return
Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/editor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,7 @@ func UploadFilePost(ctx *context.Context) {

func cleanUploadFileName(name string) string {
// Rebase the filename
name = strings.Trim(path.Clean("/"+name), "/")
name = strings.Trim(util.CleanPath(name), "/")
// Git disallows any filenames to have a .git directory in them.
for _, part := range strings.Split(name, "/") {
if strings.ToLower(part) == ".git" {
Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/lfs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ func LFSLockFile(ctx *context.Context) {
ctx.Redirect(ctx.Repo.RepoLink + "/settings/lfs/locks")
return
}
lockPath = path.Clean("/" + lockPath)[1:]
lockPath = util.CleanPath(lockPath)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can result in a lockPath with a preceding "/". This is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and everywhere else too

if len(lockPath) == 0 {
ctx.Flash.Error(ctx.Tr("repo.settings.lfs_invalid_locking_path", originalPath))
ctx.Redirect(ctx.Repo.RepoLink + "/settings/lfs/locks")
Expand Down
4 changes: 2 additions & 2 deletions services/migrations/gitea_uploader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"fmt"
"io"
"os"
"path"
"path/filepath"
"strconv"
"strings"
Expand All @@ -30,6 +29,7 @@ import (
"code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/uri"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/services/pull"

"github.com/google/uuid"
Expand Down Expand Up @@ -866,7 +866,7 @@ func (g *GiteaLocalUploader) CreateReviews(reviews ...*base.Review) error {
}

// SECURITY: The TreePath must be cleaned!
comment.TreePath = path.Clean("/" + comment.TreePath)[1:]
comment.TreePath = util.CleanPath(comment.TreePath)

var patch string
reader, writer := io.Pipe()
Expand Down
4 changes: 2 additions & 2 deletions services/packages/container/blob_uploader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ import (
"errors"
"io"
"os"
"path"
"path/filepath"
"strings"

packages_model "code.gitea.io/gitea/models/packages"
packages_module "code.gitea.io/gitea/modules/packages"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
)

var (
Expand All @@ -33,7 +33,7 @@ type BlobUploader struct {
}

func buildFilePath(id string) string {
return filepath.Join(setting.Packages.ChunkedUploadPath, path.Clean("/" + strings.ReplaceAll(id, "\\", "/"))[1:])
return filepath.Join(setting.Packages.ChunkedUploadPath, util.CleanPath(strings.ReplaceAll(id, "\\", "/")))
}

// NewBlobUploader creates a new blob uploader for the given id
Expand Down
4 changes: 2 additions & 2 deletions services/repository/files/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ import (
"context"
"fmt"
"net/url"
"path"
"strings"
"time"

repo_model "code.gitea.io/gitea/models/repo"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/git"
api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/util"
)

// GetFileResponseFromCommit Constructs a FileResponse from a Commit object
Expand Down Expand Up @@ -129,7 +129,7 @@ func GetAuthorAndCommitterUsers(author, committer *IdentityOptions, doer *user_m
// CleanUploadFileName Trims a filename and returns empty string if it is a .git directory
func CleanUploadFileName(name string) string {
// Rebase the filename
name = strings.Trim(path.Clean("/"+name), "/")
name = strings.Trim(util.CleanPath(name), "/")
// Git disallows any filenames to have a .git directory in them.
for _, part := range strings.Split(name, "/") {
if strings.ToLower(part) == ".git" {
Expand Down