Handle textarea inline links to managed files in a simple, yet efficient manner.

Allows you to move managed files around without breaking links from inline content, e.g. from entity textareas and custom blocks.

Project Page: Permanent Filelink

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/vingborg/2098361.git permanent_filelink

Manual reviews of other projects

https://www.drupal.org/node/2293613#comment-9676437
https://www.drupal.org/node/2441453#comment-9673087
https://www.drupal.org/node/2379411#comment-9674915

Automated Review
http://pareview.sh/pareview/httpgitdrupalorgsandboxvingborg2098361git

Comments

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/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.

vingborg’s picture

Issue summary: View changes
vingborg’s picture

Issue summary: View changes
vingborg’s picture

Issue summary: View changes
Xen’s picture

Status: Needs work » Reviewed & tested by the community

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.

vingborg’s picture

Issue summary: View changes
Issue tags: +PAReview: review bonus
klausi’s picture

Assigned: Unassigned » naveenvalecha
Status: Reviewed & tested by the community » Needs work
Issue tags: -PAReview: review bonus +PAReview: security

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.

DamienMcKenna’s picture

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

naveenvalecha’s picture

Assigned: naveenvalecha » Unassigned

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’s picture

Thanks a lot for this round of feedback!

I believe that I have handled most -- if not all -- issues raised here.

@klausi:
Permanent Filelink was made in direct response to Pathologic shortcomings, but I didn't know about Portable Path. My module does much the same, but goes a few steps further with a few more features. Said features (live links while wysiwig editing) was an essential requirement. A patch to Portable Path would essentially be a complete rewrite. Also, my module is, IMHO, a simpler and better implementation ;-)

I will do a bunch of reviews for review bonus, but not for another day or two.

@naveenvalecha:
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.

About #6 on your list: I don't quite understand?

It is quite possible to allow for private files, and I have implemented a possible solution.

vingborg’s picture

Status: Needs work » Needs review
naveenvalecha’s picture

Assigned: Unassigned » naveenvalecha

@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.

naveenvalecha’s picture

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

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.

vingborg’s picture

Status: Needs work » Needs review

@naveenvalecha:

I believe I have catered for all outstanding issues.

As far as I can tell, XSS has been dealt with as comprehensibly as possible.

The entity_type limitation was irrelevant, so I have removed it. Also I've have added support for display suite code fields. Others will follow upon request.

What is the next step? An updated project page and README file?

naveenvalecha’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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.

vingborg’s picture

@naveenvalecha:

All is done :-)

I was using my work e-mail for the git commits. I have now added it to my Drupal account, but the older commits are still flagged with my proper name only. Is there any way to update this retroactively? I can do it on a local repository (and on my GitHub repositories), but I don't seem to have the rights on git.drupal.org.

I will review a bunch of project applications in the coming days ... taking a few cues from your example ;-)

naveenvalecha’s picture

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

naveenvalecha’s picture

Assigned: Unassigned » er.pushpinderrana

Assigning to Pushpinder to give it final look.

naveenvalecha’s picture

Assigned: er.pushpinderrana » klausi
Status: Reviewed & tested by the community » Needs work

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

  3. 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.

klausi’s picture

Status: Needs work » Needs review

@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?

naveenvalecha’s picture

Assigned: klausi » naveenvalecha

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

naveenvalecha’s picture

Assigned: naveenvalecha » Unassigned
Status: Needs review » Fixed

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:

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.

vingborg’s picture

@naveenvalecha @klausi

Thanks a lot! Especially since it happened on my 7 year "birthday" on drupal.org, almost to the day. The process has been enlightning and not at all taxing. The module has become better for it, plain and simple. All projects should go through such a vetting process.

I will give it a final workover, and then I will promote it to a full project.

Also, I *will* make good on my pledge to review a bunch of pending projects in the coming time. Whether I will join the group of reviewers is up in the air, but I have followed the group's work for some time, and I am very interested in the initiatives that have made recently, concerning a major revision of the overall process. Such a revision is long overdue. Hat tip to @klausi for that!

About involvement: I am by no means a beginner in this department. My modest claim to "fame" is a certain rewrite of a certain command line tool, quite a few years ago. See this https://www.drupal.org/node/335360 ... if you catch my meaning ;-)

Again: Thanks a lot, folks. It's a great feeling.

Status: Fixed » Closed (fixed)

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