Skip to content

Commit 50a9974

Browse files
dawsontothclaude
andcommitted
fix(security): resolve the target database identically in authorization and handlers
`verifyPerms` resolved the database an operation targets as `requestJson.schema ?? requestJson.database`. The handlers resolve it in `commonUtils.transformReq`, which runs AFTER authorization. The two disagreed in three ways, each of which let a request be authorized against one target and executed against another: 1. Neither key present. `operationSchema` was undefined, so `schemaTableMap` stayed empty; `hasPermissions` iterates that map, so it authorized by vacuous truth, and `getAttributePermissions` returned an empty map so attribute permissions went unchecked too. `transformReq` meanwhile defaults to the default database, and the handler wrote there. Any non-empty role therefore had unconditional read/write/delete on `data`: verified for insert/update/upsert, delete, csv_data_load, create_attribute, and all three search_* operations. A role scoped exclusively to another database read, updated and deleted records in `data`. 2. `database: 0` (or any falsy-but-present value). `??` kept the `0`, which is not a database, so the map stayed empty as in (1) — while `transformReq`, which tests falsy, defaulted to `data` and wrote there. `Joi.number()` is an accepted type for the field, so `0` arrives validated. 3. Opposite precedence. `verifyPerms` preferred `schema`; `transformReq` prefers `database`. So `{schema:'data', database:'elsewhere'}` was authorized against `data` and written to `elsewhere`. This one reaches any named database, not just the default. Fix: one resolver, `commonUtils.resolveTargetDatabase`, which `transformReq` now delegates to, so authorization and the handlers cannot drift again. Falsy rather than nullish, and `database` over `schema`, matching what the handlers have always done. Plus a fail-closed backstop in `verifyPerms`: a named table with an empty `schemaTableMap` is denied rather than authorized. That is the shape of this whole bug class, and of the SQL path's GHSA-5c29-q62v-jrwf, whose fix carries the same guard. With the shared resolver it is unreachable by construction, so no test covers it; it is there so a future change to target resolution fails safe. Affected 4.2.0 through 5.2.6. `transformReq`'s default arrived in 4.2.0 (5d5e58f); 4.1 declared `schema` required, so earlier versions are not exploitable. The vacuous-map behaviour itself dates to ~4.0.8 (76ce6a2) but was inert until that default existed. 5da23c3 only carried the expression into the 5.x file. Not affected: REST, MQTT/WebSocket and GraphQL bind the database on the resource class at path-match time and never take it from the request body. The SQL path is the same bug class but a separate, already-fixed code path. Regression coverage in integrationTests/database/full-record-write.test.ts asserts all three cases, and asserts them in both directions — a denial alone does not prove resolution, since the backstop also denies an unresolved target, so the role that DOES hold rights on the default database must still be allowed and its write must land there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 4c8fa90 commit 50a9974

3 files changed

Lines changed: 180 additions & 7 deletions

File tree

integrationTests/database/full-record-write.test.ts

Lines changed: 142 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
* npm run test:integration -- "integrationTests/database/full-record-write.test.ts"
2323
* HARPER_STORAGE_ENGINE=lmdb npm run test:integration -- "integrationTests/database/full-record-write.test.ts"
2424
*/
25-
import { suite, test, before, after } from 'node:test';
25+
import { suite, test, describe, before, after } from 'node:test';
2626
import { ok, strictEqual, deepStrictEqual } from 'node:assert';
2727
import { resolve } from 'node:path';
2828
import { setTimeout as sleep } from 'node:timers/promises';
@@ -551,6 +551,147 @@ suite('full_record: full replace over the operations API', { skip: skipSuite },
551551
strictEqual((await read('Dog', 'us-8')).color, 'black', 'no rejected request may have written');
552552
});
553553

554+
// Authorization resolves the target database with `commonUtils.resolveTargetDatabase`, the same
555+
// helper the handlers use via `transformReq`. Each case below was a working bypass against a
556+
// divergent copy of that logic: `hasPermissions` iterates `schemaTableMap`, so a target it fails
557+
// to resolve leaves the map empty and authorizes by vacuous truth, while the handler goes on to
558+
// write whatever the handlers' own resolution says.
559+
describe('target-database resolution is shared with the handlers', () => {
560+
let deniedHeaders: Record<string, string>;
561+
let dataOnlyHeaders: Record<string, string>;
562+
563+
before(async () => {
564+
await insert('Dog', [{ id: 'authz-1', name: 'Penny', breed: 'Mutt' }]);
565+
deniedHeaders = await addScopedRole(
566+
'no_update_data',
567+
{
568+
data: {
569+
tables: {
570+
Dog: { read: true, insert: false, update: false, delete: false, attribute_permissions: [] },
571+
},
572+
},
573+
},
574+
'no_update_data_user'
575+
);
576+
// Full rights on `data` and none anywhere else.
577+
dataOnlyHeaders = await addScopedRole(
578+
'data_only',
579+
{
580+
data: {
581+
tables: {
582+
Dog: { read: true, insert: true, update: true, delete: true, attribute_permissions: [] },
583+
},
584+
},
585+
},
586+
'data_only_user'
587+
);
588+
});
589+
590+
// The original finding: no database key at all.
591+
test('an omitted database does not skip the table permission check', async () => {
592+
const r = await asUser(deniedHeaders, {
593+
operation: 'update',
594+
table: 'Dog',
595+
records: [{ id: 'authz-1', name: 'bypass' }],
596+
});
597+
strictEqual(r.status, 403, `expected a denial; got ${r.status} ${JSON.stringify(r.body)}`);
598+
strictEqual((await read('Dog', 'authz-1')).name, 'Penny', 'nothing may have been written');
599+
});
600+
601+
test('the same request is denied when it does name the database', async () => {
602+
const r = await asUser(deniedHeaders, {
603+
operation: 'update',
604+
database: 'data',
605+
table: 'Dog',
606+
records: [{ id: 'authz-1', name: 'bypass' }],
607+
});
608+
strictEqual(r.status, 403, `expected a denial; got ${r.status} ${JSON.stringify(r.body)}`);
609+
});
610+
611+
// A falsy-but-present database. `??` kept the `0`, which is not a database and left the map
612+
// empty; `transformReq` tests falsy and so defaulted to `data` and wrote there. `Joi.number()`
613+
// is an accepted type for the field, so `0` arrives validated.
614+
//
615+
// Both directions, because "denied" alone does not prove resolution: the fail-closed backstop
616+
// also denies an unresolved target, so a nullish-coalescing resolver would pass a
617+
// denial-only assertion while still not agreeing with the handlers. The role that HAS rights on
618+
// `data` must be allowed, and the write must land in `data` — which only holds if authorization
619+
// resolved `database: 0` to the default database exactly as `transformReq` does.
620+
test('a falsy database value resolves to the default database, not to nothing', async () => {
621+
for (const database of [0, -0]) {
622+
const denied = await asUser(deniedHeaders, {
623+
operation: 'update',
624+
database,
625+
table: 'Dog',
626+
records: [{ id: 'authz-1', name: 'bypass' }],
627+
});
628+
strictEqual(
629+
denied.status,
630+
403,
631+
`database: ${JSON.stringify(database)} must be denied for a role without update; got ${denied.status} ${JSON.stringify(denied.body)}`
632+
);
633+
}
634+
strictEqual((await read('Dog', 'authz-1')).name, 'Penny', 'nothing may have been written');
635+
636+
const allowed = await asUser(dataOnlyHeaders, {
637+
operation: 'update',
638+
database: 0,
639+
table: 'Dog',
640+
records: [{ id: 'authz-1', name: 'resolved to data' }],
641+
});
642+
strictEqual(
643+
allowed.status,
644+
200,
645+
`a role with rights on the default database must be allowed; got ${allowed.status} ${JSON.stringify(allowed.body)}`
646+
);
647+
strictEqual((await read('Dog', 'authz-1')).name, 'resolved to data', 'the write must land in `data`');
648+
});
649+
650+
// The worst of the three, because it reaches any named database rather than just the default:
651+
// authorization preferred `schema` while the handlers prefer `database`, so a role with rights
652+
// on `data` could be authorized against `data` and have the write land in another database.
653+
test('a request cannot be authorized against one database and written to another', async () => {
654+
strictEqual((await ops({ operation: 'create_database', database: 'elsewhere' })).status, 200);
655+
strictEqual(
656+
(await ops({ operation: 'create_table', database: 'elsewhere', table: 'Dog', primary_key: 'id' })).status,
657+
200
658+
);
659+
strictEqual(
660+
(
661+
await ops({
662+
operation: 'insert',
663+
database: 'elsewhere',
664+
table: 'Dog',
665+
records: [{ id: 'e-1', name: 'Keep' }],
666+
})
667+
).status,
668+
200
669+
);
670+
671+
const r = await asUser(dataOnlyHeaders, {
672+
operation: 'update',
673+
schema: 'data',
674+
database: 'elsewhere',
675+
table: 'Dog',
676+
records: [{ id: 'e-1', name: 'crossed over' }],
677+
});
678+
strictEqual(
679+
r.status,
680+
403,
681+
`the write targets 'elsewhere', which this role has no rights to; got ${r.status} ${JSON.stringify(r.body)}`
682+
);
683+
684+
const stored = await ops({
685+
operation: 'search_by_id',
686+
database: 'elsewhere',
687+
table: 'Dog',
688+
ids: ['e-1'],
689+
get_attributes: ['*'],
690+
});
691+
strictEqual(stored.body?.[0]?.name, 'Keep', `the record in 'elsewhere' must be untouched`);
692+
});
693+
});
694+
554695
test('a non-boolean full_record is rejected rather than coerced', async () => {
555696
await insert('Dog', [{ id: 'fr-6', name: 'Penny', breed: 'Mutt', color: 'black' }]);
556697

utility/common_utils.ts

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -828,16 +828,30 @@ export function httpRequest(options: any, data: any): Promise<http.IncomingMessa
828828
});
829829
}
830830

