Media Editor: Harden cropper math layer against non-finite inputs#78321
Merged
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +447 B (+0.01%) Total Size: 7.97 MB 📦 View Changed
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
update/cropper-fuzz-testing
branch
from
4b7bb09 to
7cdadb9
Compare
ramonjd
added
[Feature] Media
60 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the media editor cropper math against non-finite numeric values by adding sanitization helpers, applying them across camera/containment/source-region paths, and adding regression tests.
Changes:
- Adds
safeBoundedNumberandsanitizeCropperState. - Applies sanitization to camera, export, containment, and source-region calculations.
- Adds regression tests for hostile cropper state inputs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
packages/media-editor/src/image-editor/core/math/sanitize.ts |
Adds numeric/state sanitization helpers. |
packages/media-editor/src/image-editor/core/math/test/sanitize.ts |
Adds unit tests for sanitization behavior. |
packages/media-editor/src/image-editor/core/camera.ts |
Adds guards/sanitization to camera and bbox calculations. |
packages/media-editor/src/image-editor/core/containment.ts |
Applies sanitized state/inputs to crop containment math. |
packages/media-editor/src/image-editor/core/source-region.ts |
Sanitizes state before source-region calculation. |
packages/media-editor/src/image-editor/core/test/camera.ts |
Adds non-finite regression coverage for camera/containment/source-region helpers. |
Comments suppressed due to low confidence (4)
packages/media-editor/src/image-editor/core/containment.ts:302
cropRectis still the raw argument here, so corrupted crop dimensions can drivegetMinZoomForCovertoNaN(for examplecropRect.width = NaNmakesMath.max( 1, NaN, … )returnNaN). ThatNaNcan then be returned aszoom, despite the state sanitization. Sanitize the crop rect argument before using it in the zoom and stencil-corner math.
const minZoom = getMinZoomForCover(
safeState.rotation,
aspectRatio,
cropRect
);
packages/media-editor/src/image-editor/core/camera.ts:333
outputSizeremains unvalidated even though it participates directly in the export transform. Passing{ width: Infinity, height: 400 }(or an output size derived from an unsafe crop before this call) makes the translation computefinite - Infinity + Infinity, producingNaNmatrix entries. Guard or sanitizeoutputSizebefore using it in the transform math.
const { rotation, flip, cropRect, zoom, pan } =
sanitizeCropperState( state );
packages/media-editor/src/image-editor/core/camera.ts:79
- Only
rotationis bounded here; non-finitecontainerSizevalues still flow intofitScale. For example,containerSize.width = NaNmakesMath.min( NaN, … )returnNaN, soelementSize.width/heightare non-finite even though this function is meant to harden the fit math against bad inputs. Validate or sanitize the size arguments before computing the scale.
const safeRotation = safeBoundedNumber( rotation, 0 );
packages/media-editor/src/image-editor/core/camera.ts:145
- State sanitization alone does not make
createCamerasafe for all non-finite inputs:containerSizeandimageSizeare still only checked for=== 0. If an image dimension isNaN/Infinity,getRotatedBBoxreturns{ 0, 0 },fitScalebecomesInfinity, and later matrix operations can produceNaN. Add finite/bounded size guards or the same zero-bbox short-circuit used ingetImageFit.
const safeState = sanitizeCropperState( state );
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (2)
packages/media-editor/src/image-editor/core/math/sanitize.ts:83
sanitizeRectkeeps sub-normalwidth/heightvalues becausesafeBoundedNumberaccepts them. That leaves another non-finite path:createExportCameratreats a tiny positive crop dimension as> 0, divides the output size by it, and can produce anInfinityscale forctx.setTransform. Width and height need the same minimum-positive guard as zoom (or should fall back to0) before they are used as divisors.
const width = safeBoundedNumber( rect.width, 0 );
const height = safeBoundedNumber( rect.height, 0 );
packages/media-editor/src/image-editor/core/camera.ts:340
- Sanitizing only inside
createExportCamerais too late for the export APIs that call it with a default output size:renderToCanvas/applyToCanvascomputeoutW/outHfrom the rawstate.rotationandstate.cropRectbefore calling this function. A hostile crop rect or rotation can still produce a zero/invalid canvas (or invalidoutputSize) before this guard runs, so the export path is not fully hardened unless those callers use the sanitized state/rect when deriving the canvas dimensions too.
if ( ! isValidSize( imageSize ) || ! isValidSize( outputSize ) ) {
return m;
}
const { rotation, flip, cropRect, zoom, pan } =
sanitizeCropperState( state );
|
Flaky tests detected in 45639a1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/26010517902
|
andrewserong
approved these changes
May 18, 2026
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
Good idea hardening the internals of the image cropper, especially as we reach a solid MVP for the editor itself 👍
All testing well for me manually as on trunk, just left a comment that came up while reviewing with Claude. I can't think of any practical issue where we'd encounter it, so more of a consistency thing since we're checking isValidSize( imageSize) in other places too.
LGTM 🚀
Member
Author
|
Thanks for checking this one out! The fuzz testing revealed that our functions are pretty strong, so this just "tightens the lid on the bottle". |
Adds defense-in-depth guards in the cropper math layer (packages/media-editor/src/image-editor/core/) so non-finite or overflow-prone inputs (NaN, ±Infinity, MAX_VALUE, sub-normal denormals) get substituted with safe defaults instead of silently propagating into the crop rect, camera matrix, or REST /edit payload. The reducer already normalizes rotation, zoom, and pan on every action, so these paths aren't user-reachable today. But programmatic callers and deserialized state can still feed corrupted values, and the math previously trusted them — letting bad input reach ctx.drawImage() or the server-side /edit endpoint far from the source rather than failing loudly at the boundary. Adds a small math/sanitize.ts helper (safeBoundedNumber, sanitizeCropperState) applied at the entry of each public math function: getRotatedBBox, getImageFit, createCamera, createExportCamera, restrictCropRect, restrictPanZoom, getImageCropBounds, getSourceRegion. The three stencil-math resize functions already had defensive early-return guards on zero/negative imageSize and ratio — they were the model for this work. Adds regression unit tests covering the guard behavior in test/camera.ts and math/test/sanitize.ts.
Addresses Copilot review feedback by tightening the previously-added sanitization layer. The original PR sanitized state numerics but missed primitive size/rect arguments and a couple of early-return paths that bypassed sanitization. - getRotatedBBox: short-circuit on Number.MAX_VALUE rotation (passed isFinite but overflowed degreesToRadians) - getImageFit, createCamera, createExportCamera: validate Size args with new isValidSize helper, not just === 0 - restrictCropRect: sanitize cropRect once at the top and return the sanitized rect on the no-resize path (was leaking raw NaN fields) - restrictPanZoom: sanitize the cropRect argument so NaN dimensions don't drive getMinZoomForCover to NaN - getSourceRegion: sanitize state before the zero-size early return so hostile rotation/zoom can't leak through the metadata fields - sanitizeRect: preserve reference equality for clean inputs so the reducer's "no change" short-circuit keeps working Also fixes the safeBoundedNumber doc comment that incorrectly claimed sub-normal protection (that guard lives in sanitizeCropperState's zoom check, not the helper). Adds regression tests in test/camera.ts and math/test/sanitize.ts covering each new gap.
Addresses round-2 Copilot review feedback: - isValidSize: reject sub-normal dimensions (Number.MIN_VALUE). They pass `> 0` but `container / MIN_VALUE` overflows to Infinity in getImageFit / createCamera. Now requires `>= Number.EPSILON`. - computeTransformStyle: sanitize state. Without this, hostile state yields a finite matrix from createCamera but `matrix(NaN, ...)` from the CSS preview path — math and render layers were diverging. - sanitizeCropperState: add a no-change fast path. The function is called once per pointermove via restrictPanZoom + createCamera, so unconditional allocation was avoidable per-frame GC. Mirrors sanitizeRect's existing pattern; preserves pan/basePan references individually when their fields are clean. - restrictPanZoom: validate imageSize with isValidSize before deriving aspectRatio. Infinity / Infinity = NaN was leaking out as `zoom: NaN` through the no-correction early return. Plus doc/comment cleanup that Copilot caught: - Test comment named the wrong guard (safeBoundedNumber vs isSafeNumber). - getSourceRegion comment incorrectly listed state.flip alongside numeric metadata (flip is boolean and can't propagate NaN). - sanitizeCropperState JSDoc claimed "all numeric fields finite" while leaving state.image numerics intentionally untouched. Adds regression tests for each behavioral fix.
Round-2 sanitization only covered state fields, leaving translate components vulnerable to NaN/Infinity imageSize values. Adds an isValidSize early return that emits the identity CSS matrix — mirroring the same short-circuit pattern in createCamera and getSourceRegion so the preview path stays consistent with the math path when imageSize itself is hostile.
ramonjd
force-pushed
the
update/cropper-fuzz-testing
branch
from
fc2f04c to
45639a1
Compare
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
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.
Ran property-based fuzz testing on the cropper math layer and it surfaced a handful of silent NaN/Infinity propagation paths. Low severity in practice (the reducer normalizes inputs upstream), but worth fixing so bad input fails at the boundary instead of reaching
ctx.drawImage()or the REST/editendpoint.The fuzz testing itself isn't being checked in. This PR is just the fixes plus regression unit tests.
What changed
core/math/sanitize.tswithsafeBoundedNumber(rejects non-finite values and anything beyond ±1e6) andsanitizeCropperState(returns a state with all numeric fields safe).getRotatedBBox,getImageFit,createCamera,createExportCamera,restrictCropRect,restrictPanZoom,getImageCropBounds,getSourceRegion.core/test/camera.tsandcore/math/test/sanitize.ts.The three
stencil-mathresize functions already had this pattern with their existing zero/negative guards — they were the model.Test plan
npm start(rotate, zoom, drag, snap-rotate, flip) — the guards are no-ops under normal reducer-normalized state