{"thread":{"id":"51176","subject":"[PATCH v2] list-objects-filter: disable 'sparse:path' filters","startedAt":"2019-05-25T14:28:57Z","lastAt":"2019-05-29T07:30:09Z","messageCount":4,"participants":["Christian Couder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"376218","messageId":"20190525142834.6168-1-chriscool@tuxfamily.org","threadId":"51176","inReplyTo":null,"subject":"[PATCH v2] list-objects-filter: disable 'sparse:path' filters","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-05-25T14:28:34Z","receivedAt":"2019-05-25T14:28:57Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"If 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>\n---\n\nChanges since the RFC version are the following:\n\n  - improved the error message when 'sparse:path' is used,\n  - updated \"git-completion.bash\",\n  - freed \"sparse_path_value\" field in list_objects_filter_release(),\n  - updated tests (t5317 and t6112).\n\nThanks to Matthew and Jeff for the suggestions.\n\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 6 files changed, 33 insertions(+), 113 deletions(-)\n\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.1.ge905c6e19a.dirty\n\n"},{"id":"376274","messageId":"20190528063006.GF7946@sigill.intra.peff.net","threadId":"51176","inReplyTo":"20190525142834.6168-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v2] list-objects-filter: disable 'sparse:path' filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-28T06:30:07Z","receivedAt":"2019-05-28T06:30:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 25, 2019 at 04:28:34PM +0200, Christian Couder wrote:\n\n> If someone wants to use as a filter a sparse file that is in the\n> repository, something like \"--filter=sparse:oid=<ref>:<path>\"\n> already works.\n> \n> So 'sparse:path' is only interesting if the sparse file is not in\n> the repository. In this case though the current implementation has\n> a big security issue, as it makes it possible to ask the server to\n> read any file, like for example /etc/password, and to explore the\n> filesystem, as well as individual lines of files.\n> \n> If someone is interested in using a sparse file that is not in the\n> repository as a filter, then at the minimum a config option, such\n> as \"uploadpack.sparsePathFilter\", should be implemented first to\n> restrict the directory from which the files specified by\n> 'sparse:path' can be read.\n> \n> For now though, let's just disable 'sparse:path' filters.\n\nThanks for picking this up. The patch looks fine to me (versus just\ndisabling it for remote invocations) assuming we are OK with the\npossible regression. I suppose cooking this in 'next' for a while is one\nway we might find out if anybody yells loudly.\n\n-Peff\n"},{"id":"376344","messageId":"xmqqo93m1i49.fsf@gitster-ct.c.googlers.com","threadId":"51176","inReplyTo":"20190525142834.6168-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v2] list-objects-filter: disable 'sparse:path' filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-28T19:41:58Z","receivedAt":"2019-05-28T19:42:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> If someone wants to use as a filter a sparse file that is in the\n> repository, something like \"--filter=sparse:oid=<ref>:<path>\"\n> already works.\n>\n> So 'sparse:path' is only interesting if the sparse file is not in\n> the repository. In this case though the current implementation has\n> a big security issue, as it makes it possible to ask the server to\n> read any file, like for example /etc/password, and to explore the\n> filesystem, as well as individual lines of files.\n>\n> If someone is interested in using a sparse file that is not in the\n> repository as a filter, then at the minimum a config option, such\n> as \"uploadpack.sparsePathFilter\", should be implemented first to\n> restrict the directory from which the files specified by\n> 'sparse:path' can be read.\n>\n> For now though, let's just disable 'sparse:path' filters.\n>\n> Helped-by: Matthew DeVore <matvore@google.com>\n> Helped-by: Jeff Hostetler <git@jeffhostetler.com>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>\n> Changes since the RFC version are the following:\n>\n>   - improved the error message when 'sparse:path' is used,\n>   - updated \"git-completion.bash\",\n>   - freed \"sparse_path_value\" field in list_objects_filter_release(),\n>   - updated tests (t5317 and t6112).\n>\n> Thanks to Matthew and Jeff for the suggestions.\n>\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>  6 files changed, 33 insertions(+), 113 deletions(-)\n\nWhat is curious is that this does not touch Documentation/ hierarchy\nat all---is that a sign that nobody makes any serious use of the\n--filter=... thing and we can freely drop \"features\" around it when\nwe see it necessary (like in this case)?\n\nOr do we need something like this on top (or squashed in)?  I can\nlive with or without \"Note that...\" myself.\n\nThanks.\n\n\n Documentation/rev-list-options.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex ddbc1de43f..73aafea8d6 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,9 @@ 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 from\n+an arbitrary path on the filesystem is not supported, for security reasons.\n \n --no-filter::\n \tTurn off any previous `--filter=` argument.\n\n\n"},{"id":"376381","messageId":"CAP8UFD08pOmUGkTT3BtDqLwB0g3Q_C0_+RctGb_WfFq7K-7wsA@mail.gmail.com","threadId":"51176","inReplyTo":"xmqqo93m1i49.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] list-objects-filter: disable 'sparse:path' filters","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-05-29T07:29:55Z","receivedAt":"2019-05-29T07:30:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, May 28, 2019 at 9:42 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> What is curious is that this does not touch Documentation/ hierarchy\n> at all---is that a sign that nobody makes any serious use of the\n> --filter=... thing and we can freely drop \"features\" around it when\n> we see it necessary (like in this case)?\n\nI just forgot about the Documentation.\n\n> Or do we need something like this on top (or squashed in)?  I can\n> live with or without \"Note that...\" myself.\n\nYeah, I will squash something like what you suggest soon.\n\n>  Documentation/rev-list-options.txt | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index ddbc1de43f..73aafea8d6 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,9 @@ 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 from\n> +an arbitrary path on the filesystem is not supported, for security reasons.\n\nI'd rather say:\n\n\"Note that the form '--filter=sparse:path=<path>' that wants to read from\nan arbitrary path on the filesystem has been dropped for security reasons.\"\n\nto be more consistent with the error message (that Matthew suggested\nin a comment following my initial RFC patch).\n\nThanks,\nChristian.\n"}]}