From 20f957d869bc73df1d5577b9a341f742e397cdc1 Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Mon, 7 Sep 2026 12:54:22 -0400 Subject: [PATCH 1/2] src: throw on a malformed localStorage file The localStorage backing file is a user-specified path, and the schema is created with CREATE TABLE IF NOT EXISTS, so a file that already contains tables of those names is adopted as-is. Its stored values may then have any SQLite type, but every read asserted the expected type with CHECK, so a wrong-typed value aborted the process. A bad schema_version was the worst case: that assertion is in Storage::Open(), so any access aborted and the application had no chance to inspect or repair the file. Report these as ERR_INVALID_STATE instead, matching the throw four lines below the schema_version assertion for a version that is too new. Storage::GetAll() has no JavaScript caller to throw at, so it returns std::nullopt and the DOM storage inspector agent reports a protocol error. Now that a failed open returns instead of aborting, Open() has to clean up after itself: adopt the sqlite3* into a conn_unique_ptr immediately, so that an error does not leak the connection and leave the next access to open another one. Storage::GetAll() also ignored the result of sqlite3_prepare_v2() and the status its row loop ended on, reporting a malformed file or a mid-scan error as an empty store. Both now return std::nullopt. Also drop a redundant second sqlite3_exec() of the init SQL that clobbered the result of the sqlite3_prepare_v2() above it, hiding prepare failures behind a misleading "bad parameter or other API misuse". Signed-off-by: Trevor Burnham Assisted-by: Claude Opus 5 --- src/inspector/dom_storage_agent.cc | 4 + src/node_webstorage.cc | 73 ++++++++++++-- src/node_webstorage.h | 8 +- test/parallel/test-webstorage.js | 155 ++++++++++++++++++++++++++++- 4 files changed, 224 insertions(+), 16 deletions(-) diff --git a/src/inspector/dom_storage_agent.cc b/src/inspector/dom_storage_agent.cc index 3708d3b59975..05a41d933d70 100644 --- a/src/inspector/dom_storage_agent.cc +++ b/src/inspector/dom_storage_agent.cc @@ -103,6 +103,10 @@ protocol::DispatchResponse DOMStorageAgent::getDOMStorageItems( auto web_storage_obj = getWebStorage(is_local_storage); if (web_storage_obj) { storage_map_fallback = web_storage_obj.value()->GetAll(); + if (!storage_map_fallback.has_value()) { + return protocol::DispatchResponse::ServerError( + "Could not read DOM storage items"); + } storage_map = &storage_map_fallback.value(); } } diff --git a/src/node_webstorage.cc b/src/node_webstorage.cc index 21f846fbeb62..d30dac025445 100644 --- a/src/node_webstorage.cc +++ b/src/node_webstorage.cc @@ -59,6 +59,19 @@ using v8::Value; } \ } while (0) +// The backing file is a user-specified path, and the schema below is created +// with IF NOT EXISTS, so a file that already holds tables of those names is +// adopted as-is and its values may have any type. A wrong type is therefore a +// statement about untrusted input, not a broken internal invariant. +#define CHECK_COLUMN_TYPE_OR_THROW(env, stmt, idx, expected, detail, ret) \ + do { \ + if (sqlite3_column_type((stmt), (idx)) != (expected)) { \ + THROW_ERR_INVALID_STATE((env), \ + "localStorage database is malformed: " detail); \ + return (ret); \ + } \ + } while (0) + static void ThrowQuotaExceededException(Local context) { Isolate* isolate = Isolate::GetCurrent(); auto quota_exceeded_str = @@ -173,6 +186,12 @@ Maybe Storage::Open() { } int r = sqlite3_open(location_.c_str(), &db); + // Adopt the connection before anything below can return early, so that a + // failure does not leak it. sqlite3_open() allocates a connection to be + // closed even when it fails. This is declared ahead of the statement below + // so that the statement is finalized first; sqlite3_close() fails while a + // statement is still open, and conn_deleter treats that as fatal. + auto conn = conn_unique_ptr(db); CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing()); r = sqlite3_exec(db, init_sql_v0.data(), nullptr, nullptr, nullptr); CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing()); @@ -184,12 +203,16 @@ Maybe Storage::Open() { get_schema_version_sql.size(), &s, nullptr); - r = sqlite3_exec(db, init_sql_v0.data(), nullptr, nullptr, nullptr); - CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing()); auto stmt = stmt_unique_ptr(s); + CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing()); CHECK_ERROR_OR_THROW( env(), sqlite3_step(stmt.get()), SQLITE_ROW, Nothing()); - CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_INTEGER); + CHECK_COLUMN_TYPE_OR_THROW(env(), + stmt.get(), + 0, + SQLITE_INTEGER, + "expected schema_version to be an integer", + Nothing()); int schema_version = sqlite3_column_int(stmt.get(), 0); stmt = nullptr; // Force finalization. @@ -209,7 +232,7 @@ Maybe Storage::Open() { CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing()); } - db_ = conn_unique_ptr(db); + db_ = std::move(conn); return JustVoid(); } @@ -266,7 +289,12 @@ MaybeLocal Storage::Enumerate() { LocalVector values(env()->isolate()); Local value; while ((r = sqlite3_step(stmt.get())) == SQLITE_ROW) { - CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_BLOB); + CHECK_COLUMN_TYPE_OR_THROW(env(), + stmt.get(), + 0, + SQLITE_BLOB, + "expected key to be a blob", + Local()); auto size = sqlite3_column_bytes(stmt.get(), 0) / sizeof(uint16_t); if (!String::NewFromTwoByte(env()->isolate(), reinterpret_cast( @@ -282,9 +310,10 @@ MaybeLocal Storage::Enumerate() { return Array::New(env()->isolate(), values.data(), values.size()); } -std::unordered_map Storage::GetAll() { +std::optional> +Storage::GetAll() { if (!Open().IsJust()) { - return {}; + return std::nullopt; } static constexpr std::string_view sql = @@ -292,10 +321,17 @@ std::unordered_map Storage::GetAll() { sqlite3_stmt* s = nullptr; int r = sqlite3_prepare_v2(db_.get(), sql.data(), sql.size(), &s, nullptr); auto stmt = stmt_unique_ptr(s); + // Unlike the other accessors, this one has no JavaScript caller to throw at, + // so every failure below is reported to the inspector agent instead. + if (r != SQLITE_OK) { + return std::nullopt; + } std::unordered_map result; while ((r = sqlite3_step(stmt.get())) == SQLITE_ROW) { - CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_BLOB); - CHECK(sqlite3_column_type(stmt.get(), 1) == SQLITE_BLOB); + if (sqlite3_column_type(stmt.get(), 0) != SQLITE_BLOB || + sqlite3_column_type(stmt.get(), 1) != SQLITE_BLOB) { + return std::nullopt; + } auto key_size = sqlite3_column_bytes(stmt.get(), 0) / sizeof(uint16_t); auto value_size = sqlite3_column_bytes(stmt.get(), 1) / sizeof(uint16_t); auto key_uint16( @@ -308,6 +344,9 @@ std::unordered_map Storage::GetAll() { result.emplace(std::move(key), std::move(value)); } + if (r != SQLITE_DONE) { + return std::nullopt; + } return result; } @@ -324,6 +363,8 @@ MaybeLocal Storage::Length() { auto stmt = stmt_unique_ptr(s); CHECK_ERROR_OR_THROW( env(), sqlite3_step(stmt.get()), SQLITE_ROW, Local()); + // Unlike the reads above, this one is not a claim about the file's contents: + // count(*) is an integer whatever the table holds. CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_INTEGER); int result = sqlite3_column_int(stmt.get(), 0); return Integer::New(env()->isolate(), result); @@ -351,7 +392,12 @@ MaybeLocal Storage::Load(Local key) { CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Local()); r = sqlite3_step(stmt.get()); if (r == SQLITE_ROW) { - CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_BLOB); + CHECK_COLUMN_TYPE_OR_THROW(env(), + stmt.get(), + 0, + SQLITE_BLOB, + "expected value to be a blob", + Local()); auto size = sqlite3_column_bytes(stmt.get(), 0) / sizeof(uint16_t); return String::NewFromTwoByte(env()->isolate(), reinterpret_cast( @@ -383,7 +429,12 @@ MaybeLocal Storage::LoadKey(const int index) { r = sqlite3_step(stmt.get()); if (r == SQLITE_ROW) { - CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_BLOB); + CHECK_COLUMN_TYPE_OR_THROW(env(), + stmt.get(), + 0, + SQLITE_BLOB, + "expected key to be a blob", + Local()); auto size = sqlite3_column_bytes(stmt.get(), 0) / sizeof(uint16_t); return String::NewFromTwoByte(env()->isolate(), reinterpret_cast( diff --git a/src/node_webstorage.h b/src/node_webstorage.h index 938a2333194b..02de9c79b84c 100644 --- a/src/node_webstorage.h +++ b/src/node_webstorage.h @@ -3,6 +3,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include #include #include "base_object.h" #include "node_mem.h" @@ -41,7 +42,12 @@ class Storage : public BaseObject { v8::MaybeLocal LoadKey(const int index); v8::Maybe Remove(v8::Local key); v8::Maybe Store(v8::Local key, v8::Local value); - std::unordered_map GetAll(); + // Returns nothing if the backing store could not be read, e.g. because it + // holds values of an unexpected type. Opening the store can also throw, so + // the caller must hold a v8::TryCatch: an empty return does not say which of + // the two happened, and a pending exception is left for the caller to + // handle. + std::optional> GetAll(); SET_MEMORY_INFO_NAME(Storage) SET_SELF_SIZE(Storage) diff --git a/test/parallel/test-webstorage.js b/test/parallel/test-webstorage.js index 383e239d7d68..106a969cb2fe 100644 --- a/test/parallel/test-webstorage.js +++ b/test/parallel/test-webstorage.js @@ -1,11 +1,14 @@ 'use strict'; -const { skipIfSQLiteMissing, spawnPromisified } = require('../common'); +const { + isLinux, isMacOS, skipIfSQLiteMissing, spawnPromisified, +} = require('../common'); skipIfSQLiteMissing(); const tmpdir = require('../common/tmpdir'); const assert = require('node:assert'); const { join } = require('node:path'); const { readdir } = require('node:fs/promises'); +const { DatabaseSync } = require('node:sqlite'); const { test, describe } = require('node:test'); let cnt = 0; @@ -15,6 +18,16 @@ function nextLocalStorage() { return join(tmpdir.path, `${++cnt}.localstorage`); } +// The tests below assert on which .localstorage files exist, so malformed +// fixtures are named so as not to be counted among them. +function nextMalformedLocalStorage() { + return join(tmpdir.path, `malformed-${++cnt}.db`); +} + +async function localStorageFiles() { + return (await readdir(tmpdir.path)).filter((f) => f.endsWith('.localstorage')); +} + test('Storage instances cannot be created in userland', async () => { const cp = await spawnPromisified(process.execPath, [ '-e', 'new globalThis.Storage()', @@ -46,7 +59,7 @@ test('sessionStorage is not persisted', async () => { ]); assert.strictEqual(cp.code, 0); assert.match(cp.stdout, /undefined/); - assert.strictEqual((await readdir(tmpdir.path)).length, 0); + assert.deepStrictEqual(await localStorageFiles(), []); }); test('localStorage returns undefined and warns without --localstorage-file', async () => { @@ -74,7 +87,7 @@ test('localStorage is not persisted if it is unused', async () => { ]); assert.strictEqual(cp.code, 0); assert.match(cp.stdout, /true/); - assert.strictEqual((await readdir(tmpdir.path)).length, 0); + assert.deepStrictEqual(await localStorageFiles(), []); }); test('localStorage is persisted if it is used', async () => { @@ -85,7 +98,7 @@ test('localStorage is persisted if it is used', async () => { ]); assert.strictEqual(cp.code, 0); assert.match(cp.stdout, /barbaz/); - const entries = await readdir(tmpdir.path); + const entries = await localStorageFiles(); assert.strictEqual(entries.length, 1); assert.match(entries[0], /\d+\.localstorage/); @@ -146,3 +159,137 @@ test('disabled with --no-webstorage', async () => { assert(cp.stderr.includes(`ReferenceError: ${api} is not defined`)); } }); + +describe('a malformed localStorage file throws instead of aborting', () => { + // Node's own tables are STRICT, so it cannot store a wrong-typed value + // itself. But they are created with IF NOT EXISTS, so a file that already + // contains tables of those names is adopted as-is. Declare the same schema + // without STRICT: BLOB columns have no affinity, so TEXT stays TEXT. + function malformedLocalStorage(fill) { + const file = nextMalformedLocalStorage(); + const db = new DatabaseSync(file); + db.exec(` + CREATE TABLE nodejs_webstorage( + key BLOB NOT NULL, value BLOB NOT NULL, PRIMARY KEY(key) + ); + CREATE TABLE nodejs_webstorage_state( + max_size INTEGER NOT NULL DEFAULT 10485760, + total_size INTEGER NOT NULL, + schema_version INTEGER NOT NULL DEFAULT 1, + single_row_ INTEGER NOT NULL DEFAULT 1 CHECK(single_row_ = 1), + PRIMARY KEY(single_row_) + ); + `); + fill({ + insert: (key, value) => db.prepare( + 'INSERT INTO nodejs_webstorage (key, value) VALUES (?, ?)', + ).run(key, value), + setSchemaVersion: (schemaVersion) => db.prepare( + 'INSERT INTO nodejs_webstorage_state (total_size, schema_version)' + + ' VALUES (0, ?)', + ).run(schemaVersion), + }); + db.close(); + return file; + } + + // Keys are stored UTF-16LE, so a real key is needed for lookups to match. + const utf16 = (str) => Buffer.from(str, 'utf16le'); + + for (const [name, fill, expression, detail] of [ + [ + 'a text schema_version', + ({ setSchemaVersion }) => setSchemaVersion('one'), + 'localStorage.length', + 'expected schema_version to be an integer', + ], + [ + 'a text key read by key()', + ({ insert, setSchemaVersion }) => { + insert('greeting', utf16('hello')); + setSchemaVersion(1); + }, + 'localStorage.key(0)', + 'expected key to be a blob', + ], + [ + 'a text key read by enumeration', + ({ insert, setSchemaVersion }) => { + insert('greeting', utf16('hello')); + setSchemaVersion(1); + }, + 'Object.keys(localStorage)', + 'expected key to be a blob', + ], + [ + 'a text value', + ({ insert, setSchemaVersion }) => { + insert(utf16('greeting'), 'hello'); + setSchemaVersion(1); + }, + "localStorage.getItem('greeting')", + 'expected value to be a blob', + ], + ]) { + test(`${name}, via ${expression}`, async () => { + const cp = await spawnPromisified(process.execPath, [ + '--localstorage-file', malformedLocalStorage(fill), + '-e', expression, + ]); + + assert.strictEqual(cp.code, 1); + assert.strictEqual(cp.signal, null); + assert(cp.stderr.includes( + `Error: localStorage database is malformed: ${detail}`, + )); + assert(cp.stderr.includes("code: 'ERR_INVALID_STATE'")); + }); + } +}); + +test('a malformed localStorage file does not leak connections', { + // Counting the process's own descriptors needs a /proc/self/fd or /dev/fd + // that lists all of them. AIX and IBM i expose only 0, 1 and 2 there, which + // would make the count constant and the test vacuous. + skip: (!isLinux && !isMacOS) && 'cannot enumerate open descriptors', +}, async () => { + const file = nextMalformedLocalStorage(); + const db = new DatabaseSync(file); + db.exec(` + CREATE TABLE nodejs_webstorage_state( + max_size INTEGER NOT NULL DEFAULT 10485760, + total_size INTEGER NOT NULL, + schema_version INTEGER NOT NULL DEFAULT 1, + single_row_ INTEGER NOT NULL DEFAULT 1 CHECK(single_row_ = 1), + PRIMARY KEY(single_row_) + ); + `); + db.prepare('INSERT INTO nodejs_webstorage_state (total_size, schema_version)' + + ' VALUES (0, ?)').run('one'); + db.close(); + + // A failed open used to leave its sqlite3* behind, two descriptors at a time, + // so repeated access exhausted the descriptor limit and degraded the error + // into a misleading "unable to open database file". + const cp = await spawnPromisified(process.execPath, [ + '--localstorage-file', file, + '-e', ` + const assert = require('assert'); + const { readdirSync } = require('fs'); + const fdDir = process.platform === 'linux' ? '/proc/self/fd' : '/dev/fd'; + const openDescriptors = () => readdirSync(fdDir).length; + const attempt = () => assert.throws(() => localStorage.length, { + code: 'ERR_INVALID_STATE', + message: /expected schema_version to be an integer/, + }); + + attempt(); + const before = openDescriptors(); + for (let i = 0; i < 200; i++) attempt(); + const leaked = openDescriptors() - before; + assert.ok(leaked < 20, 'leaked ' + leaked + ' descriptors'); + `, + ]); + assert.strictEqual(cp.code, 0, cp.stderr); + assert.strictEqual(cp.stdout, ''); +}); From 43e1abd0b35e0f4e08283891014486c9da1f5014 Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Mon, 7 Sep 2026 12:54:34 -0400 Subject: [PATCH 2/2] inspector: catch errors reading DOM storage A protocol message from a remote frontend is dispatched from a libuv callback with no HandleScope on the stack, inside the SealHandleScope that MainThreadInterface::DispatchMessages() installs. Opening the localStorage backing file can throw, so allocating the error object was fatal: FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope Every Storage::Open() failure was affected, including a --localstorage-file that names a directory, so this did not need a malformed file to reach. getWebStorage() already opens a HandleScope and a TryCatch for its own handle use; do the same around the GetAll() call and report the failure as a protocol error. Signed-off-by: Trevor Burnham Assisted-by: Claude Opus 5 --- src/inspector/dom_storage_agent.cc | 21 ++++ .../test-inspector-dom-storage-malformed.js | 95 +++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 test/parallel/test-inspector-dom-storage-malformed.js diff --git a/src/inspector/dom_storage_agent.cc b/src/inspector/dom_storage_agent.cc index 05a41d933d70..32d61db86513 100644 --- a/src/inspector/dom_storage_agent.cc +++ b/src/inspector/dom_storage_agent.cc @@ -102,7 +102,28 @@ protocol::DispatchResponse DOMStorageAgent::getDOMStorageItems( if (storage_map->empty()) { auto web_storage_obj = getWebStorage(is_local_storage); if (web_storage_obj) { + // A message from a remote frontend is dispatched without a HandleScope + // on the stack, and opening the backing file can throw, so give the + // exception a scope to be allocated in and somewhere to land. + v8::HandleScope handle_scope(env_->isolate()); + v8::TryCatch try_catch(env_->isolate()); storage_map_fallback = web_storage_obj.value()->GetAll(); + if (try_catch.HasCaught()) { + // Pass the reason along; "the file was written by a newer Node.js" and + // "the file is locked" are not the same problem to the user. Read it + // off the Message, which was built when the exception was thrown. + // Converting the exception itself would call a user-patchable + // Error.prototype.toString, and there is no JavaScript frame here to + // run it from. + Local message = try_catch.Message(); + if (!message.IsEmpty()) { + Utf8Value reason(env_->isolate(), message->Get()); + return protocol::DispatchResponse::ServerError( + std::string("Could not read DOM storage items: ") + reason.out()); + } + return protocol::DispatchResponse::ServerError( + "Could not read DOM storage items"); + } if (!storage_map_fallback.has_value()) { return protocol::DispatchResponse::ServerError( "Could not read DOM storage items"); diff --git a/test/parallel/test-inspector-dom-storage-malformed.js b/test/parallel/test-inspector-dom-storage-malformed.js new file mode 100644 index 000000000000..525adb7380a1 --- /dev/null +++ b/test/parallel/test-inspector-dom-storage-malformed.js @@ -0,0 +1,95 @@ +// Reading a malformed localStorage file through the DOMStorage domain should +// report a protocol error rather than abort the process. A message from a +// remote frontend is dispatched without a HandleScope on the stack, so this +// drives the protocol over the WebSocket endpoint rather than through an +// in-process inspector Session. +'use strict'; + +const common = require('../common'); +common.skipIfSQLiteMissing(); +common.skipIfInspectorDisabled(); +const { NodeInstance } = require('../common/inspector-helper.js'); +const tmpdir = require('../common/tmpdir'); +const assert = require('node:assert'); +const { join } = require('node:path'); +const { DatabaseSync } = require('node:sqlite'); +tmpdir.refresh(); + +// Node's own tables are STRICT, but they are created with IF NOT EXISTS, so a +// file that already contains tables of those names is adopted as-is. Declare +// the same schema without STRICT: BLOB columns have no affinity, so a TEXT +// value stays TEXT. +function malformedLocalStorage(name, schemaVersion, value) { + const file = join(tmpdir.path, name); + const db = new DatabaseSync(file); + db.exec(` + CREATE TABLE nodejs_webstorage( + key BLOB NOT NULL, value BLOB NOT NULL, PRIMARY KEY(key) + ); + CREATE TABLE nodejs_webstorage_state( + max_size INTEGER NOT NULL DEFAULT 10485760, + total_size INTEGER NOT NULL, + schema_version INTEGER NOT NULL DEFAULT 1, + single_row_ INTEGER NOT NULL DEFAULT 1 CHECK(single_row_ = 1), + PRIMARY KEY(single_row_) + ); + `); + db.prepare('INSERT INTO nodejs_webstorage (key, value) VALUES (?, ?)') + .run(Buffer.from('greeting', 'utf16le'), value); + db.prepare('INSERT INTO nodejs_webstorage_state (total_size, schema_version)' + + ' VALUES (0, ?)').run(schemaVersion); + db.close(); + return file; +} + +async function getDOMStorageItems(localStorageFile) { + const instance = new NodeInstance([ + '--inspect=0', + '--experimental-storage-inspection', + `--localstorage-file=${localStorageFile}`, + ], 'setInterval(() => {}, 1000);'); + + const session = await instance.connectInspectorSession(); + await session.send({ method: 'DOMStorage.enable' }); + const { storageKey } = await session.send({ + method: 'Storage.getStorageKey', + }); + + try { + return await session.send({ + method: 'DOMStorage.getDOMStorageItems', + params: { + storageId: { isLocalStorage: true, securityOrigin: '', storageKey }, + }, + }); + } finally { + await session.disconnect(); + await instance.kill(); + } +} + +(async () => { + // A wrong-typed value is rejected by Storage::GetAll() itself, which has no + // exception to report, so the reason is not available. + await assert.rejects( + getDOMStorageItems( + malformedLocalStorage('bad-value.db', 1, 'hello')), + { message: 'Could not read DOM storage items' }, + ); + + // A wrong-typed schema_version makes Storage::Open() throw, which has to be + // caught rather than left pending on an isolate with no JavaScript running. + // Its message reaches the frontend. + await assert.rejects( + getDOMStorageItems( + malformedLocalStorage( + 'bad-schema-version.db', 'one', Buffer.from('hello', 'utf16le'))), + { + // The reason comes off the v8::Message, hence the "Uncaught" prefix; + // converting the exception itself would run user JavaScript. + message: 'Could not read DOM storage items: Uncaught Error: ' + + 'localStorage database is malformed: expected schema_version to be ' + + 'an integer', + }, + ); +})().then(common.mustCall());