diff --git a/changelog.d/scaffold-404-guard.fixed.md b/changelog.d/scaffold-404-guard.fixed.md new file mode 100644 index 000000000..541bc7617 --- /dev/null +++ b/changelog.d/scaffold-404-guard.fixed.md @@ -0,0 +1 @@ +- Scaffolded controllers now return **404** for a key that matches no record, including a soft-deleted one. Previously `show`, `edit`, `update` and `delete` called `findByKey` bare, and a missing row made the action operate on an empty value — the view threw and the user saw a 500. A `requireRecord` before-filter now throws `Wheels.RecordNotFound`, which the framework maps to 404 diff --git a/cli/lucli/templates/codegen/CRUDContent.txt b/cli/lucli/templates/codegen/CRUDContent.txt index e034e6e03..fa83f15d3 100644 --- a/cli/lucli/templates/codegen/CRUDContent.txt +++ b/cli/lucli/templates/codegen/CRUDContent.txt @@ -8,6 +8,17 @@ */ function config() { super.config(); + // 404 rather than a 500 when the key matches nothing. findByKey returns a + // non-object for BOTH a missing key and a soft-deleted row, and the + // actions below then operate on that empty value — `post.title` in the + // view, or `.update()` here, throws. + // + // A BEFORE filter that deliberately does NOT assign the record: the + // actions keep their conventional single-finder body, which the + // `--belongsTo` parent wiring depends on — it only rewrites `show()` + // when the finder is the whole body, and skips a customized one. + // Loading twice is cheap and keeps that contract intact. + filters(through="requireRecord", only="show,edit,update,delete"); } /** @@ -81,4 +92,21 @@ redirectTo(route="|ObjectNamePlural|"); } + /** + * Guard: abort with a 404 when the key matches no record. + * + * The message is a plain string on purpose. The scaffold's parent wiring + * scans this file with ScaffoldSource, which refuses interpolated strings + * (`##...##`) wholesale — a single `##params.key##` here would silently turn + * off `include=` wiring for every `--belongsTo` child of this model. + **/ + private function requireRecord() { + if (!IsObject(model("|ObjectNameSingularC|").findByKey(key=params.key))) { + Throw( + type = "Wheels.RecordNotFound", + message = "|ObjectNameSingularC| not found for the requested key." + ); + } + } + } diff --git a/cli/lucli/tests/specs/services/ScaffoldSpec.cfc b/cli/lucli/tests/specs/services/ScaffoldSpec.cfc index 6fd26f898..286f1bedd 100644 --- a/cli/lucli/tests/specs/services/ScaffoldSpec.cfc +++ b/cli/lucli/tests/specs/services/ScaffoldSpec.cfc @@ -77,6 +77,40 @@ component extends="wheels.wheelstest.system.BaseSpec" { expect(content).toInclude("function delete()"); }); + it("guards every key-loading action against a missing record", () => { + // findByKey returns a non-object for BOTH a missing key and a + // soft-deleted row. Without a guard the action renders against + // that empty value and the view throws a 500 where a 404 + // belongs — reproduced on a soft-deleted post before the fix. + var result = scaffold.generateScaffold( + name = "Notefile", + properties = [{name = "title", type = "string"}], + force = true + ); + expect(result.success).toBeTrue(); + var content = fileRead(tempRoot & "/app/controllers/Notefiles.cfc"); + + // A BEFORE filter covering all four key-loading actions. + expect(content).toInclude('filters(through="requireRecord", only="show,edit,update,delete")'); + expect(content).toInclude("private function requireRecord()"); + expect(content).toInclude('if (!IsObject(model("Notefile").findByKey(key=params.key))) {'); + expect(content).toInclude('type = "Wheels.RecordNotFound"'); + + // The guard must NOT live inside show(): ScaffoldSource only + // rewrites show() when its finder is the whole body, so an + // inline guard there disables `--belongsTo` include= wiring. + var showBody = ReMatchNoCase("function show\(\) \{[^}]+\}", content); + expect(ArrayLen(showBody)).toBe(1); + expect(showBody[1]).notToInclude("IsObject"); + + // And the generated file must still SCAN. ScaffoldSource fails + // closed on any interpolated string, so a single `##...##` in + // the guard's message would silently switch off the parent + // wiring for every child of this model. Pin it at the source. + var scanned = new cli.lucli.services.ScaffoldSource().scan(content); + expect(scanned.valid).toBeTrue(scanned.reason); + }); + it("emits a full CRUD controller spec with model().create() test data", () => { var result = scaffold.generateScaffold( name = "Chronicle",