[wp-trac] [WordPress Trac] #26829: Use of strpos() in extract_from_markers() and insert_with_markers() can target wrong BEGIN and END markers.

[WordPress Trac]

#26829: Use of strpos() in extract_from_markers() and insert_with_markers() can
target wrong BEGIN and END markers.
----------------------------------------+------------------------------
 Reporter:  Faison                      |       Owner:
     Type:  defect (bug)                |      Status:  new
 Priority:  normal                      |   Milestone:  Awaiting Review
Component:  Rewrite Rules               |     Version:  trunk
 Severity:  normal                      |  Resolution:
 Keywords:  has-patch needs-unit-tests  |
----------------------------------------+------------------------------
Changes (by nacin):

 * keywords:   => has-patch needs-unit-tests
 * component:  General => Rewrite Rules


Comment:

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

--
Ticket URL: <https://core.trac.wordpress.org/ticket/26829#comment:1>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform