PR#351: Instead of bending linters to allow eval, stop using eval - FedoraReview

FedoraReview

#351 Instead of bending linters to allow eval, stop using eval

Merged 5 years ago by ngompa. Opened 5 years ago by churchyard.

churchyard/FedoraReview noeval into master

Instead of bending linters to allow eval, stop using eval

Miro Hrončok • 5 years ago

3a05843

plugins/shell_api.py

file modified

+2 -3

		`@@ -20,7 +20,7 @@`
		`and the regular python plugins.`
		`"""`

		`- import logging # pylint: disable=W0611`
		`+ import logging`
		`import os`
		`import os.path`
		`import re`
		`@@ -473,8 +473,7 @@`
		`for line in f.readlines():`
		`try:`
		`tag, msg = line.split(":")`
		`- # pylint: disable=eval-used`
		`- level = eval("logging." + tag.upper())`
		`+ level = getattr(logging, tag.upper())`
		`except (ValueError, AttributeError):`
		`self.log.error("Malformed plugin log: " + line)`
		`self.log.log(level, msg)`

src/FedoraReview/settings.py

file modified

+3 -3

		`@@ -462,9 +462,9 @@`
		`if not lvl:`
		`if "REVIEW_LOGLEVEL" in os.environ:`
		`try:`
		`- # pylint: disable=eval-used`
		`- lvl = eval("logging." + os.environ["REVIEW_LOGLEVEL"].upper())`
		`- except (ValueError, SyntaxError):`
		`+ lvl = os.environ["REVIEW_LOGLEVEL"].upper()`
		`+ lvl = getattr(logging, lvl)`
		`+ except AttributeError:`
		`msg = "Cannot set loglevel from REVIEW_LOGLEVEL"`
		`lvl = logging.INFO`
		`else:`

churchyard commented 5 years ago

This is not tested, I'm doing it on a train. However it should work and it is certainly a proper way to do it, something I cannot say about the current way.

😱😱😱

rebased onto 1d112a0bfb5a7c2c6009f6c8161a4f40f586fb9d

5 years ago

churchyard commented 5 years ago

Not to mention, that the current code is actually exploitable (at least on 3.8, but if you know some upper case Python magic, maybe even sooner):

Python 3.8.0a2 (default, Feb 26 2019, 22:04:39) 
[GCC 8.2.1 20181215 (Red Hat 8.2.1-6)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> PARSER_SECTION = "review"
>>> import logging
>>> import os
>>> os.environ["REVIEW_LOGLEVEL"] = "DEBUG * (PARSER_SECTION:='boom!')"
>>> PARSER_SECTION
'review'
>>> lvl = eval("logging." + os.environ["REVIEW_LOGLEVEL"].upper())
>>> PARSER_SECTION
'BOOM!'

Edited 5 years ago by churchyard

rebased onto d22b13c1f2359b2cbb8adb3c5294e8d2c78dcdd5

5 years ago

Metadata Update from @churchyard:
- Request assigned

5 years ago

rebased onto 3a05843

5 years ago

ngompa commented 5 years ago

@churchyard I'm confused, is this actually a complete change that can be pulled in? I don't see if there's anything else that needs to be done other than I pull it in...?