set CSP header in FastBoot #113
Conversation
jelhan
force-pushed
the
fastboot-support
branch
2 times, most recently
from
8f4db5a to
0b9ee61
Compare
|
Need to check what is going on with the tests. They seem to be flickering. Added WIP. Sorry for the noise. |
|
|
||
| // reads addon config stored in meta element | ||
| function readAddonConfig(appInstance) { | ||
| let config = appInstance.resolveRegistration('config:environment'); |
There was a problem hiding this comment.
Doesn't this read the config from the "legacy" location (in config/environment.js)?
There was a problem hiding this comment.
You are right that it reads the run-time configuration, which is normally provided by consumer via config/environment.js. But in this case the configuration is meant to be provided by addon's config hook only. It's a subset of addons configuration. It only includes reportOnly option and the build policy string. Both are required at run-time for FastBoot support.
| @@ -19,11 +19,12 @@ | |||
| "start": "ember serve", | |||
| "test": "ember test", | |||
| "test:all": "ember try:each", | |||
| "test:node": "mocha node-tests/**" | |||
| "test:node": "for i in node-tests/*/*; do mocha $i; done" | |||
There was a problem hiding this comment.
There seems to be strange issues with ember-cli-addon-tests triggered only if multiple tests are executed at the same time and also depending on the order in which the tests are executed. While this is definitely not a good solution it gets us unblocked here.
There was a problem hiding this comment.
Reported this upstream: tomdale/ember-cli-addon-tests#215
There was a problem hiding this comment.
Eeck, seems fine but that sounds like it was pretty gnarly to track down.
jelhan
force-pushed
the
fastboot-support
branch
from
771d600 to
e5fe84a
Compare
|
This is ready to be merged if we accept the ugly hack for CI. There is one question still open, which I would love to get some feedback.
I'm tending towards no, cause we have the @rwjblue @sandstrom Do you have any input on that one? |
|
@jelhan I don't think it makes sense with dedicated config for disabling CSP under Fastboot. If that need arises, one could simply use the existing I know too little about fastboot to review the code though, sorry about that! |
|
@rwjblue Any objections merging this one? |
| @@ -19,11 +19,12 @@ | |||
| "start": "ember serve", | |||
| "test": "ember test", | |||
| "test:all": "ember try:each", | |||
| "test:node": "mocha node-tests/**" | |||
| "test:node": "for i in node-tests/*/*; do mocha $i; done" | |||
There was a problem hiding this comment.
Eeck, seems fine but that sounds like it was pretty gnarly to track down.
|
Added some documentation in 42b1e83. There isn't much to say about the default usage as this should just work. |
DELIVERY_HEADER and DELIVERY_META const wasn't used consistently. Also they increased the code length and didn't improved readability.
Let me give a quick overview of the architecture. Especially cause I'm not that used to FastBoot development and there might be problems that I have missed.
reportOnlyand the calculated policy string) as run-time configuration by returning it fromconfighook. Therefore the calculation was moved fromincludedhook toconfighook.fastbootservice.ember servenot only FastBoot but also the registered express middleware inserverMiddlewarehook sets CSP header. Test is usingfastboot-app-serverto prevent false-positives.There are some potential improvements, which would reduce bundle size:
ember-cli-fastbootas a dependency.Documented that ones as todos in code. To be honest I don't think that few bytes would make a real impact on performance. 😇
This lays the foundation for manipulation of the policy per request as required by #67 and jelhan/ember-style-modifier#11.
Open questions: