Beefy Boxes and Bandwidth Generously Provided by pair Networks
P is for Practical

Re: Help understanding this script (Russ: waving red flags)

by Russ (Deacon)
on Jul 18, 2002 at 17:14 UTC ( [id://182916]=note: print w/replies, xml ) Need Help??

in reply to Help understanding this script

WarrenBullockIII, I'd be happy to help. However, before I answer your specific question, we need to discuss some things. Have a seat, son, this may take a while :-)

#!/usr/bin/perl This should be: #!/usr/bin/perl -Tw You should always, Always, ALWAYS, ALWAYS develop with warnings enabled. You should use taint mode when dealing with potentially dangerous user data. As we will see below, you are likely dealing with dangerous data, here.

use strict;
This line is missing. See above for how often to use strict (hint, it starts with "always")
use CGI; $query = new CGI; $topic_directory = "topics";
You are using the CGI module. Good call. You are also using global variables (which 'use strict' will not allow). Not the end of the world, but most of us consider this a bad habit. Let's put my in front of these variable declarations, just to satisfy the purists. Your script here won't notice the difference, much, and I won't have to cringe. :-)

Eliminating global variables means we will have to pass arguments to the subroutines you call, but that's a simple change to make, and will greatly improve all your code's maintainability. It's worth it.

if($query->param('topic')){ $topic = $query->param('topic'); &file_error unless (&open_topic);
Here is where we are assigning to $topic. If you got one from the browser (either from a hidden field (not in this code example), or when the user clicked on a topic in your index), we store it in $topic here. I think this was your original question...

Let's talk about &file_error. Someone is afraid of parentheses, it appears. Perl invokes a special "side-effect" when you call a subroutine with & and no parens. It passes the caller's @_ to the called subroutine. Of course, here, you don't have @_ (since we are at file scope when you make the call). So, this code is making a specific statement about its intent, but it doesn't do what it says it does. This is bad for maintainability. Don't use &function_call unless you really intend to pass your @_ to the subroutine you are calling. function_call() does the same thing, based on how you are using it, but doesn't lie about what it's doing. ;-)

Now let's talk about &open_topic. The code is not shown here, but I am going to assume the worst (good security types do that instinctively). param('topic') is data under the user's control. This data is probably used in open_topic() to open a file. If open_topic() is not EXTREMELY careful with $topic, it could allow a user to open literally any file on your server. For example, the file containing all the user's passwords. Specific mechanisms are beyond the scope of my intent here, but search for "null character" and "cgi security" for details.

This is what taint mode will help you identify. It would force you to sanitize $topic (because it came from the user) before you could use it for anything dangerous (like opening files on the server). How you clean up data like this is also beyond this scope, for now, but let me say here that open_topic is very suspicious. You may be opening your server to a very simple attack. Learn about taint mode and CGI security before you go too far with this.

sub print_response_form{ print "<FORM METHOD=\"post\" ACTION=\"\">\n"; print "<INPUT TYPE=\"hidden\" NAME=\"action\" ";
Unless you really like looking at backslash-spaghetti <grin>, use perl's quoting operators to clean this up.
# isn't this print qq{<FORM METHOD="post" ACTION="">\n}; # better than this? print "<FORM METHOD=\"post\" ACTION=\"\">\n";
Who needs all those escape slashes? This is just style, of course, but that's what this entire post is about - good perl style (and a smattering of security thrown in for good measure ;-)

The specific answer to your question: param('topic') is "assigned" in the link text printed in print_topic_index. print "<A HREF=\"$topic\">"; When the script runs, if it sees param('topic') (because the user clicked on a link with that parameter in it), the script shows that file. If not, it prints a clickable list of files with topic=filename as a GET parameter.

Does all this help? No offense intended (please don't take any of this as hurtful criticism; but as meaningful, helpful mentoring). Good perl style comes with experience, and PerlMonks is a great place to learn.

Brainbench 'Most Valuable Professional' for Perl

Log In?

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: note [id://182916]
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others taking refuge in the Monastery: (4)
As of 2024-04-22 22:49 GMT
Find Nodes?
    Voting Booth?

    No recent polls found