Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Advice detection incorrectly rejects valid preview() actions because it counts the inherited framework view() helper as an action.
1 open finding
What changed in this PR
Prevents handler advice methods from being requested directly as main actions, using ColdBox’s existing invalid-event handling.
Changes:
- Adds advice detection to handler dispatch.
- Adds fixtures and integration tests for rejected requests and preserved behavior.
| File | Description |
|---|---|
| tests/specs/integration/AdviceDispatchSpec.cfc | Tests advice rejection and normal dispatch. |
| test-harness/handlers/adviceDispatch.cfc | Provides advice and action fixtures. |
| system/web/services/HandlerService.cfc | Rejects main-event requests targeting advice. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
476830b to
395969f
Compare
…ions 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.
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.
395969f to
420722e
Compare
| // Internal runEvent() calls and the automatic advice calls are not affected. | ||
| if ( | ||
| !arguments.ehBean.getIsPrivate() AND | ||
| arguments.ehBean.getFullEvent() EQ oRequestContext.getCurrentEvent() AND |
There was a problem hiding this comment.
You can't test if the full event is the current event. This would break on internal event executions. Which can also execute private methods. So just leave the advice check
There was a problem hiding this comment.
Done in ca2008b. The check is now just isAdviceAction(), with no current-event or private-event condition. An advice method is rejected for every execution: requests, private events and runEvent(). runEvent() of an advice method now throws, the same way runEvent() of an action that doesn't exist already does. Both cases have a spec.
| * @oEventHandler The event handler instance | ||
| */ | ||
| private boolean function isAdviceAction( required string action, required oEventHandler ){ | ||
| if ( listFindNoCase( "preHandler,postHandler,aroundHandler", arguments.action ) ) { |
There was a problem hiding this comment.
Don't use lists they are slow. Use a static struct. Hash map existence checks are fast. This is a hot path
There was a problem hiding this comment.
Done in ca2008b. preHandler, postHandler and aroundHandler are keys of a struct built once in init(), checked with structKeyExists().
| if ( listFindNoCase( "preHandler,postHandler,aroundHandler", arguments.action ) ) { | ||
| return true | ||
| } | ||
| // pre{Action}, post{Action} or around{Action} for an action that exists |
There was a problem hiding this comment.
Again this is a hot path and doing Regex will be slow.
It's preferable to just do struct checks.
There was a problem hiding this comment.
Done in ca2008b. The pre, post and around prefixes are checked with left() and mid(). An action without one of those prefixes, which is almost every action, returns without reading any metadata.
lmajano
left a comment
lmajano
left a comment
There was a problem hiding this comment.
I like the idea of this this is great thank you so much point however we need to make sure that it is incredibly fast in order to avoid bottlenecks for multiple requests because this is a hot path.
Using lists or regex can be very simple but you pay a price for it so I have been basically going over all internal codes to switch them to very fast lookups especially using struct key exists which are a thousand times faster than searching through an array or lists. Also if we can avoid regex thata a plus.
Again this is for hot paths
…very 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.
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.
|
Thanks @lmajano, that makes sense. All three notes are fixed in ca2008b. Timing the old and new checks, 100,000 calls each, on Lucee 5:
I also added a short "Hot Paths" section to I applied the same rules to two of my other open PRs that run on every request:
|
Description
Handler advice methods (
preHandler(),postHandler(),aroundHandler()and action advice such aspreIndex()) have to be visible for the framework to call them. They can also be requested as the action, for example?event=main.preHandler. ColdBox then runs the advice as the action, outside its normal context: without theactionargument, and forpreHandler, a second time as advice for the same request. The request usually fails afterwards looking for amain/preHandlerview.This PR treats such a request as an invalid event, the same way framework methods tagged
cbMethodare already treated. The check sits inHandlerService.getHandler(), right after the existingcbMethodcheck, and callsprocessInvalidEvent(). Apps get theirinvalidEventHandleror a 404, with no new exception type.The rule, in a new private helper
isAdviceAction( action, oEventHandler ), is the oneController.runEvent()already uses to find advice:preHandler,postHandleroraroundHandlerpre,postoraroundfollowed by the name of an action that exists in the same handler, case-insensitiveThe check only applies to the main request event, using the same test as the event caching block (
!ehBean.getIsPrivate() AND ehBean.getFullEvent() EQ requestContext.getCurrentEvent()). Not affected:oHandler.preHandler(...)and the other advice directly.runEvent()calls to advice or private methods.preflight()with noflight()action.Edge cases:
view()action: the framework already runspreview()as advice before everyviewrequest, so a direct?event=handler.previewis now refused as well.view()andlayout(), which are taggedcbMethodand can never run as actions. The advice target must be a real action, sopreview()in a handler without its ownview()is still a normal action. Thanks to the Copilot review for catching this.Jira Issues
Type of change
Checklist
test-harness/handlers/adviceDispatch.cfc, andadviceInherited.cfc(apreview()action with noview()action).tests/specs/integration/AdviceDispatchSpec.cfc. Direct requests forpreHandler,postHandler,aroundHandler,preIndexandpreviewreach invalid event handling.indexwith its automatic advice,preflight,preview()without aview()action, and an internalrunEvent()call topreHandlerstill work.aroundHandler, because there's notargetAction. The "still works" specs pass with and without the change.runner.cfmandrunner-integration.cfm, compared with a run ofdevelopmentwithout the change.AdviceDispatchSpec,HandlerServiceTest,EventExecutionsSpecandMainSpecpass 46/46 on each of those engines. The newpreview()spec fails without that commit.