From 1fab1dfb1e4ccfef36f5d9b7974fea6355db9efa Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 18:31:19 +0000 Subject: [PATCH] Reject a negative padding and a non-positive size [patch] Validate only checked padding's upper bound, which is not a bound at all for a negative number: -5 >= 64 is false. EffectivePadding then handed the value back unchanged, the content was resized to finalSize - padding * 2 -- larger than the canvas -- and ImageSharp's Pad never shrinks an image, so the output PNG came out bigger than the documented min(squareSize, size), quietly and with exit code 0. Size gets its own check for the same reason. It was only ever caught incidentally by the padding comparison derived from it, which stops being a net once padding is negative too: -20 >= -10 / 2 is false, so a negative size and a negative padding together passed validation untouched. A negative padding is reported on its own rather than alongside "less than half the size of the image", which is not the complaint about a negative number, and half of an invalid size is not a bound worth quoting. Fixes #115 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012betVHk3gFcj5RYkEe4vrm --- IconHelper.Test/ArgumentsTests.cs | 48 +++++++++++++++++++++++++++++++ IconHelper/Arguments.cs | 22 +++++++++++++- README.md | 4 +++ 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/IconHelper.Test/ArgumentsTests.cs b/IconHelper.Test/ArgumentsTests.cs index 4808371..dee59b1 100644 --- a/IconHelper.Test/ArgumentsTests.cs +++ b/IconHelper.Test/ArgumentsTests.cs @@ -77,6 +77,54 @@ public void ValidateRejectsPaddingEqualToHalfTheSize() Assert.AreEqual("Padding must be less than half the size of the image.", errors[0]); } + [TestMethod] + public void ValidateRejectsNegativePadding() + { + // The upper bound never caught this: -5 >= 64 is false. The run then produced an output + // larger than the documented min(squareSize, size) canvas and exited 0 to say it worked. + using TempDirectory temp = new(); + Arguments args = ValidArguments(temp); + args.Size = 128; + args.Padding = -5; + + bool valid = args.Validate(out Collection errors); + + Assert.IsFalse(valid, "Negative padding grows the canvas instead of insetting content."); + Assert.HasCount(1, errors); + Assert.AreEqual("Padding must not be negative.", errors[0]); + } + + [TestMethod] + public void ValidateRejectsASizeThatIsNotPositive() + { + using TempDirectory temp = new(); + Arguments args = ValidArguments(temp); + args.Size = 0; + args.Padding = 0; + + bool valid = args.Validate(out Collection errors); + + Assert.IsFalse(valid, "A size of zero leaves no canvas at all."); + Assert.HasCount(1, errors, $"Expected only the size error but got: {string.Join(", ", errors)}"); + Assert.AreEqual("Size must be greater than zero.", errors[0]); + } + + [TestMethod] + public void ValidateRejectsANegativeSizeAndPaddingTogether() + { + // The combination the upper bound was least able to catch: -20 >= -10 / 2 is false, so + // both values passed validation untouched and the pair reached the pipeline. + using TempDirectory temp = new(); + Arguments args = ValidArguments(temp); + args.Size = -10; + args.Padding = -20; + + bool valid = args.Validate(out Collection errors); + + Assert.IsFalse(valid, "Neither a negative size nor a negative padding is usable."); + Assert.HasCount(2, errors, $"Expected a size and a padding error but got: {string.Join(", ", errors)}"); + } + [TestMethod] public void ValidateUsesIntegerDivisionForOddSizes() { diff --git a/IconHelper/Arguments.cs b/IconHelper/Arguments.cs index fc7022c..0fc97ca 100644 --- a/IconHelper/Arguments.cs +++ b/IconHelper/Arguments.cs @@ -30,7 +30,27 @@ internal sealed class Arguments internal bool Validate(out Collection errors) { errors = []; - if (Padding >= Size / 2) + + if (Size <= 0) + { + errors.Add("Size must be greater than zero."); + } + + // The upper bound alone was not a bound. A negative padding sails past it -- -5 >= 64 is + // false -- and then EffectivePadding hands it back unchanged, so the content is resized to + // finalSize - padding * 2, which is larger than the canvas. ImageSharp's Pad never shrinks + // an image, so the output comes out bigger than the documented min(squareSize, size), + // quietly and with exit code 0. Reporting it here rather than clamping it downstream keeps + // the documented invariant -- padding insets content, it never changes the canvas -- true + // by construction. + // + // Reported on its own, because "less than half the size" is not the complaint about a + // negative number, and half of an invalid size is not a bound worth quoting either. + if (Padding < 0) + { + errors.Add("Padding must not be negative."); + } + else if (Size > 0 && Padding >= Size / 2) { errors.Add("Padding must be less than half the size of the image."); } diff --git a/README.md b/README.md index b186266..5cad723 100644 --- a/README.md +++ b/README.md @@ -247,6 +247,10 @@ the artwork leaves it alone. `finalSize - padding * 2` and then padded back out to `finalSize`, so the output is always `finalSize` square whatever the padding. +That holds only while the padding is positive, so a negative `--padding` is rejected rather than +clamped: it would resize the content *larger* than the canvas, and padding back out never shrinks +an image. `--size` must likewise be greater than zero. + Because the canvas is `min(squareSize, --size)` rather than `--size`, the padding is measured against that effective canvas and **clamped to `(finalSize - 1) / 2`** so at least one pixel of content survives. This only engages on artwork that trims smaller than `--size`: `--size 100 --padding 45`