Repository navigation
Conversation
homestar9
marked this pull request as ready for review
October 16, 2026 19:21
homestar9
force-pushed
the
fix/implicit-views-declared-roots
branch
from
October 16, 2026 19:22
66e8e89 to
d01701c
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A containment bypass, stale module roots, missing-file acceptance, and ineffective regression fixtures remain unresolved.
4 open findings
What changed in this PR
Restricts ColdBox’s event-selected implicit views to declared view folders while preserving explicit view selection.
Changes:
- Adds restricted lookup and a clear missing-implicit-view exception.
- Applies restricted lookup during dispatch and rendering.
- Adds lookup tests, integration tests, and template fixtures.
| File | Description |
|---|---|
| tests/specs/web/RendererTest.cfc | Tests restricted view lookup. |
| tests/specs/integration/ImplicitViewsSpec.cfc | Tests implicit dispatch and explicit selection. |
| test-harness/views/implicitViews/withView.cfm | Provides an allowed implicit view. |
| test-harness/rootOnlyView.cfm | Provides a root-only missing-handler fixture. |
| test-harness/main/rootOnlyAction.cfm | Provides a root-only missing-action fixture. |
| test-harness/implicitViews/noView.cfm | Provides an explicit-only root template. |
| test-harness/handlers/implicitViews.cfc | Adds implicit and explicit rendering actions. |
| system/web/services/HandlerService.cfc | Restricts implicit view dispatch. |
| system/web/Renderer.cfc | Adds folder containment checks and lookup validation. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… declared view folders Views chosen from the request's event name used the same search list as explicit views, which also tries the application root and the bare view path, so any template under the app root matching an event could be included as a view. The new Renderer.locateImplicitView() only accepts views in the views folder, the external views location or a module's views folders. isViewDispatch() uses it for missing handlers and actions, and discoverImplicitViews() throws Renderer.ImplicitViewNotFoundException instead of rendering a template outside those folders. Explicit setView()/view() calls are unchanged.
homestar9
force-pushed
the
fix/implicit-views-declared-roots
branch
from
October 16, 2026 19:30
d01701c to
0f3e318
Compare
- Compare view folders case-sensitively - Treat a view that locateView() returns unchanged as not found, also with an uppercase extension - Read a module's views folder from its current config, so a module reload takes effect - Lowercase the root-only fixtures: view dispatch lowercases the event, so on a case-sensitive file system the old lookup missed them
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Description
With
implicitViewsenabled, a view chosen from the request's event name is resolved with the same search list as an explicitsetView(). Afterviews/andviewsExternalLocation, that list also tries the application root and finally the bare view name. So?event=tools.cleanupincludes an existing/tools/cleanup.cfmfrom the app root, and for a handler with a missing action,?event=main.legacyincludes/main/legacy.cfm. These files are usually not views.This PR limits views chosen from the event name to the declared view folders:
Renderer.locateImplicitView( view, module = "" ).views/folder orviewsExternalLocation. For a module event, it also accepts the module's views folder and the app'sviews/modulesoverride folders.""...segments, and on a case-sensitive file system a folder whose name differs only by case, can't pass as a view folder. Symlinks inside a view folder keep working.viewsLocationtakes effect. It adds no file system calls beyond the existing lookup.HandlerService.isViewDispatch()uses it in both the module and non-module branches. This covers a missing handler and an existing handler with a missing action.Renderer.discoverImplicitViews()uses it for an action that runs but sets no view. If no permitted view exists, it throwsRenderer.ImplicitViewNotFoundExceptioninstead of including a root-level file. The same clear exception replaces the old missing-include error when the view doesn't exist anywhere.Views named explicitly by handler code, with
event.setView()orview(), are unchanged and keep the full search list.No extra state passes between dispatch and rendering.
locateView()andlocateModuleView()always search the view folders before the app root, so a view accepted bylocateImplicitView()is the same file rendering picks.Breaking change: apps that relied on implicit views resolving to files outside their view folders must move those files into
views/or callsetView()explicitly. Implicit views are opt-in since 8.3.0 (COLDBOX-1461), which limits who is affected. No existing spec relied on root-level implicit views, and all existing specs pass unchanged.Open question (from the Jira issue): should this ship as-is in 8.x, behind a setting, or in 9.0? I'm happy to put it behind a setting if you prefer.
Jira Issues
Type of change
Checklist
views/, external views, module views and module implicit views still dispatch..cfmfiles no longer dispatch, for a missing handler or a missing action.views/view.setView()andview()calls to root-level views still work.tests/specs/integration/ImplicitViewsSpec.cfc, pluslocateImplicitView()specs inRendererTest. Those also cover a missing view with an uppercase extension and a module whoseviewsLocationchanges.test-harness/rootonlyview.cfm, for a missing handlertest-harness/main/rootonlyaction.cfm, for a missing action. Both are lowercase because view dispatch lowercases the event, so on a case-sensitive file system the old lookup finds them too.test-harness/handlers/implicitViews.cfchandler with actions that set no viewviews/implicitViews/withView.cfm, and the root-onlyimplicitViews/noView.cfm, which the handler also selects explicitlyimplicitViewson, since the harness leaves it off.runner.cfmandrunner-integration.cfm, compared with a run ofdevelopmentwithout the change.mvcfailures on each engine, apart from the disk store specs below.CacheBoxWithDiskSoreTest, which also fails intermittently without this change, andBuilderTest.testbuildfeed, which fetches a live RSS feed ("Unable to read the source URL"). In the follow-up run,CacheBoxWithDiskSoreTestandDiskStoreTesterrored on Lucee and Adobe withCan't delete file [tests/tmp/cacheDepot], a local file lock unrelated to this change, and oneScopeWiringConcurrencyTestspec errored once on Adobe 2023.