{"thread":{"id":"60921","subject":"[PATCH] t/lib-credential: clean additional credential","startedAt":"2024-02-15T01:04:00Z","lastAt":"2024-02-17T04:58:22Z","messageCount":4,"participants":["Bo Anderson via GitGitGadget","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"488678","messageId":"pull.1664.git.1707959036807.gitgitgadget@gmail.com","threadId":"60921","inReplyTo":null,"subject":"[PATCH] t/lib-credential: clean additional credential","fromName":"Bo Anderson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-15T01:03:56Z","receivedAt":"2024-02-15T01:04:00Z","isPatch":true,"sender":{"key":"mail@boanderson.me","avatar":"https://avatars.githubusercontent.com/u/1190754?v=4"},"body":"From: Bo Anderson <mail@boanderson.me>\n\n71201ab0e5 (t/lib-credential.sh: ensure credential helpers handle long\nheaders, 2023-05-01) added a test which stores credentials with the host\nvictim.example.com but this was never cleaned up, leaving residual data\nin the credential store after running the tests.\n\nAdd a cleanup call for this credential to resolve this issue.\n\nSigned-off-by: Bo Anderson <mail@boanderson.me>\n---\n    t/lib-credential: clean additional credential\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1664%2FBo98%2Ft-credential-missing-clean-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1664/Bo98/t-credential-missing-clean-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1664\n\n t/lib-credential.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/lib-credential.sh b/t/lib-credential.sh\nindex 15fc9a31e2c..44799c0d38f 100644\n--- a/t/lib-credential.sh\n+++ b/t/lib-credential.sh\n@@ -50,6 +50,7 @@ helper_test_clean() {\n \treject $1 https example.com user-overwrite\n \treject $1 https example.com user-erase1\n \treject $1 https example.com user-erase2\n+\treject $1 https victim.example.com user\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n \treject $1 https sso.tld\n\nbase-commit: efb050becb6bc703f76382e1f1b6273100e6ace3\n-- \ngitgitgadget\n"},{"id":"488679","messageId":"20240215043900.GA2821179@coredump.intra.peff.net","threadId":"60921","inReplyTo":"pull.1664.git.1707959036807.gitgitgadget@gmail.com","subject":"Re: [PATCH] t/lib-credential: clean additional credential","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-15T04:39:00Z","receivedAt":"2024-02-15T04:45:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 15, 2024 at 01:03:56AM +0000, Bo Anderson via GitGitGadget wrote:\n\n> From: Bo Anderson <mail@boanderson.me>\n> \n> 71201ab0e5 (t/lib-credential.sh: ensure credential helpers handle long\n> headers, 2023-05-01) added a test which stores credentials with the host\n> victim.example.com but this was never cleaned up, leaving residual data\n> in the credential store after running the tests.\n> \n> Add a cleanup call for this credential to resolve this issue.\n\nGood catch. The patch looks obviously correct.\n\nI'm not surprised nobody noticed until now, as I expect it is pretty\nrare for people to run t0303 against system helpers (it is not a problem\nfor t0301, etc, because they only touch the internal trash directory).\n\nI wonder if we might want something like this, as well, which can catch\nleftovers:\n\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex 716bf1af9f..4183154243 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -6,6 +6,11 @@ test_description='credential-store tests'\n \n helper_test store\n \n+helper_test_clean store\n+test_expect_success 'test cleanup removes everything' '\n+\ttest_must_be_empty \"$HOME/.git-credentials\"\n+'\n+\n test_expect_success 'when xdg file does not exist, xdg file not created' '\n \ttest_path_is_missing \"$HOME/.config/git/credentials\" &&\n \ttest -s \"$HOME/.git-credentials\"\n\n-Peff\n"},{"id":"488743","messageId":"xmqqle7lskvg.fsf@gitster.g","threadId":"60921","inReplyTo":"20240215043900.GA2821179@coredump.intra.peff.net","subject":"Re: [PATCH] t/lib-credential: clean additional credential","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-15T17:22:27Z","receivedAt":"2024-02-15T17:22:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Feb 15, 2024 at 01:03:56AM +0000, Bo Anderson via GitGitGadget wrote:\n>\n>> From: Bo Anderson <mail@boanderson.me>\n>> \n>> 71201ab0e5 (t/lib-credential.sh: ensure credential helpers handle long\n>> headers, 2023-05-01) added a test which stores credentials with the host\n>> victim.example.com but this was never cleaned up, leaving residual data\n>> in the credential store after running the tests.\n>> \n>> Add a cleanup call for this credential to resolve this issue.\n>\n> Good catch. The patch looks obviously correct.\n>\n> I'm not surprised nobody noticed until now, as I expect it is pretty\n> rare for people to run t0303 against system helpers (it is not a problem\n> for t0301, etc, because they only touch the internal trash directory).\n>\n> I wonder if we might want something like this, as well, which can catch\n> leftovers:\n\nSounds like a good hygiene ;-).\n\n>\n> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> index 716bf1af9f..4183154243 100755\n> --- a/t/t0302-credential-store.sh\n> +++ b/t/t0302-credential-store.sh\n> @@ -6,6 +6,11 @@ test_description='credential-store tests'\n>  \n>  helper_test store\n>  \n> +helper_test_clean store\n> +test_expect_success 'test cleanup removes everything' '\n> +\ttest_must_be_empty \"$HOME/.git-credentials\"\n> +'\n> +\n>  test_expect_success 'when xdg file does not exist, xdg file not created' '\n>  \ttest_path_is_missing \"$HOME/.config/git/credentials\" &&\n>  \ttest -s \"$HOME/.git-credentials\"\n>\n> -Peff\n"},{"id":"488838","messageId":"20240217045814.GA539459@coredump.intra.peff.net","threadId":"60921","inReplyTo":"xmqqle7lskvg.fsf@gitster.g","subject":"[PATCH] t0303: check that helper_test_clean removes all credentials","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-17T04:58:14Z","receivedAt":"2024-02-17T04:58:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 15, 2024 at 09:22:27AM -0800, Junio C Hamano wrote:\n\n> > I wonder if we might want something like this, as well, which can catch\n> > leftovers:\n> \n> Sounds like a good hygiene ;-).\n\nUnfortunately, it is not quite so simple. There are a bunch of other\ntests run by t0303 that are not in t0302, because credential-store does\nnot support caching, expiration, etc.\n\nPutting it in t0301 would be better, but we don't have a way to inspect\nthe internal state of the cache daemon. We could perhaps add one, but\nhere's an even hackier solution. ;)\n\n-- >8 --\nSubject: [PATCH] t0303: check that helper_test_clean removes all credentials\n\nOur lib-credential.sh library comes with a \"clean\" function that removes\nall of the credentials used in its tests (to avoid leaving cruft in\nsystem credential storage). But it's easy to add a test that uses a new\ncredential but forget to add it to the clean function.  E.g., the case\nfixed by 83e6eb7d7a (t/lib-credential: clean additional credential,\n2024-02-15).\n\nWe should be able to catch this automatically, but it's a little tricky.\n\nWe can't just compare the contents of the helper's storage before and\nafter the test run, because there isn't a way to ask a helper to dump\nall of its storage. And in most cases we don't have direct access to the\nunderlying storage (since the whole point of the helper is to abstract\nthat away). We can work around that by using our own \"store\" helper,\nsince we can directly inspect its state by looking at its on-disk file.\n\nBut there's a catch: the \"store\" helper doesn't support features like\ncaching or expiration, so using it naively fails tests (and skipping\nthose tests would give us incomplete coverage). Implementing all of\nthose features would be non-trivial. But we can hack around that by\noverriding the \"check\" function used by the tests to turn most requests\ninto noop success (except for \"approve\" requests, which actually store\nthings).\n\nAnd then at the end we can check that running the \"clean\" function takes\nus back to an empty state.\n\nNote that because we've skipped any tests that erase credentials\n(because of our noop check function), the state we see at cleanup time\nmay be larger than it would be normally. That's OK. The point of the\nclean function is to clean up any cruft we _might_ have left in place,\nso we're just being doubly thorough.\n\nThe way this is bolted onto t0303 feels a little messy. But it's really\nthe best place to do it, because then we know that it is running the\nexact sequence of tests that we'd use for testing a real external\nhelper. In a normal run of \"make test\" it currently does nothing (the\nidea is that you run it manually after pointing it at some helper\nprogram). But now with this patch, \"make test\" will sanity-check the\nscript itself.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis should be applied on top of ba/credential-test-clean-fix,\nnaturally, or it will fail. :)\n\n t/t0303-credential-external.sh | 26 ++++++++++++++++++++++++--\n 1 file changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh\nindex 095574bfc6..72ae405c3e 100755\n--- a/t/t0303-credential-external.sh\n+++ b/t/t0303-credential-external.sh\n@@ -32,9 +32,24 @@ commands.\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-credential.sh\n \n+# If we're not given a specific external helper to run against,\n+# there isn't much to test. But we can still run through our\n+# battery of tests with a fake helper and check that the\n+# test themselves are self-consistent and clean up after\n+# themselves.\n+#\n+# We'll use the \"store\" helper, since we can easily inspect\n+# its state by looking at the on-disk file. But since it doesn't\n+# implement any caching or expiry logic, we'll cheat and override\n+# the \"check\" function to just report all results as OK.\n if test -z \"$GIT_TEST_CREDENTIAL_HELPER\"; then\n-\tskip_all=\"used to test external credential helpers\"\n-\ttest_done\n+\tGIT_TEST_CREDENTIAL_HELPER=store\n+\tGIT_TEST_CREDENTIAL_HELPER_TIMEOUT=store\n+\tcheck () {\n+\t\ttest \"$1\" = \"approve\" || return 0\n+\t\tgit -c credential.helper=store credential approve\n+\t}\n+\tcheck_cleanup=t\n fi\n \n test -z \"$GIT_TEST_CREDENTIAL_HELPER_SETUP\" ||\n@@ -59,4 +74,11 @@ fi\n # might be long-term system storage\n helper_test_clean \"$GIT_TEST_CREDENTIAL_HELPER\"\n \n+if test \"$check_cleanup\" = \"t\"\n+then\n+\ttest_expect_success 'test cleanup removes everything' '\n+\t\ttest_must_be_empty \"$HOME/.git-credentials\"\n+\t'\n+fi\n+\n test_done\n-- \n2.44.0.rc1.410.g8325d5f159\n\n"}]}