[wp-trac] [WordPress Trac] #58764: Incorrect return type in `WP_Rewrite::using_index_permalinks()`.

[WordPress Trac]

#58764: Incorrect return type in `WP_Rewrite::using_index_permalinks()`.
--------------------------+-----------------------------
 Reporter:  costdev       |      Owner:  (none)
     Type:  defect (bug)  |     Status:  new
 Priority:  normal        |  Milestone:  Awaiting Review
Component:  Permalinks    |    Version:  2.7
 Severity:  normal        |   Keywords:  2nd-opinion
  Focuses:                |
--------------------------+-----------------------------
 == Overview
 In [8899], [https://github.com/WordPress/wordpress-develop/blob/6.2/src
 /wp-includes/class-wp-rewrite.php#L357-L373
 WP_Rewrite::using_index_permalinks()] was documented to return `bool`.
 However, its actual return type is `int|false`.

 A guard explicitly returns `false`, however the final return statement
 returns the result of a `preg_match()` call.

 Per [https://www.php.net/manual/en/function.preg-match.php the PHP
 manual]:
 > `preg_match()` returns 1 if the pattern matches given subject, 0 if it
 does not, or false on failure.

 == Docs change, or `bool` cast?

 === Docs change
 We could change the return type to:

 {{{#!php
 @return int|false 1 if permalink links are enabled and index.php is in the
 URL,
                   0 if permalinks are enabled but there was a failure,
                   false if permalinks are disabled.
 }}}

 However, I think this is an overly complicated return for a method called
 `using_index_permalinks()` that suggests, and is currently documented as,
 a simple `bool` return type.

 === `bool` cast: Is there a backward compatibility concern?
 Yes, but a very small one.

 WP Directory shows:
   - [https://wpdirectory.net/search/01H4T8ZA3EYAHNCJH57EKTSYH2 all 135
 plugins] and [https://wpdirectory.net/search/01H4TEEJ598PDEK1WCRJ4C3SGW
 all 138 themes] that call this method use a loose check on the result,
 treating `0` and `false` as "No, it does not use index permalinks". These
 are safe from a `(bool)` cast.
   - It's possible that someone out there is using `1 ===
 $wp_rewrite->using_index_permalinks()`, but as the method is documented to
 return `bool`, they're most likely using a loose check after an assumed
 attempt of `true ===` would have always failed, as the method currently
 ''never'' returns `true`.

 === When does `preg_match()` return `false`?

 Per PHP-src, a call to [https://github.com/php/php-
 src/blob/c6b9db2131deda7ca0683a6006fe9ae8dd767f51/ext/pcre/php_pcre.c#L1465
 preg_match] eventually leads to [https://github.com/php/php-
 src/blob/master/ext/pcre/php_pcre.c#L589C26-L589C58
 pcre_get_compiled_regex_cache_ex].

 The following failure conditions are listed:
 - [https://github.com/php/php-src/blob/master/ext/pcre/php_pcre.c#L642
 Empty regular expression]
 - [https://github.com/php/php-src/blob/master/ext/pcre/php_pcre.c#L654
 Delimiter must not be alphanumeric, backslash, or NUL]
 - [https://github.com/php/php-src/blob/master/ext/pcre/php_pcre.c#L698 No
 ending delimiter '%c' found]
 - [https://github.com/php/php-src/blob/master/ext/pcre/php_pcre.c#L700 No
 ending matching delimiter '%c' found]
 - [https://github.com/php/php-src/blob/master/ext/pcre/php_pcre.c#L750
 Unknown modifier '%c']
 - [https://github.com/php/php-src/blob/master/ext/pcre/php_pcre.c#L752 NUL
 is not a valid modifier]
 - [https://github.com/php/php-src/blob/master/ext/pcre/php_pcre.c#L764 The
 /e modifier is no longer supported, use preg_replace_callback instead]
 - [https://github.com/php/php-src/blob/master/ext/pcre/php_pcre.c#L779
 Failed to generate locale character tables]
 - [https://github.com/php/php-src/blob/master/ext/pcre/php_pcre.c#L801
 Compilation failed: %s at offset %zu]
 - [https://github.com/php/php-src/blob/master/ext/pcre/php_pcre.c#L852
 Internal pcre2_pattern_info() error %d]
 - [https://github.com/php/php-src/blob/master/ext/pcre/php_pcre.c#L862
 Internal pcre_pattern_info() error %d]

 === Which of these failures could happen here?
 In the context of `WP_Rewrite::using_index_permalinks()`, there is only
 one possible failure: `WP_Rewrite::$index` contains the delimiter, `#`.
 [https://3v4l.org/M94EH 3v4l].

 The `index` property is set to `'index.php'` by default. However, it's a
 `public` property and could therefore be directly overridden, or set in a
 subclass of `WP_Rewrite`.

 WP Directory shows:
 - [https://wpdirectory.net/search/01H4TBP4HXJZQ8G58BGB6A0QFE 0 plugins]
 directly assign a value to this property.
 - [https://wpdirectory.net/search/01H4TBT9RAKWGY41CCPD320RBA 0 themes]
 directly assign a value to this property.
 - [https://wpdirectory.net/search/01H4TBW9JQNVX9Q580Z0FPD6DH 1 plugin]
 extends the `WP_Rewrite` class, but does not set a value for the property.
 - [https://wpdirectory.net/search/01H4TC0P3CYKZ27FK2KBGJQSP5 0 themes]
 extend the `WP_Rewrite` class.

 === Can we prevent the potential failure?
 Yes, by using `preg_quote( $this->index, '#' )`. [https://3v4l.org/CYsS7
 3v4l].

 === ''Should'' we prevent the potential failure?
 As demonstrated by the failure, the regex is currently susceptible to
 special characters. When the default value of `$this->index`
 (`'index.php'`) is concatenated, the resulting pattern is
 `'#^/*index.php#'`.

 See the `.`? The `preg_match()` call is currently vulnerable to `index[any
 character except newline]php`. [https://3v4l.org/LQSOG 3v4l].

 Given this, it's possible that a developer out there has set a custom
 `$index` value that contains a regex pattern. However, I think that's very
 unlikely.

 Contrarily, the flawed regex has the potential to cause false positives
 if, for example, the permalink structure starts with `/index2php`. While
 that's also unlikely, using `preg_quote()` here would prevent that false
 positive.

 -----

 == Call for opinions

 1. Should we just change the documented return type and description? If
 so, what makes sense here?
 2. OR should we cast the result to `bool`?
 3. ADDITIONALLY should we use `preg_quote()` here to prevent the potential
 failure?

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/58764>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform