Also consider actions user when evaluating push privileges on protected branch

[Naxdy]

Currently, when attempting to push to a protected branch from a Gitea Action using the provided token, the push will always fail, even if "enable push" is set in the branch protection rule.

Relevant log output:

../private/hook_pre_receive.go:266:preReceiveBranch() [E] Unable to GetUserByID for commits from 171e01532f958325249e84c6e7e74f4f0804c59a to 1e73df32ae79467068a7dfec4abe6484753a7a97 in <Repository 39:NaxdyOrg/Infrastructure>: user does not exist [uid: -2, name: ]

This is because if a branch protection rule exists, only deploy keys are checked for push privileges, and it is not considered that the pusher may be an actions user.

See:

gitea/routers/private/hook_pre_receive.go

Lines 168 to 172 in 6599efb

	// Allow pushes to non-protected branches
	if protectBranch == nil {
	return
	}
	protectBranch.Repo = repo

gitea/routers/private/hook_pre_receive.go

Lines 254 to 277 in 6599efb

	// 5. Check if the doer is allowed to push (and force-push if the incoming push is a force-push)
	var canPush bool
	if ctx.opts.DeployKeyID != 0 {
	// This flag is only ever true if protectBranch.CanForcePush is true
	if isForcePush {
	canPush = !changedProtectedfiles && protectBranch.CanPush && (!protectBranch.EnableForcePushAllowlist || protectBranch.ForcePushAllowlistDeployKeys)
	} else {
	canPush = !changedProtectedfiles && protectBranch.CanPush && (!protectBranch.EnableWhitelist || protectBranch.WhitelistDeployKeys)
	}
	} else {
	user, err := user_model.GetUserByID(ctx, ctx.opts.UserID)
	if err != nil {
	log.Error("Unable to GetUserByID for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err)
	ctx.JSON(http.StatusInternalServerError, private.Response{
	Err: fmt.Sprintf("Unable to GetUserByID for commits from %s to %s: %v", oldCommitID, newCommitID, err),
	})
	return
	}
	if isForcePush {
	canPush = !changedProtectedfiles && protectBranch.CanUserForcePush(ctx, user)
	} else {
	canPush = !changedProtectedfiles && protectBranch.CanUserPush(ctx, user)
	}
	}

Also checking for the actions user ID solves this problem, and grants them the same permissions as deploy keys for the purposes of pushing / force-pushing, as imo would be expected behavior.

[wxiaoguang]

The question is: do we want to make the Actions user be able to "push" (even more: have "write" permission to the whole repo)?

Another question is what's the GitHub's behavior?

(I don't know details, so just ask questions)

[Naxdy]

For me personally it's very much expected that if I explicitly allow pushing to a branch within its protection rules, that that permission carries over to Actions tokens. The fact that it currently carries over to deploy keys, but not Actions tokens is rather unintuitive imo.

In GitHub, this is handled via the permissions section of the workflow config, which we do not support yet (relevant discussion: #24635).

Edit:

(even more: have "write" permission to the whole repo)

Actions tokens do already have "write" permissions to the whole repo, save for protected branches (regardless of their push settings, which this PR aims to rectify).

[wxiaoguang]

Then should we implement the permissions first?

Or, could there be a security problem? for example: a contributor with read access proposed a PR, then the Actions run the PR, then the PR use the Actions token to write the repository (which it shouldn't because the contributor only has "read" access)?

[wxiaoguang]

Actions tokens do already have "write" permissions to the whole repo

Hmm, if it is already so, then I don't have more questions.

[Naxdy]

To clarify, Actions run from the main repo have write permissions, forked repos are treated differently.

See the relevant section here:

gitea/routers/private/hook_pre_receive.go

Lines 493 to 504 in 6599efb

	if ctx.opts.UserID == user_model.ActionsUserID {
	ctx.user = user_model.NewActionsUser()
	ctx.userPerm.AccessMode = perm_model.AccessMode(ctx.opts.ActionPerm)
	if err := ctx.Repo.Repository.LoadUnits(ctx); err != nil {
	log.Error("Unable to get User id %d Error: %v", ctx.opts.UserID, err)
	ctx.JSON(http.StatusInternalServerError, private.Response{
	Err: fmt.Sprintf("Unable to get User id %d Error: %v", ctx.opts.UserID, err),
	})
	return false
	}
	ctx.userPerm.SetUnitsWithDefaultAccessMode(ctx.Repo.Repository.Units, ctx.userPerm.AccessMode)
	} else {

Later there is an assertion that will return if the user cannot write to the repo (i.e. if it's an Action run from another repo):

gitea/routers/private/hook_pre_receive.go

Lines 65 to 77 in 6599efb

	// AssertCanWriteCode returns true if pusher can write code
	func (ctx *preReceiveContext) AssertCanWriteCode() bool {
	if !ctx.CanWriteCode() {
	if ctx.Written() {
	return false
	}
	ctx.JSON(http.StatusForbidden, private.Response{
	UserMsg: "User permission denied for writing.",
	})
	return false
	}
	return true
	}

(if my understanding is correct)

[wxiaoguang]

But "agit" PR will run in the same base repo?

[Naxdy]

It depends on whether the workflow runs on pull_request or pull_request_target. If it runs on pull_request, it will always run in the head repo, with access to that repo's code, secrets and variables. That's how it is on GitHub anyway, and from what I can gather, the same on Gitea. Here, if head repo & base repo are the same, Actions can push to base repo.

pull_request_target always runs in the base repo, with base repo code as well as secrets and variables.

Any other workflow trigger, like push, schedule, workflow_dispatch, etc. run in the repo where they were triggered.

[Naxdy]

I'm not familiar with agit, but if PRs created using it are always in the base repo, then they will have access to that repo's secrets & variables, as well as read/write permission (the latter coming in this PR).

[wxiaoguang]

I would suggest that we'd better to figure out the whole (and correct) design first and fully understand the problem before changing the behavior.