fix(handlers): validate all modifier arguments

[pi0x]

Modifier arguments were passed straight to sharp, which threw raw errors with no status code and surfaced as a 500. Every argument is now validated up front and returns a 400 instead.

Follows up on the VArg TODO in src/handlers/utils.ts.

Arg mappers

Reusable mappers in src/handlers/utils.ts, wired into every handler:

Mapper	Accepts
VNumber(name, { min, max, integer })	bounded number; rejects NaN / Infinity / true / null
VEnum(name, values)	one of a fixed set
VColor(name)	hex (3/4/6/8 digits, # optional and re-added), functional or named
VSize(name)	{width}x{height} or square {size}, positive integers
VRequired(name, mapper)	wraps a mapper to reject an omitted value

Bounds mirror the ones sharp enforces (read off sharp/dist/{resize,operation,colour}.mjs rather than guessed). VArg is unchanged and still exported — it is the base coercion the mappers build on.

applyHandler also wraps handler.apply() so sharp's own validation (unknown colour names, which sharp only parses when the colour is used) surfaces as a 400 IPX_INVALID_MODIFIER rather than a 500.

Errors are 400 IPX_INVALID_MODIFIER_ARG, or 400 IPX_MISSING_MODIFIER_ARG for a required argument:

Invalid `extend.extendWith` modifier argument: `foo` (expected one of: background, copy, repeat, mirror)
Invalid `extend.top` modifier argument: `-10` (expected an integer between 0 and 10000)

Bugs fixed along the way

Pre-existing 500s, not regressions from this change:

• /trim_100/ — sharp 0.35 requires trim({ threshold }); a bare number throws Expected object for trim. The documented README example was broken.
• /sharpen/ — passed { sigma: undefined, … }, and sharp rejects an options object without a sigma. Now falls back to sharpen().
• /blur/, /gamma/, /threshold/ and other valueless forms — passed "" straight through, which sharp rejects. "" is now treated as omitted so sharp's defaults apply.
• /extract_0_0_10/ — sharp requires all four values; missing ones now give a clear 400.

Positions

_ separates modifier arguments, so sharp's two-word positions (right top) were effectively unreachable — pos_right_top silently dropped top. pos_right-top is now accepted, alongside the existing %20 form.

Notes

• quality is now 1–100 (README previously said 0–100). 0 is only valid for PNG in sharp, so this takes the safe intersection. Happy to allow 0 instead if you prefer.
• Errors thrown at sharp.toBuffer() are still 500s — e.g. /extract_0_0_99999999_99999999/ passes extract()'s validation and only fails later in libvips. Out of scope for the arg TODO, but it is the remaining path where bad input yields a 500.

Docs

The modifiers table now documents the accepted values and ranges for every modifier, plus the shared colour syntax.

Tests

• test/handlers/utils.test.ts — unit tests per mapper.
• test/handlers/handlers.test.ts — table-driven valid/invalid cases for every modifier, driven through applyHandler.
• test/index.test.ts — every modifier exercised against real sharp (mocking is what hid the trim / sharpen / valueless-arg bugs), plus 400 assertions for invalid input.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Enhancements
  • Strengthened validation for modifier arguments (numeric bounds/integer checks, required values, allowed enums, dimensions, and color formats including hex/CSS names).
  • Updated behavior for resize, trim, extend, extract, rotate, sharpen/blur, gamma/threshold/modulate, and tint/grayscale, including correct omission of optional settings and proper zero-argument handling.
  • Invalid or missing modifier inputs now reliably return HTTP 400 errors.
• Documentation
  • Expanded and clarified the modifiers reference with accepted formats, defaults, constraints, and error expectations.
• Tests
  • Expanded modifier test coverage for valid and invalid argument combinations, including regressions for omission and ordering.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2d9fb551-265f-47e5-ad79-da099d8d0c0e

📥 Commits

Reviewing files that changed from the base of the PR and between 1b76f70 and 62efb15.

📒 Files selected for processing (1)

• test/index.test.ts

---

📝 Walkthrough

Walkthrough

Modifier handling now uses typed argument mappers with explicit validation, structured 400 errors, updated Sharp handler contracts, expanded tests, and more precise README documentation.

Changes

Modifier validation and handler contracts

Layer / File(s)	Summary
Argument mapper contracts and error handling src/handlers/utils.ts, test/handlers/utils.test.ts	Adds numeric, enum, color, size, and required-argument mappers, plus consistent structured errors and applyHandler error wrapping.
Typed handler wiring and Sharp operations src/handlers/handlers.ts, README.md	Updates modifier handlers to use constrained arguments, typed resize and extract values, zero-argument operations, conditional modulate options, and documented validation rules.
Handler and integration validation test/handlers/handlers.test.ts, test/index.test.ts	Adds coverage for valid and invalid arguments, omitted values, Sharp error conversion, modifier behavior, and image dimensions.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ModifierRequest
  participant applyHandler
  participant ArgMapper
  participant SharpPipeline
  ModifierRequest->>applyHandler: modifier arguments
  applyHandler->>ArgMapper: parse and validate arguments
  ArgMapper-->>applyHandler: typed values or HTTPError
  applyHandler->>SharpPipeline: invoke handler with typed values
  SharpPipeline-->>applyHandler: result or parsing error

Loading

Possibly related PRs

• unjs/ipx#315: Both changes update extend handling and its extendWith argument.
• unjs/ipx#317: Both changes update modulate argument parsing and omission behavior.

Suggested reviewers: pi0

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: validating handler modifier arguments before passing them to Sharp.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/varg-validation

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 98.91304% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.90%. Comparing base (84ff786) to head (62efb15).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/handlers/utils.ts	98.57%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
+ Coverage   80.08%   84.90%   +4.82%     
==========================================
  Files           9        9              
  Lines         487      563      +76     
  Branches      132      159      +27     
==========================================
+ Hits          390      478      +88     
+ Misses         79       69      -10     
+ Partials       18       16       -2     

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pi0x]

