Skip to content

Check that a Range response is the requested window in HTTP cat_file - #2247

Open
feiiiiii5 wants to merge 1 commit into
fsspec:masterfrom
feiiiiii5:fix/http-cat-file-range-window
Open

feiiiiii5 wants to merge 1 commit into
fsspec:masterfrom
feiiiiii5:fix/http-cat-file-range-window

Conversation

@feiiiiii5

Copy link
Copy Markdown
Contributor

cat_file on the HTTP filesystem does not check that the response is the window that was asked for, so against a server that ignores Range — python -m http.server, many proxies — the whole file comes back instead:

>>> len(h.cat_file(url, start=10, end=20))
1024          # the file is 1024 bytes
>>> h.cat_ranges([url, url], [10, 300], [20, 310])[0] == h.cat_ranges([url, url], [10, 300], [20, 310])[1]
True

A consumer that walks a file with cat_ranges gets the same bytes at every offset, so everything downstream of it is silently misaligned. Both the async and the sync filesystem do this: HTTPFileSystem._cat_file returns the body as read, and HTTPFileSystem.cat_file in http_sync.py returns r.content.

The streaming file interface already validates the response for the same request — async_fetch_range checks for 206, a matching Content-Range start, or a Content-Length no larger than the window (#1140 fixed that path). This PR shares that check and uses it in both cat_file implementations, so the two paths can no longer disagree:

  • a 206 response, or a body no longer than the window, is returned unchanged;
  • a longer body is the whole file, which starts at its beginning, so the window is read out of it — the prefix for a window starting at 0, the tail for a suffix range;
  • a window that starts inside the file raises ValueError, the same one the file interface raises, because a body longer than the window that starts at the requested offset cannot be told apart from one that starts at 0.

test_cat_file_range on a range-honouring server still passes unchanged, which is the point: the contract there was already right, only the unhonoured case was missing.

Test: pytest fsspec/implementations/tests/test_http.py fsspec/implementations/tests/test_http_sync.py — the four new cases (two server configurations × async and sync) fail on master with the whole body coming back and pass on this branch; the full suite is 2160 passed, 251 skipped, 2 xfailed. ruff check and ruff format (0.14.3, the pinned version) are clean on the five changed files.

Not addressed here: a window whose two bounds have opposite signs, like start=-8, end=1000, needs the file size to resolve, and is passed through as before. #2124 also touches cat_file but adds retries, a different root cause.

A server that ignores the Range header replies with the whole file instead of
the window that was asked for, and both the async and the sync HTTP cat_file
handed that back as if it were the window, so a caller walking a file with
cat_ranges got the same bytes at every offset. The streaming file interface
already validates the response for the same request; share that check, and read
the window out of the whole body when the window starts at the beginning of the
file.
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.

1 participant