Make WordPress Core

Opened 11 months ago

Last modified 11 months ago

#58764 assigned defect (bug)

Incorrect return type in `WP_Rewrite::using_index_permalinks()`.

Reported by: costdev's profile costdev Owned by: costdev's profile costdev
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 2.7
Component: Permalinks Keywords: 2nd-opinion
Focuses: Cc:

Description

Overview

In [8899], 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 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:

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

  • all 135 plugins and 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 preg_match eventually leads to pcre_get_compiled_regex_cache_ex.

The following failure conditions are listed:


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, #. 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:

  • 0 plugins directly assign a value to this property.
  • 0 themes directly assign a value to this property.
  • 1 plugin extends the WP_Rewrite class, but does not set a value for the property.
  • 0 themes extend the WP_Rewrite class.


Can we prevent the potential failure?

Yes, by using preg_quote( $this->index, '#' ). 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. 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?

Change History (1)

#1 @costdev
11 months ago

  • Owner set to costdev
  • Status changed from new to assigned
Note: See TracTickets for help on using tickets.