fix: patch three critical defects from the SDK review

- 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
This commit is contained in:
mountain 2026-07-07 10:05:38 +08:00
parent 284ce005f5
commit 6a73279c0c
6 changed files with 38 additions and 9 deletions

View file

@ -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():

View file

@ -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,

View file

@ -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)

View file

@ -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

View file

@ -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):

View file

@ -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()