Restructure processing and service, and close the geometry compatibility gaps - #66
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 644b9b667b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| config.source_rules = SourceRules { | ||
| base_url: optional_var(ENV_BASE_URL)?.filter(|value| !value.trim().is_empty()), | ||
| allowed: list_var(ENV_ALLOWED_SOURCES)? | ||
| .unwrap_or_default() | ||
| .iter() | ||
| .map(|pattern| SourcePattern::parse(pattern)) | ||
| .collect(), |
There was a problem hiding this comment.
When IMGFORGE_ALLOWED_SOURCES is set, these patterns are stored in config.source_rules, but the request path decodes the source URL and passes it directly to fetch_image without ever calling SourceRules::resolve or SourceRules::permits. As a result, deployments relying on this new setting for SSRF protection still fetch arbitrary URLs, especially when unsigned URLs are enabled.
Useful? React with 👍 / 👎.
| let result = Config::from_env(); | ||
| config.max_animation_frames = security_limit_var(ENV_MAX_ANIMATION_FRAMES)?; | ||
| config.max_animation_frame_resolution = security_limit_var(ENV_MAX_ANIMATION_FRAME_RESOLUTION)?; | ||
| config.signature_size = resolve_signature_size()?; |
There was a problem hiding this comment.
When IMGFORGE_SIGNATURE_SIZE is set below 32, startup accepts and stores the value, but parse_and_authorize still calls validate_signature without it, and that function uses verify_slice against the full SHA-256 tag. Consequently every URL generated with the configured truncated signature is rejected as invalid, making all non-default values of this new option unusable.
Useful? React with 👍 / 👎.
| let (width, height) = (source_image.get_width(), source_image.get_height()); | ||
| let (Ok(width), Ok(height)) = (u32::try_from(width), u32::try_from(height)) else { | ||
| return source_image; |
There was a problem hiding this comment.
For an animated WebP opened with n=-1, libvips represents the frames as one vertically stacked image, so source_image.get_height() is the sum of all frame heights. Passing that height to load_scale_factor can over-shrink landscape frames; for example, ten 2000×1000 frames resized to 100×100 produce a scale of 1/20 instead of 1/10, leaving each decoded frame only 100×50, which enlarge:false cannot restore to the requested box. Use the page height rather than the stacked height when animation geometry is present.
Useful? React with 👍 / 👎.
| if crop.is_relative() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
When a crop mixes a fractional extent with an absolute one, such as crop:0.5:1000, crop.is_relative() returns true and this early return skips rewriting both axes after a reduced decode. The fractional width remains correct, but the absolute height still refers to the full-resolution source and is therefore clamped to or selects the wrong region of the shrunk image; only the fractional axis should be exempt from rescaling.
Useful? React with 👍 / 👎.
| debug!("Applying rotation: {}", rotation); | ||
| img = transform::apply_rotation(img, rotation)?; | ||
| } | ||
| enforce_result_dimension(&parsed_options, &img)?; |
There was a problem hiding this comment.
For animated output, animation::join has already stacked all processed frames vertically before this check, so max_result_dimension compares the limit against frame_height × frame_count rather than the user-visible frame height. A ten-frame 100×100 animation is therefore rejected under a 500-pixel result limit even though every output frame is within the configured ceiling; enforce the limit on the processed frames before joining, or use page_height here.
Useful? React with 👍 / 👎.
644b9b6 to
4a50761
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a5076174d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub is_raw: bool, | ||
| pub max_result_dimension: Option<MaxResultDimension>, |
There was a problem hiding this comment.
When a persistent cache already contains an animated result and IMGFORGE_MAX_ANIMATION_FRAMES or IMGFORGE_MAX_ANIMATION_FRAME_RESOLUTION is subsequently tightened, this key remains unchanged because it includes only max_result_dimension. The cache hit in src/service/mod.rs:123-132 then returns the old entry before either animation limit runs, potentially serving excess frames or an image that should now be rejected; include both effective animation limits in the key, as is already done for the result-dimension limit.
Useful? React with 👍 / 👎.
| // `raw` returns the source untouched; `skip_processing` does the same for | ||
| // the formats it names, which is how imgproxy keeps an already-optimised | ||
| // asset from being re-encoded. | ||
| if parsed_options.raw || can_skip_processing(&parsed_options, source_format) { |
There was a problem hiding this comment.
When skip_processing matches the source format, this early return occurs before enforce_security_constraints at src/service/mod.rs:203-209. Although the fetch-time file-size ceiling still applies, configured allowed_mime_types and max_src_resolution are bypassed, so a URL using this new option can return a source that the same deployment rejects on the normal processing path; enforce those source checks before returning untouched bytes.
Useful? React with 👍 / 👎.
| if frames.images.len() <= 1 { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
When an animated source is collapsed with disable_animation, converted to a still format, or explicitly loaded with pages:1, LoadPlan::resolve opens only one frame and animation::split consequently returns one image. This branch then skips max_animation_frame_resolution entirely, allowing an oversized animation frame through despite the configured security limit; distinguish a collapsed animation from a true still image rather than using the processed vector length as the predicate.
Useful? React with 👍 / 👎.
| pub channels: u32, | ||
| pub has_alpha: bool, | ||
| pub orientation: Option<u32>, | ||
| pub pages: u32, |
There was a problem hiding this comment.
The metadata service now computes and exposes ImageInfo.pages, but info_handler still constructs its JSON object in src/handlers.rs:35-44 without this field. Consequently /info/... clients never receive the newly collected frame/page count even though it is cached and returned by image_info; add pages to the serialized response.
Useful? React with 👍 / 👎.
4a50761 to
513b08c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 513b08c9b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub struct CacheKeyParts<'a> { | ||
| pub path: &'a str, | ||
| pub default_format: DefaultOutputFormat, | ||
| pub has_explicit_format: bool, | ||
| pub is_raw: bool, | ||
| pub max_result_dimension: Option<MaxResultDimension>, |
There was a problem hiding this comment.
When a persistent cache survives a configuration change, output-affecting defaults such as IMGFORGE_QUALITY, IMGFORGE_AUTO_ROTATE, IMGFORGE_ENFORCE_THUMBNAIL, and the metadata/HDR defaults are absent from this key. process_path seeds these values before the cache lookup, but a hit returns the bytes generated under the previous defaults, so changing (for example) quality from 85 to 20 has no effect for previously cached URLs; include the effective defaults or a configuration-derived namespace in the cache identity.
Useful? React with 👍 / 👎.
| if crop.width >= 1.0 { | ||
| crop.width = (crop.width * (sw / ow)).ceil().max(1.0); | ||
| } | ||
| if crop.height >= 1.0 { | ||
| crop.height = (crop.height * (sh / oh)).ceil().max(1.0); |
There was a problem hiding this comment.
When a JPEG or WebP is decoded at reduced scale and the crop uses an absolute gravity offset, this rewrites only the crop extents while leaving crop.gravity (or the request-level fallback gravity) in full-resolution pixels. For example, a 4× load shrink changes a 400px northwest crop offset into a 400px offset on the shrunk image rather than 100px, selecting the wrong source region; absolute gravity offsets need the same per-axis scale adjustment while fractional offsets should remain unchanged.
Useful? React with 👍 / 👎.
| let (crop_x, crop_y) = calc_position( | ||
| i64::from(resized_w), | ||
| i64::from(resized_h), | ||
| i64::from(crop_w), | ||
| i64::from(crop_h), | ||
| gravity, | ||
| 1.0, | ||
| false, |
There was a problem hiding this comment.
For a fill resize with dpr > 1 and an absolute gravity offset, apply_dpr enlarges the target dimensions but never changes gravity.x or gravity.y, so passing 1.0 here leaves (for example) a 10px offset at 10 output pixels instead of 20 for DPR 2. The adjacent comment also notes that imgproxy supplies its DPR scale; pass that scale into the fill positioning path so the crop selects the requested region.
Useful? React with 👍 / 👎.
| let name = match format.to_lowercase().as_str() { | ||
| "jpg" | "jpeg" => "jpeg", | ||
| "heic" | "heif" => "heif", | ||
| "tif" | "tiff" => "tiff", | ||
| other => return FORMATS.iter().find(|spec| spec.name == other), |
There was a problem hiding this comment.
When a URL requests format:tif, this new alias successfully selects the TIFF encoder, but process_path retains the original string and passes it to format_to_content_type, which recognizes only tiff and therefore falls back to image/jpeg. Clients receive TIFF bytes labeled as JPEG; return the canonical format from resolution or add the alias to the MIME mapping.
Useful? React with 👍 / 👎.
513b08c to
5e9661f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e9661f6bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(cached_image) = state.cache.get(cache_key.as_ref()).await { | ||
| debug!("Image found in cache for path={}", path); | ||
|
|
||
| return Ok(ProcessedImage { |
There was a problem hiding this comment.
The new post-fetch enforce_source_constraints call fixes cache misses, but a persistent cache hit still returns before that call. If a raw or skip_processing response was cached and the deployment later enables or tightens allowed_mime_types, max_src_resolution, or max_src_file_size, the same path continues serving the previously cached source bytes without applying the new policy; source constraints must either participate in the cache identity or be enforced for passthrough cache hits.
Useful? React with 👍 / 👎.
| config.ttl = parsed_var::<u64>(ENV_TTL)?; | ||
| config.cache_control_passthrough = bool_var(ENV_CACHE_CONTROL_PASSTHROUGH, false)?; | ||
| config.use_etag = bool_var(ENV_USE_ETAG, false)?; | ||
| config.last_modified_enabled = bool_var(ENV_LAST_MODIFIED_ENABLED, false)?; | ||
| config.set_canonical_header = bool_var(ENV_SET_CANONICAL_HEADER, false)?; |
There was a problem hiding this comment.
When any of IMGFORGE_TTL, cache-control passthrough, ETag, last-modified, or canonical-header support is enabled, these values are only stored in Config: process_path discards the corresponding FetchedImage headers and image_forge_handler emits only content type, cache status, and disposition. Consequently these newly accepted settings have no effect, including conditional requests never producing a 304.
Useful? React with 👍 / 👎.
| config.path_prefix = normalize_path_prefix(optional_var(ENV_PATH_PREFIX)?.as_deref()); | ||
| config.health_check_path = normalize_health_check_path(optional_var(ENV_HEALTH_CHECK_PATH)?.as_deref()); |
There was a problem hiding this comment.
Setting IMGFORGE_PATH_PREFIX or IMGFORGE_HEALTH_CHECK_PATH does not change routing: src/server.rs:71-89 still hard-codes /status, /info/{*path}, and /{*path} and never reads either field. A deployment that configures its liveness probe to the advertised health path therefore reaches the image wildcard instead of a health handler, while the requested prefix is not applied.
Useful? React with 👍 / 👎.
| config.user_agent = optional_var(ENV_USER_AGENT)? | ||
| .filter(|value| !value.trim().is_empty()) | ||
| .unwrap_or_else(|| DEFAULT_USER_AGENT.to_string()); | ||
| config.max_redirects = parsed_var::<usize>(ENV_MAX_REDIRECTS)?.unwrap_or(10); |
There was a problem hiding this comment.
When IMGFORGE_USER_AGENT or IMGFORGE_MAX_REDIRECTS is changed, source requests still use a client built solely from download_timeout in src/app.rs:161-164. Thus origins never receive the configured user agent and custom redirect limits are ignored, which can both break sources that require the advertised agent and allow more redirects than an operator intended.
Useful? React with 👍 / 👎.
| config.enable_webp_detection = bool_var(ENV_ENABLE_WEBP_DETECTION, false)?; | ||
| config.enforce_webp = bool_var(ENV_ENFORCE_WEBP, false)?; | ||
| config.enable_avif_detection = bool_var(ENV_ENABLE_AVIF_DETECTION, false)?; | ||
| config.enforce_avif = bool_var(ENV_ENFORCE_AVIF, false)?; | ||
| config.enable_client_hints = bool_var(ENV_ENABLE_CLIENT_HINTS, false)?; |
There was a problem hiding this comment.
Enabling WebP/AVIF detection, enforcement, or client hints cannot affect output because the handler never extracts Accept, Width, or DPR, and process_path always supplies negotiated_format: None. Requests therefore continue using the URL/default format under every combination of these settings, making all five newly parsed options ineffective.
Useful? React with 👍 / 👎.
| if same_space(current, target) { | ||
| return Ok(img); |
There was a problem hiding this comment.
For a wide-gamut RGB source such as a Display-P3 JPEG, libvips commonly reports the pixel interpretation as Srgb while retaining the embedded ICC profile. This early return therefore skips icc_transform even though the RGB values are profile-dependent, so subsequent processing treats P3 values as sRGB and can produce visibly shifted or oversaturated output; profile presence, not only the interpretation enum, must determine whether the transform runs.
Useful? React with 👍 / 👎.
5e9661f to
372ffea
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 372ffea26d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match options.format.as_deref() { | ||
| Some(requested) => matches_source(requested), | ||
| None => true, | ||
| } |
There was a problem hiding this comment.
When IMGFORGE_DEFAULT_FORMAT is fixed and the URL omits format, this None => true branch treats the requested output as matching the source. For example, a JPEG request with skip_processing:jpeg under a WebP default returns the original JPEG before process_path applies the configured WebP conversion. Compare skip_processing against the effective default format rather than assuming an absent URL format always matches.
Useful? React with 👍 / 👎.
| let quality = parsed_options | ||
| .quality | ||
| .or_else(|| parsed_options.save.format_quality.get(&output_format).copied()) | ||
| .unwrap_or(85); |
There was a problem hiding this comment.
When an alias is used in both options, such as format:tif/format_quality:tif:20 or format:jpg/format_quality:jpg:20, process_path canonicalizes the output to tiff or jpeg but leaves the map key as tif or jpg. This lookup therefore misses the requested quality and silently falls back to 85; canonicalize the format_quality keys or retain the original alias for lookup.
Useful? React with 👍 / 👎.
|
|
||
| // A limit only matters once it is below what was going to be read. | ||
| let count = match (count, options.max_animation_frames) { | ||
| (Some(count), Some(limit)) => Some(count.min(limit.get())), |
There was a problem hiding this comment.
When a request or preset supplies both pages:N and disable_animation:true, the Some(pages) arm wins and loads all requested pages, so an animation-capable output remains animated despite the explicit disable option. Since disable_animation is defined to collapse the source to one frame, it should force a count of one regardless of pages while still honoring the selected starting page.
Useful? React with 👍 / 👎.
372ffea to
6773bd4
Compare
options, transform and service had grown past a thousand lines each and were about to absorb a large compatibility release. Each is now a directory whose submodules own one concern: option groups parse their own arguments, transform stages sit beside the geometry they need, and the service separates cache identity, limits and source handling from request flow. The move carries several behaviour changes that the split made cheap: - gravity gains offsets and focus-point positioning, following imgproxy's calcPosition, and crop, fill and extend all route through it. A crop with no gravity now centres rather than pinning to the top-left, which is what imgproxy has always done. - crop extents below 1 are read as a fraction of the source. - resizing types are an enum and include fill-down. - zoom takes independent x and y factors. - extend takes its own gravity, and extend_aspect_ratio is implemented. - brightness and contrast are applied through vips_linear, which the crate does expose after all; the roadmap's note that it does not was wrong, and watermarking had been using it all along. - metadata keep flags are expressed as a combination, so stripping the colour profile no longer discards EXIF as a side effect. - every encoder goes through the save-suffix parser, which is what makes that combination expressible and keeps one code path across libvips versions. - an upstream 404 is reported as an upstream status instead of surfacing later as "failed to decode source image".
6773bd4 to
5d7b714
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d7b71405c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(thumbnail) = metadata::embedded_thumbnail(image_bytes) { | ||
| let thumbnail = Bytes::from(thumbnail); | ||
| match VipsImage::new_from_buffer(&thumbnail, "") { | ||
| Ok(img) => { | ||
| debug!("Using the source's embedded thumbnail ({} bytes)", thumbnail.len()); | ||
| return Ok((img, thumbnail)); |
There was a problem hiding this comment.
When enforce_thumbnail is enabled and a JPEG contains a typical small EXIF thumbnail, this branch selects it without comparing its dimensions with the requested resize, crop, minimum dimensions, DPR, or zoom. A request for a 1000px result with enlarge:false can therefore return only the 160×120 thumbnail, and a request without resize options returns the thumbnail instead of the full-sized source. Use the thumbnail only when it is large enough to satisfy the effective geometry; otherwise continue with the main image.
Useful? React with 👍 / 👎.
| match VipsImage::new_from_buffer(&thumbnail, "") { | ||
| Ok(img) => { | ||
| debug!("Using the source's embedded thumbnail ({} bytes)", thumbnail.len()); | ||
| return Ok((img, thumbnail)); |
There was a problem hiding this comment.
When enforce_thumbnail is combined with strip_metadata:true/keep_copyright:true, returning the extracted thumbnail as source_bytes discards the containing JPEG's EXIF block. process_image later calls read_copyright(source_bytes), so it reads the usually metadata-free thumbnail and silently fails to reattach the source's copyright; the same substitution also loses the parent EXIF orientation used by auto-rotation. Keep the original bytes available for metadata reads while using the thumbnail only as the pixel source.
Useful? React with 👍 / 👎.
| let options = ops::IccTransformOptions { | ||
| embedded: true, | ||
| intent: ops::Intent::Relative, | ||
| ..Default::default() | ||
| }; |
There was a problem hiding this comment.
For a profiled 16-bit source with preserve_hdr:true and a high-bit-depth output such as PNG or AVIF, IccTransformOptions::default() leaves libvips' transform depth at 8 bits. The successful embedded-profile path therefore quantizes the pixels before convert_colourspace converts them back to an Rgb16 interpretation, so the output has a 16-bit container but only 8-bit precision. Set the ICC output depth according to keep_high_bit_depth.
Useful? React with 👍 / 👎.
An undersized EXIF thumbnail was substituted unconditionally, so a 1000px request with enlarge:false came back at the thumbnail's own 160px, and a request with no resize at all got the thumbnail instead of the source. The stand-in is now taken only when it is at least as large as everything the request asks of it — resize, minimums, dpr and zoom folded in the same way the scale-on-load plan folds them — and never under a crop, trim, raw, or a force resize with a zero axis. The substitution also replaced the bytes downstream metadata reads came from, so keep_copyright read the metadata-free thumbnail and silently dropped the source's copyright, and auto-rotation lost the parent's EXIF orientation. Opening the source now hands back the decode bytes and the metadata bytes separately: sniffing and reduced-scale reopening see the bytes the pixels came from, while orientation and copyright always read the original source. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The transform's device space depth defaults to 8 bits, so a profiled 16-bit source bound for a high-bit-depth output was quantised on its way through — the later hop restored the Rgb16 interpretation around pixels that had already lost half their bits. The depth now follows the processing space the pipeline chose. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a7608cb74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| &decode_bytes, | ||
| source_content_type.as_deref(), | ||
| Some(&source_image), |
There was a problem hiding this comment.
When enforce_thumbnail selects an embedded thumbnail, decode_bytes and source_image both describe that thumbnail, so the subsequent max_src_resolution check measures (for example) 160×120 instead of the original 10000×10000 JPEG. This bypasses the configured source-resolution security ceiling whenever the thumbnail is large enough for the requested output; inspect the original image header for source constraints before substituting the thumbnail.
Useful? React with 👍 / 👎.
| (target_width < 1.0 || f64::from(thumb_width) >= target_width) | ||
| && (target_height < 1.0 || f64::from(thumb_height) >= target_height) |
There was a problem hiding this comment.
When auto-rotation is enabled and EXIF orientation 5–8 swaps the axes, this gate compares the requested geometry against the thumbnail before that rotation. For example, a stored 160×120 thumbnail passes a force:150:100 request, but orientation 6 turns it into 120×160 and enlarge:false then limits the result to 120×80 even though the full source could produce 150×100. Fresh evidence in the current revision is that the newly added size gate still compares the stored axes directly; swap the thumbnail dimensions for these orientations before deciding it covers the request.
Useful? React with 👍 / 👎.
Substituting the embedded thumbnail handed the security check the stand-in, so max_src_resolution judged a 10000px source at its thumbnail's size. Opening the source now keeps the original alongside the stand-in and the ceilings are measured on it — a source that will not even open no longer gets to hide behind a thumbnail that would pass. The size gate also compared the thumbnail's stored axes, but the parent's EXIF orientation applies to the thumbnail's pixels too, and orientations 5-8 transpose them after decoding. The gate now judges the shape the viewer gets, so a portrait request is neither accepted on the landscape numbers nor refused on them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d561eb886
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| config.use_etag = bool_var(ENV_USE_ETAG, false)?; | ||
| config.last_modified_enabled = bool_var(ENV_LAST_MODIFIED_ENABLED, false)?; | ||
| config.set_canonical_header = bool_var(ENV_SET_CANONICAL_HEADER, false)?; | ||
| config.allow_origin = optional_var(ENV_ALLOW_ORIGIN)?.filter(|value| !value.trim().is_empty()); |
There was a problem hiding this comment.
When IMGFORGE_ALLOW_ORIGIN is set, the value is stored here but never read again; image_forge_handler builds its response headers in src/handlers.rs:73-90 without Access-Control-Allow-Origin. Browser clients therefore remain unable to read image responses cross-origin despite enabling the advertised setting.
Useful? React with 👍 / 👎.
| config.allow_origin = optional_var(ENV_ALLOW_ORIGIN)?.filter(|value| !value.trim().is_empty()); | ||
| config.path_prefix = normalize_path_prefix(optional_var(ENV_PATH_PREFIX)?.as_deref()); | ||
| config.health_check_path = normalize_health_check_path(optional_var(ENV_HEALTH_CHECK_PATH)?.as_deref()); | ||
| config.development_errors_mode = bool_var(ENV_DEVELOPMENT_ERRORS_MODE, false)?; |
There was a problem hiding this comment.
When IMGFORGE_DEVELOPMENT_ERRORS_MODE=true, this flag has no effect because neither handler consults it and ServiceError::message in src/service/error.rs:77-114 always returns the same redacted text. Consequently fetch, decode, and encoder failures still produce generic responses, defeating the purpose of the newly accepted development setting.
Useful? React with 👍 / 👎.
| config.path_prefix = normalize_path_prefix(optional_var(ENV_PATH_PREFIX)?.as_deref()); | ||
| config.health_check_path = normalize_health_check_path(optional_var(ENV_HEALTH_CHECK_PATH)?.as_deref()); | ||
| config.development_errors_mode = bool_var(ENV_DEVELOPMENT_ERRORS_MODE, false)?; | ||
| config.enable_debug_headers = bool_var(ENV_ENABLE_DEBUG_HEADERS, false)?; |
There was a problem hiding this comment.
Setting IMGFORGE_ENABLE_DEBUG_HEADERS=true only stores this boolean; the processing result carries no origin metadata and src/handlers.rs:73-90 emits only content type, cache status, and disposition. Requests therefore never receive the promised X-Origin-* headers, so the new option is entirely ineffective.
Useful? React with 👍 / 👎.
1 participant
First of a stack of nine. Base:
main.options,transformandservicehad each grown past a thousand lines and were about to absorb a large compatibility release. Each is now a directory whose submodules own one concern: option groups parse their own arguments, transform stages sit beside the geometry they need, and the service separates cache identity, limits and source handling from request flow.The restructure introduced the types the rest of the stack needs — a
Gravitystruct with offsets, aResizingTypeenum, fractionalCropextents — so the behaviour changes that depend on them are here rather than in a follow-up that would have had to reshape them again.What changes
gravitygains offsets and focus-point positioning, following imgproxy'scalcPosition;crop,fillandextendall route through it.cropextents below 1 are read as a fraction of the source.fill-down.zoomtakes independent x and y factors.extendtakes its own gravity, andextend_aspect_ratiois implemented.brightnessandcontrastare applied throughvips_linear. The roadmap said the crate did not expose it; it does, and the watermark code had been calling it all along.keepflags are expressed as a combination, sostrip_color_profileno longer discards EXIF as a side effect. Every encoder moved to the save-suffix parser, which is what makes that combination expressible and keeps one code path across libvips versions.Behaviour changes reviewers should look at
crop:300:200URL will select a different region.:nowerestores the old behaviour.x_offset/y_offsetarguments.wm:0.5:soea:10:10reproduces it.brightnessandcontrastnow alter images that previously ignored them.Validation