fix(parsers): harden RPC/price/updater parsing against valid-but-unhandled input
Audit of the full-node RPC response parsers and updater release-body parsers (find -> adversarially-verify workflow) surfaced three worth fixing; three others guard formats the project doesn't emit and are backstopped by signature verification, so they're documented rather than churned. - Price (Medium): parseCoinGeckoPriceResponse used .value(key, 0.0), which throws type_error on a PRESENT null. CoinGecko emits null for usd_24h_change/ usd_24h_vol on illiquid tokens (DRGX is one) while still returning a valid spot price; the outer catch turned that into no price update at all. Read null-tolerantly so the valid usd/btc survives. - Daemon updater (Medium): parseDaemonChecksums blanked '|'/backtick but not markdown emphasis, so a bolded **archive.zip** checksum row was dropped and a valid, correctly-signed release would be refused. Also blank '*'/'_' (cannot cause a wrong-asset match; the 64-hex + .zip-suffix tests are unchanged). - Opid poll (Low, severe failure mode): parseOperationStatusPoll read id/status via .value() (throws on a present non-string) and the call site parsed OUTSIDE its try/catch, so a throw left opid_poll_in_progress_ stuck true and wedged all z-operation polling for the session. Type-check the reads and parse inside the guard. (dragonxd can't emit non-string id/status; this is defense-in-depth.) Regression tests: CoinGecko null field; opid non-string id/status; **bold** checksum row. Suite green (1/1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1129,6 +1129,7 @@ void App::update()
|
|||||||
auto* rpc = (fast_rpc_ && fast_rpc_->isConnected()) ? fast_rpc_.get() : rpc_.get();
|
auto* rpc = (fast_rpc_ && fast_rpc_->isConnected()) ? fast_rpc_.get() : rpc_.get();
|
||||||
if (!rpc) return [this](){ opid_poll_in_progress_ = false; };
|
if (!rpc) return [this](){ opid_poll_in_progress_ = false; };
|
||||||
json result;
|
json result;
|
||||||
|
services::NetworkRefreshService::OperationStatusPollResult parsed;
|
||||||
try {
|
try {
|
||||||
rpc::RPCClient::TraceScope trace("Send tab / Operation status");
|
rpc::RPCClient::TraceScope trace("Send tab / Operation status");
|
||||||
// No per-opid filter: this daemon rejects z_getoperationstatus(["opid"]) with
|
// No per-opid filter: this daemon rejects z_getoperationstatus(["opid"]) with
|
||||||
@@ -1136,10 +1137,12 @@ void App::update()
|
|||||||
// "Waiting for operation". The no-arg form returns ALL operations;
|
// "Waiting for operation". The no-arg form returns ALL operations;
|
||||||
// parseOperationStatusPoll() filters down to the opids we're tracking.
|
// parseOperationStatusPoll() filters down to the opids we're tracking.
|
||||||
result = rpc->call("z_getoperationstatus", json::array());
|
result = rpc->call("z_getoperationstatus", json::array());
|
||||||
|
// Parse INSIDE the guard: a malformed/type-anomalous element must never abort the
|
||||||
|
// poll and leave opid_poll_in_progress_ stuck true for the whole connected session.
|
||||||
|
parsed = services::NetworkRefreshService::parseOperationStatusPoll(result, opids);
|
||||||
} catch (...) {
|
} catch (...) {
|
||||||
return [this](){ opid_poll_in_progress_ = false; };
|
return [this](){ opid_poll_in_progress_ = false; };
|
||||||
}
|
}
|
||||||
auto parsed = services::NetworkRefreshService::parseOperationStatusPoll(result, opids);
|
|
||||||
return [this, parsed = std::move(parsed)]() mutable {
|
return [this, parsed = std::move(parsed)]() mutable {
|
||||||
opid_poll_in_progress_ = false;
|
opid_poll_in_progress_ = false;
|
||||||
|
|
||||||
|
|||||||
@@ -426,12 +426,21 @@ std::optional<NetworkRefreshService::PriceRefreshResult> NetworkRefreshService::
|
|||||||
if (!parsed.contains("dragonx-2")) return std::nullopt;
|
if (!parsed.contains("dragonx-2")) return std::nullopt;
|
||||||
|
|
||||||
const auto& data = parsed["dragonx-2"];
|
const auto& data = parsed["dragonx-2"];
|
||||||
|
// CoinGecko emits JSON null (not an omitted key) for fields it can't currently compute —
|
||||||
|
// commonly usd_24h_change on illiquid/newly-listed tokens — while still returning a valid
|
||||||
|
// spot price in the same object. .value(key, default) throws type_error on a PRESENT null,
|
||||||
|
// which the outer catch turns into "no price update at all", so read null-tolerantly and
|
||||||
|
// keep the valid usd/btc rather than discarding the whole refresh.
|
||||||
|
auto num = [&data](const char* key, double def) {
|
||||||
|
auto it = data.find(key);
|
||||||
|
return (it != data.end() && it->is_number()) ? it->get<double>() : def;
|
||||||
|
};
|
||||||
PriceRefreshResult result;
|
PriceRefreshResult result;
|
||||||
result.market.price_usd = data.value("usd", 0.0);
|
result.market.price_usd = num("usd", 0.0);
|
||||||
result.market.price_btc = data.value("btc", 0.0);
|
result.market.price_btc = num("btc", 0.0);
|
||||||
result.market.change_24h = data.value("usd_24h_change", 0.0);
|
result.market.change_24h = num("usd_24h_change", 0.0);
|
||||||
result.market.volume_24h = data.value("usd_24h_vol", 0.0);
|
result.market.volume_24h = num("usd_24h_vol", 0.0);
|
||||||
result.market.market_cap = data.value("usd_market_cap", 0.0);
|
result.market.market_cap = num("usd_market_cap", 0.0);
|
||||||
|
|
||||||
char buf[64];
|
char buf[64];
|
||||||
// Runs on the RPC worker thread — std::localtime shares a process-wide static tm, so use the
|
// Runs on the RPC worker thread — std::localtime shares a process-wide static tm, so use the
|
||||||
@@ -1101,12 +1110,16 @@ NetworkRefreshService::OperationStatusPollResult NetworkRefreshService::parseOpe
|
|||||||
std::set<std::string> reported;
|
std::set<std::string> reported;
|
||||||
for (const auto& op : result) {
|
for (const auto& op : result) {
|
||||||
if (!op.is_object()) continue;
|
if (!op.is_object()) continue;
|
||||||
std::string opid = op.value("id", std::string());
|
// Type-checked reads: .value(key, default) throws if the key is PRESENT with a non-string
|
||||||
|
// type, which would abort the whole poll (and wedge it for the session — see the call site).
|
||||||
|
if (!op.contains("id") || !op["id"].is_string()) continue;
|
||||||
|
std::string opid = op["id"].get<std::string>();
|
||||||
if (opid.empty()) continue;
|
if (opid.empty()) continue;
|
||||||
if (requested.find(opid) == requested.end()) continue; // not one of ours — ignore
|
if (requested.find(opid) == requested.end()) continue; // not one of ours — ignore
|
||||||
reported.insert(opid);
|
reported.insert(opid);
|
||||||
|
|
||||||
std::string status = op.value("status", std::string());
|
std::string status = (op.contains("status") && op["status"].is_string())
|
||||||
|
? op["status"].get<std::string>() : std::string();
|
||||||
if (status == "success") {
|
if (status == "success") {
|
||||||
parsed.doneOpids.push_back(opid);
|
parsed.doneOpids.push_back(opid);
|
||||||
parsed.anySuccess = true;
|
parsed.anySuccess = true;
|
||||||
|
|||||||
@@ -140,15 +140,16 @@ std::map<std::string, std::string> parseDaemonChecksums(const std::string& body)
|
|||||||
// | File | SHA-256 |
|
// | File | SHA-256 |
|
||||||
// |------|---------|
|
// |------|---------|
|
||||||
// | dragonx-1.0.2-linux-amd64.zip | `85f1dd…16` |
|
// | dragonx-1.0.2-linux-amd64.zip | `85f1dd…16` |
|
||||||
// Per line: blank out the table/code delimiters ('|' and '`'), then find the 64-hex token (the
|
// Per line: blank out the table/code/emphasis delimiters ('|', '`', and markdown '*'/'_' so a
|
||||||
// hash) and a token ending in ".zip" (the archive name). Header/separator/prose rows lack one
|
// bolded **archive.zip** still tokenizes), then find the 64-hex token (the hash) and a token
|
||||||
// or the other and are skipped, so this is robust to surrounding text and column order.
|
// ending in ".zip" (the archive name). Header/separator/prose rows lack one or the other and are
|
||||||
|
// skipped, so this is robust to surrounding text and column order.
|
||||||
std::map<std::string, std::string> out;
|
std::map<std::string, std::string> out;
|
||||||
std::istringstream in(body);
|
std::istringstream in(body);
|
||||||
std::string line;
|
std::string line;
|
||||||
while (std::getline(in, line)) {
|
while (std::getline(in, line)) {
|
||||||
for (char& c : line)
|
for (char& c : line)
|
||||||
if (c == '|' || c == '`') c = ' ';
|
if (c == '|' || c == '`' || c == '*' || c == '_') c = ' ';
|
||||||
std::istringstream ls(line);
|
std::istringstream ls(line);
|
||||||
std::string tok, hash, name;
|
std::string tok, hash, name;
|
||||||
while (ls >> tok) {
|
while (ls >> tok) {
|
||||||
|
|||||||
@@ -2032,6 +2032,20 @@ void testNetworkRefreshResultModels()
|
|||||||
EXPECT_EQ(state.market.price_history.size(), static_cast<size_t>(1));
|
EXPECT_EQ(state.market.price_history.size(), static_cast<size_t>(1));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Regression: CoinGecko emits JSON null (not an omitted key) for fields it can't compute —
|
||||||
|
// commonly usd_24h_change on illiquid tokens like DRGX — while still returning a valid spot
|
||||||
|
// price. The parser must keep the valid usd/btc, not discard the whole update on the null.
|
||||||
|
auto priceNull = Refresh::parseCoinGeckoPriceResponse(
|
||||||
|
R"({"dragonx-2":{"usd":0.42,"btc":0.000009,"usd_24h_change":null,"usd_24h_vol":null,"usd_market_cap":50000}})",
|
||||||
|
0);
|
||||||
|
EXPECT_TRUE(priceNull.has_value());
|
||||||
|
if (priceNull) {
|
||||||
|
EXPECT_NEAR(priceNull->market.price_usd, 0.42, 0.00000001);
|
||||||
|
EXPECT_NEAR(priceNull->market.price_btc, 0.000009, 0.00000001);
|
||||||
|
EXPECT_NEAR(priceNull->market.change_24h, 0.0, 0.0001); // null -> default, not a throw
|
||||||
|
EXPECT_NEAR(priceNull->market.market_cap, 50000.0, 0.0001);
|
||||||
|
}
|
||||||
|
|
||||||
Refresh::markPriceRefreshStarted(state);
|
Refresh::markPriceRefreshStarted(state);
|
||||||
Refresh::applyPriceRefreshFailure(state, "timeout");
|
Refresh::applyPriceRefreshFailure(state, "timeout");
|
||||||
EXPECT_FALSE(state.market.price_loading);
|
EXPECT_FALSE(state.market.price_loading);
|
||||||
@@ -2213,6 +2227,20 @@ void testOperationStatusPollParsing()
|
|||||||
EXPECT_FALSE(malformed.anySuccess);
|
EXPECT_FALSE(malformed.anySuccess);
|
||||||
EXPECT_TRUE(malformed.doneOpids.empty());
|
EXPECT_TRUE(malformed.doneOpids.empty());
|
||||||
EXPECT_TRUE(malformed.staleOpids.empty());
|
EXPECT_TRUE(malformed.staleOpids.empty());
|
||||||
|
|
||||||
|
// Regression: a type-anomalous element (non-string "id"/"status") must be skipped, not throw —
|
||||||
|
// a throw here would escape the parser and permanently wedge opid polling for the session. A
|
||||||
|
// valid tracked opid alongside the anomaly must still be processed.
|
||||||
|
auto typeSafe = Refresh::parseOperationStatusPoll(json::array({
|
||||||
|
json{{"id", 12345}, {"status", "failed"}}, // non-string id -> skipped, no throw
|
||||||
|
json{{"id", "op-ok"}, {"status", 7}}, // non-string status -> "" (not done)
|
||||||
|
json{{"id", "op-good"}, {"status", "success"}, {"result", json{{"txid", "tx-good"}}}}
|
||||||
|
}), {"op-ok", "op-good", "op-x"});
|
||||||
|
EXPECT_TRUE(typeSafe.anySuccess);
|
||||||
|
EXPECT_EQ(typeSafe.successTxidsByOpid.at("op-good"), std::string("tx-good"));
|
||||||
|
EXPECT_TRUE(typeSafe.failureMessages.empty()); // the non-string-id "failed" was skipped
|
||||||
|
EXPECT_EQ(typeSafe.staleOpids.size(), static_cast<size_t>(1)); // op-x absent; op-ok was seen (not stale)
|
||||||
|
EXPECT_EQ(typeSafe.staleOpids[0], std::string("op-x"));
|
||||||
}
|
}
|
||||||
|
|
||||||
void testSecureVaultScope()
|
void testSecureVaultScope()
|
||||||
@@ -6260,6 +6288,12 @@ void testDaemonChecksumParsing()
|
|||||||
"| DragonX-1.0.2-Win64.ZIP | `dd6a554ac05c834da9910ae796215567e97c426f3aed15b54af1f7b90d48c43a` |");
|
"| DragonX-1.0.2-Win64.ZIP | `dd6a554ac05c834da9910ae796215567e97c426f3aed15b54af1f7b90d48c43a` |");
|
||||||
EXPECT_EQ(mixed.at("dragonx-1.0.2-win64.zip"),
|
EXPECT_EQ(mixed.at("dragonx-1.0.2-win64.zip"),
|
||||||
std::string("dd6a554ac05c834da9910ae796215567e97c426f3aed15b54af1f7b90d48c43a"));
|
std::string("dd6a554ac05c834da9910ae796215567e97c426f3aed15b54af1f7b90d48c43a"));
|
||||||
|
// Regression: a markdown-bolded filename (**archive.zip**) must still parse — otherwise a valid,
|
||||||
|
// correctly-signed release whose body bolds the name would fail checksum lookup and be refused.
|
||||||
|
const auto bold = parseDaemonChecksums(
|
||||||
|
"| **dragonx-1.0.2-win64.zip** | `dd6a554ac05c834da9910ae796215567e97c426f3aed15b54af1f7b90d48c43a` |");
|
||||||
|
EXPECT_EQ(bold.at("dragonx-1.0.2-win64.zip"),
|
||||||
|
std::string("dd6a554ac05c834da9910ae796215567e97c426f3aed15b54af1f7b90d48c43a"));
|
||||||
}
|
}
|
||||||
|
|
||||||
void testDaemonBasenamesAndVersionCore()
|
void testDaemonBasenamesAndVersionCore()
|
||||||
|
|||||||
Reference in New Issue
Block a user