From: Luben Tuikov <ltuikov@yahoo.com>
To: Jakub Narebski <jnareb@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH 1/5] gitweb: Cleanup input validation and error messages
Date: Fri, 4 Aug 2006 16:54:14 -0700 (PDT) [thread overview]
Message-ID: <20060804235414.66496.qmail@web31801.mail.mud.yahoo.com> (raw)
In-Reply-To: <200608050038.20534.jnareb@gmail.com>
--- Jakub Narebski <jnareb@gmail.com> wrote:
> Clean up input validation, including removing $rss_link variable and
> making error messages more explicit. Expand and uniquify other error
> messages.
Can you fix your patch then?
Error messages in general do not end with a period.
Luben
>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> This probably conflicts "[PATCH 4/4] gitweb: No periods for error messages".
> It uses periods for error messages which does not end in with some
> value of some variable.
>
> gitweb/gitweb.perl | 88 ++++++++++++++++++++++++----------------------------
> 1 files changed, 40 insertions(+), 48 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 58eb5b1..dfc2d09 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -71,13 +71,15 @@ if (! -d $git_temp) {
> mkdir($git_temp, 0700) || die_error("Couldn't mkdir $git_temp");
> }
>
> +
> +# ======================================================================
> # input validation and dispatch
> our $action = $cgi->param('a');
> if (defined $action) {
> if ($action =~ m/[^0-9a-zA-Z\.\-_]/) {
> - undef $action;
> - die_error(undef, "Invalid action parameter.");
> + die_error(undef, "Invalid action parameter $action");
> }
> + # action which does not check rest of parameters
> if ($action eq "opml") {
> git_opml();
> exit;
> @@ -85,22 +87,17 @@ if (defined $action) {
> }
>
> our $project = ($cgi->param('p') || $ENV{'PATH_INFO'});
> -if (defined $project) {
> - $project =~ s|^/||; $project =~ s|/$||;
> - $project = validate_input($project);
> - if (!defined($project)) {
> - die_error(undef, "Invalid project parameter.");
> +$project =~ s|^/||; $project =~ s|/$||;
> +if (defined $project || $project) {
> + if (!validate_input($project)) {
> + die_error(undef, "Invalid project parameter $project");
> }
> if (!(-d "$projectroot/$project")) {
> - undef $project;
> - die_error(undef, "No such directory.");
> + die_error(undef, "No such directory $project");
> }
> if (!(-e "$projectroot/$project/HEAD")) {
> - undef $project;
> - die_error(undef, "No such project.");
> + die_error(undef, "No such project $project");
> }
> - $rss_link = "<link rel=\"alternate\" title=\"" . esc_param($project) . " log\" href=\"" .
> - "$my_uri?" . esc_param("p=$project;a=rss") . "\" type=\"application/rss+xml\"/>";
> $ENV{'GIT_DIR'} = "$projectroot/$project";
> } else {
> git_project_list();
> @@ -109,49 +106,43 @@ if (defined $project) {
>
> our $file_name = $cgi->param('f');
> if (defined $file_name) {
> - $file_name = validate_input($file_name);
> - if (!defined($file_name)) {
> - die_error(undef, "Invalid file parameter.");
> + if (!validate_input($file_name)) {
> + die_error(undef, "Invalid file parameter $file_name");
> }
> }
>
> our $hash = $cgi->param('h');
> if (defined $hash) {
> - $hash = validate_input($hash);
> - if (!defined($hash)) {
> - die_error(undef, "Invalid hash parameter.");
> + if (!validate_input($hash)) {
> + die_error(undef, "Invalid hash parameter $hash");
> }
> }
>
> our $hash_parent = $cgi->param('hp');
> if (defined $hash_parent) {
> - $hash_parent = validate_input($hash_parent);
> - if (!defined($hash_parent)) {
> - die_error(undef, "Invalid hash parent parameter.");
> + if (!validate_input($hash_parent)) {
> + die_error(undef, "Invalid hash parent parameter $hash_parent");
> }
> }
>
> our $hash_base = $cgi->param('hb');
> if (defined $hash_base) {
> - $hash_base = validate_input($hash_base);
> - if (!defined($hash_base)) {
> - die_error(undef, "Invalid hash base parameter.");
> + if (!validate_input($hash_base)) {
> + die_error(undef, "Invalid hash base parameter $hash_base");
> }
> }
>
> our $page = $cgi->param('pg');
> if (defined $page) {
> if ($page =~ m/[^0-9]$/) {
> - undef $page;
> - die_error(undef, "Invalid page parameter.");
> + die_error(undef, "Invalid page parameter $page");
> }
> }
>
> our $searchtext = $cgi->param('s');
> if (defined $searchtext) {
> if ($searchtext =~ m/[^a-zA-Z0-9_\.\/\-\+\:\@ ]/) {
> - undef $searchtext;
> - die_error(undef, "Invalid search parameter.");
> + die_error(undef, "Invalid search parameter $searchtext");
> }
> $searchtext = quotemeta $searchtext;
> }
> @@ -180,8 +171,7 @@ my %actions = (
>
> $action = 'summary' if (!defined($action));
> if (!defined($actions{$action})) {
> - undef $action;
> - die_error(undef, "Unknown action.");
> + die_error(undef, "Unknown action $action");
> }
> $actions{$action}->();
> exit;
> @@ -871,11 +861,13 @@ sub git_header_html {
> <meta name="robots" content="index, nofollow"/>
> <title>$title</title>
> <link rel="stylesheet" type="text/css" href="$stylesheet"/>
> -$rss_link
> -</head>
> -<body>
> EOF
> - print "<div class=\"page_header\">\n" .
> + print "<link rel=\"alternate\" title=\"" . esc_param($project) . " log\" href=\"" .
> + "$my_uri?" . esc_param("p=$project;a=rss") . "\" type=\"application/rss+xml\"/>\n" .
> + "</head>\n";
> +
> + print "<body>\n" .
> + "<div class=\"page_header\">\n" .
> "<a href=\"http://www.kernel.org/pub/software/scm/git/docs/\" title=\"git
> documentation\">" .
> "<img src=\"$logo\" width=\"72\" height=\"27\" alt=\"git\" style=\"float:right;
> border-width:0px;\"/>" .
> "</a>\n";
> @@ -1471,18 +1463,18 @@ sub git_blame2 {
> my $fd;
> my $ftype;
> die_error(undef, "Permission denied.") if (!git_get_project_config_bool ('blame'));
> - die_error('404 Not Found', "File name not defined") if (!$file_name);
> + die_error('404 Not Found', "File name not defined.") if (!$file_name);
> $hash_base ||= git_read_head($project);
> - die_error(undef, "Reading commit failed") unless ($hash_base);
> + die_error(undef, "Couldn't find base commit.") unless ($hash_base);
> my %co = git_read_commit($hash_base)
> - or die_error(undef, "Reading commit failed");
> + or die_error(undef, "Reading commit failed.");
> if (!defined $hash) {
> $hash = git_get_hash_by_path($hash_base, $file_name, "blob")
> - or die_error(undef, "Error looking up file");
> + or die_error(undef, "Error looking up file $file_name");
> }
> $ftype = git_get_type($hash);
> if ($ftype !~ "blob") {
> - die_error("400 Bad Request", "object is not a blob");
> + die_error("400 Bad Request", "Object is not a blob.");
> }
> open ($fd, "-|", $GIT, "blame", '-l', $file_name, $hash_base)
> or die_error(undef, "Open git-blame failed.");
> @@ -1529,14 +1521,14 @@ sub git_blame2 {
> sub git_blame {
> my $fd;
> die_error('403 Permission denied', "Permission denied.") if (!git_get_project_config_bool
> ('blame'));
> - die_error('404 Not Found', "What file will it be, master?") if (!$file_name);
> + die_error('404 Not Found', "File name not defined.") if (!$file_name);
> $hash_base ||= git_read_head($project);
> - die_error(undef, "Reading commit failed.") unless ($hash_base);
> + die_error(undef, "Couldn't find base commit.") unless ($hash_base);
> my %co = git_read_commit($hash_base)
> or die_error(undef, "Reading commit failed.");
> if (!defined $hash) {
> $hash = git_get_hash_by_path($hash_base, $file_name, "blob")
> - or die_error(undef, "Error lookup file.");
> + or die_error(undef, "Error lookup file $file_name");
> }
> open ($fd, "-|", $GIT, "annotate", '-l', '-t', '-r', $file_name, $hash_base)
> or die_error(undef, "Open git-annotate failed.");
> @@ -1649,7 +1641,7 @@ sub git_blob_plain {
> if (defined $file_name) {
> my $base = $hash_base || git_read_head($project);
> $hash = git_get_hash_by_path($base, $file_name, "blob")
> - or die_error(undef, "Error lookup file.");
> + or die_error(undef, "Error lookup file $file_name");
> } else {
> die_error(undef, "No file name defined.");
> }
> @@ -1682,7 +1674,7 @@ sub git_blob {
> if (defined $file_name) {
> my $base = $hash_base || git_read_head($project);
> $hash = git_get_hash_by_path($base, $file_name, "blob")
> - or die_error(undef, "Error lookup file.");
> + or die_error(undef, "Error lookup file $file_name");
> } else {
> die_error(undef, "No file name defined.");
> }
> @@ -2122,7 +2114,7 @@ sub git_commitdiff {
> open my $fd, "-|", $GIT, "diff-tree", '-r', $hash_parent, $hash
> or die_error(undef, "Open git-diff-tree failed.");
> my @difftree = map { chomp; $_ } <$fd>;
> - close $fd or die_error(undef, "Reading diff-tree failed.");
> + close $fd or die_error(undef, "Reading git-diff-tree failed.");
>
> # non-textual hash id's can be cached
> my $expires;
> @@ -2202,7 +2194,7 @@ sub git_commitdiff_plain {
> open my $fd, "-|", $GIT, "diff-tree", '-r', $hash_parent, $hash
> or die_error(undef, "Open git-diff-tree failed.");
> my @difftree = map { chomp; $_ } <$fd>;
> - close $fd or die_error(undef, "Reading diff-tree failed.");
> + close $fd or die_error(undef, "Reading git-diff-tree failed.");
>
> # try to figure out the next tag after this commit
> my $tagname;
> @@ -2493,7 +2485,7 @@ sub git_rss {
> open my $fd, "-|", $GIT, "rev-list", "--max-count=150", git_read_head($project)
> or die_error(undef, "Open git-rev-list failed.");
> my @revlist = map { chomp; $_ } <$fd>;
> - close $fd or die_error(undef, "Reading rev-list failed.");
> + close $fd or die_error(undef, "Reading git-rev-list failed.");
> print $cgi->header(-type => 'text/xml', -charset => 'utf-8');
> print "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n".
> "<rss version=\"2.0\" xmlns:content=\"http://purl.org/rss/1.0/modules/content/\">\n";
> --
> 1.4.1.1
>
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
next prev parent reply other threads:[~2006-08-04 23:54 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-08-04 22:36 [PATCH 0/5] Some further gitweb patches Jakub Narebski
2006-08-04 22:38 ` [PATCH 1/5] gitweb: Cleanup input validation and error messages Jakub Narebski
2006-08-04 23:54 ` Luben Tuikov [this message]
2006-08-05 0:02 ` [PATCH 6/5] gitweb: No periods for " Jakub Narebski
2006-08-04 23:54 ` [PATCH 1/5] gitweb: Cleanup input validation and " Luben Tuikov
2006-08-05 0:15 ` Junio C Hamano
2006-08-05 0:26 ` Jakub Narebski
2006-08-05 10:51 ` [PATCH 0/9] gitweb: First patch corrected and split into separate patches Jakub Narebski
2006-08-05 10:55 ` [PATCH 1/9] gitweb: Separate input validation and dispatch, add comment about opml action Jakub Narebski
2006-08-05 10:56 ` [PATCH 2/9] gitweb: die_error first (optional) parameter is HTTP status Jakub Narebski
2006-08-05 10:56 ` [PATCH 3/9] gitweb: Use undef for die_error to use default first (status) parameter value Jakub Narebski
2006-08-05 10:58 ` [PATCH 4/9] gitweb: Don't undefine query parameter related variables before die_error Jakub Narebski
2006-08-05 11:12 ` [PATCH 5/9] gitweb: Cleanup and uniquify error messages Jakub Narebski
2006-08-05 11:13 ` [PATCH 6/9] gitweb: No periods for " Jakub Narebski
2006-08-05 15:55 ` Luben Tuikov
2006-08-05 16:15 ` Jakub Narebski
2006-08-05 11:15 ` [PATCH 7/9] gitweb: No error messages with unescaped/unprotected user input Jakub Narebski
2006-08-05 11:16 ` [PATCH 8/9] gitweb: PATH_INFO=/ means no project Jakub Narebski
2006-08-05 11:18 ` [PATCH 9/9] gitweb: Inline $rss_link Jakub Narebski
2006-08-04 22:39 ` [PATCH 2/5] gitweb: Great subroutines renaming Jakub Narebski
2006-08-04 22:40 ` [PATCH 3/5] gitweb: Separate ref parsing in git_read_refs into parse_ref Jakub Narebski
2006-08-04 22:42 ` [PATCH 4/5] gitweb: git_heads cleanup Jakub Narebski
2006-08-04 22:43 ` [PATCH 5/5] gitweb: Change appereance of marker of refs pointing to given object Jakub Narebski
2006-08-05 11:42 ` [PATCH 7/5] Merge changes in "split patch 1" series Jakub Narebski
2006-08-05 14:55 ` Johannes Schindelin
2006-08-05 15:05 ` Jakub Narebski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20060804235414.66496.qmail@web31801.mail.mud.yahoo.com \
--to=ltuikov@yahoo.com \
--cc=git@vger.kernel.org \
--cc=jnareb@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).