ngclient: Don't use target path as local path

Doing so is not always safe and has various other issues
(like target paths "a/../b" and "b" ending up as the same
local path).

Instead URL-encode the target path to make it a plain filename. This
removes any opportunity for path trickery and removes the need to create
the required sub directories (which we were not doing currently, leading
to failed downloads). URL-encoding encodes much more than we really need
but doing so should not hurt: the important thing is that it encodes
all path separators.

Return the actual filepath as return value. I would like to modify the
arguments so caller could decide the filename if they want to. But I
won't do it now because updated_targets() (the caching mechanism)
relies on filenames being chosen by TUF. The plan is to make it
possible for caller to choose the filename though.

This is clearly a "filesystem API break" for anyone depending on the
actual target file names, and does not make sense if we do not plan to
go forward with other updated_targets()/download_target() changes
listed in theupdateframework#1580.

This is part of bigger plan in theupdateframework#1580
Fixes theupdateframework#1571

Signed-off-by: Jussi Kukkonen <[email protected]>

tests/test_updater_with_simulator.py

@@ -80,32 +80,37 @@ def test_refresh(self):
         self._run_refresh()
 
     def test_targets(self):
-        # target does not exist yet
-        updater = self._run_refresh()
-        self.assertIsNone(updater.get_one_valid_targetinfo("file"))
+        targets = {
+            "targetpath": b"content",
+            "åäö": b"more content"
+        }
 
+        # Add targets to repository
         self.sim.targets.version += 1
-        self.sim.add_target("targets", b"content", "file")
+        for targetpath, content in targets.items():
+            self.sim.add_target("targets", content, targetpath)
         self.sim.update_snapshot()
 
-        # target now exists, is not in cache yet
         updater = self._run_refresh()
-        file_info = updater.get_one_valid_targetinfo("file")
-        self.assertIsNotNone(file_info)
-        self.assertEqual(
-            updater.updated_targets([file_info], self.targets_dir), [file_info]
-        )
-
-        # download target, assert it is in cache and content is correct
-        updater.download_target(file_info, self.targets_dir)
-        self.assertEqual(
-            updater.updated_targets([file_info], self.targets_dir), []
-        )
-        with open(os.path.join(self.targets_dir, "file"), "rb") as f:
-            self.assertEqual(f.read(), b"content")
-
-        # TODO: run the same download tests for
-        #     self.sim.add_target("targets", b"more content", "dir/file2")
+        for targetpath, content in targets.items():
+            # target now exists, is not in cache yet
+            file_info = updater.get_one_valid_targetinfo(targetpath)
+            self.assertIsNotNone(file_info)
+            self.assertEqual(
+                updater.updated_targets([file_info], self.targets_dir),
+                [file_info]
+            )
+
+            # download target, assert it is in cache and content is correct
+            local_path = updater.download_target(file_info, self.targets_dir)
+            self.assertEqual(
+                updater.updated_targets([file_info], self.targets_dir), []
+            )
+            self.assertTrue(local_path.startswith(self.targets_dir))
+            with open(local_path, "rb") as f:
+                self.assertEqual(f.read(), content)
+
+        # TODO: run the same download tests for target paths like "dir/file2")
         # This currently fails because issue #1576
 
     def test_keys_and_signatures(self):

tuf/ngclient/updater.py

@@ -188,31 +188,27 @@ def updated_targets(
         returned in a list. The list items can be downloaded with
         'download_target()'.
         """
-        # Keep track of the target objects and filepaths of updated targets.
-        # Return 'updated_targets' and use 'updated_targetpaths' to avoid
-        # duplicates.
-        updated_targets = []
-        updated_targetpaths = []
+        # Keep track of TargetFiles and local paths. Return 'updated_targets'
+        # and use 'local_paths' to avoid duplicates.
+        updated_targets: List[TargetFile] = []
+        local_paths: List[str] = []
 
         for target in targets:
-            # Prepend 'destination_directory' to the target's relative filepath
-            # (as stored in metadata.)  Verify the hash of 'target_filepath'
-            # against each hash listed for its fileinfo.  Note: join() discards
-            # 'destination_directory' if 'filepath' contains a leading path
-            # separator (i.e., is treated as an absolute path).
-            target_filepath = os.path.join(destination_directory, target.path)
-
-            if target_filepath in updated_targetpaths:
+            # URL encode to get local filename like download_target() does
+            filename = parse.quote(target.path, "")
+            local_path = os.path.join(destination_directory, filename)
+
+            if local_path in local_paths:
                 continue
 
             try:
-                with open(target_filepath, "rb") as target_file:
+                with open(local_path, "rb") as target_file:
                     target.verify_length_and_hashes(target_file)
             # If the file does not exist locally or length and hashes
             # do not match, append to updated targets.
             except (OSError, exceptions.LengthOrHashMismatchError):
                 updated_targets.append(target)
-                updated_targetpaths.append(target_filepath)
+                local_paths.append(local_path)
 
         return updated_targets
 
@@ -221,7 +217,7 @@ def download_target(
         targetinfo: TargetFile,
         destination_directory: str,
         target_base_url: Optional[str] = None,
-    ) -> None:
+    ) -> str:
         """Downloads the target file specified by 'targetinfo'.
 
         Args:
@@ -236,6 +232,9 @@ def download_target(
         Raises:
             TODO: download-related errors
             TODO: file write errors
+
+        Returns:
+            Path to downloaded file
         """
 
         if target_base_url is None:
@@ -266,12 +265,13 @@ def download_target(
                     f"{target_filepath} length or hashes do not match"
                 ) from e
 
-            # Store the target file name without the HASH prefix.
-            local_filepath = os.path.join(
-                destination_directory, targetinfo.path
-            )
+            # Use a URL encoded targetpath as the local filename
+            filename = parse.quote(targetinfo.path, "")
+            local_filepath = os.path.join(destination_directory, filename)
             sslib_util.persist_temp_file(target_file, local_filepath)
 
+            return local_filepath
+
     def _download_metadata(
         self, rolename: str, length: int, version: Optional[int] = None
     ) -> bytes: