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

Ensure Site Health enqueued assets module excludes admin-specific assets #134

Open
felixarntz opened this issue Jan 28, 2022 · 3 comments
Open
Labels
Needs Discussion Anything that needs a discussion/agreement [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Type] Enhancement A suggestion for improvement of an existing feature

Comments

@felixarntz
Copy link
Member

In the current implementation of the Site Health enqueued assets module (see #25), all assets loaded in the frontend are considered when an administrator is logged in. The latter is partly for security purposes, partly to avoid issues with frontend caching.

This is problematic since for example it will currently include admin-specific assets loaded in the frontend only for logged in administrators, e.g. things related to the admin bar or other tooling scripts.

We need to find a way to record this information in a way similar to how a non-logged-in visitor would see the site, while ensuring this remains an appropriate implementation (e.g. having a random visitor cause a database change is considered problematic in WordPress context).

This needs some technical discussion. Let's evaluate alternative approaches to accomplish this goal.

@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature [Focus] Site Health Needs Discussion Anything that needs a discussion/agreement labels Jan 28, 2022
@felixarntz
Copy link
Member Author

For this one I would think a solid approach could be the following:

  • Instead of using a "random" administrator session in the frontend to collect the data, we could send a remote (cookie-less) GET request to the frontend when the administrator is logged in (and only when the transient is expired, of course).
  • The GET request could include a specific query parameter to enforce this "asset recording logic" to trigger (e.g. ?wp_record_assets=1). It could in addition to that include some "security key" value in another query parameter, that can tie the request to the admin logged in - for that it has to be ensured this can't be triggered by someone else, who e.g. is not logged in.
  • By using a remote (cookie-less) GET request we have the "regular" frontend, without anybody logged-in, since there's no awareness of the current browser session. At the same time, we can use query parameters to only run the logic and set the transient(s) if that given "key" is "valid" - how exactly to validate that would be part of the detailed approach here.

This may seem complex, but IMO it's not too crazy actually. Another benefit this provides is that it really happens at a specific point, rather than a "random" visitor, which could result in unexpected side effects.

Curious to hear your ideas and thoughts!

cc @aristath @manuelRod @audrasjb

@aristath
Copy link
Member

The above implementation seems reasonable and safe. I think using a nonce as "security key" would be a necessary precaution, just to be on the safe side of things and avoid accidental/malicious cache resets for performance reasons.
It may sound like a complicated solution, but it shouldn't be especially hard to implement since the steps are clear, specific, and simple enough. 👍

@manuelRod
Copy link
Contributor

Would this approach imply a certain action from the administrator to run the test? or automatic when under certain circumstances? (admin logged in and transient expired?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Discussion Anything that needs a discussion/agreement [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants