Yet another ugly web app



  • Posting abominations in PHP here is like shooting fish in a barrel, unfortunately, but every once in a while I come across something that is worse than the average clueless script kiddie product.

    High points of this one (a form I had to add a new field to):

     - GET is used indiscriminately for all forms in the page, including those that result in database entries.
     - The same script acts as the form page, the preview page and also does the database insertion.
     - Which of the above actions is being performed is determined by the url argument "action" as in script.php?action=preview, script.php?action=send, etc.
     - Input validation (required fields, etc.) is done on the "preview" action, but not the "send" action, so it's possible to send unvalidated data by simple modification of the URL.
     - Absolutely no protection against SQL injection.

     

    Oh, and this one just does it: After the form is sent and saved in the database, it may be viewed in printer-friendly format.

    Now, professionals implement printer-friendly formats by using the "media" attribute in CSS stylesheet links. Less experienced programmers might use an extra URL parameter as in script.php?id=800&print=on.

    What does our script do?

     
    There is a separate file, scriptprint.php. This file does not take the ID of the request that was just saved. Instead, it takes the ENTIRE data entered in the form, also of course inside URL parameters. The page renders a link to scriptprint.php with all this data.

     Oh, and the data is of course not passed in separate arguments - far too easy. Instead, the data is imploded with a ";" delimiter and then split back up in printscript.php. This has the added advantage of removing the names from these values and making them accessible only by their numerical position in this long array.

     
    Excuse me while I wash my brain with acid. 😞



  • @Arancaytar said:


     - The same script acts as the form page, the preview page and also does the database insertion.
     - Which of the above actions is being performed is determined by the url argument "action" as in script.php?action=preview, script.php?action=send, etc.

    Not necessarily bad, could be an implementation of the "front controller" design pattern. Which, of course, would at least mean that the main script delegates the real work to some other scripts...



  • Doesn't, unfortunately. It's a one-script setup (aside from the print view I mentioned); it works a bit like this.

     if($action=='send') send($HTTP_GET_VARS);
     else if ($action=='preview') {
       validate($HTTP_GET_VARS);
       preview($HTTP_GET_VARS);
    } else form();

     Bonus points for at least using functions, I guess; it could have lumped all the code into one huge if-else block.



  • With proper validation of input theres absolutley nothing wrong with the first 3 points you make. The last 2 are inexcusable however.

    The rest is just bad..... 



  • Technically, the first point is at least a major WTF, though maybe not inexcusable. GET is meant to be idempotent and cacheable. If I reload the page ten times, it's not supposed to cause any problems on the database end, and if Google decides to crawl it too, that shouldn't cause trouble either.



  • Just reread the first point, your right.



  • In some situations, yes.  Depending on how your page is structured, you might want to have links rather than forms for every action.  I have a script like that, but it defaults to only being visible to registered users, it has rel="nofollow" just in case they decide to make it viewable by guests, and even if it gets visited twice by the same person, they'll just see a page telling them they don't have permission to do that.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.