Basic Auth staging protections conflicts with App Passwords

[TimothyBlynJacobs]

Application Passwords uses Basic Authentication to pass the username and password authentication data.

The first time is_user_logged_in is called during WP_REST_Server::serve_request(), we attempt authentication. If the PHP_AUTH_USER variable isn't populated, we don't attempt App Passwords authentication at all. If it is present, we attempt to authentication based on it. If an error is encountered, this is stored and then returned via the filter in WP_REST_Server::check_authentication and the rest_application_password_check_errors callback.

This becomes an issue if the site is globally behind Basic Authentication of its own. Because now the REST API will see the Basic Auth credentials, and try to authenticate the user even if it isn't intended. For reference: ​https://wordpress.org/support/topic/sitewide-basic-authentication-and-application-password-causing-issues/#post-13745874

There are a couple of different possible solutions.

There are two goals I'd like to maintain. For it to be a standards based as possible, and maintain the friendliest DUX. A developer should be able to tell that there was an issue with authentication.

1. Switch from using the Authorization header to something like WP-Authorization. It would follow the same format, but since it is a different header, there would be no chance of conflict. It would also allow for using App Passwords when a site is behind Basic Auth. This has a number of issues though since middleware wouldn't know that this is an authenticated request anymore. Particularly since we also aren't passing any cookies. It also breaks DUX of clients that have support for basic auth built in.

2. Instead of a whole different header, we can use a different authentication scheme. Such as WP-App-Password {token} instead of Basic {token}. This alleviates the issues due to the Authorization header not being used, but we still have the DUX issues.

3. Don't report errors if the password isn't exactly 24 characters, ie the format of App Passwords. Since App Passwords are unlikely to be typed by hand, and even if they were, an Application could reject them if they weren't 24 characters, this would be a fairly good indicator that App Passwords were attempted to be used. But if the server level Basic Auth password was 24 characters long, you'd also have this issue.

4. Require the client to prefix the App Password with something like wpapp. This is a more surefire signifier than the password being 24 characters, but it would require clients to juggle adding the prefix or not for existing App Passwords. Versus some of the following options, this has the advantage of the signifier being colocated with the other authentication data.

The following solutions would all also return authentication errors in a separate response header. For instance, we could send X-WP-Auth-Failed: {code}={message} or something.

5. That's it. Never return an authentication error in the main body of the response.

6. Require a signifier in the request to return an authentication error, if it is missing don't return the error from rest_application_password_check_errors(). For instance, a client could always pass a X-WP-App-Passwords: true header. This is quite simple for a developer to include.

7. If the REST API response is a 401 Unauthorized, see rest_authorization_required_code, return the authentication error if any. The 401 status code indicates that we tried to authenticate you as a user, but we couldn't determine who you were. Versus a 403 indicating you didn't have sufficient capabilities.

This would be implemented something like this.

<?php
$authenticated = $this->check_authentication();

if ( ! is_wp_error( $authenticated ) || rest_application_password_collect_status( null ) === $authenticated ) {
        $result = $this->dispatch( $request );
} else {
        $result = $authenticated;
}

$response = $this->dispatch();

// This indicates an error due to the user being logged out instead of
// not having the right caps. See rest_authorization_required_code().
if ( $response->get_status() === 401 && is_wp_error( $authenticated ) ) {
        $response = $authenticated;
}

---

There are some disadvantages to not returning the authentication error immediately. Let's say you are submitting a comment. If anonymous comments are on, the comments controller doesn't require you to be logged in. So you may end up submitting a comment anonymously even if you were trying to submit it as a logged in user. A similar thing could happen for the route that was originally reported, a contact form submission.

Of note, we somewhat already have the anonymous issue with cookie auth. If the nonce is missing, no error is returned. While this is very confusing to developers who are learning the REST API for the first time, once the coding error is fixed an error is always returned. Whereas with App Passwords the error is more of a runtime issue than a coding logic issue, the App Password might be invalid or deleted.

If at all possible, I'd like to be able to return the authentication error in the body of the response. It is clearer to clients that an authentication error happened versus putting that information in the headers of the response. It also is a higher fidelity transfer mechanism than the response headers. We report the $data included in the WP_Error object returned.

My current preference I think is option 7. This lets us still be standards based and the vast majority of the time would allow for us to return the authentication error in the body of the response. That wouldn't catch the issue with routes that can be anonymous, if we want to also solve that issue I would prefer option 6.

[georgestephanis]

Also, if the site itself is accessed via basic auth, maybe we could detect that in wp-admin and set an option disabling application passwords in the first place?

[TimothyBlynJacobs]

Of note, we have to protect against people using rest_authentication_errors to report capability issues versus just reporting on the state of the authentication attempt. In other words Authentication vs Authorization. This is why we are specifically checking if that was an app passwords generated error.

[TimothyBlynJacobs]

Also, if the site itself is accessed via basic auth, maybe we could detect that and set an option disabling application passwords in the first place?

Ooh nice. I like how deviously simple this would be. Where would we put that logic? Perhaps detect that on wp_loaded()? We'd need to regularly invalidate it and make sure that they weren't passing it to a REST API route.

[georgestephanis]

Replying to TimothyBlynJacobs:

Also, if the site itself is accessed via basic auth, maybe we could detect that and set an option disabling application passwords in the first place?

Ooh nice. I like how deviously simple this would be. Where would we put that logic? Perhaps detect that on wp_loaded()? We'd need to regularly invalidate it and make sure that they weren't passing it to a REST API route.

I have no opinions to when the check this. I'd shy away from dropping it in a transient in case the transient expires when unauthed api requests are still happening?

[TimothyBlynJacobs]

I agree, we should put it in an option somewhere. Should it be network wide or per-site?

We could put it in the beginning of admin.php like how we handle the upgrade checks.

Ideally, we'd also be able to handle if the user is auto-updated when they aren't visiting the admin. Depending on how the auto update happens, the basic auth header might not be populated in upgrade_560. For instance if cron is fired out of band, isn't protected by basic auth at all, or the host uses WP-CLI to update.

We could potentially do a loopback request to the homage page and check for a 401 error or WWW-Authenticate header in the response? Loopback doesn't always work, but we've been reporting that error in Site Health for a while now, so maybe this isn't a major issue.

We also need to handle invalidating this. Would there be an issue checking for PHP_AUTH_USER on every admin.php and deleting it immediately? Or should we only delete it if it has been let's say 24 hours of it not being reported/

We should also report that App Passwords is disabled in Site Health due to this in Site Health, but this can wait until 5.6.1.

Don't attempt App Passwords auth if the site hasn't generated any app passwords.

Show an error message if trying to create an App Password and the Basic Auth header is in use.

Trac ticket: https://core.trac.wordpress.org/ticket/51939

[TimothyBlynJacobs]

After discussion in #core-passwords we decided on a different solution.

We now won't return any errors from wp_authenticate_application_password if there are no App Passwords created. Additionally, we prevent creating new App Passwords if it looks like the site is already behind Basic Authentication.

[adamsilverstein]

I was able to test this in my local setup and verify both the issue and that the proposed fix in the PR works as expected:

• After setting up local auth and submitting a form (using CF7 as described in the issue) as a non-logged in user, I received an authentication error from the rest API. Status 401 message: "Unknown username. Check again or try your email address."
• Testing the PR the form submission/endpoint works as expected.
• Trying to set up Application Password when my site is behind basic auth, I discover I cannot:

[helen]

Patch looks good to me and this seems like a good approach for 5.6.

[TimothyBlynJacobs]

In 49752:

App Passwords: Prevent conflicts when Basic Auth is already used by the site.

Application Passwords uses Basic Authentication to transfer authentication details. If the site is already using Basic Auth, for instance to implement a private staging environment, then the REST API will treat this as an authentication attempt and would end up generating an error for any REST API request.

Now, Application Password authentication will only be attempted if Application Passwords is in use by a site. This is flagged by setting an option whenever an Application Password is created. An upgrade routine is added to set this option if any App Passwords already exist.

Lastly, creating an Application Password will be prevented if the site appears to already be using Basic Authentication.

Props chexwarrior, georgestephanis, adamsilverstein, helen, Clorith, marybaum, TimothyBlynJacobs.
Fixes #51939.

[helen]

In 49754:

App Passwords: Prevent conflicts when Basic Auth is already used by the site.

Application Passwords uses Basic Authentication to transfer authentication details. If the site is already using Basic Auth, for instance to implement a private staging environment, then the REST API will treat this as an authentication attempt and would end up generating an error for any REST API request.

Now, Application Password authentication will only be attempted if Application Passwords is in use by a site. This is flagged by setting an option whenever an Application Password is created. An upgrade routine is added to set this option if any App Passwords already exist.

Lastly, creating an Application Password will be prevented if the site appears to already be using Basic Authentication.

Props chexwarrior, georgestephanis, adamsilverstein, helen, Clorith, marybaum, TimothyBlynJacobs.
Reviewed by TimothyBlynJacobs, helen.
Merges [49752] to the 5.6 branch.
Fixes #51939.

Merged in 38361be.

[spacedmonkey]

After reviewing this patch, as the maintainer of multisite, I have one simple recommendation, save the OPTION_KEY_IN_USE against the main network. This can be done like this.

update_network_option( get_main_network_id(), WP_Application_Passwords::OPTION_KEY_IN_USE, 1 );

return (bool) get_network_option(  get_main_network_id(), self::OPTION_KEY_IN_USE );

This adds a key benefit, if you are using multi network, then this option is stored locally on the main network. Using the main network's option as a global store is a pattern ​already found in core.

CC other multisite maintainers @jeremyfelt @johnjamesjacoby @flixos90

Small fix to save detection of application password support to the main network to improve support for multi network support.

Trac ticket: https://core.trac.wordpress.org/ticket/51939

CC @TimothyBJacobs

[spacedmonkey]

In ​#793 I had a simple patch to better support WordPress multi network.

Detecting a feature like is application password supported, is a global feature not a per network feature. So if application password is set, should be stored globally like site meta support.

[TimothyBlynJacobs]

In 49764:

App Passwords: Store the "in use" option in the main network options.

Whether App Passwords are being used is a global featurel, not a per-network feature. This fixes issues on Multi Network installs if App Passwords are used on a different network from where they were created.

Props spacedmonkey.
Fixes #51939.
See [49752].

[helen]

In 49765:

App Passwords: Store the "in use" option in the main network options.

Whether App Passwords are being used is a global featurel, not a per-network feature. This fixes issues on Multi Network installs if App Passwords are used on a different network from where they were created.

Props spacedmonkey.
Fixes #51939.
See [49752].
Merges [49764] to the 5.6 branch.

[mrahmadawais]

Hi folks,

I have yet another use case that has become problematic because of this.

While trying to use OAuth 2 — ​https://www.npmjs.com/package/openid-client — which requires clientId and clientSecret to be sent in an Authorization: Basic urlSafeBase64('clientId:clientSecret') header — I keep hitting 401: Not Authorized error from WordPress since WP thinks I'm trying to use App Passwords.

Now, I do want to use App Passwords for another use case and don't want to disable them, but I'm stuck on how to handle Authorization: Basic XYZ Header-based requests as this global feature doesn't even let me run my code.

Any thoughts?

On another note, WP also doesn't use URL safe decoder for base64'd user:pass params — which is how openid-client specs and sends data. This means, even if I use user:pass in place of clientId:clientSecret — it doesn't work due to clientId:clientSecret using URL safe base64'd strings.

[lucasbustamante]

It would be really cool if we could use Application Passwords on a site that is behind Basic Auth :(

Don't report errors if the password isn't exactly 24 characters, ie the format of App Passwords. Since App Passwords are unlikely to be typed by hand, and even if they were, an Application could reject them if they weren't 24 characters, this would be a fairly good indicator that App Passwords were attempted to be used. But if the server level Basic Auth password was 24 characters long, you'd also have this issue.

Since sites behind Basic Auth are usually under development/staging, I think this could be a good solution for us to enable through a filter, such as: add_filter('wp_application_password_basic_auth_compat', '__return_true');, which then would trigger the behavior above, as a solution for those that need to use App Pass on Basic Auth environments?