Ran an independent review pass over the diff (verified against sharp 0.35.3's source and by running the real pipeline, not just reading the code). Pushed 1b76f70 with the fixes; the rest is written up below for your call.

Fixed

• Numeric position was a regression. POSITIONS only listed the string names, but sharp accepts the gravity (0-8) and strategy (16-17) integers too — and HandlerContext.position is already typed number | string. /pos_3,s_100x100/ returned 200 on main and 400 on this branch. Now accepted again.
• rotate had no bounds and still produced a 500: /rotate_1e10/ fails inside libvips (value ... is invalid or out of range for property 'angle' of type 'gdouble'). Bounded to -3600..3600 — measured, 1e6 works and 1e10 does not.
• error instanceof HTTPError → HTTPError.isError(). h3 matches duck-typed for exactly this reason; with instanceof, a custom handler throwing an HTTPError from a differently-resolved h3 copy would have had its status silently rewritten to 400.
• A test that could not fail. The invalid-args loop passed an empty context, so resize.apply threw a TypeError on context.meta before VSize was ever consulted — every resize invalid case would have passed even if VSize accepted everything. Now given a full context (confirmed: a valid arg no longer throws, so the assertions are real).

Not changed — flagging for your call

• extract/crop out-of-bounds is still a 500. /crop_5000_5000_100_100/ on a 3840x2160 image passes sharp's JS validation and fails in libvips at toBuffer(), outside applyHandler's try/catch. Tempting to bound it against context.meta, but that would be wrong: handlers are ordered, and extract after resize crops the resized image, not the source. Fixing it properly means bounding at toBuffer() time, which is a bigger change than this PR.
• applyHandler's catch is deliberately broad. It converts any non-HTTPError to a 400 and echoes error.message, so a genuine internal error would be misreported as client error. Narrowing it to sharp's invalidParameterError shape does not work: the case the catch exists for (unknown colour names) comes from the color package with a different message shape. I kept the broad catch, but it is a real trade-off.
• Resource limits are unchanged. The mappers mirror sharp's bounds, which are correctness bounds, not DoS bounds. /blur_1000/ on the 3840x2160 test image burns >120s of CPU on a single request, and width/height/resize have no maximum at all. All pre-existing, but this PR's README now explicitly advertises those maxima as supported. An IPX-level pixel cap would be a good follow-up.
• Minor tightenings that are intentional but worth a look: /fit_foo/ and /kernel_bicubic/ without a resize used to be silently ignored and now 400; /s_200x/ used to fall back to a 200x200 square; q_0 is now rejected, which is right for every format except gif (sharp's gif encoder allows 0).

Correction to the commit message

It claims two-word positions "were unreachable since _ separates arguments". That is overstated — %20 already worked (/pos_left%20bottom/ returns byte-identical output on main). pos_right-top is a nicer alias, not a fix for something unreachable.

Everything else in the description checked out: all four claimed 500-fixes reproduce as 500 on main and 200/400 on this branch, the 400s propagate through h3 to real HTTP responses, and every documented range matches sharp's source. No ReDoS in the new regexes (measured against 50K-200K char adversarial inputs), and no validation bypass found.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (6)

test/index.test.ts (2)

106-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Hardcoded source dimensions make this assertion fixture-brittle.

Deriving the base size from bliss.jpg meta (or from an unmodified process() result) keeps the test valid if the fixture is ever replaced.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/index.test.ts` around lines 106 - 113, Update the “extend resizes the
canvas” test to derive the original width and height from bliss.jpg metadata or
an unmodified process() result instead of hardcoding 3840 and 2160. Use those
derived dimensions when asserting the extended width and height, while
preserving the existing extension offsets.

---

55-55: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

enlarge case upscales to 8000x8000 in CI.

That's a ~64 MP output per run for a fixture that is 3840x2160; a smaller target (e.g. 5000x5000 or a smaller source) keeps the assertion while cutting memory/CPU in the test suite.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/index.test.ts` at line 55, Reduce the target dimensions in the `enlarge`
test case within the test fixture configuration from 8000x8000 to a smaller size
such as 5000x5000, preserving the upscaling behavior and existing assertion
while lowering CI memory and CPU usage.

README.md (1)

233-233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Colour docs omit the functional notations the validator accepts.

VColor also accepts rgb()/rgba()/hsl()/hsla()/hwb(); worth listing so users know they are supported (URL-encoding caveats aside).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README.md` at line 233, Update the Colours documentation to list the
supported functional colour notations rgb(), rgba(), hsl(), hsla(), and hwb()
alongside the existing hex and CSS colour-name formats, while preserving the
current URL-path caveat.

src/handlers/handlers.ts (1)

245-256: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

sharpen silently drops flat/jagged when sigma is omitted.

/sharpen__1_2/ validates fine but the two provided values are discarded. Rejecting that combination (or documenting it) would be more predictable than ignoring user input.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/handlers/handlers.ts` around lines 245 - 256, The sharpen apply handler
currently ignores provided flat and jagged values when sigma is omitted. Update
the apply logic for sharpen so supplying flat or jagged without sigma is
rejected or otherwise explicitly handled, rather than silently calling
pipe.sharpen() and discarding those values; preserve the existing default
sharpening behavior when all options are omitted.

src/handlers/utils.ts (2)

237-252: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Blanket 400 also hides genuine server-side faults.

Any throw from handler.apply (including a TypeError from a handler bug) is reported as a client error, which will mask real 500s in metrics/logs. Consider logging the original cause at warn/error level, or restricting the 400 conversion to sharp validation errors.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/handlers/utils.ts` around lines 237 - 252, Restrict the error conversion
in the handler.apply catch block to known sharp validation errors, allowing
unexpected handler faults such as TypeError to propagate as server errors.
Preserve existing HTTPError passthrough and the 400 IPX_INVALID_MODIFIER
response for genuine modifier-validation failures; use the original error type
or established sharp validation symbols to distinguish them.

---

56-61: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Widen isOmitted’s parameter type so undefined is type-safe.

isOmitted is used with the raw argument from parseArgs, where missing slots can be undefined, but it is currently typed as string. TypeScript 6 with strict does not error on string === undefined; widening the declared type instead makes the omission contract explicit without changing behavior.

♻️ Suggested typing fix

-function isOmitted(argument: string): boolean {
+function isOmitted(argument: string | undefined): boolean {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/handlers/utils.ts` around lines 56 - 61, Update the isOmitted function
parameter type to accept string or undefined, matching raw parseArgs values
while preserving its existing omission checks and behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/handlers/handlers.ts`:
- Around line 99-104: Update the kernel handler in src/handlers/handlers.ts at
lines 99-104 to set order: -1, matching the other context modifiers so it
executes before resize. In test/index.test.ts at line 56, add coverage with
resize before kernel; README.md at line 240 requires no direct change and should
remain once ordering is enforced.

---

Nitpick comments:
In `@README.md`:
- Line 233: Update the Colours documentation to list the supported functional
colour notations rgb(), rgba(), hsl(), hsla(), and hwb() alongside the existing
hex and CSS colour-name formats, while preserving the current URL-path caveat.

In `@src/handlers/handlers.ts`:
- Around line 245-256: The sharpen apply handler currently ignores provided flat
and jagged values when sigma is omitted. Update the apply logic for sharpen so
supplying flat or jagged without sigma is rejected or otherwise explicitly
handled, rather than silently calling pipe.sharpen() and discarding those
values; preserve the existing default sharpening behavior when all options are
omitted.

In `@src/handlers/utils.ts`:
- Around line 237-252: Restrict the error conversion in the handler.apply catch
block to known sharp validation errors, allowing unexpected handler faults such
as TypeError to propagate as server errors. Preserve existing HTTPError
passthrough and the 400 IPX_INVALID_MODIFIER response for genuine
modifier-validation failures; use the original error type or established sharp
validation symbols to distinguish them.
- Around line 56-61: Update the isOmitted function parameter type to accept
string or undefined, matching raw parseArgs values while preserving its existing
omission checks and behavior.

In `@test/index.test.ts`:
- Around line 106-113: Update the “extend resizes the canvas” test to derive the
original width and height from bliss.jpg metadata or an unmodified process()
result instead of hardcoding 3840 and 2160. Use those derived dimensions when
asserting the extended width and height, while preserving the existing extension
offsets.
- Line 55: Reduce the target dimensions in the `enlarge` test case within the
test fixture configuration from 8000x8000 to a smaller size such as 5000x5000,
preserving the upscaling behavior and existing assertion while lowering CI
memory and CPU usage.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: bce1fcba-2bc9-45ea-a7cf-c2d75b2281e3

📥 Commits

Reviewing files that changed from the base of the PR and between 84ff786 and 2bc05b0.

📒 Files selected for processing (6)

• README.md
• src/handlers/handlers.ts
• src/handlers/utils.ts
• test/handlers/handlers.test.ts
• test/handlers/utils.test.ts
• test/index.test.ts