diff --git a/backend/app/api/routes/archives.py b/backend/app/api/routes/archives.py index c8b226d30..2188d06e1 100644 --- a/backend/app/api/routes/archives.py +++ b/backend/app/api/routes/archives.py @@ -1658,13 +1658,17 @@ async def update_archive( async def toggle_favorite( archive_id: int, db: AsyncSession = Depends(get_db), - _: User | None = RequirePermissionIfAuthEnabled(Permission.ARCHIVES_UPDATE_OWN), + auth_result: tuple[User | None, bool] = Depends( + require_ownership_permission( + Permission.ARCHIVES_UPDATE_ALL, + Permission.ARCHIVES_UPDATE_OWN, + ) + ), ): """Toggle favorite status for an archive.""" + user, can_modify_all = auth_result result = await db.execute(select(PrintArchive).where(PrintArchive.id == archive_id)) - archive = result.scalar_one_or_none() - if not archive: - raise HTTPException(404, "Archive not found") + archive = _ensure_archive_visible(result.scalar_one_or_none(), user, can_modify_all) archive.is_favorite = not archive.is_favorite await db.commit() @@ -2213,13 +2217,17 @@ async def get_timelapse( async def delete_timelapse( archive_id: int, db: AsyncSession = Depends(get_db), - _: User | None = RequirePermissionIfAuthEnabled(Permission.ARCHIVES_DELETE_OWN), + auth_result: tuple[User | None, bool] = Depends( + require_ownership_permission( + Permission.ARCHIVES_DELETE_ALL, + Permission.ARCHIVES_DELETE_OWN, + ) + ), ): """Remove the timelapse video from an archive.""" + user, can_modify_all = auth_result result = await db.execute(select(PrintArchive).where(PrintArchive.id == archive_id)) - archive = result.scalar_one_or_none() - if not archive: - raise HTTPException(404, "Archive not found") + archive = _ensure_archive_visible(result.scalar_one_or_none(), user, can_modify_all) if not archive.timelapse_path: raise HTTPException(404, "No timelapse attached to this archive") @@ -2727,13 +2735,17 @@ async def upload_photo( archive_id: int, file: UploadFile = File(...), db: AsyncSession = Depends(get_db), - _: User | None = RequirePermissionIfAuthEnabled(Permission.ARCHIVES_UPDATE_OWN), + auth_result: tuple[User | None, bool] = Depends( + require_ownership_permission( + Permission.ARCHIVES_UPDATE_ALL, + Permission.ARCHIVES_UPDATE_OWN, + ) + ), ): """Upload a photo of the printed result.""" + user, can_modify_all = auth_result result = await db.execute(select(PrintArchive).where(PrintArchive.id == archive_id)) - archive = result.scalar_one_or_none() - if not archive: - raise HTTPException(404, "Archive not found") + archive = _ensure_archive_visible(result.scalar_one_or_none(), user, can_modify_all) if not file.filename or not file.filename.lower().endswith((".jpg", ".jpeg", ".png", ".webp")): raise HTTPException(400, "File must be an image (.jpg, .jpeg, .png, .webp)") @@ -2817,13 +2829,17 @@ async def delete_photo( archive_id: int, filename: str, db: AsyncSession = Depends(get_db), - _: User | None = RequirePermissionIfAuthEnabled(Permission.ARCHIVES_DELETE_OWN), + auth_result: tuple[User | None, bool] = Depends( + require_ownership_permission( + Permission.ARCHIVES_DELETE_ALL, + Permission.ARCHIVES_DELETE_OWN, + ) + ), ): """Delete a photo.""" + user, can_modify_all = auth_result result = await db.execute(select(PrintArchive).where(PrintArchive.id == archive_id)) - archive = result.scalar_one_or_none() - if not archive: - raise HTTPException(404, "Archive not found") + archive = _ensure_archive_visible(result.scalar_one_or_none(), user, can_modify_all) if not archive.photos or filename not in archive.photos: raise HTTPException(404, "Photo not found") @@ -4091,15 +4107,19 @@ async def update_project_page( archive_id: int, update_data: dict, db: AsyncSession = Depends(get_db), - _: User | None = RequirePermissionIfAuthEnabled(Permission.ARCHIVES_UPDATE_OWN), + auth_result: tuple[User | None, bool] = Depends( + require_ownership_permission( + Permission.ARCHIVES_UPDATE_ALL, + Permission.ARCHIVES_UPDATE_OWN, + ) + ), ): """Update project page metadata in the 3MF file.""" from backend.app.services.archive import ProjectPageParser + user, can_modify_all = auth_result service = ArchiveService(db) - archive = await service.get_archive(archive_id) - if not archive: - raise HTTPException(404, "Archive not found") + archive = _ensure_archive_visible(await service.get_archive(archive_id), user, can_modify_all) file_path = settings.base_dir / archive.file_path if not file_path.is_file(): @@ -4209,13 +4229,17 @@ async def upload_source_3mf( archive_id: int, file: UploadFile = File(...), db: AsyncSession = Depends(get_db), - _: User | None = RequirePermissionIfAuthEnabled(Permission.ARCHIVES_UPDATE_OWN), + auth_result: tuple[User | None, bool] = Depends( + require_ownership_permission( + Permission.ARCHIVES_UPDATE_ALL, + Permission.ARCHIVES_UPDATE_OWN, + ) + ), ): """Upload the original source 3MF project file for an archive.""" + user, can_modify_all = auth_result result = await db.execute(select(PrintArchive).where(PrintArchive.id == archive_id)) - archive = result.scalar_one_or_none() - if not archive: - raise HTTPException(404, "Archive not found") + archive = _ensure_archive_visible(result.scalar_one_or_none(), user, can_modify_all) if not file.filename or not file.filename.endswith(".3mf"): raise HTTPException(400, "File must be a .3mf file") @@ -4470,13 +4494,17 @@ async def upload_source_3mf_by_name( async def delete_source_3mf( archive_id: int, db: AsyncSession = Depends(get_db), - _: User | None = RequirePermissionIfAuthEnabled(Permission.ARCHIVES_DELETE_OWN), + auth_result: tuple[User | None, bool] = Depends( + require_ownership_permission( + Permission.ARCHIVES_DELETE_ALL, + Permission.ARCHIVES_DELETE_OWN, + ) + ), ): """Delete the source 3MF project file from an archive.""" + user, can_modify_all = auth_result result = await db.execute(select(PrintArchive).where(PrintArchive.id == archive_id)) - archive = result.scalar_one_or_none() - if not archive: - raise HTTPException(404, "Archive not found") + archive = _ensure_archive_visible(result.scalar_one_or_none(), user, can_modify_all) if not archive.source_3mf_path: raise HTTPException(404, "No source 3MF attached to this archive") @@ -4503,13 +4531,17 @@ async def upload_f3d( archive_id: int, file: UploadFile = File(...), db: AsyncSession = Depends(get_db), - _: User | None = RequirePermissionIfAuthEnabled(Permission.ARCHIVES_UPDATE_OWN), + auth_result: tuple[User | None, bool] = Depends( + require_ownership_permission( + Permission.ARCHIVES_UPDATE_ALL, + Permission.ARCHIVES_UPDATE_OWN, + ) + ), ): """Upload a Fusion 360 design file for an archive.""" + user, can_modify_all = auth_result result = await db.execute(select(PrintArchive).where(PrintArchive.id == archive_id)) - archive = result.scalar_one_or_none() - if not archive: - raise HTTPException(404, "Archive not found") + archive = _ensure_archive_visible(result.scalar_one_or_none(), user, can_modify_all) if not file.filename or not file.filename.endswith(".f3d"): raise HTTPException(400, "File must be a .f3d file") @@ -4583,13 +4615,17 @@ async def download_f3d( async def delete_f3d( archive_id: int, db: AsyncSession = Depends(get_db), - _: User | None = RequirePermissionIfAuthEnabled(Permission.ARCHIVES_DELETE_OWN), + auth_result: tuple[User | None, bool] = Depends( + require_ownership_permission( + Permission.ARCHIVES_DELETE_ALL, + Permission.ARCHIVES_DELETE_OWN, + ) + ), ): """Delete the Fusion 360 design file from an archive.""" + user, can_modify_all = auth_result result = await db.execute(select(PrintArchive).where(PrintArchive.id == archive_id)) - archive = result.scalar_one_or_none() - if not archive: - raise HTTPException(404, "Archive not found") + archive = _ensure_archive_visible(result.scalar_one_or_none(), user, can_modify_all) if not archive.f3d_path: raise HTTPException(404, "No F3D file attached to this archive") diff --git a/backend/tests/integration/test_ownership_permissions.py b/backend/tests/integration/test_ownership_permissions.py index df49af5c6..e6492d470 100644 --- a/backend/tests/integration/test_ownership_permissions.py +++ b/backend/tests/integration/test_ownership_permissions.py @@ -1272,3 +1272,148 @@ class TestReadIDORClosure(TestOwnershipPermissionsSetup): # change auth-enable/disable behavior. Pin not-404 to avoid masking a # regression where auth-disabled callers would lose access. assert response.status_code in (200, 401) + + +# Every archive WRITE sub-resource route: (id, http method, path suffix, request kwargs). +# The ownership gate (_ensure_archive_visible) fires immediately after the fetch, +# before any resource-specific logic, so a not-owned / ownerless row 404s regardless +# of whether the timelapse / photo / source / f3d actually exists. Upload routes still +# need a body so FastAPI reaches the handler instead of 422-ing on the missing File(...). +_WRITE_SUBRESOURCE_ROUTES = [ + ("favorite", "post", "/favorite", {}), + ("timelapse_delete", "delete", "/timelapse", {}), + ("photo_upload", "post", "/photos", {"files": {"file": ("x.jpg", b"\x89PNG\r\n\x1a\n", "image/jpeg")}}), + ("photo_delete", "delete", "/photos/nonexistent.jpg", {}), + ("project_page", "patch", "/project-page", {"json": {"title": "hijacked"}}), + ("source_upload", "post", "/source", {"files": {"file": ("x.3mf", b"PK\x03\x04", "application/octet-stream")}}), + ("source_delete", "delete", "/source", {}), + ("f3d_upload", "post", "/f3d", {"files": {"file": ("x.f3d", b"f3d-bytes", "application/octet-stream")}}), + ("f3d_delete", "delete", "/f3d", {}), +] + + +class TestWriteSubResourceIDORClosure(TestOwnershipPermissionsSetup): + """Regression tests for the archive write SUB-RESOURCE IDOR. + + The read sub-resource routes were closed under maziggy/bambuddy-security #2 + via ``_ensure_archive_visible``, but the *write* sub-resource routes + (favorite, timelapse, photos, project-page, source, f3d) were left gating + on the bare ``RequirePermissionIfAuthEnabled(ARCHIVES_*_OWN)`` scope and + fetched the row by id only — never comparing ``created_by_id`` to the + caller. An operator holding only ``ARCHIVES_*_OWN`` (or an API key with + ``can_manage_archives``) could delete/overwrite files on ANY user's + archive, most severely rewriting the project-page metadata inside another + user's ``.3mf`` on disk. Each route is now gated by + ``require_ownership_permission`` + ``_ensure_archive_visible`` → 404 (not + 403, to stay non-enumerable and match the read side) on a not-owned or + ownerless row. + """ + + @pytest.mark.parametrize( + "name,method,suffix,kwargs", + _WRITE_SUBRESOURCE_ROUTES, + ids=[r[0] for r in _WRITE_SUBRESOURCE_ROUTES], + ) + @pytest.mark.asyncio + @pytest.mark.integration + async def test_operator_cannot_write_others_archive_subresource( + self, + async_client: AsyncClient, + auth_setup, + archive_factory, + printer_factory, + db_session, + name, + method, + suffix, + kwargs, + ): + """SECURITY.md rule 4: right credentials, wrong ownership → 404. + + operator1 (ARCHIVES_*_OWN) targeting a route on admin's archive. + """ + printer = await printer_factory() + archive = await archive_factory( + printer.id, + print_name="Admin's Archive", + created_by_id=auth_setup["admin_user"]["id"], + ) + response = await getattr(async_client, method)( + f"/api/v1/archives/{archive.id}{suffix}", + headers={"Authorization": f"Bearer {auth_setup['operator_token']}"}, + **kwargs, + ) + assert response.status_code == 404, f"{name}: expected 404, got {response.status_code}" + + @pytest.mark.parametrize( + "name,method,suffix,kwargs", + _WRITE_SUBRESOURCE_ROUTES, + ids=[r[0] for r in _WRITE_SUBRESOURCE_ROUTES], + ) + @pytest.mark.asyncio + @pytest.mark.integration + async def test_operator_cannot_write_ownerless_archive_subresource( + self, + async_client: AsyncClient, + auth_setup, + archive_factory, + printer_factory, + db_session, + name, + method, + suffix, + kwargs, + ): + """Ownerless rows (created_by_id = null, legacy data) require *_ALL — an + operator with only *_OWN has no 'I own this' claim, so fail closed → 404.""" + printer = await printer_factory() + archive = await archive_factory( + printer.id, + print_name="Ownerless Archive", + created_by_id=None, + ) + response = await getattr(async_client, method)( + f"/api/v1/archives/{archive.id}{suffix}", + headers={"Authorization": f"Bearer {auth_setup['operator_token']}"}, + **kwargs, + ) + assert response.status_code == 404, f"{name}: expected 404, got {response.status_code}" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_operator_can_favorite_own_archive( + self, async_client: AsyncClient, auth_setup, archive_factory, printer_factory, db_session + ): + """Positive control: the owner still gets through the new gate. Favorite + is the one write sub-resource that needs no pre-existing file, so it + cleanly proves the *_OWN happy path returns 200 (not a false 404).""" + printer = await printer_factory() + archive = await archive_factory( + printer.id, + print_name="Operator's Own", + created_by_id=auth_setup["operator_user"]["id"], + ) + response = await async_client.post( + f"/api/v1/archives/{archive.id}/favorite", + headers={"Authorization": f"Bearer {auth_setup['operator_token']}"}, + ) + assert response.status_code == 200 + assert response.json()["is_favorite"] is True + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_admin_can_favorite_any_archive( + self, async_client: AsyncClient, auth_setup, archive_factory, printer_factory, db_session + ): + """Positive control for the *_ALL path: admin can act on a user's archive.""" + printer = await printer_factory() + archive = await archive_factory( + printer.id, + print_name="Operator's Own", + created_by_id=auth_setup["operator_user"]["id"], + ) + response = await async_client.post( + f"/api/v1/archives/{archive.id}/favorite", + headers={"Authorization": f"Bearer {auth_setup['admin_token']}"}, + ) + assert response.status_code == 200 diff --git a/backend/tests/unit/test_archive_filtering.py b/backend/tests/unit/test_archive_filtering.py index 51abffae7..d9d17122d 100644 --- a/backend/tests/unit/test_archive_filtering.py +++ b/backend/tests/unit/test_archive_filtering.py @@ -773,6 +773,7 @@ class TestDeleteTimelapse: mock_archive = MagicMock() mock_archive.timelapse_path = "archives/1/timelapse.mp4" + mock_archive.deleted_at = None mock_db = AsyncMock() mock_db.execute = AsyncMock() @@ -782,7 +783,8 @@ class TestDeleteTimelapse: with patch("backend.app.api.routes.archives.settings") as mock_settings: mock_settings.base_dir = tmp_path - result = await delete_timelapse(archive_id=1, db=mock_db) + # auth_result=(None, True) → the auth-disabled / can_modify_all path. + result = await delete_timelapse(archive_id=1, db=mock_db, auth_result=(None, True)) assert result == {"status": "deleted"} assert mock_archive.timelapse_path is None @@ -798,6 +800,7 @@ class TestDeleteTimelapse: mock_archive = MagicMock() mock_archive.timelapse_path = None + mock_archive.deleted_at = None mock_db = AsyncMock() mock_result = MagicMock() @@ -805,7 +808,7 @@ class TestDeleteTimelapse: mock_db.execute = AsyncMock(return_value=mock_result) with pytest.raises(HTTPException) as exc_info: - await delete_timelapse(archive_id=1, db=mock_db) + await delete_timelapse(archive_id=1, db=mock_db, auth_result=(None, True)) assert exc_info.value.status_code == 404 @@ -822,6 +825,6 @@ class TestDeleteTimelapse: mock_db.execute = AsyncMock(return_value=mock_result) with pytest.raises(HTTPException) as exc_info: - await delete_timelapse(archive_id=999, db=mock_db) + await delete_timelapse(archive_id=999, db=mock_db, auth_result=(None, True)) assert exc_info.value.status_code == 404