From 1f636492d494456045b3b9d94f052bf6b2cad89b Mon Sep 17 00:00:00 2001 From: Ashwin A Nair Date: Mon, 4 May 2020 19:37:21 -0700 Subject: [PATCH 1/4] Initial Changes --- .../joyent/manta/client/MantaMetadata.java | 81 ++++++++++++++----- .../MantaIllegalMetadataException.java | 40 +++++++++ .../manta/client/MantaMetadataTest.java | 32 +++++++- 3 files changed, 132 insertions(+), 21 deletions(-) create mode 100644 java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaIllegalMetadataException.java diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java index 961a049d..5778d93e 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015-2019, Joyent, Inc. All rights reserved. + * Copyright (c) 2015-2020, Joyent, Inc. All rights reserved. * * This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this @@ -14,6 +14,7 @@ import org.apache.commons.lang3.builder.ToStringBuilder; import java.io.Serializable; +import java.nio.charset.CharsetEncoder; import java.nio.charset.StandardCharsets; import java.util.Collection; import java.util.Locale; @@ -44,26 +45,42 @@ public class MantaMetadata implements Map, Cloneable, Serializab * Prefix required for metadata keys being stored via HTTP headers on Manta. */ public static final String METADATA_PREFIX = "m-"; + /** * Prefix required for encrypted metadata keys being stored in ciphertext. */ public static final String ENCRYPTED_METADATA_PREFIX = "e-"; + /** * An array of characters considered to be illegal in metadata keys. */ static final char[] ILLEGAL_KEY_CHARS = "()<>@,;: innerMap; + /** + * Helper object for validating keys. + */ + private static final Predicate PREDICATE_KEY = new HttpHeaderPredicate(true); + + /** + * Helper object for validating values. + */ + private static final Predicate PREDICATE_VALUE = new HttpHeaderPredicate(false); + /** * Create a new instance backed with the specified map. + * * @param m the backing map */ public MantaMetadata(final Map m) { @@ -76,8 +93,7 @@ public MantaMetadata(final Map m) { */ public MantaMetadata() { final Map map = new CaseInsensitiveMap<>(); - final Predicate keyPredicate = new HttpHeaderNameKeyPredicate(); - this.innerMap = PredicatedMap.predicatedMap(map, keyPredicate, null); + this.innerMap = PredicatedMap.predicatedMap(map, PREDICATE_KEY, PREDICATE_VALUE); } @SuppressWarnings("MethodDoesntCallSuperMethod") @@ -93,11 +109,12 @@ public void removeAllEncrypted() { final Set> set = entrySet(); set.removeIf(entry -> entry.getKey() - .startsWith(ENCRYPTED_METADATA_PREFIX)); + .startsWith(ENCRYPTED_METADATA_PREFIX)); } /** * Deletes user-supplied metadata associated with a Manta object. + * * @param key key to delete */ public void delete(final String key) { @@ -253,7 +270,7 @@ public String toString() { for (Map.Entry entry : innerMap.entrySet()) { builder.append(" [").append(entry.getKey()).append("] = [") - .append(entry.getValue()).append("]\n"); + .append(entry.getValue()).append("]\n"); } return builder.toString(); @@ -262,30 +279,56 @@ public String toString() { /** * Implements the predicate used to validate header key values. */ - protected static class HttpHeaderNameKeyPredicate implements Predicate { + protected static class HttpHeaderPredicate implements Predicate { + + /** + * ASCII-based character set encoder to determine if the input is valid. + */ + private static final CharsetEncoder ASCII_ENCODER = StandardCharsets.US_ASCII.newEncoder(); + + /** + * Includes header prefix rule (m- and e-) which is only relevant for metadata header names and + * forbids null. + */ + private final boolean keyPredicate; + + /** + * Construct a header validator. + * + * @param keyPredicate Whether or not to include the prefix rule and forbid null. + */ + protected HttpHeaderPredicate(final boolean keyPredicate) { + this.keyPredicate = keyPredicate; + } /** * {@inheritDoc} */ @Override public boolean evaluate(final String object) { - return object != null - && !object.isEmpty() - && !hasIllegalChars(object) - && isIso88591(object) - && validPrefix(object); + // NEXT: throw MantaIllegalMetadataException instead + + if (keyPredicate) { + return object != null + && !object.isEmpty() + && validPrefix(object) + && !hasIllegalKeyChars(object) + && isAscii(object) + && validPrefix(object); + } else { + // delete is put(key, null) so we must permit a null value + return object == null || isAscii(object); + } } /** - * Test a string for iso8859-1 character encoding. + * Test a string for US ASCII character encoding. * * @param input string value to be tested * @return true if the string is entirely iso8859-1, false otherwise. */ - private boolean isIso88591(final String input) { - final byte[] bytes = input.getBytes(StandardCharsets.ISO_8859_1); - final String result = new String(bytes, StandardCharsets.ISO_8859_1); - return result.equals(input); + private boolean isAscii(final String input) { + return ASCII_ENCODER.canEncode(input); } /** @@ -296,7 +339,7 @@ private boolean isIso88591(final String input) { */ private boolean validPrefix(final String input) { return input.toLowerCase(Locale.ENGLISH).startsWith(METADATA_PREFIX) - || input.toLowerCase(Locale.ENGLISH).startsWith(ENCRYPTED_METADATA_PREFIX); + || input.toLowerCase(Locale.ENGLISH).startsWith(ENCRYPTED_METADATA_PREFIX); } /** @@ -305,7 +348,7 @@ private boolean validPrefix(final String input) { * @param input string value to be tested * @return true if the string contains illegal characters, false otherwise. */ - private boolean hasIllegalChars(final String input) { + private boolean hasIllegalKeyChars(final String input) { final char[] chars = input.toCharArray(); for (final char c : chars) { @@ -330,7 +373,7 @@ private boolean hasIllegalChars(final String input) { * @return true if the character is a control character, false otherwise. */ private boolean isControlCharacter(final char c) { - final int intVal = (int)c; + final int intVal = (int) c; return intVal < ASCIICODE_32_SPACE; } diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaIllegalMetadataException.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaIllegalMetadataException.java new file mode 100644 index 00000000..a1f0709a --- /dev/null +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaIllegalMetadataException.java @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2020, Joyent, Inc. All rights reserved. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ +package com.joyent.manta.exception; + +/** + * Exception while adding metadata. + * + * @author Ashwin A Nair + */ +public class MantaIllegalMetadataException extends MantaException { + + private static final long serialVersionUID = 3946821793051523289L; + + /** + * @param message The error message. + */ + public MantaIllegalMetadataException(final String message) { + super(message); + } + + /** + * @param cause The cause of the exception. + */ + public MantaIllegalMetadataException(final Throwable cause) { + super(cause); + } + + /** + * @param message The error message. + * @param cause The cause. + */ + public MantaIllegalMetadataException(final String message, final Throwable cause) { + super(message, cause); + } +} diff --git a/java-manta-client-unshaded/src/test/java/com/joyent/manta/client/MantaMetadataTest.java b/java-manta-client-unshaded/src/test/java/com/joyent/manta/client/MantaMetadataTest.java index 0be2ec0d..93542fac 100644 --- a/java-manta-client-unshaded/src/test/java/com/joyent/manta/client/MantaMetadataTest.java +++ b/java-manta-client-unshaded/src/test/java/com/joyent/manta/client/MantaMetadataTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015-2017, Joyent, Inc. All rights reserved. + * Copyright (c) 2015-2020, Joyent, Inc. All rights reserved. * * This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this @@ -7,13 +7,18 @@ */ package com.joyent.manta.client; +import com.joyent.manta.exception.MantaIllegalMetadataException; import org.testng.Assert; import org.testng.annotations.Test; +import java.nio.charset.CharsetEncoder; +import java.nio.charset.StandardCharsets; + /** * Unit test class that verifies the correct functioning of {@link MantaMetadata}. * * @author Elijah Zupancic + * @author Ashwin A Nair */ @SuppressWarnings("ModifiedButNotUsed") // Errorprone is confused by these tests public class MantaMetadataTest { @@ -65,7 +70,7 @@ public void cantAddMetadataKeyThatIllegalCharacter() { try { instance.put(key, "world"); - } catch (IllegalArgumentException e) { + } catch (final IllegalArgumentException e) { caught = true; } @@ -73,6 +78,29 @@ public void cantAddMetadataKeyThatIllegalCharacter() { } } + /* Until MANTA-3527 is resolved we should prevent users from using non-ascii values for both keys + and values */ + @Test + public void cantAddMetadataKeyWithNonAsciiCharacters() { + final CharsetEncoder asciiEncoder = StandardCharsets.US_ASCII.newEncoder(); + + final char[] firstNonAsciiCharacter = Character.toChars(128); + final String badChar = String.format("%s", firstNonAsciiCharacter[0]); + Assert.assertFalse(asciiEncoder.canEncode(badChar)); + + final MantaMetadata instance = new MantaMetadata(); + final MantaIllegalMetadataException keyEx = Assert.expectThrows(MantaIllegalMetadataException.class, () -> + instance.put(String.format("m-%s", badChar), "value")); + final MantaIllegalMetadataException valEx = Assert.expectThrows(MantaIllegalMetadataException.class, () -> + instance.put("m-key", badChar)); + final MantaIllegalMetadataException bothEx = Assert.expectThrows(MantaIllegalMetadataException.class, () -> + instance.put(String.format("m-%s", badChar), badChar)); + + Assert.assertTrue(keyEx.getMessage().contains("ASCII")); + Assert.assertTrue(valEx.getMessage().contains("ASCII")); + Assert.assertTrue(bothEx.getMessage().contains("ASCII")); + } + @Test public void canMarkKeyAsDeleted() { MantaMetadata instance = new MantaMetadata(); From 20a440ea062cc61d4a1045d2360d02fb422b933b Mon Sep 17 00:00:00 2001 From: Ashwin A Nair Date: Mon, 4 May 2020 20:19:03 -0700 Subject: [PATCH 2/4] Binary Search optimization --- .../com/joyent/manta/client/MantaMetadata.java | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java index 5778d93e..a31eb9fb 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java @@ -16,11 +16,7 @@ import java.io.Serializable; import java.nio.charset.CharsetEncoder; import java.nio.charset.StandardCharsets; -import java.util.Collection; -import java.util.Locale; -import java.util.Map; -import java.util.Set; -import java.util.Objects; +import java.util.*; import java.util.function.BiConsumer; import java.util.function.BiFunction; import java.util.function.Function; @@ -350,17 +346,13 @@ private boolean validPrefix(final String input) { */ private boolean hasIllegalKeyChars(final String input) { final char[] chars = input.toCharArray(); + Arrays.sort(ILLEGAL_KEY_CHARS); for (final char c : chars) { - if (isControlCharacter(c)) { + final int illegalKeyCharPresent = Arrays.binarySearch(ILLEGAL_KEY_CHARS, c); + if (isControlCharacter(c) || illegalKeyCharPresent >= 0) { return true; } - - for (char illegalKeyChar : ILLEGAL_KEY_CHARS) { - if (c == illegalKeyChar) { - return true; - } - } } return false; From 107dfdcf8696ebf038f23d04ee2bcc531de25c7d Mon Sep 17 00:00:00 2001 From: Ashwin A Nair Date: Mon, 4 May 2020 20:33:04 -0700 Subject: [PATCH 3/4] Suppress checkstyle warnings --- .../main/java/com/joyent/manta/client/MantaMetadata.java | 7 ++++++- .../manta/exception/MantaIllegalMetadataException.java | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java index a31eb9fb..2b453c1d 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java @@ -16,7 +16,12 @@ import java.io.Serializable; import java.nio.charset.CharsetEncoder; import java.nio.charset.StandardCharsets; -import java.util.*; +import java.util.Arrays; +import java.util.Collection; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import java.util.Objects; import java.util.function.BiConsumer; import java.util.function.BiFunction; import java.util.function.Function; diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaIllegalMetadataException.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaIllegalMetadataException.java index a1f0709a..1e159e8a 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaIllegalMetadataException.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaIllegalMetadataException.java @@ -12,6 +12,7 @@ * * @author Ashwin A Nair */ +@SuppressWarnings("unused") public class MantaIllegalMetadataException extends MantaException { private static final long serialVersionUID = 3946821793051523289L; From a59a97268f26b73210ef6c22f3bf998d9873dda6 Mon Sep 17 00:00:00 2001 From: Ashwin A Nair Date: Wed, 6 May 2020 14:53:19 -0700 Subject: [PATCH 4/4] MantaInvalidMetadataException for non-ASCII values --- .../joyent/manta/client/MantaMetadata.java | 13 ++++++++---- ...ava => MantaInvalidMetadataException.java} | 12 ++++++----- .../manta/client/MantaMetadataTest.java | 20 +++++++++---------- 3 files changed, 26 insertions(+), 19 deletions(-) rename java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/{MantaIllegalMetadataException.java => MantaInvalidMetadataException.java} (70%) diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java index 2b453c1d..c2dc0493 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaMetadata.java @@ -7,6 +7,7 @@ */ package com.joyent.manta.client; +import com.joyent.manta.exception.MantaInvalidMetadataException; import com.joyent.manta.util.NotThreadSafe; import org.apache.commons.collections4.Predicate; import org.apache.commons.collections4.map.CaseInsensitiveMap; @@ -249,8 +250,14 @@ public void clear() { } @Override - public String put(final String key, final String value) { - return innerMap.put(key, value); + public String put(final String key, final String value) throws IllegalArgumentException { + try { + return innerMap.put(key, value); + } catch (IllegalArgumentException e) { + final String msg = String.format("[%s, %s] is in non-ASCII format", + key, value); + throw new MantaInvalidMetadataException(msg, e); + } } @Override @@ -307,8 +314,6 @@ protected HttpHeaderPredicate(final boolean keyPredicate) { */ @Override public boolean evaluate(final String object) { - // NEXT: throw MantaIllegalMetadataException instead - if (keyPredicate) { return object != null && !object.isEmpty() diff --git a/java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaIllegalMetadataException.java b/java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaInvalidMetadataException.java similarity index 70% rename from java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaIllegalMetadataException.java rename to java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaInvalidMetadataException.java index 1e159e8a..daaaedf1 100644 --- a/java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaIllegalMetadataException.java +++ b/java-manta-client-unshaded/src/main/java/com/joyent/manta/exception/MantaInvalidMetadataException.java @@ -11,23 +11,25 @@ * Exception while adding metadata. * * @author Ashwin A Nair + * @since 3.5.0 */ -@SuppressWarnings("unused") -public class MantaIllegalMetadataException extends MantaException { +public class MantaInvalidMetadataException extends MantaClientException { private static final long serialVersionUID = 3946821793051523289L; /** * @param message The error message. */ - public MantaIllegalMetadataException(final String message) { + @SuppressWarnings("unused") + public MantaInvalidMetadataException(final String message) { super(message); } /** * @param cause The cause of the exception. */ - public MantaIllegalMetadataException(final Throwable cause) { + @SuppressWarnings("unused") + public MantaInvalidMetadataException(final Throwable cause) { super(cause); } @@ -35,7 +37,7 @@ public MantaIllegalMetadataException(final Throwable cause) { * @param message The error message. * @param cause The cause. */ - public MantaIllegalMetadataException(final String message, final Throwable cause) { + public MantaInvalidMetadataException(final String message, final Throwable cause) { super(message, cause); } } diff --git a/java-manta-client-unshaded/src/test/java/com/joyent/manta/client/MantaMetadataTest.java b/java-manta-client-unshaded/src/test/java/com/joyent/manta/client/MantaMetadataTest.java index 93542fac..bdf880e4 100644 --- a/java-manta-client-unshaded/src/test/java/com/joyent/manta/client/MantaMetadataTest.java +++ b/java-manta-client-unshaded/src/test/java/com/joyent/manta/client/MantaMetadataTest.java @@ -7,7 +7,7 @@ */ package com.joyent.manta.client; -import com.joyent.manta.exception.MantaIllegalMetadataException; +import com.joyent.manta.exception.MantaInvalidMetadataException; import org.testng.Assert; import org.testng.annotations.Test; @@ -29,31 +29,31 @@ public void canAddMetadataKeyValue() { Assert.assertEquals(instance.get("m-hello"), "world"); } - @Test(expectedExceptions = {IllegalArgumentException.class}) + @Test(expectedExceptions = {MantaInvalidMetadataException.class}) public void cantAddNullMetadataKey() { MantaMetadata instance = new MantaMetadata(); instance.put(null, "world"); } - @Test(expectedExceptions = {IllegalArgumentException.class}) + @Test(expectedExceptions = {MantaInvalidMetadataException.class}) public void cantAddEmptyMetadataKey() { MantaMetadata instance = new MantaMetadata(); instance.put("", "world"); } - @Test(expectedExceptions = {IllegalArgumentException.class}) + @Test(expectedExceptions = {MantaInvalidMetadataException.class}) public void cantAddMetadataKeyThatDoesntBeginWithM() { MantaMetadata instance = new MantaMetadata(); instance.put("hello", "world"); } - @Test(expectedExceptions = {IllegalArgumentException.class}) + @Test(expectedExceptions = {MantaInvalidMetadataException.class}) public void cantAddMetadataKeyThatContainsSpace() { MantaMetadata instance = new MantaMetadata(); instance.put("m-hello my dear", "world"); } - @Test(expectedExceptions = {IllegalArgumentException.class}) + @Test(expectedExceptions = {MantaInvalidMetadataException.class}) public void cantAddMetadataKeyThatIsntISO88591() { MantaMetadata instance = new MantaMetadata(); String key = "m-\u3053\u3093\u306B\u3061\u306F"; @@ -70,7 +70,7 @@ public void cantAddMetadataKeyThatIllegalCharacter() { try { instance.put(key, "world"); - } catch (final IllegalArgumentException e) { + } catch (final MantaInvalidMetadataException e) { caught = true; } @@ -89,11 +89,11 @@ public void cantAddMetadataKeyWithNonAsciiCharacters() { Assert.assertFalse(asciiEncoder.canEncode(badChar)); final MantaMetadata instance = new MantaMetadata(); - final MantaIllegalMetadataException keyEx = Assert.expectThrows(MantaIllegalMetadataException.class, () -> + final MantaInvalidMetadataException keyEx = Assert.expectThrows(MantaInvalidMetadataException.class, () -> instance.put(String.format("m-%s", badChar), "value")); - final MantaIllegalMetadataException valEx = Assert.expectThrows(MantaIllegalMetadataException.class, () -> + final MantaInvalidMetadataException valEx = Assert.expectThrows(MantaInvalidMetadataException.class, () -> instance.put("m-key", badChar)); - final MantaIllegalMetadataException bothEx = Assert.expectThrows(MantaIllegalMetadataException.class, () -> + final MantaInvalidMetadataException bothEx = Assert.expectThrows(MantaInvalidMetadataException.class, () -> instance.put(String.format("m-%s", badChar), badChar)); Assert.assertTrue(keyEx.getMessage().contains("ASCII"));