Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions packages/api-common/source/server.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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, {});

Expand Down
12 changes: 10 additions & 2 deletions packages/api-common/source/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions packages/api-database/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"dependencies": {
"@mainsail/constants": "workspace:*",
"@mainsail/container": "workspace:*",
"@mainsail/exceptions": "workspace:*",
"@mainsail/kernel": "workspace:*",
"@mainsail/utils": "workspace:*",
"dayjs": "1.11.20",
Expand Down
50 changes: 50 additions & 0 deletions packages/api-database/source/search/filters/wallet-filter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
});
});
});
12 changes: 12 additions & 0 deletions packages/api-database/source/search/filters/wallet-filter.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { InvalidCriteria } from "@mainsail/exceptions";
import { isObject } from "@mainsail/utils";

import type { Wallet } from "../../models/index.js";
Expand Down Expand Up @@ -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<string, NumericCriteria<string>>,
): Promise<OrExpression<Wallet>> {
Expand All @@ -85,6 +93,10 @@ export class WalletFilter {
v = v[nestedAttribute];
}

if (!this.attributeKeyRegex.test(k)) {
throw new InvalidCriteria(`Invalid attribute key '${k}'`);
}

return handleComparisonCriteria<Wallet, "attributes">("attributes", v, {
cast: this.inferAttributeCastType(k),
fieldName: k,
Expand Down
58 changes: 58 additions & 0 deletions packages/api-database/source/search/query-helper.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }) => {
Expand Down
14 changes: 10 additions & 4 deletions packages/api-database/source/search/query-helper.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import type { EntityMetadata } from "typeorm";

import { escapeLiteral } from "pg";

import type { Expression, JsonFieldAccessor } from "./types/expressions.js";

export type SqlExpression = {
Expand All @@ -25,17 +27,21 @@ export class QueryHelper<TEntity> {
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}`;
Expand Down
40 changes: 40 additions & 0 deletions packages/api-http/integration/routes/validators.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
64 changes: 64 additions & 0 deletions packages/api-http/integration/routes/wallets.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions packages/api-http/test/helpers/prepare-sandbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ export const prepareSandbox = async (context: { app: Application }): Promise<Api
error: (message) => console.log(message),
info: (message) => console.log(message),
notice: (message) => console.log(message),
warn: (message) => console.log(message),
warning: (message) => console.log(message),
});

Expand Down
5 changes: 5 additions & 0 deletions packages/exceptions/source/database.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { Exception } from "./base.js";

export class DatabaseException extends Exception {}

export class InvalidCriteria extends DatabaseException {}
1 change: 1 addition & 0 deletions packages/exceptions/source/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
Loading
Loading