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

WIP: ease usage with ember-cli-content-security-policy #345

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jelhan
Copy link
Contributor

@jelhan jelhan commented Feb 6, 2021

Currently all users of Ember CLI Content Security Policy must explicitly set forbidEval to true in Ember Auto Import's configuration. Unless the very rare case that they allow "unsafe-eval" in style-src directive.

This is disruptive for users of both sides. Since Ember CLI blueprints for new application includes Ember Auto Import and many addons depend on it as well nearly all user of Ember CLI Content Security Policy must set that configuration. It does not help with adoption of CSP or testing for CSP compliance in Ember ecosystem.

This pull request propose to change the default value to depend on the presence of ember-cli-content-security-policy depenendency. If the application depends on ember-cli-content-security-policy forbidEval defaults to true. Otherwise it is false.

I would not consider this as a breaking change. At least I'm not aware of a meaningful use case to not having forbidEval set to true if CSP is used.

@jelhan jelhan changed the title ease usage with ember-cli-content-security-policy WIP: ease usage with ember-cli-content-security-policy Feb 6, 2021
@jelhan
Copy link
Contributor Author

jelhan commented Feb 6, 2021

Not fully sure why dependencySatisfies macro from @embroider/macros fails. But before digging deeper into it I would appreciate some feedback if you are open to accept such a change.

@ef4
Copy link
Collaborator

ef4 commented Feb 12, 2021

Thanks, I like this.

dependencySatisfies only asks about your own dependencies, because you can only reliably access your own dependencies (without depending on accidental implementation details that we don't want to lock in forever).

In a case like this one, ember-auto-import can declare an optional peer dependency on ember-cli-content-security-policy. The benefit of requiring this is that it makes it clear to package managers that you want access to the same copy of that library that is present in your parent package, if it happens to be there.

@jelhan
Copy link
Contributor Author

jelhan commented Feb 12, 2021

I added ember-cli-content-security-policy as an optional peer dependency in 29d4183. But tests are still failing. Not sure if I done something wrong. But I'm wondering if @embroider/macros babel plugin is running at all. I would have expected it to fail when running the babel plugin. And not silently skipping the code and than throwing at execution time.

I can refactor to classical methods to determine if the consuming application has the dependency. I was mostly using @embroider/macros to demonstrate the concept. Haven't checked if this specific usage is supported at all, if targeted Ember versions are matching, etc.

@jelhan
Copy link
Contributor Author

jelhan commented Mar 3, 2021

@ef4 Would it be an acceptable path forward for you to not use @embroider/macros but a classic strategy like ember-cli-version-checker?

@jelhan jelhan force-pushed the ease-usage-with-ember-cli-content-security-policy branch from 29d4183 to 536ad5f Compare October 28, 2021 23:31
@jelhan jelhan force-pushed the ease-usage-with-ember-cli-content-security-policy branch from 536ad5f to fb36452 Compare October 28, 2021 23:33

// fallback to default value, which should be `true` unless application
// has ember-cli-content-security-policy dependency
return !dependencySatisfies('ember-cli-content-security-policy', '*');
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't want this to ever return true when this.isAddon is true, because addons don't get to decide this. Only the Package instance representing the app should decide to maybe return true.

I think the difficulty you're having with @embroider/macros is that they're designed to go in runtime code, not node code. This is node code. Instead, take a look in this same file where we're checking the version of ember-source that the app is using. You can check whether the app is using ember-cli-content-security-policy the same way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants