@W-14446652@ Change CSP enforcement to an opt-in middleware. #1528

wjhsf · 2023-11-07T17:30:26Z

In v3.2.0, to facilitate adoption of Storefront Preview, we introduced logic to enforce a Content-Security-Policy header containing all the directives that are required for PWA Kit / Storefront Preview to function. However, while making CSP management easier for those who use it, this is a breaking change for users who explicitly do not use the Content-Security-Policy header. 🤕 Oops.

This PR simply removes the middleware function from the internal logic of pwa-kit-runtime and exports it so that end users must opt in to using the function.

Description

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

(change1)

How to Test-Drive This PR

Check out v3.2.0, launch the app, save the CSP header.
Check out this code, launch the app. CSP header should be the same.

Remove the two security header middleware functions. Restart the app. CSP header should not be present.

pwa-kit/packages/template-retail-react-app/app/ssr.js

Lines 38 to 60 in 3582944

    
           app.use(defaultSecurityHeaders) 
        
           // Set custom HTTP security headers 
        
           app.use( 
        
               helmet({ 
        
                   contentSecurityPolicy: { 
        
                       useDefaults: true, 
        
                       directives: { 
        
                           'img-src': [ 
        
                               // Default source for product images - replace with your CDN 
        
                               '*.commercecloud.salesforce.com' 
        
                           ], 
        
                           'script-src': [ 
        
                               // Used by the service worker in /worker/main.js 
        
                               'storage.googleapis.com' 
        
                           ], 
        
                           'connect-src': [ 
        
                               // Connect to Einstein APIs 
        
                               'api.cquotient.com' 
        
                           ] 
        
                       } 
        
                   } 
        
               }) 
        
           )

Bonus fun: move the defaultSecurityHeaders middleware to after the helmet middleware and see that it doesn't change the CSP!

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...

Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
Changes comply with WCAG 2.0 guidelines levels A and AA
Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

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

kevinxh · 2023-11-07T18:06:36Z

packages/template-retail-react-app/app/ssr.js

@@ -34,11 +34,11 @@ const options = {
 const runtime = getRuntime()

 const {handler} = runtime.createHandler(options, (app) => {
-    // Set HTTP security headers
+    // Use default HTTP security headers required by PWA Kit
+    app.use(defaultSecurityHeaders)


What does "default" mean in this context? 🤔 we might want to be a bit more specific and call it likestorefrontPreviewSecurityHeaders(prob also a bad name) ?

By "default" I just mean "mandatory for PWA Kit to work". So it's not just Storefront Preview, even though that was the motivation for the change. And they're not strictly required if a project is using pwa-kit-runtime for something other than the retail react app.

Naming things is hard. 🤔

If I'm a developer, I would want to know the benefit of opting in defaultSecurityHeaders

The answer is: you can't use storefront preview on runtime admin... <- this might not be clear to end users.

wait... i think i might have misunderstanding...

if it is "mandatory for PWA Kit to work", why do we make it an opt-in?

Because some users don't want to have any CSP header, and @johnboxall said we should make it opt-in, rather than opt-out.

On naming: https://chat.openai.com/share/d5a18a26-08b3-47ea-a1ac-952dee512285

I don't have strong feelings, but the name should communicate that a) it is a express.js middleware [can also be implied from import] b) it sets security related headers required for using the PWA Kit

Yes, folks should be allowed to opt into these headers – LWR sites as an example don't want the PWA Kit CSP.

Also, some customers struggle to create a CSP. They're hard. Sometimes letting them go is the only way they can truly be free.

My vote is on adding 'pwakit' to the name, like defaultPwaKitSecurityHeaders, so that it's more distinguishable. Because helmet also has a similar concept of defaults (see useDefaults on line 43).

+1 agree defaultPwaKitSecurityHeaders is preferable

kevinxh · 2023-11-07T18:10:37Z

packages/template-retail-react-app/app/ssr.js

@@ -34,11 +34,11 @@ const options = {
 const runtime = getRuntime()

 const {handler} = runtime.createHandler(options, (app) => {


The behavior of enabling default headers seems more natural to me if it’s a configuration value…

const options = { ..., enableDefaultSecurityHeaders: true }

packages/pwa-kit-runtime/src/ssr/server/express.js

johnboxall · 2023-11-07T18:31:10Z

packages/pwa-kit-runtime/src/ssr/server/express.js

@@ -460,3 +463,102 @@ export const getRuntime = () => {

    return boundRuntime
 }
+
+/**
+ * Patches `res.setHeader` to ensure that the Content-Security-Policy header always includes the


This comment describes part of what this function does:

It is an express.js middleware that sets default security headers including CSP + HSTS for PWA Kit sites.

It patches res.setHeader to include defaults normally required for PWA Kit operation.

packages/pwa-kit-runtime/src/ssr/server/express.js

packages/pwa-kit-runtime/CHANGELOG.md

vmarta · 2023-11-07T20:24:25Z

packages/template-retail-react-app/app/ssr.js

@@ -34,11 +34,11 @@ const options = {
 const runtime = getRuntime()

 const {handler} = runtime.createHandler(options, (app) => {
-    // Set HTTP security headers
+    // Use default HTTP security headers required by PWA Kit
+    app.use(defaultSecurityHeaders)


My vote is on adding 'pwakit' to the name, like defaultPwaKitSecurityHeaders, so that it's more distinguishable. Because helmet also has a similar concept of defaults (see useDefaults on line 43).

bfeister

I think @johnboxall's point about extending the existing patch and then the PWA Kit namespacing of the mixin variable need some resolution before we merge this

vmarta · 2023-11-07T22:07:13Z

@wjhsf Let's review what the target branch should be for this PR. If we want this to be a hot fix, then we'll want to merge it into the release-3.2.x branch instead.

packages/template-retail-react-app/app/ssr.js

* Declare constant in file that uses it. * Update CHANGELOGs. * Move CSP middleware to utils. * Naming things is hard. * Renaming things is hard. * Don't touch files that don't need to be touched. * Re-delete moved code. * Remove outdated comment.

wjhsf requested a review from a team as a code owner November 7, 2023 17:30

wjhsf requested review from bfeister and kevinxh November 7, 2023 17:30

kevinxh reviewed Nov 7, 2023

View reviewed changes

kevinxh previously approved these changes Nov 7, 2023

View reviewed changes

johnboxall reviewed Nov 7, 2023

View reviewed changes

packages/pwa-kit-runtime/src/ssr/server/express.js Outdated Show resolved Hide resolved

johnboxall reviewed Nov 7, 2023

View reviewed changes

packages/pwa-kit-runtime/src/ssr/server/express.js Outdated Show resolved Hide resolved

vmarta reviewed Nov 7, 2023

View reviewed changes

bfeister requested changes Nov 7, 2023

View reviewed changes

wjhsf dismissed kevinxh’s stale review via 13d22b0 November 7, 2023 21:00

wjhsf requested review from bfeister, vmarta and kevinxh November 7, 2023 22:06

wjhsf changed the base branch from develop to release-3.2.x November 7, 2023 22:10

bfeister approved these changes Nov 7, 2023

View reviewed changes

wjhsf added 5 commits November 7, 2023 17:14

Declare constant in file that uses it.

200861c

Update CHANGELOGs.

39d6c59

Move CSP middleware to utils.

9e69bd6

Naming things is hard.

f615010

Renaming things is hard.

44749e2

wjhsf force-pushed the wjh/v3-fix-csp branch from f1995f0 to 44749e2 Compare November 7, 2023 22:16

wjhsf added 2 commits November 7, 2023 17:18

Don't touch files that don't need to be touched.

1bd6515

Re-delete moved code.

f045e2b

vmarta reviewed Nov 7, 2023

View reviewed changes

packages/template-retail-react-app/app/ssr.js Outdated Show resolved Hide resolved

Remove outdated comment.

94630ab

wjhsf requested a review from vmarta November 7, 2023 22:42

vmarta approved these changes Nov 7, 2023

View reviewed changes

bfeister approved these changes Nov 7, 2023

View reviewed changes

wjhsf merged commit 6a614ba into release-3.2.x Nov 7, 2023
20 checks passed

wjhsf deleted the wjh/v3-fix-csp branch November 7, 2023 23:25

wjhsf mentioned this pull request Nov 8, 2023

@W-14446652@ v2 Change CSP enforcement to an opt-in middleware. #1530

Merged

12 tasks

vmarta mentioned this pull request Nov 8, 2023

Hotfix SDK 3.2.1 - @W-14446652@ #1532

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@W-14446652@ Change CSP enforcement to an opt-in middleware. #1528

@W-14446652@ Change CSP enforcement to an opt-in middleware. #1528

wjhsf commented Nov 7, 2023

kevinxh Nov 7, 2023 •

edited

wjhsf Nov 7, 2023

kevinxh Nov 7, 2023 •

edited

kevinxh Nov 7, 2023

wjhsf Nov 7, 2023

johnboxall Nov 7, 2023

vmarta Nov 7, 2023

bfeister Nov 7, 2023

kevinxh Nov 7, 2023 •

edited

johnboxall Nov 7, 2023

vmarta Nov 7, 2023

bfeister left a comment

vmarta commented Nov 7, 2023

	app.use(defaultSecurityHeaders)
	// Set custom HTTP security headers
	app.use(
	helmet({
	contentSecurityPolicy: {
	useDefaults: true,
	directives: {
	'img-src': [
	// Default source for product images - replace with your CDN
	'*.commercecloud.salesforce.com'
	],
	'script-src': [
	// Used by the service worker in /worker/main.js
	'storage.googleapis.com'
	],
	'connect-src': [
	// Connect to Einstein APIs
	'api.cquotient.com'
	]
	}
	}
	})
	)

		@@ -34,11 +34,11 @@ const options = {
		const runtime = getRuntime()

		const {handler} = runtime.createHandler(options, (app) => {

@W-14446652@ Change CSP enforcement to an opt-in middleware. #1528

@W-14446652@ Change CSP enforcement to an opt-in middleware. #1528

Conversation

wjhsf commented Nov 7, 2023

Description

Types of Changes

Changes

How to Test-Drive This PR

Checklists

General

Accessibility Compliance

Localization

kevinxh Nov 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinxh Nov 7, 2023 • edited

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

kevinxh Nov 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfeister left a comment

Choose a reason for hiding this comment

vmarta commented Nov 7, 2023

kevinxh Nov 7, 2023 •

edited

kevinxh Nov 7, 2023 •

edited

kevinxh Nov 7, 2023 •

edited