fix: validate folder_id ownership on chat create + folder-update endpoints (#24588)
POST /api/v1/chats/new and POST /api/v1/chats/{id}/folder accepted a
caller-supplied folder_id with no validation — neither ownership, nor
existence, nor UUID format. The row was persisted with the supplied
value verbatim, so the DB ended up with chat rows whose folder_id
referenced another user's folder, a non-existent UUID, or even a
non-UUID string.
No read path surfaces this across users — every chat-folder read is
user_id-filtered on both sides — so this is referential-integrity
hardening rather than a security boundary fix. But there's no reason
to accept dangling references either, and the downstream consumers
shouldn't have to assume the column is clean.
Add a Folders.get_folder_by_id_and_user_id() lookup at both writers:
if a folder_id is supplied, it must match a folder owned by the
caller. None remains allowed (chat-without-folder is the default).
Non-existent and non-UUID values fall through to 404.
Reported by ShigekiTsuchiyama in GHSA-4vrg-2vcq-q7jc.
Co-authored-by: ShigekiTsuchiyama <ShigekiTsuchiyama@users.noreply.github.com>
This commit is contained in:
1 parent
f607337582
commit
ba78ebea27
1 file changed
+21
@@ -557,6 +557,18 @@ async def create_new_chat(
|
|||||||
user=Depends(get_verified_user),
|
user=Depends(get_verified_user),
|
||||||
db: AsyncSession = Depends(get_async_session),
|
db: AsyncSession = Depends(get_async_session),
|
||||||
):
|
):
|
||||||
|
# Reject a folder_id that doesn't belong to the caller. Without this the
|
||||||
|
# row is persisted with a dangling foreign reference — no read path
|
||||||
|
# surfaces it across users (all chat reads are user_id-filtered), but
|
||||||
|
# the row state is meaningless and downstream consumers shouldn't have
|
||||||
|
# to assume the column is clean. Also catches non-UUID / nonexistent IDs.
|
||||||
|
if form_data.folder_id is not None:
|
||||||
|
if not await Folders.get_folder_by_id_and_user_id(form_data.folder_id, user.id, db=db):
|
||||||
|
raise HTTPException(
|
||||||
|
status_code=status.HTTP_404_NOT_FOUND,
|
||||||
|
detail=ERROR_MESSAGES.NOT_FOUND,
|
||||||
|
)
|
||||||
|
|
||||||
try:
|
try:
|
||||||
chat = await Chats.insert_new_chat(str(uuid4()), user.id, form_data, db=db)
|
chat = await Chats.insert_new_chat(str(uuid4()), user.id, form_data, db=db)
|
||||||
return ChatResponse(**chat.model_dump())
|
return ChatResponse(**chat.model_dump())
|
||||||
@@ -1484,6 +1496,15 @@ async def update_chat_folder_id_by_id(
|
|||||||
):
|
):
|
||||||
chat = await Chats.get_chat_by_id_and_user_id(id, user.id, db=db)
|
chat = await Chats.get_chat_by_id_and_user_id(id, user.id, db=db)
|
||||||
if chat:
|
if chat:
|
||||||
|
# Same ownership check as the create path — reject foreign / dangling
|
||||||
|
# folder_id values. None is allowed (moves the chat out of any folder).
|
||||||
|
if form_data.folder_id is not None:
|
||||||
|
if not await Folders.get_folder_by_id_and_user_id(form_data.folder_id, user.id, db=db):
|
||||||
|
raise HTTPException(
|
||||||
|
status_code=status.HTTP_404_NOT_FOUND,
|
||||||
|
detail=ERROR_MESSAGES.NOT_FOUND,
|
||||||
|
)
|
||||||
|
|
||||||
chat = await Chats.update_chat_folder_id_by_id_and_user_id(id, user.id, form_data.folder_id, db=db)
|
chat = await Chats.update_chat_folder_id_by_id_and_user_id(id, user.id, form_data.folder_id, db=db)
|
||||||
return ChatResponse(**chat.model_dump())
|
return ChatResponse(**chat.model_dump())
|
||||||
else:
|
else:
|
||||||
|
|||||||
Reference in new issue
Block a user