Repository navigation
Conversation
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>
Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Docs updatedAdded a doc commit to this PR:
|
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The PR correctly diagnoses the silent-drop problem and the gating on showErrorInformation is the right production-safety call. However, the static allowlist in allowedSubArgs does not account for arbitrary route-segment variables (e.g., userId, slug) that callers legitimately pass to paginationNav() when using named routes with URL parameters. Those arguments will be falsely thrown as InvalidArgument in development mode, which is a functional regression for any application that paginates over named route URLs. Additionally, the validation is unreachable on single-page result sets because the early-return guard runs first. This review is submitted as a comment because GitHub does not permit self-reviews — the findings below are change requests in intent.
Correctness
C1 — Route-variable false positives (vendor/wheels/view/pagination.cfc, lines 357–368)
The allowlist is a static union of sub-helper parameter names plus the URL-building keys forwarded by $paginationLinkToArgs. But $paginationLinkToArgs has a second pass-through mechanism (lines 461–468) for route segment variables:
local.routeVars = ListToArray(local.routeConfig.foundvariables);
for (local.key in local.routeVars) {
if (StructKeyExists(arguments.args, local.key) && local.key != arguments.name) {
local.linkArgs[local.key] = arguments.args[local.key];
}
}A caller using a named route with segment variables must pass those variables to paginationNav():
##paginationNav(route="userTimeline", userId=user.id)##
userId ends up in arguments, is not in skipArgs, is not in allowedSubArgs, and therefore gets appended to local.unknownArgs. The validation then throws Wheels.PaginationNav.InvalidArgument even though the call is correct. This is a regression for any application that uses route-based pagination with URL segment parameters.
Suggested fix — before deciding to throw, filter out keys that appear in the route's foundvariables:
if (Len(local.unknownArgs) && StructKeyExists(local.subArgs, "route") && Len(local.subArgs.route)) {
local.routeVarList = $findRoute(argumentCollection = local.subArgs).foundvariables;
local.filteredUnknown = "";
for (local.uk in ListToArray(local.unknownArgs)) {
if (!ListFindNoCase(local.routeVarList, local.uk)) {
local.filteredUnknown = ListAppend(local.filteredUnknown, local.uk);
}
}
local.unknownArgs = local.filteredUnknown;
}
if (Len(local.unknownArgs) && application.wheels.showErrorInformation) {
Throw(...);
}C2 — Validation is unreachable for single-page (or empty) results (vendor/wheels/view/pagination.cfc, lines 342–375)
The unknown-arg check runs after the early return:
local.pg = pagination(arguments.handle);
if (local.pg.totalPages <= 1 && !arguments.showSinglePage) {
return ""; // returns here; unknownArgs loop never runs
}
// ... allowedSubArgs build, unknownArgs check ...A developer who passes a misspelled argument against a table with fewer records than perPage will never see the InvalidArgument error, defeating the purpose of developer-mode validation. The argument loop should be hoisted above the early return so the check fires regardless of page count.
Tests
T1 — No test for named-route segment variables (vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc)
The three new specs cover the typo case and the production silent-drop fallback, but there is no test for passing a legitimate route variable alongside route=:
paginationNav(route="someNamedRoute", userId=someUser.id)A test asserting this does not throw would directly catch the regression in C1 and would have blocked the merge. This test is required alongside the fix for C1.
T2 — Minor: _origShowErr written to variables scope without var (lines 284, 296, 313)
An unscoped assignment inside an it() closure writes to the enclosing CFC's shared variables scope. The try/finally block ensures the value is always restored, so no real failure results in synchronous runs, and the existing tests in this file follow the same pattern. Noting for awareness rather than requiring a change.
Docs
The PR checklist marks framework and AI reference docs as handled separately by bot-update-docs.yml. Acceptable. The CHANGELOG entry under [Unreleased] > Fixed accurately describes the change and is consistent in verbosity with the adjacent entries.
Commits
Single commit fix(view): paginationNav rejects unknown sub-helper arguments — type fix, clear subject within 100 chars, Signed-off-by trailer present. Commit message correctly explains the why (CFML's argumentCollection silent-drop). DCO satisfied.
Security
No concerns. The interpolated unknown-arg names in the error message are only reachable when showErrorInformation = true (development mode), where error output is already exposed to developers.
Wheels Bot — Reviewer B (round 1)A's review is accurate. Both correctness findings (C1 and C2) are confirmed against the actual code, the suggested fixes are sound, and the verdict aligns with the findings. No sycophancy, no false positives, no missed issues. One housekeeping note: A's review marker cites SHA SycophancyNone detected. A opened with a fair positive framing of the overall approach, but immediately raised two substantive correctness issues that are change-blocking. No unsubstantiated LGTM. False positivesNone detected.
Missed issuesNone that warrant blocking. One minor observation A did not call out: the Verdict alignmentA stated "change requests in intent" and listed two correctness bugs plus a test gap. That is consistent with a request-changes verdict. Correct. ConvergenceAligned. Both C1 (route-variable false positives) and C2 (validation bypassed by early return) are confirmed and need to be addressed before this merges, along with the T1 test for the C1 fix. A's suggested approach for each is sound. Emitting |
- 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>
Wheels Bot — Address Review (round 1)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):
Skipped findings (not in the consensus to act on):
Local test results (Lucee 7 + SQLite, view layer): The view layer was 516/0/0 before this commit; the new T1 spec adds one. The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on `approve` or the outer-loop cap (5 rounds) is reached. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Round 1 fixes correctly address both consensus findings from the prior review cycle. C1 (route-variable false positives) is resolved via the $findRoute filter, C2 (validation unreachable on single-page result sets) is resolved by hoisting the check above the early return, and the missing regression test (T1) is now present and well-structured. No new correctness, cross-engine, or security issues were introduced. Submitting as a comment; findings below are nits only.
Correctness
C1 resolved — route-variable filter is correct (vendor/wheels/view/pagination.cfc, lines 368-376)
The new filter correctly calls $findRoute(argumentCollection = local.subArgs) using the already-built local.subArgs struct (which contains route and all route-segment variables) to retrieve foundvariables, then removes matching entries from local.unknownArgs before the throw decision. Verified against $findRoute's implementation in Global.cfc (open-args signature, uses only arguments.route and arguments.method): extra keys like handle and encode that land in local.subArgs are silently ignored. Logic is sound.
C2 resolved — early return hoisted correctly (vendor/wheels/view/pagination.cfc, lines 388-393)
local.pg = pagination(arguments.handle) and the totalPages <= 1 guard now run after the full validation block. A one-page result set in development mode will now surface the InvalidArgument error as intended.
Minor: $findRoute is called regardless of showErrorInformation (vendor/wheels/view/pagination.cfc, lines 368-376)
The route-variable filter is entered whenever Len(local.unknownArgs) && ... route ... is true, with no showErrorInformation gate. In production (showErrorInformation = false), this causes a $findRoute lookup that ultimately makes no difference (the final Throw() is still gated). The wasted work is negligible. The behavioral side-effect is also negligible: a non-existent route name in production was already a 500 waiting to happen at link-build time; it now surfaces slightly earlier. Not a blocker, but gating the filter on showErrorInformation would make the production path cleaner:
// Proposed tightening (optional)
if (Len(local.unknownArgs) && application.wheels.showErrorInformation) {
if (StructKeyExists(local.subArgs, "route") && Len(local.subArgs.route)) {
// filter route vars before throwing
...
}
if (Len(local.unknownArgs)) {
Throw(...);
}
}Tests
T1 resolved -- route-segment regression test is well-structured (vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc, lines 324-349)
The test correctly:
- Backs up and restores
application.wheels.routes,staticRoutes,namedRoutePositions,URLRewriting, andshowErrorInformationin afinallyblock. - Calls the local
$clearRoutes()helper (correctly defined at line 357 per the CLAUDE.md pattern fromlinksSpec.cfc). - Registers a
userTimelineroute with pattern"users/[userId]/timeline"so that$stripRouteVariablesproducesfoundvariables = "userId". - Calls
g.$setNamedRoutePositions()to rebuild the name-to-position index after the route is registered. - Asserts
notToThrow(), which is the minimal correct assertion for this regression.
One nit: all the _orig* save variables (_origShowErr, _origRoutes, _origStaticRoutes, _origNamedRoutePositions, _origRewrite) are assigned without var, writing to the enclosing CFC's variables scope. This is consistent with the existing pattern in this file (acknowledged as pre-existing in T2 of the prior round) and is safe for synchronous test runs. Flagging for awareness only.
Docs
CLAUDE.md update accurate, but missing route-variable nuance (CLAUDE.md, pagination section)
The new sentence correctly lists all 22 accepted pass-through keys and notes the showErrorInformation gate. However, it does not mention that named route segment variables (e.g., userId in paginationNav(route="userTimeline", userId=user.id)) are NOT considered unknown arguments. A developer reading only the CLAUDE.md note could reasonably conclude that userId would throw since it is absent from the listed keys. Suggested addition:
Named route segment variables (e.g.,
userIdwhenroute="userTimeline"and the pattern contains[userId]) are automatically exempted from this check and will not throw.
The CHANGELOG entry accurately describes the shipped behavior and is consistent in verbosity with adjacent entries.
Commits
Three commits on this branch:
| SHA | Message |
|---|---|
6c37756a8 |
fix(view): paginationNav rejects unknown sub-helper arguments |
a652ddbe7 |
docs: note paginationNav argument validation in CLAUDE.md |
8d82bc6eb |
fix(view): address Reviewer A/B consensus findings (round 1) |
All conform to commitlint: valid types (fix, docs), scope view is apt, subjects are sentence-case and under 100 characters. Signed-off-by: trailers present (DCO satisfied). No concerns.
Security
No concerns. The local.unknownArgs value interpolated into the error message consists of CFML argument names from the developer-authored template, not user-supplied HTTP input. The error is only reachable in development mode (showErrorInformation = true). No XSS risk in production.
Wheels Bot — Reviewer B (round 1)A's review is accurate and the verdict is appropriate. All findings are verified against the diff. No sycophancy, no false positives, no missed issues of substance. Emitting SycophancyNone detected. A opens with a positive TL;DR but immediately identifies a genuine code-path concern (the unconditional False positivesNone detected.
Missed issuesNone that warrant blocking. One minor observation A did not raise: Verdict alignmentA submitted as a comment with findings labeled "nits only." Given that C1 and C2 are confirmed resolved, the remaining items are a code-quality suggestion (optional ConvergenceAligned. The implementation is correct, the regression test is present, and no new issues were introduced. A's optional suggestion and docs note are reasonable improvements but not blockers. Joint recommendation: approve. |
…mation 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>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Round 2 polish commit (56c95b4ca) addresses the one open nit from the previous review cycle — gating the $findRoute call inside the showErrorInformation block so the production path skips both the lookup and the throw entirely. The restructuring is correct, the CLAUDE.md doc is now complete, and no new issues were introduced. No findings — this diff is clean.
Correctness
Round 2 nit resolved — $findRoute is now gated on showErrorInformation (vendor/wheels/view/pagination.cfc, lines 366–388)
The previous round's nit noted that the route-var filter entered $findRoute whenever local.unknownArgs was non-empty and a route was present, even in production where the final throw was already gated. The fix nests both the filter and the throw inside the outer showErrorInformation guard:
if (Len(local.unknownArgs) && application.wheels.showErrorInformation) {
if (StructKeyExists(local.subArgs, "route") && Len(local.subArgs.route)) {
local.routeVarList = $findRoute(argumentCollection = local.subArgs).foundvariables;
// ... filter local.unknownArgs ...
}
if (Len(local.unknownArgs)) {
Throw(type = "Wheels.PaginationNav.InvalidArgument", ...);
}
}Production path: local.unknownArgs is built by the for loop (negligible — the loop runs anyway to populate local.subArgs), but neither $findRoute nor Throw are invoked. Dev path: same filter, same throw, no behavioral change. Logic is sound.
One observation worth noting but not worth holding: local.unknownArgs is accumulated in the for loop unconditionally (outside the showErrorInformation guard), so there is a small string-building cost per request even in production. Given the loop already iterates over arguments to build local.subArgs, this is genuinely negligible. Not a blocker.
Docs
CLAUDE.md updated with route-variable exemption (CLAUDE.md, Pagination View Helpers section)
The paragraph now includes the suggested sentence:
Named route segment variables (e.g.,
userIdwhenroute="userTimeline"and the pattern contains[userId]) are automatically exempted from this check. In production the unknown argument is silently dropped.
This closes the documentation gap flagged in round 2 — a developer reading only CLAUDE.md can now understand that paginationNav(route="userTimeline", userId=user.id) is valid and will not throw.
Commits
Four commits total on this branch:
| SHA | Message |
|---|---|
6c37756a8 |
fix(view): paginationNav rejects unknown sub-helper arguments |
a652ddbe7 |
docs: note paginationNav argument validation in CLAUDE.md |
8d82bc6eb |
fix(view): address Reviewer A/B consensus findings (round 1) |
56c95b4ca |
refactor(view): gate paginationNav route-var lookup on showErrorInformation |
All conform to commitlint: valid types (fix, docs, refactor), scope view is apt throughout, subjects are sentence-case and under 100 characters. Signed-off-by: trailers present on all commits (DCO satisfied). The round 2 commit message correctly names the Reviewer A nit it addresses and describes the behavioral invariant (same filter, same throw, same allowlist in dev mode). No concerns.
Security
No concerns. The interpolated local.unknownArgs value in the error message is composed of CFML argument names from developer-authored templates, not user-supplied HTTP input. The error is only reachable under showErrorInformation = true (development mode). Production is unaffected.
Wheels Bot — Reviewer B (round 1)A's review of One procedural note: A submitted as a SycophancyNone detected. A opens with a positive TL;DR but supports every claim with specific lines from the diff (lines 366-388 for the guard restructure, the CLAUDE.md hunk, the commit table). No bare LGTM. False positivesNone detected.
Missed issuesNone that warrant blocking. One pre-existing nit already on record from B's round 1 on SHA No cross-engine concerns: Verdict alignmentA filed as ConvergenceAligned. The implementation is correct -- |
…ationnav-silently-discards-unknown-arguments # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The new head is a merge commit (739651664) that pulls origin/develop into the PR branch and resolves a single CHANGELOG conflict. No files touched by the PR's own implementation (vendor/wheels/view/pagination.cfc, vendor/wheels/tests/specs/view/paginationHelpersSpec.cfc, CLAUDE.md) were changed by the merge. The CHANGELOG resolution is correct — both the paginationNav entry (from this branch) and the develop entries are present in the final file. No new correctness, cross-engine, security, or commit-style issues were introduced. Submitting as a comment; GitHub blocks self-review approvals since wheels-bot is also the PR author.
Correctness
Merge conflict resolution is accurate (CHANGELOG.md, [Unreleased] > Fixed section)
The diff --cc shows the paginationNav entry (authored on this branch) was added as the first item under Fixed, with the develop-side entries following it in the correct order. No entries were dropped or duplicated. Resolution is clean.
No other files changed in the merge commit. The implementation in pagination.cfc, the four specs in paginationHelpersSpec.cfc, and the CLAUDE.md update all carry over unchanged from commits 6c37756-56c95b4 (reviewed in prior rounds).
One standing observation (not a blocker, noted in round 3): if a caller simultaneously passes a mistyped route name and an unknown argument, the showErrorInformation block enters $findRoute and throws Wheels.RouteNotFound before it can reach the Wheels.PaginationNav.InvalidArgument throw. This edge case existed before this PR (the same $findRoute call fires at link-build time in production) and the developer-facing error is still actionable. No new regression.
Commits
Five commits on the branch at this head:
| SHA | Author | Message |
|---|---|---|
6c37756 |
claude[bot] | fix(view): paginationNav rejects unknown sub-helper arguments |
a652ddbe |
claude[bot] | docs: note paginationNav argument validation in CLAUDE.md |
8d82bc6e |
claude[bot] | fix(view): address Reviewer A/B consensus findings (round 1) |
56c95b4c |
Peter Amiri | refactor(view): gate paginationNav route-var lookup on showErrorInformation |
73965166 |
Peter Amiri | Merge origin/develop into branch |
All bot-authored commits carry Signed-off-by: claude[bot] trailers; the human-authored 56c95b4c carries Signed-off-by: Peter Amiri. The merge commit is a standard Merge ... commit; the DCO GitHub App exempts merge commits from the trailer requirement. DCO satisfied across all five commits. All conventional-commit subjects conform to commitlint (valid types, sentence-case, under 100 chars).
Security
No concerns. The interpolated local.unknownArgs value in the error message is composed of CFML argument names from developer-authored templates, not user-supplied HTTP input. The error path is gated on application.wheels.showErrorInformation = true (development mode). Production is unaffected.
Wheels Bot — Reviewer B (round 1)A's core conclusion (no implementation blockers) is correct, but A made a factual error in characterizing the merge commit scope that should be on record. One false positive, no missed blocking issues, verdict alignment is appropriate. Converging approve. SycophancyNone detected. A opened with a positive TL;DR but immediately qualified it by noting one standing edge-case observation and correctly explaining the self-review restriction. No bare LGTM. False positivesOne, notable.
Missed issuesNone that warrant blocking. The 23 develop-side files the merge introduced (deploy CLI refactors, docs, visual baseline) are unrelated to this PR's implementation and don't conflict with the paginationNav changes. The standing edge-case about Verdict alignmentA submitted as a COMMENT with no blockers identified, and correctly explained that GitHub prevents self-review approvals. Consistent with the findings. ConvergenceAligned. A's false positive is a scope description error, not an implementation quality error — correcting it does not change the verdict. Even with the full 24-file scope understood, the paginationNav implementation files are unchanged, all prior consensus findings (C1 route-variable false positives, C2 validation-before-early-return, T1 regression spec) were addressed and confirmed in earlier rounds, and no new issues were introduced by the merge. Joint recommendation: approve. |
…keys Reviewer B (round 1) round 5 noted that the `Accepted pass-through keys` list in the Pagination View Helpers section of CLAUDE.md omits `addActiveClassToPrependedParent`, even though `local.allowedSubArgs` in `paginationNav()` already accepts it (the merge with #2726 added it to the allowlist). Without this, a developer scanning only the list would conclude the keyword fails validation when in fact it passes through to `pageNumberLinks()`. One-word addition to keep the list aligned with the code. Signed-off-by: Peter Amiri <peter@alurium.com>
…er-anchor wrappers (#2730) * fix(view): paginationNav now accepts Bootstrap-style prepend/append/per-anchor wrappers paginationNav() gains prepend / append (outer wrappers inside <nav>), prependToPage / appendToPage (per-anchor wrappers that now apply to first / previous / next / last as well as the numbered links), addActiveClassToPrependedParent (injects "active " into the current page's prependToPage class= attribute), and anchorDivider. pageNumberLinks() also gains addActiveClassToPrependedParent so the same idiom flows through when helpers are composed by hand. Bootstrap-styled 3.x apps can now do a like-for-like swap from paginationLinks() to paginationNav(). Fixes #2715 Signed-off-by: wheels-bot[bot] <wheels-bot[bot]@users.noreply.github.com> * docs(web/guides): document paginationNav Bootstrap args and update CLAUDE.md Add Bootstrap 5 like-for-like swap example to the 3x-to-4x upgrade guide's paginationLinks() section, and update CLAUDE.md Pagination View Helpers with the new prepend/append/prependToPage/appendToPage/addActiveClassToPrependedParent/ anchorDivider args added in #2715. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> * fix(view): scrub event handlers from paginationNav prependToPage/appendToPage paginationNav() is marketed as the canonical migration target for paginationLinks(), but the new $paginationWrapAnchor() helper (added in #2730) returned arguments.prependToPage & anchor & arguments.appendToPage verbatim, so a user who swapped paginationLinks() for paginationNav() silently lost the entity-decode + on\w+= / javascript: scrub that paginationLinks() applies in vendor/wheels/view/links.cfc. The numbered page wrappers in pageNumberLinks() had the same gap. Centralise the scrub in $paginationSanitizeWrapper() (decode HTML numeric entities first so &##111;nmouseover-style payloads cannot bypass the regex pass, then strip event-handler attributes and javascript: URIs). Call it from $paginationWrapAnchor() for the first/prev/next/last anchors and from the top of pageNumberLinks() so the numbered-page loop is covered too. The helper mirrors the parallel scrub in links.cfc so both surfaces share one audit point. Strengthen the anchorDivider spec from expect(toInclude("|")), which could match a stray | anywhere in a URL or page-number text, to a sentinel ("XDIVX") plus a placement assertion that it must appear between a closing tag and the next opening tag. Note in the doc-comment for @addActiveClassToPrependedParent on both pageNumberLinks() and paginationNav() that the option no-ops when prependToPage contains no class attribute. Add six new specs to PaginationXssSpec covering paginationNav with: decimal/hex entity-encoded onmouseover in prependToPage, entity-encoded javascript: URI, entity-encoded onmouseover in appendToPage, plain onmouseover without encoding, and a benign class/id passthrough. Local results (Lucee 7 + SQLite): paginationHelpersSpec: pass=34 fail=0 error=0 security suite: pass=180 fail=0 error=0 controller suite: pass=423 fail=0 error=0 view suite: pass=518 fail=0 error=0 Addresses Reviewer A and Reviewer B findings on #2730. Signed-off-by: Peter Amiri <peter@alurium.com> * refactor(view): hoist paginationNav XSS scrub and document unsanitized prepend/append Reviewer A non-blocking nits on PR #2730 head SHA 3c504bb: 1. `$paginationSanitizeWrapper()` was called up to four times on the same input from `paginationNav()`'s four `$paginationWrapAnchor()` sites. Hoist the scrub to the top of `paginationNav()`, overwrite `arguments.prependToPage` / `arguments.appendToPage` with the sanitized values once, and strip the per-call scrub from `$paginationWrapAnchor()` (which is internal and only invoked from here). Single audit point now lives at the `paginationNav()` entry. `pageNumberLinks()` keeps its own top-of-function scrub for defense-in-depth on direct callers; the redundant idempotent pass when invoked through `paginationNav()` is acceptable. 2. The asymmetry between sanitized `prependToPage`/`appendToPage` and unsanitized `prepend`/ `append` was intentional but undocumented. Add an inline comment at the content-assembly site in `paginationNav()` spelling out the rationale (developer-authored structural markup vs. theme-author / CMS per-page extension point) so a future contributor doesn't conclude the protection contract applies uniformly. 3. The `@addActiveClassToPrependedParent` doc-tag on `paginationNav()` already said it forwarded to `pageNumberLinks`, but didn't make explicit that first / previous / next / last anchors never receive the `active ` injection (they are never "current" in the Bootstrap sense). Add that clarification. 4. Add two direct `pageNumberLinks(prependToPage = ...)` XSS specs to PaginationXssSpec — an entity-encoded handler and a plain handler — so the CHANGELOG claim that `pageNumberLinks()` itself scrubs has a direct regression anchor independent of `paginationNav()` coverage. Local results (Lucee 7 + SQLite): security suite: pass=182 fail=0 error=0 (was 180 — +2 pageNumberLinks direct specs) view suite: pass=518 fail=0 error=0 (unchanged) No behavior change for any user-facing API. Sanitization continues to apply to both code paths; the scrub site just moves to a single location. Direct callers of `$paginationWrapAnchor()` (an internal `$`-prefixed helper) MUST pre-sanitize per the updated doc-comment. Signed-off-by: Peter Amiri <peter@alurium.com> * docs: list addActiveClassToPrependedParent in CLAUDE.md pass-through keys Reviewer B (round 1) round 5 noted that the `Accepted pass-through keys` list in the Pagination View Helpers section of CLAUDE.md omits `addActiveClassToPrependedParent`, even though `local.allowedSubArgs` in `paginationNav()` already accepts it (the merge with #2726 added it to the allowlist). Without this, a developer scanning only the list would conclude the keyword fails validation when in fact it passes through to `pageNumberLinks()`. One-word addition to keep the list aligned with the code. Signed-off-by: Peter Amiri <peter@alurium.com> --------- Signed-off-by: wheels-bot[bot] <wheels-bot[bot]@users.noreply.github.com> Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Signed-off-by: Peter Amiri <peter@alurium.com> Co-authored-by: wheels-bot[bot] <wheels-bot[bot]@users.noreply.github.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Peter Amiri <peter@alurium.com>
Summary
paginationNav()built a passthrough struct of every argument it did notrecognize as its own and handed it to each sub-helper via
argumentCollection. CFML's dispatch silently drops keys that are notdeclared in the receiving function's signature, so typos such as
prependToList="<ul>"had no effect and gave the caller no signal thatthe argument was rejected. This PR adds validation: when
application.wheels.showErrorInformationis on, any unrecognizedpass-through argument now throws
Wheels.PaginationNav.InvalidArgumentwith a message listing both the rejected names and the full allowlist of
accepted keys. Production environments (
showErrorInformation = false)keep the silent-drop behavior so live traffic is unaffected.
Related Issue
Closes #2717
Type of Change
Feature Completeness Checklist
Signed-off-by:(usegit commit -s)vendor/wheels/tests/specs/view/paginationHelpersSpec.cfccovering the rejection (failing → passing), the documented-args happy path, and the production-mode silent-drop fallbackbot-update-docs.ymlbot-update-docs.ymlbot-update-docs.yml[Unreleased] > Fixed516 pass, 0 fail, 0 errorfor the view layer (was515 pass, 1 failbefore the implementation, confirming the TDD red→green cycle)Test Plan
#paginationNav(prependToList="<ul>")#showErrorInformation = true) this now throwsWheels.PaginationNav.InvalidArgument; in production the call returnsthe rendered nav unchanged, matching the prior behavior.