COLDBOX-1464 Reject advice methods requested directly as actions

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

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,6 +181,13 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" {
 			return processInvalidEvent( arguments.ehBean, oRequestContext )
 		}
 
+		// 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 )
+		}
+
 		/* ::::::::::::::::::::::::::::::::::::::::: EVENT CACHING :::::::::::::::::::::::::::::::::::::::::::: */
 
 		// Event Caching Routines, if using caching, NOT a private event and we are executing the main event
@@ -755,6 +768,46 @@ 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. 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 ( structKeyExists( variables.ADVICE_METHODS, arguments.action ) ) {
+			return true
+		}
+
+		// 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
+		}
+		// 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" )
+	}
+
 	/**
 	 * Verifies setup of base handler classes in WireBox
 	 *

test-harness/handlers/adviceDispatch.cfc

@@ -0,0 +1,83 @@
+/**
+ * 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" )
+	}
+
+
+	/************************************** PRIVATE *********************************************/
+
+	private function record( required prc, required name ){
+		if ( !arguments.prc.keyExists( "ran" ) ) {
+			arguments.prc.ran = []
+		}
+		arguments.prc.ran.append( arguments.name )
+	}
+
+}

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" )
+	}
+
+}

tests/specs/integration/AdviceDispatchSpec.cfc

@@ -0,0 +1,86 @@
+/**
+ * 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( "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( "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" )
+			} )
+
+			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" )
+			} )
+		} )
+	}
+
+}