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.

Sign up for GitHub

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

Jump to bottom

[Shopper Experience] ImageWithText component #991

Merged
merged 20 commits into from
Feb 24, 2023
Merged

[Shopper Experience] ImageWithText component #991

merged 20 commits into from
Feb 24, 2023

Conversation

adamraya
Copy link
Collaborator

@adamraya adamraya commented Feb 16, 2023 •
edited
Loading

Description

The PR adds the Shopper Experience ImageWithText component to PWA Kit implementing the equivalent functionality to the same SFRA Page designer component.

SFRA Page designer UI uses a WYSIWYG for the Heading and Text Below Image fields. That means the API returns HTML for those fields.
The solution renders the HTML returned by the API in JSX using dangerouslySetInnerHTML with the string previously sanitized using the 3PP DOMPurify to prevent potential XSS attacks.

An alternative package sanitize-html was also considered and implemented in this commit but discarded since it increases the bundle size to 323.45 KB vs 272.23 KB bundle size using DOMPurify.

DOMPurify has the drawback that only works client-side.

3PP request
https://gus.lightning.force.com/lightning/r/ADM_Third_Party_Software__c/a0qEE0000006JUbYAM/view

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • Added ImageWithText component.

How to Test-Drive This PR

  1. Start the Retail React template
npm run start --prefix packages/template-retail-react-app/
  1. Navigate to url http://localhost:3000/experience
  2. Verify the component works as expected.
  3. Confirm the same SFRA functionality is implemented and tests pass.

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@adamraya adamraya marked this pull request as ready for review February 21, 2023 18:25
@adamraya adamraya requested a review from a team as a code owner February 21, 2023 18:25
@adamraya adamraya added the ready for review PR is ready to be reviewed label Feb 21, 2023
bendvc
bendvc reviewed Feb 22, 2023
View reviewed changes
packages/template-retail-react-app/app/components/experience/image-with-text/index.jsx Outdated
import React, {useEffect, useState} from 'react'
import PropTypes from 'prop-types'
import {Image, Box, Text, Link} from '@chakra-ui/react'
import DOMPurify from 'dompurify'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are currently using a library called serialize-javascript in the SDK when serializing the global variables into the returned html.

Do you think it's possible to use this library to achieve what we want here? Look at the section called Automatic Escaping of HTML Characters, you might be able to do something similar, and maybe this works on both server and client, and doesn't increase the bundle size. (Not sure if it will increase the client bundle since we are only using it in the server code, worth a shot tho)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like serialize-javascript doesn't do what we want as it serializes all the HTML tags.

Input:

<p><em>Text</em> <strong>Overlay</strong>

Output:

\u003Cp\u003E\u003Cem\u003EText\u003C\u002Fem\u003E \u003Cstrong\u003EOverlay\u003C\u002Fstrong\u003E

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. thanks for looking into that. I'm still a little hesitant on adding that additional library for this situation. In the past we have used a simple utility like this to sanitize things. I'm sure it could be altered to only escape script tags. (Remember that the data coming back to out knowledge is already sanitized, we'd have to clarify that tho, but from it's current implementation it is)

Copy link
Collaborator Author

@adamraya adamraya Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, I started using a RegEx to filter the HTML tags like in this commit f81d812, but soon I realized the sanitation problem is tricky and more complex than just removing the script tag.
Examples: attributes like onerror can be added to an img tag with js code or include js code in a href attribute.

adamraya and others added 7 commits February 22, 2023 15:47
@adamraya adamraya merged commit 572407b into develop Feb 24, 2023
@adamraya adamraya deleted the add-experience-image-with-text-component branch February 24, 2023 21:53
bfeister added a commit that referenced this pull request Feb 28, 2023
@bfeister
Merge branch 'develop' into spike/upgradeability
f323101 
* develop:
  Support Node 16 (#965)
  [W-12450361] Introduce short-circuit method for bypassing auth in Commerce Provider by passing in a fetchedToken (#1010)
  remove jest-silent-reporter (#1009)
  Clean up Page Designer Code (#1004)
  [Shopper Experience] `ImageWithText` component (#991)
  Feature: Page Designer Carousel Component (#977)
  [Feature] Page Designer Layout Components WIP (#993)
  remove updatePw from not implemented list (#996)
  Back-port Shopper Experience Base Components into Retail Template (#992)

# Conflicts:
#	packages/commerce-sdk-react/package-lock.json
#	packages/commerce-sdk-react/package.json
#	packages/pwa-kit-dev/package-lock.json
#	packages/pwa-kit-dev/src/configs/webpack/config.js
#	packages/template-retail-react-app/package-lock.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bendvc bendvc bendvc approved these changes

Assignees

@bendvc bendvc

Labels
ready for review PR is ready to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adamraya @bendvc