diff --git a/dms/controllers/portal.py b/dms/controllers/portal.py index e1eaa7d64..f0f7e6feb 100644 --- a/dms/controllers/portal.py +++ b/dms/controllers/portal.py @@ -184,7 +184,11 @@ def _get_files(self, access_token, dms_directory_id, search, search_in, sort_br) # items file_model = request.env["dms.file"] - is_access_token_valid = file_model.check_access_token(access_token) + # The token belongs to a directory, so it must be validated against the + # directory being browsed (or one of its ancestors), never against an + # empty dms.file recordset. Same pattern as _get_directories below. + directory_to_check = request.env["dms.directory"].browse(dms_directory_id) + is_access_token_valid = directory_to_check.check_access_token(access_token) file_model = file_model.sudo() if is_access_token_valid else file_model dms_file_items = file_model.search(file_domain, order=sort_br) request.session["my_dms_file_history"] = dms_file_items.ids diff --git a/dms/models/directory.py b/dms/models/directory.py index 103d5b17e..7fe340fa6 100644 --- a/dms/models/directory.py +++ b/dms/models/directory.py @@ -254,10 +254,15 @@ def check_access_token(self, access_token=False): return True # sudo because the user might not usually have access to the record but # now the token is valid. + # `seen` bounds the walk: _check_directory_recursion rejects + # cycles created through the ORM, but this path is reachable + # anonymously and must not hang on corrupted data. directory_item = self.sudo() - while directory_item.parent_id: + seen = set() + while directory_item.parent_id and directory_item.id not in seen: if directory_item.id == item.id: return True + seen.add(directory_item.id) directory_item = directory_item.parent_id # Fix last level if directory_item.id == item.id: diff --git a/dms/models/dms_file.py b/dms/models/dms_file.py index 49c06d89a..9c2917491 100644 --- a/dms/models/dms_file.py +++ b/dms/models/dms_file.py @@ -174,15 +174,22 @@ def check_access_token(self, access_token=False): ) if items: item = items[0] - if self.directory_id.id == item.id: - return True - directory_item = self.directory_id - while directory_item.parent_id: - if directory_item.id == self.directory_id.id: + # The token is known to belong to some directory, but it is not yet + # valid for this file: it only is when that directory is the file's + # own directory or one of its ancestors. sudo() so the walk can + # traverse ancestors the caller is not allowed to read. + # `seen` bounds the walk: _check_directory_recursion rejects cycles + # created through the ORM, but this path is reachable anonymously + # and must not hang on corrupted data. + directory_item = self.sudo().directory_id + seen = set() + while directory_item.parent_id and directory_item.id not in seen: + if directory_item.id == item.id: return True + seen.add(directory_item.id) directory_item = directory_item.parent_id # Fix last level - if directory_item.id == self.directory_id.id: + if directory_item.id == item.id: return True return False diff --git a/dms/tests/__init__.py b/dms/tests/__init__.py index 2502c8f40..55f0de35e 100644 --- a/dms/tests/__init__.py +++ b/dms/tests/__init__.py @@ -5,3 +5,4 @@ from . import test_file from . import test_benchmark from . import test_portal +from . import test_access_token diff --git a/dms/tests/test_access_token.py b/dms/tests/test_access_token.py new file mode 100644 index 000000000..47be5284a --- /dev/null +++ b/dms/tests/test_access_token.py @@ -0,0 +1,92 @@ +# Copyright 2026 Millow AB +# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl). +"""Regression tests for share-token scoping. + +``dms.file.check_access_token`` used to compare the directory walker against +itself instead of against the directory that owns the token, which made the +whole branch collapse to ``return True``. Any valid directory token therefore +granted read access to *every* file in the database, including files in +unrelated trees and in directories with broken group inheritance. +""" + +import uuid + +from .common import StorageFileBaseCase + + +class TestDmsAccessToken(StorageFileBaseCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + # Two unrelated trees under the same storage. + # shared_root / shared_child <- the token lives on shared_root + # other_root <- must stay unreachable + cls.shared_root = cls.create_directory(storage=cls.storage) + cls.shared_child = cls.create_directory(directory=cls.shared_root) + cls.other_root = cls.create_directory(storage=cls.storage) + + cls.file_in_shared_root = cls.create_file(directory=cls.shared_root) + cls.file_in_shared_child = cls.create_file(directory=cls.shared_child) + cls.file_in_other_root = cls.create_file(directory=cls.other_root) + + cls.token = uuid.uuid4().hex + cls.shared_root.access_token = cls.token + + # ------------------------------------------------------------------ + # The defect + # ------------------------------------------------------------------ + def test_token_does_not_grant_access_to_unrelated_tree(self): + """A token on shared_root must NOT unlock a file under other_root.""" + self.assertFalse( + self.file_in_other_root.check_access_token(self.token), + "Directory token leaked into an unrelated directory tree", + ) + + def test_token_does_not_grant_access_to_root_level_file(self): + """The `# Fix last level` branch self-compared too. + + A file whose directory has no parent skipped the loop entirely and + fell through to a second always-true comparison, so root-level files + leaked as well. + """ + self.assertFalse( + self.file.check_access_token(self.token), + "Directory token leaked into a root-level file of another tree", + ) + + def test_unrelated_token_value_is_rejected(self): + self.assertFalse( + self.file_in_shared_child.check_access_token(uuid.uuid4().hex), + "An unknown token value was accepted", + ) + + def test_no_token_is_rejected(self): + self.assertFalse(self.file_in_shared_child.check_access_token(False)) + + # ------------------------------------------------------------------ + # Legitimate behaviour that must keep working + # ------------------------------------------------------------------ + def test_token_grants_access_to_file_in_the_shared_directory(self): + self.assertTrue( + self.file_in_shared_root.check_access_token(self.token), + "Token did not unlock a file in its own directory", + ) + + def test_token_grants_access_to_file_in_a_descendant_directory(self): + self.assertTrue( + self.file_in_shared_child.check_access_token(self.token), + "Token did not unlock a file in a descendant directory", + ) + + def test_file_own_token_still_works(self): + own_token = uuid.uuid4().hex + self.file_in_other_root.access_token = own_token + self.assertTrue( + self.file_in_other_root.check_access_token(own_token), + "A file's own access token stopped working", + ) + + def test_directory_token_grants_access_to_descendant_directory(self): + """The directory-side implementation was already correct; pin it.""" + self.assertTrue(self.shared_child.check_access_token(self.token)) + self.assertFalse(self.other_root.check_access_token(self.token))