Skip to content

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

Merged
lunny merged 7 commits into
go-gitea:mainfrom
lunny:lunny/prevent_update_pull_ref
Sep 2, 2024
Merged

Prevent update pull refs manually and will not affect other refs update#31931
lunny merged 7 commits into
go-gitea:mainfrom
lunny:lunny/prevent_update_pull_ref

Conversation

@lunny

@lunny lunny commented Aug 28, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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 added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 28, 2024
@whmmm

whmmm commented Aug 28, 2024 •
edited
Loading

Copy link
Copy Markdown

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

wxiaoguang
wxiaoguang reviewed Aug 28, 2024
View reviewed changes
Comment thread routers/private/hook_pre_receive.go Outdated
@wxiaoguang

wxiaoguang commented Aug 28, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

lunny commented Aug 28, 2024

Copy link
Copy Markdown
Member Author

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

wxiaoguang commented Aug 28, 2024

Copy link
Copy Markdown
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).

@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

lunny commented Aug 28, 2024

Copy link
Copy Markdown
Member Author

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

lunny commented Aug 28, 2024

Copy link
Copy Markdown
Member Author

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

@a1012112796 a1012112796 left a comment

Copy link
Copy Markdown
Member

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
Closed
@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 and removed size/S labels Sep 2, 2024
@lunny

lunny commented Sep 2, 2024

Copy link
Copy Markdown
Member Author

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
@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 <xiaolunwen@gmail.com>
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

tsinbal commented Nov 12, 2024 •
edited
Loading

Copy link
Copy Markdown

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

lunny commented Nov 12, 2024

Copy link
Copy Markdown
Member Author

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

@wolfogre wolfogre wolfogre approved these changes

+1 more reviewer

@a1012112796 a1012112796 a1012112796 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. 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