Duansg commented on PR #4259:
URL: https://github.com/apache/hertzbeat/pull/4259#issuecomment-5232440193
Thanks for tracking this down — I reproduced the issue and the direction is
right. metric and table are interpolated into "%s" with no charset check, a "
closes the identifier, and any payload without a . gets past the controller's
three-segment split. replace("'", "\\'") is genuinely wrong for QuestDB (ANSI
quote doubling, backslash is not an escape). The endpoint is reachable by the
guest role (sureness.yml:31), so this is worth fixing.
A few things I'd like to resolve before this lands.
1. This fixes the least-exposed of four storages with the same pattern.
TDengine (TdEngineDataStorage:367) and InfluxDB (InfluxdbDataStorage:255)
interpolate history — a raw @RequestParam — straight into the SQL, with no
equivalent of QuestDB's getDateAdd() parsing. InfluxDB doesn't even have the
(broken) quote escaping for instanceValue. IoTDB has the same shape. Merging
this as-is means an operator on TDengine or InfluxDB has a larger injection
surface than the one we just closed.
Two options: extend this PR to tdengine/influxdb/iotdb, or do the identifier
and range validation once in MetricsDataServiceImpl — where all four storages
converge — and keep only escapeStringLiteral() here, since quote escaping is
the one genuinely dialect-specific piece that can't move up. Either way, could
we open a tracking issue for the remaining storages? I'd rather not have this
marked "fixed" while three of them stay open.
2. The read-side allowlist is narrower than what the write side can produce.
saveData() writes cell.getField().getName() verbatim as a column name, and
verifyDefineAppContent() applies no charset validation to field names — we ship
heap.memory.used (app-seatunnel.yml) and rocksdb.backup.read.bytes
(app-nebula_graph.yml) today. The REST path won't regress because the
three-segment split already rejects dotted names, but hertzbeat-ai's
MetricsToolsImpl:172 calls the service directly and its fieldParameter is
required = false, so it can be null or dotted. As a rule, a read-side validator
shouldn't be stricter than the writer — otherwise some stored columns become
unreadable by construction. Either constrain column names in saveData() too, or
widen the pattern to at least . and /.
3. Prefer normalizing table over validating it. The folding in
generateTable() is conditional:
if (instance.contains(".") || instance.contains(":") ||
instance.contains("["))
An instance like a]b skips the branch entirely, keeps the ], and gets
rejected by the new check. Dropping the if and folding unconditionally (or
replacing anything outside the charset) makes the table slot non-injectable by
construction and removes the need for validateIdentifier(table, ...). Note that
a full-charset normalization would change existing table names, so that part
needs a compatibility call. Keeping metric as reject-only is correct — it's a
column name and must match exactly.
4. Tests. The module already has TdEngineDataStorageTest,
GreptimeDbDataStorageTest, VictoriaMetricsDataStorageTest. Making the two new
methods package-private and adding four cases would lock this in: an identifier
that closes the quoting is rejected; what generateTable actually produces still
passes (127.0.0.1:8080, [::1]:8080, hyphenated hostnames); it's → it''s; null
metric.
Minor:
- QUERY_HISTORY_SQL_WITH_INSTANCE (line 70) has no references anywhere and
carries the same unescaped '%s' — worth deleting while we're here.
- The IDENTIFIER constant sits between methods around line 370 while the
other constants are at the top of the class. Checkstyle has no DeclarationOrder
module so CI is fine, just a readability nit.
- Unrelated, possibly worth its own issue: getDateAdd() only handles
d/h/m/s, but the chart buttons send 1W/4W/12W, which throws on QuestDB.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
