legLess has asked for the wisdom of the Perl Monks concerning the following question:

Monks ~

I'm writing a multi-form CGI script. Thanks to some input I've received here it's going quite well.

I'm currently using this code, where I slurp all CGI params into a hash, then pass that hash to each sub. Some subs add key/value pairs (parameters, basically), so I return the whole hash or some part of it. Some other parts of the code add parameters, too. 

    my $q   = CGI->new();
    my %prm = $q->Vars();
...
    if ($prm{'action'} eq 'Login') {
        if ($prm{'mid'}) {
            $prm{'warning'} = 'Already logged in, bonehead.';
            show_member($q,%prm);
        } else {
            do_login($q,%prm);
        }
    }

[download]
And I'm thinking about something like this, where I dispense with the hash entirely and use just the $q CGI query object. 
    my $q   = CGI->new();
...
    if ($q->param('action') eq 'Login') {
        if ($q->param('mid')) {
            $q->param('warning') = 'Already logged in, bonehead.';
            show_member($q);
        } else {
            do_login($q);
        }
    }

[download]
I started this a couple weeks ago, and I know an order of magnitude more Perl now, so it's good to re-examine my previous assumptions. I'd guess that the first example would be a little quicker, without the OO overhead. The second example looks a little cleaner, and is probably easier to maintain (this alone argues in its favor).

(I did a quick search/replace on the file and it nailed 115 instances (in ~700 lines) of $prm{'x'}, replacing them with $q->param('x').)

It seems to me that the second example is superior in many ways, but I'd enjoy comments from any passing Monks.

Thanks.
--
man with no legs, inc.

Replies are listed 'Best First'.
Re: CGI OO 'param' vs. hash
by tadman (Prior) on Jul 09, 2001 at 22:29 UTC
    I prefer passing the $q query object in its entirety for a few reasons. In particular, it is always handy to be able to get "other" information from it without having to pass this extra stuff explicitly. For example, if you had one function that not only needed the parameters but the REMOTE_HOST info, you would have to extract this and pass it explicitly.

    Better to just toss the $q variable into the function and extract params like you would.

    Don't forget, though, that since you are extracting the parameters directly from the CGI object, they are tainted until decontaminated by your program. This decontamination can be done by a simple module which is used throughout your program, or by sub-classing CGI to build in this functionality. Consider: 
    # Regular CGI
my $quantity = $q->param('quantity');            # Tainted

# "Fancy" CGI of your own construction
my $quantity = $q->SafeNumberParam('quantity');  # De-tainted

    [download]
    Where your SafeNumberParam function might look like: 
    sub SafeNumberParam
{
    my ($self)  = shift;
    my ($param) = @_;

    my ($number) = $self->param($param) =~ /^(\d+)/;

    return $number;
}

    [download]
[reply]
[d/l]
[select]

      Another good reason to subclass CGI.pm would be to get rid of that Vars() method and replace it with one that uses data structures properly.

      sub Vars {
    my $self = shift;
    my %formdata;
    foreach my $name ( $self->param ) {
        my @values = $self->param( $name );
        $formdata{ $name } = scalar @values == 1 ? $values[0] :
                                                  \@values    ;
    }
    return \%formdata;
}

      [download]

      Of course, that's quick-n-dirty and I haven't tested it. It has the nice effect of getting rid of all of those nasty ASCII zeroes in the original Vars function, thus stopping a nasty potential security hole.

      Cheers,
      Ovid

      Vote for paco!

      Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.

[reply]
[d/l]
        Quoth Ovid:
        Of course, that's quick-n-dirty and I haven't tested it. It has the nice effect of getting rid of all of those nasty ASCII zeroes in the original Vars function, thus stopping a nasty potential security hole.
        Can you elaborate, or shoot me a URL? I have no idea what you mean.
        --
        man with no legs, inc.
