diff --git a/changelog.d/policy-hardener-s1-s3-s4.changed.md b/changelog.d/policy-hardener-s1-s3-s4.changed.md new file mode 100644 index 000000000..5e28ecda9 --- /dev/null +++ b/changelog.d/policy-hardener-s1-s3-s4.changed.md @@ -0,0 +1,2 @@ +- `authorize()` / `can()` throw `Wheels.Policy.UnknownAction` when the policy has no method for the action, instead of treating a typo as a deny +- `$currentUserForPolicy()` no longer swallows a throwing DI `currentUser` into the authenticator user, or a throwing authenticator into guest `""` diff --git a/changelog.d/policy-hardener-s1-s3-s4.security.md b/changelog.d/policy-hardener-s1-s3-s4.security.md new file mode 100644 index 000000000..e02b4f471 --- /dev/null +++ b/changelog.d/policy-hardener-s1-s3-s4.security.md @@ -0,0 +1 @@ +- `authorize()` / `can()` grant only when a policy method returns boolean `true`. The CFML strings `"yes"` and `"true"` no longer authorize diff --git a/vendor/wheels/controller/authorization.cfc b/vendor/wheels/controller/authorization.cfc index 2d19e86e0..4b9f021b7 100644 --- a/vendor/wheels/controller/authorization.cfc +++ b/vendor/wheels/controller/authorization.cfc @@ -15,7 +15,9 @@ component { * A missing policy class throws `Wheels.Policy.NotDefined` in development and * testing (loud, Pundit-style, to catch typos) and silently denies in * production — the same environment posture as `tableName()` (##3079). A - * policy class that lacks a method for the action denies. + * policy class that lacks a method for the action throws + * `Wheels.Policy.UnknownAction` (a typo, not a deny). Reserved `init` and + * `scope` still deny as `Wheels.NotAuthorized`. Only boolean `true` grants. * * [section: Controller] * [category: Authorization Functions] @@ -42,46 +44,29 @@ component { } local.modelName = $policyModelName(arguments.record); local.policy = $policyFor(arguments.record); - local.allowed = false; - if ( - IsObject(local.policy) - && Len(local.action) - && !$isReservedPolicyAction(local.action) - && StructKeyExists(local.policy, local.action) - && IsCustomFunction(local.policy[local.action]) - ) { - // Dynamic dispatch via the built-in Invoke() — Adobe CF's compiler - // rejects a direct `local.policy[local.action]()` call outright - // (InvalidIdentifierException at compile time, verified on Adobe - // 2023), and extracting the function reference first drops the - // receiver binding on BoxLang. Invoke(instance, methodName) is the - // cross-engine-proven form (see QueryBuilder/ScopeChain - // onMissingMethod). The StructKeyExists + IsCustomFunction guard - // mirrors the action-dispatch gate in processing.cfc ($callAction). - local.allowed = Invoke(local.policy, local.action); - // A policy method that forgets to return yields null — on Adobe CF a - // null assignment deletes the variable, so re-materialize the deny. - if (IsNull(local.allowed)) { - local.allowed = false; - } - } - if (!IsBoolean(local.allowed) || !local.allowed) { + local.allowed = $invokePolicyAction( + policy = local.policy, + action = local.action, + modelName = local.modelName + ); + if (!$policyGranted(local.allowed)) { $notAuthorized(action = local.action, modelName = local.modelName); } return arguments.record; } /** - * Non-throwing boolean policy check for conditionals and views (views run in - * the controller's `variables` scope, so `can()` is available in templates + * Boolean policy check for conditionals and views (views run in the + * controller's `variables` scope, so `can()` is available in templates * automatically): * * ``` * ##linkTo(text="Edit", route="editPost", key=post.id)## * ``` * - * Returns `false` (deny) for a guest, for an empty record, and for actions the - * policy has no method for. A missing policy class still throws + * Returns `false` (deny) for a guest, for an empty record, and for a policy + * method that does not return boolean `true`. A missing method throws + * `Wheels.Policy.UnknownAction`. A missing policy class still throws * `Wheels.Policy.NotDefined` in development/testing so typos fail loud; in * production it returns `false`. * @@ -93,19 +78,13 @@ component { */ public boolean function can(required string action, any record = "") { local.policy = $policyFor(arguments.record); - if ( - !IsObject(local.policy) - || $isReservedPolicyAction(arguments.action) - || !StructKeyExists(local.policy, arguments.action) - || !IsCustomFunction(local.policy[arguments.action]) - ) { - return false; - } - // Dynamic dispatch via Invoke() (see authorize() for the cross-engine - // reasoning). The IsNull guard covers a policy method that forgets to - // return — null deletes the variable on Adobe CF. - local.allowed = Invoke(local.policy, arguments.action); - return !IsNull(local.allowed) && IsBoolean(local.allowed) && local.allowed; + return $policyGranted( + $invokePolicyAction( + policy = local.policy, + action = arguments.action, + modelName = $policyModelName(arguments.record) + ) + ); } /** @@ -228,43 +207,71 @@ component { return ListFindNoCase("init,scope", arguments.actionName) > 0; } + /** + * Internal function. Dispatches a policy action. Missing methods throw + * `Wheels.Policy.UnknownAction`. Reserved `init`/`scope`, an empty action, + * or a missing policy object return false (deny). Dynamic dispatch uses + * Invoke() — Adobe CF rejects `policy[action]()` at compile time, and an + * extracted function reference drops the receiver on BoxLang. + */ + public any function $invokePolicyAction(required any policy, required string action, string modelName = "") { + if (!IsObject(arguments.policy) || !Len(arguments.action) || $isReservedPolicyAction(arguments.action)) { + return false; + } + if (!StructKeyExists(arguments.policy, arguments.action) || !IsCustomFunction(arguments.policy[arguments.action])) { + local.target = Len(arguments.modelName) ? " on the `#arguments.modelName#` policy" : ""; + Throw( + type = "Wheels.Policy.UnknownAction", + message = "No `#arguments.action#` method#local.target#.", + extendedInfo = "A missing policy method is a typo, not a deny. Declare the method to grant, or inherit the default-deny from wheels.Policy for a known action. Reserved `init` and `scope` deny as Wheels.NotAuthorized." + ); + } + local.allowed = Invoke(arguments.policy, arguments.action); + if (IsNull(local.allowed)) { + return false; + } + return local.allowed; + } + + /** + * Internal function. Only boolean `true` grants. CFML string truthies + * (`yes`, `true`) and numeric `1` serialize to something other than the + * JSON boolean `true`, so they deny. + */ + public boolean function $policyGranted(required any allowed) { + if (IsNull(arguments.allowed)) { + return false; + } + return SerializeJSON(arguments.allowed) == "true"; + } + /** * Internal function. Resolves the identity policies are evaluated against, in * order: (1) the DI service registered as `currentUser` when present, (2) the * first registered authenticator strategy that exposes a `currentUser()` * method (e.g. `wheels.auth.SessionStrategy`) and reports a non-empty - * principal, (3) an empty string (guest). Apps customize by registering the + * principal, (3) an empty string (guest). A throwing `currentUser` service + * or authenticator strategy propagates. Apps customize by registering the * `currentUser` DI service or by overriding this method on their base * controller. */ public any function $currentUserForPolicy() { - // 1. Explicit DI registration wins. - try { - if (IsDefined("application.wheelsdi") && application.wheelsdi.containsInstance("currentUser")) { - return application.wheelsdi.getInstance("currentUser"); - } - } catch (any e) { - // A broken resolver must not turn every request into a 500 — fall through to the next seam. + if (IsDefined("application.wheelsdi") && application.wheelsdi.containsInstance("currentUser")) { + return application.wheelsdi.getInstance("currentUser"); } - // 2. A configured authenticator whose strategy can report the current user. - try { - if (IsDefined("application.wheelsdi") && application.wheelsdi.containsInstance("authenticator")) { - local.authenticator = application.wheelsdi.getInstance("authenticator"); - local.strategyNames = local.authenticator.getStrategyNames(); - for (local.strategyName in local.strategyNames) { - local.strategy = local.authenticator.getStrategy(local.strategyName); - if (StructKeyExists(local.strategy, "currentUser")) { - local.candidate = local.strategy.currentUser(); - if (IsStruct(local.candidate) && !StructIsEmpty(local.candidate)) { - return local.candidate; - } + if (IsDefined("application.wheelsdi") && application.wheelsdi.containsInstance("authenticator")) { + local.authenticator = application.wheelsdi.getInstance("authenticator"); + local.strategyNames = local.authenticator.getStrategyNames(); + for (local.strategyName in local.strategyNames) { + local.strategy = local.authenticator.getStrategy(local.strategyName); + if (StructKeyExists(local.strategy, "currentUser")) { + local.candidate = local.strategy.currentUser(); + if (IsStruct(local.candidate) && !StructIsEmpty(local.candidate)) { + return local.candidate; } } } - } catch (any e) { - // Session scope unavailable or authenticator misconfigured — treat as guest. } - // 3. Guest. return ""; } @@ -278,7 +285,7 @@ component { */ public void function $notAuthorized(required string action, string modelName = "") { $header(statusCode = 403); - if ($get("showErrorInformation")) { + if ($get("showErrorInformation") || StructKeyExists(request, "$wheelsIsolateAbort")) { local.target = Len(arguments.modelName) ? " on `#arguments.modelName#`" : ""; Throw( type = "Wheels.NotAuthorized", diff --git a/vendor/wheels/tests/_assets/policies/CurrentUserThrowingStub.cfc b/vendor/wheels/tests/_assets/policies/CurrentUserThrowingStub.cfc new file mode 100644 index 000000000..7fbe30af0 --- /dev/null +++ b/vendor/wheels/tests/_assets/policies/CurrentUserThrowingStub.cfc @@ -0,0 +1,11 @@ +/** + * DI `currentUser` whose init() throws. S3: getInstance("currentUser") must + * not fall through to the authenticator. + */ +component { + + public any function init() { + Throw(type = "Wheels.Policy.CurrentUserBoom", message = "forced currentUser failure for S3"); + } + +} diff --git a/vendor/wheels/tests/_assets/policies/PostPolicy.cfc b/vendor/wheels/tests/_assets/policies/PostPolicy.cfc index 49ca09db9..da43fc7a1 100644 --- a/vendor/wheels/tests/_assets/policies/PostPolicy.cfc +++ b/vendor/wheels/tests/_assets/policies/PostPolicy.cfc @@ -5,7 +5,9 @@ * - show: everyone (including guests) * - update: only the post's author * - scope: authors see their own posts; guests see nothing (inherited default-deny) - * - publish (custom action): intentionally NOT defined — must deny + * - publish (custom action): intentionally NOT defined — S1 throws + * - yesGrant / trueGrant: return CFML string truthies — S4 must not grant + * - boolGrant: boolean true — S4 positive control * - create/edit/delete/new: inherited default-deny from the base */ component extends="Policy" { @@ -33,4 +35,16 @@ component extends="Policy" { return super.scope(arguments.collection); } + public any function yesGrant() { + return "yes"; + } + + public any function trueGrant() { + return "true"; + } + + public boolean function boolGrant() { + return true; + } + } diff --git a/vendor/wheels/tests/_assets/policies/ThrowingCurrentUserStrategy.cfc b/vendor/wheels/tests/_assets/policies/ThrowingCurrentUserStrategy.cfc new file mode 100644 index 000000000..9125ff79e --- /dev/null +++ b/vendor/wheels/tests/_assets/policies/ThrowingCurrentUserStrategy.cfc @@ -0,0 +1,11 @@ +/** + * Authenticator strategy whose currentUser() throws. S3: a broken strategy + * must not become guest "". + */ +component { + + public any function currentUser() { + Throw(type = "Wheels.Policy.AuthenticatorBoom", message = "forced authenticator failure for S3"); + } + +} diff --git a/vendor/wheels/tests/specs/Authorization/AuthorizationSpec.cfc b/vendor/wheels/tests/specs/Authorization/AuthorizationSpec.cfc index 6da369a40..38a8d328b 100644 --- a/vendor/wheels/tests/specs/Authorization/AuthorizationSpec.cfc +++ b/vendor/wheels/tests/specs/Authorization/AuthorizationSpec.cfc @@ -1,10 +1,9 @@ /** * 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. + * PROVEN: S1 unknown action throws Wheels.Policy.UnknownAction, S2 empty-id + * fail-closed, S3 throwing identity seams fail loud, S4 only boolean true + * grants, S5 production InvalidCollection, S6 DI/authenticator identity, + * S7 guest "", S8 production 403, S9 reserved action=scope/init. * * Directory-scoped so `wheels test --core --ci --filter=Authorization` discovers * this folder (a single-file directory= scope finds 0 bundles). @@ -35,6 +34,7 @@ component extends="wheels.WheelsTest" { application.wheels.policyPath = $savedPolicyPath application.wheels.showErrorInformation = $savedShowError StructDelete(request, "$policyTestUser") + StructDelete(request, "$wheelsIsolateAbort") try { g.$header(statusCode = 200) } catch (any e) { @@ -138,14 +138,37 @@ component extends="wheels.WheelsTest" { ) }) - it("denies a custom action the policy has no method for", () => { + it("S1: authorize() throws Wheels.Policy.UnknownAction for a missing policy method", () => { request.$policyTestUser = {id = author.id} expect(() => _controller.authorize(record = post, action = "publish")).toThrow( + type = "Wheels.Policy.UnknownAction" + ) + }) + + it("S4: authorize() denies the CFML string yes", () => { + request.$policyTestUser = {id = author.id} + + expect(() => _controller.authorize(record = post, action = "yesGrant")).toThrow( type = "Wheels.NotAuthorized" ) }) + it("S4: authorize() denies the CFML string true", () => { + request.$policyTestUser = {id = author.id} + + expect(() => _controller.authorize(record = post, action = "trueGrant")).toThrow( + type = "Wheels.NotAuthorized" + ) + }) + + it("S4: authorize() returns the record when the policy returns boolean true", () => { + request.$policyTestUser = {id = author.id} + result = _controller.authorize(record = post, action = "boolGrant") + + expect(result.id).toBe(post.id) + }) + it("denies the boolean false a missed finder returns", () => { request.$policyTestUser = {id = author.id} @@ -156,6 +179,7 @@ component extends="wheels.WheelsTest" { it("S8: authorize() denial in production is HTTP 403, not a silent allow", () => { application.wheels.showErrorInformation = false + request.$wheelsIsolateAbort = true request.$policyTestUser = {id = otherAuthor.id} denied = {allowed = false, status = 0, type = ""} try { @@ -165,13 +189,17 @@ component extends="wheels.WheelsTest" { denied.type = e.type } denied.status = Val(g.$statusCode()) + src = FileRead(ExpandPath("/wheels/controller/authorization.cfc")) expect(denied.allowed).toBeFalse() expect(denied.status).toBe(403) + expect(denied.type).toBe("Wheels.NotAuthorized") + expect(FindNoCase("abort;", src)).toBeGT(0) try { g.$header(statusCode = 200) } catch (any e) { } + StructDelete(request, "$wheelsIsolateAbort") }) it("S9: authorize() with action=scope throws Wheels.NotAuthorized and does not Invoke scope", () => { @@ -215,10 +243,18 @@ component extends="wheels.WheelsTest" { expect(_controller.can("show", post)).toBeTrue() }) - it("returns false for a custom action the policy has no method for", () => { + it("S1: can() throws Wheels.Policy.UnknownAction for a missing policy method", () => { + request.$policyTestUser = {id = author.id} + + expect(() => _controller.can("publish", post)).toThrow(type = "Wheels.Policy.UnknownAction") + }) + + it("S4: can() denies the CFML strings yes and true and grants boolean true", () => { request.$policyTestUser = {id = author.id} - expect(_controller.can("publish", post)).toBeFalse() + expect(_controller.can("yesGrant", post)).toBeFalse() + expect(_controller.can("trueGrant", post)).toBeFalse() + expect(_controller.can("boolGrant", post)).toBeTrue() }) it("returns false for an empty record", () => { @@ -341,6 +377,65 @@ component extends="wheels.WheelsTest" { expect(identity.name).toBe("policy-di-user") }) + it("S3: a throwing DI currentUser does not fail over to the authenticator user", () => { + savedDi = application.wheelsdi + di = new wheels.Injector(binderPath = "wheels.tests._assets.di.TestBindings") + di.map("currentUser").to("wheels.tests._assets.policies.CurrentUserThrowingStub") + di.map("authenticator").to("wheels.auth.Authenticator").asSingleton() + application.wheelsdi = di + state = {threw = false, type = "", userId = ""} + 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"}) + try { + resolved = plain.$currentUserForPolicy() + if (IsStruct(resolved) && StructKeyExists(resolved, "id")) { + state.userId = resolved.id + } + } catch (any e) { + state.threw = true + state.type = e.type + } + } finally { + application.wheelsdi = savedDi + StructDelete(session, "wheels") + } + + expect(state.threw).toBeTrue() + expect(state.userId).toBe("") + expect(state.type).toBe("Wheels.Policy.CurrentUserBoom") + }) + + it("S3: a throwing authenticator currentUser does not become guest empty string", () => { + savedDi = application.wheelsdi + di = new wheels.Injector(binderPath = "wheels.tests._assets.di.TestBindings") + di.map("authenticator").to("wheels.auth.Authenticator").asSingleton() + application.wheelsdi = di + state = {threw = false, type = "", guest = false} + try { + auth = di.getInstance("authenticator") + throwingStrategy = CreateObject("component", "wheels.tests._assets.policies.ThrowingCurrentUserStrategy") + auth.registerStrategy(name = "throwing", strategy = throwingStrategy) + plain = g.controller("test", {controller = "test", action = "show"}) + try { + resolved = plain.$currentUserForPolicy() + state.guest = (IsSimpleValue(resolved) && resolved == "") + } catch (any e) { + state.threw = true + state.type = e.type + } + } finally { + application.wheelsdi = savedDi + } + + expect(state.threw).toBeTrue() + expect(state.guest).toBeFalse() + expect(state.type).toBe("Wheels.Policy.AuthenticatorBoom") + }) + it("S6: policy identity comes from the authenticator strategy currentUser()", () => { savedDi = application.wheelsdi di = new wheels.Injector(binderPath = "wheels.tests._assets.di.TestBindings")