Please never implicitly accept a trust prompt

[matthijskooijman]

The README says:

There is a caveat worth mentioning: if a terminal (tty) can't be allocated, then the default action is to accept the changes or new hooks. Let me know in an issue if you strongly disagree, and you think this is a big enough risk worth having slightly worse UX instead.

I strongly disagree :-)

Because hooks can run arbitrary code, this is a significant security issue and should never be done without explicit consent. If a value cannot be read from the user, the request should be rejected IMHO. The script could maybe print a command you could run to explicitly allow the hooks in the current dir (which seems to be just githooks accept) and maybe sleep for a bit to allow aborting if there is a user looking, but then continue without the hook.

Currently, the prompt also shows Y as the default (i.e. what would be used if the user pressed "enter" without entering anything). I would also suggest (but less strongly) to change that and require explicit consent from the user. Pressing enter just a bit too soon is easy to do, and can have security implications here. It would be good to be explicit about this: When a user presses enter directly, show the prompt again and ask for an explicit answer. This needs distinguishing an "empty response" from "no response", but I think the read exit status should allow this (might need some fiddling in show_prompt() to actually return the right value, but maybe it already works because read is the last command, see also https://github.com/rycus86/githooks/pull/81#issuecomment-614782632).

At the very least this should be configurable, and I also strongly suggest making the default the secure option. If this is annoying for users and they trust their repository, they can always use the "trust" option to get rid of the annoying prompts.

[gabyx]

I agree. :-). See #85 : The thing is if we strong need a user input from

 MESSAGE="$(printf "%s\n%s" "$MESSAGE: $HOOK_PATH" "  Do you accept the changes?")"
            show_prompt ACCEPT_CHANGES "? $MESSAGE" "(Yes, all, no, disable)" "Y/a/n/d" "Yes" "All" "No" "Disable"

what should we do if ACCEPT_CHANGES is empty (e.g we have no controlling terminal meaning)

• As you suggested we just print an error message to the user

  New detected hooks need to be accepted, but could not ask for a prompt. 
  Either enable trusting hooks with `git hooks trust` or run the git command from a terminal.
  Aborting due to errors!

  the stderr and exit immediately with 1

[matthijskooijman]

what should we do if ACCEPT_CHANGES is empty (e.g we have no controlling terminal meaning) My suggestion would be to look at the exit status of show_prompt to detect an error vs an empty reply. show_prompt could probably just return the exit status of read directly (I think it might already because the read is the last command in show_prompt), or it could add another layer on top (e.g. it could return 1 when the output is not a tty, and 2 when another error occured, which would allow the caller to update its output messages accordingly.

the stderr and exit immediately with 1

I'm not sure if that would make sense, since the meaning of the exit status is hook-dependent (I suspect that most if not all of them just check for 0 vs non-0), but in some cases (e.g. post-commit) you're too late to prevent anything of course. Just assuming "no" and continuing might be safer (from an arbitrary-execution-standpoint at least), though that might also end up doing things that you wanted to prevent in your hooks (so exiting with error might be safer in that aspect).

To prevent unattended (e.g. CI build) commands from failing, you could either make sure to accept or disable all hooks needed in advance, or some option to control this behaviour (e.g. --assume-yes) might be useful as well.

[rycus86]

Thanks for raising this issue! While I think you'll probably want to (mostly) work on repositories you trust, this is probably not super critical, but I agree if you try hard enough perhaps other could be tricked into pulling your malicious hooks, so I'm happy to discuss changing the default.

For the UX side, I think it's nice and convenient to have Yes as the default option, perhaps we could try to see if there's tty attached and only make Yes the default then, or if unsure then default to no. I hear your concern about fat-fingering an Enter at the prompt, but not convinced this would be super frequent that it needs the worse UX.

[gabyx]

Default accepting should probably be disabled by default. And an opt-in feature. Especially in the new implementation. https://github.com/gabyx/githooks So any Git GUI will fail in the background because no prompt can be shown, we will however hopefully have a default platform independent Prompt Dialog if no Terminal is attached, which resolves this issue once and for all...

[gabyx]

FYI: This is now implemented in the new version. See https://github.com/gabyx/githooks#user-prompts