* [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot
@ 2009-09-12 23:03 Mark Rada
0 siblings, 0 replies; 6+ messages in thread
From: Mark Rada @ 2009-09-12 23:03 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Junio C Hamano
I changed some variable names to be nicer looking.
--
Mark Rada (ferrous26)
marada@uwaterloo.ca
--->8---
Makes things nicer in cases when you hand craft the snapshot URL but
make a typo in defining the hash variable (e.g. netx instead of next);
you will now get an error message instead of a broken tarball.
To maintain backwards compatibility, git_get_head_hash is now a wrapper
for git_get_full_hash, as suggested by Jakub Narebski.
Tests for t9501 are included to demonstrate added functionality.
Signed-off-by: Mark Rada <marada@uwaterloo.ca>
---
gitweb/gitweb.perl | 19 +++++++++++++------
t/t9501-gitweb-standalone-http-status.sh | 29 +++++++++++++++++++++++++++++
2 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 24b2193..e1beca5 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1983,14 +1983,20 @@ sub quote_command {
# get HEAD ref of given project as hash
sub git_get_head_hash {
+ return git_get_full_hash(shift, 'HEAD');
+}
+
+# given a project and tree-ish, returns full hash
+sub git_get_full_hash {
my $project = shift;
+ my $hash = shift;
my $o_git_dir = $git_dir;
my $retval = undef;
$git_dir = "$projectroot/$project";
- if (open my $fd, "-|", git_cmd(), "rev-parse", "--verify", "HEAD") {
- my $head = <$fd>;
+ if (open my $fd, '-|', git_cmd(), 'rev-parse', '--verify', $hash) {
+ $hash = <$fd>;
close $fd;
- if (defined $head && $head =~ /^([0-9a-fA-F]{40})$/) {
+ if (defined $hash && $hash =~ /^([0-9a-fA-F]{40})$/) {
$retval = $1;
}
}
@@ -5196,8 +5202,9 @@ sub git_snapshot {
die_error(403, "Unsupported snapshot format");
}
- if (!defined $hash) {
- $hash = git_get_head_hash($project);
+ my $full_hash = git_get_full_hash($project, $hash);
+ if (!$full_hash) {
+ die_error(404, 'Hash id was not valid');
}
my $name = $project;
@@ -5210,7 +5217,7 @@ sub git_snapshot {
$cmd = quote_command(
git_cmd(), 'archive',
"--format=$known_snapshot_formats{$format}{'format'}",
- "--prefix=$name/", $hash);
+ "--prefix=$name/", $full_hash);
if (exists $known_snapshot_formats{$format}{'compressor'}) {
$cmd .= ' | ' . quote_command(@{$known_snapshot_formats{$format}{'compressor'}});
}
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index d0ff21d..632007e 100644
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -75,4 +75,33 @@ test_expect_success \
test_debug 'cat gitweb.output'
+# ----------------------------------------------------------------------
+# snapshot hash ids
+
+test_expect_success \
+ 'snapshots: good treeish id' \
+ 'gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+ grep "Status: 200 OK" gitweb.output'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+ 'snapshots: bad treeish id' \
+ 'gitweb_run "p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz" &&
+ grep "404 - Hash id was not valid" gitweb.output'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+ 'snapshots: good object id' \
+ 'ID=`git rev-parse --verify HEAD` &&
+ gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
+ grep "Status: 200 OK" gitweb.output'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+ 'snapshots: bad object id' \
+ 'gitweb_run "p=.git;a=snapshot;h=abcdef01234;sf=tgz" &&
+ grep "404 - Hash id was not valid" gitweb.output'
+test_debug 'cat gitweb.output'
+
+
test_done
--
1.6.4.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot
@ 2009-09-13 0:09 Mark Rada
2009-09-13 3:30 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Mark Rada @ 2009-09-13 0:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narebski
Makes things nicer in cases when you hand craft the snapshot URL but
make a typo in defining the hash variable (e.g. netx instead of next);
you will now get an error message instead of a broken tarball.
To maintain backwards compatibility, git_get_head_hash is now a wrapper
for git_get_full_hash, as suggested by Jakub Narebski.
Tests for t9501 are included to demonstrate added functionality.
Signed-off-by: Mark Rada <marada@uwaterloo.ca>
---
This is just a re-send based on getting torn a new one by Junio.
Changes since v3:
- variables have been renamed for readability
gitweb/gitweb.perl | 19 +++++++++++++------
t/t9501-gitweb-standalone-http-status.sh | 29 +++++++++++++++++++++++++++++
2 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 24b2193..e1beca5 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1983,14 +1983,20 @@ sub quote_command {
# get HEAD ref of given project as hash
sub git_get_head_hash {
+ return git_get_full_hash(shift, 'HEAD');
+}
+
+# given a project and tree-ish, returns full hash
+sub git_get_full_hash {
my $project = shift;
+ my $hash = shift;
my $o_git_dir = $git_dir;
my $retval = undef;
$git_dir = "$projectroot/$project";
- if (open my $fd, "-|", git_cmd(), "rev-parse", "--verify", "HEAD") {
- my $head = <$fd>;
+ if (open my $fd, '-|', git_cmd(), 'rev-parse', '--verify', $hash) {
+ $hash = <$fd>;
close $fd;
- if (defined $head && $head =~ /^([0-9a-fA-F]{40})$/) {
+ if (defined $hash && $hash =~ /^([0-9a-fA-F]{40})$/) {
$retval = $1;
}
}
@@ -5196,8 +5202,9 @@ sub git_snapshot {
die_error(403, "Unsupported snapshot format");
}
- if (!defined $hash) {
- $hash = git_get_head_hash($project);
+ my $full_hash = git_get_full_hash($project, $hash);
+ if (!$full_hash) {
+ die_error(404, 'Hash id was not valid');
}
my $name = $project;
@@ -5210,7 +5217,7 @@ sub git_snapshot {
$cmd = quote_command(
git_cmd(), 'archive',
"--format=$known_snapshot_formats{$format}{'format'}",
- "--prefix=$name/", $hash);
+ "--prefix=$name/", $full_hash);
if (exists $known_snapshot_formats{$format}{'compressor'}) {
$cmd .= ' | ' . quote_command(@{$known_snapshot_formats{$format}{'compressor'}});
}
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index d0ff21d..632007e 100644
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -75,4 +75,33 @@ test_expect_success \
test_debug 'cat gitweb.output'
+# ----------------------------------------------------------------------
+# snapshot hash ids
+
+test_expect_success \
+ 'snapshots: good treeish id' \
+ 'gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+ grep "Status: 200 OK" gitweb.output'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+ 'snapshots: bad treeish id' \
+ 'gitweb_run "p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz" &&
+ grep "404 - Hash id was not valid" gitweb.output'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+ 'snapshots: good object id' \
+ 'ID=`git rev-parse --verify HEAD` &&
+ gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
+ grep "Status: 200 OK" gitweb.output'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+ 'snapshots: bad object id' \
+ 'gitweb_run "p=.git;a=snapshot;h=abcdef01234;sf=tgz" &&
+ grep "404 - Hash id was not valid" gitweb.output'
+test_debug 'cat gitweb.output'
+
+
test_done
--
1.6.4.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot
2009-09-13 0:09 Mark Rada
@ 2009-09-13 3:30 ` Junio C Hamano
2009-09-13 5:37 ` Mark Rada
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2009-09-13 3:30 UTC (permalink / raw)
To: Mark Rada; +Cc: git, Jakub Narebski
Mark Rada <marada@uwaterloo.ca> writes:
> Makes things nicer in cases when you hand craft the snapshot URL but
> make a typo in defining the hash variable (e.g. netx instead of next);
> you will now get an error message instead of a broken tarball.
>
> To maintain backwards compatibility, git_get_head_hash is now a wrapper
> for git_get_full_hash, as suggested by Jakub Narebski.
>
> Tests for t9501 are included to demonstrate added functionality.
>
> Signed-off-by: Mark Rada <marada@uwaterloo.ca>
> ---
>
> This is just a re-send based on getting torn a new one by Junio.
> Changes since v3:
> - variables have been renamed for readability
Much nicer to read. Thanks.
> @@ -5196,8 +5202,9 @@ sub git_snapshot {
> die_error(403, "Unsupported snapshot format");
> }
>
> - if (!defined $hash) {
> - $hash = git_get_head_hash($project);
> + my $full_hash = git_get_full_hash($project, $hash);
> + if (!$full_hash) {
> + die_error(404, 'Hash id was not valid');
> }
This is in the context of "snapshot", so obviously you care more about
just "such an object exists", don't you? You also want it to be a
tree-ish. Try giving it $hash = 'junio-gpg-pub' and see how it breaks.
> @@ -5210,7 +5217,7 @@ sub git_snapshot {
> $cmd = quote_command(
> git_cmd(), 'archive',
> "--format=$known_snapshot_formats{$format}{'format'}",
> - "--prefix=$name/", $hash);
> + "--prefix=$name/", $full_hash);
Why? There was no justification as to why this change is necessary in the
commit log message.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot
2009-09-13 3:30 ` Junio C Hamano
@ 2009-09-13 5:37 ` Mark Rada
2009-09-13 5:42 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Mark Rada @ 2009-09-13 5:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narebski
On 09-09-12 11:30 PM, Junio C Hamano wrote:
>> @@ -5196,8 +5202,9 @@ sub git_snapshot {
>> die_error(403, "Unsupported snapshot format");
>> }
>>
>> - if (!defined $hash) {
>> - $hash = git_get_head_hash($project);
>> + my $full_hash = git_get_full_hash($project, $hash);
>> + if (!$full_hash) {
>> + die_error(404, 'Hash id was not valid');
>> }
>
> This is in the context of "snapshot", so obviously you care more about
> just "such an object exists", don't you? You also want it to be a
> tree-ish. Try giving it $hash = 'junio-gpg-pub' and see how it breaks.
You have confused me. How is using 'junio-gpg-pub' different from the
second test case that tries to use 'frizzumFrazzum'?
--
Mark Rada (ferrous26)
marada@uwaterloo.ca
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot
2009-09-13 5:37 ` Mark Rada
@ 2009-09-13 5:42 ` Junio C Hamano
2009-09-13 15:03 ` Jakub Narebski
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2009-09-13 5:42 UTC (permalink / raw)
To: Mark Rada; +Cc: Junio C Hamano, git, Jakub Narebski
Mark Rada <marada@uwaterloo.ca> writes:
> On 09-09-12 11:30 PM, Junio C Hamano wrote:
>>> @@ -5196,8 +5202,9 @@ sub git_snapshot {
>>> die_error(403, "Unsupported snapshot format");
>>> }
>>>
>>> - if (!defined $hash) {
>>> - $hash = git_get_head_hash($project);
>>> + my $full_hash = git_get_full_hash($project, $hash);
>>> + if (!$full_hash) {
>>> + die_error(404, 'Hash id was not valid');
>>> }
>>
>> This is in the context of "snapshot", so obviously you care more about
>> just "such an object exists", don't you? You also want it to be a
>> tree-ish. Try giving it $hash = 'junio-gpg-pub' and see how it breaks.
>
> You have confused me. How is using 'junio-gpg-pub' different from the
> second test case that tries to use 'frizzumFrazzum'?
junio-gpg-pub tag exists in git.git but it tags a blob not a tree.
$ git rev-parse junio-gpg-pub
6019c27d966fe3ce8adcc0e9f12078eef96ca6ef
$ git archive junio-gpg-pub
fatal: not a tree object
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot
2009-09-13 5:42 ` Junio C Hamano
@ 2009-09-13 15:03 ` Jakub Narebski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Narebski @ 2009-09-13 15:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Mark Rada, git
On Sun, 13 Sep 2009, Junio C Hamano wrote:
> Mark Rada <marada@uwaterloo.ca> writes:
>> On 09-09-12 11:30 PM, Junio C Hamano wrote:
>>>> @@ -5196,8 +5202,9 @@ sub git_snapshot {
>>>> die_error(403, "Unsupported snapshot format");
>>>> }
>>>>
>>>> - if (!defined $hash) {
>>>> - $hash = git_get_head_hash($project);
>>>> + my $full_hash = git_get_full_hash($project, $hash);
>>>> + if (!$full_hash) {
>>>> + die_error(404, 'Hash id was not valid');
>>>> }
>>>
>>> This is in the context of "snapshot", so obviously you care more about
>>> just "such an object exists", don't you? You also want it to be a
>>> tree-ish. Try giving it $hash = 'junio-gpg-pub' and see how it breaks.
>>
>> You have confused me. How is using 'junio-gpg-pub' different from the
>> second test case that tries to use 'frizzumFrazzum'?
>
> junio-gpg-pub tag exists in git.git but it tags a blob not a tree.
>
> $ git rev-parse junio-gpg-pub
> 6019c27d966fe3ce8adcc0e9f12078eef96ca6ef
> $ git archive junio-gpg-pub
> fatal: not a tree object
So the proper solution with respect to snapshot parameters validation
would be to use
my $type = git_get_type("$hash^{}");
and check it:
* if $type is empty or undef (if it is false-ish) then requested object
does not exist and we return '404 - No such object' (or something like
that)
* if $type is 'blob' then we return '400 - Object is not a tree-ish'
(or something like that)
* otherwise $type is 'commit' or 'tree'
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-09-13 15:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-12 23:03 [RFC/PATCH v4 1/2] gitweb: check given hash before trying to create snapshot Mark Rada
-- strict thread matches above, loose matches on Subject: below --
2009-09-13 0:09 Mark Rada
2009-09-13 3:30 ` Junio C Hamano
2009-09-13 5:37 ` Mark Rada
2009-09-13 5:42 ` Junio C Hamano
2009-09-13 15:03 ` Jakub Narebski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).