Jelajahi Sumber

Security fix (security-issue #6)

maziggy 1 bulan lalu
induk
melakukan
83ac5b361c

+ 7 - 2
backend/app/api/routes/archives.py

@@ -3983,8 +3983,12 @@ async def slice_archive(
     )
     )
 
 
     archive = await db.get(PrintArchive, archive_id)
     archive = await db.get(PrintArchive, archive_id)
-    if archive is None:
-        raise HTTPException(status_code=404, detail="Archive not found")
+    # Per-row ownership gate — mirror the archive read routes. LIBRARY_UPLOAD
+    # alone let a READ_OWN caller slice another user's archive by raw id even
+    # though GET on that id returned 404. API-key / auth-disabled callers
+    # (current_user is None) keep can_read_all=True — no per-row identity.
+    can_read_all = current_user is None or current_user.has_permission(Permission.ARCHIVES_READ_ALL.value)
+    archive = _ensure_archive_visible(archive, current_user, can_read_all)
 
 
     src_relative = archive.source_3mf_path or archive.file_path
     src_relative = archive.source_3mf_path or archive.file_path
     if not src_relative:
     if not src_relative:
@@ -4055,6 +4059,7 @@ async def slice_archive(
         kind="archive",
         kind="archive",
         source_id=archive.id,
         source_id=archive.id,
         source_name=archive.print_name or archive.filename or f"archive {archive.id}",
         source_name=archive.print_name or archive.filename or f"archive {archive.id}",
+        owner_id=user_id,
         run=_run,
         run=_run,
     )
     )
     return {
     return {

+ 9 - 2
backend/app/api/routes/library.py

@@ -4160,8 +4160,14 @@ async def slice_library_file(
 
 
     src_result = await db.execute(LibraryFile.active().where(LibraryFile.id == file_id))
     src_result = await db.execute(LibraryFile.active().where(LibraryFile.id == file_id))
     lib_file = src_result.scalar_one_or_none()
     lib_file = src_result.scalar_one_or_none()
-    if not lib_file:
-        raise HTTPException(status_code=404, detail="File not found")
+    # Per-row ownership gate. LIBRARY_UPLOAD alone let a READ_OWN caller (e.g. the
+    # built-in Operators group) slice another user's model by raw id even though
+    # GET on that id returned 404 — the sliced output was then attributed to and
+    # downloadable by the requester. Enforce the same visibility the read routes
+    # use before reading the source off disk. API-key / auth-disabled callers
+    # (current_user is None) keep can_read_all=True — no per-row identity.
+    can_read_all = current_user is None or current_user.has_permission(Permission.LIBRARY_READ_ALL.value)
+    lib_file = _ensure_library_file_visible(lib_file, current_user, can_read_all)
 
 
     src_lower = (lib_file.filename or "").lower()
     src_lower = (lib_file.filename or "").lower()
     if not (
     if not (
@@ -4233,6 +4239,7 @@ async def slice_library_file(
         kind="library_file",
         kind="library_file",
         source_id=lib_file.id,
         source_id=lib_file.id,
         source_name=lib_file.filename,
         source_name=lib_file.filename,
+        owner_id=user_id,
         run=_run,
         run=_run,
     )
     )
     return {
     return {

+ 18 - 5
backend/app/api/routes/pipeline_runs.py

@@ -374,11 +374,21 @@ async def _resolve_source(
     *,
     *,
     library_file_id: int | None,
     library_file_id: int | None,
     archive_id: int | None,
     archive_id: int | None,
+    user: User | None,
 ) -> tuple[SourceKind, int, str, Path]:
 ) -> tuple[SourceKind, int, str, Path]:
+    # Per-row ownership gate (IDOR fix): a caller may only run a pipeline on a
+    # source they can see. Without this a READ_OWN caller could reference
+    # another user's library file / archive by raw id and have it sliced (and,
+    # via /run, printed) even though a direct GET on that id returned 404.
+    # Auth-disabled and API-key callers (user is None) keep can_read_all=True —
+    # no per-row identity, matching the library/archive read helpers.
+    from backend.app.api.routes.archives import _ensure_archive_visible
+    from backend.app.api.routes.library import _ensure_library_file_visible
+
     if library_file_id is not None:
     if library_file_id is not None:
         lib = (await db.execute(select(LibraryFile).where(LibraryFile.id == library_file_id))).scalar_one_or_none()
         lib = (await db.execute(select(LibraryFile).where(LibraryFile.id == library_file_id))).scalar_one_or_none()
-        if lib is None:
-            raise HTTPException(404, "Source library file not found")
+        can_read_all = user is None or user.has_permission(Permission.LIBRARY_READ_ALL.value)
+        lib = _ensure_library_file_visible(lib, user, can_read_all)
         src_path = (
         src_path = (
             Path(app_settings.base_dir) / lib.file_path
             Path(app_settings.base_dir) / lib.file_path
         )  # SEC-PATH-OK: lib.file_path is a LibraryFile DB column set only by the upload route, which writes a UUID-named file under base_dir/library_files/.
         )  # SEC-PATH-OK: lib.file_path is a LibraryFile DB column set only by the upload route, which writes a UUID-named file under base_dir/library_files/.
@@ -388,8 +398,8 @@ async def _resolve_source(
 
 
     assert archive_id is not None
     assert archive_id is not None
     arc = (await db.execute(select(PrintArchive).where(PrintArchive.id == archive_id))).scalar_one_or_none()
     arc = (await db.execute(select(PrintArchive).where(PrintArchive.id == archive_id))).scalar_one_or_none()
-    if arc is None:
-        raise HTTPException(404, "Source archive not found")
+    can_read_all = user is None or user.has_permission(Permission.ARCHIVES_READ_ALL.value)
+    arc = _ensure_archive_visible(arc, user, can_read_all)
     rel = arc.source_3mf_path or arc.file_path
     rel = arc.source_3mf_path or arc.file_path
     if not rel:
     if not rel:
         raise HTTPException(400, "Archive has no source file to slice")
         raise HTTPException(400, "Archive has no source file to slice")
@@ -625,7 +635,7 @@ def _make_orchestration_callable(
 async def check_eligibility(
 async def check_eligibility(
     pipeline_id: int,
     pipeline_id: int,
     body: CheckEligibilityRequest,
     body: CheckEligibilityRequest,
-    _: User | None = RequirePermissionIfAuthEnabled(Permission.PIPELINES_READ),
+    current_user: User | None = RequirePermissionIfAuthEnabled(Permission.PIPELINES_READ),
     db: AsyncSession = Depends(get_db),
     db: AsyncSession = Depends(get_db),
 ):
 ):
     pipeline = await _load_pipeline(db, pipeline_id)
     pipeline = await _load_pipeline(db, pipeline_id)
@@ -633,6 +643,7 @@ async def check_eligibility(
         db,
         db,
         library_file_id=body.source_library_file_id,
         library_file_id=body.source_library_file_id,
         archive_id=body.source_archive_id,
         archive_id=body.source_archive_id,
+        user=current_user,
     )
     )
     if pipeline.target_kind == "printer_class" and pipeline.target_printer_id is None:
     if pipeline.target_kind == "printer_class" and pipeline.target_printer_id is None:
         report = await check_pipeline_eligibility(db, pipeline, status_lookup=_make_status_lookup())
         report = await check_pipeline_eligibility(db, pipeline, status_lookup=_make_status_lookup())
@@ -662,6 +673,7 @@ async def run_pipeline(
         db,
         db,
         library_file_id=body.source_library_file_id,
         library_file_id=body.source_library_file_id,
         archive_id=body.source_archive_id,
         archive_id=body.source_archive_id,
+        user=current_user,
     )
     )
 
 
     # Cap copies against the configured ceiling.
     # Cap copies against the configured ceiling.
@@ -732,6 +744,7 @@ async def run_pipeline(
         kind="library_file" if src_kind == "library_file" else "archive",
         kind="library_file" if src_kind == "library_file" else "archive",
         source_id=src_id,
         source_id=src_id,
         source_name=src_filename,
         source_name=src_filename,
+        owner_id=current_user.id if current_user else None,
         run=orchestrate,
         run=orchestrate,
     )
     )
 
 

+ 12 - 7
backend/app/api/routes/slice_jobs.py

@@ -18,22 +18,27 @@ router = APIRouter(prefix="/slice-jobs", tags=["slice-jobs"])
 @router.get("/{job_id}")
 @router.get("/{job_id}")
 async def get_slice_job(
 async def get_slice_job(
     job_id: int,
     job_id: int,
-    # Job IDs are sequential integers and the body leaks source filenames
-    # plus the resulting library_file_id / archive_id. Gate on the library
-    # read permission family (own/all). NOTE: SliceJob is in-memory with no
-    # owner field, so we cannot per-row scope; callers with either OWN or
-    # ALL can poll any job_id. Adding owner_id to SliceJob is the proper
-    # follow-up (out of scope for the IDOR fix train).
-    _: tuple[User | None, bool] = Depends(
+    # Job IDs are sequential integers and the body leaks source filenames plus
+    # the resulting library_file_id / archive_id. Gate on the library read
+    # permission family (own/all) and then scope per-row: a READ_OWN caller may
+    # only poll jobs they started (SliceJob.owner_id).
+    auth: tuple[User | None, bool] = Depends(
         require_ownership_permission(
         require_ownership_permission(
             Permission.LIBRARY_READ_ALL,
             Permission.LIBRARY_READ_ALL,
             Permission.LIBRARY_READ_OWN,
             Permission.LIBRARY_READ_OWN,
         )
         )
     ),
     ),
 ):
 ):
+    user, can_read_all = auth
     job = slice_dispatch.get(job_id)
     job = slice_dispatch.get(job_id)
     if job is None:
     if job is None:
         raise HTTPException(status_code=404, detail="Slice job not found or expired")
         raise HTTPException(status_code=404, detail="Slice job not found or expired")
+    # Per-row scoping. Jobs started by API-key / auth-disabled callers have
+    # owner_id=None and are visible only to READ_ALL pollers (fail-closed,
+    # mirrors the library ownerless-row rule). 404 not 403 to avoid job-id
+    # enumeration.
+    if not can_read_all and (user is None or job.owner_id != user.id):
+        raise HTTPException(status_code=404, detail="Slice job not found or expired")
     body: dict = {
     body: dict = {
         "job_id": job.id,
         "job_id": job.id,
         "status": job.status,
         "status": job.status,

+ 6 - 0
backend/app/services/slice_dispatch.py

@@ -30,6 +30,10 @@ class SliceJob:
     kind: Literal["library_file", "archive"]
     kind: Literal["library_file", "archive"]
     source_id: int
     source_id: int
     source_name: str
     source_name: str
+    # JWT user id that started the job, for per-row scoping of the polling
+    # endpoint. None for API-key / auth-disabled callers (no per-row identity);
+    # those jobs are visible only to READ_ALL pollers — see slice_jobs.py.
+    owner_id: int | None = None
     status: SliceJobStatus = "pending"
     status: SliceJobStatus = "pending"
     created_at: datetime = field(default_factory=lambda: datetime.now(timezone.utc))
     created_at: datetime = field(default_factory=lambda: datetime.now(timezone.utc))
     started_at: datetime | None = None
     started_at: datetime | None = None
@@ -68,6 +72,7 @@ class SliceDispatchService:
         kind: Literal["library_file", "archive"],
         kind: Literal["library_file", "archive"],
         source_id: int,
         source_id: int,
         source_name: str,
         source_name: str,
+        owner_id: int | None = None,
         run: Callable[[int], Awaitable[dict[str, Any]]],
         run: Callable[[int], Awaitable[dict[str, Any]]],
     ) -> SliceJob:
     ) -> SliceJob:
         """Register a new slice job and start it on the event loop.
         """Register a new slice job and start it on the event loop.
@@ -83,6 +88,7 @@ class SliceDispatchService:
                 kind=kind,
                 kind=kind,
                 source_id=source_id,
                 source_id=source_id,
                 source_name=source_name,
                 source_name=source_name,
+                owner_id=owner_id,
             )
             )
             self._next_id += 1
             self._next_id += 1
             self._jobs[job.id] = job
             self._jobs[job.id] = job

+ 220 - 0
backend/tests/integration/test_ownership_permissions.py

@@ -1417,3 +1417,223 @@ class TestWriteSubResourceIDORClosure(TestOwnershipPermissionsSetup):
             headers={"Authorization": f"Bearer {auth_setup['admin_token']}"},
             headers={"Authorization": f"Bearer {auth_setup['admin_token']}"},
         )
         )
         assert response.status_code == 200
         assert response.status_code == 200
