From 7f7861cf17ec0685402141918525eeaf168c871d Mon Sep 17 00:00:00 2001 From: David Levin Date: Fri, 16 Oct 2026 10:15:54 -0700 Subject: [PATCH 1/4] COLDBOX-1464 #resolve Reject advice methods requested directly as actions preHandler(), postHandler(), aroundHandler() and action advice such as preIndex() could be requested as the action, so they ran outside their normal context, and preHandler() ran twice. getHandler() now sends such a request to invalid event handling, like cbMethod methods. The rule matches how the Controller finds advice, and it only applies to the main request event, so the automatic advice calls and internal runEvent() calls keep working. --- system/web/services/HandlerService.cfc | 31 +++++++ test-harness/handlers/adviceDispatch.cfc | 89 +++++++++++++++++++ .../specs/integration/AdviceDispatchSpec.cfc | 72 +++++++++++++++ 3 files changed, 192 insertions(+) create mode 100644 test-harness/handlers/adviceDispatch.cfc create mode 100644 tests/specs/integration/AdviceDispatchSpec.cfc diff --git a/system/web/services/HandlerService.cfc b/system/web/services/HandlerService.cfc index 1e1d53cc1..797783be0 100644 --- a/system/web/services/HandlerService.cfc +++ b/system/web/services/HandlerService.cfc @@ -175,6 +175,17 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { return processInvalidEvent( arguments.ehBean, oRequestContext ) } + // Are they requesting an advice method (preHandler, preIndex, etc) as the action of the main event? + // Internal runEvent() calls and the automatic advice calls are not affected. + if ( + !arguments.ehBean.getIsPrivate() AND + arguments.ehBean.getFullEvent() EQ oRequestContext.getCurrentEvent() AND + isAdviceAction( arguments.ehBean.getMethod(), oEventHandler ) + ) { + // Invalid Event processing + return processInvalidEvent( arguments.ehBean, oRequestContext ) + } + /* ::::::::::::::::::::::::::::::::::::::::: EVENT CACHING :::::::::::::::::::::::::::::::::::::::::::: */ // Event Caching Routines, if using caching, NOT a private event and we are executing the main event @@ -755,6 +766,26 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { return getHandler( getHandlerBean( targetInvalidEvent ), arguments.requestContext ); } + /** + * Verify if an action is one of the handler's advice methods: preHandler(), postHandler(), aroundHandler(), + * or the pre/post/around advice of another action in the same handler. This is the same rule the + * Controller uses to find the advice it runs around an action. + * + * @action The requested action + * @oEventHandler The event handler instance + */ + private boolean function isAdviceAction( required string action, required oEventHandler ){ + if ( listFindNoCase( "preHandler,postHandler,aroundHandler", arguments.action ) ) { + return true + } + // pre{Action}, post{Action} or around{Action} for an action that exists + var adviceTarget = reReplaceNoCase( arguments.action, "^(pre|post|around)", "" ) + if ( !len( adviceTarget ) || adviceTarget == arguments.action ) { + return false + } + return arguments.oEventHandler._actionExists( adviceTarget ) + } + /** * Verifies setup of base handler classes in WireBox * diff --git a/test-harness/handlers/adviceDispatch.cfc b/test-harness/handlers/adviceDispatch.cfc new file mode 100644 index 000000000..37f7dbc5d --- /dev/null +++ b/test-harness/handlers/adviceDispatch.cfc @@ -0,0 +1,89 @@ +/** + * Advice methods that must not be requestable as actions, next to normal actions. + * Every method records itself in prc.ran so the specs can tell what executed. + * Tested by tests/specs/integration/AdviceDispatchSpec.cfc + */ +component { + + /************************************** ADVICE *********************************************/ + + function preHandler( + event, + rc, + prc, + action = "", + eventArguments = {} + ){ + // The framework passes the action when it runs this as advice + record( prc, len( arguments.action ) ? "preHandler" : "preHandler:action" ) + } + + function postHandler( + event, + rc, + prc, + action = "", + eventArguments = {} + ){ + record( prc, "postHandler" ) + } + + function aroundHandler( + event, + rc, + prc, + targetAction, + eventArguments = {} + ){ + record( prc, "aroundHandler" ) + return arguments.targetAction( + event = arguments.event, + rc = arguments.rc, + prc = arguments.prc + ) + } + + function preIndex( event, rc, prc ){ + record( prc, "preIndex" ) + } + + // Advice for view(): the framework runs it before every view() request + function preview( event, rc, prc ){ + record( prc, "preview" ) + } + + /************************************** ACTIONS *********************************************/ + + function index( event, rc, prc ){ + record( prc, "index" ) + event.renderData( type = "plain", data = "ADVICE-INDEX-OK" ) + } + + function view( event, rc, prc ){ + record( prc, "view" ) + event.renderData( type = "plain", data = "ADVICE-VIEW-OK" ) + } + + // Starts with "pre", but there is no flight() action, so this is a normal action + function preflight( event, rc, prc ){ + record( prc, "preflight" ) + event.renderData( type = "plain", data = "ADVICE-PREFLIGHT-OK" ) + } + + // Internal calls to an advice method keep working + function internalAdvice( event, rc, prc ){ + record( prc, "internalAdvice" ) + runEvent( event = "adviceDispatch.preHandler", prePostExempt = true ) + event.renderData( type = "plain", data = "ADVICE-INTERNAL-OK" ) + } + + /************************************** PRIVATE *********************************************/ + + private function record( required prc, required name ){ + if ( !arguments.prc.keyExists( "ran" ) ) { + arguments.prc.ran = [] + } + arguments.prc.ran.append( arguments.name ) + } + +} diff --git a/tests/specs/integration/AdviceDispatchSpec.cfc b/tests/specs/integration/AdviceDispatchSpec.cfc new file mode 100644 index 000000000..646ab65f5 --- /dev/null +++ b/tests/specs/integration/AdviceDispatchSpec.cfc @@ -0,0 +1,72 @@ +/** + * Advice methods (preHandler, postHandler, aroundHandler, pre/post/around{Action}) can't be requested as the action. + * Fixture: test-harness/handlers/adviceDispatch.cfc + */ +component extends="tests.resources.BaseIntegrationTest" { + + function run(){ + describe( "Advice methods requested as actions", function(){ + beforeEach( function( currentSpec ){ + // Setup as a new ColdBox request + setup() + } ) + + var adviceEvents = [ + "adviceDispatch.preHandler", + "adviceDispatch.postHandler", + "adviceDispatch.aroundHandler", + "adviceDispatch.preIndex", + "adviceDispatch.PREINDEX", + "adviceDispatch.preview" + ] + + for ( var adviceEvent in adviceEvents ) { + it( + title = "rejects #adviceEvent# as an invalid event", + data = { event : adviceEvent }, + body = function( data ){ + var e = execute( event = data.event, renderResults = true ) + var prc = e.getPrivateCollection() + + expect( prc.invalidevent ?: "" ).toBe( data.event ) + expect( e.getStatusCode() ).toBe( 404 ) + expect( e.getRenderedContent() ).toInclude( "Invalid Page" ) + // Nothing in the advice handler ran + expect( prc.keyExists( "ran" ) ).toBeFalse() + } + ) + } + + it( "runs a normal action with its automatic advice", function(){ + var e = execute( event = "adviceDispatch.index", renderResults = true ) + + expect( e.getRenderedContent() ).toInclude( "ADVICE-INDEX-OK" ) + expect( e.getPrivateValue( "ran" ).toList() ).toBe( "preHandler,preIndex,aroundHandler,index,postHandler" ) + } ) + + it( "still runs preview() as advice for view()", function(){ + var e = execute( event = "adviceDispatch.view", renderResults = true ) + + expect( e.getRenderedContent() ).toInclude( "ADVICE-VIEW-OK" ) + expect( e.getPrivateValue( "ran" ).toList() ).toBe( "preHandler,preview,aroundHandler,view,postHandler" ) + } ) + + it( "runs an action that starts with pre but advises no action", function(){ + var e = execute( event = "adviceDispatch.preflight", renderResults = true ) + + expect( e.getRenderedContent() ).toInclude( "ADVICE-PREFLIGHT-OK" ) + expect( e.getPrivateValue( "ran" ).toList() ).toBe( "preHandler,aroundHandler,preflight,postHandler" ) + } ) + + it( "lets handler code run an advice method with runEvent()", function(){ + var e = execute( event = "adviceDispatch.internalAdvice", renderResults = true ) + + expect( e.getRenderedContent() ).toInclude( "ADVICE-INTERNAL-OK" ) + expect( e.getPrivateValue( "ran" ).toList() ).toBe( + "preHandler,aroundHandler,internalAdvice,preHandler:action,postHandler" + ) + } ) + } ) + } + +} From 420722e815b080d280749b3b4168fe8d2dfcb34c Mon Sep 17 00:00:00 2001 From: David Levin Date: Fri, 16 Oct 2026 11:00:41 -0700 Subject: [PATCH 2/4] COLDBOX-1464 Ignore inherited framework methods when matching advice Every handler inherits framework helpers such as view(), which _actionExists() reports but which can never run as actions (cbMethod). A handler with preview() and no view() action of its own had its preview() rejected as advice. The advice target must now be a real action. --- system/web/services/HandlerService.cfc | 11 +++++++++-- test-harness/handlers/adviceInherited.cfc | 12 ++++++++++++ tests/specs/integration/AdviceDispatchSpec.cfc | 7 +++++++ 3 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 test-harness/handlers/adviceInherited.cfc diff --git a/system/web/services/HandlerService.cfc b/system/web/services/HandlerService.cfc index 797783be0..ef0f4c702 100644 --- a/system/web/services/HandlerService.cfc +++ b/system/web/services/HandlerService.cfc @@ -769,7 +769,8 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { /** * Verify if an action is one of the handler's advice methods: preHandler(), postHandler(), aroundHandler(), * or the pre/post/around advice of another action in the same handler. This is the same rule the - * Controller uses to find the advice it runs around an action. + * Controller uses to find the advice it runs around an action. Inherited framework methods tagged + * cbMethod, such as view(), can never run as actions, so preview() next to them is a normal action. * * @action The requested action * @oEventHandler The event handler instance @@ -783,7 +784,13 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { if ( !len( adviceTarget ) || adviceTarget == arguments.action ) { return false } - return arguments.oEventHandler._actionExists( adviceTarget ) + if ( !arguments.oEventHandler._actionExists( adviceTarget ) ) { + return false + } + // The target must be an action, not an inherited framework method + var targetMetadata = arguments.oEventHandler._actionMetadata( adviceTarget ) + var annotations = targetMetadata.keyExists( "annotations" ) ? targetMetadata.annotations : targetMetadata + return !annotations.keyExists( "cbMethod" ) } /** diff --git a/test-harness/handlers/adviceInherited.cfc b/test-harness/handlers/adviceInherited.cfc new file mode 100644 index 000000000..479aa9e39 --- /dev/null +++ b/test-harness/handlers/adviceInherited.cfc @@ -0,0 +1,12 @@ +/** + * preview() with no view() action of its own: the inherited framework view() helper is not an action, + * so preview() is a normal action, not advice. + * Tested by tests/specs/integration/AdviceDispatchSpec.cfc + */ +component { + + function preview( event, rc, prc ){ + event.renderData( type = "plain", data = "INHERITED-PREVIEW-OK" ) + } + +} diff --git a/tests/specs/integration/AdviceDispatchSpec.cfc b/tests/specs/integration/AdviceDispatchSpec.cfc index 646ab65f5..700cd7f5b 100644 --- a/tests/specs/integration/AdviceDispatchSpec.cfc +++ b/tests/specs/integration/AdviceDispatchSpec.cfc @@ -58,6 +58,13 @@ component extends="tests.resources.BaseIntegrationTest" { expect( e.getPrivateValue( "ran" ).toList() ).toBe( "preHandler,aroundHandler,preflight,postHandler" ) } ) + it( "runs preview() as an action when view() is only the inherited framework helper", function(){ + var e = execute( event = "adviceInherited.preview", renderResults = true ) + + expect( e.getPrivateValue( "invalidevent", "" ) ).toBe( "" ) + expect( e.getRenderedContent() ).toInclude( "INHERITED-PREVIEW-OK" ) + } ) + it( "lets handler code run an advice method with runEvent()", function(){ var e = execute( event = "adviceDispatch.internalAdvice", renderResults = true ) From ca2008bdd60977969fc6d6bb2cafb2e9cc0fb447 Mon Sep 17 00:00:00 2001 From: David Levin Date: Wed, 28 Oct 2026 12:57:45 -0700 Subject: [PATCH 3/4] COLDBOX-1464 Check for advice with struct lookups and reject it for every execution From review: isAdviceAction() runs for every event, so it now uses a lookup struct built once in init() and plain left() and mid() checks instead of a list search and a regex. An action without a pre, post or around prefix returns without reading any metadata. The check no longer compares the event with the request's current event or skips private events. An advice method is now rejected for every execution, including runEvent() and private events, which then throw like runEvent() of an action that doesn't exist. --- system/web/services/HandlerService.cfc | 37 +++++++++++++------ test-harness/handlers/adviceDispatch.cfc | 6 --- .../specs/integration/AdviceDispatchSpec.cfc | 19 +++++++--- 3 files changed, 39 insertions(+), 23 deletions(-) diff --git a/system/web/services/HandlerService.cfc b/system/web/services/HandlerService.cfc index ef0f4c702..b6d6a9513 100644 --- a/system/web/services/HandlerService.cfc +++ b/system/web/services/HandlerService.cfc @@ -63,6 +63,12 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { // Default registries variables.registeredHandlers = {} variables.registeredExternalHandlers = {} + // Handler-level advice methods, a struct for fast existence checks on every event + variables.ADVICE_METHODS = { + "preHandler" : true, + "postHandler" : true, + "aroundHandler" : true + } return this } @@ -175,13 +181,9 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { return processInvalidEvent( arguments.ehBean, oRequestContext ) } - // Are they requesting an advice method (preHandler, preIndex, etc) as the action of the main event? - // Internal runEvent() calls and the automatic advice calls are not affected. - if ( - !arguments.ehBean.getIsPrivate() AND - arguments.ehBean.getFullEvent() EQ oRequestContext.getCurrentEvent() AND - isAdviceAction( arguments.ehBean.getMethod(), oEventHandler ) - ) { + // Are they requesting an advice method (preHandler, preIndex, etc) as an action? + // The Controller runs advice by calling the methods directly, so they never come through here. + if ( isAdviceAction( arguments.ehBean.getMethod(), oEventHandler ) ) { // Invalid Event processing return processInvalidEvent( arguments.ehBean, oRequestContext ) } @@ -772,18 +774,31 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { * Controller uses to find the advice it runs around an action. Inherited framework methods tagged * cbMethod, such as view(), can never run as actions, so preview() next to them is a normal action. * + * This runs for every event, so it only uses struct lookups and plain string checks. An action + * without a pre, post or around prefix, which is almost every action, returns without any metadata lookup. + * * @action The requested action * @oEventHandler The event handler instance */ private boolean function isAdviceAction( required string action, required oEventHandler ){ - if ( listFindNoCase( "preHandler,postHandler,aroundHandler", arguments.action ) ) { + if ( structKeyExists( variables.ADVICE_METHODS, arguments.action ) ) { return true } - // pre{Action}, post{Action} or around{Action} for an action that exists - var adviceTarget = reReplaceNoCase( arguments.action, "^(pre|post|around)", "" ) - if ( !len( adviceTarget ) || adviceTarget == arguments.action ) { + + // pre{Action}, post{Action} or around{Action}: the action it would advise + var adviceTarget = "" + if ( left( arguments.action, 3 ) == "pre" ) { + adviceTarget = mid( arguments.action, 4, len( arguments.action ) ) + } else if ( left( arguments.action, 4 ) == "post" ) { + adviceTarget = mid( arguments.action, 5, len( arguments.action ) ) + } else if ( left( arguments.action, 6 ) == "around" ) { + adviceTarget = mid( arguments.action, 7, len( arguments.action ) ) + } + if ( !len( adviceTarget ) ) { return false } + + // Only for an action with an advice prefix: does the advised action exist? if ( !arguments.oEventHandler._actionExists( adviceTarget ) ) { return false } diff --git a/test-harness/handlers/adviceDispatch.cfc b/test-harness/handlers/adviceDispatch.cfc index 37f7dbc5d..cf4b819af 100644 --- a/test-harness/handlers/adviceDispatch.cfc +++ b/test-harness/handlers/adviceDispatch.cfc @@ -70,12 +70,6 @@ component { event.renderData( type = "plain", data = "ADVICE-PREFLIGHT-OK" ) } - // Internal calls to an advice method keep working - function internalAdvice( event, rc, prc ){ - record( prc, "internalAdvice" ) - runEvent( event = "adviceDispatch.preHandler", prePostExempt = true ) - event.renderData( type = "plain", data = "ADVICE-INTERNAL-OK" ) - } /************************************** PRIVATE *********************************************/ diff --git a/tests/specs/integration/AdviceDispatchSpec.cfc b/tests/specs/integration/AdviceDispatchSpec.cfc index 700cd7f5b..f62959d56 100644 --- a/tests/specs/integration/AdviceDispatchSpec.cfc +++ b/tests/specs/integration/AdviceDispatchSpec.cfc @@ -65,13 +65,20 @@ component extends="tests.resources.BaseIntegrationTest" { expect( e.getRenderedContent() ).toInclude( "INHERITED-PREVIEW-OK" ) } ) - it( "lets handler code run an advice method with runEvent()", function(){ - var e = execute( event = "adviceDispatch.internalAdvice", renderResults = true ) + it( "rejects an advice method requested with runEvent(), like a missing action", function(){ + // runEvent() of an action that doesn't exist throws, and so does an advice method now + expect( function(){ + getController().runEvent( event = "adviceDispatch.preHandler" ) + } ).toThrow() + expect( getRequestContext().getPrivateValue( "invalidevent", "" ) ).toBe( "adviceDispatch.preHandler" ) + expect( getRequestContext().getPrivateValue( "ran", [] ) ).notToInclude( "preHandler:action" ) + } ) - expect( e.getRenderedContent() ).toInclude( "ADVICE-INTERNAL-OK" ) - expect( e.getPrivateValue( "ran" ).toList() ).toBe( - "preHandler,aroundHandler,internalAdvice,preHandler:action,postHandler" - ) + it( "rejects an advice method requested as a private event", function(){ + var e = execute( event = "adviceDispatch.preHandler", private = true ) + + expect( e.getPrivateValue( "invalidevent", "" ) ).toBe( "adviceDispatch.preHandler" ) + expect( e.getPrivateValue( "ran", [] ) ).notToInclude( "preHandler:action" ) } ) } ) } From 2d156052751b387d9cbead2ff5e24dd61ad7a56c Mon Sep 17 00:00:00 2001 From: David Levin Date: Wed, 28 Oct 2026 12:58:18 -0700 Subject: [PATCH 4/4] COLDBOX-1464 Document the hot path performance rules from review Code that runs on every request should use struct lookups instead of list or array searches, avoid regular expressions, build lookup structs once, and remember results that don't change between requests. --- AGENTS.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 8eb04b094..8a3e24df2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -23,6 +23,15 @@ ColdBox is an HMVC (Hierarchical Model-View-Controller) framework designed for t - Do not start a docblock line with `@` in an example, because BoxLang reads it as documentation metadata. - Prefer the elvis operator `?:` and safe navigation `?.` over `structKeyExists()` and `isNull()` checks when they say the same thing. +### Hot Paths +Code that runs on every request must be as fast as possible: event dispatch in `HandlerService` and `Controller`, routing, rendering and view lookups, interceptor announcements, WireBox `getInstance()`, and `RequestContext` getters. +- Look things up in a struct, with `structKeyExists()` or `keyExists()`. Don't search a list or an array (`listFind()`, `listFindNoCase()`, `arrayFind()`, `arrayContains()`). +- Don't use regular expressions. Use `left()`, `right()`, `mid()`, `find()` or `compare()`, or a lookup struct. +- Build lookup structs once, in `init()` or `onConfigurationLoad()`, not on every call. +- Put the cheapest check first, so the common case returns early. +- Remember results that don't change between requests in a struct, gated by the matching setting (`handlerCaching`, `viewDiscoveryCaching`) like the existing caches. +- Don't read metadata (`getMetadata()`), serialize (`serializeJSON()`) or hash on every call. + ## JavaScript Coding Standards ### Spacing and Formatting