feat: safer-defaults helpers and unify EXIF orientation parsing#16
Merged
feat: safer-defaults helpers and unify EXIF orientation parsing#16
Conversation
…helper Introduce ResourceLimits::for_untrusted_input() (alias safe_default()) with sane caps for processing untrusted input: 100 MP per frame, 200 MP total, 16384×16384 max dims, 1 GiB memory, 256 MiB input, 65536 frames, 1h duration. ResourceLimits::default() continues to return all-None (no limits) for backwards compatibility — the new helper is the recommended starting point for services accepting bytes from the network or end users. Adds 6 tests verifying the caps reject oversized input, accept typical 4K/12 MP images, and that default() behavior is unchanged.
The metadata.rs orientation parser was a looser duplicate of helpers/exif.rs — it only read u16 at +8 regardless of TIFF type field, missing TIFF_LONG (type 4) values that store data in different bytes for big-endian, and it lacked the IFD entry-count cap and tag-sort early exit that helpers/exif.rs provides as DoS protection. Have metadata::parse_exif_orientation delegate to the canonical impl in helpers/exif.rs. Adds two tests verifying TIFF_LONG type is now handled correctly (BE and LE) — the previous parser silently missed those values.
The DynDecodeJob and DynEncodeJob shims silently no-op'd if a setter was called after the inner job was moved out by an into_* method. This is unreachable through the public API (every into_* method takes Box<Self>), but the silent fallback masked the bug if the path were ever reached through internal misuse or a future refactor. Consolidate the setter bodies through a try_apply helper that fires debug_assert! when the inner job is missing, so any future regression that exposes the consumed-after path fails loudly in tests and dev builds. Release-build behaviour is unchanged (silent no-op preserved) to avoid any chance of a panic in production code. The DynDecodeJob/DynEncodeJob trait signatures are unchanged — these remain infallible setters for backwards compatibility per the audit's guidance to avoid breaking changes to public types in this foundation crate.
Member
Author
|
@lilith — ready for review. Part of the 2026-05-06 security audit campaign (see /home/lilith/work/feedback/security-audit-2026-05-06/FIX-RESULTS.md and REVIEW-RESULTS.md). |
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.
Addresses MEDIUM findings from the 2026-05-06 security audit. All changes are additive or internal — public type fields and method signatures are unchanged.
ResourceLimits::default()continues to return all-Nonefor backwards compatibility; the newfor_untrusted_input()helper is the recommended path going forward.Findings addressed
M1 —
ResourceLimits::default()is no-limitsAdds
ResourceLimits::for_untrusted_input()(aliassafe_default()) with sane caps for processing untrusted input:max_pixels: 100 MP per framemax_total_pixels: 200 MP across an animationmax_width/max_height: 16384 eachmax_memory_bytes: 1 GiBmax_input_bytes: 256 MiBmax_frames: 65 536max_animation_ms: 1 hourResourceLimits::default()is unchanged — additive helper, no breaking change. 6 new tests verify the caps reject oversized input, accept typical 4K/12 MP images, and thatdefault()still has no caps.M2 —
DecodePolicy::strict()not visibly promotedModule-level docs in
policy.rsand theDecodePolicystruct doc now recommendDecodePolicy::strict()as the starting point for untrusted input, paired withResourceLimits::for_untrusted_input. No code changes.M3 — Duplicated EXIF orientation parsing
metadata::parse_exif_orientationwas a looser duplicate ofhelpers/exif.rs::parse_exif_orientation:u16regardless of the TIFF type field, missingTIFF_LONG(type 4) values for big-endian inputs.Now delegates to the canonical implementation. 2 new tests verify TIFF_LONG (BE + LE) is now handled correctly through the metadata module path.
M4 — Dyn-job setters silently no-op after consumption
DynDecodeJobandDynEncodeJobshim setters silently no-op'd if invoked after the inner job had been moved out by aninto_*method. This path is structurally unreachable from external code (everyinto_*takesBox<Self>) but the silent fallback masked any future regression.The setters now route through a
try_applyhelper that firesdebug_assert!on the consumed-after path, catching misuse loudly in tests and dev builds. Release behaviour is unchanged (silent no-op preserved). Trait signatures are unchanged — making the setters fallible would break the public API of every codec implementing the trait, which the audit explicitly told this PR to avoid.Validation
cargo build(host +wasm32-unknown-unknown --no-default-features)cargo fmt --checkcargo clippy --all-targets -- -D warningscargo test— 432 unit + 100 integration + 29 integration (other suites) + 14 doctests, all passingTest plan
for_untrusted_inputrejection of oversized images and inputsparse_exif_orientationaccepting TIFF_LONG type (BE + LE)cargo build --target wasm32-unknown-unknown --no-default-featuressucceeds (no_std + alloc preserved)