Skip to content

Commit f65f1f6

Browse files
clavedelunaclavedeluna
authored
New codemod: safe-lxml-parsing (pixee#64)
* codemod safe lxml parsing * fix import lxml to import lxml.etree * up lint time
1 parent 1c506b5 commit f65f1f6

12 files changed

Lines changed: 216 additions & 9 deletions

‎.github/workflows/lint.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ jobs:
1818
linting:
1919
name: Pylint & Black Checks
2020
runs-on: ubuntu-20.04
21-
timeout-minutes: 3
21+
timeout-minutes: 5
2222
steps:
2323
- name: Set Up Python
2424
uses: actions/setup-python@v4

‎integration_tests/test_lxml_safe_parser_defaults.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,6 @@ class TestLxmlSafeParserDefaults(BaseIntegrationTest):
1111
original_code, expected_new_code = original_and_expected_from_code_path(
1212
code_path, [(1, "parser = lxml.etree.XMLParser(resolve_entities=False)\n")]
1313
)
14-
expected_diff = "--- \n+++ \n@@ -1,2 +1,2 @@\n import lxml\n-parser = lxml.etree.XMLParser()\n+parser = lxml.etree.XMLParser(resolve_entities=False)\n"
14+
expected_diff = "--- \n+++ \n@@ -1,2 +1,2 @@\n import lxml.etree\n-parser = lxml.etree.XMLParser()\n+parser = lxml.etree.XMLParser(resolve_entities=False)\n"
1515
expected_line_change = "2"
1616
change_description = LxmlSafeParserDefaults.CHANGE_DESCRIPTION

‎integration_tests/test_lxml_safe_parsing.py‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
from core_codemods.lxml_safe_parsing import LxmlSafeParsing
2+
from integration_tests.base_test import (
3+
BaseIntegrationTest,
4+
original_and_expected_from_code_path,
5+
)
6+
7+
8+
class TestLxmlSafeParsing(BaseIntegrationTest):
9+
codemod = LxmlSafeParsing
10+
code_path = "tests/samples/lxml_parsing.py"
11+
original_code, expected_new_code = original_and_expected_from_code_path(
12+
code_path,
13+
[
14+
(
15+
1,
16+
'lxml.etree.parse("path_to_file", parser=lxml.etree.XMLParser(resolve_entities=False))\n',
17+
),
18+
(
19+
2,
20+
'lxml.etree.fromstring("xml_str", parser=lxml.etree.XMLParser(resolve_entities=False))\n',
21+
),
22+
],
23+
)
24+
expected_diff = '--- \n+++ \n@@ -1,3 +1,3 @@\n import lxml.etree\n-lxml.etree.parse("path_to_file")\n-lxml.etree.fromstring("xml_str")\n+lxml.etree.parse("path_to_file", parser=lxml.etree.XMLParser(resolve_entities=False))\n+lxml.etree.fromstring("xml_str", parser=lxml.etree.XMLParser(resolve_entities=False))\n'
25+
expected_line_change = "2"
26+
num_changes = 2
27+
change_description = LxmlSafeParsing.CHANGE_DESCRIPTION

‎src/core_codemods/__init__.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
from .jwt_decode_verify import JwtDecodeVerify
1111
from .limit_readline import LimitReadline
1212
from .lxml_safe_parser_defaults import LxmlSafeParserDefaults
13+
from .lxml_safe_parsing import LxmlSafeParsing
1314
from .order_imports import OrderImports
1415
from .process_creation_sandbox import ProcessSandbox
1516
from .remove_unnecessary_f_str import RemoveUnnecessaryFStr
@@ -38,6 +39,7 @@
3839
JwtDecodeVerify,
3940
LimitReadline,
4041
LxmlSafeParserDefaults,
42+
LxmlSafeParsing,
4143
OrderImports,
4244
ProcessSandbox,
4345
RemoveUnnecessaryFStr,

‎src/core_codemods/docs/pixee_python_safe-lxml-parser-defaults.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ Parameter `resolve_entities` has an unsafe default value of `True`. This codemod
1111
The changes look as follows:
1212

1313
```diff
14-
import lxml
14+
import lxml.etree
1515

1616
- parser = lxml.etree.XMLParser()
1717
- parser = lxml.etree.XMLParser(resolve_entities=True)

‎src/core_codemods/docs/pixee_python_safe-lxml-parsing.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
This codemod sets the `parser` parameter in calls to `lxml.etree.parse` and `lxml.etree.fromstring`
2+
if omitted or set to `None` (the default value). Unfortunately, the default `parser=None` means `lxml`
3+
will rely on an unsafe parser, making your code potentially vulnerable to entity expansion
4+
attacks and external entity (XXE) attacks.
5+
6+
The changes look as follows:
7+
8+
```diff
9+
import lxml.etree
10+
- lxml.etree.parse("path_to_file")
11+
- lxml.etree.fromstring("xml_str")
12+
+ lxml.etree.parse("path_to_file", parser=lxml.etree.XMLParser(resolve_entities=False))
13+
+ lxml.etree.fromstring("xml_str", parser=lxml.etree.XMLParser(resolve_entities=False))
14+
```

‎src/core_codemods/lxml_safe_parser_defaults.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ def rule(cls):
2727
- pattern: XMLTreeBuilder
2828
- pattern: XMLPullParser
2929
- pattern-inside: |
30-
import lxml
30+
import lxml.etree
3131
...
3232
"""
3333

‎src/core_codemods/lxml_safe_parsing.py‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
from codemodder.codemods.base_codemod import ReviewGuidance
2+
from codemodder.codemods.api import SemgrepCodemod
3+
from codemodder.codemods.api.helpers import NewArg
4+
5+
6+
class LxmlSafeParsing(SemgrepCodemod):
7+
NAME = "safe-lxml-parsing"
8+
REVIEW_GUIDANCE = ReviewGuidance.MERGE_WITHOUT_REVIEW
9+
SUMMARY = "Use safe parsers in lxml parsing functions"
10+
DESCRIPTION = (
11+
"Call `lxml.etree.parse` and `lxml.etree.fromstring` with a safe parser"
12+
)
13+
14+
@classmethod
15+
def rule(cls):
16+
return """
17+
rules:
18+
- pattern-either:
19+
- patterns:
20+
- pattern: lxml.etree.$FUNC(...)
21+
- pattern-not: lxml.etree.$FUNC(...,parser=..., ...)
22+
- metavariable-pattern:
23+
metavariable: $FUNC
24+
patterns:
25+
- pattern-either:
26+
- pattern: parse
27+
- pattern: fromstring
28+
- pattern-inside: |
29+
import lxml.etree
30+
...
31+
- patterns:
32+
- pattern: lxml.etree.$FUNC(..., parser=None, ...)
33+
- metavariable-pattern:
34+
metavariable: $FUNC
35+
patterns:
36+
- pattern-either:
37+
- pattern: parse
38+
- pattern: fromstring
39+
- pattern-inside: |
40+
import lxml.etree
41+
...
42+
"""
43+
44+
def on_result_found(self, original_node, updated_node):
45+
self.remove_unused_import(original_node)
46+
self.add_needed_import("lxml.etree")
47+
safe_parser = "lxml.etree.XMLParser(resolve_entities=False)"
48+
new_args = self.replace_args(
49+
original_node,
50+
[NewArg(name="parser", value=safe_parser, add_if_missing=True)],
51+
)
52+
return self.update_arg_target(updated_node, new_args)

‎tests/codemods/test_lxml_safe_parameter_defaults.py‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,12 @@ def test_name(self):
1515

1616
@each_class
1717
def test_import(self, tmpdir, klass):
18-
input_code = f"""import lxml
18+
input_code = f"""import lxml.etree
1919
2020
parser = lxml.etree.{klass}()
2121
var = "hello"
2222
"""
23-
expexted_output = f"""import lxml
23+
expexted_output = f"""import lxml.etree
2424
2525
parser = lxml.etree.{klass}(resolve_entities=False)
2626
var = "hello"
@@ -104,11 +104,11 @@ def test_import_alias(self, tmpdir, klass):
104104
)
105105
@each_class
106106
def test_verify_variations(self, tmpdir, klass, input_args, expected_args):
107-
input_code = f"""import lxml
107+
input_code = f"""import lxml.etree
108108
parser = lxml.etree.{klass}({input_args})
109109
var = "hello"
110110
"""
111-
expexted_output = f"""import lxml
111+
expexted_output = f"""import lxml.etree
112112
parser = lxml.etree.{klass}({expected_args})
113113
var = "hello"
114114
"""

‎tests/codemods/test_lxml_safe_parsing.py‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
import pytest
2+
from core_codemods.lxml_safe_parsing import LxmlSafeParsing
3+
from tests.codemods.base_codemod_test import BaseSemgrepCodemodTest
4+
5+
each_func = pytest.mark.parametrize("func", ["parse", "fromstring"])
6+
7+
8+
class TestLxmlSafeParsing(BaseSemgrepCodemodTest):
9+
codemod = LxmlSafeParsing
10+
11+
def test_name(self):
12+
assert self.codemod.name() == "safe-lxml-parsing"
13+
14+
@each_func
15+
def test_import(self, tmpdir, func):
16+
input_code = f"""import lxml.etree
17+
18+
lxml.etree.{func}("path_to_file")
19+
var = "hello"
20+
"""
21+
expexted_output = f"""import lxml.etree
22+
23+
lxml.etree.{func}("path_to_file", parser=lxml.etree.XMLParser(resolve_entities=False))
24+
var = "hello"
25+
"""
26+
27+
self.run_and_assert(tmpdir, input_code, expexted_output)
28+
29+
@each_func
30+
def test_from_import(self, tmpdir, func):
31+
input_code = f"""from lxml.etree import {func}
32+
33+
{func}("path_to_file")
34+
var = "hello"
35+
"""
36+
expexted_output = f"""from lxml.etree import {func}
37+
import lxml.etree
38+
39+
{func}("path_to_file", parser=lxml.etree.XMLParser(resolve_entities=False))
40+
var = "hello"
41+
"""
42+
43+
self.run_and_assert(tmpdir, input_code, expexted_output)
44+
45+
@each_func
46+
def test_from_import_module(self, tmpdir, func):
47+
input_code = f"""from lxml import etree
48+
49+
etree.{func}("path_to_file")
50+
var = "hello"
51+
"""
52+
expexted_output = f"""from lxml import etree
53+
import lxml.etree
54+
55+
etree.{func}("path_to_file", parser=lxml.etree.XMLParser(resolve_entities=False))
56+
var = "hello"
57+
"""
58+
59+
self.run_and_assert(tmpdir, input_code, expexted_output)
60+
61+
@each_func
62+
def test_import_alias(self, tmpdir, func):
63+
input_code = f"""from lxml.etree import {func} as func
64+
65+
func("path_to_file")
66+
var = "hello"
67+
"""
68+
expexted_output = f"""from lxml.etree import {func} as func
69+
import lxml.etree
70+
71+
func("path_to_file", parser=lxml.etree.XMLParser(resolve_entities=False))
72+
var = "hello"
73+
"""
74+
75+
self.run_and_assert(tmpdir, input_code, expexted_output)
76+
77+
@pytest.mark.parametrize(
78+
"input_args,expected_args",
79+
[
80+
(
81+
"'str', parser=None",
82+
"'str', parser=lxml.etree.XMLParser(resolve_entities=False)",
83+
),
84+
(
85+
"source, base_url='url', parser=None",
86+
"source, base_url='url', parser=lxml.etree.XMLParser(resolve_entities=False)",
87+
),
88+
(
89+
"source, parser=lxml.etree.XMLParser(resolve_entities=False)",
90+
"source, parser=lxml.etree.XMLParser(resolve_entities=False)",
91+
),
92+
# This case would be changed by `safe-lxml-parser-defaults` codemod
93+
(
94+
"source, parser=lxml.etree.XMLParser()",
95+
"source, parser=lxml.etree.XMLParser()",
96+
),
97+
],
98+
)
99+
@each_func
100+
def test_verify_variations(self, tmpdir, func, input_args, expected_args):
101+
input_code = f"""import lxml.etree
102+
lxml.etree.{func}({input_args})
103+
var = "hello"
104+
"""
105+
expexted_output = f"""import lxml.etree
106+
lxml.etree.{func}({expected_args})
107+
var = "hello"
108+
"""
109+
self.run_and_assert(tmpdir, input_code, expexted_output)

0 commit comments

Comments
 (0)