Guard beacon script against saving not expected values into the database about wp-rocket HOT 22 CLOSED

I guess there are 2 ways of handling those exceptions: preventing them to be identified in the first place as candidates, or filter out before storing in DB some given patterns.
I would highly recommend the 1st option, as it should allow at some point to get a refined detection system, whereas the second would just be a list of exceptions and it won't properly scale nor improve accuracy.
That being said, @DahmaniAdame & @piotrbak, for each above example, we should rather focus on finding examples reproducing them. Otherwise, we'll "just" filter out based on some patterns, and those patterns are yet to be determined anyway 🤔 Those examples just like that are unfortunately not useful for us :/

from wp-rocket.

@DahmaniAdame I experienced it once on the testing website, it wasn't there after cleaning the data anymore. I suspect it'll be reproducible with big div element that's set with background like this.

from wp-rocket.

@wp-media/engineering-plugin-team

I created this PR #6605 to validate the idea and seems like it's working and covering many cases, can u plz take a look and once the idea is validated I'll fix the tests and may add some more test cases.

from wp-rocket.

AC 1 & 3 would need to be refined I believe, that's quite free to interpretation here. We would really need examples to reproduce, otherwise we won't be able to validate the development, nor assess exactly what we should guard against. Typically, it might not be straightforward to distinguish relative URLs from the rest of possible values in a src attribute. Without examples, we won't know what to work on.

from wp-rocket.

@DahmaniAdame Would you be able to provide steps to reproduce the problem with Chrome extension?

from wp-rocket.

@DahmaniAdame Would you be able to provide steps to reproduce the problem with Chrome extension?

Use an extension that adds an overlay with an image on it, like HarpaAI for example, and run the script. There should be entries related to the icon used on the overlay.

There should be an entry similar to this:

chrome-extension://eanggfilgoajaocelnaflolkadkeghjp/img/commands/main.svg

from wp-rocket.

Unexpected values not added to the db

Examples of what can be excluded:

https://domain.ext/file.php?url=img.jpg
https://domain.ext/file.js?url=img.jpg
https://domain.ext/file.php#url=img.jpg
https://domain.ext/file.js#url=img.jpg
chrome-extension://extension-hash/path/to/image/x.svg
linear-gradient(160deg, rgb(255, 255, 255) 0%, rgb(248, 246, 243) 100%)

About data URI they are accepted as LCP elements.

from wp-rocket.

@DahmaniAdame @benorfaz what do you think? I'd agree with Mathieu.

If so, @DahmaniAdame could you add exact steps for this one?

We'd be adding each case separately then.

from wp-rocket.

Option 1 sounds right.

First thing, let's drop the ones with # as they are unlikely to be generally used. ? is the norm on passing arguments.

https://domain.ext/file.php?url=img.jpg

I couldn't find the plugin that served that, but you can use the below for testing:

https://call.adame.io/images/adaptive.php?image_url=https://image.civitai.com/xG1nkqKTMzGDvpLrqFT7WA/09af39a1-c481-4ad0-9283-aa4a4b6db0c9/original=true/00010-360343057.jpeg&width=200

https://domain.ext/file.js?url=img.jpg

More for external services using node to served adaptive images. I don't have anything on hand for now. We an dismiss it until we have a case.

chrome-extension://extension-hash/path/to/image/x.svg

Install the HarpaAI extension on Chrome and test - https://chromewebstore.google.com/detail/harpa-ai-automation-agent/eanggfilgoajaocelnaflolkadkeghjp?authuser=2

linear-gradient(160deg, rgb(255, 255, 255) 0%, rgb(248, 246, 243) 100%)

I don't have any example on this one. @piotrbak where did you experience that?

from wp-rocket.

@piotrbak can you provide a sample the code for engineering/QA to take it into consideration?

from wp-rocket.

Scope a solution ✅

src/wp-rocket/assets/js/lcp-beacon.js

In _getElementInfo() check if element_into.src is a valid url before returning the element_info. We could do something like this

