$SQL = "UPDATE $regtable SET favorites='$favorites' WHERE ID='$ID'";
[download]

my $sth = $dbh->prepare(qq{UPDATE $regtable SET favorites=? WHERE ID=?
+});
$sth->execute($favorites, $ID) or die $dbh->errstr;
[download]

That way the escaping is done for you, and you avoid sql injection attacks.

Also, I advise you to use strict and use warnings. It appears that you're relying on global variables instead of passing parameters to your subs. This is very bad practice.

Thanks for the suggestions. Here is the Do_Sql sub referred to in my code. Is this still bad practice?

$SQL = "UPDATE $regtable SET favorites='$favorites' WHERE ID='$ID'";
&Do_SQL;


sub Do_SQL{
  eval {
    $sth = $dbh->prepare($SQL);
  }; # end of eval

  # check for errors
  if($@){
    $dbh->disconnect;
    print "Content-type: text/html\n\n";
    print "An ERROR occurred! $@\n";
    exit;
  } else {
    $sth->execute;
  } # end of if/else
  return ($sth);
}
[download]

Yes, there are a few problems with that code:

• 1) Your subroutine relies on the global variable $SQL, instead of a passed parameter.
• 2) Your $SQL statements are vulnerable to injection attacks since they aren't escaped and/or you're not using placeholders.
• 3) You rolling your own to browser error reporting, instead of letting CGI::Carp do the work for you.

#!/usr/bin/perl -wT

use CGI;
use CGI::Carp qw(fatalsToBrowser);
use DBI;

use strict;
use warnings;

...

    my $sth = $dbh->prepare(qq{UPDATE $regtable SET favorites=? WHERE 
+ID=?});
    $sth->execute($favorites, $ID) or die $dbh->errstr;

...
[download]

The above is all you need to accomplish the same thing. If there is an error with your statement, then CGI::Carp will trap it and display it in the browser and also the error log.

Congratulations, you appear to be preventing sql injection, but your program flow is buried -- a function called Do_SQL usually has no business calling exit Try something like this

Main(@ARGS);
exit(0);
sub Main {
   my $query = CGI->new;
   my $ret = eval { DoSql($query); };
   if( $@ ){
      print_error_response();
   } else {
      print_results($ret, $query);
   }
}
[download]

See CGI::Application/Catalyst/CGI::Prototype for different ways of simplifying this separation of concerns

You'll have way fewer problems if you were to think a bit more abstractly. The things you really want to do are:

• Convert text pairs into a URL query.
• Convert text into a JS string literal.
• Convert text into an HTML attribute.
• Convert text into an SQL string literal.

So make a function for each of them!

sub build_url {
   my $url = URI->new(shift(@_));
   $url->query_form(@_);
   return $url;
}

sub text_to_js_lit {
   my $s = $_[0];
   $s =~ s/\\/\\\\/g;
   $s =~ s/'/\''/g;
   return "'$s'";
}

sub text_to_html_val {
   my $s = $_[0];
   $s =~ s/&/&/g;
   $s =~ s/</&lt;/g;
   $s =~ s/"/&quote;/g;
   return qq{"$s"};
}
[download]

my $url = build_url('/cgi-bin/script.pl',
   favadd   => 'yes',
   num      => '6210',
   favtitle => "jones's title",
   favlink  => '6210.html',
);

my $js_url = 'javascript:popUp(' . text_to_js_lit($url) . ')';

print('<a href=' . text_to_html_val($js_url) . '>');
[download]

As for converting text into an SQL string literal, the function already exists as $dbh->quote(). That said, you'd be better off using placeholders as previously mentioned.