Skip to content

Permission checks in the collections router were inert, disclosing private members and the user roster #822

Description

@bencap

Summary

Every permission check in routers/collections.py was inert. PermissionResponse defined no __bool__, so an instance was always truthy, and this router was the only module that tested the response object directly instead of reading .permitted.

Fixed in #821.

Root cause

>>> denied = PermissionResponse(False, 404, "not found")
>>> denied.permitted
False
>>> bool(denied)
True

Consequently:

[assoc for assoc in item.score_set_associations if has_permission(...)]   # keeps everything
if not has_permission(user_data, item, Action.ADD_ROLE):                  # never taken

Every other module in the codebase reads .permitted. collections.py did not, at all 24 sites.

Impact

Collection user roster disclosed to non-admins — the more serious half

The narrowing branch at collections.py:152 never executed, so the full roster was returned to anyone who could read the collection, including anonymous callers on a public collection:

"viewers": [{"orcidId": "1234-5678-8765-4321", "firstName": "Extra", "lastName": "User"}]

Names and ORCID iDs of every editor and viewer. No navigation required — the disclosure is complete in a single GET. For private-but-shared collections this reveals collaboration structure.

The intended rule was implemented correctly in list_my_collections, which gates on the caller's own collection role. Only fetch_collection and the write endpoints were affected.

Private member URNs disclosed — bounded

The 18 association filters kept every member, so a readable collection listed the URNs of private score sets and experiments:

"scoreSetUrns": ["tmp:398c098d-24ae-483d-bc9e-7f7f8da970ed"]

Bounded in severity: the collection view model emits URNs only, and GET /score-sets/{urn} still returns 404 for an unentitled caller because assert_permission reads .permitted correctly. So this is existence-and-membership disclosure that dead-ends — an unentitled caller learns a private score set exists and belongs to this collection, and that it is unpublished (tmp: URN), but cannot retrieve it.

Access to private collections was never affected.

Affected endpoints

fetch_collection, update_collection, add_score_set_to_collection, delete_score_set_from_collection, add_experiment_to_collection, delete_experiment_from_collection, add_user_to_collection, remove_user_from_collection, and the association filters in list_my_collections.

Secondary risk introduced by fixing it

Both association relationships carry cascade="all, delete-orphan", and the filters are spelled as assignments back to the mapped collection. While the filters were inert nothing was ever dropped, so no orphans were staged. Once they work, real orphan deletes are staged on every affected request — unpersisted only because get_db never commits, and in 8 sites only because the narrowing happens after the handler's db.commit().

That invariant held by accident and had no test. #821 adds test_narrowing_associations_does_not_delete_them to pin it. #812 and #818 remove the idiom structurally.

Why this matters beyond the fix

This represents additional evidence for #808.

Metadata

Metadata

Assignees

Labels

app: backendTask implementation touches the backend

Type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions