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.

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

Avoid merging policies in build time configuration #101

Conversation

jelhan
Copy link
Collaborator

@jelhan jelhan commented Jul 3, 2019

Should be backward-compatible as it still merges the policy object set by contentSecurityPolicy runtime config.

Closes #99

@jelhan jelhan mentioned this pull request Jul 5, 2019
10 tasks
@jelhan
Copy link
Collaborator Author

jelhan commented Jul 5, 2019

@sandstrom This is ready to be reviewed and merged from my side.

@jelhan jelhan mentioned this pull request Jul 8, 2019
@jelhan
Copy link
Collaborator Author

jelhan commented Jul 13, 2019

@rwjblue Seems like sandstorm is busy. Would be great if you have some time for a review.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

LGTM, this will be a major bump, right (since it changes merging behaviors)?

@jelhan
Copy link
Collaborator Author

jelhan commented Jul 15, 2019

LGTM, this will be a major bump, right (since it changes merging behaviors)?

It only changes merging behavior for policies that are defined using new configuration but not for legacy configuration. There wasn't any release yet that includes new configuration. So it should be backward compatible. There is also a test that legacy configuration is merged, which isn't touched by this PR: https://github.com/rwjblue/ember-cli-content-security-policy/blob/2266d1a7bba293af4330459b0ffb9c94a3042fee/node-tests/unit/config-test.js#L53-L74

@jelhan
Copy link
Collaborator Author

jelhan commented Jul 15, 2019

Please have a look at #100 for a proposal for next release. The main idea is to use the altered configuration interface as a backward-compatible toggle for new defaults. It lists the changes that should be bundled together to prevent the need of a breaking change.

@rwjblue
Copy link
Member

rwjblue commented Jul 25, 2019

It only changes merging behavior for policies that are defined using new configuration but not for legacy configuration. There wasn't any release yet that includes new configuration. So it should be backward compatible.

Awesome, thank you. I'm sorry I missed that.

@rwjblue rwjblue changed the title policy object should not be merged Avoid merging policies in build time configuration Jul 25, 2019
@rwjblue rwjblue merged commit 98f9022 into adopted-ember-addons:master Jul 25, 2019
@jelhan jelhan mentioned this pull request Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Policy object should not be merged but replaced by application configuration
2 participants