Security hardening (maziggy/bambuddy-security #5)

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")

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

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