Filenames with spaces at the cause bag incompleteness errors. #143

Activity

1. kieranjol commented on Feb 11, 2020

   

   kieranjol

   on Feb 11, 2020 via email

   Contributor

   More actions

   This isn't a block or anything, but trailing spaces in filenames can produce terrible issues on windows (maybe ntfs in general?). Windows prevents you from doing it, but I've seen drives where the space was created in another operating system and the folder/file often not only doesn't appear to a user, the space on the disk is designated free space ready for overwriting. See cause 6 here: https://support.microsoft.com/en-ie/help/320081/you-cannot-delete-a-file-or-a-folder-on-an-ntfs-file-system-volume
2. kieranjol commented on Feb 11, 2020

   

   kieranjol

   on Feb 11, 2020 via email

   Contributor

   More actions

   Also I doubt that the line of code was concerned with this issue and it was more about cleaning up trailing whitespace.
3. acdha commented on Feb 11, 2020

   

   acdha

   on Feb 11, 2020

   Member

   More actions

   Doesn’t the RFC say one or more spaces as a separator? If I remember correctly - on phone, sorry - that would require using the URL encoded form.
4. dustincahillumn-max commented on Sep 7, 2026

   

   dustincahillumn-max

   on Sep 7, 2026

   Last edited by dustincahillumn-max

   More actions

   Hello from 2026 — this issue hadn’t been touched in six years, so sorry for the necro.
   I don’t really understand any of this, but I accidentally pointed an AI at the old thread and it decided this was the bug it wanted to “fix.” I asked another AI if the change was reasonable and got the usual enthusiastic yes in sycophantese. Looks like an edge case but it seemed dumb to burn the tokens and not at least leave the result here.

   The problem is still present: line.strip() on manifest/fetch lines removes trailing whitespace that belongs to the filename, so validation fails with incompleteness errors.
   Suggested change is just to strip the line endings instead:

   # fetch
   url, file_size, filename = line.rstrip("\r\n").split(None, 2)
   
   # manifest
   line = line.rstrip("\r\n")
   if not line.strip() or line.lstrip().startswith("#"):
       continue
   

   Click to expand full patch + regression tests

   diff --git a/pyproject.toml b/pyproject.toml
   index 937984ae628ef3f48632a4bfb94d7fddc3f04a14..5239f5f3a47b313c88442c3d2b5418d4f40cc426 100644
   --- a/pyproject.toml
   +++ b/pyproject.toml
   @@ -38,7 +38,7 @@ default_section = "THIRDPARTY"
    known_first_party = "bagit"
    
    [tool.pytest.ini_options]
   -testpaths = ["test.py"]
   +testpaths = ["test.py", "test_manifest_whitespace.py"]
    addopts = "-v"
    
    [tool.coverage.run]
   diff --git a/src/bagit/__init__.py b/src/bagit/__init__.py
   index 2b4adcebf0164d3254d01f2532578103b8df9dee..fd774b6a2005cbd5a8fd4a9d0c737eabff2d7263 100755
   --- a/src/bagit/__init__.py
   +++ b/src/bagit/__init__.py
   @@ -572,7 +572,8 @@ class Bag(object):
                    fetch_file_path, "r", encoding=self.encoding
                ) as fetch_file:
                    for line in fetch_file:
   -                    url, file_size, filename = line.strip().split(None, 2)
   +                    # Whitespace at the end of the filename is significant.
   +                    url, file_size, filename = line.rstrip("\r\n").split(None, 2)
    
                        if self._path_is_dangerous(filename):
                            raise BagError(
   @@ -675,10 +676,11 @@ class Bag(object):
                            manifest_file.seek(0)  # Pretend the first read never happened
    
                    for line in manifest_file:
   -                    line = line.strip()
   +                    # Remove line endings, not whitespace belonging to the path.
   +                    line = line.rstrip("\r\n")
    
                        # Ignore blank lines and comments.
   -                    if line == "" or line.startswith("#"):
   +                    if not line.strip() or line.lstrip().startswith("#"):
                            continue
    
                        entry = line.split(None, 1)
   diff --git a/test_manifest_whitespace.py b/test_manifest_whitespace.py
   new file mode 100644
   index 0000000000000000000000000000000000000000..5db5f70d00e524e0db07010bebefd95d06bd5a4b
   --- /dev/null
   +++ b/test_manifest_whitespace.py
   @@ -0,0 +1,157 @@
   +"""Regression coverage for filenames whose trailing whitespace is significant."""
   +
   +import hashlib
   +import os
   +import tempfile
   +import unittest
   +from pathlib import Path
   +
   +import bagit
   +
   +
   +class TestManifestFilenameWhitespace(unittest.TestCase):
   +    suffixes = (" ", "  ", "\t", "\u00a0", "\u2003", " \t\u2003")
   +    line_endings = ("\n", "\r\n", "\r", "")
   +    checksum = hashlib.sha256(b"payload").hexdigest()
   +
   +    def setUp(self):
   +        temporary_directory = tempfile.TemporaryDirectory()
   +        self.addCleanup(temporary_directory.cleanup)
   +        self.root = Path(temporary_directory.name)
   +        (self.root / "data").mkdir()
   +        self.write_text(
   +            "bagit.txt",
   +            "BagIt-Version: 1.0\nTag-File-Character-Encoding: UTF-8\n",
   +        )
   +        self.write_text("manifest-sha256.txt", "")
   +
   +    def write_text(self, name, text):
   +        # Write bytes so CR, LF and CRLF cases reach the parser unchanged.
   +        (self.root / name).write_bytes(text.encode("utf-8"))
   +
   +    def test_payload_manifest_preserves_filename_suffix(self):
   +        for suffix in self.suffixes:
   +            for ending in self.line_endings:
   +                with self.subTest(suffix=repr(suffix), ending=repr(ending)):
   +                    filename = "data/report.txt" + suffix
   +                    self.write_text(
   +                        "manifest-sha256.txt", f"{self.checksum}  {filename}{ending}"
   +                    )
   +                    bag = bagit.Bag(str(self.root))
   +                    self.assertEqual(
   +                        {os.path.normpath(filename): {"sha256": self.checksum}},
   +                        bag.entries,
   +                    )
   +
   +    def test_tag_manifest_preserves_filename_suffix(self):
   +        for suffix in self.suffixes:
   +            for ending in self.line_endings:
   +                with self.subTest(suffix=repr(suffix), ending=repr(ending)):
   +                    filename = "metadata.txt" + suffix
   +                    self.write_text(
   +                        "tagmanifest-sha256.txt", f"{self.checksum} {filename}{ending}"
   +                    )
   +                    bag = bagit.Bag(str(self.root))
   +                    self.assertEqual({filename: {"sha256": self.checksum}}, bag.entries)
   +
   +    def test_filenames_with_and_without_suffix_are_distinct(self):
   +        other_checksum = hashlib.sha256(b"different payload").hexdigest()
   +        for suffix in self.suffixes:
   +            with self.subTest(suffix=repr(suffix)):
   +                plain_name = "data/report.txt"
   +                suffixed_name = plain_name + suffix
   +                self.write_text(
   +                    "manifest-sha256.txt",
   +                    f"{self.checksum}  {plain_name}\n"
   +                    f"{other_checksum}  {suffixed_name}\n",
   +                )
   +                bag = bagit.Bag(str(self.root))
   +                self.assertEqual(
   +                    {
   +                        os.path.normpath(plain_name): {"sha256": self.checksum},
   +                        os.path.normpath(suffixed_name): {"sha256": other_checksum},
   +                    },
   +                    bag.entries,
   +                )
   +
   +    def test_fetch_preserves_filename_remainder(self):
   +        url = "https://example.org/report"
   +        for suffix in self.suffixes:
   +            for ending in self.line_endings:
   +                with self.subTest(suffix=repr(suffix), ending=repr(ending)):
   +                    filename = "data/annual report.txt" + suffix
   +                    self.write_text("fetch.txt", f"{url}\t-\t{filename}{ending}")
   +                    bag = bagit.Bag(str(self.root))
   +                    self.assertEqual([(url, "-", filename)], list(bag.fetch_entries()))
   +                    self.assertEqual([filename], list(bag.files_to_be_fetched()))
   +
   +    def test_fetch_distinguishes_filenames_with_and_without_suffix(self):
   +        url = "https://example.org/report"
   +        self.write_text(
   +            "fetch.txt",
   +            f"{url} 7 data/report.txt\n{url} 9 data/report.txt \n",
   +        )
   +        bag = bagit.Bag(str(self.root))
   +        self.assertEqual(
   +            [(url, "7", "data/report.txt"), (url, "9", "data/report.txt ")],
   +            list(bag.fetch_entries()),
   +        )
   +
   +    def test_existing_manifest_comments_and_filename_content(self):
   +        filename = "data/annual report #1.txt"
   +        for ending in self.line_endings[:-1]:
   +            with self.subTest(ending=repr(ending)):
   +                self.write_text(
   +                    "manifest-sha256.txt",
   +                    ending.join(
   +                        (
   +                            "",
   +                            " \t",
   +                            "# comment",
   +                            "  # indented comment",
   +                            f"{self.checksum}\t{filename}",
   +                            "",
   +                        )
   +                    ),
   +                )
   +                bag = bagit.Bag(str(self.root))
   +                self.assertEqual(
   +                    {os.path.normpath(filename): {"sha256": self.checksum}}, bag.entries
   +                )
   +
   +    def test_validation_preserves_real_unicode_whitespace_files(self):
   +        # Unicode whitespace is portable even on systems rejecting trailing ASCII
   +        # spaces or tabs. Different contents also expose accidental name collisions.
   +        payloads = {"data/report": b"plain", "data/report\u2003": b"with suffix"}
   +        manifest_lines = []
   +        for filename, contents in payloads.items():
   +            (self.root / filename).write_bytes(contents)
   +            checksum = hashlib.sha256(contents).hexdigest()
   +            manifest_lines.append(f"{checksum}  {filename}\n")
   +        self.write_text("manifest-sha256.txt", "".join(manifest_lines))
   +        tag_filename = "metadata\u00a0"
   +        (self.root / tag_filename).write_bytes(b"metadata")
   +        tag_checksum = hashlib.sha256(b"metadata").hexdigest()
   +        manifest_checksum = hashlib.sha256(
   +            (self.root / "manifest-sha256.txt").read_bytes()
   +        ).hexdigest()
   +        self.write_text(
   +            "tagmanifest-sha256.txt",
   +            f"{tag_checksum} {tag_filename}\n{manifest_checksum} manifest-sha256.txt\n",
   +        )
   +
   +        bag = bagit.Bag(str(self.root))
   +        self.assertTrue(bag.validate())
   +
   +        # Preserving the name must still allow detection of payload corruption.
   +        (self.root / "data/report\u2003").write_bytes(b"corrupted")
   +        with self.assertRaises(bagit.BagValidationError) as raised:
   +            bag.validate()
   +        self.assertEqual(1, len(raised.exception.details))
   +        mismatch = raised.exception.details[0]
   +        self.assertIsInstance(mismatch, bagit.ChecksumMismatch)
   +        self.assertEqual(os.path.normpath("data/report\u2003"), mismatch.path)
   +
   +
   +if __name__ == "__main__":
   +    unittest.main()