Skip to content

COLDBOX-1464 Reject advice methods requested directly as actions - #724

Open
homestar9 wants to merge 2 commits into
ColdBox:developmentfrom
homestar9:fix/reject-direct-advice-actions
Open

homestar9 wants to merge 2 commits into
ColdBox:developmentfrom
homestar9:fix/reject-direct-advice-actions

Conversation

@homestar9

@homestar9 homestar9 commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Description

Handler advice methods (preHandler(), postHandler(), aroundHandler() and action advice such as preIndex()) 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 the action argument, and for preHandler, a second time as advice for the same request. The request usually fails afterwards looking for a main/preHandler view.

This PR treats such a request as an invalid event, the same way framework methods tagged cbMethod are already treated. The check sits in HandlerService.getHandler(), right after the existing cbMethod check, and calls processInvalidEvent(). Apps get their invalidEventHandler or a 404, with no new exception type.

The rule, in a new private helper isAdviceAction( action, oEventHandler ), is the one Controller.runEvent() already uses to find advice:

  • preHandler, postHandler or aroundHandler
  • pre, post or around followed by the name of an action that exists in the same handler, case-insensitive

The 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:

  • The framework's own advice calls: the Controller calls oHandler.preHandler(...) and the other advice directly.
  • Internal runEvent() calls to advice or private methods.
  • Actions that only look like advice: for example preflight() with no flight() action.

Edge cases:

  • A handler with its own view() action: the framework already runs preview() as advice before every view request, so a direct ?event=handler.preview is now refused as well.
  • Inherited framework helpers: every handler inherits helpers such as view() and layout(), which are tagged cbMethod and can never run as actions. The advice target must be a real action, so preview() in a handler without its own view() is still a normal action. Thanks to the Copilot review for catching this.

Jira Issues

Type of change

  • Improvement

Checklist

  • My code follows the style guidelines of this project (cfformat run on touched files: no changes beyond the fix)
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective:
    • New fixtures test-harness/handlers/adviceDispatch.cfc, and adviceInherited.cfc (a preview() action with no view() action).
    • New tests/specs/integration/AdviceDispatchSpec.cfc. Direct requests for preHandler, postHandler, aroundHandler, preIndex and preview reach invalid event handling. index with its automatic advice, preflight, preview() without a view() action, and an internal runEvent() call to preHandler still work.
    • Without the change, all six rejection specs fail: the advice runs as the action, and the request then fails because there's no view, or for aroundHandler, because there's no targetAction. The "still works" specs pass with and without the change.
  • New and existing unit tests pass locally with my changes:
    • runner.cfm and runner-integration.cfm, compared with a run of development without the change.
    • No new failures, and the new specs pass, on Lucee 5.4, Adobe 2023, Adobe 2025, BoxLang 1 and BoxLang 1 with CFML compat. Integration runner 107/107 on each engine.
    • After the follow-up commit for inherited framework helpers, AdviceDispatchSpec, HandlerServiceTest, EventExecutionsSpec and MainSpec pass 46/46 on each of those engines. The new preview() spec fails without that commit.
    • MySQL wasn't available locally, so ORM was turned off and the DB log appender removed for these runs. The specs that need the database fail the same way with and without the change.

…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.
@homestar9
homestar9 marked this pull request as ready for review October 16, 2026 17:28
Copilot AI balanced review requested due to automatic review settings October 16, 2026 17:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread system/web/services/HandlerService.cfc Outdated
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.
@homestar9
homestar9 force-pushed the fix/reject-direct-advice-actions branch from 476830b to 395969f Compare October 16, 2026 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants