gh-100690: Prevent prefix "called_" for attributes of safe mock objects#100691

cklein · 2023-01-02T15:51:44Z

Prevent prefix "called_" for mocks in safe mode

Issue: Prevent prefix "called_" for methods on mock objects in safe mode #100690

bedevere-bot · 2023-01-02T15:51:48Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

cjw296

I'm -0 on this change, feels like we're into the diminishing realms where someone out there is going to have a legitimate use case for called_name or some such and this then prevents them using a safe mock.

Who else is hitting this?

cklein · 2023-01-02T17:04:56Z

@cjw296, the same applies to any of the forbidden prefixes. This could be solved by allowing those prefixes with a spec. Currently, even if the name appears in _mock_methods (https://github.com/python/cpython/blob/main/Lib/unittest/mock.py#L651 evaluates to true), an error will be raised. But I guess this could be a different and more fundamental discussion.

cklein · 2023-01-02T21:32:12Z

I have been thinking a bit more about the whole problem. It seems the current behavior does not match the error message:

>>> classFoo: ... defassert_bar(self): ... pass ... >>> from unittest.mock import Mock >>> m = Mock(spec=Foo) >>> m.assert_bar() Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/opt/local/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/unittest/mock.py", line 648, in __getattr__ raise AttributeError( AttributeError: 'assert_bar' is not a valid assertion. Use a spec for the mock if 'assert_bar' is meant to be an attribute.

The error message says "Use a spec for the mock if 'assert_bar' is meant to be an attribute.", but this example should pass according to the error message. There's a spec and the name assert_bar should be supported.

With cklein@37aa291 applied, it behaves like described:

>>> classFoo: ... defassert_bar(self): ... pass ... >>> from unittest.mock import Mock >>> m = Mock(spec=Foo) >>> m.assert_bar() <Mock name='mock.assert_bar()' id='4384202080'> >>> m.assert_frobnicated() Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/cklein/github/cklein/cpython/Lib/unittest/mock.py", line 652, in __getattr__ raise AttributeError("Mock object has no attribute %r" % name) AttributeError: Mock object has no attribute 'assert_frobnicated'

merwok · 2023-01-03T00:13:26Z

The prefixes checked could be more specific like called_once_with, called_once, called_with to alleviate the concern about forbidding too much.

cjw296 · 2023-01-03T10:12:57Z

@cklein - I think you're right about this being a complicated issue, perhaps a PR isn't the best place to discuss it? Could you maybe close this PR an open an issue explaining your concerns and some possible ways forward?

AlexWaygood · 2023-01-03T10:48:32Z

Could you maybe close this PR an open an issue explaining your concerns and some possible ways forward?

OP has already done so:

They first opened a discussion on discuss.python.org, https://discuss.python.org/t/include-prefix-called-in-list-of-forbidden-method-prefixes-for-mock-objects-in-unsafe-mode/22249.
After discussing it there, they were advised by a core developer to open an issue: Prevent prefix "called_" for methods on mock objects in safe mode #100690
And a PR (this one).

cklein · 2023-01-03T10:49:43Z

I can also create another issue for my second branch about the spec not being properly checked, if that helps

Edit: See https://discuss.python.org/t/check-for-assert-prefixes-doesnt-respect-spec/22388

… mode Raise an AttributeError when calling e.g. `mock_obj.called_once` instead of `mock_obj.assert_called_once`.

cklein · 2023-01-03T15:33:18Z

I'm aware that the documentation is now off. I'd like to get some feedback on the change first, if that's OK

…prefix

… spec

Lib/unittest/mock.py

cjw296 · 2023-01-05T14:10:32Z

Lib/unittest/mock.py



+# gh-100690 Denylist for forbidden attribute names in safe mode
+ATTRIB_DENY_LIST={name.removeprefix("assert_") fornameindir(NonCallableMock) ifname.startswith("assert_")}


Much prefer this approach! 🎉

cjw296 · 2023-01-05T14:12:03Z

Approach is good, so once the docs are fixed up, should be good to land.
Would a backport release once that's done be helpful? (I mean, I'll probably crank the handle anyway, but thought I'd ask!)

cklein · 2023-01-05T21:01:52Z

I'm struggling a bit with documentation. What about the following:

 * `unsafe`: By default, accessing any attribute whose name starts with *assert*, *assret*, *asert*, *aseert*, or *assrt* raises an AttributeError. Additionally, an AttributeError is raised when accessing attributes that match the name of assertion method without the prefix `assert_`, e.g. accessing `called_once` instead of `assert_called_once`. Passing `unsafe=True` will allow access to these attributes.

Misc/NEWS.d/next/Library/2023-01-02-16-59-49.gh-issue-100690.2EgWPS.rst

cklein · 2023-01-06T17:45:00Z

Would a backport release once that's done be helpful?

Personally I'd like to see it in earlier versions of Python.
But then I'm a bit torn because on the one hand this will definitely make some tests fail, on the other hand those tests will finally be properly executed and some unwanted behavior might be uncovered.

cjw296 · 2023-01-06T18:00:42Z

@cklein - 3.10 and 3.11 are open for bug fixes so their next third point release would contain this once its landed.
...and to be clear: I consider this a bug in that there are checks for these problems but they are insufficient.

cjw296 · 2023-01-06T18:36:34Z

The Azure Pipelines failure seems unrelated.

AlexWaygood · 2023-01-06T18:38:58Z

The Azure Pipelines failure seems unrelated.

Yes, that failure happens intermittently on a lot of PRs

bedevere-bot · 2023-01-06T19:48:58Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE RHEL7 LTO 3.x has failed when building commit 1d4d677.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/503/builds/2845) and take a look at the build logs.
Check if the failure is related to this commit (1d4d677) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/503/builds/2845

Failed tests:

test_nntplib

Failed subtests:

tearDownClass - test.test_nntplib.NetworkedNNTPTests
test_with_statement - test.test_nntplib.NetworkedNNTPTests.test_with_statement

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

408 tests OK.

10 slowest tests:

test_concurrent_futures: 2 min 46 sec
test_multiprocessing_spawn: 1 min 45 sec
test_multiprocessing_forkserver: 1 min 20 sec
test_multiprocessing_fork: 1 min 9 sec
test_tokenize: 1 min 7 sec
test_asyncio: 1 min 2 sec
test_logging: 54.8 sec
test_signal: 48.0 sec
test_venv: 42.2 sec
test_io: 38.8 sec

1 test failed:
test_nntplib

24 tests skipped:
test_check_c_globals test_devpoll test_gdb test_idle test_ioctl
test_kqueue test_launcher test_msilib test_peg_generator
test_perf_profiler test_smtpnet test_ssl test_startfile test_tcl
test_tix test_tkinter test_ttk test_ttk_textonly test_turtle
test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

1 re-run test:
test_nntplib

Total duration: 7 min 7 sec

Click to see traceback logs

Traceback (most recent call last): File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/test/test_nntplib.py", line 252, in wrapped meth(self) File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/test/test_nntplib.py", line 286, in test_with_statement server =self.NNTP_CLASS(self.NNTP_HOST, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 341, in __init__self._base_init(readermode) File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 359, in _base_initself.getcapabilities() File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 421, in getcapabilities resp, caps =self.capabilities() ^^^^^^^^^^^^^^^^^^^ File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 590, in capabilities resp, lines =self._longcmdstring("CAPABILITIES") ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 557, in _longcmdstring resp, list=self._getlongresp(file) ^^^^^^^^^^^^^^^^^^^^^^^ File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 508, in _getlongresp resp =self._getresp() ^^^^^^^^^^^^^^^ File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 481, in _getresp resp =self._getline() ^^^^^^^^^^^^^^^ File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 469, in _getlineifnot line: raiseEOFError^^^^^^^^^^^^^^ EOFError Traceback (most recent call last): File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/test/test_nntplib.py", line 347, in tearDownClasscls.server.quit() File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 933, in quit resp =self._shortcmd('QUIT') ^^^^^^^^^^^^^^^^^^^^^^ File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 543, in _shortcmdreturnself._getresp() ^^^^^^^^^^^^^^^ File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 481, in _getresp resp =self._getline() ^^^^^^^^^^^^^^^ File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/nntplib.py", line 464, in _getline line =self.file.readline(_MAXLINE+1) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le.lto/build/Lib/socket.py", line 707, in readintoreturnself._sock.recv_into(b) ^^^^^^^^^^^^^^^^^^^^^^^TimeoutError: timed out

sobolevn · 2023-01-07T09:57:35Z

Lib/unittest/mock.py



+# Denylist for forbidden attribute names in safe mode
+ATTRIB_DENY_LIST={name.removeprefix("assert_") fornameindir(NonCallableMock) ifname.startswith("assert_")}


Sorry for being late to the party, but anyways :)
I think that ATTRIB_DENY_LIST is an implementation detail, so it should be _ATTRIB_DENY_LIST. And also, it is not suited for mutation, isn't it? So, it should be a frozenset instead :)

Since this PR is already merged, I've opened #100819 with this proposal. Feel free to close if it should be public and mutable!

It is already non-public by not being included in __all__, and the mutability is not really an issue.

cklein requested a review from cjw296 as a code owner January 2, 2023 15:51

bedevere-bot added the awaiting review label Jan 2, 2023

cklein changed the title ~~Prevent prefix "called_" for attributes of safe mock objects~~gh-100690: Prevent prefix "called_" for attributes of safe mock objectsJan 2, 2023

