Skip to content

fix(files): honor AsyncCursorPage returned by post_call_success_hook in list_files - #28958

Closed
oss-agent-shin wants to merge 3 commits into
BerriAI:litellm_oss_agent_shin_daily_branchfrom
oss-agent-shin:shin/lit-3386-list-files-async-cursor-page
Closed

fix(files): honor AsyncCursorPage returned by post_call_success_hook in list_files#28958
oss-agent-shin wants to merge 3 commits into
BerriAI:litellm_oss_agent_shin_daily_branchfrom
oss-agent-shin:shin/lit-3386-list-files-async-cursor-page

Conversation

@oss-agent-shin

@oss-agent-shin oss-agent-shin commented May 27, 2026

Copy link
Copy Markdown
Contributor

Problem

The managed-files hook (enterprise/litellm_enterprise/proxy/hooks/managed_files.py::async_post_call_success_hook) returns an AsyncCursorPage for GET /v1/files responses with data filtered to the files the calling user actually owns. Prior to this PR, the list_files endpoint only honored the hook return value when it was an OpenAIFileObject:

_response = await proxy_logging_obj.post_call_success_hook(...)
if _response is not None and isinstance(_response, OpenAIFileObject):  # ← misses AsyncCursorPage
    response = _response

isinstance(AsyncCursorPage_instance, OpenAIFileObject) is always False, so the hook's return value is silently discarded. The hook also mutates response.data in place inside the AsyncCursorPage branch (managed_files.py:1228), which partially masks the bug today, but the type check is still wrong and would break the moment any hook returns a freshly-constructed page object — which is exactly what happens when ownership filtering produces an empty or different data list and the hook chooses to allocate a new page.

This is the remaining bug from #28294 — the related issue also covers Fix 1 (raw output_file_id → managed ID conversion in CheckBatchCost), which is already in place after #27984.

Fix

Broaden the isinstance check to (OpenAIFileObject, AsyncCursorPage) in litellm/proxy/openai_files_endpoints/files_endpoints.py::list_files, and add AsyncCursorPage to the imports from litellm.types.llms.openai. Added a NOTE comment explaining the masking behavior so future readers do not “simplify” the tuple back.

The narrow check on the file-create path (line ~524, returning a single OpenAIFileObject) is left alone — that hook contract returns OpenAIFileObject, not a page.

Evidence

Single regression test that exercises the real /v1/files HTTP path with TestClient, stubs the provider list response with two OpenAIFileObjects, and stubs the post_call_success_hook to return a fresh AsyncCursorPage containing only the owned file.

BEFORE — test fails on un-patched code

> assert returned_ids == ["file-owned-by-user"], (
        f"expected only owned file id, got {returned_ids}"
    )
E AssertionError: expected only owned file id, got ['file-leaked-raw', 'file-owned-by-user']
E assert ['file-leaked...wned-by-user'] == ['file-owned-by-user']
E   At index 0 diff: 'file-leaked-raw' != 'file-owned-by-user'
E   Left contains one more item: 'file-owned-by-user'
FAILED tests/test_litellm/proxy/openai_files_endpoint/test_files_endpoint.py::test_list_files_uses_async_cursor_page_returned_by_post_call_hook
============================== 1 failed in 11.05s ==============================

The raw provider file ID (file-leaked-raw) bypassed the hook and leaked into the response.

AFTER — test passes with the fix

tests/test_litellm/proxy/openai_files_endpoint/test_files_endpoint.py::test_list_files_uses_async_cursor_page_returned_by_post_call_hook PASSED [100%]

============================== 1 passed in 9.97s ===============================

Only the user-owned file is returned.

Full file_endpoint suite still green

collected 28 items

tests/test_litellm/proxy/openai_files_endpoint/test_files_endpoint.py ss [  7%]
..........................                                               [100%]

======================== 26 passed, 2 skipped in 9.47s =========================

(The 2 skipped tests are pre-existing skips for test_create_file_and_call_chat_completion_e2e and test_create_file_for_each_model that require live OpenAI credentials — unrelated to this PR.)

Note on PR plumbing

This PR was pushed via the GitHub Contents API (one PUT per file) because the agent's token lacks workflow scope for git push / update-branch. The diff is minimal: two files, +99/-2.

Refs

