ImageCache Auto

This is a really cool module, but it seems there might be a little risk. You allow an attack vector whereby someone can create N=(maxwidth*maxheight) number of images on your server pretty easily. It wouldn't take that long to fill up your entire server. How can this be mitigated?

This module needs to take steps to stop abuse as described by jrowny

Hi!

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/imagecache_auto.module:
   +2: [minor] Commits to the Git repository do not require the CVS $Id$ keyword in each file.
   +27: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +55: [normal] Use "elseif" in place of "else if"
   +63: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +64: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
  
  Status Messages:
   Coder found 1 projects, 1 files, 4 normal warnings, 1 minor warnings, 0 warnings were flagged to be ignored
  

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ...ites/all/modules/pareview_temp/test_candidate/imagecache_auto.admin.inc
  --------------------------------------------------------------------------------
  FOUND 4 ERROR(S) AFFECTING 4 LINE(S)
  --------------------------------------------------------------------------------
    2 | ERROR | You must use "/**" style comments for a file comment
   22 | ERROR | Whitespace found at end of line
   31 | ERROR | Whitespace found at end of line
   40 | ERROR | Whitespace found at end of line
  --------------------------------------------------------------------------------
  
  
  FILE: ...p709/sites/all/modules/pareview_temp/test_candidate/imagecache_auto.inc
  --------------------------------------------------------------------------------
  FOUND 6 ERROR(S) AFFECTING 4 LINE(S)
  --------------------------------------------------------------------------------
    2 | ERROR | You must use "/**" style comments for a file comment
   33 | ERROR | Concat operator must be surrounded by spaces
   33 | ERROR | Concat operator must be surrounded by spaces
   82 | ERROR | Concat operator must be surrounded by spaces
   82 | ERROR | Concat operator must be surrounded by spaces
   83 | ERROR | Whitespace found at end of line
  --------------------------------------------------------------------------------
  
  
  FILE: ...9/sites/all/modules/pareview_temp/test_candidate/imagecache_auto.module
  --------------------------------------------------------------------------------
  FOUND 11 ERROR(S) AND 1 WARNING(S) AFFECTING 8 LINE(S)
  --------------------------------------------------------------------------------
    2 | ERROR   | You must use "/**" style comments for a file comment
   12 | WARNING | Format should be * Implements hook_foo().
   27 | ERROR   | Concat operator must be surrounded by spaces
   40 | ERROR   | "include_once" is a statement not a function; no parentheses
      |         | are required
   42 | ERROR   | Expected 1 space after comma in function call; 2 found
   55 | ERROR   | Use "elseif" in place of "else if"
   63 | ERROR   | Concat operator must be surrounded by spaces
   63 | ERROR   | Concat operator must be surrounded by spaces
   64 | ERROR   | Concat operator must be surrounded by spaces
   64 | ERROR   | Concat operator must be surrounded by spaces
   64 | ERROR   | Concat operator must be surrounded by spaces
   64 | ERROR   | Concat operator must be surrounded by spaces
  --------------------------------------------------------------------------------
  

• Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• Remove all old CVS $Id tags, they are not needed anymore.
  

  imagecache_auto.admin.inc:2:// $Id$
  imagecache_auto.inc:2:// $Id$
  imagecache_auto.info:1:; $Id$
  imagecache_auto.module:2:// $Id$
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./imagecache_auto.admin.inc ./README.txt ./imagecache_auto.info ./LICENSE.txt ./imagecache_auto.inc ./imagecache_auto.module
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Addition:
You should extend your description with the Drupal branch your project applies to.

@silvio has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

@silvio, if you want a review you must change status to 'needs review'

The security issue has been reduced to a finite and calculable degree, which I believe is acceptable, as it causes no more harm than a site that has a large amount of pre-defined presets. However, you may wish to add an option to disable public access, so that sites that wish to only use it internally do not have to account for this. Another option would be to create a permission for URL access.


Remaining Issues:

