HarperFast / HarperFast/harper
evaluateSQL should not honor a caller-supplied parsed_sql_object without a trusted dispatch marker
- Dominant language
- JavaScript
- Stars
- 89
- Forks
- 10
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 200
Description
## The invariant, and why it currently rests on an allowlist
`sqlTranslator/index.ts:40` is:
```js
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.
Contributor guide
Research direction
Start with sqlTranslator/index.ts and trace evaluateSQL callers through chooseOperation in server/serverHelpers/serverUtilities.ts and the worker path in server/jobs/jobProcess.ts. Read runWithDispatchedOperation in server/serverHelpers/operationAuthorizationState.ts first, then verify that only its trusted dispatch context permits a supplied parsed_sql_object and that an untrusted object cannot bypass authorization; add coverage for the untrusted caller path.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- sql, typescript
- Domain
- backend-api-design, databases, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 56/100