Jacques1

Your function parameter is called “user_id”, but inside the function, you try to access the variable “user” (without the “_id”). You need to decide which name to use.

The first question is why you want to stop supporting HTTPS. It will be a lot of trouble for you, it will be a lot of trouble for your users, and it's less security for everybody. Is money the problem? You can get a free certificate from StartCom. If you need more features (like multiple domains), there are still a lot of CAs with reasonable prices. No, you can't just go back to HTTP once the client makes an HTTPS request. That's the whole point of HTTPS. If there's anything wrong with the connection, your users will get an error. You can only choose between different errors: You may close port 443 entirely, in which case all HTTPS requests will fail. Smart users will try plain HTTP on port 80, inexperienced users will think the entire site is dead. You may keep using the expired certificate, which will lead to big red warnings. Some users will simply ignore the warnings, others will leave the site. You may issue your own certificate and use it instead of your current one. This also leads to warnings, but it's more reasonable than using an invalid certificate. Experienced users will understand the difference. Either way, there will be errors and confusion, and you may lose a lot of users.

It might make sense to discuss the password hashing topic in a separate thread ... Either way, using crypt() directly is not recommended and rarely works out. Unfortunately, your code is no exception: You have no error checks in place, which means you keep going even if bcrypt has failed. In the worst case, you'll end up with no password check at all. Generating and encoding the salt is also a common source for errors. Are you sure you have 16 strong random bytes encoded with that special Base64 variant used by crypt()? It makes no sense to store the salt in a separate column, because it's already included in the bcrypt “hash”. The output of bcrypt contains all hash parameters (algorithm identifier, cost, salt) together with the actual hash: A cost factor of 7 is on the very low end. You'll want something around 14 on current hardware. A common recommendation is to start with 10 and then increase the cost until the hash calculation takes around 1 second. Your code doesn't support increasing the cost during operation – but that's actually the whole point of bcrypt. The cost should be in a configuration file, and whenever it's changed, the current hashes should be updated as the users log in. The “2a” prefix is obsolete. There was a critical bug in the low-level bcrypt implementation, so a new prefix was introduced for the updated version: “2y”. Long story short, don't use crypt() directly. Since PHP 5.5, there's a new Password Hashing API built on top of crypt(). If you have an older version, you can use the compatibility library from the same author.

A join makes no sense in this context, because there's no relation between the items. The OP just wants one item from each table, and that's what a union does. To give concrete advice, however, we need to see the actual tables and the desired result set.

It's necessary. A single remember-me session which is valid for 30 days is bad enough. Starting two, three, four, ... of them at the same time is suicidal, especially when you consider the naivity of many users. For example, people happily use HTTP over a public Wi-Fi without realizing that anybody with access to the same network can read their entire traffic (which includes the remember-me cookies). The whole remember-me approach is actually rather poor. This should be done client-side. For example, KeePass has an auto-type feature which copies the password into the right field and submits the log-in form when you press a certain key sequence. Some plugins are even more comfortable and automatically fill out the log-in form as soon as you visit the page. Why not focus on those secure alternatives instead of promoting insecure behaviour?

PHPass is long obsolete and seems to have been abandoned by its author. The last version is from 2006. While the script is certainly mature, it's also full of unnecessary and potentially dangerous legacy stuff like a fallback to MD5. In addition to that, the code is very cryptic (no pun intended) due to lots of low-level byte fiddling. So, no, I wouldn't use PHPass in new applications. The author did a great job at introducing bcrypt to the PHP world (the underlying C implementation is also from him), but now we simply have better libraries. I'm not sure what that has to do with PHPass. Theoretically, it can make sense to mix a secret key into the hashes if the key and the hashes are stored separately (ideally on two different machines). This forces an attacker to compromise both the database and the key storage. However, the concrete implementation is tricky. Do you have an extra server at hand? If not, then the whole approach is rather questionable, because the key will be sitting right next to the hashes. How do you get around the input limit of bcrypt? As you probably know, it can only process up to 56 bytes. If you reserve, say, 16 bytes for the key, then the actual password is limited to merely 40 bytes, which is an actual problem for passphrases. This can be fixed by reducing the input with an HMAC before it's hashed, but this would be highly experimental and risky. So I'd stick to plain bcrypt.

They don't need to fill out anything, a simple submit button is enough. You can leave the activation token in the URL or copy it into a hidden field. A button is also very important for usability, because it allows the client to explicitly decide whether or not they want to activate the account. If you automatically do the activation just because the user has visited a certain page, that's very confusing and potentially against their will. It's simply not how the www works. The user ID can and should stay in the URL. The point is that you use an appropriate request method (not GET) and include an anti-CSRF token. For example, I'd use the following URL to reference a particular user in the admin area: https://admin.yoursite.com/users/123 To delete a user, you send a POST request with two parameters to this URL: One parameter specifies the action (e. g. action=delete), the other parameter is for the anti-CSRF token. If your application uses Ajax rather than classical form-based interaction, you can use the DELETE method instead of POST and omit the action parameter. That's even more elegant. Unfortunately, HTML forms are currently limited to GET and POST.

That may be the case for big e-mail providers, but some people run their own mailserver, so they aren't restricted by any such policies. You want your application to handle all possible input, not just most of it. Actually, why do you even need the e-mail address or user ID? The activation token is supposed to come from a strong random number generator, which means it's automatically unique. So you might as well create a UNIQUE index for the column where you store the token hashes and use that as a lookup key. No need for any additional information. This is a much cleaner solution. Is the actual activation triggered merely by visting a certain URL? If that's the case, then it's a conceptual error. GET requests must not have side effects. Their sole purpose is to get a resource (hence the name). If you abuse the GET method for data changes, this can have serious consequences ranging from accidental requests to actual attacks. For example, any image with the source https://yoursite.com/admin/delete_user/123 will automatically trigger a request to that URL. Of course the account activation is a fairly harmless case, because it doesn't cause any damage and involves a secret (the activation token). But you should design your framework in a way that there's a clear distinction between getting a resource (with GET) and changing a resource (with POST, PUT etc.). Speaking of attacks, do you have anti-CSRF tokens?

What error? What exactly makes you think there's something wrong with your old script? The only error you've mentioned so far came from your MySQLi update. If your script merely triggers a bunch of deprecation warnings, that doesn't mean you have to do complete rewrite. But even if there actually is a critical error due to some old feature you've used, fixing it should be a matter of minutes. PHP hasn't changed a lot in the last years (besides additional features, of course).

No. E-mail addresses are much more complex than you may think. If you just drop them into your URL, the URL may break. Even worse, you seem to use URLs to trigger actions (which is very wrong). Combined with a URL injection through the e-mail address, this might be used for actual attacks. Of course you could percent-encode the e-mail address to make sure it won't alter the URL. But that will of course look incredibly ugly.

None of this makes any sense, and almost every line is a gaping security hole. Do you honestly store the plaintext password in your database and the session and a frigging cookie? Why on earth would you do that? The password is secret data. You don't store it anywhere, especially not in a session or cookie! Since you're obviously new to web programming, I strongly recommend that you learn the basics of security before you go any further. Things like: What do I do with user passwords? How do I pass PHP values to an SQL query? This remember-me stuff is particularly nasty and should be avoided at all cost. If you absolutely must have it, then you need to learn twice as much and actually think before you write code. One possible approach is to use temporary random access tokens: You generate a strong random number. For example, read 16 bytes from /dev/urandom or a similar device. You store this number (not the password or the username) in a special remember-me cookie. Since this cookie is critical, make sure that it's protected. For example, set the HTTPOnly and the Secure flag (the latter only works if your site supports HTTPS). Then you store the current timestamp and a hash(!) of the random number in your database. Do not store the actual number. This would allow anybody to log-in as any user once they've gained access to your database. The random number is equivalent to a password, so it must be protected just as well. When a user sends you a remember-me cookie, you hash the value and check if it's a valid. The hash must be present in your database, and it must not be older than, say, 30 days (if that's the time limit of the remember-me feature). The hash also tells you the user, so you go through the standard log-in procedure which creates a session etc. When the user logs out, you delete the hash in the database and ask the user's browser to delete the remember-me cookie. IP checks like you did in your code make no sense, because users typically change their IP address very often. This would make the entire feature useless. Sounds complicated? The implementation will be even more complicated. Like I said, avoid this feature unless you cannot live without it.

