Re-attempt to delete temporary upload if the file is locked by another process (#12447)

Replace all calls to os.Remove/os.RemoveAll by retrying util.Remove/util.RemoveAll and remove circular dependencies from util.

Fix #12339

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: silverwind <me@silverwind.io>

6 files changed, 59 insertions, 8 deletions

diff --git a/routers/repo/http.go b/routers/repo/http.go
index 565ee5eff4..bc3b81f511 100644
--- a/routers/repo/http.go
+++ b/routers/repo/http.go
@@ -31,6 +31,7 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/util"
 	repo_service "code.gitea.io/gitea/services/repository"
 )
 
@@ -391,7 +392,7 @@ func dummyInfoRefs(ctx *context.Context) {
 		}
 
 		defer func() {
-			if err := os.RemoveAll(tmpDir); err != nil {
+			if err := util.RemoveAll(tmpDir); err != nil {
 				log.Error("RemoveAll: %v", err)
 			}
 		}()
diff --git a/routers/repo/lfs.go b/routers/repo/lfs.go
index d8f1ce62a9..8aff89dd6a 100644
--- a/routers/repo/lfs.go
+++ b/routers/repo/lfs.go
@@ -11,7 +11,6 @@ import (
 	gotemplate "html/template"
 	"io"
 	"io/ioutil"
-	"os"
 	"path"
 	"path/filepath"
 	"sort"
@@ -29,6 +28,7 @@ import (
 	"code.gitea.io/gitea/modules/lfs"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	gogit "github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/plumbing"
@@ -356,7 +356,7 @@ func LFSDelete(ctx *context.Context) {
 	// Please note a similar condition happens in models/repo.go DeleteRepository
 	if count == 0 {
 		oidPath := filepath.Join(oid[0:2], oid[2:4], oid[4:])
-		err = os.Remove(filepath.Join(setting.LFS.ContentPath, oidPath))
+		err = util.Remove(filepath.Join(setting.LFS.ContentPath, oidPath))
 		if err != nil {
 			ctx.ServerError("LFSDelete", err)
 			return
diff --git a/routers/repo/settings_test.go b/routers/repo/settings_test.go
index 2325c0255c..97c3fb0471 100644
--- a/routers/repo/settings_test.go
+++ b/routers/repo/settings_test.go
@@ -7,7 +7,6 @@ package repo
 import (
 	"io/ioutil"
 	"net/http"
-	"os"
 	"testing"
 
 	"code.gitea.io/gitea/models"
@@ -15,6 +14,7 @@ import (
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/test"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -31,7 +31,7 @@ func createSSHAuthorizedKeysTmpPath(t *testing.T) func() {
 
 	return func() {
 		setting.SSH.RootPath = oldPath
-		os.RemoveAll(tmpDir)
+		util.RemoveAll(tmpDir)
 	}
 }
 
diff --git a/routers/user/auth.go b/routers/user/auth.go
index a253e6cdbb..4e6ac9c87f 100644
--- a/routers/user/auth.go
+++ b/routers/user/auth.go
@@ -22,7 +22,7 @@ import (
 	"code.gitea.io/gitea/modules/recaptcha"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/timeutil"
-	"code.gitea.io/gitea/modules/util"
+	"code.gitea.io/gitea/routers/utils"
 	"code.gitea.io/gitea/services/externalaccount"
 	"code.gitea.io/gitea/services/mailer"
 
@@ -537,7 +537,7 @@ func handleSignInFull(ctx *context.Context, u *models.User, remember bool, obeyR
 		return setting.AppSubURL + "/"
 	}
 
-	if redirectTo := ctx.GetCookie("redirect_to"); len(redirectTo) > 0 && !util.IsExternalURL(redirectTo) {
+	if redirectTo := ctx.GetCookie("redirect_to"); len(redirectTo) > 0 && !utils.IsExternalURL(redirectTo) {
 		ctx.SetCookie("redirect_to", "", -1, setting.AppSubURL, "", setting.SessionConfig.Secure, true)
 		if obeyRedirect {
 			ctx.RedirectToFirst(redirectTo)
@@ -1540,7 +1540,7 @@ func MustChangePasswordPost(ctx *context.Context, cpt *captcha.Captcha, form aut
 
 	log.Trace("User updated password: %s", u.Name)
 
-	if redirectTo := ctx.GetCookie("redirect_to"); len(redirectTo) > 0 && !util.IsExternalURL(redirectTo) {
+	if redirectTo := ctx.GetCookie("redirect_to"); len(redirectTo) > 0 && !utils.IsExternalURL(redirectTo) {
 		ctx.SetCookie("redirect_to", "", -1, setting.AppSubURL)
 		ctx.RedirectToFirst(redirectTo)
 		return
diff --git a/routers/utils/utils.go b/routers/utils/utils.go
index 64b132ff3e..7c845f8763 100644
--- a/routers/utils/utils.go
+++ b/routers/utils/utils.go
@@ -6,7 +6,10 @@ package utils
 
 import (
 	"html"
+	"net/url"
 	"strings"
+
+	"code.gitea.io/gitea/modules/setting"
 )
 
 // RemoveUsernameParameterSuffix returns the username parameter without the (fullname) suffix - leaving just the username
@@ -46,3 +49,16 @@ func SanitizeFlashErrorString(x string) string {
 
 	return strings.Replace(html.EscapeString(x), "\n", "<br>", -1)
 }
+
+// IsExternalURL checks if rawURL points to an external URL like http://example.com
+func IsExternalURL(rawURL string) bool {
+	parsed, err := url.Parse(rawURL)
+	if err != nil {
+		return true
+	}
+	appURL, _ := url.Parse(setting.AppURL)
+	if len(parsed.Host) != 0 && strings.Replace(parsed.Host, "www.", "", 1) != strings.Replace(appURL.Host, "www.", "", 1) {
+		return true
+	}
+	return false
+}
diff --git a/routers/utils/utils_test.go b/routers/utils/utils_test.go
index d96e1d7d26..ec5e69862a 100644
--- a/routers/utils/utils_test.go
+++ b/routers/utils/utils_test.go
@@ -7,6 +7,7 @@ package utils
 import (
 	"testing"
 
+	"code.gitea.io/gitea/modules/setting"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -32,3 +33,36 @@ func TestIsValidSlackChannel(t *testing.T) {
 		assert.Equal(t, v.expected, IsValidSlackChannel(v.channelName))
 	}
 }
+
+func TestIsExternalURL(t *testing.T) {
+	setting.AppURL = "https://try.gitea.io"
+	type test struct {
+		Expected bool
+		RawURL   string
+	}
+	newTest := func(expected bool, rawURL string) test {
+		return test{Expected: expected, RawURL: rawURL}
+	}
+	for _, test := range []test{
+		newTest(false,
+			"https://try.gitea.io"),
+		newTest(true,
+			"https://example.com/"),
+		newTest(true,
+			"//example.com"),
+		newTest(true,
+			"http://example.com"),
+		newTest(false,
+			"a/"),
+		newTest(false,
+			"https://try.gitea.io/test?param=false"),
+		newTest(false,
+			"test?param=false"),
+		newTest(false,
+			"//try.gitea.io/test?param=false"),
+		newTest(false,
+			"/hey/hey/hey#3244"),
+	} {
+		assert.Equal(t, test.Expected, IsExternalURL(test.RawURL))
+	}
+}