MimeDetect module is not a new module, I'm the maintainer, but I cannot opt into security advisory coverage. It was at beta for a long time and I released the first stable version.

I relaunch the application process in order to get the necessary permissions, following indications at the project edit form:

You must be given access to opt into security advisory coverage. Learn how to apply

GIT clone

git clone --branch 7.x-1.x https://git.drupal.org/project/mimedetect.git

Manual reviews of other projects

Comments

manuel.adan created an issue. See original summary.

PA robot’s picture

Status: Needs review » Needs work

Git clone command for the sandbox is missing in the issue summary, please add it.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

manuel.adan’s picture

Issue summary: View changes
Status: Needs work » Needs review
PA robot’s picture

Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpsgitdrupalorgprojectmimedetectgit

I'm a robot and this is an automated message from Project Applications Scraper.

manuel.adan’s picture

Issue summary: View changes
Status: Needs work » Needs review
manuel.adan’s picture

Issue tags: +PAreview: review bonus
khurrami’s picture

Hi,

function mimedetect_settings() {
// Mime detection engines.
$form['engines'] = array(
'#type' => 'fieldset',
'#title' => t('MIME detection engines'),
);
in above function (mimedetect.admin.inc) please compare with https://www.drupal.org/docs/develop/standards/coding-standards#array

Thanks

manuel.adan’s picture

Status: Needs review » Reviewed & tested by the community

Thank's Khurram,

Fixed with white space stripping to accomplish coding standards.

klausi’s picture

Status: Reviewed & tested by the community » Needs review

@manuel.adan: please don't RTBC your own issues, see the workflow: https://www.drupal.org/node/532400

manuel.adan’s picture

Hi Klaus,

I'm sorry, that's my fault. I've just don't remember the exact workflow, my last PAR was several years ago.

A custom application to manage the PAR rather than a generic issue queue might be useful to prevent this. Are there any proposal for that?

Thank's

manuel.adan’s picture

Priority: Normal » Major
manuel.adan’s picture

Priority: Major » Critical
tatarbj’s picture

Assigned: Unassigned » tatarbj

I'm reviewing the module, results will be posted soon.

tatarbj’s picture

Assigned: tatarbj » Unassigned
Status: Needs review » Needs work

Hi @manuel.adan,

i've reviewed your contrib and i have to admit it's a really nice work and has some excellent practice that is followed, i'm glad to be the reviewer of it :)

Here are some things that are totally not blocker issues in my eyes, but could be nice to implement them before going forward:
* I would implement a dedicated permission instead of usage of administer site configuration (at least in our projects we often give it to editors in order to increase the flexbility of their work as webmasters).
* In order to use the $additional_mimes in a more drupal way, implement hook_file_mimetype_mapping_alter in the module and define your extentions instead of using a static variable in mimedetect_mime() that is like an entry point to other modules (like the mimedetect_fileupload).
Then in its last if condition where code uses file_get_mimetype() you don't need to call it twice and use any tricks, it will detect your hook implementation and will work :)
* I would suggest to use #disabled instead of #attributes['disabled] in the mimedetect_settings() as #attributes is meant to be used for classes not to disable the access to a form field (I'm also not fully sure it's crossbrowser enough to ensure it will work everywhere as you expect). (mimedetect.admin.inc:31) The same with mimedetect_enable_file_extension form element.
* Because mimedetect_file_binary form element is a required one, the form won't be able to be submitted if mimedetect_enable_file_binary is not checked (as it's the default value of it) - so i would remove this line (mimedetect.admin.inc:47) and handle it if it's really important to be mandatory in an #element_validate.
* Regarding the usage of exec in the settings form's validate, i'm still thinking about how it can be proofed it's unsafe, also because drupal7 minimum requirement is 5.2.5+ (https://www.drupal.org/docs/7/system-requirements/php#php_required) i would suggest to brainstorm about the safe mode (even it got deprecated on 5.3 then removed on 5.4) - but don't know how execution could be totally safe, especially because here you have a textfield that will get a path that would be executed (pretty complex, it seems, to cause damage because attacker need to put his file somehow to the server than from this interface could execute it). (The same happens in the mimedetect_requirements too).

Drupal Coding standards are followed and code seems well documented.

If you wish I even can contribute on the listed things under the issue queue of the module, but for now i would say it needs some work (love?), so i'm changing the status of this issue.

Bests,
Balazs.

manuel.adan’s picture

Status: Needs work » Needs review

Hi Balazs !

Thank you so much for your review of the mimedetect module. It took a while but it was worth the wait ;)

A new 7.x-dev version has been released with changes from your suggestions as follows:

* dedicated permission: I'm with you in implementing a dedicated permission is a good practice in general, but this module will usually be installed and configured only once so I prefer not to add useless permission to the (generally large) permission table

