{"thread":{"id":"18835","subject":"[PATCH] t1301-shared-repo: Fix forced modes test","startedAt":"2009-04-11T21:32:05Z","lastAt":"2009-04-13T02:49:41Z","messageCount":3,"participants":["Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"111100","messageId":"49E10C55.1050005@kdbg.org","threadId":"18835","inReplyTo":null,"subject":"[PATCH] t1301-shared-repo: Fix forced modes test","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-04-11T21:32:05Z","receivedAt":"2009-04-11T21:32:05Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This test was added recently (5a688fe, \"core.sharedrepository = 0mode\"\nshould set, not loosen; 2009-03-28). It checked the result of a sed\ninvocation for emptyness, but in some cases it forgot to print anything\nat all, so that those checks would never be false.\n\nDue to this mistake, it went unnoticed that the files in objects/info are\nnot necessarily 0440, but can also be 0640. This directory is now exempt\nfrom the check.\n\nFinally, this test cannot be run on Windows (requires POSIXPERM).\n\nSigned-off-by: Johannes Sixt \n---\n\n I don't have a Linux/Unix readily available right now, so this is only\n tested on Windows.\n\n -- Hannes\n\n t/t1301-shared-repo.sh | 8 +++++---\n 1 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex 750fbb3..fb7c51d 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -126,7 +126,7 @@ test_expect_success POSIXPERM 'git reflog expire\n \tesac\n '\n\n-test_expect_success 'forced modes' '\n+test_expect_success POSIXPERM 'forced modes' '\n \tmkdir -p templates/hooks &&\n \techo update-server-info >templates/hooks/post-update &&\n \tchmod +x templates/hooks/post-update &&\n@@ -145,7 +145,7 @@ test_expect_success 'forced modes' '\n \txargs ls -ld >actual &&\n\n \t# Everything must be unaccessible to others\n-\ttest -z \"$(sed -n -e \"/^.......---/d\" actual)\" &&\n+\ttest -z \"$(sed -e \"/^.......---/d\" actual)\" &&\n\n \t# All directories must have either 2770 or 770\n \ttest -z \"$(sed -n -e \"/^drwxrw[sx]---/d\" -e \"/^d/p\" actual)\" &&\n@@ -156,10 +156,12 @@ test_expect_success 'forced modes' '\n \t\tp\n \t}\" actual)\" &&\n\n-\t# All files inside objects must be 0440\n+\t# All files inside objects (except objects/info) must be 0440\n \ttest -z \"$(sed -n -e \"/objects\\//{\n \t\t/^d/d\n+\t\t/info/d\n \t\t/^-r--r-----/d\n+\t\tp\n \t}\" actual)\"\n '\n\n-- \n1.6.2.2.1458.gf2216d6\n"},{"id":"111138","messageId":"200904122122.03040.j6t@kdbg.org","threadId":"18835","inReplyTo":"49E10C55.1050005@kdbg.org","subject":"[PATCH v2] t1301-shared-repo: Fix forced modes test, but it still shows a flaw","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-04-12T19:22:02Z","receivedAt":"2009-04-12T19:22:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"This test was added recently (5a688fe, \"core.sharedrepository = 0mode\"\nshould set, not loosen; 2009-03-28). It checked the result of a sed\ninvocation for emptyness, but in some cases it forgot to print anything\nat all, so that those checks would never be false.\n\nDue to this mistake, it went unnoticed that the files in objects/info are\nnot necessarily 0440, but can also be 0640. This directory is now exempt\nfrom the check.\n\nMoreover, COMMIT_EDITMSG is still world-readable. This is either a bug in\ngit, or a flaw in the test (the first sed expression). This patch does not\ndisambiguate these two cases, but only declares the test case as an\nexpected failure.\n\nFinally, this test cannot be run on Windows (requires POSIXPERM).\n\nSigned-off-by: Johannes Sixt \n---\nSorry, the first round was whitespace damaged for some reason.\n\nI could now test the result on Linux, and, lo and behold, it shows a bug:\nEither in git, or in the test: COMMIT_EDITMSG is still world-readable, as\nis shown by the first sed expression that was fixed (2nd hunk).\n\n-- Hannes\n\n t/t1301-shared-repo.sh | 8 +++++---\n 1 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex 750fbb3..71be308 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -126,7 +126,7 @@ test_expect_success POSIXPERM 'git reflog expire honors \ncore.sharedRepository' '\n \tesac\n '\n \n-test_expect_success 'forced modes' '\n+test_expect_failure POSIXPERM 'forced modes' '\n \tmkdir -p templates/hooks &&\n \techo update-server-info >templates/hooks/post-update &&\n \tchmod +x templates/hooks/post-update &&\n@@ -145,7 +145,7 @@ test_expect_success 'forced modes' '\n \txargs ls -ld >actual &&\n \n \t# Everything must be unaccessible to others\n-\ttest -z \"$(sed -n -e \"/^.......---/d\" actual)\" &&\n+\ttest -z \"$(sed -e \"/^.......---/d\" actual)\" &&\n \n \t# All directories must have either 2770 or 770\n \ttest -z \"$(sed -n -e \"/^drwxrw[sx]---/d\" -e \"/^d/p\" actual)\" &&\n@@ -156,10 +156,12 @@ test_expect_success 'forced modes' '\n \t\tp\n \t}\" actual)\" &&\n \n-\t# All files inside objects must be 0440\n+\t# All files inside objects (except objects/info) must be 0440\n \ttest -z \"$(sed -n -e \"/objects\\//{\n \t\t/^d/d\n+\t\t/info/d\n \t\t/^-r--r-----/d\n+\t\tp\n \t}\" actual)\"\n '\n \n-- \n1.6.2.1.224.g2225f\n"},{"id":"111151","messageId":"7vd4bhxo6y.fsf@gitster.siamese.dyndns.org","threadId":"18835","inReplyTo":"200904122122.03040.j6t@kdbg.org","subject":"Re: [PATCH v2] t1301-shared-repo: Fix forced modes test, but it still shows a flaw","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-13T02:49:41Z","receivedAt":"2009-04-13T02:49:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> This test was added recently (5a688fe, \"core.sharedrepository = 0mode\"\n> should set, not loosen; 2009-03-28). It checked the result of a sed\n> invocation for emptyness, but in some cases it forgot to print anything\n> at all, so that those checks would never be false.\n>\n> Due to this mistake, it went unnoticed that the files in objects/info are\n> not necessarily 0440, but can also be 0640. This directory is now exempt\n> from the check.\n>\n> Moreover, COMMIT_EDITMSG is still world-readable. This is either a bug in\n> git, or a flaw in the test (the first sed expression). This patch does not\n> disambiguate these two cases, but only declares the test case as an\n> expected failure.\n>\n> Finally, this test cannot be run on Windows (requires POSIXPERM).\n>\n> Signed-off-by: Johannes Sixt \n> ---\n> Sorry, the first round was whitespace damaged for some reason.\n\nSo is this one but that was minor and I fixed up by hand.\n\n> I could now test the result on Linux, and, lo and behold, it shows a bug:\n> Either in git, or in the test: COMMIT_EDITMSG is still world-readable, as\n> is shown by the first sed expression that was fixed (2nd hunk).\n\nI personally do not think it is worth worrying about transient stuff that\nare meant for repositories with a work tree, such as COMMIT_EDITMSG file\nand the index. If you are working on a shared machine and on a sekrit\nstuff, the root of the work tree would be with mode 0700 (or 0750 to allow\npeeking by other people in the group), and that would mean that neither\nthe index nor .git/COMMIT_EDITMSG in it would be readable by the strangers\nanyway.\n\nThe index happens to be protected already as-is and I am not saying we\nshould loosen it. I am just saying it is not as important as what would\nappear in a bare repository).\n\nAlso, in the real-world use case, .git/COMMIT_EDITMSG will be given to an\narbitrary editor the user happens to use, and we have no guarantee what it\ndoes (e.g. it may create a new file with umask and replace, it may rewrite\nin place, it may leave an editor backup file but use umask to create it,\netc.), and the protection of the file lies majorly on the protection of\nthe root of the work tree.\n\nBut objects/info is meant for users of a bare repository used as a\ndistribution point, and it should obey the stricter mode. If the code\nmakes it world readable, that is indeed a breakage. If it makes it 0660,\nthen the test case is broken.\n\nThe thing is, these tests are meant to cover the 0mode protection, and not\nabout \"because object files are immutable, they should not have w-bit\".\nThe rule should apply no matter what core.sharedrepository setting is in\neffect for the repository, and such a test should be done elsewhere. It\nis not a part of core.sharedrepository settings topic.\n\nSo how about doing it this way instead? If somebody cares, s/he can fix\nthe COMMIT_EDITMSG codepath and then drop the exclusion of it, but as I\nalready stated why, I do not think it is worth it.\n\n\tSide note: I think info/refs and objects/info/packs files are made\n\t0660, not 0640, as you stated. As far as I can remember, these\n\tare updated by the usual creat-write-rename and do not have be\n\twritable, but it is a separate matter.\n\nNote that this patch is against the tip of the original jc/shared-literally\ntopic (1b89eaa) that can be merged to 'maint'; I'll have a fix-up to add\nPOSIXPERM when merging it to 'master'.\n\n-- >8 --\nFrom: Johannes Sixt \nDate: Sun, 12 Apr 2009 21:22:02 +0200\nSubject: [PATCH] t1301-shared-repo: fix forced modes test\n\nThis test was added recently (5a688fe, \"core.sharedrepository = 0mode\"\nshould set, not loosen; 2009-03-28). It checked the result of a sed\ninvocation for emptyness, but in some cases it forgot to print anything\nat all, so that those checks would never be false.\n\nDue to this mistake, it went unnoticed that the files in objects/info are\nnot necessarily 0440, but can also be 0660. Because the 0mode setting\ntries to guarantee that the files are accessible only to the people they\nare meant to be used by, we should only make sure that they are readable\nby the user and the group when the configuration is set to 0660. It is a\nseparate matter from the core.shredrepository settings that w-bit from\nimmutable object files under objects/[0-9a-f][0-9a-f] directories should\nbe dropped.\n\nCOMMIT_EDITMSG is still world-readable, but it (and any transient files\nthat are meant for repositories with a work tree) does not matter. If you\nare working on a shared machine and on a sekrit stuff, the root of the\nwork tree would be with mode 0700 (or 0750 to allow peeking by other\npeople in the group), and that would mean that .git/COMMIT_EDITMSG in such\na repository would not be readable by the strangers anyway.\n\nAlso, in the real-world use case, .git/COMMIT_EDITMSG will be given to an\narbitrary editor the user happens to use, and we have no guarantee what it\ndoes (e.g. it may create a new file with umask and replace, it may rewrite\nin place, it may leave an editor backup file but use umask to create it,\netc.), and the protection of the file lies majorly on the protection of\nthe root of the work tree.\n\nThis test cannot be run on Windows; it requires POSIXPERM when merged to\n'master'.\n\nSigned-off-by: Johannes Sixt \nSigned-off-by: Junio C Hamano \n---\n t/t1301-shared-repo.sh | 12 ++++++++----\n 1 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex 3c8a237..3fddc9e 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -141,11 +141,14 @@ test_expect_success 'forced modes' '\n \t\tgit commit -a -m initial &&\n \t\tgit repack\n \t) &&\n-\tfind new/.git -print |\n+\t# List repository files meant to be protected; note that\n+\t# COMMIT_EDITMSG does not matter---0mode is not about a\n+\t# repository with a work tree.\n+\tfind new/.git -type f -name COMMIT_EDITMSG -prune -o -print |\n \txargs ls -ld >actual &&\n \n \t# Everything must be unaccessible to others\n-\ttest -z \"$(sed -n -e \"/^.......---/d\" actual)\" &&\n+\ttest -z \"$(sed -e \"/^.......---/d\" actual)\" &&\n \n \t# All directories must have either 2770 or 770\n \ttest -z \"$(sed -n -e \"/^drwxrw[sx]---/d\" -e \"/^d/p\" actual)\" &&\n@@ -156,10 +159,11 @@ test_expect_success 'forced modes' '\n \t\tp\n \t}\" actual)\" &&\n \n-\t# All files inside objects must be 0440\n+\t# All files inside objects must be accessible by us\n \ttest -z \"$(sed -n -e \"/objects\\//{\n \t\t/^d/d\n-\t\t/^-r--r-----/d\n+\t\t/^-r.-r.----/d\n+\t\tp\n \t}\" actual)\"\n '\n \n-- \n1.6.2.2.527.gea4fd\n"}]}