[wheels-bot]

Summary

Fix an open-redirect residual deferred from #2898: $isSafeRedirectUrl() ran its same-origin classification on raw input, so URLs containing leading whitespace or embedded ASCII tab/CR/LF (e.g. <TAB>//evil.com, /<LF>/evil.com) were classified as safe relative paths by the gate but navigated off-domain once the browser applied WHATWG URL normalization. Mirrored the browser normalization at the top of the function: strip embedded Chr(9)/Chr(10)/Chr(13), trim leading/trailing whitespace, and reject any URL retaining a C0 control character (Chr(0)-Chr(31) or Chr(127)). The new normalization runs ahead of the existing backslash check so post-strip backslashes are still caught, preserving the layered defense from #2898.

Related Issue

Closes #2972

Type of Change

• Bug fix
• New feature
• Enhancement to existing feature
• Documentation update
• Refactoring

Feature Completeness Checklist

• DCO sign-off — git commit -s; the trailer matches the configured claude[bot] author identity
• Tests — 9 specs added to vendor/wheels/tests/specs/controller/redirectionSpec.cfc covering Chr(9)/Chr(10)/Chr(13) in leading and embedded positions, leading ASCII space, leading Chr(0) (NUL), and embedded Chr(11) (vertical tab). Verified red→green locally on Lucee 7 + SQLite: pre-fix 8 of 9 fail with The incoming function did not throw an expected exception. Type=[Wheels.UnsafeRedirect]; post-fix all controller specs pass (HTTP 200 from /wheels/core/tests, 0 fail / 0 error)
• Framework Docs — handled by bot-update-docs.yml
• AI Reference Docs — handled by bot-update-docs.yml
• CLAUDE.md — handled by bot-update-docs.yml
• CHANGELOG.md — entry added to [Unreleased] → Security immediately above the fix(controller): reject backslash and schemeless open-redirect bypass in $isSafeRedirectUrl #2898 entry
• Test runner passes — curl http://localhost:60007/wheels/core/tests?db=sqlite&format=json&directory=wheels.tests.specs.controller returned HTTP 200 with no Failed/Error status records after the fix

Test Plan

1. Pre-fix red: 8 of the 9 new specs throw The incoming function did not throw an expected exception. Type=[Wheels.UnsafeRedirect] because $isSafeRedirectUrl() returns true for whitespace-bearing inputs (the leading-NUL case is incidentally rejected by upstream NUL stripping on Lucee, but is pinned regardless to guard against engine drift).
2. Apply the fix in vendor/wheels/controller/redirection.cfc:177-191.
3. Post-fix green: all 9 new specs throw Wheels.UnsafeRedirect; the existing #2898 specs and redirectTo happy-path specs (including the same-domain protocol-relative //<servername>/page allow case) still pass.

Cross-engine notes

• Public $-prefixed helper consistent with invariant New master - readme #7; no closures, no attributeCollection, no Adobe-only constructs.
• ReFind("[\x00-\x1F\x7F]", ...) uses hex character-class escapes that already ship in vendor/wheels/Global.cfc:2341, so the pattern is portable across Lucee 5/6/7, Adobe 2018-2025, and BoxLang.
• Trim() is unchanged engine behavior; Replace(..., "all") with Chr(9)/Chr(10)/Chr(13) is the standard cross-engine strip pattern (matches Migrator.cfc usage).
• Cross-engine matrix runs in per-PR CI.

[wheels-bot]

Wheels Bot — No doc updates

Reviewed this PR's diff and found no docs that need updating (internal security fix — the open-redirect normalization in $isSafeRedirectUrl() is not user-visible API; CHANGELOG.md is already updated in the PR; no existing guide covers the allowExternalRedirects gate; no pattern in .ai/wheels/ or CLAUDE.md changed).

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This PR correctly patches the remaining WHATWG-normalization open-redirect vector deferred from #2898: strip Chr(9)/Chr(10)/Chr(13), trim leading/trailing whitespace, reject any surviving C0 control. The implementation is clean and cross-engine portable, the attack-vector test coverage is solid, and the CHANGELOG and commit are well-formed. Two minor observations below — neither blocks merge.

---

Tests

Missing positive regression test for the strip-then-allow path.

The 9 new specs all verify rejection. There is no test confirming that a URL which passes through the normalization step and is legitimately same-origin is still allowed. For example:

it("allows redirectTo url with leading tab that trims to a relative path", () => {
    // Chr(9) is stripped; "/relative/path" is a safe relative URL.
    expect(function(){
        _controller.redirectTo(url = Chr(9) & "/relative/path")
    }).notToThrow()
})

