fix(history): handle sanitization when fetching versions #20212

markkaylor · 2024-04-25T15:35:04Z

What does it do?

Adjust history sanitization so it doesn't break the e2e tests
Build media and relations object in their own dedicated functions

How to test?

The e2e tests for history should pass (locally I have them passing as before they were broken, here in the CI they are sometimes flaky but I think it is unrelated)
History versions and their relations should not return sensitive data
Verify everything is working as normal (missing relations, restoration, etc...)
Admin user relations should not be completely sanitized out of the response

vercel · 2024-04-25T15:40:32Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
contributor-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 30, 2024 8:18am

packages/core/content-manager/server/src/history/controllers/history-version.ts

packages/core/content-manager/server/src/history/services/history.ts

packages/core/content-manager/admin/src/history/components/VersionInputRenderer.tsx

packages/core/content-manager/server/src/history/services/history.ts

Marc-Roig · 2024-04-29T12:55:26Z

I would rather not tie data population with sanitization if possible, and looking at this situation, would it be possible to keep the entry structure the same (without the results & meta object wrapping it) , and keeping a context of missing relations?

markkaylor · 2024-04-29T13:02:36Z

I would rather not tie data population with sanitization if possible, and looking at this situation, would it be possible to keep the entry structure the same (without the results & meta object wrapping it) , and keeping a context of missing relations?

I would be very interested in refactoring to find a better solution in an immediate follow up PR, but I was thinking of this PR as a patch to fix failing e2e tests currently on v5/main.

I also have this PR open that refactors history quite a bit: #20179 as well, so it might be nice to have that before making big changes here.

wdyt? Would you prefer I handle the refactor now on this PR?

Marc-Roig · 2024-04-29T13:27:10Z

sounds good to me to move this forward to fix the e2e 🥳

Marc-Roig · 2024-04-29T13:27:56Z

packages/core/content-manager/server/src/history/controllers/history-version.ts

+            createdBy: version.createdBy
+              ? pick(['id', 'firstname', 'lastname', 'username', 'email'], version.createdBy)
+              : undefined,


arent the creator fields already inside the version data?

We are omitting them as well 🤔 :

strapi/packages/core/content-manager/server/src/history/constants.ts

Line 2 in b5974b3

