{"thread":{"id":"20914","subject":"[PATCH v3] gitweb: check given hash before trying to create snapshot","startedAt":"2009-09-12T16:32:53Z","lastAt":"2009-09-12T16:32:53Z","messageCount":1,"participants":["Mark Rada"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"122966","messageId":"4AABCD35.9020506@mailservices.uwaterloo.ca","threadId":"20914","inReplyTo":null,"subject":"[PATCH v3] gitweb: check given hash before trying to create snapshot","fromName":"Mark Rada","fromEmail":"marada@uwaterloo.ca","sentAt":"2009-09-12T16:32:53Z","receivedAt":"2009-09-12T16:32:53Z","isPatch":true,"sender":{"key":"marada@uwaterloo.ca","avatar":"https://avatars.githubusercontent.com/u/38430?v=4"},"body":"> [This mail was very strangely wrapped; I fixed this for readability]\nYeah, sorry, format=flowed\n\n> In first test you check that _contents_ contain specific error message,\n> but you do not check if HTTP status code matches it (it should, because\n> of how die_error works).  In second test you check HTTP status.  If the\n> t/t9501-gitweb-standalone-http-status.sh is to be about status, I guess\n> that you should check HTTP status, and not contents of the page (which\n> is more likely to change, e.g. due to some prettifying).\n> \n> In t9501 tests you need, I think, only the HTTP headers part, unless\n> you want also to check that the contents matches.  There was some sed\n> script shown to extract only HTTP headers.\n\nYes, t9501 also checks for the specific error message, this was originally\nhow it had to be done in order to show what trap was being executed for\nthat first block of checks in the snapshot routine. The header does not\ncontain the specific error string that gitweb outputs though, only the\nstandard message (i.e. 404 Not Found), so I check the script output for\nnegative test cases.\n\n\n>> Also, right now gitweb will not accept tags for hashes. This seems to be\n>> because it passes the --verify option to rev-parse, but the output  \n>> from using and not using the verify option seems to be the same (other\n>> than also accepting all tree-ishes). Could you let me know if there is\n>> a good reason not to take off the --verify option? Otherwise, I would\n>> like to take it off in the next version of this patch.\n>\n> Errr, what?\n>\n>  $ 5096:[gitweb/web@git]# git rev-parse --verify v1.5.0            \n>  6db027ffe03210324939b3dd655c4223ca023b45\n>  $ git rev-parse --verify refs/tags/v1.5.0\n>  6db027ffe03210324939b3dd655c4223ca023b45\n>\n> So it works as intended.  The problem must be in some other place.\n\nLet's just say the problem is not in the computer. :(\n\n\n--\nMark Rada (ferrous26)\nmarada@uwaterloo.ca\n\n\n--->8---\nMakes 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 changed functionality.\n\nSigned-off-by: Mark Rada <marada@uwaterloo.ca>\n---\n gitweb/gitweb.perl                       |   13 ++++++++++---\n t/t9501-gitweb-standalone-http-status.sh |   29 +++++++++++++++++++++++++++++\n 2 files changed, 39 insertions(+), 3 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 24b2193..e9cac8d 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1983,11 +1983,17 @@ 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+\tif (open my $fd, '-|', git_cmd(), 'rev-parse', '--verify', $hash) {\n \t\tmy $head = <$fd>;\n \t\tclose $fd;\n \t\tif (defined $head && $head =~ /^([0-9a-fA-F]{40})$/) {\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 $snapshot = git_get_full_hash($project, $hash);\n+\tif (!$snapshot) {\n+\t\tdie_error(404, 'Hash id was not valid');\n \t}\n \n \tmy $name = $project;\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"}]}