diff --git a/src/services/onboardingService.test.ts b/src/services/onboardingService.test.ts index 104be6b..e6a561d 100644 --- a/src/services/onboardingService.test.ts +++ b/src/services/onboardingService.test.ts @@ -260,9 +260,19 @@ describe("initializeGuidedPath — rule D9 (#396)", () => { ["INSERT", ONBOARDING_GUIDED_INITIALIZED_KEY], ["COMMIT", undefined], ]); - // The flag and the transaction count are read after BEGIN. - const firstSelect = fake.db.select.mock.invocationCallOrder[0]; - expect(firstSelect).toBeGreaterThan(fake.db.execute.mock.invocationCallOrder[0]); + // The flag is read once outside any transaction, then again after BEGIN, + // with the transaction count. + const begin = fake.db.execute.mock.invocationCallOrder[0]; + const reads = fake.db.select.mock.calls.map(([sql], i) => ({ + sql: String(sql), + order: fake.db.select.mock.invocationCallOrder[i], + })); + const flagReads = reads.filter((r) => r.sql.includes("FROM user_preferences WHERE key = $1")); + expect(flagReads).toHaveLength(2); + expect(flagReads[0].order).toBeLessThan(begin); + expect(flagReads[1].order).toBeGreaterThan(begin); + const count = reads.find((r) => r.sql.includes("COUNT(*) AS transactions FROM transactions"))!; + expect(count.order).toBeGreaterThan(begin); expect(fake.prefs()).toEqual({ [ONBOARDING_CHECKLIST_HIDDEN_KEY]: "1", [ONBOARDING_GUIDED_INITIALIZED_KEY]: "1", @@ -278,17 +288,57 @@ describe("initializeGuidedPath — rule D9 (#396)", () => { expect(logInfo).not.toHaveBeenCalled(); }); - it("with the flag already set: no write at all, even with transactions", async () => { + it("with the flag already set: no transaction and no write, even with transactions", async () => { const fake = useFakeDb({ transactions: 3, prefs: { [ONBOARDING_GUIDED_INITIALIZED_KEY]: "1" }, }); await expect(initializeGuidedPath()).resolves.toEqual({ hidden: false }); - expect(fake.statements).toEqual(["BEGIN", "COMMIT"]); + expect(withTransaction).not.toHaveBeenCalled(); + expect(fake.statements).toEqual([]); + expect(fake.db.select).toHaveBeenCalledTimes(1); expect(fake.prefs()).toEqual({ [ONBOARDING_GUIDED_INITIALIZED_KEY]: "1" }); expect(logInfo).not.toHaveBeenCalled(); }); + it('counts the flag as set only for the value "1", outside and inside the transaction', async () => { + for (const value of ["0", "true", ""]) { + vi.mocked(withTransaction).mockClear(); + const fake = useFakeDb({ + transactions: 3, + prefs: { [ONBOARDING_GUIDED_INITIALIZED_KEY]: value }, + }); + await expect(initializeGuidedPath()).resolves.toEqual({ hidden: true }); + expect(withTransaction).toHaveBeenCalledTimes(1); + expect(fake.statements).toEqual([ + "BEGIN", + "INSERT INTO user_preferences", + "INSERT INTO user_preferences", + "COMMIT", + ]); + expect(fake.prefs()[ONBOARDING_GUIDED_INITIALIZED_KEY]).toBe("1"); + } + }); + + it("re-reads the flag inside the transaction: a first load that set it in between wins (CWE-362)", async () => { + const fake = useFakeDb({ transactions: 3 }); + const read = fake.db.select.getMockImplementation()!; + let flagReads = 0; + fake.db.select.mockImplementation(async (sql: string, params?: unknown[]) => { + const rows = await read(sql, params); + if (params?.[0] === ONBOARDING_GUIDED_INITIALIZED_KEY && ++flagReads === 1) { + // Another opening runs D9 to its end right after this read. + fake.prefs()[ONBOARDING_GUIDED_INITIALIZED_KEY] = "1"; + } + return rows; + }); + await expect(initializeGuidedPath()).resolves.toEqual({ hidden: false }); + expect(flagReads).toBe(2); + expect(fake.statements).toEqual(["BEGIN", "COMMIT"]); + expect(fake.prefs()[ONBOARDING_CHECKLIST_HIDDEN_KEY]).toBeUndefined(); + expect(logInfo).not.toHaveBeenCalled(); + }); + it("a profile shown again after D9 is not hidden a second time", async () => { // D9 hid it, the user showed it again from Settings → Users. const fake = useFakeDb({ transactions: 3 }); diff --git a/src/services/onboardingService.ts b/src/services/onboardingService.ts index bdd78cc..3a0fc29 100644 --- a/src/services/onboardingService.ts +++ b/src/services/onboardingService.ts @@ -123,15 +123,24 @@ export async function getOnboardingSnapshot(): Promise { * Rule D9, once per profile: at the first opening under this version, a * profile that already has transactions gets the path hidden (same preference * as "Hide", so Settings → Users shows it again); an empty or new profile sees - * it. Runs as one transaction under the DB lock, flag re-read inside it, so two - * concurrent first loads cannot apply it twice (CWE-362): the hidden preference - * is written first, then the flag, then COMMIT; any failure rolls both back and - * leaves the flag unset, so the next opening tries again. + * it. + * + * The flag is read first, outside any transaction: once D9 ran, every later + * opening costs that one queued read and opens no transaction (while a + * `withTransaction` callback runs, `db.ts` lets every other query bypass its + * lock). Without the flag, D9 runs as one transaction under the DB lock, flag + * re-read inside it, so two concurrent first loads cannot apply it twice + * (CWE-362): the hidden preference is written first, then the flag, then + * COMMIT; any failure rolls both back and leaves the flag unset, so the next + * opening tries again. Only the value "1" counts as set, on both reads. * * Returns `{ hidden: true }` only when this call hid the path, which it also * writes to the in-app log. Not announced on the data channel. */ export async function initializeGuidedPath(): Promise<{ hidden: boolean }> { + if ((await getPreference(ONBOARDING_GUIDED_INITIALIZED_KEY)) === "1") { + return { hidden: false }; + } const hidden = await withTransaction(async (db) => { await db.execute("BEGIN"); try {