Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/loopover-miner/lib/laptop-init.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,10 @@ export function checkLaptopStateSqlite(env = process.env) {
};
}
try {
const db = new DatabaseSync(dbPath, { readonly: true });
// `readOnly` (camelCase) -- node:sqlite silently IGNORES `readonly` (lowercase) as an unrecognized option
// and opens read-write anyway, which would break doctor's own "no writes, no network" contract. Same
// footgun already documented in claim-ledger.js's openClaimLedgerReadOnly and purge-cli.js.
const db = new DatabaseSync(dbPath, { readOnly: true });
db.prepare("SELECT 1").get();
db.close();
return { name: "laptop-state-sqlite", ok: true, detail: dbPath };
Expand Down
44 changes: 44 additions & 0 deletions test/unit/miner-laptop-init.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,26 @@ import {
} from "node:fs";
import { tmpdir } from "node:os";
import { delimiter, join } from "node:path";
import { DatabaseSync } from "node:sqlite";
import { afterEach, describe, expect, it, vi } from "vitest";

/** Records every `new DatabaseSync(path, options)` the module under test performs, so a test can assert the
* OPTIONS it opens with (#6765) -- checkLaptopStateSqlite never exposes its own handle. The subclass just
* records and delegates, so every other test's behavior is unchanged. */
const { databaseSyncOptions } = vi.hoisted(() => ({ databaseSyncOptions: [] as Array<Record<string, unknown> | undefined> }));

vi.mock("node:sqlite", async (importOriginal) => {
const actual = await importOriginal<typeof import("node:sqlite")>();
class RecordingDatabaseSync extends actual.DatabaseSync {
// Rest-args forwarding: passing an explicit `undefined` options arg is NOT the same as omitting it, so
// every existing `new DatabaseSync(path)` caller must reach super with its original arity.
constructor(...args: ConstructorParameters<typeof actual.DatabaseSync>) {
databaseSyncOptions.push(args[1] as Record<string, unknown> | undefined);
super(...args);
}
}
return { ...actual, DatabaseSync: RecordingDatabaseSync };
});
import {
checkDockerPresent,
checkLaptopStateSqlite,
Expand Down Expand Up @@ -81,6 +100,31 @@ describe("loopover-miner laptop init (#2329)", () => {
expect(check.detail).toContain("loopover-miner init");
});

it("REGRESSION: doctor's laptop-state check opens a driver-enforced read-only connection (#6765)", () => {
const root = tempRoot();
const env = { LOOPOVER_MINER_CONFIG_DIR: join(root, "state") };
initLaptopState(env);
const dbPath = resolveLaptopStateDbPath(env);
databaseSyncOptions.length = 0;

expect(checkLaptopStateSqlite(env).ok).toBe(true);
// The check must open with camelCase `readOnly`. node:sqlite silently IGNORES the lowercase `readonly`
// key as an unrecognized option and opens read-write anyway -- which is what this used to pass, breaking
// doctor's own documented "no writes, no network" contract.
expect(databaseSyncOptions).toEqual([{ readOnly: true }]);

// ...and that option is what makes the connection driver-enforced: a write through it throws, whereas the
// silently-ignored lowercase spelling happily writes.
const enforced = new DatabaseSync(dbPath, { readOnly: true });
expect(() => enforced.exec("CREATE TABLE probe_camel (id INTEGER)")).toThrow();
enforced.close();

const ignored = new DatabaseSync(dbPath, { readonly: true } as never);
expect(() => ignored.exec("CREATE TABLE probe_lower (id INTEGER)")).not.toThrow();
ignored.exec("DROP TABLE probe_lower");
ignored.close();
});

it("doctor sqlite check reports unreadable files", () => {
const root = tempRoot();
const env = { LOOPOVER_MINER_CONFIG_DIR: join(root, "state") };
Expand Down