Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect user capabilities result in unexpected 401 responses from the REST API #1611

Open
dcalhoun opened this issue Jul 13, 2024 · 13 comments · May be fixed by #2110
Open

Incorrect user capabilities result in unexpected 401 responses from the REST API #1611

dcalhoun opened this issue Jul 13, 2024 · 13 comments · May be fixed by #2110
Assignees
Labels
[Type] Bug An existing feature does not function as intended

Comments

@dcalhoun
Copy link
Member

dcalhoun commented Jul 13, 2024

When querying the REST API with successful authentication, endpoints return unexpected 401 response statuses. It appears the user's capabilities are not accurately portrayed in the context of WordPress Playground.

I discovered this issue while using curl to query a site running with the Studio app, but I also reproduced the issue using wp-now. Contrastingly, performing the following steps with a site running with wp-env result in the expected outcome.

Steps to Reproduce

  1. Start a Playground-powered site: npx @wp-now/wp-now start
  2. Modify the site's ~/.wp-now/wordpress-versions/latest/wp-config.php to set the environment to local: define('WP_ENVIRONMENT_TYPE', 'local');
  3. Visit the site's WP Admin and navigate to UsersEdit (the admin user).
  4. Create and copy an application password.
  5. Create and copy a Base64 version of the combined username and application password: echo -n 'admin:<application_password>' | base64 | pbcopy
  6. Generate a curl request for the block types endpoint: curl --header "Authorization: Basic <base64_string>" -L http://localhost:<port>/?rest_route=/wp/v2/block-types

Expected Outcome

A 200 response containing the site's block types is returned.

Actual Outcome

A 401 response is returned:

{
  "code": "rest_block_type_cannot_view",
  "message": "Sorry, you are not allowed to manage block types.",
  "data": { "status": 401 }
}
@brandonpayton
Copy link
Member

I'm curious whether this has anything to do with #1539 and will grab this to test after the other PR is merged.

@brandonpayton brandonpayton self-assigned this Jul 16, 2024
@brandonpayton brandonpayton added the [Type] Bug An existing feature does not function as intended label Jul 16, 2024
@brandonpayton brandonpayton moved this from Inbox to Up next in Playground Board Jul 16, 2024
@dcalhoun
Copy link
Member Author

@brandonpayton thank you for the support!

When I investigated briefly, the permissions check unexpectedly returned false. If I recall correctly, when I logged $this->capabilities on the user class, it was empty.

@brandonpayton
Copy link
Member

@dcalhoun, it looks like the auth is not working for some reason. I'm getting the same message, and if I instrument the permissions check to error_log get_current_user_id() the value is 0.

Hoping to look into this a bit more later.

@brandonpayton
Copy link
Member

@dcalhoun After some debugging, it looks like Playground is auto-sending cookies and interfering with application password auth. Here's how:

Playground is auto-sending login cookies here:

async #dispatchToPHP(
php: PHP,
request: PHPRequest,
scriptPath: string
): Promise<PHPResponse> {
let preferredMethod: PHPRunOptions['method'] = 'GET';
const headers: Record<string, string> = {
host: this.#HOST,
...normalizeHeaders(request.headers || {}),
cookie: this.#cookieStore.getCookieRequestHeader(),
};

When Playground auto-adds cookies to these requests that contain Authorization headers, it causes WordPress to skip evaluation of the application password. Instead, cookie-based auth initially succeeds in an earlier filter here:

add_filter( 'determine_current_user', 'wp_validate_auth_cookie' );
add_filter( 'determine_current_user', 'wp_validate_logged_in_cookie', 20 );
add_filter( 'determine_current_user', 'wp_validate_application_password', 20 );

Then when rest_cookie_check_errors() looks for the nonce required by the REST API for cookie-based auth, it finds there is no nonce and clears the current user here:

if ( null === $nonce ) {
	// No nonce at all, so act as if it's an unauthenticated request.
	wp_set_current_user( 0 );
	return true;
}

This is an interesting one. Maybe it would make sense for the PHPRequestHandler to omit cookies if a request appears to contain another auth-related header. But cookies aren't just for auth. So it seems presumptuous to remove cookies when we don't know the user's intent. We could filter out specific WP cookies, but it still might be solving a problem caused by a bit of magic using a bit more magic.

Read and understand our cookie store code better would probably help generate other ideas.

@adamziel @bgrgicak do you have any thoughts on this?

@adamziel
Copy link
Collaborator

adamziel commented Nov 4, 2024

I wonder if it's time to get rid of the internal cookie store and scope all cookies to the specific scoped WordPress site path

@brandonpayton
Copy link
Member

I wonder if it's time to get rid of the internal cookie store and scope all cookies to the specific scoped WordPress site path

@adamziel, this is a great idea.

I think there is a rare possibility of the following:

  1. Cookies created for a specific slug-based Playground scope path
  2. That site is deleted
  3. A new site is created that happens to have the same slug as before 🍀
  4. The old site's login cookies are used with the new site

Maybe we should add site ID to the scope to avoid that rare possibility.

@brandonpayton
Copy link
Member

I wonder if it's time to get rid of the internal cookie store and scope all cookies to the specific scoped WordPress site path

I think this may turn out to be tricky because of restrictions on using Set-Cookie headers with created Response objects.

It would be nice if set cookies could be treated as HttpOnly, but it seems like we may have to set cookies with JS.

@brandonpayton
Copy link
Member

brandonpayton commented Dec 16, 2024

If we want to get rid of the cookie store, it may be necessary to do something like:
Message a server client tab and ask it to set cookies before the service worker resolves a fetch response that would contain those cookies.

This is not great because the cookies would not be set at the exact time as they would be if they were actually included as Set-Cookie headers in an HTTP response.

Alternately, to solve this GH issue, we could keep the cookie store and do the following:

  • Have Playground web app boot set a flag cookie like playground-web-app-sending-cookies=1
  • When the service worker receives a fetch event, check for the presence of the flag cookie.
    • If the cookie is present:
      • Remove it from the request
      • Infer that the browser is including cookies with the fetch
      • Relay cookies from the cookie store
    • If the cookie is absent:
      • Infer that the fetch is omitting cookies
      • Do not relay cookies from the cookie store

cc @adamziel

@brandonpayton
Copy link
Member

Because we cannot directly relay Set-Cookie headers via custom Response objects (they are forbidden response headers), I think we still need the HttpCookieStore for php-wasm/web, but we should be able to drop it elsewhere.

I think a good solution may be to support filter callbacks for requests and responses within PHPRequestHandler so the bulk of the logic can be maintained in php-wasm/universal while allow php-wasm/web to maintain a cookie store.

Then, as mentioned above, we can address this bug by always setting a cookie in the web client and letting its presence or absence tell the worker whether to relay cookies from the cookie store.

@brandonpayton
Copy link
Member

I think a good solution may be to support filter callbacks for requests and responses within PHPRequestHandler so the bulk of the logic can be maintained in php-wasm/universal while allow php-wasm/web to maintain a cookie store.

On second thought, we should just be able to handle this in the service worker with a per-scope cookie store. AFAICT, this would require no API changes. I am working on this.

@brandonpayton
Copy link
Member

On second thought, we should just be able to handle this in the service worker with a per-scope cookie store. AFAICT, this would require no API changes. I am working on this.

As I work through this, one awkwardness with handling this in the service worker is that there is no access to Set-Cookie headers via the Response object. So we'll need another way to relay that info or solve this at another layer. Other than that restricted access, handling cookie storage in the service worker which is already scope-aware seems to be a natural fit.

@brandonpayton
Copy link
Member

brandonpayton commented Dec 24, 2024

It looks like convertFetchEventToPHPRequest() in the @php-wasm/web-service-worker package may be a good place to for this. It is specific to running in web browsers, where we are forbidden from accessing Set-Cookie headers on actual Response objects. And this function is what handles mapping fetch() requests to PHP requests and PHP responses to fetch() responses.

The two things I still want to figure out are:
1. Where to best coordinate setting a playground-fetch-includes-cookies=1 cookie to serve as a hint whether cookies should be relayed with a given fetch request.
2. How to best cleanup stored cookies for a scope that is no longer loaded.

@brandonpayton
Copy link
Member

  1. Where to best coordinate setting a playground-fetch-includes-cookies=1 cookie to serve as a hint whether cookies should be relayed with a given fetch request.

Scratch that. We should just respect the credentials option on a request. It's how the browser would behave normally. There's no need for another side-channel hint about whether cookies ought to be included.

So I plan to:

  • Create a PR to move cookie store management so that it is only used for Playground running in a browser
  • Make sure that custom cookie storage respects the original request's credentials property

That should keep Playground running as expected in the browser and allow Playground CLI to behave more like a normal web server (which does not apply the same cookies for all clients).

@brandonpayton brandonpayton linked a pull request Dec 27, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

3 participants