Honeypot Filter #1063

dvdnwoke · 2018-06-03T08:05:17Z

Implementation of bot Honeypot as discussed on this issue

…honeypot

lonnieezell

Good start, but there's a few things that need to be changed up to fit with the rest of the framework. Thanks!

lonnieezell · 2018-06-05T04:04:01Z

system/Honeypot/Honeypoter.php

+use CodeIgniter\HTTP\ResponseInterface;
+use Config\Honeypot;
+
+class Honeypoter 


Let's rename this class/file to simply Honeypot.

lonnieezell · 2018-06-05T04:09:19Z

system/Honeypot/Honeypoter.php

+            new self(): self::$selfObject;
+
+        // TODO Will there be need to protect against bad data?
+        if($request->getVar(self::$selfObject->name)){


According to our style guide the opening brackets should be on a line of their own.

lonnieezell · 2018-06-05T04:13:56Z

application/Filters/Honeypot.php

+    {
+
+        // Checks honeypot field if value was entered then show blank if so.
+        if(Honeypoter::honeypotHasContent($request))


To maintain consistency with the rest of the framework, this should not be a class with static methods. Instead, if should be instantiated with methods called non-statically. Since you need to inject a config class as a dependency, you should add a honeypot method to CodeIgniter\Services and use the service to get the instance. Like:

$honeypot = Services::honeypot(); if ($honeypot->hasContent($request) { ... }

lonnieezell · 2018-06-05T04:15:15Z

system/Honeypot/Honeypoter.php

+        self::$selfObject = (self::$selfObject === null) ? 
+            new self(): self::$selfObject;
+
+        // TODO Will there be need to protect against bad data?


No need to protect the data since it's not be displayed or saved anywhere. Please remove the comment.

lonnieezell · 2018-06-05T04:16:12Z

system/Honeypot/Honeypoter.php

+            return true;
+        }
+
+        if($request->getGet(self::$selfObject->name)){


This check and the one from getPost are redundant. getVar already checks both of those arrays.

lonnieezell · 2018-06-05T04:20:44Z

system/Honeypot/Honeypoter.php

+	 */
+    protected function getStyle(): string
+    {
+        return '<script type="text/css" media="all">


This isn't a script tag - it should be a style tag. And I think this might be overkill, actually. While it's good, and thorough, the developer should I think keeping a default of hidden in the config file is good enough. The developer can change as needed at that point. They'll notice the honeypot fields showing up during testing.

lonnieezell · 2018-06-05T04:21:11Z

system/Honeypot/Honeypoter.php

+	 */
+    protected function getDefaultTemplate(): string
+    {
+        return '<div class="hidden" style="display:none">


Please move the default values to the config file. Easier for the developer spot/change that way.

lonnieezell · 2018-06-05T04:21:33Z

system/Honeypot/Honeypoter.php

+	 */
+    protected function getDefaultLabel(): string
+    {
+        return 'Fill This Field';


Please move to config file.

lonnieezell · 2018-06-05T04:21:39Z

system/Honeypot/Honeypoter.php

+	 */
+    protected function getDefaultName(): string
+    {   
+        return 'honeypot';


Please move to config file.

lonnieezell · 2018-06-05T04:22:19Z

tests/system/Honeypot/HoneypotTest.php

+    public function setUp()
+    {
+        parent::setUp();
+        $this->request = new IncomingRequest(new App(),


This is better done by pulling it from the Services class:

$this->request = Services::request(); ``

jim-parry · 2018-06-05T07:22:40Z

For future reference, we expect commits to be GPG-signed :-/

jim-parry · 2018-06-05T15:58:25Z

You are not synchronizing properly. You would synch your develop branch locally, then merge your develop into your honeypot branch.
As it sits, you are trying to re-merge the most recent changes that were merged into the main repo, leaving all sorts of inaoorioriate commits in your PR :-/

dvdnwoke · 2018-06-05T20:18:14Z

@jim-parry the issue with the merge has been solve sorry for that

lonnieezell

Looking better - a few more small items noted. Also - will need some form of basic docs in place, please. We can always expand them later if needed, but a start would be appreciated.

lonnieezell · 2018-06-08T04:07:09Z

application/Config/Honeypot.php

+	 * 
+	 * @var boolean
+	 */
+	public $hidden = '';


Please add the default values in this file for all values.

lonnieezell · 2018-06-08T04:09:51Z

application/Config/Services.php

 //        return new \CodeIgniter\Example();
 //    }

+    public static function honeypot($getShared = true)


This should take an instance of Config\Honeypot as an optional parameter. Something like:

public static function honeypot(BaseConfig $config = null, $getShared = true) { if ($getShared) { ... } if (is_null($config)) { $config = new Config\Honeypot(); } return new CodeIgniter\Honeypot\Honeypot($config); }

lonnieezell · 2018-06-08T04:10:51Z

system/Honeypot/Honeypot.php

+
+    //--------------------------------------------------------------------
+
+    function __construct () {


should accept an instance of the BaseConfig in the parameter instead of creating an instance in side the header. Makes it more robust and easier to test.

lonnieezell · 2018-06-08T04:12:24Z

application/Filters/Honeypot.php

+        $honeypot = Services::honeypot();
+        if($honeypot->hasContent($request))
+        {
+            die();


This should throw an exception with an explanatory method. Then devs have something they can catch and provide a better experience for users. In the near future, the exception handler in the framework will be upgraded to allow custom handlers.

I know die() is not a good option but never knew what to replace it with.
Thanks for that suggestion.

lonnieezell · 2018-06-08T04:15:09Z

application/Filters/Honeypot.php

+    public function after (RequestInterface $request, ResponseInterface $response) 
+    {
+        $honeypot = Services::honeypot();
+        $honeypot->attachHoneypot($response);


Pretty sure uou need to return the modified $response object here in order for it to work....

@lonnieezell You are saying
$response = $honeypot->attachHoneypot($response);
should return the modified response?

Correct. Unless it's working for you as it is. But I'm pretty sure you need to return the modified response for the change to stick.

It's working as it is. But do you feel returning it would be better?

Nah - if it's working, that's cool. I might have been remembering it wrong. I built it then moved on and haven't had a chance to use it much just yet. :)

lonnieezell · 2018-06-08T04:15:58Z

env

+# HONEYPOT
+#--------------------------------------------------------------------
+
+#  honeypot.hidden = 'true'


I don't have an issue with them being here - but need to make sure to include default values in the config file. Any values in a .env file will overwrite those, so it all works as it should.

lonnieezell · 2018-06-08T04:17:22Z

system/Honeypot/Honeypot.php

+	 * Self Instance of Class
+	 * @var Honeypot
+	 */
+    protected $honeypotConfig;


I'm nit-picking here, but why use honeypot in the name? It's in a class named Honeypot already, so I'd go with the simpler $config personally.

dvdnwoke · 2018-06-08T14:03:25Z

@lonnieezell

Also - will need some form of basic docs in place, please. We can always expand them later if needed, but a start would be appreciated.

Do you mean documentation on the class or what? Please explain further.

lonnieezell · 2018-06-08T14:13:22Z

@dvdnwoke

Do you mean documentation on the class or what? Please explain further.

Yes, some documentation for the class. It would go in this folder. You should be able to look at any of the files in that area for an example. You'll also need to add it to the index.rst file that's in that directory.

If you have any questions feel free to ask!

dvdnwoke · 2018-06-08T19:09:25Z

application/Config/Services.php

    {
-        if ($getShared)
-        {   
-            return self::getSharedInstance('honeypot');


This line was removed, Because after the changes it keeps throwing error.

And i don't know the actual function of that line. If it's needed just suggest to me how to add it.

Yeah - it is needed. That allows the instance to be cached and shared among all classes. The problem is that you need to pass the $config param into it. Basically, any params that get passed in the main function need to be passed into the getSharedInstance method:

return self::getSharedInstance('honeypot', $config);

dvdnwoke · 2018-06-08T19:13:50Z

@lonnieezell @jim-parry

For future reference, we expect commits to be GPG-signed :-/

Can you advice me on how to add this?

jim-parry · 2018-06-08T19:20:41Z

@dvdnwoke https://bcit-ci.github.io/CodeIgniter4/contributing/index.html
Specifically, https://bcit-ci.github.io/CodeIgniter4/contributing/signing.html

Signed-off-by: Nwoke David Udoka <[email protected]>

dvdnwoke · 2018-06-09T12:04:39Z

@lonnieezell I think everything is ok here. If there is any more changes just let me know am available.

lonnieezell · 2018-06-12T04:08:26Z

Looks pretty good, @dvdnwoke, thanks! There are a couple of small things, but I can tweak those later.

dvdnwoke added 2 commits June 3, 2018 08:10

Honeypot Implementation

a12c409

Merge branch 'develop' of git://github.com/bcit-ci/CodeIgniter4 into …

90a0ff2

…honeypot

dvdnwoke changed the title ~~Honeypot~~ Honeypot Filter Jun 3, 2018

lonnieezell requested changes Jun 5, 2018

View reviewed changes

Honeypot Implementation

8e0c7d4

Merge branch 'develop' into honeypot

c4dc0d8

dvdnwoke force-pushed the honeypot branch from 6c6102e to c4dc0d8 Compare June 5, 2018 20:15

Honeypot Implementation - added honeypot config to env

696227d

lonnieezell requested changes Jun 8, 2018

View reviewed changes

dvdnwoke added 2 commits June 8, 2018 19:46

Honeypot Implementation - Modification with Documentation

3d04f1b

Merge branch 'develop' into honeypot

84bbbcc

dvdnwoke commented Jun 8, 2018

View reviewed changes

dvdnwoke added 2 commits June 9, 2018 12:48

Correted typo error

faa0089

Signed-off-by: Nwoke David Udoka <[email protected]>

Fixed the cached class error.

33e431b

Signed-off-by: Nwoke David Udoka <[email protected]>

dvdnwoke closed this Jun 9, 2018

dvdnwoke reopened this Jun 9, 2018

lonnieezell merged commit 2a89af6 into codeigniter4:develop Jun 12, 2018

neznaika0 mentioned this pull request Sep 13, 2025

fix: Add missing translation #9719

Merged

5 tasks


		//--------------------------------------------------------------------

		function __construct () {

Honeypot Filter #1063

Honeypot Filter #1063

Uh oh!

Conversation

dvdnwoke commented Jun 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lonnieezell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jim-parry commented Jun 5, 2018

Uh oh!

jim-parry commented Jun 5, 2018

Uh oh!

dvdnwoke commented Jun 5, 2018

Uh oh!

lonnieezell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvdnwoke commented Jun 8, 2018

Uh oh!

lonnieezell commented Jun 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvdnwoke commented Jun 8, 2018

Uh oh!

jim-parry commented Jun 8, 2018

Uh oh!

dvdnwoke commented Jun 9, 2018

Uh oh!

lonnieezell commented Jun 12, 2018

Uh oh!

Uh oh!

dvdnwoke commented Jun 3, 2018 •

edited

Loading