Allow ssr cookies #1318

bredmond-sf · 2023-06-26T18:09:02Z

Description

SSR currently blocks setting cookies and strips them from requests because our CloudFront doesn't pass cookies. We are adding an environment setting to allow it in CloudFront and this allows the setting to disable the cookie blocking code from here.

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

Put setting in SSR code to disable cookie blocking
Add test endpoint to MRT app for cookies

How to Test-Drive This PR

Locally set MRT_ALLOW_COOKIES to true in env or configure localAllowCookies to true and then see that cookies work
Use /cookie?name=test&value=test in mrt app to check

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)

hajinsuha1

LGTM!

…into ssr-allow-cookies

bfeister

@bredmond-sf don't we want to test this in local dev / build-dev-server.js?

I was thinking about our recent conversation and realized that although we don't have .env.development you could still test this locally via SSR_ALLOW_COOKIES=true && npm start if you add the change there

bfeister

Also... @bredmond-sf you need to run npm run lint:fix from the monorepo root to get passing tests and fix your linting errors

hajinsuha1 · 2023-06-27T20:23:44Z

packages/pwa-kit-runtime/src/ssr/server/build-remote-server.js

@@ -138,6 +141,12 @@ export const RemoteServerFactory = {
        // This is the ORIGIN under which we are serving the page.
        // because it's an origin, it does not end with a slash.
        options.appOrigin = process.env.APP_ORIGIN = `${options.protocol}://${options.appHostname}`
+
+        if ('SSR_ALLOW_COOKIES' in process.env) {


superr nit: based on this discussion we could strip the ssr_ prefix everywhere and just use allow_cookies throughout the codebase

As per our Slack discussion, we'll want to use a reserved prefix here to avoid clashes with user provided environment variables.

A good name would MRT_ALLOW_COOKIES as the MRT prefix is reserved and there is emerging precedent for using it in the code.

johnboxall · 2023-06-27T23:13:41Z

packages/pwa-kit-runtime/src/ssr/server/build-remote-server.js

+            // Toggle cookies being passed and set
+            ssrAllowCookies: false


The impact of this setting, is that when set during local development, the development server will not strip cookies sent in HTTP requests and will not strip cookies set in HTTP responses.

I don't believe we need the ssr prefix here.

Is there a name we could use that suggests that this setting is only for local development and is not respected in MRT?

localAllowCookies would work for me. devAllowCookies could work but seems less clear to me.

johnboxall · 2023-06-27T23:17:42Z

packages/pwa-kit-runtime/src/ssr/server/build-remote-server.js

+
+        if ('SSR_ALLOW_COOKIES' in process.env) {
+            // Toggle cookies being passed and set
+            options.ssrAllowCookies = process.env.SSR_ALLOW_COOKIES?.toLowerCase() === 'true'


Nice guard. This is also the right typing here, because Lambda will pass us in a string.

johnboxall

Please address the naming of the env var and server setting, then we're in good shape to ...

🚢 🚢 🚢

packages/pwa-kit-runtime/src/ssr/server/build-remote-server.js

bfeister

Looks good 🚢

bredmond-sf · 2023-06-29T01:40:43Z

@bredmond-sf don't we want to test this in local dev / build-dev-server.js?

I was thinking about our recent conversation and realized that although we don't have .env.development you could still test this locally via SSR_ALLOW_COOKIES=true && npm start if you add the change there

@bfeister I dug a bit and understand this better. I'm using the mixin now in dev server

bredmond-sf · 2023-06-29T01:41:27Z

Please address the naming of the env var and server setting, then we're in good shape to ...

Allow ssr cookies #1318 (comment)

Allow ssr cookies #1318 (comment)

🚢 🚢 🚢

@johnboxall I think I've got this right now

bredmond-sf · 2023-06-29T01:42:10Z

Also... @bredmond-sf you need to run npm run lint:fix from the monorepo root to get passing tests and fix your linting errors

@bfeister thank you, done!

…into ssr-allow-cookies

hajinsuha1 · 2023-06-29T14:22:30Z

packages/pwa-kit-runtime/src/ssr/server/build-remote-server.js

@@ -899,6 +915,9 @@ export const RemoteServerFactory = {
     * contain both the certificate and the private key.
     * @param {function} customizeApp - a callback that takes an express app
     * as an argument. Use this to customize the server.
+     * @param {Boolean} [options.ssrAllowCookies] - This boolean value indicates


should this now be:

Suggested change

* @param {Boolean} [options.ssrAllowCookies] - This boolean value indicates

* @param {Boolean} [options.allowCookies] - This boolean value indicates

hajinsuha1 · 2023-06-29T14:22:42Z

packages/pwa-kit-runtime/src/ssr/server/build-remote-server.js

@@ -920,8 +939,9 @@ export const RemoteServerFactory = {
 * ExpressJS middleware that processes any non-proxy request passing
 * through the Express app.
 *
- * Strips Cookie headers from incoming requests, and configures the
- * Response so that it cannot have cookies set on it.
+ * If ssrAllowCookies is false, strips Cookie headers from incoming requests, and


Suggested change

* If ssrAllowCookies is false, strips Cookie headers from incoming requests, and

* If allowCookies is false, strips Cookie headers from incoming requests, and

hajinsuha1

🙌 🙌 🙌

hajinsuha1 · 2023-06-29T14:47:18Z

packages/template-mrt-reference-app/app/ssr.js

+/**
+ * Express handler that sets a simple cookie and returns a JSON response with
+ * diagnostic values.
+ */
+const cookieTest = async (req, res) => {


is it worth including a comment stating that you need to use /cookie?name=xxx&value=xxx to set the cookie?

absolutely, done

johnboxall · 2023-06-29T14:53:31Z

packages/template-mrt-reference-app/app/ssr.js

@bredmond-sf – thinking ahead, do you also need an endpoint that returns a "private" cache-control directive so that can we can confirm that not caching works correctly?

Good plan, done!

…into ssr-allow-cookies

kevinxh · 2023-06-29T21:08:27Z

packages/template-mrt-reference-app/app/ssr.js

+ * ?name=test-name&value=test-value to set a cookie.
+ */
+const cookieTest = async (req, res) => {
+    if (Object.hasOwn(req.query, 'name')) {


curious why did you choose to do Object.hasOwn instead of just

if (req.query.name) {

Allow ssr cookies

6be0716

bredmond-sf requested review from johnboxall, yunakim714 and hajinsuha1 June 26, 2023 18:09

bredmond-sf added 2 commits June 26, 2023 14:09

Remove debug

1d75f77

Fix env logic

3cbd369

hajinsuha1 previously approved these changes Jun 27, 2023

View reviewed changes

bredmond-sf added 2 commits June 27, 2023 10:59

Merge branch 'develop' of github.com:SalesforceCommerceCloud/pwa-kit …

dcb4e79

…into ssr-allow-cookies

Fix quotes

2fde41a

bredmond-sf dismissed hajinsuha1’s stale review via 2fde41a June 27, 2023 15:00

Add testing

7f2910d

bredmond-sf requested review from echessman and bfeister June 27, 2023 18:22

bredmond-sf marked this pull request as ready for review June 27, 2023 18:30

bredmond-sf requested a review from a team as a code owner June 27, 2023 18:30

Merge branch 'develop' of github.com:SalesforceCommerceCloud/pwa-kit …

7bec25c

…into ssr-allow-cookies

bredmond-sf requested review from olibrook, noahadams and bendvc June 27, 2023 18:33

bfeister reviewed Jun 27, 2023

View reviewed changes

hajinsuha1 reviewed Jun 27, 2023

View reviewed changes

johnboxall reviewed Jun 27, 2023

View reviewed changes

johnboxall previously approved these changes Jun 27, 2023

View reviewed changes

Add simple cookie test endpoint

d3d8a94

bredmond-sf dismissed johnboxall’s stale review via d3d8a94 June 28, 2023 01:24

Linting

bf04c6e

bfeister reviewed Jun 28, 2023

View reviewed changes

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

Update var names

cd01754

bfeister approved these changes Jun 29, 2023

View reviewed changes

Merge branch 'develop' of github.com:SalesforceCommerceCloud/pwa-kit …

583560f

…into ssr-allow-cookies

bfeister previously approved these changes Jun 29, 2023

View reviewed changes

hajinsuha1 reviewed Jun 29, 2023

View reviewed changes

hajinsuha1 previously approved these changes Jun 29, 2023

View reviewed changes

hajinsuha1 reviewed Jun 29, 2023

View reviewed changes

johnboxall reviewed Jun 29, 2023

View reviewed changes

Add private header to cookie endpoint

53f71dd

bredmond-sf dismissed stale reviews from hajinsuha1 and bfeister via 53f71dd June 29, 2023 15:49

bredmond-sf added 2 commits June 29, 2023 11:53

Add comments

467d0c7

Add max-age

97a4a12

hajinsuha1 self-requested a review June 29, 2023 16:28

hajinsuha1 previously approved these changes Jun 29, 2023

View reviewed changes

bredmond-sf requested a review from wjhsf June 29, 2023 16:30

Comment

68a8c8a

bredmond-sf dismissed hajinsuha1’s stale review via 68a8c8a June 29, 2023 18:35

Merge branch 'develop' of github.com:SalesforceCommerceCloud/pwa-kit …

aa0b754

…into ssr-allow-cookies

bredmond-sf requested a review from kevinxh June 29, 2023 18:58

hajinsuha1 approved these changes Jun 29, 2023

View reviewed changes

kevinxh reviewed Jun 29, 2023

View reviewed changes

kevinxh approved these changes Jun 29, 2023

View reviewed changes

johnboxall enabled auto-merge (squash) June 29, 2023 21:23

Merge branch 'develop' into ssr-allow-cookies

8733ed8

johnboxall merged commit e0a936f into develop Jun 30, 2023

wjhsf deleted the ssr-allow-cookies branch October 13, 2023 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ssr cookies #1318

Allow ssr cookies #1318

bredmond-sf commented Jun 26, 2023 •

edited

Loading

hajinsuha1 left a comment

bfeister left a comment

bfeister left a comment

hajinsuha1 Jun 27, 2023

johnboxall Jun 27, 2023

johnboxall Jun 27, 2023

bredmond-sf Jun 28, 2023

johnboxall Jun 27, 2023

johnboxall left a comment •

edited

Loading

bfeister left a comment

bredmond-sf commented Jun 29, 2023

bredmond-sf commented Jun 29, 2023

bredmond-sf commented Jun 29, 2023

hajinsuha1 Jun 29, 2023

hajinsuha1 Jun 29, 2023

hajinsuha1 left a comment

hajinsuha1 Jun 29, 2023

bredmond-sf Jun 29, 2023

johnboxall Jun 29, 2023

bredmond-sf Jun 29, 2023

kevinxh Jun 29, 2023

		// Toggle cookies being passed and set
		ssrAllowCookies: false

	* @param {Boolean} [options.ssrAllowCookies] - This boolean value indicates
	* @param {Boolean} [options.allowCookies] - This boolean value indicates

	* If ssrAllowCookies is false, strips Cookie headers from incoming requests, and
	* If allowCookies is false, strips Cookie headers from incoming requests, and

Allow ssr cookies #1318

Allow ssr cookies #1318

Conversation

bredmond-sf commented Jun 26, 2023 • edited Loading

Description

Types of Changes

Changes

How to Test-Drive This PR

Checklists

General

Accessibility Compliance

Localization

hajinsuha1 left a comment

Choose a reason for hiding this comment

bfeister left a comment

Choose a reason for hiding this comment

bfeister left a 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

johnboxall left a comment • edited Loading

Choose a reason for hiding this comment

bfeister left a comment

Choose a reason for hiding this comment

bredmond-sf commented Jun 29, 2023

bredmond-sf commented Jun 29, 2023

bredmond-sf commented Jun 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hajinsuha1 left a 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

bredmond-sf commented Jun 26, 2023 •

edited

Loading

johnboxall left a comment •

edited

Loading