Howdy, Stranger!

It looks like you're new here. If you want to get involved, click one of these buttons!


Crowd Sourcing Code Review
New on LowEndTalk? Please Register and read our Community Rules.

All new Registrations are manually reviewed and approved, so a short delay after registration may occur before your account becomes active.

Crowd Sourcing Code Review

agoldenbergagoldenberg Member, Host Rep

Hi Everyone.

I know there are some much more experienced PHP developers here than I and I wanted to have some simple code reviewed by anyone who cares to just to figure out if everything is being done properly.

https://github.com/agoldenberg/LearnPHP

I'm working on small tutorials to teach a friend basic php, mysql, html and css.

Thanks for your advice in advance :D

Andrew

Comments

  • agoldenbergagoldenberg Member, Host Rep
    edited November 2013

    Thanks @adduc for your input and help :)

  • FiberVMFiberVM Member
    edited November 2013

    It is a very good idea to put all processing code in the top of your page, and only echo variables inside the page. Best is to use a templating system (like Smarty) or a full MVC (like Laravel).

    The following snippet I took from index.php won't work for example, because output has already been sent, making defining a header impossible at that stage.

    if(isset($_GET['logout']))
    {
      //If logout is set, destroy current session and redirect to index.php
      session_destroy();
      header("Location: index.php");
    }

    I do not have the time to review it in full, but I think the gesture you're trying to express is very good. Keep on rocking!

    Edit: oh, and take a look at PDO for MySQL ;-)

  • Looks like a nice start. I second the idea of putting all processing code at the top, and only do simple loops and echos at the bottom. Even if it creates slightly more code, it will be easier to maintain.

    This particularly true for things like 'header' functions (as pointed out). The tricky thing with 'header' is that sometimes it does work if you call it after send a small amount of text, but this is not guaranteed and will lead to problems.

    While using something like smarty and a better template engine, I think that might be overkill for an intro tutorial type thing. Essentially , you want to separate logic from display. There are many ways to accomplish this.

  • adamMc said: While using something like smarty and a better template engine, I think that might be overkill for an intro tutorial type thing. Essentially , you want to separate logic from display. There are many ways to accomplish this.

    It just depends what you want to learn people. If you just want to learn them some PHP snippets then no engine is fine. If you want to learn them building structured websites (which really isn't that hard with the right tools/libraries!) then use a templating engine or MVC that does a lot of work for you already and will save you trouble later when everything becomes spaghetti.

  • Use PDO.

    Thanked by 1tchen
  • @FiberVM I would tend to agree, it just a question of where an intro tutorial should place it's focus.

  • jhjh Member

    If you intend to teach people, it's even more important to use best practices. Separate the MVC components and use PDO.

    Thanked by 1FiberVM
  • agoldenbergagoldenberg Member, Host Rep

    Made a slight modification using what I think to be PDO.

    Would anyone mind taking a look again?

  • I am not primarily a php developer (I am java developer) so I will let someone else comment on the PDO.

    But you still have output before the header() function. there should not be any possible output before you decide on redirects, this include echos and "normal" html. That is to say, even the <!DOCTYPE html> should go after any possibility of a redirect with the header function.

  • agoldenbergagoldenberg Member, Host Rep

    @adamMc I understand. Working on one thing at a time.

  • agoldenbergagoldenberg Member, Host Rep

    Moved all processing code to top of file.

  • @agoldenberg - sorry did not intend to come off as nitpicky... looking good

  • I don't know how deep in best practices you want to go, but don't use sha1 straight up like that. At the very least, salt it at the application level or better yet, per row. I know it's getting nitpicky, but one less php-tutorial out there showing bad security practices would be nice :)

  • agoldenbergagoldenberg Member, Host Rep

    How do I salt it?

  • vemacsvemacs Member
    edited November 2013

    @agoldenberg said:
    How do I salt it?

    Hash it with a string (application level) appended.

    Pseudocode:

    sha1_hash(password + "MYAPPLICATIONSALT2048")
    

    This basically deters rainbow tables. Preferably, you should store a per-user salt.

  • @vemacs said:
    Hash it with a string (application level) appended.

    He he he I was going to answer "it is the one next to the pepper". @agoldenberg keep up the good work

  • agoldenbergagoldenberg Member, Host Rep

    Thanks guys! Really appreciate the help! I assume checking it in SQL should be pretty much the same right?

    Thanked by 1vRozenSch00n
  • agoldenbergagoldenberg Member, Host Rep

    @vemacs mind looking at the push I just did, and tell me if that's the correct way of doing what you suggested?

  • tchentchen Member
    edited November 2013

    @agoldenberg said:
    How do I salt it?

    See @vemacs 's response* for application level salts.

    *For application salts, don't embed it directly in the code. It needs to be install defined - hence different for everyone, otherwise you can still construct a custom rainbow table for all installations of your app.

    For user (row-level), typically create a random salt (something long) and store it with the user row. Alternatively, some people encode it with the password field as

    $salt.":". password_hash($salt.$password)

    and extract it using substr during the compare.

    Also, using a heavier hash function helps a lot in avoiding any offline attacks. password_hash since 5.5 will allow bcrypt. There's no reason for login authentication to be sha1 fast :)

  • agoldenbergagoldenberg Member, Host Rep

    yeah I'm using php 5.3 at the moment.

  • tbh, I feel that its a bit weird that the schema of the database is not given lol. How can one try when he/she doesn't have the database schema?

  • wcypierrewcypierre Member
    edited November 2013

    @tchen said:
    Also, using a heavier hash function helps a lot in avoiding any offline attacks. password_hash since 5.5 will allow bcrypt. There's no reason for login authentication to be sha1 fast :)

    however, it limits the user to be only to be able to use php 5.5 and above which somewhat hampers the experience as the newbies may not even know about which version of php they are using(assuming that they're using some free shared hosting). But well, for me, I'll indeed install php 5.5 though, gonna drop those bad practices in a way or two anyway.

    @agoldenberg, btw, try to use one salt per user, what you're doing right now is just slowing down the crackers a lil bit, while if you use one unique salt per user then it'll slow down the cracker a lot(he/she can only do a directed attack instead)

  • tchentchen Member
    edited November 2013

    @agoldenberg said:
    yeah I'm using php 5.3 at the moment.

    Then try

    //PHP5.3+
    $salt = '$2a$07$';
    $salt = $salt . md5(strtolower($username)); // or rand.. or whatever 22 chars min
    $hash = crypt($password, $salt);

    which will spew something like

    $2a$07$usesomesillystringfore2uDLvp1Ii2e./U9C8sBjqp8I90dH6hi

    which is the bcrypt salt and hash. Verification pulls the substr for the salt.

Sign In or Register to comment.