{"thread":{"id":"20921","subject":"[RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot","startedAt":"2009-09-13T00:09:23Z","lastAt":"2009-09-13T15:03:45Z","messageCount":5,"participants":["Mark Rada","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"123013","messageId":"4AAC3833.8060905@mailservices.uwaterloo.ca","threadId":"20921","inReplyTo":null,"subject":"[RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot","fromName":"Mark Rada","fromEmail":"marada@uwaterloo.ca","sentAt":"2009-09-13T00:09:23Z","receivedAt":"2009-09-13T00:09:23Z","isPatch":true,"sender":{"key":"marada@uwaterloo.ca","avatar":"https://avatars.githubusercontent.com/u/38430?v=4"},"body":"Makes things nicer in cases when you hand craft the snapshot URL but\nmake a typo in defining the hash variable (e.g. netx instead of next);\nyou will now get an error message instead of a broken tarball.\n\nTo maintain backwards compatibility, git_get_head_hash is now a wrapper\nfor git_get_full_hash, as suggested by Jakub Narebski.\n\nTests for t9501 are included to demonstrate added functionality.\n\nSigned-off-by: Mark Rada <marada@uwaterloo.ca>\n---\n\n\tThis is just a re-send based on getting torn a new one by Junio.\n\tChanges since v3:\n\t\t- variables have been renamed for readability\n\n\n gitweb/gitweb.perl                       |   19 +++++++++++++------\n t/t9501-gitweb-standalone-http-status.sh |   29 +++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 6 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 24b2193..e1beca5 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1983,14 +1983,20 @@ sub quote_command {\n \n # get HEAD ref of given project as hash\n sub git_get_head_hash {\n+\treturn git_get_full_hash(shift, 'HEAD');\n+}\n+\n+# given a project and tree-ish, returns full hash\n+sub git_get_full_hash {\n \tmy $project = shift;\n+\tmy $hash = shift;\n \tmy $o_git_dir = $git_dir;\n \tmy $retval = undef;\n \t$git_dir = \"$projectroot/$project\";\n-\tif (open my $fd, \"-|\", git_cmd(), \"rev-parse\", \"--verify\", \"HEAD\") {\n-\t\tmy $head = <$fd>;\n+\tif (open my $fd, '-|', git_cmd(), 'rev-parse', '--verify', $hash) {\n+\t\t$hash = <$fd>;\n \t\tclose $fd;\n-\t\tif (defined $head && $head =~ /^([0-9a-fA-F]{40})$/) {\n+\t\tif (defined $hash && $hash =~ /^([0-9a-fA-F]{40})$/) {\n \t\t\t$retval = $1;\n \t\t}\n \t}\n@@ -5196,8 +5202,9 @@ sub git_snapshot {\n \t\tdie_error(403, \"Unsupported snapshot format\");\n \t}\n \n-\tif (!defined $hash) {\n-\t\t$hash = git_get_head_hash($project);\n+\tmy $full_hash = git_get_full_hash($project, $hash);\n+\tif (!$full_hash) {\n+\t\tdie_error(404, 'Hash id was not valid');\n \t}\n \n \tmy $name = $project;\n@@ -5210,7 +5217,7 @@ sub git_snapshot {\n \t$cmd = quote_command(\n \t\tgit_cmd(), 'archive',\n \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n-\t\t\"--prefix=$name/\", $hash);\n+\t\t\"--prefix=$name/\", $full_hash);\n \tif (exists $known_snapshot_formats{$format}{'compressor'}) {\n \t\t$cmd .= ' | ' . quote_command(@{$known_snapshot_formats{$format}{'compressor'}});\n \t}\ndiff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\nindex d0ff21d..632007e 100644\n--- a/t/t9501-gitweb-standalone-http-status.sh\n+++ b/t/t9501-gitweb-standalone-http-status.sh\n@@ -75,4 +75,33 @@ test_expect_success \\\n test_debug 'cat gitweb.output'\n \n \n+# ----------------------------------------------------------------------\n+# snapshot hash ids\n+\n+test_expect_success \\\n+\t'snapshots: good treeish id' \\\n+\t'gitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success \\\n+\t'snapshots: bad treeish id' \\\n+\t'gitweb_run \"p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz\" &&\n+\tgrep \"404 - Hash id was not valid\" gitweb.output'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success \\\n+\t'snapshots: good object id' \\\n+\t'ID=`git rev-parse --verify HEAD` &&\n+\tgitweb_run \"p=.git;a=snapshot;h=$ID;sf=tgz\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output'\n+test_debug 'cat gitweb.output'\n+\n+test_expect_success \\\n+\t'snapshots: bad object id' \\\n+\t'gitweb_run \"p=.git;a=snapshot;h=abcdef01234;sf=tgz\" &&\n+\tgrep \"404 - Hash id was not valid\" gitweb.output'\n+test_debug 'cat gitweb.output'\n+\n+\n test_done\n-- \n1.6.4.2\n"},{"id":"123023","messageId":"7vmy4z4j35.fsf@alter.siamese.dyndns.org","threadId":"20921","inReplyTo":"4AAC3833.8060905@mailservices.uwaterloo.ca","subject":"Re: [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-13T03:30:38Z","receivedAt":"2009-09-13T03:30:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Rada <marada@uwaterloo.ca> writes:\n\n> Makes things nicer in cases when you hand craft the snapshot URL but\n> make a typo in defining the hash variable (e.g. netx instead of next);\n> you will now get an error message instead of a broken tarball.\n>\n> To maintain backwards compatibility, git_get_head_hash is now a wrapper\n> for git_get_full_hash, as suggested by Jakub Narebski.\n>\n> Tests for t9501 are included to demonstrate added functionality.\n>\n> Signed-off-by: Mark Rada <marada@uwaterloo.ca>\n> ---\n>\n> \tThis is just a re-send based on getting torn a new one by Junio.\n> \tChanges since v3:\n> \t\t- variables have been renamed for readability\n\nMuch nicer to read.  Thanks.\n\n> @@ -5196,8 +5202,9 @@ sub git_snapshot {\n>  \t\tdie_error(403, \"Unsupported snapshot format\");\n>  \t}\n>  \n> -\tif (!defined $hash) {\n> -\t\t$hash = git_get_head_hash($project);\n> +\tmy $full_hash = git_get_full_hash($project, $hash);\n> +\tif (!$full_hash) {\n> +\t\tdie_error(404, 'Hash id was not valid');\n>  \t}\n\nThis is in the context of \"snapshot\", so obviously you care more about\njust \"such an object exists\", don't you?  You also want it to be a\ntree-ish.  Try giving it $hash = 'junio-gpg-pub' and see how it breaks.\n\n> @@ -5210,7 +5217,7 @@ sub git_snapshot {\n>  \t$cmd = quote_command(\n>  \t\tgit_cmd(), 'archive',\n>  \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n> -\t\t\"--prefix=$name/\", $hash);\n> +\t\t\"--prefix=$name/\", $full_hash);\n\nWhy?  There was no justification as to why this change is necessary in the\ncommit log message.\n"},{"id":"123030","messageId":"4AAC8521.1060005@mailservices.uwaterloo.ca","threadId":"20921","inReplyTo":"7vmy4z4j35.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot","fromName":"Mark Rada","fromEmail":"marada@uwaterloo.ca","sentAt":"2009-09-13T05:37:37Z","receivedAt":"2009-09-13T05:37:37Z","isPatch":true,"sender":{"key":"marada@uwaterloo.ca","avatar":"https://avatars.githubusercontent.com/u/38430?v=4"},"body":"On 09-09-12 11:30 PM, Junio C Hamano wrote:\n>> @@ -5196,8 +5202,9 @@ sub git_snapshot {\n>>  \t\tdie_error(403, \"Unsupported snapshot format\");\n>>  \t}\n>>  \n>> -\tif (!defined $hash) {\n>> -\t\t$hash = git_get_head_hash($project);\n>> +\tmy $full_hash = git_get_full_hash($project, $hash);\n>> +\tif (!$full_hash) {\n>> +\t\tdie_error(404, 'Hash id was not valid');\n>>  \t}\n> \n> This is in the context of \"snapshot\", so obviously you care more about\n> just \"such an object exists\", don't you?  You also want it to be a\n> tree-ish.  Try giving it $hash = 'junio-gpg-pub' and see how it breaks.\n \nYou have confused me. How is using 'junio-gpg-pub' different from the \nsecond test case that tries to use 'frizzumFrazzum'?\n\n\n-- \nMark Rada (ferrous26)\nmarada@uwaterloo.ca\n"},{"id":"123032","messageId":"7vy6oj1jug.fsf@alter.siamese.dyndns.org","threadId":"20921","inReplyTo":"4AAC8521.1060005@mailservices.uwaterloo.ca","subject":"Re: [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-13T05:42:31Z","receivedAt":"2009-09-13T05:42:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Rada <marada@uwaterloo.ca> writes:\n\n> On 09-09-12 11:30 PM, Junio C Hamano wrote:\n>>> @@ -5196,8 +5202,9 @@ sub git_snapshot {\n>>>  \t\tdie_error(403, \"Unsupported snapshot format\");\n>>>  \t}\n>>>  \n>>> -\tif (!defined $hash) {\n>>> -\t\t$hash = git_get_head_hash($project);\n>>> +\tmy $full_hash = git_get_full_hash($project, $hash);\n>>> +\tif (!$full_hash) {\n>>> +\t\tdie_error(404, 'Hash id was not valid');\n>>>  \t}\n>> \n>> This is in the context of \"snapshot\", so obviously you care more about\n>> just \"such an object exists\", don't you?  You also want it to be a\n>> tree-ish.  Try giving it $hash = 'junio-gpg-pub' and see how it breaks.\n>  \n> You have confused me. How is using 'junio-gpg-pub' different from the \n> second test case that tries to use 'frizzumFrazzum'?\n\njunio-gpg-pub tag exists in git.git but it tags a blob not a tree.\n\n\t$ git rev-parse junio-gpg-pub\n        6019c27d966fe3ce8adcc0e9f12078eef96ca6ef\n        $ git archive junio-gpg-pub\n        fatal: not a tree object\n"},{"id":"123063","messageId":"200909131703.45806.jnareb@gmail.com","threadId":"20921","inReplyTo":"7vy6oj1jug.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-09-13T15:03:45Z","receivedAt":"2009-09-13T15:03:45Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 13 Sep 2009, Junio C Hamano wrote:\n> Mark Rada <marada@uwaterloo.ca> writes:\n>> On 09-09-12 11:30 PM, Junio C Hamano wrote:\n\n>>>> @@ -5196,8 +5202,9 @@ sub git_snapshot {\n>>>>  \t\tdie_error(403, \"Unsupported snapshot format\");\n>>>>  \t}\n>>>>  \n>>>> -\tif (!defined $hash) {\n>>>> -\t\t$hash = git_get_head_hash($project);\n>>>> +\tmy $full_hash = git_get_full_hash($project, $hash);\n>>>> +\tif (!$full_hash) {\n>>>> +\t\tdie_error(404, 'Hash id was not valid');\n>>>>  \t}\n>>> \n>>> This is in the context of \"snapshot\", so obviously you care more about\n>>> just \"such an object exists\", don't you?  You also want it to be a\n>>> tree-ish.  Try giving it $hash = 'junio-gpg-pub' and see how it breaks.\n>>  \n>> You have confused me. How is using 'junio-gpg-pub' different from the \n>> second test case that tries to use 'frizzumFrazzum'?\n> \n> junio-gpg-pub tag exists in git.git but it tags a blob not a tree.\n> \n> \t$ git rev-parse junio-gpg-pub\n>         6019c27d966fe3ce8adcc0e9f12078eef96ca6ef\n>         $ git archive junio-gpg-pub\n>         fatal: not a tree object\n\nSo the proper solution with respect to snapshot parameters validation\nwould be to use\n\n\tmy $type = git_get_type(\"$hash^{}\");\n\nand check it:\n * if $type is empty or undef (if it is false-ish) then requested object\n   does not exist and we return '404 - No such object' (or something like\n   that)\n * if $type is 'blob' then we return '400 - Object is not a tree-ish'\n   (or something like that)\n * otherwise $type is 'commit' or 'tree'\n\n-- \nJakub Narebski\nPoland\n"}]}