From 3027794c44b737739eebe5dd67ba44716281b16a Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 17 Jul 2026 22:28:59 +0800 Subject: [PATCH] refactor: fix legacy problems in cmd/serv.go (#38505) 1. use var names "reqOwnerName, reqRepoName", because these values are from request and should not be really used for path construction 2. simplify enable-pprof logic, don't "log.Fatal" 3. don't make runServe command to guess the repo storage path, instead, let server return RepoStoragePath 4. don't process lfs verbs when the repo is a wiki 5. construct the request URI path correctly for the lfs transfer backend (moved to the caller) 6. don't call "owner, err := user_model.GetUserByName", the "owner rename redirection" has been done before 7. fix incorrect "repo.OwnerName = ownerName", the real owner might have been "redirected" 8. fix incorrect "inactive owner" check, it should be checked even if the repo is redirected Use general error functions to handle responses, error handling code is hugely simplified. --- cmd/serv.go | 61 ++--- models/repo/repo.go | 2 +- models/repo/wiki.go | 4 +- modules/lfstransfer/backend/backend.go | 5 +- modules/lfstransfer/main.go | 14 +- modules/pprof/pprof.go | 25 +- modules/private/serv.go | 2 + routers/private/hook_post_receive.go | 27 +- routers/private/serv.go | 255 +++++------------- services/context/private.go | 16 +- services/repository/repository.go | 3 + tests/integration/git_general_test.go | 2 +- .../git_helper_for_declarative_test.go | 1 + 13 files changed, 158 insertions(+), 259 deletions(-) diff --git a/cmd/serv.go b/cmd/serv.go index b39076f6a78..eddbbcb3ab9 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -201,35 +201,27 @@ func runServ(ctx context.Context, c *cli.Command) error { return fail(ctx, "Too few arguments", "Too few arguments in cmd: %s", cmd) } - repoPath := strings.TrimPrefix(sshCmdArgs[1], "/") - repoPathFields := strings.SplitN(repoPath, "/", 2) - if len(repoPathFields) != 2 { - return fail(ctx, "Invalid repository path", "Invalid repository path: %v", repoPath) + var reqOwnerName, reqRepoName string + { + var ok bool + reqRepoPath := strings.TrimPrefix(sshCmdArgs[1], "/") + reqOwnerName, reqRepoName, ok = strings.Cut(reqRepoPath, "/") + if !ok { + return fail(ctx, "Invalid repository path", "Invalid repository path: %v", reqRepoPath) + } + reqRepoName = strings.TrimSuffix(reqRepoName, ".git") // "the-repo-name" or "the-repo-name.wiki" } - username := repoPathFields[0] - reponame := strings.TrimSuffix(repoPathFields[1], ".git") // “the-repo-name" or "the-repo-name.wiki" - - if !repo_model.IsValidSSHAccessRepoName(reponame) { - return fail(ctx, "Invalid repo name", "Invalid repo name: %s", reponame) + if !repo_model.IsValidSSHAccessRepoName(reqRepoName) { + return fail(ctx, "Invalid repo name", "Invalid repo name: %s", reqRepoName) } if c.Bool("enable-pprof") { - if err := os.MkdirAll(setting.PprofDataPath, os.ModePerm); err != nil { - return fail(ctx, "Error while trying to create PPROF_DATA_PATH", "Error while trying to create PPROF_DATA_PATH: %v", err) - } - - stopCPUProfiler, err := pprof.DumpCPUProfileForUsername(setting.PprofDataPath, username) + stopProfiler, err := pprof.DumpPprofForUsername(setting.PprofDataPath, reqOwnerName) if err != nil { - return fail(ctx, "Unable to start CPU profiler", "Unable to start CPU profile: %v", err) + return fail(ctx, "Unable to start pprof profiler", "Unable to start pprof profile: %v", err) } - defer func() { - stopCPUProfiler() - err := pprof.DumpMemProfileForUsername(setting.PprofDataPath, username) - if err != nil { - _ = fail(ctx, "Unable to dump Mem profile", "Unable to dump Mem Profile: %v", err) - } - }() + defer stopProfiler() } verb, lfsVerb := sshCmdArgs[0], "" @@ -254,30 +246,29 @@ func runServ(ctx context.Context, c *cli.Command) error { return fail(ctx, "Unknown git command", "Unknown git command %s %s", verb, lfsVerb) } - results, extra := private.ServCommand(ctx, keyID, username, reponame, requestedMode, verb, lfsVerb) + results, extra := private.ServCommand(ctx, keyID, reqOwnerName, reqRepoName, requestedMode, verb, lfsVerb) if extra.HasError() { return fail(ctx, extra.UserMsg, "ServCommand failed: %s", extra.Error) } - // because the original repoPath maybe redirected, we need to use the returned actual repository information - if results.IsWiki { - repoPath = repo_model.RelativeWikiPath(results.OwnerName, results.RepoName) - } else { - repoPath = repo_model.RelativePath(results.OwnerName, results.RepoName) - } - // LFS SSH protocol if verb == git.CmdVerbLfsTransfer { + if results.IsWiki { + return fail(ctx, "LFS Transfer is not supported for wikis", "") + } token, err := lfs.GetLFSAuthTokenWithBearer(lfs.AuthTokenOptions{Op: lfsVerb, UserID: results.UserID, RepoID: results.RepoID}) if err != nil { return err } - return lfstransfer.Main(ctx, repoPath, lfsVerb, token) + return lfstransfer.Main(ctx, results.OwnerName, results.RepoName, lfsVerb, token) } // LFS token authentication if verb == git.CmdVerbLfsAuthenticate { - url := fmt.Sprintf("%s%s/%s.git/info/lfs", setting.AppURL, url.PathEscape(results.OwnerName), url.PathEscape(results.RepoName)) + if results.IsWiki { + return fail(ctx, "LFS Authenticate is not supported for wikis", "") + } + lfsTokenHref := fmt.Sprintf("%s%s/%s.git/info/lfs", setting.AppURL, url.PathEscape(results.OwnerName), url.PathEscape(results.RepoName)) token, err := lfs.GetLFSAuthTokenWithBearer(lfs.AuthTokenOptions{Op: lfsVerb, UserID: results.UserID, RepoID: results.RepoID}) if err != nil { @@ -286,7 +277,7 @@ func runServ(ctx context.Context, c *cli.Command) error { tokenAuthentication := &git_model.LFSTokenResponse{ Header: make(map[string]string), - Href: url, + Href: lfsTokenHref, } tokenAuthentication.Header["Authorization"] = token @@ -307,12 +298,12 @@ func runServ(ctx context.Context, c *cli.Command) error { verbFields := strings.SplitN(verb, "-", 2) if len(verbFields) == 2 { // use git binary with the sub-command part: "C:\...\bin\git.exe", "upload-pack", ... - command = exec.CommandContext(ctx, gitcmd.GitExecutable, verbFields[1], repoPath) + command = exec.CommandContext(ctx, gitcmd.GitExecutable, verbFields[1], results.RepoStoragePath) } } if command == nil { // by default, use the verb (it has been checked above by allowedCommands) - command = exec.CommandContext(ctx, gitBinVerb, repoPath) + command = exec.CommandContext(ctx, gitBinVerb, results.RepoStoragePath) } process.SetSysProcAttribute(command) diff --git a/models/repo/repo.go b/models/repo/repo.go index 6ad933d3c9a..b783b06993b 100644 --- a/models/repo/repo.go +++ b/models/repo/repo.go @@ -230,7 +230,7 @@ func RelativePath(ownerName, repoName string) string { return strings.ToLower(ownerName) + "/" + strings.ToLower(repoName) + ".git" } -// RelativePath should be an unix style path like username/reponame.git +// RelativePath should be a unix style path like "owner-name/repo-name.git" func (repo *Repository) RelativePath() string { return RelativePath(repo.OwnerName, repo.Name) } diff --git a/models/repo/wiki.go b/models/repo/wiki.go index cbc1229c9f3..dd99a0422c3 100644 --- a/models/repo/wiki.go +++ b/models/repo/wiki.go @@ -79,8 +79,8 @@ func RelativeWikiPath(ownerName, repoName string) string { return strings.ToLower(ownerName) + "/" + strings.ToLower(repoName) + ".wiki.git" } -// WikiStorageRepo returns the storage repo for the wiki -// The wiki repository should have the same object format as the code repository +// WikiStorageRepo returns the storage repo for the wiki like "owner-name/repo-name.wiki.git" +// The wiki repository should have the same object format as the code repository. TODO: REALLY? Why? func (repo *Repository) WikiStorageRepo() StorageRepo { return StorageRepo(RelativeWikiPath(repo.OwnerName, repo.Name)) } diff --git a/modules/lfstransfer/backend/backend.go b/modules/lfstransfer/backend/backend.go index 42a01bf492f..6dde3237060 100644 --- a/modules/lfstransfer/backend/backend.go +++ b/modules/lfstransfer/backend/backend.go @@ -40,13 +40,12 @@ type GiteaBackend struct { logger transfer.Logger } -func New(ctx context.Context, repo, op, token string, logger transfer.Logger) (transfer.Backend, error) { - // runServ guarantees repo will be in form [owner]/[name].git +func New(ctx context.Context, reqPath, op, token string, logger transfer.Logger) (transfer.Backend, error) { server, err := url.Parse(setting.LocalURL) if err != nil { return nil, err } - server = server.JoinPath("api/internal/repo", repo, "info/lfs") + server = server.JoinPath(reqPath) return &GiteaBackend{ctx: ctx, server: server, op: op, authToken: token, internalAuth: "Bearer " + setting.InternalToken, logger: logger}, nil } diff --git a/modules/lfstransfer/main.go b/modules/lfstransfer/main.go index e5c8a122a4a..503936f245b 100644 --- a/modules/lfstransfer/main.go +++ b/modules/lfstransfer/main.go @@ -6,6 +6,7 @@ package lfstransfer import ( "context" "fmt" + "net/url" "os" "gitea.dev/modules/lfstransfer/backend" @@ -13,23 +14,24 @@ import ( "github.com/charmbracelet/git-lfs-transfer/transfer" ) -func Main(ctx context.Context, repo, verb, token string) error { +func Main(ctx context.Context, ownerName, repoName, verb, token string) error { logger := newLogger() - pktline := transfer.NewPktline(os.Stdin, os.Stdout, logger) - giteaBackend, err := backend.New(ctx, repo, verb, token, logger) + backendReqPath := fmt.Sprintf("api/internal/repo/%s/%s.git/info/lfs", url.PathEscape(ownerName), url.PathEscape(repoName)) + giteaBackend, err := backend.New(ctx, backendReqPath, verb, token, logger) if err != nil { return err } + pktLine := transfer.NewPktline(os.Stdin, os.Stdout, logger) for _, cap := range backend.Capabilities { - if err := pktline.WritePacketText(cap); err != nil { + if err := pktLine.WritePacketText(cap); err != nil { logger.Log("error sending capability due to error:", err) } } - if err := pktline.WriteFlush(); err != nil { + if err := pktLine.WriteFlush(); err != nil { logger.Log("error flushing capabilities:", err) } - p := transfer.NewProcessor(pktline, giteaBackend, logger) + p := transfer.NewProcessor(pktLine, giteaBackend, logger) defer logger.Log("done processing commands") switch verb { case "upload": diff --git a/modules/pprof/pprof.go b/modules/pprof/pprof.go index 138d33305f6..c00bfb8f169 100644 --- a/modules/pprof/pprof.go +++ b/modules/pprof/pprof.go @@ -6,15 +6,15 @@ package pprof import ( "fmt" "os" + "path/filepath" "runtime" "runtime/pprof" "gitea.dev/modules/log" ) -// DumpMemProfileForUsername dumps a memory profile at pprofDataPath as memprofile__ -func DumpMemProfileForUsername(pprofDataPath, username string) error { - f, err := os.CreateTemp(pprofDataPath, fmt.Sprintf("memprofile_%s_", username)) +func dumpMemProfileForUsername(pprofDataPath, subName string) error { + f, err := os.CreateTemp(pprofDataPath, fmt.Sprintf("pprof_mem_%s_", filepath.Clean(subName))) if err != nil { return err } @@ -23,23 +23,30 @@ func DumpMemProfileForUsername(pprofDataPath, username string) error { return pprof.WriteHeapProfile(f) } -// DumpCPUProfileForUsername dumps a CPU profile at pprofDataPath as cpuprofile__ -// the stop function it returns stops, writes and closes the CPU profile file -func DumpCPUProfileForUsername(pprofDataPath, username string) (func(), error) { - f, err := os.CreateTemp(pprofDataPath, fmt.Sprintf("cpuprofile_%s_", username)) +func DumpPprofForUsername(pprofDataPath, subName string) (func(), error) { + if err := os.MkdirAll(pprofDataPath, os.ModePerm); err != nil { + return nil, fmt.Errorf(`os.MkdirAll(pprofDataPath) failed: %v`, err) + } + + f, err := os.CreateTemp(pprofDataPath, fmt.Sprintf("pprof_cpu_%s_", filepath.Clean(subName))) if err != nil { return nil, err } err = pprof.StartCPUProfile(f) if err != nil { - log.Fatal("StartCPUProfile: %v", err) + _ = f.Close() + return nil, fmt.Errorf("StartCPUProfile: %w", err) } return func() { pprof.StopCPUProfile() err = f.Close() if err != nil { - log.Fatal("StopCPUProfile Close: %v", err) + log.Error("StopCPUProfile Close: %v", err) + } + err = dumpMemProfileForUsername(pprofDataPath, subName) + if err != nil { + log.Error("DumpMemProfile: %v", err) } }, nil } diff --git a/modules/private/serv.go b/modules/private/serv.go index cde255f36a4..20768d10196 100644 --- a/modules/private/serv.go +++ b/modules/private/serv.go @@ -43,6 +43,8 @@ type ServCommandResults struct { OwnerName string RepoName string RepoID int64 + + RepoStoragePath string } // ServCommand preps for a serv call diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 745c84d3a8a..a809f2813c2 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -63,7 +63,7 @@ func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts } if update.IsDelRef() { if err := git_model.MarkBranchAsDeleted(ctx, repo.ID, update.RefFullName.BranchName(), update.PusherID); err != nil { - ctx.PrivateError(http.StatusInternalServerError, err, fmt.Sprintf("failed to mark branch %s as deleted", update.RefFullName)) + ctx.PrivateInternalErrorf("failed to mark branch %s as deleted: %v", update.RefFullName, err) return false } } else { @@ -79,7 +79,7 @@ func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts gitRepo, err := gitrepo.RepositoryFromRequestContextOrOpen(ctx, repo) if err != nil { - ctx.PrivateError(http.StatusInternalServerError, err, "failed to open repository") + ctx.PrivateInternalErrorf("failed to open repository: %v", err) return false } @@ -91,7 +91,7 @@ func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts } if err = repo_service.SyncBranchesToDB(ctx, repo.ID, opts.UserID, gitRepo, branchNames, commitIDs); err != nil { - ctx.PrivateError(http.StatusInternalServerError, err, "failed to sync branch to DB") + ctx.PrivateInternalErrorf("failed to sync branch to DB: %v", err) return false } return true @@ -133,7 +133,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { // push async updates if err := repo_service.PushUpdates(updates...); err != nil { - ctx.PrivateError(http.StatusInternalServerError, err, "failed to push updates") + ctx.PrivateInternalErrorf("failed to push updates: %v", err) return } @@ -147,16 +147,16 @@ func hookPostReceiveUpdateRepoByOptions(ctx *gitea_context.PrivateContext, opts if isPrivate.Has() || isTemplate.Has() { pusher, err := loadContextCacheUser(ctx, opts.UserID) if err != nil { - ctx.PrivateError(http.StatusInternalServerError, err, "failed to load pusher user") + ctx.PrivateInternalErrorf("failed to load pusher user: %v", err) return false } perm, err := access_model.GetDoerRepoPermission(ctx, repo, pusher) if err != nil { - ctx.PrivateError(http.StatusInternalServerError, err, "failed to load doer repo permission") + ctx.PrivateInternalErrorf("failed to load doer repo permission: %v", err) return false } if !perm.IsOwner() && !perm.IsAdmin() { - ctx.PrivateError(http.StatusNotFound, nil, "permission denied") + ctx.PrivateUserErrorf(http.StatusNotFound, "permission denied") return false } @@ -191,7 +191,7 @@ func hookPostReceiveRespondWithTrailer(ctx *gitea_context.PrivateContext, opts * baseRepo := repo if repo.IsFork { if err := repo.GetBaseRepo(ctx); err != nil { - ctx.PrivateError(http.StatusInternalServerError, err, "failed to load base repo") + ctx.PrivateInternalErrorf("failed to load base repo: %v", err) return } if repo.BaseRepo.AllowsPulls(ctx) { @@ -222,7 +222,7 @@ func hookPostReceiveRespondWithTrailer(ctx *gitea_context.PrivateContext, opts * pr, err := issues_model.GetUnmergedPullRequest(ctx, repo.ID, baseRepo.ID, branch, baseRepo.DefaultBranch, issues_model.PullRequestFlowGithub) if err != nil && !errors.Is(err, util.ErrNotExist) { - ctx.PrivateError(http.StatusInternalServerError, err, "failed to get active PR for branch "+branch) + ctx.PrivateInternalErrorf("failed to get active PR for branch %s: %v", branch, err) return } if pr == nil { @@ -252,20 +252,19 @@ func loadContextCacheUser(ctx context.Context, id int64) (*user_model.User, erro // hookPostReceiveHandlePullRequestMerging handle pull request merging, a pull request action should push at least 1 commit func hookPostReceiveHandlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.HookOptions, updates []*repo_module.PushUpdateOptions) bool { if len(updates) == 0 { - err := fmt.Errorf("Pushing a merged PR (pr:%d) no commits pushed ", opts.PullRequestID) - ctx.PrivateError(http.StatusInternalServerError, err, "no push update") + ctx.PrivateInternalErrorf("Pushing a merged PR (pr:%d) no commits pushed ", opts.PullRequestID) return false } pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID) if err != nil { - ctx.PrivateError(http.StatusInternalServerError, err, "failed to load pull request") + ctx.PrivateInternalErrorf("failed to get pull request %d: %v", opts.PullRequestID, err) return false } pusher, err := loadContextCacheUser(ctx, opts.UserID) if err != nil { - ctx.PrivateError(http.StatusInternalServerError, err, "failed to load pusher user") + ctx.PrivateInternalErrorf("failed to load pusher user %d: %v", opts.UserID, err) return false } @@ -273,7 +272,7 @@ func hookPostReceiveHandlePullRequestMerging(ctx *gitea_context.PrivateContext, // here to keep it as before, that maybe PullRequestStatusMergeable _, err = pull_service.SetMerged(ctx, pr, updates[len(updates)-1].NewCommitID, timeutil.TimeStampNow(), pusher, pr.Status) if err != nil { - ctx.PrivateError(http.StatusInternalServerError, err, "failed to set pr to merged") + ctx.PrivateInternalErrorf("failed to set pr %d to merged: %v", pr.ID, err) return false } return true diff --git a/routers/private/serv.go b/routers/private/serv.go index 7c7d0db5a6c..02135ea6e47 100644 --- a/routers/private/serv.go +++ b/routers/private/serv.go @@ -4,7 +4,6 @@ package private import ( - "fmt" "net/http" "strings" @@ -18,6 +17,7 @@ import ( "gitea.dev/modules/log" "gitea.dev/modules/private" "gitea.dev/modules/setting" + "gitea.dev/modules/util" "gitea.dev/services/context" repo_service "gitea.dev/services/repository" wiki_service "gitea.dev/services/wiki" @@ -27,24 +27,18 @@ import ( func ServNoCommand(ctx *context.PrivateContext) { keyID := ctx.PathParamInt64("keyid") if keyID <= 0 { - ctx.JSON(http.StatusBadRequest, private.Response{ - UserMsg: fmt.Sprintf("Bad key id: %d", keyID), - }) + ctx.PrivateUserErrorf(http.StatusBadRequest, "Bad key id: %d", keyID) + return } results := private.KeyAndOwner{} key, err := asymkey_model.GetPublicKeyByID(ctx, keyID) if err != nil { if asymkey_model.IsErrKeyNotExist(err) { - ctx.JSON(http.StatusUnauthorized, private.Response{ - UserMsg: fmt.Sprintf("Cannot find key: %d", keyID), - }) + ctx.PrivateUserErrorf(http.StatusUnauthorized, "Cannot find key: %d", keyID) return } - log.Error("Unable to get public key: %d Error: %v", keyID, err) - ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: err.Error(), - }) + ctx.PrivateInternalErrorf("Unable to get public key: %d Error: %v", keyID, err) return } results.Key = key @@ -53,21 +47,14 @@ func ServNoCommand(ctx *context.PrivateContext) { user, err := user_model.GetUserByID(ctx, key.OwnerID) if err != nil { if user_model.IsErrUserNotExist(err) { - ctx.JSON(http.StatusUnauthorized, private.Response{ - UserMsg: fmt.Sprintf("Cannot find owner with id: %d for key: %d", key.OwnerID, keyID), - }) + ctx.PrivateUserErrorf(http.StatusUnauthorized, "Cannot find owner with id: %d for key: %d", key.OwnerID, keyID) return } - log.Error("Unable to get owner with id: %d for public key: %d Error: %v", key.OwnerID, keyID, err) - ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: err.Error(), - }) + ctx.PrivateInternalErrorf("Unable to get owner with id: %d for public key: %d Error: %v", key.OwnerID, keyID, err) return } if !user.IsActive || user.ProhibitLogin { - ctx.JSON(http.StatusForbidden, private.Response{ - UserMsg: "Your account is disabled.", - }) + ctx.PrivateUserErrorf(http.StatusForbidden, "Your account is disabled.") return } results.Owner = user @@ -78,87 +65,56 @@ func ServNoCommand(ctx *context.PrivateContext) { // ServCommand returns information about the provided keyid func ServCommand(ctx *context.PrivateContext) { keyID := ctx.PathParamInt64("keyid") - ownerName := ctx.PathParam("owner") - repoName := ctx.PathParam("repo") + reqOwnerName := ctx.PathParam("owner") + reqRepoName := ctx.PathParam("repo") mode := perm.AccessMode(ctx.FormInt("mode")) verb := ctx.FormString("verb") // Set the basic parts of the results to return results := private.ServCommandResults{ - RepoName: repoName, - OwnerName: ownerName, + OwnerName: reqOwnerName, // it might be changed if there is "renamed user redirection" + RepoName: reqRepoName, // it might be changed if there is "renamed repo redirection", or the repo is a wiki KeyID: keyID, } + repoLogName := reqOwnerName + "/" + reqRepoName - // Now because we're not translating things properly let's just default some English strings here - modeString := "read" - if mode > perm.AccessModeRead { - modeString = "write to" - } - - // The default unit we're trying to look at is code - unitType := unit.TypeCode - - // Unless we're a wiki... - if strings.HasSuffix(repoName, ".wiki") { - // in which case we need to look at the wiki - unitType = unit.TypeWiki - // And we'd better munge the reponame and tell downstream we're looking at a wiki + if reqWikiRepoName, ok := strings.CutSuffix(reqRepoName, ".wiki"); ok { + // in which case we need to look at the wiki, trim the ".wiki" suffix, only use the main repo name results.IsWiki = true - results.RepoName = repoName[:len(repoName)-5] + results.RepoName = reqWikiRepoName } + unitType := util.Iif(results.IsWiki, unit.TypeWiki, unit.TypeCode) + modeString := mode.ToString() owner, err := user_model.GetUserByName(ctx, results.OwnerName) if err != nil { if !user_model.IsErrUserNotExist(err) { - log.Error("Unable to get repository owner: %s/%s Error: %v", results.OwnerName, results.RepoName, err) - ctx.JSON(http.StatusForbidden, private.Response{ - UserMsg: fmt.Sprintf("Unable to get repository owner: %s/%s %v", results.OwnerName, results.RepoName, err), - }) + ctx.PrivateInternalErrorf("Unable to get repository owner for %s, error: %v", repoLogName, err) return } // Check if there is a user redirect for the requested owner redirectedUserID, err := user_model.LookupUserRedirect(ctx, results.OwnerName) if err != nil { - // User is fetching/cloning a non-existent repository - log.Warn("Failed authentication attempt (cannot find repository: %s/%s) from %s", results.OwnerName, results.RepoName, ctx.RemoteAddr()) - ctx.JSON(http.StatusNotFound, private.Response{ - UserMsg: fmt.Sprintf("Cannot find repository: %s/%s", results.OwnerName, results.RepoName), - }) + ctx.PrivateUserErrorf(http.StatusNotFound, "Cannot find repository %s", repoLogName) return } - redirectUser, err := user_model.GetUserByID(ctx, redirectedUserID) if err != nil { - // User is fetching/cloning a non-existent repository - log.Warn("Failed authentication attempt (cannot find repository: %s/%s) from %s", results.OwnerName, results.RepoName, ctx.RemoteAddr()) - ctx.JSON(http.StatusNotFound, private.Response{ - UserMsg: fmt.Sprintf("Cannot find repository: %s/%s", results.OwnerName, results.RepoName), - }) + ctx.PrivateUserErrorf(http.StatusNotFound, "Cannot find repository: %s", repoLogName) return } - log.Info("User %s has been redirected to %s", results.OwnerName, redirectUser.Name) + log.Debug("User %s has been redirected to %s", results.OwnerName, redirectUser.Name) results.OwnerName = redirectUser.Name owner = redirectUser } - if !owner.IsOrganization() && !owner.IsActive { - ctx.JSON(http.StatusForbidden, private.Response{ - UserMsg: "Repository cannot be accessed, you could retry it later", - }) - return - } // Now get the Repository and set the results section - repoExist := true repo, err := repo_model.GetRepositoryByName(ctx, owner.ID, results.RepoName) if err != nil { if !repo_model.IsErrRepoNotExist(err) { - log.Error("Unable to get repository: %s/%s Error: %v", results.OwnerName, results.RepoName, err) - ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: fmt.Sprintf("Unable to get repository: %s/%s %v", results.OwnerName, results.RepoName, err), - }) + ctx.PrivateInternalErrorf("Unable to get repository %s, error: %v", repoLogName, err) return } @@ -166,53 +122,53 @@ func ServCommand(ctx *context.PrivateContext) { if err == nil { redirectedRepo, err := repo_model.GetRepositoryByID(ctx, redirectedRepoID) if err == nil { - log.Info("Repository %s/%s has been redirected to %s/%s", results.OwnerName, results.RepoName, redirectedRepo.OwnerName, redirectedRepo.Name) + log.Info("Repository %s has been redirected to %s/%s", repoLogName, redirectedRepo.OwnerName, redirectedRepo.Name) + repo = redirectedRepo + if err = repo.LoadOwner(ctx); err != nil { + ctx.PrivateInternalErrorf("Unable to repository owner %d", repo.OwnerID) + return + } + owner = repo.Owner results.RepoName = redirectedRepo.Name results.OwnerName = redirectedRepo.OwnerName - repo = redirectedRepo - owner.ID = redirectedRepo.OwnerID + repoLogName = results.OwnerName + "/" + results.RepoName + util.Iif(results.IsWiki, ".wiki", "") } else { - log.Warn("Repo %s/%s has a redirect to repo with ID %d, but no repo with this ID could be found. Trying without redirect...", results.OwnerName, results.RepoName, redirectedRepoID) + log.Warn("Repo %s has a redirect to repo with ID %d, but no repo with this ID could be found. Trying without redirect...", repoLogName, redirectedRepoID) } } if repo == nil { - repoExist = false if mode == perm.AccessModeRead { // User is fetching/cloning a non-existent repository - log.Warn("Failed authentication attempt (cannot find repository: %s/%s) from %s", results.OwnerName, results.RepoName, ctx.RemoteAddr()) - ctx.JSON(http.StatusNotFound, private.Response{ - UserMsg: fmt.Sprintf("Cannot find repository: %s/%s", results.OwnerName, results.RepoName), - }) + ctx.PrivateUserErrorf(http.StatusNotFound, "Cannot find repository %s", repoLogName) return } } } - if repoExist { + if !owner.IsOrganization() && !owner.IsActive { + ctx.PrivateUserErrorf(http.StatusForbidden, "Repository cannot be accessed, the owner is inactive.") + return + } + + if repo != nil { repo.Owner = owner - repo.OwnerName = ownerName + repo.OwnerName = owner.Name results.RepoID = repo.ID if repo.IsBeingCreated() { - ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: "Repository is being created, you could retry after it finished", - }) + ctx.PrivateUserErrorf(http.StatusForbidden, "Repository is being created, you could retry after it finished") return } if repo.IsBroken() { - ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: "Repository is in a broken state", - }) + ctx.PrivateUserErrorf(http.StatusForbidden, "Repository is in a broken state") return } // We can shortcut at this point if the repo is a mirror if mode > perm.AccessModeRead && repo.IsMirror { - ctx.JSON(http.StatusForbidden, private.Response{ - UserMsg: fmt.Sprintf("Mirror Repository %s/%s is read-only", results.OwnerName, results.RepoName), - }) + ctx.PrivateUserErrorf(http.StatusForbidden, "Mirror Repository %s is read-only", repoLogName) return } } @@ -221,48 +177,33 @@ func ServCommand(ctx *context.PrivateContext) { key, err := asymkey_model.GetPublicKeyByID(ctx, keyID) if err != nil { if asymkey_model.IsErrKeyNotExist(err) { - ctx.JSON(http.StatusNotFound, private.Response{ - UserMsg: fmt.Sprintf("Cannot find key: %d", keyID), - }) + ctx.PrivateUserErrorf(http.StatusNotFound, "Cannot find key: %d", keyID) return } - log.Error("Unable to get public key: %d Error: %v", keyID, err) - ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: fmt.Sprintf("Unable to get key: %d Error: %v", keyID, err), - }) + ctx.PrivateInternalErrorf("Unable to get key: %d, error: %v", keyID, err) return } results.KeyName = key.Name results.KeyID = key.ID results.UserID = key.OwnerID - // If repo doesn't exist, deploy key doesn't make sense - if !repoExist && key.Type == asymkey_model.KeyTypeDeploy { - ctx.JSON(http.StatusNotFound, private.Response{ - UserMsg: fmt.Sprintf("Cannot find repository %s/%s", results.OwnerName, results.RepoName), - }) - return - } - // Deploy Keys have ownerID set to 0 therefore we can't use the owner // So now we need to check if the key is a deploy key // We'll keep hold of the deploy key here for permissions checking var deployKey *asymkey_model.DeployKey var user *user_model.User if key.Type == asymkey_model.KeyTypeDeploy { - var err error + if repo == nil { + ctx.PrivateUserErrorf(http.StatusNotFound, "Cannot find repository %s", repoLogName) + return + } deployKey, err = asymkey_model.GetDeployKeyByRepo(ctx, key.ID, repo.ID) if err != nil { if asymkey_model.IsErrDeployKeyNotExist(err) { - ctx.JSON(http.StatusNotFound, private.Response{ - UserMsg: fmt.Sprintf("Public (Deploy) Key: %d:%s is not authorized to %s %s/%s.", key.ID, key.Name, modeString, results.OwnerName, results.RepoName), - }) + ctx.PrivateUserErrorf(http.StatusNotFound, "Deploy key %d:%s has no %q permission for %s.", key.ID, key.Name, modeString, repoLogName) return } - log.Error("Unable to get deploy for public (deploy) key: %d in %-v Error: %v", key.ID, repo, err) - ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: fmt.Sprintf("Unable to get Deploy Key for Public Key: %d:%s in %s/%s.", key.ID, key.Name, results.OwnerName, results.RepoName), - }) + ctx.PrivateInternalErrorf("Unable to get deploy for public (deploy) key %d for %s, error: %v", key.ID, repoLogName, err) return } results.DeployKeyID = deployKey.ID @@ -278,26 +219,18 @@ func ServCommand(ctx *context.PrivateContext) { } } else { // Get the user represented by the Key - var err error user, err = user_model.GetUserByID(ctx, key.OwnerID) if err != nil { if user_model.IsErrUserNotExist(err) { - ctx.JSON(http.StatusUnauthorized, private.Response{ - UserMsg: fmt.Sprintf("Public Key: %d:%s owner %d does not exist.", key.ID, key.Name, key.OwnerID), - }) + ctx.PrivateUserErrorf(http.StatusUnauthorized, "Public key %d:%s owner %d does not exist.", key.ID, key.Name, key.OwnerID) return } - log.Error("Unable to get owner: %d for public key: %d:%s Error: %v", key.OwnerID, key.ID, key.Name, err) - ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: fmt.Sprintf("Unable to get Owner: %d for Deploy Key: %d:%s in %s/%s.", key.OwnerID, key.ID, key.Name, ownerName, repoName), - }) + ctx.PrivateInternalErrorf("Unable to get key owner %d for public key %d:%s, error: %v", key.OwnerID, key.ID, key.Name, err) return } if !user.IsActive || user.ProhibitLogin { - ctx.JSON(http.StatusForbidden, private.Response{ - UserMsg: "Your account is disabled.", - }) + ctx.PrivateUserErrorf(http.StatusForbidden, "Your account is disabled.") return } @@ -308,25 +241,21 @@ func ServCommand(ctx *context.PrivateContext) { } // Don't allow pushing if the repo is archived - if repoExist && mode > perm.AccessModeRead && repo.IsArchived { - ctx.JSON(http.StatusUnauthorized, private.Response{ - UserMsg: fmt.Sprintf("Repo: %s/%s is archived.", results.OwnerName, results.RepoName), - }) + if repo != nil && mode > perm.AccessModeRead && repo.IsArchived { + ctx.PrivateUserErrorf(http.StatusUnauthorized, "Repo %s is archived.", repoLogName) return } // Permissions checking: - if repoExist && + if repo != nil && (mode > perm.AccessModeRead || repo.IsPrivate || owner.Visibility.IsPrivate() || - (user != nil && user.IsRestricted) || // user will be nil if the key is a deploykey + (user != nil && user.IsRestricted) || // user will be nil if the key is a deploy key setting.Service.RequireSignInViewStrict) { if key.Type == asymkey_model.KeyTypeDeploy { - if deployKey.Mode < mode { - ctx.JSON(http.StatusUnauthorized, private.Response{ - UserMsg: fmt.Sprintf("Deploy Key: %d:%s is not authorized to %s %s/%s.", key.ID, key.Name, modeString, results.OwnerName, results.RepoName), - }) + if deployKey == nil || deployKey.Mode < mode { + ctx.PrivateUserErrorf(http.StatusUnauthorized, "Deploy key %d:%s has no %q permission for %s.", key.ID, key.Name, modeString, repoLogName) return } } else { @@ -338,56 +267,34 @@ func ServCommand(ctx *context.PrivateContext) { mode = perm.AccessModeRead } - perm, err := access_model.GetDoerRepoPermission(ctx, repo, user) + userPerm, err := access_model.GetDoerRepoPermission(ctx, repo, user) if err != nil { - log.Error("Unable to get permissions for %-v with key %d in %-v Error: %v", user, key.ID, repo, err) - ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: fmt.Sprintf("Unable to get permissions for user %d:%s with key %d in %s/%s Error: %v", user.ID, user.Name, key.ID, results.OwnerName, results.RepoName, err), - }) + ctx.PrivateInternalErrorf("Unable to get permissions for %-v with key %d in %-v, error: %v", user, key.ID, repo, err) return } - userMode := perm.UnitAccessMode(unitType) - + userMode := userPerm.UnitAccessMode(unitType) if userMode < mode { - log.Warn("Failed authentication attempt for %s with key %s (not authorized to %s %s/%s) from %s", user.Name, key.Name, modeString, ownerName, repoName, ctx.RemoteAddr()) - ctx.JSON(http.StatusUnauthorized, private.Response{ - UserMsg: fmt.Sprintf("User: %d:%s with Key: %d:%s is not authorized to %s %s/%s.", user.ID, user.Name, key.ID, key.Name, modeString, ownerName, repoName), - }) + ctx.PrivateUserErrorf(http.StatusUnauthorized, "User %d with key %d:%s has no %q permission for %s", key.OwnerID, key.ID, key.Name, modeString, repoLogName) return } } } // We already know we aren't using a deploy key - if !repoExist { - owner, err := user_model.GetUserByName(ctx, ownerName) - if err != nil { - ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: fmt.Sprintf("Unable to get owner: %s %v", results.OwnerName, err), - }) - return - } - + if repo == nil { if owner.IsOrganization() && !setting.Repository.EnablePushCreateOrg { - ctx.JSON(http.StatusForbidden, private.Response{ - UserMsg: "Push to create is not enabled for organizations.", - }) + ctx.PrivateUserErrorf(http.StatusForbidden, "Push to create is not enabled for organizations.") return } if !owner.IsOrganization() && !setting.Repository.EnablePushCreateUser { - ctx.JSON(http.StatusForbidden, private.Response{ - UserMsg: "Push to create is not enabled for users.", - }) + ctx.PrivateUserErrorf(http.StatusForbidden, "Push to create is not enabled for users.") return } repo, err = repo_service.PushCreateRepo(ctx, user, owner, results.RepoName) if err != nil { - log.Error("pushCreateRepo: %v", err) - ctx.JSON(http.StatusNotFound, private.Response{ - UserMsg: fmt.Sprintf("Cannot find repository: %s/%s", results.OwnerName, results.RepoName), - }) + ctx.PrivateInternalErrorf("pushCreateRepo: %v", err) return } results.RepoID = repo.ID @@ -397,38 +304,22 @@ func ServCommand(ctx *context.PrivateContext) { // Ensure the wiki is enabled before we allow access to it if _, err := repo.GetUnit(ctx, unit.TypeWiki); err != nil { if repo_model.IsErrUnitTypeNotExist(err) { - ctx.JSON(http.StatusForbidden, private.Response{ - UserMsg: "repository wiki is disabled", - }) + ctx.PrivateUserErrorf(http.StatusForbidden, "repository wiki is disabled") return } - log.Error("Failed to get the wiki unit in %-v Error: %v", repo, err) - ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: fmt.Sprintf("Failed to get the wiki unit in %s/%s Error: %v", ownerName, repoName, err), - }) + ctx.PrivateInternalErrorf("Failed to get the wiki unit in %-v, error: %v", repo, err) return } // Finally if we're trying to touch the wiki we should init it if err = wiki_service.InitWiki(ctx, repo); err != nil { - log.Error("Failed to initialize the wiki in %-v Error: %v", repo, err) - ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: fmt.Sprintf("Failed to initialize the wiki in %s/%s Error: %v", ownerName, repoName, err), - }) + ctx.PrivateInternalErrorf("Failed to initialize the wiki in %-v, error: %v", repo, err) return } } - log.Debug("Serv Results:\nIsWiki: %t\nDeployKeyID: %d\nKeyID: %d\tKeyName: %s\nUserName: %s\nUserID: %d\nOwnerName: %s\nRepoName: %s\nRepoID: %d", - results.IsWiki, - results.DeployKeyID, - results.KeyID, - results.KeyName, - results.UserName, - results.UserID, - results.OwnerName, - results.RepoName, - results.RepoID) + results.RepoStoragePath = util.Iif(results.IsWiki, repo_model.RelativeWikiPath(repo.OwnerName, repo.Name), repo.RelativePath()) + log.Debug("Serv Results: %+v", results) ctx.JSON(http.StatusOK, results) // We will update the keys in a different call. } diff --git a/services/context/private.go b/services/context/private.go index e687821b07f..a761706ea34 100644 --- a/services/context/private.go +++ b/services/context/private.go @@ -5,10 +5,12 @@ package context import ( "context" + "fmt" "net/http" "time" "gitea.dev/modules/graceful" + "gitea.dev/modules/log" "gitea.dev/modules/private" "gitea.dev/modules/process" "gitea.dev/modules/web" @@ -50,12 +52,14 @@ func (ctx *PrivateContext) Err() error { return ctx.Base.Err() } -func (ctx *PrivateContext) PrivateError(status int, err error, userMsg string) { - errMsg := "" - if err != nil { - errMsg = err.Error() - } - ctx.JSON(status, private.Response{Err: errMsg, UserMsg: userMsg}) +func (ctx *PrivateContext) PrivateInternalErrorf(format string, args ...any) { + s := fmt.Sprintf(format, args...) + log.ErrorWithSkip(1, "Internal error: %s", s) + ctx.JSON(http.StatusInternalServerError, private.Response{Err: s}) +} + +func (ctx *PrivateContext) PrivateUserErrorf(status int, format string, args ...any) { + ctx.JSON(status, private.Response{UserMsg: fmt.Sprintf(format, args...)}) } type privateContextKeyType struct{} diff --git a/services/repository/repository.go b/services/repository/repository.go index 7d1b47b6f8a..c02bf86ce56 100644 --- a/services/repository/repository.go +++ b/services/repository/repository.go @@ -72,6 +72,9 @@ func DeleteRepository(ctx context.Context, doer *user_model.User, repo *repo_mod // PushCreateRepo creates a repository when a new repository is pushed to an appropriate namespace func PushCreateRepo(ctx context.Context, authUser, owner *user_model.User, repoName string) (*repo_model.Repository, error) { + if authUser == nil { + return nil, errors.New("cannot push-create repository anonymously") + } if !authUser.IsAdmin { if owner.IsOrganization() { if ok, err := organization.CanCreateOrgRepo(ctx, owner.ID, authUser.ID); err != nil { diff --git a/tests/integration/git_general_test.go b/tests/integration/git_general_test.go index 4da46711f38..4439104d65f 100644 --- a/tests/integration/git_general_test.go +++ b/tests/integration/git_general_test.go @@ -171,7 +171,7 @@ func doSSHLFSAccessTest(_ APITestContext, keyID int64) func(*testing.T) { _, err := cmd.Output() var errExit *exec.ExitError require.ErrorAs(t, err, &errExit) // inaccessible, error - assert.Contains(t, string(errExit.Stderr), fmt.Sprintf("User: 2:user2 with Key: %d:test-key is not authorized to write to user5/repo4.", keyID)) + assert.Contains(t, string(errExit.Stderr), fmt.Sprintf(`User 2 with key %d:test-key has no "write" permission for user5/repo4`, keyID)) }) } } diff --git a/tests/integration/git_helper_for_declarative_test.go b/tests/integration/git_helper_for_declarative_test.go index bd0aedf6c91..e649899ea56 100644 --- a/tests/integration/git_helper_for_declarative_test.go +++ b/tests/integration/git_helper_for_declarative_test.go @@ -137,6 +137,7 @@ func onGiteaRun[T testing.TB](t T, callback func(T, *url.URL)) { func doGitClone(dstLocalPath string, u *url.URL) func(*testing.T) { return func(t *testing.T) { + t.Helper() assert.NoError(t, git.Clone(t.Context(), u.String(), dstLocalPath, git.CloneRepoOptions{})) exist, err := util.IsExist(filepath.Join(dstLocalPath, "README.md")) assert.NoError(t, err)