1 file changed, 41 insertions, 25 deletions

diff --git a/tests/t0200-security.sh b/tests/t0200-security.shindex 9238262..f475301 100755--- a/tests/t0200-security.sh+++ b/tests/t0200-security.sh@@ -1,10 +1,19 @@ #!/bin/sh +# Collects the regression tests for the security fixes and for the behaviour+# this fork adds on top of upstream cgit. Each case builds the smallest+# repository and config that reproduce the original problem and then asks for+# the page that used to mishandle it. The comment above a case says what the+# page is being defended against, because a request that looks ordinary is+# usually the whole point of the attack.+ test_description='Check security fixes and fork-specific behavior' . ./setup.sh -# A repo with an oversized blob, readmes that carry markup, and a directory,-# plus a config that pins a tiny blob limit and groups directories.+# Most of what follows shares one repository and one config, so the fixture+# carries everything they need at once, a blob over the size limit, readmes+# holding markup that must not reach the page as markup, and a subdirectory+# to sort ahead of the files. test_expect_success 'set up security fixtures' ' 	mkrepo repos/sec 1 && 	(@@ -31,9 +40,9 @@ test_expect_success 'set up security fixtures' '  secq() { CGIT_CONFIG="$PWD/seccgitrc" QUERY_STRING="$1" cgit; } -# --- Argument injection through the log id= parameter ------------------------# A tip beginning with a dash would be parsed as a git option, and-# id=--output=<path> would create or truncate an arbitrary file.+# A revision beginning with a dash reaches git as an option rather than as a+# tip, so a request for id=--output=<path> could create or truncate any file+# the server is able to write. test_expect_success 'log id=--output does not write a file' ' 	rm -f pwned && 	cgit_query "url=foo/log&id=--output=$PWD/pwned" >tmp 2>&1 &&@@ -55,7 +64,9 @@ test_expect_success 'a valid id= still renders the log' ' 	grep -i "commit 5" tmp ' -# --- max-blob-size is enforced before the object is read --------------------+# max-blob-size is enforced before the object is read, so every view that+# would otherwise inline a file has to turn the same one away rather than+# inflate it first and think better of it afterwards. test_expect_success 'tree view refuses an oversized blob' ' 	secq "url=sec/tree/big.txt" | grep -iE "exceeds|too large" '@@ -72,7 +83,9 @@ test_expect_success 'a small blob is still served' ' 	secq "url=sec/plain/afile" | grep -F "top" ' -# --- Readme rendering escapes untrusted repository content ------------------+# A readme is repository content, so with no about filter configured it has+# to reach the page escaped instead of as live markup, whatever its name+# suggests about the format. test_expect_success 'markdown readme without a filter is escaped as plain text' ' 	{ 		echo "virtual-root=/" &&@@ -104,9 +117,9 @@ test_expect_success 'non-markdown readme keeps its line structure' ' 	grep "pre class=.plaintext." tmp ' -# --- Auto-submitting selects carry no inline handlers -------------------------# A Content-Security-Policy without unsafe-inline blocks inline onchange-# handlers, so the forms mark their selects and cgit.js wires them up.+# A Content-Security-Policy without unsafe-inline stops an inline onchange+# handler from ever running, so the option forms only mark their selects and+# cgit.js wires the submit up from outside the page. test_expect_success 'diff option selects use the autosubmit marker' ' 	sha=$(git -C repos/foo rev-parse HEAD) && 	cgit_query "url=foo/commit&id=$sha" >tmp &&@@ -114,7 +127,8 @@ test_expect_success 'diff option selects use the autosubmit marker' ' 	! grep "onchange" tmp ' -# --- Fork feature: directories are grouped before files in the tree ---------+# Grouping directories ahead of files is behaviour this fork adds, so nothing+# upstream covers it. test_expect_success 'tree groups directories before files' ' 	secq "url=sec/tree/" >tmp && 	dirline=$(grep -n "tree/zsub" tmp | head -1 | cut -d: -f1) &&@@ -124,9 +138,9 @@ test_expect_success 'tree groups directories before files' ' 	test "$dirline" -lt "$fileline" ' -# --- Side-by-side diff percent-encodes a file path into its links ------------# A file name is repository content and may contain a quote, which would-# otherwise break out of the href attribute of the line-number links.+# A file name is repository content and may hold a quote, which would break+# out of the href attribute on the line number links of a side by side diff,+# so the path is percent-encoded on its way into them. test_expect_success 'ssdiff percent-encodes a quoted file path' ' 	mkrepo repos/xss 1 && 	name=$(printf "x\047y.txt") &&@@ -150,7 +164,8 @@ test_expect_success 'ssdiff percent-encodes a quoted file path' ' 	! grep "href=.[^>]*x.y.txt.[^>]*>" tmp ' -# --- A commit with no message must not crash the history views --------------+# git itself will write a commit with an empty message, so the log and the+# summary both have to have something to print where the subject goes. test_expect_success 'a message-less commit renders without crashing' ' 	mkrepo repos/nomsg 1 && 	(@@ -172,12 +187,12 @@ test_expect_success 'a message-less commit renders without crashing' ' 	grep "no commit message" tmp ' -# --- A branch need not point at a commit -------------------------------------# struct refinfo keeps taginfo and commitinfo in a union and fills only the one-# matching the object type. Sorting branches by reading the commit member read-# past the end of the smaller taginfo, and read NULL for a tree, which crashed.-# git update-ref refuses to write these, so the refs go in as loose files: a-# repository is just files on disk and cgit reads whatever is there.+# A branch need not point at a commit. struct refinfo keeps taginfo and+# commitinfo in a union and fills only the member matching the object type,+# so sorting branches through the commit member read past the end of the+# smaller taginfo, and read NULL for a tree, which crashed. git update-ref+# refuses to create such a ref, hence the loose files written by hand below,+# and a repository is only files on disk so cgit meets whatever is there. test_expect_success 'set up a repo whose branches point at odd objects' ' 	mkrepo repos/oddref 2 && 	(@@ -229,10 +244,11 @@ test_expect_success 'name-sorted branches are unaffected' ' 	grep "</html>" tmp ' -# --- A repository cannot supply a printf format string -----------------------# module-link is a template, and scan-path lets a repository set it through its-# own cgitrc. Handing it to printf let a repo owner crash the process, or read-# stack memory into the served page, with surplus conversions.+# module-link is a template and scan-path lets a repository set its own from+# a cgitrc in the tree, so handing that string to printf let the owner of a+# scanned repository crash the process, or read stack memory into the served+# page, merely by adding conversions past the two that are filled. The tests+# after the fixture also pin down the templates that must keep working. test_expect_success 'set up a submodule fixture with a hostile module-link' ' 	mkrepo repos/modlink 1 && 	(
diff --git a/tests/t0200-security.sh b/tests/t0200-security.sh
index 9238262..f475301 100755
--- a/tests/t0200-security.sh
+++ b/tests/t0200-security.sh
@@ -1,10 +1,19 @@
 #!/bin/sh
 
+# Collects the regression tests for the security fixes and for the behaviour
+# this fork adds on top of upstream cgit. Each case builds the smallest
+# repository and config that reproduce the original problem and then asks for
+# the page that used to mishandle it. The comment above a case says what the
+# page is being defended against, because a request that looks ordinary is
+# usually the whole point of the attack.
+
 test_description='Check security fixes and fork-specific behavior'
 . ./setup.sh
 
-# A repo with an oversized blob, readmes that carry markup, and a directory,
-# plus a config that pins a tiny blob limit and groups directories.
+# Most of what follows shares one repository and one config, so the fixture
+# carries everything they need at once, a blob over the size limit, readmes
+# holding markup that must not reach the page as markup, and a subdirectory
+# to sort ahead of the files.
 test_expect_success 'set up security fixtures' '
 	mkrepo repos/sec 1 &&
 	(
@@ -31,9 +40,9 @@ test_expect_success 'set up security fixtures' '
 
 secq() { CGIT_CONFIG="$PWD/seccgitrc" QUERY_STRING="$1" cgit; }
 
-# --- Argument injection through the log id= parameter -----------------------
-# A tip beginning with a dash would be parsed as a git option, and
-# id=--output=<path> would create or truncate an arbitrary file.
+# A revision beginning with a dash reaches git as an option rather than as a
+# tip, so a request for id=--output=<path> could create or truncate any file
+# the server is able to write.
 test_expect_success 'log id=--output does not write a file' '
 	rm -f pwned &&
 	cgit_query "url=foo/log&id=--output=$PWD/pwned" >tmp 2>&1 &&
@@ -55,7 +64,9 @@ test_expect_success 'a valid id= still renders the log' '
 	grep -i "commit 5" tmp
 '
 
-# --- max-blob-size is enforced before the object is read --------------------
+# max-blob-size is enforced before the object is read, so every view that
+# would otherwise inline a file has to turn the same one away rather than
+# inflate it first and think better of it afterwards.
 test_expect_success 'tree view refuses an oversized blob' '
 	secq "url=sec/tree/big.txt" | grep -iE "exceeds|too large"
 '
@@ -72,7 +83,9 @@ test_expect_success 'a small blob is still served' '
 	secq "url=sec/plain/afile" | grep -F "top"
 '
 
-# --- Readme rendering escapes untrusted repository content ------------------
+# A readme is repository content, so with no about filter configured it has
+# to reach the page escaped instead of as live markup, whatever its name
+# suggests about the format.
 test_expect_success 'markdown readme without a filter is escaped as plain text' '
 	{
 		echo "virtual-root=/" &&
@@ -104,9 +117,9 @@ test_expect_success 'non-markdown readme keeps its line structure' '
 	grep "pre class=.plaintext." tmp
 '
 
-# --- Auto-submitting selects carry no inline handlers ------------------------
-# A Content-Security-Policy without unsafe-inline blocks inline onchange
-# handlers, so the forms mark their selects and cgit.js wires them up.
+# A Content-Security-Policy without unsafe-inline stops an inline onchange
+# handler from ever running, so the option forms only mark their selects and
+# cgit.js wires the submit up from outside the page.
 test_expect_success 'diff option selects use the autosubmit marker' '
 	sha=$(git -C repos/foo rev-parse HEAD) &&
 	cgit_query "url=foo/commit&id=$sha" >tmp &&
@@ -114,7 +127,8 @@ test_expect_success 'diff option selects use the autosubmit marker' '
 	! grep "onchange" tmp
 '
 
-# --- Fork feature: directories are grouped before files in the tree ---------
+# Grouping directories ahead of files is behaviour this fork adds, so nothing
+# upstream covers it.
 test_expect_success 'tree groups directories before files' '
 	secq "url=sec/tree/" >tmp &&
 	dirline=$(grep -n "tree/zsub" tmp | head -1 | cut -d: -f1) &&
@@ -124,9 +138,9 @@ test_expect_success 'tree groups directories before files' '
 	test "$dirline" -lt "$fileline"
 '
 
-# --- Side-by-side diff percent-encodes a file path into its links -----------
-# A file name is repository content and may contain a quote, which would
-# otherwise break out of the href attribute of the line-number links.
+# A file name is repository content and may hold a quote, which would break
+# out of the href attribute on the line number links of a side by side diff,
+# so the path is percent-encoded on its way into them.
 test_expect_success 'ssdiff percent-encodes a quoted file path' '
 	mkrepo repos/xss 1 &&
 	name=$(printf "x\047y.txt") &&
@@ -150,7 +164,8 @@ test_expect_success 'ssdiff percent-encodes a quoted file path' '
 	! grep "href=.[^>]*x.y.txt.[^>]*>" tmp
 '
 
-# --- A commit with no message must not crash the history views --------------
+# git itself will write a commit with an empty message, so the log and the
+# summary both have to have something to print where the subject goes.
 test_expect_success 'a message-less commit renders without crashing' '
 	mkrepo repos/nomsg 1 &&
 	(
@@ -172,12 +187,12 @@ test_expect_success 'a message-less commit renders without crashing' '
 	grep "no commit message" tmp
 '
 
-# --- A branch need not point at a commit ------------------------------------
-# struct refinfo keeps taginfo and commitinfo in a union and fills only the one
-# matching the object type. Sorting branches by reading the commit member read
-# past the end of the smaller taginfo, and read NULL for a tree, which crashed.
-# git update-ref refuses to write these, so the refs go in as loose files: a
-# repository is just files on disk and cgit reads whatever is there.
+# A branch need not point at a commit. struct refinfo keeps taginfo and
+# commitinfo in a union and fills only the member matching the object type,
+# so sorting branches through the commit member read past the end of the
+# smaller taginfo, and read NULL for a tree, which crashed. git update-ref
+# refuses to create such a ref, hence the loose files written by hand below,
+# and a repository is only files on disk so cgit meets whatever is there.
 test_expect_success 'set up a repo whose branches point at odd objects' '
 	mkrepo repos/oddref 2 &&
 	(
@@ -229,10 +244,11 @@ test_expect_success 'name-sorted branches are unaffected' '
 	grep "</html>" tmp
 '
 
-# --- A repository cannot supply a printf format string ----------------------
-# module-link is a template, and scan-path lets a repository set it through its
-# own cgitrc. Handing it to printf let a repo owner crash the process, or read
-# stack memory into the served page, with surplus conversions.
+# module-link is a template and scan-path lets a repository set its own from
+# a cgitrc in the tree, so handing that string to printf let the owner of a
+# scanned repository crash the process, or read stack memory into the served
+# page, merely by adding conversions past the two that are filled. The tests
+# after the fixture also pin down the templates that must keep working.
 test_expect_success 'set up a submodule fixture with a hostile module-link' '
 	mkrepo repos/modlink 1 &&
 	(