Skip to content

Guard installs when hooks path is external#232

Open
nshkrdotcom wants to merge 2 commits into
qgadrian:masterfrom
nshkrdotcom:guard-external-hooks-path
Open

Guard installs when hooks path is external#232
nshkrdotcom wants to merge 2 commits into
qgadrian:masterfrom
nshkrdotcom:guard-external-hooks-path

Conversation

@nshkrdotcom

@nshkrdotcom nshkrdotcom commented Jan 4, 2026

Copy link
Copy Markdown

Summary

  • Skip installation when the resolved hooks path is outside the repo unless explicitly allowed.
  • Add allow_external_hooks_path config + GIT_HOOKS_ALLOW_EXTERNAL env override.
  • Add install tests for external hooks path + opt-in.
  • Keep tests warning-free by avoiding deprecated MFA.new/3 in core code and silencing git init hints.

Testing

  • mix test

@qgadrian

Copy link
Copy Markdown
Owner

Thanks @nshkrdotcom

The guard against an external hooksPath is worth having. But I have two asks before this can be merged:

  • Please drop all the MFA.new/from_config changes and rebase on master. PR Fix compilation warnings on Elixir 1.20 / OTP 29 #237 already removes that deprecation tag, so those changes conflict
  • Can we keep the install.ex change reviewable? extract the current body into a private do_install/1 and add the guard in front, rather than re-indenting the whole block.

Also: how does this behave in a git worktree/submodule, where --git-path hooks points at the main repo's .git/hooks Want to make sure we don't refuse installs there by default.

@nshkrdotcom
nshkrdotcom force-pushed the guard-external-hooks-path branch from e7d0657 to e0a354c Compare July 10, 2026 19:41
@nshkrdotcom

Copy link
Copy Markdown
Author

Thanks @qgadrian, it's updated.

I rebased on the latest master, dropped the MFA.new/from_config changes, and kept install.ex reviewable by putting the guard in install/1 and moving the existing install body into do_install/1.

Linked worktrees and submodules are allowed by default: the guard trusts hooks paths inside Git's common directory, while arbitrary external core.hooksPath values are still blocked unless explicitly enabled. I added regression coverage for both cases, plus real install/no-overwrite tests and both opt-in methods.

Everything passes locally on Elixir 1.20.2 / OTP 29: 55 tests, formatting, strict Credo, warnings-as-errors compilation, and Dialyzer. The GitHub Actions run is waiting for your approval.

I haven't put this through the paces, so it will certainly need your detailed review. The implementation itself was done with gpt-5.6-sol on xhigh per my guidance.

@qgadrian

Copy link
Copy Markdown
Owner

@nshkrdotcom I would appreciate if you can resolve the conflicts on this PR 🙏

Also, it is not enforced yet at project level, but a commit message stating the model/effort used would also be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants