500 error if PR merge is rejected by pre-receive hook (exit 1)

[mshgh]

• Gitea version (or commit ref): 1.11.1 (docker image)
• Git version: not relevant
• Operating system: not relevant
• 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

I tried to test pre-receive git hook (by editing via gitea web interface) and setup simple rejection

#!/bin/sh
echo "Forced rejection" >&2
exit 1

every direct (again via web interface) file modification is rejected as expected with the error message above. However if I try to merge a pull request I get 500 error page instead.

[zeripath]

Yes that's because pre-receive hooks run on merge, crud and repository creation too

The simplest way you can detect internal actions is by looking at the contents of SSH_ORIGINAL_COMMAND. We have however not completely specd out the what contents of this should be and would like to think a bit more carefully about how this should work

[mshgh]

I seem to be not clear enough. I actually want to abort internal gitea operation (under certain conditions). See below what happens if I try to edit a file. I expect the same error message for the Pull Request page, but 500 error page is shown instead. I believe this is a bug.

[mshgh]

@zeripath @lunny this was labeled as a 'question', but please can you give this second thought? My hook is doing exactly what I intend to do - to prevent pull request merge if appropriate. However the user experience is disaster - 500 error page instead of a nice error message on the top of the pull request page where there is clean explanation (coming from my hook) why the merge was prevented.

From my personal point of view I see this as a bug as I have high expectations from gitea :-) But if you say "Enhancement" I would not complain. It just makes a lot of sense to me gitea behaves this way.

[lunny]

500 should be a bug, we should prompt an error message.

[zeripath]

OK I think we should try to provide the rejection message but the message displayed above is bad - we should not display the real repository path.

[zeripath]

Now the error messages in CRUD actions also clearly need to be sanitised. We should not be exposing the system filepath of the repositories.

[guillep2k]

Now the error messages in CRUD actions also clearly need to be sanitised. We should not be exposing the system filepath of the repositories.

I think we could shamelessly replace setting.RepoRootPath with "{repositories}" in the error output (a similar thing with the temporary path too). There are edge cases were the error message could be garbled, but it would probably be better than it currently is.

[zeripath]

I've just done that and extended my changes to affect web CRUD too.

[mshgh]

There is very curious bug in Gitea 1.11.3 docker image I can see. If the message provided from pre-receive hook ends with new line the error message shows the internal repository part again. See details how to reproduce below.

One more thing. The message cannot contain HTML anymore. As the whole message is center-aligned multi-line messages are not pretty. In my case I am showing list of commits preventing the merge. You can imagine how funny the list of commits with comments looks like if this is center-aligned. This is definitely minor, but when the bug above is being fixed can you consider to
a) allow HTML messages. GitHooks can be created by site admin only so I recon we can consider them safe
b) place the message into pre-fromatted block which itself is centered, but its content is left-aligned

OK case

#!/bin/sh

echo -e "Foo" >&2
exit 1

Merge Failed: The push was rejected with the following message:
Foo
Review the githooks for this repository

BAD case

#!/bin/sh

echo -e "Foo\n" >&2
exit 1

Merge Failed: The push was rejected with the following message:
Foo

To /data/git/repositories/gadmin/test.git
! [remote rejected] base -> master (pre-receive hook declined)
error: failed to push some refs to '/data/git/repositories/gadmin/test.git'
Review the githooks for this repository

[zeripath]

@mshgh please try not to message on old bugs - it's very hard to keep up with the amount of notifications on github for gitea so it may get ignored.

I'll take a look though.

[zeripath]

Lets discuss message formatting and html in a new issue.

[mshgh]

@zeripath thank you for noticing. I was considering this approach or new issue. Now I know better ;)

Separate issue created #10778