[D7] Permanent Filelink

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

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.

Licensing:
All original code. No stray license texts.

Duplication:
I tried searching for similar modules, but it didn't turn up anything.

Drupal API understanding:
Excellent.

Security:
Nothing untowards spotted. Seems thought through.

Coding style:
Coder gives no warnings, apart from a few longish lines, but not more than usual.

Code comments:
Outstanding, per contrib standards. I wish all contrib modules was this well commented.

manual review:

1. project page: could you add the differences to https://www.drupal.org/project/pathologic and https://www.drupal.org/project/portable_path to the project page?
2. permafilelink_enable(): do not juggle with module weights as that is not reliable. Use hook_module_implements_alter() instead if you need to run before/after a specific hook.
3. README.txt is incomplete, see https://www.drupal.org/node/2181737
4. This module has a security vulnerability and I'm assigning this to Naveen as part of our git administrator training program. If he does not find it I will post details in one week. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
5. "db_query('SELECT * FROM file_managed WHERE fid=?', array($fid)": we use ":" placeholders in Drupal, see https://www.drupal.org/node/310072

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

I gave this a quick review and also saw a security issue, so klausi isn't making this up ;-)

Manual Review(Read d994aefa30f3a37aaba...)

1. Project page is short.Add more information on the project page.https://www.drupal.org/node/997024
2. Move the Administration callback in seperate file modulename.admin.inc.It will give some performance hit as well.
3. hook_help() is missing in this module.It would be nice to add it.

4.   // Hook up the PFL route. TODO: access issues?
     $items[PERMAFILELINK_ROUTE . '/%'] = array(
       'page callback' => '_permafilelink_pass_file',
       'page arguments' => array(1),
       'access callback' => TRUE,
       'type' => MENU_CALLBACK,
     );

   The permission is loosely coupled.I would suggest to use extra permission 'access content' permission OR provide extra permission by module because currently it is access bypass the files.So you need to do the file validation as well.
   _permafilelink_pass_file : In the else case you need to return drupal_access_denied.

   if ($fres = db_query('SELECT * FROM file_managed WHERE fid=?', array($fid))) {
       if ($fres->rowCount() == 1) {
         $file = $fres->fetchObject();
         $headers = array(
           'Content-Type' => $file->filemime,
         );
         file_transfer($file->uri, $headers);
       }
     }

   Instead of directly getting the file details using query I would suggest you to use the file_load function.Something like below.

    $fid  = intval($fid);
     $file = file_load($fid);
   
     if (empty($fid) || empty($file) || !$file->status) {
       return drupal_access_denied();
     }
   ...........
   
     if (in_array(-1, $headers)) {
       return drupal_access_denied();
     }
   

   I think you should forbid private files completely for now or use file_download_access()

5. Remove the unused variables(permafilelink_verbose_reporting) from module using hook_uninstall
6. permafilelink_form_alter : Please add on the project page that module currently supports only these entities$entity_types = array('taxonomy_term', 'node', 'user');
7. _permafilelink_fid_by_path : $uri = 'public://' . urldecode($path); Please add a comment regarding the private files.
8. (+)The _permafilelink_filter_settings is exploited from XSS.Please use the appropirate sanitization functions https://www.drupal.org/writing-secure-code http://drupal.stackexchange.com/questions/10280/is-check-plain-enough https://api.drupal.org/api/drupal/includes!common.inc/group/sanitization/7
   

     $elements['short_base'] = array(
       '#type' => 'textfield',
       '#title' => t('Shortening replacement pattern'),
       '#description' => t("Links of the form '!fulldummy.doc' will become '!shortdummy.doc'.", array('!full' => $full['path'], '!short' => $filter->settings['short_base'])),
       '#default_value' => $filter->settings['short_base'],
     );

   The description has short_base user provided text.It can leads to XSS so to fix this use @ it will do free validation for you.
   Correct Usage :
   '#description' => t("Links of the form '!fulldummy.doc' will become '@short dummy.doc'.", array('!full' => $full['path'], '@short' => $filter->settings['short_base'])),

@vingborg,
Thanks for the updation.
Updated the point 6.It looks it got deleted.

I have also taken your suggestions and worked them in where appropriate. You are spot on about using file_load and file_download_access, I have no idea why I didn't think of that myself.

Nop that's why we usually do 2 reviews of same project.
Will review the code again that may be tonight.

you are near RTBC :)
Still only needs to address #9 point 8 because it has not been addressed fine as specified above.
Rest points has been considered.

Read aaa8dc1...
Readme.txt is fine. Add configuration section on the project page as well.
Fix these coder reviews warnings http://pareview.sh/pareview/httpgitdrupalorgsandboxvingborg2098361git
Your commits https://www.drupal.org/node/2098361/commits The Git commits are not connected to your user account. You need to specify an email address.
See https://www.drupal.org/node/1022156 andhttps://www.drupal.org/node/1051722.

1. Add information about module using hook_help.
2. Use drupal_parse_url instead of parse_url in _permafilelink_uri_parts

Rest looks good to me.RTBC :) Get Review bonus by doing another 3 reviews of other projects to come in admin radar.

Regarding git commits feel free to file the issue in webmasters issue queue regarding the same https://www.drupal.org/project/issues/webmasters?categories=All

Assigning to Pushpinder to give it final look.

Read 189c151...

1. Use the hook_help to define the help regarding the module.Using with another case case 'admin/help#permafilelink':
2. 
   

     $items[PERMAFILELINK_ROUTE . '/%'] = array(
       'page callback' => '_permafilelink_pass_file',
       'page arguments' => array(1),
       'access arguments' => array('access content'),
       'type' => MENU_CALLBACK,
     );

   This menu callback also seems exploit from Access by pass.So a hacker can increase the load on the server just by writing some script to download the files. and also by adding some js code snippet at somewhere in other pages to download the file automatically.You need some confirmation token in the path as well to download the files.See how to prevent this

   1. http://epiqo.com/en/all-your-pants-are-danger-csrf-explained
   2. https://docs.acquia.com/articles/using-filter-functions-intended-filterx...

Addressed by klausi in #20

Please do 3 more reviews to get another review bonus to come up in high priority list.
@klausi,
Need your eyes on the private file system access implmenetation b/c I'm not able to exploit it anymore.

@naveenvalecha: I don't see an access bypass here, _permafilelink_pass_file() applies access checks? I also don't see a CSRF vulnerability here since _permafilelink_pass_file() does not perform a write operation?

Thanks @klausi for clarification. Rest seemed good to me here. I'll take a final look at it tonight.

Manual Review : Read 82a55b8202....

1. #19.1
2. Remove the unused comment // Hook up the PFL route. TODO: access issues?

No objections for more than 1 week.Thanks for your contribution @vingborg!,

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.