Page MenuHomeMiraheze

Request for Security Reviewer
Closed, DeclinedPublic

Description

Hello, I’m requesting to be a security reviewer on Miraheze. I am experienced in PHP, and I have experience with SQL. Thanks, @Bukkit

Event Timeline

Bukkit triaged this task as Normal priority.Jun 19 2021, 17:32
Bukkit created this task.

Please complete a security review of an outstanding extension / skin request. Be as detailed as possible in your review and comment on anything that could pose a security risk and why it is / is not. (Use a private task for security issues found if any).

Another reviewer will see if they agree and come back with any feedback. We'll probably ask you a knowledge quiz too.

Please complete a security review of an outstanding extension / skin request. Be as detailed as possible in your review and comment on anything that could pose a security risk and why it is / is not. (Use a private task for security issues found if any).

Another reviewer will see if they agree and come back with any feedback. We'll probably ask you a knowledge quiz too.

Got it.

Please complete a security review of an outstanding extension / skin request. Be as detailed as possible in your review and comment on anything that could pose a security risk and why it is / is not. (Use a private task for security issues found if any).

Another reviewer will see if they agree and come back with any feedback. We'll probably ask you a knowledge quiz too.

I reviewed T7490, everything there seems safe, and doesn’t seem to be dangerous or pose a security risk.

Be as detailed as possible in your review and comment on anything that could pose a security risk and why it is / is not.

There is no detail in your review. You need to demonstrate you understand why something doesn't pose a risk of XSS/SQLi/ACE/<insert other issue>.

I'm wanting to hear comments like "Line 9 of SpecialTest.php correctly escapes the users input so is XSS safe"

Be as detailed as possible in your review and comment on anything that could pose a security risk and why it is / is not.

There is no detail in your review. You need to demonstrate you understand why something doesn't pose a risk of XSS/SQLi/ACE/<insert other issue>.

I'm wanting to hear comments like "Line 9 of SpecialTest.php correctly escapes the users input so is XSS safe"

Ah, my bad. The screen.css in the skin is full with formatting, and using a font family, and after reading the css, that file is safe. In the i18n, which seems to be the language files has brief .json, and those files are safe. In Skin.json, it has even more formatting, and the header things, so after carefully reviewing all of the files, the skin seems safe.

Ah, my bad. The screen.css in the skin is full with formatting, and using a font family, and after reading the css, that file is safe. In the i18n, which seems to be the language files has brief .json, and those files are safe. In Skin.json, it has even more formatting, and the header things, so after carefully reviewing all of the files, the skin seems safe.

Just out of curiosity (I know I don't have a say here), did you review the Mustache and Vue.JS templates?

Unknown Object (User) removed a project: Security.Jun 19 2021, 19:33
Unknown Object (User) added a comment.EditedJun 19 2021, 19:47

I appreciate the wanting to help but I unfortunately can't support this.

Ah, my bad. The screen.css in the skin is full with formatting, and using a font family, and after reading the css, that file is safe. In the i18n, which seems to be the language files has brief .json, and those files are safe. In Skin.json, it has even more formatting, and the header things, so after carefully reviewing all of the files, the skin seems safe.

Does not demonstrate any knowledge of security or things to look for when reviewing security, security reviews have nothing to do with json formatting. That response just makes it sound like you are trying to sound technical while from my perspective it doesn't as it doesn't have much to do with reviews at all. It doesn't say anything good or bad from a security perspective.

My advice to you is to read through some of the guides here and come back when you have gained more knowledge about security reviewing. Thank you!

In T7491#150211, @R4356th wrote:

Ah, my bad. The screen.css in the skin is full with formatting, and using a font family, and after reading the css, that file is safe. In the i18n, which seems to be the language files has brief .json, and those files are safe. In Skin.json, it has even more formatting, and the header things, so after carefully reviewing all of the files, the skin seems safe.

Just out of curiosity (I know I don't have a say here), did you review the Mustache and Vue.JS templates?

Forgot to add that. My bad, but yes, I did see that.

I appreciate the wanting to help but I unfortunately can't support this.

Ah, my bad. The screen.css in the skin is full with formatting, and using a font family, and after reading the css, that file is safe. In the i18n, which seems to be the language files has brief .json, and those files are safe. In Skin.json, it has even more formatting, and the header things, so after carefully reviewing all of the files, the skin seems safe.

Does not demonstrate any knowledge of security or things to look for when reviewing security, security reviews have nothing to do with json formatting. That response just makes it sound like you are trying to sound technical while from my perspective it doesn't as it doesn't have much to do with reviews at all. It doesn't say anything good or bad from a security perspective.

My advice to you is to read through some of the guides here and come back when you have gained more knowledge about security reviewing. Thank you!

I was showing how nothing there seemed harmful or unsecure

Unknown Object (User) added a subscriber: Southparkfan.Jun 19 2021, 19:57

I read the guide, so now I think there are more things I want to point out about why I believe it is secure.

  1. There were no SQL injections (SQLi) which is a +1, since those are deemed insecure.
  2. The CSS seems sanitized, so another +1, for user privacy
  3. It doesn’t use any insecure functions so another +1.

You haven't explained how you came to them conclusions.

You haven't explained how you came to them conclusions.

There were no SQL injections (SQLi) which is a +1, since those are deemed insecure.

As the guide states, this is what an SQL injection looks like.

$limit = $wgRequest->getVal( 'limit' );
$res = $db->query( "SELECT * from kitties LIMIT $limit" );

But none of that in ANY of the code

The CSS seems sanitized, so another +1, for user privacy

I do not see any outside links.

Bukkit claimed this task.

I think I should learn more about security reviewing and then apply later.

Bukkit removed Bukkit as the assignee of this task.Jun 20 2021, 13:08

When would a good time to try again be?

When would a good time to try again be?

In at least a few months