Switch to using $_REQUEST directly instead of filter_input() and remove backup_2fa

[dd32]

Currently there's two different routes into the 2FA credentials window, action=validate_2fa and action=backup_2fa, these are exactly the same, except that the former uses POST and the latter uses GET.

This is partially caused by the usage of filter_input() which has other problems for us, such as being unable to be unit tested.

This PR simplifies it by combining action=validate_2fa and action=backup_2fa and simplifying the resulting code.

This is part of an initial work towards #484.

[lkraav]

This is partially caused by the usage of filter_intput() which has other problems for us, such as being unable to be unit tested.

Interesting, I never knew filter_input() could have drawbacks (other than not able to do $_REQUEST).

It this ChatGPT reply about all there is to know?

Why is it difficult to unit test PHP filter_input()?

Unit testing PHP's filter_input() function can be challenging because it is a function that relies on external input, and its behavior can be affected by the environment in which it is running. Here are some reasons why it can be difficult to unit test:

External input dependency: filter_input() relies on external input from the user or from other sources, which can be difficult to simulate in a unit test environment. For example, if you want to test the behavior of filter_input() when it receives a specific input value, you need to find a way to simulate that input in your test code.

Side effects: filter_input() can have side effects, such as changing the values of superglobal arrays such as $_GET, $_POST, and $_COOKIE. This can make it difficult to write isolated unit tests that don't interfere with the rest of the system.

Environment dependency: The behavior of filter_input() can be affected by the environment in which it is running, such as the PHP version, the configuration settings, and the underlying operating system. This can make it difficult to write tests that are consistent across different environments.

To address these challenges, it is often helpful to use a combination of unit tests and integration tests. Unit tests can focus on the behavior of filter_input() itself, while integration tests can test the interaction between filter_input() and other parts of the system, such as the database or other external services. It is also important to use test data that covers a range of input values and edge cases, to ensure that the function behaves correctly in a variety of scenarios.

[dd32]

Interesting, I never knew filter_input() could have drawbacks (other than not able to do $_REQUEST).

That response is mostly accurate, although there are some things that I don't believe are truthful (such as filter_input changing the superglobals).

The best description is IMHO that: filter_input() only works on the raw input to PHP, it does not use the php user-space super globals, it only uses the data passed to the php sapi process at the start of the request. That has some benefits, in that it enforces code not alter the source-of-truth for the request, but has the downside of making unit testing harder. filter_var() can replace filter_input except you loose the array-key-exists checks and have to do it yourself, which looses most of the advantages of it, for example:

-		$request_nonce = filter_input( INPUT_GET, self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG, FILTER_CALLBACK, array( 'options' => 'sanitize_key' ) );
+		$request_nonce = isset( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ) ? filter_var( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ], FILTER_CALLBACK, array( 'options' => 'sanitize_key' ) ) : '';

Using the PHP7+ null-coalescing operator can make it similar though:

-		$request_nonce = filter_input( INPUT_GET, self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG, FILTER_CALLBACK, array( 'options' => 'sanitize_key' ) );
+		$request_nonce = filter_var( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ?? '', FILTER_CALLBACK, array( 'options' => 'sanitize_key' ) );

In other words, while the filter_* API offers some benefits, in the space of a WordPress plugin it offers very little benefit where custom callbacks exist for validation/coercing and you need to mock requests for unit testing.

My experience of the filter extension is a bit biased, as back in the early PHP 5.x era where it was a new feature, it was very hard to rely upon due to it being disabled sometimes, and security vulnerabilities / bugs existed in older versions which although you may have called FILTER_SANITIZE_URL it may return different values depending upon which PHP version it was.

There's another reason for the change here though too - running sanitize_key() over the input just means that clients can send junk and the application interprets it as something valid and expected, which in something that's a security plugin is kind of not expected behaviour. User input sure, you should coerce user-input into the format they intended on supplying, but something such as a hard-coded action form value should be taken as what's sent, not a 'sanitized' version of it.

[iandunn]

I think the backup_2fa stuff is necessary.

The $_REQUEST stuff looks good, though, and I agree that filter_input() adds minimal value, and makes testing harder in an environment like WP.