First of all, your code looks rock-solid, obvious in what it does and very clean. This is not something that I would expect from someone "new in development". Whoever you learned all this from, you should keep them.
You got all of the major topics of a login system right:
- Your code uses parameterized statements for all database access, to protect against SQL injections.
- Your code saves passwords only in their hashed form, to protect against data breaches.
- You use a decent password hashing algorithm. Good that PHP provides a sane default here and makes it easy to use. Throughout its long history, PHP had not always made it easy to write correct and secure code. For password hashing it succeeded to provide a good API, and you use it perfectly as well.
- You use the full available character set from Unicode by connecting to the MySQL database using the
utf8mb4
encoding. This allows emojis in user names 🙂 to be saved correctly. - All HTML that is printed carefully avoids to reflect any user-provided data. This prevents cross-site scripting.
Next, I'm looking at the details of the code, from top to bottom, see if there are any unnecessary stylistic variations.
database.php
For me, the comment on the DatabaseConnection
is redundant. The name of the class already expresses clearly that the DatabaseConnection connects to the database. Therefore I would not need that comment. Since you are still new in development, it's perfectly fine to keep that comment until you (and whoever else works on your project) don't need it anymore because it's obvious.
Same for the will be inherited
part in the comment. My IDE lets me quickly see the type hierarchy of classes, which makes this comment redundant as well. Plus, will be
refers to the future and is probably outdated by now.
The constructor of DatabaseConnection
initializes the field $this->database
, but when looking at the class DatabaseConnection
alone, that field is not used anywhere. This feels strange at first. I'm somehow missing the private $database
declaration, like you wrote it in the class SignUp
.
Still in the constructor of DatabaseConnection
, you wrote try{
without a space in between. It is more common to write try {
with a space in between, just like there is a space after if
and catch
. If you are using an IDE, it has a command to "format the source code", so you don't have to do that yourself. There are many different styles of formatting the code, it doesn't matter which one you choose. Just choose one that you like and apply it consistently to all of the code.
Still in the constructor of DatabaseConnection
, when I first saw the code, I didn't see a reason for the try catch
block at all. If you remove that block and only keep the single line $this->database = ...
instead, the original stack trace will be preserved, including all information about the actual cause of the exception. By wrapping the PDOException
in a plain Exception
and only copying the message, you give away this context information. So far for my first impression.
The original stack trace contains a lot of information that is useful for tracking down errors. However, it also contains the database credentials, and these must not end up in the log files of the server. Therefore stripping the exception from all unnecessary information is essential in this case. To alert future readers of the code, I would add a comment explaining this in a single line, or just referring to a Stack Overflow answer with all the details.
registration.php
The comment This is a registration script
is redundant. Keeping the author information is good if you plan on passing the project to someone else later, so that they have someone to contact. Whether or not the copyright 2021
is necessary or helpful is something I cannot decide for you.
Declaring strict_types=1
is good style, you should add it to database.php
as well.
The class SignUp
has no comment above it. This leaves a reader a bit unclear about what a SignUp
really is. It might be "all data that is collected on the sign-up page", or it might be "the data that is actually required to perform a sign-up operation". The difference between these two is that the former would include the password confirmation field, but the latter would not.
You declare SignUp extends DatabaseConnection
, which sounds strange. It's not that "every sign-up is a specialized database connection", but that's what the keyword extends
usually means. Instead, it's more likely that the class SignUpPage
would use a database connection. Therefore I would drop the extends DatabaseConnection
from the class SignUp
.
It's a nice trick that by declaring private $database
in the class SignUp
, you make that previously undeclared field of the class DatabaseConnection
visible. Having tricks like these in the code may be confusing for readers, or they may even expect them. Instead of this trick, I would rather declare a field private $database
of type DatabaseConnection
and initialize this field in the constructor.
I like the field name $passwordHash
since it makes it obvious that the password is never stored in its plain text form. I hope that you use that field in exactly this way, I haven't looked at the actual code yet. But even if at some place there were some code that read $passwordHash = $_POST['password']
, this would immediately alert any careful reader that something is wrong here, which is a good second-line defense.
I'm not sure what the purpose of the field $signUpPage
is. It could be a complete URL like https://localhost:3000/signup
, it could be the HTML that makes up the sign-up page, or it could be the simple word signup
, as in your code. A more precise name would be $relativeSignUpPageUrl
, but that name is rather long, so it's understandable you chose a shorter name. In that case it might help the reader to add a small comment describing an example value for the variable. From this example value it is usually simple enough to infer all allowed values.
The function setName
has the comment Assign the username
. First, when I let my IDE render that comment, it looks like this:
The word Assign
is listed in the section of the parameter $name
while it really describes the function, not the parameter. The comment should rather be:
/**
* Assign the username
* @param string $name
*/
If you were to rename the function to setUsername
, each word of the comment would be redundant, allowing you to leave out the comment completely.
In the function setPassword
, the comment contains the words & $confirm
. The &
should be written as &
since the comments contain HTML, just like Javadoc (different programming language, same idea). When you search for @param
in that document, you will find in match 16/36 that each parameter has a @param
of its own. It's the same for PHP.
In the comment for signUpPage
, I don't see a need to write Page
with a capital P. Same for returnWithError
.
The comment for existingName
says "Checks whether a name exists". The return type is void
, which is unexpected to me. I would have expected bool
here since that's the correct type for "whether". Instead, the function exits if the name does not exist. Therefore I would rename that function to checkNameExists
. Function names starting with check
usually behave this way, throwing either an exception on failure or exiting plainly.
In the function ValidateNameLength
you started the name with a capital V
, that is unusual. The other function names start with a lowercase letter.
In ValidateNameLength
you check the length of the name using mb_strlen
. This is very careful, many other programmers would have used the wrong strlen
instead.
In addUser
, you use a \w
in the regular expression. The documentation for the \w was a bit hard to find since it is not directly referenced in the documentation of preg_match
, but anyways. The \w
includes all "word" characters, even outside [A-Za-z]
. This may include Cyrillic, Hangul and several other alphabets. If you want to avoid usernames that look equal but are not (the Latin a
and the Cyrillic а
), you should rather restrict the allowed characters to [A-Za-z]
first. You can always extend the character set later, restricting it is more work since it might affect existing usernames.
Still in addUser
, the SQL statement currently depends on the order of the column names in the database. If the order of the columns should change in the future, your code will put the data in the wrong columns. To prevent this, you can list the column names explicitly, as in INSERT INTO users(name, email, password_hash, remote_addr, client_ip, forwarded_for, user_agent)
. This change, of course, will make your code dependent on the names of the columns instead of their position. Choose either way, I'm not sure which change is more likely to happen.
main.php
The call to new SignUp
with the database connection parameters looks strange. This is because a Page
object should not need to know these connection parameters as individual values, it should rather only have a single parameter of type DatabaseConnection
. Assume that the database connection needs more more parameter, for whatever reason. Then you would have to adjust the call to new SignUp
, and that feels wrong because a page should be independent from this low-level change.
Summary
Even after these many detailed remarks, the code still looks very solid to me. The remarks can be fixed easily, and the overall structure of the code is very nice and easy to understand. Very well done.
And to answer your initial main question: Yes, your code is secure. All user-generated data is only used in safe ways, without allowing any injection attacks.