It makes no sense to “convert” the script to MySQLi when you merely append an “i” to the function calls and forget half of them. All that gives you is a whole lot of broken code which isn't any better than the one before. The question is what you want to achieve. Are you scared because of the mysql_* deprecation warnings? Don't worry, the PHP core devs are well aware that there are tons of legacy code out there, so they won't just remove the functions tomorrow. And even if that happens somewhere in the distant future, there may be smarter alternatives than a complete rewrite (sticking to an old PHP version, using wrapper functions etc.). So don't just blindly start changing legacy code when there's no reason for it. If you do need to update your database code (why?), then do it right and actually make use of the new features. For example, we now use prepared statements to securely pass values to queries. This escaping stuff is more or less dead. I also wonder why you've picked MySQLi instead of the much more user-friendly and versatile PDO. Just because the name sounded familiar?

Like I said, you need to do the bind_result() and fetch() outside of the function. In other words, the only job of your function is to create and execute the prepared statement. After taking a closer look at your function, however, there are more problems. Where does $conn come from? Where does $var come from? You need to actually pass those variables to the function. Otherwise they're not available. So your function needs to look like this: <?php function display_all($conn, $var) { $stmt = $conn->prepare(' SELECT whatever FROM idontcare '); $stmt->bind_param('i', $var); $stmt->execute(); return $stmt; } And you call it like this: <?php // $conn and $var need to be defined at this point $all_entries = display_all($conn, $var); $all_entries->bind_result($foo, $bar, $foobar); while ($all_entries->fetch()) { // do something } I you have a chance to give up MySQLi and switch to PDO, consider doing that. It will make a lot of things easier, including this case.

“Built-in validation function”? What do you mean? # I guess you mean filter_var() with the FILTER_VALIDATE_INT flag? Well, this will also accept signed integers like “+36”, which doesn't make a lot of sense for the age. But if you're OK with that, sure, you can use it.

Prepared statements don't work like this. Read the manual, especially the part about fetching data from a prepared statement. Since you've decided to use MySQLi, you have to choose between two ugly solutions: Either you fetch() all rows within the function, stuff them into an array and return that array. Or you return the raw statement, in which case you have to call bind_result() and fetch() after your function has returned.

Well, the server-side validation works exactly as described above. To count the number of characters, you use mb_strlen() and pass the string as well as the encoding you've used in the database column. Let's say the encoding is UTF-8 (MySQL calls it “utf8mb4”). Then your check looks like this: <?php // just for readability define('MAX_NAME_LENGTH', 30); $name_length = mb_strlen($_POST['username'], 'UTF-8'); if ($name_length !== false && $name_length <= MAX_NAME_LENGTH) { // all good } else { // name either not valid UTF-8 or too long } And the age check should be obvious: <?php // just for readability define('MIN_AGE', 1); define('MAX_AGE', 100); if (ctype_digit($_POST['age']) && $_POST['age'] >= MIN_AGE && $_POST['age'] <= MAX_AGE) { // all good } else { // age either not numeric or out of range }

Using PDO has nothing to do with “object-oriented programming”. It's merely a different syntax, and it's easy to understand without any prior knowledge whatsoever. I mean, $database->query($sql) is literally pseudo code. You can hardly make that more obvious. Actually, I wonder how you came to the conclusion that MySQLi is great for beginners. It's frigging complicated. Even I constantly have to look up the details in the manual. Take a prepared statement, for example: <?php $mysqli_driver = new mysqli_driver(); $mysqli_driver->report_mode = MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT; $database = mysqli_connect('localhost', 'foo', 'bar', 'test'); mysqli_set_charset($database, 'utf8mb4'); $author_id = 1234; $publication_date = '2014-12-23'; $blog_posts = mysqli_prepare($database, ' SELECT post_id, content FROM blog_posts WHERE author_id = ? AND publication_date = ? '); mysqli_stmt_bind_param($blog_posts, 'is', $author_id, $publication_date); mysqli_stmt_execute($blog_posts); mysqli_stmt_bind_result($blog_posts, $post_id, $post_content); while (mysqli_stmt_fetch($blog_posts)) { var_dump($post_id, $post_content); } It takes four(!) function calls plus a while loop only to make a simple query. This parameter binding stuff is also very hard to grasp for a beginner (I've personally experienced that). When using PDO, you just call prepare(), execute() and loop through the result set with foreach like with any other query: <?php $database = new PDO('mysql:host=localhost;dbname=test;charset=utf8mb4', 'foo', 'bar', array( PDO::ATTR_EMULATE_PREPARES => false, PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, )); $author_id = 1234; $publication_date = '2014-12-23'; $blog_posts = $database->prepare(' SELECT post_id, content FROM blog_posts WHERE author_id = :author_id AND publication_date = :publication_date '); $blog_posts->execute(array( 'author_id' => $author_id, 'publication_date' => $publication_date, )); foreach ($blog_posts as $blog_post) { var_dump($blog_post); } How is that more complicated? I think the only reason why people use MySQLi at all is because the name sounds familiar. That's understandable, but it's hardly a good reason.

