Jacques1

Using absolute paths and then expecting them to somehow be turned into relative paths is a really, really bad idea. The path /includes/file-name.php is absolute and literally means: “Right below the root filesystem, there's an includes directory with the script file-name.php”. This obviously makes no sense. Maybe your Windows PC interprets the path differently, but all Unix-based servers will see an absolute path. If you want a path to be relative to some base directory, then you actually need a relative path. Use the include_path directive to set the base directory: // assuming C:/htdocs/ng is in your include_path include 'includes/some-file.php';

If there's a vulnerability specifically within an “UPDATE users SET ...” query, then the entire authentication and authorization system is worthless. But this is rare. Most attacks happen by finding any vulnerable query and using that to fetch secret data from arbitrary tables.

How often do you really have to change the table or column names? I've worked on a lot of PHP applications where the identifiers were simply hard-coded within the query strings, and I never had a problem with that. I'm generally not sure if your database abstraction approach is a good idea. I mean, how is $this->select_from_table_condition_user_input('username', 'users', 'id = ', $user_id) better than $this->query('SELECT username FROM users WHERE id = :id', ['id' => $user_id]); ? All I see is disadvantages: Your code is harder to read, because it's not plain SQL. It's less secure, because you don't use prepared statements. Even worse: Your code almost provokes SQL injection vulnerabilities, because there's no clear concept for escaping the input. Some arguments are copied verbatim into the query string, others are appearently auto-escaped. This is extremely confusing. Similar approaches have shown a lot of security problems in the past. Your code is less efficient. For example, fetching all records in order to do a count() within PHP is much, much slower compared to a COUNT(*) within the database system itself. As soon as the queries become more complex, your class will be in the way, because you expect a simple, fixed query structure. What if I need to join multiple tables? What if I need a union? I my opinion, you should either use plain SQL or a professional database abstraction layer like Doctrine. But don't try to invent your own, especially when that's not even your goal (you said you want to implement a forum).

The tokens are equivalent to passwords, because anybody who knows them can take over the account. That's why you need to protect them in the same way that you protect passwords. An SQL injection attack that reveals all current tokens would be a disaster. Even worse: Since anybody can request a token, it's even possible to target specific accounts (e. g. admins).

The code makes no sense and is actually dangerous, because you allow anybody to update anybody's password and take over their account. For example, I could request the following URL: https://www.yoursite.com/reset_password.php?u=admin&token=idontcare You would change the token of “admin” to “idontcare” (which is obviously nonsensical) and allow me to set a new password for the admin account. Now I'm an admin of your site. It's really important that you understand the logic of a password reset before you write the code. Draw a diagram or write down a verbal description of the steps. Make sure you know what you're doing. I'll repeat the steps again: A visitor requests a password reset token for a particular user account. At that point, the visitor cannot change the password, because they first have to prove that they actually own the account. You generate a random token, send it to the e-mail address of the account and store a hash of the token in the database. The visitor clicks on the password reset link in e-mail. This automatically submits the token to the actual password reset page. On the password reset page, you hash the token and check if that hash is present in your database. If it is, then the visitor may change the password. So there are two steps: You generate a secret (the token) and send it to the user's e-mail account. The user can then use this secret to prove their identity and change their password. You should make two separate PHP scripts for the two steps. OK? If you're still not sure how it works, simply try out the password reset on Facebook, Amazon or whatever site you're using.

Well, “data:image/gif;” certainly isn't valid Base64. So you have to extract the actual image data before you decode it.

I mean this: UPDATE users SET reset = ".$tokenHash." WHERE username = :u ^^^^^^^^^^^^^^ You should do this: UPDATE users SET reset = :hash WHERE username = :u ^^^^^

Your $handler variable is either not defined at all or pointing to something other than a PDO instance. Also, don't insert $tokenHash directly into the query string. You already have a prepared statement, so use a second parameter for the hash.

Do your own homework.

You're not supposed to store the plaintext token, but you do store its SHA-256 hash. To check if a token is valid, you hash it and search that hash in the database. Generating a token: <?php // generate 16 random bytes $rawToken = mcrypt_create_iv(16, MCRYPT_DEV_URANDOM); // encode the random bytes and send the result to the user $encodedToken = bin2hex($rawToken); // hash the random bytes and store this hash in the database $tokenHash = hash('sha256', $rawToken); Looking up a token: <?php $encodedToken = $_GET['token']; // decode the token and hash it $rawToken = hex2bin($encodedToken); $tokenHash = hash('sha256', $rawToken); // now look up $tokenHash

Let's not argue about terms here. Sure, the term “session” might not have been ideal in this context, but I think we all understand the situation now: Users can start surveys, but every user should only have one survey at a time. Yes, this can be solved with a UNIQUE constraint.

SQL doesn't work like this. If you want the author_id to be unique, then add a UNIQUE constraint to that column: ALTER TABLE sessions ADD CONSTRAINT UNIQUE (author_id); This may be problematic in the context of sessions, though, because if the session isn't terminated (for whatever reason), you'll not be able to insert a new session. It might more sense to overwrite the old session or keep multiple sessions with only one marked as active.

The character_set_server setting is only the default value for the storage encoding. If you've explicitly declared the storage encoding of your database as UTF-8, that setting is irrelevant. Since the default connection encoding is UTF-8 as well, you don't necessarily have to declare it with PDO.

Then you actually have to use the PDO charset attribute, or else MySQL will use Latin-1 as the connection encoding (which is incompatible with any Unicode encoding). Alternatively, you can set the default-character-set in the MySQL configuration. Since your data already is stored UTF-8 and your application supports UTF-8 as well, it makes no sense to transcode everything to UTF-16.

You've mixed up two naming styles: $this->allowedTags isn't the same as $this->allowed_tags. If you fix this, you can in fact define the attribute in the class body. Is this the best approach? Well, it's difficult to tell given this very abstract code, but hard-coding the tags inside the class means you won't be able to ever change them (unless you change the class definition, of course). It might make more sense the define the tags outside of the class and pass them through the constructor.

There are multiple encodings involved when PHP interacts with MySQL. On the one hand, there's the encoding of the stored data. This is determined by the CHARACTER SET declaration within a CREATE TABLE or CREATE DATABASE query. Then there's the connection encoding which affects a particular database session. That's what the charset attribute in the PDO DSN string is for. The connection encoding may be different from the storage encoding. For example, the client may prefer to send and receive UTF-16 encoded strings while the database uses UTF-8 for its data. It's theoretically possible to define a default connection encoding in the MySQL configuration, but in my experience, most people do not do this. In that case, MySQL will fall back to Latin-1 (aka ISO 8859-1). Since you can't always rely on the default setting, it's a good idea to explicitly declare it in your DSN string. And of course there's the encoding of the PHP strings. All three have to be compatible. Otherwise you'll end up with weird characters or question marks. Are you talking about the storage encoding or the connection encoding?

The second method is definitely not a good idea, because it relies on implementation details of PHP arrays and can lead to very nasty bugs. For example: It shouldn't make any difference in what order you enter the data, because the CSS classes only depend on the number ranges. So $data = [ 100 => 'emission_a', 120 => 'emission_b', ]; should be the same as $data = [ 120 => 'emission_b', 100 => 'emission_a', ]; or $data = []; $data[120] = 'emission_b'; $data[100] = 'emission_a'; But your implementation does not meet this basic expectation, because you rely on the indexes being ordered. If they aren't ordered (be it because you forgot it, be it because somebody else doesn't fully understand your implementation), the whole thing will fall apart and produce the weirdest results. So, no, you cannot use this. If you insist on this method, you'll have to at least sort the array before you use it. But then again: Is this really the best approach? Sure, it's more flexible, but do you even need this flexibility? Is it worth making the code less readable?

You should avoid degenerate switch statements like that, though. They're extremely confusing, and many people won't understand them at all. The switch statement is specifically for fixed values. But even then you should think twice before you use it, because the fall-through mechanism (all cases up to the next break; statement are executed) make it very error-prone and have lead to a lot of serious defects. The syntax is also rather weird. Use the most readable solution, and that's an if statement in this case.

Because if a cross-site scripting attack happens, the same-origin policy will prevent the code from accessing your actual site. I wouldn't do any checks for the sake of (pseudo-)security, no. But of course you need to somehow determine the intended file type. Personally, I use the file extension. Since Windows is very picky about the extension, that's usually a good estimate. If no file extension is present, you may use the MIME info as a fallback. Whether you take the client-provided info or do your own check doesn't make much of a difference, but since the server-side check is guarenteed to return a valid type, it may be preferrable.

Like I said, neither the MIME info nor the extension nor the result of finfo_file() say anything about the actual file content. A harmless file doesn't become malicious just because the browser has somehow gotten the MIME info wrong, and a malicious file doesn't become harmless just because it's declared as a PDF document or starts with a PDF header. So none of those methods provide protection. At best, they can help well-meaning users detect errors (e. g. they accidentally uploaded the wrong file). Protection depends on how you deliver the file: Serve it as an attachment or make sure the Content-Type is harmless. Make sure the target file extension (which doesn't have to be the original extension) is harmless. Serve the file from a separate domain. Consider doing a malware scan (how much this helps is debatable, of course).

What exactly are you trying to achieve, NotionCommotion? If the MIME info in my request says “§%&!” or “application/dangerous-php-script”, will you reject it? Why? Isn't this info entirely irrelevant? Also, what are you trying to achieve with server-side MIME sniffing? This merely checks the basic structure of the file or the presence of some magic bytes. It does not say anything about the actual file content. For example, it's perfectly possible to embed malicious code into a PNG structure.

You don't let them see any plaintext password. Never send plaintext passwords. Use a temporary random reset token as described above. With this token, the user can choose a new password.

You cannot “decrypt” a hash, because hashing is not encryption. Actually, the whole point of hashing passwords is that it's practically impossible to retrieve the password (assuming it's sufficiently strong). If anybody could just turn the hash back into the password, the whole exercise would be rather useless, don't you thin? So, no, you can't send people their old password. But you can reset the password: Generate a sufficiently long random number (at least 16 bytes) using openssl_random_pseudo_bytes() or mcrypt_create_iv(). Hash the number with something like SHA-2 and store the hash in the database. Send the number to the user by e-mail, embedded into a password reset link. For example https://www.yoursite.com/reset_password.php?token=01b81eb4247dd47bb554537db0c500a4 In the password reset script, you check if the random number is valid, and if it is, the user may change their password.

The whole reason why an empty action attribute was prohibited in the HTML5 spec is because browsers would not handle this consistently. So, no, this isn't just a theoretical requirement for making the validator and a bunch of language purists happy. Quite the opposite. I guess all current non-crappy browsers do treat an empty action attribute correctly, but if you're also dealing with legacy browsers, I wouldn't rely on it. Plus: Why would anybody prefer an explicit empty action over no action at all?

You haven't answered the second question: Is it OK if anybody can take over any account? If it is, then simply put the user ID into a cookie and use it to identify the current visitor. Of course this is incredibly stupid, because anybody can manipulate their cookies and claim that they're logged in. But given the stupid requirements, this approach seems to be appropriate.