halfer / awooga-app

Web application to report coding tutorials of non-optimal quality
0 stars 0 forks source link

"Issue codes may not be duplicated in a report" error when trying to submit a new report #14

Closed PeeHaa closed 9 years ago

PeeHaa commented 9 years ago

I just tried to add a new report, but it doesn't let me submit. I get the following error when trying to submit:

Issue codes may not be duplicated in a report

I have tried two browsers (Chrome canary + FF whatever is latest. probably firefox 138) both with the same result.

The entire report I tried to submit:

URL(s):

http://www.wikihow.com/Create-a-Secure-Login-Script-in-PHP-and-MySQL

Title:

How to Create a Secure Login Script in PHP and MySQL

Description:

There are several strange / awkward things going on in this tutorial, which some scary practices (considering the title is secure login script).

The buggest issue is the incorrect hashing of passwords.

Issue(s) (first line is type, issues are delimited by ----------):

password-inadequate-hashing

The tutorial hashes passwords using sha512 with a single round.

----------

uncategorised

Not a security issue, but rather really awkward. OP gets a userid from the db which is a integer field, but does some crazy regex check to "prevent XSS":

 $user_id = preg_replace("/[^0-9]+/", "", $user_id);

----------

uncategorised

Trying to prevent XSS when it is not yet needed (premature prevention) and trying to prevent XSS the wrong way. It uses some regex instead of using the tool for the job htmlspecialchars:

$username = preg_replace("/[^a-zA-Z0-9_\-]+/", 
                                                            "", 
                                                            $username)

----------

password-inadequate-hashing

The tutorial could really use a better source of randomness for generating the salt (or actually use bcrypt instead):

$random_salt = hash('sha512', uniqid(mt_rand(1, mt_getrandmax()), true))

There is a comment above this code trying to use openssl_random_pseudo_bytes which "doesn't work":

//$random_salt = hash('sha512', uniqid(openssl_random_pseudo_bytes(16), TRUE)); // Did not work

----------

uncategorised

The tutorial uses a Javascript library to hash passwords before sending it over the connection. The author indeed speaks about the fact that you should always use use an encrypted connection (which is the correct way). But when the author fails recognize is that when using a proper secure connection there is no need to use a javascript library to hash the password kaing the entire thing basically useless.

halfer commented 9 years ago

Aha! I hadn't understood earlier that there was more than one instance of uncategorised. This restriction is to prevent two instances of the same problem (e.g. sql-injection) appearing in a report.

One solution is to make uncategorised a null issue type, so it is the only one that can be entered several times. Does that sound like it would work, or would there be a reason to dup any issue type?

PeeHaa commented 9 years ago

Ah I see. So I should actually group all issue based on category?

halfer commented 9 years ago

Well, the way I see it is there is no need for a report to say a resource contains, for example, sql-injection more than once. However I can change it so uncategorised is permitted many times - of course there could be several of these, as your example shows. How's that?

PeeHaa commented 9 years ago

Works for me

halfer commented 9 years ago

I've made that change, seems to be happy my side. Would you give your report another go?

PeeHaa commented 9 years ago

Yes I can confirm that this fixes the issue I had http://awooga.jondh.me.uk/report/41.

Thanks!

halfer commented 9 years ago

Excellent, good report!