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

fix: Honeypot field appears when CSP is enabled #7029

Merged
merged 11 commits into from Jan 5, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Dec 27, 2022

Description
From https://forum.codeigniter.com/showthread.php?tid=85960

  • fix bug
  • add Config\Honeypot::$containerId

How to Test

diff --git a/app/Config/App.php b/app/Config/App.php
index 0a23462b6..e8f314371 100644
--- a/app/Config/App.php
+++ b/app/Config/App.php
@@ -446,5 +446,5 @@ class App extends BaseConfig
      * @see http://www.html5rocks.com/en/tutorials/security/content-security-policy/
      * @see http://www.w3.org/TR/CSP/
      */
-    public bool $CSPEnabled = false;
+    public bool $CSPEnabled = true;
 }
diff --git a/app/Config/Filters.php b/app/Config/Filters.php
index 7b70c4fb3..12bb9619c 100644
--- a/app/Config/Filters.php
+++ b/app/Config/Filters.php
@@ -29,13 +29,13 @@ class Filters extends BaseConfig
      */
     public array $globals = [
         'before' => [
-            // 'honeypot',
+            'honeypot',
             // 'csrf',
             // 'invalidchars',
         ],
         'after' => [
             'toolbar',
-            // 'honeypot',
+            'honeypot',
             // 'secureheaders',
         ],
     ];
diff --git a/app/Controllers/Home.php b/app/Controllers/Home.php
index 7f867e95f..c451f5302 100644
--- a/app/Controllers/Home.php
+++ b/app/Controllers/Home.php
@@ -6,6 +6,6 @@ class Home extends BaseController
 {
     public function index()
     {
-        return view('welcome_message');
+        return '<head></head><form></form>';
     }
 }

You will see Fill This Field before this PR.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them 4.3 labels Dec 27, 2022

$config = new App();
$config->CSPEnabled = true;
Factories::injectMock('config', 'App', $config);
Copy link
Member Author

@kenjis kenjis Dec 27, 2022

Choose a reason for hiding this comment

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

Because CodeIgniter\HTTP\ContentSecurityPolicy depends on config('App')
(and Response pulls Services::csp()), we must inject Config\App into the Factories.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation! We can also just use the factory version here:

config('App')->CSPEnabled = true;

But nothing wrong with the explicit version.

Copy link
Member Author

@kenjis kenjis Dec 28, 2022

Choose a reason for hiding this comment

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

Factories are reset each time during tests, so there is probably no problem with that way.
However, I do not think it is healthy that most framework classes depend on Factories (config()).

It's not easy to get rid of this dependency now, but I would like to reduce it if possible.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Looks good! One typo.

You sent this to 4.3 - is that your vote that we release 4.3 next? I believe we're ready.

app/Config/Honeypot.php Outdated Show resolved Hide resolved

$config = new App();
$config->CSPEnabled = true;
Factories::injectMock('config', 'App', $config);
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation! We can also just use the factory version here:

config('App')->CSPEnabled = true;

But nothing wrong with the explicit version.

Co-authored-by: MGatner <mgatner@icloud.com>
@kenjis
Copy link
Member Author

kenjis commented Dec 28, 2022

You sent this to 4.3 - is that your vote that we release 4.3 next? I believe we're ready.

I sent to 4.3 because this needs a new Config item.

We merged two bug fixes into develop, so I would like to release v4.2.12 before v4.3.0.

@kenjis kenjis merged commit b86e62c into codeigniter4:4.3 Jan 5, 2023
@kenjis kenjis deleted the fix-honeypot-csp-bug branch January 5, 2023 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants