From f134e956c3b81215e9c519313fb3664416ed90aa Mon Sep 17 00:00:00 2001 From: Peter Amiri Date: Tue, 9 Jun 2026 23:29:52 -0700 Subject: [PATCH] fix(router): except= normalization, scope option passthrough, end() underflow guard Addresses three wave-2 review findings in the mapper (routing:1, routing:5, routing:11): - resources()/resource() only/except lists are now normalized (Trim + LCase per item) and except filtering uses array operations instead of a case-sensitive ReReplace, so except="Delete" or except="new, delete" can no longer silently leave excluded destructive routes registered. Unknown action names throw Wheels.InvalidResource when showErrorInformation is enabled (development) and are dropped otherwise. - namespace(), package(), and group() now forward all arguments to scope(), so options like middleware and binding are no longer silently dropped. api() and version() delegate to group() and inherit the fix. - end() now throws a clear Wheels.InvalidRoute error on scope-stack underflow (an extra end()) instead of a raw array-index engine error. Specs: vendor/wheels/tests/specs/mapper/MapperRobustnessSpec.cfc covers all three behaviors and fails against the pre-fix code. Co-Authored-By: Claude Fable 5 Signed-off-by: Peter Amiri --- vendor/wheels/mapper/mapping.cfc | 11 + vendor/wheels/mapper/resources.cfc | 53 ++++- vendor/wheels/mapper/scoping.cfc | 28 +-- .../specs/mapper/MapperRobustnessSpec.cfc | 202 ++++++++++++++++++ 4 files changed, 274 insertions(+), 20 deletions(-) create mode 100644 vendor/wheels/tests/specs/mapper/MapperRobustnessSpec.cfc diff --git a/vendor/wheels/mapper/mapping.cfc b/vendor/wheels/mapper/mapping.cfc index a6d7f1a11a..6ed407427e 100644 --- a/vendor/wheels/mapper/mapping.cfc +++ b/vendor/wheels/mapper/mapping.cfc @@ -24,6 +24,17 @@ component { * [category: Routing] */ public struct function end() { + // Guard against unbalanced end() calls: once the scope stack is empty there + // is no open block left to close, so fail with a clear DSL error instead of + // a raw array-index engine error. + if (ArrayIsEmpty(variables.scopeStack)) { + Throw( + type = "Wheels.InvalidRoute", + message = "Unbalanced `end()` call in the route configuration.", + detail = "Each `end()` closes one block opened by `mapper()`, `scope()`, `namespace()`, `package()`, `group()`, `resource()`, or `resources()`. This `end()` has no matching open block, so remove the extra `end()`." + ); + } + local.formatPattern = ""; if (StructKeyExists(variables.scopeStack[1], "mapFormat") && variables.scopeStack[1].mapFormat) { diff --git a/vendor/wheels/mapper/resources.cfc b/vendor/wheels/mapper/resources.cfc index fb2eb01dcc..0efccb9b9a 100644 --- a/vendor/wheels/mapper/resources.cfc +++ b/vendor/wheels/mapper/resources.cfc @@ -100,18 +100,27 @@ component { local.args.collectionPath = arguments.path; // Consider only / except REST routes for resources. - // Allow arguments.only to override local.args.only. + // Allow arguments.only to override local.args.actions. + // Normalize (trim + lowercase) each item so values like "Index" or " show" still match. if (StructKeyExists(arguments, "only")) { - local.args.actions = LCase(arguments.only); + local.args.actions = $normalizeRestActions(arguments.only); } - // Remove unwanted routes from local.args.only. + // Remove unwanted routes from local.args.actions. + // Filter with array operations on normalized values so case or whitespace + // differences (e.g. except="Delete" or except="new, delete") cannot leave + // an excluded (potentially destructive) route registered. if (StructKeyExists(arguments, "except") && ListLen(arguments.except) > 0) { - local.except = ListToArray(arguments.except); - local.iEnd = ArrayLen(local.except); + local.except = ListToArray($normalizeRestActions(arguments.except)); + local.actions = ListToArray(local.args.actions); + local.remaining = []; + local.iEnd = ArrayLen(local.actions); for (local.i = 1; local.i <= local.iEnd; local.i++) { - local.args.actions = ReReplace(local.args.actions, "\b#local.except[local.i]#\b(,?|$)", ""); + if (!ArrayFindNoCase(local.except, local.actions[local.i])) { + ArrayAppend(local.remaining, local.actions[local.i]); + } } + local.args.actions = ArrayToList(local.remaining); } // If controller name was passed, use it. @@ -248,4 +257,36 @@ component { public struct function collection() { return scope(path = variables.scopeStack[1].collectionPath, $call = "collection"); } + + /** + * Internal function. + * Normalizes a comma-delimited list of REST action names used by the `only` / + * `except` arguments: trims and lowercases each item and drops empty entries so + * filtering matches reliably. When `showErrorInformation` is enabled (i.e. in + * development), unknown action names throw instead of being silently ignored. + */ + public string function $normalizeRestActions(required string actions) { + local.validActions = "index,new,create,show,edit,update,delete"; + local.items = ListToArray(arguments.actions); + local.rv = []; + local.iEnd = ArrayLen(local.items); + for (local.i = 1; local.i <= local.iEnd; local.i++) { + local.item = LCase(Trim(local.items[local.i])); + if (!Len(local.item)) { + continue; + } + if (!ListFindNoCase(local.validActions, local.item)) { + if ($get("showErrorInformation")) { + Throw( + type = "Wheels.InvalidResource", + message = "`#local.item#` is not a valid REST action name for the `only` / `except` arguments.", + detail = "Valid action names are: #local.validActions#." + ); + } + continue; + } + ArrayAppend(local.rv, local.item); + } + return ArrayToList(local.rv); + } } diff --git a/vendor/wheels/mapper/scoping.cfc b/vendor/wheels/mapper/scoping.cfc index 9d4b4de172..bdec6ae9a4 100644 --- a/vendor/wheels/mapper/scoping.cfc +++ b/vendor/wheels/mapper/scoping.cfc @@ -108,7 +108,9 @@ component { string package = arguments.name, string path = hyphenize(arguments.name) ) { - return scope(name = arguments.name, package = arguments.package, path = arguments.path, $call = "namespace"); + // Forward all arguments so scope() options like middleware and binding are + // not silently dropped. + return scope(argumentCollection = arguments, $call = "namespace"); } /** @@ -121,7 +123,9 @@ component { * @package Subfolder (package) to reference for controllers. This defaults to the value provided for `name`. */ public struct function package(required string name, string package = arguments.name) { - return scope(name = arguments.name, package = arguments.package, $call = "package"); + // Forward all arguments so scope() options like middleware and binding are + // not silently dropped. + return scope(argumentCollection = arguments, $call = "package"); } /** @@ -165,20 +169,16 @@ component { struct constraints, any callback ) { + // Forward every passed argument so scope() options like middleware and + // binding are not silently dropped. Only `callback` is consumed by group() + // itself. (api() and version() delegate here, so they inherit this too.) local.args = {}; - local.args.$call = "group"; - - if (StructKeyExists(arguments, "name")) { - local.args.name = arguments.name; - } - - if (StructKeyExists(arguments, "path")) { - local.args.path = arguments.path; - } - - if (StructKeyExists(arguments, "constraints")) { - local.args.constraints = arguments.constraints; + for (local.key in arguments) { + if (local.key != "callback" && StructKeyExists(arguments, local.key) && !IsNull(arguments[local.key])) { + local.args[local.key] = arguments[local.key]; + } } + local.args.$call = "group"; scope(argumentCollection = local.args); diff --git a/vendor/wheels/tests/specs/mapper/MapperRobustnessSpec.cfc b/vendor/wheels/tests/specs/mapper/MapperRobustnessSpec.cfc new file mode 100644 index 0000000000..ecc92f5d7f --- /dev/null +++ b/vendor/wheels/tests/specs/mapper/MapperRobustnessSpec.cfc @@ -0,0 +1,202 @@ +/** + * Mapper robustness specs: + * - resources() only/except normalization (case + whitespace) so excluded + * destructive routes cannot stay registered, plus development-mode validation + * of unknown REST action names. + * - namespace()/package()/group()/api()/version() forwarding scope() options + * such as middleware and binding instead of silently dropping them. + * - end() scope-stack underflow guard. + */ +component extends="wheels.WheelsTest" { + + function beforeAll() { + // Mapper.$addRoute() also appends to the application-scoped route data, + // so snapshot and restore it to avoid polluting other specs. + _originalRoutes = Duplicate(application.wheels.routes); + _originalStaticRoutes = StructKeyExists(application.wheels, "staticRoutes") ? StructCopy( + application.wheels.staticRoutes + ) : {}; + } + + function afterAll() { + application.wheels.routes = _originalRoutes; + application.wheels.staticRoutes = _originalStaticRoutes; + } + + function run() { + describe("resources() only/except normalization", function() { + beforeEach(function(currentSpec) { + m = new wheels.Mapper(); + m.$init(); + }); + + afterEach(function(currentSpec) { + StructDelete(variables, "m"); + }); + + it("excludes destructive routes when except uses different casing", function() { + m.$draw().resources(name = "users", except = "Delete", mapFormat = false).end(); + var routes = m.getRoutes(); + var found = false; + for (var route in routes) { + if (StructKeyExists(route, "action") && route.action == "delete") { + found = true; + } + } + expect(found).toBeFalse(); + expect(routes).toBeArray().toHaveLength(7); + }); + + it("excludes routes listed with surrounding whitespace in except", function() { + m.$draw().resources(name = "users", except = "new, delete", mapFormat = false).end(); + var routes = m.getRoutes(); + var found = false; + for (var route in routes) { + if (StructKeyExists(route, "action") && ListFindNoCase("new,delete", route.action)) { + found = true; + } + } + expect(found).toBeFalse(); + expect(routes).toBeArray().toHaveLength(6); + }); + + it("trims and lowercases items in only", function() { + m.$draw().resources(name = "users", only = "Index, Show", mapFormat = false).end(); + var routes = m.getRoutes(); + expect(routes).toBeArray().toHaveLength(2); + var actions = ""; + for (var route in routes) { + if (StructKeyExists(route, "action")) { + actions = ListAppend(actions, route.action); + } + } + expect(ListFindNoCase(actions, "index")).toBeGT(0); + expect(ListFindNoCase(actions, "show")).toBeGT(0); + }); + + it("throws on unknown action names when showErrorInformation is enabled", function() { + var originalSetting = application.wheels.showErrorInformation; + application.wheels.showErrorInformation = true; + try { + expect(function() { + m.$draw().resources(name = "users", except = "destroy", mapFormat = false).end(); + }).toThrow(type = "Wheels.InvalidResource"); + } finally { + application.wheels.showErrorInformation = originalSetting; + } + }); + + it("ignores unknown action names when showErrorInformation is disabled", function() { + var originalSetting = application.wheels.showErrorInformation; + application.wheels.showErrorInformation = false; + try { + m.$draw().resources(name = "users", except = "destroy", mapFormat = false).end(); + expect(m.getRoutes()).toBeArray().toHaveLength(8); + } finally { + application.wheels.showErrorInformation = originalSetting; + } + }); + }); + + describe("scope wrapper option passthrough", function() { + beforeEach(function(currentSpec) { + m = new wheels.Mapper(); + m.$init(); + }); + + afterEach(function(currentSpec) { + StructDelete(variables, "m"); + }); + + it("namespace() forwards middleware to child routes", function() { + m.$draw() + .namespace(name = "admin", middleware = ["AdminAuth"]) + .get(name = "dashboard", to = "dashboard##index") + .end() + .end(); + var routes = m.getRoutes(); + expect(routes[1]).toHaveKey("middleware"); + expect(routes[1].middleware).toBeArray().toHaveLength(1); + expect(routes[1].middleware[1]).toBe("AdminAuth"); + }); + + it("namespace() forwards binding to child routes", function() { + m.$draw() + .namespace(name = "admin", binding = true) + .get(name = "users", to = "users##index") + .end() + .end(); + var routes = m.getRoutes(); + expect(routes[1]).toHaveKey("binding"); + expect(routes[1].binding).toBeTrue(); + }); + + it("package() forwards middleware to child routes", function() { + m.$draw() + .package(name = "admin", middleware = ["AdminAuth"]) + .get(name = "dashboard", to = "dashboard##index") + .end() + .end(); + var routes = m.getRoutes(); + expect(routes[1]).toHaveKey("middleware"); + expect(routes[1].middleware[1]).toBe("AdminAuth"); + }); + + it("group() forwards middleware to child routes", function() { + m.$draw() + .group(path = "admin", middleware = ["AdminAuth"], callback = function(map) { + map.get(name = "dashboard", to = "admin##dashboard"); + }) + .end(); + var routes = m.getRoutes(); + expect(routes[1]).toHaveKey("middleware"); + expect(routes[1].middleware[1]).toBe("AdminAuth"); + }); + + it("api() forwards middleware to child routes", function() { + m.$draw() + .api(middleware = ["ApiAuth"], callback = function(apiMap) { + apiMap.get(name = "users", to = "users##index"); + }) + .end(); + var routes = m.getRoutes(); + expect(routes[1].pattern).toBe("/api/users"); + expect(routes[1]).toHaveKey("middleware"); + expect(routes[1].middleware[1]).toBe("ApiAuth"); + }); + + it("version() forwards middleware to child routes", function() { + m.$draw() + .api(callback = function(apiMap) { + apiMap.version(number = 1, middleware = ["V1Auth"], callback = function(v1) { + v1.get(name = "users", to = "users##index"); + }); + }) + .end(); + var routes = m.getRoutes(); + expect(routes[1].pattern).toBe("/api/v1/users"); + expect(routes[1]).toHaveKey("middleware"); + expect(routes[1].middleware[1]).toBe("V1Auth"); + }); + }); + + describe("end() scope-stack underflow", function() { + beforeEach(function(currentSpec) { + m = new wheels.Mapper(); + m.$init(); + }); + + afterEach(function(currentSpec) { + StructDelete(variables, "m"); + }); + + it("throws a clear DSL error instead of a raw engine error on an extra end()", function() { + m.$draw().end(); + expect(function() { + m.end(); + }).toThrow(type = "Wheels.InvalidRoute"); + }); + }); + } + +}