fix(audit): P0-P3 audit fixes — 838 ruff errors → 0, 30 F821 bugs fixed, 118 files changed
- P0: hooks.py 3-tuple fix, trigger_dispatcher Contract, contacts/plugin unregister_actions_by_owner - P0: 5 test files — check_permission mocks removed, hardcoded DB credential → env var - P1: attachment_service DmsFile via Contract helper, restore_registry/history_hooks dedup - P1: mail/plugin restore unregister, mcp_client datetime.now(UTC), saved_views/filters patterns - P1: ProtectedRoute fail-closed, 13 test assertion fixes (bcrypt, DB-URLs, SECRET_KEYs) - P2: deprecated notifications → post_system_message (3 files), forgejo Base, report_generator lazy import - P2: webhooks permissions, deps.py/roles.py plugin perms removed, import_export default - P2: address/tags/entity_links patterns removed, worker.py Contract-Umgehungen fixed - P2: 28 frontend TODOs (hardcoded constants, deprecated notification API) - P3: dead code, duplicates, deprecated imports, private attr, __import__ inline - P3: 8 frontend TODOs (LucideIcons, inline styles, XSS, i18n) - ruff: 838 → 0 (612 auto-fix + 246 manual + 27 F821 regression fix) - F821: 30 → 0 (AutomationDefinition, DmsFile, user_id, Path, Any, String) - Contract-Umgehungen: 2 neue gefunden (worker.py:169, worker.py:280) und gefixt
This commit is contained in:
@@ -15,14 +15,23 @@ import uuid
|
||||
from datetime import UTC, datetime
|
||||
from typing import Any
|
||||
|
||||
from sqlalchemy import select, func
|
||||
from sqlalchemy import select
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
from app.core.audit import log_audit
|
||||
from app.core.storage import get_storage_backend
|
||||
from app.core.visibility import apply_visibility_filter, check_single_entity_access
|
||||
from app.models.entity_attachment import EntityAttachment
|
||||
from app.plugins.builtins.dms.models import File as DmsFile
|
||||
|
||||
# DMS File model accessed via contract to avoid Core→Plugin dependency (P1-9 fix)
|
||||
|
||||
def _get_dms_file_model():
|
||||
"""Get DMS File model via contract registry, or None if DMS plugin inactive."""
|
||||
from app.plugins.builtins.contracts import get_contract
|
||||
dms_contract = get_contract("dms")
|
||||
if dms_contract is not None:
|
||||
return dms_contract.dms_file
|
||||
return None
|
||||
|
||||
|
||||
# File size limit: 50MB
|
||||
@@ -35,7 +44,7 @@ def _generate_unique_filename(original_filename: str) -> str:
|
||||
return f"{uuid.uuid4().hex}{ext}"
|
||||
|
||||
|
||||
def _entity_attachment_to_dict(ea: EntityAttachment, dms_file: DmsFile | None = None) -> dict[str, Any]:
|
||||
def _entity_attachment_to_dict(ea: EntityAttachment, dms_file: Any = None) -> dict[str, Any]:
|
||||
"""Serialize an EntityAttachment + DMS File to dict."""
|
||||
return {
|
||||
"id": str(ea.id),
|
||||
@@ -69,8 +78,6 @@ async def save_attachment(
|
||||
|
||||
Streams the file in chunks to avoid loading entire file into RAM.
|
||||
"""
|
||||
import hashlib
|
||||
from app.core.storage import get_storage_backend
|
||||
|
||||
# Generate unique filename and storage path
|
||||
unique_filename = _generate_unique_filename(filename)
|
||||
@@ -79,14 +86,14 @@ async def save_attachment(
|
||||
# Stream file to storage — compute hash and size during streaming
|
||||
sha256 = hashlib.sha256()
|
||||
file_size = 0
|
||||
CHUNK_SIZE = 1024 * 1024 # 1MB chunks
|
||||
chunk_size = 1024 * 1024 # 1MB chunks
|
||||
|
||||
async def chunk_stream():
|
||||
nonlocal file_size
|
||||
if hasattr(file, 'read'):
|
||||
# UploadFile object
|
||||
while True:
|
||||
chunk = await file.read(CHUNK_SIZE)
|
||||
chunk = await file.read(chunk_size)
|
||||
if not chunk:
|
||||
break
|
||||
file_size += len(chunk)
|
||||
@@ -111,9 +118,9 @@ async def save_attachment(
|
||||
|
||||
# Check for blocked file types
|
||||
import os as _os
|
||||
_BLOCKED = {".exe", ".bat", ".cmd", ".sh", ".jar", ".com", ".scr", ".msi", ".dll", ".vbs", ".ps1", ".app", ".bin", ".reg", ".inf"}
|
||||
_blocked = {".exe", ".bat", ".cmd", ".sh", ".jar", ".com", ".scr", ".msi", ".dll", ".vbs", ".ps1", ".app", ".bin", ".reg", ".inf"}
|
||||
_ext = _os.path.splitext(filename)[1].lower()
|
||||
if _ext in _BLOCKED:
|
||||
if _ext in _blocked:
|
||||
await storage.delete(storage_path)
|
||||
raise ValueError(f"File type not allowed: {_ext}")
|
||||
|
||||
@@ -128,12 +135,18 @@ async def save_attachment(
|
||||
|
||||
content_hash = sha256.hexdigest()
|
||||
|
||||
# Get DMS File model via contract (avoid Core→Plugin dependency)
|
||||
dms_file = _get_dms_file_model()
|
||||
if dms_file is None:
|
||||
await storage.delete(storage_path)
|
||||
raise RuntimeError("DMS plugin not available — cannot save attachment")
|
||||
|
||||
# Check for existing DMS file with same hash in same tenant (deduplication)
|
||||
existing_file = await db.execute(
|
||||
select(DmsFile).where(
|
||||
DmsFile.tenant_id == tenant_id,
|
||||
DmsFile.content_hash == content_hash,
|
||||
DmsFile.deleted_at.is_(None),
|
||||
select(dms_file).where(
|
||||
dms_file.tenant_id == tenant_id,
|
||||
dms_file.content_hash == content_hash,
|
||||
dms_file.deleted_at.is_(None),
|
||||
).limit(1)
|
||||
)
|
||||
existing_dms_file = existing_file.scalar_one_or_none()
|
||||
@@ -144,7 +157,7 @@ async def save_attachment(
|
||||
await storage.delete(storage_path) # Remove the duplicate we just saved
|
||||
else:
|
||||
# File already streamed to storage — create DMS File record
|
||||
dms_file = DmsFile(
|
||||
dms_file = dms_file(
|
||||
tenant_id=tenant_id,
|
||||
name=filename,
|
||||
folder_id=None, # Attachments don't go in DMS folders
|
||||
@@ -190,15 +203,18 @@ async def list_attachments(
|
||||
is_system_admin: bool = False,
|
||||
) -> dict[str, Any]:
|
||||
"""List attachments for a specific entity (via DMS files)."""
|
||||
dms_file = _get_dms_file_model()
|
||||
if dms_file is None:
|
||||
return {"items": [], "total": 0}
|
||||
q = (
|
||||
select(EntityAttachment, DmsFile)
|
||||
.join(DmsFile, EntityAttachment.dms_file_id == DmsFile.id)
|
||||
select(EntityAttachment, dms_file)
|
||||
.join(dms_file, EntityAttachment.dms_file_id == dms_file.id)
|
||||
.where(
|
||||
EntityAttachment.tenant_id == tenant_id,
|
||||
EntityAttachment.entity_type == entity_type,
|
||||
EntityAttachment.entity_id == entity_id,
|
||||
EntityAttachment.deleted_at.is_(None),
|
||||
DmsFile.deleted_at.is_(None),
|
||||
dms_file.deleted_at.is_(None),
|
||||
)
|
||||
.order_by(EntityAttachment.created_at.desc())
|
||||
)
|
||||
@@ -222,9 +238,12 @@ async def get_attachment(
|
||||
is_system_admin: bool = False,
|
||||
) -> dict[str, Any] | None:
|
||||
"""Get a single attachment by ID (with DMS file info)."""
|
||||
dms_file = _get_dms_file_model()
|
||||
if dms_file is None:
|
||||
return None
|
||||
q = (
|
||||
select(EntityAttachment, DmsFile)
|
||||
.join(DmsFile, EntityAttachment.dms_file_id == DmsFile.id)
|
||||
select(EntityAttachment, dms_file)
|
||||
.join(dms_file, EntityAttachment.dms_file_id == dms_file.id)
|
||||
.where(
|
||||
EntityAttachment.id == attachment_id,
|
||||
EntityAttachment.tenant_id == tenant_id,
|
||||
@@ -253,14 +272,17 @@ async def get_attachment_download_path(
|
||||
is_system_admin: bool = False,
|
||||
) -> str | None:
|
||||
"""Get the storage path for downloading an attachment's DMS file."""
|
||||
dms_file = _get_dms_file_model()
|
||||
if dms_file is None:
|
||||
return None
|
||||
q = (
|
||||
select(EntityAttachment, DmsFile)
|
||||
.join(DmsFile, EntityAttachment.dms_file_id == DmsFile.id)
|
||||
select(EntityAttachment, dms_file)
|
||||
.join(dms_file, EntityAttachment.dms_file_id == dms_file.id)
|
||||
.where(
|
||||
EntityAttachment.id == attachment_id,
|
||||
EntityAttachment.tenant_id == tenant_id,
|
||||
EntityAttachment.deleted_at.is_(None),
|
||||
DmsFile.deleted_at.is_(None),
|
||||
dms_file.deleted_at.is_(None),
|
||||
)
|
||||
)
|
||||
result = await db.execute(q)
|
||||
@@ -285,7 +307,7 @@ async def delete_attachment(
|
||||
is_system_admin: bool = False,
|
||||
) -> bool:
|
||||
"""Soft-delete an entity_attachments reference.
|
||||
|
||||
|
||||
The DMS file is NOT deleted because other entities may reference it.
|
||||
DMS file cleanup happens via DMS's own deletion workflow.
|
||||
"""
|
||||
|
||||
Reference in New Issue
Block a user