t/lib-credential: clean additional credential

Commit Message

Bo Anderson Feb. 15, 2024, 1:03 a.m. UTC

From: Bo Anderson <[e-mail]>

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 <[e-mail]>
---
    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(+)


base-commit: efb050becb6bc703f76382e1f1b6273100e6ace3

Comments

Jeff King Feb. 15, 2024, 4:39 a.m. UTC | #1

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

> From: Bo Anderson <[e-mail]>
> 
> 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

Junio C Hamano Feb. 15, 2024, 5:22 p.m. UTC | #2

Jeff King <[e-mail]> writes:

> On Thu, Feb 15, 2024 at 01:03:56AM +0000, Bo Anderson via GitGitGadget wrote:
>
>> From: Bo Anderson <[e-mail]>
>> 
>> 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

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