{"thread":{"id":"51195","subject":"[PATCH v3] list-objects-filter: disable 'sparse:path' filters","startedAt":"2019-05-29T12:44:55Z","lastAt":"2019-05-29T12:44:55Z","messageCount":1,"participants":["Christian Couder"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"376397","messageId":"20190529124432.5707-1-chriscool@tuxfamily.org","threadId":"51195","inReplyTo":null,"subject":"[PATCH v3] list-objects-filter: disable 'sparse:path' filters","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-05-29T12:44:32Z","receivedAt":"2019-05-29T12:44:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder <christian.couder@gmail.com>\n\nIf someone wants to use as a filter a sparse file that is in the\nrepository, something like \"--filter=sparse:oid=<ref>:<path>\"\nalready works.\n\nSo 'sparse:path' is only interesting if the sparse file is not in\nthe repository. In this case though the current implementation has\na big security issue, as it makes it possible to ask the server to\nread any file, like for example /etc/password, and to explore the\nfilesystem, as well as individual lines of files.\n\nIf someone is interested in using a sparse file that is not in the\nrepository as a filter, then at the minimum a config option, such\nas \"uploadpack.sparsePathFilter\", should be implemented first to\nrestrict the directory from which the files specified by\n'sparse:path' can be read.\n\nFor now though, let's just disable 'sparse:path' filters.\n\nHelped-by: Matthew DeVore <matvore@google.com>\nHelped-by: Jeff Hostetler <git@jeffhostetler.com>\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\nChanges since version 2 are the following:\n\n  - added changes to Documentation/rev-list-options.txt, as\n    discussed with Junio.\n\n Documentation/rev-list-options.txt     |  7 +--\n contrib/completion/git-completion.bash |  2 +-\n list-objects-filter-options.c          | 10 ++--\n list-objects-filter-options.h          |  2 -\n list-objects-filter.c                  | 22 --------\n t/t5317-pack-objects-filter-objects.sh | 71 +++++---------------------\n t/t6112-rev-list-filters-objects.sh    | 39 +++++---------\n 7 files changed, 37 insertions(+), 116 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex ddbc1de43f..71a1fcc093 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -725,9 +725,6 @@ specification contained in the blob (or blob-expression) '<blob-ish>'\n to omit blobs that would not be not required for a sparse checkout on\n the requested refs.\n +\n-The form '--filter=sparse:path=<path>' similarly uses a sparse-checkout\n-specification contained in <path>.\n-+\n The form '--filter=tree:<depth>' omits all blobs and trees whose depth\n from the root tree is >= <depth> (minimum depth if an object is located\n at multiple depths in the commits traversed). <depth>=0 will not include\n@@ -737,6 +734,10 @@ tree and blobs which are referenced directly by a commit reachable from\n <commit> or an explicitly-given object. <depth>=2 is like <depth>=1\n while also including trees and blobs one more level removed from an\n explicitly-given commit or tree.\n++\n+Note that the form '--filter=sparse:path=<path>' that wants to read\n+from an arbitrary path on the filesystem has been dropped for security\n+reasons.\n \n --no-filter::\n \tTurn off any previous `--filter=` argument.\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 3eefbabdb1..9f71bcde96 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1536,7 +1536,7 @@ _git_fetch ()\n \t\treturn\n \t\t;;\n \t--filter=*)\n-\t\t__gitcomp \"blob:none blob:limit= sparse:oid= sparse:path=\" \"\" \"${cur##--filter=}\"\n+\t\t__gitcomp \"blob:none blob:limit= sparse:oid=\" \"\" \"${cur##--filter=}\"\n \t\treturn\n \t\t;;\n \t--*)\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex c0036f7378..a15d0f7829 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -78,9 +78,12 @@ static int gently_parse_list_objects_filter(\n \t\treturn 0;\n \n \t} else if (skip_prefix(arg, \"sparse:path=\", &v0)) {\n-\t\tfilter_options->choice = LOFC_SPARSE_PATH;\n-\t\tfilter_options->sparse_path_value = strdup(v0);\n-\t\treturn 0;\n+\t\tif (errbuf) {\n+\t\t\tstrbuf_addstr(\n+\t\t\t\terrbuf,\n+\t\t\t\t_(\"sparse:path filters support has been dropped\"));\n+\t\t}\n+\t\treturn 1;\n \t}\n \t/*\n \t * Please update _git_fetch() in git-completion.bash when you\n@@ -136,7 +139,6 @@ void list_objects_filter_release(\n {\n \tfree(filter_options->filter_spec);\n \tfree(filter_options->sparse_oid_value);\n-\tfree(filter_options->sparse_path_value);\n \tmemset(filter_options, 0, sizeof(*filter_options));\n }\n \ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex e3adc78ebf..c54f0000fb 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -13,7 +13,6 @@ enum list_objects_filter_choice {\n \tLOFC_BLOB_LIMIT,\n \tLOFC_TREE_DEPTH,\n \tLOFC_SPARSE_OID,\n-\tLOFC_SPARSE_PATH,\n \tLOFC__COUNT /* must be last */\n };\n \n@@ -44,7 +43,6 @@ struct list_objects_filter_options {\n \t * choice.\n \t */\n \tstruct object_id *sparse_oid_value;\n-\tchar *sparse_path_value;\n \tunsigned long blob_limit_value;\n \tunsigned long tree_exclude_depth;\n };\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex ee449de3f7..53f90442c5 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -478,27 +478,6 @@ static void *filter_sparse_oid__init(\n \treturn d;\n }\n \n-static void *filter_sparse_path__init(\n-\tstruct oidset *omitted,\n-\tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n-{\n-\tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n-\tif (add_excludes_from_file_to_list(filter_options->sparse_path_value,\n-\t\t\t\t\t   NULL, 0, &d->el, NULL) < 0)\n-\t\tdie(\"could not load filter specification\");\n-\n-\tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n-\td->array_frame[d->nr].defval = 0; /* default to include */\n-\td->array_frame[d->nr].child_prov_omit = 0;\n-\n-\t*filter_fn = filter_sparse;\n-\t*filter_free_fn = filter_sparse_free;\n-\treturn d;\n-}\n-\n typedef void *(*filter_init_fn)(\n \tstruct oidset *omitted,\n \tstruct list_objects_filter_options *filter_options,\n@@ -514,7 +493,6 @@ static filter_init_fn s_filters[] = {\n \tfilter_blobs_limit__init,\n \tfilter_trees_depth__init,\n \tfilter_sparse_oid__init,\n-\tfilter_sparse_path__init,\n };\n \n void *list_objects_filter__init(\ndiff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\nindex 4c0201c34b..2d2f5d0229 100755\n--- a/t/t5317-pack-objects-filter-objects.sh\n+++ b/t/t5317-pack-objects-filter-objects.sh\n@@ -277,6 +277,10 @@ test_expect_success 'verify normal and blob:limit packfiles have same commits/tr\n '\n \n # Test sparse:path=<path> filter.\n+# !!!!\n+# NOTE: sparse:path filter support has been dropped for security reasons,\n+# so the tests have been changed to make sure that using it fails.\n+# !!!!\n # Use a local file containing a sparse-checkout specification to filter\n # out blobs not required for the corresponding sparse-checkout.  We do not\n # require sparse-checkout to actually be enabled.\n@@ -315,73 +319,24 @@ test_expect_success 'verify blob count in normal packfile' '\n \ttest_cmp expected observed\n '\n \n-test_expect_success 'verify sparse:path=pattern1' '\n-\tgit -C r3 ls-files -s dir1/sparse1 dir1/sparse2 >ls_files_result &&\n-\tawk -f print_2.awk ls_files_result |\n-\tsort >expected &&\n-\n-\tgit -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&\n+test_expect_success 'verify sparse:path=pattern1 fails' '\n+\ttest_must_fail git -C r3 pack-objects --revs --stdout \\\n+\t\t--filter=sparse:path=../pattern1 <<-EOF\n \tHEAD\n \tEOF\n-\tgit -C r3 index-pack ../filter.pack &&\n-\n-\tgit -C r3 verify-pack -v ../filter.pack >verify_result &&\n-\tgrep blob verify_result |\n-\tawk -f print_1.awk |\n-\tsort >observed &&\n-\n-\ttest_cmp expected observed\n-'\n-\n-test_expect_success 'verify normal and sparse:path=pattern1 packfiles have same commits/trees' '\n-\tgit -C r3 verify-pack -v ../all.pack >verify_result &&\n-\tgrep -E \"commit|tree\" verify_result |\n-\tawk -f print_1.awk |\n-\tsort >expected &&\n-\n-\tgit -C r3 verify-pack -v ../filter.pack >verify_result &&\n-\tgrep -E \"commit|tree\" verify_result |\n-\tawk -f print_1.awk |\n-\tsort >observed &&\n-\n-\ttest_cmp expected observed\n '\n \n-test_expect_success 'verify sparse:path=pattern2' '\n-\tgit -C r3 ls-files -s sparse1 dir1/sparse1 >ls_files_result &&\n-\tawk -f print_2.awk ls_files_result |\n-\tsort >expected &&\n-\n-\tgit -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&\n+test_expect_success 'verify sparse:path=pattern2 fails' '\n+\ttest_must_fail git -C r3 pack-objects --revs --stdout \\\n+\t\t--filter=sparse:path=../pattern2 <<-EOF\n \tHEAD\n \tEOF\n-\tgit -C r3 index-pack ../filter.pack &&\n-\n-\tgit -C r3 verify-pack -v ../filter.pack >verify_result &&\n-\tgrep blob verify_result |\n-\tawk -f print_1.awk |\n-\tsort >observed &&\n-\n-\ttest_cmp expected observed\n-'\n-\n-test_expect_success 'verify normal and sparse:path=pattern2 packfiles have same commits/trees' '\n-\tgit -C r3 verify-pack -v ../all.pack >verify_result &&\n-\tgrep -E \"commit|tree\" verify_result |\n-\tawk -f print_1.awk |\n-\tsort >expected &&\n-\n-\tgit -C r3 verify-pack -v ../filter.pack >verify_result &&\n-\tgrep -E \"commit|tree\" verify_result |\n-\tawk -f print_1.awk |\n-\tsort >observed &&\n-\n-\ttest_cmp expected observed\n '\n \n # Test sparse:oid=<oid-ish> filter.\n-# Like sparse:path, but we get the sparse-checkout specification from\n-# a blob rather than a file on disk.\n+# Use a blob containing a sparse-checkout specification to filter\n+# out blobs not required for the corresponding sparse-checkout.  We do not\n+# require sparse-checkout to actually be enabled.\n \n test_expect_success 'setup r4' '\n \tgit init r4 &&\ndiff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh\nindex 9c11427719..acd7f5ab80 100755\n--- a/t/t6112-rev-list-filters-objects.sh\n+++ b/t/t6112-rev-list-filters-objects.sh\n@@ -157,6 +157,10 @@ test_expect_success 'verify blob:limit=1m' '\n '\n \n # Test sparse:path=<path> filter.\n+# !!!!\n+# NOTE: sparse:path filter support has been dropped for security reasons,\n+# so the tests have been changed to make sure that using it fails.\n+# !!!!\n # Use a local file containing a sparse-checkout specification to filter\n # out blobs not required for the corresponding sparse-checkout.  We do not\n # require sparse-checkout to actually be enabled.\n@@ -176,37 +180,20 @@ test_expect_success 'setup r3' '\n \techo sparse1 >pattern2\n '\n \n-test_expect_success 'verify sparse:path=pattern1 omits top-level files' '\n-\tgit -C r3 ls-files -s sparse1 sparse2 >ls_files_result &&\n-\tawk -f print_2.awk ls_files_result |\n-\tsort >expected &&\n-\n-\tgit -C r3 rev-list --quiet --objects --filter-print-omitted \\\n-\t\t--filter=sparse:path=../pattern1 HEAD >revs &&\n-\tawk -f print_1.awk revs |\n-\tsed \"s/~//\" |\n-\tsort >observed &&\n-\n-\ttest_cmp expected observed\n+test_expect_success 'verify sparse:path=pattern1 fails' '\n+\ttest_must_fail git -C r3 rev-list --quiet --objects \\\n+\t\t--filter-print-omitted --filter=sparse:path=../pattern1 HEAD\n '\n \n-test_expect_success 'verify sparse:path=pattern2 omits both sparse2 files' '\n-\tgit -C r3 ls-files -s sparse2 dir1/sparse2 >ls_files_result &&\n-\tawk -f print_2.awk ls_files_result |\n-\tsort >expected &&\n-\n-\tgit -C r3 rev-list --quiet --objects --filter-print-omitted \\\n-\t\t--filter=sparse:path=../pattern2 HEAD >revs &&\n-\tawk -f print_1.awk revs |\n-\tsed \"s/~//\" |\n-\tsort >observed &&\n-\n-\ttest_cmp expected observed\n+test_expect_success 'verify sparse:path=pattern2 fails' '\n+\ttest_must_fail git -C r3 rev-list --quiet --objects \\\n+\t\t--filter-print-omitted --filter=sparse:path=../pattern2 HEAD\n '\n \n # Test sparse:oid=<oid-ish> filter.\n-# Like sparse:path, but we get the sparse-checkout specification from\n-# a blob rather than a file on disk.\n+# Use a blob containing a sparse-checkout specification to filter\n+# out blobs not required for the corresponding sparse-checkout.  We do not\n+# require sparse-checkout to actually be enabled.\n \n test_expect_success 'setup r3 part 2' '\n \techo dir1/ >r3/pattern &&\n-- \n2.22.0.rc1.7.gaa5a13be45.dirty\n\n"}]}