Skip to content

Harden File construction: explicit factories, strict array shapes, deprecate implicit filesystem access - #308

Merged
jeffpaul merged 4 commits into
WordPress:trunkfrom
saarnilauri:fix/file-secure-by-default
Oct 28, 2026
Merged

jeffpaul merged 4 commits into
WordPress:trunkfrom
saarnilauri:fix/file-secure-by-default

Conversation

@saarnilauri

@saarnilauri saarnilauri commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Hardening of the File DTO and the code paths that construct it from data the SDK did not produce itself (array-shaped input and provider API responses).

Today File::__construct() guesses what a string is by trying, in order, URL, data URI, existing local file path, and plain base64. That makes the constructor's behaviour depend on the filesystem, which is surprising for a DTO and means callers have to be careful about where a string came from. This PR makes the kind of input explicit for the code paths that handle external data, and keeps the generic constructor backward compatible while marking its implicit path detection as deprecated.

Changes

  • File
    • New named constructors: File::fromUrl(), File::fromDataUri(), File::fromBase64() and File::fromLocalPath(). Only fromLocalPath() reads from the filesystem, and it rejects stream wrappers. fromBase64() initialises the inline fields directly, so large payloads are not copied into a data URI and parsed again.
    • The constructor still accepts an http(s) URL, a data URI, plain base64 data with a MIME type, and (deprecated) an existing local file path, in the same precedence as before. Non-http(s) URL schemes are now rejected before any filesystem access, and the local-path branch emits a silenced E_USER_DEPRECATED notice pointing to File::fromLocalPath(). Removal is left for a breaking release.
    • Empty payloads are accepted consistently: new File('', $mime), File::fromBase64('', $mime), an empty data URI and an empty local file all produce an empty inline file that round-trips through toArray() / fromArray().
    • fromArray() now honours fileType: remote requires an http(s) url, inline requires base64 base64Data and a mimeType. A null mimeType is treated as absent (remote files still infer it from the URL extension). Mismatched or wrongly typed keys raise the SDK InvalidArgumentException instead of a TypeError.
    • isArrayShape() is a structural check that mirrors the fromArray() rules without constructing the object.
    • Regexes are anchored with \z / D so trailing newlines are not accepted.
  • MessagePart, Message: structural isArrayShape() overrides and type guards in fromArray(), so shape probing has no side effects and nested files follow the strict File rules. The Message check mirrors the role/part rules of the constructor and, like fromArray(), does not require parts keys to be consecutive.
  • AbstractOpenAiCompatibleImageGenerationModel: result files are built via File::fromUrl() / File::fromBase64(), and invalid or missing values become a ResponseException naming the offending data[i] entry.
  • FileInputUtil::fromString() (new): the resolver behind PromptBuilder::withFile() and MessageBuilder::withFile(). The string form of withFile() keeps accepting URL, data URI, base64 and local path inputs exactly as before; the docblocks now say that local path resolution reads from the filesystem and should only be used with developer-controlled strings.
  • MimeType::isValid(): D modifier.
  • Docs: File factories added to the architecture diagram, a short note on which constructors touch the filesystem, and the traditional-API file example now shows a valid message part shape.

Backward compatibility

  • new File('/existing/path') keeps working and emits one silenced deprecation notice. Use File::fromLocalPath() going forward. The builder methods withFile() are unaffected.
  • new File('file:///path'), new File('ftp://...') and other non-http(s) schemes now throw.
  • File::fromArray() rejects arrays whose fileType does not match the keys present, and inline shapes without a mimeType.
  • Message::isArrayShape() is structural, so a role/part-incompatible message array now fails inside fromArray() with a precise message instead of falling through to the list-of-parts branch in PromptBuilder.
  • The image parser's missing-key error now names data[i] instead of choices[i].

No public signature changes. n.e.x.t placeholders and AiClient::VERSION are left for the release PR.

Testing

  • composer lint and composer test:unit pass (1294 tests).
  • New coverage: the four factories (happy paths and rejections), the deprecated constructor path including an extension-less file, empty payloads, strict fromArray() / isArrayShape() for File, MessagePart and Message (including non-consecutive part keys and null mimeType), the image response parser, FileInputUtil, builder tests confirming withFile() behaviour is unchanged, and a regression test bounding peak memory for large payloads.

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Mythos 5.1 (implementation), GPT-6 Astra (independent review)
Used for: Identifying the finding requiering the hardening, planning for the hardening solution and code / test implementations. Reviews, iterating, tweaking and testing done by me.

File::__construct() no longer treats an existing local path as a file to read.
Local files are loaded only through the explicit File::fromLocalPath() factory.
New factories File::fromUrl(), File::fromDataUri() and File::fromBase64() make the
kind of input explicit and never touch the filesystem.

File::fromArray() now honours fileType: remote files require an http(s) url,
inline files require base64 data and a mimeType; anything else is rejected.
File, MessagePart and Message implement structural isArrayShape() checks, so
shape probing no longer constructs objects (and no longer reads files).

AbstractOpenAiCompatibleImageGenerationModel builds result files only via
File::fromUrl()/fromBase64() and wraps invalid values in ResponseException.

PromptBuilder::withFile() and MessageBuilder::withFile() keep accepting
URL | data URI | base64 | local path strings through the new FileInputUtil,
which rejects non-http(s) schemes before consulting the filesystem.

Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @tunelko, @jiwhan8655.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: tunelko, jiwhan8655.

