Jacques1

That “2” could come from anything, especially when you have a print_r() right in front of the echo. Make the output unambiguous, for example printf('<br>Inserted rows: %d<br>', $q_news->rowCount());

And how exactly is the incoming password “wrong”? What's the expected input, what's the actual input?

An array has the least syntax noise and reflects the idea of mapping keys to paths, so I'd use that. // Don't bother about micro-optimizations (or rather nano-optimizations in this case). I can assure you that there are much, much worse bottlenecks lurking somewhere in your application.

“Doesn't work” doesn't work as an error description. If you want help with a problem, you need to actually tell us what that problem is. Either way, the general approach already doesn't make a lot of sense. Why on earth would you terminate the session just because the user has closed some tab or window with your site in it? People today use lots of tabs at the same time, and they constantly open and close them. Just because I close, say, one of five phpfreaks tabs doesn't mean that I want to leave the site. In fact, I'd be seriously pissed off if I was automatically logged out while trying to submit a long reply. The implementation is also ... weird. You use the cache headers in an attempt to delay PHP execution? You generate a new session ID only to throw it away two lines later? You unset the internal $_SESSION superglobal? Are you sure you know what you're doing? What exactly is the goal here? I mean, why and for whom do you need this?

ignace doing his favorite thing, overengineering. What you totally forgot, though, is the JavaSeriousBusinessEnterpriseNoSQLCloudConfigurationFactoryObserver! Now the code is not hip anymore.

The whole 5.2 branch is dead since 4 years, and even the 5.3 branch has died a couple of months ago. Why on earth would you want to install that? PHP is at version 5.6.

No, $app is literally the App object. Since the configuration clearly belongs to the application, it makes sense to create it within the App class and expose it through a getSetting() method. Then you don't need an extra parameter in the controllers. To get a setting, you simply call $app->getSetting('security.password_hash_algorithm') which then delegates the task to its Config object.

So you don't have 5 minutes to fix the account, but you do have all the time in the word to try out workarounds which you already know are stupid? Check your priorities.

I find the implementation a bit weird. The Configuration class pretends to be a singleton with the instance stored in $instance, but then it turns out that $instance is actually an array from parse_ini_file(). This makes no sense. You're also going through a lot of trouble only to have static methods when that's actually the poorer solution compared to a standard object with dependency injection. Why not simply instantiate a Configuration object, do the config file parsing in the constructor and then pass the object to whoever needs it? It doesn't even need to be a singleton. If you pass the file path to the constructor, you can have multiple different configurations, and you avoid the hard-coded path in the class: <?php class Config { protected $settings; public function __construct(App $app, $config_file) { // make sure the config file is valid (just an example) if (preg_match('/\\A[a-z_][a-z0-9_]*\\.ini\\z/i', $config_file)) { $settings = parse_ini_file($app->getConfigDir() . $config_file, true); if ($settings !== false) { $this->settings = $settings; } else { throw new Exception('Failed to parse configuration file'); } } else { throw new Exception('Invalid configuration file.'); } } } And then you use it like this: $base_config = new Config($app, 'base.ini'); $password_algorithm = $base_config->getSetting('security.password_algorithm');

Unrestricted file inclusion is never a good idea. You seem to think that $class is only the actual class name, but it contains the complete class path including namespaces, e. g. “Foo\Bar”. Since the backslash also happens to be a directory separator on Windows, you may end up including files that were never intended for that. You're lucky that PHP currently doesn't support relative namespaces, because then you might end up with something like ..\your_config_file_with_the_app_passwords Always restrict file inclusions, don't just append the input to some path. Think about how you want to handle namespaces, otherwise restrict the names to a-zA-Z or something like that. This DIRECTORY_SEPARATOR stuff is unnecessary, just use forward slashes. That's what all operating systems except Windows use, and even Windows accepts slashes. Last but not least, strtolower() is likely to cause trouble if you use any characters outside of the ASCII range (like umlauts). Yes, that's permitted in class names.

... and “$SESSION” and “mysql_fect_assoc” and “$row[username]” ... Your really need to write your code more carefully. If you tend to make lots of typos, slow down and double-check each line before you go on. I also recommend that you use an IDE like Netbeans instead of a simple text editor. This will help you recognize obvious mistakes as you write, so that you don't have to hit F5 in your browser all the time.

This is indeed way too much. The script has become a kind of data dump where you put everything that is somehow relevant for you. Keep things separate: First of all, there's the app configuration. The data should be in a separate file for easy access (INI is fine), and you should process it as described above. So you have a Configuration object with a method like getSetting(). Then there's the meta data about the request. Consider making a Request object with methods like isHTTPS(), isJavaScriptEnabled() etc. And finally, you have a fixed set of data about your application (paths etc.). Why not make an App object with methods like getLibPath()? In addition to that, you may want specialized classes, for example a Template class for managing all the Twig stuff. This is really what OOP is all about: You divide your application into specific aspects, and then you design objects for those aspects.

Maybe you should get some sleep and try again. Concrete code is always better than vague high-level descriptions. Show us what you have (code excerpts, database tables etc.), and tell us specifically what you want. Then I'm pretty sure we can help you. The better your question, the better the answer. If you just do a quick brain dump with no effort whatsoever, don't expect anything.

w3schools is not a reputable source. It's infamous for security vulnerabilities, bad practices, outdated information and plain nonsense. If you have any doubts about that, just look at their file upload script, for example. Good information can be found at the Mozilla Developer Network. Or buy a good book.

A bunch of constants is even less modular, because now you've hard-coded a specific configuration mechanism, not just a certain class. If you later decide to, for example, put the configuration into an external JSON file, it can't be done unless you either rewrite all classes or implement some kind of emulation layer which defines the necessary constant at runtime (not pretty). For clarification, none of those approaches is inherently wrong. You'll see all of them in reputable projects, and they work fine for the specific case. If I just write a bunch of procedural scripts from scratch, I don't care about modularity or code recuse. I simply stuff all configuration values into constants. At our company, we also stick to procedural code, and the default configuration is stored in a $CONFIG global which we can override at runtime (with a local development configuration, for example). Your case is different, though: You have an OOP framework. Modularity makes a lot of sense here, because the configuration mechanism may change. You might use different mechanisms for different projects, or maybe some day you decide that you want a clean YAML file instead of all those constants. Why not prepare for that? Your framework should accept any configuration logic.

Yes. For convenience, make a base class with a getSetting() method which returns $this->configuration->getSetting('foo'). Then it's simply $this->getSetting('foo'). Well, there would be one Configuration class which has a static getSetting() method. Whenever you need a setting in one of your classes, you call Configuration::getSetting('foo'). The downside is that you'll have a hard-coded reference to this specific class (see the discussion about dependency injection).

This depends entirely on the architecture of your application. Is it mostly procedural code? Then simply make a get_setting() function which returns the value for a particular configuration key. Is it OOP with a central entry point like a front controller? Then instantiate a Configuration object in that main script and pass it to the request handlers (through the constructor, for example). If there's no central entry point, you either have to manually pass the object around, or you'll need a hard-coded reference to some Configuration class which has static methods for getting a configuration value.

This doesn't work. If two PHP processes come up with the same new number at almost the same time, then they're both allowed to have it. You'll end up with a duplicate. The uniqueness check must be done at database level with a UNIQUE constraint. You first try to insert the number, and if that fails, you try again with another number: <?php // MySQL error code define('MYSQL_ER_DUP_ENTRY', 1062); define('ACCOUNT_ID_PREFIX', 'XY'); define('ACCOUNT_NUMBER_LENGTH', 7); define('MAX_RETRIES', 3); // this should be in a separate file $database = new PDO('mysql:host=localhost;dbname=YOUR_DB;charset=utf8mb4', 'YOUR_USER', 'YOUR_PW', [ PDO::ATTR_EMULATE_PREPARES => false, PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, ]); $registration_stmt = $database->prepare(' INSERT INTO users SET account_id = :account_id '); /* * Generate a random account number and let the database check if it's unique. If the number is not unqiue, try again. * Give up after a certain number of attempts. */ $registration_successful = false; $failed_attempts = 0; while (!$registration_successful && $failed_attempts < MAX_RETRIES) { try { /* * If l is the desired number of digits, the possible numbers range from 0 to 10**l - 1. * For example, if l = 7, then the number is in the interval 0..9999999 */ $account_number = mt_rand(0, pow(10, ACCOUNT_NUMBER_LENGTH) - 1); $account_id = ACCOUNT_ID_PREFIX . str_pad($account_number, ACCOUNT_NUMBER_LENGTH, '0', STR_PAD_LEFT); $registration_stmt->execute(['account_id' => $account_id]); $registration_successful = true; // this is only executed if the query succeeded } catch (PDOException $registration_error) { $error_code = $registration_error->errorInfo[1]; // If the error was due to a violation of the UNIQUE constraint, catch it and try again; otherwise rethrow it if ($error_code == MYSQL_ER_DUP_ENTRY) { $failed_attempts++; if ($failed_attempts == MAX_RETRIES) { throw new Exception('Gave up trying to generate unique account ID after ' . MAX_RETRIES . ' attempts. The pool might be exhausted.'); } } else { throw $registration_error; } } }

I think the problem is that the sample code has absolutely nothing to do with the requirements. So instead of wasting our time with it, I'd rather start from scratch.

Don't use code you found somewhere on the Internet. Most of it is crap. Why do the numbers have to be random? Do they just have to be random-looking, or is it important that they are indeed unpredictable? One thing is for sure: Random and unique and only 7 digits long will be difficult.

Don't use MySQLi, it's incredibly cumbersome (as you just saw). Go with PDO. It's a universal database interface which works with all mainstream systems (not just MySQL), and has a very nice API. You'll find it much easier to use: $player_id_stmt = $database->prepare(' SELECT id FROM players WHERE username = :username '); $player_id_stmt->execute([ 'username' => $_POST['username'], ]); $player_id = $player_id_stmt->fetchColumn(); It's even more obvious when you have to fetch multiple rows. With PDO, you simply use a foreach loop. With MySQLi, you need bind_result(), fetch() and a while loop. Also note that PDO supports named parameters.

What do you mean? Case insensitivity doesn't help when two different characters look similar. For example “I” (uppercase “i”) and “l” (lowercase “L”). The delay of the hash computation is massive, it's usually supposed to be a full second. So anybody can easily see this in their browser: If they get a response immediately, the hash calculation has been skipped; if they always have to wait for one second when they enter a specific username, they can be pretty sure it's due to the calculation. Sure, you can do a dummy calculation, but that just means the time differences get more subtle. The control flow of the script is still different depending on whether or not the username exists. Yes, this can be measured, even over an Internet connection. You just need to take lots and lots of samples to filter out the noise. And there may be other information leaks as well, for example a certain error which is only possible in a certain branch. Personally, I've given up trying to conceil the accounts. I simply go with public usernames (instead of e-mail addresses).

A binary data type is wrong in this case, because all crypt-based hashes are ASCII-encoded strings, not raw bytes. If you (ab)use BINARY or VARBINARY for the hashes, you lose type safety and may end up with garbage data. A raw byte sequence is entirely unrestricted, but ASCII only ranges from 0x00 to 0x7F. So while your database will accept any input, you may later find out that the data you've stored isn't even valid (due to a bug, a confused admin or whatever). Always choose the correct type for the content so that the database system can do its job. Don't try any hacks to optimize performance. Even if one of them actually works out and saves you some microseconds, you risk the integrity of your data. That's not worth it. In this particular case, the performance argument makes even less sense: Password hashing is slow by design, so performance is not a concern here. The last thing you want is a bug in the password check, so any kind of hack is out of the question. The correct type for bcrypt is CHAR(60). If the algorithm is unspecified, use TEXT or VARCHAR with a sufficient length (whatever that may be).

This is much more serious than the annoying little link. Somebody was able to inject code into your site. Who says it's only a link? They might have injected actual malware as well, or maybe they're planning to do so in the next few days. This is a major security breach, and there's something very wrong with your Wordpress installation or your server. Change all passwords of your SSH/FTP accounts, admin accounts, database users etc. Check the sanity of your server. Any FTP accounts that shouldn't be there? Files you didn't upload? Make sure your server is configured correctly (file permissions, PHP settings etc.). Update Wordpress as well as all plugins. Ideally, you'd backup your current content, start with a fresh installation and then carefully restore your content piece by piece. If you can't make a fresh start, download your current content (the files and the database) and analyze everything until you've found the link as well as any other code injection. When everything is done, change all passwords again. Yes, this will be painful and take a lot of time. But again, this is a very serious problem. Personally, I'd probably shut down the site until I know exactly what the hell is happening on my server.

It makes no sense to use a concrete cost factor for the generic PASSWORD_DEFAULT, because the underlying algorithm is unspecified and may change at any time. For example, one of the next PHP releases might switch to scrypt, and then your cost factor of 12 isn't even valid. scrypt has three hash parameters, so a plain integer will blow up the code. It's either-or: Either you rely on the default settings, or you set a concrete algorithm with its specific parameters (like a single cost factor in the case of bcrypt). This depends on whether or not there's somebody to adjust the configuration as time goes by. If you're going to maintain the software throughout its lifetime, then definitely fine-tune the hash settings and update them whenever necessary. This will provide optimal security for your specific hardware. But if the application will be written once and never be touched again, use PASSWORD_DEFAULT and let the PHP core devs take care of the hash parameters. Of course the default values are necessarily weak, because they need to work on any platform. If you go with PASSWORD_DEFAULT, I'd use a TEXT column. That's more than enough for any output length now and in the future. Either way, use password_needs_rehash() to check whether a hash must be updated. The password must not be trimmed or altered in any way. Hash it as is. Every byte counts, including spaces, exotic Unicode characters and whatnot. The only limitation is that the password must not be longer than 56 bytes, because that's the input limit of bcrypt. With usernames, it's a very different story. They identify accounts, so it's very important that they're unambiguous. Allowing arbitrary whitespace or Unicode characters would be a bad idea, because you may end up with similar-looking names. In fact, an attacker could exploit the similarity of certain characters to impersonate, say, an admin. It's best to stick to the printable ASCII chars and add some rules: no case-sensitivity; if “John” is already taken, you can't have “john” similar-looking characters are considered equal (e. g. the uppercase “i” and the lowercase “L”) at least one non-whitespace character no whitespace left or right, only single space characters in between; you may correct this automatically For the general logic of the log-in, I'd use something like the following: <?php require_once __DIR__ . '/include/basic.php'; $database = get_database_connection(); if (isset($_POST['user_name'], $_POST['password'])) { $user_data_stmt = $database->prepare(' SELECT user_id , password_hash FROM users WHERE user_name = :user_name '); $user_data_stmt->execute([ 'user_name' => $_POST['user_name'], ]); $user_data = $user_data_stmt->fetch(); if ($user_data && password_verify($_POST['password'], $user_data['password_hash'])) { echo 'Welcome!'; // update hash if necessary if (password_needs_rehash($user_data['password_hash'], constant(get_config_value('password_hash_algorithm')), ['cost' => get_config_value('password_hash_cost')])) { $hash_update_stmt = $database->prepare(' UPDATE users SET password_hash = :password_hash WHERE user_id = :user_id '); $hash_update_stmt->execute([ 'user_id' => $user_data['user_id'], 'password_hash' => password_hash($_POST['password'], constant(get_config_value('password_hash_algorithm')), ['cost' => get_config_value('password_hash_cost')]), ]); } } else { client_error('Incorrect username or password.'); } } else { client_error('Parameter "user_name" or "password" is missing.'); } Note that the above code will tell everybody whether or not the username exists, even if the message just says “wrong username or password”. The hash computation takes a long time and is only done if the username has been found, so that's a clear indicator. If you want to hide the usernames, you'll need a much more complicated control flow to eliminate obvious time differences. But even then it's probably not feasible.