Allow hooks to be provided in separate gems #498
Conversation
When you have several repositories, and a set of common hooks, it is very complex to update all the hooks in each repository. With this change, "built in" hooks are searched in the whole load path, so the user can put one or more hooks in a gem and manage them consistently
sds
left a comment
sds
left a comment
There was a problem hiding this comment.
Thanks for the PR, @saverio-kantox.
I absolutely agree that Overcommit should make it easy to share hooks via gems. Unfortunately, the implementation here bypasses some security protections, so I'd like to see this solved in a more comprehensive way.
| @@ -1,5 +1,6 @@ | |||
| require 'digest' | |||
| require 'json' | |||
| require 'English' | |||
There was a problem hiding this comment.
Are you sure this is necessary? I'm able to access $LOAD_PATH in an IRB session without require 'English'.
There was a problem hiding this comment.
You probably have English loaded by IRB itself. I have been bitten by this previously, so now I require English whenever I use english global constants.
|
|
||
| File.exist?(File.join(Overcommit::HOME, 'lib', 'overcommit', 'hook', | ||
| hook_context.hook_type_name, "#{hook_name}.rb")) | ||
| $LOAD_PATH.any? do |dir| |
There was a problem hiding this comment.
I'm wary of this approach because it bypasses the standard mechanism for loading hook plugins, which ensures the source code of the plugins is properly signed.
By allowing code loading this way, an attacker can easily inject malicious code by simply adding a gem to the Gemfile specified in the .overcommit.yml gemfile option. We need to have a more explicit way of specifying which code is loaded so that Overcommit can properly sign the hooks to protect against this attack.
|
As mentioned in previous comments, we're definitely open to this feature, but we'd like an implementation that doesn't bypass the security provided by hook signing. Feel free to open another PR. Thanks! |
2 participants
When you have several repositories, and a set of common hooks, it is very complex to update all the hooks in each repository. With this change, "built in" hooks are searched in the whole load path, so the user can put one or more hooks in a gem and manage them consistently