diff --git a/packages/api-common/source/server.test.ts b/packages/api-common/source/server.test.ts index 8f8723acc..0d864a2b0 100644 --- a/packages/api-common/source/server.test.ts +++ b/packages/api-common/source/server.test.ts @@ -1,5 +1,6 @@ import { Enums, Identifiers } from "@mainsail/constants"; import { injectable } from "@mainsail/container"; +import { InvalidCriteria } from "@mainsail/exceptions"; import { Application } from "@mainsail/kernel"; import { describe } from "@mainsail/test-runner"; import { mkdtempSync, writeFileSync } from "fs"; @@ -135,6 +136,27 @@ describe<{ error.neverCalled(); }); + it("onPreResponse - converts a DatabaseException to badRequest and logs warn", async ({ subject, logger }) => { + await subject.initialize(Enums.Api.ServerType.Http, {}); + + await subject.route({ + handler() { + throw new InvalidCriteria("Invalid attribute key 'a'||pg_sleep(5)||'b'"); + }, + method: "GET", + path: "/invalid-criteria", + }); + + const warn = spy(logger, "warn"); + const error = spy(logger, "error"); + + const response = await subject.inject({ method: "GET", url: "/invalid-criteria" }); + assert.is(response.statusCode, 400); + assert.is(response.result.message, "Invalid attribute key 'a'||pg_sleep(5)||'b'"); + warn.calledOnce(); + error.neverCalled(); + }); + it("logs an error thrown from a request extension exactly once", async ({ subject, logger }) => { await subject.initialize(Enums.Api.ServerType.Http, {}); diff --git a/packages/api-common/source/server.ts b/packages/api-common/source/server.ts index 0380c315a..6cab6e63d 100644 --- a/packages/api-common/source/server.ts +++ b/packages/api-common/source/server.ts @@ -11,6 +11,7 @@ import { } from "@hapi/hapi"; import { Identifiers } from "@mainsail/constants"; import { inject, injectable } from "@mainsail/container"; +import { DatabaseException } from "@mainsail/exceptions"; import { ensureError, merge } from "@mainsail/utils"; import { readFileSync } from "fs"; @@ -59,8 +60,15 @@ export abstract class AbstractServer { if ("isBoom" in request.response && request.response.isBoom && request.response.isServer) { request.app.errorLogged = true; - if (request.response.name === "QueryFailedError") { - const message = `${request.response.name} ${request.response.message}`; + // Database-layer errors caused by bad request data — our own DatabaseException and + // TypeORM's QueryFailedError — are client faults: respond 400 (not 500) and log at warn + // without the stack. QueryFailedError is external so it can't extend DatabaseException; + // match it by name. + const isQueryFailedError = request.response.name === "QueryFailedError"; + if (isQueryFailedError || request.response instanceof DatabaseException) { + const message = isQueryFailedError + ? `${request.response.name} ${request.response.message}` + : request.response.message; this.logger.warn(`${request.path} - ${message}`); request.response = Boom.badRequest(message); } else { diff --git a/packages/api-database/package.json b/packages/api-database/package.json index 03df482b3..29e0ddbf3 100644 --- a/packages/api-database/package.json +++ b/packages/api-database/package.json @@ -23,6 +23,7 @@ "dependencies": { "@mainsail/constants": "workspace:*", "@mainsail/container": "workspace:*", + "@mainsail/exceptions": "workspace:*", "@mainsail/kernel": "workspace:*", "@mainsail/utils": "workspace:*", "dayjs": "1.11.20", diff --git a/packages/api-database/source/search/filters/wallet-filter.test.ts b/packages/api-database/source/search/filters/wallet-filter.test.ts index de3643c11..b997b6de9 100644 --- a/packages/api-database/source/search/filters/wallet-filter.test.ts +++ b/packages/api-database/source/search/filters/wallet-filter.test.ts @@ -131,4 +131,54 @@ describe("WalletFilter.getExpression", ({ it, assert }) => { value: "alice", }); }); + + it("should reject an attribute key with SQL-injection characters", async () => { + await assert.rejects( + () => WalletFilter.getExpression({ attributes: { "a'||pg_sleep(5)||'b": "1" } }), + "Invalid attribute key 'a'||pg_sleep(5)||'b'", + ); + }); + + it("should reject a nested attribute key with SQL-injection characters", async () => { + // The injection is in the nested key, which is only visible after flattening to a dotted path. + await assert.rejects( + () => WalletFilter.getExpression({ attributes: { validatorLastBlock: { "number')::bigint--": "1" } } }), + "Invalid attribute key 'validatorLastBlock.number')::bigint--'", + ); + }); + + it("should reject attribute keys containing injection metacharacters", async () => { + const maliciousKeys = [ + "x'; DROP TABLE wallets; --", // stacked statement + "x') UNION SELECT secret FROM users --", // union + "x\\'", // backslash + quote (standard_conforming_strings=off breakout) + "a b", // whitespace + 'a"b', // double quote + "a;b", // semicolon + "a(b)", // parentheses + ".leadingDot", // structural: leading dot + "trailingDot.", // structural: trailing dot + "double..dot", // structural: empty segment + ]; + + for (const key of maliciousKeys) { + await assert.rejects( + () => WalletFilter.getExpression({ attributes: { [key]: "1" } }), + `Invalid attribute key '${key}'`, + ); + } + }); + + it("should accept a safe custom attribute key outside the known list", async () => { + // The allowlist permits letters/digits/underscore dotted paths, not only the known attributes, + // so an unknown-but-safe key still builds a query (with undefined cast). + const expression = await WalletFilter.getExpression({ attributes: { my_custom_attr0: "1" } }); + + assert.equal(expression, { + jsonFieldAccessor: { cast: undefined, fieldName: "my_custom_attr0", operator: "->>" }, + op: "equal", + property: "attributes", + value: "1", + }); + }); }); diff --git a/packages/api-database/source/search/filters/wallet-filter.ts b/packages/api-database/source/search/filters/wallet-filter.ts index 3b388f143..a10cda49f 100644 --- a/packages/api-database/source/search/filters/wallet-filter.ts +++ b/packages/api-database/source/search/filters/wallet-filter.ts @@ -1,3 +1,4 @@ +import { InvalidCriteria } from "@mainsail/exceptions"; import { isObject } from "@mainsail/utils"; import type { Wallet } from "../../models/index.js"; @@ -71,6 +72,13 @@ export class WalletFilter { }; } + // jsonb attribute keys are interpolated into the SQL path expression, so restrict them to a + // safe identifier-path charset. This rejects quotes/operators an attacker could use for + // injection before the key reaches the query builder (defense-in-depth alongside the + // literal-escaping in QueryHelper.getColumnName). Real attributes are dotted identifier paths + // (e.g. "validatorLastBlock.number"), so this does not constrain legitimate queries. + private static readonly attributeKeyRegex = /^[A-Za-z0-9_]+(\.[A-Za-z0-9_]+)*$/; + public static async handleAttributesCriteria( criteria: Record>, ): Promise> { @@ -85,6 +93,10 @@ export class WalletFilter { v = v[nestedAttribute]; } + if (!this.attributeKeyRegex.test(k)) { + throw new InvalidCriteria(`Invalid attribute key '${k}'`); + } + return handleComparisonCriteria("attributes", v, { cast: this.inferAttributeCastType(k), fieldName: k, diff --git a/packages/api-database/source/search/query-helper.test.ts b/packages/api-database/source/search/query-helper.test.ts index 6d59fe1a6..cce6527a9 100644 --- a/packages/api-database/source/search/query-helper.test.ts +++ b/packages/api-database/source/search/query-helper.test.ts @@ -67,6 +67,64 @@ describe<{ ); }); + it("getColumnName escapes single quotes in json path segments (SQLi)", ({ helper, metadata }) => { + // A single quote in a path segment must be doubled so it cannot break out of the + // single-quoted SQL literal. Without escaping this produced `data->>'a'||pg_sleep(5)||'b'`. + assert.equal(helper.getColumnName(metadata, "data", { fieldName: "a'||pg_sleep(5)||'b", operator: "->>" }), { + isNullable: true, + name: `data->>'a''||pg_sleep(5)||''b'`, + }); + }); + + it("getColumnName escapes single quotes in nested json path segments (SQLi)", ({ helper, metadata }) => { + assert.equal( + helper.getColumnName(metadata, "data", { + cast: "bigint", + fieldName: "x')::bigint--.number", + operator: "->>", + }), + { isNullable: true, name: `(data->'x'')::bigint--'->>'number')::bigint` }, + ); + }); + + it("getColumnName escapes a backslash+quote segment as an E'' literal (GUC-independent, SQLi)", ({ + helper, + metadata, + }) => { + // A backslash before a quote breaks out of a plain literal when standard_conforming_strings + // is off. escapeLiteral doubles the backslash and emits an E'' literal, which is safe for + // any value of that setting. `"x\\'"` is the segment x + backslash + single-quote. + assert.equal(helper.getColumnName(metadata, "data", { fieldName: "x\\'", operator: "->>" }), { + isNullable: true, + name: `data->> E'x\\\\'''`, + }); + }); + + it("getColumnName neutralizes a stacked-statement payload into a single literal (SQLi)", ({ helper, metadata }) => { + // The `'; DROP TABLE ...; --` stays entirely inside one quoted literal — no statement break. + assert.equal( + helper.getColumnName(metadata, "data", { fieldName: "x'; DROP TABLE wallets; --", operator: "->>" }), + { isNullable: true, name: `data->>'x''; DROP TABLE wallets; --'` }, + ); + }); + + it("getColumnName neutralizes a UNION-select payload into a single literal (SQLi)", ({ helper, metadata }) => { + assert.equal( + helper.getColumnName(metadata, "data", { + fieldName: "x') UNION SELECT secret FROM users --", + operator: "->>", + }), + { isNullable: true, name: `data->>'x'') UNION SELECT secret FROM users --'` }, + ); + }); + + it("getColumnName doubles every quote in a segment with multiple quotes (SQLi)", ({ helper, metadata }) => { + assert.equal(helper.getColumnName(metadata, "data", { fieldName: "a''b", operator: "->>" }), { + isNullable: true, + name: `data->>'a''''b'`, + }); + }); + // --- getWhereExpressionSql: constants --- it("op true -> TRUE with no parameters", ({ helper, metadata }) => { diff --git a/packages/api-database/source/search/query-helper.ts b/packages/api-database/source/search/query-helper.ts index 5c780b20f..675331b2b 100644 --- a/packages/api-database/source/search/query-helper.ts +++ b/packages/api-database/source/search/query-helper.ts @@ -1,5 +1,7 @@ import type { EntityMetadata } from "typeorm"; +import { escapeLiteral } from "pg"; + import type { Expression, JsonFieldAccessor } from "./types/expressions.js"; export type SqlExpression = { @@ -25,17 +27,21 @@ export class QueryHelper { throw new Error(`Can't apply json field accessor to ${String(property)} column`); } + // jsonb path segments are interpolated into the SQL string as quoted literals, so they + // must be escaped to prevent breaking out of the literal (SQL injection). escapeLiteral + // returns the segment as a fully-quoted literal. + // 'validatorBlock.number' => ['validatorBlock', 'number'] const pathFields = jsonFieldAccessor.fieldName.split("."); - // ['validatorBlock', 'number'] => ['validatorBlock'] - const lastField = pathFields.splice(-1, 1); + // ['validatorBlock', 'number'] => 'number' + const lastField = pathFields.pop() ?? ""; // ['validatorBlock', 'nested', 'attribute'] => 'validatorBlock'->'nested'->'attribute' - const fieldPath = pathFields.map((f) => `'${f}'`).join("->"); + const fieldPath = pathFields.map((f) => escapeLiteral(f)).join("->"); // 'validatorBlock'->'last' => 'validatorBlock'->'last'->>'number' - let fullFieldPath = `${fieldPath}${jsonFieldAccessor.operator}'${lastField}'`; + let fullFieldPath = `${fieldPath}${jsonFieldAccessor.operator}${escapeLiteral(lastField)}`; if (fieldPath.length > 0) { // 'validatorBlock'->'last'->>'number' => column->'validatorBlock'->'last'->>'number' fullFieldPath = `${column.databaseName}->${fullFieldPath}`; diff --git a/packages/api-http/integration/routes/validators.test.ts b/packages/api-http/integration/routes/validators.test.ts index 87973f54a..a6e470148 100644 --- a/packages/api-http/integration/routes/validators.test.ts +++ b/packages/api-http/integration/routes/validators.test.ts @@ -55,6 +55,46 @@ describe<{ assert.equal(data.data, sorted); }); + it("/validators?attributes filters by a jsonb attribute (escapeLiteral path)", async () => { + await apiContext.walletRepository.save(validators); + + // Exercises WalletFilter.handleAttributesCriteria -> QueryHelper.getColumnName -> escapeLiteral + // with a legitimate user-supplied key, to guard against the escaping breaking real queries. + const { statusCode, data } = await request("/validators?attributes.validatorRank=5", options); + assert.equal(statusCode, 200); + assert.equal( + data.data, + validators.filter((v) => v.attributes.validatorRank === 5), + ); + }); + + it("/validators?attributes rejects SQL injection in attribute keys", async () => { + await apiContext.walletRepository.save(validators); + + // Same injectable position as /wallets (attributes route through the shared filter), so the + // validators endpoint must reject it identically. + const injectionPaths = [ + `/validators?attributes.${encodeURIComponent("a'||pg_sleep(3)||'b")}=1`, // timing / boolean-blind + `/validators?attributes.${encodeURIComponent("x'; DROP TABLE wallets; --")}=1`, // stacked statement + `/validators?attributes.validatorLastBlock.${encodeURIComponent("n')::bigint--")}=1`, // nested key + ]; + + for (const path of injectionPaths) { + let statusCode; + try { + statusCode = (await request(path, options)).statusCode; // must never be a 2xx + } catch (ex) { + statusCode = ex.response?.statusCode; + } + + assert.equal(statusCode, 400); + } + + // Intact and still queryable afterwards. + const after = await request("/validators", options); + assert.equal(after.statusCode, 200); + }); + it("/validators/{id}", async () => { await apiContext.walletRepository.save(validators); diff --git a/packages/api-http/integration/routes/wallets.test.ts b/packages/api-http/integration/routes/wallets.test.ts index 7bb1b4706..a7ff1b732 100644 --- a/packages/api-http/integration/routes/wallets.test.ts +++ b/packages/api-http/integration/routes/wallets.test.ts @@ -92,6 +92,42 @@ describe<{ } }); + it("/wallets?attributes rejects SQL injection in attribute keys", async () => { + await apiContext.walletRepository.save(wallets); + + // Control: a legitimate attribute filter still works. + const legit = await request("/wallets?attributes.validatorProducedBlocks.from=1", options); + assert.equal(legit.statusCode, 200); + + // Each payload places SQL metacharacters in the jsonb attribute KEY — the injectable position. + // They are rejected before any query is built/run without breaking out + // of the quoted jsonb path literal (blind exfiltration / pg_sleep DoS / stacked statements). + const injectionPaths = [ + `/wallets?attributes.${encodeURIComponent("a'||pg_sleep(3)||'b")}=1`, // timing / boolean-blind + `/wallets?attributes.${encodeURIComponent("x'; DROP TABLE wallets; --")}=1`, // stacked statement + `/wallets?attributes.${encodeURIComponent("x') UNION SELECT 1 --")}=1`, // union + `/wallets?attributes.${encodeURIComponent("x\\'")}=1`, // backslash+quote (SCS=off breakout) + `/wallets?attributes.validatorLastBlock.${encodeURIComponent("n')::bigint--")}=1`, // nested key + ]; + + for (const path of injectionPaths) { + let statusCode; + try { + statusCode = (await request(path, options)).statusCode; // must never be a 2xx + } catch (ex) { + statusCode = ex.response?.statusCode; + } + + // Rejected as a client error (bad request), not accepted and not a server error. + assert.equal(statusCode, 400); + } + + // The DB is intact and still queryable after the injection attempts (nothing was dropped/altered). + const after = await request("/wallets", options); + assert.equal(after.statusCode, 200); + assert.equal(after.data.data.length, wallets.length); + }); + it("/wallets/top", async () => { await apiContext.walletRepository.save(wallets); @@ -164,6 +200,34 @@ describe<{ } }); + it("/wallets?orderBy rejects SQL injection in the sort field", async () => { + await apiContext.walletRepository.save(wallets); + + // The sort field also feeds QueryHelper.getColumnName/escapeLiteral, so it must be rejected + // before it reaches the query builder. The orderBy schema (a stricter [.a-z] property guard) + // catches it at request validation, which surfaces as 422 rather than the 400 the attribute + // allowlist returns — either way it is a client error, never executed. + const injectionPaths = [ + `/wallets?orderBy=attributes.${encodeURIComponent("a'||pg_sleep(3)||'b")}:asc`, + `/wallets?orderBy=${encodeURIComponent("attributes.x'); DROP TABLE wallets; --")}:asc`, + ]; + + for (const path of injectionPaths) { + let statusCode; + try { + statusCode = (await request(path, options)).statusCode; // must never be a 2xx + } catch (ex) { + statusCode = ex.response?.statusCode; + } + + assert.equal(statusCode, 422); + } + + // A legitimate sort still works. + const after = await request("/wallets?orderBy=attributes.validatorRank:asc", options); + assert.equal(after.statusCode, 200); + }); + it("/wallets/{id}/transactions", async () => { await apiContext.walletRepository.save(wallets); await apiContext.transactionRepository.save(transactions); diff --git a/packages/api-http/test/helpers/prepare-sandbox.ts b/packages/api-http/test/helpers/prepare-sandbox.ts index c4bce5668..5bf358cae 100644 --- a/packages/api-http/test/helpers/prepare-sandbox.ts +++ b/packages/api-http/test/helpers/prepare-sandbox.ts @@ -205,6 +205,7 @@ export const prepareSandbox = async (context: { app: Application }): Promise console.log(message), info: (message) => console.log(message), notice: (message) => console.log(message), + warn: (message) => console.log(message), warning: (message) => console.log(message), }); diff --git a/packages/exceptions/source/database.ts b/packages/exceptions/source/database.ts new file mode 100644 index 000000000..20882e601 --- /dev/null +++ b/packages/exceptions/source/database.ts @@ -0,0 +1,5 @@ +import { Exception } from "./base.js"; + +export class DatabaseException extends Exception {} + +export class InvalidCriteria extends DatabaseException {} diff --git a/packages/exceptions/source/index.ts b/packages/exceptions/source/index.ts index e9a60881d..ddb35683e 100644 --- a/packages/exceptions/source/index.ts +++ b/packages/exceptions/source/index.ts @@ -4,6 +4,7 @@ export * from "./config.js"; export * from "./consensus.js"; export * from "./container.js"; export * from "./crypto.js"; +export * from "./database.js"; export * from "./filesystem.js"; export * from "./logic.js"; export * from "./p2p.js"; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f0da3f1cf..904aba446 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -253,6 +253,9 @@ importers: '@mainsail/container': specifier: workspace:* version: link:../container + '@mainsail/exceptions': + specifier: workspace:* + version: link:../exceptions '@mainsail/kernel': specifier: workspace:* version: link:../kernel