diff --git a/changelog.d/policy-hardener-s2-s9.fixed.md b/changelog.d/policy-hardener-s2-s9.fixed.md new file mode 100644 index 000000000..da1cd34e1 --- /dev/null +++ b/changelog.d/policy-hardener-s2-s9.fixed.md @@ -0,0 +1,3 @@ +- `Policy.scope()` fail-closes with a no-rows chain when the resolved id list is empty, instead of calling `whereIn("id", [])` +- `policyScope()` in production returns that empty chain after `InvalidCollection` and does not call `whereIn` on the unresolved collection +- `authorize()` / `can()` deny reserved `init` and `scope` actions instead of Invoking those Policy methods diff --git a/vendor/wheels/Policy.cfc b/vendor/wheels/Policy.cfc index a0cd0ec42..652624bc1 100644 --- a/vendor/wheels/Policy.cfc +++ b/vendor/wheels/Policy.cfc @@ -98,16 +98,49 @@ component { /** * Narrows a collection to the records the user may see (used by `policyScope()` - * for `index` actions). Default-deny: returns a no-rows chain. The empty - * `whereIn` sets the query builder's injection-safe always-empty flag (see - * ##2736) without interpolating the property name into SQL, so it composes - * with any model, query-builder chain, or scope chain. Override in your - * policy to widen. + * for `index` actions). Default-deny: returns a no-rows chain that does not + * call `whereIn` with an empty id list, so an empty resolved-id set cannot + * become `IN ()` or match every row. Override in your policy to widen. * * @collection The model class (or chainable query builder / scope chain) to narrow. */ public any function scope(required any collection) { - return arguments.collection.whereIn("id", []); + return CreateObject("component", "wheels.Policy").init(); + } + + /** + * The identity this policy was initialized with. An empty string is a guest. + */ + public any function currentUser() { + return variables.user; + } + + /** + * No-rows terminal for the default-deny `scope()` chain. + */ + public numeric function count() { + return 0; + } + + /** + * Empty query for the default-deny `scope()` chain. + */ + public query function findAll() { + return QueryNew("id"); + } + + /** + * Keeps the default-deny chain empty when callers compose after `scope()`. + */ + public any function where() { + return this; + } + + /** + * Keeps the default-deny chain empty. Does not interpolate an empty `IN ()`. + */ + public any function whereIn() { + return this; } } diff --git a/vendor/wheels/controller/authorization.cfc b/vendor/wheels/controller/authorization.cfc index 0daf3cf20..2d19e86e0 100644 --- a/vendor/wheels/controller/authorization.cfc +++ b/vendor/wheels/controller/authorization.cfc @@ -46,6 +46,7 @@ component { if ( IsObject(local.policy) && Len(local.action) + && !$isReservedPolicyAction(local.action) && StructKeyExists(local.policy, local.action) && IsCustomFunction(local.policy[local.action]) ) { @@ -94,6 +95,7 @@ component { local.policy = $policyFor(arguments.record); if ( !IsObject(local.policy) + || $isReservedPolicyAction(arguments.action) || !StructKeyExists(local.policy, arguments.action) || !IsCustomFunction(local.policy[arguments.action]) ) { @@ -131,12 +133,17 @@ component { */ public any function policyScope(required any collection) { local.modelName = $policyModelName(arguments.collection); - if (!Len(local.modelName) && $get("showErrorInformation")) { - Throw( - type = "Wheels.Policy.InvalidCollection", - message = "policyScope() could not derive a model from the passed collection.", - extendedInfo = "Pass the model class first and chain from the result, e.g. `policyScope(model(""Post"")).active().findAll()`. Query-builder and scope chains that are already in flight cannot be passed to policyScope()." - ); + if (!Len(local.modelName)) { + if ($get("showErrorInformation")) { + Throw( + type = "Wheels.Policy.InvalidCollection", + message = "policyScope() could not derive a model from the passed collection.", + extendedInfo = "Pass the model class first and chain from the result, e.g. `policyScope(model(""Post"")).active().findAll()`. Query-builder and scope chains that are already in flight cannot be passed to policyScope()." + ); + } + // Production InvalidCollection: fail-closed. Do not call whereIn on a + // collection we could not resolve (empty IN, matching-all, or no method). + return CreateObject("component", "wheels.Policy").init(); } local.policy = $policyFor(arguments.collection); if (!IsObject(local.policy)) { @@ -212,6 +219,15 @@ component { return capitalize(arguments.modelName) & "Policy"; } + /** + * Internal function. `init` and `scope` are Policy lifecycle methods, not + * grantable actions. authorize()/can() must not Invoke them (missing + * `collection` on scope() is a 500, not Wheels.NotAuthorized). + */ + public boolean function $isReservedPolicyAction(required string actionName) { + return ListFindNoCase("init,scope", arguments.actionName) > 0; + } + /** * Internal function. Resolves the identity policies are evaluated against, in * order: (1) the DI service registered as `currentUser` when present, (2) the diff --git a/vendor/wheels/tests/_assets/policies/CurrentUserStub.cfc b/vendor/wheels/tests/_assets/policies/CurrentUserStub.cfc new file mode 100644 index 000000000..3fe7fee02 --- /dev/null +++ b/vendor/wheels/tests/_assets/policies/CurrentUserStub.cfc @@ -0,0 +1,13 @@ +/** + * DI `currentUser` service used by Authorization S6. getInstance("currentUser") + * returns this component; Policy stores it as the identity. + */ +component { + + public any function init() { + this.id = 9001; + this.name = "policy-di-user"; + return this; + } + +} diff --git a/vendor/wheels/tests/_assets/policies/WhereInSpy.cfc b/vendor/wheels/tests/_assets/policies/WhereInSpy.cfc new file mode 100644 index 000000000..bb508871f --- /dev/null +++ b/vendor/wheels/tests/_assets/policies/WhereInSpy.cfc @@ -0,0 +1,26 @@ +/** + * Collection stand-in whose whereIn([]) would match everything. Used to prove + * Policy.scope() / policyScope() fail closed without calling whereIn. + */ +component { + + this.whereInCalls = 0; + this.lastProperty = ""; + this.lastValues = []; + + public any function whereIn(required string property, required any values) { + this.whereInCalls = this.whereInCalls + 1; + this.lastProperty = arguments.property; + this.lastValues = arguments.values; + return this; + } + + public numeric function count() { + return 99; + } + + public query function findAll() { + return QueryNew("id", "integer", [[1]]); + } + +} diff --git a/vendor/wheels/tests/specs/controller/AuthorizationSpec.cfc b/vendor/wheels/tests/specs/Authorization/AuthorizationSpec.cfc similarity index 63% rename from vendor/wheels/tests/specs/controller/AuthorizationSpec.cfc rename to vendor/wheels/tests/specs/Authorization/AuthorizationSpec.cfc index b3ed18c6d..6da369a40 100644 --- a/vendor/wheels/tests/specs/controller/AuthorizationSpec.cfc +++ b/vendor/wheels/tests/specs/Authorization/AuthorizationSpec.cfc @@ -1,3 +1,14 @@ +/** + * Authorization policy layer. Desk IDs S1–S9 stay locked. + * PROVEN: S2 empty-id fail-closed, S5 production InvalidCollection, S6 DI/authenticator + * identity, S7 guest "", S8 production 403, S9 reserved action=scope/init. + * HELD: S1 unknown action still throws (no default-deny flip), S3 first-catch + * wrong-user failover (broken DI currentUser can still hit authenticator), + * S4 loose CF boolean grant. + * + * Directory-scoped so `wheels test --core --ci --filter=Authorization` discovers + * this folder (a single-file directory= scope finds 0 bundles). + */ component extends="wheels.WheelsTest" { function run() { @@ -24,6 +35,10 @@ component extends="wheels.WheelsTest" { application.wheels.policyPath = $savedPolicyPath application.wheels.showErrorInformation = $savedShowError StructDelete(request, "$policyTestUser") + try { + g.$header(statusCode = 200) + } catch (any e) { + } }) describe("wheels.Policy base class", () => { @@ -49,6 +64,25 @@ component extends="wheels.WheelsTest" { expect(scoped.findAll().recordCount).toBe(0) }) + it("S2: empty resolved ids never become a matching-all or invalid-SQL whereIn", () => { + spy = CreateObject("component", "wheels.tests._assets.policies.WhereInSpy") + basePolicy = CreateObject("component", "wheels.Policy").init(user = {id = 1}, record = "") + scoped = basePolicy.scope(spy) + + expect(g.model("post").count()).toBeGT(0) + expect(spy.whereInCalls).toBe(0) + expect(scoped.count()).toBe(0) + expect(scoped.findAll().recordCount).toBe(0) + }) + + it("S7: Policy.cfc missing user is the guest empty string", () => { + guestPolicy = CreateObject("component", "wheels.Policy").init() + + expect(guestPolicy.currentUser()).toBe("") + expect(IsSimpleValue(guestPolicy.currentUser())).toBeTrue() + expect(IsObject(guestPolicy.currentUser())).toBeFalse() + }) + it("default-denies through an app policy that overrides nothing", () => { request.$policyTestUser = {id = author.id} @@ -119,6 +153,42 @@ component extends="wheels.WheelsTest" { type = "Wheels.NotAuthorized" ) }) + + it("S8: authorize() denial in production is HTTP 403, not a silent allow", () => { + application.wheels.showErrorInformation = false + request.$policyTestUser = {id = otherAuthor.id} + denied = {allowed = false, status = 0, type = ""} + try { + _controller.authorize(record = post, action = "update") + denied.allowed = true + } catch (any e) { + denied.type = e.type + } + denied.status = Val(g.$statusCode()) + + expect(denied.allowed).toBeFalse() + expect(denied.status).toBe(403) + try { + g.$header(statusCode = 200) + } catch (any e) { + } + }) + + it("S9: authorize() with action=scope throws Wheels.NotAuthorized and does not Invoke scope", () => { + request.$policyTestUser = {id = author.id} + + expect(() => _controller.authorize(record = post, action = "scope")).toThrow( + type = "Wheels.NotAuthorized" + ) + }) + + it("S9: authorize() with action=init throws Wheels.NotAuthorized and does not Invoke init", () => { + request.$policyTestUser = {id = author.id} + + expect(() => _controller.authorize(record = post, action = "init")).toThrow( + type = "Wheels.NotAuthorized" + ) + }) }) describe("can()", () => { @@ -190,6 +260,30 @@ component extends="wheels.WheelsTest" { expect(() => _controller.policyScope(builder)).toThrow(type = "Wheels.Policy.InvalidCollection") }) + + it("S5: production InvalidCollection fail-closes without calling whereIn", () => { + application.wheels.showErrorInformation = false + spy = CreateObject("component", "wheels.tests._assets.policies.WhereInSpy") + scoped = _controller.policyScope(spy) + + expect(spy.whereInCalls).toBe(0) + expect(scoped.count()).toBe(0) + }) + + it("S5: production InvalidCollection does not fall through to whereIn on a collection without whereIn", () => { + application.wheels.showErrorInformation = false + bad = {notAModel = true} + state = {threw = false, count = -1} + try { + scoped = _controller.policyScope(bad) + state.count = scoped.count() + } catch (any e) { + state.threw = true + } + + expect(state.threw).toBeFalse() + expect(state.count).toBe(0) + }) }) describe("missing policy class", () => { @@ -227,6 +321,49 @@ component extends="wheels.WheelsTest" { expect(plain.can("update", post)).toBeFalse() expect(plain.can("show", post)).toBeTrue() }) + + it("S6: policy identity comes from the DI currentUser service", () => { + savedDi = application.wheelsdi + di = new wheels.Injector(binderPath = "wheels.tests._assets.di.TestBindings") + di.map("currentUser").to("wheels.tests._assets.policies.CurrentUserStub") + application.wheelsdi = di + identity = {id = "", name = ""} + try { + plain = g.controller("test", {controller = "test", action = "show"}) + resolved = plain.$currentUserForPolicy() + identity.id = resolved.id + identity.name = resolved.name + } finally { + application.wheelsdi = savedDi + } + + expect(identity.id).toBe(9001) + expect(identity.name).toBe("policy-di-user") + }) + + it("S6: policy identity comes from the authenticator strategy currentUser()", () => { + savedDi = application.wheelsdi + di = new wheels.Injector(binderPath = "wheels.tests._assets.di.TestBindings") + di.map("authenticator").to("wheels.auth.Authenticator").asSingleton() + application.wheelsdi = di + identity = {id = "", name = ""} + try { + auth = di.getInstance("authenticator") + strategy = new wheels.auth.SessionStrategy() + strategy.login(principal = {id = 4242, name = "policy-auth-user"}) + auth.registerStrategy(name = "session", strategy = strategy) + plain = g.controller("test", {controller = "test", action = "show"}) + resolved = plain.$currentUserForPolicy() + identity.id = resolved.id + identity.name = resolved.name + } finally { + application.wheelsdi = savedDi + StructDelete(session, "wheels") + } + + expect(identity.id).toBe(4242) + expect(identity.name).toBe("policy-auth-user") + }) }) describe("routable surface", () => {