WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 weeks ago

#26829 new defect (bug)

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

Reported by: Faison Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version: 2.0
Component: Rewrite Rules Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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

Attachments (7)

26829.diff (1.2 KB) - added by Faison 4 years ago.
26829.2.diff (3.9 KB) - added by Faison 4 years ago.
New patch to allow for proper unit testing.
26829.tests.diff (16.6 KB) - added by Faison 4 years ago.
Comprehensive unit tests for the patch
26829.3.diff (4.2 KB) - added by Faison 4 years ago.
New patch that adds brackets to single-lined if's and foreach's
26829.refreshed.diff (3.8 KB) - added by chesio 6 months ago.
Refreshed patch using new split_by_markers() function to consistently handle lines splitting
26829.tests.refreshed.diff (3.0 KB) - added by chesio 6 months ago.
Tests for refreshed patch (also covering new function)
26829.refreshed.2.diff (3.9 KB) - added by chesio 3 weeks ago.
Update for refreshed patch

Download all attachments as: .zip

Change History (19)

@Faison
4 years ago

#1 @nacin
4 years ago

  • Component changed from General to Rewrite Rules
  • Keywords has-patch needs-unit-tests added

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

#2 @Faison
4 years ago

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

#3 @Faison
4 years ago

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.

Last edited 4 years ago by Faison (previous) (diff)

@Faison
4 years ago

New patch to allow for proper unit testing.

@Faison
4 years ago

Comprehensive unit tests for the patch

#4 @Faison
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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

#5 @jeremyfelt
4 years ago

  • Version changed from trunk to 2.0

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

#6 @jeremyfelt
4 years ago

  • Milestone changed from Awaiting Review to Future Release

@Faison
4 years ago

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

#7 @chriscct7
2 years ago

  • Keywords needs-refresh added

@chesio
6 months ago

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

@chesio
6 months ago

Tests for refreshed patch (also covering new function)

#8 @chesio
6 months ago

  • Keywords needs-refresh removed

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.

#9 @SergeyBiryukov
4 months ago

  • Milestone changed from Future Release to 4.9

This ticket was mentioned in Slack in #core by melchoyce. View the logs.


6 weeks ago

#11 @SergeyBiryukov
5 weeks ago

  • Milestone changed from 4.9 to 5.0

@chesio
3 weeks ago

Update for refreshed patch

#12 @chesio
3 weeks ago

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

Note: See TracTickets for help on using tickets.