Closed (fixed)
Project:
Module Grants
Version:
6.x-3.5
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
26 Mar 2010 at 19:44 UTC
Updated:
21 Apr 2010 at 01:20 UTC
There is a lot of complicated logic in this update that does wildcard matching and regex replaces on strings that are just static strings. Also, while everything appears to be properly escaped /e use should be avoided. The security team has recently posted a handbook page and coder will likely be complaining about this soon as well(#752734: Add a security check for /e flag in preg_replace()). I've gone ahead and made a patch and it worked very smoothly for my update. It uses a lot less magic and should be less likely to match weird strings or try updating permissions that don't belong to it.
| Comment | File | Size | Author |
|---|---|---|---|
| cleanup_module_grants_update_6207.patch | 2.11 KB | neclimdul |
Comments
Comment #1
rdeboerFair enough. Thanks for the patch. Will have a closer look at it soon and check in.
Comment #2
rdeboerAppreciate the tip on regexps with /e and thanks again for the patch.
As far as the fix goes, I feel it comes down to personal preferences.
regexp code tends to be terse and more difficult to read for the less experienced, I agree, but it requires fewer lines of code without a double-nested loop and the declaration of longish arrays.
I went for a fix using
preg_replace_callback(instead ofpreg_replace) following the advice in http://drupal.org/node/750148Comment #3
rdeboerChecked into CVS HEAD. Will be available in 6.x-3.6 and later.
Comment #4
neclimdulThanks for taking my security suggestion!
I just want to be clear though on why I chose that approach and argue for my patch a little. I don't believe regex is the right fix for the problem not because of readability(or grok-ability despite my titling this issue "overly complicated") but because its doesn't really fit. You want to replace a set list of strings but your regex is very broad. As I noted in my original post "should be less likely to match weird strings or try updating permissions that don't belong to it." In my opinion update hooks should be
So the regex part of this sort of falls under the old "now you have two problems" regex banner because it doesn't fit either of those very well in this case.
One more argument for my patch over the committed alternative, its significantly faster. The original took about 15 seconds to run on my setup where as my patch was instant. I haven't tested the new version but create_function isn't fast so I'm thinking its not going to be a speed demon either.
Anyways, for posterity that's the argument for my patch.
Comment #5
rdeboerPoint taken and I may still change it to your suggestion.
Although the code runs as a one-off, I am worried that it would take 15 seconds.... it does so little.... Unless you have millions of roles on your system....
Comment #6
neclimdulThat wouldn't make sense actually. We do have several roles with a large number of permissions which means there is a fairly large string for the regex to run on. I was probably more the eval() on the large string though.