[wp-trac] [WordPress Trac] #51939: Basic Auth staging protections conflicts with App Passwords

[WordPress Trac]

#51939: Basic Auth staging protections conflicts with App Passwords
-----------------------------------+--------------------
 Reporter:  TimothyBlynJacobs      |      Owner:  (none)
     Type:  defect (bug)           |     Status:  new
 Priority:  highest omg bbq        |  Milestone:  5.6
Component:  Application Passwords  |    Version:  5.6
 Severity:  blocker                |   Keywords:
  Focuses:  rest-api               |
-----------------------------------+--------------------
 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
 <?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.

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