Skip to main content
added 102 characters in body
Source Link
tim
  • 25.1k
  • 3
  • 30
  • 76

Data Handling & Security

You use prepared statements, which protects you from SQL injection, which is good. However, the way you handle passwords could be improved to increase security.

It's not a good idea to semi-randomly apply sanitation functions to user input as it results in dirty data.

stripslashes for example really only makes sense when magic quotes is enabled, so you should check that first (and then only strip the slashes in some init file, not every time you access GET). For example, if a user has a password like this: a\'b\\c\'d, stripping slashes reduces the password length from 9 to 6 characters.

Same problem with strip_tags, only much worse. If my password is a<super$secure!password, now my password is a.

You also don't need strip_tags. As XSS protection it's not sufficient anyways (and definitely not needed for passwords, which aren't echoed); You should HTML-encode data when echoing it instead.

There's also really no need to have a length restriction on the password, especially not one that is shorter than what your hashing algorithm allows.

Hashing

Simple sha is really not good enough anymore, as it's too fast. Use bcrypt instead.

Error Handling

Don't echo database error messages directly to the enduser. They won't know what to do with it, and it may provide attackers with information or enable some forms of attacks (such as error based SQL injection).

It's also not a good idea to echo anything in a function, as side-effects like that make it harder to reuse.

Structure

I think your structure is pretty clean, and your code easy to read. Just a couple of small points:

storeFormValues doesn't seem to have any purpose. I wouldn't call the constructor inside a function of the same class, but just use it directly.

I would also not pass a magic array to the constructor, as it's hard to use and reuse (I have to guess how the array must be build and what it must contain, and I have to use the pre-defined array keys).

Instead, just change your constructor to __construct($username, $password). This increases clarity and reduces future bugs.

Misc

  • remove unused variables such as $correct.
  • comments such as //store the parameters are not really needed as they don't add any information.
  • same things should be named in the same way to avoid confusion. For example, registering and logging in are quite similar. But the names userLogin and register are constructed differently. Just use login and register.

Data Handling & Security

It's not a good idea to semi-randomly apply sanitation functions to user input as it results in dirty data.

stripslashes for example really only makes sense when magic quotes is enabled, so you should check that first (and then only strip the slashes in some init file, not every time you access GET). For example, if a user has a password like this: a\'b\\c\'d, stripping slashes reduces the password length from 9 to 6 characters.

Same problem with strip_tags, only much worse. If my password is a<super$secure!password, now my password is a.

You also don't need strip_tags. As XSS protection it's not sufficient anyways (and definitely not needed for passwords, which aren't echoed); You should HTML-encode data when echoing it instead.

There's also really no need to have a length restriction on the password, especially not one that is shorter than what your hashing algorithm allows.

Hashing

Simple sha is really not good enough anymore, as it's too fast. Use bcrypt instead.

Error Handling

Don't echo database error messages directly to the enduser. They won't know what to do with it, and it may provide attackers with information or enable some forms of attacks (such as error based SQL injection).

It's also not a good idea to echo anything in a function, as side-effects like that make it harder to reuse.

Structure

storeFormValues doesn't seem to have any purpose. I wouldn't call the constructor inside a function of the same class, but just use it directly.

I would also not pass a magic array to the constructor, as it's hard to use and reuse (I have to guess how the array must be build and what it must contain, and I have to use the pre-defined array keys).

Instead, just change your constructor to __construct($username, $password). This increases clarity and reduces future bugs.

Misc

  • remove unused variables such as $correct.
  • comments such as //store the parameters are not really needed as they don't add any information.
  • same things should be named in the same way to avoid confusion. For example, registering and logging in are quite similar. But the names userLogin and register are constructed differently. Just use login and register.

Data Handling & Security

You use prepared statements, which protects you from SQL injection, which is good. However, the way you handle passwords could be improved to increase security.

It's not a good idea to semi-randomly apply sanitation functions to user input as it results in dirty data.

stripslashes for example really only makes sense when magic quotes is enabled, so you should check that first (and then only strip the slashes in some init file, not every time you access GET). For example, if a user has a password like this: a\'b\\c\'d, stripping slashes reduces the password length from 9 to 6 characters.

Same problem with strip_tags, only much worse. If my password is a<super$secure!password, now my password is a.

You also don't need strip_tags. As XSS protection it's not sufficient anyways (and definitely not needed for passwords, which aren't echoed); You should HTML-encode data when echoing it instead.

There's also really no need to have a length restriction on the password, especially not one that is shorter than what your hashing algorithm allows.

Hashing

Simple sha is really not good enough anymore, as it's too fast. Use bcrypt instead.

Error Handling

Don't echo database error messages directly to the enduser. They won't know what to do with it, and it may provide attackers with information or enable some forms of attacks (such as error based SQL injection).

It's also not a good idea to echo anything in a function, as side-effects like that make it harder to reuse.

Structure

I think your structure is pretty clean, and your code easy to read. Just a couple of small points:

storeFormValues doesn't seem to have any purpose. I wouldn't call the constructor inside a function of the same class, but just use it directly.

I would also not pass a magic array to the constructor, as it's hard to use and reuse (I have to guess how the array must be build and what it must contain, and I have to use the pre-defined array keys).

Instead, just change your constructor to __construct($username, $password). This increases clarity and reduces future bugs.

Misc

  • remove unused variables such as $correct.
  • comments such as //store the parameters are not really needed as they don't add any information.
  • same things should be named in the same way to avoid confusion. For example, registering and logging in are quite similar. But the names userLogin and register are constructed differently. Just use login and register.
Source Link
tim
  • 25.1k
  • 3
  • 30
  • 76

Data Handling & Security

It's not a good idea to semi-randomly apply sanitation functions to user input as it results in dirty data.

stripslashes for example really only makes sense when magic quotes is enabled, so you should check that first (and then only strip the slashes in some init file, not every time you access GET). For example, if a user has a password like this: a\'b\\c\'d, stripping slashes reduces the password length from 9 to 6 characters.

Same problem with strip_tags, only much worse. If my password is a<super$secure!password, now my password is a.

You also don't need strip_tags. As XSS protection it's not sufficient anyways (and definitely not needed for passwords, which aren't echoed); You should HTML-encode data when echoing it instead.

There's also really no need to have a length restriction on the password, especially not one that is shorter than what your hashing algorithm allows.

Hashing

Simple sha is really not good enough anymore, as it's too fast. Use bcrypt instead.

Error Handling

Don't echo database error messages directly to the enduser. They won't know what to do with it, and it may provide attackers with information or enable some forms of attacks (such as error based SQL injection).

It's also not a good idea to echo anything in a function, as side-effects like that make it harder to reuse.

Structure

storeFormValues doesn't seem to have any purpose. I wouldn't call the constructor inside a function of the same class, but just use it directly.

I would also not pass a magic array to the constructor, as it's hard to use and reuse (I have to guess how the array must be build and what it must contain, and I have to use the pre-defined array keys).

Instead, just change your constructor to __construct($username, $password). This increases clarity and reduces future bugs.

Misc

  • remove unused variables such as $correct.
  • comments such as //store the parameters are not really needed as they don't add any information.
  • same things should be named in the same way to avoid confusion. For example, registering and logging in are quite similar. But the names userLogin and register are constructed differently. Just use login and register.