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.

Sign up for GitHub

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

fix: enable serving in separate process from the build #332

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+11 −2
Open

fix: enable serving in separate process from the build #332

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ module.exports = {
// FastBoot. This one is returned here as default configuration in order to make it
// available at run time.
config: function (environment, runConfig) {
if (process.env.DISABLE_CSP) {
Copy link

Choose a reason for hiding this comment

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

I had to return after setting _runConfig, because https://github.com/rwjblue/ember-cli-content-security-policy/blob/master/lib/utils.js#L96 was null (Cannot read properties of null (reading 'contentSecurityPolicy'))

this._runConfig = runConfig;

if (process.env.DISABLE_CSP) {
  return {};
}

return {};
}
debug('### Cache run-time config locally in config hook');

// store run config to be available later
Expand Down Expand Up @@ -101,6 +104,9 @@ module.exports = {
},

serverMiddleware: function ({ app: expressApp, options }) {
if (process.env.DISABLE_CSP) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason to have an environment variable to disable CSP in addon hard-coded? Having it in userland configuration would reduce complexity. A consumer would not need to look into addons documentation but only in project's CSP configuration to discover and understand the environment variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this allows libraries to easily turn it off via config when running the command vs needing to learn about and change config files

return;
}
debug('### Register middleware to set CSP headers in development server');

const requiresLiveReload = options.liveReload;
Expand All @@ -124,10 +130,13 @@ module.exports = {
// Use policy for test environment if both of these conditions are met:
// 1. the request is for tests and
// 2. the build include tests
let buildIncludeTests = this.app.tests;
const env = process.env.EMBER_ENV;
let buildIncludeTests = this.app
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we actually need to determine if build includes tests. Maybe it's enough to analyze if the request is for tests? Is there a case of legitim requests for /tests if app does not include tests?

Copy link
Contributor Author

@runspired runspired Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  • you can do a build and then serve tests off the build directly.
  • you can name a route tests :(

? this.app.tests
: process.env.USE_TESTS_CSP || false;
let isRequestForTests =
req.originalUrl.startsWith('/tests') && buildIncludeTests;
let environment = isRequestForTests ? 'test' : this.app.env;
let environment = isRequestForTests ? 'test' : env;

debug(
buildIncludeTests
Expand Down