Skip to content

Commit

Permalink
Clean paths when looking in Storage (go-gitea#19124) (go-gitea#19179)
Browse files Browse the repository at this point in the history 
Backport go-gitea#19124

* Clean paths when looking in Storage

Ensure paths are clean for minio aswell as local storage.

Use url.Path not RequestURI/EscapedPath in storageHandler.

Signed-off-by: Andrew Thornton <[email protected]>

* Apply suggestions from code review

Co-authored-by: Lauris BH <[email protected]>
  • Loading branch information
@zeripath @lafriks
zeripath and lafriks authored Mar 23, 2022
1 parent 743553f commit d21b7fd
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 56 deletions.
34 changes: 8 additions & 26 deletions modules/storage/local.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package storage

import (
"context"
"errors"
"io"
"net/url"
"os"
Expand All @@ -18,8 +17,6 @@ import (
"code.gitea.io/gitea/modules/util"
)

// ErrLocalPathNotSupported represents an error that path is not supported
var ErrLocalPathNotSupported = errors.New("local path is not supported")
var _ ObjectStorage = &LocalStorage{}

// LocalStorageType is the type descriptor for local storage
Expand Down Expand Up @@ -62,21 +59,18 @@ func NewLocalStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
}, nil
}

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

// Open a file
func (l *LocalStorage) Open(path string) (Object, error) {
if !isLocalPathValid(path) {
return nil, ErrLocalPathNotSupported
}
return os.Open(filepath.Join(l.dir, path))
return os.Open(l.buildLocalPath(path))
}

// Save a file
func (l *LocalStorage) Save(path string, r io.Reader, size int64) (int64, error) {
if !isLocalPathValid(path) {
return 0, ErrLocalPathNotSupported
}

p := filepath.Join(l.dir, path)
p := l.buildLocalPath(path)
if err := os.MkdirAll(filepath.Dir(p), os.ModePerm); err != nil {
return 0, err
}
Expand Down Expand Up @@ -116,24 +110,12 @@ func (l *LocalStorage) Save(path string, r io.Reader, size int64) (int64, error)

// Stat returns the info of the file
func (l *LocalStorage) Stat(path string) (os.FileInfo, error) {
return os.Stat(filepath.Join(l.dir, path))
}

func isLocalPathValid(p string) bool {
a := path.Clean(p)
if strings.HasPrefix(a, "../") || strings.HasPrefix(a, "..\\") {
return false
}
return a == p
return os.Stat(l.buildLocalPath(path))
}

// Delete delete a file
func (l *LocalStorage) Delete(path string) error {
if !isLocalPathValid(path) {
return ErrLocalPathNotSupported
}
p := filepath.Join(l.dir, path)
return util.Remove(p)
return util.Remove(l.buildLocalPath(path))
}

// URL gets the redirect URL to a file
Expand Down
34 changes: 21 additions & 13 deletions modules/storage/local_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,36 +10,44 @@ import (
"github.com/stretchr/testify/assert"
)

func TestLocalPathIsValid(t *testing.T) {
func TestBuildLocalPath(t *testing.T) {
kases := []struct {
path string
valid bool
localDir string
path string
expected string
}{
{
"a",
"0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
"a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
true,
},
{
"../a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
false,
"a",
"../0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
"a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
},
{
"a\\0\\a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
true,
"a",
"0\\a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
"a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
},
{
"b/../a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
false,
"b",
"a/../0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
"b/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
},
{
"..\\a/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
false,
"b",
"a\\..\\0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
"b/0/a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a14",
},
}

for _, k := range kases {
t.Run(k.path, func(t *testing.T) {
assert.EqualValues(t, k.valid, isLocalPathValid(k.path))
l := LocalStorage{dir: k.localDir}

assert.EqualValues(t, k.expected, l.buildLocalPath(k.path))
})
}
}
2 changes: 1 addition & 1 deletion modules/storage/minio.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func NewMinioStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
}

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

// Open open a file
Expand Down
32 changes: 16 additions & 16 deletions routers/web/base.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"net/http"
"os"
"path"
"path/filepath"
"strings"

"code.gitea.io/gitea/modules/context"
Expand All @@ -27,6 +26,8 @@ import (
)

func storageHandler(storageSetting setting.Storage, prefix string, objStore storage.ObjectStorage) func(next http.Handler) http.Handler {
prefix = strings.Trim(prefix, "/")

return func(next http.Handler) http.Handler {
if storageSetting.ServeDirect {
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
Expand All @@ -35,12 +36,14 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
return
}

if !strings.HasPrefix(req.URL.RequestURI(), "/"+prefix) {
if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") {
next.ServeHTTP(w, req)
return
}

rPath := strings.TrimPrefix(req.URL.RequestURI(), "/"+prefix)
rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
rPath = path.Clean("/" + strings.ReplaceAll(rPath, "\\", "/"))[1:]

u, err := objStore.URL(rPath, path.Base(rPath))
if err != nil {
if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) {
Expand All @@ -52,11 +55,12 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
http.Error(w, fmt.Sprintf("Error whilst getting URL for %s %s", prefix, rPath), 500)
return
}

http.Redirect(
w,
req,
u.String(),
301,
http.StatusMovedPermanently,
)
})
}
Expand All @@ -67,28 +71,24 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
return
}

prefix := strings.Trim(prefix, "/")

if !strings.HasPrefix(req.URL.EscapedPath(), "/"+prefix+"/") {
if !strings.HasPrefix(req.URL.Path, "/"+prefix+"/") {
next.ServeHTTP(w, req)
return
}

rPath := strings.TrimPrefix(req.URL.EscapedPath(), "/"+prefix+"/")
rPath = strings.TrimPrefix(rPath, "/")
rPath := strings.TrimPrefix(req.URL.Path, "/"+prefix+"/")
rPath = path.Clean("/" + strings.ReplaceAll(rPath, "\\", "/"))[1:]
if rPath == "" {
http.Error(w, "file not found", 404)
return
}
rPath = path.Clean("/" + filepath.ToSlash(rPath))
rPath = rPath[1:]

fi, err := objStore.Stat(rPath)
if err == nil && httpcache.HandleTimeCache(req, w, fi) {
return
}

//If we have matched and access to release or issue
// If we have matched and access to release or issue
fr, err := objStore.Open(rPath)
if err != nil {
if os.IsNotExist(err) || errors.Is(err, os.ErrNotExist) {
Expand Down Expand Up @@ -121,7 +121,7 @@ func (d *dataStore) GetData() map[string]interface{} {
// Recovery returns a middleware that recovers from any panics and writes a 500 and a log if so.
// This error will be created with the gitea 500 page.
func Recovery() func(next http.Handler) http.Handler {
var rnd = templates.HTMLRenderer()
rnd := templates.HTMLRenderer()
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
defer func() {
Expand All @@ -131,14 +131,14 @@ func Recovery() func(next http.Handler) http.Handler {

sessionStore := session.GetSession(req)

var lc = middleware.Locale(w, req)
var store = dataStore{
lc := middleware.Locale(w, req)
store := dataStore{
"Language": lc.Language(),
"CurrentURL": setting.AppSubURL + req.URL.RequestURI(),
"i18n": lc,
}

var user = context.GetContextUser(req)
user := context.GetContextUser(req)
if user == nil {
// Get user from session if logged in - do not attempt to sign-in
user = auth.SessionUser(sessionStore)
Expand Down

0 comments on commit d21b7fd

Please sign in to comment.