Murmele / Gittyup

Understand your Git history!
https://murmele.github.io/Gittyup
MIT License
1.56k stars 113 forks source link

Support more Git hooks #261

Open alex-ball opened 2 years ago

alex-ball commented 2 years ago

I am running Gittyup v1.1.2 on Ubuntu 20.04 via flatpak.

Expected behaviour: In a Git repo with executable files at .git/hooks/pre-commit and .git/hooks/post-commit, committing some changes should run those two scripts immediately before and after the commit, respectively.

Actual behaviour: The commit succeeds but the two files do not execute.

I'm afraid I don't know enough about these things to know if

  1. this is a security limitation of flatpaks and can only be solved by installing some other way;
  2. this can be solved by editing the app permissions;
  3. this is a bug in the code;
  4. this would be an enhancement.

I have not tested whether the other Git hooks are triggered (by push, receive, etc.).

exactly-one-kas commented 2 years ago

Currently, only the pre-push hook is supported, although I'm not sure if this works with flatpak. @Murmele do you know if flatpak needs additional permissions to execute programs outside its sandbox even if they're within opened directories?

exactly-one-kas commented 2 years ago

Also, the pre-push requires bash for some reason? I think this restriction should be lifted

Murmele commented 2 years ago

currently for the merge commands we are leaving the flatpak sandbox. there we are using flatpak-spawn --host sh -c "<command>" to execute a command on the host system. It works withouth any additional permissions

Murmele commented 2 years ago

It depends on what is written in the script. If some host commands are used then it will not work (or only if executed from host). If just common commands are used I have to check

Murmele commented 2 years ago

@exactly-one-kas how it behaves without sandbox? Do I have to do chmod +x first on those files, or is git able to execute them without?

exactly-one-kas commented 2 years ago

Git calls them as executables and requires the executable bit to be set, so I'd say we should use flatpack-spawn --host "<path to hook>"

Murmele commented 2 years ago

Git calls them as executables and requires the executable bit to be set, so I'd say we should use flatpack-spawn --host "<path to hook>"

yes I think that makes it easier. The question is are we then going around some sandboxing features?

exactly-one-kas commented 2 years ago

Since we're running arbitrary scripts on the host, I'd say yes. Can we run them inside the sandbox or is that prevented?

Murmele commented 2 years ago

I think that should be also possible.

lonix1 commented 5 months ago

For future consideration: I think .git/hooks/pre-commit is the most common/useful, so if there's much work to support each type of hook, it's best to start with the most important one.

schnedann commented 2 months ago

I just implemented a prepare-commit-hook and learned gittyup does not execute it. I see this functionality as very important, as the hooks are one of the few ways to mitigate some of the worst design fails of git...

So from me: please, hooks are important, please implement

anselme16 commented 3 weeks ago

Hi, so if i understand well, git hooks are not supported yet only in the flatpak version ? If i compile it from source i will not be sandboxed and the pre-commit hook will be executed ?

Murmele commented 3 weeks ago

No, currently we do not support hooks. The problem is that libgit2 does not support this. In this case we have to execute them manually and this is not yet implemented

Anselme FRANÇOIS @.***> schrieb am Mo., 4. Nov. 2024, 13:35:

Hi, so if i understand well, git hooks are not supported yet only in the flatpak version ? If i compile it from source i will not be sandboxed and the pre-commit hook will be executed ?

— Reply to this email directly, view it on GitHub https://github.com/Murmele/Gittyup/issues/261#issuecomment-2454602466, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACNBWTP7Q7L6POMODCYIY5DZ65SXRAVCNFSM6AAAAABJSDQ5QCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJUGYYDENBWGY . You are receiving this because you were mentioned.Message ID: @.***>

anselme16 commented 3 weeks ago

Thanks.

By the way for context i found the related libgit2 issue where they discuss on hook support : https://github.com/libgit2/libgit2/issues/964

They will indeed never support hooks because they don't want to be responsible for calling external processes, so they assume every app using libgit2 will have to reimplement their own hooks.

Murmele commented 3 weeks ago

Thanks.

By the way for context i found the related libgit2 issue where they discuss on hook support : libgit2/libgit2#964

They will indeed never support hooks because they don't want to be responsible for calling external processes, so they assume every app using libgit2 will have to reimplement their own hooks.

Yes exactly

Murmele commented 3 weeks ago

We have already a few external calls, for example for the merge tool / diff tool, so this code could be checked out for implementation details

https://github.com/Murmele/Gittyup/blob/80231ac219d45edc579dd4d65dc255861ec585ff/src/tools/EditTool.cpp#L75-L91

schnedann commented 3 weeks ago

Of course its unsafe - but it is git which introduced the hooks, so a lib which mimics git command.... anyway, one could improve security if the app at first start scans the hooks and stores checksums for all hook files. Then, if there is a new or changed file, request the users consent... Thats not really safe, also but if one would be able to commit the file containing the checksums, chances are good an altered file would be detected.

Or what about use a VCS for the hook files? :-)

Murmele commented 3 weeks ago

We could have already that problem with the merge tools. Just fake for example kdiff3 by adding a script called the name of the mergetool somewhere and if the path is before the correct binary, we execute that script instead of the real merge tool :) In this case we could bundle all merge tools within the package, but it is a lot more effort

schnedann commented 3 weeks ago

Of course, just a note: bundling things is just the old windows way of cluttering the harddisk. if you run a linux with SW repository bundling is just what you not want.

As written: I would be OK to be warned for security problems, also logging that warning - Yes, good idea. But practically there is no real measure to prevent calling others Code / Tools.

One further measure on linux might be to implement for the biggest distributions / Debian based, Arch, Fedora, Suse,... to ask the repository management to verify the called SW (if they offer such a function)

kind of overkill but should be fast enough on modern machines.