Handle empty and malformed $_SERVER request variables

[BrianHenryIE]

When WordPress is loaded locally, i.e. without a HTTP request, the expected $_SERVER['REQUEST_METHOD'] is not set, resulting in PHP Notice: Undefined index: REQUEST_METHOD in /path/to/wp-content/plugins/satispress/src/ServiceProvider.php on line 156 being logged.

The commit checks if WordPress is DOING_CRON and if so, returns a instantiated but unconfigured Request (WP_REST_Request) object.

[bradyvercher]

Thanks for the PR, @BrianHenryIE! Rather than check wp_doing_cron(), let's just check to see if that REQUEST_METHOD key exists and return the default if not:

$request = new Request( $_SERVER['REQUEST_METHOD'] ?? '' );

Does that work for you?

[BrianHenryIE]

Yeah, that should work too. I wrote it as a cron check because I was being too lazy too consider any other means of invocation. It probably affects wp shell too.

I had another two related logs this morning. get_request_path() is parsing $_SERVER['REQUEST_URI'] and failing at ltrim() so I'm returning an empty string early, which the two calling methods should be happy with.

[bradyvercher]

Can you clarify how you're loading WordPress that's causing those notices to be triggered? I believe WordPress should populate $_SERVER['REQUEST_URI'] fairly early in the bootstrap process, so I'm curious when this would be an issue.

[BrianHenryIE]

I looked at my transfer logs and it seems that was triggered by: "GET ///?author=1 HTTP/1.1" 500 2836 "-" "Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:62) Gecko/20100101 Firefox/62.0"

So $_SERVER['REQUEST_URI'] = ///?author=1 (not empty as I assumed).

?author=1 is a username phishing attempt but I'm not sure what the sequence of / is for.

They're causing wp_parse_url() to return false.

The multiple sequential / could be reduced to one in all cases except :// with: $request_uri = preg_replace( '/(?<!:)\/{2,}/', '/', $_SERVER['REQUEST_URI']);

but that doesn't seem like the right way to address it.

[bradyvercher]

Was that request also the one that causes that issue that led to e6a7a67? Or is that unrelated?

It looks like WordPress does a lot of processing to determine the request path. I'd rather not parse the URI more than absolutely necessary, especially since this is for an invalid request.

Since that method is really only used for determining if it's a SatisPress request, we could either ltrim() slashes before passing the URI to wp_parse_url(), or handle cases where wp_parse_url() returns false, which apparently has been an issue before.

[BrianHenryIE]

So, two unrelated requests. First is from starting WordPress via cron or command line. Second is via HTTP with a malformed REQUEST_URI. Sorry for mixing up the PR.

I think the wp_parse_url() should really be taking care of the extra slashes, so wouldn't bother here. It's safe to assume a malformed REQUEST_URI is not going to be a SatisPress request. The two places get_request_path() is used, register_hooks() and is_satispress_request(), both run strpos() on the result so I suggest returning an empty string if the URL isn't successfully parsed, i.e.

if( false === $request_path ) {
    return '';
}

That should accommodate other malformed URLs that might come up, and the behaviour is still correct.

[bradyvercher]

Thanks for the explanation. I just wanted to make sure I understood when these issues were occurring. If you want to push up that fix, I'll get this merged.

[bradyvercher]

Thanks @BrianHenryIE! I squashed a couple of commits and merged this manually, so it didn't close automatically, but they have been committed. I appreciate you reporting and discussing this. 🍻