git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/5] gitweb: Cleanup input validation and error messages

From
LTLuben Tuikov <ltuikov@yahoo.com>
Date
Aug 4, 2006, 23:54 UTC
Message-ID
<20060804235414.66496.qmail@web31801.mail.mud.yahoo.com>
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
Show 247 quoted lines
> 
> 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
> 
Previous: Jakub NarebskiNext: Jakub Narebski
Message 3 of 26 in “Some further gitweb patches”
  1. 0/5 Some further gitweb patchesJakub Narebski, Aug 4, 2006
  2. 1/5 gitweb: Cleanup input validation and error messagesJakub Narebski, Aug 4, 2006
  3. Luben TuikovAug 4, 2006
  4. 6/5 gitweb: No periods for error messagesJakub Narebski, Aug 5, 2006
  5. Luben TuikovAug 4, 2006
  6. Junio C HamanoAug 5, 2006
  7. Jakub NarebskiAug 5, 2006
  8. 0/9 gitweb: First patch corrected and split into separate patchesJakub Narebski, Aug 5, 2006
  9. 1/9 gitweb: Separate input validation and dispatch, add comment about opml actionJakub Narebski, Aug 5, 2006
  10. 2/9 gitweb: die_error first (optional) parameter is HTTP statusJakub Narebski, Aug 5, 2006
  11. 3/9 gitweb: Use undef for die_error to use default first (status) parameter valueJakub Narebski, Aug 5, 2006
  12. 4/9 gitweb: Don't undefine query parameter related variables before die_errorJakub Narebski, Aug 5, 2006
  13. 5/9 gitweb: Cleanup and uniquify error messagesJakub Narebski, Aug 5, 2006
  14. 6/9 gitweb: No periods for error messagesJakub Narebski, Aug 5, 2006
  15. Luben TuikovAug 5, 2006
  16. Jakub NarebskiAug 5, 2006
  17. 7/9 gitweb: No error messages with unescaped/unprotected user inputJakub Narebski, Aug 5, 2006
  18. 8/9 gitweb: PATH_INFO=/ means no projectJakub Narebski, Aug 5, 2006
  19. 9/9 gitweb: Inline $rss_linkJakub Narebski, Aug 5, 2006
  20. 2/5 gitweb: Great subroutines renamingJakub Narebski, Aug 4, 2006
  21. 3/5 gitweb: Separate ref parsing in git_read_refs into parse_refJakub Narebski, Aug 4, 2006
  22. 4/5 gitweb: git_heads cleanupJakub Narebski, Aug 4, 2006
  23. 5/5 gitweb: Change appereance of marker of refs pointing to given objectJakub Narebski, Aug 4, 2006
  24. 7/5 Merge changes in "split patch 1" seriesJakub Narebski, Aug 5, 2006
  25. Johannes SchindelinAug 5, 2006
  26. Jakub NarebskiAug 5, 2006

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.