search

rycus86 / githooks

Githooks: per-repo and global Git hooks with version control
MIT License
382 stars 20 forks source link

Allow urls and local paths in shared hooks #119

Closed gabyx closed 4 years ago

gabyx commented 4 years ago

See #82. Not testet, first implementation.

Comments highly welcome.

gabyx commented 4 years ago

We want strict defined behavior, and thats why I go with these rules:

For clonable urls, the following protcols are allowed (https?|file|ssh|ftp|git|file)://

For local paths

Windows is not a concern, since everything is wrapped through bash.exe anyways and it works :-)! Paths should be with forward slashes (and anything otherwise we define as unsupported, it might work however...), we do it as git does it...

rycus86 commented 4 years ago

Is this code shared with the in-repo shared hooks file handling? I'd like to make sure that I can't commit a shared hook config into a repo we both use that runs something malicious from your machine's local file system. Not sure yet how I would exploit it, but let's make sure we can't. Thanks!

gabyx commented 4 years ago

Ah jeah, your are right :-) may be we should only consider global shared hooks. I did not check that yet :-) Thx!

gabyx commented 4 years ago

Is this code shared with the in-repo shared hooks file handling? I'd like to make sure that I can't commit a shared hook config into a repo we both use that runs something malicious from your machine's local file system. Not sure yet how I would exploit it, but let's make sure we can't. Thanks!

May be you can exploit it by simply putting . as a url. Meaning we execute the hooks in this repo again -> endless recursion lool, dont know...

rycus86 commented 4 years ago

New logic seems OK to me. 👍 Can you add some tests please?

gabyx commented 4 years ago

Jeah, will do that later!

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 770

Changes Missing Coverage Covered Lines Changed/Added Lines %
base-template.sh 86 94 91.49%
cli.sh 73 91 80.22%
install.sh 13 48 27.08%
<!-- Total: 172 233 73.82% -->
Files with Coverage Reduction New Missed Lines %
install.sh 1 72.71%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 751: -0.9%
Covered Lines: 2281
Relevant Lines: 2787
💛 - Coveralls
gabyx commented 4 years ago

Added tests and finished for review. Seems to work fine.

gabyx commented 4 years ago

Note: I changed the name how clones shared repos are stored in .githooks/shared/xxx I needed this, since I think its good to have the full URL there... full distinguising between

rycus86 commented 4 years ago

Note: I changed the name how clones shared repos are stored in .githooks/shared/xxx I needed this, since I think its good to have the full URL there... full distinguising between

Can you please put it here an example of absolute paths these would become in the checked out shared repo on the filesystem? Without normalization can't they contain characters that are invalid on some OSes, like Windows and : for example?

rycus86 commented 4 years ago

FYI there seem to be test failures but the Travis integration keeps breaking and doesn't show up here anymore. 😔 https://travis-ci.org/github/rycus86/githooks

gabyx commented 4 years ago

Can you please put it here an example of absolute paths these would become in the checked out shared repo on the filesystem? Without normalization can't they contain characters that are invalid on some OSes, like Windows and : for example?

Correct, we replace all special characters as you did before!

gabyx commented 4 years ago

I will check the tests later.

rycus86 commented 4 years ago

Can you please put it here an example of absolute paths these would become in the checked out shared repo on the filesystem? Without normalization can't they contain characters that are invalid on some OSes, like Windows and : for example?

Correct, we replace all special characters as you did before!

Excellent, that should work then hopefully. Could you perhaps add a test to showcase this please? (if not too difficult)

gabyx commented 4 years ago

@rycus86 : We have another covarage kcov to-old-git issue -> good news: i think I finally fixed kcov v36 :-) See next commit:

With that fixes in coverage.sh it runs through and I report towards "~/.githooks/release".

There are some ugly thing we should fix sometime:

RUN useradd -ms /bin/bash coverage
RUN chown -R coverage:coverage /var /usr/share/git-core

because we have tests which assume we are running as root... hm... what ever...

rycus86 commented 4 years ago

@rycus86 : We have another covarage kcov to-old-git issue -> good news: i think I finally fixed kcov v36 :-) See next commit:

  • it needs a user in the docker container
  • I (for now, other solution?) added the kvov run directly into the image building
  • copy the output from the image

Ooh, exciting! I'll have a look later today. Thanks!

gabyx commented 4 years ago

With that fixes in coverage.sh it runs through and I report towards "~/.githooks/release".

Not sure, but in any case if we use var/lib/githooks all release clone calls in ~/.githooks/release are anyway not counted, aren't they? Not sure but I dont think kcov has support for merging same file names? --merge ? Hm... So one should maybe rewrite the tests by having the direct wrapper calls (not install if you really dont want it) but just place them in the same directory :-) -> ~/.githooks/release/... at the beginngin of the test. Or hardlinking towards /var/lib/githooks/.... might also be a nice option...

rycus86 commented 4 years ago

With that fixes in coverage.sh it runs through and I report towards "~/.githooks/release".

