Skip to content

Commit ce47c80

Browse files
wheels-bot[bot]github-actions[bot]bpamiri
authored
fix(view): paginationNav rejects unknown sub-helper arguments (#2726)
* fix(view): paginationNav rejects unknown sub-helper arguments paginationNav() built a passthrough struct of unrecognized arguments and handed it to every sub-helper via argumentCollection. CFML's dispatch silently drops keys that aren't declared in the receiving signature, so typos like prependToList="<ul>" had no effect and gave the caller no signal that the argument was rejected. After this change, paginationNav() validates the passthrough struct against the union of its sub-helpers' declared arguments (and the URL-building keys forwarded by $paginationLinkToArgs). Any leftover keys throw Wheels.PaginationNav.InvalidArgument when application.wheels.showErrorInformation is on; the message lists both the rejected names and the full allowlist. Production environments (showErrorInformation = false) keep the silent-drop behavior so live traffic is unaffected. Fixes #2717 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> * docs: note paginationNav argument validation in CLAUDE.md Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> * fix(view): address Reviewer A/B consensus findings (round 1) - C1: filter out named-route segment variables (forwarded by $paginationLinkToArgs at link-build time) from the unknown-args list before throwing, so paginationNav(route="userTimeline", userId=...) is no longer a false-positive InvalidArgument (vendor/wheels/view/pagination.cfc) - C2: hoist the unknown-args validation above the totalPages early return so the check still fires on single-page (or empty) result sets — previously a typo against a small dataset was invisible (vendor/wheels/view/pagination.cfc) - T1: add a regression spec covering paginationNav(route=..., <segmentVar>=...) with showErrorInformation=true; verifies the C1 fix and would have caught the regression (vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc) Verified locally against Lucee 7 + SQLite: view layer 517 pass / 0 fail (was 516 before this commit; the new T1 spec adds one). Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> * refactor(view): gate paginationNav route-var lookup on showErrorInformation Round 2 polish from Reviewer A: the $findRoute filter was entered whenever local.unknownArgs was non-empty and a route was present, even in production where the final throw is gated. Nest both the route-var filter and the throw inside a single application.wheels.showErrorInformation guard so the production path skips the $findRoute lookup entirely. Behavior in dev mode is unchanged — same filter, same throw, same allowlist. Also documents the route-variable exemption in CLAUDE.md so a reader of the pagination section understands that named route segment variables are auto-exempted from the unknown-arg check. Local verification on Lucee 7 + SQLite (this worktree): view layer: 517 pass, 0 fail, 0 error security layer: 174 pass, 0 fail, 0 error full core suite: 3557 pass, 0 fail, 0 error Signed-off-by: Peter Amiri <peter@alurium.com> --------- Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Signed-off-by: Peter Amiri <peter@alurium.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Peter Amiri <peter@alurium.com>
1 parent f9a4312 commit ce47c80

4 files changed

Lines changed: 123 additions & 6 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ All historical references to "CFWheels" in this changelog have been preserved fo
3434

3535
### Fixed
3636

37+
- `paginationNav()` now throws `Wheels.PaginationNav.InvalidArgument` when passed an argument that none of its sub-helpers (`paginationInfo`, `firstPageLink`, `previousPageLink`, `pageNumberLinks`, `nextPageLink`, `lastPageLink`) accept. Previously, typos such as `prependToList="<ul>"` were silently dropped by CFML's `argumentCollection` dispatch, leaving users to wonder why a styling argument had no effect. The check is gated on `application.wheels.showErrorInformation` so production is unaffected; development environments fail fast and the error names both the rejected arguments and the full allowlist of accepted pass-through keys (#2717)
3738
- `wheels --help` no longer summarises the `packages` command as `Install, update, search Wheels packages` — that phrasing nudged users to type `wheels packages install <name>`, which LuCLI's built-in extension installer intercepts before module dispatch and silently no-ops (`[INFO] No git or extension dependencies to install`, exit 0, nothing under `vendor/`). The summary now leads with the canonical verb (`Add, update, search ...`) and parenthesises the gotcha so the doc surface stops contradicting the runtime. Same trap that earlier renamed `wheels browser install` to `wheels browser setup` (#2706)
3839
- `wheels.middleware.Cors` now emits `Vary: Origin` alongside the reflected `Access-Control-Allow-Origin` header so CDN, reverse-proxy, and browser disk caches key the response on the request Origin instead of serving a cached response with the wrong ACAO to a different origin. Matches the behavior of the legacy 3.x `Global.cfc::$setCORSHeaders` path (vendor/wheels/Global.cfc:3565). The header is only emitted when an origin is actually being reflected — wildcard (`allowOrigins="*"`) responses and disallowed-origin responses are unchanged (#2707)
3940
- `wheels.middleware.Cors` no longer emits the raw comma-delimited `allowOrigins` list as the `Access-Control-Allow-Origin` header value when a request arrives with no `Origin` header (same-origin, server-to-server, or curl-without-`-H`). Previously, the default `local.allowOrigin = variables.allowOrigins` seeded the raw list, and the `Origin`-header guard only reassigned it when an `Origin` was present — so multi-origin configurations like `allowOrigins="https://a.com,https://b.com"` shipped that exact string in the response header, violating the CORS spec requirement that `Access-Control-Allow-Origin` be a single origin or `*`. Origin resolution is now extracted into `$resolveAllowOrigin()` and only returns a value when the incoming `Origin` is in the allowlist (or when `allowOrigins == "*"`); same-origin and S2S responses no longer carry the header at all (#2704)

CLAUDE.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -476,6 +476,8 @@ Requires a paginated query: `findAll(page=params.page, perPage=25)`. The recomme
476476

477477
Disabled links render as `<span class="disabled">` by default. All helpers accept `handle` for named pagination queries.
478478

479+
In development (`showErrorInformation = true`), `paginationNav()` throws `Wheels.PaginationNav.InvalidArgument` if passed an argument not accepted by any of its sub-helpers. Accepted pass-through keys: `format`, `text`, `name`, `class`, `disabledClass`, `showDisabled`, `pageNumberAsParam`, `windowSize`, `classForCurrent`, `linkToCurrentPage`, `prependToPage`, `appendToPage`, `route`, `controller`, `action`, `key`, `anchor`, `onlyPath`, `host`, `protocol`, `port`, `params`. Named route segment variables (e.g., `userId` when `route="userTimeline"` and the pattern contains `[userId]`) are automatically exempted from this check. In production the unknown argument is silently dropped.
480+
479481
## Testing Quick Reference
480482

481483
**All new tests use WheelsTest BDD syntax.** RocketUnit (`test_` prefix, `assert()`) is legacy only — never use it for new tests.

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

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,10 +280,84 @@ component extends="wheels.WheelsTest" {
280280
expect(result).toInclude("custom-pagination")
281281
})
282282

283+
it("throws when passed an unknown argument and showErrorInformation is on", () => {
284+
_origShowErr = application.wheels.showErrorInformation
285+
application.wheels.showErrorInformation = true
286+
try {
287+
g.model("author").findAll(page = 2, perPage = 3, order = "lastName")
288+
expect(() => _controller.paginationNav(prependToList = "<ul>"))
289+
.toThrow(type = "Wheels.PaginationNav.InvalidArgument")
290+
} finally {
291+
application.wheels.showErrorInformation = _origShowErr
292+
}
293+
})
294+
295+
it("does not throw for documented sub-helper args", () => {
296+
_origShowErr = application.wheels.showErrorInformation
297+
application.wheels.showErrorInformation = true
298+
try {
299+
g.model("author").findAll(page = 2, perPage = 3, order = "lastName")
300+
expect(() => _controller.paginationNav(
301+
windowSize = 3,
302+
classForCurrent = "active",
303+
prependToPage = "<li>",
304+
appendToPage = "</li>",
305+
class = "page-link"
306+
)).notToThrow()
307+
} finally {
308+
application.wheels.showErrorInformation = _origShowErr
309+
}
310+
})
311+
312+
it("does not throw on unknown arg when showErrorInformation is off", () => {
313+
_origShowErr = application.wheels.showErrorInformation
314+
application.wheels.showErrorInformation = false
315+
try {
316+
g.model("author").findAll(page = 2, perPage = 3, order = "lastName")
317+
expect(() => _controller.paginationNav(prependToList = "<ul>"))
318+
.notToThrow()
319+
} finally {
320+
application.wheels.showErrorInformation = _origShowErr
321+
}
322+
})
323+
324+
it("does not throw when passing a named-route segment variable", () => {
325+
// Regression test for the C1 false positive: paginationNav(route=..., <segmentVar>=...)
326+
// must not trip InvalidArgument, because $paginationLinkToArgs forwards the route's
327+
// foundvariables to linkTo() at link-build time.
328+
_origShowErr = application.wheels.showErrorInformation
329+
_origRoutes = Duplicate(application.wheels.routes)
330+
_origStaticRoutes = StructKeyExists(application.wheels, "staticRoutes") ? StructCopy(application.wheels.staticRoutes) : {}
331+
_origNamedRoutePositions = StructKeyExists(application.wheels, "namedRoutePositions") ? StructCopy(application.wheels.namedRoutePositions) : {}
332+
_origRewrite = application.wheels.URLRewriting
333+
application.wheels.showErrorInformation = true
334+
try {
335+
$clearRoutes()
336+
g.mapper().$match(name = "userTimeline", pattern = "users/[userId]/timeline", to = "users##timeline").end()
337+
g.$setNamedRoutePositions()
338+
application.wheels.URLRewriting = "On"
339+
g.model("author").findAll(page = 2, perPage = 3, order = "lastName")
340+
expect(() => _controller.paginationNav(route = "userTimeline", userId = 42))
341+
.notToThrow()
342+
} finally {
343+
application.wheels.showErrorInformation = _origShowErr
344+
application.wheels.routes = _origRoutes
345+
application.wheels.staticRoutes = _origStaticRoutes
346+
application.wheels.namedRoutePositions = _origNamedRoutePositions
347+
application.wheels.URLRewriting = _origRewrite
348+
}
349+
})
350+
283351
})
284352

285353
})
286354

287355
}
288356

357+
public void function $clearRoutes() {
358+
application.wheels.routes = []
359+
application.wheels.staticRoutes = {}
360+
application.wheels.namedRoutePositions = {}
361+
}
362+
289363
}

vendor/wheels/view/pagination.cfc

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -336,23 +336,63 @@ component {
336336
any encode
337337
) {
338338
$args(name = "paginationNav", args = arguments);
339-
local.pg = pagination(arguments.handle);
340-
341-
// Return empty if only one page and showSinglePage is false
342-
if (local.pg.totalPages <= 1 && !arguments.showSinglePage) {
343-
return "";
344-
}
345339

346340
// Build passthrough arguments for sub-helpers
347341
local.subArgs = {};
348342
local.subArgs.handle = arguments.handle;
349343
local.subArgs.encode = arguments.encode;
350344
// Pass through any extra arguments (route, controller, action, key, params, etc.)
351345
local.skipArgs = "handle,navClass,showFirst,showLast,showPrevious,showNext,showInfo,showSinglePage,encode";
346+
// Union of args accepted by sub-helpers (paginationInfo, firstPageLink,
347+
// previousPageLink, pageNumberLinks, nextPageLink, lastPageLink) plus the
348+
// URL-building keys forwarded by $paginationLinkToArgs. Keys outside this
349+
// allowlist are silently dropped by CFML's argumentCollection dispatch,
350+
// which makes typos like prependToList="<ul>" invisible — see issue #2717.
351+
local.allowedSubArgs = "format,text,name,class,disabledClass,showDisabled,pageNumberAsParam"
352+
& ",windowSize,classForCurrent,linkToCurrentPage,prependToPage,appendToPage"
353+
& ",route,controller,action,key,anchor,onlyPath,host,protocol,port,params";
354+
local.unknownArgs = "";
352355
for (local.key in arguments) {
353356
if (!ListFindNoCase(local.skipArgs, local.key)) {
354357
local.subArgs[local.key] = arguments[local.key];
358+
if (!ListFindNoCase(local.allowedSubArgs, local.key)) {
359+
local.unknownArgs = ListAppend(local.unknownArgs, local.key);
360+
}
361+
}
362+
}
363+
// Validate before the totalPages early-return so the check fires on
364+
// single-page (or empty) result sets too. Gated on showErrorInformation
365+
// so production skips both the $findRoute lookup and the throw entirely.
366+
if (Len(local.unknownArgs) && application.wheels.showErrorInformation) {
367+
// Named-route segment variables (e.g. userId in route "userTimeline") are
368+
// forwarded by $paginationLinkToArgs at link-build time but are not in the
369+
// static allowlist. Filter them out before throwing — otherwise
370+
// paginationNav(route="userTimeline", userId=user.id) trips a false-positive
371+
// InvalidArgument.
372+
if (StructKeyExists(local.subArgs, "route") && Len(local.subArgs.route)) {
373+
local.routeVarList = $findRoute(argumentCollection = local.subArgs).foundvariables;
374+
local.filteredUnknown = "";
375+
for (local.uk in ListToArray(local.unknownArgs)) {
376+
if (!ListFindNoCase(local.routeVarList, local.uk)) {
377+
local.filteredUnknown = ListAppend(local.filteredUnknown, local.uk);
378+
}
379+
}
380+
local.unknownArgs = local.filteredUnknown;
355381
}
382+
if (Len(local.unknownArgs)) {
383+
Throw(
384+
type = "Wheels.PaginationNav.InvalidArgument",
385+
message = "paginationNav() received unknown argument(s): [#local.unknownArgs#].",
386+
detail = "Accepted pass-through arguments are: #local.allowedSubArgs#. paginationNav's own arguments are: #local.skipArgs#."
387+
);
388+
}
389+
}
390+
391+
local.pg = pagination(arguments.handle);
392+
393+
// Return empty if only one page and showSinglePage is false
394+
if (local.pg.totalPages <= 1 && !arguments.showSinglePage) {
395+
return "";
356396
}
357397

358398
local.content = "";

0 commit comments

Comments
 (0)