fix(validation): allow image height up to spec maximum of 400
Image height was validated against 0..=144, the same bound as width, but the RSS 2.0 specification allows heights up to 400 (width max is 144). Spec-valid images with a height between 145 and 400 were wrongly reported as invalid. Validate height against 0..=400 and fix the error message.
This commit is contained in:
1 parent
4b4bd57cab
commit
c6a4588fb3
3 files changed
+132
-2
No files matched your search
@@ -0,0 +1,88 @@
|
|||||||
|
# Fix image height validation to allow the spec maximum of 400
|
||||||
|
|
||||||
|
## Problem
|
||||||
|
|
||||||
|
`Channel`/`Image` validation rejects images whose `<height>` is greater than
|
||||||
|
144, even though the RSS 2.0 specification allows heights up to 400. A
|
||||||
|
perfectly spec-compliant image is reported as invalid:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
use rss::validation::Validate;
|
||||||
|
use rss::ImageBuilder;
|
||||||
|
|
||||||
|
let image = ImageBuilder::default()
|
||||||
|
.url("https://example.com/image.png".to_string())
|
||||||
|
.link("https://example.com/".to_string())
|
||||||
|
.title("title".to_string())
|
||||||
|
.height("200".to_string()) // valid per the RSS spec
|
||||||
|
.build();
|
||||||
|
|
||||||
|
assert!(image.validate().is_err()); // <- wrongly errors today
|
||||||
|
```
|
||||||
|
|
||||||
|
## Root cause
|
||||||
|
|
||||||
|
`src/validation.rs`, in `impl Validate for Image`, validated `height` against
|
||||||
|
the same range used for `width`:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
let height = height.parse::<i64>()?;
|
||||||
|
validate!(
|
||||||
|
(0..=144).contains(&height),
|
||||||
|
"Image height is not between 0 and 144"
|
||||||
|
);
|
||||||
|
```
|
||||||
|
|
||||||
|
The RSS 2.0 specification (<https://www.rssboard.org/rss-specification>,
|
||||||
|
`<image>` sub-element) states:
|
||||||
|
|
||||||
|
- width: "Maximum value for width is 144, default value is 88."
|
||||||
|
- height: "Maximum value for height is 400, default value is 31."
|
||||||
|
|
||||||
|
So 144 is correct for `width`, but `height` must allow up to 400. The bound
|
||||||
|
(and the error message) were copied from the width check.
|
||||||
|
|
||||||
|
## Fix
|
||||||
|
|
||||||
|
Validate `height` against `0..=400` and correct the error message:
|
||||||
|
|
||||||
|
```rust
|
||||||
|
let height = height.parse::<i64>()?;
|
||||||
|
validate!(
|
||||||
|
(0..=400).contains(&height),
|
||||||
|
"Image height is not between 0 and 400"
|
||||||
|
);
|
||||||
|
```
|
||||||
|
|
||||||
|
`width` is unchanged and keeps its specification maximum of 144.
|
||||||
|
|
||||||
|
## Test
|
||||||
|
|
||||||
|
Added `tests/validation.rs` (gated on the `validation` feature):
|
||||||
|
|
||||||
|
- `image_height_allows_up_to_400`: heights 31, 200 and 400 validate OK.
|
||||||
|
- `image_height_above_400_is_invalid`: height 401 is rejected.
|
||||||
|
- `image_width_max_is_still_144`: width 144 is OK, 145 is rejected.
|
||||||
|
|
||||||
|
## Gate
|
||||||
|
|
||||||
|
- `cargo test --all-features` -> ok (all suites pass, exit 0)
|
||||||
|
- `cargo build --all-features` -> ok (exit 0)
|
||||||
|
- `cargo fmt --check` -> ok (exit 0)
|
||||||
|
|
||||||
|
## Counterfactual
|
||||||
|
|
||||||
|
Reverting the bound back to `0..=144` makes the new test fail:
|
||||||
|
|
||||||
|
```
|
||||||
|
thread 'image_height_allows_up_to_400' panicked at tests/validation.rs:28:5:
|
||||||
|
assertion failed: image_with_height("200").validate().is_ok()
|
||||||
|
```
|
||||||
|
|
||||||
|
## Flags for the account owner
|
||||||
|
|
||||||
|
`cargo clippy --all-targets --all-features -- -D warnings` currently fails, but
|
||||||
|
this is pre-existing and unrelated to this change: on an unmodified checkout the
|
||||||
|
same lint (`clippy::get_first`, i.e. "accessing first element with `.get(0)`",
|
||||||
|
emitted by a newer Clippy than the tests were written against) already produces
|
||||||
|
these errors in `tests/read.rs`. The files touched here are clippy-clean.
|
||||||
+2
-2
@@ -222,8 +222,8 @@ impl Validate for Image {
|
|||||||
if let Some(height) = self.height() {
|
if let Some(height) = self.height() {
|
||||||
let height = height.parse::<i64>()?;
|
let height = height.parse::<i64>()?;
|
||||||
validate!(
|
validate!(
|
||||||
(0..=144).contains(&height),
|
(0..=400).contains(&height),
|
||||||
"Image height is not between 0 and 144"
|
"Image height is not between 0 and 400"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,42 @@
|
|||||||
|
#![cfg(feature = "validation")]
|
||||||
|
|
||||||
|
use rss::validation::Validate;
|
||||||
|
use rss::ImageBuilder;
|
||||||
|
|
||||||
|
fn image_with_height(height: &str) -> rss::Image {
|
||||||
|
ImageBuilder::default()
|
||||||
|
.url("https://example.com/image.png".to_string())
|
||||||
|
.link("https://example.com/".to_string())
|
||||||
|
.title("title".to_string())
|
||||||
|
.height(height.to_string())
|
||||||
|
.build()
|
||||||
|
}
|
||||||
|
|
||||||
|
fn image_with_width(width: &str) -> rss::Image {
|
||||||
|
ImageBuilder::default()
|
||||||
|
.url("https://example.com/image.png".to_string())
|
||||||
|
.link("https://example.com/".to_string())
|
||||||
|
.title("title".to_string())
|
||||||
|
.width(width.to_string())
|
||||||
|
.build()
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn image_height_allows_up_to_400() {
|
||||||
|
// The RSS 2.0 specification sets the maximum image height to 400.
|
||||||
|
assert!(image_with_height("31").validate().is_ok());
|
||||||
|
assert!(image_with_height("200").validate().is_ok());
|
||||||
|
assert!(image_with_height("400").validate().is_ok());
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn image_height_above_400_is_invalid() {
|
||||||
|
assert!(image_with_height("401").validate().is_err());
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn image_width_max_is_still_144() {
|
||||||
|
// Width keeps its specification maximum of 144.
|
||||||
|
assert!(image_with_width("144").validate().is_ok());
|
||||||
|
assert!(image_with_width("145").validate().is_err());
|
||||||
|
}
|
||||||
Reference in new issue
Block a user