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

Let me give you a little overview of what the following snippet of code is supposed to do: The script used to display the topics has two modes. If it's passed a topic ID through the topic parameter, it attempts to display that specific topic. If no parameters are passed to it, it displays a list of the topics available to read. The topic list is generated by listing all the files in the topic directory and extracting the title and author from those files. The topic IDs correspond to the names of the files in the topic directory. If the topic parameter is supplied, the full text of the topic is displayed. Each of the responses is displayed on the same page as the original topic, and a form used to respond to the topic is also displayed. The full source code appears below: 

#!/usr/bin/perl

use CGI;
$query = new CGI;

$topic_directory = "topics";

if($query->param('topic')){
     $topic = $query->param('topic');
     &file_error unless (&open_topic);
     &format_error unless (&parse_topic);
     &print_page_start;
     &print_topic;
     &print_response_form;
     print "<P>Return to the <A HREF=\"display.pl\">Topic
list</A>.</P>\n";
     &print_page_end;
}
else{
     &page_title = "Topic Index";
     &print_page_start;
     &print_topic_index;
     &print_page_end;
}

sub print_page_start{
    print $query->header;

sub print_topic_index{
     eval{
     opendir(TOPICS, "$topic_directory")
          or die "Can't open $topic_directory";
     @topics = grep (/^.+\.txt$/, readdir (TOPICS));
     foreach $topic (@topics){
          $topic =~ s/(.+)\.txt/$1/;
          &open_topic;
          &parse_topic;
          print "<A HREF=\"display.pl?topic=$topic\">";
          print "$page_title</A>, $topic_author<BR>\n";
}
};
}
........
.......#<CODE HERE WAS OMITTED>
The following code contains the form that holds the variable contents 
+for
$topic   The problem that I am having is I do not understand how the t
+opic
parameter in the form is passed a topic ID when it is a hidden field? 
+ The
code for the form creation is below:


sub print_response_form{
     print "<FORM METHOD=\"post\" ACTION=\"post.pl\">\n";
     print "<INPUT TYPE=\"hidden\" NAME=\"action\" ";
     print "VALUE=\"response\">\n";
     print "<INPUT TYPE=\"hidden\" NAME=\"response_to\" ";
     print "VALUE=\"$topic\">\n";
     print "Author: ";
     print "<INPUT TYPE=\"text\" NAME=\"author\" ";
     print "SIZE=40 MAXLENGHT=72 VALUE=\"$author\"><BR>\n";
     print "Message body: <BR>\n";
     print "<TEXTAREA ROWS=10 COLS=60 NAME=\"post\";
     print "WRAP=\"virtual\">";
     print ".........
..................................
.

[download]
Again I just don't understand how the parameter is assinged its value? Can anyone help me with this?

Replies are listed 'Best First'.
Re: Help understanding this script (Russ: waving red flags)
by Russ (Deacon) on Jul 18, 2002 at 17:14 UTC
    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;

    [download]
    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";

    [download]
    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);

    [download]
    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=\"post.pl\">\n";
     print "<INPUT TYPE=\"hidden\" NAME=\"action\" ";

    [download]
    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="post.pl">\n};

# better than this?
print "<FORM METHOD=\"post\" ACTION=\"post.pl\">\n";

    [download]
    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=\"display.pl?topic=$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.

    Russ
    Brainbench 'Most Valuable Professional' for Perl

[reply]
[d/l]
[select]
Re: Help understanding this script
by ides (Deacon) on Jul 18, 2002 at 16:25 UTC
    Without all of the code and/or corresponding HTML pages, I'm going to assume that is filled in by another part of the script that we are not seeing. Basically it is used in this script to determine if you are wanting to view a specific topic or the whole topic list.

    Hope this helps.

    -----------------------------------
    Frank Wiles <frank@wiles.org>
    http://frank.wiles.org

[reply]
      ok so since it is $topic as the value... whatever topic is assigned later in the script that's what the value is?
[reply]
        Well not necessarily "later".it could be somewhere in the code you omitted.

        -----------------------------------
        Frank Wiles <frank@wiles.org>
        http://frank.wiles.org

[reply]
Re: Help understanding this script
by sedhed (Scribe) on Jul 18, 2002 at 16:31 UTC

    The only difference between a hidden field and a regular text box or other field is that it's hidden from the user.(*) It can still hold a value, assigned at runtime like above or even assigned via Javascript (like document.myform.somefield.value = "hello"). The value is passed in the POST or GET to the form receiver as any other field would be. It's just not directly visible to the user.

    An aside: I invite you to read up on the usage of CGI.pm. It provides much easier ways if doing what you're doing, via methods like textfield(), hidden(), etc. (perldoc CGI will get you the CGI docs) There are also excellent tutorials here.

    * And it's not truly hidden, anyone can view source, copy it insert their own values, post the form, so don't think of it as secret, just hidden. cheers!

[reply]

Back to Seekers of Perl Wisdom