diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 62d8e3f3..8203ef9a 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -34,6 +34,7 @@ from nextcloud_mcp_server.search.context import ( get_chunk_bbox_and_page_from_qdrant, get_chunk_with_context, ) +from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id from nextcloud_mcp_server.vector.oauth_sync import ( NotProvisionedError, get_user_client_basic_auth, @@ -502,8 +503,9 @@ async def get_chunk_context(request: Request) -> JSONResponse: # otherwise pass through to get_chunk_with_context and bottom out as a # 404 from deep inside, not a clear 400. Nextcloud IDs are unsigned # ints from MySQL auto_increment; doc_id stays a str downstream - # (Qdrant payload index is keyword-typed). - if not doc_id.isdigit(): + # (Qdrant payload index is keyword-typed). is_valid_nextcloud_doc_id + # rejects "0", leading zeros, and Unicode digits that pass isdigit(). + if not is_valid_nextcloud_doc_id(doc_id): return JSONResponse( { "success": False, diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index 5c2eee3d..688d03e9 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -37,6 +37,7 @@ from nextcloud_mcp_server.search.context import ( get_chunk_bbox_and_page_from_qdrant, get_chunk_with_context, ) +from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id from nextcloud_mcp_server.vector.oauth_sync import ( NotProvisionedError, get_user_client_basic_auth, @@ -564,8 +565,9 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: # otherwise pass through to get_chunk_with_context and bottom out as a # 404 from deep inside, not a clear 400. Nextcloud IDs are unsigned # ints from MySQL auto_increment; doc_id stays a str downstream - # (Qdrant payload index is keyword-typed). - if not doc_id.isdigit(): + # (Qdrant payload index is keyword-typed). is_valid_nextcloud_doc_id + # rejects "0", leading zeros, and Unicode digits that pass isdigit(). + if not is_valid_nextcloud_doc_id(doc_id): return JSONResponse( { "success": False, diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index 6cd583a7..c233007f 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -11,6 +11,7 @@ from qdrant_client.models import FieldCondition, Filter, MatchValue from nextcloud_mcp_server.client import NextcloudClient from nextcloud_mcp_server.config import get_settings +from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id from nextcloud_mcp_server.vector.html_processor import html_to_markdown from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client @@ -577,10 +578,11 @@ async def _fetch_document_text( """ try: if doc_type == "note": - # Note IDs are integers in the Nextcloud API; reject non-numeric - # doc_ids explicitly so a malformed payload surfaces in logs - # rather than getting silently swallowed by `except Exception`. - if not doc_id.isdigit(): + # Note IDs are positive ASCII integers (MySQL AUTO_INCREMENT). + # is_valid_nextcloud_doc_id rejects "0", leading zeros, and Unicode + # digits that pass str.isdigit(); a malformed payload surfaces in + # logs rather than getting silently swallowed by `except Exception`. + if not is_valid_nextcloud_doc_id(doc_id): logger.warning( "Expected numeric note doc_id, got %r — skipping document fetch", doc_id, @@ -594,10 +596,11 @@ async def _fetch_document_text( content = note.get("content", "") return f"{title}\n\n{content}" elif doc_type == "news_item": - # News item IDs are integers in the Nextcloud News API; reject - # non-numeric doc_ids explicitly so malformed payloads surface - # rather than getting swallowed by the broad except below. - if not doc_id.isdigit(): + # News item IDs are positive ASCII integers (MySQL AUTO_INCREMENT). + # is_valid_nextcloud_doc_id rejects "0", leading zeros, and Unicode + # digits that pass str.isdigit(); malformed payloads surface in + # logs rather than getting swallowed by the broad except below. + if not is_valid_nextcloud_doc_id(doc_id): logger.warning( "Expected numeric news_item doc_id, got %r — skipping document fetch", doc_id, @@ -621,12 +624,13 @@ async def _fetch_document_text( content_parts.append(body_markdown) return "\n".join(content_parts) elif doc_type == "deck_card": - # Deck card IDs are integers in the Nextcloud Deck API; reject - # non-numeric doc_ids explicitly so malformed payloads surface - # rather than getting swallowed by the broad except below. The - # numeric check covers both the metadata-fast-path (line ~600) - # and the iteration fallback (line ~635). - if not doc_id.isdigit(): + # Deck card IDs are positive ASCII integers (MySQL AUTO_INCREMENT). + # is_valid_nextcloud_doc_id rejects "0", leading zeros, and Unicode + # digits that pass str.isdigit(); malformed payloads surface in + # logs rather than getting swallowed by the broad except below. + # The numeric check covers both the metadata-fast-path and the + # iteration fallback below. + if not is_valid_nextcloud_doc_id(doc_id): logger.warning( "Expected numeric deck_card doc_id, got %r — skipping document fetch", doc_id, diff --git a/nextcloud_mcp_server/utils/validation.py b/nextcloud_mcp_server/utils/validation.py new file mode 100644 index 00000000..2f9d0287 --- /dev/null +++ b/nextcloud_mcp_server/utils/validation.py @@ -0,0 +1,15 @@ +"""Shared validators for primitive types crossing system boundaries.""" + +import re + +# Nextcloud object IDs are unsigned ints from MySQL AUTO_INCREMENT, which +# starts at 1. Restrict to ASCII positive integers to exclude Unicode digit +# classes (e.g. superscripts, Arabic-Indic numerals) that pass str.isdigit() +# / str.isdecimal() but would never be valid Nextcloud IDs, and to reject "0" +# and leading zeros. +_NEXTCLOUD_DOC_ID_RE = re.compile(r"^[1-9][0-9]*$") + + +def is_valid_nextcloud_doc_id(value: str) -> bool: + """True iff `value` is the str form of a positive ASCII integer (>= 1).""" + return bool(_NEXTCLOUD_DOC_ID_RE.fullmatch(value)) diff --git a/nextcloud_mcp_server/vector/qdrant_client.py b/nextcloud_mcp_server/vector/qdrant_client.py index c4876bc4..b51b74bf 100644 --- a/nextcloud_mcp_server/vector/qdrant_client.py +++ b/nextcloud_mcp_server/vector/qdrant_client.py @@ -204,12 +204,14 @@ def _group_int_doc_ids(points: list[Any]) -> tuple[dict[str, list[Any]], int]: value = payload.get("doc_id") if value is None or isinstance(value, str): continue - if not isinstance(value, int): - # Producers only ever write int or str; anything else is a - # producer bug. Stringifying e.g. a float would write "3.0", - # which producers (str(int)) and the keyword index would - # never match, and which int() on the verification side - # would later reject. Skip and log loudly instead. + # Strict type check: bool is a subclass of int in Python, so an + # `isinstance(value, int)` guard would let `True`/`False` slip + # through and be stringified to `"True"`/`"False"` — which the + # keyword index would never match and the verification side + # would later reject. Producers only ever write int or str; + # anything else (bool, float, etc.) is a producer bug. Skip + # and log loudly instead. + if type(value) is not int: logger.warning( "Unexpected doc_id type %s on point %s; skipping rewrite", type(value).__name__, diff --git a/tests/unit/utils/__init__.py b/tests/unit/utils/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/unit/utils/test_validation.py b/tests/unit/utils/test_validation.py new file mode 100644 index 00000000..8e1f38ed --- /dev/null +++ b/tests/unit/utils/test_validation.py @@ -0,0 +1,54 @@ +"""Unit tests for shared boundary validators.""" + +import pytest + +from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id + + +@pytest.mark.unit +@pytest.mark.parametrize( + "value", + [ + "1", + "42", + "1234567890", + "9999999999999999999", + ], +) +def test_accepts_positive_ascii_integers(value): + """Any positive ASCII integer (no leading zero) is a valid doc_id.""" + assert is_valid_nextcloud_doc_id(value) is True + + +@pytest.mark.unit +@pytest.mark.parametrize( + "value,reason", + [ + ("", "empty string"), + ("0", "MySQL AUTO_INCREMENT starts at 1"), + ("01", "leading zero"), + ("00", "leading zeros"), + ("-1", "negative"), + ("+1", "explicit sign"), + ("1.0", "float-like"), + (" 1", "leading whitespace"), + ("1 ", "trailing whitespace"), + ("1\n", "trailing newline"), + ("abc", "alphabetic"), + ("1a", "trailing letter"), + ("a1", "leading letter"), + # Unicode digit classes that pass str.isdigit() but are not ASCII. + # `²` (U+00B2) is a superscript and would slip past the old guard. + ("²", "Unicode superscript-2"), + # `٢` (U+0662) Arabic-Indic digit two — passes both isdigit() and + # isdecimal(), so only an explicit ASCII regex catches it. + ("٢", "Arabic-Indic digit two"), + # `१` (U+0967) Devanagari digit one — same story. + ("१", "Devanagari digit one"), + # Mixed ASCII + Unicode digits. + ("1٢", "mixed ASCII + Arabic-Indic"), + ], +) +def test_rejects_invalid_doc_ids(value, reason): + """Reject empty/zero/leading-zero/non-ASCII/non-digit inputs.""" + assert is_valid_nextcloud_doc_id(value) is False, f"should reject: {reason}" diff --git a/tests/unit/vector/test_qdrant_client.py b/tests/unit/vector/test_qdrant_client.py index 4c2ebadd..6611aedc 100644 --- a/tests/unit/vector/test_qdrant_client.py +++ b/tests/unit/vector/test_qdrant_client.py @@ -678,6 +678,36 @@ def test_group_int_doc_ids_skips_float_and_warns(caplog): assert "99" in msg +@pytest.mark.unit +def test_group_int_doc_ids_skips_bool_and_warns(caplog): + """A bool doc_id is not stringified to "True"/"False"; it logs and skips. + + ``isinstance(True, int)`` is ``True`` because ``bool`` is a subclass of + ``int`` in Python, so a naive ``isinstance(value, int)`` guard would let + a boolean payload through and write ``str(True)`` → ``"True"`` into + Qdrant. Producers never write bools, but the strict ``type(value) is + int`` guard ensures any future producer bug surfaces as a WARNING and is + not silently stringified. + """ + bool_point = SimpleNamespace(id=33, payload={"doc_id": True}) + int_point = SimpleNamespace(id=42, payload={"doc_id": 7}) + + with caplog.at_level("WARNING", logger="nextcloud_mcp_server.vector.qdrant_client"): + by_value, scanned = _group_int_doc_ids([bool_point, int_point]) + + # Only the int point made it into by_value — "True" is *not* a key. + assert by_value == {"7": [42]} + assert "True" not in by_value + assert "False" not in by_value + assert scanned == 2 + + warnings = [r for r in caplog.records if r.levelname == "WARNING"] + assert len(warnings) == 1 + msg = warnings[0].getMessage() + assert "bool" in msg + assert "33" in msg + + @pytest.mark.unit def test_group_int_doc_ids_handles_str_and_missing_silently(caplog): """str / missing doc_id payloads are skipped without warning.