Verification (ship-pr)

  • Behavioral fix lives in production code path (not a test-only or comment-only change): litellm/proxy/openai_files_endpoints/files_endpoints.py::list_files — broadened isinstance tuple changes runtime behavior.
  • Regression test added that fails on un-patched code and passes on patched code (verified by reverting/re-applying the single isinstance line in sandbox before filing; before/after pytest output included in the Evidence section above).
  • Full existing test_files_endpoint.py suite still green (26 passed, 2 skipped — same skip count as main).
  • CI: 44/44 checks green (all GitHub Actions, semgrep, code-quality, lint, build-ui, secret-scan, plus all integration & proxy unit-test suites).
  • Greptile: 5/5 confidence on commit 0955d24 after addressing the P2 pyright-suppression scope comment.
  • Veria AI - PR Review: success.
  • mergeable_state: clean.
  • No secrets touched; diff is +25/-3 across 2 files.
  • Linear LIT-3386 updated with PR link.

…in list_files

The managed-files hook returns an AsyncCursorPage for GET /v1/files
responses with the data list filtered to the files the calling user
owns. Before this change the list_files endpoint only honored the hook
return value when it was an OpenAIFileObject, so a freshly-constructed
page object was silently dropped and the unfiltered raw provider listing
was returned. Refs: LIT-3386, BerriAI#28294
…st_files

Asserts that when the post_call_success_hook returns a fresh
AsyncCursorPage filtering out files the user does not own, the
list_files endpoint uses that filtered page and does not leak the raw
provider listing back to the caller. Refs: LIT-3386
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@greptile-apps

greptile-apps Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Fixes a silent data-leak in list_files where the post_call_success_hook return value was discarded whenever the hook returned an AsyncCursorPage (the managed-files ownership-filter case), causing the unfiltered raw provider listing to reach the caller. The fix broadens a single isinstance check and imports AsyncCursorPage directly from openai.pagination, removing the need for a broad pyright-suppression comment on the shared import block.

  • files_endpoints.py: isinstance(_response, OpenAIFileObject)isinstance(_response, (OpenAIFileObject, AsyncCursorPage)); AsyncCursorPage is now imported from openai.pagination directly instead of through litellm.types.llms.openai, keeping the remaining symbols on that import statement fully type-checked.
  • test_files_endpoint.py: New regression test exercises the full /v1/files HTTP path via TestClient, stubs the hook to return a freshly-constructed filtered page, and asserts only the owned file ID reaches the response body.

Confidence Score: 5/5

Safe to merge — the change is minimal, targeted, and covered by a new regression test that directly demonstrates the previously-leaking file IDs are no longer returned.

The two-file diff makes a single logical change: widening one isinstance tuple so the hook's filtered page is honoured instead of dropped. The previous pyright-suppression concern is also resolved by importing AsyncCursorPage directly from openai.pagination. The regression test exercises the exact failure mode described in the issue and the full existing suite remains green.

No files require special attention.

Important Files Changed

Filename Overview
litellm/proxy/openai_files_endpoints/files_endpoints.py Broadens isinstance check in list_files to accept AsyncCursorPage alongside OpenAIFileObject, and imports AsyncCursorPage directly from openai.pagination (resolving the previous pyright-suppression concern).
tests/test_litellm/proxy/openai_files_endpoint/test_files_endpoint.py Adds a regression test that stubs afile_list to return two files and the post_call_success_hook to return a filtered AsyncCursorPage, asserting only the owned file reaches the caller.

Reviews (2): Last reviewed commit: "refactor(files): narrow pyright suppress..." | Re-trigger Greptile

@codecov

codecov Bot commented May 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

…directly from openai.pagination

Addresses Greptile P2: avoid suppressing future type errors on the unrelated symbols in the litellm.types.llms.openai import block. Refs: LIT-3386
@oss-agent-shin

Copy link
Copy Markdown
Contributor Author

@greptileai review

Addressed P2 from previous review: narrowed the pyright suppression by importing AsyncCursorPage directly from openai.pagination, so the litellm.types.llms.openai import block no longer carries a broad # pyright: ignore[reportAttributeAccessIssue] comment. The other symbols on that block are fully checked again.

The behavioral fix (broadened isinstance tuple) is unchanged and the regression test still passes.

@oss-agent-shin

Copy link
Copy Markdown
Contributor Author

Closing — bulk cleanup of PRs filed by this account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants