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.

Sign up for GitHub

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

Jump to bottom

Prevent update pull refs manually and will not affect other refs update #31931

Merged
merged 7 commits into from
Sep 2, 2024
Merged

Prevent update pull refs manually and will not affect other refs update #31931

merged 7 commits into from
Sep 2, 2024

Conversation

lunny
Copy link
Member

@lunny lunny commented Aug 28, 2024 •
edited
Loading

All refs under refs/pull should only be changed from Gitea inside but not by pushing from outside of Gitea.
This PR will prevent the pull refs update but allow other refs to be updated on the same pushing with --mirror operations.

The main changes are to add checks on update hook but not pre-receive because update will be invoked by every ref but pre-receive will revert all changes once one ref update fails.

@lunny lunny added type/bug backport/v1.22 This PR should be backported to Gitea 1.22 labels Aug 28, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 28, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 28, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Aug 28, 2024
@whmmm
Copy link

whmmm commented Aug 28, 2024 •
edited
Loading

If a repository contains open pull requests, executing git push --mirror will result in an error. This behavior seems somewhat unreasonable to me.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 28, 2024 •
edited
Loading

There are other ref "types", eg: IsRemote, and there will be more in the future (there is a comment TODO: /refs/for-review for suggest change interface)

Ideally, I think the switch/default block should be changed:

  • If a ref "type" is recognized (eg: branch, tag, agit-for), then handle it.
  • Otherwise (default:), do not accept the update
    • If it is impossible to skip the update, then stop the push, just like what the PR did in the first commit

@lunny
Copy link
Member Author

lunny commented Aug 28, 2024

I need to clarify there are two kinds of skips here. One is just executed and don't return error. another is don't execute it and don't return error.

@wxiaoguang
Copy link
Contributor

ps: just realized that this logic is in "pre-receive" hook , no idea whether it is possible to only skip some branches. If it is impossible to skip, then maybe the only choice is to "stop the loop" (just like what the PR did in the first commit).

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 28, 2024
@github-actions github-actions bot added the modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin label Aug 28, 2024
@lunny lunny changed the title Prevent update pull refs manually Prevent update pull refs manually and will not affect other refs update Aug 28, 2024
@lunny
Copy link
Member Author

lunny commented Aug 28, 2024

If a repository contains open pull requests, executing git push --mirror will result in an error. This behavior seems somewhat unreasonable to me.

I think now it should resolve your problem. Although you will get some errors but that will not affect other refs update. Currently I don't know how to let git ignore those error informations.

@lunny
Copy link
Member Author

lunny commented Aug 28, 2024

ps: just realized that this logic is in "pre-receive" hook , no idea whether it is possible to only skip some branches. If it is impossible to skip, then maybe the only choice is to "stop the loop" (just like what the PR did in the first commit).

I changed it to update hook. I think it should be OK now.

a1012112796
a1012112796 approved these changes Aug 29, 2024
View reviewed changes
Copy link
Member

@a1012112796 a1012112796 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest add some test code.

@GiteaBot 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 Aug 29, 2024
This was referenced Aug 29, 2024
Merged
Open
@GiteaBot 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 Sep 2, 2024
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 2, 2024
@lunny
Copy link
Member Author

lunny commented Sep 2, 2024

suggest add some test code.

Tests added.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 2, 2024
@lunny lunny enabled auto-merge (squash) September 2, 2024 07:10
@lunny lunny merged commit ac34449 into go-gitea:main Sep 2, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Sep 2, 2024
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Sep 2, 2024
@lunny @GiteaBot
Prevent update pull refs manually and will not affect other refs upda…
6ac5840 
…te (go-gitea#31931)

All refs under `refs/pull` should only be changed from Gitea inside but
not by pushing from outside of Gitea.
This PR will prevent the pull refs update but allow other refs to be
updated on the same pushing with `--mirror` operations.

The main changes are to add checks on `update` hook but not
`pre-receive` because `update` will be invoked by every ref but
`pre-receive` will revert all changes once one ref update fails.
@GiteaBot GiteaBot mentioned this pull request Sep 2, 2024
Merged
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Sep 2, 2024
@lunny lunny deleted the lunny/prevent_update_pull_ref branch September 2, 2024 07:39
@a1012112796 a1012112796 mentioned this pull request Sep 2, 2024
Closed
wolfogre pushed a commit that referenced this pull request Sep 2, 2024
@GiteaBot @lunny
Prevent update pull refs manually and will not affect other refs upda…
6f5748c 
…te (#31931) (#31955)

Backport #31931 by @lunny

All refs under `refs/pull` should only be changed from Gitea inside but
not by pushing from outside of Gitea.
This PR will prevent the pull refs update but allow other refs to be
updated on the same pushing with `--mirror` operations.

The main changes are to add checks on `update` hook but not
`pre-receive` because `update` will be invoked by every ref but
`pre-receive` will revert all changes once one ref update fails.

Co-authored-by: Lunny Xiao <[email protected]>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 5, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
6dce37d 
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  [skip ci] Updated translations via Crowdin
  Remove html tags from create tag and branch translation (go-gitea#31973)
  Replace v-html with v-text in search inputbox (go-gitea#31966)
  [skip ci] Updated translations via Crowdin
  [skip ci] Updated translations via Crowdin
  Improve get feed with pagination (go-gitea#31821)
  Remove urls from translations (go-gitea#31950)
  Prevent update pull refs manually and will not affect other refs update (go-gitea#31931)
  [skip ci] Updated translations via Crowdin
  nix wording nit in todo code comment
  Fix 500 error when `state` params is set when editing issue/PR by API (go-gitea#31880)
  Fix sort order for organization home and user profile page (go-gitea#31921)
  Improve textarea paste (go-gitea#31948)
  Fix index too many file names bug (go-gitea#31903)
  [skip ci] Updated translations via Crowdin
  Move web globals to `web_src/js/globals.d.ts` (go-gitea#31943)
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request Oct 11, 2024
@Gusted
[FEAT] Don't allow modification to internal reference
84e77c4 
- This a port of go-gitea/gitea#31931 in a
behavior-sense. None of the code was actually ported.
- Follow up for #2834, now also don't allow modification.
- Integration test added.
- Unit test modified.
@tsinbal
Copy link

tsinbal commented Nov 12, 2024 •
edited
Loading

This fix branch addresses an issue: when I attempt to mirror push from one Gitea service to another Gitea Service, the following error occurs:

PushRejected Error: remote: error: hook declined to update refs/pull/1/head
remote: error: hook declined to update refs/pull/2/head
remote: error: hook declined to update refs/pull/3/head
Screenshot_20241112_104122

@lunny
Copy link
Member Author

lunny commented Nov 12, 2024

Please fire a new issue to report it.

@go-gitea go-gitea locked as off-topic and limited conversation to collaborators Nov 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@a1012112796 a1012112796 a1012112796 approved these changes

@wolfogre wolfogre wolfogre approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/v1.22 This PR should be backported to Gitea 1.22 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/go Pull requests that update Go code size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/bug
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.
7 participants
@lunny @whmmm @wxiaoguang @tsinbal @wolfogre @a1012112796 @GiteaBot