Bug in regex used in _filefield_paths_replace_path can cause incorrect escaping of HTML code [#1949508]

I've discovered a bug in the regex used in _filefield_paths_replace_path which in certain instances can cause incorrect escaping and serious corruption of HTML code when multiple objects in said HTML code will have their path updated (for example, a snippet of HTML that has two images that will have their path updated).

Here is the code that generates the regex:

$regex = str_replace('/', '\/', "({$absolute}|{$relative}|{$info['scheme']}://)(styles/.*?/{$info['scheme']}/|)({$info['host']}{$info['path']})");

The problem is: /.*?/

The above will match anything EXCEPT a newline. So if there is not a newline between the first and second objects to be manipulated in the HTML code being inspected the regex does not match the correct portion of the HTML after the first iteration.

Consider a page that has two inline images. In all HTML below the portion that should be matched, replaced, or updated is in bold.

Image 1: test-133.jpg
Image 2: test-233.jpg

The patterns for $old and $new that are passed to _filefield_paths_replace_path in both iterations are (obviously replacing 'test-133' with 'test-233' for the second iteration):

$old: public://test-133.jpg
$new: public://sections/outdoor-recreation-tourism-management/test-133.jpg

The original HTML is as follows:

The first call to _filefield_paths_replace_path will result in the following regex:

(https:\/\/unbc.debug\/sites\/default\/files\/|\/sites\/default\/files\/|public:\/\/)(styles\/.*?\/public\/|)(test-133.jpg)

The following is the match:

The resulting updated HTML returned from preg_replace is:

The second call to _filefield_paths_replace_path results in the following regex:

(https:\/\/unbc.debug\/sites\/default\/files\/|\/sites\/default\/files\/|public:\/\/)(styles\/.*?\/public\/|)(test-233.jpg)

And the following is the match:

As you can see, the match is incorrect. The first image path starts with "/sites/default/files/", which is one of the patterns that will be matched by the regular expression. Since the dot character in the regular expression will match anything except a newline (unless the PCRE_DOTALL modifier is used), and since there is no newline anywhere between the beginning of the first image path and end of the second image path, everything between these two points is matched as the code to be replaced.

Because the use of the PREG_REPLACE_EVAL modifier in the call to preg_replace results in certain characters in the strings that replace the backreferences in the regex being escaped, the incorrectly updated HTML returned from preg_replace is:

I think a suitable solution would be to change /.*?/ by replacing the dot with a character class similar to /[a-z0-9\-_/]*?/. This would match a folder name that would be generated by Drupal, and also match multiple folders in said path (note: the forward slash in the character class is not escaped since escaping is performed by the call to str_replace when generating the regular expression).

I am not certain that this character class is 100% accurate, but I have based it on what are defined as the valid characters for image styles, which are themselves used to create the URLs for generated images.

Using the above character class in the regex, the HTML for the second match above would be:

And the HTML returned from preg_replace is as follows, which is correct:

If someone can tell me if the character class I've used is correct that'd be great.

We unfortunately do not use Git here, we're a Mercurial shop, but according to the docs I think I should be able to generate a Git patch using Mercurial. If not it's probably time I set up my own Git instance so I can generate patches.

Thanks,
Pablo

Comment	File	Size	Author
#4	filefield_paths-replace-path-regex-bug-1949508-4.patch	835 bytes	Pablo Gosse
#1	filefield_paths-replace-path-regex-bug-1949508.patch	836 bytes	Pablo Gosse

Comments

Comment #1

Pablo Gosse commented 21 March 2013 at 23:06

Status	File	Size
new	filefield_paths-replace-path-regex-bug-1949508.patch	836 bytes

Uploading a patch that applies this change. If someone knows whether the character class I used is not correct for a folder path to be generated by Drupal please let me know or modify the character class as needed.

Pablo

Comment #2

Pablo Gosse commented 21 March 2013 at 23:31

I forgot to include that in order for this bug to be triggered the image field must have an default insert style set, or the user must select an insert style, since the problem in the regular expression occurs in the second grouping which won't be matched if a style isn't used.

Pablo

Comment #2.0

Pablo Gosse commented 21 March 2013 at 23:31

Issue summary:

View changes

Realized that the character class I suggested would only match one folder name.

I updated it to [a-z0-9\-_\/] which will match multiple folders each separated by "/"

Comment #3

Pablo Gosse commented 26 March 2013 at 18:02

Assigned:	Unassigned	» Pablo Gosse
Status:	Active	» Needs review

Assigning to myself and marking as needs review so someone else can review the patch and verify this is the correct solution to the problem.

Comment #3.0

Pablo Gosse commented 26 March 2013 at 18:02

Issue summary:

View changes

Updated the description of the fix to the regular expression.

Comment #4

Pablo Gosse commented 27 March 2013 at 21:36

Status	File	Size
new	filefield_paths-replace-path-regex-bug-1949508-4.patch	835 bytes

There was an error in the first patch. The character class mistakenly had the forward slash escaped, when the escaping is actually done by the call to str_replace that wraps the code that generates the regex.

Corrected patch is attached.

Comment #5

Golem07 commented 2 April 2013 at 15:26

Thanks a lot for the patch! I was unable to insert multibpe images (with image module) before which was driving me crazy. I was about to completely get rid of the module until I found this post!