Not sure, but in any case if we use var/lib/githooks all release clone calls in ~/.githooks/release are anyway not counted, aren't they? Not sure but I dont think kcov has support for merging same file names? --merge ? Hm... So one should maybe rewrite the tests by having the direct wrapper calls (not install if you really dont want it) but just place them in the same directory :-) -> ~/.githooks/release/... at the beginngin of the test. Or hardlinking towards /var/lib/githooks/.... might also be a nice option...

https://coveralls.io/builds/33289402 seems to be fine for me, is there anything missing from it? Looks like cli.sh is not there, not sure if that's related to this change? Otherwise I wouldn't be worried where we get the data from as long as it's there.

gabyx commented 4 years ago

https://coveralls.io/builds/33289402 seems to be fine for me, is there anything missing from it? Looks like cli.sh is not there, not sure if that's related to this change? Otherwise I wouldn't be worried where we get the data from as long as it's there.

Hm... thats utter strange, why is cli.sh not there? I though we dispatch to the release folder for the cli as well ? Hmm have to check that

gabyx commented 4 years ago

Do you know how kcov can track invocations through git commit which triggers base-template.sh. Isn't that whats happening now? This is magic?

gabyx commented 4 years ago

Its really sad:

So in the last commit I refactored the tests a bit.

All tests with direct template execution:

All tests which use an install:

image

gabyx commented 4 years ago

On the way of improving the coverage a bit more: Need some more replacements :-)

gabyx commented 4 years ago

FYI: the coverage dropped, because there is something wrong, it does nto include for example

echo "
asdasd
asd
asd
"

Thats why the rating dropped. May be we can reformat (sed) these lines into separate echo just for coverage... Kcov and Bash sucks, Bash has this PS4 security fixes and probably other issues which kcov is not aware. I try with latest kcov/kcov:latest later... I am guessing if there is a better coverage tool hm...

rycus86 commented 4 years ago

If you find anything else that can do Shell coverage, I'm happy to give it a try. When I set up the project, this was the one I found working.

It might be good if you could put up a separate PR just for the kcov upgrade but no other functionality change, then we'd get that into master and we'd get a more relevant picture of coverage changes in this (and future) PRs.

Would that be too much work? Tell me if it is, but hoping now that you know how to get it working, it's easy to redo on top of master.

gabyx commented 4 years ago

I can do that, its a bit work, but may be yes, better.

gabyx commented 4 years ago

By the way: echo replacements for kcov

echo "asdasd
asdasd
echo \"
line 1
line 2
\"" | sed -E '/echo "$/,/^"/{  s/echo "/echo ""/ ; s/^"$/echo ""/  ;  /"/! { s/(.*)/echo "\1"/ } }'
rycus86 commented 4 years ago

I can do that, its a bit work, but may be yes, better.

Let me know if it too much, then I can try to extract it from this PR if I ever find some time. 🙂

rycus86 commented 4 years ago

By the way: echo replacements for kcov

echo "asdasd
asdasd
echo \"
line 1
line 2
\"" | sed -E '/echo "$/,/^"/{  s/echo "/echo ""/ ; s/^"$/echo ""/  ;  /"/! { s/(.*)/echo "\1"/ } }'

That messes with the line numbers though, right? Maybe not a huge issue, but also lower coverage is fine if we know it's just due to echos and not because of genuinely uncovered lines.

gabyx commented 4 years ago

No I think, it want:

asdasd
asdasd
echo ""
echo "line 1"
echo "line 2"
echo ""
gabyx commented 4 years ago

No coverage stuff inside, this PR. Coverage will fail on travis.

gabyx commented 4 years ago

Here, sorry for the long mess about the Coverage above, again the PR with squased commits.

Todos: From the discussion:

gabyx commented 4 years ago

@matthijskooijman Supported is the following: See https://github.com/rycus86/githooks/pull/119#issuecomment-687400696

gabyx commented 4 years ago
gabyx commented 4 years ago

Can we discuss the missing point

That should classify as local path as well I think... In that way all tests will probably now fail. Because we can't add file:///tmp/shared to the local shared hooks. And how should we test this...

So up to now file:// is treated as a normal url and it is cloned and can be checked in (local shared hooks) to .githooks/.shared

gabyx commented 4 years ago

And that is already interactively verified based on checksums, so I wonder whether this security check needs to exist at all? If we're worried about cloning from local paths as well as executing hooks, I wonder if we should be worried about cloning from remote urls and maybe ask confirmation before any clone?

I totally agree, I was thinking the same when implementing this: Maybe we should only do this:

gabyx commented 4 years ago

The new changes include all of the following: test-117.sh is especially for local stuff...

We mock certain tests with git config --global githooks.testingTreatFileProtocolAsRemote "true" to pass gracefully with file:// simulating a remote url. Test 117 doesn't have this and checks if rejection properly works,

gabyx commented 4 years ago

I think we need an option to allow for all type of urls/paths in local shared hooks. Scenario for hooks in repos on a server (gitea for example) is that you probably really want to setup local urls/paths in

So I suggest an option to allow this as opt-in:

git config githooks.allowLocalPathsInLocalSharedHooks

We can set that --global or per repo wise, passing through the rejection.

matthijskooijman commented 4 years ago

