DOP-2430: Harden the getfiles() implementation to avoid crawling above snooty.toml

Author: i80and

snooty/parser.py

@@ -1406,7 +1406,9 @@ def build(
         pool = multiprocessing.Pool(max_workers)
         with util.PerformanceLogger.singleton().start("parse rst"):
             try:
-                paths = util.get_files(self.config.source_path, RST_EXTENSIONS)
+                paths = util.get_files(
+                    self.config.source_path, RST_EXTENSIONS, self.config.root
+                )
                 logger.debug("Processing rst files")
                 results = pool.imap_unordered(partial(parse_rst, self.parser), paths)
                 for sequence in results:
@@ -1421,7 +1423,9 @@ def build(
         # Categorize our YAML files
         logger.debug("Categorizing YAML files")
         categorized: Dict[str, List[Path]] = collections.defaultdict(list)
-        for path in util.get_files(self.config.source_path, (".yaml",)):
+        for path in util.get_files(
+            self.config.source_path, (".yaml",), self.config.root
+        ):
             prefix = get_giza_category(path)
             if prefix in self.yaml_mapping:
                 categorized[prefix].append(path)

snooty/test_util.py

@@ -50,16 +50,22 @@ def test_option_flag() -> None:
 
 def test_get_files() -> None:
     # The test_data/getfiles path tests how we handle symbolic links. To wit,
-    # we ensure that we don't fail on loops; files with the same inode only
-    # report once; and that we enter symlinks.
+    # we ensure that we don't fail on loops; files with the same resolved path
+    # only report once; and that we enter symlinks.
 
-    assert set(util.get_files(Path("test_data/getfiles/files1"), (".toml",))) == {
+    reference_set = {
         Path("test_data/getfiles/files1/1.toml"),
         Path("test_data/getfiles/files1/2.toml"),
-        Path("test_data/getfiles/files1/files2/subdirectory/4.toml"),
-        Path("test_data/getfiles/files1/files2/3.toml"),
+        Path("test_data/getfiles/files1/loop1/loop1.toml"),
+        Path("test_data/getfiles/files1/loop2/loop2.toml"),
     }
 
+    # Either subdirectory or dup are acceptable, but not both
+    assert set(util.get_files(Path("test_data/getfiles/files1"), (".toml",))) in [
+        reference_set.union({Path("test_data/getfiles/files1/subdirectory/5.toml")}),
+        reference_set.union({Path("test_data/getfiles/files1/dup/5.toml")}),
+    ]
+
 
 def test_add_doc_target_ext() -> None:
     # Set up target filenames

snooty/util.py

@@ -61,23 +61,59 @@ def reroot_path(
     return rel_fn, project_root.joinpath(rel_fn).resolve()
 
 
-def get_files(root: Path, extensions: Container[str]) -> Iterator[Path]:
+def is_relative_to(a: Path, b: Path) -> bool:
+    try:
+        a.relative_to(b)
+        return True
+    except ValueError:
+        return False
+
+
+def get_files(
+    root: Path, extensions: Container[str], must_be_relative_to: Optional[Path] = None
+) -> Iterator[Path]:
     """Recursively iterate over files underneath the given root, yielding
     only filenames with the given extensions. Symlinks are followed, but
-    any given concrete directory is only scanned once."""
+    any given concrete directory is only scanned once.
+
+    By default, directories above the given root in the filesystem are not
+    scanned, but this can be overridden with the must_be_relative_to parameter."""
+    root_resolved = root.resolve()
     seen: Set[Path] = set()
 
+    if must_be_relative_to is None:
+        must_be_relative_to = root_resolved
+
     for base, dirs, files in os.walk(root, followlinks=True):
-        dirs_set = set(Path(d).resolve() for d in dirs)
-        dirs[:] = [d.name for d in (dirs_set - seen)]
+        base_resolved = Path(base).resolve()
+        if not is_relative_to(base_resolved, must_be_relative_to):
+            # Prevent a race between our checking if a symlink is valid, and our
+            # actually entering it.
+            continue
+
+        # Preserve both the actual resolved path and the directory name
+        dirs_set = dict(((base_resolved.joinpath(d).resolve(), d) for d in dirs))
+
+        # Only recurse into directories which are within our prefix
+        dirs[:] = [
+            d_name
+            for d_path, d_name in ((k, v) for k, v in dirs_set.items() if k not in seen)
+            if is_relative_to(
+                base_resolved.joinpath(d_path).resolve(), must_be_relative_to
+            )
+        ]
+
         seen.update(dirs_set)
 
         for name in files:
             ext = os.path.splitext(name)[1]
             if ext not in extensions:
                 continue
 
-            yield Path(os.path.join(base, name))
+            path = Path(os.path.join(base, name))
+            # Detect and ignore symlinks outside of our jail
+            if is_relative_to(path.resolve(), must_be_relative_to):
+                yield path
 
 
 def get_line(node: docutils.nodes.Node) -> int:

test_data/getfiles/files1/3.toml

@@ -0,0 +1 @@
+../files2/3.toml

test_data/getfiles/files1/dup

@@ -0,0 +1 @@
+subdirectory

test_data/getfiles/files1/escape

@@ -0,0 +1 @@
+../../

test_data/getfiles/files1/loop1/loop1.toml

@@ -0,0 +1 @@
+../loop2

test_data/getfiles/files1/loop1/loop2

@@ -0,0 +1 @@
+../loop1