Make WordPress Core

Opened 3 days ago

Last modified 2 days ago

#66484 new defect (bug)

WP_Http_Streams writes response headers into streamed files when headers span more than one read

Reported by: dpknauss Owned by:
Priority: normal Milestone: Awaiting Review
Component: HTTP API Version: trunk
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses:

Description (last modified by dpknauss)

Problem

In streamed mode, WP_Http_Streams::request() writes every block it reads to the destination file, including blocks read while it is still accumulating the response headers. Only the block that completes the \r\n\r\n header/body separator is replaced with the parsed body. Every header byte read in earlier blocks stays in the file.

This happens whenever the headers arrive over more than one fread():

  • Headers larger than the 4,096-byte read size, wherever the separator falls.
  • A separator that straddles the 4,096-byte boundary.
  • Small headers delivered in more than one socket read. fread() on a socket returns whatever is available, so a slow server, a proxy, or TLS record boundaries can split the headers.

The response is still parsed correctly and returns status 200, so the corrupted file is not reported as an error.

WP_Http_Streams has been deprecated since 6.4, and WP_Http no longer uses it, so this only affects code that calls the transport directly with 'stream' => true. It is present on trunk and was found while testing ​PR 14288 for #65817. That PR does not introduce or change it.

Reproduction

Serve each raw response below from a local socket and request it directly through WP_Http_Streams with:

array(
    'stream'   => true,
    'filename' => $filename,
    'timeout'  => 5,
)

Results on trunk (a8f43c520d), PHP 8.1. Each response has the body hello:

Raw response Status File size File contents
Small headers, one write (control) 200 5 hello
Separator at byte 4,094 (straddles the read boundary) 200 4,101 4,096 header bytes + hello
Separator at byte 6,000 200 4,101 4,096 header bytes + hello
Separator at byte 10,000 200 8,197 8,192 header bytes + hello
HTTP/1.0 200 OK\r\nContent-Type: text/plain, a 200 ms pause, then \r\n\r\nhello 200 46 41 header bytes + hello

The ​PR 14288 head (da0980d) gives identical results.

The straddling case makes a deterministic PHPUnit regression test, because no single read can contain a separator that ends at byte 4,098:

$prefix       = "HTTP/1.0 200 OK\r\nX-Fill: ";
$raw_response = $prefix . str_repeat( 'a', 4094 - strlen( $prefix ) ) . "\r\n\r\nhello";

Expected result

The destination file contains only the response body (hello).

Suggested fix

Write nothing to the file until the headers have been parsed, then write only the body part of the block that completes them. Regression tests should cover the straddling case and headers larger than 4 KB.

AI assistance: Yes
Tool(s): OpenAI Codex, Claude Code
Model(s): GPT-6.1, Claude Opus 5.5
Used for: Reviewing the transport, building and running the reproductions, and drafting this description; the reproductions and results were reviewed by me.

Change History (2)

#1 @dpknauss
2 days ago

  • Description modified (diff)
  • Summary WP_Http_Streams writes partial headers to streamed files when the separator crosses a read boundary → WP_Http_Streams writes response headers into streamed files when headers span more than one read

This ticket was mentioned in ​PR #15487 on ​WordPress/wordpress-develop by ​@sarequl.


2 days ago
#2

  • Keywords has-patch has-unit-tests added

Fixes response headers being written into streamed files when the headers span more than one read.

See https://core.trac.wordpress.org/ticket/66484

What

WP_Http_Streams::request() in streamed mode writes each fread() block to the destination file, even while the response headers are still accumulating. Only the block that completes the \r\n\r\n header/body separator is replaced with the parsed body, so when the separator straddles the 4,096-byte read boundary, header bytes leak into the downloaded file. The response still parses as 200, so the corruption is silent.

Why

The headers can arrive over more than one fread(): headers larger than the 4,096-byte read size, a separator straddling the boundary, or small headers split across socket reads. In every case the earlier header bytes stay in the file.

How

Before the headers are complete, the block is skipped instead of written. Once \r\n\r\n is present, WP_Http::processResponse() runs and only the body portion of the completing block is written. The non-streaming branch is untouched; WP_Http_Streams is deprecated since 6.4 and WP_Http no longer uses it, so the blast radius is direct callers with 'stream' => true.

Testing Instructions

  1. Run the streams tests: vendor/bin/phpunit --group external-http tests/phpunit/tests/http/streams.php → OK (29 tests, 66 assertions). The class is annotated @group external-http, which phpunit.xml.dist excludes by default; without --group external-http PHPUnit reports "No tests executed!" (exit 0) — a false green.
  2. The two new tests (test_file_stream_headers_span_multiple_reads and test_file_stream_header_separator_straddles_read_boundary) fail on the base commit with the full header block plus hello in the destination file, and pass with this change.
  3. Curl transport regression: vendor/bin/phpunit --group external-http tests/phpunit/tests/http/curl.php → OK (27 tests, 60 assertions).
  4. Independent boundary probe: with fill = 3925, the header block is 4,098 bytes and the separator straddles the 4,096-byte read boundary. The base commit writes a 4,101-byte file (headers + hello); this branch writes a 5-byte file (hello).
  5. composer lint reports only pre-existing violations; composer phpstan has a pre-existing config error (DisallowedBacktickRule not found); npm run build passes.
Note: See TracTickets for help on using tickets.