[fix] prevent SQL injection in QuestDB history queries

[Aias00]

Summary

Fixes SQL injection in the QuestDB history-data query path.

Vulnerability

QuestdbDataStorage builds all four history-query SQL templates with String.format, interpolating metric (a column name), table, and instance directly into the SQL. These values trace back to REST path variables:

GET /api/monitor/{instance}/metric/{metricFull}        // metricFull -> app.metrics.metric

So instance and metricFull are attacker-controlled. Two concrete gaps:

1. Identifiers (metric, table) are placed inside double-quoted identifiers with no charset check. A path value carrying a " or other SQL metacharacter can break out of the identifier.
2. String literal for instance in the WHERE metric_labels = '...' clause is escaped with replace("'", "\\'"). QuestDB does not treat backslash as an escape (it follows the ANSI rule of doubling the quote), so a stored metric_labels value containing a ' stayed injectable.

Fix

QuestDB's HTTP /exec endpoint does not support bind parameters, so the read path is hardened with validation + correct escaping:

• validateIdentifier(name, label) — rejects metric and table values outside ^[A-Za-z0-9_-]+$ before they reach String.format. Fails closed with IllegalArgumentException. None of the allowed characters can terminate the surrounding "..." identifier or introduce SQL syntax (a -- inside a quoted identifier is harmless).
• escapeStringLiteral(value) — replaces the broken backslash escape with the correct QuestDB/ANSI escape: doubling the single quote (' → '').

Verification

• mvn -pl hertzbeat-warehouse -am compile ✅ (JDK 25)
• mvn -pl hertzbeat-warehouse -am checkstyle:check ✅
• Existing valid inputs continue to work: instance=127.0.0.1:8080 → table linux_cpu_127_0_0_1_8080 (matches allowlist); hyphenated hostnames also pass.

No QuestDB unit tests exist in the module today, so no existing test coverage was extended here.

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[zqr10159]

Reviewed against the 1.9.0 pre-release QuestDB history-query finding. Identifier allowlisting and ANSI string-literal escaping close the reported injection paths; the current CI checks pass.

[Duansg]

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.