Closes #6599 validate image source before saving it into the DB #6605

wordpressfan · 2024-05-02T12:21:24Z

Description

Documentation

User documentation

We just add another layer of sanitizing and checking the valid lcp/atf to be valid images

Technical documentation

Here we will exclude the following patterns and more:

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%)

Type of change

Enhancement (non-breaking change which improves an existing functionality).

New dependencies

None

Risks

None

Checklists

Feature validation

I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
I triggered all changed lines of code at least once without new errors/warnings/notices.
I implemented built-in tests to cover the new/changed code.

Code style

I wrote self-explanatory code about what it does.
I wrote comments to explain why it does it.
I named variables and functions explicitely.
I protected entry points against unexpected inputs.
I did not introduce unecessary complexity.
I listed the introduced external dependencies explicitely on the PR.
I validated the repo-specific guidelines from CONTRIBUTING.md.

Observability

I handled errors when needed.
I wrote user-facing messages that are understandable and provide actionable feedbacks.
I prepared ways to observe the implemented system (logs, data, etc.).

Risks

I explicitely mentioned performance risks in the PR.
I explicitely mentioned security risks in the PR.

…the src url

piotrbak · 2024-05-16T09:54:45Z

Could we also validate the resource usage/time spent on this one?

codacy-production · 2024-05-24T12:28:54Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for `3877532`¹	✅ 85.00% (target: 50.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`3877532`)	Report Missing	Report Missing	Report Missing
Head commit (`7ec3c8e`)	37186	14358	38.61%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#6605)	20	17	85.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more}

Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

inc/Engine/Media/AboveTheFold/AJAX/Controller.php

tests/Unit/inc/Engine/Media/AboveTheFold/AJAX/Controller/addLcpData.php

inc/Engine/Media/AboveTheFold/AJAX/Controller.php

Mai-Saad · 2024-06-03T12:31:29Z

Reproducible for links here except for linear gradient case ... now we aren't adding those kind of URLs to DB. However, on this template, https://new.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testwebp .webp is atf and not LCP while in PSI it's LCP https://e2e.rocketlabsqa.ovh/lcp_6599_template2/ (needs further investigation)

