finos / finos/git-proxy

checkUserPushPermission is checking the wrong user's permission to push

Open Beginner friendly
#1,400 16 comments 0 reactions 0 assignees View on GitHub
bug tech-debt
Dominant language
TypeScript
Stars
249
Forks
176
Avg merge
3d 8h
Merged PRs (30d)
20

Description

**Describe the bug**
`checkUserPushPermission` is intended to check whether the user performing the push is allowed to do so. However, it is currently checking the permission of the committer of the last commit in the push in its current location, rather than the user pushing it into git proxy.

Commits in git are tagged with two users' details: the _author_ (who created the commit and originally pushed it to a repo) and the _committer_ (initially the same as the author, ~but changes whenever the commit is pushed to a different repository or branch~ but changes whenever a commit is created or modified (e.g., via `git commit`, `git commit --amend`, `git rebase`, `git merge`) - `git push` does NOT change the _committer_. When we process a push in git proxy, we do so using the original committer of the last commit rather than the user performing the push - which is not the intent of the check.

`parsePush` is extracting the `committer` and `committerEmail` from the last commit in the push and setting them as the `user` and `userEmail` in the action object:

https://github.com/finos/git-proxy/blob/6fb63d0597212e9e0056330d3a90df9997f2d505/src/proxy/processors/push-action/parsePush.ts#L93-L96

This is not the correct user information and needs to be corrected in `parsePush`, without using `action.commitData`.

**To Reproduce**
1. Setup a project in Git proxy where you are allowed to contribute but no other user is
2. Have a different user push a commit to a branch of the respository.
3. Clone that branch yourself.
4. While that different user's commit is the last one on that branch, push the branch to git proxy.
5. `checkUserPushPermission` will reject the push as it will use the other user's information as the committer,

**Expected behavior**
`parsePush` should use the details of the user trying to push through the proxy as the `user` and `userEmail` values of the action (rather than the user that original pushed the last commit), ensuring that `checkUserPushPermission` checks the correct user's details.

Contributor guide

Open the contributing guide

Research direction

Read src/proxy/processors/push-action/parsePush.ts around lines 93-96 and trace how the action's user and userEmail are populated. Confirm the values represent the user performing the proxy push rather than the last commit's committer, without using action.commitData. Reproduce the described push scenario and verify checkUserPushPermission evaluates the pushing user's details.

Written by the indexing model from the issue text.

Assessment

Tech stack
git, typescript
Domain
backend, security
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.