Jacques1

The if ($result) check is indeed unnecessary (altough this is somewhat nitpicky). It seems you're confusing the classical return-value-based error handling with the new exception-based error handling. In the classical system, you'd indeed check the return value of the execute() method to see if the query succeeded or failed. However, you're using the exception-based system in your code, so PHP will throw an exception in case of an error rather than make execute() return false. In other words, the return value is irrelevant, because the success or failure is determined by the control flow. Since we're already nitpicking, you should replace the human-readable feedback with machine-readable feedback. As you've already said, the entire purpose of the script is to give feedback to JavaScript, and a message like “Data Successfully Inserted!” is not very suitable for that. You should use the HTTP response code to indicate the status: 2xx means success, 4xx means a client error, 5xx means a server error. You can give the exact cause as a message, but you should not use some hard-coded English text. Use error identifiers like “duplicate_entry”. Then you can map them to an actual message later (and use different languages, for example). If you have a complex response, use JSON. This allows you to have an actual data structure which is easy to parse in JavaScript.

To match the regex, a string must have the following properties: a sequence of 8 characters which are not newlines (the dot exclused newlines unless the s modifier is set) a digit anyhwere in the string a lowercase latin letter anwhere in the string an uppercase latin letter anywhere in the string However, the regex is extremely inefficient and leads to excessive backtracking: It first consumes everything up to the first newline due to the .* pattern. From that position, it tries to find a sequence of 8 characters which are not newlines. If the .* pattern has consumed too much (like the entire string), then the parser has to go back character by character until it finds the sequence. From that position, it again reads all characters up to the next newline. Now it tries to find it a digit. It likely has to go back until it finds one. It may even have to go back in the very first .* pattern. Then the same procedure happens for the lowercase and the uppercase latin letter. So the poor regex parser has to go back and forth dozens of times only to check a few trivial properties. Is there any reason why you cannot use standard string functions like strlen()? It's also much simply to just apply three separate regexes for the digit, the lowercase and the uppercase letter. If you stuff it all into one big regex, you have to be very careful with how it is parsed and how the parts interact with each other. Besides that, there's a conceptual issue: Your regex is based on your personal ideas of how a password looks like, which means you'll reject many strong passwords just because they use a different scheme. For example, I usually generate 16 random bytes and then encode them as 32 hexadecimal characters. This is extremely strong, yet you reject it based on the fact that I don't have uppercase letters. This is obviously silly. And what's wrong with using only symbols or non-latin letters? There are many different languages with many different alphabets, and people should actually be encouraged to use a large space of letters and not just A-Z. You generally need to be very careful with password policies. Let me put it this way: Your check is good enough to make the average boss happy. If that's your goal, you can call it a day and go home. But if you're seriously interested in improving password security, you need to realize that there's a large variety of password schemes. Forcing everybody to comply to a particular set of rules can be very annoying and often borders cultural ignorance. It's a bit like trying to validate human names: You really can't. It does make sense to enforce a minimum length, because this is a fairly strong indicator of security. But besides that, you probably shouldn't do anything. Do you really think that forcing people to follow a bunch of stupid rules will result in better password? I think it's much more effective to support and encourage them. Tell them about password managers, give practical tips for coming up with good passwords (like the famous “Correct Horse Battery Staple”). Work with your users, not against them.

You should generally avoid messing with home-made query generators unless you're very experienced and fully understand the implications. Otherwise, you'll quickly end up with tons of SQL injection vulnerabilities without even realizing it. For example, in your code above, both $table and $field are entirely unprotected. You drop them right into the query string. So if you ever let the user provide the table or column name (like in a dynamic search), you immediately have a security vulnerability. And you cannot even see it from the outside. That is obviously a problem. In general, this problem has been solved many times by very smart people, so you're better off using their results rather than reinventing the wheel. For example, all big frameworks like Laravel or Symfony have a query builder, and some of those are available as a standalone package: The Doctrine Project from Symfony The Illuminate Database component from Laravel Of course you're free to build your own classes on top of those. The point is that they provide a solid foundation and help you avoid major mistakes (like using unescaped identifiers).

I wonder where on earth you got the text/plain from in the first place. Let me guess: w3schools? This is an incredibly exotic encoding which is explicitly marked as “for humans only”, so you're unlikely to find any language which parses this. In fact, I've never seen it being used in real life (outside of w3schools). In any case: You need a better learning resource.

The code is fine. I was mainly concerned about PDO::ATTR_EMULATE_PREPARES being turned on (which is the default), but you did deactivate it. You might want to use utfmb4 instead of utf8 as your character set. What MySQL calls “utf8” is only the Base Multilingual Plane (~65,000 characters) rather than the complete Unicode space (currently ~110,000 characters). While the BMP is usually sufficient, most people aren't even aware that they only get a subset of Unicode. Well, of course all checks need to be done server-side, but this has nothing to do with Ajax or JavaScript in particular. You'd have the exact same problem with a classical HTML form, so the comment seemed to be somewhat out of place. It sounded like if there was something inherently insecure about Ajax-base registration (which of course isn't the case).

I'm impressed. It's very. very rare to see such excellent code outside of big projects: bcrypt, prepared statements, even a proper uniqueness check. Wow. Only one thing: bcrypt has an input limit of 56 bytes, so you should validate that. Longer strings are outside of the specificiation, and everything after 72 bytes is truncated. Besides that, I don't see any security issues in the code. What does your code for establishing a PDO connection look like? This is a common source for errors. @ gristoi: What does JavaScript have to do with anything? If he sent the data with a classical form, it would be client-side as well, so there's nothing special about Ajax requests.

You do not call htmlentities() at all. What this function does is convert all characters for which there's a named HTML entity. This is absolutely useless. It is particularly useless for HTML-escaping, because only the five characters <, >, ", ' and & have a special meaning in HTML. Converting harmless characters like umlauts is entirely unnecesary and only wastes energy. What you want is htmlspecialchars(). However, you still can't call this function like you did above where you only specified the input string. How is PHP supposed to know the encoding of the string? In other words, how is it supposed to recognize the characters from the raw bytes? If you don't tell it, then it will use a default encoding which differs accross PHP versions may or may not be correct. You always have to specify the character encoding. There's also a pitfall: By default, htmlspecialchars() does not convert single quotes, so you're likely to run into problems of even security vulnerabilities. Always specify the ENT_COMPAT flag to make sure both single and double quotes are converted. As an example: <?php // the character encoding of the document is UTF-8 header('Content-Type: text/html;charset=utf-8'); $input = 'Those should all be converted: <>"\'&'; echo htmlspecialchars($input, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); Note the ENT_QUOTES flag and the explicit declaration of the encoding. The ENT_SUBSTITUTE flag can only be used in conjunction with Unicode strings and replaces invalid characters with an error symbol. Without this, any invalid character will make the entire return value empty, which is usually not what you want. Since it's very cumbersome to repeat this piece of code all the time, it's a good idea to make a custom html_escape() function: function html_escape($input, $encoding) { return htmlspecialchars($input, ENT_QUOTES | ENT_SUBSTITUTE, $encoding); } Now you simply call this function whenever you need to HTML-escape a string.

This is somewhat oversimplified. Yes, in a classical Apache-CGI setup, the PHP scripts typically run under the same user as the webserver. But that is not a necessity, and more modern systems like PHP-FPM can run the scripts under an arbitrary UID and GID. So it's best to actually check this: posix_getuid(), The blank screen is simply because PHP doesn't display the error message on the screen, which is a wise choice for an online webserver. You need to look it up in the error log.

Besides that, the implementation is horrible. Sorry. Regexes are for patterns. They're not suitable for range checks. That's what we have the mathematical operators “<”, “<=”, “>” and “>=” for. It's also insane to do date calculations by hand. Even if you actually manage to find a PHP version which doesn't have checkdate(), the basic date functions are always available. I mean, handling dates is one of the core features of every language. And, yes, PHP can do it as well.

You have more than enough time to update your code. The PHP core developers are well aware that there are tons of legacy applications and many people who haven't even heard the “news”. However, you should not switch to MySQLi. It's very inconvenient and, of course, again limits you to MySQL databases. Use PDO instead. It's a well-designed universal database interface which can be used for all mainstream database systems, not just MySQL.

<?php require_once __DIR__ . '/lib/password.php'; $cost = 14; $start = microtime(true); $hash = password_hash('foobar', PASSWORD_BCRYPT, array('cost' => $cost)); echo microtime(true) - $start;

You generally don't catch exceptions at all, not on the top level and especially not at random locations in the code. I wonder why so many people believe that they should clutter their scripts with try-catch blocks. The purpose of this statement is to handle errors. In some rare cases, you may indeed be able to recover from an error or at least do something useful. But most of the time, you really cannot do anything about the problem. For example, if a query fails, there's either a bug in your code or an issue with the database server. Neither can be fixed from within PHP, so the correct reaction is to leave the exception alone and let PHP do its standard error procedure. And, no, you do not tell your users about a failed database connection or a crashed query. What are they supposed to do with this information? It's none of their business. A big try-catch block on the top level doesn't make sense either, because most PHP errors are not exceptions, which means they aren't affected by this at all. And again: Why not just use the standard error procedure? Yes, PHP itself is perfectly capable of dealing with errors. It generates detailed messages with all relevant information, it can display those messages to ease development, it can log them in production, and it can emit the right HTTP error code so that the webserver may display a user-friendly error page. The only reason why you'd write your own error handling procedure is if you want to do something special. Is that the case? What do you want to do apart from logging the message and displaying a friendly error page?

It makes no sense to literally delete the sessions after a certain time. Just store the timestamp when the session is started and delete expired sessions when you encounter them. This has the exact same effect. And unused sessions are automatically removed by the garbage collector. Store this information in the users table. It makes no sense to put so much effort into maintaining the one-to-one relationship between the two tables when you might as well store the data directly in the user records. The session ID already is a secret unique identifier, so it's not very useful to store another identifier next to it. To protect the session against hijacking, use HTTPS and set the Secure and HTTPOnly flag for the session cookie. SHA-512 is absolutely unsuitable for password hashing. A stock PC can calculate hundreds of millions of SHA-512 hashes per second, so an attacker is able to find passwords simply by trying out many different combinations. Don't let the fancy name fool you: With regard to password hashing, there's no fundamental difference between SHA-512 and, say, MD5. They're just as bad. Sure, SHA-512 is a bit more expensive, but an attacker can easily make up for it by adding some more computing power or waiting a bit longer. The HMAC also makes no sense. Now your entire password security depends on the secret key, which is exactly what we do not want. The reason why we hash passwords instead of encrypting them is because we want them to be secure even if the attacker has compromised the entire system. There shouldn't be any shortcuts to finding out the passwords. Last but not least, 50 characters are excessive. Random strings in a security context are typically 16 bytes long. If you generate them properly using a device like /dev/urandom, there's absolutely no risk of anybody guessing them. It's physically infeasible. The cost argument specifies how much computing power it takes to calculate a hash. Of course you want this to be as much as possible to slow down attackers. However, if the value is too high, you slow down your own site as well and make it susceptible to denial-of-service attacks. A common recommendation is to decide how much time you're willing to spend on the hashing on your current hardware. Let's say one second. Then you try different values until the hash calculation indeed takes one second. You'll probably end up with something around 15. It's also important to increase the value over time as hardware becomes better. That's actually the whole point of the parameter and a very important property of modern password hash algorithms. Simple algorithms like SHA-512 require only a fixed amount of computing power, which means the situation gets worse for us with each new generation of hardware. In the 90s, MD5 or SHA may have been acceptable for hashing passwords, because people only had slow CPUs. Nowadays, we're dealing with extremely powerful GPUs or even specialized hardware like ASICs and FPGAs. The only way to deal with that is to use an adaptable hash algorithm with a variable cost factor. bcrypt is actually rather primitive in that regard. There are much more sophisticated schemes like scrypt which let you fine-tune all kinds of different parameters. However, scrypt is too young and not well supported, so it's best to stick with bcrypt for now.

What is the unique key supposed to do? What's the point of the logged_in_users table? The standard way of authenticating users is very simple: The user submits their credentials, you check them, and if they're correct, you create a fresh session and store the user ID. That's it. On each protected page, you simply check the presence of a user ID in the session and possibly look up the user for authorization checks. As long as the session ID is sufficiently random (which is determined by session.entropy_file and session.entropy_length), this scheme is fairly secure and sufficient for average websites. If you want to limit the number of sessions to only one, you simply store the active session IDs in the users table and add a check for this. And of course you can have additional security features like a time limit. Besides that, there are several issues in your code: Don't establish a new database connection for every single function call. This is extremely inefficient and bloats the code. Use a single shared database connection for the entire application. For example, make a script which creates a global $db variable holding the database connection, include this script when needed and access the connection through $db. Your error handling makes no sense. What is the point of telling the user that the database connection or a query has failed? What are they supposed to do with this information? Just let the exception terminate the script and create a nice generic error page for your users. You're using some home-made password hash algorithm where you prepend a salt and hash the result probably with something like MD5 or SHA. This is a very bad idea. Do not invent your own algorithms, because you never know if they're sound. If you indeed use MD5(salt + password) or SHA(salt + password), this is not secure at all. Both MD5 and SHA are extremely vulnerable to brute-force attacks. Salting doesn't help either, because if the attacker can search the entire space of possible passwords in a few seconds, they don't care about the salt. So always use an established algorithm like bcrypt. What's the point of making a second query to see if the password is correct? You already know the hash from the previous query. Just check it with PHP (using bcrypt).

printf() lets you pad strings to a fixed length. However, you'll need to loop through the array twice to first get the longest title.

A tree is fine, but that doesn't mean that we, as humans, should build that tree. That's a job for the computer. For example, we might write down the base SQL query, have it parsed and then manipulate it through method calls. Escaping is (mostly) dead. We have prepared statements now.

The idea of generating SQL queries with PHP is nothing new and has been many different times in many different ways. Most approaches are object-oriented. See the Doctrine Query Builder, for example. The problem is: This idea is incredibly stupid. It's good to have different languages for different purposes. SQL is excellent at querying relational databases, because that's exactly what it's designed for. Why on earth would I want some half-assed emulation written in PHP which makes the queries five times as long and absolutely unreadable? What's the benefit of that? Sure, it's all PHP then. But who says that's a good idea? By all means, I do not want to fumble with dozens of nested PHP arrays instead of writing down one simple SQL query. Even your trivial example is difficult to read. And you've actually cheated. You can't just use strings, because then the SQL identifier foo is indistinguishable from the SQL string 'foo'. So you need even more arrays to hold the type information. Now imagine the equivalent of a more complex query with joins, subqueries, grouping, aggregate functions etc. It will simply be unusable. I mean, this is certainly an interesting project if you're into compiler building. But in practice: No, this is not a good idea.

Caching the permissions in the session is a very bad idea, because changes only take effect after the session has ended. If you take away a permission from a user, they still have it for as long as they can keep the session alive, and this may be forever. I think the whole caching idea doesn't make a lot of sense and is downright harmful. The point of caching is to improve performance, but you haven't said anything about performance issues. So you're effectively replacing a virtual issue with a real issue: Now you have to worry about cache invalidation, and if you screw this up, the whole permission system falls apart. So throw away the caching stuff and just use a plain function. Should you indeed experience performance issues, approach this in a smart way: Is the permissions function actually the cause? If that's the case, is the performance advantage worth the security disadvantage? If both is the case, then it's time to think about the best approach. And that's certainly not a session-based cache, because you need to be able to invalidate the cache at any time. Besides that, you've developed some strange coding habits: Do you really think it's a good idea to dump all MySQL error messages on the screen for the whole world to see? Why even mess with manual error handling? Turn on error reporting in the mysqli_driver class, and MySQLi will either trigger an error or throw an exception whenever something goes wrong. Why do you fetch the entire permissions rows to count them in PHP? Are you aware that MySQL can do the counting itself? You can also use an EXISTS query, because you only care if there's any row. You need to stop polluting your code with exit statements. This makes the control flow extremely hard to understand and is also very error-prone, because who knows what your code does if you forget one of those statements? Create a proper structure which runs from the top to the bottom without any tricks. What exactly is the point of mapping the return value of in_array() to a boolean with an if statement? The return value already is a boolean, and it doesn't get “truer” or “falser”. So why not just return it? Please start formatting your queries. Those endless one-liners are really a pain to read. Doing a left join is nonsensical, because all additional NULL rows immediately get rejected by the WHERE clause. You need an inner join. The staff_roles table is useless in your query. Don't mix different naming schemes in your table. Drop the camelCase notation and always use underscores. <?php function checkUserPermission($permission) { global $db; $hasPermission = false; if (isset($_SESSION['userid'])) { $permissionCheckStmt = $db->prepare(' SELECT EXISTS ( SELECT 1 FROM staff JOIN staff_roles_permissions ON staff.staff_roles_id = staff_roles_permissions.staff_roles_id JOIN staff_permissions ON staff_roles_permissions.staff_permissions_id = staff_permissions.id WHERE staff_id = ? AND staff_permissions.permissions_name = ? ) '); $permissionCheckStmt->bind_param('is', $permission, $_SESSION['userid']); $permissionCheckStmt->execute(); $permissionCheckStmt->bind_result($permissionAssignmentExists); $permissionCheckStmt->fetch(); $hasPermission = (boolean) $permissionAssignmentExists; } return $hasPermission; } (completely untested)

You've already asked this question, and I've already answered it.

Besides that: This is complete bollocks. The execute privilege has abolutely nothing to do with script execution as it's done by a webserver. The webserver only reads the file and passes the content to the PHP interpreter. You also need to worry about client-side scripts. If people use your upload feature to attack your users, that's just as bad as an attack against the server. But if you don't give a shit, then I don't give a shit explaining this.

Keep away from MySQLi. It's cumbersome as hell and of course limited to the MySQL database system. PDO, on the other hand, is a universal database interface with a very nice API. Don't let the seeming similarity of the mysqli_* functions and the old mysql_* functions fool you: Switching to MySQLi is not as easy as adding an “i”. It requires you to rewrite the entire database code and get rid of several bad habits like stuffing dynamic values into query strings. So you might as well do it right and jump straight to PDO. Check out this PDO tutorial.

The whole code is badly broken and really needs a major rewrite. A lot of this doesn't even make sense. The mysql_* functions are obsolete since more than 10(!) years and will be removed in one of the next PHP releases. Haven't you seen the big red warning signs in the manual? Nowadays, we use PDO. You need to start thinking about security. You can't just drop raw user input into SQL queries or your HTML document, because this allows anybody to inject malicious code and attack your server or your users. Read up on security basics like escaping and prepared statements. Don't use PHPHTML spaghetti code. Keep all your PHP application logic on top of the script and all HTML markup at the very bottom. This will also fix this backslash jungle. You set status to htmlspecialchars( @$_GET ['p'] ) and then check for $status == $_GET["p"]. Um, what? When exactly do you expect this condition to not be true? ... Whatever book or tutorial or person you've learned PHP from: Keep away from them in the future. This is (bad) 90s code. Check out the links above to learn proper and modern PHP. It also helps to read the manual to keep up-to-date (careful with examples and the comment section, though).

You two definitely need to get rid of this strange habit of enclosing all identifiers in backticks. It's just ugly, unnecessary and can lead to hard-to-find bugs. For example, if you accidentally put the dot of a qualified name between the backticks (e. g., `foo.bar`), that's still a formally valid identifier but of course not what you meant. Even worse, the error message will look like the column foo.bar is missing, so you'll scratch your head until you visually inspect the query and spot the messed-up identifier. I've seen people spend hours on this. The backticks only exist for one reason: To allow the use of non-standard identifiers with strange symbols or reserved words. Since you should never use non-standard identifiers, there's never a need for backticks. So just get rid of them.

Sorry, but I'm not a fan. The fancy JavaScript desktop stuff is interesting for the first few seconds, but it quickly becomes annoying due to the ugly graphics and the complete lack of basic usability features. For example, it's impossible to navigate on the page with standard browser functionalities. I cannot reload the current page, I cannot go back, I cannot create bookmarks. In the end, it looks rather unprofessional. Besides that, there are several technical issues: You've declared the document as XHTML, but it's neither valid XHTML nor served as XHTML. In fact, you don't even seem to be aware of XHTML basics like marking inline scripts as CDATA sections. That's a problem. You generally shouldn't use XHTML unless you have a specific reason and the knowledge and discipline to get it right. Otherwise, just use plain HTML. Inline scripts and event attributes are evil. While you declare the character encoding in the document, you've failed to declare it in the HTTP headers. Always do both. Stretching images doesn't look good.

Not the mixture of PHP and HTML itself is bad but the mixture of application logic and graphical presentation. If you put HTML snippets into your code, then you make the code unreadable and the HTML inflexible. Even the tiniest design update requires you to wade through the entire spaghetti code. In the worst case, you even have to make major changes to the application logic just because you want the baked-in HTML to look differently. So, yes, do separate the two aspects. The minimum is to put all application logic on top on all HTML to the bottom. What on earth does an HTTP redirect have to do with some admin bar in the GUI? Why do you check the log-in status in a function which is supposed to do nothing but render an HTML snippet? This makes no sense. Do all your authentication, redirects and whatnot on top of the script and then render the HTML based on the results.