[PATCH] t/lib-credential: clean additional credential

[PATCH] t/lib-credential: clean additional credential

[Bo Anderson via GitGitGadget]

From: Bo Anderson <mail@boanderson.me>

71201ab0e5 (t/lib-credential.sh: ensure credential helpers handle long
headers, 2023-05-01) added a test which stores credentials with the host
victim.example.com but this was never cleaned up, leaving residual data
in the credential store after running the tests.

Add a cleanup call for this credential to resolve this issue.

Signed-off-by: Bo Anderson <mail@boanderson.me>
---
    t/lib-credential: clean additional credential

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1664%2FBo98%2Ft-credential-missing-clean-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1664/Bo98/t-credential-missing-clean-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1664

 t/lib-credential.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index 15fc9a31e2c..44799c0d38f 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -50,6 +50,7 @@ helper_test_clean() {
 	reject $1 https example.com user-overwrite
 	reject $1 https example.com user-erase1
 	reject $1 https example.com user-erase2
+	reject $1 https victim.example.com user
 	reject $1 http path.tld user
 	reject $1 https timeout.tld user
 	reject $1 https sso.tld

base-commit: efb050becb6bc703f76382e1f1b6273100e6ace3
-- 
gitgitgadget

Re: [PATCH] t/lib-credential: clean additional credential

[Jeff King]

On Thu, Feb 15, 2024 at 01:03:56AM +0000, Bo Anderson via GitGitGadget wrote:

> From: Bo Anderson <mail@boanderson.me>
> 
> 71201ab0e5 (t/lib-credential.sh: ensure credential helpers handle long
> headers, 2023-05-01) added a test which stores credentials with the host
> victim.example.com but this was never cleaned up, leaving residual data
> in the credential store after running the tests.
> 
> Add a cleanup call for this credential to resolve this issue.

Good catch. The patch looks obviously correct.

I'm not surprised nobody noticed until now, as I expect it is pretty
rare for people to run t0303 against system helpers (it is not a problem
for t0301, etc, because they only touch the internal trash directory).

I wonder if we might want something like this, as well, which can catch
leftovers:

diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index 716bf1af9f..4183154243 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -6,6 +6,11 @@ test_description='credential-store tests'
 
 helper_test store
 
+helper_test_clean store
+test_expect_success 'test cleanup removes everything' '
+	test_must_be_empty "$HOME/.git-credentials"
+'
+
 test_expect_success 'when xdg file does not exist, xdg file not created' '
 	test_path_is_missing "$HOME/.config/git/credentials" &&
 	test -s "$HOME/.git-credentials"

-Peff

Re: [PATCH] t/lib-credential: clean additional credential

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Feb 15, 2024 at 01:03:56AM +0000, Bo Anderson via GitGitGadget wrote:
>
>> From: Bo Anderson <mail@boanderson.me>
>> 
>> 71201ab0e5 (t/lib-credential.sh: ensure credential helpers handle long
>> headers, 2023-05-01) added a test which stores credentials with the host
>> victim.example.com but this was never cleaned up, leaving residual data
>> in the credential store after running the tests.
>> 
>> Add a cleanup call for this credential to resolve this issue.
>
> Good catch. The patch looks obviously correct.
>
> I'm not surprised nobody noticed until now, as I expect it is pretty
> rare for people to run t0303 against system helpers (it is not a problem
> for t0301, etc, because they only touch the internal trash directory).
>
> I wonder if we might want something like this, as well, which can catch
> leftovers:

Sounds like a good hygiene ;-).

>
> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index 716bf1af9f..4183154243 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -6,6 +6,11 @@ test_description='credential-store tests'
>  
>  helper_test store
>  
> +helper_test_clean store
> +test_expect_success 'test cleanup removes everything' '
> +	test_must_be_empty "$HOME/.git-credentials"
> +'
> +
>  test_expect_success 'when xdg file does not exist, xdg file not created' '
>  	test_path_is_missing "$HOME/.config/git/credentials" &&
>  	test -s "$HOME/.git-credentials"
>
> -Peff

