Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CODEC-314: Fix possible IndexOutOfBoundsException thrown by PercentCodec.insertAlwaysEncodeChars() method #222

Closed
wants to merge 2 commits into from
Closed

CODEC-314: Fix possible IndexOutOfBoundsException thrown by PercentCodec.insertAlwaysEncodeChars() method #222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
009b192
CODEC-314: Fix possible IndexOutOfBoundsException
arthurscchan Nov 21, 2023
e193cd6
CODEC-314: Add unit test
arthurscchan Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/main/java/org/apache/commons/codec/net/PercentCodec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,15 @@ public PercentCodec() {
*
* @param alwaysEncodeChars the unsafe characters that should always be encoded
* @param plusForSpace the flag defining if the space character should be encoded as '+'
* @throws EncoderException if the alwaysEncodeChars byte array contains invalid bytes
*/
public PercentCodec(final byte[] alwaysEncodeChars, final boolean plusForSpace) {
public PercentCodec(final byte[] alwaysEncodeChars, final boolean plusForSpace) throws EncoderException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @arthurscchan
Now that I look at this again, it doesn't make sense to me, this seems like the exact use case for an IllegalArgumentException. In addition, the constructor does not encode anything, so an EncoderException really does not make sense IMO.

I propose:

diff --git a/src/main/java/org/apache/commons/codec/net/PercentCodec.java b/src/main/java/org/apache/commons/codec/net/PercentCodec.java
index 8e51538..df623df 100644
--- a/src/main/java/org/apache/commons/codec/net/PercentCodec.java
+++ b/src/main/java/org/apache/commons/codec/net/PercentCodec.java
@@ -231,6 +231,9 @@
      * @param b the byte that is candidate for min and max limit
      */
     private void insertAlwaysEncodeChar(final byte b) {
+        if (b < 0) {
+            throw new IllegalArgumentException("byte must be >= 0");
+        }
         this.alwaysEncodeChars.set(b);
         if (b < alwaysEncodeCharsMin) {
             alwaysEncodeCharsMin = b;
diff --git a/src/test/java/org/apache/commons/codec/net/PercentCodecTest.java b/src/test/java/org/apache/commons/codec/net/PercentCodecTest.java
index 34f8ecb..a537efb 100644
--- a/src/test/java/org/apache/commons/codec/net/PercentCodecTest.java
+++ b/src/test/java/org/apache/commons/codec/net/PercentCodecTest.java
@@ -106,6 +106,12 @@
     }
 
     @Test
+    public void testInvalidByte() throws Exception {
+        final byte[] invalid = { (byte) -1, (byte) 'A' };
+        assertThrows(IllegalArgumentException.class, () -> new PercentCodec(invalid, true));
+    }
+
+    @Test
     public void testPercentEncoderDecoderWithNullOrEmptyInput() throws Exception {
         final PercentCodec percentCodec = new PercentCodec(null, true);
         assertNull(percentCodec.encode(null), "Null input value encoding test");

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it is a better changes and interpretation of the exceptions. I will change it accordingly.
@garydgregory

this.plusForSpace = plusForSpace;
insertAlwaysEncodeChars(alwaysEncodeChars);
try {
insertAlwaysEncodeChars(alwaysEncodeChars);
} catch (IndexOutOfBoundsException e) {
throw new EncoderException(e);
}
}

private boolean canEncode(final byte c) {
Expand Down
7 changes: 7 additions & 0 deletions src/test/java/org/apache/commons/codec/net/PercentCodecTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,13 @@ public void testEncodeUnsupportedObject() {
assertThrows(EncoderException.class, () -> percentCodec.encode("test"));
}

@Test
public void testInvalidByte() throws Exception {
final byte[] invalid = {(byte)-1, (byte)'A'};

assertThrows(EncoderException.class, () -> new PercentCodec(invalid, true));
}

@Test
public void testPercentEncoderDecoderWithNullOrEmptyInput() throws Exception {
final PercentCodec percentCodec = new PercentCodec(null, true);
Expand Down