const image_src = element_info.src
const valid_image_extensions = ['.jpg', '.jpeg', '.png', '.gif'];
const file_extension = image_src.split('.').pop().toLowerCase();

if (image_src.startsWith('http://') || image_src.startsWith('https://') &&  valid_image_extensions.includes('.' + file_extension) && !image_src.includes('?')) {
           return element_info;
} else if( image_src.startsWith('data:image')) {
          return element_info;
}

Estimate the effort ✅

[S]

from wp-rocket.

We can also do the same thing on the backend should in case anything slip through the frontend

from wp-rocket.

as a final decision, we need to exclude the following urls:

https://domain.ext/file.php?url=img.jpg
https://domain.ext/file.js?url=img.jpg
https://domain.ext/file.php#url=img.jpg
chrome-extension://extension-hash/path/to/image/x.svg
linear-gradient(160deg, rgb(255, 255, 255) 0%, rgb(248, 246, 243) 100%)

Correct?

from wp-rocket.

I was thinking of doing that from the php side, exactly here:

not from the JS side @Khadreal

from wp-rocket.

I was thinking of doing that from the php side, exactly here:

wp-rocket/inc/Engine/Media/AboveTheFold/AJAX/Controller.php

Line 53 in 715e19c

$images = isset( $_POST['images'] ) ? json_decode( sanitize_text_field( wp_unslash( $_POST['images'] ) ) ) : [];

not from the JS side @Khadreal

Okay. I'm thinking we could that on both side. I'll add grooming for the backend

from wp-rocket.

as a final decision, we need to exclude the following urls:

https://domain.ext/file.php?url=img.jpg
https://domain.ext/file.js?url=img.jpg
https://domain.ext/file.php#url=img.jpg
chrome-extension://extension-hash/path/to/image/x.svg
linear-gradient(160deg, rgb(255, 255, 255) 0%, rgb(248, 246, 243) 100%)

Correct?

@DahmaniAdame FYA

from wp-rocket.

Thanks @wordpressfan ! The safeguard/bail-out with elementInfo and empty( $image->type ) look OK to me.
My main concern is about:

  $image_src_filetype_array = wp_check_filetype( $image_src_path );
  return ! empty( $image_src_filetype_array['type'] ) && str_starts_with( $image_src_filetype_array['type'], 'image/' );

@DahmaniAdame & @piotrbak, can you confirm that all elements that we should capture for LCP/ATF should always be URLs to a file of image type? Meaning, the URL we store should always be directly and explicitely pointing to a .jpg, .png or something like this?

@wordpressfan how does it behave for this image for instance? http://1.gravatar.com/avatar/d7a973c7dab26985da5f961be7b74480?s=120&d=mm&r=g
Currently, it is the LCP on http://mathieu.e2e.rocketlabsqa.ovh/hello-world

from wp-rocket.

can you confirm that all elements that we should capture for LCP/ATF should always be URLs to a file of image type?

Yes, but that only applies to the collected src attribute. It needs to be a single image URL.

from wp-rocket.

@DahmaniAdame so this URL should not make it to the DB?
http://1.gravatar.com/avatar/d7a973c7dab26985da5f961be7b74480?s=120&d=mm&r=g on http://mathieu.e2e.rocketlabsqa.ovh/hello-world

from wp-rocket.

@MathieuLamiot this one should be accepted. It's a valid image.

The reason why I added .php and .js specifically as part of the exclusions is because they will process images on the fly without caching them.

If we preload them, the time needed to process will have negative effects on INP as well.

Anything else is fair game.

from wp-rocket.

@MathieuLamiot checking the mime is still applicable if we have a filter to allow URLs that are still valid but don't have an image mime type, like gravatar.com.

I would consider them more of an edge case and they will usually come from a specific provider. We can maintain a list of auto exclusions if needed.

That way, we are sure that it's a valid image.

from wp-rocket.

Done, now we have a filter to validate the image src

from wp-rocket.