gh-103186: assert in tests that UnsafeMailcapInput warnings are provided by TabLand · Pull Request #103217 · python/cpython

TabLand · 2023-04-03T17:35:20Z

These warnings are expected as the function result being asserted is None

Issue: Regression tests pass but return unnecessary output (errors and warnings) #103186

The asserted result is the None value (unsafe input rejected by mailcap)

iritkatriel · 2023-04-05T12:18:14Z

+ with warnings_helper.check_warnings(('', tc[2]["warn_type"]), quiet=True):
+ self.assertEqual(mailcap.subst(*tc[0]), tc[1])
+ else:
+ self.assertEqual(mailcap.subst(*tc[0]), tc[1])


This seems a bit over-engineered. I'd split this into a separate test function, which tests that the warning is issued in cases that deserve the warning. This exception is raised from 3 places in the Lib/mailcap.py, so shouldn't we have at least 3 test cases for it?
CC @encukou .

Hi Irit,
Thank you, I'll rework the affected cases into a separate test function and update the patch.
Will add 2x new test cases for unsafe filename and parameters, only the MIME type warnings are raised at present

iritkatriel · 2023-04-06T14:07:27Z

+ with self.assertWarnsRegex(mailcap.UnsafeMailcapInput,'Refusing to substitute MIME type.*into a shell'):
+ self.assertEqual(mailcap.subst("echo %t", "audio/*", "foo.txt"), None)
+
+ with self.assertWarnsRegex(mailcap.UnsafeMailcapInput,'Refusing to use mailcap with filename.*Use a safe temporary filename.'):


PEP 8: wrap lines at 80 chars, and have whitespace between the , and the next function arg.
Suggested change
withself.assertWarnsRegex(mailcap.UnsafeMailcapInput,'Refusing to use mailcap with filename.*Use a safe temporary filename.'):
withself.assertWarnsRegex(mailcap.UnsafeMailcapInput,
'Refusing to use mailcap with filename.*Use a safe temporary filename.'):

iritkatriel · 2023-04-06T14:08:15Z

 self._run_cases(cases)

+ def test_unsafe_mailcap_input(self):
+ c = MAILCAPDICT


Suggested change
c=MAILCAPDICT
c=MAILCAPDICT

this is only used in one place, you could move it there.

iritkatriel · 2023-04-06T16:39:11Z

+ self.assertEqual(mailcap.findmatch(MAILCAPDICT,
+ "audio/wav", filename="foo*.txt"), (None, None))


We don't want to break the first arg into two lines and then continue with the second arg on the same line.
You can assign the return value of findmatch to a variable and then compare the variable to expected value.
Otherwise, add a newline before the (None, None) and indent it like "mailcap.findmatch".

iritkatriel · 2023-04-06T16:39:45Z


 def test_unsafe_mailcap_input(self):
- c = MAILCAPDICT
 plist = ["total=*"]


This is also only used in one place.

Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>

iritkatriel · 2023-04-06T16:47:04Z

+ self.assertEqual(mailcap.subst("echo %t", "audio/*", "foo.txt"), None)
+
+ with self.assertWarnsRegex(mailcap.UnsafeMailcapInput,
+ 'Refusing to use mailcap with filename.*Use a safe temporary filename.'):


Suggested change
'Refusing to use mailcap with filename.*Use a safe temporary filename.'):
'Refusing to use mailcap with filename.*'
'Use a safe temporary filename.'):

iritkatriel · 2023-04-06T16:47:31Z

+ plist = ["total=*"]
+ with self.assertWarnsRegex(mailcap.UnsafeMailcapInput,
+ 'Refusing to substitute parameter.*into a shell command'):
+ self.assertEqual(mailcap.subst("echo %{total}", "audio/wav", "foo.txt", plist), None)


this is also > 80 chars

iritkatriel · 2023-04-06T18:14:38Z

Thank you @TabLand, congrats on your first commit to cpython!

TabLand · 2023-04-06T18:25:45Z

First of hopefully lots of more in the future!
Many thanks!

… provided (python#103217)

miss-islington · 2023-09-02T05:11:24Z

Thanks @TabLand for the PR, and @iritkatriel for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

bedevere-bot · 2023-09-02T05:11:37Z

GH-108800 is a backport of this pull request to the 3.11 branch.

… provided (pythonGH-103217) (cherry picked from commit 1724553) Co-authored-by: Ijtaba Hussain <ijtabahussain@live.com>

…e provided (GH-103217) (GH-108800) (cherry picked from commit 1724553) Co-authored-by: Ijtaba Hussain <ijtabahussain@live.com>

TabLand added 2 commits April 3, 2023 22:29

Suppress warnings for test cases for unsafe input
a18d5f0
The asserted result is the None value (unsafe input rejected by mailcap)

Merge branch 'python:main' into fix-issue-103186-test_mailcap
f479570

TabLand requested a review from a team as a code owner April 3, 2023 17:35

bedevere-bot added the awaiting review label Apr 3, 2023

bedevere-bot mentioned this pull request Apr 3, 2023
Regression tests pass but return unnecessary output (errors and warnings) #103186
Closed

bedevere-bot added the tests Tests in the Lib/test dir label Apr 3, 2023

This comment was marked as outdated.
Sign in to view

arhadthedev added the skip news label Apr 3, 2023

iritkatriel reviewed Apr 5, 2023
View reviewed changes

arhadthedevand others added 2 commits April 6, 2023 05:36

Run patchcheck.py
f412f07

Rework unsafe input warnings into a new test
605d93f

arhadthedev added the topic-email label Apr 6, 2023

iritkatriel reviewed Apr 6, 2023
View reviewed changes

Style improvements
12e8373

iritkatriel reviewed Apr 6, 2023
View reviewed changes

Comment threadLib/test/test_mailcap.py Outdated

iritkatriel reviewed Apr 6, 2023
View reviewed changes

TabLandand others added 2 commits April 6, 2023 21:40

Formatting improvements
80d0ef7
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>

Further formatting improvements
8bd668d

iritkatriel reviewed Apr 6, 2023
View reviewed changes

TabLand added 2 commits April 6, 2023 21:54

Formatting improvements
820564d

Trim new test function code to less than 80 chars[F
bf5f22d

iritkatriel approved these changes Apr 6, 2023
View reviewed changes

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

iritkatriel changed the title ~~gh-103186 Suppress UnsafeMailcapInput warnings~~gh-103186: assert in tests that UnsafeMailcapInput warnings are providedApr 6, 2023

iritkatriel merged commit 1724553 into python:mainApr 6, 2023

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

warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
pythongh-103186: assert in tests that UnsafeMailcapInput warnings are…
e23bf26
… provided (python#103217)

serhiy-storchaka added the needs backport to 3.11 only security fixes label Sep 2, 2023

bedevere-bot removed the needs backport to 3.11 only security fixes label Sep 2, 2023

serhiy-storchaka pushed a commit that referenced this pull request Sep 2, 2023
[3.11]gh-103186: assert in tests that UnsafeMailcapInput warnings ar…
fc114a7
…e provided (GH-103217) (GH-108800) (cherry picked from commit 1724553) Co-authored-by: Ijtaba Hussain <ijtabahussain@live.com>

	withself.assertWarnsRegex(mailcap.UnsafeMailcapInput,'Refusing to use mailcap with filename.*Use a safe temporary filename.'):
	withself.assertWarnsRegex(mailcap.UnsafeMailcapInput,
	'Refusing to use mailcap with filename.*Use a safe temporary filename.'):

		self.assertEqual(mailcap.findmatch(MAILCAPDICT,
		"audio/wav", filename="foo*.txt"), (None, None))

	'Refusing to use mailcap with filename.*Use a safe temporary filename.'):
	'Refusing to use mailcap with filename.*'
	'Use a safe temporary filename.'):

Uh oh!

Uh oh!

Conversation

TabLand commented Apr 3, 2023• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iritkatriel commented Apr 6, 2023

Uh oh!

TabLand commented Apr 6, 2023

Uh oh!

miss-islington commented Sep 2, 2023

Uh oh!

bedevere-bot commented Sep 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

TabLand commented Apr 3, 2023•
edited by bedevere-bot
Loading