Without this, a future change that accidentally inverts the strip-then-classify order (classify first, then strip) would not be caught by the new specs — the attack-vector tests would still pass because the existing relative-path classification would reject Chr(9) & "//evil.com" the same way. A single "allows" case pins the semantics.

---

Correctness

Vertical-tab test comment is slightly inaccurate.

vendor/wheels/tests/specs/controller/redirectionSpec.cfc:389–394

it("throws on redirectTo url with embedded vertical-tab in protocol-relative path", () => {
    // Vertical tab (Chr(11)) is a C0 control not stripped by browsers but
    // can confuse the same-origin classifier. Reject outright.
    expect(function(){
        _controller.redirectTo(url = "/" & Chr(11) & "/evil.com")
    }).toThrow("Wheels.UnsafeRedirect")
})

"/" & Chr(11) & "/evil.com" does not confuse the same-origin classifier — it would be classified as a relative URL (Left(url, 1) == "/" → return true) and allowed without the guard. The risk is that WHATWG percent-encodes Chr(11) to %0B rather than stripping it, so browsers navigate to /%0B/evil.com (a relative path) not //evil.com. In practice the guard prevents both scenarios, and rejecting all C0 controls is the right call. The comment is just inaccurate about the mechanism: "can confuse the same-origin classifier" → something like "C0 control in a URL is malformed per RFC 3986; browser percent-encoding behavior is engine-specific, so reject rather than classify" would be more precise.

---

Conventions, Cross-engine, Security, Commits, Docs

All clean:

• $-prefixed public helper — correct per CLAUDE.md invariant #7.
• Replace(url, Chr(9), "", "all") / Trim() / ReFind("[\x00-\x1F\x7F]", ...) — all standard CFML. The hex-escape character-class pattern has prior art in vendor/wheels/Global.cfc:2341 ([^\x00-\x80]), confirming portability across Lucee 5/6/7, Adobe 2018–2025, and BoxLang.
• No closures, no attributeCollection, no Adobe-conflicting constructs.
• CHANGELOG [Unreleased] → Security entry present and correctly positioned above the #2898 entry.
• Commit fix(controller): normalize whitespace and control chars in $isSafeRedirectUrl (76 chars) conforms to commitlint. DCO trailer present (Signed-off-by: claude[bot]).
• Three-file change set (implementation + test + changelog) — appropriately minimal.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is accurate and well-reasoned. Both findings check out against the actual diff; no sycophancy or false positives detected. I'm aligned on the verdict. Emitting converged-changes so the address-review bot can apply the two concrete fixes.

Sycophancy

None detected. Each positive assertion in A's review is backed by specific evidence (regex prior-art citation, cross-engine check, commit header char count). A issues a COMMENTED verdict rather than APPROVE, which is appropriate given two real findings.

False positives

None detected.

• Finding 1 (missing positive test): Verified against the diff — all 9 new specs exercise rejection paths only. A is correct that a strip-then-allow test (e.g., Chr(9) & "/relative/path" normalises to a legitimate relative URL and should pass) is absent. A's reasoning holds: if the strip-then-classify order were accidentally inverted in a future change, the attack-vector rejection tests would still pass (because an unstripped Chr(9) & "//evil.com" fails classification for other reasons), leaving only the allow-path test to catch the regression.

• Finding 2 (inaccurate VT comment): Verified. The test URL is "/" & Chr(11) & "/evil.com". After the three-character global strips and Trim() (which only removes leading/trailing bytes up to 0x20 — Chr(11) is not a leading character here), Chr(11) remains embedded. The ReFind("[\x00-\x1F\x7F]", ...) check fires and returns false — that is the mechanism, not any classifier confusion. A is correct that without the C0 guard, Left(url, 1) == "/" would classify this as a same-origin relative path and allow it. The comment is inaccurate about why rejection is necessary. (The browser side: WHATWG percent-encodes VT as %0B rather than stripping it, so the path becomes /%0B/evil.com, not an open redirect — yet malformed and right to reject.)

Missed issues

None significant. One trivial arithmetic note not worth acting on: A states the commit header is 76 chars; the actual count is 77. Both are well under the 100-char limit so this has no effect.

Verdict alignment

A's COMMENTED verdict is consistent with the findings. Neither finding is a correctness defect in the security logic — the implementation is sound. Two concrete, actionable improvements remain.

Convergence

Aligned. A's two findings are both valid, and the suggested fixes are well-specified: add a single notToThrow spec for a tab-prefixed legitimate relative URL, and update the VT test comment to describe the C0 rejection mechanism rather than classifier confusion. Per the convergence rules, COMMENTED with concrete actionable findings maps to converged-changes.