Skip to content

[FIX] dms: scope directory share tokens to the token's own subtree - #499

Open
Hotdgo9 wants to merge 2 commits into
OCA:17.0from
Hotdgo9:17.0-fix-dms-share-token
Open

[FIX] dms: scope directory share tokens to the token's own subtree#499
Hotdgo9 wants to merge 2 commits into
OCA:17.0from
Hotdgo9:17.0-fix-dms-share-token

Conversation

@Hotdgo9

@Hotdgo9 Hotdgo9 commented Jul 27, 2026

Copy link
Copy Markdown

check_access_token in dms/models/dms_file.py walks up the directory tree to decide whether the token's directory is the file's own directory or one of its ancestors — but both comparisons test the walker against itself rather than against the token's directory (item). On 17.0, in check_access_token:

directory_item = self.directory_id
while directory_item.parent_id:
    if directory_item.id == self.directory_id.id:   # true on the first iteration
        return True
    directory_item = directory_item.parent_id
# Fix last level
if directory_item.id == self.directory_id.id:       # also always true
    return True

On the first iteration directory_item is self.directory_id, so it returns immediately; and if the directory has no parent the loop body never runs, at which point the "fix last level" line makes the same always-true comparison. The whole if items: block reduces to return True — the token's own directory is never consulted. Stated once: the effect is that any valid directory share token authorises read of every dms.file in the database, irrespective of directory or group inheritance.

dms.directory.check_access_token (models/directory.py L241) does the same walk correctly, comparing against item.id at L251/L257/L261 — the intended logic is already in the tree next door.

The portal hunk is mandatory, not optional

_get_files (controllers/portal.py L162) calls check_access_token on an empty dms.file recordset at L187, where self.directory_id.id is False. The loop is skipped and the always-true False == False on the last-level line is currently what makes shared-folder listings work at all.

Fix the model alone and every shared folder renders empty: HTTP 200, no traceback, nothing in the log. So the two hunks have to land together. The token belongs to a directory, so it is validated against the directory being browsed — exactly what _get_directories (L193) already does. _get_files returns early when dms_directory_id is falsy, so the browse() is always on a real id.

Bounded walk

The ancestor walk is unbounded in both files. Before this fix the file-side loop always returned on iteration one, so it never actually traversed; once it does, a parent_id cycle would spin indefinitely on a request that needs no authentication. _check_directory_recursion rejects cycles created through the ORM, so realistically only corrupted data reaches it, but the seen set costs nothing.

Tests

8 regression tests in dms/tests/test_access_token.py — unrelated trees, root-level files, descendant directories, a file's own token, and the shared-folder listing the portal hunk protects. Red before, green after.

Scope

This block is byte-identical on 12.0 through 18.0 and dates to the 13.0 migration, so the released branches carry it too. Landing it in #475 means 19.0 never ships it. This PR is the 17.0 backport. The same change is going to 17.0, and to the 19.0 migration in #475. 16.0 and older carry the same model defect but need a hand-port, since _get_files/_get_directories do not exist there — happy to send those on request rather than opening PRs unasked.

Disclosure

Being straightforward: the root cause and impact are already described in the commit body on our public fork. We found this auditing our own production instance and fixed it there before working out how to raise it upstream, and OCA/dms has private vulnerability reporting disabled — so by the time we got here there was no quiet route left. Flagging it rather than leaving you to find it. Our own instance had never created a share token, so we were never exposed.

Assisted-by: Claude Opus 5

Hotdgo9 added 2 commits July 27, 2026 18:16
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants