Change repo update hook behaviour
git-bruh opened this issue · 13 comments
[ ! -x update ] || {
log "$PWD" "Running update hook"
./update
}
I suggest that the update hook in repos should just be cat
-ed (these "hooks" aren't used much, and they're mostly to deliver news like is being done in repo-community
) instead of executed because if you use an unofficial repo, the maintainer can just put something like ssu rm -rf /*
(Very unlikely to happen) in update
and mess up a few people's systems. This is different from post-install
because you can inspect those scripts manually, but if you just run kiss u
then you won't be able to check the update
file beforehand.
What do you think @dilyn-corner ?
I'm uncertain how much I like this in general, but if it is kept there are a couple things I'd want from it...
- don't exec, just cat (as you've suggested). Easy change, it's like a post-install hook at that point.
- only print when the file itself gets updated - no need to reprint it on every
kiss u
.
I'll think about whether to keep it and in the meantime probably just implement your suggested change!
I added the same comment on the commit itself, but I will re-add here so it doesn't get lost in the commit d1287cb#commitcomment-47679816
I didn't see the issue tracker before adding the comment, but I also think that catting is the better option. Repositories themselves should never run code on your behalf, that is bad repository management. In my opinion, repository update hooks should solely exist so that they relay a message.
post-install
scripts on the other hand exist for the package you explicitly install. If you are installing a package from the repository, you are explicitly giving the permission to run those commands unlike update hooks.
I think the custom update script is a valuable mechanism for some cases, even if I only use it for minor clever things in not-very-important ways.
I propose that a new post-update
file should be added to the package spec, which will just be a text message like post-install
. And public repo creators should be discouraged from using update
unless strictly neccesary (e.g. other VCS than git). It will prevent cases of accidental mishap, and people would explicitly pay attention when it is actually there.
Preserves backwards compatibility too, even if that doesn't exactly matter much here.
To elaborate on my comment above, I don't think this is a security issue in a meaningful sense. If we're talking malice, then there's many ways to do bad stuff if the user doesn't do their job of inspection, the build scripts are executables after all. The only problem to solve here are IMO accidents and maybe conceptual simplification.
If it's very rarely used, since the primary responsibility it currently fulfills would be shifted to post-update
, I think it would be fair to put the burden of inspecting it when it is actually used on the user.
For instance, I added pre-fetch and post-fetch hooks to CPT back at May 2020 (which I am planning on altering the functionality). By this, I mean user hooks and not scripts owned by repositories. In my opinion, THOSE should be used instead, simply because I only see value out of personal configurations interacting with repository updates.
Perhaps the method used for post-install could also be used to achieve what you are saying, which is, run it if it is an executable, print it if it is a normal file. This limits the complexity and confusion, and also preserves backwards compatibility.
Both of those sound like pretty neat alternatives to me, I personally prefer the second one more but both are reasonable. I didn't actually know post-install
worked that way.
you don't even get to know the contents of the script until the script is finished/failed.
I actually didn't understand what you meant here, so I looked at the code and did some tests and realised that my understanding of update
's usage was actually pretty wrong, haha. I didn't know that the hook was run after a git pull
too. Because of the limited situations in which I have been using it (simple non-git repositories), I thought it was supposed to conceptually operate as an alternative to kiss
running git pull
, that if it exists, kiss
would not run pull itself at all and it was the responsibility of the script to fetch any updates.
So to clarify, I would say that currently update
theoretically serves two separate conceptual functions:
-
Running code/displaying messages after a pull made by
kiss
-
Actually doing the update procedure itself, whether it be calling another VCS, or retrieving raw data itself, etc.
I simply can't think of a good reason why a repository maintainer would need to run a code on someone's computer.
I completely agree, for when the repository is managed by git itself it serves no purpose. The first function underlined above should be trimmed down to just displaying messages.
The second function, however, I find valuable, in cases like when the repository is hosted on a separate VCS, the script can call the correct command itself. Or if it is a git repository but has a... peculiar relationship with remote branches, instead of pulling and tripping on an error it would fetch and merge/rebase in whatever way it wants to.
So I guess what I'm proposing is... splitting the current functions of update
to two different files. One with a simple messaging system shown after an update, and another with a hook to be run to receive updates instead of kiss doing its hardcoded update procedure.
Aside from the ones I gave about custom git workflows or other VCSs, I guess an interesting one I made was a joke repository that harvested firmware and kernel packages from other distros. Like, the script would retrieve the latest kernel firmware package from arch, checksum it and create a versioned kiss package. The update
script was essentially acting as the repository maintainer itself, which was executed on a kiss u
. It was pretty crazy stuff.
As I did say, it's not that personally important to me that this is preserved, because I can do the same thing I'm doing now in other ways too. But I think this is a useful and convenient gimmick for some cases.
Rough patch ideas:
https://github.com/FriendlyNeighborhoodShane/kiss/commit/7b91f97d7856037a7796a1bac516fe25ec3da220
https://github.com/FriendlyNeighborhoodShane/kiss/commit/d06d78fa66bb3c169e658a9cde471958e858939b
The names of update-hook
and post-update
are arbitrary, of course. I'd have preferred to keep the hook as just update
, but with that any users using repositories with that hook will find themselves never getting an update.
Lots of interesting discussion happening around this.
@FriendlyNeighborhoodShane those two commits are very similar to the idea I had and what I was working on before I dropped the update
idea entirely - indeed I ran into the issue you specify in that second commit. I mostly just didn't want to deal with it.
Ultimately, my thinking is this:
-
The way we run updates via
kiss u
is a straightforward and simple way, but does not easily allow users to vet changes to the repository. Therefore, nothing outside of the minimum required to actually update the repo should be done. Specifically, no code should ever be ran by package manager that doesn't directly facilitate updating the repositories, and no code should ever be ran that the user has not had at least an opportunity to inspect. Therefore, any filekiss
sees that resembles anupdate-hook
should only ever be printed. -
If a repository updates in such a way that it results in breaking changes, the user has to be notified in some way. I feel like the huge number of filenames, patches, + and - that scroll across stdin when the repository gets updated telegraphs this fine, and the user will find out if their packages have moved soon enough, yes? But it shouldn't happen often if ever (in my mind, repository structure should be stable. Otherwise, it isn't much of a repository), so this isn't a problem for
kiss
to solve. -
If a repository updates in such a way that user intervention is required on a package level, then there's no need to tell every user of the repository about this update - this is part of what
post-install
is for, after all. Further, if I need to tell users something, bumpingbaselayout
and addingpost-install
is the best way. It's a package (most) everyone has, requires no extra files in the repository, and requires no extra functionality.
Ultimately, if repository maintainers need to print things to stdin to tell users about horrible things, something horrible has happened. I feel like this adds a crutch we don't want.
I've been thinking about this issue a lot since it was raised, but I can't come up with an instance (that would happen in repo
or community
) where this would be useful. Arch has a similar feature (except you have to shudder go to their website) to inform users about what they broke. But user intervention is a thing we don't want to encourage. I think this is best relegated to the thing we use that already talks to users: post-install
.
Tangentially, kiss
supporting different VCS is interesting and something I'm looking into, but it's unclear to me how much it matters - git
probably outnumbers the sum total of fossil
, mercurial
et al. 100:1. I only know of two users who have fossil
packaged, and one of them is me!
I agree with about all of the points made by @dilyn-corner about it. Though I'm not a repository maintainer, so I'll leave opinions on whether they need a per-repo messaging system to those who are.
Ah, but just so we're clear on this, having an update-hook
from the first patch was not going to lead to situations where unexaminable code would be run. Since kiss would not be running pull on those repositories, the script to be run would always be the one that the user could have looked at before running a kiss update
.
Again, I have to express, being the only person in the thread in defence of the hook concept, that literally the only reason I have used it for is meaningless wacky fun shit. I am a git-lover (though "git but meaner" Fossil is pretty neat in its own regards) and not the maintainer of anything (technically except a practically-dead package in kiss). If the kiss ecosystem is better off with this complexity removed, then that's what's to be done. If I want to do that meaningless wacky fun shit again, I'll simply copy-paste the function from my patch into a kiss extension.
Oh of course not, haha. Didn't interpret your comments that way. Just thought it's kinda important to put my stances on the issue in perspective of how much practicalities they're based on, some just self-evaluation ;P
To put things another way, my only intention here was to add nuance to the case for an update hook, not to add (significant) weight to it.