Bind values as parameters in the local wallet database queries

[eastagiletracker]

This PR proposes binding caller-supplied values as SQL parameters in the local wallet database layer, so that a search term or a contact field can no longer change the statement that runs against the wallet's sqlite file. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/275. You can sign in with your GitHub ID to claim ownership of the project.

What is happening today

Most statements in src/api/db/statements/* are built by interpolating the caller's value into the SQL string — for example select * from Contacts where name like "${value}%" or address like "${value}%" — and DatabaseManager.update() does the same for column values, via flattenObjectToSetClause wrapping strings in single quotes.

Those values arrive straight from the UI. SearchContacts passes what the user typed into searchContactsByValue → databaseManager.getContactsByValue; the send-funds and receive-funds address lists do the same through getWalletAddressesByValue; transaction search reaches searchTransactionsForValue; the contact and wallet sidepanels pass their form fields to updateContactById / updateWalletById; and updateTxByTxId uses a transaction id that came back from the explorer API.

Three things follow from that, in increasing order of seriousness. A value containing a quote character produces a syntax error, so searching for Jean "Ace" Doe or saving a contact named Sam O'Neil rejects the query and the search results or the save silently do nothing. A crafted search value can select from another table, and the result rows are rendered in the list — the search path returns rows through db.prepare, so a union select reaches the seed and private key columns of Wallets and Addresses. And an update reaches db.run without params, which runs every statement in the string, so a contact field is enough to execute a second statement of the attacker's choosing.

Reproduction on master

Against master at 2a3b646, with only tests/unit/api/db/database-manager.unit.test.js from this branch added and src/ untouched, yarn run test:unit reports:

✕ treats a search value containing a double quote as data
✕ does not let a search value select columns of another table
✕ does not let a search value select the stored private keys
✕ treats a search value containing a double quote as data
✕ stores a name containing a single quote verbatim
✕ does not run a statement embedded in a contact field
Tests: 6 failed, 24 passed, 30 total

The two that matter most: getContactsByValue('" union select id, name, seed, description from Wallets --') comes back with the wallet's mnemonic seed in the contact result set, and updateContactById(1, { name: "Mallory'; delete from Wallets; --" }) leaves the Wallets table empty.

The change

The SELECT entries that took a value are now constants holding ? placeholders instead of functions that interpolate one, and every caller-supplied value is bound through the params path that DatabaseManager.query() already implements and documents. update() and delete() take the values for their where clause the same way, through a new optional whereParams, and flattenObjectToSetClause now emits col=? and lets the values bind. Nothing new was added to the dependency list — sqlstring-sqlite is already declared but not imported anywhere, and binding through the existing query() path makes it unnecessary.

DatabaseManager is the only consumer of STATEMENTS in the repo, so no other caller changes shape. LIKE semantics are preserved exactly: the % wildcards are appended to the bound value, in the same prefix or substring position each statement used before.

Verification

tests/unit/api/db/database-manager.unit.test.js adds 30 assertions covering every query method the change touches: the 6 above fail without the src/ half of this branch and pass with it, and the other 24 pass on both trees — they are the controls that the legitimate behaviour did not move (prefix search still matches, date filters still filter, updates and deletes still hit exactly one row, getTxAddressIds, getAddressName, getBalanceByAddress, getPrivKeyFromAddress and the theme and db-version updates all still return what they did before).

On this branch the full unit suite is green — Tests: 34 passed, 34 total, the 4 that existed plus these 30 — run with node 14 to match .github/workflows/test.yml. yarn run lint behaves the same before and after: it stops at the same TypeError: Cannot read property 'range' of null inside eslint's template-curly-spacing rule that master already hits on multi-line template literals, which is untouched by this change.

How this was managed

We track this work on a live board built from your repo's own issues and pull requests: this change is the story Bind SQL parameters in the local wallet database queries, on the board at https://eastagiletracker.com/projects/275.

If you'd rather not receive contributions like this, reply no-more-prs on this pull request and we won't open any further ones on your repositories.

---

Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com