Don't trigger ColdBox's invalid event looping protection #14

homestar9 · 2020-08-16T19:17:19Z

This is the same fix that @elpete implemented in cbGuard for an issue I reported where introducing cbSecurity or cbGuard to an app would break the invalidEventHandler Coldbox setting, See here for details: coldbox-modules/cbguard#15

@elpete

This is the same fix that @elpete implemented in cbGuard for an issue I reported where introducing cbSecurity or cbGuard to an app would break the `invalidEventHandler` Coldbox setting, See here for details: coldbox-modules/cbguard#15

homestar9 · 2020-08-16T20:03:22Z

It looks like some of the unit tests are causing the checks to fail. I'm not exactly sure how to address these. My guess is that the tests themselves need to be updated to account for the new invalidEventHandler property in Security.cfc. I may need some assistance in identifying what needs to be changed in order to pass all the tests.

lmajano · 2020-08-19T21:08:57Z

So @homestar9 before I left on holiday, I had resolved an issue with this, where the invalid event handler was creating this loop. I added a fix for it. ColdBox/coldbox-platform@39ebe21

So, can you try again WITHOUT this change. Because I tested it with it and it was working without it.

lmajano · 2020-08-19T21:09:05Z

Just make sure you are using the BE

homestar9 · 2020-08-19T21:12:01Z

Thanks, @lmajano. I do see the change in Coldbox 6, and it appears to work. However, I'm having an issue with an app that still uses Coldbox 5.6.2. Will the fix you applied to Coldbox 6 also work on Coldbox 5?

lmajano · 2020-08-24T16:26:59Z

Well, no. It will only work for ColdBox 6. So maybe, we can pivot this fix to ONLY work with ColdBox 5

Checks the version of Coldbox and only applies the fix for Coldbox 5.

homestar9 · 2020-08-24T23:53:12Z

Okay, I have updated the pull request.

I know in Coldbox 6 I could use something like controller.getSystemSetting( "version" ) or controller.getColdboxSetting( "version" ) which would spit out the version number. However, it looks like Coldbox 5 doesn't have that. I wound up using controller.getColdboxSettings().version which should work on Coldbox 5 and 6.

This does create a reliance on future versions of Coldbox supporting getColdboxSettings(), but that's the only way I could think of to change the fix so it would only apply to Coldbox 5. If you know a better way, please let me know. :)

lmajano · 2020-09-04T13:37:08Z

I know in Coldbox 6 I could use something like controller.getSystemSetting( "version" ) or controller.getColdboxSetting( "version" ) which would spit out the version number. However, it looks like Coldbox 5 doesn't have that. I wound up using controller.getColdboxSettings().version which should work on Coldbox 5 and 6.

Perfect!!!

This does create a reliance on future versions of Coldbox supporting getColdboxSettings(), but that's the only way I could think of to change the fix so it would only apply to Coldbox 5. If you know a better way, please let me know. :)

That's perfect as that is the getter for the entire struct and that will probably will never go away

lmajano

Great job! Needs some more optimizations!

lmajano · 2020-09-04T13:40:01Z

interceptors/Security.cfc

 	}