[reply]
      Thanks tadman, that's a nice WTDI. I do validate every CGI param coming from the user before I stick it into the database. That way I can warn them and give them another shot at getting it right. 
      if ($prm{'name'} =~ s/[^\w \-]//g) {
    $warning .= '<LI>Illegal characters were removed from your Login N
+ame. Only letters, numbers, spaces, underscores and dashes are allowe
+d.';
    }

      [download]

      Your way is more elegant though. Gives me something to think about.
      --
      man with no legs, inc.

[reply]
[d/l]
(Ovid) Re: CGI OO 'param' vs. hash
by Ovid (Cardinal) on Jul 09, 2001 at 23:27 UTC

    legLess wrote:

    (I did a quick search/replace on the file and it nailed 115 instances (in ~700 lines) of $prm{'x'}, replacing them with $q->param('x').)

    Are all 115 instances going to be called every time? For a 700 line script, I suspect not. Usually, when I get a script that size, it's a variety of different modes and not all are used on any particular invocation. I'm in tadman's camp on this one. Using the CGI object is cleaner because you'll probably use it anyway and why pass the data twice?

    I am, however, concerned about something that I am seeing:

        my $q   = CGI->new();
...
    if ($q->param('action') eq 'Login') {
        if ($q->param('mid')) {    ############# <---- What's this?
            $q->param('warning') = 'Already logged in, bonehead.';
            show_member($q);
        } else {
            do_login($q);
        }
    }

    [download]

    What that seems to suggest is that I should take your form, save it to my box, make sure that it's pointing at your server, and insert a hidden field with the name 'mid' and with a value that evaluates as true. If I do that, your script thinks I'm logged in. At least, that's what the code snippet suggests. How does authentication occur here?

    Cheers,
    Ovid

    Vote for paco!

    Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.

[reply]
[d/l]
      Good eye, Ovid, and thanks for the reply. The mid is a member ID number that's returned from the get_member sub. It does it's work where the "..." snip is.

      Right now the mid comes right out of a cookie, so yes, you could edit your cookie and be logged in as whoever you chose. But this is nowhere near production yet; here are my plans:

      First, I may use session cookies, which will help a little.

      Second, I've got a member_info table, then a logged_in_members table with 3 columns: member id, login time, and hash. The hash is some sort of one-way random-looking collection of garbage characters (MD5, maybe), and it's what I'm going to store in the cookie. You login via SSL, then the cookie is set with said hash (different for every login). That'll be checked at each page request against the logged_in_members table to authenticate, then blown away from that table when (a) the user logs out, or (b) x time has passed.

      Regardless of what parameters are passed to the script, mid is set every time by the script itself - either a member id or 0 - it's never accepted as a CGI param.

      Potential problems: I'm not likely to run the entire site under SSL, so someone could sniff the hash value as it's being sent over HTTP to the server. This would allow them access to the member's info (no CC #s, ever), and allow them to request a password and/or email address change. Password and email changes will have to be authenticated by replying to an email sent from the server (or something similar, e.g. navigating to a link in that email which contains a "?verify=<hash>"). So the attacker would not only have to sniff the hash and setup their own cookie, but intercept the email coming from the server and reply to it. I know this is possible, but I consider it unlikely enough that the risk is acceptable.

      I don't know if this is the best possible method, but it's one I've seen recommended by people who seem to know what they're doing. I'm not a security expert, but it seems pretty good to me. What do you think?
      --
      man with no legs, inc.

[reply]
Re: CGI OO 'param' vs. hash
by TGI (Parson) on Jul 10, 2001 at 05:11 UTC

    Although I am mostly just repeating what everyone else has said, use the CGI param() method, I can testify from recent maintenance experience that it is a good thing to do.

    Last month I had to update a CGI script I wrote over a year ago. It formats stuff from a data file based on settings from a config file. I needed to give that data file the ability to specify which config file to use (formerly automagic). Happily, I used the CGI object to handle the internal passing of config file choice. Because I used the CGI object in a fit of laziness, it was trivial to add the new feature. Had I been more diligent and manually passed the info, I would have had to make some ugly changes to the code to get the configuration info passed around properly.

    PS: Don't let unchecked params anywhere near your filesystem.


    TGI says moo

[reply]
[d/l]

Back to Seekers of Perl Wisdom