fix(controller): namespace pagination handles under request.wheels.$pagination

changelog.d/3339-pagination-handle-namespace.fixed.md

@@ -0,0 +1 @@
+- Pagination handles are now stored under `request.wheels.$pagination[handle]` instead of directly in `request.wheels[handle]`. Handles are caller-supplied names, so the flat layout put arbitrary user input in the same case-insensitive keyspace as framework-owned request state, and the collision ran both ways. Writing: `setPagination(handle="tenant")` replaced the resolved tenant context with a pagination struct, and `handle="$queryCache"` did the same to the per-request finder cache — silently, since neither is validated. Reading: `pagination()` only checks that a handle exists when `showErrorInformation` is on, so in production an unknown handle that happened to name a framework key returned that key's struct as though it were pagination data. `request.wheels` currently holds around thirty-five framework-owned keys — including `params`, `execution`, `currentRoute`, `transactions`, `flashKeep` and `exception` — every one of which was reachable this way. Handles now resolve only inside their own sub-struct, so neither direction can cross over. `Wheels.QueryHandleNotFound` behaviour is unchanged (#3339)

vendor/wheels/Global.cfc

@@ -3847,16 +3847,37 @@ return local.$wheels;
 	 * @handle The handle given to the query to return pagination information for.
 	 */
 	public struct function pagination(string handle = "query") {
+		$ensurePaginationStore();
 		if ($get("showErrorInformation")) {
-			if (!StructKeyExists(request.wheels, arguments.handle)) {
+			if (!StructKeyExists(request.wheels["$pagination"], arguments.handle)) {
 				Throw(
 					type = "Wheels.QueryHandleNotFound",
 					message = "Wheels couldn't find a query with the handle of `#arguments.handle#`.",
 					extendedInfo = "Make sure your `findAll` call has the `page` argument specified and matching `handle` argument if specified."
 				);
 			}
 		}
-		return request.wheels[arguments.handle];
+		return request.wheels["$pagination"][arguments.handle];
+	}
+
+	/**
+	 * Internal function.
+	 * Creates the reserved per-request pagination namespace if it doesn't exist yet.
+	 *
+	 * Pagination handles are caller-supplied names, so storing them directly in `request.wheels`
+	 * put arbitrary user input in the same case-insensitive keyspace as framework-owned request
+	 * state. A handle matching a framework key overwrote it, and — because `pagination()` only
+	 * validates the handle when `showErrorInformation` is on — production reads of an unknown
+	 * handle returned whatever framework struct happened to occupy that key. Both directions are
+	 * closed by confining handles to their own sub-struct (#3339, same fix shape as #3336).
+	 */
+	public void function $ensurePaginationStore() {
+		if (!StructKeyExists(request, "wheels")) {
+			request.wheels = {};
+		}
+		if (!StructKeyExists(request.wheels, "$pagination")) {
+			request.wheels["$pagination"] = {};
+		}
 	}
 
 	/**
@@ -3924,7 +3945,8 @@ return local.$wheels;
 
 		local.args = Duplicate(arguments);
 		StructDelete(local.args, "handle");
-		request.wheels[arguments.handle] = local.args;
+		$ensurePaginationStore();
+		request.wheels["$pagination"][arguments.handle] = local.args;
 	}
 
 	/**

vendor/wheels/tests/specs/controller/paginationHandleCollisionSpec.cfc

@@ -0,0 +1,131 @@
+/**
+ * Regression coverage for #3339.
+ *
+ * `setPagination()` / `pagination()` key themselves on a caller-supplied handle name (default
+ * `"query"`), which used to be written straight into `request.wheels`. CFML struct keys are
+ * case-insensitive, so a handle matching a framework-owned key collided with it in both
+ * directions:
+ *
+ *   1. Write: `setPagination(handle="tenant")` overwrote the resolved tenant context with a
+ *      pagination struct. `handle="$queryCache"` did the same to the per-request finder cache.
+ *   2. Read: `pagination()` only validates the handle when `showErrorInformation` is on, so in
+ *      production an unknown handle that happened to name a framework key returned that key's
+ *      struct as though it were pagination state.
+ *
+ * Handles now live under the reserved `request.wheels.$pagination` sub-struct. Same fix shape as
+ * #3336, which moved the finder cache to `request.wheels.$queryCache`.
+ */
+component extends="wheels.WheelsTest" {
+
+	function run() {
+
+		g = application.wo;
+
+		describe("pagination handle / framework key collision (##3339)", () => {
+
+			// The whole core suite runs inside a single request, so request.wheels is shared across
+			// spec files. Only ever remove this spec's own handles — deleting the $pagination
+			// namespace wholesale would destroy handles other specs set up.
+			ownHandles = "articles,comments,tenant,$queryCache,noSuchHandleXYZ";
+
+			beforeEach(() => {
+				originalShowErr = application.wheels.showErrorInformation;
+				originalCacheSetting = application.wheels.cacheQueriesDuringRequest;
+				StructDelete(request.wheels, "tenant");
+				g.$ensurePaginationStore();
+				for (var h in ListToArray(ownHandles)) {
+					StructDelete(request.wheels["$pagination"], h, false);
+				}
+			})
+
+			afterEach(() => {
+				application.wheels.showErrorInformation = originalShowErr;
+				application.wheels.cacheQueriesDuringRequest = originalCacheSetting;
+				StructDelete(request.wheels, "tenant");
+				g.$ensurePaginationStore();
+				for (var h in ListToArray(ownHandles)) {
+					StructDelete(request.wheels["$pagination"], h, false);
+				}
+			})
+
+			it("stores handles under the reserved namespace, not the bare key", () => {
+				g.setPagination(totalRecords = 100, currentPage = 2, perPage = 10, handle = "articles");
+
+				expect(StructKeyExists(request.wheels, "$pagination")).toBeTrue();
+				expect(StructKeyExists(request.wheels["$pagination"], "articles")).toBeTrue();
+				expect(StructKeyExists(request.wheels, "articles")).toBeFalse();
+			})
+
+			it("round-trips pagination data through the namespace", () => {
+				g.setPagination(totalRecords = 100, currentPage = 2, perPage = 10, handle = "articles");
+				var pg = g.pagination("articles");
+
+				expect(pg.totalRecords).toBe(100);
+				expect(pg.currentPage).toBe(2);
+				expect(pg.perPage).toBe(10);
+				expect(pg.totalPages).toBe(10);
+			})
+
+			// Write direction — a handle named after a framework key must not clobber it.
+			it("does not overwrite resolved tenant context when a handle is named tenant", () => {
+				request.wheels.tenant = {id = "acme", dataSource = "tenant_acme", config = {}, "$locked" = true};
+
+				g.setPagination(totalRecords = 50, currentPage = 1, perPage = 25, handle = "tenant");
+
+				expect(IsDefined("request.wheels.tenant")).toBeTrue();
+				expect(request.wheels.tenant.id).toBe("acme");
+				expect(request.wheels.tenant.dataSource).toBe("tenant_acme");
+				expect(g.$tenantDataSource()).toBe("tenant_acme");
+			})
+
+			it("does not overwrite the finder cache namespace when a handle is named \$queryCache", () => {
+				application.wheels.cacheQueriesDuringRequest = true;
+				model("author").findAll(where = "lastName = 'Djurner'");
+				var cachedBefore = StructCount(request.wheels["$queryCache"]["author"]);
+
+				g.setPagination(totalRecords = 50, currentPage = 1, perPage = 25, handle = "$queryCache");
+
+				expect(StructKeyExists(request.wheels["$queryCache"], "author")).toBeTrue();
+				expect(StructCount(request.wheels["$queryCache"]["author"])).toBe(cachedBefore);
+			})
+
+			// Read direction — the case showErrorInformation hides in production.
+			it("does not return a framework struct for an unknown handle when errors are hidden", () => {
+				application.wheels.showErrorInformation = false;
+				request.wheels.tenant = {id = "acme", dataSource = "tenant_acme", config = {}, "$locked" = true};
+
+				// Pre-fix this returned the tenant struct as though it were pagination data.
+				// It must now fail to resolve rather than hand back foreign state.
+				var result = {returnedTenant = false, threw = false};
+				try {
+					var pg = g.pagination("tenant");
+					result.returnedTenant = IsStruct(pg) && StructKeyExists(pg, "dataSource");
+				} catch (any e) {
+					result.threw = true;
+				}
+
+				expect(result.returnedTenant).toBeFalse();
+				expect(result.threw).toBeTrue();
+			})
+
+			it("still throws Wheels.QueryHandleNotFound for an unknown handle in development", () => {
+				application.wheels.showErrorInformation = true;
+
+				expect(function() {
+					g.pagination("noSuchHandleXYZ");
+				}).toThrow("Wheels.QueryHandleNotFound");
+			})
+
+			it("keeps distinct handles isolated from each other", () => {
+				g.setPagination(totalRecords = 100, currentPage = 1, perPage = 10, handle = "articles");
+				g.setPagination(totalRecords = 30, currentPage = 3, perPage = 5, handle = "comments");
+
+				expect(g.pagination("articles").totalRecords).toBe(100);
+				expect(g.pagination("comments").totalRecords).toBe(30);
+				expect(g.pagination("comments").currentPage).toBe(3);
+			})
+
+		})
+
+	}
+}

vendor/wheels/tests/specs/controller/requestSpec.cfc

@@ -154,13 +154,16 @@ component extends="wheels.WheelsTest" {
 		describe("Tests that pagination", () => {
 
 			beforeEach(() => {
-				request.wheels["myhandle"] = {test = "true"}
+				application.wo.$ensurePaginationStore()
+				request.wheels["$pagination"]["myhandle"] = {test = "true"}
 				params = {controller = "dummy", action = "dummy"}
 				_controller = application.wo.controller("dummy", params)
 			})
 
 			afterEach(() => {
-				StructDelete(request.wheels, "myhandle", false)
+				// Delete only this spec's handle. The whole core suite runs in one request, so
+				// wiping the shared $pagination namespace would destroy other specs' handles.
+				StructDelete(request.wheels["$pagination"], "myhandle", false)
 			})
 
 			it("handle exists", () => {

vendor/wheels/tests/specs/model/crudSpec.cfc

@@ -1477,10 +1477,10 @@ component extends="wheels.WheelsTest" {
 					order = "id"
 				)
 
-				expect(request.wheels.pagination_test_1.CURRENTPAGE).toBe(1)
-				expect(request.wheels.pagination_test_1.TOTALPAGES).toBe(0)
-				expect(request.wheels.pagination_test_1.TOTALRECORDS).toBe(0)
-				expect(request.wheels.pagination_test_1.ENDROW).toBe(1)
+				expect(request.wheels["$pagination"].pagination_test_1.CURRENTPAGE).toBe(1)
+				expect(request.wheels["$pagination"].pagination_test_1.TOTALPAGES).toBe(0)
+				expect(request.wheels["$pagination"].pagination_test_1.TOTALRECORDS).toBe(0)
+				expect(request.wheels["$pagination"].pagination_test_1.ENDROW).toBe(1)
 				expect(e.recordcount).toBe(0)
 			})
 
@@ -1490,32 +1490,32 @@ component extends="wheels.WheelsTest" {
 				/* 1st page */
 				e = user.findAll(select = "id", perpage = "2", page = "1", handle = "pagination_test_2", order = "id")
 
-				expect(request.wheels.pagination_test_2.CURRENTPAGE).toBe(1)
-				expect(request.wheels.pagination_test_2.TOTALPAGES).toBe(3)
-				expect(request.wheels.pagination_test_2.TOTALRECORDS).toBe(5)
-				expect(request.wheels.pagination_test_2.ENDROW).toBe(2)
+				expect(request.wheels["$pagination"].pagination_test_2.CURRENTPAGE).toBe(1)
+				expect(request.wheels["$pagination"].pagination_test_2.TOTALPAGES).toBe(3)
+				expect(request.wheels["$pagination"].pagination_test_2.TOTALRECORDS).toBe(5)
+				expect(request.wheels["$pagination"].pagination_test_2.ENDROW).toBe(2)
 				expect(e.recordcount).toBe(2)
 				expect(e.id[1]).toBe(r.id[1])
 				expect(e.id[2]).toBe(r.id[2])
 
 				/* 2nd page */
 				e = user.findAll(perpage = "2", page = "2", handle = "pagination_test_3", order = "id")
 
-				expect(request.wheels.pagination_test_3.CURRENTPAGE).toBe(2)
-				expect(request.wheels.pagination_test_3.TOTALPAGES).toBe(3)
-				expect(request.wheels.pagination_test_3.TOTALRECORDS).toBe(5)
-				expect(request.wheels.pagination_test_3.ENDROW).toBe(4)
+				expect(request.wheels["$pagination"].pagination_test_3.CURRENTPAGE).toBe(2)
+				expect(request.wheels["$pagination"].pagination_test_3.TOTALPAGES).toBe(3)
+				expect(request.wheels["$pagination"].pagination_test_3.TOTALRECORDS).toBe(5)
+				expect(request.wheels["$pagination"].pagination_test_3.ENDROW).toBe(4)
 				expect(e.recordcount).toBe(2)
 				expect(e.id[1]).toBe(r.id[3])
 				expect(e.id[2]).toBe(r.id[4])
 
 				/* 3rd page */
 				e = user.findAll(perpage = "2", page = "3", handle = "pagination_test_4", order = "id")
 
-				expect(request.wheels.pagination_test_4.CURRENTPAGE).toBe(3)
-				expect(request.wheels.pagination_test_4.TOTALPAGES).toBe(3)
-				expect(request.wheels.pagination_test_4.TOTALRECORDS).toBe(5)
-				expect(request.wheels.pagination_test_4.ENDROW).toBe(5)
+				expect(request.wheels["$pagination"].pagination_test_4.CURRENTPAGE).toBe(3)
+				expect(request.wheels["$pagination"].pagination_test_4.TOTALPAGES).toBe(3)
+				expect(request.wheels["$pagination"].pagination_test_4.TOTALRECORDS).toBe(5)
+				expect(request.wheels["$pagination"].pagination_test_4.ENDROW).toBe(5)
 				expect(e.recordcount).toBe(1)
 				expect(e.id[1]).toBe(r.id[5])
 			})
@@ -1568,10 +1568,10 @@ component extends="wheels.WheelsTest" {
 					handle = "pagination_order_test_1"
 				)
 
-				expect(request.wheels.pagination_order_test_1.CURRENTPAGE).toBe(1)
-				expect(request.wheels.pagination_order_test_1.TOTALPAGES).toBe(13)
-				expect(request.wheels.pagination_order_test_1.TOTALRECORDS).toBe(250)
-				expect(request.wheels.pagination_order_test_1.ENDROW).toBe(20)
+				expect(request.wheels["$pagination"].pagination_order_test_1.CURRENTPAGE).toBe(1)
+				expect(request.wheels["$pagination"].pagination_order_test_1.TOTALPAGES).toBe(13)
+				expect(request.wheels["$pagination"].pagination_order_test_1.TOTALRECORDS).toBe(250)
+				expect(request.wheels["$pagination"].pagination_order_test_1.ENDROW).toBe(20)
 			})
 
 			it("works with renamed primary key", () => {
@@ -1590,10 +1590,10 @@ component extends="wheels.WheelsTest" {
 					where = "description1 LIKE '%photo%'"
 				)
 
-				expect(request.wheels.pagination_order_test_1.CURRENTPAGE).toBe(1)
-				expect(request.wheels.pagination_order_test_1.TOTALPAGES).toBe(13)
-				expect(request.wheels.pagination_order_test_1.TOTALRECORDS).toBe(250)
-				expect(request.wheels.pagination_order_test_1.ENDROW).toBe(20)
+				expect(request.wheels["$pagination"].pagination_order_test_1.CURRENTPAGE).toBe(1)
+				expect(request.wheels["$pagination"].pagination_order_test_1.TOTALPAGES).toBe(13)
+				expect(request.wheels["$pagination"].pagination_order_test_1.TOTALRECORDS).toBe(250)
+				expect(request.wheels["$pagination"].pagination_order_test_1.ENDROW).toBe(20)
 			})
 
 			it("works with parameterize set to false with numeric", () => {
@@ -1607,10 +1607,10 @@ component extends="wheels.WheelsTest" {
 					where = "id = 1"
 				)
 
-				expect(request.wheels.pagination_order_test_1.CURRENTPAGE).toBe(1)
-				expect(request.wheels.pagination_order_test_1.TOTALPAGES).toBe(1)
-				expect(request.wheels.pagination_order_test_1.TOTALRECORDS).toBe(1)
-				expect(request.wheels.pagination_order_test_1.ENDROW).toBe(1)
+				expect(request.wheels["$pagination"].pagination_order_test_1.CURRENTPAGE).toBe(1)
+				expect(request.wheels["$pagination"].pagination_order_test_1.TOTALPAGES).toBe(1)
+				expect(request.wheels["$pagination"].pagination_order_test_1.TOTALRECORDS).toBe(1)
+				expect(request.wheels["$pagination"].pagination_order_test_1.ENDROW).toBe(1)
 			})
 
 			it("works with compound keys", () => {

vendor/wheels/tests/specs/model/miscellaneousSpec.cfc

@@ -202,9 +202,9 @@ component extends="wheels.WheelsTest" {
 	function assert_pagination(required string handle) {
 		args = arguments
 
-		expect(request.wheels).toHaveKey(args.handle)
+		expect(request.wheels["$pagination"]).toHaveKey(args.handle)
 
-		p = request.wheels[args.handle]
+		p = request.wheels["$pagination"][args.handle]
 		StructDelete(args, "handle", false)
 
 		for (i in args) {

vendor/wheels/tests/specs/view/miscellaneousSpec.cfc

@@ -255,7 +255,8 @@ component extends="wheels.WheelsTest" {
 			})
 
 			it("is passing through class", () => {
-				request.wheels.testPaginationLinksQuery = {currentPage = 2, totalPages = 3}
+				g.$ensurePaginationStore()
+				request.wheels["$pagination"].testPaginationLinksQuery = {currentPage = 2, totalPages = 3}
 				r = g.controller("dummy").paginationLinks(classForCurrent = "active", handle = "testPaginationLinksQuery")
 
 				expect(r).toInclude("<span class=""active"">2</span>")