Price Oracle: validate input parameters and extend test coverage: (XRPLF#5013)

* Price Oracle: validate input parameters and extend test coverage:

Validate trim, time_threshold, document_id are valid
Int, UInt, or string convertible to UInt. Validate base_asset
and quote_asset are valid currency. Update error codes.
Extend Oracle and GetAggregatePrice unit-tests.
Denote unreachable coverage code.

* Set one-line LCOV_EXCL_LINE

* Move ledger_entry tests to LedgerRPC_test.cpp

* Add constants for "None"

* Fix LedgerRPC test

---------

Co-authored-by: Scott Determan <[email protected]>

Author: gregtatcam

src/ripple/app/tx/impl/DeleteOracle.cpp

@@ -48,7 +48,7 @@ TER
 DeleteOracle::preclaim(PreclaimContext const& ctx)
 {
     if (!ctx.view.exists(keylet::account(ctx.tx.getAccountID(sfAccount))))
-        return terNO_ACCOUNT;
+        return terNO_ACCOUNT;  // LCOV_EXCL_LINE
 
     if (auto const sle = ctx.view.read(keylet::oracle(
             ctx.tx.getAccountID(sfAccount), ctx.tx[sfOracleDocumentID]));
@@ -60,8 +60,10 @@ DeleteOracle::preclaim(PreclaimContext const& ctx)
     else if (ctx.tx.getAccountID(sfAccount) != sle->getAccountID(sfOwner))
     {
         // this can't happen because of the above check
+        // LCOV_EXCL_START
         JLOG(ctx.j.debug()) << "Oracle Delete: invalid account.";
         return tecINTERNAL;
+        // LCOV_EXCL_STOP
     }
     return tesSUCCESS;
 }
@@ -74,18 +76,20 @@ DeleteOracle::deleteOracle(
     beast::Journal j)
 {
     if (!sle)
-        return tesSUCCESS;
+        return tecINTERNAL;  // LCOV_EXCL_LINE
 
     if (!view.dirRemove(
             keylet::ownerDir(account), (*sle)[sfOwnerNode], sle->key(), true))
     {
+        // LCOV_EXCL_START
         JLOG(j.fatal()) << "Unable to delete Oracle from owner.";
         return tefBAD_LEDGER;
+        // LCOV_EXCL_STOP
     }
 
     auto const sleOwner = view.peek(keylet::account(account));
     if (!sleOwner)
-        return tecINTERNAL;
+        return tecINTERNAL;  // LCOV_EXCL_LINE
 
     auto const count =
         sle->getFieldArray(sfPriceDataSeries).size() > 5 ? -2 : -1;
@@ -104,7 +108,7 @@ DeleteOracle::doApply()
             keylet::oracle(account_, ctx_.tx[sfOracleDocumentID])))
         return deleteOracle(ctx_.view(), sle, account_, j_);
 
-    return tecINTERNAL;
+    return tecINTERNAL;  // LCOV_EXCL_LINE
 }
 
 }  // namespace ripple

src/ripple/app/tx/impl/SetOracle.cpp

@@ -74,7 +74,7 @@ SetOracle::preclaim(PreclaimContext const& ctx)
     auto const sleSetter =
         ctx.view.read(keylet::account(ctx.tx.getAccountID(sfAccount)));
     if (!sleSetter)
-        return terNO_ACCOUNT;
+        return terNO_ACCOUNT;  // LCOV_EXCL_LINE
 
     // lastUpdateTime must be within maxLastUpdateTimeDelta seconds
     // of the last closed ledger
@@ -88,8 +88,7 @@ SetOracle::preclaim(PreclaimContext const& ctx)
     std::size_t const lastUpdateTimeEpoch =
         lastUpdateTime - epoch_offset.count();
     if (closeTime < maxLastUpdateTimeDelta)
-        Throw<std::runtime_error>(
-            "Oracle: close time is less than maxLastUpdateTimeDelta");
+        return tecINTERNAL;  // LCOV_EXCL_LINE
     if (lastUpdateTimeEpoch < (closeTime - maxLastUpdateTimeDelta) ||
         lastUpdateTimeEpoch > (closeTime + maxLastUpdateTimeDelta))
         return tecINVALID_UPDATE_TIME;
@@ -194,7 +193,7 @@ adjustOwnerCount(ApplyContext& ctx, int count)
         return true;
     }
 
-    return false;
+    return false;  // LCOV_EXCL_LINE
 }
 
 static void
@@ -274,7 +273,7 @@ SetOracle::doApply()
         auto const newCount = pairs.size() > 5 ? 2 : 1;
         auto const adjust = newCount - oldCount;
         if (adjust != 0 && !adjustOwnerCount(ctx_, adjust))
-            return tefINTERNAL;
+            return tefINTERNAL;  // LCOV_EXCL_LINE
 
         ctx_.view().update(sle);
     }
@@ -295,13 +294,13 @@ SetOracle::doApply()
         auto page = ctx_.view().dirInsert(
             keylet::ownerDir(account_), sle->key(), describeOwnerDir(account_));
         if (!page)
-            return tecDIR_FULL;
+            return tecDIR_FULL;  // LCOV_EXCL_LINE
 
         (*sle)[sfOwnerNode] = *page;
 
         auto const count = series.size() > 5 ? 2 : 1;
         if (!adjustOwnerCount(ctx_, count))
-            return tefINTERNAL;
+            return tefINTERNAL;  // LCOV_EXCL_LINE
 
         ctx_.view().insert(sle);
     }

src/ripple/rpc/handlers/GetAggregatePrice.cpp

@@ -85,18 +85,18 @@ iteratePriceData(
 
         auto const ledger = context.ledgerMaster.getLedgerBySeq(prevSeq);
         if (!ledger)
-            return;
+            return;  // LCOV_EXCL_LINE
 
         meta = ledger->txRead(prevTx).second;
 
+        prevChain = chain;
         for (STObject const& node : meta->getFieldArray(sfAffectedNodes))
         {
             if (node.getFieldU16(sfLedgerEntryType) != ltORACLE)
             {
                 continue;
             }
 
-            prevChain = chain;
             chain = &node;
             isNew = node.isFieldPresent(sfNewFields);
             // if a meta is for the new and this is the first
@@ -170,21 +170,49 @@ doGetAggregatePrice(RPC::JsonContext& context)
     if (!params.isMember(jss::quote_asset))
         return RPC::missing_field_error(jss::quote_asset);
 
+    // Lambda to validate uint type
+    // support positive int, uint, and a number represented as a string
+    auto validUInt = [](Json::Value const& params,
+                        Json::StaticString const& field) {
+        auto const& jv = params[field];
+        std::uint32_t v;
+        return jv.isUInt() || (jv.isInt() && jv.asInt() >= 0) ||
+            (jv.isString() && beast::lexicalCastChecked(v, jv.asString()));
+    };
+
     // Lambda to get `trim` and `time_threshold` fields. If the field
     // is not included in the input then a default value is returned.
-    auto getField = [&params](
+    auto getField = [&params, &validUInt](
                         Json::StaticString const& field,
                         unsigned int def =
                             0) -> std::variant<std::uint32_t, error_code_i> {
         if (params.isMember(field))
         {
-            if (!params[field].isConvertibleTo(Json::ValueType::uintValue))
-                return rpcORACLE_MALFORMED;
+            if (!validUInt(params, field))
+                return rpcINVALID_PARAMS;
             return params[field].asUInt();
         }
         return def;
     };
 
+    // Lambda to get `base_asset` and `quote_asset`. The values have
+    // to conform to the Currency type.
+    auto getCurrency =
+        [&params](SField const& sField, Json::StaticString const& field)
+        -> std::variant<Json::Value, error_code_i> {
+        try
+        {
+            if (params[field].asString().empty())
+                return rpcINVALID_PARAMS;
+            currencyFromJson(sField, params[field]);
+            return params[field];
+        }
+        catch (...)
+        {
+            return rpcINVALID_PARAMS;
+        }
+    };
+
     auto const trim = getField(jss::trim);
     if (std::holds_alternative<error_code_i>(trim))
     {
@@ -206,8 +234,18 @@ doGetAggregatePrice(RPC::JsonContext& context)
         return result;
     }
 
-    auto const& baseAsset = params[jss::base_asset];
-    auto const& quoteAsset = params[jss::quote_asset];
+    auto const baseAsset = getCurrency(sfBaseAsset, jss::base_asset);
+    if (std::holds_alternative<error_code_i>(baseAsset))
+    {
+        RPC::inject_error(std::get<error_code_i>(baseAsset), result);
+        return result;
+    }
+    auto const quoteAsset = getCurrency(sfQuoteAsset, jss::quote_asset);
+    if (std::holds_alternative<error_code_i>(quoteAsset))
+    {
+        RPC::inject_error(std::get<error_code_i>(quoteAsset), result);
+        return result;
+    }
 
     // Collect the dataset into bimap keyed by lastUpdateTime and
     // STAmount (Number is int64 and price is uint64)
@@ -220,8 +258,7 @@ doGetAggregatePrice(RPC::JsonContext& context)
             RPC::inject_error(rpcORACLE_MALFORMED, result);
             return result;
         }
-        auto const documentID = oracle[jss::oracle_document_id].isConvertibleTo(
-                                    Json::ValueType::uintValue)
+        auto const documentID = validUInt(oracle, jss::oracle_document_id)
             ? std::make_optional(oracle[jss::oracle_document_id].asUInt())
             : std::nullopt;
         auto const account =
@@ -235,7 +272,7 @@ doGetAggregatePrice(RPC::JsonContext& context)
         std::shared_ptr<ReadView const> ledger;
         result = RPC::lookupLedger(ledger, context);
         if (!ledger)
-            return result;
+            return result;  // LCOV_EXCL_LINE
 
         auto const sle = ledger->read(keylet::oracle(*account, *documentID));
         iteratePriceData(context, sle, [&](STObject const& node) {
@@ -246,9 +283,9 @@ doGetAggregatePrice(RPC::JsonContext& context)
                     series.end(),
                     [&](STObject const& o) -> bool {
                         return o.getFieldCurrency(sfBaseAsset).getText() ==
-                            baseAsset &&
+                            std::get<Json::Value>(baseAsset) &&
                             o.getFieldCurrency(sfQuoteAsset).getText() ==
-                            quoteAsset &&
+                            std::get<Json::Value>(quoteAsset) &&
                             o.isFieldPresent(sfAssetPrice);
                     });
                 iter != series.end())
@@ -287,10 +324,15 @@ doGetAggregatePrice(RPC::JsonContext& context)
             prices.left.erase(
                 prices.left.upper_bound(upperBound), prices.left.end());
 
+        // At least one element should remain since upperBound is either
+        // equal to oldestTime or is less than latestTime, in which case
+        // the data is deleted between the oldestTime and upperBound.
         if (prices.empty())
         {
-            RPC::inject_error(rpcOBJECT_NOT_FOUND, result);
+            // LCOV_EXCL_START
+            RPC::inject_error(rpcINTERNAL, result);
             return result;
+            // LCOV_EXCL_STOP
         }
     }
     result[jss::time] = latestTime;

src/ripple/rpc/handlers/LedgerEntry.cpp

@@ -624,7 +624,7 @@ doLedgerEntry(RPC::JsonContext& context)
                 auto const& oracle = context.params[jss::oracle];
                 auto const documentID = [&]() -> std::optional<std::uint32_t> {
                     auto const& id = oracle[jss::oracle_document_id];
-                    if (id.isConvertibleTo(Json::ValueType::uintValue))
+                    if (id.isUInt() || (id.isInt() && id.asInt() >= 0))
                         return std::make_optional(id.asUInt());
                     else if (id.isString())
                     {