Skip to content

Commit 7103760

Browse files
stsewdhumitos
stsewd
and
humitos
authored
Merge pull request from GHSA-368m-86q9-m99w
* Prevent symlink following in path operations * Black * Fix tests * Updates * Tests * Add tests * Tests * Updates from review * Use build exception * Typo * Fix tests * Fix tests * Always raise an exception if the usage of symlink is invalid - the symlink points to a file that does not exist - the symlink points to a file _outside_ the DOCROOT/base_path - symlinks are not allowed in the operation In any of these cases, we hard fail the build and communicate a generic error (for now) to the user: "Symlinks are not fully supported". We can improve the message with more value in another commit/iteration. These are 3 different exceptions, so we can get as much granular as we want here. All of them are managed by Celery `on_failure` handler, which will automatically do the correct thing to finalize the build properly. * Convert `path=` to str() to avoid "not JSON serializable" issue * Update tests to check for new exceptions instead of `None` * Run `pre-commit` since it was not enabled by default Co-authored-by: Manuel Kaufmann <[email protected]>
1 parent 0ba968e commit 7103760

22 files changed

+538
-94
lines changed

‎readthedocs/builds/storage.py

+22-3
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
1-
import structlog
21
from pathlib import Path
32

3+
import structlog
44
from django.conf import settings
55
from django.core.exceptions import SuspiciousFileOperation
66
from django.core.files.storage import FileSystemStorage
77
from storages.utils import get_available_overwrite_name, safe_join
88

9+
from readthedocs.core.utils.filesystem import safe_open
10+
911
log = structlog.get_logger(__name__)
1012

1113

@@ -84,11 +86,20 @@ def copy_directory(self, source, destination):
8486
source = Path(source)
8587
for filepath in source.iterdir():
8688
sub_destination = self.join(destination, filepath.name)
89+
90+
# Don't follow symlinks when uploading to storage.
91+
if filepath.is_symlink():
92+
log.info(
93+
"Skipping symlink upload.",
94+
path_resolved=str(filepath.resolve()),
95+
)
96+
continue
97+
8798
if filepath.is_dir():
8899
# Recursively copy the subdirectory
89100
self.copy_directory(filepath, sub_destination)
90101
elif filepath.is_file():
91-
with filepath.open('rb') as fd:
102+
with safe_open(filepath, "rb") as fd:
92103
self.save(sub_destination, fd)
93104

94105
def sync_directory(self, source, destination):
@@ -114,12 +125,20 @@ def sync_directory(self, source, destination):
114125
copied_dirs = set()
115126
for filepath in source.iterdir():
116127
sub_destination = self.join(destination, filepath.name)
128+
# Don't follow symlinks when uploading to storage.
129+
if filepath.is_symlink():
130+
log.info(
131+
"Skipping symlink upload.",
132+
path_resolved=str(filepath.resolve()),
133+
)
134+
continue
135+
117136
if filepath.is_dir():
118137
# Recursively sync the subdirectory
119138
self.sync_directory(filepath, sub_destination)
120139
copied_dirs.add(filepath.name)
121140
elif filepath.is_file():
122-
with filepath.open('rb') as fd:
141+
with safe_open(filepath, "rb") as fd:
123142
self.save(sub_destination, fd)
124143
copied_files.add(filepath.name)
125144

‎readthedocs/config/config.py

+6-1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
from django.conf import settings
1212

1313
from readthedocs.config.utils import list_to_dict, to_dict
14+
from readthedocs.core.utils.filesystem import safe_open
1415
from readthedocs.projects.constants import GENERIC
1516

1617
from .find import find_one
@@ -1380,7 +1381,11 @@ def load(path, env_config):
13801381

