Jacques1

mysqli_stmt::get_result() yields a MySQLi result set from a prepared statement.

If you hate MySQLi, then why did you choose it in the first place? PDO has a much nicer API, and it can be used with all mainstream database systems, not just MySQL. So if there's a chance to switch to PDO, consider doing that. Otherwise you'll have to be a lot more specific than “doesn't work”. Both PHP and MySQL provide detailed error messages. Is your PHP error reporting enabled? Where's the code for handling SQL errors?

I'm not familiar with those management panels, but you should be able to view the file properties and see exactly which user and group a particular file belongs to. All users are stored in /etc/passwd, and the groups are in /etc/group. The upload directory should be assigned to the user and group of the webserver (usually something like www-data). By the way, don't use FTP. It provides neither confidentiality nor integrity, so anybody who happens to sit between you and the server can read the traffic (including your password) and possibly even manipulate it. Use a modern, secure protocol like SFTP or SCP. There's a single system group called “wheel” which contains privileged administration users. Do not use this for regular users!

This is the classical singleton approach. While it indeed works, it comes with huge problems: Your classes have ties to all kinds of global identifiers, which makes testing extremely difficult. You can't just take the individual User class and test it with a mock database object. You'd have to simulate the entire environment and point all Db, PDO etc. references to test classes. You're restricted to a single database connection once and forever. This can become a problem. For example, should you decide to store your PHP sessions in the database, you need two parallel connections. So you'd have to rewrite your whole database class and all classes depending on it. I'd take a different approach: Instead of passing a single database connection around, use a connection manager which supports an arbitrary number of named connections (e. g. “default” for the standard connection and “session” for the separate session connection). Instead of hard-coding the Db class, use Dependency Injection. You can either manually pass the connection manager to the User class, or you can use a Dependency Injection Container like Pimple.

If somebody tells you that you need 777 permissions for uploads, then they clearly don't know what they're doing, and you probably shouldn't use their code. Why should the group have any permissions? Why should “others” have any permissions? Why should the folder be readable (which enables directory listing)? Why should the individual files be executable when they clearly aren't supposed to contain code? At most, the upload directory would be executable + writable for the owner (300), and the invidual files would be writable + readable for the owner (600). But again: Just because the printer expects plaintext content behind a “.txt” URL doesn't mean you physically need a text file. It should be perfectly possible and much easier to “emulate” the text file with a PHP script hiding behind that URL. The script can read the individual orders from a database and serve them as plaintext content. Anyway, if you disagree, we can leave it at that.

