mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-10-14 02:42:22 +02:00
fix(history): reject falsy non-string message IDs; drop redundant keep_count guard
The or-chain used to resolve before_msg_id / message_id evaluated
falsy non-strings (False, 0) to '' before the isinstance(…, str) check
was reached, so they silently bypassed type validation. Replace the
chain with explicit presence-aware logic that falls through on None or
empty string (preserving alias semantics) but rejects any other
non-string value with HTTP 400.
Also remove the now-tautological 'if keep_count >= 0:' guard, which
is always True after the negativity check introduced in the parent
commit. The DB block is unconditional and indented accordingly.
Tests: add {'before_msg_id': False}, {'before_msg_id': 0},
{'message_id': 0} to the parametrised rejection suite.
36 passed, 0 failed (Python 3.14.4 / pytest 9.1.1).
This commit is contained in:
@@ -397,10 +397,17 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter:
|
||||
raise HTTPException(400, "Request body must be valid JSON")
|
||||
if not isinstance(body, dict):
|
||||
raise HTTPException(400, "Request body must be a JSON object")
|
||||
before_msg_id = body.get("before_msg_id") or body.get("message_id") or ""
|
||||
if not isinstance(before_msg_id, str):
|
||||
# Resolve before_msg_id / message_id alias with explicit presence checks.
|
||||
# None (JSON null) and empty string are both treated as "not provided here"
|
||||
# and fall through to the alias, preserving the original OR-chain semantics.
|
||||
# Any other non-string value (False, 0, True, 123, list, …) is rejected
|
||||
# immediately so falsy non-strings cannot bypass type validation.
|
||||
raw_msg_id = body.get("before_msg_id") if "before_msg_id" in body else None
|
||||
if raw_msg_id is None or raw_msg_id == "":
|
||||
raw_msg_id = body.get("message_id")
|
||||
if raw_msg_id is not None and not isinstance(raw_msg_id, str):
|
||||
raise HTTPException(400, "Message ID must be a string")
|
||||
before_msg_id = before_msg_id.strip()
|
||||
before_msg_id = (raw_msg_id or "").strip()
|
||||
if "keep_count" not in body and not before_msg_id:
|
||||
raise HTTPException(400, "keep_count or before_msg_id required")
|
||||
raw_count = body.get("keep_count", 0)
|
||||
@@ -415,22 +422,22 @@ def setup_history_routes(session_manager, upload_handler=None) -> APIRouter:
|
||||
if keep_count < 0:
|
||||
raise HTTPException(400, "keep_count must be a non-negative integer")
|
||||
deleted_sft_pairs: list[dict[str, str]] = []
|
||||
if keep_count >= 0:
|
||||
db = SessionLocal()
|
||||
try:
|
||||
all_db_messages = db.query(DbChatMessage).filter(
|
||||
DbChatMessage.session_id == session_id
|
||||
).order_by(DbChatMessage.timestamp).all()
|
||||
if before_msg_id:
|
||||
resolved_keep_count = _keep_count_before_message(all_db_messages, before_msg_id)
|
||||
if resolved_keep_count is None:
|
||||
raise HTTPException(404, "Message not found")
|
||||
keep_count = resolved_keep_count
|
||||
for pos, row in enumerate(all_db_messages):
|
||||
row._will_delete_for_sft = pos >= keep_count
|
||||
deleted_sft_pairs = _deleted_sft_pairs_from_db_rows(all_db_messages)
|
||||
finally:
|
||||
db.close()
|
||||
# keep_count is always >= 0 here; the block is unconditional.
|
||||
db = SessionLocal()
|
||||
try:
|
||||
all_db_messages = db.query(DbChatMessage).filter(
|
||||
DbChatMessage.session_id == session_id
|
||||
).order_by(DbChatMessage.timestamp).all()
|
||||
if before_msg_id:
|
||||
resolved_keep_count = _keep_count_before_message(all_db_messages, before_msg_id)
|
||||
if resolved_keep_count is None:
|
||||
raise HTTPException(404, "Message not found")
|
||||
keep_count = resolved_keep_count
|
||||
for pos, row in enumerate(all_db_messages):
|
||||
row._will_delete_for_sft = pos >= keep_count
|
||||
deleted_sft_pairs = _deleted_sft_pairs_from_db_rows(all_db_messages)
|
||||
finally:
|
||||
db.close()
|
||||
result = session_manager.truncate_messages(session_id, keep_count)
|
||||
_remove_deleted_sft_trace_rows(
|
||||
owner=effective_user(request),
|
||||
|
||||
@@ -47,6 +47,7 @@ def saved_messages(database):
|
||||
{"keep_count": 1.9}, {"keep_count": -1}, {"keep_count": "bad"},
|
||||
{"keep_count": []}, {"keep_count": {}},
|
||||
{"before_msg_id": True}, {"before_msg_id": ["message"]},
|
||||
{"before_msg_id": False}, {"before_msg_id": 0}, {"message_id": 0},
|
||||
[], None, "two", 2, False,
|
||||
])
|
||||
def test_invalid_target_preserves_history(history_client, payload):
|
||||
|
||||
Reference in New Issue
Block a user