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.
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
agoldenberg
Member, Host Rep
in Help
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
Andrew
Comments
Thanks @adduc for your input and help
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.
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.
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.
@FiberVM I would tend to agree, it just a question of where an intro tutorial should place it's focus.
If you intend to teach people, it's even more important to use best practices. Separate the MVC components and use PDO.
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.
@adamMc I understand. Working on one thing at a time.
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
How do I salt it?
Hash it with a string (application level) appended.
Pseudocode:
This basically deters rainbow tables. Preferably, you should store a per-user salt.
He he he I was going to answer "it is the one next to the pepper". @agoldenberg keep up the good work
Thanks guys! Really appreciate the help! I assume checking it in SQL should be pretty much the same right?
@vemacs mind looking at the push I just did, and tell me if that's the correct way of doing what you suggested?
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
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
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?
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)
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
which is the bcrypt salt and hash. Verification pulls the substr for the salt.