Hello and welcome to our community! Is this your first visit?
Register
Enjoy an ad free experience by logging in. Not a member yet? Register.
Results 1 to 4 of 4
  1. #1
    New Coder
    Join Date
    Aug 2002
    Posts
    50
    Thanks
    0
    Thanked 0 Times in 0 Posts

    Will A PHP Guru Please Critique My Code?

    I wrote a PHP template script and was wondering if a PHP guru would look over my code and see if there's any security holes. If you see a better way to do something instead of how I coded it please point it out.

    Also, how can I make a printer friendly page so a visitor can click on a link like: http://www.domain.com/index.php?section=about&page=resume&action=print and it will come up with a page with nothing but the content from the PHP file containing the main content and not the header and footer?

    Here are the main files that contain the code.

    index.php [template]

    PHP Code:
    <?php
    // format directory/page names for page title
    $sectiontitle str_replace("_"" "$section);
    $pagetitle str_replace("_"" "$page);
    $st ucwords($sectiontitle);
    $pt ucwords($pagetitle);
    $title "My Site | " $st " | " $pt ."";
    ?>

    <?php echo "<?xml version=\"1.0\" encoding=\"iso-8859-1\"?".">"?>
    <!DOCTYPE html PUBLIC "-//w3c//dtd xhtml 1.0 transitional//en"
        "http://www.w3.org/tr/xhtml1/DTD/xhtml1-transitional.dtd">
    <html xmlns="http://www.w3.org/1999/xhtml">
    <title><?php print $title ?></title>
    <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
    </head>

    <body bgcolor="#FFFFFF" text="#000000">
    <table width="700" cellpadding="0" cellspacing="0" align="center" border="1">
      <tr>
        <td width="700" colspan="2" align="center"><?php include "includes/header.php" ?></td>
      </tr>
      <tr>
        <td width="700" colspan="2" align="center"><?php include "includes/navigation.php" ?></td>
      </tr>
      <tr>
        <td width="150" valign="top">
          <?php
          
          
    /* This code is for the sub navigation links that change
             based on what section of the site the visitor is viewing */
             
          
    $subnav = Array('home','about','links','contact');
             
          
    $sn = (!isset($_GET['section']) ) ? 'no section' : (!in_array($_GET['section'], $subnav)) ? 'home' $_GET['section'] ;
             
          
    $subnav "includes/subnavigation/" $sn ".php";
          include(
    $subnav);
          
          
    ?>
        </td>
        <td width="550" valign="top">
          <?php
          
          
    /* This code pulls the main content for the page. */
          
          
    $content = Array('overview','resume','references');
          
          
    $c = (!isset($_GET['page']) ) ? 'no page' : (!in_array($_GET['page'], $content)) ? 'overview' $_GET['page'] ;
          
          
    $content "includes/" $sn "/" $c ".php";
          include(
    $content);
          
          
    ?>
        </td>
      </tr>
      <tr>
        <td width="700" colspan="2" align="center"><?php include "includes/footer.php" ?></td>
      </tr>
    </table>
    </body>
    </html>
    includes/navigation.php [main navigation links]

    PHP Code:
    <?php
    // home
    if ($section == "" || $section == home) {
        print 
    "Home&nbsp;&nbsp;&nbsp;";
        }
        else {
        print 
    "<a href=\"index.php?section=home\">Home</a>&nbsp;&nbsp;&nbsp;";
        }

    // about
    if ($section == about) {
        print 
    "About&nbsp;&nbsp;&nbsp;";
        }
        else {
        print 
    "<a href=\"index.php?section=about\">About</a>&nbsp;&nbsp;&nbsp;";
        }

    // links
    if ($section == links) {
        print 
    "Links&nbsp;&nbsp;&nbsp;";
        }
        else {
        print 
    "<a href=\"index.php?section=links\">Links</a>&nbsp;&nbsp;&nbsp;";
        }

    // contact
    if ($section == contact) {
        print 
    "Contact&nbsp;&nbsp;&nbsp;";
        }
        else {
        print 
    "<a href=\"index.php?section=contact\">Contact</a>&nbsp;&nbsp;&nbsp;";
        }
    ?>
    includes/subnavigation/about.php

    PHP Code:
    <?php
    // overview
    if ($page == overview) {
        print 
    "Overview<br>";
        }
        else {
        print 
    "<a href=\"index.php?section=about&page=overview\">Overview</a><br>";
        }

    // resume
    if ($page == resume) {
        print 
    "Resume<br>";
        }
        else {
        print 
    "<a href=\"index.php?section=about&page=resume\">Resume</a><br>";
        }

    // references
    if ($page == references) {
        print 
    "References<br>";
        }
        else {
        print 
    "<a href=\"index.php?section=about&page=references\">References</a><br>";
        }
    ?>

    Any advice would be greatly appreciated.


    Regards,

    John

  • #2
    Senior Coder
    Join Date
    Jun 2002
    Location
    frankfurt, german banana republic
    Posts
    1,848
    Thanks
    0
    Thanked 0 Times in 0 Posts
    The code looks quite secure, you did a IMO a good job to protect against an inclusion attack by looking up the GET parameter of "section" very thoroughly and comparing it against the values in those arrays.

    You have however some slight errors in your if-statements that follow in the included files

    PHP Code:
    if ($section == about) { 
    You are not comparing against a string, but against a constant. It may only work because you set your error level low enough, and then it doesn't report if a constant value is treated like a string. Include

    PHP Code:
    error_reporting(E_ALL); 
    at the top of your script and see for yourself....

  • #3
    New Coder
    Join Date
    Aug 2002
    Posts
    50
    Thanks
    0
    Thanked 0 Times in 0 Posts
    Hello mordred,

    Thanks for looking over my script. I added the error reporting script to the top of my page and it came up with the error below for each one of the links it printed on both the include pages.

    Notice: Use of undefined constant home - assumed 'home' in /home/user22/htdocs/includes/navigation.php on line 3

    Can you tell me how I can fix this and what exactly I'm doing wrong?

    How about the print function? Is there a way I can easily add a section that will send just the main content include page to a printer friendly format?


    Regards,

    John

  • #4
    Regular Coder
    Join Date
    Jun 2002
    Location
    UK
    Posts
    577
    Thanks
    0
    Thanked 0 Times in 0 Posts
    if($section == "about")

    I posted a more in depth analysis over at sitepoint including a few hints about using $_GET and something on using arrays to simplify your code and make it more easily extendable.

    On another subnote: using an xhtml doctype while keeping the align, width and height parameters in table and td tags defeats the purpose and will do you more harm than good.

    print:

    echo '<a href="print.php'.$_SERVER['QUERY_STRING'].'">printable version</a>';
    Ökii - formerly pootergeist
    teckis - take your time and it'll save you time.


  •  

    Posting Permissions

    • You may not post new threads
    • You may not post replies
    • You may not post attachments
    • You may not edit your posts
    •