The invariant, and why it currently rests on an allowlist
sqlTranslator/index.ts:40 is:
let parsedSql = jsonMessage.parsed_sql_object;
if (!parsedSql) { parsedSql = convertSQLToAST(jsonMessage.sql); ... }A supplied parsed_sql_object is honored verbatim and skips parsing. Since #2202 made processAST's permission denial live, that object's permissions_checked flag is load-bearing: an AST arriving with permissions_checked: true executes with no authorization check.
#2202 closed every path that exists today by deleting the field at the points a client-supplied object can reach:
server/serverHelpers/serverUtilities.ts (chooseOperation) — deletes json.search_operation.parsed_sql_object at dispatch. Only the nested one, because the top-level is overwritten by that dispatch's own parse — though only inside its SQL branch, which is why the worker needs both.server/jobs/jobProcess.ts — deletes both positions when the worker loads the persisted hdb_job row, since it re-enters from storage rather than re-dispatching.
That is sound for the current call graph — evaluateSQL:40 is the only consumer, and the only client-reachable objects handed to it are the top-level request and search_operation. But it is a per-field allowlist, not an invariant. The next evaluateSQL caller that forwards a client-controlled object defeats all three deletes, and nothing fails: no test covers it, and the failure is a silent authorization skip rather than an error.
Proposed fix
Honor a supplied parsed_sql_object only when a trusted dispatch marker is present in async context — mirroring runWithDispatchedOperation in server/serverHelpers/operationAuthorizationState.ts, which #2202 introduced for exactly this class of problem (a value a request cannot set). Otherwise re-parse.
This keeps the dispatch-time parse reuse that the alternative — always re-parsing — would cost on every SQL call, and moves the invariant into the function that owns it.
Alternatives considered
- Always re-parse, ignoring any supplied AST. Simplest and strictly safe, but discards the parse
chooseOperation already did on every SQL request; wants a hot-path measurement first.
- Keep deleting the field at each new site. What ships today. Correct now; silently defeated by the next caller.
Context
Raised in #2202 review by both cursor-composer and the Harper domain adjudicator, and recorded in that PR's decision ledger as distrust-parsed-sql-vs-delete-field / silent-strip-vs-reject. Deliberately not bundled there: it changes a hot path and deserves its own review rather than being added to a change that had already been through six review rounds.
The invariant, and why it currently rests on an allowlist
sqlTranslator/index.ts:40is:A supplied
parsed_sql_objectis honored verbatim and skips parsing. Since #2202 madeprocessAST's permission denial live, that object'spermissions_checkedflag is load-bearing: an AST arriving withpermissions_checked: trueexecutes with no authorization check.#2202 closed every path that exists today by deleting the field at the points a client-supplied object can reach:
server/serverHelpers/serverUtilities.ts(chooseOperation) — deletesjson.search_operation.parsed_sql_objectat dispatch. Only the nested one, because the top-level is overwritten by that dispatch's own parse — though only inside its SQL branch, which is why the worker needs both.server/jobs/jobProcess.ts— deletes both positions when the worker loads the persistedhdb_jobrow, since it re-enters from storage rather than re-dispatching.That is sound for the current call graph —
evaluateSQL:40is the only consumer, and the only client-reachable objects handed to it are the top-level request andsearch_operation. But it is a per-field allowlist, not an invariant. The nextevaluateSQLcaller that forwards a client-controlled object defeats all three deletes, and nothing fails: no test covers it, and the failure is a silent authorization skip rather than an error.Proposed fix
Honor a supplied
parsed_sql_objectonly when a trusted dispatch marker is present in async context — mirroringrunWithDispatchedOperationinserver/serverHelpers/operationAuthorizationState.ts, which #2202 introduced for exactly this class of problem (a value a request cannot set). Otherwise re-parse.This keeps the dispatch-time parse reuse that the alternative — always re-parsing — would cost on every SQL call, and moves the invariant into the function that owns it.
Alternatives considered
chooseOperationalready did on every SQL request; wants a hot-path measurement first.Context
Raised in #2202 review by both cursor-composer and the Harper domain adjudicator, and recorded in that PR's decision ledger as
distrust-parsed-sql-vs-delete-field/silent-strip-vs-reject. Deliberately not bundled there: it changes a hot path and deserves its own review rather than being added to a change that had already been through six review rounds.