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: #65879
Fixes: #65878
Fixes: #64640
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Author: TrevorBurnham

src/inspector/dom_storage_agent.cc

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

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;
 }
 
@@ -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*>(

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,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)

test/parallel/test-inspector-dom-storage-malformed.js

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