HarperFast / HarperFast/harper

evaluateSQL should not honor a caller-supplied parsed_sql_object without a trusted dispatch marker

Open
#2,304 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.