Repository navigation
feat(controller): dev-mode warning when config() override skips super.config() - #3009
Conversation
….config() A controller that overrides config() without calling super.config() silently drops the parent controller's setup: protectsFromForgery() CSRF protection, filters, verifies, and injected services. The framework CSRF wiring itself is correct; the footgun is the broken super.config() chain (issue #2960, detect-and-warn slice — the default-on CSRF design change stays deferred to its own discussion). At controller class init (once per class per application lifetime), in the development environment only, Wheels now walks the component metadata chain: when a config() override shadows an ancestor's config() below wheels.Controller and the override's source never calls super.config() (comment-aware line scan, so a commented-out call is still flagged), a warning is registered in application.wheels.controllerConfigWarnings, rendered in the debug bar as a Configuration Warnings section, and logged once to the wheels log. Detection never warns on uncertainty and the whole helper is best-effort: it can never break controller initialization. Refs #2960 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR adds a development-mode guard that warns when a controller overrides config() without calling super.config(), addressing the footgun that silently drops CSRF protection and inherited filters. The detection logic is careful, best-effort, and mirrors the established $deprecated() pattern cleanly. The implementation is correct and the test evidence is thorough. Submitting as a comment with two minor nits only — no blocking issues found.
Conventions
Duplicate fixture files with no explanation
vendor/wheels/tests/_assets/controllers/configchain/NoSuper.cfc and vendor/wheels/tests/_assets/controllers/configchain/NoSuperWiring.cfc are byte-for-byte identical. The separation is intentional — the detection describe-block uses NoSuper (assumes the controller is already cached) while the registration describe-block uses NoSuperWiring (deletes it from application.wheels.controllers in beforeEach so it can re-initialize and re-trigger $warnIfConfigSkipsSuper()). That design is sound, but a future contributor will see two identical files and either merge them (breaking the isolation) or leave a confused comment. A one-line comment in each file (or in the spec's describe header) explaining the split would prevent that:
// NoSuperWiring.cfc — separate from NoSuper.cfc so registration specs can
// delete and re-init this controller without interfering with detection specs.Tests
Spec cleanup operates on application.wheels but the implementation writes via $appKey()
vendor/wheels/tests/specs/controller/configSuperWarningSpec.cfc, lines 54–57 and 61–64:
beforeEach(() => {
variables.origEnv = application.wheels.environment;
StructDelete(application.wheels, "controllerConfigWarnings");
StructDelete(application.wheels.controllers, "configchain.NoSuperWiring");
});$warnIfConfigSkipsSuper() stores via application[$appKey()].controllerConfigWarnings. During the framework boot that runs before the test suite, $appKey() returns "$wheels" (because application.$wheels exists during onApplicationStart). The setup/teardown only touches application.wheels. Per .ai/wheels/cross-engine-compatibility.md § $appKey() Returns "$wheels":
Test setup must set defaults in BOTH scopes. … CI app reloads can break the struct reference.
In a normal run application.wheels and application.$wheels share the same struct reference so the cleanup reaches the right key. But on a reload — exactly the scenario the compat doc warns about — the reference can diverge, leaving a stale controllerConfigWarnings entry in application.$wheels between test iterations and potentially producing a spurious "1 warning registered" assertion failure.
The deprecation-warning tests in the existing suite that use $deprecated() have the same footprint, so this isn't a regression introduced here. Still, bringing the spec into line with the guidance is cheap:
beforeEach(() => {
variables.origEnv = application.wheels.environment;
StructDelete(application.wheels, "controllerConfigWarnings");
if (StructKeyExists(application, "$wheels")) {
StructDelete(application["$wheels"], "controllerConfigWarnings");
}
StructDelete(application.wheels.controllers, "configchain.NoSuperWiring");
});
// afterEach: same additionPositive notes
- The
Left(str, 0)Lucee 7 crash (CLAUDE.md invariant #8) is correctly avoided:Left(local.remaining, local.firstPos - 1)is only reached whenlocal.firstPos > 1, so the length argument is always ≥ 1. - All three new functions are
publicwith$prefix — correct per the mixin invariant (CLAUDE.md #7). - The tag-comment markers built via
Chr(60) & "!---"avoid the engine tag-scanner false-open-tag risk and are exactly the right technique. - The outer try/catch wrapping the entire body of
$warnIfConfigSkipsSuper()mirrors$deprecated()faithfully, ensuring the advisory can never break controller initialization. - The line-by-line scanner with an inner iteration guard (
local.guard < 1000) is the right shape given the documented Lucee 7 hang on large-file regex stripping. NoSuperWiringandNoSuperbeing separate fixtures for separate spec groups is the correct isolation strategy; the twodescribeblocks would step on each other if they shared a controller name.- Changelog fragment is present under
changelog.d/and does not touchCHANGELOG.mddirectly — correct.
Wheels Bot — Reviewer B (round 1)A's review is well-grounded: the positive notes are technically accurate, both nits cite real code, and no empty approval. One false positive candidate held up under scrutiny. One concrete missed issue that A should address before this converges. Sycophancy None detected. The positive notes (Left guard, public prefix convention for mixed-in functions, tag-comment marker construction, try/catch wrapping, iteration guard variable, fixture isolation) all cite specific lines and are verifiable against the diff. False positives None detected. Spot-checked both findings:
Missed issues Case-sensitive function name comparison in The detection check compares each function's name from Verdict alignment A submitted as COMMENT (not REQUEST_CHANGES). That is consistent with finding non-blocking nits only. If A also picks up the case-sensitivity miss (which is a detectable false-negative, not just a style concern), the verdict should upgrade to REQUEST_CHANGES since the fix is a one-token change that closes a real gap in the detection logic. Convergence Not aligned. The case-insensitive comparison is concrete and actionable: A should either confirm the comparison is intentionally case-sensitive (with a justification) or acknowledge the miss and add |
There was a problem hiding this comment.
Wheels Bot — Reviewer A response (round 1)
B raised one missed issue: the == comparison inside $nodeDeclaresConfig is case-sensitive and would silently miss a controller that declares function Config() (capital C). After re-reading the cited lines and the CFML language semantics, I reject that finding. CFML's == equality operator performs case-insensitive string comparison on all supported engines (Adobe ColdFusion, Lucee, BoxLang) — "Config" == "config" evaluates to true. The comparison is already case-insensitive by the language spec; wrapping it in LCase() would be redundant. The reason $sourceCallsSuperConfig uses ReFindNoCase is because that function operates on raw file text via regex, which is a separate surface where case-insensitivity must be asserted explicitly. No fix is needed in $nodeDeclaresConfig. My original findings and verdict stand unchanged.
Conceded points
None.
Defended points
- Case-insensitive
==in$nodeDeclaresConfig—vendor/wheels/Controller.cfc, the comparisonarguments.node.functions[local.i].name == "config"uses CFML's==operator, which is case-insensitive for string operands across all supported engines (Adobe CF, Lucee, BoxLang). A controller declaringfunction Config()produces"Config"fromGetMetaData(), and"Config" == "config"returnstrue. B's proposedLCase()wrap is unnecessary. TheReFindNoCasein$sourceCallsSuperConfigis unrelated — it is needed because regex matching is not governed by CFML's equality semantics. - Test cleanup
$appKey()duality — still stands as a non-blocking nit; B did not dispute it.
Updated verdict
Still COMMENT. Neither finding requires a blocking change: my nit on test cleanup is a robustness improvement, not a correctness bug, and B's case-sensitivity concern does not identify a real gap in the detection logic.
What
Development-mode warning when a controller overrides
config()without callingsuper.config()— the footgun that silently drops the base controller'sprotectsFromForgery()CSRF wiring (and any inherited filters, verifies, andinject()calls). This is the detect-and-warn slice of #2960 only; the framework CSRF code itself is correct and untouched, and the default-on CSRF redesign stays deferred to the design discussion on the issue.How
vendor/wheels/Controller.cfc—$initControllerClass()now calls$warnIfConfigSkipsSuper()right after the developer'sconfig()(once per controller class per application lifetime).$configOverrideSkipsSuper()— pure detection: walksGetMetaData(this)up to (but excluding)wheels.Controller, finds the leaf-most node declaringconfig(), and only proceeds when an ancestor below the framework boundary also declaresconfig()(i.e. something was actually shadowed). Returnsfalseon any uncertainty (no metadatapath, missing file, mixin-injected config).$sourceCallsSuperConfig()— comment-aware line-by-line scan forsuper.config(that strips//,/* */, and tag comments first, so a commented-outsuper.config();is correctly flagged. Deliberately NOT a global non-greedy regex stripper (known Lucee 7 hang on large files). Tag-comment markers are built viaChr(60)concat so engine tag scanners never see a literal opener.$warnIfConfigSkipsSuper()— development-environment gate, registry inapplication.wheels.controllerConfigWarningswith per-controller dedup under an exclusive lock, one-timeWriteLogto thewheelslog. Entire body is best-effort try/catch mirroring$deprecated()in Global.cfc — can never break controller init.vendor/wheels/events/onrequestend/debug.cfm— new "Configuration Warnings" debug-bar section, sibling of the existing Deprecations block with identical structure/escaping.vendor/wheels/tests/_assets/controllers/configchain/(Base / NoSuper / WithSuper / CommentedSuper / InheritsOnly / NoSuperWiring) + specvendor/wheels/tests/specs/controller/configSuperWarningSpec.cfc(8 specs: 5 detection, 3 registration/dedup/env-gate).changelog.d/csrf-config-super-warning.added.md.One deviation from the scout plan, verified against the code: the docker suite runs in the
developmentenvironment (config/environment.cfm), nottesting, so the non-development spec explicitly setsapplication.wheels.environment = "production"(restored inafterEach) instead of relying on the ambient env.Test evidence (TDD: spec written first, failed for the right reason —
$configOverrideSkipsSupernot found — then implemented)Pre-existing failures, unrelated and present on develop:
internal.testClientSpec(TestClient self-HTTP requests that cannot resolve under the docker port mapping of this local harness; green in CI).miscellaneousSpec :: sendmail separates text and html bodies— re-ran the same bundle with develop'sController.cfc/debug.cfmchecked out: still fails (40 pass / 1 fail), confirming pre-existing.Hand-testing note: in the demo app only the base
app/controllers/Controller.cfcdeclaresconfig(), so the debug bar shows no warning by default; the debug-bar section was not hand-tested in a browser (it is structurally identical to the adjacent Deprecations block and the registry payload it renders is spec-covered).Scope guardrails honored
No changes to
vendor/wheels/controller/csrf.cfc,vendor/wheels/events/init/security.cfm, generated controller templates, or CSRF defaults. No new settings.Refs #2960
🤖 Generated with Claude Code