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

Allow CSP and SRI in UAT/test environments #24

Merged
merged 12 commits into from
Oct 5, 2022

Conversation

matt-in-a-hat
Copy link
Contributor

This makes it possible to configure the module such that CSP and SRI are enabled in test environments if desired. This reduces our chance of pushing code which is otherwise tested to production to then realise it gets blocked by CSP or the SRI isn't being calculated for some reason.

Fixes #22 including the issue of depending on a group named 'administrators' which can be renamed in the CMS.

---
Firesphere\CSPHeaders\View\CSPBackend:
csp_config:
enabled: false
Copy link
Owner

Choose a reason for hiding this comment

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

This would disable the headers in production...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's scoped to "Except" "Live", so should disable in all environments other than live.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, nevermind, I mis-read it due to the collapsing that GitHub does.

@@ -119,14 +119,14 @@ public function onBeforeInit()
{
/** @var ContentController $owner */
$owner = $this->owner;
$this->addPolicyHeaders = Director::isLive() || static::checkCookie($owner->getRequest());
$ymlConfig = CSPBackend::config()->get('csp_config');
$this->addPolicyHeaders = ($ymlConfig['enabled'] ?? false) || static::checkCookie($owner->getRequest());
Copy link
Owner

Choose a reason for hiding this comment

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

Please add some automated tests for these changes

@Firesphere
Copy link
Owner

Please rebase to fix the issues?

@Firesphere
Copy link
Owner

Could you please rebase this, so the automated tests will run?

@StephenMakrogianni
Copy link

@matt-in-a-hat bump 🙂

@wilr
Copy link
Contributor

wilr commented Sep 22, 2022

@Firesphere possible to get this re-based / merged on behalf of the poster. Be good to have this testable in test :)

@Firesphere
Copy link
Owner

@Firesphere possible to get this re-based / merged on behalf of the poster. Be good to have this testable in test :)

Maybe during the long weekend? I might... Or pull it in to this repo for a specific dev- tag?

@Firesphere
Copy link
Owner

Firesphere commented Oct 1, 2022

Okay, rebased, let's see where this leads :) (ping @wilr )

@Firesphere
Copy link
Owner

@matt-in-a-hat I've rebased and fixed up the tests (I think...)

Please give it a final sanity-check?

@codecov
Copy link

codecov bot commented Oct 1, 2022

Codecov Report

Base: 93.54% // Head: 94.39% // Increases project coverage by +0.84% 🎉

Coverage data is based on head (4855d55) compared to base (95918d4).
Patch coverage: 94.11% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##             master      #24      +/-   ##
============================================
+ Coverage     93.54%   94.39%   +0.84%     
- Complexity      161      163       +2     
============================================
  Files            12       12              
  Lines           403      410       +7     
============================================
+ Hits            377      387      +10     
+ Misses           26       23       -3     
Impacted Files Coverage Δ
src/Builders/SRIBuilder.php 83.33% <87.50%> (+3.33%) ⬆️
src/Builders/CSSBuilder.php 100.00% <100.00%> (+3.57%) ⬆️
src/Builders/JSBuilder.php 100.00% <100.00%> (+3.57%) ⬆️
src/Extensions/ControllerCSPExtension.php 85.00% <100.00%> (+1.39%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Firesphere
Copy link
Owner

Right, gonna merge this one in and see from there. Marking as a new beta release or something

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.

Allow applying CSP to non live environments
4 participants