First of all: Security doesn't work like this. You don't start with all possible privileges and then think about the damage this can cause. You start with zero privileges and then figure out what is required for the job. So you have it backwards. Using 777 permissions on a folder means that every single account on the system (including the webserver) can create new files, delete existing files and list the folder content. Even worse: Since this is the document root, your webserver will probably execute any file ending with “.php”. So once an attacker has gained access to the folder (which probably isn't too hard in your case), they can create a malicious PHP script, execute it through your webserver and get deeper into the system. If you routinely use 777 permissions, you're now screwed. Why on earth would you risk that? To be honest, the whole approach doesn't make a terrible lot of sense to me. Is there any reason why you need physical text files all over the place? Can't you use a proper database and a PHP script which serves the data dynamically?

So what exactly prevents you from using an actual AUTO_INCREMENT column? This “PROD...” stuff is only how you would display the numeric ID, and it can easily be done using (s)printf(): <?php $id = 5; printf('The ID is PROD%05d', $id); In any case, don't try to invent your own auto-increment mechanism. If you naively select the latest ID, increment it and then update the ID, you'll get duplicate IDs when two PHP processes run at almost the same time.

Generally speaking, you shouldn't use the session for anything but temporary unimportant data. Sessions have lots of security issues: They can be hijacked, fixated and poisoned. Usually all websites on the same server have unrestricted access to the session files. There's no fine-grained permission system. Sessions can fall out of sync (as requinix already pointed out), which is a huge problem for critical data. For example: If you revoke certain privileges from a user, they may still have a session claiming they do have those privileges. A session contains serialized PHP values, not just plain strings. This may be used to manipulate the control flow of your program. For example, the check $_SESSION['key'] == $expected_key will always yield true if $_SESSION['key'] is the boolean value true instead of a string. While it's theoretically possible to evade some of those risks with Authenticated Encryption, a much simpler solution is to put critical data into the database system.

You can actually have both a public link and tight access control by using share links. For example, your customer gets this link: https://yoursite.com/quotes.php?token=7c043ece6892c4869db68d3e824ef5bc All tokens are stored in the database together with their status (valid/invalid) and the file they map to. When the script receives a token, it tries to look it up in the database, and if everything is OK, it displays the corresponding file. This provides maximum convenience for your customers (it's just like a normal file link), but at the same time you can control the file access. You can make the links expire after a while, you can manually disable them in case they're leaked etc.

This makes no sense. You want $servername, $dbnewuser etc. to be replaced with the corresponding values, but at the same time you want $mysqli_connection to show up as a literal variable in the generated script. How is PHP supposed to tell the difference? If you want a literal dollar sign within a double-quoted string, you need to escape it with a backslash: echo "\$some_variable"; Secondly, you can't just insert strings into your code template, because those will end up as raw words. What you want are string literals, and those have quotes: $string_content = 'yada yada yada'; echo "String literal: '".addslashes($string_content)."'"; You should escape the content with addslashes() so that a literal quote within the string content won't blow up your generated PHP script.

Well, nesting three languages just isn't a good idea: You have a PHP code generating shell code generating PHP code. No wonder you have syntax conflicts. This can be made a lot simpler. PHP can in fact write files, there's no need to delegate this to the shell. There's also no need to generate an entire PHP script. Just define a bunch of constants and put the application logic into static scripts: <?php const DB_USER = <insert string here>; const DB_PASSWORD = <insert string here>; ... You should also consider using a simple data format like JSON, YAML or XML instead of a full-blown PHP script.

Storing the user ID in the session is the standard approach. It's pretty much the whole point of sessions (besides storing temporary data).

So who broke the original cookie parameters, and what are the values before you override them? It's not normal that you have to reset the parameters in every single script. This should be fixed on a higher level like the global php.ini or a custom ini file or an .htaccess file.

Before you randomly change the cookie settings, you should investigate the problem and figure out what's really going on. Who says it's a cookie problem? It could be pretty much anything. What does var_dump($_SESSION); tell you? Is the session in the target script entirely empty? Modern browsers also come with developer tools (usually accessible via F12). Clear all session cookies, then try to log in. Does the server send a session cookie? Does your browser send it back when visiting the target script?

You should actually get rid of this silly try-catch stuff altogether. The whole point of making PDO throw exceptions is that you do not have to manually check every single query for errors. With exceptions, query errors are automatically detected and trigger a fatal error with all relevant information. If you prefer to copy-and-paste the same error handling procedure over and over again, you don't need exceptions (but I'd rather do the opposite: keep the exceptions, get rid of the error code).

This is not secure. At all. There's not even a consistent approach to security: Sometimes you do escape the input values[1], sometimes you just stuff them right into the query string[2]. Sometimes you do try to escape identifiers[3] (using the wrong procedure[4]), sometimes you just stuff them right into the query string[5]. And then you append entire strings to your query with no checks whatsoever[6]. So this is full of SQL injection vulnerabilities and won't even withstand the most basic attacks. Unfortunately, the code cannot really be fixed either, because a lot of problems are on a conceptual level. For example, if you accept an entire WHERE clause through a parameter, you just can't make it secure (unless you're willing to implement a full-blown SQL parser). And escaping identifiers is incredibly hard. The developers of Doctrine, one of the most sophisticated database abstraction layers in PHP, eventually gave up. Do you think you can do better? This begs the question if the class even makes sense: You'll need a lot more knowledge, attention and effort to get this halfway secure, and then your users again need a lot of knowledge, attention and effort to avoid the inevitable gaps. For what? A bit of syntax sugar? Meh ... I understand that database abstraction layers look like a good idea, but implementing them correctly is incredibly hard, and using them correctly is even harder. It's virtually impossible to create real abstraction, so everybody ends up with some half-assed query builder for expert users who know exactly what they can and cannot do with that. Personally, I'd stick to good old mysqli_query(). [1] Example: $v in insertRecords() [2] Example: $value in updateRecords() [3] Example: $f in insertRecords() [4] mysqli_real_escape_string() is strictly for escaping MySQL string literals (hence the name). You cannot escape identifiers with it. [5] Actually, most identifiers are entirely unprotected. For example, $table in all query methods or $WK in selectRecords(). [6] Example: $condition in deleteRecords() or $selection in selectRecords().

No, the procedural style is perfectly fine.

Clickjacking isn't a problem of your code, it's primarily a webserver configuration issue. Right now, anybody can embed your website in a frame and put a fake GUI on top of it. When an unsuspecting user interacts with this fake GUI, they'll actually trigger actions on your site. This is why big sites like paypal.com, amazon.com etc. all restrict framing. To fix the problem, add the X-Frame-Options header in your response. For example, this will block framing altogether: X-Frame-Options: DENY You can either set the header in your webserver configuration, or you can send it with PHP using the header() function.

You've adopted some weird and even dangerous techniques, so the first thing you should do is actually learn how the MySQLi extension works. Queries don't just fail. Whenever there's a problem, MySQLi provides a detailed error report. This can either be manually requested through mysqli_error(), or you can ask MySQLi to automatically throw exceptions: <?php const DB_HOST = 'localhost'; const DB_USER = '...'; const DB_PASSWORD = '...'; const DB_NAME = '...'; // Turn on exceptions so that you don't have to manually check for errors $mysqliDriver = new mysqli_driver(); $mysqliDriver->report_mode = MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT; $databaseConnection = new mysqli('localhost', DB_USER, DB_PASSWORD, DB_NAME); You can't just insert PHP variables into query strings. This will lead to SQL injection attacks and crash your application whenever the input happens to include a single quote (which does happen in the English language). To fix this problem, use prepared statements: $registrationStmt = $databaseConnection->prepare(' INSERT INTO student SET firstname = ?, lastname = ?, email = ? '); $registrationStmt->bindParam('sss', $_POST['firstname'], $_POST['lastname'], $_POST['email']); $registrationStmt->execute(); As you can see, the $_POST values never touch the query string directly. Instead, you create a query template with three parameters (the question marks), and then you bind the values to those parameters. This provides perfect security and robustness. Last but not least, you should get rid of this weird “CamelCase” naming style. Make the identifiers all-lowercase to avoid confusion and mistakes.

Even if there was a sane way of keeping the database credentials in some kind of session, it's still an awful idea to let end users choose passwords for internal database roles. Many people routinely use passwords like “123456”, and you certainly don't want that as a database password. End users are not database administrators (unless you're implementing some kind of PHPmyadmin), so it's your job to create an abstraction layer between the application users and the database roles. It does make sense to use separate roles for different user groups (e. g. standard users vs. administrators), but it's nonsensical to create a new role for each user.

There are a lot of things that need to be revised and optimized. But I'd do this step-by-step, not all at once. Fixing the HTML and learning a new database interface and diving into database normalization and optimizing the queries might be a bit too much.

No, this is not one form. Look at your code again: There's an empty form before the for loop (for whatever reason), then you create a sequence of forms with no submit button (for whatever reason), and finally you create yet another form with no data but a submit form (for whatever reason). This obviously makes no sense. It seems what you actually want is one big form with multiple sections: <form method="post" action="test.php"> <fieldset> <legend>Question 1</legend> <p>What's the answer to question 1?</p> <input type="text" name="answer[0]"> </fieldset> <fieldset> <legend>Question 2</legend> <p>What's the answer to question 2?</p> <input type="text" name="answer[1]"> </fieldset> <fieldset> <legend>Question 3</legend> <p>What's the answer to question 3?</p> <input type="text" name="answer[2]"> </fieldset> <input type="submit"> </form> Note the bracket notation in the name attributes. This turns $_POST['answer'] into an array of the answers. Otherwise the values would overwrite each other.

And it's “method”, not “metod”. Besides that, your code is wide open to SQL injection attacks, uses deprecated database functions and obviously lacks errors handling. Even worse: It tells the user that the insertion was successful when that's clearly not the case. I strongly recommend you switch to the PDO database interface and use prepared statements to safely pass user input to queries. You should also check for errors and give proper feedback.

This isn't just about your toy website. Insecure applications can compromise the entire server, and I'm sure that is a problem. Sure, you can do whatever you like (as long as nobody holds you accountable for it). But it'll be hard to find a programmer who's willing to give up all his self-respect and help you with this. So if you want to go the wrong way, you're probably on your own.

You have much bigger problems than people logging in too often. That code you've found is horribly outdated, poorly written and violates almost every security principle. Plaintext passwords? Connecting to the database as root? Really? It makes no sense to add advanced security features when this doesn't even pass the most basic security checks. Implementing log-in limits is also much harder than it may seem. For example, there are proxy servers and VPNs where thousands of (legitimate) users share the same IP address. Does that mean they'll all be locked out just because one user pressed the log-in button too often? At the same time, actual attackers can easily switch to a different IP, because they typically have large botnets. So a naïve implementation will cause more harm than good. I think you should take care of the basics first: Learn how to use a modern database interface, learn how to hash passwords and learn the basics of security. This will be much more useful than copy-pasting bad code you found somewhere on the Internet.