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

Author: TrevorBurnham

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();
     }
   }

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> 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,20 +310,28 @@ 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 =
       "SELECT key, value FROM nodejs_webstorage";
   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;
 }
 
@@ -351,7 +390,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 +427,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*>(

src/node_webstorage.h

@@ -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,9 @@ 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.
+  std::optional<std::unordered_map<std::u16string, std::u16string>> GetAll();
 
   SET_MEMORY_INFO_NAME(Storage)
   SET_SELF_SIZE(Storage)

test/parallel/test-webstorage.js

@@ -1,11 +1,12 @@
 'use strict';
 
-const { skipIfSQLiteMissing, spawnPromisified } = require('../common');
+const { isWindows, 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 { readdir, writeFile } = require('node:fs/promises');
+const { DatabaseSync } = require('node:sqlite');
 const { test, describe } = require('node:test');
 let cnt = 0;
 
@@ -146,3 +147,133 @@ 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 = nextLocalStorage();
+    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', async () => {
+  // Needs `ulimit` to hold the descriptor limit down; Node raises its own soft
+  // limit at startup, so an unconstrained run would not notice the leak.
+  if (isWindows) return;
+
+  const file = nextLocalStorage();
+  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 limit and degraded the error into a
+  // misleading "unable to open database file".
+  const script = join(tmpdir.path, 'reopen.js');
+  await writeFile(script, `
+    const assert = require('assert');
+    for (let i = 0; i < 200; i++) {
+      assert.throws(() => localStorage.length, {
+        code: 'ERR_INVALID_STATE',
+        message: /expected schema_version to be an integer/,
+      });
+    }
+  `);
+
+  const cp = await spawnPromisified('/bin/sh', [
+    '-c',
+    `ulimit -n 96 && exec '${process.execPath}' ` +
+    `--localstorage-file='${file}' '${script}'`,
+  ]);
+  assert.strictEqual(cp.stderr, '');
+  assert.strictEqual(cp.code, 0);
+});