src: throw on a malformed localStorage file

src/inspector/dom_storage_agent.cc

@@ -102,7 +102,16 @@ 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() || !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-inspector-dom-storage-malformed.js

@@ -0,0 +1,87 @@
+// 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}`,
+  ], 'setTimeout(() => {}, 10000);');
+
+  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 {
+    session.disconnect();
+    instance.kill();
+  }
+}
+
+(async () => {
+  // A wrong-typed value is rejected by Storage::GetAll() itself.
+  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.
+  await assert.rejects(
+    getDOMStorageItems(
+      malformedLocalStorage(
+        'bad-schema-version.db', 'one', Buffer.from('hello', 'utf16le'))),
+    { message: 'Could not read DOM storage items' },
+  );
+})().then(common.mustCall());