IIUC, you're saying that you might want to do this in the .githooks directory of a bare repository, right?

If so, then maybe the "global/local" distinction is not accurate? From a security standpoint we feel (I'm not 100% convinced, but I'll roll with this assumption for now) that you should not be trusting contents that is stored in the repository itself. Iow, anything that can be changed by a push to the repo, should be potentially untrusted and thus not be allowed to refer to a local path. Aside from trust issues, it would not actually make sense to let the repository contents refer to a local path, since there isn't really any way to guarantee such a local path would be available on every machine that potentially has a clone of this repo.

However, from this perspective, a .githooks directory in a bare repository, which AFAICS would be manually created and managed by the server administrator and cannot be accessed through git itself, should be in the same "trusted" category as global configuration (and in a different category than .githooks in a non-bare repository, which can be accessed through git itself).

gabyx commented 4 years ago

@matthijskooijman: totally correct. Because this .githooks folder is manually managed on the server and cannot be changed thats why I simply introduced a opt in githooks.allowLocalPathsInLocalSharedHooks.

p.s: I think we cannot distinguish really between a server setup or not. Meaning we are dealing with a manually managed bare repo on the server.....

matthijskooijman commented 4 years ago

p.s: I think we cannot distinguish really between a server setup or not. Meaning we are dealing with a manually managed bare repo on the server.....

Why not? Isn't the characteristic to check "Git repo is bare or not"? If it is bare, the contents of the directory is not managed by git, if it is non-bare, the contents is managed by (and controllable through) git?

gabyx commented 4 years ago

Hm.. That migth be it. I was not sure, about. Whats about bare repos on your machine... Hm doesn't matter. so I think we can maybe get away with the new config and just pass the rejection when its a bare repo...

rycus86 commented 4 years ago

I think we need an option to allow for all type of urls/paths in local shared hooks. Scenario for hooks in repos on a server (gitea for example) is that you probably really want to setup local urls/paths in

  • server-repo/.githooks/.shared

So I suggest an option to allow this as opt-in:

git config githooks.allowLocalPathsInLocalSharedHooks

We can set that --global or per repo wise, passing through the rejection.

If this is an admin managed server, can't you just set up global hooks with the cli pointing to those shared hooks, rather than using a .shared file that is checked into the repo? I'm trying to understand why do we need this workaround, and I feel like I'm missing something.

gabyx commented 4 years ago

That is possible for sure, but it would be good to have control over which repos habe which shared hooks. So local shared hooks are the way to go, which work already in bare repos.

Von meinem iPhone gesendet

Am 11.09.2020 um 12:56 schrieb Viktor Adam notifications@github.com:

I think we need an option to allow for all type of urls/paths in local shared hooks. Scenario for hooks in repos on a server (gitea for example) is that you probably really want to setup local urls/paths in

server-repo/.githooks/.shared So I suggest an option to allow this as opt-in:

git config githooks.allowLocalPathsInLocalSharedHooks We can set that --global or per repo wise, passing through the rejection.

If this is an admin managed server, can't you just set up global hooks with the cli pointing to those shared hooks, rather than using a .shared file that is checked into the repo? I'm trying to understand why do we need this workaround, and I feel like I'm missing something.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

rycus86 commented 4 years ago

Ah right, I thought we have a "set shared repos for this repository only" command, but I see now that only exists for the global setting. Would it work if we could set that config per repo too (not only globally)? I'd prefer that over checking in a .shared file pointing to local paths that really only make sense on the server in your use case (if I understand it correctly) and that way we could avoid this new feature toggle.

What do you think?

gabyx commented 4 years ago

hi rycus, Ah this exist? :) I never seen this: Can you point me at a line :). You can set global shared hooks for certain repos only? I thought we check always —global on githooks.shared?

Von meinem iPhone gesendet

Am 12.09.2020 um 12:29 schrieb Viktor Adam notifications@github.com:

Ah right, I thought we have a "set shared repos for this repository only" command, but I see now that only exists for the global setting. Would it work if we could set that config per repo too (not only globally)? I'd prefer that over checking in a .shared file pointing to local paths that really only make sense on the server in your use case (if I understand it correctly) and that way we could avoid this new feature toggle.

What do you think?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

rycus86 commented 4 years ago

Yeah, it might be global only right now, I'm wondering if we can also make it local for things like what you want it for.

gabyx commented 4 years ago

I think instead of checking —global we can easily just leave it out and support that feature I want, that way. Thx @rycus. That option is exactly what I want. So we can forget to expose the allowLocalPathsFor... option, and I need to introduce the sed replacement again to pass file:// in certain tests. So we can do that in seperate PR :)!

Von meinem iPhone gesendet

Am 12.09.2020 um 22:32 schrieb Viktor Adam notifications@github.com:

Yeah, it might be global only right now, I'm wondering if we can also make it local for things like what you want it for.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

gabyx commented 4 years ago

I reverted the option change:

If tests pases, this shoudld be ready to final review and merge.

gabyx commented 4 years ago

@rycus86 :

Can we sooner or later merge this, would be great.

gabyx commented 4 years ago

Closed in Favor of #125 which contains these changes.