Skip to content

Commit

Permalink
fix: prevent path traversal attacks (#631)
Browse files Browse the repository at this point in the history
This commit fixes a path traversal vulnerability in the repository
management code. The `SanitizeRepo` function now correctly returns a
sanitized version of the given repository name. It uses an absolute
path along with `path.Clean` to ensure that the path is cleaned
before being used.
  • Loading branch information
aymanbagabas authored Jan 7, 2025
1 parent 0fb868c commit a8d1bf3
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 21 deletions.
37 changes: 18 additions & 19 deletions pkg/backend/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"path"
"path/filepath"
"strconv"
"strings"
"time"

"github.com/charmbracelet/soft-serve/git"
Expand All @@ -24,10 +25,6 @@ import (
"github.com/charmbracelet/soft-serve/pkg/webhook"
)

func (d *Backend) reposPath() string {
return filepath.Join(d.cfg.DataPath, "repos")
}

// CreateRepository creates a new repository.
//
// It implements backend.Backend.
Expand All @@ -37,8 +34,7 @@ func (d *Backend) CreateRepository(ctx context.Context, name string, user proto.
return nil, err
}

repo := name + ".git"
rp := filepath.Join(d.reposPath(), repo)
rp := filepath.Join(d.repoPath(name))

var userID int64
if user != nil {
Expand Down Expand Up @@ -78,7 +74,7 @@ func (d *Backend) CreateRepository(ctx context.Context, name string, user proto.
}
}

return hooks.GenerateHooks(ctx, d.cfg, repo)
return hooks.GenerateHooks(ctx, d.cfg, name)
}); err != nil {
d.logger.Debug("failed to create repository in database", "err", err)
err = db.WrapError(err)
Expand All @@ -100,8 +96,7 @@ func (d *Backend) ImportRepository(_ context.Context, name string, user proto.Us
return nil, err
}

repo := name + ".git"
rp := filepath.Join(d.reposPath(), repo)
rp := filepath.Join(d.repoPath(name))

tid := "import:" + name
if d.manager.Exists(tid) {
Expand Down Expand Up @@ -217,8 +212,7 @@ func (d *Backend) ImportRepository(_ context.Context, name string, user proto.Us
// It implements backend.Backend.
func (d *Backend) DeleteRepository(ctx context.Context, name string) error {
name = utils.SanitizeRepo(name)
repo := name + ".git"
rp := filepath.Join(d.reposPath(), repo)
rp := filepath.Join(d.repoPath(name))

user := proto.UserFromContext(ctx)
r, err := d.Repository(ctx, name)
Expand Down Expand Up @@ -330,10 +324,8 @@ func (d *Backend) RenameRepository(ctx context.Context, oldName string, newName
return nil
}

oldRepo := oldName + ".git"
newRepo := newName + ".git"
op := filepath.Join(d.reposPath(), oldRepo)
np := filepath.Join(d.reposPath(), newRepo)
op := filepath.Join(d.repoPath(oldName))
np := filepath.Join(d.repoPath(newName))
if _, err := os.Stat(op); err != nil {
return proto.ErrRepoNotFound
}
Expand Down Expand Up @@ -389,7 +381,7 @@ func (d *Backend) Repositories(ctx context.Context) ([]proto.Repository, error)
for _, m := range ms {
r := &repo{
name: m.Name,
path: filepath.Join(d.reposPath(), m.Name+".git"),
path: filepath.Join(d.repoPath(m.Name)),
repo: m,
}

Expand Down Expand Up @@ -418,7 +410,7 @@ func (d *Backend) Repository(ctx context.Context, name string) (proto.Repository
return r, nil
}

rp := filepath.Join(d.reposPath(), name+".git")
rp := filepath.Join(d.repoPath(name))
if _, err := os.Stat(rp); err != nil {
if !errors.Is(err, fs.ErrNotExist) {
d.logger.Errorf("failed to stat repository path: %v", err)
Expand Down Expand Up @@ -552,7 +544,7 @@ func (d *Backend) SetHidden(ctx context.Context, name string, hidden bool) error
// It implements backend.Backend.
func (d *Backend) SetDescription(ctx context.Context, name string, desc string) error {
name = utils.SanitizeRepo(name)
rp := filepath.Join(d.reposPath(), name+".git")
rp := filepath.Join(d.repoPath(name))

// Delete cache
d.cache.Delete(name)
Expand All @@ -572,7 +564,7 @@ func (d *Backend) SetDescription(ctx context.Context, name string, desc string)
// It implements backend.Backend.
func (d *Backend) SetPrivate(ctx context.Context, name string, private bool) error {
name = utils.SanitizeRepo(name)
rp := filepath.Join(d.reposPath(), name+".git")
rp := filepath.Join(d.repoPath(name))

// Delete cache
d.cache.Delete(name)
Expand Down Expand Up @@ -636,6 +628,13 @@ func (d *Backend) SetProjectName(ctx context.Context, repo string, name string)
)
}

// repoPath returns the path to a repository.
func (d *Backend) repoPath(name string) string {
name = utils.SanitizeRepo(name)
rn := strings.ReplaceAll(name, "/", string(os.PathSeparator))
return filepath.Join(filepath.Join(d.cfg.DataPath, "repos"), rn+".git")
}

var _ proto.Repository = (*repo)(nil)

// repo is a Git repository with metadata stored in a SQLite database.
Expand Down
2 changes: 1 addition & 1 deletion pkg/ssh/cmd/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ func checkIfAdmin(cmd *cobra.Command, args []string) error {
func checkIfCollab(cmd *cobra.Command, args []string) error {
var repo string
if len(args) > 0 {
repo = args[0]
repo = utils.SanitizeRepo(args[0])
}

ctx := cmd.Context()
Expand Down
5 changes: 4 additions & 1 deletion pkg/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,15 @@ import (

// SanitizeRepo returns a sanitized version of the given repository name.
func SanitizeRepo(repo string) string {
// We need to use an absolute path for the path to be cleaned correctly.
repo = strings.TrimPrefix(repo, "/")
repo = "/" + repo

// We're using path instead of filepath here because this is not OS dependent
// looking at you Windows
repo = path.Clean(repo)
repo = strings.TrimSuffix(repo, ".git")
return repo
return repo[1:]
}

// ValidateUsername returns an error if any of the given usernames are invalid.
Expand Down

0 comments on commit a8d1bf3

Please sign in to comment.