From 65477519366adc9dff1368aa8dc368889703d659 Mon Sep 17 00:00:00 2001 From: jony376 Date: Thu, 7 May 2026 22:49:23 -0700 Subject: [PATCH] Fix: missing authorization checks in `/files/link-to-datasets` (#14649) ### Related issues Closes #14648 ### What problem does this PR solve? This PR fixes an authorization flaw in `POST /files/link-to-datasets`. Before this change, the endpoint only checked whether the supplied `file_ids` and `kb_ids` existed. It did not verify whether the authenticated user was actually allowed to access those files or target datasets. As a result, an authenticated user who knew valid IDs could relink another user's files to arbitrary datasets. This was especially risky because the relinking flow is state-changing: the background worker removes existing file-document mappings and then recreates documents under the attacker-supplied dataset IDs. This change makes the route enforce the same permission model already used by nearby file and document operations: - each resolved file must pass `check_file_team_permission(...)` - each target dataset must pass `check_kb_team_permission(...)` - authorization is enforced before scheduling background relinking work ### Type of change - [x] Bug Fix (non-breaking change which fixes an issue) - [ ] New Feature (non-breaking change which adds functionality) - [ ] Documentation Update - [ ] Refactoring - [ ] Performance Improvement - [ ] Other (please describe): ### Testing - Added regression coverage in `test/testcases/test_web_api/test_file_app/test_file2document_routes_unit.py` - Covered: - unauthorized file access is rejected - unauthorized dataset access is rejected - existing success path still returns immediately after scheduling background work - Attempted to run: - `python -m pytest test\\testcases\\test_web_api\\test_file_app\\test_file2document_routes_unit.py -q` - Local execution in this workspace is currently blocked by missing test dependencies during bootstrap, including `ragflow_sdk` --------- Co-authored-by: jony376 --- api/apps/restful_apis/file2document_api.py | 59 ++++++++++++++++++- .../test_file2document_routes_unit.py | 45 ++++++++------ 2 files changed, 86 insertions(+), 18 deletions(-) diff --git a/api/apps/restful_apis/file2document_api.py b/api/apps/restful_apis/file2document_api.py index e599eb04ad..9c466a441d 100644 --- a/api/apps/restful_apis/file2document_api.py +++ b/api/apps/restful_apis/file2document_api.py @@ -18,6 +18,7 @@ import asyncio import logging from pathlib import Path +from api.common.check_team_permission import check_file_team_permission, check_kb_team_permission from api.db.services.file2document_service import File2DocumentService from api.db.services.file_service import FileService @@ -28,6 +29,8 @@ from common.misc_utils import get_uuid from api.db import FileType from api.db.services.document_service import DocumentService +logger = logging.getLogger(__name__) + def _convert_files(file_ids, kb_ids, user_id): """Synchronous worker: delete old docs and insert new ones for the given file/kb pairs.""" @@ -88,13 +91,29 @@ async def convert(): # Validate all files exist before starting any work for file_id in file_ids: if not files_set.get(file_id): + logger.warning( + "user_id=%s resource_type=file resource_id=%s action=validate_file_lookup result=not_found file_ids=%s kb_ids=%s", + current_user.id, + file_id, + file_ids, + kb_ids, + ) return get_data_error_result(message="File not found!") # Validate all kb_ids exist before scheduling background work + kb_map = {} for kb_id in kb_ids: - e, _ = KnowledgebaseService.get_by_id(kb_id) + e, kb = KnowledgebaseService.get_by_id(kb_id) if not e: + logger.warning( + "user_id=%s resource_type=dataset resource_id=%s action=validate_dataset_lookup result=not_found file_ids=%s kb_ids=%s", + current_user.id, + kb_id, + file_ids, + kb_ids, + ) return get_data_error_result(message="Can't find this dataset!") + kb_map[kb_id] = kb # Expand folders to their innermost file IDs all_file_ids = [] @@ -106,6 +125,38 @@ async def convert(): all_file_ids.append(file_id) user_id = current_user.id + for file_id in all_file_ids: + e, file = FileService.get_by_id(file_id) + if not e or not file: + logger.warning( + "user_id=%s resource_type=file resource_id=%s action=validate_expanded_file_lookup result=not_found file_ids=%s kb_ids=%s", + user_id, + file_id, + file_ids, + kb_ids, + ) + return get_data_error_result(message="File not found!") + if not check_file_team_permission(file, user_id): + logger.warning( + "user_id=%s resource_type=file resource_id=%s action=authorize_file result=denied file_ids=%s kb_ids=%s", + user_id, + file_id, + file_ids, + kb_ids, + ) + return get_data_error_result(message="No authorization.") + + for kb_id, kb in kb_map.items(): + if not check_kb_team_permission(kb, user_id): + logger.warning( + "user_id=%s resource_type=dataset resource_id=%s action=authorize_dataset result=denied file_ids=%s kb_ids=%s", + user_id, + kb_id, + file_ids, + kb_ids, + ) + return get_data_error_result(message="No authorization.") + # Run the blocking DB work in a thread so the event loop is not blocked. # For large folders this prevents 504 Gateway Timeout by returning as # soon as the background task is scheduled. @@ -114,6 +165,12 @@ async def convert(): future.add_done_callback( lambda f: logging.error("_convert_files failed: %s", f.exception()) if f.exception() else None ) + logger.info( + "user_id=%s resource_type=file_to_dataset_link resource_id=batch action=schedule_convert result=scheduled file_ids=%s kb_ids=%s", + user_id, + all_file_ids, + kb_ids, + ) return get_json_result(data=True) except Exception as e: return server_error_response(e) diff --git a/test/testcases/test_web_api/test_file_app/test_file2document_routes_unit.py b/test/testcases/test_web_api/test_file_app/test_file2document_routes_unit.py index cd9de79260..e4850e3643 100644 --- a/test/testcases/test_web_api/test_file_app/test_file2document_routes_unit.py +++ b/test/testcases/test_web_api/test_file_app/test_file2document_routes_unit.py @@ -34,17 +34,6 @@ class _DummyManager: return decorator -class _AwaitableValue: - def __init__(self, value): - self._value = value - - def __await__(self): - async def _co(): - return self._value - - return _co().__await__() - - class _DummyFile: def __init__(self, file_id, file_type, *, name="file.txt", location="loc", size=1): self.id = file_id @@ -109,6 +98,16 @@ def _load_file2document_module(monkeypatch): services_pkg.__path__ = [] monkeypatch.setitem(sys.modules, "api.db.services", services_pkg) + common_pkg = ModuleType("api.common") + common_pkg.__path__ = [] + monkeypatch.setitem(sys.modules, "api.common", common_pkg) + + permission_mod = ModuleType("api.common.check_team_permission") + permission_mod.check_file_team_permission = lambda *_args, **_kwargs: True + permission_mod.check_kb_team_permission = lambda *_args, **_kwargs: True + monkeypatch.setitem(sys.modules, "api.common.check_team_permission", permission_mod) + common_pkg.check_team_permission = permission_mod + file2document_mod = ModuleType("api.db.services.file2document_service") class _StubFile2DocumentService: @@ -244,25 +243,37 @@ def test_convert_branch_matrix_unit(monkeypatch): req_state = {"kb_ids": ["kb-1"], "file_ids": ["f1"]} _set_request_json(monkeypatch, module, req_state) - # Falsy file → "File not found!" (synchronous validation) + # Falsy file returns "File not found!" during synchronous validation. monkeypatch.setattr(module.FileService, "get_by_ids", lambda _ids: [_FalsyFile("f1", module.FileType.DOC.value)]) res = _run(module.convert()) assert res["message"] == "File not found!" - # Valid file but invalid kb → "Can't find this dataset!" (synchronous validation) - # KnowledgebaseService stub returns (False, None) by default + # Valid file but invalid kb returns "Can't find this dataset!" during synchronous validation. monkeypatch.setattr(module.FileService, "get_by_ids", lambda _ids: [_DummyFile("f1", module.FileType.DOC.value)]) res = _run(module.convert()) assert res["message"] == "Can't find this dataset!" - # Valid file and kb → schedules background work, returns data=True immediately kb = SimpleNamespace(id="kb-1", parser_id="naive", pipeline_id="p1", parser_config={}) monkeypatch.setattr(module.KnowledgebaseService, "get_by_id", lambda _kb_id: (True, kb)) + + # Unauthorized file access is rejected before scheduling background work. + monkeypatch.setattr(module, "check_file_team_permission", lambda *_args, **_kwargs: False) + res = _run(module.convert()) + assert res["message"] == "No authorization." + + # Unauthorized dataset access is rejected before scheduling background work. + monkeypatch.setattr(module, "check_file_team_permission", lambda *_args, **_kwargs: True) + monkeypatch.setattr(module, "check_kb_team_permission", lambda *_args, **_kwargs: False) + res = _run(module.convert()) + assert res["message"] == "No authorization." + + # Valid file and kb schedule background work and return data=True immediately. + monkeypatch.setattr(module, "check_kb_team_permission", lambda *_args, **_kwargs: True) res = _run(module.convert()) assert res["code"] == 0 assert res["data"] is True - # Folder expansion → schedules background work, returns data=True immediately + # Folder expansion schedules background work and returns data=True immediately. req_state["file_ids"] = ["folder-1"] monkeypatch.setattr(module.FileService, "get_by_ids", lambda _ids: [_DummyFile("folder-1", module.FileType.FOLDER.value, name="folder")]) monkeypatch.setattr(module.FileService, "get_all_innermost_file_ids", lambda _file_id, _acc: ["inner-1"]) @@ -270,7 +281,7 @@ def test_convert_branch_matrix_unit(monkeypatch): assert res["code"] == 0 assert res["data"] is True - # Exception in file lookup → 500 + # Exception in file lookup returns 500. req_state["file_ids"] = ["f1"] monkeypatch.setattr( module.FileService,