13811382
if not filename:
13821383
raise ConfigFileNotFound(path)
1383-
with open(filename, 'r') as configuration_file:
1384+
1385+
# Allow symlinks, but only the ones that resolve inside the base directory.
1386+
with safe_open(
1387+
filename, "r", allow_symlinks=True, base_path=path
1388+
) as configuration_file:
13841389
try:
13851390
config = parse(configuration_file.read())
13861391
except ParseError as error:

‎readthedocs/config/tests/test_config.py

+23-11
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import pytest
88
from django.conf import settings
9+
from django.test import override_settings
910
from pytest import raises
1011

1112
from readthedocs.config import (
@@ -80,7 +81,8 @@ def test_load_no_config_file(tmpdir, files):
8081
apply_fs(tmpdir, files)
8182
base = str(tmpdir)
8283
with raises(ConfigFileNotFound) as e:
83-
load(base, {})
84+
with override_settings(DOCROOT=tmpdir):
85+
load(base, {})
8486
assert e.value.code == CONFIG_FILE_REQUIRED
8587

8688

@@ -92,13 +94,15 @@ def test_load_empty_config_file(tmpdir):
9294
)
9395
base = str(tmpdir)
9496
with raises(ConfigError):
95-
load(base, {})
97+
with override_settings(DOCROOT=tmpdir):
98+
load(base, {})
9699

97100

98101
def test_minimal_config(tmpdir):
99102
apply_fs(tmpdir, yaml_config_dir)
100103
base = str(tmpdir)
101-
build = load(base, {})
104+
with override_settings(DOCROOT=tmpdir):
105+
build = load(base, {})
102106
assert isinstance(build, BuildConfigV1)
103107

104108

@@ -111,7 +115,8 @@ def test_load_version1(tmpdir):
111115
},
112116
)
113117
base = str(tmpdir)
114-
build = load(base, {})
118+
with override_settings(DOCROOT=tmpdir):
119+
build = load(base, {})
115120
assert isinstance(build, BuildConfigV1)
116121

117122

@@ -124,7 +129,8 @@ def test_load_version2(tmpdir):
124129
},
125130
)
126131
base = str(tmpdir)
127-
build = load(base, {})
132+
with override_settings(DOCROOT=tmpdir):
133+
build = load(base, {})
128134
assert isinstance(build, BuildConfigV2)
129135

130136

@@ -138,7 +144,8 @@ def test_load_unknow_version(tmpdir):
138144
)
139145
base = str(tmpdir)
140146
with raises(ConfigError) as excinfo:
141-
load(base, {})
147+
with override_settings(DOCROOT=tmpdir):
148+
load(base, {})
142149
assert excinfo.value.code == VERSION_INVALID
143150

144151

@@ -159,7 +166,8 @@ def test_load_raise_exception_invalid_syntax(tmpdir):
159166
)
160167
base = str(tmpdir)
161168
with raises(ConfigError) as excinfo:
162-
load(base, {})
169+
with override_settings(DOCROOT=tmpdir):
170+
load(base, {})
163171
assert excinfo.value.code == CONFIG_SYNTAX_INVALID
164172

165173

@@ -176,13 +184,15 @@ def test_yaml_extension(tmpdir):
176184
},
177185
)
178186
base = str(tmpdir)
179-
config = load(base, {})
187+
with override_settings(DOCROOT=tmpdir):
188+
config = load(base, {})
180189
assert isinstance(config, BuildConfigV1)
181190

182191

183192
def test_build_config_has_source_file(tmpdir):
184193
base = str(apply_fs(tmpdir, yaml_config_dir))
185-
build = load(base, {})
194+
with override_settings(DOCROOT=tmpdir):
195+
build = load(base, {})
186196
assert build.source_file == os.path.join(base, 'readthedocs.yml')
187197

188198

@@ -196,7 +206,8 @@ def test_build_config_has_list_with_single_empty_value(tmpdir):
196206
),
197207
},
198208
))
199-
build = load(base, {})
209+
with override_settings(DOCROOT=tmpdir):
210+
build = load(base, {})
200211
assert isinstance(build, BuildConfigV1)
201212
assert build.formats == []
202213

