| Message ID | 20220427170649.4949-4-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 756d15923bf6806f94484054873e284728a89c4b |
| Headers | show |
| Series | t0033-safe-directory: test improvements and a documentation clarification | expand |
SZEDER Gábor <szeder.dev@gmail.com> writes: > The description of 'safe.directory' mentions that it's respected in > the system and global configs, and ignored in the repository config > and on the command line, but it doesn't mention whether it's respected > or ignored when specified via environment variables (nor does the > commit message adding 'safe.directory' [1]). If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like $PATH does), that would have been absolutely necessary to document how it works, but GIT_CONFIG_* is merely an implementation detail of how "git -c var=val" works and I am not sure if it is even a good idea to hardcode how they happen to work like these tests. The only thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are used internally by the implementation and they should not muck with it, no? I am moderately negative about this change. > diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh > index 82dac0eb93..238b25f91a 100755 > --- a/t/t0033-safe-directory.sh > +++ b/t/t0033-safe-directory.sh > @@ -21,6 +21,21 @@ test_expect_success 'ignoring safe.directory on the command line' ' > grep "unsafe repository" err > ' > > +test_expect_success 'ignoring safe.directory in the environment' ' > + test_must_fail env GIT_CONFIG_COUNT=1 \ > + GIT_CONFIG_KEY_0="safe.directory" \ > + GIT_CONFIG_VALUE_0="$(pwd)" \ > + git status 2>err && > + grep "unsafe repository" err > +' > + > +test_expect_success 'ignoring safe.directory in GIT_CONFIG_PARAMETERS' ' > + test_must_fail env \ > + GIT_CONFIG_PARAMETERS="${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}" \ > + git status 2>err && > + grep "unsafe repository" err > +' > + > test_expect_success 'ignoring safe.directory in repo config' ' > ( > unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
Junio C Hamano <gitster@pobox.com> writes: > If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like > $PATH does), that would have been absolutely necessary to document > how it works, but GIT_CONFIG_* is merely an implementation detail of > how "git -c var=val" works and I am not sure if it is even a good > idea to hardcode how they happen to work like these tests. The only > thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are > used internally by the implementation and they should not muck with > it, no? I misremembered. GIT_CONFIG_COUNT and stuff are usable by end user scripts, but then ... > diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt > index 6d764fe0cc..ae0e2e3bdb 100644 > --- a/Documentation/config/safe.txt > +++ b/Documentation/config/safe.txt > @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a > `safe.directory` entry with an empty value. > + > This config setting is only respected when specified in a system or global > -config, not when it is specified in a repository config or via the command > -line option `-c safe.directory=<path>`. > +config, not when it is specified in a repository config, via the command > +line option `-c safe.directory=<path>`, or in environment variables. ... this part must clarify what environment variables it is talking about. ... via the command line option `-c safe.directory=<path>`, or via the GIT_CONFIG_{KEY,VALUE} mechanism. or something, perhaps. I actually do think it is a useful addition to have GIT_SAFE_DIRECTORIES environment variable that should NOT be ignored.
On 4/27/2022 4:49 PM, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like >> $PATH does), that would have been absolutely necessary to document >> how it works, but GIT_CONFIG_* is merely an implementation detail of >> how "git -c var=val" works and I am not sure if it is even a good >> idea to hardcode how they happen to work like these tests. The only >> thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are >> used internally by the implementation and they should not muck with >> it, no? > > I misremembered. GIT_CONFIG_COUNT and stuff are usable by end user > scripts, but then ... > >> diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt >> index 6d764fe0cc..ae0e2e3bdb 100644 >> --- a/Documentation/config/safe.txt >> +++ b/Documentation/config/safe.txt >> @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a >> `safe.directory` entry with an empty value. >> + >> This config setting is only respected when specified in a system or global >> -config, not when it is specified in a repository config or via the command >> -line option `-c safe.directory=<path>`. >> +config, not when it is specified in a repository config, via the command >> +line option `-c safe.directory=<path>`, or in environment variables. > > ... this part must clarify what environment variables it is talking > about. > > ... via the command line option `-c safe.directory=<path>`, or > via the GIT_CONFIG_{KEY,VALUE} mechanism. > > or something, perhaps. I actually do think it is a useful addition > to have GIT_SAFE_DIRECTORIES environment variable that should NOT be > ignored. I agree on both points. Thanks for working on this! -Stolee
On Wed, Apr 27, 2022 at 01:49:32PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like > > $PATH does), that would have been absolutely necessary to document > > how it works, but GIT_CONFIG_* is merely an implementation detail of > > how "git -c var=val" works and I am not sure if it is even a good > > idea to hardcode how they happen to work like these tests. The only > > thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are > > used internally by the implementation and they should not muck with > > it, no? > > I misremembered. GIT_CONFIG_COUNT and stuff are usable by end user > scripts, but then ... > > > diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt > > index 6d764fe0cc..ae0e2e3bdb 100644 > > --- a/Documentation/config/safe.txt > > +++ b/Documentation/config/safe.txt > > @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a > > `safe.directory` entry with an empty value. > > + > > This config setting is only respected when specified in a system or global > > -config, not when it is specified in a repository config or via the command > > -line option `-c safe.directory=<path>`. > > +config, not when it is specified in a repository config, via the command > > +line option `-c safe.directory=<path>`, or in environment variables. > > ... this part must clarify what environment variables it is talking > about. > > ... via the command line option `-c safe.directory=<path>`, or > via the GIT_CONFIG_{KEY,VALUE} mechanism. Well, the proposed phrasing covers all environment variables that affect the configuration. It doesn't feel right to explicitly list all those variables, including GIT_CONFIG_PARAMETERS, because that vairable is such an implementation detail that it isn't even mentioned elsewhere in the documentation at all. OTOH, listing only some of the ignored variables but omitting others doesn't feel quite right either, hence the general "in environment variables". > or something, perhaps. I actually do think it is a useful addition > to have GIT_SAFE_DIRECTORIES environment variable that should NOT be > ignored. Is it safe to have such an environment variable? So, it's quite clear why 'safe.directory' is ignored in the repository config: it can be under control of someone else, and thus a malicious repositoy could trivially safe-list itself. However, it's unclear (to me) why 'git -c safe.directory=...' is ignored: 8959555cee (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02) only states that it's ignored, but doesn't justify why. Now, let's suppose that there was a compelling reason to do so, e.g. in some way that I don't readily see it can be spoofed and thus could be abused by an attacker to safe-list a malicious repository. If that were indeed the case, then couldn't 'GIT_SAFE_DIRECTORIES=/path/... git cmd' could be spoofed and abused just as easily? Or to approach it from the opposite direction: if such a GIT_SAFE_DIRECTORIES environment variable is safe, then why should we ignore 'safe.directory' in the command line, or in the environment for that matter?
SZEDER Gábor <szeder.dev@gmail.com> writes: > repositoy could trivially safe-list itself. However, it's unclear (to > me) why 'git -c safe.directory=...' is ignored: 8959555cee > (setup_git_directory(): add an owner check for the top-level > directory, 2022-03-02) only states that it's ignored, but doesn't > justify why. Now, let's suppose that there was a compelling reason to Correct. I do not think it is a restriction with any sensible justification, just that it happened to be implemented that way. IOW, I am saying that GIT_SAFE_DIRECTORIES may be a lot nicer user interface than fixing the "we ignore 'git -c safe.directory'?" bug.
On Mon, May 09, 2022 at 02:45:21PM -0700, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > > > repositoy could trivially safe-list itself. However, it's unclear (to > > me) why 'git -c safe.directory=...' is ignored: 8959555cee > > (setup_git_directory(): add an owner check for the top-level > > directory, 2022-03-02) only states that it's ignored, but doesn't > > justify why. Now, let's suppose that there was a compelling reason to > > Correct. I do not think it is a restriction with any sensible > justification, just that it happened to be implemented that way. > > IOW, I am saying that GIT_SAFE_DIRECTORIES may be a lot nicer user > interface than fixing the "we ignore 'git -c safe.directory'?" bug. In light of this, I'm not sure we want to test said buggy behavior. IOW, do we want 2/3 and 3/3 of this patch series at all? Or perhaps add those test as 'test_expect_failure' instead?
diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt index 6d764fe0cc..ae0e2e3bdb 100644 --- a/Documentation/config/safe.txt +++ b/Documentation/config/safe.txt @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a `safe.directory` entry with an empty value. + This config setting is only respected when specified in a system or global -config, not when it is specified in a repository config or via the command -line option `-c safe.directory=<path>`. +config, not when it is specified in a repository config, via the command +line option `-c safe.directory=<path>`, or in environment variables. + The value of this setting is interpolated, i.e. `~/<path>` expands to a path relative to the home directory and `%(prefix)/<path>` expands to a diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh index 82dac0eb93..238b25f91a 100755 --- a/t/t0033-safe-directory.sh +++ b/t/t0033-safe-directory.sh @@ -21,6 +21,21 @@ test_expect_success 'ignoring safe.directory on the command line' ' grep "unsafe repository" err ' +test_expect_success 'ignoring safe.directory in the environment' ' + test_must_fail env GIT_CONFIG_COUNT=1 \ + GIT_CONFIG_KEY_0="safe.directory" \ + GIT_CONFIG_VALUE_0="$(pwd)" \ + git status 2>err && + grep "unsafe repository" err +' + +test_expect_success 'ignoring safe.directory in GIT_CONFIG_PARAMETERS' ' + test_must_fail env \ + GIT_CONFIG_PARAMETERS="${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}" \ + git status 2>err && + grep "unsafe repository" err +' + test_expect_success 'ignoring safe.directory in repo config' ' ( unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
The description of 'safe.directory' mentions that it's respected in the system and global configs, and ignored in the repository config and on the command line, but it doesn't mention whether it's respected or ignored when specified via environment variables (nor does the commit message adding 'safe.directory' [1]). Clarify that 'safe.directory' is ignored when specified in the environment, and add tests to make sure that it remains so. [1] 8959555cee (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02) Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- Documentation/config/safe.txt | 4 ++-- t/t0033-safe-directory.sh | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-)