From fcc3cb71d4e54cc85572c9384de47038f82f1375 Mon Sep 17 00:00:00 2001 From: 0xbigapple Date: Tue, 25 Aug 2026 19:06:09 +0800 Subject: [PATCH 1/5] fix(api): reject a contract query for a malformed address getContract and getContractInfo answer null for an address that is not well formed, before the account store is read. --- .../src/main/java/org/tron/core/Wallet.java | 6 + .../WalletContractAddressValidationTest.java | 121 ++++++++++++++++++ .../java/org/tron/core/WalletMockTest.java | 8 +- 3 files changed, 133 insertions(+), 2 deletions(-) create mode 100644 framework/src/test/java/org/tron/core/WalletContractAddressValidationTest.java diff --git a/framework/src/main/java/org/tron/core/Wallet.java b/framework/src/main/java/org/tron/core/Wallet.java index ac54cb2b7ff..fbf1370edff 100755 --- a/framework/src/main/java/org/tron/core/Wallet.java +++ b/framework/src/main/java/org/tron/core/Wallet.java @@ -3201,6 +3201,9 @@ public Transaction callConstantContract(TransactionCapsule trxCap, public SmartContract getContract(GrpcAPI.BytesMessage bytesMessage) { byte[] address = bytesMessage.getValue().toByteArray(); + if (!DecodeUtil.addressValid(address)) { + return null; + } AccountCapsule accountCapsule = chainBaseManager.getAccountStore().get(address); if (accountCapsule == null) { logger.warn( @@ -3230,6 +3233,9 @@ public SmartContract getContract(GrpcAPI.BytesMessage bytesMessage) { */ public SmartContractDataWrapper getContractInfo(GrpcAPI.BytesMessage bytesMessage) { byte[] address = bytesMessage.getValue().toByteArray(); + if (!DecodeUtil.addressValid(address)) { + return null; + } AccountCapsule accountCapsule = chainBaseManager.getAccountStore().get(address); if (accountCapsule == null) { logger.warn( diff --git a/framework/src/test/java/org/tron/core/WalletContractAddressValidationTest.java b/framework/src/test/java/org/tron/core/WalletContractAddressValidationTest.java new file mode 100644 index 00000000000..9063bc7a0e0 --- /dev/null +++ b/framework/src/test/java/org/tron/core/WalletContractAddressValidationTest.java @@ -0,0 +1,121 @@ +package org.tron.core; + +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; +import static org.mockito.AdditionalMatchers.aryEq; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import com.google.protobuf.ByteString; +import java.lang.reflect.Field; +import java.util.Arrays; +import org.junit.BeforeClass; +import org.junit.Test; +import org.mockito.MockedStatic; +import org.tron.api.GrpcAPI.BytesMessage; +import org.tron.common.utils.Base58; +import org.tron.core.capsule.AccountCapsule; +import org.tron.core.capsule.ContractCapsule; +import org.tron.core.store.AbiStore; +import org.tron.core.store.AccountStore; +import org.tron.core.store.ContractStore; +import org.tron.protos.contract.SmartContractOuterClass.SmartContract; + +public class WalletContractAddressValidationTest { + + // the stores are re-mocked per case, but the wallet itself carries no state these tests read; + // constructing one generates a keypair, so it is built once rather than once per address + private static Wallet wallet; + private static Field chainBaseManagerField; + + @BeforeClass + public static void setUpClass() throws Exception { + wallet = new Wallet(); + chainBaseManagerField = Wallet.class.getDeclaredField("chainBaseManager"); + chainBaseManagerField.setAccessible(true); + } + + /** + * Installs a chain base manager whose account store finds nothing, and hands back that store so + * the caller can assert whether it was ever consulted. + */ + private static AccountStore installMissingAccount() throws Exception { + ChainBaseManager chainBaseManager = mock(ChainBaseManager.class); + AccountStore accountStore = mock(AccountStore.class); + when(accountStore.get(any(byte[].class))).thenReturn((AccountCapsule) null); + when(chainBaseManager.getAccountStore()).thenReturn(accountStore); + chainBaseManagerField.set(wallet, chainBaseManager); + return accountStore; + } + + private static BytesMessage bytes(byte[] raw) { + return BytesMessage.newBuilder().setValue(ByteString.copyFrom(raw)).build(); + } + + private static byte[] canonicalAddress() { + byte[] address = new byte[21]; + Arrays.fill(address, (byte) 1); + address[0] = Wallet.getAddressPreFixByte(); + return address; + } + + @Test + public void invalidAddressesAreRejectedBeforeStorageOrBase58() throws Exception { + byte[][] invalidAddresses = { + new byte[0], + new byte[20], + new byte[22], + new byte[32 * 1024], + canonicalAddress() + }; + invalidAddresses[4][0] = (byte) (Wallet.getAddressPreFixByte() + 1); + + for (byte[] invalidAddress : invalidAddresses) { + AccountStore accountStore = installMissingAccount(); + try (MockedStatic base58 = mockStatic(Base58.class)) { + assertNull(wallet.getContract(bytes(invalidAddress))); + assertNull(wallet.getContractInfo(bytes(invalidAddress))); + verifyNoInteractions(accountStore); + base58.verifyNoInteractions(); + } + } + } + + @Test + public void canonicalMissingAddressRetainsNoResultBehavior() throws Exception { + AccountStore accountStore = installMissingAccount(); + byte[] address = canonicalAddress(); + + assertNull(wallet.getContract(bytes(address))); + assertNull(wallet.getContractInfo(bytes(address))); + + verify(accountStore, times(2)).get(aryEq(address)); + } + + @Test + public void canonicalExistingAddressRetainsContractResult() throws Exception { + ChainBaseManager chainBaseManager = mock(ChainBaseManager.class); + AccountStore accountStore = mock(AccountStore.class); + ContractStore contractStore = mock(ContractStore.class); + AbiStore abiStore = mock(AbiStore.class); + ContractCapsule contractCapsule = mock(ContractCapsule.class); + SmartContract contract = SmartContract.newBuilder().setName("existing").build(); + byte[] address = canonicalAddress(); + + when(chainBaseManager.getAccountStore()).thenReturn(accountStore); + when(chainBaseManager.getContractStore()).thenReturn(contractStore); + when(chainBaseManager.getAbiStore()).thenReturn(abiStore); + when(accountStore.get(any(byte[].class))).thenReturn(mock(AccountCapsule.class)); + when(contractStore.get(any(byte[].class))).thenReturn(contractCapsule); + when(contractCapsule.getInstance()).thenReturn(contract); + when(abiStore.get(any(byte[].class))).thenReturn(null); + chainBaseManagerField.set(wallet, chainBaseManager); + + assertSame(contract, wallet.getContract(bytes(address))); + } +} diff --git a/framework/src/test/java/org/tron/core/WalletMockTest.java b/framework/src/test/java/org/tron/core/WalletMockTest.java index 2f4c08d8f9f..7874af854a6 100644 --- a/framework/src/test/java/org/tron/core/WalletMockTest.java +++ b/framework/src/test/java/org/tron/core/WalletMockTest.java @@ -1399,8 +1399,10 @@ public void testBuildShieldedTRC20Input() throws Exception { @Test public void testGetContractInfo() throws Exception { Wallet wallet = new Wallet(); + byte[] address = new byte[21]; + address[0] = Wallet.getAddressPreFixByte(); GrpcAPI.BytesMessage bytesMessage = GrpcAPI.BytesMessage.newBuilder() - .setValue(ByteString.copyFrom("test".getBytes())) + .setValue(ByteString.copyFrom(address)) .build(); ChainBaseManager chainBaseManagerMock = mock(ChainBaseManager.class); @@ -1419,8 +1421,10 @@ public void testGetContractInfo() throws Exception { @Test public void testGetContractInfo1() throws Exception { Wallet wallet = new Wallet(); + byte[] address = new byte[21]; + address[0] = Wallet.getAddressPreFixByte(); GrpcAPI.BytesMessage bytesMessage = GrpcAPI.BytesMessage.newBuilder() - .setValue(ByteString.copyFrom("test".getBytes())) + .setValue(ByteString.copyFrom(address)) .build(); ChainBaseManager chainBaseManagerMock = mock(ChainBaseManager.class); From 74ae1ef12fc36f7e3df44f56b3ca579fd0c12c25 Mon Sep 17 00:00:00 2001 From: 0xbigapple Date: Tue, 25 Aug 2026 19:07:21 +0800 Subject: [PATCH 2/5] fix(api): bound json numbers before conversion A string or number longer than 64 characters is rejected before BigDecimal sees it, and Permission_id must convert identically through both coercion paths, so a value one path silently reinterprets no longer gets through. --- .../org/tron/core/services/http/Util.java | 50 ++++++- .../tron/core/services/http/BaseHttpTest.java | 16 ++- .../http/GetExchangeByIdServletTest.java | 15 +++ .../http/JsonLongValueValidationTest.java | 97 ++++++++++++++ .../services/http/TransferServletTest.java | 99 ++++++++++++++ .../org/tron/core/services/http/UtilTest.java | 123 +++++++++++++++++- 6 files changed, 392 insertions(+), 8 deletions(-) create mode 100644 framework/src/test/java/org/tron/core/services/http/JsonLongValueValidationTest.java diff --git a/framework/src/main/java/org/tron/core/services/http/Util.java b/framework/src/main/java/org/tron/core/services/http/Util.java index 5be2495e1f7..85057421670 100644 --- a/framework/src/main/java/org/tron/core/services/http/Util.java +++ b/framework/src/main/java/org/tron/core/services/http/Util.java @@ -69,6 +69,9 @@ public class Util { "'events' field is deprecated and no longer supported"; public static final String PERMISSION_ID = "Permission_id"; + private static final String INVALID_PERMISSION_ID = + "invalid " + PERMISSION_ID + ": expect a 32-bit integer"; + private static final int MAX_JSON_INTEGER_VALUE_LENGTH = 64; public static final String VISIBLE = "visible"; public static final String INT64_AS_STRING_PARAM = "int64_as_string"; public static final String TRANSACTION = "transaction"; @@ -431,14 +434,48 @@ public static String getHexString(final String string) { return ByteArray.toHexString(ByteString.copyFromUtf8(string).toByteArray()); } + /** + * Rejects a json value that cannot be a bounded-length number, before anything converts it. A + * container is not a number and reaches the conversion as its full serialized form, so it has to + * be turned away by type rather than by length. An absent value is left to the caller, which is + * what distinguishes an optional key from a required one. + * + * @throws InvalidParameterException if the value is not a number, or is longer than the bound + */ + private static void checkJsonNumberValue(Object rawValue, String message) { + if (rawValue == null) { + return; + } + if (!(rawValue instanceof String || rawValue instanceof Number) + || rawValue.toString().length() > MAX_JSON_INTEGER_VALUE_LENGTH) { + throw new InvalidParameterException(message); + } + } + public static Transaction setTransactionPermissionId(JSONObject jsonObject, Transaction transaction) { - if (jsonObject.containsKey(PERMISSION_ID)) { - int permissionId = jsonObject.getInteger(PERMISSION_ID); - return setTransactionPermissionId(permissionId, transaction); + if (!jsonObject.containsKey(PERMISSION_ID)) { + return transaction; } - - return transaction; + int permissionId; + try { + Object rawValue = jsonObject.get(PERMISSION_ID); + checkJsonNumberValue(rawValue, INVALID_PERMISSION_ID); + BigDecimal value = jsonObject.getBigDecimal(PERMISSION_ID); + if (value == null) { + throw new InvalidParameterException(INVALID_PERMISSION_ID); + } + // Preserve getInteger's legacy string syntax, but require it to match the exact conversion. + int exact = value.intValueExact(); + Integer legacy = jsonObject.getInteger(PERMISSION_ID); + if (legacy == null || legacy != exact) { + throw new InvalidParameterException(INVALID_PERMISSION_ID); + } + permissionId = exact; + } catch (NumberFormatException | ArithmeticException | JSONException e) { + throw new InvalidParameterException(INVALID_PERMISSION_ID); + } + return setTransactionPermissionId(permissionId, transaction); } public static Transaction setTransactionPermissionId(int permissionId, Transaction transaction) { @@ -505,6 +542,9 @@ public static long getJsonLongValue(final JSONObject jsonObject, final String ke } public static long getJsonLongValue(JSONObject jsonObject, String key, boolean required) { + Object rawValue = jsonObject.get(key); + checkJsonNumberValue(rawValue, "invalid key [" + key + "]: expect a number of at most " + + MAX_JSON_INTEGER_VALUE_LENGTH + " characters"); BigDecimal bigDecimal = jsonObject.getBigDecimal(key); if (required && bigDecimal == null) { throw new InvalidParameterException("key [" + key + "] does not exist"); diff --git a/framework/src/test/java/org/tron/core/services/http/BaseHttpTest.java b/framework/src/test/java/org/tron/core/services/http/BaseHttpTest.java index 47710a8ca93..f4a4f507f97 100644 --- a/framework/src/test/java/org/tron/core/services/http/BaseHttpTest.java +++ b/framework/src/test/java/org/tron/core/services/http/BaseHttpTest.java @@ -4,6 +4,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import com.google.protobuf.Any; import com.google.protobuf.ByteString; import java.lang.reflect.Field; import javax.servlet.http.HttpServlet; @@ -20,6 +21,8 @@ import org.tron.core.Wallet; import org.tron.core.config.args.Args; import org.tron.protos.Protocol.Transaction; +import org.tron.protos.Protocol.Transaction.Contract.ContractType; +import org.tron.protos.contract.AccountContract.AccountCreateContract; /** * Base class for HTTP servlet unit tests. @@ -32,8 +35,19 @@ */ public abstract class BaseHttpTest { + /** + * A stand-in transaction for the mocked wallet. The type is the one an all-default Contract + * already declared (enum 0); what it did not carry was a matching payload, and + * Util.printTransactionToJSON drops a contract whose Any does not match its declared type -- + * silently, and only once TransactionFactory knows that type, which depends on whether any + * actuator has been constructed elsewhere in the jvm. Packing the payload the way the node's + * own builders do makes the rendering independent of that global state. + */ protected static final Transaction MINIMAL_TX = Transaction.newBuilder() - .setRawData(Transaction.raw.newBuilder().addContract(Transaction.Contract.newBuilder())) + .setRawData(Transaction.raw.newBuilder().addContract( + Transaction.Contract.newBuilder() + .setType(ContractType.AccountCreateContract) + .setParameter(Any.pack(AccountCreateContract.getDefaultInstance())))) .build(); @Mock diff --git a/framework/src/test/java/org/tron/core/services/http/GetExchangeByIdServletTest.java b/framework/src/test/java/org/tron/core/services/http/GetExchangeByIdServletTest.java index f67072e9856..51cfcbda108 100644 --- a/framework/src/test/java/org/tron/core/services/http/GetExchangeByIdServletTest.java +++ b/framework/src/test/java/org/tron/core/services/http/GetExchangeByIdServletTest.java @@ -1,10 +1,12 @@ package org.tron.core.services.http; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import com.google.protobuf.ByteString; @@ -49,4 +51,17 @@ public void testGet() throws Exception { assertEquals(200, response.getStatus()); assertTrue(response.getContentAsString().contains("exchange_id")); } + + @Test + public void oversizedQuotedIdIsRejectedBeforeWalletCall() throws Exception { + String oversized = "99999999999999999999999999999999999999999999999999999999999999999"; + MockHttpServletResponse response = newResponse(); + + servlet.doPost(postRequest("{\"id\":\"" + oversized + "\"}"), response); + + verifyNoInteractions(wallet); + assertTrue(response.getContentAsString().contains("id")); + assertTrue(response.getContentAsString().contains("64 characters")); + assertFalse(response.getContentAsString().contains(oversized)); + } } diff --git a/framework/src/test/java/org/tron/core/services/http/JsonLongValueValidationTest.java b/framework/src/test/java/org/tron/core/services/http/JsonLongValueValidationTest.java new file mode 100644 index 00000000000..09211d99e27 --- /dev/null +++ b/framework/src/test/java/org/tron/core/services/http/JsonLongValueValidationTest.java @@ -0,0 +1,97 @@ +package org.tron.core.services.http; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import java.security.InvalidParameterException; +import org.apache.commons.lang3.StringUtils; +import org.junit.Test; +import org.tron.json.JSONObject; + +public class JsonLongValueValidationTest { + + private static JSONObject value(String value) { + return JSONObject.parseObject("{\"id\":\"" + value + "\"}"); + } + + /** The same value submitted unquoted, which reaches the node as a number rather than a string. */ + private static JSONObject unquoted(String value) { + return JSONObject.parseObject("{\"id\":" + value + "}"); + } + + @Test + public void stringLengthBoundaryIsEnforcedBeforeDecimalConversion() { + assertEquals(0L, Util.getJsonLongValue(value(StringUtils.repeat('0', 64)), "id", true)); + + String oversized = StringUtils.repeat('9', 65); + InvalidParameterException exception = assertThrows(InvalidParameterException.class, + () -> Util.getJsonLongValue(value(oversized), "id", true)); + assertTrue(exception.getMessage().contains("id")); + assertFalse(exception.getMessage().contains(oversized)); + } + + /** An unquoted token reaches this check as an already-normalized Number. */ + @Test + public void unquotedNumberNormalizedTextLengthBoundaryIsEnforced() { + // json forbids leading zeros, so the accepted case is shown by the failure it reaches + // instead: at 64 characters the bound is passed and the exact conversion is what rejects it + assertThrows(ArithmeticException.class, + () -> Util.getJsonLongValue(unquoted(StringUtils.repeat('9', 64)), "id", true)); + + String oversized = StringUtils.repeat('9', 65); + InvalidParameterException exception = assertThrows(InvalidParameterException.class, + () -> Util.getJsonLongValue(unquoted(oversized), "id", true)); + assertTrue(exception.getMessage().contains("id")); + assertFalse(exception.getMessage().contains(oversized)); + } + + @Test + public void exactLongRepresentationsRemainCompatible() { + assertEquals(Long.MIN_VALUE, + Util.getJsonLongValue(value(Long.toString(Long.MIN_VALUE)), "id", true)); + assertEquals(Long.MAX_VALUE, + Util.getJsonLongValue(value(Long.toString(Long.MAX_VALUE)), "id", true)); + assertEquals(0L, Util.getJsonLongValue(value("0"), "id", true)); + assertEquals(1L, Util.getJsonLongValue(value("1.0"), "id", true)); + assertEquals(100L, Util.getJsonLongValue(value("1e2"), "id", true)); + } + + @Test + public void existingExactConversionFailuresRemainInEffect() { + assertThrows(ArithmeticException.class, + () -> Util.getJsonLongValue(value("1.5"), "id", true)); + assertThrows(ArithmeticException.class, + () -> Util.getJsonLongValue(value("9223372036854775808"), "id", true)); + } + + /** + * A container is not a number. It used to skip the length bound altogether and reach the + * conversion as its full serialized form, bounded only by the shim's 65535-character fallback. + */ + @Test + public void nonNumericTypesAreRejectedBeforeConversion() { + String digits = StringUtils.repeat('9', 128); + String[] rawJsonValues = {"[" + digits + "]", "{\"a\":" + digits + "}", "true"}; + for (String rawJsonValue : rawJsonValues) { + JSONObject jsonObject = JSONObject.parseObject("{\"id\":" + rawJsonValue + "}"); + InvalidParameterException exception = assertThrows(InvalidParameterException.class, + () -> Util.getJsonLongValue(jsonObject, "id", true)); + assertTrue(exception.getMessage().contains("id")); + assertFalse(exception.getMessage().contains(digits)); + } + } + + /** An absent optional key must stay distinguishable from one the node refuses. */ + @Test + public void optionalMissingValueStillDefaultsToZero() { + assertEquals(0L, Util.getJsonLongValue(new JSONObject(), "id", false)); + } + + @Test + public void requiredMissingValueStillFails() { + assertThrows(InvalidParameterException.class, + () -> Util.getJsonLongValue(new JSONObject(), "id", true)); + } +} diff --git a/framework/src/test/java/org/tron/core/services/http/TransferServletTest.java b/framework/src/test/java/org/tron/core/services/http/TransferServletTest.java index b04c6255dac..67863a53f63 100644 --- a/framework/src/test/java/org/tron/core/services/http/TransferServletTest.java +++ b/framework/src/test/java/org/tron/core/services/http/TransferServletTest.java @@ -1,5 +1,8 @@ package org.tron.core.services.http; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; @@ -12,6 +15,7 @@ import org.tron.common.crypto.ECKey; import org.tron.common.utils.ByteArray; import org.tron.core.capsule.TransactionCapsule; +import org.tron.json.JSONObject; import org.tron.protos.Protocol; import org.tron.protos.contract.BalanceContract; @@ -52,4 +56,99 @@ && addressEquals(((BalanceContract.TransferContract) c) eq(Protocol.Transaction.Contract.ContractType.TransferContract)); assertTransactionResponse(response); } + + private String transferJson(String permissionIdJson) { + return "{" + + "\"owner_address\": \"" + ownerAddr + "\"," + + "\"to_address\": \"" + toAddr + "\"," + + "\"amount\": 100," + + "\"Permission_id\": " + permissionIdJson + + "}"; + } + + private MockHttpServletResponse post(String permissionIdJson) throws Exception { + MockHttpServletResponse response = newResponse(); + servlet.doPost(postRequest(transferJson(permissionIdJson)), response); + return response; + } + + @Test + public void testPermissionIdReachesTheBuiltTransaction() throws Exception { + MockHttpServletResponse response = post("2"); + + assertTransactionResponse(response); + JSONObject contract = JSONObject.parseObject(response.getContentAsString()) + .getJSONObject("raw_data").getJSONArray("contract").getJSONObject(0); + assertEquals(2, contract.getIntValue("Permission_id")); + } + + /** + * A fraction never reaches Util.setTransactionPermissionId. Permission_id is not a + * TransferContract field, so JsonFormat.merge skips it through handleMissingField, which + * accepts only integers, booleans, strings and null -- lookingAtInteger() sees the leading + * digit, hands "1.9" to consumeInt64() and that fails first. Kept as a guard that the + * endpoint rejects it whichever layer does the rejecting. + */ + @Test + public void testFractionalPermissionIdIsRejected() throws Exception { + String content = post("1.9").getContentAsString(); + + assertTrue("must report an error", content.contains("\"Error\"")); + assertFalse("must not hand back a transaction", content.contains("txID")); + assertFalse(content.contains("\"raw_data\"")); + } + + /** + * The reachable case: 2^32+1 is a valid int64, so it clears JsonFormat.merge and lands in + * Util.setTransactionPermissionId, where BigDecimal.intValue() used to wrap it to 1 -- + * handing back a transaction built against permission 1, which the caller never asked for. + */ + @Test + public void testOutOfRangePermissionIdIsRejected() throws Exception { + String content = post("4294967297").getContentAsString(); + + assertTrue("must report an error", content.contains("\"Error\"")); + assertFalse("must not hand back a transaction built against permission 1", + content.contains("txID")); + assertFalse("must not echo the submitted value", content.contains("4294967297")); + } + + @Test + public void testQuotedScientificPermissionIdIsRejected() throws Exception { + String content = post("\"1e2\"").getContentAsString(); + + assertTrue("must report an error", content.contains("\"Error\"")); + assertFalse("must not hand back a transaction", content.contains("txID")); + assertFalse(content.contains("\"raw_data\"")); + } + + /** + * An explicit null used to reach getInteger(), whose null return blew up on unboxing with a + * bare NullPointerException. It stays an error -- writing the key states intent, so a null + * there is a caller-side defect. Omitting the key is how a caller asks for the default. + */ + @Test + public void testExplicitNullPermissionIdIsRejected() throws Exception { + String content = post("null").getContentAsString(); + + assertTrue("must report an error", content.contains("\"Error\"")); + assertFalse("must not hand back a transaction", content.contains("txID")); + assertFalse("must not surface a NullPointerException", + content.contains("NullPointerException")); + } + + @Test + public void testOmittedPermissionIdBuildsTransactionWithoutPermission() throws Exception { + MockHttpServletResponse response = newResponse(); + servlet.doPost(postRequest("{" + + "\"owner_address\": \"" + ownerAddr + "\"," + + "\"to_address\": \"" + toAddr + "\"," + + "\"amount\": 100" + + "}"), response); + + assertTransactionResponse(response); + JSONObject contract = JSONObject.parseObject(response.getContentAsString()) + .getJSONObject("raw_data").getJSONArray("contract").getJSONObject(0); + assertFalse(contract.containsKey("Permission_id")); + } } diff --git a/framework/src/test/java/org/tron/core/services/http/UtilTest.java b/framework/src/test/java/org/tron/core/services/http/UtilTest.java index c619fd0de54..d31a43afe37 100644 --- a/framework/src/test/java/org/tron/core/services/http/UtilTest.java +++ b/framework/src/test/java/org/tron/core/services/http/UtilTest.java @@ -1,7 +1,10 @@ package org.tron.core.services.http; import com.google.protobuf.ByteString; +import java.security.InvalidParameterException; +import java.util.Arrays; import javax.annotation.Resource; +import org.apache.commons.lang3.StringUtils; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -236,7 +239,7 @@ public void testPackCreateSmartContractOmitsNullAbiOutputs() throws Exception { Assert.assertEquals(0, contract.getNewContract().getAbi().getEntrys(0).getOutputsCount()); } - private Transaction buildTooManySigsTransaction() { + private Transaction buildTransferTransaction() { String strTransaction = "{\n" + " \"visible\": false,\n" + " \"txID\": \"fc33817936b06e50d4b6f1797e62f52d69af6c0da580a607241a9c03a48e390e\",\n" @@ -264,7 +267,11 @@ private Transaction buildTooManySigsTransaction() { + "0a1541c076305e35aea1fe45a772fcaaab8a36e87bdb551215415624c12e308b03a1a6b21d9b86e3942fac1a" + "b92b180a70b2ccb8ea8930\"\n" + "}"; - Transaction transaction = Util.packTransaction(strTransaction, false); + return Util.packTransaction(strTransaction, false); + } + + private Transaction buildTooManySigsTransaction() { + Transaction transaction = buildTransferTransaction(); int totalSignNum = dbManager.getDynamicPropertiesStore().getTotalSignNum(); ByteString dummySig = ByteString.copyFrom(new byte[65]); Transaction.Builder builder = transaction.toBuilder(); @@ -301,4 +308,116 @@ public void testPrintSignWeightTooManySigsHttpPath() { Assert.assertTrue(jsonObject.getJSONObject("result").getString("message") .contains("too many signatures")); } + + /** + * A container is not a number. It used to skip the length bound altogether and reach the + * conversion as its full serialized form, bounded only by the shim's 65535-character fallback. + */ + @Test + public void testPermissionIdRejectsNonNumericTypes() { + assertRejected("[" + StringUtils.repeat('9', 128) + "]"); + assertRejected("{\"a\":" + StringUtils.repeat('9', 128) + "}"); + assertRejected("true"); + } + + private Transaction applyPermissionId(String rawJsonValue) { + JSONObject jsonObject = + JSONObject.parseObject("{\"" + Util.PERMISSION_ID + "\":" + rawJsonValue + "}"); + return Util.setTransactionPermissionId(jsonObject, buildTransferTransaction()); + } + + private int permissionIdOf(Transaction transaction) { + return transaction.getRawData().getContract(0).getPermissionId(); + } + + @Test + public void testPermissionIdAcceptsLegacyExactIntegerRepresentations() { + Assert.assertEquals(2, permissionIdOf(applyPermissionId("2"))); + Assert.assertEquals(2, permissionIdOf(applyPermissionId("\"2\""))); + Assert.assertEquals(1, permissionIdOf(applyPermissionId("1.0"))); + Assert.assertEquals(100, permissionIdOf(applyPermissionId("1e2"))); + Assert.assertEquals(1, permissionIdOf(applyPermissionId("\"1.0\""))); + Assert.assertEquals(1, permissionIdOf(applyPermissionId("\"1.\""))); + Assert.assertEquals(1000, permissionIdOf(applyPermissionId("\"1,000\""))); + } + + @Test + public void testPermissionIdAcceptsMaximumLengthNumericString() { + char[] digits = new char[64]; + Arrays.fill(digits, '0'); + digits[digits.length - 1] = '2'; + + Assert.assertEquals(2, permissionIdOf(applyPermissionId("\"" + new String(digits) + "\""))); + } + + /** + * One character past the bound is refused, and so is a value far beyond it: the field is + * bounded by its own length rather than by whatever the numeric conversion happens to survive. + */ + @Test + public void testPermissionIdRejectsStringsPastTheLengthBoundary() { + assertRejected("\"" + StringUtils.repeat('0', 64) + "2\""); + assertRejected("\"" + StringUtils.repeat('9', 10_000) + "\""); + } + + /** Quoting must not decide which regime applies: the same digits unquoted are refused too. */ + @Test + public void testPermissionIdRejectsUnquotedNumbersPastTheLengthBoundary() { + assertRejected("9" + StringUtils.repeat('9', 64)); + } + + private void assertRejected(String rawJsonValue) { + InvalidParameterException e = Assert.assertThrows(InvalidParameterException.class, + () -> applyPermissionId(rawJsonValue)); + Assert.assertTrue(e.getMessage().contains(Util.PERMISSION_ID)); + Assert.assertFalse("the message must not echo the submitted value", + e.getMessage().contains(rawJsonValue)); + } + + @Test + public void testPermissionIdRejectsFraction() { + assertRejected("1.9"); + assertRejected("2.999"); + } + + @Test + public void testPermissionIdRejectsNonLegacyNumericStrings() { + assertRejected("\"1e2\""); + assertRejected("\"1E2\""); + assertRejected("\".0\""); + } + + @Test + public void testPermissionIdRejectsIntOverflow() { + assertRejected("4294967297"); + assertRejected("99999999999"); + } + + @Test + public void testPermissionIdRejectsNonNumber() { + assertRejected("\"abc\""); + assertRejected("true"); + assertRejected("[1]"); + } + + @Test + public void testPermissionIdRejectsExplicitNull() { + InvalidParameterException e = Assert.assertThrows(InvalidParameterException.class, + () -> applyPermissionId("null")); + Assert.assertTrue(e.getMessage().contains(Util.PERMISSION_ID)); + } + + @Test + public void testAbsentPermissionIdLeavesTransactionUnchanged() { + JSONObject jsonObject = JSONObject.parseObject("{\"amount\":1}"); + Transaction transaction = buildTransferTransaction(); + Assert.assertEquals(0, + permissionIdOf(Util.setTransactionPermissionId(jsonObject, transaction))); + } + + @Test + public void testPermissionIdNotPositiveLeavesTransactionUnchanged() { + Assert.assertEquals(0, permissionIdOf(applyPermissionId("0"))); + Assert.assertEquals(0, permissionIdOf(applyPermissionId("-1"))); + } } From ac3860f9b00308ac67f7cc7a328562d1987aaa28 Mon Sep 17 00:00:00 2001 From: 0xbigapple Date: Tue, 25 Aug 2026 19:07:39 +0800 Subject: [PATCH 3/5] fix(api): answer an unusable address identically on every endpoint getAddress reports every way a request can fail to name an address as one fixed message, and the reward and brokerage servlets write it, so the fullnode, solidity and PBFT endpoints no longer differ on a given malformed request. An absent address is one of those ways: it used to be answered with the service default. The body is bounded while it is read, since this is the only body-reading path that does not go through PostParams. --- .../services/http/GetBrokerageServlet.java | 25 +-- .../core/services/http/GetRewardServlet.java | 31 ++- .../org/tron/core/services/http/Util.java | 80 +++++-- .../http/AddressQueryServletTestBase.java | 196 ++++++++++++++++++ .../http/GetBrokerageServletTest.java | 96 +++------ .../services/http/GetRewardServletTest.java | 140 +++---------- 6 files changed, 339 insertions(+), 229 deletions(-) create mode 100644 framework/src/test/java/org/tron/core/services/http/AddressQueryServletTestBase.java diff --git a/framework/src/main/java/org/tron/core/services/http/GetBrokerageServlet.java b/framework/src/main/java/org/tron/core/services/http/GetBrokerageServlet.java index 1fbd94fe690..cbe8fe05e78 100644 --- a/framework/src/main/java/org/tron/core/services/http/GetBrokerageServlet.java +++ b/framework/src/main/java/org/tron/core/services/http/GetBrokerageServlet.java @@ -1,10 +1,8 @@ package org.tron.core.services.http; -import java.io.IOException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import lombok.extern.slf4j.Slf4j; -import org.bouncycastle.util.encoders.DecoderException; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; import org.tron.core.db.Manager; @@ -18,21 +16,20 @@ public class GetBrokerageServlet extends RateLimiterServlet { private Manager manager; protected void doGet(HttpServletRequest request, HttpServletResponse response) { + byte[] address; + try { + address = Util.getAddress(request); + } catch (IllegalArgumentException e) { + Util.writeError(response, e.getMessage()); + return; + } catch (Exception e) { + Util.processError(e, response); + return; + } try { - int value = 0; - byte[] address = Util.getAddress(request); long cycle = manager.getDynamicPropertiesStore().getCurrentCycleNumber(); - if (address != null) { - value = manager.getDelegationStore().getBrokerage(cycle, address); - } + int value = manager.getDelegationStore().getBrokerage(cycle, address); response.getWriter().println("{\"brokerage\": " + value + "}"); - } catch (DecoderException | IllegalArgumentException e) { - try { - response.getWriter() - .println("{\"Error\": " + "\"INVALID address, " + e.getMessage() + "\"}"); - } catch (IOException ioe) { - logger.debug("IOException: {}", ioe.getMessage()); - } } catch (Exception e) { Util.processError(e, response); } diff --git a/framework/src/main/java/org/tron/core/services/http/GetRewardServlet.java b/framework/src/main/java/org/tron/core/services/http/GetRewardServlet.java index 61b88d1160f..09d3d67d850 100644 --- a/framework/src/main/java/org/tron/core/services/http/GetRewardServlet.java +++ b/framework/src/main/java/org/tron/core/services/http/GetRewardServlet.java @@ -1,10 +1,8 @@ package org.tron.core.services.http; -import java.io.IOException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import lombok.extern.slf4j.Slf4j; -import org.bouncycastle.util.encoders.DecoderException; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; import org.tron.core.db.Manager; @@ -18,30 +16,25 @@ public class GetRewardServlet extends RateLimiterServlet { private Manager manager; protected void doGet(HttpServletRequest request, HttpServletResponse response) { + byte[] address; try { - long value = 0; - byte[] address = Util.getAddress(request); - if (address != null) { - value = manager.getMortgageService().queryReward(address); - } + address = Util.getAddress(request); + } catch (IllegalArgumentException e) { + Util.writeError(response, e.getMessage()); + return; + } catch (Exception e) { + Util.processError(e, response); + return; + } + try { + long value = manager.getMortgageService().queryReward(address); String out = JsonFormat.isInt64AsString() ? "{\"reward\": \"" + value + "\"}" : "{\"reward\": " + value + "}"; response.getWriter().println(out); - } catch (DecoderException | IllegalArgumentException e) { - try { - response.getWriter() - .println("{\"Error\": " + "\"INVALID address, " + e.getMessage() + "\"}"); - } catch (IOException ioe) { - logger.debug("IOException: {}", ioe.getMessage()); - } } catch (Exception e) { logger.error("", e); - try { - response.getWriter().println(Util.printErrorMsg(e)); - } catch (IOException ioe) { - logger.debug("IOException: {}", ioe.getMessage()); - } + Util.processError(e, response); } } diff --git a/framework/src/main/java/org/tron/core/services/http/Util.java b/framework/src/main/java/org/tron/core/services/http/Util.java index 85057421670..26af2382dbe 100644 --- a/framework/src/main/java/org/tron/core/services/http/Util.java +++ b/framework/src/main/java/org/tron/core/services/http/Util.java @@ -25,6 +25,7 @@ import javax.servlet.http.HttpServletResponse; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; +import org.bouncycastle.util.encoders.DecoderException; import org.bouncycastle.util.encoders.Hex; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.MimeTypes; @@ -41,6 +42,7 @@ import org.tron.common.crypto.Hash; import org.tron.common.parameter.CommonParameter; import org.tron.common.utils.ByteArray; +import org.tron.common.utils.DecodeUtil; import org.tron.common.utils.Sha256Hash; import org.tron.core.Constant; import org.tron.core.actuator.TransactionFactory; @@ -72,6 +74,9 @@ public class Util { private static final String INVALID_PERMISSION_ID = "invalid " + PERMISSION_ID + ": expect a 32-bit integer"; private static final int MAX_JSON_INTEGER_VALUE_LENGTH = 64; + private static final int BODY_READ_BUFFER_SIZE = 4096; + private static final String INVALID_ADDRESS = "INVALID address"; + private static final String INVALID_JSON_BODY = "INVALID JSON body"; public static final String VISIBLE = "visible"; public static final String INT64_AS_STRING_PARAM = "int64_as_string"; public static final String TRANSACTION = "transaction"; @@ -574,6 +579,16 @@ public static void processError(Exception e, HttpServletResponse response) { } } + static void writeError(HttpServletResponse response, String message) { + JSONObject error = new JSONObject(); + error.put("Error", message); + try { + response.getWriter().println(error.toJSONString()); + } catch (IOException ioe) { + logger.debug("IOException: {}", ioe.getMessage()); + } + } + public static String convertOutput(Account account) { if (account.getAssetIssuedID().isEmpty()) { return JsonFormat.printToString(account, false); @@ -599,16 +614,43 @@ public static void printAccount(Account reply, HttpServletResponse response, Boo } } + /** + * Returns the address the request carries. Every way the request can fail to name one is + * reported as an IllegalArgumentException whose message is the fixed text the caller is + * answered with, so that the address-keyed endpoints and their solidity/PBFT mirrors answer a + * given malformed request identically without each having to classify the failure. Nothing + * derived from the request may go into that message: it is written straight to the response. + */ public static byte[] getAddress(HttpServletRequest request) throws Exception { - byte[] address = null; String addressParam = "address"; - String addressStr = checkGetParam(request, addressParam); - if (StringUtils.isNotBlank(addressStr)) { - if (StringUtils.startsWith(addressStr, Constant.ADD_PRE_FIX_STRING_MAINNET)) { - address = Hex.decode(addressStr); - } else { - address = decodeFromBase58Check(addressStr); - } + String addressStr; + try { + addressStr = checkGetParam(request, addressParam); + } catch (JSONException e) { + throw new IllegalArgumentException(INVALID_JSON_BODY); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException(INVALID_ADDRESS); + } + if (StringUtils.isBlank(addressStr)) { + throw new IllegalArgumentException(INVALID_ADDRESS); + } + + boolean hex = StringUtils.startsWith(addressStr, Constant.ADD_PRE_FIX_STRING_MAINNET); + // bound the hex input before decoding, mirroring the base58 length short-circuit + if (hex && addressStr.length() != DecodeUtil.ADDRESS_SIZE) { + throw new IllegalArgumentException(INVALID_ADDRESS); + } + + byte[] address; + try { + address = hex ? Hex.decode(addressStr) : decodeFromBase58Check(addressStr); + } catch (DecoderException | IllegalArgumentException exception) { + // both decoders name the offending character and its offset, which is caller input + throw new IllegalArgumentException(INVALID_ADDRESS); + } + // base58 is validated inside the decoder; hex used to be returned unchecked + if (address == null || (hex && !DecodeUtil.addressValid(address))) { + throw new IllegalArgumentException(INVALID_ADDRESS); } return address; } @@ -642,14 +684,24 @@ private static String checkGetParam(HttpServletRequest request, String key) thro return null; } - public static String getRequestValue(HttpServletRequest request) throws IOException { - BufferedReader reader = new BufferedReader(new InputStreamReader(request.getInputStream())); - String line; + /** + * Reads the request body. This is the only body-reading path in the http layer that does not go + * through {@link PostParams}, so the {@link #checkBodySize} every other path performs is applied + * here as well. + */ + public static String getRequestValue(HttpServletRequest request) throws Exception { StringBuilder sb = new StringBuilder(); - while ((line = reader.readLine()) != null) { - sb.append(line); + char[] buffer = new char[BODY_READ_BUFFER_SIZE]; + try (BufferedReader reader = new BufferedReader( + new InputStreamReader(request.getInputStream()))) { + int read; + while ((read = reader.read(buffer)) != -1) { + sb.append(buffer, 0, read); + } } - return sb.toString(); + String value = sb.toString(); + checkBodySize(value); + return value; } public static List convertLogAddressToTronAddress(TransactionInfo transactionInfo) { diff --git a/framework/src/test/java/org/tron/core/services/http/AddressQueryServletTestBase.java b/framework/src/test/java/org/tron/core/services/http/AddressQueryServletTestBase.java new file mode 100644 index 00000000000..697a8816745 --- /dev/null +++ b/framework/src/test/java/org/tron/core/services/http/AddressQueryServletTestBase.java @@ -0,0 +1,196 @@ +package org.tron.core.services.http; + +import static java.nio.charset.StandardCharsets.UTF_8; + +import java.util.Arrays; +import org.apache.commons.lang3.StringUtils; +import org.junit.Assert; +import org.junit.Test; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.tron.common.BaseTest; +import org.tron.common.utils.StringUtil; +import org.tron.core.Wallet; +import org.tron.core.config.args.Args; +import org.tron.json.JSONObject; + +/** + * The shared address-parameter contract for the reward and brokerage endpoints. The cases live + * here rather than in each servlet's test so that the endpoints cannot drift apart in how they + * answer a given malformed request. + */ +public abstract class AddressQueryServletTestBase extends BaseTest { + + static final String INVALID_ADDRESS_ERROR = "INVALID address"; + + /** Invokes the concrete servlet's GET handler. */ + protected abstract void invokeGet(MockHttpServletRequest request, + MockHttpServletResponse response); + + /** Invokes the concrete servlet's POST handler. */ + protected abstract void invokePost(MockHttpServletRequest request, + MockHttpServletResponse response); + + /** The key this endpoint answers with, for instance {@code reward}. */ + protected abstract String valueKey(); + + /** What this endpoint answers for a well-formed address it holds no state for. */ + protected abstract int valueWithoutState(); + + protected static MockHttpServletRequest postRequest(String contentType) { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setMethod("POST"); + request.setContentType(contentType); + request.setCharacterEncoding(UTF_8.name()); + return request; + } + + protected static MockHttpServletRequest getRequest(String address) { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setMethod("GET"); + if (address != null) { + request.addParameter("address", address); + } + return request; + } + + protected static MockHttpServletRequest formRequest(String address) { + MockHttpServletRequest request = postRequest("application/x-www-form-urlencoded"); + if (address != null) { + request.addParameter("address", address); + } + return request; + } + + protected static MockHttpServletRequest jsonRequest(String address) { + return jsonRequest(address, "application/json"); + } + + protected static MockHttpServletRequest jsonRequest(String address, String contentType) { + MockHttpServletRequest request = postRequest(contentType); + String json = address == null ? "{}" : "{\"address\":\"" + address + "\"}"; + request.setContent(json.getBytes(UTF_8)); + return request; + } + + /** A base58check payload whose first byte is not the address prefix. */ + protected static String wrongPrefixAddress() { + byte[] address = new byte[21]; + address[0] = (byte) (Wallet.getAddressPreFixByte() + 1); + return StringUtil.encode58Check(address); + } + + /** A well-formed address that no store holds anything for. */ + protected static String canonicalNoStateAddress() { + byte[] address = new byte[21]; + Arrays.fill(address, (byte) 7); + address[0] = Wallet.getAddressPreFixByte(); + return StringUtil.encode58Check(address); + } + + /** Runs the request through the servlet with the verb it carries, and returns the raw body. */ + protected String invoke(MockHttpServletRequest request) throws Exception { + MockHttpServletResponse response = new MockHttpServletResponse(); + if ("GET".equals(request.getMethod())) { + invokeGet(request, response); + } else { + invokePost(request, response); + } + return response.getContentAsString(); + } + + private void assertInvalid(MockHttpServletRequest request, String submitted) throws Exception { + String body = invoke(request); + JSONObject result = JSONObject.parseObject(body); + Assert.assertEquals(INVALID_ADDRESS_ERROR, result.get("Error")); + Assert.assertNull(result.get(valueKey())); + if (submitted != null && !submitted.isEmpty()) { + Assert.assertFalse("the error must not echo the submitted address", body.contains(submitted)); + } + } + + private void assertValue(MockHttpServletRequest request, int expected) throws Exception { + JSONObject result = JSONObject.parseObject(invoke(request)); + Assert.assertEquals(expected, (int) result.get(valueKey())); + Assert.assertNull(result.get("Error")); + } + + @Test + public void getAndPostRejectMalformedAddresses() throws Exception { + String invalidBase58 = "Taaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + // '0' is outside the base58 alphabet, so the decoder itself raises + String illegalBase58Char = "Taaaaaaaaaaaaaaaaa0aaaaaaaaaaaaaaa"; + String invalidHex = "41zz00000000000000000000000000000000000000"; + // unlike a raw "42..." string this reaches the prefix check instead of failing the alphabet + String wrongPrefix = wrongPrefixAddress(); + String oversizedHex = "41" + StringUtils.repeat('0', 8192); + + assertInvalid(getRequest(invalidBase58), invalidBase58); + assertInvalid(getRequest(illegalBase58Char), illegalBase58Char); + assertInvalid(getRequest(wrongPrefix), wrongPrefix); + assertInvalid(getRequest(oversizedHex), oversizedHex); + assertInvalid(jsonRequest(invalidBase58), invalidBase58); + assertInvalid(jsonRequest(invalidHex), invalidHex); + } + + /** + * An address that is absent or blank is rejected like any other address the endpoint cannot + * use, on every verb and body encoding. It used to answer with the service default instead. + */ + @Test + public void missingOrBlankAddressIsRejected() throws Exception { + assertInvalid(getRequest(null), null); + assertInvalid(getRequest(""), null); + assertInvalid(formRequest(null), null); + assertInvalid(formRequest(""), null); + assertInvalid(jsonRequest(null), null); + } + + @Test + public void validAddressWithoutStateRetainsServiceDefault() throws Exception { + assertValue(getRequest(canonicalNoStateAddress()), valueWithoutState()); + } + + /** + * The charset-suffixed content type is what browsers actually send, and it must take the + * json-body branch of Util.checkGetParam rather than the form-parameter branch. The body + * carries a well-formed address: on the form branch no address would be found at all and + * the answer would be a rejection instead of the value. + */ + @Test + public void jsonBodyWithCharsetSuffixIsParsed() throws Exception { + assertValue(jsonRequest(canonicalNoStateAddress(), "application/json; charset=utf-8"), + valueWithoutState()); + } + + @Test + public void malformedJsonBodyIsRejectedWithoutEchoingIt() throws Exception { + String marker = "UniqueMarkerValue"; + MockHttpServletRequest request = postRequest("application/json"); + request.setContent(("{\"address\":" + marker + "}").getBytes(UTF_8)); + + String body = invoke(request); + Assert.assertFalse(body.contains(marker)); + JSONObject result = JSONObject.parseObject(body); + Assert.assertEquals("INVALID JSON body", result.get("Error")); + Assert.assertNull(result.get(valueKey())); + } + + /** + * This is the only body-reading path in the http layer that does not go through PostParams, so + * it has to enforce httpMaxMessageSize itself. The configured bound is read rather than + * lowered: these servlets share one Args with every other test in the jvm, and a test that + * moves a global limit while other classes are using it is how the suite gets flaky. + */ + @Test + public void oversizedBodyIsRejected() throws Exception { + int limit = (int) Args.getInstance().getHttpMaxMessageSize(); + MockHttpServletRequest request = postRequest("application/json"); + request.setContent(("{\"address\":\"" + StringUtils.repeat('a', limit) + "\"}") + .getBytes(UTF_8)); + + JSONObject result = JSONObject.parseObject(invoke(request)); + Assert.assertTrue(String.valueOf(result.get("Error")).contains("body size is too big")); + Assert.assertNull(result.get(valueKey())); + } +} diff --git a/framework/src/test/java/org/tron/core/services/http/GetBrokerageServletTest.java b/framework/src/test/java/org/tron/core/services/http/GetBrokerageServletTest.java index 9b37c2e4205..29a67d387b0 100644 --- a/framework/src/test/java/org/tron/core/services/http/GetBrokerageServletTest.java +++ b/framework/src/test/java/org/tron/core/services/http/GetBrokerageServletTest.java @@ -1,20 +1,18 @@ package org.tron.core.services.http; -import java.io.UnsupportedEncodingException; import javax.annotation.Resource; import org.junit.Assert; import org.junit.Test; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; -import org.tron.common.BaseTest; import org.tron.common.TestConstants; import org.tron.core.config.args.Args; import org.tron.json.JSONObject; -public class GetBrokerageServletTest extends BaseTest { +public class GetBrokerageServletTest extends AddressQueryServletTestBase { @Resource - private GetBrokerageServlet getBrokerageServlet; + private GetBrokerageServlet getBrokerageServlet; static { Args.setParam( @@ -24,84 +22,40 @@ public class GetBrokerageServletTest extends BaseTest { ); } - public MockHttpServletRequest createRequest(String contentType) { - MockHttpServletRequest request = new MockHttpServletRequest(); - request.setMethod("POST"); - request.setContentType(contentType); - request.setCharacterEncoding("UTF-8"); - return request; + @Override + protected void invokeGet(MockHttpServletRequest request, MockHttpServletResponse response) { + getBrokerageServlet.doGet(request, response); } - @Test - public void getBrokerageValueByJsonTest() { - int expect = 20; - String jsonParam = "{\"address\": \"TGSzEq4t7oMTRcn1VxDghRu5r5bWAE5D1W\"}"; - MockHttpServletRequest request = createRequest("application/json"); - request.setContent(jsonParam.getBytes()); - MockHttpServletResponse response = new MockHttpServletResponse(); + @Override + protected void invokePost(MockHttpServletRequest request, MockHttpServletResponse response) { getBrokerageServlet.doPost(request, response); - try { - String contentAsString = response.getContentAsString(); - JSONObject result = JSONObject.parseObject(contentAsString); - int brokerage = (int)result.get("brokerage"); - Assert.assertEquals(expect, brokerage); - } catch (UnsupportedEncodingException e) { - Assert.fail(e.getMessage()); - } } + @Override + protected String valueKey() { + return "brokerage"; + } - @Test - public void getBrokerageByJsonUTF8Test() { - int expect = 20; - String jsonParam = "{\"address\": \"TGSzEq4t7oMTRcn1VxDghRu5r5bWAE5D1W\"}"; - MockHttpServletRequest request = createRequest("application/json; charset=utf-8"); - request.setContent(jsonParam.getBytes()); - MockHttpServletResponse response = new MockHttpServletResponse(); - getBrokerageServlet.doPost(request, response); - try { - String contentAsString = response.getContentAsString(); - JSONObject result = JSONObject.parseObject(contentAsString); - int brokerage = (int)result.get("brokerage"); - Assert.assertEquals(expect, brokerage); - } catch (UnsupportedEncodingException e) { - Assert.fail(e.getMessage()); - } + /** An address the delegation store holds nothing for still answers the default brokerage. */ + @Override + protected int valueWithoutState() { + return 20; } @Test - public void getBrokerageValueTest() { - int expect = 20; - MockHttpServletRequest request = createRequest("application/x-www-form-urlencoded"); - request.addParameter("address", "TGSzEq4t7oMTRcn1VxDghRu5r5bWAE5D1W"); - MockHttpServletResponse response = new MockHttpServletResponse(); - getBrokerageServlet.doPost(request, response); - try { - String contentAsString = response.getContentAsString(); - JSONObject result = JSONObject.parseObject(contentAsString); - int brokerage = (int)result.get("brokerage"); - Assert.assertEquals(expect, brokerage); - } catch (UnsupportedEncodingException e) { - Assert.fail(e.getMessage()); - } + public void getBrokerageValueByJsonTest() throws Exception { + MockHttpServletRequest request = jsonRequest("TGSzEq4t7oMTRcn1VxDghRu5r5bWAE5D1W"); + + JSONObject result = JSONObject.parseObject(invoke(request)); + Assert.assertEquals(20, (int) result.get("brokerage")); } @Test - public void getByBlankParamTest() { - int expect = 0; - MockHttpServletRequest request = createRequest("application/x-www-form-urlencoded"); - request.addParameter("address", ""); - MockHttpServletResponse response = new MockHttpServletResponse(); - getBrokerageServlet.doPost(request, response); - try { - String contentAsString = response.getContentAsString(); - JSONObject result = JSONObject.parseObject(contentAsString); - int brokerage = (int)result.get("brokerage"); - Assert.assertEquals(expect, brokerage); - String content = (String) result.get("Error"); - Assert.assertNull(content); - } catch (UnsupportedEncodingException e) { - Assert.fail(e.getMessage()); - } + public void getBrokerageValueTest() throws Exception { + MockHttpServletRequest request = formRequest("TGSzEq4t7oMTRcn1VxDghRu5r5bWAE5D1W"); + + JSONObject result = JSONObject.parseObject(invoke(request)); + Assert.assertEquals(20, (int) result.get("brokerage")); } } diff --git a/framework/src/test/java/org/tron/core/services/http/GetRewardServletTest.java b/framework/src/test/java/org/tron/core/services/http/GetRewardServletTest.java index 9afa5607a66..438e4545706 100644 --- a/framework/src/test/java/org/tron/core/services/http/GetRewardServletTest.java +++ b/framework/src/test/java/org/tron/core/services/http/GetRewardServletTest.java @@ -2,7 +2,6 @@ import static org.tron.common.utils.Commons.decodeFromBase58Check; -import java.io.UnsupportedEncodingException; import javax.annotation.Resource; import lombok.extern.slf4j.Slf4j; import org.junit.Assert; @@ -10,7 +9,6 @@ import org.junit.Test; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; -import org.tron.common.BaseTest; import org.tron.common.TestConstants; import org.tron.core.config.args.Args; import org.tron.core.db.Manager; @@ -19,7 +17,7 @@ import org.tron.json.JSONObject; @Slf4j -public class GetRewardServletTest extends BaseTest { +public class GetRewardServletTest extends AddressQueryServletTestBase { @Resource private Manager manager; @@ -31,22 +29,30 @@ public class GetRewardServletTest extends BaseTest { private DelegationStore delegationStore; @Resource - GetRewardServlet getRewardServlet; + private GetRewardServlet getRewardServlet; static { - Args.setParam( - new String[]{ - "--output-directory", dbPath(), - }, TestConstants.TEST_CONF - ); + Args.setParam(new String[]{"--output-directory", dbPath()}, TestConstants.TEST_CONF); } - public MockHttpServletRequest createRequest(String contentType) { - MockHttpServletRequest request = new MockHttpServletRequest(); - request.setMethod("POST"); - request.setContentType(contentType); - request.setCharacterEncoding("UTF-8"); - return request; + @Override + protected void invokeGet(MockHttpServletRequest request, MockHttpServletResponse response) { + getRewardServlet.doGet(request, response); + } + + @Override + protected void invokePost(MockHttpServletRequest request, MockHttpServletResponse response) { + getRewardServlet.doPost(request, response); + } + + @Override + protected String valueKey() { + return "reward"; + } + + @Override + protected int valueWithoutState() { + return 0; } @Before @@ -58,107 +64,19 @@ public void init() { } @Test - public void getRewardValueByJsonTest() { - int expect = 138181; - String jsonParam = "{\"address\": \"TNboetpFgv9SqMoHvaVt626NLXETnbdW1K\"}"; - MockHttpServletRequest request = createRequest("application/json"); - MockHttpServletResponse response = new MockHttpServletResponse(); - request.setContent(jsonParam.getBytes()); - try { - getRewardServlet.doPost(request, response); - String contentAsString = response.getContentAsString(); - JSONObject result = JSONObject.parseObject(contentAsString); - int reward = (int)result.get("reward"); - Assert.assertEquals(expect, reward); - } catch (UnsupportedEncodingException e) { - Assert.fail(e.getMessage()); - } - } + public void getRewardValueByJsonTest() throws Exception { + MockHttpServletRequest request = jsonRequest("TNboetpFgv9SqMoHvaVt626NLXETnbdW1K"); - @Test - public void getRewardByJsonUTF8Test() { - int expect = 138181; - String jsonParam = "{\"address\": \"TNboetpFgv9SqMoHvaVt626NLXETnbdW1K\"}"; - MockHttpServletRequest request = createRequest("application/json; charset=utf-8"); - MockHttpServletResponse response = new MockHttpServletResponse(); - request.setContent(jsonParam.getBytes()); - try { - getRewardServlet.doPost(request, response); - String contentAsString = response.getContentAsString(); - JSONObject result = JSONObject.parseObject(contentAsString); - int reward = (int)result.get("reward"); - Assert.assertEquals(expect, reward); - } catch (UnsupportedEncodingException e) { - Assert.fail(e.getMessage()); - } + JSONObject result = JSONObject.parseObject(invoke(request)); + Assert.assertEquals(138181, (int) result.get("reward")); } @Test - public void getRewardValueTest() { - int expect = 138181; - MockHttpServletRequest request = createRequest("application/x-www-form-urlencoded"); - MockHttpServletResponse response = new MockHttpServletResponse(); + public void getRewardValueTest() throws Exception { mortgageService.payStandbyWitness(); - request.addParameter("address", "TNboetpFgv9SqMoHvaVt626NLXETnbdW1K"); - getRewardServlet.doPost(request, response); - try { - String contentAsString = response.getContentAsString(); - JSONObject result = JSONObject.parseObject(contentAsString); - int reward = (int)result.get("reward"); - Assert.assertEquals(expect, reward); - } catch (UnsupportedEncodingException e) { - Assert.fail(e.getMessage()); - } - } - - @Test - public void getByBlankParamTest() { - MockHttpServletRequest request = createRequest("application/x-www-form-urlencoded"); - MockHttpServletResponse response = new MockHttpServletResponse(); - request.addParameter("address", ""); - GetRewardServlet getRewardServlet = new GetRewardServlet(); - getRewardServlet.doPost(request, response); - try { - String contentAsString = response.getContentAsString(); - JSONObject result = JSONObject.parseObject(contentAsString); - int reward = (int)result.get("reward"); - Assert.assertEquals(0, reward); - String content = (String) result.get("Error"); - Assert.assertNull(content); - } catch (UnsupportedEncodingException e) { - Assert.fail(e.getMessage()); - } - } + MockHttpServletRequest request = formRequest("TNboetpFgv9SqMoHvaVt626NLXETnbdW1K"); - @Test - public void getRewardByOversizedValidCharAddressTest() { - // 41-char, all-valid-Base58 address: the length guard returns null -> reward 0. - MockHttpServletRequest request = createRequest("application/x-www-form-urlencoded"); - MockHttpServletResponse response = new MockHttpServletResponse(); - request.addParameter("address", "T" + new String(new char[40]).replace('\0', 'a')); - new GetRewardServlet().doPost(request, response); - try { - JSONObject result = JSONObject.parseObject(response.getContentAsString()); - Assert.assertEquals(0, (int) result.get("reward")); - Assert.assertNull(result.get("Error")); - } catch (UnsupportedEncodingException e) { - Assert.fail(e.getMessage()); - } + JSONObject result = JSONObject.parseObject(invoke(request)); + Assert.assertEquals(138181, (int) result.get("reward")); } - - @Test - public void getRewardByOversizedIllegalCharAddressTest() { - MockHttpServletRequest request = createRequest("application/x-www-form-urlencoded"); - MockHttpServletResponse response = new MockHttpServletResponse(); - request.addParameter("address", "T" + new String(new char[40]).replace('\0', '0')); - new GetRewardServlet().doPost(request, response); - try { - JSONObject result = JSONObject.parseObject(response.getContentAsString()); - Assert.assertEquals(0, (int) result.get("reward")); - Assert.assertNull(result.get("Error")); - } catch (UnsupportedEncodingException e) { - Assert.fail(e.getMessage()); - } - } - } From f52d635a290357136cb9d63ccc74a28237b80081 Mon Sep 17 00:00:00 2001 From: 0xbigapple Date: Tue, 25 Aug 2026 19:07:39 +0800 Subject: [PATCH 4/5] fix(shielded): bound the shielded TRC-20 description counts Mint takes exactly one receive description; transfer takes one or two of each and, without an ask, a matching spend authority signature count. The counts are checked before the merge loops, because ByteUtil.merge copies the accumulated buffer on every iteration and checking afterwards would make an unbounded list quadratic. --- .../zen/ShieldedTRC20ParametersBuilder.java | 27 +++- .../java/org/tron/core/WalletMockTest.java | 1 + .../ShieldedTRC20ParametersBuilderTest.java | 139 ++++++++++++++++++ 3 files changed, 160 insertions(+), 7 deletions(-) create mode 100644 framework/src/test/java/org/tron/core/zen/ShieldedTRC20ParametersBuilderTest.java diff --git a/framework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.java b/framework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.java index 4ee4f75a171..4600ebdcd5d 100644 --- a/framework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.java +++ b/framework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.java @@ -402,6 +402,9 @@ private String mintParamsToHexString(GrpcAPI.ShieldedTRC20Parameters mintParams, if (value.compareTo(BigInteger.ZERO) <= 0) { throw new IllegalArgumentException("require the value be positive"); } + if (mintParams.getReceiveDescriptionCount() != 1) { + throw new IllegalArgumentException("invalid mint description number"); + } ShieldContract.ReceiveDescription revDesc = mintParams.getReceiveDescription(0); byte[] zeros = new byte[12]; @@ -422,12 +425,28 @@ private String mintParamsToHexString(GrpcAPI.ShieldedTRC20Parameters mintParams, private String transferParamsToHexString(GrpcAPI.ShieldedTRC20Parameters transferParams, List spendAuthoritySignature, boolean withAsk) { + List spendDescs = transferParams.getSpendDescriptionList(); + List recvDescs = transferParams.getReceiveDescriptionList(); + long spendCount = spendDescs.size(); + long recvCount = recvDescs.size(); + if (spendCount < 1 || spendCount > 2) { + throw new IllegalArgumentException("invalid transfer input number"); + } + if (recvCount < 1 || recvCount > 2) { + throw new IllegalArgumentException("invalid transfer output number"); + } + // the !withAsk branch below indexes the signatures positionally, so bound them here too: + // this block is the argument contract for a method reachable from getTriggerContractInput + if (!withAsk && (spendAuthoritySignature == null + || spendAuthoritySignature.size() != spendCount)) { + throw new IllegalArgumentException("invalid spend authority signature number"); + } + byte[] input = new byte[0]; byte[] spendAuthSig = new byte[0]; byte[] output = new byte[0]; byte[] c = new byte[0]; byte[] bindingSig; - List spendDescs = transferParams.getSpendDescriptionList(); for (ShieldContract.SpendDescription spendDesc : spendDescs) { input = ByteUtil.merge(input, spendDesc.getNullifier().toByteArray(), @@ -441,10 +460,6 @@ private String transferParamsToHexString(GrpcAPI.ShieldedTRC20Parameters transfe spendAuthSig, spendDesc.getSpendAuthoritySignature().toByteArray()); } } - long spendCount = spendDescs.size(); - if (spendCount < 1 || spendCount > 2) { - throw new IllegalArgumentException("invalid transfer input number"); - } if (!withAsk) { if (spendCount == 1) { spendAuthSig = spendAuthoritySignature.get(0).getValue().toByteArray(); @@ -458,7 +473,6 @@ private String transferParamsToHexString(GrpcAPI.ShieldedTRC20Parameters transfe byte[] spendCountBytes = ByteUtil.longTo32Bytes(spendCount); byte[] authOffsetBytes = ByteUtil.longTo32Bytes(192 + 32 + 320 * spendCount); - List recvDescs = transferParams.getReceiveDescriptionList(); for (ShieldContract.ReceiveDescription recvDesc : recvDescs) { output = ByteUtil.merge(output, recvDesc.getNoteCommitment().toByteArray(), @@ -474,7 +488,6 @@ private String transferParamsToHexString(GrpcAPI.ShieldedTRC20Parameters transfe ); } - long recvCount = recvDescs.size(); byte[] recvCountBytes = ByteUtil.longTo32Bytes(recvCount); byte[] outputOffsetbytes = ByteUtil .longTo32Bytes(192 + 32 + 320 * spendCount + 32 + 64 * spendCount); diff --git a/framework/src/test/java/org/tron/core/WalletMockTest.java b/framework/src/test/java/org/tron/core/WalletMockTest.java index 7874af854a6..2e1e81cd3ab 100644 --- a/framework/src/test/java/org/tron/core/WalletMockTest.java +++ b/framework/src/test/java/org/tron/core/WalletMockTest.java @@ -907,6 +907,7 @@ public void testGetTriggerInputForShieldedTRC20Contract1() GrpcAPI.ShieldedTRC20Parameters shieldedTRC20Parameters = GrpcAPI.ShieldedTRC20Parameters.newBuilder() .addSpendDescription(spendDescription) + .addReceiveDescription(ShieldContract.ReceiveDescription.getDefaultInstance()) .setParameterType("transfer") .build(); GrpcAPI.BytesMessage bytesMessage = diff --git a/framework/src/test/java/org/tron/core/zen/ShieldedTRC20ParametersBuilderTest.java b/framework/src/test/java/org/tron/core/zen/ShieldedTRC20ParametersBuilderTest.java new file mode 100644 index 00000000000..9d8dff536ba --- /dev/null +++ b/framework/src/test/java/org/tron/core/zen/ShieldedTRC20ParametersBuilderTest.java @@ -0,0 +1,139 @@ +package org.tron.core.zen; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mockStatic; + +import java.math.BigInteger; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import org.junit.Test; +import org.mockito.MockedStatic; +import org.tron.api.GrpcAPI.BytesMessage; +import org.tron.api.GrpcAPI.ShieldedTRC20Parameters; +import org.tron.common.utils.ByteUtil; +import org.tron.protos.contract.ShieldContract; + +public class ShieldedTRC20ParametersBuilderTest { + + private static final String GOLDEN_ONE_TO_ONE = + "00000000000000000000000000000000000000000000000000000000000000c0" + + "0000000000000000000000000000000000000000000000000000000000000220" + + "0000000000000000000000000000000000000000000000000000000000000280" + + "00000000000000000000000000000000000000000000000000000000000003c0" + + "0000000000000000000000000000000000000000000000000000000000000001" + + "0000000000000000000000000000000000000000000000000000000000000001" + + "0000000000000000000000000000000000000000000000000000000000000001" + + "0000000000000000000000000000000000000000000000000000000000000001" + + "000000000000000000000000"; + + private static final String GOLDEN_TWO_TO_TWO = + "00000000000000000000000000000000000000000000000000000000000000c0" + + "0000000000000000000000000000000000000000000000000000000000000360" + + "0000000000000000000000000000000000000000000000000000000000000400" + + "0000000000000000000000000000000000000000000000000000000000000660" + + "0000000000000000000000000000000000000000000000000000000000000002" + + "0000000000000000000000000000000000000000000000000000000000000002" + + "0000000000000000000000000000000000000000000000000000000000000002" + + "0000000000000000000000000000000000000000000000000000000000000002" + + "000000000000000000000000000000000000000000000000"; + + private static final String GOLDEN_ONE_TO_TWO = + "00000000000000000000000000000000000000000000000000000000000000c0" + + "0000000000000000000000000000000000000000000000000000000000000220" + + "0000000000000000000000000000000000000000000000000000000000000280" + + "00000000000000000000000000000000000000000000000000000000000004e0" + + "0000000000000000000000000000000000000000000000000000000000000001" + + "0000000000000000000000000000000000000000000000000000000000000001" + + "0000000000000000000000000000000000000000000000000000000000000002" + + "0000000000000000000000000000000000000000000000000000000000000002" + + "000000000000000000000000000000000000000000000000"; + + private static final String GOLDEN_TWO_TO_ONE = + "00000000000000000000000000000000000000000000000000000000000000c0" + + "0000000000000000000000000000000000000000000000000000000000000360" + + "0000000000000000000000000000000000000000000000000000000000000400" + + "0000000000000000000000000000000000000000000000000000000000000540" + + "0000000000000000000000000000000000000000000000000000000000000002" + + "0000000000000000000000000000000000000000000000000000000000000002" + + "0000000000000000000000000000000000000000000000000000000000000001" + + "0000000000000000000000000000000000000000000000000000000000000001" + + "000000000000000000000000"; + + private static ShieldedTRC20ParametersBuilder transferBuilder() throws Exception { + return new ShieldedTRC20ParametersBuilder("transfer"); + } + + private static ShieldedTRC20Parameters params(int spends, int receives) { + ShieldedTRC20Parameters.Builder parameters = ShieldedTRC20Parameters.newBuilder(); + for (int i = 0; i < spends; i++) { + parameters.addSpendDescription(ShieldContract.SpendDescription.getDefaultInstance()); + } + for (int i = 0; i < receives; i++) { + parameters.addReceiveDescription(ShieldContract.ReceiveDescription.getDefaultInstance()); + } + return parameters.build(); + } + + private static String run(int spends, int receives) throws Exception { + return run(spends, receives, Collections.emptyList(), true); + } + + private static String run(int spends, int receives, List signatures, + boolean withAsk) throws Exception { + return transferBuilder().getTriggerContractInput( + params(spends, receives), signatures, BigInteger.ZERO, withAsk, new byte[21]); + } + + @Test + public void invalidCountsAreRejectedBeforeAnyMerge() throws Exception { + Object[][] invalidCounts = { + {0, 1, "invalid transfer input number"}, + {3, 1, "invalid transfer input number"}, + {1, 0, "invalid transfer output number"}, + {1, 3, "invalid transfer output number"}, + {1, 10_000, "invalid transfer output number"}, + }; + try (MockedStatic byteUtil = mockStatic(ByteUtil.class)) { + for (Object[] counts : invalidCounts) { + IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, + () -> run((int) counts[0], (int) counts[1])); + assertEquals(counts[2], exception.getMessage()); + } + byteUtil.verifyNoInteractions(); + } + } + + /** + * The asymmetric shapes are the ones that pin the arithmetic: both offsets mix the two counts, + * so with spendCount == recvCount a transposition of the two produces identical bytes and the + * symmetric vectors alone would pass under it. + */ + @Test + public void validOneAndTwoEntryOutputsRemainByteForByteCompatible() throws Exception { + assertEquals(GOLDEN_ONE_TO_ONE, run(1, 1)); + assertEquals(GOLDEN_TWO_TO_TWO, run(2, 2)); + assertEquals(GOLDEN_ONE_TO_TWO, run(1, 2)); + assertEquals(GOLDEN_TWO_TO_ONE, run(2, 1)); + } + + /** + * Without an ask the signatures are supplied by the caller and indexed positionally, so their + * count is part of the same argument contract as the description counts. + */ + @Test + public void spendAuthoritySignatureCountIsBoundedForCallerSuppliedSignatures() { + int[][] mismatched = {{1, 0}, {2, 0}, {2, 1}, {1, 2}}; + for (int[] counts : mismatched) { + List signatures = new ArrayList<>(); + for (int i = 0; i < counts[1]; i++) { + signatures.add(BytesMessage.getDefaultInstance()); + } + IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, + () -> run(counts[0], 1, signatures, false)); + assertTrue(exception.getMessage().contains("invalid spend authority signature number")); + } + } +} From 248cbe03ec696c2fe678e57242a759124257c341 Mon Sep 17 00:00:00 2001 From: 0xbigapple Date: Tue, 25 Aug 2026 19:07:39 +0800 Subject: [PATCH 5/5] fix(shielded): report unusable shielded trigger input as a validation failure An amount string past 80 characters and a trigger input the builder rejects are answered as a ContractValidateException naming the argument, instead of escaping the wallet as an IllegalArgumentException. --- .../src/main/java/org/tron/core/Wallet.java | 26 ++- .../WalletShieldedAmountValidationTest.java | 149 ++++++++++++++++++ ...lletShieldedDescriptionValidationTest.java | 76 +++++++++ 3 files changed, 247 insertions(+), 4 deletions(-) create mode 100644 framework/src/test/java/org/tron/core/WalletShieldedAmountValidationTest.java create mode 100644 framework/src/test/java/org/tron/core/WalletShieldedDescriptionValidationTest.java diff --git a/framework/src/main/java/org/tron/core/Wallet.java b/framework/src/main/java/org/tron/core/Wallet.java index fbf1370edff..ae8415ee2cf 100755 --- a/framework/src/main/java/org/tron/core/Wallet.java +++ b/framework/src/main/java/org/tron/core/Wallet.java @@ -261,6 +261,9 @@ public class Wallet { "Shielded transaction API is disabled; " + "set node.allowShieldedTransactionApi=true to enable."; private static final String PAYMENT_ADDRESS_FORMAT_WRONG = "paymentAddress format is wrong"; + // the authoritative bound is checkBigIntegerRange: uint256 max is 78 decimal digits, + // this only keeps the string short enough to convert cheaply + private static final int MAX_SHIELDED_AMOUNT_LENGTH = 80; private static final String SHIELDED_TRANSACTION_SCAN_RANGE = "request requires start_block_index >= 0 && end_block_index > " + "start_block_index && end_block_index - start_block_index <= 1000"; @@ -4220,6 +4223,9 @@ private BigInteger getBigIntegerFromString(String in) { if (trimmedIn.length() == 0) { return BigInteger.ZERO; } + if (trimmedIn.length() > MAX_SHIELDED_AMOUNT_LENGTH) { + throw new IllegalArgumentException("invalid shielded amount"); + } return new BigInteger(trimmedIn, 10); } @@ -4331,7 +4337,12 @@ public BytesMessage getTriggerInputForShieldedTRC20Contract( ShieldedTRC20Parameters shieldedTRC20Parameters = request.getShieldedTRC20Parameters(); List spendAuthoritySignature = request.getSpendAuthoritySignatureList(); - BigInteger value = getBigIntegerFromString(request.getAmount()); + BigInteger value; + try { + value = getBigIntegerFromString(request.getAmount()); + } catch (IllegalArgumentException e) { + throw new ContractValidateException("invalid amount"); + } checkBigIntegerRange(value); byte[] transparentToAddress = request.getTransparentToAddress().toByteArray(); byte[] transparentToAddressTvm = new byte[20]; @@ -4396,9 +4407,16 @@ public BytesMessage getTriggerInputForShieldedTRC20Contract( } parametersBuilder.setBurnCiphertext(burnCiper); } - String input = parametersBuilder - .getTriggerContractInput(shieldedTRC20Parameters, spendAuthoritySignature, value, false, - transparentToAddressTvm); + String input; + try { + input = parametersBuilder + .getTriggerContractInput(shieldedTRC20Parameters, spendAuthoritySignature, value, false, + transparentToAddressTvm); + } catch (IllegalArgumentException e) { + // the builder names the offending argument in fixed strings; none of them echoes an input + throw new ContractValidateException( + "invalid shielded TRC-20 trigger input: " + e.getMessage(), e); + } if (Objects.isNull(input)) { throw new ZksnarkException("generate the trigger contract parameters error"); } diff --git a/framework/src/test/java/org/tron/core/WalletShieldedAmountValidationTest.java b/framework/src/test/java/org/tron/core/WalletShieldedAmountValidationTest.java new file mode 100644 index 00000000000..f050795e749 --- /dev/null +++ b/framework/src/test/java/org/tron/core/WalletShieldedAmountValidationTest.java @@ -0,0 +1,149 @@ +package org.tron.core; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; + +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.math.BigInteger; +import org.apache.commons.lang3.StringUtils; +import org.junit.BeforeClass; +import org.junit.Test; +import org.mockito.MockedStatic; +import org.tron.api.GrpcAPI; +import org.tron.common.parameter.CommonParameter; +import org.tron.core.config.args.Args; +import org.tron.core.exception.ContractValidateException; +import org.tron.core.exception.ZksnarkException; + +public class WalletShieldedAmountValidationTest { + + // both methods under test are pure and never touch instance state, so one wallet is enough; + // constructing one generates a keypair, which is not worth paying for on every invocation + private static Wallet wallet; + private static Method bigIntegerFromString; + private static Method checkBigIntegerRange; + + @BeforeClass + public static void setUpClass() throws Exception { + wallet = new Wallet(); + bigIntegerFromString = + Wallet.class.getDeclaredMethod("getBigIntegerFromString", String.class); + bigIntegerFromString.setAccessible(true); + checkBigIntegerRange = + Wallet.class.getDeclaredMethod("checkBigIntegerRange", BigInteger.class); + checkBigIntegerRange.setAccessible(true); + } + + private static BigInteger parse(String value) throws Exception { + try { + return (BigInteger) bigIntegerFromString.invoke(wallet, value); + } catch (InvocationTargetException exception) { + throw (Exception) exception.getCause(); + } + } + + private static void checkRange(BigInteger value) throws Exception { + try { + checkBigIntegerRange.invoke(wallet, value); + } catch (InvocationTargetException exception) { + throw (Exception) exception.getCause(); + } + } + + @Test + public void amountLengthBoundaryIsAppliedAfterTrimAndBeforeParsing() throws Exception { + assertEquals(BigInteger.ZERO, parse(" ")); + assertEquals(BigInteger.ZERO, parse(" " + StringUtils.repeat('0', 80) + " ")); + + String oversized = StringUtils.repeat('9', 81); + IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, + () -> parse(oversized)); + assertEquals("invalid shielded amount", exception.getMessage()); + assertFalse(exception.getMessage().contains(oversized)); + assertThrows(IllegalArgumentException.class, () -> parse(StringUtils.repeat('0', 81))); + } + + @Test + public void uint256AndSignRangeChecksRemainAuthoritative() throws Exception { + BigInteger maximum = BigInteger.ONE.shiftLeft(256).subtract(BigInteger.ONE); + assertEquals(maximum, parse(maximum.toString())); + checkRange(maximum); + + ContractValidateException tooLarge = assertThrows(ContractValidateException.class, + () -> checkRange(parse(BigInteger.ONE.shiftLeft(256).toString()))); + assertTrue(tooLarge.getMessage().contains("256 bits")); + + ContractValidateException negative = assertThrows(ContractValidateException.class, + () -> checkRange(parse("-1"))); + assertTrue(negative.getMessage().contains("non-negative")); + assertEquals(BigInteger.valueOf(123), parse(" 123 ")); + } + + /** Runs the public entrypoint with the shielded api enabled, so the amount is reached. */ + private static void triggerInputWithAmount(String amount) throws Exception { + GrpcAPI.ShieldedTRC20TriggerContractParameters request = + GrpcAPI.ShieldedTRC20TriggerContractParameters.newBuilder() + .setAmount(amount) + .setShieldedTRC20Parameters(GrpcAPI.ShieldedTRC20Parameters.newBuilder() + .setParameterType("transfer") + .build()) + .build(); + CommonParameter commonParameter = mock(Args.class); + try (MockedStatic mocked = mockStatic(CommonParameter.class)) { + when(CommonParameter.getInstance()).thenReturn(commonParameter); + when(commonParameter.isAllowShieldedTransactionApi()).thenReturn(true); + wallet.getTriggerInputForShieldedTRC20Contract(request); + } + } + + /** + * getBigIntegerFromString reports an overlong amount, and a malformed one, with unchecked + * exceptions. The entrypoint declares only ZksnarkException and ContractValidateException, and + * printErrorMsg names the class, so an untranslated failure reaches http clients as + * "class java.lang.IllegalArgumentException" instead of the validation error every other + * amount entrypoint answers with. + */ + @Test + public void unusableAmountsAreReportedAsValidationFailures() { + String[] unusable = {StringUtils.repeat('9', 81), "not-a-number", "1.5"}; + for (String amount : unusable) { + ContractValidateException exception = assertThrows(ContractValidateException.class, + () -> triggerInputWithAmount(amount)); + assertEquals("invalid amount", exception.getMessage()); + } + } + + /** A well-formed amount past the uint256 range still reports the range failure, not "invalid". */ + @Test + public void outOfRangeAmountRetainsRangeValidationMessage() { + ContractValidateException exception = assertThrows(ContractValidateException.class, + () -> triggerInputWithAmount(BigInteger.ONE.shiftLeft(256).toString())); + assertTrue(exception.getMessage().contains("256 bits")); + } + + @Test + public void disabledFeatureGateRunsBeforeAmountOrDescriptionValidation() { + GrpcAPI.ShieldedTRC20TriggerContractParameters request = + GrpcAPI.ShieldedTRC20TriggerContractParameters.newBuilder() + .setAmount(StringUtils.repeat('9', 81)) + .setShieldedTRC20Parameters(GrpcAPI.ShieldedTRC20Parameters.newBuilder() + .setParameterType("transfer") + .build()) + .build(); + CommonParameter commonParameter = mock(Args.class); + try (MockedStatic mocked = mockStatic(CommonParameter.class)) { + when(CommonParameter.getInstance()).thenReturn(commonParameter); + when(commonParameter.isAllowShieldedTransactionApi()).thenReturn(false); + + ZksnarkException exception = assertThrows(ZksnarkException.class, + () -> wallet.getTriggerInputForShieldedTRC20Contract(request)); + assertTrue(exception.getMessage().contains("Shielded transaction API is disabled")); + } + } +} diff --git a/framework/src/test/java/org/tron/core/WalletShieldedDescriptionValidationTest.java b/framework/src/test/java/org/tron/core/WalletShieldedDescriptionValidationTest.java new file mode 100644 index 00000000000..2339459c315 --- /dev/null +++ b/framework/src/test/java/org/tron/core/WalletShieldedDescriptionValidationTest.java @@ -0,0 +1,76 @@ +package org.tron.core; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.mockito.MockedStatic; +import org.tron.api.GrpcAPI; +import org.tron.api.GrpcAPI.BytesMessage; +import org.tron.common.parameter.CommonParameter; +import org.tron.core.config.args.Args; +import org.tron.core.exception.ContractValidateException; +import org.tron.protos.contract.ShieldContract; + +public class WalletShieldedDescriptionValidationTest { + + private static Wallet wallet; + + @BeforeClass + public static void setUpClass() { + wallet = new Wallet(); + } + + private static void triggerInput(GrpcAPI.ShieldedTRC20Parameters parameters, + String amount, int signatureCount) throws Exception { + GrpcAPI.ShieldedTRC20TriggerContractParameters.Builder request = + GrpcAPI.ShieldedTRC20TriggerContractParameters.newBuilder() + .setAmount(amount) + .setShieldedTRC20Parameters(parameters); + for (int i = 0; i < signatureCount; i++) { + request.addSpendAuthoritySignature(BytesMessage.getDefaultInstance()); + } + + CommonParameter commonParameter = mock(Args.class); + try (MockedStatic mocked = mockStatic(CommonParameter.class)) { + when(CommonParameter.getInstance()).thenReturn(commonParameter); + when(commonParameter.isAllowShieldedTransactionApi()).thenReturn(true); + wallet.getTriggerInputForShieldedTRC20Contract(request.build()); + } + } + + @Test + public void transferDescriptionCardinalityIsReportedAsValidationFailure() { + GrpcAPI.ShieldedTRC20Parameters.Builder parameters = + GrpcAPI.ShieldedTRC20Parameters.newBuilder() + .setParameterType("transfer") + .addSpendDescription(ShieldContract.SpendDescription.getDefaultInstance()); + for (int i = 0; i < 3; i++) { + parameters.addReceiveDescription(ShieldContract.ReceiveDescription.getDefaultInstance()); + } + + ContractValidateException exception = assertThrows(ContractValidateException.class, + () -> triggerInput(parameters.build(), "0", 1)); + assertEquals("invalid shielded TRC-20 trigger input: invalid transfer output number", + exception.getMessage()); + } + + @Test + public void mintDescriptionCardinalityIsReportedAsValidationFailure() { + GrpcAPI.ShieldedTRC20Parameters parameters = + GrpcAPI.ShieldedTRC20Parameters.newBuilder() + .setParameterType("mint") + .addReceiveDescription(ShieldContract.ReceiveDescription.getDefaultInstance()) + .addReceiveDescription(ShieldContract.ReceiveDescription.getDefaultInstance()) + .build(); + + ContractValidateException exception = assertThrows(ContractValidateException.class, + () -> triggerInput(parameters, "1", 0)); + assertEquals("invalid shielded TRC-20 trigger input: invalid mint description number", + exception.getMessage()); + } +}