Use of strpos() in extract_from_markers() and insert_with_markers() can target wrong BEGIN and END markers.

[Faison]

While working on a plugin, I came across the need to have two sections of BEGIN and END markers in the .htaccess file. The most important marker section is located at the end of the file with the marker "e3r", while the second marker section is at the beginning of the file with the marker "e3r_query_redirects". Now when I call extract_from_markers while passing "e3r" as the marker, the contents between the "e3r_query_redirects" markers are returned.

Here are steps you can take to see this issue with the function insert_with_markers():

1. Open the .htaccess file
2. Add # BEGIN WordPress_Foo to the top of the file, add a couple of newlines, and add # END WordPress_Foo
3. Save the .htaccess file and close it
4. Log into the WordPress Dashboard and navigate to Settings -> Permalinks
5. Press the Save Changes Button
6. Reopen the .htaccess file
7. Notice that the marker section originally labeled WordPress_Foo is now a duplicate WordPress marker section

You can continue to repeat those steps to generate more duplicate WordPress marker sections.

This issue is caused by the use of the strpos() function, which looks for the position of a substring in a string. So when you have a marker section labeled WordPress_Foo above the section labeled WordPress, the WordPress_Foo section is found by strpos(), which stops the WordPress section from being found.

I already wrote a solution that has worked for me so far and I'm including a diff file with the changes. Basically, instead of using the function strpos(), which looks for the position of a substring and causes the aforementioned bug, I use the following statement: if ( "# BEGIN {$marker}" === $markerline ).

Since this is my first patch to core, I would definitely appreciate all feedback :)

Thanks,
Faison

[nacin]

Hi Faison, thanks for the excellent and detailed bug report. Tentatively, this looks good. The next question I have is why it's currently strpos() instead of an exact check. Time for some spelunking.

My first guess is that the worry could be line endings. And looking at the function gives us some indications that may be the case. if ( $markerdata = explode( "\n", implode( '', file( $filename ) ) )); suggests there was an attempt to work around file()'s desire to return an array of lines ending with \n. This code actually works pretty well, despite the oddity. This function does have a FILE_IGNORE_NEW_LINES flag, but it looks like the $flags argument was added in PHP 5, and this code was most likely written back when we supported PHP 4. It also doesn't work well for Windows line endings, which is a concern for user-editable files like wp-config.php and .htaccess.

Of course, this explode/implode trick doesn't handle \r\n line endings either, but in that case, the strpos() will avoid messing with the \r at the end of the line. An rtrim( $line, "\r\n" ) could work and still allow for a direct comparison.

This sounds like it could benefit from some unit tests. :-) If you're up for it: ​http://make.wordpress.org/core/handbook/automated-testing/.

[Faison]

Thank you for the direction, Nacin, I'm going to get started on that right away!

[Faison]

Hi Nacin,

I actually need some input on the best way to handle testing extract_from_markers() and insert_with_markers(). Since the code uses the functions file(), fopen(), etc., we would need to create files just for the unit tests, create temporary files programmatically within the tests, or rewrite the functions to use an object that we could mock.

I see that there's a directory for files required for certain unit tests, but I'm not sure where to put test htaccess files. Also, are the files in the /phpunit/data/ directory only to be used for read operations? If that's the case, I start to lean towards wanting to create a temp file with tempname(), but there is a possibility that tempname() won't work on some computers.

Is there a way we can replace the php filesystem calls with an object that can be mocked? I just found the WP_Filesystem_Direct class which has the function get_contents_array() which is just an abstraction for file(). The calls to fopen, fwrite, and fclose in the insert_with_markers() function could be removed by first making changes into a new array, imploding that array, and passing the resulting string into the put_contents() function.

So what route do you believe would be the best for WordPress and the Test Suite as a whole? I'm a fan of refactoring the functions to allow for mocks, but you know much more about this whole project than me and I'm willing to go a different route.

[Faison]

New patch to allow for proper unit testing.

[Faison]

Comprehensive unit tests for the patch

[Faison]

Hi Nacin,

Looked like you were busy with 3.8.1 and the other core initiatives, so I went ahead and wrote unit tests and updated my patch to allow for mocking of the WP_Filesystem_Direct object.

Thanks,
Faison

[jeremyfelt]

strpos() was strstr() until [4990]. Looks like the current behavior goes back to #1661

[Faison]

New patch that adds brackets to single-lined if's and foreach's

[chesio]

Refreshed patch using new split_by_markers() function to consistently handle lines splitting

[chesio]

Tests for refreshed patch (also covering new function)

[chesio]

Hi,

I run into this issue too and subsequently found this old ticket.

I propose a slightly different fix in my patch. Since both extract_from_markers() and insert_with_markers() functions need to split input lines in the same way, I re-factored the splitting part into separate function. Benefits are consistency and easier unit testing.

Btw. this is the first time I submit unit tests for WordPress, so I'd be glad for any comments on what could be done better.

[chesio]

Update for refreshed patch

[chesio]

@SergeyBiryukov I just uploaded updated patch that applies cleanly on trunk after [41928].

If there's anything else I can do to get this patch merged, please let me know.

[chesio]

@SergeyBiryukov I've updated both patches (src and tests) to apply cleanly on current trunk, see
26829.refreshed.3.diff and 26829.tests.refreshed.3.diff.

[pento]

I think this patch is okay, but it needs testing and review.

[pento]

After reviewing this some more, I like the direction of 26829.refreshed.3.diff​, I don't think we need to provide the ability to mock the filesystem classes, as 26829.2.diff​ does.

There are a few issues with 26829.refreshed.3.diff​ that need to be addressed:

• Using the $wp_filesystem global would be better than the direct file access that insert_with_markers() currently does.
• Does the FILE_IGNORE_NEW_LINES flag allow for \r\n newlines? If not, we should be stripping them.
• The behaviour in Core allows for the markers to be indented. trim()ing the line before comparing it will allow for that, too.

More unit tests for different line endings and indenting would be good, too.

[pento]

Bumping to Future Release, pending patch updates.