From 6a73279c0c3745c3327f374bedee632c5d6bc96b Mon Sep 17 00:00:00 2001 From: mountain Date: Tue, 7 Jul 2026 10:05:38 +0800 Subject: [PATCH] fix: patch three critical defects from the SDK review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - local delete_collection: validate the collection name before rmtree. An unvalidated name like "../.." escaped files_dir and deleted arbitrary directories (path traversal). - legacy page_index(): restore the node_id/summary/text/description enhancements. IndexConfig now carries booleans (pydantic coerces the legacy 'yes'/'no' strings at the boundary), but page_index_main still compared `opt.if_add_node_id == 'yes'` — always False — so every enhancement was silently skipped for legacy-API callers. Conditions now branch on the booleans, matching pageindex/index/page_index.py. - LegacyCloudAPI._request: bound every request with a timeout (30s, 120s read timeout for streamed responses) so a dead connection can't hang legacy submit/poll/chat callers forever. The legacy contract tests pinned the missing timeout; updated to pin its presence instead. Adds regression tests: path-traversal rejection, 'yes'/'no' -> bool coercion, and timeout assertions. Claude-Session: https://claude.ai/code/session_01Kx5DgKbhK1N8autqXH8SmS --- pageindex/backend/local.py | 3 +++ pageindex/cloud_api.py | 4 ++++ pageindex/page_index.py | 17 ++++++++++------- tests/test_config.py | 8 ++++++++ tests/test_legacy_sdk_contract.py | 5 +++-- tests/test_local_backend.py | 10 ++++++++++ 6 files changed, 38 insertions(+), 9 deletions(-) diff --git a/pageindex/backend/local.py b/pageindex/backend/local.py index cfbd288..812be1e 100644 --- a/pageindex/backend/local.py +++ b/pageindex/backend/local.py @@ -58,6 +58,9 @@ class LocalBackend: return self._storage.list_collections() def delete_collection(self, name: str) -> None: + # Validate before touching the filesystem — an unvalidated name like + # "../.." would make the rmtree below escape files_dir entirely. + self._validate_collection_name(name) self._storage.delete_collection(name) col_dir = self._files_dir / name if col_dir.exists(): diff --git a/pageindex/cloud_api.py b/pageindex/cloud_api.py index b4011aa..303702a 100644 --- a/pageindex/cloud_api.py +++ b/pageindex/cloud_api.py @@ -21,6 +21,10 @@ class LegacyCloudAPI: return {"api_key": self.api_key} def _request(self, method: str, path: str, error_prefix: str, **kwargs) -> requests.Response: + # Always bound the request so a dead connection can't hang callers + # forever. Streamed responses get a longer read timeout since it + # applies between chunks, not to the whole response. + kwargs.setdefault("timeout", 120 if kwargs.get("stream") else 30) try: response = requests.request( method, diff --git a/pageindex/page_index.py b/pageindex/page_index.py index fab2283..911cc27 100644 --- a/pageindex/page_index.py +++ b/pageindex/page_index.py @@ -1081,17 +1081,20 @@ def page_index_main(doc, opt=None): async def page_index_builder(): structure = await tree_parser(page_list, opt, doc=doc, logger=logger) - if opt.if_add_node_id == 'yes': - write_node_id(structure) - if opt.if_add_node_text == 'yes': + # IndexConfig fields are booleans (pydantic coerces legacy 'yes'/'no' + # strings at the boundary) — comparing against 'yes' here would be + # always-False and silently skip every enhancement. + if opt.if_add_node_id: + write_node_id(structure) + if opt.if_add_node_text: add_node_text(structure, page_list) - if opt.if_add_node_summary == 'yes': - if opt.if_add_node_text == 'no': + if opt.if_add_node_summary: + if not opt.if_add_node_text: add_node_text(structure, page_list) await generate_summaries_for_structure(structure, model=opt.model) - if opt.if_add_node_text == 'no': + if not opt.if_add_node_text: remove_structure_text(structure) - if opt.if_add_doc_description == 'yes': + if opt.if_add_doc_description: # Create a clean structure without unnecessary fields for description generation clean_structure = create_clean_structure_for_description(structure) doc_description = generate_doc_description(clean_structure, model=opt.model) diff --git a/tests/test_config.py b/tests/test_config.py index be3b003..db6b73e 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -26,3 +26,11 @@ def test_model_copy_with_update(): updated = config.model_copy(update={"model": "gpt-5.4"}) assert updated.model == "gpt-5.4" assert updated.toc_check_page_num == 30 + + +def test_legacy_yes_no_strings_coerce_to_bool(): + """Legacy page_index()/run_pageindex callers pass 'yes'/'no' strings; + pydantic must coerce them to the booleans the pipeline now branches on.""" + config = IndexConfig(if_add_node_id="yes", if_add_node_summary="no") + assert config.if_add_node_id is True + assert config.if_add_node_summary is False diff --git a/tests/test_legacy_sdk_contract.py b/tests/test_legacy_sdk_contract.py index 65c9bdb..12b76e5 100644 --- a/tests/test_legacy_sdk_contract.py +++ b/tests/test_legacy_sdk_contract.py @@ -107,7 +107,7 @@ def test_submit_document_uses_legacy_endpoint(monkeypatch, tmp_path): assert calls[0]["method"] == "POST" assert calls[0]["url"] == "https://api.pageindex.ai/doc/" assert calls[0]["headers"] == {"api_key": "pi-test"} - assert "timeout" not in calls[0]["kwargs"] + assert calls[0]["kwargs"]["timeout"] == 30 assert calls[0]["data"]["if_retrieval"] is True assert calls[0]["data"]["mode"] == "mcp" assert calls[0]["data"]["beta_headers"] == '["block_reference"]' @@ -214,7 +214,8 @@ def test_chat_completions_stream_parses_text_chunks(monkeypatch): )) assert chunks == ["hel", "lo"] - assert "timeout" not in calls[0]["kwargs"] + # Streamed requests still get a (longer, between-chunk) read timeout. + assert calls[0]["kwargs"]["timeout"] == 120 def test_chat_completions_stream_metadata_returns_raw_chunks(monkeypatch): diff --git a/tests/test_local_backend.py b/tests/test_local_backend.py index 0155cdb..c4d6c11 100644 --- a/tests/test_local_backend.py +++ b/tests/test_local_backend.py @@ -155,3 +155,13 @@ def test_delete_document_missing_raises(backend): backend.get_or_create_collection("papers") with pytest.raises(DocumentNotFoundError, match="ghost"): backend.delete_document("papers", "ghost") + + +def test_delete_collection_rejects_path_traversal(backend, tmp_path): + # Regression: an unvalidated name like "../.." would rmtree outside files_dir. + from pageindex.errors import PageIndexError + canary = tmp_path / "canary.txt" + canary.write_text("still here") + with pytest.raises(PageIndexError, match="Invalid collection name"): + backend.delete_collection("../..") + assert canary.exists()