* additional mimes: the "$additional_mimes" is a reminiscence from 6.x version and has been completely removed in favor of the usage of 'File MIME' recommended module. See commit: http://cgit.drupalcode.org/mimedetect/commit/?h=7.x-1.x&id=19ee5c4baee3e...

* disabling form fields: following your suggestion, I've updated how form fields are disabled. See commit: http://cgit.drupalcode.org/mimedetect/commit/?h=7.x-1.x&id=02eac80880fb6...

* mimedetect_file_binary settings form field: no longer required, form validation function already checks the value entered by user if this engine is enabled. See commit: http://cgit.drupalcode.org/mimedetect/commit/?h=7.x-1.x&id=4af7559bd3f43...

* 'exec' security concerns: I've added an additional validation for the file executable path that requires its basename must to be 'file'. This verification is done in the settings form validation as well as in the requirements and even in the MIME detection itself. It prevents from some scenarios, like lauching other system commands or some uploaded binary file with a non-suspicius name (like execme.jpg). See commit: http://cgit.drupalcode.org/mimedetect/commit/?h=7.x-1.x&id=418facf5e0700...

Some of these changes will be ported to the 8.x version.

It was nice to work together! ;)

jayesh_makwana’s picture

Hello manuel.adan,

I checked your module, however there are some coding issues and recommendations that you may be interested in :

FILE: ...t/repos/pareviewsh/pareview_temp/src/Form/MimeDetectSettingsForm.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------
74 | WARNING | t() calls should be avoided in classes, use dependency
| | injection and $this->t() instead
80 | WARNING | t() calls should be avoided in classes, use dependency
| | injection and $this->t() instead
--------------------------------------------------------------------------

FILE: /root/repos/pareviewsh/pareview_temp/src/MimeDetectService.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
--------------------------------------------------------------------------
31 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
55 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
74 | WARNING | \Drupal calls should be avoided in classes, use
| | dependency injection instead
--------------------------------------------------------------------------

FILE: ...h/pareview_temp/mimedetect_fileupload/mimedetect_fileupload.info.yml
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------
7 | WARNING | All dependencies must be prefixed with the project name,
| | for example "drupal:"
8 | WARNING | All dependencies must be prefixed with the project name,
| | for example "drupal:"
--------------------------------------------------------------------------

FILE: /root/repos/pareviewsh/pareview_temp/mimedetect.install
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
16 | ERROR | [x] Short array syntax must be used to define arrays
40 | ERROR | [x] Expected newline after closing brace
57 | ERROR | [x] Short array syntax must be used to define arrays
----------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: /root/repos/pareviewsh/pareview_temp/README.txt
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
120 | ERROR | [x] Expected 1 newline at end of file; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: ...t/repos/pareviewsh/pareview_temp/src/Form/MimeDetectSettingsForm.php
--------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
45 | ERROR | [x] Short array syntax must be used to define arrays
69 | ERROR | [x] Short array syntax must be used to define arrays
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: /root/repos/pareviewsh/pareview_temp/src/MimeDetectService.php
--------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
186 | ERROR | [x] Inline comments must end in full-stops, exclamation
| | marks, colons, question marks, or closing parentheses
188 | ERROR | [x] Expected newline after closing brace
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

Thank You.

tatarbj’s picture

Assigned: Unassigned » tatarbj

Hi @jayesh_makwana,
as the application is for the D7 version of this module, i would suggest to not post its 8.x phpcs problems under this issue in order to keep it clearly delegated to its original scope.
@manuel.adan, i will check today a bit later your fixes and get back to you, but for sure thanks for the detailed reply!
Bests,
Balazs.

tatarbj’s picture

Assigned: tatarbj » Unassigned
Status: Needs review » Reviewed & tested by the community

Hi @manuel.adan,
i've reviewed the module with your changes and even they were not pointed as blocker, i'm very happy to get them in before the new release.
I've also carefully tested the module again and it seems it deserves to get the approval by Security Team, so i'm changing the status to RTBC!
Great work and collaboration!!
Bests,
Balazs.

manuel.adan’s picture

Hi Jayesh & Balazs,

Thank's for your review of the D8 version (still in beta) of mimedetect, I'll take it into account for the next revision; created issue #2891790: Follow coding standards for that. As Balazs notes, this PAR is for the D7 version with a stable release available.

It is always exciting to contribute to the Drupal community ;)

apaderno’s picture

Assigned: Unassigned » apaderno
Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

apaderno’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

apaderno’s picture

Status: Closed (fixed) » Fixed

I am giving credits to the users who participated in this issue.

kiamlaluno credited zzolo.

apaderno’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.