Whitespace and comment issues. Please see the attached output from PAReview.sh.

Use the t() function. All text that is displayed to users should pass through it, including the items below.

imagecache_auto.module lines 16 and 17

  'title' => 'ImageCache Auto',
  'description' => 'ImageCache Auto settings.',

imagecache_auto.inc line 43

  $message = 'Maximum number of imagecache presets reached. Please consider increasing the max number of presets.';

After these issues are resolved, I believe it should be ready for RTBC. Also, congrats on an awesome idea!

Forgot to change status.

Forgot to add status. New comment will fix it..

Hello
You still have some minor coding style issues.
You can check these http://ventral.org/pareview/httpgitdrupalorgsandboxsilvio1247766git
And i would also recommend you to read about [META] Review bonus at http://drupal.org/node/1410826

regards

Have you checked to ensure that it is using Unix line endings?

I did a manual code review on this module and found a few more items that I think need to be fixed before it gets released. Some of these may seem nit-picky, but this module is different from many others: it has important security implications for a site, and we know that many Drupal administrators "trust" modules without really understanding all of their characteristics. Please bear with these additional comments - I believe they're important and worth addressing.

1. The security additions are helpful, but the default state for each is way too high for a "default" installation. _max_width/height at 10,000 and _max_presets at 250 means a pretty large upper bound on files that may be created.

You are not allowing upscaling, which is good, but consider what might be a typical use case: a site admin is uploading digital camera photos. I ran a sample 8 megapixel (which is small in these days of cheap T1i's and better) file through the filter starting from the original size of 2446x3038. With the default settings I was able to create nearly a gigabyte of resized files with a simple script against a single source file - you can imagine how a gallery of even a few photos would affect a "typical" Web hosting account with limited space. I'd strongly recommend:

a. Reducing these default limits,
b. Mentioning them in README.txt and discussing these security implications there,
c. Discussing the security implications in #description elements in the admin form as well (specifically, that width/height are just as important as max preset count, and also that your module uses the total number of presets, not the number of automatic presets, as its measurement/limit).

2. You are directly updating {imagecache_action}. Modules can get away with this when they are updating their own tables, but they're not supposed to write to those owned by others for a variety of reasons (tightly coupling to another module's schema, not working properly if memcache is installed, etc). In this case it's not even necessary: imagecache exposes imagecache_action_load()/_save()/_delete() functions that would do this for you. I'm confused by this code because you DO call imagecache_preset_* CRUD functions...? Did you have a problem with the functions for managing _action_'s?

3. In imagecache_auto_count_presets() it would be better to call imagecache_presets() and just sizeof/count the results. There are two reasons: 1. This lets you leverage imagecache's own caching of its settings so sites with memcache can take advantage of that automatically, and 2. Module authors can expose default presets in code and alter this information through hooks. If you directly query the database you can't detect these.

You're only using this count for your limit check, but that's actually more confusing for two reasons:

a. This fact isn't documented in the function comment block, and the function isn't prefixed with _ so it's not "private". Somebody else might think this means they can call this function for their own security/behavior checks and not get the behavior it appears to provide.
b. You include all system presets in the limit check. At 250 nobody will notice. But as soon as this gets set to 10 this is going to get confusing. If the administrator wants a realistic limit, every time they add a manual preset of their own, they'll have to increase the limit to allow for it if they don't want the automatic side disrupted. This is not documented anywhere.

4. JUST A SUGGESTION: The naming convention here is a little tricky because when I went to clean up all the presets I created they were hard to distinguish from the WxH presets I had made myself. This comment should not block future code review, but it would be very helpful if you provided some sort of control over naming convention. Even just a prefix would be great (auto_WxH or similar). Even if you don't feel like adding the functionality to the form, hard-coding it would still be useful and only a 1-line change it looks like.

This looks like a very useful module and I can already envision a site where it could be used. I look forward to its release!

Closing due to lack of activity. Feel free to reopen if you are still working on this application.