Fix PATCH /repos/{owner}/{repo} panic #14637

anton-khimich · 2021-02-10T23:12:15Z

When attempting to modify the "default_branch" field of an empty repository through the PATCH /repos/{owner}/{repo} endpoint, you get a runtime error due to a nil pointer dereference:

2021/02/09 23:13:15 Started PATCH /api/v1/repos/user234/test_repo3?token=f86a2ad0daf6c7585548380dca3ab5661389f6b2 for 127.0.0.1:41978
2021/02/09 23:13:16 models/repo.go:341:getUnits() [T] repo.Units: [0xc007a29ef0 0xc007a29f50 0xc007a29fb0 0xc007a4a030 0xc007a4a090 0xc007a4a0f0]
2021/02/09 23:13:16 ...s/repo_permission.go:154:func1() [T] Permission Loaded for 1:user1234 in 4:user1234/test_repo3:
        Permissions: AccessMode: 4:owner, 6 Units, 0 UnitsMode(s): [ 
        Units[0]: ID: 79 RepoID: 4 Type: 1:UnitTypeCode Config: {}
        Units[1]: ID: 80 RepoID: 4 Type: 2:UnitTypeIssues Config: {"EnableTimetracker":false,"AllowOnlyContributorsToTrackTime":true,"EnableDependencies":true}
        Units[2]: ID: 81 RepoID: 4 Type: 3:UnitTypePullRequests Config: {"IgnoreWhitespaceConflicts":false,"AllowMerge":true,"AllowRebase":true,"AllowRebaseMerge":true,"AllowSquash":true}
        Units[3]: ID: 82 RepoID: 4 Type: 4:UnitTypeReleases Config: {}
        Units[4]: ID: 83 RepoID: 4 Type: 5:UnitTypeWiki Config: {}
        Units[5]: ID: 84 RepoID: 4 Type: 8:UnitTypeProjects Config: {} ]
2021/02/09 23:13:16 .../api/v1/repo/repo.go:577:updateBasicProperties() [T] opts not nil
2021/02/09 23:13:16 .../api/v1/repo/repo.go:579:updateBasicProperties() [T] Default Branch name: string
2021/02/09 23:13:16 ...outers/routes/web.go:80:1() [E] PANIC: runtime error: invalid memory address or nil pointer dereference
        /usr/local/go/src/runtime/panic.go:212 (0x4569d2)
                panicmem: panic(memoryError)
        /usr/local/go/src/runtime/signal_unix.go:742 (0x456852)
                sigpanic: panicmem()
        /home/anton/gitea/modules/git/repo_branch.go:59 (0x8b8847)
                (*Repository).SetDefaultBranch: _, err := NewCommand("symbolic-ref", "HEAD", BranchPrefix+name).RunInDir(repo.Path)
        /home/anton/gitea/routers/api/v1/repo/repo.go:588 (0x210fcb5)
                updateBasicProperties: if err := ctx.Repo.GitRepo.SetDefaultBranch(*opts.DefaultBranch); err != nil {
        /home/anton/gitea/routers/api/v1/repo/repo.go:494 (0x210f0c4)
                Edit: if err := updateBasicProperties(ctx, opts); err != nil {
        /home/anton/gitea/modules/web/route.go:59 (0x1f3c04b)
                Wrap.func1: t(ctx)
        /usr/local/go/src/net/http/server.go:2042 (0x76f163)
                HandlerFunc.ServeHTTP: f(w, r)
        /home/anton/gitea/vendor/github.com/go-chi/chi/mux.go:437 (0x1b1298a)
                (*Mux).routeHTTP: h.ServeHTTP(w, r)
        /usr/local/go/src/net/http/server.go:2042 (0x76f163)
                HandlerFunc.ServeHTTP: f(w, r)
        /home/anton/gitea/modules/web/route.go:105 (0x1f3c581)
                MiddleAPI.func1.1: next.ServeHTTP(ctx.Resp, ctx.Req)
        /usr/local/go/src/net/http/server.go:2042 (0x76f163)
                HandlerFunc.ServeHTTP: f(w, r)
        /home/anton/gitea/modules/context/api.go:271 (0x1b29778)
                APIContexter.func1.1: next.ServeHTTP(ctx.Resp, ctx.Req)
        /usr/local/go/src/net/http/server.go:2042 (0x76f163)
                HandlerFunc.ServeHTTP: f(w, r)
        /home/anton/gitea/routers/api/v1/api.go:1048 (0x214a630)
                securityHeaders.func1.1: next.ServeHTTP(resp, req)
        /usr/local/go/src/net/http/server.go:2042 (0x76f163)
                HandlerFunc.ServeHTTP: f(w, r)
        /home/anton/gitea/vendor/gitea.com/go-chi/session/session.go:256 (0x15104af)
                Sessioner.func1.1: next.ServeHTTP(w, req)
        /usr/local/go/src/net/http/server.go:2042 (0x76f163)
                HandlerFunc.ServeHTTP: f(w, r)
        /home/anton/gitea/vendor/github.com/go-chi/chi/mux.go:69 (0x1b1063e)
                (*Mux).ServeHTTP: mx.handler.ServeHTTP(w, r)
        /home/anton/gitea/vendor/github.com/go-chi/chi/mux.go:312 (0x1b16adb)
                (*Mux).Mount.func1: handler.ServeHTTP(w, r)
        /usr/local/go/src/net/http/server.go:2042 (0x76f163)
                HandlerFunc.ServeHTTP: f(w, r)
        /home/anton/gitea/vendor/github.com/go-chi/chi/mux.go:437 (0x1b1298a)
                (*Mux).routeHTTP: h.ServeHTTP(w, r)
        /usr/local/go/src/net/http/server.go:2042 (0x76f163)
                HandlerFunc.ServeHTTP: f(w, r)
        /home/anton/gitea/routers/routes/web.go:88 (0x2171a3d)
                commonMiddlewares.func2.1: next.ServeHTTP(resp, req)
        /usr/local/go/src/net/http/server.go:2042 (0x76f163)
                HandlerFunc.ServeHTTP: f(w, r)
        /home/anton/gitea/routers/routes/base.go:37 (0x216d87b)
                LoggerHandler.func1.1: next.ServeHTTP(w, req)
        /usr/local/go/src/net/http/server.go:2042 (0x76f163)
                HandlerFunc.ServeHTTP: f(w, r)
        /home/anton/gitea/vendor/github.com/go-chi/chi/middleware/realip.go:34 (0x2168f5c)
                RealIP.func1: h.ServeHTTP(w, r)
        /usr/local/go/src/net/http/server.go:2042 (0x76f163)
                HandlerFunc.ServeHTTP: f(w, r)
        /home/anton/gitea/routers/routes/web.go:61 (0x217157c)
                commonMiddlewares.func1.1: next.ServeHTTP(context.NewResponse(resp), req)
        /usr/local/go/src/net/http/server.go:2042 (0x76f163)
                HandlerFunc.ServeHTTP: f(w, r)
        /home/anton/gitea/vendor/github.com/go-chi/chi/mux.go:86 (0x1b103e3)
                (*Mux).ServeHTTP: mx.handler.ServeHTTP(w, r)
        /home/anton/gitea/modules/web/route.go:267 (0x1f3b333)
                (*Route).ServeHTTP: r.R.ServeHTTP(w, req)
        /home/anton/gitea/vendor/github.com/gorilla/context/context.go:141 (0x11781d3)
                ClearHandler.func1: h.ServeHTTP(w, r)
        /usr/local/go/src/net/http/server.go:2042 (0x76f163)
                HandlerFunc.ServeHTTP: f(w, r)
        /usr/local/go/src/net/http/server.go:2843 (0x772762)
                serverHandler.ServeHTTP: handler.ServeHTTP(rw, req)
        /usr/local/go/src/net/http/server.go:1925 (0x76df6c)
                (*conn).serve: serverHandler{c.server}.ServeHTTP(w, w.req)
        /usr/local/go/src/runtime/asm_amd64.s:1374 (0x478d00)
                goexit: BYTE    $0x90   // NOP

2021/02/09 23:13:16 Completed PATCH /api/v1/repos/user1234/test_repo3?token=f86a2ad0daf6c7585548380dca3ab5661389f6b2 500 Internal Server Error in 193.267751ms

This PR adds a check to see if the pointer is nil before attempting to dereference.

Using the `PATCH /repos/{owner}/{repo}` endpoint and attempting to modify `default_branch` on an empty repository will cause a panic. This commit adds a check for a nil pointer before attempting to dereference it.

zeripath · 2021-02-11T08:06:08Z

I think the problem is that the ctx.Repo.GitRepo should never not be nil.

It's likely that we need to load the gitrepo directly ourselves.

If we merged this pr as it stands git's idea of the default branch won't be updated.

anton-khimich · 2021-02-11T15:49:47Z

We can take a look at the code responsible for injecting GitRepo into context. What's happening is that it checks to see if ctx.Repo.Repository is empty (in this case it is), and returning with ctx.Repo.GitRepo = nil. I'm open to suggestions, but I believe that RepoRefForAPI should be the function modified if my check is inadequate.

routers/api/v1/repo/repo.go

zeripath

As this stands this will not work.

6543 · 2021-02-11T17:53:03Z

a other option would be to just check if !project.IsEmpty instead ...

If you change the default branch for a repository you must change it in git too. Therefore you must open the repository before changing the default branch. Signed-off-by: Andrew Thornton <art27@cantab.net>

Fixed the issue now.

6543 · 2021-02-11T17:56:08Z

@zeripath on an empty repo you can NOT open a git repo!

6543

blocking per comment

zeripath · 2021-02-11T18:03:25Z

Why don't you try...

zeripath · 2021-02-11T18:10:48Z

You can very definitely open a git repository on an empty repository. The git repository is there and the set default branch etc will work fine.

In fact you need to set the default branch on the empty repo otherwise the default branch will not work correctly.

Now the change where you now test if the branch exists is an issue.

If the repo is empty we should allow any branch to be set as the default branch. If the repo is not empty we will need to ensure that the branch existing I suspect.

Signed-off-by: Andrew Thornton <art27@cantab.net>

anton-khimich · 2021-02-11T18:14:37Z

Sorry, to be clear, the repository isn't just empty, but it has not yet even been initialized. Should it be possible to open an uninitialized git repository?

6543 · 2021-02-11T18:23:15Z

@zeripath your patch works, but it just looks wrong ..

☝️

6543

it works ...

Anton Khimich added 2 commits February 10, 2021 17:12

Fix a runtime error when modifying a repository through API call

50996b8

Using the `PATCH /repos/{owner}/{repo}` endpoint and attempting to modify `default_branch` on an empty repository will cause a panic. This commit adds a check for a nil pointer before attempting to dereference it.

Merge branch 'master' into fix_patch_default_branch_panic

c58f313

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 11, 2021

6543 added the modifies/api This PR adds API routes or modifies them label Feb 11, 2021

6543 reviewed Feb 11, 2021

View reviewed changes

routers/api/v1/repo/repo.go Outdated Show resolved Hide resolved

6543 reviewed Feb 11, 2021

View reviewed changes

routers/api/v1/repo/repo.go Outdated Show resolved Hide resolved

Apply suggestions from code review

be5c83a

6543 reviewed Feb 11, 2021

View reviewed changes

routers/api/v1/repo/repo.go Outdated Show resolved Hide resolved

Apply suggestions from code review

4f1711e

6543 added the type/bug label Feb 11, 2021

6543 added this to the 1.14.0 milestone Feb 11, 2021

6543 approved these changes Feb 11, 2021

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 11, 2021

Merge branch 'master' into fix_patch_default_branch_panic

c3068b1

zeripath previously requested changes Feb 11, 2021

View reviewed changes

Ensure that the git repository is loaded

bf4164a

If you change the default branch for a repository you must change it in git too. Therefore you must open the repository before changing the default branch. Signed-off-by: Andrew Thornton <art27@cantab.net>

6543 previously requested changes Feb 11, 2021

View reviewed changes

Allow empty repos to have their default branches changed

28c47f9

Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath approved these changes Feb 11, 2021

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 11, 2021

Merge branch 'master' into fix_patch_default_branch_panic

949ed90

6543 approved these changes Feb 11, 2021

View reviewed changes

6543 merged commit 5a18712 into go-gitea:master Feb 11, 2021

go-gitea locked and limited conversation to collaborators May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix PATCH /repos/{owner}/{repo} panic #14637

Fix PATCH /repos/{owner}/{repo} panic #14637

anton-khimich commented Feb 10, 2021

zeripath commented Feb 11, 2021

anton-khimich commented Feb 11, 2021

zeripath left a comment

6543 commented Feb 11, 2021

6543 commented Feb 11, 2021

6543 left a comment

zeripath commented Feb 11, 2021

zeripath commented Feb 11, 2021

anton-khimich commented Feb 11, 2021

6543 commented Feb 11, 2021

6543 left a comment

Fix PATCH /repos/{owner}/{repo} panic #14637

Fix PATCH /repos/{owner}/{repo} panic #14637

Conversation

anton-khimich commented Feb 10, 2021

zeripath commented Feb 11, 2021

anton-khimich commented Feb 11, 2021

zeripath left a comment

Choose a reason for hiding this comment

6543 commented Feb 11, 2021

6543 commented Feb 11, 2021

6543 left a comment

Choose a reason for hiding this comment

zeripath commented Feb 11, 2021

zeripath commented Feb 11, 2021

anton-khimich commented Feb 11, 2021

6543 commented Feb 11, 2021

6543 left a comment

Choose a reason for hiding this comment