-
Notifications
You must be signed in to change notification settings - Fork 212
W-21432256: Few follow up fixes to address Basket V2-V1 gaps and content security policy headers #3728
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.
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
W-21432256: Few follow up fixes to address Basket V2-V1 gaps and content security policy headers #3728
Changes from 6 commits
5494666
497a4c9
9a49666
79d79eb
8a44c05
7a17308
21c6f31
2cf865e
ccf6af6
52c3523
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,9 +33,14 @@ const options = { | |
| port: 3000, | ||
|
|
||
| // The protocol on which the development Express app listens. | ||
| // Set DEV_SERVER_PROTOCOL to 'https' for HTTPS; defaults to 'http' when unset. | ||
| // Note that http://localhost is treated as a secure context for development, | ||
| // except by Safari. | ||
| protocol: 'http', | ||
| protocol: process.env.DEV_SERVER_PROTOCOL || 'http', | ||
|
|
||
| // Optional. Path to SSL certificate (.pem) for HTTPS development. Typically a | ||
| // self-signed cert for localhost; set DEV_SERVER_SSL_FILE_PATH when using https. | ||
| sslFilePath: process.env.DEV_SERVER_SSL_FILE_PATH, | ||
|
|
||
| // Option for whether to set up a special endpoint for handling | ||
| // private SLAS clients | ||
|
|
@@ -346,7 +351,9 @@ const {handler} = runtime.createHandler(options, (app) => { | |
| // Default source for product images - replace with your CDN | ||
| '*.commercecloud.salesforce.com', | ||
| '*.demandware.net', | ||
| '*.adyen.com' // Payment gateways | ||
| '*.adyen.com', // Payment gateways | ||
| 'pay.google.com', // Google Pay payment handler icon | ||
| 'www.gstatic.com' // optional, if icon is on gstatic | ||
| ], | ||
| 'script-src': [ | ||
| // Used by the service worker in /worker/main.js | ||
|
|
@@ -378,8 +385,10 @@ const {handler} = runtime.createHandler(options, (app) => { | |
| '*.paypal.com', | ||
| 'pay.google.com', | ||
| 'payments.google.com', | ||
| 'google.com', | ||
| 'www.google.com' | ||
| 'google.com/pay', | ||
| 'google.com/pay/', | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are these both needed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added them based on what I saw in the browser for CSP errors
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When specifying a path for the CSP header, a trailing slash is significant. It means like matching the any requests that start with this path.
So I'm guessing |
||
| 'www.google.com/pay', | ||
| 'www.google.com/pay/' | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess same question. Do we need www. and google.com? Just want to confirm.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added them based on what I saw in the browser for CSP errors. It would help if someone else can run the checks during our testing phase or locally. We initially had *.google.com which is not safe but google pay seems to be depending on several url patterns |
||
| ], | ||
| 'frame-src': [ | ||
| // Allow frames from Salesforce site.com (Needed for MIAW) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joeluong-sfcc do we need the slash '/' at the end or not? Wondering why previously it had the slash at the end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amittapalli please add the trailing slash to all the query keys for consistency sake. Yes, functionally, it doesn't matter because query key is just a string (not url). But we have an existing pattern/convention, which only this file does not follow. It may be confusing later in the future for us and the end users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to consider a lint rule for this, if that's possible. Not for this case but for future development.