@@ -682,7 +693,8 @@ def test_load_calls_validate(tmpdir):
682693
apply_fs(tmpdir, yaml_config_dir)
683694
base = str(tmpdir)
684695
with patch.object(BuildConfigV1, 'validate') as build_validate:
685-
load(base, {})
696+
with override_settings(DOCROOT=tmpdir):
697+
load(base, {})
686698
assert build_validate.call_count == 1
687699

688700

‎readthedocs/core/tests/test_filesystem_utils.py

+181
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
from pathlib import Path
2+
from tempfile import mkdtemp
3+
4+
import pytest
5+
from django.core.exceptions import SuspiciousFileOperation
6+
from django.test import TestCase, override_settings
7+
8+
from readthedocs.core.utils.filesystem import safe_copytree, safe_open, safe_rmtree
9+
from readthedocs.doc_builder.exceptions import (
10+
FileIsNotRegularFile,
11+
SymlinkOutsideBasePath,
12+
UnsupportedSymlinkFileError,
13+
)
14+
15+
16+
class TestFileSystemUtils(TestCase):
17+
def assert_files_equal(self, directory, files):
18+
self.assertEqual(
19+
{str(p.relative_to(directory)) for p in directory.iterdir()}, files
20+
)
21+
22+
def test_copytree(self):
23+
from_directory = Path(mkdtemp())
24+
docroot_path = from_directory.parent
25+
to_directory = Path(mkdtemp()) / "target"
26+
27+
(from_directory / "test.txt").touch()
28+
29+
self.assertFalse(to_directory.exists())
30+
31+
with override_settings(DOCROOT=docroot_path):
32+
safe_copytree(from_directory, to_directory)
33+
34+
self.assert_files_equal(to_directory, {"test.txt"})
35+
36+
def test_copytree_outside_docroot(self):
37+
from_directory = Path(mkdtemp())
38+
(from_directory / "test.txt").touch()
39+
to_directory = Path(mkdtemp()) / "target"
40+
docroot_path = Path(mkdtemp())
41+
42+
with pytest.raises(SuspiciousFileOperation):
43+
with override_settings(DOCROOT=docroot_path):
44+
safe_copytree(from_directory, to_directory)
45+
46+
def test_copytree_with_symlinks(self):
47+
from_directory = Path(mkdtemp())
48+
docroot_path = from_directory.parent
49+
to_directory = Path(mkdtemp()) / "target"
50+
51+
file_a = from_directory / "test.txt"
52+
file_a.touch()
53+
54+
symlink_a = from_directory / "symlink.txt"
55+
symlink_a.symlink_to(file_a)
56+
symlink_b = from_directory / "symlink-dir"
57+
symlink_b.symlink_to(to_directory.parent)
58+
59+
self.assertFalse(to_directory.exists())
60+
61+
with override_settings(DOCROOT=docroot_path):
62+
safe_copytree(from_directory, to_directory)
63+
64+
# Symlinks are copied as symlinks, not as files.
65+
self.assert_files_equal(
66+
to_directory, {"test.txt", "symlink.txt", "symlink-dir"}
67+
)
68+
self.assertTrue((to_directory / "symlink.txt").is_symlink())
69+
self.assertTrue((to_directory / "symlink-dir").is_symlink())
70+
71+
def test_copytree_from_dir_as_symlink(self):
72+
root_directory = Path(mkdtemp())
73+
docroot_path = root_directory
74+
from_directory = root_directory / "a"
75+
from_directory.mkdir()
76+
(from_directory / "test.txt").touch()
77+
78+
to_directory = root_directory / "b"
79+
80+
from_directory_symlink = root_directory / "symlink-a"
81+
from_directory_symlink.symlink_to(from_directory)
82+
83+
self.assertFalse(to_directory.exists())
84+
85+
with override_settings(DOCROOT=docroot_path):
86+
self.assertFalse(safe_copytree(from_directory_symlink, to_directory))
87+
88+
self.assertFalse(to_directory.exists())
89+
90+
def test_open(self):
91+
root_directory = Path(mkdtemp())
92+
docroot_path = root_directory
93+
file_a = root_directory / "test.txt"
94+
file_a.touch()
95+
96+
with override_settings(DOCROOT=docroot_path):
97+
context_manager = safe_open(file_a, allow_symlinks=False)
98+
self.assertIsNotNone(context_manager)
99+
100+
with override_settings(DOCROOT=docroot_path):
101+
context_manager = safe_open(
102+
file_a, allow_symlinks=True, base_path=root_directory
103+
)
104+
self.assertIsNotNone(context_manager)
105+
106+
def test_open_outside_docroot(self):
107+
root_directory = Path(mkdtemp())
108+
docroot_path = Path(mkdtemp())
109+
file_a = root_directory / "test.txt"
110+
file_a.touch()
111+
112+
with pytest.raises(SuspiciousFileOperation):
113+
with override_settings(DOCROOT=docroot_path):
114+
safe_open(file_a)
115+
116+
def test_open_with_symlinks(self):
117+
root_directory = Path(mkdtemp())
118+
docroot_path = root_directory
119+
file_a = root_directory / "test.txt"
120+
file_a.touch()
121+
122+
symlink_a = root_directory / "symlink.txt"
123+
symlink_a.symlink_to(file_a)
124+
125+
# Symlinks aren't allowed.
126+
with pytest.raises(UnsupportedSymlinkFileError):
127+
with override_settings(DOCROOT=docroot_path):
128+
safe_open(symlink_a, allow_symlinks=False)
129+
130+
# Symlinks are allowed if they are under the root_directory.
131+
with override_settings(DOCROOT=docroot_path):
132+
context_manager = safe_open(
133+
symlink_a, allow_symlinks=True, base_path=root_directory
134+
)
135+
self.assertIsNotNone(context_manager)
136+
137+
# Symlinks aren't allowed if they aren't under the root_directory.
138+
with pytest.raises(SymlinkOutsideBasePath):
139+
with override_settings(DOCROOT=docroot_path):
140+
new_root_directory = root_directory / "dir"
141+
new_root_directory.mkdir()
142+
safe_open(symlink_a, allow_symlinks=True, base_path=new_root_directory)
143+
144+
def test_rmtree(self):
145+
root_directory = Path(mkdtemp())
146+
docroot_path = root_directory
147+
(root_directory / "test.txt").touch()
148+
149+
self.assertTrue(root_directory.exists())
150+
151+
with override_settings(DOCROOT=docroot_path):
152+
safe_rmtree(root_directory)
153+
self.assertFalse(root_directory.exists())
154+
155+
def test_rmtree_outside_docroot(self):
156+
root_directory = Path(mkdtemp())
157+
docroot_path = Path(mkdtemp())
158+
(root_directory / "test.txt").touch()
159+
160+
self.assertTrue(root_directory.exists())
161+
162+
with pytest.raises(SuspiciousFileOperation):
163+
with override_settings(DOCROOT=docroot_path):
164+
safe_rmtree(root_directory)
165+
166+
def test_rmtree_with_symlinks(self):
167+
root_directory = Path(mkdtemp())
168+
docroot_path = root_directory
169+
170+
dir_a = root_directory / "test"
171+
dir_a.mkdir()
172+
(dir_a / "test.txt").touch()
173+
174+
symlink_a = root_directory / "symlink"
175+
symlink_a.symlink_to(dir_a)
176+
177+
# Directories that point to a symlink aren't deleted.
178+
self.assertTrue(symlink_a.exists())
179+
with override_settings(DOCROOT=docroot_path):
180+
safe_rmtree(symlink_a)
181+
self.assertTrue(symlink_a.exists())

0 commit comments

Comments
 (0)