Keep your whiny ad-hominem bullshit. I'm here for the code. You just told a user that variable injection is perfectly fine if only they add some “validation” (whatever that means). This is dangerous nonsense, and I'm not gonna keep quiet about it. The goal of this community is to help people. Of course this doesn't always work out, and sometimes we make mistakes ourselves. But what's definitely not acceptable is to harm people by telling them to blow up their server. This is not w3schools. If you're looking for a warm, fuzzy place where nobody ever disagrees with you, you won't find that here or in any other serious IT community. Go back to devshed.

Bollocks. Validation is not a security measure, and most if the time it's entirely useless. It's one of those myths which people keep repeating without ever thinking about it. So that's not your problem. The problem is that you allow the user to inject arbitrary variables into your application: extract() by default takes all input parameters and converts them to PHP variables. Existing variables are silently overwritten. It's easy to see what an incredibly stupid and dangerous “feature” that is. The user can change any variable and manipulate the entire control flow of the script. For example, let's say you have an $is_admin variable which is set to false. Well, the user will simply change that to true and declare himself admin. Not good. So do not use extract()! Actually, I wonder why you can't just use $_POST directly like everybody else. What's wrong with $_POST['some_parameter']? Too long? C'mon, do you really want to screw up your entire application only to save 9 characters? Just when we thought that the “register globals” nightmare is finally over, it's coming through the backdoor.

And please forget about this “an admin page doesn't need any protection” nonsense. This is exactly how Ubuntu Forums were “hacked” last year: The attacker gained access to a privileged moderator account and was then able to inject arbitrary JavaScript code into the site. An admin page needs the exact same level of protection like any other page. Just because you think that only admins have access to your page doesn't mean that this is actually the case. For example, you have no CSRF protection whatsoever, so anybody can “tunnel” arbitrary requests through one of the admin accounts simply by getting the person to visit a website. Admins aren't necessarily smarter than the average user. They'll make the same mistakes, and sometimes they're plain stupid. For example, I recently encountered a forum admin who used his nickname as the password – and that same nickname was also on his public user profile. There's a big difference between letting somebody manage your site content and giving them full access to your server. Right now, you more or less do both at the same time, because you allow admins to directly access your database system and continue from there.

If you're so worried, generate 100k test rows, try a bunch of queries and see for yourself. You'll realize that even “complex queries” are typically a matter of milliseconds (assuming that the database design, the indexes and the query itself are all sane). Aren't you aware that MySQL is also used by big companies? They have thousands of rows per second, not total.

Manually checking every single return value for an error status is obsolete and should not be done. Yes, it used to be necessary back in the days of the old MySQL extension. But both PDO and MySQLi support modern ways of error handling. For example, you can make them throw an exception whenever a query fails. In that case, the script automatically stops, activates the standard error procedure and gracefully shuts down. No need for any manual intervention.

Data processing is done by the database system. That's exactly what it's for. What may look like “lots of data” to you is a joke for MySQL. It can easily process hundreds of columns and millions of rows. So let it do its job and don't try to outsmart it. If you experience performance issues, that's usually due to broken database design or missing indexes. The EXPLAIN statement will tell you what's wrong. Of course there are special cases where the processing logic requires a full-blown programming language. But your example doesn't even come close to this.

Do you have permission to fetch this data? Then read their API manual or ask their support for help. Or did you never ask them and simply decided to scrape their website? Then I recommend you don't do that. Some companies don't find that funny, especially when you start breaking their obfuscation techniques.

It makes no sense to merely append an “i” to all function names. This doesn't improve your code one bit, and it will lead to a lot of bugs (as you just saw). Is this new code you wrote for yourself? Then I recommend you throw it away, learn PDO (not MySQLi) and actually use its new features. When you're a beginner, your first attempts are naturally poor, so don't waste time trying to save them. Take it as a learning experience and start over with your new, improved skills. Or is this some big application from somebody else? Then either leave the code as it is or reserve the next weeks/months/years for a complete rewrite. I strongly recommend the former. For a beginner, rewriting a big legacy application is really too much and likely to end in a disaster: You'll waste enormous amounts of time, leave the entire application in an unstable state, get bugs after bugs etc.