feat: support font URLs in GPDFAPI::add_pdf_font() - #1692
Draft
jakejackson1 wants to merge 1 commit into
Draft
Conversation
Each of the four font keys now accepts an http(s) URL as well as an
absolute server path, and the two can be mixed within the one call.
Downloads go through the new RemoteFontDownloader, which streams the
response to a temp file with wp_remote_get(). The URL is put through
wp_http_validate_url() *before* the request rather than relying solely on
reject_unsafe_urls: WP applies that flag after the pre_http_request
short-circuit, so a plugin filtering that hook would otherwise bypass the
SSRF check entirely. reject_unsafe_urls is still passed so each redirect
hop is revalidated.
Anything with a scheme is routed to the downloader (and rejected unless
it is http/https) instead of falling through to the local-path branch,
because is_file('file:///etc/passwd') returns true.
limit_response_size is set to MAX_FILE_SIZE + 1 rather than MAX_FILE_SIZE
so a truncated oversized response always trips the post-download size
check; capping at exactly the limit would let a silently-truncated font
through as if it were complete. Content is still proven to be a real TTF
by the existing TtfFontValidation before it reaches the font directory.
The synthetic $_FILES entry drops the 'file' and 'size' keys - the
vendored upload library reads only tmp_name/name/error, so 'file' was
loading every font fully into memory for nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Coverage report for commit: 43efeee Summary - Lines: 92.99% | Methods: 88.31% | Branches: 81.37%
🤖 Jest coverage report |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GPDFAPI::add_pdf_font()now accepts an http(s) URL for any of the four font keys (regular,italics,bold,bolditalics) alongside the existing absolute server path. The two forms can be mixed in the one call:URLs are fetched with
wp_remote_get(). Failures returnWP_Error( 'font_download_error', [ '<font key>' => '<message>' ] ), matching the shape of the existingfont_validation_error.Security notes
Worth a careful look, since this adds an outbound fetch driven by a caller-supplied URL:
wp_http_validate_url()runs before the request, not just viareject_unsafe_urls. WP applies that flag after thepre_http_requestshort-circuit, so a plugin filtering that hook would otherwise bypass the SSRF check entirely. This blocks loopback/private/0.0.0.0hosts, embedded credentials, and non-standard ports.reject_unsafe_urlsis still passed so each redirect hop is revalidated; redirects capped at 3, timeout 30s.scheme://is routed to the downloader and rejected unless it is http/https — it never falls through to the local-path branch. This matters becauseis_file( 'file:///etc/passwd' )returnstrue.stream => true— the body never enters PHP memory.limit_response_sizeisMAX_FILE_SIZE + 1, notMAX_FILE_SIZE. The transport truncates rather than errors when the cap is hit, so capping at exactly the limit would let a silently-truncated font through as if complete. The extra byte guarantees the post-download size check catches it. Cap is 32 MB, large enough for CJK fonts.basename()first (discards traversal), thensanitize_file_name(), then a hard.ttfrequirement.TtfFontValidation(mPDF metrics parse) before it is copied into the font directory.finally, alongside the$_FILESrestore, so neither leaks on any exit path.Scope was deliberately kept to
GPDFAPI::add_pdf_font(). Moving the fetch intoController_Custom_Fontswould hand a server-side fetch primitive to the REST endpoint, letting anygravityforms_edit_formsuser probe internal addresses through the admin UI — an SSRF surface expansion that isn't needed here (the JS font manager posts multipart uploads and has no URL use case).Incidental cleanup
The synthetic
$_FILESentry drops itsfileandsizekeys. The vendored upload library (Upload\File::__construct) reads onlytmp_name,nameanderror, so'file' => file_get_contents( … )was loading every font fully into memory for nothing.Testing
tests/phpunit/integration/Helper/Fonts/Test_RemoteFontDownloader.php(new) — every SSRF rejection case, proof that a rejected URL never reaches the HTTP layer, filename derivation (encoded spaces/slashes, traversal, query strings, uppercase extension), the size boundary at exactly max and max+1, and temp-file cleanup on both success and failure.tests/phpunit/integration/Test_Api.php— URL install, mixed URL+path install, download failure wiring, and a 200 response whose body isn't a font (still rejected by the TTF validator).All mock
pre_http_requestand use IP literals, so no DNS or network is touched.Full suite green (1613 tests, single-site and multisite for the affected classes); PHPCS clean.
Follow-up (not in this PR)
Controller_Save_Core_Fonts::download_and_save_font()is a secondwp_remote_get( stream )font fetcher with no size cap, noreject_unsafe_urlsand no redirect limit — it relies solely on its GitHub repo allowlist. Worth folding ontoRemoteFontDownloaderseparately.🤖 Generated with Claude Code