Skip to content

Commit 9ec3d51

Browse files
borealisalastor-byteXiaJunjie2020
authored andcommitted
test(datasource): cover the Excel upload routes
Reuses the offline harness of the import-traversal guard and posts multipart bodies with hostile file names.
1 parent 4748f9a commit 9ec3d51

1 file changed

Lines changed: 124 additions & 0 deletions

File tree

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
"""Offline HTTP regressions for the Excel upload routes (CWE-22, issue #1448).
2+
3+
These tests post real multipart bodies to `/datasource/parseExcel` and to the
4+
deprecated `/datasource/uploadExcel`, and reuse the offline harness from
5+
`test_datasource_import_path_traversal.py`: identity, audit logging, the database
6+
session and the Excel reader are test adapters, so no application startup,
7+
external database or deployed service is used.
8+
9+
Before the fix the stored name kept whatever the client sent apart from POSIX
10+
directory parts (`..\\..\\evil.xlsx` -> `..\\evil_<hash>.xlsx`), which resolves
11+
outside the upload directory on a Windows host, and `uploadExcel` matched bare
12+
suffixes and indexed the name with `split('.')[1]`. The assertions below are the
13+
platform-independent view of that contract: whatever the client sends, the stored
14+
name is a single component inside the upload directory, or the request is refused.
15+
"""
16+
17+
from unittest.mock import Mock
18+
19+
import pytest
20+
from test_datasource_import_path_traversal import (
21+
api, # noqa: F401 # shared offline harness, resolved by fixture name
22+
)
23+
24+
from apps.datasource.api import datasource
25+
26+
PARSE_URL = "/datasource/parseExcel"
27+
UPLOAD_URL = "/datasource/uploadExcel"
28+
XLSX_TYPE = "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"
29+
INVALID_FILE_PATH = "i18n_ds_invalid_file_path"
30+
31+
# Names whose directory part must be dropped, not written.
32+
STRIPPED_NAMES = [
33+
"..\\..\\evil.xlsx",
34+
"..\\evil.xlsx",
35+
"nested\\..\\..\\evil.xlsx",
36+
"../../evil.xlsx",
37+
"/tmp/orders.xlsx",
38+
"C:\\Users\\me\\orders.xlsx",
39+
]
40+
41+
# Names that would still address another location after dropping the separators.
42+
REJECTED_NAMES = ["C:evil.xlsx", "C:.xlsx", "orders.xlsx:extra"]
43+
44+
45+
def stored_files(root):
46+
return {path for path in root.rglob("*") if path.is_file()}
47+
48+
49+
def upload(client, url, file_name):
50+
return client.post(url, files={"file": (file_name, b"a\n1\n", XLSX_TYPE)})
51+
52+
53+
@pytest.fixture
54+
def harness(request):
55+
"""The offline API harness defined by the import-traversal guard."""
56+
return request.getfixturevalue("api")
57+
58+
59+
@pytest.fixture
60+
def parse_harness(harness):
61+
# The preview reader is not part of this contract and would need a real workbook.
62+
harness.monkeypatch.setattr(
63+
datasource, "parse_excel_preview", Mock(return_value=[])
64+
)
65+
return harness
66+
67+
68+
@pytest.mark.parametrize("file_name", STRIPPED_NAMES)
69+
def test_parse_excel_stores_one_component_inside_the_upload_dir(
70+
parse_harness, file_name
71+
):
72+
before = stored_files(parse_harness.tmp_path)
73+
response = upload(parse_harness.client, PARSE_URL, file_name)
74+
assert response.status_code == 200, response.text
75+
stored = response.json()["filePath"]
76+
assert "/" not in stored and "\\" not in stored and ":" not in stored, stored
77+
assert (parse_harness.upload_dir / stored).is_file()
78+
created = stored_files(parse_harness.tmp_path) - before
79+
assert created, f"nothing was stored for {file_name!r}"
80+
assert all(parse_harness.upload_dir in path.parents for path in created), created
81+
82+
83+
@pytest.mark.parametrize("file_name", STRIPPED_NAMES)
84+
def test_upload_excel_stores_one_component_inside_the_upload_dir(harness, file_name):
85+
before = stored_files(harness.tmp_path)
86+
response = upload(harness.client, UPLOAD_URL, file_name)
87+
# The deprecated route continues into the import engine, which the harness
88+
# stubs out; only the stored name is under test here.
89+
assert response.status_code in (200, 500), response.text
90+
created = stored_files(harness.tmp_path) - before
91+
assert created, f"nothing was stored for {file_name!r}"
92+
assert all(harness.upload_dir in path.parents for path in created), created
93+
assert all(path.name == path.name.strip("/\\") for path in created)
94+
assert all(":" not in path.name for path in created)
95+
96+
97+
@pytest.mark.parametrize("url", [PARSE_URL, UPLOAD_URL])
98+
@pytest.mark.parametrize("file_name", REJECTED_NAMES)
99+
def test_names_that_keep_a_drive_relative_location_are_refused(harness, url, file_name):
100+
before = stored_files(harness.tmp_path)
101+
response = upload(harness.client, url, file_name)
102+
assert response.status_code == 400, response.text
103+
assert response.json() == INVALID_FILE_PATH
104+
assert stored_files(harness.tmp_path) == before
105+
106+
107+
@pytest.mark.parametrize("file_name", ["evil.txlsx", "evilxlsx"])
108+
def test_upload_excel_refuses_names_that_only_end_with_the_bare_suffix(
109+
harness, file_name
110+
):
111+
# The whitelist used to be {"xlsx", "xls", "csv"}: "evil.txlsx" was accepted
112+
# and the dot-less "evilxlsx" made `split('.')[1]` raise IndexError.
113+
before = stored_files(harness.tmp_path)
114+
response = upload(harness.client, UPLOAD_URL, file_name)
115+
assert response.status_code == 400, response.text
116+
assert stored_files(harness.tmp_path) == before
117+
118+
119+
def test_normal_names_keep_their_own_stem_and_extension(parse_harness):
120+
response = upload(parse_harness.client, PARSE_URL, "a.b.xlsx")
121+
assert response.status_code == 200, response.text
122+
stored = response.json()["filePath"]
123+
assert stored.startswith("a.b_") and stored.endswith(".xlsx"), stored
124+
assert (parse_harness.upload_dir / stored).is_file()

0 commit comments

Comments
 (0)