Fix guarded_getattr ImplPython/ImplC assertion-key parity#165
Merged
Conversation
The pure-Python ``guarded_getattr`` keyed the container assertion on ``type(value.__self__)`` while the C implementation keys on ``type(inst)``. As a result, under ``PURE_PYTHON`` a bound method whose ``__self__`` is an allow-listed simple type (``tuple``/``bytes``/``range``/...) took the truthy-assertion short-circuit and was returned without calling ``validate()``, diverging from the C engine. Key the assertion lookup on ``type(inst)`` to match ``cAccessControl``, and add a regression test (red under ``PURE_PYTHON`` before the fix, green after; the C engine was already correct). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dataflake
approved these changes
Jun 6, 2026
dataflake
left a comment
dataflake
left a comment
Member
There was a problem hiding this comment.
Nice catch. How did you even find this?
Member
Author
Actually I was hunting something completely different - with agentic help - with focus on a report addressed to the Plone security team. And while doing so the agents deep analysis came up with this one too. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #166.
What
guarded_getattrderived its container-assertion lookup key from different objects in the two implementations:ImplPythonkeyed ontype(value.__self__)(src/AccessControl/ImplPython.py).cAccessControlkeys ontype(inst)(src/AccessControl/cAccessControl.c, whose own comment readsassertion = Containers(type(inst))).When an attribute of an object that has no container assertion is a bound method whose
__self__is an allow-listed simple type (tuple,bytes,range, the BTree*Itemsviews,dict_keys/dict_values/dict_items, or anyallow_type(T, 1)type), the pure-Python engine took the truthy-assertion short-circuit and returned the value without callingvalidate(), while the C engine routed throughvalidate(). So the two engines could reach different access decisions, and underPURE_PYTHON/ PyPy the permission check was skipped.Fix
Key the assertion short-circuit in
ImplPython.guarded_getattrontype(inst), matchingcAccessControl. (Thevalue.__self__derivation was the only difference; the rest of the function already mirrors the C logic.)Test
Added
TestGuardedGetattr.test_validates_bound_method_of_allow_listed_type, which accesses an attribute that is a bound method of an allow-listedtupleon an object with no container assertion under a rejectingSecurityManager, and assertsUnauthorizedis raised andvalidate()is called.PURE_PYTHON=1: the new test fails (Unauthorized not raised by guarded_getattr).Full suite green on CPython 3.14.3:
Ran 320 tests with 0 failures, 0 errors.PURE_PYTHON=1:Ran 299 tests with 0 failures, 0 errors and 21 skipped.