export const FIELDS_TO_IGNORE = [

do we need to omit it if we are readding it again here?

@remidej do you remember why we omit this in data and then return it here in the response?

packages/core/content-manager/server/src/history/controllers/history-version.ts

remidej · 2024-04-30T08:45:37Z

packages/core/content-manager/server/src/history/services/history.ts

                const relatedEntry = await strapi.db
                  .query('plugin::upload.file')
                  .findOne({ where: { id: entry.id } });

-                if (relatedEntry) {
-                  currentRelationData.results.push(relatedEntry);
+                const sanitizedEntry = await permissionChecker.sanitizeOutput(relatedEntry);


I wonder why you picked this in the end and not mainfield only population

Here for upload I don't think a mainField is available 🤔 but maybe it is. For the other instance with relations I did test it out but I think I abandoned it quite quickly when values weren't showing up as expected. I'll try it again real quick.

as you want I'm just curious, don't want to block you. And I doubt sanitization is needed on media assets

Sounds good, I might just merge this now then and circle back to the idea in another PR with some of the refactor ideas.

fix(history): handle sanitization when fetching versions

089818f

vercel bot deployed to Preview – contributor-docs April 25, 2024 15:40 View deployment

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 25, 2024 15:40 Failure

chore: filter user relations in the backend

0c5d32d

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 25, 2024 19:55 Failure

remidej reviewed Apr 25, 2024

View reviewed changes

packages/core/content-manager/server/src/history/controllers/history-version.ts Outdated Show resolved Hide resolved

packages/core/content-manager/server/src/history/services/history.ts Outdated Show resolved Hide resolved

vercel bot deployed to Preview – contributor-docs April 25, 2024 20:01 View deployment

Merge branch 'v5/main' into fix/history-tests

43332a9

vercel bot deployed to Preview – contributor-docs April 26, 2024 07:33 View deployment

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 26, 2024 07:33 Failure

Merge branch 'v5/main' into fix/history-tests

3e8c68b

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 26, 2024 12:21 Failure

vercel bot deployed to Preview – contributor-docs April 26, 2024 12:27 View deployment

markkaylor mentioned this pull request Apr 26, 2024

chore(tests): update snapshots #20214

Closed

chore: sanitze relations

e2d36b9

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 26, 2024 16:05 Failure

vercel bot deployed to Preview – contributor-docs April 26, 2024 16:11 View deployment

markkaylor added 2 commits April 29, 2024 10:05

chore: sanitize admin users

3963e39

chore: add type to async map

050e98c

vercel bot deployed to Preview – contributor-docs April 29, 2024 08:15 View deployment

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 29, 2024 08:15 Failure

markkaylor added 3 commits April 29, 2024 10:31

chore: rename variable

f248d50

fix: update types in tests

77891ff

Merge branch 'v5/main' into fix/history-tests

f96f7d5

vercel bot deployed to Preview – contributor-docs April 29, 2024 08:41 View deployment

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 29, 2024 08:41 Failure

markkaylor added 2 commits April 29, 2024 10:48

fix: protect frontned from undefined values

0d67fb8

fix: adjust types in test

171e3ac

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 29, 2024 08:52 Failure

vercel bot deployed to Preview – contributor-docs April 29, 2024 11:56 View deployment

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 29, 2024 11:56 Failure

remidej reviewed Apr 29, 2024

View reviewed changes

chore: pr feedback

38c0861

Marc-Roig reviewed Apr 29, 2024

View reviewed changes

packages/core/content-manager/server/src/history/services/history.ts Outdated Show resolved Hide resolved

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 29, 2024 12:49 Failure

Marc-Roig reviewed Apr 29, 2024

View reviewed changes

packages/core/content-manager/server/src/history/services/history.ts Show resolved Hide resolved

vercel bot deployed to Preview – contributor-docs April 29, 2024 12:55 View deployment

Marc-Roig reviewed Apr 29, 2024

View reviewed changes

packages/core/content-manager/server/src/history/controllers/history-version.ts Outdated Show resolved Hide resolved

fix: do not omit locale

b5974b3

vercel bot deployed to Preview – contributor-docs April 29, 2024 14:37 View deployment

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 29, 2024 14:37 Failure

chore: sanitize media

db27e41

vercel bot deployed to Preview – contributor-docs April 29, 2024 15:09 View deployment

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 29, 2024 15:09 Failure

fix: ensure proper history page navigation and await click events

5d0d00d

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 30, 2024 07:59 Failure

fix: prettify

a8a945f

vercel bot had a problem deploying to Preview – helper-plugin-storybook April 30, 2024 08:12 Failure

vercel bot deployed to Preview – contributor-docs April 30, 2024 08:18 View deployment

markkaylor requested review from remidej and Marc-Roig April 30, 2024 08:36

Marc-Roig approved these changes Apr 30, 2024

View reviewed changes

remidej approved these changes Apr 30, 2024

View reviewed changes

markkaylor merged commit 4a26739 into v5/main Apr 30, 2024
79 of 83 checks passed

markkaylor deleted the fix/history-tests branch April 30, 2024 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(history): handle sanitization when fetching versions #20212

fix(history): handle sanitization when fetching versions #20212

markkaylor commented Apr 25, 2024 •

edited

vercel bot commented Apr 25, 2024 •

edited

Marc-Roig commented Apr 29, 2024

markkaylor commented Apr 29, 2024

Marc-Roig commented Apr 29, 2024

Marc-Roig Apr 29, 2024

markkaylor Apr 29, 2024

Marc-Roig Apr 30, 2024

markkaylor Apr 30, 2024

remidej Apr 30, 2024

markkaylor Apr 30, 2024

remidej Apr 30, 2024

markkaylor Apr 30, 2024

fix(history): handle sanitization when fetching versions #20212

fix(history): handle sanitization when fetching versions #20212

Conversation

markkaylor commented Apr 25, 2024 • edited

What does it do?

How to test?

vercel bot commented Apr 25, 2024 • edited

Marc-Roig commented Apr 29, 2024

markkaylor commented Apr 29, 2024

Marc-Roig commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markkaylor commented Apr 25, 2024 •

edited

vercel bot commented Apr 25, 2024 •

edited