831+
/**
832+
* Which database an operation request targets: `database` wins over the legacy `schema`, and a
833+
* request that names neither targets the default database.
834+
*
835+
* The single source of truth for that question. Authorization has to answer it identically to the
836+
* handlers — it runs first, and if the two disagree then the permissions checked are not the
837+
* permissions for the write that happens. Two divergences between this and the copy that used to
838+
* live in `verifyPerms` (`schema ?? database`) were each exploitable on their own: nullish
839+
* coalescing let a falsy-but-present `database: 0` through where this defaults, and the reversed
840+
* precedence let a request authorize against `schema` while the handler wrote `database`.
841+
*
842+
* Deliberately falsy rather than nullish: a `database` of `0` or `''` is not a database, and
843+
* `Joi.number()` is an accepted type for the field, so `0` reaches here validated.
844+
*/
845+
export function resolveTargetDatabase(req: any): string {
846+
return req.database || req.schema || terms.DEFAULT_DATABASE_NAME;
847+
}
848+
831849
/**
832850
* Will set default schema/database or set database to schema
833851
* @param req
834852
*/
835853
export function transformReq(req: any) {
836-
if (!req.schema && !req.database) {
837-
req.schema = terms.DEFAULT_DATABASE_NAME;
838-
return;
839-
}
840-
if (req.database) req.schema = req.database;
854+
req.schema = resolveTargetDatabase(req);
841855
}
842856

843857
export function convertToMS(interval: any) {

utility/operation_authorization.ts

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -674,7 +674,15 @@ export function verifyPerms(requestJson: any, operation: any, options?: { apiOpe
674674
//we need to use the action value, if present, to ensure the correct permission is checked below
675675
let action = requestJson.action;
676676

677-
let operationSchema = requestJson.schema ?? requestJson.database;
677+
// Resolved through the same helper the handlers use (`transformReq` delegates to it), because
678+
// authorization runs BEFORE `transformReq` and any disagreement about the target means the
679+
// permissions checked are not the permissions for the write that happens. This used to be a
680+
// local `schema ?? database`, which diverged from the handlers in two separately exploitable
681+
// ways: it kept a falsy-but-present `database: 0` instead of defaulting, and it preferred
682+
// `schema` where the handlers prefer `database` — so a request could be authorized against one
683+
// database and written to another. With no target resolved at all, `schemaTableMap` stayed empty
684+
// and `hasPermissions` iterated nothing, authorizing by vacuous truth.
685+
let operationSchema = commonUtils.resolveTargetDatabase(requestJson);
678686
let table = requestJson.table;
679687

680688
let schemaTableMap = new Map();
@@ -801,6 +809,16 @@ export function verifyPerms(requestJson: any, operation: any, options?: { apiOpe
801809
}
802810
}
803811

812+
// Fail closed on an unresolved target. `hasPermissions` iterates `schemaTableMap`, so an empty map
813+
// authorizes by vacuous truth — the shape of this whole bug class, and of the SQL path's
814+
// GHSA-5c29-q62v-jrwf, whose fix carries the same backstop. `resolveTargetDatabase` always
815+
// returns a database, so a named table always populates the map and this is unreachable today; it
816+
// is here so that a future change to target resolution fails safe instead of silently authorizing
817+
// everything. That also means no test can cover it, which is the point rather than an omission.
818+
if (table && schemaTableMap.size === 0) {
819+
return permsResponse.handleUnauthorizedItem(HDB_ERROR_MSGS.UNKNOWN_OP_AUTH_ERROR(op, operationSchema, table));
820+
}
821+
804822
let failedPermissions = hasPermissions(requestJson.hdb_user, op, schemaTableMap, permsResponse, action);
805823
//check if failedTablePerms are back and return them B/C it will be an op-level permission issue
806824
if (failedPermissions) {

0 commit comments

Comments
 (0)