Note: on trunk, webP is set as lcp (while we commented the BG image code but it isn't on branch)

jeawhanlee · 2024-06-03T19:16:05Z

@wordpressfan We have a regression here while running e2e tests, see here

MathieuLamiot · 2024-06-04T11:23:23Z

Manual tests 🆗 https://wp-media.slack.com/archives/CUKB44GNN/p1717491699283409?thread_ts=1717423732.490199&cid=CUKB44GNN
Rocket-E2E KO, need to retry after fixing #6693 (which is the root cause of the aforementioned regression)
Moving to blocked

Mai-Saad · 2024-06-05T05:44:06Z

The test template is fine after merging 6693 to the branch, lcp not found and webP is ATF https://new.rocketlabsqa.ovh/lcp_6599_template2/.

Note: visit the mobile version of the page using browserstack, is not added to DB (same on the trunk), visiting normal WP mobile page added normally to ATF table ===> In general further investigation is needed to make the template works

If e2e is green then we can merge

jeawhanlee · 2024-06-05T16:34:26Z

E2E Test Report: https://wp-media.slack.com/docs/T02RYMVKQ/F076AFQK40P

MathieuLamiot · 2024-06-05T17:36:48Z

@wordpressfan Is that expected that images from another hostname are filtered out with this PR?
If yes, then we have a lot of test failing because of this and a lot of clean up will be necessary on Rocket-E2E.
If not, then the report must be further investigated.

@wordpressfan There seems to be a regression, for instance on the template http://mike.e2e.rocketlabsqa.ovh/lcp_no_dimensions_picture, the LCP is 'not found' instead of /wp-content/rocket-test-data/images/lcp/testavif.avif and I don't think it is expected? Note that this image testavif.avif is filtered out on many templates, and it looks like its PHP that filters it out. I don't think this is expected. Can you check? Seems to happen with webp too sometimes (testwebp.webp)

@piotrbak I would advice not to move forward with this PR in 3.16.1 as the result is quite uncertain at this stage.

wordpressfan · 2024-06-05T17:50:48Z

We changed the structure of the data being saved into the database a bit in other PRs and this PR is validating based on the old structure, so for sure many things will fail and we need to check, that's why this unit test case is failing as an example.

codacy-production · 2024-06-06T17:39:17Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for `4dd69dd`¹	✅ 90.00% (target: 50.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`4dd69dd`)	Report Missing	Report Missing	Report Missing
Head commit (`fbc765e`)	37269	14439	38.74%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#6605)	20	18	90.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

inc/Engine/Media/AboveTheFold/AJAX/Controller.php

jeawhanlee · 2024-06-11T12:50:31Z

Ran e2e tests 2x and we are good for desktop, mobile scenario failures still the same as the one here

inc/Engine/Media/AboveTheFold/AJAX/Controller.php

wordpressfan · 2024-06-12T11:26:05Z

I think it's working now properly, thanks @jeawhanlee
I can't approve it because the PR is mine

jeawhanlee · 2024-06-12T11:29:58Z

I can't approve too because technically it's mine as well 😁

Tabrisrp

It's missing the minified version of the beacon script

Tabrisrp · 2024-06-12T13:21:05Z

Can you update the PR description to remove irrelevant data and complete the checklist?

jeawhanlee · 2024-06-13T11:12:35Z

I have run e2e on this branch again after merging develop into it and I get just one template failing on desktop but this template is also failing on dev, so no regression against dev here:

Expected Viewport for desktop - wp-content/rocket-test-data/images/lcp/testjpg.jpg for http://mike.e2e.rocketlabsqa.ovh/lcp_no_dimensions_picture is not present in actual - [{"type":"picture","src":"http://mike.e2e.rocketlabsqa.ovh/test.png","sources":[]},{"type":"picture","src":"http://mike.e2e.rocketlabsqa.ovh/wp-content/rocket-test-data/images/test3.gif","sources":[{"srcset":"http://mike.e2e.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testwebp.webp","media":"(max-width: 700px)","type":"","sizes":""},{"srcset":"http://mike.e2e.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testjpeg.jpeg","media":"(max-width: 450px)","type":"","sizes":""}]}]

MathieuLamiot · 2024-06-18T20:52:50Z

Next step to test this PR is to re-run E2E once wp-media/wp-rocket-e2e#88 is implemented.

…-validate-image-src

codacy-production · 2024-06-20T16:52:06Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for `5b286ce`¹	✅ 89.29% (target: 50.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`5b286ce`)	Report Missing	Report Missing	Report Missing
Head commit (`77f89a5`)	37309	14498	38.86%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#6605)	28	25	89.29%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{🚀 Don’t miss a bit, follow what’s new on Codacy.}

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

MathieuLamiot · 2024-06-20T17:07:55Z

E2E Results with E2E commit 7edc2237af34f62bf79185baa39023c8d83e0afd:

Expected Viewport for desktop - https://new.rocketlabsqa.ovh/file.php?url=img.jpg for http://mathieu.e2e.rocketlabsqa.ovh/lcp_6599_template2 is not present in actual - [{"type":"img","src":"https://new.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testwebp.webp"}]

Expected Viewport for mobile - https://new.rocketlabsqa.ovh/file.php?url=img.jpg for http://mathieu.e2e.rocketlabsqa.ovh/lcp_6599_template2 is not present in actual - [{"type":"img","src":"https://new.rocketlabsqa.ovh/wp-content/rocket-test-data/images/lcp/testwebp.webp"}]

According to the expected behavior here, I think it's OK and that the E2E must be adapted. Here is my suggestion for the PR to adapt: wp-media/wp-rocket-e2e#117

Tested with the E2E PR, it's all OK.

I'll move this to QA Done, and wait for QA team to come back to merge.

wordpressfan added 2 commits May 2, 2024 15:12

Just send elements with images (bg or src)

2b3eb92

Validate the image src to make sure it's a valid image

fe32023

wordpressfan self-assigned this May 2, 2024

wordpressfan added 2 commits May 2, 2024 17:01

refactor add_lcp_data nested part and change the logic to only check …

5026c47

…the src url

some more enhancements

7282c38

wordpressfan mentioned this pull request May 2, 2024

Guard beacon script against saving not expected values into the database #6599

Closed

add a filter for validating the image

f085a49

fix old tests and add more tests

d38508b

wordpressfan marked this pull request as ready for review May 24, 2024 12:24

wordpressfan requested a review from a team May 24, 2024 12:32

Merge branch 'refs/heads/develop' into fix/6599-validate-image-src

63d3ce7

Tabrisrp requested changes May 24, 2024

View reviewed changes

inc/Engine/Media/AboveTheFold/AJAX/Controller.php Outdated Show resolved Hide resolved

tests/Unit/inc/Engine/Media/AboveTheFold/AJAX/Controller/addLcpData.php Outdated Show resolved Hide resolved

Comply feedback

7ec3c8e

Miraeld approved these changes May 26, 2024

View reviewed changes

tests/Unit/inc/Engine/Media/AboveTheFold/AJAX/Controller/addLcpData.php Outdated Show resolved Hide resolved

inc/Engine/Media/AboveTheFold/AJAX/Controller.php Outdated Show resolved Hide resolved

Miraeld requested a review from Tabrisrp May 26, 2024 22:25

Tabrisrp approved these changes May 27, 2024

View reviewed changes

Merge branch 'develop' into fix/6599-validate-image-src

3764b34

fix most of unit tests

89038e3

wordpressfan added 2 commits June 6, 2024 19:53

Merge branch 'refs/heads/develop' into fix/6599-validate-image-src

c46e6ca

fix tests by supporting only src attribute if it's found

fbc765e

wordpressfan commented Jun 11, 2024

View reviewed changes

inc/Engine/Media/AboveTheFold/AJAX/Controller.php Outdated Show resolved Hide resolved

wordpressfan commented Jun 11, 2024

View reviewed changes

inc/Engine/Media/AboveTheFold/AJAX/Controller.php Outdated Show resolved Hide resolved

jeawhanlee added 3 commits June 11, 2024 13:33

Updated tests

54ff310

Updated controller

a393f39

phpcs fix

e34c587

jeawhanlee requested review from MathieuLamiot, Miraeld and Tabrisrp June 11, 2024 12:50

wordpressfan commented Jun 11, 2024

View reviewed changes

inc/Engine/Media/AboveTheFold/AJAX/Controller.php Outdated Show resolved Hide resolved

guard filter

3dae306

Tabrisrp requested changes Jun 12, 2024

View reviewed changes

Tabrisrp changed the title ~~3.16 - validate image source before saving it into the DB~~ Closes #6599 validate image source before saving it into the DB Jun 12, 2024

jeawhanlee added 2 commits June 13, 2024 10:41

Added minified version of beacon script

049ac4d

Merged develop into fix/6599-validate-image-src

0b80d3c

jeawhanlee requested review from jeawhanlee and Tabrisrp June 13, 2024 11:10

Miraeld approved these changes Jun 13, 2024

View reviewed changes

Merge commit '5b286ce8c571285bd0924f8f223440e7f9f4c0b4' into fix/6599…

77f89a5

…-validate-image-src

MathieuLamiot added this pull request to the merge queue Jun 28, 2024

Merged via the queue into develop with commit 3db0518 Jun 28, 2024
13 checks passed

MathieuLamiot deleted the fix/6599-validate-image-src branch June 28, 2024 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #6599 validate image source before saving it into the DB #6605

Closes #6599 validate image source before saving it into the DB #6605

wordpressfan commented May 2, 2024 •

edited by jeawhanlee

Loading

piotrbak commented May 16, 2024

codacy-production bot commented May 24, 2024 •

edited

Loading

Mai-Saad commented Jun 3, 2024 •

edited

Loading

jeawhanlee commented Jun 3, 2024 •

edited

Loading

MathieuLamiot commented Jun 4, 2024 •

edited

Loading

Mai-Saad commented Jun 5, 2024

jeawhanlee commented Jun 5, 2024

MathieuLamiot commented Jun 5, 2024

wordpressfan commented Jun 5, 2024

codacy-production bot commented Jun 6, 2024

jeawhanlee commented Jun 11, 2024

wordpressfan commented Jun 12, 2024

jeawhanlee commented Jun 12, 2024

Tabrisrp left a comment

Tabrisrp commented Jun 12, 2024

jeawhanlee commented Jun 13, 2024 •

edited

Loading

MathieuLamiot commented Jun 18, 2024

codacy-production bot commented Jun 20, 2024

MathieuLamiot commented Jun 20, 2024 •

edited

Loading

Closes #6599 validate image source before saving it into the DB #6605

Closes #6599 validate image source before saving it into the DB #6605

Conversation

wordpressfan commented May 2, 2024 • edited by jeawhanlee Loading

Description

Documentation

User documentation

Technical documentation

Type of change

New dependencies

Risks

Checklists

Feature validation

Code style

Observability

Risks

piotrbak commented May 16, 2024

codacy-production bot commented May 24, 2024 • edited Loading

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

Footnotes

Mai-Saad commented Jun 3, 2024 • edited Loading

jeawhanlee commented Jun 3, 2024 • edited Loading

MathieuLamiot commented Jun 4, 2024 • edited Loading

Mai-Saad commented Jun 5, 2024

jeawhanlee commented Jun 5, 2024

MathieuLamiot commented Jun 5, 2024

wordpressfan commented Jun 5, 2024

codacy-production bot commented Jun 6, 2024

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

Footnotes

jeawhanlee commented Jun 11, 2024

wordpressfan commented Jun 12, 2024

jeawhanlee commented Jun 12, 2024

Tabrisrp left a comment

Choose a reason for hiding this comment

Tabrisrp commented Jun 12, 2024

jeawhanlee commented Jun 13, 2024 • edited Loading

MathieuLamiot commented Jun 18, 2024

codacy-production bot commented Jun 20, 2024

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

Footnotes

MathieuLamiot commented Jun 20, 2024 • edited Loading

wordpressfan commented May 2, 2024 •

edited by jeawhanlee

Loading

codacy-production bot commented May 24, 2024 •

edited

Loading

Mai-Saad commented Jun 3, 2024 •

edited

Loading

jeawhanlee commented Jun 3, 2024 •

edited

Loading

MathieuLamiot commented Jun 4, 2024 •

edited

Loading

jeawhanlee commented Jun 13, 2024 •

edited

Loading

MathieuLamiot commented Jun 20, 2024 •

edited

Loading