bedevere-bot mentioned this pull request Jan 2, 2023
Prevent prefix "called_" for methods on mock objects in safe mode #100690
Closed

cklein force-pushed the disallow_mock_method_prefix branch 2 times, most recently from 25e26d8 to 7bc8c46Compare January 2, 2023 16:25

cjw296 reviewed Jan 2, 2023
View reviewed changes

cklein force-pushed the disallow_mock_method_prefix branch from 7bc8c46 to b84bc75Compare January 2, 2023 16:46

Prevent calling assert functions without the prefix "assert_" in safe…
154012f
… mode Raise an AttributeError when calling e.g. `mock_obj.called_once` instead of `mock_obj.assert_called_once`.

cklein force-pushed the disallow_mock_method_prefix branch from b84bc75 to 154012fCompare January 3, 2023 11:35

merwok added the DO-NOT-MERGE label Jan 3, 2023

cklein added 2 commits January 5, 2023 07:43

Merge remote-tracking branch 'python/main' into disallow_mock_method_…
26dc793
…prefix

Added tests to show that names from denylist still can be used with a…
2bd97a1
… spec

cjw296 reviewed Jan 5, 2023
View reviewed changes

Lib/unittest/mock.py Outdated Show resolvedHide resolved

cjw296 reviewed Jan 5, 2023
View reviewed changes

merwok added type-feature A feature request or enhancement stdlib Standard Library Python modules in the Lib/ directory labels Jan 5, 2023

Changed documentation
01946cb

merwok reviewed Jan 6, 2023
View reviewed changes

Misc/NEWS.d/next/Library/2023-01-02-16-59-49.gh-issue-100690.2EgWPS.rst Outdated Show resolvedHide resolved

Reformatted NEWS
e9b8f73

cjw296 approved these changes Jan 6, 2023
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jan 6, 2023

Merge branch 'main' into disallow_mock_method_prefix
674f5bc

cjw296 added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes type-bug An unexpected behavior, bug, or error and removed DO-NOT-MERGE type-feature A feature request or enhancement labels Jan 6, 2023

merwok added the DO-NOT-MERGE label Jan 6, 2023

cjw296 removed DO-NOT-MERGE needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Jan 6, 2023

cjw296 merged commit 1d4d677 into python:mainJan 6, 2023

bedevere-bot removed the awaiting merge label Jan 6, 2023

sobolevn reviewed Jan 7, 2023
View reviewed changes

sobolevn mentioned this pull request Jan 7, 2023
gh-100690: [mock] hide ATTRIB_DENY_LIST and make it immutable #100819
Merged

carlosa54 mentioned this pull request Nov 5, 2024
Prevent .called_ prefix calls in mocks until we upgrade to python 3.12 routablehq/flake8-routable#22
Closed



		# gh-100690 Denylist for forbidden attribute names in safe mode
		ATTRIB_DENY_LIST={name.removeprefix("assert_") fornameindir(NonCallableMock) ifname.startswith("assert_")}



		# Denylist for forbidden attribute names in safe mode
		ATTRIB_DENY_LIST={name.removeprefix("assert_") fornameindir(NonCallableMock) ifname.startswith("assert_")}

Uh oh!

gh-100690: Prevent prefix "called_" for attributes of safe mock objects#100691

gh-100690: Prevent prefix "called_" for attributes of safe mock objects #100691

Uh oh!

Conversation

cklein commented Jan 2, 2023• edited by AlexWaygood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Jan 2, 2023

Uh oh!

cjw296 left a comment

Choose a reason for hiding this comment

Uh oh!

cklein commented Jan 2, 2023

Uh oh!

cklein commented Jan 2, 2023• edited by AlexWaygood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merwok commented Jan 3, 2023

Uh oh!

cjw296 commented Jan 3, 2023

Uh oh!

AlexWaygood commented Jan 3, 2023

Uh oh!

cklein commented Jan 3, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cklein commented Jan 3, 2023

Uh oh!

Uh oh!

cjw296Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

cjw296 commented Jan 5, 2023

Uh oh!

cklein commented Jan 5, 2023

Uh oh!

Uh oh!

cklein commented Jan 6, 2023

Uh oh!

cjw296 commented Jan 6, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjw296 commented Jan 6, 2023

Uh oh!

AlexWaygood commented Jan 6, 2023

Uh oh!

bedevere-bot commented Jan 6, 2023

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

sobolevnJan 7, 2023

Choose a reason for hiding this comment

Uh oh!

sobolevnJan 7, 2023

Choose a reason for hiding this comment

Uh oh!

merwokJan 7, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cklein commented Jan 2, 2023•
edited by AlexWaygood
Loading

cklein commented Jan 2, 2023•
edited by AlexWaygood
Loading

cklein commented Jan 3, 2023•
edited
Loading

cjw296 commented Jan 6, 2023•
edited
Loading