[PATCH] t0303: check that helper_test_clean removes all credentials

[Jeff King]

On Thu, Feb 15, 2024 at 09:22:27AM -0800, Junio C Hamano wrote:

> > I wonder if we might want something like this, as well, which can catch
> > leftovers:
> 
> Sounds like a good hygiene ;-).

Unfortunately, it is not quite so simple. There are a bunch of other
tests run by t0303 that are not in t0302, because credential-store does
not support caching, expiration, etc.

Putting it in t0301 would be better, but we don't have a way to inspect
the internal state of the cache daemon. We could perhaps add one, but
here's an even hackier solution. ;)

-- >8 --
Subject: [PATCH] t0303: check that helper_test_clean removes all credentials

Our lib-credential.sh library comes with a "clean" function that removes
all of the credentials used in its tests (to avoid leaving cruft in
system credential storage). But it's easy to add a test that uses a new
credential but forget to add it to the clean function.  E.g., the case
fixed by 83e6eb7d7a (t/lib-credential: clean additional credential,
2024-02-15).

We should be able to catch this automatically, but it's a little tricky.

We can't just compare the contents of the helper's storage before and
after the test run, because there isn't a way to ask a helper to dump
all of its storage. And in most cases we don't have direct access to the
underlying storage (since the whole point of the helper is to abstract
that away). We can work around that by using our own "store" helper,
since we can directly inspect its state by looking at its on-disk file.

But there's a catch: the "store" helper doesn't support features like
caching or expiration, so using it naively fails tests (and skipping
those tests would give us incomplete coverage). Implementing all of
those features would be non-trivial. But we can hack around that by
overriding the "check" function used by the tests to turn most requests
into noop success (except for "approve" requests, which actually store
things).

And then at the end we can check that running the "clean" function takes
us back to an empty state.

Note that because we've skipped any tests that erase credentials
(because of our noop check function), the state we see at cleanup time
may be larger than it would be normally. That's OK. The point of the
clean function is to clean up any cruft we _might_ have left in place,
so we're just being doubly thorough.

The way this is bolted onto t0303 feels a little messy. But it's really
the best place to do it, because then we know that it is running the
exact sequence of tests that we'd use for testing a real external
helper. In a normal run of "make test" it currently does nothing (the
idea is that you run it manually after pointing it at some helper
program). But now with this patch, "make test" will sanity-check the
script itself.

Signed-off-by: Jeff King <peff@peff.net>
---
This should be applied on top of ba/credential-test-clean-fix,
naturally, or it will fail. :)

 t/t0303-credential-external.sh | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index 095574bfc6..72ae405c3e 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -32,9 +32,24 @@ commands.
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
+# If we're not given a specific external helper to run against,
+# there isn't much to test. But we can still run through our
+# battery of tests with a fake helper and check that the
+# test themselves are self-consistent and clean up after
+# themselves.
+#
+# We'll use the "store" helper, since we can easily inspect
+# its state by looking at the on-disk file. But since it doesn't
+# implement any caching or expiry logic, we'll cheat and override
+# the "check" function to just report all results as OK.
 if test -z "$GIT_TEST_CREDENTIAL_HELPER"; then
-	skip_all="used to test external credential helpers"
-	test_done
+	GIT_TEST_CREDENTIAL_HELPER=store
+	GIT_TEST_CREDENTIAL_HELPER_TIMEOUT=store
+	check () {
+		test "$1" = "approve" || return 0
+		git -c credential.helper=store credential approve
+	}
+	check_cleanup=t
 fi
 
 test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
@@ -59,4 +74,11 @@ fi
 # might be long-term system storage
 helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
 
+if test "$check_cleanup" = "t"
+then
+	test_expect_success 'test cleanup removes everything' '
+		test_must_be_empty "$HOME/.git-credentials"
+	'
+fi
+
 test_done
-- 
2.44.0.rc1.410.g8325d5f159