+
+	private boolean function isInvalidEventHandlerBean( required handlerBean ) {
+        if ( isNull( variables.onInvalidEventHandlerBean ) ) {


I think this check belongs outside of the declared function. In reality, this function should never be called if the set invalid event handler bean is never set. This way we can shortcut it on line 232 as well.

lmajano · 2020-09-04T13:40:55Z

interceptors/Security.cfc

+        var handlerBean = variables.handlerService.getHandlerBean( arguments.event.getCurrentEvent() );
+
+		if ( 
+            listGetAt( controller.getColdboxSettings().version, 1, "." ) == 5 && 


The 3 conditions are

is this coldbox < 6

do we have an onInvalidEventHandlerBean that is not null

is this the invalid event handler bean.

This way the shortcuts will work and the code will be fast.

lmajano · 2020-09-04T13:43:59Z

interceptors/Security.cfc

+        var handlerBean = variables.handlerService.getHandlerBean( arguments.event.getCurrentEvent() );
+
+		if ( 
+            listGetAt( controller.getColdboxSettings().version, 1, "." ) == 5 && 


Since this executes in every annotation check, this code needs to be extra fast. Therefore, evaluating the version into a list and comparing it takes time. Do this once no prob. 50 times in a request and it adds up.

I suggest you move the ColdBox version detection to the configure() method and set a boolean feature flag.

variables.invalidHandlerCleanupEnabled = getToken( controller.getColdboxSettings().version, 1, "." ) < 6;

Then in your if statement, you can just do a boolean evaluation:

if( variables.invalidHandlerCleanupEnabled && .... )

I'm so glad you made the comment about performance and moving the check to configure(). That was an excellent suggestion! I'll get to work on it and update the thread once complete.

Moved Coldbox version and the invalidEventHandlerBean null checks to the `configure()` method. Refactored the `isInvalidEventHandlerBean()` so it only checks whether the passed handlerBean is a match (removed the null check). Simplified the `if` statement in `processAnnotationRules()` for performance and legibility.

homestar9 · 2020-09-09T00:37:53Z

All set. I also moved the !isNull( onInvalidEventHandlerBean ) to the configure() method as well since generally the onInvalidEventHandler config shouldn't change without reinitializing the app. This should hopefully speed up performance a bit more as well.

My comments tend to be a bit verbose, so let me know if you'd like me to amend them.

lmajano · 2020-09-09T14:23:36Z

@homestar9 perfection. Now the issue are the tests :(

lmajano · 2020-09-09T14:23:54Z

Once the tests pass, I can merge, but you need to update them due to the changes in the source.

homestar9 · 2020-09-09T16:50:35Z

I'm a little stumped with how to address the test changes. The error message I'm seeing is:
Failure: The incoming function threw exception [expression] [Component [cbsecurity.interceptors.Security] has no accessible Member with name [INVALIDEVENTHANDLER]] [] different than expected params type=[Security.ValidatorMethodException], regex=[.*]

Is this a symptom that Wirebox isn't injecting the invalidEventHandler property?
property name="invalidEventHandler" inject="coldbox:setting:invalidEventHandler";

Or is something else trying to treat invalidEventHandler as a public property for some reason?

Here's an example of the unit tests that fails:

it( "can configure with default settings", function(){
	security.setProperties( settings );
	security
		.$( "getInstance" )
		.$args( settings.validator )
		.$results( wirebox.getInstance( settings.validator ) );
	security.configure();
	expect( security.getProperty( "rules", [] ) ).toBeEmpty();
} );

Another thought I had is that since this is a unit test, we may need to mock the invalidEventHandler property, but my mocking skills are nothing to brag about so I'm not exactly sure how to do that if indeed that is the case.

Any tips you could provide would be very helpful!

lmajano · 2020-09-10T14:12:38Z

Yep, you need to inject the mocks since they don't exist. Since you have defined it as a setter, you can just call it: interceptor.setInvalidEventHandler(). However, you also need to mock, the call to get the handler bean as well.

homestar9 · 2020-09-13T22:02:11Z

I had to go back to section 3 on mocking from my trusty course lecture printout from CBOX-205 for this one.

The invalidEventHandler property doesn't need to be public so let me know if you would prefer that I make it private.

I also added 2 new unit tests that ensure the extra invalid event handler processing only occurs when the Coldbox version used is less than 6.

If you see anything that could be improved, please do let me know. :)

lmajano · 2020-09-14T14:56:51Z

WOW! @homestar9 You have made me proud! I can guarantee you that your coding will become faster and more solid now! KUDOS!! 🎆 🍾 🍻

homestar9 · 2020-09-14T15:29:50Z

Thank you Luis! I hope you have a great start to your week. :)

Only make the change to _lastInvalidEvent if Coldbox 5 detected

4455bfe

Checks the version of Coldbox and only applies the fix for Coldbox 5.

lmajano requested changes Sep 4, 2020

View reviewed changes

lmajano assigned homestar9 Sep 8, 2020

lmajano added the Waiting on Customer label Sep 8, 2020

Add unit tests for invalid handler processing

95e3d94

lmajano merged commit efec77b into coldbox-modules:development Sep 14, 2020

homestar9 deleted the patch-1 branch September 14, 2020 15:30

Don't trigger ColdBox's invalid event looping protection #14

Don't trigger ColdBox's invalid event looping protection #14

Uh oh!

Conversation

homestar9 commented Aug 16, 2020

Uh oh!

homestar9 commented Aug 16, 2020

Uh oh!

lmajano commented Aug 19, 2020

Uh oh!

lmajano commented Aug 19, 2020

Uh oh!

homestar9 commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmajano commented Aug 24, 2020

Uh oh!

homestar9 commented Aug 24, 2020

Uh oh!

lmajano commented Sep 4, 2020

Uh oh!

lmajano left a comment

Choose a reason for hiding this comment

Uh oh!

lmajano Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

lmajano Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

lmajano Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

homestar9 Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

homestar9 commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmajano commented Sep 9, 2020

Uh oh!

lmajano commented Sep 9, 2020

Uh oh!

homestar9 commented Sep 9, 2020

Uh oh!

lmajano commented Sep 10, 2020

Uh oh!

homestar9 commented Sep 13, 2020

Uh oh!

lmajano commented Sep 14, 2020

Uh oh!

homestar9 commented Sep 14, 2020

Uh oh!

Uh oh!

homestar9 commented Aug 19, 2020 •

edited

Loading

homestar9 commented Sep 9, 2020 •

edited

Loading