Skip to content

Commit 919f2d9

Browse files
dereuromarkdereuromark
authored
Fix isPublic plugin/prefix scope leak and hasRole type juggling (#165)
AclTrait::_isPublic() (src/Auth/AclTrait.php:317) used asymmetric guards that short-circuited to false when the request had no plugin or prefix, so a plugin-scoped or prefix-scoped allow rule would match a request without one. Mirror the symmetric check from AllowTrait::_getAllowRule() so a rule is skipped whenever either side has a scope and they do not match. AuthUserTrait::hasRole() (src/Auth/AuthUserTrait.php:122) used a loose in_array() that matched 0 against any non-numeric string and treated auto-int array keys as alias matches. Coerce both sides to string and compare strictly while still treating numeric-string and int as the same role id; only string array keys count as alias matches. Reject non-scalar inputs early. Adds regression tests for both code paths.
1 parent fae4a8b commit 919f2d9

4 files changed

Lines changed: 120 additions & 6 deletions

File tree

‎src/Auth/AclTrait.php‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -315,11 +315,23 @@ protected function _isPublic(array $params) {
315315
$authentication = $this->_getAuth();
316316

317317
foreach ($authentication as $rule) {
318-
if ($params['plugin'] && $params['plugin'] !== $rule['plugin']) {
319-
continue;
318+
if (!empty($params['plugin'])) {
319+
if ($params['plugin'] !== $rule['plugin']) {
320+
continue;
321+
}
322+
} else {
323+
if (!empty($rule['plugin'])) {
324+
continue;
325+
}
320326
}
321-
if (!empty($params['prefix']) && $params['prefix'] !== $rule['prefix']) {
322-
continue;
327+
if (!empty($params['prefix'])) {
328+
if ($params['prefix'] !== $rule['prefix']) {
329+
continue;
330+
}
331+
} else {
332+
if (!empty($rule['prefix'])) {
333+
continue;
334+
}
323335
}
324336
if ($params['controller'] !== $rule['controller']) {
325337
continue;

‎src/Auth/AuthUserTrait.php‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,14 @@ public function roles(): array {
104104
/**
105105
* Check if the current session has this role.
106106
*
107+
* Comparison is performed strictly after coercing both the expected role
108+
* and the provided role values to string. Without this normalization, the
109+
* loose `in_array()` semantics make e.g. `0 == 'admin'` evaluate to true
110+
* (PHP < 8) and mixed-type role IDs from different sources (DB ints vs.
111+
* Configure strings) would match by accident. String coercion preserves
112+
* cross-type equivalence between numeric strings and ints (the common
113+
* legacy case) while rejecting unrelated scalar/string juggling.
114+
*
107115
* @param mixed $expectedRole
108116
* @param mixed|null $providedRoles
109117
* @return bool Success
@@ -119,8 +127,23 @@ public function hasRole($expectedRole, $providedRoles = null) {
119127
return false;
120128
}
121129

122-
if (array_key_exists($expectedRole, $roles) || in_array($expectedRole, $roles)) {
123-
return true;
130+
if (!is_scalar($expectedRole)) {
131+
return false;
132+
}
133+
134+
$expectedRoleString = (string)$expectedRole;
135+
foreach ($roles as $key => $role) {
136+
// Only string keys are role aliases; auto-int keys would otherwise
137+
// produce a false positive for `hasRole(0, [0 => 'admin'])`.
138+
if (is_string($key) && $key === $expectedRoleString) {
139+
return true;
140+
}
141+
if (!is_scalar($role)) {
142+
continue;
143+
}
144+
if ((string)$role === $expectedRoleString) {
145+
return true;
146+
}
124147
}
125148

126149
return false;

‎tests/TestCase/Controller/Component/AuthUserComponentTest.php‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -461,6 +461,27 @@ public function testHasRole() {
461461
$this->assertFalse($this->AuthUser->hasRole(3, [2, 4]));
462462
}
463463

464+
/**
465+
* Regression: role lookup must not accept type-juggled false positives.
466+
*
467+
* Without normalization, `in_array(0, ['admin'])` evaluates to `true` on
468+
* PHP < 8 and `in_array('1abc', [1])` evaluates to `true` on any version
469+
* — both of which would let an unrelated value masquerade as a real role.
470+
*
471+
* @return void
472+
*/
473+
public function testHasRoleRejectsLooseTypeJugglingMatches() {
474+
$this->assertFalse($this->AuthUser->hasRole(0, ['admin', 'moderator']));
475+
$this->assertFalse($this->AuthUser->hasRole('1abc', [1, 2, 3]));
476+
$this->assertFalse($this->AuthUser->hasRole('admin', [0, 1, 2]));
477+
$this->assertFalse($this->AuthUser->hasRole(null, ['admin']));
478+
$this->assertFalse($this->AuthUser->hasRole(true, ['admin']));
479+
480+
// Numeric-string / int equivalence is preserved on purpose.
481+
$this->assertTrue($this->AuthUser->hasRole('2', [1, 2, 3]));
482+
$this->assertTrue($this->AuthUser->hasRole(2, ['1', '2', '3']));
483+
}
484+
464485
/**
465486
* @return void
466487
*/

‎tests/TestCase/Controller/Component/AuthenticationComponentTest.php‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,64 @@ public function testIsPublicFail() {
9999
$this->assertFalse($result);
100100
}
101101

102+
/**
103+
* Regression: a rule scoped to a plugin must not match a request without one.
104+
*
105+
* `Extras.Offers = "!superPrivate", *` defines a plugin-scoped rule.
106+
* A request to non-plugin `Offers::view` previously matched it because
107+
* `_isPublic()` short-circuited the plugin guard when the request had none.
108+
*
109+
* @return void
110+
*/
111+
public function testIsPublicDoesNotMatchPluginRuleForNonPluginRequest() {
112+
$request = new ServerRequest([
113+
'params' => [
114+
'controller' => 'Offers',
115+
'action' => 'view',
116+
'plugin' => null,
117+
'_ext' => null,
118+
'pass' => [],
119+
],
120+
]);
121+
$controller = $this->getControllerMock($request);
122+
$registry = new ComponentRegistry($controller);
123+
$this->component = new AuthenticationComponent($registry, $this->componentConfig);
124+
125+
$result = $this->component->isPublic();
126+
$this->assertFalse($result);
127+
}
128+
129+
/**
130+
* Regression: a rule scoped to a prefix must not match a request without one.
131+
*
132+
* `Admin/Users = index` is prefix-scoped to `Admin`. A non-prefix request
133+
* to `Users::index` is already covered by the unprefixed `Users` rule, but
134+
* a non-prefix request to `Users::view` (which is allowed unprefixed too)
135+
* must not pull in the Admin-scoped rule. We exercise the inverse path by
136+
* requesting an action that is only allowed under the Admin prefix to make
137+
* sure the prefix-scoped rule does not leak to non-prefix requests.
138+
*
139+
* @return void
140+
*/
141+
public function testIsPublicDoesNotMatchPrefixRuleForNonPrefixRequest() {
142+
$request = new ServerRequest([
143+
'params' => [
144+
'controller' => 'MyTest',
145+
'action' => 'myPublic',
146+
'plugin' => null,
147+
'prefix' => null,
148+
'_ext' => null,
149+
'pass' => [],
150+
],
151+
]);
152+
$controller = $this->getControllerMock($request);
153+
$registry = new ComponentRegistry($controller);
154+
$this->component = new AuthenticationComponent($registry, $this->componentConfig);
155+
156+
$result = $this->component->isPublic();
157+
$this->assertFalse($result);
158+
}
159+
102160
/**
103161
* @return void
104162
*/

0 commit comments

Comments
 (0)