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 <trevorburnham@gmail.com>
Assisted-by: Claude Opus 5
PR-URL: https://github.com/nodejs/node/pull/65879
Fixes: https://github.com/nodejs/node/issues/65878
Fixes: https://github.com/nodejs/node/issues/64640
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This commit is contained in:
Trevor Burnham authored and GitHub committed 2026-09-26 02:30:46 +00:00
1 parent f2b698f1cf
commit 8812357aff
5 files changed
+358 -17

No files matched your search

+32 -1
View File
@@ -101,11 +101,42 @@ protocol::DispatchResponse DOMStorageAgent::getDOMStorageItems(
std::optional<StorageMap> storage_map_fallback;
if (storage_map->empty()) {
auto web_storage_obj = getWebStorage(is_local_storage);
// Each way of failing below says something different about the store, so
// each reports a different reason. A frontend that cannot read a store
// otherwise has no way to tell a missing one from a corrupt one.
if (!web_storage_obj) {
return protocol::DispatchResponse::ServerError(
"Could not read DOM storage items");
"Could not read DOM storage items: storage is unavailable");
}
// 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<v8::Message> 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());
}
// V8 builds that Message on a best-effort basis, so the throw is all we
// can report when it is missing.
return protocol::DispatchResponse::ServerError(
"Could not read DOM storage items: the backing store could not be "
"opened");
}
if (!storage_map_fallback.has_value()) {
return protocol::DispatchResponse::ServerError(
"Could not read DOM storage items: the backing file is malformed");
}
storage_map = &storage_map_fallback.value();
}
+62 -11
View File
@@ -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> context) {
Isolate* isolate = Isolate::GetCurrent();
auto quota_exceeded_str =
@@ -173,6 +186,12 @@ Maybe<void> 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<void>());
r = sqlite3_exec(db, init_sql_v0.data(), nullptr, nullptr, nullptr);
CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
@@ -184,12 +203,16 @@ Maybe<void> 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<void>());
auto stmt = stmt_unique_ptr(s);
CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
CHECK_ERROR_OR_THROW(
env(), sqlite3_step(stmt.get()), SQLITE_ROW, Nothing<void>());
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<void>());
int schema_version = sqlite3_column_int(stmt.get(), 0);
stmt = nullptr; // Force finalization.
@@ -209,7 +232,7 @@ Maybe<void> Storage::Open() {
CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
}
db_ = conn_unique_ptr(db);
db_ = std::move(conn);
return JustVoid();
}
@@ -266,7 +289,12 @@ MaybeLocal<Array> Storage::Enumerate() {
LocalVector<Value> values(env()->isolate());
Local<Value> 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<Array>());
auto size = sqlite3_column_bytes(stmt.get(), 0) / sizeof(uint16_t);
if (!String::NewFromTwoByte(env()->isolate(),
reinterpret_cast<const uint16_t*>(
@@ -282,9 +310,10 @@ MaybeLocal<Array> Storage::Enumerate() {
return Array::New(env()->isolate(), values.data(), values.size());
}
std::unordered_map<std::u16string, std::u16string> Storage::GetAll() {
std::optional<std::unordered_map<std::u16string, std::u16string>>
Storage::GetAll() {
if (!Open().IsJust()) {
return {};
return std::nullopt;
}
static constexpr std::string_view sql =
@@ -292,10 +321,17 @@ std::unordered_map<std::u16string, std::u16string> 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<std::u16string, std::u16string> 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<std::u16string, std::u16string> Storage::GetAll() {
result.emplace(std::move(key), std::move(value));
}
if (r != SQLITE_DONE) {
return std::nullopt;
}
return result;
}
@@ -324,6 +363,8 @@ MaybeLocal<Value> Storage::Length() {
auto stmt = stmt_unique_ptr(s);
CHECK_ERROR_OR_THROW(
env(), sqlite3_step(stmt.get()), SQLITE_ROW, Local<Value>());
// 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<Value> Storage::Load(Local<Name> key) {
CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Local<Value>());
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<Value>());
auto size = sqlite3_column_bytes(stmt.get(), 0) / sizeof(uint16_t);
return String::NewFromTwoByte(env()->isolate(),
reinterpret_cast<const uint16_t*>(
@@ -383,7 +429,12 @@ MaybeLocal<Value> 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<Value>());
auto size = sqlite3_column_bytes(stmt.get(), 0) / sizeof(uint16_t);
return String::NewFromTwoByte(env()->isolate(),
reinterpret_cast<const uint16_t*>(
+7 -1
View File
@@ -3,6 +3,7 @@
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
#include <optional>
#include <unordered_map>
#include "base_object.h"
#include "node_mem.h"
@@ -41,7 +42,12 @@ class Storage : public BaseObject {
v8::MaybeLocal<v8::Value> LoadKey(const int index);
v8::Maybe<void> Remove(v8::Local<v8::Name> key);
v8::Maybe<void> Store(v8::Local<v8::Name> key, v8::Local<v8::Value> value);
std::unordered_map<std::u16string, std::u16string> 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<std::unordered_map<std::u16string, std::u16string>> GetAll();
SET_MEMORY_INFO_NAME(Storage)
SET_SELF_SIZE(Storage)
@@ -0,0 +1,101 @@
// 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 { once } = require('node:events');
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}`,
], 'console.log("ready"); setInterval(() => {}, 1000);');
// The inspector accepts connections before pre-execution defines
// globalThis.localStorage, and a command that arrives first reports the
// store as unavailable.
const ready = once(instance, 'stdout');
const session = await instance.connectInspectorSession();
await ready;
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 opens
// the file successfully and has no exception to report.
await assert.rejects(
getDOMStorageItems(
malformedLocalStorage('bad-value.db', 1, 'hello')),
{ message: 'Could not read DOM storage items: the backing file is malformed' },
);
// 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());
+156 -4
View File
@@ -1,11 +1,15 @@
'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 { endianness } = require('node:os');
const { DatabaseSync } = require('node:sqlite');
const { test, describe } = require('node:test');
let cnt = 0;
@@ -15,6 +19,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 +60,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 +88,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 +99,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 +160,141 @@ 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 as UTF-16 code units in the platform's byte order, so a
// key only matches a lookup if it is encoded the same way.
const utf16 = (str) => {
const buf = Buffer.from(str, 'utf16le');
return endianness() === 'BE' ? buf.swap16() : buf;
};
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, '');
});