From 9c1d70e4fb0eb8e7af41be33ee58e4b147d7bbac Mon Sep 17 00:00:00 2001 From: sajjad Date: Mon, 27 Jul 2026 14:25:49 +0200 Subject: [PATCH 1/2] [FIX] dms: scope directory share tokens to the token's own subtree dms.file.check_access_token compared the directory walker against self.directory_id instead of against the directory owning the token, so both the loop test and the "fix last level" test were self-comparisons that are always true. The branch collapsed to `return True` whenever any directory carried the supplied token, granting read access to every dms.file in the database regardless of directory or group inheritance. Compare against item.id, matching the correct implementation in dms.directory.check_access_token, and sudo the walk so ancestors the caller cannot read are still traversable. _get_files in the portal controller relied on the always-true behaviour: it called check_access_token on an empty dms.file recordset, where False == False happened to authorise the shared listing. Validate the token against the directory being browsed instead, mirroring the pattern already used by _get_directories. Adds regression tests covering unrelated trees, root-level files, descendant directories and a file's own token. Assisted-by: Claude Opus 5 --- dms/controllers/portal.py | 6 ++- dms/models/dms_file.py | 10 ++-- dms/tests/__init__.py | 1 + dms/tests/test_access_token.py | 92 ++++++++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 6 deletions(-) create mode 100644 dms/tests/test_access_token.py 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/dms_file.py b/dms/models/dms_file.py index 49c06d89a..ff1c5a506 100644 --- a/dms/models/dms_file.py +++ b/dms/models/dms_file.py @@ -174,15 +174,15 @@ 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 + # sudo because the user might not usually have access to the record but + # now the token is valid. + directory_item = self.sudo().directory_id while directory_item.parent_id: - if directory_item.id == self.directory_id.id: + if directory_item.id == item.id: return True 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)) From 1e0eb791be4cb62f45be9a317ce3729959250e6e Mon Sep 17 00:00:00 2001 From: sajjad Date: Mon, 27 Jul 2026 14:40:04 +0200 Subject: [PATCH 2/2] [FIX] dms: bound the share-token ancestor walk The file-side walk previously returned on its first iteration because of the self-comparison bug, so it never actually traversed. Now that it does, a parent cycle would spin forever in a request reachable without authentication. _check_directory_recursion rejects cycles created through the ORM, so this only bites on corrupted data or direct SQL, but an unbounded loop on an anonymous endpoint is not worth leaving open. Track visited ids in both dms.file and dms.directory rather than imposing an arbitrary depth limit, so legitimate deep trees are unaffected. Assisted-by: Claude Opus 5 --- dms/models/directory.py | 7 ++++++- dms/models/dms_file.py | 13 ++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) 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 ff1c5a506..9c2917491 100644 --- a/dms/models/dms_file.py +++ b/dms/models/dms_file.py @@ -174,12 +174,19 @@ def check_access_token(self, access_token=False): ) if items: item = items[0] - # sudo because the user might not usually have access to the record but - # now the token is valid. + # 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 - 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: