From d34b2004141f5e5deb886ff887e24a05e12239ff Mon Sep 17 00:00:00 2001 From: splitsec2 <35583321+splitsec2@users.noreply.github.com> Date: Tue, 20 Oct 2026 21:39:25 -0600 Subject: [PATCH] fix(activity): answer another user's item like a missing one, and skip foreign history rows (#1507) Two small fixes in `activity_routes.py`. **Another user's item answers like a missing one.** Dismissing a task or request id answered 404 when it didn't exist and 403 when it belonged to someone else, so a user could test whether an arbitrary id exists. A non-admin now gets the same 404 for both. In dismiss-many a foreign key is reported with the missing ones and the rest carry on, and the owner is still written to the log. Admins are unchanged. **A foreign row in someone's viewer state no longer gives a 500.** If a non-admin's viewer state referenced another user's download or request row, building history raised a `RuntimeError`. The row is now skipped with a warning. Not changed: history still raises when a row that viewer state points to has been deleted. That is a different case, and I didn't want to widen this. Tests: the existing forbidden test now expects the 404 and the log context, and four new ones cover a foreign download and a foreign request answering like a missing one (including dismiss-many), an admin still dismissing a foreign row, and history skipping foreign rows. Ruff, basedpyright and the backend suite pass. --- shelfmark/core/activity_routes.py | 118 +++++++++-------- tests/core/test_activity_routes_api.py | 167 +++++++++++++++++++++++-- 2 files changed, 212 insertions(+), 73 deletions(-) diff --git a/shelfmark/core/activity_routes.py b/shelfmark/core/activity_routes.py index de63457a..8746237a 100644 --- a/shelfmark/core/activity_routes.py +++ b/shelfmark/core/activity_routes.py @@ -291,14 +291,17 @@ def _activity_ws_room(actor: _ActorContext) -> str: return "admins" -def _check_item_ownership(actor: _ActorContext, row: dict[str, Any]) -> str | None: - """Return an error string if the actor doesn't own the item, else None.""" +def _actor_owns_item(actor: _ActorContext, row: dict[str, Any]) -> bool: + """Admins own every row; a user only the rows recorded under their id. + + A row the actor does not own is answered exactly like a row that does not + exist. Answering it differently (403 against 404) would let any user find + out which ids exist, one guess at a time. + """ if actor.is_admin: - return None + return True owner_user_id = normalize_positive_int(row.get("user_id")) - if owner_user_id != actor.db_user_id: - return "Forbidden" - return None + return owner_user_id == actor.db_user_id def _check_terminal_download(row: dict[str, Any]) -> str | None: @@ -737,7 +740,10 @@ def register_activity_routes( ) existing = download_history_service.get_by_task_id(task_id) - if existing is None: + if existing is None or not _actor_owns_item(actor, existing): + download_log_context: dict[str, Any] = ( + _download_row_log_context(existing) if existing is not None else {} + ) return _activity_error_response( "dismiss", status_code=404, @@ -746,6 +752,7 @@ def register_activity_routes( viewer_scope=actor.viewer_scope, item_type="download", item_key=f"download:{task_id}", + **download_log_context, ) live_queue_index = _build_queue_index(queue_status(user_id=actor.owner_scope)) @@ -753,18 +760,6 @@ def register_activity_routes( existing, has_live_queue_entry=task_id in live_queue_index, ) - ownership_error = _check_item_ownership(actor, existing) - if ownership_error is not None: - return _activity_error_response( - "dismiss", - status_code=403, - error=ownership_error, - auth_mode=resolve_auth_mode(), - viewer_scope=actor.viewer_scope, - item_type="download", - item_key=f"download:{task_id}", - **_download_row_log_context(effective_existing), - ) terminal_error = _check_terminal_download(effective_existing) if terminal_error is not None: return _activity_error_response( @@ -802,7 +797,12 @@ def register_activity_routes( ) request_row = user_db.get_request(request_id) - if request_row is None: + if request_row is None or not _actor_owns_item(actor, request_row): + request_log_context: dict[str, Any] = ( + _request_row_log_context(request_row) + if request_row is not None + else {"request_id": request_id} + ) return _activity_error_response( "dismiss", status_code=404, @@ -811,21 +811,9 @@ def register_activity_routes( viewer_scope=actor.viewer_scope, item_type="request", item_key=f"request:{request_id}", - request_id=request_id, + **request_log_context, ) - ownership_error = _check_item_ownership(actor, request_row) - if ownership_error is not None: - return _activity_error_response( - "dismiss", - status_code=403, - error=ownership_error, - auth_mode=resolve_auth_mode(), - viewer_scope=actor.viewer_scope, - item_type="request", - item_key=f"request:{request_id}", - **_request_row_log_context(request_row), - ) terminal_error = _check_terminal_request(request_row) if terminal_error is not None: return _activity_error_response( @@ -952,25 +940,27 @@ def register_activity_routes( if existing is None: missing_item_keys.append(f"download:{viewer_task_id}") continue + if not _actor_owns_item(actor, existing): + # Reported with the missing ids below; only the log tells them apart. + _log_activity_rejection( + "dismiss_many", + status_code=404, + reason="Download not found", + auth_mode=resolve_auth_mode(), + viewer_scope=actor.viewer_scope, + item_type="download", + item_key=f"download:{task_id}", + item_count=len(items), + **_download_row_log_context(existing), + ) + missing_item_keys.append(f"download:{viewer_task_id}") + continue if live_queue_index is None: live_queue_index = _build_queue_index(queue_status(user_id=actor.owner_scope)) effective_existing = _effective_download_row_for_activity( existing, has_live_queue_entry=task_id in live_queue_index, ) - ownership_error = _check_item_ownership(actor, existing) - if ownership_error is not None: - return _activity_error_response( - "dismiss_many", - status_code=403, - error=ownership_error, - auth_mode=resolve_auth_mode(), - viewer_scope=actor.viewer_scope, - item_type="download", - item_key=f"download:{task_id}", - item_count=len(items), - **_download_row_log_context(effective_existing), - ) terminal_error = _check_terminal_download(effective_existing) if terminal_error is not None: return _activity_error_response( @@ -1004,12 +994,11 @@ def register_activity_routes( if request_row is None: missing_item_keys.append(f"request:{request_id}") continue - ownership_error = _check_item_ownership(actor, request_row) - if ownership_error is not None: - return _activity_error_response( + if not _actor_owns_item(actor, request_row): + _log_activity_rejection( "dismiss_many", - status_code=403, - error=ownership_error, + status_code=404, + reason="Request not found", auth_mode=resolve_auth_mode(), viewer_scope=actor.viewer_scope, item_type="request", @@ -1017,6 +1006,8 @@ def register_activity_routes( item_count=len(items), **_request_row_log_context(request_row), ) + missing_item_keys.append(f"request:{request_id}") + continue terminal_error = _check_terminal_request(request_row) if terminal_error is not None: return _activity_error_response( @@ -1135,11 +1126,14 @@ def register_activity_routes( msg = f"Download history row not found for {item_key}" raise RuntimeError(msg) - if not actor.is_admin: - owner_user_id = normalize_positive_int(download_row.get("user_id")) - if owner_user_id != actor.db_user_id: - msg = f"Viewer state out of scope for {item_key}" - raise RuntimeError(msg) + if not _actor_owns_item(actor, download_row): + # Stale or foreign viewer state must not take the whole page down. + logger.warning( + "Activity history skipped %s: out of scope for viewer %s", + item_key, + actor.viewer_scope, + ) + continue effective_download_row = _effective_download_row_for_activity( download_row, @@ -1164,11 +1158,13 @@ def register_activity_routes( msg = f"Request row not found for {item_key}" raise RuntimeError(msg) - if not actor.is_admin: - owner_user_id = normalize_positive_int(request_row.get("user_id")) - if owner_user_id != actor.db_user_id: - msg = f"Viewer state out of scope for {item_key}" - raise RuntimeError(msg) + if not _actor_owns_item(actor, request_row): + logger.warning( + "Activity history skipped %s: out of scope for viewer %s", + item_key, + actor.viewer_scope, + ) + continue populate_request_usernames([request_row], user_db) entry = _request_history_entry( diff --git a/tests/core/test_activity_routes_api.py b/tests/core/test_activity_routes_api.py index 7422a3f0..ee7edc22 100644 --- a/tests/core/test_activity_routes_api.py +++ b/tests/core/test_activity_routes_api.py @@ -1030,18 +1030,161 @@ class TestActivityRoutes: }, ) - assert response.status_code == 403 - assert response.json["error"] == "Forbidden" - mock_warning.assert_called_once() - log_message = mock_warning.call_args.args[0] - assert "Activity dismiss_many rejected" in log_message - assert "status=403" in log_message - assert "reason=Forbidden" in log_message - assert "auth_mode=builtin" in log_message - assert f"viewer_scope=user:{intruder['id']}" in log_message - assert f"owner_user_id={owner['id']}" in log_message - assert "final_status=complete" in log_message - assert "request_id=321" in log_message + # Another user's row is answered exactly like a missing one, so the id cannot + # be probed. The log still carries the owner context for the operator. + assert response.status_code == 404 + assert response.json == { + "error": "One or more activity items were not found", + "missing_item_keys": ["download:forbidden-download-task"], + } + log_messages = [call.args[0] for call in mock_warning.call_args_list] + assert any( + "Activity dismiss_many rejected" in message + and "status=404" in message + and "auth_mode=builtin" in message + and f"viewer_scope=user:{intruder['id']}" in message + and f"owner_user_id={owner['id']}" in message + and "final_status=complete" in message + and "request_id=321" in message + for message in log_messages + ) + + def test_dismiss_answers_another_users_download_like_a_missing_one(self, main_module, client): + owner = _create_user(main_module, prefix="owner") + intruder = _create_user(main_module, prefix="intruder") + _set_session( + client, user_id=intruder["username"], db_user_id=intruder["id"], is_admin=False + ) + _record_terminal_download( + main_module, + task_id="foreign-dismiss-task", + user_id=owner["id"], + username=owner["username"], + ) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + foreign = client.post( + "/api/activity/dismiss", + json={"item_type": "download", "item_key": "download:foreign-dismiss-task"}, + ) + missing = client.post( + "/api/activity/dismiss", + json={"item_type": "download", "item_key": "download:no-such-task"}, + ) + + assert foreign.status_code == missing.status_code == 404 + assert foreign.json == missing.json == {"error": "Download not found"} + assert "download:foreign-dismiss-task" not in _hidden_item_keys( + main_module, viewer_scope=f"user:{intruder['id']}" + ) + + def test_dismiss_answers_another_users_request_like_a_missing_one(self, main_module, client): + owner = _create_user(main_module, prefix="owner") + intruder = _create_user(main_module, prefix="intruder") + _set_session( + client, user_id=intruder["username"], db_user_id=intruder["id"], is_admin=False + ) + request_row = main_module.user_db.create_request( + user_id=owner["id"], + content_type="ebook", + request_level="book", + policy_mode="request_book", + book_data={"title": "Owner's Request", "provider": "openlibrary", "provider_id": "x"}, + status="fulfilled", + delivery_state="none", + ) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + foreign = client.post( + "/api/activity/dismiss", + json={"item_type": "request", "item_key": f"request:{request_row['id']}"}, + ) + missing = client.post( + "/api/activity/dismiss", + json={"item_type": "request", "item_key": "request:987654321"}, + ) + many = client.post( + "/api/activity/dismiss-many", + json={ + "items": [ + {"item_type": "request", "item_key": f"request:{request_row['id']}"}, + {"item_type": "request", "item_key": "request:987654321"}, + ] + }, + ) + + assert foreign.status_code == missing.status_code == 404 + assert foreign.json == missing.json == {"error": "Request not found"} + assert many.status_code == 404 + assert many.json["missing_item_keys"] == [ + f"request:{request_row['id']}", + "request:987654321", + ] + + def test_admin_still_dismisses_another_users_download(self, main_module, client): + owner = _create_user(main_module, prefix="owner") + admin = _create_user(main_module, prefix="admin", role="admin") + _set_session(client, user_id=admin["username"], db_user_id=admin["id"], is_admin=True) + _record_terminal_download( + main_module, + task_id="admin-foreign-dismiss-task", + user_id=owner["id"], + username=owner["username"], + ) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + response = client.post( + "/api/activity/dismiss", + json={"item_type": "download", "item_key": "download:admin-foreign-dismiss-task"}, + ) + + assert response.status_code == 200 + assert "download:admin-foreign-dismiss-task" in _hidden_item_keys( + main_module, viewer_scope="admin:shared" + ) + + def test_history_skips_rows_the_viewer_does_not_own(self, main_module, client): + owner = _create_user(main_module, prefix="owner") + viewer = _create_user(main_module, prefix="viewer") + _set_session(client, user_id=viewer["username"], db_user_id=viewer["id"], is_admin=False) + _record_terminal_download( + main_module, + task_id="history-own-task", + user_id=viewer["id"], + username=viewer["username"], + ) + _record_terminal_download( + main_module, + task_id="history-foreign-task", + user_id=owner["id"], + username=owner["username"], + ) + foreign_request = main_module.user_db.create_request( + user_id=owner["id"], + content_type="ebook", + request_level="book", + policy_mode="request_book", + book_data={"title": "Owner's Request", "provider": "openlibrary", "provider_id": "y"}, + status="fulfilled", + delivery_state="none", + ) + # Viewer state that references rows this viewer does not own. + for item_type, item_key in ( + ("download", "download:history-foreign-task"), + ("request", f"request:{foreign_request['id']}"), + ("download", "download:history-own-task"), + ): + main_module.activity_view_state_service.dismiss( + viewer_scope=f"user:{viewer['id']}", + item_type=item_type, + item_key=item_key, + ) + + with patch.object(main_module, "get_auth_mode", return_value="builtin"): + response = client.get("/api/activity/history?limit=10&offset=0") + + assert response.status_code == 200 + assert [row["item_key"] for row in response.json] == ["download:history-own-task"] def test_snapshot_backfills_undismissed_terminal_download_from_download_history( self, main_module, client