202 lines
12 KiB
Markdown
202 lines
12 KiB
Markdown
# TODO — Database provider reorganization
|
||
|
||
Milestones 1–3 are done (provider contract, MSSQL provider, repo reorg).
|
||
SQLite provider, native schema, and query translations now exist; store wiring,
|
||
migrations, lifecycle methods, and parity tests remain outstanding.
|
||
|
||
## 1. Convert store modules to the provider API (per area, keep behavior)
|
||
Replace raw `pool` usage in `src/store/**` with `provider.query()` /
|
||
`provider.transaction()` / `provider.exec()` and the normalized result shape
|
||
(`{ rows, rowsAffected, insertId }` instead of `recordset` / `rowsAffected[]`).
|
||
For every module:
|
||
- [x] `pool.request().input(...).query(SQL)` → `provider.query(SQL, params)`
|
||
- [x] `pool.transaction(); begin; commit; catch rollback` → `provider.transaction(async (tx) => { ... })`
|
||
- [x] `result.recordset` → `result.rows`; `result.rowsAffected[0]` → `result.rowsAffected`
|
||
- [x] `request.js` helper (`(tx ?? pool).request()`) becomes obsolete — deleted
|
||
- [x] `src/store/runner.js` bridges mixed call sites: converted modules accept a
|
||
provider runner *or* a legacy mssql Transaction/`pool` (wrapped on the fly)
|
||
so partially converted callers keep working
|
||
- [x] `recordset[0].id` after `OUTPUT INSERTED.id` → keep, but prefer `insertId` where trivial
|
||
|
||
Conversion order (bottom-up by dependency):
|
||
- [x] `lang`, `tax` (leaf helpers, used by company create)
|
||
- [x] `google_auth`
|
||
- [x] `companyuser` (resolveCompanyAccess / resolveSelectedCompanyAccess first)
|
||
- [x] `company`, `companysetting`
|
||
- [x] `customergroup`, `entity`, `entityaddress`, `entitycontact`, `entityhistory`
|
||
- [x] `unit` (incl. `ensureDefault`), `brand`, `supplier`
|
||
- [x] `numbers` (see §2 — stored procs!)
|
||
- [x] `category` (recursive CTEs!)
|
||
- [x] `article` + articletext/articlepic/articleean/price/articlehistory
|
||
- [x] `articlesupplier`
|
||
- [x] `quote`, `order`, `delivery` (shared manifest helpers: `manifestLines`, `manifestDoc`, `manifestattachment`)
|
||
- [x] `posterminal`, `pic`
|
||
- [x] `initialize.js`, `purge.js` final wiring (drop `provider.pool` fallback from `index.js`)
|
||
- [x] Delete `src/store/request.js` once no module uses it
|
||
|
||
## 2. Replace MSSQL stored procedures (blocker for SQLite)
|
||
- [x] `numbers.nextNumber` uses proc `getnextnumberwithprefixandpadding` (output param)
|
||
- [x] `numbers.peekNumber` uses proc `peeknextnumberwithprefixandpadding` (output param)
|
||
- [x] Implement both in plain JS inside the store layer: `SELECT next/prefix/padding`,
|
||
compute `prefix + padded next`, `UPDATE numbers SET next = next + 1 WHERE ...`.
|
||
Must stay atomic: use `provider.transaction()` with row locking semantics
|
||
(MSSQL: `WITH (UPDLOCK, ROWLOCK)` in the SELECT; SQLite: within its write tx).
|
||
- [ ] Drop the proc definitions from schema/init scripts (`scripts/init-schema.js`).
|
||
|
||
## 3. provider contract additions / hygiene
|
||
- [ ] Add `isUniqueConstraintError` / `isForeignKeyConstraintError` to the
|
||
contract and use them in stores instead of raw mssql error numbers
|
||
(currently error messages like `Customer group in use`, `Brand in use`,
|
||
`Unit in use` rely on FK/unique detection — grep for these).
|
||
- [ ] Make `initialize()`/`purge()` real provider methods (schema migrations move
|
||
into the provider; store delegates). Decide per §5.
|
||
- [ ] Document provider contract (JSDoc typedef in `src/providers/provider.js`).
|
||
|
||
## 4. better-sqlite3 provider
|
||
- [x] Add `src/providers/better-sqlite3/index.js` implementing the same contract:
|
||
- `query()` → prepare + `.all()`/`.get()`/`.run()`; normalize
|
||
`lastInsertRowid` → `insertId`, `changes` → `rowsAffected`
|
||
- `transaction(fn)` → `db.transaction()` wrapper (note: better-sqlite3 is sync;
|
||
wrap in async signature so store code stays unchanged)
|
||
- Named params: mssql uses `@name`; better-sqlite3 uses `:name` / `@name` /
|
||
`$name` — define one (e.g. `@name`) in SQLite SQL so params pass through
|
||
- Pragmas on open: `foreign_keys = ON`, `journal_mode = WAL`, `busy_timeout = 5000`
|
||
- `BLOB` handling: return `Buffer` for pic/image/attachment binaries
|
||
- Error classification: unique → `SQLITE_CONSTRAINT_UNIQUE`, FK → `SQLITE_CONSTRAINT_FOREIGNKEY`
|
||
- Do NOT close the caller-owned db in the provider; expose `close()`
|
||
- [x] `better-sqlite3` as peerDependency (optional) in `package.json`
|
||
- [x] package.json exports: `"./providers/better-sqlite3"`
|
||
|
||
## 5. SQLite schema (native, not mechanically translated)
|
||
- [x] Native schema in `src/providers/sqlite3/schema.js`:
|
||
- `INT IDENTITY` → `INTEGER PRIMARY KEY` (rowid)
|
||
- `NVARCHAR(n)` → `TEXT`; `VARBINARY(MAX)` → `BLOB`
|
||
- `DATETIME2 DEFAULT SYSUTCDATETIME()` → `TEXT` ISO-8601 UTC
|
||
(`strftime('%Y-%m-%dT%H:%M:%fZ','now')`)
|
||
- `IF NOT EXISTS ... sys.tables` → `CREATE TABLE IF NOT EXISTS`
|
||
- `OUTPUT INSERTED...` in queries → `RETURNING ...`
|
||
- `MERGE companysetting` → `INSERT ... ON CONFLICT ... DO UPDATE`
|
||
- `[identifier]` brackets → rename or quote (`"number"`)
|
||
- [ ] Add versioned SQLite migrations around the native schema
|
||
- [ ] **Entity number uniqueness parity** (MSSQL still uses indexed view
|
||
`v_entity_company_number`):
|
||
- [x] SQLite: add `company_id` to `entity` (denormalized from customergroup, kept
|
||
in sync on insert + customergroup moves) and enforce
|
||
`CREATE UNIQUE INDEX uq_entity_company_number ON entity(company_id, number) WHERE number IS NOT NULL`
|
||
- [ ] Add the equivalent MSSQL migration for parity; remove the indexed view
|
||
- [ ] Versioned migrations via `meta` table (already exists) instead of pure
|
||
`IF NOT EXISTS` scripts for both providers
|
||
- [ ] Implement `initialize()` (migrations) and `purge()` (drop tables, FK-order)
|
||
in the SQLite provider; port `purgeSql.js` logic (currently topological
|
||
drop order — replicate)
|
||
|
||
## 6. SQL translation per area (SQLite queries)
|
||
- [x] Add all 32 modules under `src/providers/sqlite3/queries/`, preserving the
|
||
MSSQL query modules' named-export API (plus SQLite number helpers)
|
||
- [x] `TOP 1` → `LIMIT 1` (mind: TOP without ORDER BY — keep identical ordering)
|
||
- [x] `OUTER APPLY (SELECT TOP 1 ...)` (article/category first-pic joins) →
|
||
correlated subquery or `JOIN` on min(sort,id)
|
||
- [x] `FOR JSON PATH` queries → SQLite JSON1 aggregation with shapes compatible
|
||
with the current store parsers (building JSON in JS remains a future cleanup)
|
||
- [x] Recursive CTEs (`SEARCH_ARTICLES`, `RESOLVE_ARTICLE_ACCESS_BY_SKU`,
|
||
`GET_CATEGORY_TREE_FLAT`) → SQLite `WITH RECURSIVE` (mostly compatible)
|
||
- [x] `ISNULL` / `COALESCE` differences; `FLOAT` vs `REAL`
|
||
- [x] Case-insensitive comparisons relying on MSSQL default collation
|
||
(e.g. name-uniqueness checks) → `COLLATE NOCASE` in SQLite schema or
|
||
explicit `LOWER()` in both dialects
|
||
- [x] Compile all 212 complete SQLite query exports against an in-memory native
|
||
schema (zero syntax/schema errors)
|
||
|
||
## 7. Contract / parity tests
|
||
- [ ] Test harness that runs the identical suite against both providers
|
||
(SQLite in-memory for speed; MSSQL in CI)
|
||
- [ ] Cover: return shapes, auth errors (`No company selected`, `Not authorized`),
|
||
domain errors, tx rollback, FK/unique behavior (`Brand in use` etc.),
|
||
binary round-trips (pic, manifestattachment, google_auth picturebinary),
|
||
date handling, recursive category tree, importRows two-pass V/K linking,
|
||
numbers.next/peek atomicity
|
||
- [ ] `scripts/test-api.js`: parametrize over provider
|
||
- [ ] `scripts/init-schema.js`, `reset-db.js`, `purge-db.js`: accept provider
|
||
selection (e.g. `--provider=better-sqlite3` / env `DASH_DB_PROVIDER`)
|
||
|
||
## 8. Docs & tooling
|
||
- [ ] README: provider-based usage, both examples, migration notes for dashback
|
||
- [ ] `API.md` / `API.*.md` generators (`generate-api-doc.js`): update paths if
|
||
they scan `src/sql` / `src/schema`
|
||
- [ ] `generate-schema-graph.js` / `schema-graph.dot`: point at both schemas
|
||
- [ ] Deprecation note: `new LibDashStore(pool)` removed — constructor now takes
|
||
a provider (breaking change; coordinate with dashback)
|
||
|
||
## 9. Cleanup
|
||
- [ ] Remove transitional `this.pool` / `provider.pool` usage from `index.js`
|
||
- [ ] Remove `mssql` from peerDependencies only if no runtime coupling remains
|
||
(queries/schema stay mssql-specific, so likely keep it)
|
||
- [ ] Decide export for `SCHEMA_SCRIPTS` (currently re-exported from `index.js`
|
||
from the moved schema file)
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
I'll check how auth is applied across the routes.
|
||
|
||
Viewed index.js:1-305
|
||
Searched for "app\.(post|put|patch|delete|get|use|all)\("
|
||
Viewed auth.js:1-132
|
||
|
||
The middleware skips auth on several path prefixes. Next I'll check which of those can write.
|
||
|
||
Searched for "\.(post|put|patch|delete)\("
|
||
Searched for "'/(api/)?(public|mcp|oauth|\.well-known)"
|
||
|
||
Next: the telemetry public route, the `/oauth/register` handler, and the v1 router.
|
||
|
||
Viewed telemetry.js:1-118
|
||
Viewed mcp.js:360-460
|
||
Searched for "(router|app)\.(post|put|patch|delete|use|all)\(|auth|token"
|
||
Viewed index.js:1-27
|
||
Searched for "registerClient|export (async )?function"
|
||
Viewed mcp.js:455-540
|
||
Searched for "db\.\w+|verifyIdToken|allow|jwt\.sign"
|
||
|
||
**No. With no JWT, an attacker can still write to the database.** The checks I did are static code review only, and I haven't run anything.
|
||
|
||
`authMiddleware` ([auth.js](file:///home/seb/src/dashback/src/api/middleware/auth.js#L49-L68)) lets several path groups through without a token. Most of them can't write to the SQL database, but two can.
|
||
|
||
### 1. Open OAuth registration: `POST /oauth/register`
|
||
- This route is public, and it calls `mcpDb.registerClient` ([mcp.js:368](file:///home/seb/src/dashback/src/api/routes/mcp.js#L368)) to insert a client row.
|
||
- Anyone can create unlimited rows with no auth and no rate limit.
|
||
- `client_name`, `redirect_uris`, `grant_types` and `scope` all come straight from the request body.
|
||
|
||
### 2. Auth bypass: `POST /oauth/authorize`
|
||
- The middleware calls `next()` on this route even with no token ([auth.js:70-79](file:///home/seb/src/dashback/src/api/middleware/auth.js#L70-L79)).
|
||
- The handler ([mcp.js:455-503](file:///home/seb/src/dashback/src/api/routes/mcp.js#L455-L503)) takes an `email` from the body when there's no `req.user`.
|
||
- If that email is on the allowlist, it writes an auth code (`createAuthCode`) for that user and redirects with it. **No password, Google token or JWT is checked.**
|
||
- An attacker can chain this with the previous hole:
|
||
1. Register a client with `/oauth/register`.
|
||
2. Post to `/oauth/authorize` with the victim's allowlisted email and their own PKCE `code_challenge`.
|
||
3. Exchange the code at `/oauth/token`, which I haven't read.
|
||
4. Call `/mcp` as that user.
|
||
- That would be a full impersonation if the MCP tools can write. Please confirm this by testing against a dev instance.
|
||
- Also, `new URL(redirect_uri)` runs with no try/catch and no check that `redirect_uri` matches the registered URIs. A bad value throws, and a valid one is an open redirect that sends the code to the attacker.
|
||
|
||
### Not writing to the SQL database
|
||
- `/api/public/telemetry/crash` ([telemetry.js](file:///home/seb/src/dashback/src/api/routes/telemetry.js#L87-L104)) is public and unauthenticated. It writes to a file on disk rather than SQL, but there's no size or rate limit beyond `bodyLimit`, so it can fill the disk.
|
||
- `/v1` and `/api/v1` skip the middleware, but the routes I saw are all `GET`. The ones with data use `posAuthMiddleware`, and `/client`, `/pimage` and `/cimage` have no auth.
|
||
- `/api/auth/login` verifies a Google ID token and checks the allowlist. If `IMPLICIT_CREATE=true`, any valid Google account gets added to the allowlist ([login.js:107](file:///home/seb/src/dashback/src/api/routes/auth/login.js#L107)). Make sure that's off in production.
|
||
|
||
### Suggested fixes
|
||
- Make `/oauth/authorize` POST require `req.user`, and remove the `email` fallback.
|
||
- Validate `redirect_uri` against the registered URIs for the client.
|
||
- Rate-limit `/oauth/register`, or gate it.
|
||
|
||
Want me to implement these? I haven't changed any code. |