diff --git a/DESIGN.md b/DESIGN.md index f5a3347b62..bd119fe288 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -1899,3 +1899,19 @@ degrades to the historical behavior rather than replacing it. Invariants that ar ## A worker that misses an ITC ack gets its OS thread state logged (`server/threads/manageThreads.js`) `broadcastWithAcknowledgement` already times out (30 s) on a worker whose port stays open but never acks, and that shape is almost always a blocked event loop — a native lock, a runaway synchronous call — which nothing inside the worker can report (harper-pro#788: a restarted node's single http worker went byte-silent while main kept serving `cluster_status`, and the app log only said "not acknowledged by worker thread(s) 2"). So each worker posts its Linux thread id (`readlink /proc/thread-self`) to main once at startup, before anything else runs on it, and the timeout branch reads that thread's kernel state from `/proc/self/task/`: state, `wchan`, the syscall number (the first token only — the rest of that file is argument registers and stack/instruction pointers), CPU ticks, and context-switch counts, plus two cross-platform signals main already has, `worker.performance.eventLoopUtilization()` and the age of the last 1 s resource report. It samples again a second later and logs the deltas: no CPU ticks, no context switches and `event loop active +1000ms` is "parked on a lock"; ticks climbing with state `R` is "spinning". It is deliberately main-thread-only and best-effort: `workers` and the tid live on the main thread's `Worker` objects, every `/proc` field is reported individually (a hardened container may deny `wchan`/`syscall` while `stat` stays readable), a follow-up sample whose `starttime` differs from the first is discarded (the tid may have been recycled), one diagnostic runs per worker with a 30 s cooldown so concurrent timeouts on the same worker don't multiply reads, and nothing here runs when acks arrive on time. It does not name the lock owner; that still needs a native stack from the next occurrence. + +## `chooseOperation` authorizes the invoked operation against the authenticated principal (`server/serverHelpers/serverUtilities.ts`) + +`verifyPerms` takes a request-shaped object and reads _both_ halves of the permission question off it: the principal from `hdb_user`, and the tables from `schema`/`database`/`table`/`records`. `chooseOperation` used to hand it `json.search_operation` — a caller-supplied field — which made both halves body-controlled. Fixing one half and not the other is not a fix: with an empty `search_operation` the table map is empty, and `hasPermissions` iterating nothing authorizes everything. Regression cover: `integrationTests/security/choose-operation-authz.test.ts`. + +Four rules hold this together, and all four are load-bearing: + +**The principal comes from authentication.** Authentication sets only the _top-level_ `hdb_user`, and `validateRequestBodyProperties` inspects only top-level keys, so a nested `hdb_user` must be overwritten, never backfilled `if (!...)`. All four callers of `chooseOperation` (`serverHandlers`, `serverUtilities.operation`, `registeredOperations` worker forwarding, MCP) set the top-level principal before dispatch, which is why this belongs here rather than only at the HTTP boundary. + +**`search_operation` is the permission subject only for the operations that consume it.** `dataLayer/export.ts` is its sole consumer (`export_local`, `export_to_s3`); for any other operation the substitution checks the nested tables while the handler runs against the top-level ones, so it is gated on the operation name. It must also be an object naming one of export's supported operations (`search_by_value`/`search_by_hash`/`search_by_conditions`/`sql`) — a primitive, `{}`, or an unsupported operation is a request-time 400, not a wrapped 500 or an asynchronously-failed job. + +**One check cannot authorize both the outer export and its nested query.** The outer op's own `verifyPerms` returns before any table check — `export_local`/`export_to_s3` are `requires_su`, and a role that lists the operation in `operations` is granted at gate 2 (an explicit listing of an SU-only operation is a deliberate grant). The job worker then runs `search_operation` through `searchByValue`/`searchByHash`/`searchByConditions`, none of which check permissions. So the outer invocation is authorized first, and then the nested search is authorized additively against its _real_ search handler (`getOperationFunction(search_operation)`) and the authenticated principal — otherwise a role granted `export_local` could export a table it holds no grant on. A nested `sql` search takes the SQL branch instead, but the same two-part shape holds: the outer export op runs through `verifyPerms` (so its `requires_su` gate, the `operations` allowlist, and the export token scope all apply, exactly as on the non-SQL path — SQL must not be a way around the requires_su gate), the statement must be a `SELECT` because export is read-only, and `checkASTPermissions` then covers the statement's tables. A direct `sql` call has no outer job op, so there the `operations` allowlist alone is the operation-invocation check. + +**`parsed_sql_object` is dispatch state, never client input.** The export worker re-reads it off the same caller-supplied nested object (`evaluateSQL`), and it carries `permissions_checked`, so a body-supplied one runs an AST no check ever saw. It is deleted from the nested object at dispatch, and stripped from the top-level object before this dispatch's own parse is assigned. Only the direct-SQL path consumes the top-level `parsed_sql_object`; a job re-parses off `search_operation`, so setting it for a job would be inert. The bypass/`apiOperation` decision is carried on async-context state (`getOperationAuthorizationState`), not on the request body, and `processAST` honors the denial `checkASTPermissions` computes — a `PermissionResponseObject` has no `length`, so the guard tests the object itself rather than `.length` (which always refused nothing). + +The SQL and job paths are additive rather than exclusive: `verifyPermsAST` validates only the statement's tables and attributes, never the `operations` allowlist or `requires_su`, and a table-free statement gives it nothing to validate — so the allowlist check and the AST check both run for a SQL-carrying request, and the nested-search check runs alongside the outer export check for a job. diff --git a/integrationTests/apiTests/northwind.test.mjs b/integrationTests/apiTests/northwind.test.mjs index b710bf2ec4..7237d98d19 100644 --- a/integrationTests/apiTests/northwind.test.mjs +++ b/integrationTests/apiTests/northwind.test.mjs @@ -12477,7 +12477,21 @@ suite('Northwind operations', { skip: skipSuite }, (ctx) => { from northnwd.shippers`, }, }) - .expect(200); + .expect((r) => + assert.equal( + r.body.error, + 'This operation is not authorized due to role restrictions and/or invalid database items', + r.text + ) + ) + .expect((r) => + assert.equal( + r.body.unauthorized_access[0], + "Operation 'export_local' is restricted to 'super_user' roles", + r.text + ) + ) + .expect(403); }); test('Jobs Test Export To Local using SQL on RESTRICTED table as test_user', async () => { @@ -12494,16 +12508,20 @@ suite('Northwind operations', { skip: skipSuite }, (ctx) => { from northnwd.suppliers`, }, }) - .expect((r) => { + .expect((r) => assert.equal( r.body.error, 'This operation is not authorized due to role restrictions and/or invalid database items', r.text - ); - assert.equal(r.body.invalid_schema_items.length, 1, r.text); - assert.equal(r.body.invalid_schema_items[0], "Table 'northnwd.suppliers' does not exist", r.text); - assert.equal(r.body.unauthorized_access.length, 0, r.text); - }) + ) + ) + .expect((r) => + assert.equal( + r.body.unauthorized_access[0], + "Operation 'export_local' is restricted to 'super_user' roles", + r.text + ) + ) .expect(403); }); @@ -12521,7 +12539,21 @@ suite('Northwind operations', { skip: skipSuite }, (ctx) => { from northnwd.region`, }, }) - .expect(200); + .expect((r) => + assert.equal( + r.body.error, + 'This operation is not authorized due to role restrictions and/or invalid database items', + r.text + ) + ) + .expect((r) => + assert.equal( + r.body.unauthorized_access[0], + "Operation 'export_local' is restricted to 'super_user' roles", + r.text + ) + ) + .expect(403); }); test('Jobs Test Export To Local using NoSQL as test_user', async () => { diff --git a/integrationTests/security/choose-operation-authz.test.ts b/integrationTests/security/choose-operation-authz.test.ts new file mode 100644 index 0000000000..ec369187be --- /dev/null +++ b/integrationTests/security/choose-operation-authz.test.ts @@ -0,0 +1,504 @@ +/** + * Two invariants of operations-API authorization, driven over the real HTTP API: + * the principal is whatever authentication established, and the operation authorized is the + * operation invoked. Neither is derived from a request-body field. + * + * `chooseOperation` hands `verifyPerms` the nested `search_operation` when present, so that an + * export job's query is checked. `search_operation` is caller-supplied, so both the principal it + * carries and the operation name it declares have to be ignored for those two decisions. + * + * CONTROL — plain `add_user` is refused, proving the role really lacks the grant. + * PRINCIPAL — a nested `hdb_user` does not become the authorization subject. + * DISPATCH — a nested `operation: 'sql'` does not decide which check runs. + * DISPATCH-SU — the same, for an operation registered `requires_su`. + * SUBJECT — a nested object does not become the table subject: `search_operation: {}` on a + * read and on a write must not empty out the table checks. + * FORGED-AST — a body-supplied `parsed_sql_object` does not stand in for an authorized parse. + * POSITIVE-SQL — a direct SQL call the role is granted still works. + * POSITIVE-JOB — a super_user job with a SQL `search_operation` runs to completion. + * + * The two POSITIVE cases matter as much as the denials: the checks are additive rather than + * exclusive, so the fix has to leave the SQL and job paths working. + * + * Run: + * npm run build && npm run test:integration -- "integrationTests/security/choose-operation-authz.test.ts" + */ +import { suite, test, before, after } from 'node:test'; +import { ok, strictEqual } from 'node:assert'; +import { resolve, join } from 'node:path'; +import { mkdtempSync, readFileSync, readdirSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; + +import { setupHarperWithFixture, teardownHarper, type ContextWithHarper } from '@harperfast/integration-testing'; +// @ts-expect-error utils/client.mjs has no type declarations; runtime resolves fine +import { createApiClient, createHeaders } from '../apiTests/utils/client.mjs'; + +const FIXTURE_PATH = resolve(import.meta.dirname, 'fixtures/choose-operation-authz'); +const skipSuite = process.env.HARPER_RUNTIME === 'bun' || process.platform === 'win32'; + +const MALLORY = { username: 'authz_mallory', password: 'Mallory-pw-2173!' }; +const ROLE = 'authz_probe_role'; +// Mallory's role declares `operations`, so gate 1 refuses anything outside that list before the +// table checks run. The SUBJECT and FORGED-AST cases below are about the table checks themselves, so +// they need principals that actually reach them: TABLER declares no `operations` at all, and +// EXPORTER is granted the export operation but holds no table permission. +const TABLER = { username: 'authz_tabler', password: 'Tabler-pw-2173!' }; +const TABLER_ROLE = 'authz_tabler_role'; +const EXPORTER = { username: 'authz_exporter', password: 'Exporter-pw-2173!' }; +const EXPORTER_ROLE = 'authz_exporter_role'; +const DB = 'data'; +const TABLE = 'AuthzProbe'; +const NESTED_DELETE_TARGET = 'nested-delete-target'; + +/** Usernames each escalation attempt tries to create; none may exist afterwards. */ +const ESCALATION_TARGETS = { + principal: 'authz_escalated_principal', + dispatch: 'authz_escalated_dispatch', +}; + +/** A forged principal claiming super_user, shaped like the object authentication would attach. */ +const FORGED_SUPER_USER = { + username: 'forged', + role: { role: 'super_user', permission: { super_user: true } }, +}; + +/** + * Authorization refusals are 403. Asserting the exact status matters here: accepting 401 as well + * would let these cases pass on a credential failure, i.e. without exercising the check at all. + */ +function assertForbidden(response: { status: number; body: unknown }, context: string): void { + strictEqual( + response.status, + 403, + `${context}: expected 403, got ${response.status} ${JSON.stringify(response.body)}` + ); +} + +suite( + 'operations-API authorization uses the authenticated principal and the invoked operation', + { skip: skipSuite }, + (ctx: ContextWithHarper) => { + let client: ReturnType; + let malloryHeaders: Record; + let tablerHeaders: Record; + let exporterHeaders: Record; + // A real writable directory, so a completed export proves the job ran rather than failing on path. + let exportDir: string; + + /** True if the operations API can see a user by that name — checked as admin. */ + async function userExists(username: string): Promise { + const r = await client.req().send({ operation: 'list_users' }).expect(200); + ok(Array.isArray(r.body), `list_users did not return a list: ${JSON.stringify(r.body)}`); + return r.body.some((u: Record) => u.username === username); + } + + // Terminal states per JOB_STATUS_ENUM (utility/hdbTerms.ts): CREATED and IN_PROGRESS are not. + // Testing for the terminal set rather than against the in-flight set keeps a first poll that + // lands on CREATED from being read as a finished job. + async function waitForTerminalJob(jobId: string, timeoutMs = 30_000): Promise | undefined> { + const deadline = Date.now() + timeoutMs; + let job: Record | undefined; + while (Date.now() < deadline) { + const r = await client.req().send({ operation: 'get_job', id: jobId }).expect(200); + job = Array.isArray(r.body) ? r.body[0] : r.body; + if (job?.status === 'COMPLETE' || job?.status === 'ERROR') return job; + await new Promise((resolve) => setTimeout(resolve, 250)); + } + return job; + } + + before(async () => { + exportDir = mkdtempSync(join(tmpdir(), 'authz-export-')); + await setupHarperWithFixture(ctx, FIXTURE_PATH, { config: {}, env: {} }); + client = createApiClient(ctx.harper); + malloryHeaders = createHeaders(MALLORY.username, MALLORY.password); + tablerHeaders = createHeaders(TABLER.username, TABLER.password); + exporterHeaders = createHeaders(EXPORTER.username, EXPORTER.password); + + // Mallory is deliberately minimal: `user_info` only, and read on the probe table so the + // POSITIVE-SQL case has something legitimate to select. No user management, no SQL grant + // beyond that table, and emphatically not super_user. + await client + .req() + .send({ + operation: 'add_role', + role: ROLE, + permission: { + super_user: false, + operations: ['user_info', 'sql'], + [DB]: { + tables: { + [TABLE]: { + read: true, + insert: false, + update: false, + delete: false, + attribute_permissions: [], + }, + }, + }, + }, + }) + .expect(200); + + await client + .req() + .send({ + operation: 'add_user', + role: ROLE, + username: MALLORY.username, + password: MALLORY.password, + active: true, + }) + .expect(200); + + // No `operations` key: gate 1 is skipped, so a request from this role is decided by the table + // checks — which is what SUBJECT exercises. Read on the probe table, nothing else, and no + // grant of any kind on system tables. + await client + .req() + .send({ + operation: 'add_role', + role: TABLER_ROLE, + permission: { + super_user: false, + [DB]: { + tables: { + [TABLE]: { read: true, insert: false, update: false, delete: false, attribute_permissions: [] }, + }, + }, + }, + }) + .expect(200); + await client + .req() + .send({ + operation: 'add_user', + role: TABLER_ROLE, + username: TABLER.username, + password: TABLER.password, + active: true, + }) + .expect(200); + + // Granted the export operation and delete, but not read, SQL, or the delete API operation. + await client + .req() + .send({ + operation: 'add_role', + role: EXPORTER_ROLE, + permission: { + super_user: false, + operations: ['export_local', 'user_info', 'get_job'], + [DB]: { + tables: { + [TABLE]: { + read: false, + insert: false, + update: false, + delete: true, + attribute_permissions: [], + }, + }, + }, + }, + }) + .expect(200); + await client + .req() + .send({ + operation: 'add_user', + role: EXPORTER_ROLE, + username: EXPORTER.username, + password: EXPORTER.password, + active: true, + }) + .expect(200); + + await client + .req() + .send({ + operation: 'insert', + schema: DB, + table: TABLE, + records: [ + { id: 'probe-1', label: 'visible' }, + { id: NESTED_DELETE_TARGET, label: 'must-remain' }, + ], + }) + .expect(200); + }); + + after(async () => { + await teardownHarper(ctx); + rmSync(exportDir, { recursive: true, force: true }); + }); + + test('CONTROL — a plain add_user from a non-super_user is refused', async () => { + const r = await client.reqAs(malloryHeaders).send({ + operation: 'add_user', + role: 'super_user', + username: 'authz_control_target', + password: 'Control-pw-1!', + active: true, + }); + assertForbidden(r, 'plain add_user'); + strictEqual(await userExists('authz_control_target'), false); + }); + + test('PRINCIPAL — a forged nested search_operation.hdb_user does not authorize the request', async () => { + const username = ESCALATION_TARGETS.principal; + const r = await client.reqAs(malloryHeaders).send({ + operation: 'add_user', + role: 'super_user', + username, + password: 'Escalated-pw-1!', + active: true, + search_operation: { operation: 'noop', hdb_user: FORGED_SUPER_USER }, + }); + assertForbidden(r, 'forged nested principal'); + strictEqual(await userExists(username), false, 'forged principal must not create an account'); + }); + + // TABLER, not Mallory: Mallory's `operations` allowlist excludes both add_user and export_local, + // so gate 1 (`verifyOperationsAllowlist` on the top-level op) refuses her in both the fixed and a + // reverted dispatch — the test would pass without exercising the routing fix at all. TABLER + // declares no `operations`, so gate 1 is a no-op and the only thing standing between the request + // and a super_user account is that the nested `operation: 'sql'` must NOT route the outer op away + // from its `verifyPerms`/`requires_su` check. Reverting the scoping fix creates the account here. + test('DISPATCH — a SQL-shaped search_operation does not route add_user past verifyPerms', async () => { + const username = ESCALATION_TARGETS.dispatch; + const r = await client.reqAs(tablerHeaders).send({ + operation: 'add_user', + role: 'super_user', + username, + password: 'Escalated-pw-2!', + active: true, + search_operation: { operation: 'sql', sql: 'select 1' }, + }); + assertForbidden(r, 'SQL-shaped search_operation'); + strictEqual(await userExists(username), false, 'SQL-shaped search_operation must not create an account'); + }); + + // Also TABLER, and deliberately NOT the EXPORTER: export_local is `requires_su`, and a role that + // lists it in `operations` is granted it by gate 2 — so EXPORTER exporting `select 1` is allowed + // (200), which would not discriminate. TABLER has no allowlist, so its only barrier to a + // requires_su export is the `requires_su` gate inside `verifyPerms` — exactly what a nested + // `operation: 'sql'` must not route around. Reverting the scoping fix lets the export through. + test('DISPATCH-SU — a SQL-shaped search_operation does not route export_local past requires_su', async () => { + const r = await client.reqAs(tablerHeaders).send({ + operation: 'export_local', + path: '/tmp', + format: 'json', + search_operation: { operation: 'sql', sql: 'select 1' }, + }); + assertForbidden(r, 'SQL-shaped search_operation on a requires_su operation'); + }); + + // A read and a write, both against a table this role has no grant on. `verifyPerms` derives the + // tables it checks from the object it is handed, so an empty nested object must not become that + // object — otherwise there is nothing left to check. + test('SUBJECT — an empty search_operation does not empty out the table checks (read)', async () => { + const r = await client.reqAs(tablerHeaders).send({ + operation: 'search_by_value', + schema: 'system', + table: 'hdb_user', + search_attribute: 'username', + search_value: '*', + get_attributes: ['username', 'password'], + search_operation: {}, + }); + assertForbidden(r, 'search_by_value on system.hdb_user with an empty search_operation'); + }); + + test('SUBJECT — an empty search_operation does not empty out the table checks (write)', async () => { + const r = await client.reqAs(tablerHeaders).send({ + operation: 'insert', + schema: DB, + table: TABLE, + records: [{ id: 'subject-injected', label: 'should-not-write' }], + search_operation: {}, + }); + assertForbidden(r, 'insert with an empty search_operation'); + + const check = await client + .req() + .send({ + operation: 'search_by_value', + schema: DB, + table: TABLE, + search_attribute: 'id', + search_value: 'subject-injected', + get_attributes: ['id'], + }) + .expect(200); + strictEqual(check.body.length, 0, `denied insert still wrote a row: ${JSON.stringify(check.body)}`); + }); + + // `parsed_sql_object` carries `permissions_checked`, and the export worker re-reads it off the + // caller's own nested object, so a body-supplied one would run an AST nothing authorized. The + // forgery is neutralized rather than refused: the nested field is dropped and the honest `sql` + // is what runs, which this role is allowed to export — so the assertion is about what the job + // produced, not about the status code. + test('FORGED-AST — a body-supplied parsed_sql_object does not select the statement that runs', async () => { + const started = await client.reqAs(exporterHeaders).send({ + operation: 'export_local', + path: exportDir, + format: 'json', + search_operation: { + operation: 'sql', + sql: 'select 1 as harmless', + parsed_sql_object: { + variant: 'select', + permissions_checked: true, + ast: { + statements: [{ from: [{ databaseid: 'system', tableid: 'hdb_user' }], columns: [{ columnid: '*' }] }], + }, + }, + }, + }); + strictEqual(started.status, 200, `expected the honest statement to be accepted: ${JSON.stringify(started.body)}`); + const job = await waitForTerminalJob(started.body?.job_id); + strictEqual(job?.status, 'COMPLETE', `export job did not complete: ${JSON.stringify(job)}`); + + const exported = readdirSync(exportDir) + .map((name) => readFileSync(join(exportDir, name), 'utf8')) + .join(''); + ok(!exported.includes('password'), 'forged AST reached the export: it contains hdb_user material'); + ok( + exported.includes('harmless'), + `export did not contain the honest statement's result: ${exported.slice(0, 200)}` + ); + }); + + // EXPORTER may invoke export_local (its `operations` lists it) but is not otherwise authorized for + // the underlying read. The outer op is authorized at gate 2 without a table check, and the job + // worker runs search_by_value with no permission check of its own, so the nested search is + // authorized at dispatch against its real handler — which denies it. A 200 here means that nested + // check regressed and the gate-2 export gap reopened. + test('NESTED-NOSQL — a role granted export_local but not the underlying read cannot export', async () => { + const r = await client.reqAs(exporterHeaders).send({ + operation: 'export_local', + path: exportDir, + format: 'json', + search_operation: { + operation: 'search_by_value', + schema: DB, + table: TABLE, + search_attribute: 'id', + search_value: '*', + get_attributes: ['id'], + }, + }); + assertForbidden(r, 'export_local of a table the role cannot read'); + }); + + test('NESTED-SQL-DML — export SQL cannot execute a DELETE', async () => { + const started = await client.reqAs(exporterHeaders).send({ + operation: 'export_local', + path: exportDir, + format: 'json', + search_operation: { + operation: 'sql', + sql: `DELETE FROM ${DB}.${TABLE} WHERE id = '${NESTED_DELETE_TARGET}'`, + }, + }); + if (started.body?.job_id) await waitForTerminalJob(started.body.job_id); + + const check = await client + .req() + .send({ + operation: 'search_by_value', + schema: DB, + table: TABLE, + search_attribute: 'id', + search_value: NESTED_DELETE_TARGET, + get_attributes: ['id'], + }) + .expect(200); + strictEqual(check.body.length, 1, 'nested export SQL deleted a row'); + strictEqual(started.status, 400, `expected nested DELETE to be rejected: ${JSON.stringify(started.body)}`); + }); + + test('NESTED-SHAPE — a non-object search_operation is a 400, not a 500', async () => { + for (const shape of ['not-an-object', 42, true, [], null]) { + const r = await client + .reqAs(malloryHeaders) + .send({ operation: 'export_local', path: exportDir, format: 'json', search_operation: shape }); + strictEqual( + r.status, + 400, + `search_operation ${JSON.stringify(shape)}: expected 400, got ${r.status} ${JSON.stringify(r.body)}` + ); + } + }); + + // An object that names no supported operation ({} or a bogus op) must fail at request time, not + // dereference `search_operation.operation` in the worker or become an asynchronously-failed job. + // Shape validation runs before authorization, so an un-granted role still gets the 400. + test('NESTED-OP — an object search_operation without a supported operation is a 400', async () => { + for (const search_operation of [{}, { operation: 'not_a_real_op' }, { operation: '' }]) { + const r = await client + .reqAs(malloryHeaders) + .send({ operation: 'export_local', path: exportDir, format: 'json', search_operation }); + strictEqual( + r.status, + 400, + `search_operation ${JSON.stringify(search_operation)}: expected 400, got ${r.status} ${JSON.stringify(r.body)}` + ); + } + }); + + test('POSITIVE-SQL — a direct SQL call the role is granted still succeeds', async () => { + const r = await client + .reqAs(malloryHeaders) + .send({ operation: 'sql', sql: `SELECT id FROM ${DB}.${TABLE} WHERE id = 'probe-1'` }) + .expect(200); + ok(Array.isArray(r.body) && r.body.length === 1, `expected one row, got ${JSON.stringify(r.body)}`); + }); + + // A 200 here means only that the job was accepted. The job worker re-parses the nested SQL and + // runs the AST check again, so the export can still be refused after the API has answered — + // which is the case that matters, since the check that runs there was previously unable to + // deny at all. Poll to a terminal state. + test('POSITIVE-JOB — a super_user export_local with a SQL search_operation completes', async () => { + const started = await client.req().send({ + operation: 'export_local', + path: exportDir, + format: 'json', + search_operation: { operation: 'sql', sql: `SELECT id FROM ${DB}.${TABLE}` }, + }); + strictEqual(started.status, 200, `expected the job to start: ${JSON.stringify(started.body)}`); + const jobId = started.body?.job_id; + ok(jobId, `expected a job_id, got ${JSON.stringify(started.body)}`); + + const job = await waitForTerminalJob(jobId); + strictEqual(job?.status, 'COMPLETE', `export job did not complete: ${JSON.stringify(job)}`); + }); + + // The non-SQL counterpart to POSITIVE-JOB: the nested-read authorization added for the NESTED-NOSQL + // denial must still let a principal that CAN read the table export it. super_user passes both the + // outer export check and the nested search_by_value check, so the job runs to completion. + test('POSITIVE-NOSQL — a super_user export_local with a search_by_value search_operation completes', async () => { + const started = await client.req().send({ + operation: 'export_local', + path: exportDir, + format: 'json', + search_operation: { + operation: 'search_by_value', + schema: DB, + table: TABLE, + search_attribute: 'id', + search_value: '*', + get_attributes: ['id'], + }, + }); + strictEqual(started.status, 200, `expected the job to start: ${JSON.stringify(started.body)}`); + const jobId = started.body?.job_id; + ok(jobId, `expected a job_id, got ${JSON.stringify(started.body)}`); + + const job = await waitForTerminalJob(jobId); + strictEqual(job?.status, 'COMPLETE', `export job did not complete: ${JSON.stringify(job)}`); + }); + } +); diff --git a/integrationTests/security/fixtures/choose-operation-authz/config.yaml b/integrationTests/security/fixtures/choose-operation-authz/config.yaml new file mode 100644 index 0000000000..d131aa37e1 --- /dev/null +++ b/integrationTests/security/fixtures/choose-operation-authz/config.yaml @@ -0,0 +1,4 @@ +# Regression fixture — the authorization principal must come from authentication, never the body. +graphqlSchema: + files: '*.graphql' +rest: true diff --git a/integrationTests/security/fixtures/choose-operation-authz/schema.graphql b/integrationTests/security/fixtures/choose-operation-authz/schema.graphql new file mode 100644 index 0000000000..af440ff45e --- /dev/null +++ b/integrationTests/security/fixtures/choose-operation-authz/schema.graphql @@ -0,0 +1,8 @@ +# Regression fixture — chooseOperation must authorize the real principal against the real operation. +# +# A table is needed only so the fixture mounts a database; the escalation attempts below target +# system operations (add_user, export_local), not this table. +type AuthzProbe @table @export { + id: ID @primaryKey + label: String +} diff --git a/server/serverHelpers/serverUtilities.ts b/server/serverHelpers/serverUtilities.ts index 5b0fd92c6d..ec1ab31dd8 100644 --- a/server/serverHelpers/serverUtilities.ts +++ b/server/serverHelpers/serverUtilities.ts @@ -235,6 +235,17 @@ server.setMcpQuotaHandler = (handler) => { setMcpQuotaHandler(handler); }; +// The nested `search_operation` operations dataLayer/export.ts's getRecords can actually run (its +// VALID_SEARCH_OPERATIONS). Validated at dispatch so an unsupported or missing nested operation is a +// request-time 400 rather than an async job failure, and so the nested authorization below always +// resolves a real handler. +const EXPORT_SEARCH_OPERATIONS = new Set([ + terms.OPERATIONS_ENUM.SEARCH_BY_VALUE, + terms.OPERATIONS_ENUM.SEARCH_BY_HASH, + terms.OPERATIONS_ENUM.SEARCH_BY_CONDITIONS, + terms.OPERATIONS_ENUM.SQL, +]); + export function chooseOperation(json: OperationRequestBody, bypassAuth = false) { let getOpResult: OperationFunctionObject; try { @@ -255,32 +266,112 @@ export function chooseOperation(json: OperationRequestBody, bypassAuth = false) const { operation_function, job_operation_function } = getOpResult; - // Here there is a SQL statement in either the operation or the searchOperation (from jobs like export_local). Need to check the perms - // on all affected tables/attributes. + const isSqlOperation = json.operation === terms.OPERATIONS_ENUM.SQL; + // `verifyPerms` derives the tables it checks from the object it is handed, so the caller-supplied + // `search_operation` may only stand in as the permission subject for the operations that actually + // run it (dataLayer/export.ts is its only consumer). For anything else it would check the nested + // tables while the handler runs against the top-level ones. + const nestedSearch = + json.operation === terms.OPERATIONS_ENUM.EXPORT_LOCAL || json.operation === terms.OPERATIONS_ENUM.EXPORT_TO_S3 + ? json.search_operation + : undefined; + if (nestedSearch !== undefined) { + // It becomes the permission subject below, and a primitive would throw on the principal + // assignment — a 400 rather than a wrapped 500. + if (nestedSearch === null || typeof nestedSearch !== 'object' || Array.isArray(nestedSearch)) { + throw handleHDBError( + new Error(), + `'search_operation' must be an object`, + hdbErrors.HTTP_STATUS_CODES.BAD_REQUEST, + undefined, + undefined, + true + ); + } + // The object check alone lets `{}` and unsupported operations through — they would then fail + // asynchronously in the export worker (or dereference `search_operation.operation`). Require a + // supported operation at request time; the nested authorization below relies on it resolving. + if (typeof nestedSearch.operation !== 'string' || !EXPORT_SEARCH_OPERATIONS.has(nestedSearch.operation)) { + throw handleHDBError( + new Error(), + `'search_operation.operation' must be one of: ${[...EXPORT_SEARCH_OPERATIONS].join(', ')}`, + hdbErrors.HTTP_STATUS_CODES.BAD_REQUEST, + undefined, + undefined, + true + ); + } + // Dispatch state, never client input: the export worker re-reads `parsed_sql_object` off this + // same object (evaluateSQL), and it carries `permissions_checked`, so a body-supplied one would + // execute an AST no check ever saw. Deleting it rather than replacing it with the authorized + // parse is deliberate — the worker then re-parses and re-runs the check itself. + delete nestedSearch.parsed_sql_object; + } + // The AST check below covers only the statement's tables, never the caller's right to invoke the + // job operation, so it is additive to verifyPerms rather than an alternative to it. + const hasNestedSqlSearch = nestedSearch?.operation === terms.OPERATIONS_ENUM.SQL; + try { - if (json.operation === 'sql' || (json.search_operation && json.search_operation.operation === 'sql')) { + if (isSqlOperation || hasNestedSqlSearch) { const sql = require('../../sqlTranslator/index'); - const sqlStatement = json.operation === 'sql' ? json.sql : json.search_operation.sql; + const sqlStatement = isSqlOperation ? json.sql : nestedSearch.sql; // Before this dispatch's own parse is assigned, so a body-supplied object cannot survive it. stripSuppliedParsedSqlObject(json); const parsedSqlObject = sql.convertSQLToAST(sqlStatement); - json.parsed_sql_object = parsedSqlObject; + if (hasNestedSqlSearch && parsedSqlObject.variant !== terms.VALID_SQL_OPS_ENUM.SELECT) { + throw handleHDBError( + new Error(), + `'search_operation.sql' must be a SELECT statement`, + hdbErrors.HTTP_STATUS_CODES.BAD_REQUEST, + undefined, + undefined, + true + ); + } + // Only the direct-SQL path consumes `json.parsed_sql_object` (evaluateSQL). A nested export + // re-parses off `search_operation` — whose `parsed_sql_object` was deleted above — so setting + // it here for a job would be inert. + if (isSqlOperation) { + json.parsed_sql_object = parsedSqlObject; + } if (!bypassAuth) { - // The SQL path never reaches verifyPerms, so the role `operations` allowlist must be - // enforced here — otherwise an allowlisted role reaches unlisted operations via `sql`. - // json.operation is already the API name ('sql', or the outer job op like 'export_local'). - const allowlistDenial = opAuth.verifyOperationsAllowlist(json, json.operation); - if (allowlistDenial) { - operationLog.error(`${HTTP_STATUS_CODES.FORBIDDEN} from operation ${json.operation}`); - operationLog.warn(`User '${json.hdb_user?.username}' is not permitted to ${json.operation}`); - throw handleHDBError( - new Error(), - allowlistDenial, - hdbErrors.HTTP_STATUS_CODES.FORBIDDEN, - undefined, - undefined, - true - ); + if (hasNestedSqlSearch) { + // A nested-SQL export's outer op (export_local/export_to_s3) is `requires_su`, which the + // AST table check below does not enforce. Authorize invoking it through verifyPerms + // exactly as the non-SQL job path does — otherwise SQL is a way around the requires_su + // gate that path enforces. The nested table reads are then covered additively by the AST + // check. verifyPerms also runs the allowlist (gate 1) and the export token scope. + const functionToCheck = job_operation_function === undefined ? operation_function : job_operation_function; + const outerDenial = opAuth.verifyPerms(json, functionToCheck, { apiOperation: json.operation }); + if (outerDenial) { + operationLog.error(`${HTTP_STATUS_CODES.FORBIDDEN} from operation ${json.operation}`); + operationLog.warn(`User '${json.hdb_user?.username}' is not permitted to ${json.operation}`); + throw handleHDBError( + new Error(), + outerDenial, + hdbErrors.HTTP_STATUS_CODES.FORBIDDEN, + undefined, + false, + true + ); + } + } else { + // Direct SQL: the invoked operation IS `sql`, so the role `operations` allowlist is the + // operation-invocation check — otherwise an allowlisted role reaches unlisted operations + // via `sql`. json.operation is already the API name. + const allowlistDenial = opAuth.verifyOperationsAllowlist(json, json.operation); + if (allowlistDenial) { + operationLog.error(`${HTTP_STATUS_CODES.FORBIDDEN} from operation ${json.operation}`); + operationLog.warn(`User '${json.hdb_user?.username}' is not permitted to ${json.operation}`); + throw handleHDBError( + new Error(), + allowlistDenial, + hdbErrors.HTTP_STATUS_CODES.FORBIDDEN, + undefined, + undefined, + true + ); + } } // `json.operation` explicitly — the operation this dispatch already resolved, not a // field read back off the request body. @@ -309,10 +400,9 @@ export function chooseOperation(json: OperationRequestBody, bypassAuth = false) json.operation !== terms.OPERATIONS_ENUM.EXCHANGE_OIDC_TOKEN ) { const functionToCheck = job_operation_function === undefined ? operation_function : job_operation_function; - const operation_json = json.search_operation ? json.search_operation : json; - if (!operation_json.hdb_user) { - operation_json.hdb_user = json.hdb_user; - } + const operation_json = nestedSearch ?? json; + // Authentication is the only source of the principal; a nested one is overwritten, not honored. + operation_json.hdb_user = json.hdb_user; // Pass the top-level operation for the token-scope check: for an export job, operation_json // is the nested search_operation, so json.operation (export_local/export_to_s3) is the op the @@ -335,6 +425,35 @@ export function chooseOperation(json: OperationRequestBody, bypassAuth = false) true ); } + + // The check above authorizes invoking the operation, but for a non-SQL export it returns + // before any table check — a requires_su export is granted the moment its `operations` + // allowlist lists it (verifyPerms gate 2) — and the job worker then runs `search_operation` + // through searchByValue/Hash/Conditions, none of which check permissions. So the nested read + // is only enforceable here: authorize it additively against its real search handler and the + // authenticated principal (assigned above via operation_json), so a role that may invoke the + // export but lacks read on the table is denied. A nested SQL search is covered by the AST + // branch above instead. + if (nestedSearch) { + const nestedFunction = getOperationFunction(nestedSearch).operation_function; + const nestedPermsResult = opAuth.verifyPerms(nestedSearch, nestedFunction, { + apiOperation: json.operation, + }); + if (nestedPermsResult) { + operationLog.error(`${HTTP_STATUS_CODES.FORBIDDEN} from operation ${json.operation}`); + operationLog.warn( + `User '${operation_json.hdb_user?.username}' is not permitted to ${nestedSearch.operation} within ${json.operation}` + ); + throw handleHDBError( + new Error(), + nestedPermsResult, + hdbErrors.HTTP_STATUS_CODES.FORBIDDEN, + undefined, + false, + true + ); + } + } } } catch (err) { throw handleHDBError(err, `There was an error when trying to choose an operation path`, 500); diff --git a/unitTests/server/serverHelpers/serverUtilities.test.js b/unitTests/server/serverHelpers/serverUtilities.test.js index cb920fae57..d65e4fdd67 100644 --- a/unitTests/server/serverHelpers/serverUtilities.test.js +++ b/unitTests/server/serverHelpers/serverUtilities.test.js @@ -87,16 +87,15 @@ describe('Test serverUtilities.js module ', () => { return request; } - it('throws 403 for an export job whose nested write SQL is outside the token scope', function () { - // Scoped to the export itself but not to `delete`: a write statement additionally requires - // its matching data operation, which is what keeps `read_only` from admitting a DELETE. + it('rejects write SQL nested in an export job', function () { assert.throws( - () => serverUtilities.chooseOperation(exportJobRequest(['export_local'], 'DELETE FROM data.dog')), + () => serverUtilities.chooseOperation(exportJobRequest(undefined, 'DELETE FROM data.dog')), (error) => { - assert.strictEqual(error.statusCode ?? error.http_code, 403, 'expected a forbidden status'); + assert.strictEqual(error.statusCode ?? error.http_code, 400, 'expected a bad-request status'); + assert.strictEqual(error.http_resp_msg, "'search_operation.sql' must be a SELECT statement"); return true; }, - 'an export job must not smuggle write SQL past the scope gate' + 'an export job must not execute write SQL' ); });