wiki: sanitize old wiki page name when editing (#8099)

internal/db/wiki.go

@@ -18,6 +18,7 @@ import (
 	"github.com/gogs/git-module"
 
 	"gogs.io/gogs/internal/conf"
+	"gogs.io/gogs/internal/pathutil"
 	"gogs.io/gogs/internal/repoutil"
 	"gogs.io/gogs/internal/sync"
 )
@@ -38,12 +39,12 @@ func ToWikiPageURL(name string) string {
 	return url.QueryEscape(name)
 }
 
-// ToWikiPageName formats a URL back to corresponding wiki page name,
-// and removes leading characters './' to prevent changing files
-// that are not belong to wiki repository.
+// ToWikiPageName formats a URL back to corresponding wiki page name. It enforces
+// single-level hierarchy by replacing all "/" with spaces.
 func ToWikiPageName(urlString string) string {
 	name, _ := url.QueryUnescape(urlString)
-	return strings.ReplaceAll(strings.TrimLeft(path.Clean("/"+name), "/"), "/", " ")
+	name = pathutil.Clean(name)
+	return strings.ReplaceAll(name, "/", " ")
 }
 
 // WikiCloneLink returns clone URLs of repository wiki.
@@ -97,16 +98,16 @@ func discardLocalWikiChanges(localPath string) error {
 }
 
 // updateWikiPage adds new page to repository wiki.
-func (repo *Repository) updateWikiPage(doer *User, oldTitle, title, content, message string, isNew bool) (err error) {
+func (repo *Repository) updateWikiPage(doer *User, oldTitle, title, content, message string, isNew bool) error {
 	wikiWorkingPool.CheckIn(com.ToStr(repo.ID))
 	defer wikiWorkingPool.CheckOut(com.ToStr(repo.ID))
 
-	if err = repo.InitWiki(); err != nil {
+	if err := repo.InitWiki(); err != nil {
 		return fmt.Errorf("InitWiki: %v", err)
 	}
 
 	localPath := repo.LocalWikiPath()
-	if err = discardLocalWikiChanges(localPath); err != nil {
+	if err := discardLocalWikiChanges(localPath); err != nil {
 		return fmt.Errorf("discardLocalWikiChanges: %v", err)
 	} else if err = repo.UpdateLocalWiki(); err != nil {
 		return fmt.Errorf("UpdateLocalWiki: %v", err)
@@ -121,7 +122,8 @@ func (repo *Repository) updateWikiPage(doer *User, oldTitle, title, content, mes
 			return ErrWikiAlreadyExist{filename}
 		}
 	} else {
-		os.Remove(path.Join(localPath, oldTitle+".md"))
+		oldTitle = ToWikiPageName(oldTitle)
+		_ = os.Remove(path.Join(localPath, oldTitle+".md"))
 	}
 
 	// SECURITY: if new file is a symlink to non-exist critical file,
@@ -129,20 +131,20 @@ func (repo *Repository) updateWikiPage(doer *User, oldTitle, title, content, mes
 	// as a new page operation.
 	// So we want to make sure the symlink is removed before write anything.
 	// The new file we created will be in normal text format.
-	os.Remove(filename)
+	_ = os.Remove(filename)
 
-	if err = os.WriteFile(filename, []byte(content), 0666); err != nil {
+	if err := os.WriteFile(filename, []byte(content), 0o666); err != nil {
 		return fmt.Errorf("WriteFile: %v", err)
 	}
 
 	if message == "" {
 		message = "Update page '" + title + "'"
 	}
-	if err = git.Add(localPath, git.AddOptions{All: true}); err != nil {
+	if err := git.Add(localPath, git.AddOptions{All: true}); err != nil {
 		return fmt.Errorf("add all changes: %v", err)
 	}
 
-	err = git.CreateCommit(
+	err := git.CreateCommit(
 		localPath,
 		&git.Signature{
 			Name:  doer.DisplayName(),
@@ -180,8 +182,7 @@ func (repo *Repository) DeleteWikiPage(doer *User, title string) (err error) {
 	}
 
 	title = ToWikiPageName(title)
-	filename := path.Join(localPath, title+".md")
-	os.Remove(filename)
+	_ = os.Remove(path.Join(localPath, title+".md"))
 
 	message := "Delete page '" + title + "'"
 

internal/db/wiki_test.go

@@ -0,0 +1,27 @@
+package db
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestToWikiPageName(t *testing.T) {
+	tests := []struct {
+		input string
+		want  string
+	}{
+		{input: "Home", want: "Home"},
+		{input: "../../../../tmp/target_file", want: "tmp target_file"},
+		{input: "..\\..\\..\\..\\tmp\\target_file", want: "tmp target_file"},
+		{input: "A/B", want: "A B"},
+		{input: "../pwn", want: "pwn"},
+	}
+
+	for _, test := range tests {
+		t.Run(test.input, func(t *testing.T) {
+			got := ToWikiPageName(test.input)
+			require.Equal(t, test.want, got)
+		})
+	}
+}

internal/route/repo/wiki.go

@@ -49,7 +49,7 @@ func renderWikiPage(c *context.Context, isViewPage bool) (*git.Repository, strin
 		c.Error(err, "open repository")
 		return nil, ""
 	}
-	commit, err := wikiRepo.BranchCommit(database.WikiBranch(wikiPath))
+	commit, err := wikiRepo.BranchCommit(db.WikiBranch(wikiPath))
 	if err != nil {
 		c.Error(err, "get branch commit")
 		return nil, ""
@@ -125,7 +125,7 @@ func Wiki(c *context.Context) {
 	}
 
 	// Get last change information.
-	branch := database.WikiBranch(c.Repo.Repository.WikiPath())
+	branch := db.WikiBranch(c.Repo.Repository.WikiPath())
 	commits, err := wikiRepo.Log(git.RefsHeads+branch, git.LogOptions{Path: pageName + ".md"})
 	if err != nil {
 		c.Error(err, "get commits by path")
@@ -152,7 +152,7 @@ func WikiPages(c *context.Context) {
 		return
 	}
 
-	branch := database.WikiBranch(wikiPath)
+	branch := db.WikiBranch(wikiPath)
 	commit, err := wikiRepo.BranchCommit(branch)
 	if err != nil {
 		c.Error(err, "get branch commit")