-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat(mc-html-template): make transport security configurable #2244
Conversation
🦋 Changeset detectedLatest commit: eeb6322 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/DcuKXrH9RyxG12n8kYcxhAqFornQ |
a0f7e66
to
c0f2325
Compare
}, | ||
"strictTransportSecurity": { | ||
"description": "Additional configuration for the HTTP Strict-Transport-Security header (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Strict-Transport-Security)", | ||
"$ref": "#/definitions/hstsDirective" |
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.
Nit: a ref is only useful if you reuse the object within the schema. In this case you could just "inline" it.
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.
Not sure I recall 100% but inlining it might have produced different typings.
@@ -120,7 +120,9 @@ const processHeaders = (applicationConfig) => { | |||
); | |||
|
|||
return { | |||
'Strict-Transport-Security': 'max-age=31536000; includeSubDomains; preload', |
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.
So the preload
option is now NOT a default anymore? Any particular reason for that?
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.
The preload wasn't default before. The steps we took where while we only had max-age
:
- Add includeSubDomains and preload
- Reflect and consider making it configurable and hence remove the just added defaults
All cause we wanted to integrate this change slowly one app first. Now that it runs for a week or two we could remove the configuration option or change the default.
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.
Ah, I didn't see the other PR #2242 🤦♂️
Summary
Instead of changing the default let's make it configurable.