+
+
+class TestSliceOwnershipPermissions(TestOwnershipPermissionsSetup):
+    """IDOR regression: slicing and slice-job polling must honour per-row ownership.
+
+    Before the fix, ``POST /library/files/{id}/slice`` and
+    ``POST /archives/{id}/slice`` gated only on ``LIBRARY_UPLOAD``, so a
+    READ_OWN operator could slice another user's model by raw id even though a
+    direct GET on that id returned 404 — the sliced output was then attributed
+    to and downloadable by the requester. ``GET /slice-jobs/{id}`` had no owner
+    scoping at all. ``POST /slicer-pipelines/{id}/run`` (and check-eligibility)
+    resolved the source by raw id with the same gap.
+
+    The slice route enforces the gate before touching the source bytes, so the
+    owner/READ_ALL "control" cases reach the later on-disk check (a distinct 404
+    detail) rather than a real slice — enough to prove the gate lets them past.
+    """
+
+    # Any preset triplet: the ownership 404 fires before preset resolution.
+    _SLICE_BODY = {"printer_preset_id": 1, "process_preset_id": 2, "filament_preset_id": 3}
+
+    @pytest.fixture
+    async def library_file_factory(self, db_session):
+        _counter = [0]
+
+        async def _create_file(**kwargs):
+            from backend.app.models.library import LibraryFile
+
+            _counter[0] += 1
+            defaults = {
+                "filename": f"slice_src_{_counter[0]}.3mf",
+                "file_path": f"library/slice_src_{_counter[0]}.3mf",
+                "file_type": "3mf",
+                "file_size": 1024,
+            }
+            defaults.update(kwargs)
+            row = LibraryFile(**defaults)
+            db_session.add(row)
+            await db_session.commit()
+            await db_session.refresh(row)
+            return row
+
+        return _create_file
+
+    # --- library file slice ------------------------------------------------
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_operator_cannot_slice_others_library_file(self, async_client, auth_setup, library_file_factory):
+        file = await library_file_factory(created_by_id=auth_setup["operator2_user"]["id"])
+        resp = await async_client.post(
+            f"/api/v1/library/files/{file.id}/slice",
+            headers={"Authorization": f"Bearer {auth_setup['operator_token']}"},
+            json=self._SLICE_BODY,
+        )
+        assert resp.status_code == 404
+        # 404 (not 403) so a probing operator can't tell the id exists.
+        assert resp.json()["detail"] == "File not found"
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_operator_can_slice_own_library_file(self, async_client, auth_setup, library_file_factory):
+        file = await library_file_factory(created_by_id=auth_setup["operator_user"]["id"])
+        resp = await async_client.post(
+            f"/api/v1/library/files/{file.id}/slice",
+            headers={"Authorization": f"Bearer {auth_setup['operator_token']}"},
+            json=self._SLICE_BODY,
+        )
+        # Past the ownership gate — only the on-disk source is missing in tests.
+        assert resp.status_code == 404
+        assert resp.json()["detail"] == "Source file missing on disk"
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_admin_can_slice_any_library_file(self, async_client, auth_setup, library_file_factory):
+        file = await library_file_factory(created_by_id=auth_setup["operator2_user"]["id"])
+        resp = await async_client.post(
+            f"/api/v1/library/files/{file.id}/slice",
+            headers={"Authorization": f"Bearer {auth_setup['admin_token']}"},
+            json=self._SLICE_BODY,
+        )
+        # READ_ALL passes the gate even on another user's file.
+        assert resp.status_code == 404
+        assert resp.json()["detail"] == "Source file missing on disk"
+
+    # --- archive slice -----------------------------------------------------
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_operator_cannot_slice_others_archive(
+        self, async_client, auth_setup, archive_factory, printer_factory
+    ):
+        printer = await printer_factory()
+        archive = await archive_factory(printer.id, created_by_id=auth_setup["operator2_user"]["id"])
+        resp = await async_client.post(
+            f"/api/v1/archives/{archive.id}/slice",
+            headers={"Authorization": f"Bearer {auth_setup['operator_token']}"},
+            json=self._SLICE_BODY,
+        )
+        assert resp.status_code == 404
+        assert resp.json()["detail"] == "Archive not found"
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_operator_can_slice_own_archive(self, async_client, auth_setup, archive_factory, printer_factory):
+        printer = await printer_factory()
+        archive = await archive_factory(printer.id, created_by_id=auth_setup["operator_user"]["id"])
+        resp = await async_client.post(
+            f"/api/v1/archives/{archive.id}/slice",
+            headers={"Authorization": f"Bearer {auth_setup['operator_token']}"},
+            json=self._SLICE_BODY,
+        )
+        # Past the gate — the archive's source file isn't on disk in tests.
+        assert resp.status_code == 404
+        assert resp.json()["detail"] == "Archive source file missing on disk"
+
+    # --- slice-job polling -------------------------------------------------
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_slice_job_polling_is_owner_scoped(self, async_client, auth_setup):
+        from backend.app.services.slice_dispatch import slice_dispatch
+
+        async def _noop(_job_id):
+            return {}
+
+        job = await slice_dispatch.enqueue(
+            kind="library_file",
+            source_id=1,
+            source_name="secret_model.3mf",
+            owner_id=auth_setup["operator2_user"]["id"],
+            run=_noop,
+        )
+
+        # Non-owner without READ_ALL cannot see the job (404, not 403).
+        other = await async_client.get(
+            f"/api/v1/slice-jobs/{job.id}",
+            headers={"Authorization": f"Bearer {auth_setup['operator_token']}"},
+        )
+        assert other.status_code == 404
+
+        # The owner and a READ_ALL admin can.
+        owner = await async_client.get(
+            f"/api/v1/slice-jobs/{job.id}",
+            headers={"Authorization": f"Bearer {auth_setup['operator2_token']}"},
+        )
+        assert owner.status_code == 200
+        admin = await async_client.get(
+            f"/api/v1/slice-jobs/{job.id}",
+            headers={"Authorization": f"Bearer {auth_setup['admin_token']}"},
+        )
+        assert admin.status_code == 200
+
+    # --- pipeline source resolution ----------------------------------------
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_pipeline_run_cannot_reference_others_library_file(
+        self, async_client, auth_setup, library_file_factory, db_session
+    ):
+        """A pipeline runner with READ_OWN cannot resolve another user's source.
+
+        The built-in Operators group has no pipeline permissions, so this uses a
+        custom group carrying PIPELINES_RUN + READ_OWN — the realistic shape of
+        the exposure. check-eligibility resolves the source before any
+        eligibility work, so the ownership gate is what returns 404.
+        """
+        from backend.app.models.slicer_pipeline import SlicerPipeline
+
+        admin_headers = {"Authorization": f"Bearer {auth_setup['admin_token']}"}
+        group_resp = await async_client.post(
+            "/api/v1/groups/",
+            headers=admin_headers,
+            json={
+                "name": "pipeline_runners",
+                "permissions": [
+                    "pipelines:read",
+                    "pipelines:run",
+                    "library:read_own",
+                    "archives:read_own",
+                ],
+            },
+        )
+        assert group_resp.status_code == 201, group_resp.text
+        group_id = group_resp.json()["id"]
+
+        await async_client.post(
+            "/api/v1/users/",
+            headers=admin_headers,
+            json={"username": "runner1", "password": "Runnerpass1!", "group_ids": [group_id]},
+        )
+        runner_login = await async_client.post(
+            "/api/v1/auth/login",
+            json={"username": "runner1", "password": "Runnerpass1!"},
+        )
+        runner_token = runner_login.json()["access_token"]
+
+        pipeline = SlicerPipeline(
+            name="Cross-user pipeline",
+            printer_preset_source="local",
+            printer_preset_id="1",
+            process_preset_source="local",
+            process_preset_id="2",
+            filament_presets_json="[]",
+            target_kind="printer_class",
+            target_model_class="Bambu Lab X1 Carbon",
+        )
+        db_session.add(pipeline)
+        await db_session.commit()
+        await db_session.refresh(pipeline)
+
+        # Source owned by operator2, not the runner.
+        file = await library_file_factory(created_by_id=auth_setup["operator2_user"]["id"])
+        resp = await async_client.post(
+            f"/api/v1/slicer-pipelines/{pipeline.id}/check-eligibility",
+            headers={"Authorization": f"Bearer {runner_token}"},
+            json={"source_library_file_id": file.id},
+        )
+        assert resp.status_code == 404
+        assert resp.json()["detail"] == "File not found"