diff --git a/DESIGN.md b/DESIGN.md index f30892f91d..a72113b201 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -391,6 +391,80 @@ early-returns (super_user, structure_user, system-table allowances): persisted r `super_user` with other permission keys, but inline roles can combine `structure_user` with an allowlist, and the gate ordering is what keeps unlisted schema ops unreachable. +## The dispatched API operation is carried on async context, never on the request (`server/serverHelpers/operationAuthorizationState.ts`) + +`verifyPermsAST`'s token-scope check has to be told which top-level API operation the caller +invoked, because the scope is written in that namespace (`sql`, `export_local`, ...). Two things +make that awkward: + +1. On the **direct-SQL** path, the object handed to `checkASTPermissions` _is_ the client's request + body, and this check is the only gate there (`chooseOperation`'s `sql` branch is mutually + exclusive with its `verifyPerms` call). Any field read off that object is therefore a way to + name whichever operation the caller's scope happens to allow and run arbitrary SQL under it. + `jsonMessage.operation` is safe only because dispatch already routed on that same field, so it + cannot disagree with the operation running. Never add another. +2. A **job** re-parses its SQL from the nested `search_operation` in a _different_ async context — + `executeJob` persists the request and hands off to the job runner, and `jobProcess.ts` re-enters + from the `hdb_job` record. So a store established around the originating request cannot reach + it, and the re-parse would be judged as `sql` rather than as the job's own operation. + +The carrier is therefore established **in the job worker**, by `runWithDispatchedOperation`, from +the same `request.operation` that `getOperationFunction` just resolved the handler from. That +identity is the whole basis for trusting it: the value naming the operation and the value selecting +the code cannot diverge. A new carrier must preserve that property — an added request property, a +`search_operation` field, or a persisted `parsed_sql_object` would not. + +This lives in the same `AsyncLocalStorage` as the auth bypass rather than a second store, so +`processAST` reads the state once. `runWithOperationAuthorizationBypass` **preserves** an existing +carrier on both branches. That is deliberate and was initially got wrong: its enforced branch is not +a bypass, so a job handler dispatching a nested _authorized_ operation lands there, and dropping the +carrier would judge that job's re-parsed SQL as the inner `sql` and refuse it partway through its own +work. The consequence to know is the other direction — a nested dispatch inside a job is judged +against the **outer** job's operation for any `evaluateSQL` that does not pass through +`chooseOperation`. It allocates only when a carrier is present; with none, two shared frozen objects +serve the common path. All four stores are frozen, so `getOperationAuthorizationState()` cannot hand +a mutable one to a caller. + +It has four call sites, and they are not all dispatch wrappers: `server.operation()` +(`serverUtilities.ts`), the ITC path (`registeredOperations.ts`), the legacy SQL engine +(`sqlEngine/diff/differential.ts`), and Harper's own `hdb_job` query (`server/jobs/jobs.ts`) — that +last one **is** reached from the ops-API dispatch, via `search_jobs_by_start_date` → +`handleGetJobsByStartDate` → `getJobsInDateRange`. + +Harper's own internal SQL takes the bypass, not the carrier. `getJobsInDateRange` runs a fixed +`system.hdb_job` query through `evaluateSQL` beneath a handler the caller was already authorized for, +and `SqlSearchObject` hardcodes `operation: 'sql'` — so the same mismatch applies, but the answer +differs, and the reason is easy to get backwards. `verifyPermsAST`'s super_user early return is +`isSuperUser && !isSuSystemOperation`, so a `system` schema is **exempt** from it and the table check +genuinely runs. A carrier would therefore put Harper's own query through `hasPermissions` on +`system.hdb_job`, which passes only because `appendSystemTablesToRole` grants `system.*.read` to a +hydrated super_user — a super_user principal without an appended `permission.system` (an +impersonation payload, or any path that skips user-cache hydration) would start getting 403s on an +operation it is entitled to. The bypass also states the actual intent: the statement is Harper's, not +the caller's. Wrap the individual statement, not the function — a later caller-dependent statement +must not inherit it. + +A second body field has to be neutralized for any of this to hold: `evaluateSQL` trusts a supplied +`parsed_sql_object` verbatim and skips parsing, `chooseOperation` overwrites only the **top-level** +one, and `dataLayer/export.ts` hands the nested `search_operation` straight to `evaluateSQL`. So a +body-supplied `search_operation.parsed_sql_object` carrying `permissions_checked: true` would run an +arbitrary AST with the check skipped. `chooseOperation` deletes it, forcing the worker to re-parse +from the `sql` string that dispatch authorized — the nested object is never overwritten the way the +top-level one is, because nothing downstream should read one at all. + +What is untestable is not the carrier's contract — unit tests cover that by calling +`runWithDispatchedOperation` directly — but that `jobProcess` is what establishes it. Delete that call +and those tests stay green. The carrier only changes an outcome through `tokenScopeDenial`, which is +inert unless the principal carries `tokenOperations`, and that property has exactly one origin: an +OIDC trust-policy exchange, for which there is no integration harness. + +Three different mechanisms are easy to conflate here. `tokenOperations` above is the **OIDC token +operation scope** (#2174). An **inline-role scoped token** (`create_authentication_tokens` with a +`role` object) is not the same thing and cannot substitute, because `createScopedToken` mints it +`super_user: false`, so it cannot invoke a `requires_su` operation such as `export_local` at all. +**Table permissions** are a third, and also cannot substitute — see the system-schema exemption +above. See #2298. + ## TLS hot-reload: cert vs. private key follow two different propagation paths (`security/keys.ts`) A renewed **certificate** and a renewed **private key** reach a worker's live TLS secure context diff --git a/server/jobs/jobProcess.ts b/server/jobs/jobProcess.ts index 9688a3c32b..8fa7693e87 100644 --- a/server/jobs/jobProcess.ts +++ b/server/jobs/jobProcess.ts @@ -9,6 +9,8 @@ import harperLogger from '../../utility/logging/harper_logger.ts'; import * as globalSchema from '../../utility/globalSchema.ts'; import * as user from '../../security/user.ts'; import * as serverUtils from '../serverHelpers/serverUtilities.ts'; +import { runWithDispatchedOperation } from '../serverHelpers/operationAuthorizationState.ts'; +import { stripSuppliedParsedSqlObject } from '../serverHelpers/requestSanitization.ts'; import moment from 'moment'; import * as jobs from './jobs.ts'; import { cloneDeep } from 'lodash'; @@ -54,12 +56,16 @@ const JOB_ID = JOB_NAME.substring(4); throw new Error('Did not find job request in hdb_job table, unable to proceed'); } request = cloneDeep(request); + // The worker re-enters from the persisted row rather than re-dispatching, so a row queued before + // the dispatch-time strip existed — or written directly — is sanitized here. + stripSuppliedParsedSqlObject(request); const operation = serverUtils.getOperationFunction(request); harperLogger.trace('Running operation:', request.operation, 'for job', JOB_ID); - // Run the job operation. - const results = await operation.job_operation_function(request); + const results = await runWithDispatchedOperation(request.operation, () => + operation.job_operation_function(request) + ); harperLogger.trace('Result from job:', JOB_ID, results); jobObj.status = hdbTerms.JOB_STATUS_ENUM.COMPLETE; diff --git a/server/jobs/jobs.ts b/server/jobs/jobs.ts index 69ae1063a0..4d14c74025 100644 --- a/server/jobs/jobs.ts +++ b/server/jobs/jobs.ts @@ -11,6 +11,7 @@ import * as search from '../../dataLayer/search.ts'; import Search_Object from '../../dataLayer/SearchObject.ts'; import searchByHashObj from '../../dataLayer/SearchByHashObject.ts'; import SQL_Search_Object from '../../dataLayer/SqlSearchObject.ts'; +import { runWithOperationAuthorizationBypass } from '../serverHelpers/operationAuthorizationState.ts'; import * as hdbTerms from '../../utility/hdbTerms.ts'; import JobObject from './JobObject.ts'; import UpdateObject from '../../dataLayer/UpdateObject.ts'; @@ -249,7 +250,9 @@ export async function getJobsInDateRange(jsonBody: any) { const hdbSql = require('../../sqlTranslator/index'); pSqlEvaluate = promisify(hdbSql.evaluateSQL); } - return await pSqlEvaluate(sqlSearchObj); + // Harper's own statement, not the caller's. A carrier is not equivalent here — see DESIGN.md. + // Wraps only this statement: a later caller-dependent one must not inherit the bypass. + return await runWithOperationAuthorizationBypass(true, () => pSqlEvaluate(sqlSearchObj)); } catch (e) { log.error( `there was a problem searching for jobs from date ${jsonBody.from_date} to date ${jsonBody.to_date} ${e}` diff --git a/server/serverHelpers/operationAuthorizationState.ts b/server/serverHelpers/operationAuthorizationState.ts index 5ce0794667..ae3691ed40 100644 --- a/server/serverHelpers/operationAuthorizationState.ts +++ b/server/serverHelpers/operationAuthorizationState.ts @@ -1,11 +1,42 @@ import { AsyncLocalStorage } from 'node:async_hooks'; -const operationAuthorizationState = new AsyncLocalStorage(); +interface OperationAuthorizationState { + bypassAuth: boolean; + apiOperation?: string; +} + +const operationAuthorizationState = new AsyncLocalStorage(); + +// Shared, for the common case of no carrier to preserve. +const BYPASSED: OperationAuthorizationState = Object.freeze({ bypassAuth: true }); +const ENFORCED: OperationAuthorizationState = Object.freeze({ bypassAuth: false }); export function runWithOperationAuthorizationBypass(bypassAuth: boolean, callback: () => T): T { - return operationAuthorizationState.run(bypassAuth === true, callback); + // An existing carrier survives: the enforced branch is not a bypass, and a job handler dispatching + // a nested authorized operation must not lose its own operation identity. + const apiOperation = operationAuthorizationState.getStore()?.apiOperation; + if (apiOperation === undefined) { + return operationAuthorizationState.run(bypassAuth === true ? BYPASSED : ENFORCED, callback); + } + return operationAuthorizationState.run(Object.freeze({ bypassAuth: bypassAuth === true, apiOperation }), callback); +} + +/** + * `apiOperation` MUST come from the same value that selected the code now running, so it cannot name + * an operation other than the one executing. That is the entire basis for trusting it: a request + * property would be forgeable, and on the direct-SQL path this check is the only gate. + */ +export function runWithDispatchedOperation(apiOperation: string, callback: () => T): T { + return operationAuthorizationState.run( + Object.freeze({ bypassAuth: operationAuthorizationState.getStore()?.bypassAuth === true, apiOperation }), + callback + ); +} + +export function getOperationAuthorizationState(): OperationAuthorizationState | undefined { + return operationAuthorizationState.getStore(); } export function isOperationAuthorizationBypassed(): boolean { - return operationAuthorizationState.getStore() === true; + return operationAuthorizationState.getStore()?.bypassAuth === true; } diff --git a/server/serverHelpers/requestSanitization.ts b/server/serverHelpers/requestSanitization.ts new file mode 100644 index 0000000000..1d33344c7e --- /dev/null +++ b/server/serverHelpers/requestSanitization.ts @@ -0,0 +1,15 @@ +/** + * `evaluateSQL` honors a supplied `parsed_sql_object` verbatim and skips parsing, so one carrying + * `permissions_checked: true` executes an AST that no authorization check ever saw. Neither position + * is ever legitimately client-supplied: dispatch parses the statement itself, and the job worker + * re-parses from the `sql` string dispatch authorized. + * + * Both positions, because the two entry points differ: `chooseOperation` overwrites the top-level + * object with its own parse, but only inside its SQL branch, so a non-SQL job (`export_local` with a + * `search_by_value` search) can still carry a client-supplied one through to the persisted row. + */ +export function stripSuppliedParsedSqlObject(request: any): void { + if (!request) return; + delete request.parsed_sql_object; + delete request.search_operation?.parsed_sql_object; +} diff --git a/server/serverHelpers/serverUtilities.ts b/server/serverHelpers/serverUtilities.ts index 8b4c95d9cf..8af70667a7 100644 --- a/server/serverHelpers/serverUtilities.ts +++ b/server/serverHelpers/serverUtilities.ts @@ -51,6 +51,7 @@ import { setLocalOperationDispatch, } from './registeredOperations.ts'; import { runWithOperationAuthorizationBypass } from './operationAuthorizationState.ts'; +import { stripSuppliedParsedSqlObject } from './requestSanitization.ts'; const pSearchSearch = util.promisify(search.search); let pEvaluateSql: (sql: string) => Promise; @@ -247,14 +248,10 @@ export function chooseOperation(json: OperationRequestBody, bypassAuth = false) if (json.operation === 'sql' || (json.search_operation && json.search_operation.operation === 'sql')) { const sql = require('../../sqlTranslator/index'); const sqlStatement = json.operation === 'sql' ? json.sql : json.search_operation.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; - // NOTE: a job's SQL is re-parsed from its nested search_operation when the job runs, so the - // check there sees `sql` rather than the job's own operation. Carrying the real one on the - // request was tried and reverted: on the direct-SQL path the request is the client's body, - // so any property consulted by that check is forgeable, and it is the only gate on that - // path. This changes no outcome today — the branch that would act on the denial is dead - // (#2202) — but #2202 needs an unforgeable carrier before making it live. 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`. diff --git a/sqlTranslator/index.ts b/sqlTranslator/index.ts index 31efd12573..cdd44b2b31 100644 --- a/sqlTranslator/index.ts +++ b/sqlTranslator/index.ts @@ -17,7 +17,7 @@ import * as terms from '../utility/hdbTerms.ts'; import { handleHDBError } from '../utility/errors/hdbError.ts'; import { HTTP_STATUS_CODES } from '../utility/errors/commonErrors.ts'; import * as sqlEngineRouter from '../sqlEngine/router.ts'; -import { isOperationAuthorizationBypassed } from '../server/serverHelpers/operationAuthorizationState.ts'; +import { getOperationAuthorizationState } from '../server/serverHelpers/operationAuthorizationState.ts'; //here we call to define and import custom functions to alasql alasqlFunctionImporter(alasql); @@ -90,20 +90,20 @@ export function checkASTPermissions(jsonMessage: any, parsedSqlObject: any, apiO // a caller to name whichever operation their token scope happens to allow and run arbitrary // SQL under it. // - // That rules out carrying the job's real operation on the request too. A job re-parses from - // its nested search_operation, so this sees `sql` rather than `export_local` there — which - // today changes nothing, because the branch in processAST that would act on the denial is - // dead (see #2202). When #2202 makes it live, the job's operation needs a carrier that a - // client cannot forge; a request property is not one, however carefully it is stripped. + // A job's re-parse would otherwise land here with the nested search_operation, whose + // `operation` is the inner `sql`; the job worker supplies the real one out of async context + // instead (operationAuthorizationState), which a request cannot set. apiOperation ?? jsonMessage.operation ); - parsedSqlObject.permissions_checked = true; } catch (e) { throw e; } if (verifyResult) { return verifyResult; } + // Only after a pass: this flag is what processAST trusts to skip the gate, so a denied AST + // carrying it would read as already authorized. + parsedSqlObject.permissions_checked = true; return null; } @@ -150,15 +150,17 @@ export function processAST(jsonMessage: any, parsedSqlObject: any, callback: any // runWithOperationAuthorizationBypass), never body state — jsonMessage.bypass_auth is // caller-controlled and is stripped before operation code sees it (see // server/serverHelpers/serverHandlers.js and components/mcp/tools/operations.ts). - if (!isOperationAuthorizationBypassed() && !parsedSqlObject.permissions_checked) { - let permissionsCheck = checkASTPermissions(jsonMessage, parsedSqlObject); - // NOTE: this guard is dead — PermissionResponseObject has no `length`, so `undefined > 0` - // discards a denial that was computed correctly. Pre-existing and not specific to this - // feature, so it is fixed separately in #2202 rather than bundled here. This PR does not - // depend on it: the outer gate in serverUtilities refuses an out-of-scope job operation, - // and sqlWriteScopeDenial refuses write SQL, both through correct truthiness tests. - if (permissionsCheck && permissionsCheck.length > 0) { - return callback(UNAUTHORIZED_RESPONSE, permissionsCheck); + if (!parsedSqlObject.permissions_checked) { + const authorizationState = getOperationAuthorizationState(); + if (authorizationState?.bypassAuth !== true) { + let permissionsCheck = checkASTPermissions(jsonMessage, parsedSqlObject, authorizationState?.apiOperation); + // A denial is a PermissionResponseObject, which has no `length`. + if (permissionsCheck) { + // Logged here because this is the last place the reason exists: evaluateSQL drops the + // second callback argument, so a job worker records only the bare status. + logger.warn('SQL statement refused by AST permission check:', permissionsCheck); + return callback(UNAUTHORIZED_RESPONSE, permissionsCheck); + } } } diff --git a/unitTests/security/tokenOperationScope.test.js b/unitTests/security/tokenOperationScope.test.js index 723ec55125..176a4fce21 100644 --- a/unitTests/security/tokenOperationScope.test.js +++ b/unitTests/security/tokenOperationScope.test.js @@ -9,13 +9,14 @@ testUtils.preTestPrep(); const opAuth = require('#src/utility/operation_authorization'); const sql = require('#src/sqlTranslator/index'); +const { runWithDispatchedOperation } = require('#src/server/serverHelpers/operationAuthorizationState'); // `insertData` is the internal function name for the `insert` operation; verifyPerms resolves the // api_name via the permission registry, which is what a scope is written against. const INSERT_FN = 'insertData'; function requestAs(permission, tokenOperations) { - const hdb_user = { username: 'ci-deploy', role: { role: 'r', permission } }; + const hdb_user = { username: 'ci-deploy', role: { role: '_tokenScope_test', permission } }; if (tokenOperations !== undefined) hdb_user.tokenOperations = tokenOperations; return { operation: 'insert', schema: 'data', table: 'dog', hdb_user, records: [] }; } @@ -143,7 +144,7 @@ describe('token scope gates on the API operation, not the handler name', () => { describe('token-scoped narrowing on the SQL path', () => { function userWithScope(permission, tokenOperations) { - const user = { username: 'ci-deploy', role: { role: 'r', permission } }; + const user = { username: 'ci-deploy', role: { role: '_tokenScope_test', permission } }; if (tokenOperations !== undefined) user.tokenOperations = tokenOperations; return user; } @@ -153,6 +154,30 @@ describe('token-scoped narrowing on the SQL path', () => { return sql.checkASTPermissions({ operation, sql: statement, hdb_user: user }, parsed); } + /** Exactly what export.ts hands the SQL handler: the nested search_operation, no parsed_sql_object. */ + function runNestedExportSql(tokenOperations) { + return new Promise((resolve) => { + sql.evaluateSQL( + { + operation: 'sql', + sql: 'SELECT * FROM data.dog', + hdb_user: userWithScope({ super_user: true }, tokenOperations), + }, + (error) => resolve({ error }) + ); + }); + } + + /** + * The permission gate is the only path that calls back with a bare numeric status; every other + * failure forwards an Error. Identifying the refusal by shape rather than by the 403 literal keeps + * this from depending on a constant the file under test owns — and lets the admitted case assert + * it got past the gate even though the table does not exist in this context. + */ + function refusedByPermissionGate(outcome) { + return typeof outcome.error === 'number'; + } + it('denies SQL when the scope does not include it', () => { const denial = checkSql('SELECT * FROM data.dog', userWithScope({ super_user: true }, ['get_status'])); assert.ok(denial, 'a token scoped away from sql must not be able to run SQL'); @@ -241,51 +266,30 @@ describe('token-scoped narrowing on the SQL path', () => { assert.strictEqual(denial, null); }); - // TRIPWIRE for the #2202 split — this test exists to go red, not to describe desired behavior. - // - // An export job runs by re-parsing its nested search_operation, so the check inside job execution - // sees `operation: 'sql'` rather than `export_local`. Carrying the real operation on the request - // was reverted here because that object is client-supplied on the direct-SQL path, making any - // property it consults forgeable. That costs nothing TODAY only because the branch in processAST - // that would act on the denial is dead (its guard tests `.length` on an object that has none). - // - // #2202 makes that branch live. The moment it does, this admits-an-in-scope-export assertion - // fails, because the job's own SQL would be judged as `sql` against a scope naming only - // `export_local`. The two PRs can merge in either order, so whichever lands second turns CI red - // here instead of silently shipping an export-scoped token that 403s on its own export. - // - // If you are reading this because it just went red: the fix is not to relax the scope check. It is - // to give the job's real operation a carrier a client cannot set, then assert it here. + // These call runWithDispatchedOperation directly, so they pin the carrier's contract but NOT that + // jobProcess establishes it — see #2298 for why that is not reachable from a test. it('admits an in-scope export job through the path export.ts actually dispatches', async () => { - const outcome = await new Promise((resolve) => { - sql.evaluateSQL( - { - // Exactly what export.ts hands the SQL handler: the nested search_operation, with - // hdb_user attached and no parsed_sql_object, so it re-parses and re-checks. - operation: 'sql', - sql: 'SELECT * FROM data.dog', - hdb_user: userWithScope({ super_user: true }, ['export_local']), - }, - (error) => resolve({ error }) - ); - }); + const outcome = await runWithDispatchedOperation('export_local', () => runNestedExportSql(['export_local'])); - // Not "no error" — the table does not exist in this unit context, so it fails downstream. The - // assertion is specifically that it was not refused by the PERMISSION gate, identified by - // shape rather than by value: that path is the only one that calls back with a bare numeric - // status (`UNAUTHORIZED_RESPONSE`), while every other failure forwards an Error. evaluateSQL - // drops the second callback argument on error, so the denial object itself never arrives here - // — the number is the whole signal. - // - // Deliberately not `!== 403`: that literal lives in the file this test watches, so changing it - // would leave the tripwire green while the refusal it exists to catch still happened. A - // tripwire must not depend on a constant its own target owns. + // Not "no error": the table does not exist in this context, so it fails downstream instead. assert.ok( - typeof outcome.error !== 'number', - `an export_local-scoped token must not be denied by its own export job (see #2202); got status ${outcome.error}` + !refusedByPermissionGate(outcome), + `an export_local-scoped token must not be denied by its own export job; got ${outcome.error}` ); }); + it('denies a nested export job whose scope names only `sql`', async () => { + const outcome = await runWithDispatchedOperation('export_local', () => runNestedExportSql(['sql'])); + + assert.ok(refusedByPermissionGate(outcome), 'a `sql`-only scope must not start an export job'); + }); + + it('judges the inner `sql` when no dispatched operation was established', async () => { + const outcome = await runNestedExportSql(['export_local']); + + assert.ok(refusedByPermissionGate(outcome), 'absent a carrier the scope gate must fail closed'); + }); + it('gates a nested-SQL export job on the export operation, not on `sql`', () => { // export_local carries its query as SQL, but the scope names the job, not `sql`. A token scoped // only to `sql` must not be able to start an export it was never granted. diff --git a/unitTests/server/serverHelpers/requestSanitization.test.js b/unitTests/server/serverHelpers/requestSanitization.test.js new file mode 100644 index 0000000000..04af21db10 --- /dev/null +++ b/unitTests/server/serverHelpers/requestSanitization.test.js @@ -0,0 +1,71 @@ +'use strict'; + +// `evaluateSQL` honors a supplied `parsed_sql_object` verbatim and skips parsing, so one carrying +// `permissions_checked: true` would execute an AST no authorization check ever saw. Both the +// dispatch (`chooseOperation`) and the job worker (`jobProcess`) strip it through this function; +// jobProcess itself is a worker IIFE keyed off `process.env` and cannot be imported, which is why +// the invariant lives here instead of inline at each call site. + +const assert = require('assert'); +const { stripSuppliedParsedSqlObject } = require('#src/server/serverHelpers/requestSanitization'); + +const forgedAst = () => ({ variant: 'select', permissions_checked: true, ast: { statements: [{ forged: true }] } }); + +describe('stripSuppliedParsedSqlObject', () => { + it('removes a top-level parsed_sql_object', () => { + const request = { operation: 'export_local', parsed_sql_object: forgedAst() }; + + stripSuppliedParsedSqlObject(request); + + assert.strictEqual(request.parsed_sql_object, undefined); + }); + + it('removes one nested on search_operation', () => { + const request = { + operation: 'export_local', + search_operation: { operation: 'sql', sql: 'SELECT * FROM data.dog', parsed_sql_object: forgedAst() }, + }; + + stripSuppliedParsedSqlObject(request); + + assert.strictEqual(request.search_operation.parsed_sql_object, undefined); + }); + + // Both positions in one call: chooseOperation overwrites the top-level object with its own parse, + // but only inside its SQL branch, so a non-SQL job can carry one through to the persisted row. + it('removes both positions at once', () => { + const request = { + operation: 'export_local', + parsed_sql_object: forgedAst(), + search_operation: { operation: 'search_by_value', parsed_sql_object: forgedAst() }, + }; + + stripSuppliedParsedSqlObject(request); + + assert.strictEqual(request.parsed_sql_object, undefined); + assert.strictEqual(request.search_operation.parsed_sql_object, undefined); + }); + + // The authorized statement is what must survive, since the worker re-parses from it. + it('leaves the rest of the request intact', () => { + const request = { + operation: 'export_local', + path: './', + search_operation: { operation: 'sql', sql: 'SELECT * FROM data.dog', parsed_sql_object: forgedAst() }, + }; + + stripSuppliedParsedSqlObject(request); + + assert.strictEqual(request.search_operation.sql, 'SELECT * FROM data.dog'); + assert.strictEqual(request.operation, 'export_local'); + assert.strictEqual(request.path, './'); + }); + + it('tolerates a request with neither position, and a missing request', () => { + const request = { operation: 'export_local', search_operation: { operation: 'search_by_value' } }; + + assert.doesNotThrow(() => stripSuppliedParsedSqlObject(request)); + assert.doesNotThrow(() => stripSuppliedParsedSqlObject(undefined)); + assert.strictEqual(request.operation, 'export_local'); + }); +}); diff --git a/unitTests/server/serverHelpers/serverUtilities.test.js b/unitTests/server/serverHelpers/serverUtilities.test.js index 5a6a4b4007..4ac36036aa 100644 --- a/unitTests/server/serverHelpers/serverUtilities.test.js +++ b/unitTests/server/serverHelpers/serverUtilities.test.js @@ -76,10 +76,8 @@ describe('Test serverUtilities.js module ', () => { // The token scope's "can only ever subtract" invariant, asserted where it is ENFORCED rather // than where it is computed (#2171/#2174). This is the front door for an export job carrying - // nested SQL: the job is gated here, before it is ever queued, and the check inside the job's - // own SQL execution is a dead branch (#2202). So this gate is the whole safety argument, and - // the rest of the scope suite only asserts that a denial object comes back — not that anyone - // throws on it. + // nested SQL: the job is gated here, before it is ever queued, and again inside the worker + // when it re-parses (#2202). function exportJobRequest(tokenOperations, sql) { const request = testUtils.deepClone(TEST_JSON_SUPER_USER); request.operation = 'export_local'; @@ -111,6 +109,27 @@ describe('Test serverUtilities.js module ', () => { serverUtilities.chooseOperation(exportJobRequest(['export_local'], 'SELECT * FROM data.dog')) ); }); + + // evaluateSQL trusts a supplied parsed_sql_object verbatim, and export.ts hands the nested + // search_operation straight to it — so a body-supplied one carrying permissions_checked would + // execute an AST the worker never checked. The authorized `sql` string is what must survive. + it('discards a body-supplied parsed_sql_object on the nested search_operation', function () { + const request = exportJobRequest(['export_local'], 'SELECT * FROM data.dog'); + request.search_operation.parsed_sql_object = { + variant: 'select', + permissions_checked: true, + ast: { statements: [{ forged: true }] }, + }; + + serverUtilities.chooseOperation(request); + + assert.strictEqual( + request.search_operation.parsed_sql_object, + undefined, + 'a forged nested parsed_sql_object must not reach the job worker' + ); + assert.strictEqual(request.search_operation.sql, 'SELECT * FROM data.dog'); + }); }); describe('registered operation authorization envelope', function () { @@ -235,6 +254,40 @@ describe('Test serverUtilities.js module ', () => { }); assert.strictEqual(operationAuthorizationState.isOperationAuthorizationBypassed(), false); }); + + it('scopes the dispatched operation across awaits and concurrent runs', async function () { + const { runWithDispatchedOperation, getOperationAuthorizationState } = operationAuthorizationState; + const operationDuring = async (apiOperation) => + runWithDispatchedOperation(apiOperation, async () => { + await Promise.resolve(); + return getOperationAuthorizationState()?.apiOperation; + }); + + assert.deepStrictEqual(await Promise.all([operationDuring('export_local'), operationDuring('export_to_s3')]), [ + 'export_local', + 'export_to_s3', + ]); + assert.strictEqual(getOperationAuthorizationState()?.apiOperation, undefined); + }); + + it('keeps the dispatched operation across a nested enforced dispatch', async function () { + const { runWithDispatchedOperation, runWithOperationAuthorizationBypass, getOperationAuthorizationState } = + operationAuthorizationState; + + // The enforced branch is not a bypass: a job handler dispatching a nested authorized + // operation must not lose the carrier, or its re-parsed SQL is judged as the inner `sql`. + const enforced = await runWithDispatchedOperation('export_local', () => + runWithOperationAuthorizationBypass(false, () => getOperationAuthorizationState()) + ); + assert.strictEqual(enforced.bypassAuth, false); + assert.strictEqual(enforced.apiOperation, 'export_local'); + + const bypassed = await runWithDispatchedOperation('export_local', () => + runWithOperationAuthorizationBypass(true, () => getOperationAuthorizationState()) + ); + assert.strictEqual(bypassed.bypassAuth, true); + assert.strictEqual(bypassed.apiOperation, 'export_local'); + }); }); describe('test getOperationFunction', () => { diff --git a/unitTests/sqlTranslator/processAST.test.js b/unitTests/sqlTranslator/processAST.test.js index 667f93fc09..25fe3b3f10 100644 --- a/unitTests/sqlTranslator/processAST.test.js +++ b/unitTests/sqlTranslator/processAST.test.js @@ -2,12 +2,20 @@ const assert = require('assert'); const sinon = require('sinon'); +const { promisify } = require('node:util'); const sandbox = sinon.createSandbox(); const sqlTranslator = require('#src/sqlTranslator/index'); const opAuth = require('#src/utility/operation_authorization'); const sqlEngineRouter = require('#src/sqlEngine/router'); const operationAuthorizationState = require('#src/server/serverHelpers/operationAuthorizationState'); +const PermissionResponseObject = require('#src/security/data_objects/PermissionResponseObject').default; + +// verifyPermsAST denies with a PermissionResponseObject. Stubbing an array would also satisfy a +// guard that tests `.length`, so these stubs have to use the real shape to pin the guard at all. +function denial() { + return new PermissionResponseObject().handleUnauthorizedItem('denied'); +} describe('sqlTranslator processAST authorization bypass state (GHSA-7h8h-wq7f-qx65)', function () { afterEach(function () { @@ -15,8 +23,8 @@ describe('sqlTranslator processAST authorization bypass state (GHSA-7h8h-wq7f-qx }); it('ignores a caller-supplied jsonMessage.bypass_auth and still enforces AST permissions', function (done) { - sandbox.stub(opAuth, 'verifyPermsAST').returns(['denied']); - const routeStub = sandbox.stub(sqlEngineRouter, 'route'); + sandbox.stub(opAuth, 'verifyPermsAST').returns(denial()); + const routeStub = sandbox.stub(sqlEngineRouter, 'route').callsFake((_options, cb) => cb(null, [])); sqlTranslator.evaluateSQL( { sql: 'SELECT * FROM dev.dog', hdb_user: { username: 'nobody' }, bypass_auth: true }, @@ -33,7 +41,7 @@ describe('sqlTranslator processAST authorization bypass state (GHSA-7h8h-wq7f-qx }); it('honors the trusted dispatch-context bypass regardless of body state', async function () { - sandbox.stub(opAuth, 'verifyPermsAST').returns(['denied']); + sandbox.stub(opAuth, 'verifyPermsAST').returns(denial()); const routeStub = sandbox.stub(sqlEngineRouter, 'route').callsFake((_opts, cb) => cb(null, [])); // Wrap evaluateSQL's callback in a promise and await the run() call (rather than calling @@ -55,3 +63,83 @@ describe('sqlTranslator processAST authorization bypass state (GHSA-7h8h-wq7f-qx assert.strictEqual(routeStub.called, true); }); }); + +describe('sqlTranslator processAST permission denial', function () { + // Real roles rather than a stubbed verifyPermsAST: a stub asserting the denial was *computed* is + // how a dead consumer went unnoticed in the first place. A role with no table permissions cannot + // read dev.dog, so verifyPermsAST returns a real PermissionResponseObject. + function userWithRole(permission) { + return { username: 'restricted', role: { role: '_processAST_test', permission } }; + } + + /** Resolved rather than asserted inside: processAST wraps its body in try/catch, so an assertion + * thrown in the callback would be swallowed and re-reported as a second invocation. */ + function runProcessAST(jsonMessage, mutateParsed) { + const parsedSqlObject = sqlTranslator.convertSQLToAST(jsonMessage.sql); + if (mutateParsed) mutateParsed(parsedSqlObject); + return new Promise((resolve) => { + sqlTranslator.processAST(jsonMessage, parsedSqlObject, (error, results) => resolve({ error, results })); + }); + } + + it('refuses a statement the permission check denied', async function () { + const { error, results } = await runProcessAST({ + operation: 'sql', + sql: 'SELECT * FROM dev.dog', + hdb_user: userWithRole({ super_user: false }), + }); + + assert.strictEqual(error, 403, 'a denied statement must come back unauthorized'); + assert.ok(results?.unauthorized_access, 'expected the permission response, not a result set'); + }); + + // The response object is what a caller renders, so processAST has to hand it back rather than + // collapse it into the status. evaluateSQL is the layer that drops it (see the promisify case). + it('forwards the response object rather than collapsing it into the status', async function () { + const { results } = await runProcessAST({ + operation: 'sql', + sql: 'DELETE FROM dev.dog', + hdb_user: userWithRole({ super_user: false }), + }); + + assert.ok(results.error, 'expected the response object to carry its error message'); + }); + + // The other direction: this must not begin refusing what was always permitted. verifyPermsAST + // returns null for a super_user, so the branch falls through as before. + it('does not interfere when the permission check allows the statement', async function () { + const { error } = await runProcessAST({ + operation: 'sql', + sql: 'SELECT * FROM dev.dog', + hdb_user: userWithRole({ super_user: true }), + }); + + assert.notStrictEqual(error, 403, 'an authorized statement must not be refused'); + }); + + // promisify turns the denial into a rejection whose reason is the bare number, because evaluateSQL + // drops its second callback argument on error. A job worker records that as its failure message, + // so the status is all an operator sees. Preserved rather than changed: evaluateSQL's error + // contract is shared with every other caller. + it('rejects with the bare status through promisified evaluateSQL', async function () { + await assert.rejects( + promisify(sqlTranslator.evaluateSQL)({ + operation: 'sql', + sql: 'SELECT * FROM dev.dog', + hdb_user: userWithRole({ super_user: false }), + }), + (reason) => reason === 403 + ); + }); + + it('skips the check when an earlier gate already verified the statement', async function () { + const { error } = await runProcessAST( + { operation: 'sql', sql: 'SELECT * FROM dev.dog', hdb_user: userWithRole({ super_user: false }) }, + (parsed) => { + parsed.permissions_checked = true; + } + ); + + assert.notStrictEqual(error, 403, 'a pre-checked statement must not be re-denied here'); + }); +});