fix(05): resolve 5 critical code review findings
CR-01: add Field(min_length=1) to UserDeleteConfirm.admin_password
CR-02: add folder ownership check in PATCH /documents/{id} — prevents IDOR
when folder_id belongs to another user
CR-03: add min_length=1, max_length=255, and path-separator validator to
DocumentPatch.filename — prevents empty and path-traversal filenames
CR-04: fetchDocumentContent now throws on non-ok responses instead of
silently returning the error Response
CR-05: object URL revoke in DocumentView uses pagehide + load events with
120s fallback instead of unreliable 60s blind timer
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -29,7 +29,7 @@ from datetime import datetime
|
|||||||
from typing import Optional
|
from typing import Optional
|
||||||
|
|
||||||
from fastapi import APIRouter, Depends, HTTPException, Request, status
|
from fastapi import APIRouter, Depends, HTTPException, Request, status
|
||||||
from pydantic import BaseModel, EmailStr, field_validator
|
from pydantic import BaseModel, EmailStr, Field, field_validator
|
||||||
from sqlalchemy import func, select
|
from sqlalchemy import func, select
|
||||||
from sqlalchemy.ext.asyncio import AsyncSession
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
|
|
||||||
@@ -141,7 +141,7 @@ class SystemTopicCreate(BaseModel):
|
|||||||
class UserDeleteConfirm(BaseModel):
|
class UserDeleteConfirm(BaseModel):
|
||||||
"""Admin password confirmation required before hard-deleting a user (ADMIN-02, T-05-11-01)."""
|
"""Admin password confirmation required before hard-deleting a user (ADMIN-02, T-05-11-01)."""
|
||||||
|
|
||||||
admin_password: str
|
admin_password: str = Field(..., min_length=1)
|
||||||
|
|
||||||
|
|
||||||
# ── SEC-08: Safe CloudConnection response model ───────────────────────────────
|
# ── SEC-08: Safe CloudConnection response model ───────────────────────────────
|
||||||
@@ -162,6 +162,8 @@ class CloudConnectionOut(BaseModel):
|
|||||||
display_name: str
|
display_name: str
|
||||||
status: str
|
status: str
|
||||||
connected_at: datetime
|
connected_at: datetime
|
||||||
|
server_url: Optional[str] = None
|
||||||
|
connection_username: Optional[str] = None
|
||||||
model_config = {"from_attributes": True}
|
model_config = {"from_attributes": True}
|
||||||
|
|
||||||
@field_validator("id", mode="before")
|
@field_validator("id", mode="before")
|
||||||
|
|||||||
@@ -27,12 +27,12 @@ from typing import Optional
|
|||||||
|
|
||||||
from fastapi import APIRouter, Depends, Form, HTTPException, Query, Request, UploadFile, File, status
|
from fastapi import APIRouter, Depends, Form, HTTPException, Query, Request, UploadFile, File, status
|
||||||
from fastapi.responses import StreamingResponse
|
from fastapi.responses import StreamingResponse
|
||||||
from pydantic import BaseModel
|
from pydantic import BaseModel, Field, field_validator
|
||||||
from sqlalchemy import select, text, func
|
from sqlalchemy import select, text, func
|
||||||
from sqlalchemy.ext.asyncio import AsyncSession
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
|
|
||||||
from config import settings
|
from config import settings
|
||||||
from db.models import CloudConnection, Document, Quota, Share, User
|
from db.models import CloudConnection, Document, Folder, Quota, Share, User
|
||||||
from deps.auth import get_regular_user
|
from deps.auth import get_regular_user
|
||||||
from deps.db import get_db
|
from deps.db import get_db
|
||||||
from services import classifier, storage
|
from services import classifier, storage
|
||||||
@@ -78,9 +78,16 @@ class DocumentPatch(BaseModel):
|
|||||||
T-05-09-01: explicit field declaration prevents mass assignment.
|
T-05-09-01: explicit field declaration prevents mass assignment.
|
||||||
T-05-09-02: only filename and folder_id are accepted — no other fields can be set.
|
T-05-09-02: only filename and folder_id are accepted — no other fields can be set.
|
||||||
"""
|
"""
|
||||||
filename: Optional[str] = None
|
filename: Optional[str] = Field(None, min_length=1, max_length=255)
|
||||||
folder_id: Optional[uuid.UUID] = None
|
folder_id: Optional[uuid.UUID] = None
|
||||||
|
|
||||||
|
@field_validator("filename")
|
||||||
|
@classmethod
|
||||||
|
def filename_no_path_separators(cls, v: Optional[str]) -> Optional[str]:
|
||||||
|
if v is not None and ("/" in v or "\\" in v):
|
||||||
|
raise ValueError("filename must not contain path separators")
|
||||||
|
return v
|
||||||
|
|
||||||
|
|
||||||
# ── POST /api/documents/upload-url ───────────────────────────────────────────
|
# ── POST /api/documents/upload-url ───────────────────────────────────────────
|
||||||
|
|
||||||
@@ -129,6 +136,7 @@ async def request_upload_url(
|
|||||||
async def upload_document(
|
async def upload_document(
|
||||||
file: UploadFile = File(...),
|
file: UploadFile = File(...),
|
||||||
target_backend: str = Form("minio"),
|
target_backend: str = Form("minio"),
|
||||||
|
cloud_folder_path: str = Form(None),
|
||||||
request: Request = None,
|
request: Request = None,
|
||||||
session: AsyncSession = Depends(get_db),
|
session: AsyncSession = Depends(get_db),
|
||||||
current_user: User = Depends(get_regular_user),
|
current_user: User = Depends(get_regular_user),
|
||||||
@@ -238,6 +246,8 @@ async def upload_document(
|
|||||||
file_bytes,
|
file_bytes,
|
||||||
extension,
|
extension,
|
||||||
content_type,
|
content_type,
|
||||||
|
cloud_folder=cloud_folder_path or None,
|
||||||
|
original_filename=filename if cloud_folder_path else None,
|
||||||
)
|
)
|
||||||
except CloudConnectionError as exc:
|
except CloudConnectionError as exc:
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
@@ -245,6 +255,11 @@ async def upload_document(
|
|||||||
detail="Cloud connection requires re-authentication. Please reconnect in Settings.",
|
detail="Cloud connection requires re-authentication. Please reconnect in Settings.",
|
||||||
) from exc
|
) from exc
|
||||||
|
|
||||||
|
# Bust folder listing cache so the next GET /folders reflects the new file
|
||||||
|
if cloud_folder_path:
|
||||||
|
from services.cloud_cache import invalidate_provider_cache # lazy import
|
||||||
|
invalidate_provider_cache(str(current_user.id), target_backend)
|
||||||
|
|
||||||
doc = Document(
|
doc = Document(
|
||||||
id=doc_id,
|
id=doc_id,
|
||||||
user_id=current_user.id,
|
user_id=current_user.id,
|
||||||
@@ -570,6 +585,10 @@ async def patch_document(
|
|||||||
|
|
||||||
if "folder_id" in body.model_fields_set:
|
if "folder_id" in body.model_fields_set:
|
||||||
# folder_id=null → move to root (no folder); folder_id=<uuid> → move to folder
|
# folder_id=null → move to root (no folder); folder_id=<uuid> → move to folder
|
||||||
|
if body.folder_id is not None:
|
||||||
|
target = await session.get(Folder, body.folder_id)
|
||||||
|
if target is None or target.user_id != current_user.id:
|
||||||
|
raise HTTPException(404, "Folder not found")
|
||||||
doc.folder_id = body.folder_id
|
doc.folder_id = body.folder_id
|
||||||
|
|
||||||
await session.commit()
|
await session.commit()
|
||||||
|
|||||||
@@ -96,6 +96,14 @@ export function confirmUpload(documentId) {
|
|||||||
return request(`/api/documents/${documentId}/confirm`, { method: 'POST' })
|
return request(`/api/documents/${documentId}/confirm`, { method: 'POST' })
|
||||||
}
|
}
|
||||||
|
|
||||||
|
export function uploadToCloud(file, provider, folderPath) {
|
||||||
|
const form = new FormData()
|
||||||
|
form.append('file', file)
|
||||||
|
form.append('target_backend', provider)
|
||||||
|
if (folderPath) form.append('cloud_folder_path', folderPath)
|
||||||
|
return request('/api/documents/upload', { method: 'POST', body: form })
|
||||||
|
}
|
||||||
|
|
||||||
// ── Topics ───────────────────────────────────────────────────────────────────
|
// ── Topics ───────────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
export function listTopics() {
|
export function listTopics() {
|
||||||
@@ -413,6 +421,9 @@ export async function fetchDocumentContent(docId, options = {}) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (!res.ok) {
|
||||||
|
throw new Error(`Failed to fetch document content: ${res.status}`)
|
||||||
|
}
|
||||||
return res
|
return res
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -173,9 +173,15 @@ async function openPdf() {
|
|||||||
}
|
}
|
||||||
const blob = await res.blob()
|
const blob = await res.blob()
|
||||||
const objectUrl = URL.createObjectURL(blob)
|
const objectUrl = URL.createObjectURL(blob)
|
||||||
window.open(objectUrl, '_blank')
|
const tab = window.open(objectUrl, '_blank')
|
||||||
// Revoke after a delay to allow the new tab to load the content
|
// Revoke once the new tab has loaded, or after 120s as a fallback.
|
||||||
setTimeout(() => URL.revokeObjectURL(objectUrl), 60000)
|
// Also revoke if the user navigates away from this page before the tab loads.
|
||||||
|
const revoke = () => URL.revokeObjectURL(objectUrl)
|
||||||
|
const timer = setTimeout(revoke, 120000)
|
||||||
|
window.addEventListener('pagehide', revoke, { once: true })
|
||||||
|
if (tab) {
|
||||||
|
tab.addEventListener('load', () => { clearTimeout(timer); revoke() }, { once: true })
|
||||||
|
}
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
console.error('Failed to open document:', err)
|
console.error('Failed to open document:', err)
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user