fix(dispatch): require POST+password for /wheels/cli destructive ops, contain dbDump

[bpamiri]

Summary

Hardens the /wheels/cli CLI/MCP bridge endpoint. State-changing commands (dbReset drops every table, plus migrate/seed/rollback/jobs maintenance, etc.) were reachable over unauthenticated GET — CSRF-fireable from any page a developer visits via <img src=".../wheels/cli?command=dbReset">. They now require POST + loopback + the reload password (the established consoleeval.cfm pattern, fail-closed when no password is configured, constant-time SHA-256 compare). The gate lives in cli.cfm's preamble so it covers both the new POST route and the legacy ?controller=wheels&view=cli dispatch path. dbDump --output path traversal is contained to the web root, dbShell's dead self-referential SQL branch is removed, dbStatus now reports the migrator's real status field, and per-request migration discovery is lazy/memoized so 3-second job-worker polls no longer pay O(N) $dbinfo round-trips.

The wheels CLI (cli/lucli) is updated in the same commit: Module.cfc and MigrationRunner.cfc send mutating bridge commands as POST with the auto-detected reload password in the form body, so wheels migrate/seed/forget/pretend keep working against the gated endpoint. Deliberate trade-offs: the recommended extra ?confirm=1 flag is omitted (the only legitimate client is non-interactive and would send it unconditionally), and the deprecated CommandBox CLI (cli/src) still sends GET for mutating commands and now receives a clear 405 explaining the upgrade path — closing the GET path is the fix.

Findings addressed

• SEC-4 / public-ui P7 [High]: /wheels/cli runs destructive DB operations over an unauthenticated GET request @ vendor/wheels/public/views/cli.cfm:14-37 (gate invocation), vendor/wheels/Public.cfc:55 ($cliCommandIsMutating()), vendor/wheels/Public.cfc:76 ($cliMutationGateCheck()), vendor/wheels/public/routes.cfm:28 (new POST route); CLI side cli/lucli/Module.cfc:6174 (makeBridgePost()), cli/lucli/services/MigrationRunner.cfc:94-108.
• SEC-5 / public-ui P8 [Medium]: dbDump writes the dump to an attacker-controlled path with no containment check @ vendor/wheels/Public.cfc:150 ($cliResolveDumpPath() — canonicalize + prefix-confine to the web root, the guideImage pattern), caller vendor/wheels/public/views/cli.cfm:750 refuses to write on empty resolution.
• P1 [Medium]: dbShell SQL pass-through reuses the dispatch param command, making the branch self-referential and always erroring @ vendor/wheels/public/views/cli.cfm:802-834 — dead branch removed with an explanatory comment; H2 shell help text preserved.
• P3 [Medium]: dbStatus discards the migrator's real status field and substitutes a version-comparison heuristic @ vendor/wheels/Public.cfc:203 ($cliFormatMigrationStatus() maps status == "migrated" directly), consumed at vendor/wheels/public/views/cli.cfm:301; the line-257 self-assignment and the never-set loadedAt read are gone.
• P10 [Medium, perf]: CLI/MCP bridge runs full migration discovery (O(N) DB metadata round-trips) on every request, including 3-second job-worker polls @ vendor/wheels/public/views/cli.cfm:41-53 — lazy needsMigrations/needsVersion/needsDbType per-command lists; dbStatus/dbRollback/migrateUp/migrateDown/redoMigration reuse data.migrations; vendor/wheels/Public.cfc:184 ($cliDatabaseType()) memoizes the adapter probe per datasource.

Findings verified already-fixed

None — all findings in this package required code changes (staleRefs is empty; pre-fix lines cli.cfm:257, :640, :731, :808 and the GET-only route in routes.cfm were all confirmed present on origin/develop before this branch).

Source

Internal multi-agent framework review 2026-06-09, wave 2, package public-cli-endpoint.

Tests

• New spec: vendor/wheels/tests/specs/security/CliEndpointHardeningSpec.cfc (WheelsTest BDD, createObject("wheels.Public").$init() prior-art pattern) — 25 tests covering the mutating-command classifier, POST/loopback/X-Forwarded-For/password gate branches (including IPv4 + IPv6 loopback and fail-closed when no password is configured), dump-path traversal containment, and the P3 out-of-sequence-pending regression. All asserted helpers are new on this branch, so the spec fails against pre-fix code.
• Local verification (Lucee 7 + SQLite, worktree docker recipe): new spec 25/25 pass, full security area 222/222 pass, plus live-endpoint checks of every gate branch and the dbShell/dbDump behavior. CI runs the full engine × DB matrix.
• Note for maintainers: the legacy (not CI-wired) cli/tests/specs/e2e/ServerCommandsTest.cfc has a whitebox assertion that makeHttpRequest appears within the first 800 chars of runMigration; the added gate logic pushes it to ~1087, so that single offset assertion goes stale if anyone runs the legacy suite. Worth a one-line follow-up; it does not affect CI.

Cross-engine notes

• New Public.cfc helpers are public with $ prefix (mixin invariant New master - readme #7).
• cfheader/cfcontent use explicit named args — no attributeCollection = arguments (Adobe 2023/2025 invariant Fixed bug in $findRoute() that causes blow up on unmatched named route #10).
• No local.X-assigned-in-catch reads (BoxLang invariant New master #11): try-assigned locals are only read after a successful try.
• Left()/Right() in $cliResolveDumpPath() always receive lengths > 0 (canonical root is non-empty), avoiding the Lucee 7 Left(str, 0) crash.
• Spec uses a shared var state struct for closure state and no inline closures as constructor named args.

Changelog

Entry deliberately omitted; consolidated at campaign end.

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This PR correctly closes a real CSRF vector — unauthenticated GET requests could fire dbReset via a browser <img> tag. The gate implementation (POST + loopback check + constant-time password comparison) is sound, the test coverage is thorough, and the cross-engine notes are accurate. Two low-severity findings below; neither touches the gate logic itself.

---

Correctness

makeBridgePost — null error stream produces an NPE instead of a clean error

vendor/wheels/public/views/cli.cfm routes through makeBridgePost in cli/lucli/Module.cfc. On lines 6188–6190:

var responseCode = conn.getResponseCode();
var inputStream = responseCode >= 400 ? conn.getErrorStream() : conn.getInputStream();
var scanner = createObject("java", "java.util.Scanner").init(inputStream, "UTF-8");

HttpURLConnection.getErrorStream() returns null when the server returns 4xx/5xx with no response body (per the Java docs: "If the server had an error but no error data was sent, this method will return null"). Passing null to Scanner.init() throws java.lang.NullPointerException. The outer try/catch in the caller rescues this into a MigrationError, but the message becomes "null" rather than something actionable. In practice the Wheels framework always sends a JSON body on gated rejections, so this is low-probability; it would only bite if something else is listening on the port.

The same pattern exists in the pre-existing makeHttpPost() at line 6220, so this isn't new debt, but makeBridgePost is new code in this PR. Suggested guard:

var inputStream = responseCode >= 400 ? conn.getErrorStream() : conn.getInputStream();
if (IsNull(inputStream) || inputStream == javaCast("null", "")) {
    return "";
}
var scanner = createObject("java", "java.util.Scanner").init(inputStream, "UTF-8");

---

Security

cli.cfm:19 — password read from merged URL+form params instead of form body exclusively

password = StructKeyExists(request.wheels.params, "password") ? request.wheels.params.password : ""

request.wheels.params merges URL query parameters and form fields. The PR body says the password is "sent as a form field to keep it out of the URL and access logs," but a POST to /wheels/cli?command=dbReset&password=secret would still satisfy the gate. The gate itself already requires POST (preventing <img> CSRF) and loopback (preventing remote reach), so this is not a bypass — but the URL-param path contradicts the stated design and would log the password in the server's access log.

Prefer reading from the form scope explicitly:

password = StructKeyExists(form, "password") ? form.password : ""

This also makes the intent clearer for future readers.

---

Tests

Coverage is solid: 25 tests across $cliCommandIsMutating, $cliMutationGateCheck (GET, non-loopback, X-Forwarded-For bypass, empty password, missing key, wrong password, IPv4/IPv6/loopback-chain happy paths), $cliResolveDumpPath (traversal, nested paths, empty input), $cliFormatMigrationStatus (out-of-sequence, summary counts, key shape), and $cliDatabaseType memoization. BDD syntax, shared var state struct for closure-safe state — all correct.

One observation: dbDrop (line 82) and dbRestore (line 83) are listed as read-only in the spec. Both are currently stubs that return error messages directing users to their database tools (confirmed at cli.cfm:649–653 and cli.cfm:780–800), so the classification is accurate — but worth a comment in the spec so the next reader doesn't assume those entries are an oversight.

---

Commits

fix(dispatch): require POST+password for /wheels/cli destructive ops, contain dbDump — valid type, valid format, under 100 chars, sentence-case. ✓

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's two findings are real and well-cited. One genuine missed issue: the dbRollback case carries the same P3 heuristic the PR explicitly fixed elsewhere, which will miscount rollback steps in shared dev DB scenarios. A's verdict state (COMMENTED rather than REQUEST_CHANGES) is also too passive given two real bugs in new code on a security-hardening PR.

Sycophancy

None detected. A's TL;DR is positive but backed by cited code and test evidence. A did not approve despite touching a security subsystem.

False positives

None detected. Both findings check out against the diff:

• Null getErrorStream() (Module.cfc makeBridgePost): Java's HttpURLConnection.getErrorStream() does return null when the server sends a 4xx/5xx with no body. The Scanner.init(null, "UTF-8") NPE path exists on Lucee (Adobe coerces Java null to "", which silently produces an empty Scanner). The outer catch rescues it but the error message becomes "null". Finding is accurate.
• Password via merged params (cli.cfm:19): request.wheels.params merges URL and form fields. A POST to /wheels/cli?command=dbReset&password=secret satisfies the gate, contradicting the PR's stated design and logging the password in the server access log. Finding is accurate.

Missed issues

dbRollback still uses version <= currentVersion (P3-family heuristic)

cli.cfm, case "dbRollback": (around line 322 in the branch):

for (local.migration in data.migrations) {
    if (local.migration.version <= data.currentVersion) {
        arrayAppend(local.appliedMigrations, local.migration);
    }
}
local.targetIndex = arrayLen(local.appliedMigrations) - local.steps;
local.targetVersion = local.appliedMigrations[local.targetIndex].version;

The PR fixed this exact heuristic for dbStatus via $cliFormatMigrationStatus(), correctly noting that a peer-applied migration at a higher version number makes a local pending migration look applied. The same bug persists here.

Concrete failure with a shared dev DB (0001 applied, 0002 pending, 0003 applied, currentVersion = "0003"):

• local.appliedMigrations = [0001, 0002, 0003] (heuristic) vs [0001, 0003] (correct)
• dbRollback steps=2: heuristic picks targetVersion = "0001" → migrateTo("0001") → only 0003 rolls back (one step, not two). Correct path picks targetVersion = "0" → both 0003 and 0001 roll back.
• dbRollback steps=1: both paths coincidentally produce the same end state, so it's masked in the common case.

The fix is the same pattern used in $cliFormatMigrationStatus(): filter local.migration.status == "migrated" instead of the version comparison.

Verdict alignment

A's two findings (a null NPE in new code and a security logging concern on a security-hardening PR) both justify REQUEST_CHANGES. COMMENTED leaves them advisory, which is too passive here — the stated PR design says the password is "sent as a form field to keep it out of the URL and access logs," and the code contradicts that intent. The gate logic is sound, but these are fixable issues in new code before merge.

Convergence

Not aligned. A needs to address the dbRollback P3 inconsistency — it is a missed finding, not a nuance in A's existing analysis. Additionally, A should revisit the verdict state: the password-in-URL finding on a security PR warrants REQUEST_CHANGES rather than a neutral COMMENT.

[wheels-bot]

Wheels Bot — Reviewer A response (round 1)

B's round-1 critique is correct on both counts. I concede the dbRollback P3 heuristic as a genuine missed issue and agree the verdict should be REQUEST_CHANGES. No defended points — B raised no false positives against my original findings.

Conceded points

• dbRollback P3 heuristic (missed issue). B is right. The case "dbRollback": block in cli.cfm (diff lines 510–511) still does if (local.migration.version <= data.currentVersion) to build local.appliedMigrations. The PR itself documents the fix in $cliFormatMigrationStatus() and its comment at diff line 455 says "the old version-comparison heuristic was wrong" — but dbRollback was not updated in the same pass. In the shared-dev-DB scenario B describes (0001 applied, 0002 pending locally but 0003 applied by a peer), dbRollback steps=2 would include 0002 in the applied list and target 0001, rolling back only one real migration instead of two. The fix is to filter local.migration.status == "migrated" as $cliFormatMigrationStatus() does.

• Verdict upgrade to REQUEST_CHANGES. B correctly flags that COMMENTED leaves real, fixable bugs advisory on a security-hardening PR whose stated design ("password sent as form field to keep it out of access logs") is directly contradicted by the implementation. With three fixable issues now on the table — the null getErrorStream() NPE in new code, the password-via-URL path, and the dbRollback heuristic inconsistency — REQUEST_CHANGES is the appropriate gate.

Defended points

None. B identified no false positives in my original review.

Updated verdict

REQUEST_CHANGES: three fixable issues in new code before merge — null getErrorStream() NPE in makeBridgePost, password readable from URL params on a security-hardening PR, and dbRollback carrying the same P3 heuristic the PR explicitly fixed for dbStatus.