Conversation
quality, compressionLevel and colors used to clamp an out-of-range value to the nearest bound and map NaN to the low bound, with no error. They now go through validate_integer_range with the documented ranges (1-100, 0-9, 2-256). A wrong type throws ERR_INVALID_ARG_TYPE, NaN or a fraction throws ERR_INVALID_ARG_TYPE with "integer", and a value outside the range throws ERR_OUT_OF_RANGE.
|
Reproduced on released bun 1.4.1 with a 256x256 noise PNG: With this change each of those throws at the format call. The new test in This PR is stacked on #40491 (the wrong-type check). GitHub retargets it to main once that PR merges. CI: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean swap from coerce_int! clamping to validate_integer_range, matching how Bun.Archive level and Bun.password cost already validate.
What was reviewed:
get_int_option:opt.get()maps both missing andundefinedtoNone, so theT::ZEROdefault passed tovalidate_integer_rangeis unreachable and can't leak as a value; the explicit NaN pre-check correctly closes the one gap wherevalidate_integer_rangewould have returned the default.- All format methods (
jpeg/png/webp/heic/avif) route through the singleset_format, so the fix covers the whole class. - Test covers out-of-range, NaN, fractional, and exact boundary values; the removed
quality: NaNassertion in the existing clamp test is the only pre-existing coverage this invalidates. - CODEOWNERS: none of the changed paths are owned; type declarations already document these ranges as
number, so no.d.tsupdate needed.
Extended reasoning...
Overview
This PR replaces silent clamping of Bun.Image encoder options (quality, compressionLevel, colors) with strict validation that throws ERR_INVALID_ARG_TYPE for non-integers and ERR_OUT_OF_RANGE for out-of-bound values. It touches three files: src/runtime/image/Image.rs adds a get_int_option<T> helper wrapping the existing JSGlobalObject::validate_integer_range and swaps three get_number_option + coerce_int! call sites to use it; test/js/bun/image/image.test.ts gains a new test for #40490 and drops one now-throwing assertion from an existing clamp test; docs/runtime/image.mdx documents the ranges and error codes.
Security risks
None. This is input validation on a Bun-native API that tightens (not loosens) accepted values. No parsing of untrusted data, no memory management, no auth/crypto/permissions. The change makes the API stricter, which if anything reduces the surface for surprising downstream behavior.
Level of scrutiny
Low-to-medium. The implementation is a thin wrapper over an existing, widely-used validation helper (validate_integer_range), and the three call sites in set_format are the only ones changed — verified that all format methods (jpeg/png/webp/heic/avif) share set_format, so the fix covers the whole class. I traced JSValue::get to confirm it returns None for both missing and undefined properties, which means the T::ZERO default is never reachable (the one case where it would be — NaN — is explicitly rejected first). The behavior change from clamp → throw is user-facing, but the PR cites precedent in Bun.Archive level, Bun.password bcrypt cost, and Response compress.level, all of which throw on out-of-range values rather than clamping.
Other factors
Test coverage is thorough: exact error shape via toMatchObject (name, code, message), out-of-range values across all three options and multiple format methods, NaN and fractional inputs, and — importantly — the exact boundary values (1/100, 0/9, 2/256) are asserted to be accepted. The one removed assertion (quality: NaN in the existing clamp test) is justified since it now throws, and the rest of that test is preserved. CODEOWNERS covers only .d.ts, test/expectations.txt, and CODEOWNERS itself — none of the changed files. The .d.ts type declarations already document the ranges in JSDoc and keep the type as number, which remains accurate. The bug hunt exited on dry_streak with no findings.
This branch has not been deployed
2 participants
Stacked on #40491. That PR makes a wrong-type option value throw. This one makes an out-of-range value throw too. The diff shown here is only the range check.
Problem
Bun.Imageencoder option clamps in silence..jpeg({ quality: 999 })encodes at quality 100,{ quality: -5 }and{ quality: NaN }encode at quality 1. Nothing throws or warns (Bun.Image: encoder options are not validated, so a typo, a string quality, or an out-of-range value all silently produce the wrong output #40490).set_formatinsrc/runtime/image/Image.rsreadsquality,compressionLevelandcolorsthroughcoerce_int!, which exists to keep NaN and out-of-range floats out of an integer cast. It clamps instead of reporting.Fix
get_int_option. It reads the property and runs it throughJSGlobalObject::validate_integer_rangewith the documented range:quality1-100,compressionLevel0-9,colors2-256. A value outside the range throws aRangeErrorwithcode: "ERR_OUT_OF_RANGE", for exampleThe value of "quality" is out of range. It must be >= 1 and <= 100. Received 999. NaN and fractions throwERR_INVALID_ARG_TYPEwith typeinteger.Bun.Archivelevel(1-12),Bun.passwordbcryptcost(4-31) andResponsecompress.levelall throw on an out-of-range value. None of them clamp.resize,rotate,maxPixelsandmodulatekeep their existing clamps. Those values are sizes and multipliers without a documented range, and the clamp there is a guard, not an option parse.test/js/bun/image/image.test.ts(new test, fails on released bun, passes with this change). The full file passes: 97 pass, 2 skip.Background
Bun.Imagerecords encode options synchronously in.jpeg()/.png()/.webp()and does the work later in a terminal such as.bytes(). So the error surfaces at the format call, before any decode.validate_integer_rangeis the Rust port of Node'svalidateInteger. It throwsERR_INVALID_ARG_TYPEfor a non-number or a fraction andERR_OUT_OF_RANGEfor a value outsidemin..=max. It maps NaN to the default, soget_int_optionrejects NaN first, the same wayBun.builddoes forbytecodeDepth.qualiytypo in the report) stay ignored. No Bun API rejects unknown option keys, and the TypeScript types catch the typo at compile time.Notes
Measured on released bun 1.4.1 with a 256x256 noise PNG,
.jpeg(o).bytes().byteLength:{}40743,{ quality: 100 }123985,{ quality: 999 }123985 (clamped to 100),{ quality: -5 }3343,{ quality: 0 }3343,{ quality: NaN }3343 (all clamped to 1),{ quality: 80.5 }40743,{ quality: "84" }40743 (ignored)..png({ compressionLevel: 10 })and.png({ palette: true, colors: 1000 })andcolors: 1were accepted.With this change each of those throws:
quality: 999,-5,0,compressionLevel: 10,colors: 1000,colors: 1throwERR_OUT_OF_RANGE.quality: NaNand80.5throwERR_INVALID_ARG_TYPE(must be of type integer).quality: "84"throwsERR_INVALID_ARG_TYPE(must be of type number, from #40491).The existing test
non-finite / huge numeric inputs are clamped by coerceIntasserted that.jpeg({ quality: NaN })does not throw. That assertion is removed. Theresize(NaN, NaN)andmaxPixels: Infinityassertions in that test are unchanged.The docs (
docs/runtime/image.mdx) now state the ranges and the two error codes. The type declarations already document the ranges.Suites run:
bun bd test test/js/bun/image/image.test.ts(97 pass, 2 skip, 0 fail).cargo fmt --check,cargo clippy -p bun_runtime(no new warnings), prettier check on the test and docs.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/image/image.test.ts