Co-authored-by: saarnilauri <laurisaarni@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: JasonTheAdams <jason_the_adams@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.


Props Bot commands

You can trigger Props Bot actions by commenting on this PR:

  • @propsbot add <username> to give props to a GitHub user. Requires write access.
  • @propsbot remove <username> to leave a user out of the props. Requires write access.
  • @propsbot refresh to refresh this list. Adding the props-bot label works too.

@codecov

codecov Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.43750% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.12%. Comparing base (77a5995) to head (c698f21).
⚠️ Report is 3 commits behind head on trunk.

Files with missing lines Patch % Lines
src/Files/DTO/File.php 98.14% 2 Missing ⚠️
src/Messages/DTO/Message.php 94.44% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk     #308      +/-   ##
============================================
+ Coverage     86.58%   87.12%   +0.54%     
- Complexity     1383     1463      +80     
============================================
  Files            69       70       +1     
  Lines          4449     4615     +166     
============================================
+ Hits           3852     4021     +169     
+ Misses          597      594       -3     
Flag Coverage Δ
unit 87.12% <98.43%> (+0.54%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@saarnilauri
saarnilauri requested a review from gziolo October 23, 2026 12:50
@jeffpaul jeffpaul added this to the 1.6.0 milestone Oct 23, 2026

@gziolo gziolo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used Codex and Claude to help verify these findings, focusing on backward compatibility implications for public interfaces. The inline comments cover three compatibility concerns and one small error-message inconsistency.

Comment thread src/Files/DTO/File.php
Comment thread src/Files/DTO/File.php
Comment thread src/Messages/DTO/Message.php Outdated
Comment thread src/Files/DTO/File.php Outdated
saarnilauri and others added 2 commits October 26, 2026 12:39
…ayloads

- File::__construct() keeps detecting an existing local file path (after
  rejecting non-http(s) schemes) and emits an E_USER_DEPRECATED notice
  pointing to File::fromLocalPath(); removal is left for a breaking release.
- Empty payloads are supported consistently: an empty data URI round-trips
  through toArray()/fromArray(), File::fromBase64('') and an empty local
  file produce an empty inline file, and withFile('') with a MIME type works
  as before.
- Message::isArrayShape() no longer requires consecutive part keys, matching
  fromArray() and toArray().
- The image response parser reports data[$index] for the missing-key case
  as well.
- Extra MessagePart tests for wrongly typed keys.

Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
- Message::isArrayShape() only accepts the user and model roles and rejects
  user messages with function-call parts and model messages with
  function-response parts, matching fromArray().
- File::isArrayShape()/fromArray() require an inferable MIME type for remote
  entries without one, as before (no I/O involved); a null mimeType is treated
  like an absent one.
- Tests that point at a ".txt" fixture now create it, so they detect a
  regression where the file is actually read.
- Architecture note names the filesystem-free factories and the deprecated
  constructor behaviour concisely.

Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
@saarnilauri saarnilauri changed the title Harden File construction: explicit factories, strict array shapes, no implicit filesystem access Harden File construction: explicit factories, strict array shapes, deprecate implicit filesystem access Oct 26, 2026
@jeffpaul
jeffpaul requested a review from gziolo October 27, 2026 18:58

@JasonTheAdams JasonTheAdams left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One memory regression to address; finding and recommendation inline.

Comment thread src/Files/DTO/File.php Outdated
}

// Routing through the data URI form makes the constructor take a deterministic, filesystem-free path.
return new self(sprintf('data:%s;base64,%s', $mimeType, $base64));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Avoid reparsing the full base64 payload. Wrapping it in a data URI and passing it through the constructor creates several large temporary strings. On PHP 8.4, loading a 24 MiB local file peaked at 60 MiB on the base revision versus 184 MiB here, causing a fatal error with a 128 MiB limit. This also affects existing new File($path, $mimeType) and withFile() calls.

Recommendation: initialize the validated inline fields directly, without creating and reparsing a full data URI.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, thanks. The data URI copy plus the two regex captures of the payload were the cause. I changed it so that fromBase64() now creates an empty inline file via data:<mime>;base64, and assigns the validated payload directly, so no copy of it is embedded or re-parsed. With this change using 24Mib file the peak is back to 58 MiB on all three paths (constructor, fromLocalPath(), fromBase64())

testLargePayloadsDoNotMultiplyPeakMemory with an 8 MiB payload asserts the peak stays bounded.

Embedding the validated payload in a data URI and passing it through the
constructor created several additional copies of a potentially large
string (data URI, regex captures), tripling peak memory for large files on
every path that ends in fromBase64(), including fromLocalPath() and the
deprecated constructor path. Build an empty inline file through the data
URI path instead and attach the payload directly.

Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
@saarnilauri

Copy link
Copy Markdown
Contributor Author

Two other people have also reported the need for the hardening:

@propsbot add tunelko
@propsbot add jiwhan8655

@saarnilauri

Copy link
Copy Markdown
Contributor Author

@propsbot refresh

@gziolo gziolo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. The review feedback has been addressed, including the memory regression.

One small nit: please update the PR description to reflect that local-path construction remains supported with a deprecation notice, and empty payloads are accepted.

@jeffpaul
jeffpaul merged commit 546bcb2 into WordPress:trunk Oct 28, 2026
12 checks passed
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.

4 participants