Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

**Whitelist-Push** and **MergePullrequest** does not work when Branch-Protection is enabled #7754

Closed
sharpSteff opened this issue Aug 5, 2019 · 32 comments · Fixed by #7854

Comments

@sharpSteff
Copy link

commented Aug 5, 2019

  • Gitea version (or commit ref):
    1.9.0+3-g6d441de
  • Go Version
    1.11.5
  • Git version:
    2.7.4
  • Operating system:
    LXC container on ubuntu 16.04 LTS
  • Database (use [x]):
    • PostgreSQL
    • MySQL
    • MSSQL
    • SQLite
  • Can you reproduce the bug at https://try.gitea.io:
    • Yes (provide example URL)
    • No
    • Not relevant
  • Log gist:

Description

Whitelist-Push and MergePullrequest does not work when Branch-Protection is enabled:

Pushing develop
Enumerating objects: 7, done.
Delta compression using up to 12 threads
Total 4 (delta 3), reused 0 (delta 0)
POST git-receive-pack (530 bytes)
Remote: Gitea: Internal Server Error        
Remote: Fail to detect force push: exit status 128 - fatal: Not a git repository (or any parent up to mount point /srv/git)        
Remote: Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).        
Remote: 
Pushing to myserver
To myserver
Error: failed to push some refs to 'myserver'

Merging Pullrequests returns 500 Error

With Gitea 1.8.3+2-g11f6ed4 everything works just fine.

Screenshots

branch settings

@lunny

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Cannot reproduce this on my local

@zeripath

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

We need to see the Gitea log.

@sharpSteff

This comment has been minimized.

Copy link
Author

commented Aug 6, 2019

@lunny I also can not produce this bug on try.gitea.to
Might be related to our infrastructure (dynamic proxy, etc..)

I think 356854f has something to do with this.

before:

.....

// detect force push
if git.EmptySHA != oldCommitID {
	output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDir(repoPath)

	if err != nil {
		fail("Internal error", "Fail to detect force push: %v", err)
	} else if len(output) > 0 {
		fail(fmt.Sprintf("branch %s is protected from force push", branchName), "")
	}
}
.....

after

if git.EmptySHA != oldCommitID {
	env := append(os.Environ(),
		private.GitAlternativeObjectDirectories+"="+gitAlternativeObjectDirectories,
		private.GitObjectDirectory+"="+gitObjectDirectory,
		private.GitQuarantinePath+"="+gitObjectDirectory,
	)

	output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDirWithEnv(repo.RepoPath(), env)
	if err != nil {
		log.Error("Unable to detect force push between: %s and %s in %-v Error: %v", oldCommitID, newCommitID, repo, err)
		ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
			"err": fmt.Sprintf("Fail to detect force push: %v", err),
		})
		return
	} else if len(output) > 0 {
		log.Warn("Forbidden: Branch: %s in %-v is protected from force push", branchName, repo)
		ctx.JSON(http.StatusForbidden, map[string]interface{}{
			"err": fmt.Sprintf("branch %s is protected from force push", branchName),
		})
		return

	}
}

I'll try to provide the gitea log.
We downgraded to 1.8.3+2-g11f6ed4 , so it might take some time

@zeripath

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Ah.

I've figured this out.

That repo path needs to be prefixed with the Gitea repository path.

You're running Gitea from a weird working directory meaning that repo.RepoPath() won't work.


Nope this was totally wrong.

@jdehaan

This comment has been minimized.

Copy link

commented Aug 6, 2019

I am taking care of the infrastructure for the system on which the problem was found by @sharpSteff.

INFO: Gitea is running behind a reverse proxy mapping the server to a sub-directory (/git).

As mentioned rolling back to HEAD of 1.8 branch solves the issue, there must be something breaking in 1.9 regarding such setups. Is this information enough?

@jdehaan

This comment has been minimized.

Copy link

commented Aug 6, 2019

I could find the related entry in the log file (the xxxxx and yyyyyy are anonymizing internal names):

2019/08/05 06:29:28 .../xorm/session_get.go:99:nocacheGet() [I] [SQL] SELECT `id`, `repo_id`, `branch_name`, `can_push`, `enable_whitelist`, `whitelist_user_i_ds`, `whitelist_team_i_ds`, `e
2019/08/05 06:29:28 ...ters/private/hook.go:74:HookPreReceive() [E] Unable to detect force push between: aa77e1d18966bf5c48b9b913db3c857c9bfbabe9 and 62d7b4ea78941e254f95a38831c20f1a17e5361
        Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
        
2019/08/05 06:29:28 routers/repo/pull.go:629:MergePullRequest() [E] Merge: git push: From /srv/git/xxxx/yyyyyy.xxxx
         * branch            develop    -> FETCH_HEAD
         * [new branch]      develop    -> head_repo/develop
        Switched to a new branch 'head_repo_develop'
        .git/rebase-apply/patch:11: trailing whitespace.
                                        bat "AdvancedInstaller.com /edit ${AdvancedInstallerSetupFile} /SetAppdir -path [ProgramFilesFolder]YYY\\[ProductName] -buildname Build"\01503d
        .git/rebase-apply/patch:12: trailing whitespace.
                                        bat "AdvancedInstaller.com /edit ${AdvancedInstallerSetupFile} /SetAppdir -path [ProgramFilesFolder]YYY\\[ProductName] -buildname DefaultBuild"\01503
        warning: 2 lines add whitespace errors.
        Switched to branch 'master'
        remote: Gitea: Internal Server Error        
        remote: Fail to detect force push: exit status 128 - fatal: Not a git repository (or any parent up to mount point /srv/git)
        remote: Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
        remote: 
        To /srv/git/xxxx/yyyyyy.xxxx.git
         ! [remote rejected] master -> master (pre-receive hook declined)
        error: failed to push some refs to '/srv/git/xxxx/yyyyyy.xxxx.git'
@lunny

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

I think maybe environments break it.

env := append(os.Environ(),
		private.GitAlternativeObjectDirectories+"="+gitAlternativeObjectDirectories,
		private.GitObjectDirectory+"="+gitObjectDirectory,
		private.GitQuarantinePath+"="+gitObjectDirectory,
	)

We can remove these. But I can't reproduce that locally.

@lafriks

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@lunny hooks will not work without these environment variables. It should be something to do with specific configuration (gitea workdir, homedir or app.ini paths)

@jdehaan

This comment has been minimized.

Copy link

commented Aug 6, 2019

part of app.ini:

[repository]
ROOT = /srv/git

[server]
SSH_DOMAIN       = xxxxx.de
SSH_PORT         = 2222
SSH_LISTEN_PORT  = 2222
START_SSH_SERVER = true
DISABLE_SSH      = false
DOMAIN           = xxxxx.de
HTTP_PORT        = 3000
ROOT_URL         = https://xxxxx.de/git/
LFS_START_SERVER = false
OFFLINE_MODE     = true

[log]
MODE      = file
LEVEL     = Info
ROOT_PATH = /root/go/src/code.gitea.io/gitea/log
@lunny

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

repository.ROOT is where you store your all git repositories. could you confirm you have permissions on that directory?

@jdehaan

This comment has been minimized.

Copy link

commented Aug 6, 2019

I could find some path information here:
image

@lafriks

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

what is workdir in service file specified and what is home directory for git user?

@jdehaan

This comment has been minimized.

Copy link

commented Aug 6, 2019

The service file is:

[Unit]
Description=Gitea (Git with a cup of tea)
After=syslog.target
After=network.target
After=mysqld.service

[Service]
Type=simple
User=git
Group=git
WorkingDirectory=/root/go/src/code.gitea.io/gitea
ExecStart=/root/go/src/code.gitea.io/gitea/gitea web
Restart=always
Environment=USER=root HOME=/root

[Install]
WantedBy=multi-user.target
@jdehaan

This comment has been minimized.

Copy link

commented Aug 6, 2019

/home/git is the home of user git

BTW the permissions inside /srv/git/ look ok, everything owned by git.git

@lafriks

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

This does not seem correct in service file: Environment=USER=root HOME=/root

@lafriks lafriks referenced this issue Aug 6, 2019
2 of 7 tasks complete
@lunny

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@jdehaan Which git version are you using?

@jdehaan

This comment has been minimized.

Copy link

commented Aug 6, 2019

2.7.4 on that server

@jdehaan

This comment has been minimized.

Copy link

commented Aug 6, 2019

This does not seem correct in service file: Environment=USER=root HOME=/root

Gosh... you're right I must have done this wrong for years.... Strangely the same config works for Gitea 1.8.3 without trouble, I did not touch that part only upgraded Gitea.

@lunny

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@lafriks this git command will only detect force push, it's unrelated with hooks. The http request is from a prereceived hook I know.

@lafriks

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@lunny yes but it was rewritten to remove gitea serv command

@lafriks lafriks referenced this issue Aug 6, 2019
2 of 7 tasks complete
@jdehaan

This comment has been minimized.

Copy link

commented Aug 6, 2019

As the code modified something regarding the environment handling, I could give the upgrade a second try with the right parameters and report, before wasting your time. Thanks for the hints.

@jdehaan

This comment has been minimized.

Copy link

commented Aug 6, 2019

Changed the service file part:

Environment=USER=git HOME=/home/git

Same error, currently tested with Gitea Version: 1.9.0+10-gaea49d0

Remote: Gitea: Internal Server Error        
Remote: Fail to detect force push: exit status 128 - fatal: Not a git repository (or any parent up to mount point /srv/git)        
Remote: Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
@jdehaan

This comment has been minimized.

Copy link

commented Aug 6, 2019

From the log:

2019/08/06 10:03:36 ...ters/private/hook.go:74:HookPreReceive() [E] Unable to detect force push between: 30f13dbcd3a7ef5cb4838033f2a3ec13784f040f and 4afd02879bfb2ffd52ec3220d09394c322c65d95 in 824657420112:XXXX/YYYYYY.Xxxx Error: exit status 128 - fatal: Not a git repository (or any parent up to mount point /srv/git)
	Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
@jdehaan

This comment has been minimized.

Copy link

commented Aug 6, 2019

While it is probably not a clean fix..... I am aware of that I tried to patch the code in the following way:

--- a/routers/private/hook.go
+++ b/routers/private/hook.go
@@ -8,13 +8,13 @@ package private
 import (
        "fmt"
        "net/http"
-       "os"
+       //"os"
        "strings"
 
        "code.gitea.io/gitea/models"
        "code.gitea.io/gitea/modules/git"
        "code.gitea.io/gitea/modules/log"
-       "code.gitea.io/gitea/modules/private"
+       //"code.gitea.io/gitea/modules/private"
        "code.gitea.io/gitea/modules/repofiles"
        "code.gitea.io/gitea/modules/util"
 
@@ -29,8 +29,8 @@ func HookPreReceive(ctx *macaron.Context) {
        newCommitID := ctx.QueryTrim("new")
        refFullName := ctx.QueryTrim("ref")
        userID := ctx.QueryInt64("userID")
-       gitObjectDirectory := ctx.QueryTrim("gitObjectDirectory")
-       gitAlternativeObjectDirectories := ctx.QueryTrim("gitAlternativeObjectDirectories")
+       //gitObjectDirectory := ctx.QueryTrim("gitObjectDirectory")
+       //gitAlternativeObjectDirectories := ctx.QueryTrim("gitAlternativeObjectDirectories")
        prID := ctx.QueryInt64("prID")
 
        branchName := strings.TrimPrefix(refFullName, git.BranchPrefix)
@@ -63,13 +63,14 @@ func HookPreReceive(ctx *macaron.Context) {
 
                // detect force push
                if git.EmptySHA != oldCommitID {
-                       env := append(os.Environ(),
-                               private.GitAlternativeObjectDirectories+"="+gitAlternativeObjectDirectories,
-                               private.GitObjectDirectory+"="+gitObjectDirectory,
-                               private.GitQuarantinePath+"="+gitObjectDirectory,
-                       )
-
-                       output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDirWithEnv(repo.RepoPath(), env)
+                       //env := append(os.Environ(),
+                       //      private.GitAlternativeObjectDirectories+"="+gitAlternativeObjectDirectories,
+                       //      private.GitObjectDirectory+"="+gitObjectDirectory,
+                       //      private.GitQuarantinePath+"="+gitObjectDirectory,
+                       //)
+
+                       //output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDirWithEnv(repo.RepoPath(), env)
+                       output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDir(repo.RepoPath())
                        if err != nil {
                                log.Error("Unable to detect force push between: %s and %s in %-v Error: %v", oldCommitID, newCommitID, repo, err)
                                ctx.JSON(http.StatusInternalServerError, map[string]interface{}{

That solves the issue. Maybe that helps you to see where & why things are going wrong in our use case/setup...

@lunny

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

The workaround is similiar what I guess in #7754 (comment). @zeripath do you remember why we add those environments?

@zeripath

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

OK, this is very confusing. This works on testing - perhaps it's a weirdness of the early git version?

The internal call to the hook actually has the values passed in the GET request - this was deliberate to act as logging but you've not given us these. Do you have the ROUTER log turned off?

It could be that your version of git is not setting these and therefore by us setting them that causes the problem, or it could be that git deletes them and keeps a read hold on the directory meaning that the gitea process doesn't have access to them. Were they being set at all? What was their value? Did it require removing all of these environment variables to make things work?

The reasoning behind passing these in to the git process is that the head commit and the base may be in these alternative directories - I suspect that later versions of git are going to be more keen to use these alternative directories to quarantine against pollution of repositories.

@jdehaan

This comment has been minimized.

Copy link

commented Aug 6, 2019

I did not deliberately turn down some logging. I will look into the available log settings. At the moment we are just happy with the poor but working workaround that restores part of the 1.8 branch logic.

@zeripath

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

OK I've been able to replicate this with git version 1.7.2 (after fixing my problem with running test-sqlite locally)

It appears the solution is to test whether those values are nil and not set them if they're empty.

However, I then hit a number of other problems with the new merge commands.

So I've pushed up another PR - @jdehaan would you be able to build from this and try that?

@zeripath

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Ok so there's a problem with that pr which is apparent on the full tests. I think I know how to fix - and it will fix a potential problem at the same time doing so.

@lafriks lafriks added the kind/bug label Aug 6, 2019

@lafriks lafriks added this to the 1.9.1 milestone Aug 6, 2019

@jdehaan

This comment has been minimized.

Copy link

commented Aug 7, 2019

Yes I can build from that pr and I need the help of @sharpSteff to make the series of tests. I hope he is available for that today a couple of minutes..I will report. Thanks a bunch for your efforts until now!

@sharpSteff

This comment has been minimized.

Copy link
Author

commented Aug 7, 2019

@jdehaan yes I'm available for some tests

@shollander

This comment has been minimized.

Copy link

commented Aug 27, 2019

I don't think this is fixed. I tried using Gitea version 1.9.2 and receive an error when attempting to merge a pull request:

routers/repo/pull.go:629:MergePullRequest() [E] Merge: getDiffTree: git diff-tree [/gitea/data/tmp/local-repo/merge-123456789.git base:master head:head_repo/feature142]: fatal: ambiguous argument 'head_repo/feature142' unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git [...] -- [...]'

I am using Gitea on CentOS 7.6 with git version 1.8.3.1 (which is the standard version of Git from the yum repos).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.