fix(plugins): storage paths can't reach Object.prototype, increment respects the key cap, proxy redirects stay on the whitelist

docs/guides/plugin-development.md

@@ -245,8 +245,10 @@ const existed = await HomeGlow.storage.remove('buckets:user:3');
 const { result, value } = await HomeGlow.storage.increment('give-pool', 'total', 2);
 ```
 
-Limits: 64 KB per value, 500 keys per plugin. (Storage works in demo mode too —
-it just resets with the demo cycle.)
+Limits: 64 KB per value, 500 keys per plugin; a write or an increment that
+would add key 501 answers `413`. An increment's path is dot-separated, and no
+segment may be empty or `__proto__`, `constructor` or `prototype` (`400`).
+(Storage works in demo mode too — it just resets with the demo cycle.)
 
 Raw endpoints (if you prefer `fetch`):
 
@@ -663,7 +665,7 @@ setting to `total` inside my `give-pool` storage document.*
 | --- | --- |
 | `on` | An event from the catalog. |
 | `action` | `"increment"` (the only action today). |
-| `key` / `path` | Storage document and dot-path to the number (created on demand). |
+| `key` / `path` | Storage document and dot-path to the number (created on demand). No segment may be `__proto__`, `constructor` or `prototype`. |
 | `delta` | A literal number, `{ "setting": "<key>" }` (a declared **household number** setting — resolved live at fire time), or `{ "payload": "<field>" }` (a numeric field from the event payload, e.g. `amount`). |
 | `factor` | Optional multiplier on the resolved delta (default `1`). `factor: -1` builds a **mirror reaction** that compensates an undo event. |
 
@@ -929,7 +931,10 @@ To reach an external service that lacks CORS headers, route the request through
 `GET /api/proxy?targetUrl=<encoded-url>`. Two constraints:
 
 - The target host must be in the `PROXY_WHITELIST` setting (an admin adds it) —
-  otherwise `403`.
+  otherwise `403`. A redirect is followed only to another whitelisted host, so
+  a whitelisted site can't send the proxy somewhere else on the household's
+  network; one that leads off the list answers `403`.
+- Only `http://` and `https://` addresses (`400` otherwise).
 - It is **disabled in demo mode** (so the demo can't be used as an open relay).
 
 ### Beyond this list

server/index.js

@@ -259,6 +259,25 @@ const shuffleInPlace = (array) => {
 const GITHUB_REPO_OWNER = 'jherforth';
 const GITHUB_REPO_NAME = 'HomeGlowPlugins';
 const GITHUB_API_BASE = 'https://api.github.com';
+// Where the plugin listing's download addresses point, and the only place a
+// GitHub install fetches from.
+const PLUGIN_REPOSITORY_RAW_ORIGIN = 'https://raw.githubusercontent.com';
+const PLUGIN_REPOSITORY_RAW_PATH = `/${GITHUB_REPO_OWNER}/${GITHUB_REPO_NAME}/`;
+
+// A download address in the plugin repository, as the address that will be
+// fetched, or null. Checked after parsing: fetching normalizes the URL, so
+// ".../HomeGlowPlugins/%2e%2e/%2e%2e/someone/else/..." would otherwise pass
+// a check on the raw string and load another repository's file. Encoded dots,
+// slashes and backslashes are refused outright, so nothing on GitHub's side
+// can decode them into a different path either.
+function pluginRepositoryFileUrl(raw) {
+  if (typeof raw !== 'string' || raw.length > 2048) return null;
+  let url;
+  try { url = new URL(raw); } catch { return null; }
+  if (url.origin !== PLUGIN_REPOSITORY_RAW_ORIGIN || url.username || url.password) return null;
+  if (!url.pathname.startsWith(PLUGIN_REPOSITORY_RAW_PATH) || /%2e|%2f|%5c|\\/i.test(url.pathname)) return null;
+  return url.href;
+}
 
 const DEFAULT_HOMEGLOW_REPOSITORY = 'jherforth/HomeGlow';
 const BACKEND_VERSION = (process.env.BACKEND_VERSION || process.env.APP_VERSION || serverPackageJson.version || 'dev').trim();
@@ -840,6 +859,19 @@ function pluginContentSha(content) {
 const PLUGIN_SETTING_TYPES = new Set(['number', 'string', 'boolean', 'select']);
 const PLUGIN_SETTING_SCOPES = new Set(['household', 'device']);
 
+// Segments an increment's path may never walk through: on a plain object each
+// reaches Object.prototype (or a function), so a crafted path would write into
+// every object in the server process (mrramam/HomeGlow#54).
+const UNSAFE_PATH_SEGMENTS = new Set(['__proto__', 'constructor', 'prototype']);
+// What is wrong with a split increment path, or null.
+function storagePathProblem(segments) {
+  if (segments.some((segment) => segment.length === 0)) return 'path segments must be non-empty.';
+  if (segments.some((segment) => UNSAFE_PATH_SEGMENTS.has(segment))) {
+    return 'path segments may not be __proto__, constructor or prototype.';
+  }
+  return null;
+}
+
 // Returns { manifest: object|null, errors: string[] }. A missing block is not
 // an error (legacy widget); a present-but-invalid block is, so a typo'd
 // manifest fails the upload loudly instead of silently installing as legacy.
@@ -959,8 +991,8 @@ function extractPluginManifest(htmlContent) {
           errors.push(`reactions[${index}].key must be a valid storage key.`);
         }
         if (typeof reaction.path !== 'string' || reaction.path.length === 0 ||
-            reaction.path.split('.').some((segment) => segment.length === 0)) {
-          errors.push(`reactions[${index}].path must be a non-empty dot-separated path.`);
+            storagePathProblem(reaction.path.split('.'))) {
+          errors.push(`reactions[${index}].path must be a dot-separated path of non-empty segments, none of them __proto__, constructor or prototype.`);
         }
         // Optional multiplier applied to the resolved delta — factor: -1 lets a
         // mirror reaction (e.g. on chore.uncompleted) compensate a setting- or
@@ -1394,8 +1426,17 @@ fastify.delete('/api/plugin/v1/storage/:pluginId/:key', async (request, reply) =
 // the clam-bucket siphon needs ("add N to the give pool") without a general
 // transaction API. Creates the key/path if missing. Body: { path, delta }.
 const incrementPluginStorage = (pluginId, key, pathSegments, delta) => {
+  const problem = storagePathProblem(pathSegments);
+  if (problem) throw Object.assign(new Error(problem), { statusCode: 400 });
   const row = db.prepare('SELECT value_json FROM plugin_storage WHERE plugin_id = ? AND key = ?').get(pluginId, key);
   let doc = {};
+  if (!row) {
+    // A new key counts against the cap, as a PUT does.
+    const { count } = db.prepare('SELECT COUNT(*) AS count FROM plugin_storage WHERE plugin_id = ?').get(pluginId);
+    if (count >= PLUGIN_STORAGE_MAX_KEYS) {
+      throw Object.assign(new Error(`Plugin storage is limited to ${PLUGIN_STORAGE_MAX_KEYS} keys.`), { statusCode: 413 });
+    }
+  }
   if (row) {
     try {
       doc = JSON.parse(row.value_json);
@@ -1407,9 +1448,11 @@ const incrementPluginStorage = (pluginId, key, pathSegments, delta) => {
     }
   }
 
+  // Own properties only: an inherited one is never stepped onto or written.
+  const own = (object, name) => (Object.hasOwn(object, name) ? object[name] : undefined);
   let target = doc;
   for (const segment of pathSegments.slice(0, -1)) {
-    if (target[segment] === undefined) {
+    if (own(target, segment) === undefined) {
       target[segment] = {};
     }
     if (!target[segment] || typeof target[segment] !== 'object' || Array.isArray(target[segment])) {
@@ -1419,7 +1462,7 @@ const incrementPluginStorage = (pluginId, key, pathSegments, delta) => {
   }
 
   const leaf = pathSegments[pathSegments.length - 1];
-  const current = target[leaf] === undefined ? 0 : target[leaf];
+  const current = own(target, leaf) === undefined ? 0 : target[leaf];
   if (typeof current !== 'number' || !Number.isFinite(current)) {
     throw Object.assign(new Error(`Value at path is not a number.`), { statusCode: 409 });
   }
@@ -1454,8 +1497,9 @@ fastify.post('/api/plugin/v1/storage/:pluginId/:key/increment', async (request,
     return reply.status(400).send({ error: 'delta must be a finite number.' });
   }
   const pathSegments = incrementPath.split('.');
-  if (pathSegments.some((segment) => segment.length === 0)) {
-    return reply.status(400).send({ error: 'path segments must be non-empty.' });
+  const pathProblem = storagePathProblem(pathSegments);
+  if (pathProblem) {
+    return reply.status(400).send({ error: pathProblem });
   }
 
   try {
@@ -2015,16 +2059,23 @@ fastify.post('/api/widgets/github/install', async (request, reply) => {
     if (!download_url || !filename) {
       return reply.status(400).send({ error: 'download_url and filename are required' });
     }
+    // Only a file in the plugin repository, as the listing gives it: the
+    // server fetches this address, so it must not be one a caller chooses.
+    const downloadUrl = pluginRepositoryFileUrl(download_url);
+    if (!downloadUrl) {
+      return reply.status(400).send({ error: `download_url must be a file in ${GITHUB_REPO_OWNER}/${GITHUB_REPO_NAME} on raw.githubusercontent.com` });
+    }
 
     console.log(`Installing widget ${filename} from GitHub...`);
 
     // Download the widget file
-    const response = await axios.get(download_url, {
+    const response = await axios.get(downloadUrl, {
       headers: {
         'Accept': 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
         'User-Agent': 'HomeGlow-Server/1.0'
       },
-      timeout: 15000
+      timeout: 15000,
+      maxRedirects: 0,
     });
 
     // Sanitize filename
@@ -2038,7 +2089,7 @@ fastify.post('/api/widgets/github/install', async (request, reply) => {
       fallbackName: name || sanitizedFilename.replace('.html', ''),
       content,
       source: 'github',
-      originalUrl: download_url,
+      originalUrl: downloadUrl,
     });
     if (result.error) {
       return reply.status(result.status).send({ error: result.error });
@@ -2056,15 +2107,11 @@ fastify.post('/api/widgets/github/install', async (request, reply) => {
   } catch (error) {
     console.error('Error installing GitHub widget:', error);
     if (error.response) {
-      return reply.status(error.response.status).send({
-        error: `Failed to download widget: ${error.response.status} ${error.response.statusText}`,
-        details: error.response.data
+      return reply.status(502).send({
+        error: `Failed to download widget: GitHub answered ${error.response.status} ${error.response.statusText}`,
       });
     }
-    return reply.status(500).send({
-      error: 'Failed to install widget from GitHub',
-      details: error.message
-    });
+    return reply.status(500).send({ error: 'Failed to install widget from GitHub' });
   }
 });
 
@@ -5168,11 +5215,17 @@ fastify.get('/api/proxy', async (request, reply) => {
     whitelist = []; // Default to empty for security
   }
 
+  // Set when a redirect leads off the whitelist, to answer 403 for it.
+  let blockedRedirect = null;
   try {
     const target = new URL(targetUrl);
     const targetHostname = target.hostname;
     console.log('Target hostname:', targetHostname);
 
+    if (!['http:', 'https:'].includes(target.protocol)) {
+      return reply.status(400).send({ error: 'The proxy only fetches http:// and https:// addresses.' });
+    }
+
     if (!whitelist.includes(targetHostname)) {
       console.warn(`Proxy request blocked for non-whitelisted domain: ${targetHostname}`);
       return reply.status(403).send({ error: 'Access to this domain is not allowed through the proxy.' });
@@ -5191,6 +5244,16 @@ fastify.get('/api/proxy', async (request, reply) => {
         'Accept-Encoding': 'gzip, deflate'
       },
       maxRedirects: 5,
+      // A whitelisted host may redirect, but only to another whitelisted one:
+      // otherwise any of them could send the proxy to an address on the
+      // household's own network.
+      beforeRedirect: (options) => {
+        const hop = String(options.hostname || '').replace(/^\[|\]$/g, '');
+        if (!['http:', 'https:'].includes(options.protocol) || !whitelist.includes(hop)) {
+          blockedRedirect = hop || String(options.protocol);
+          throw new Error(`Redirect to ${blockedRedirect} is not on the proxy whitelist.`);
+        }
+      },
       validateStatus: function (status) {
         return status < 500; // Resolve only if the status code is less than 500
       },
@@ -5222,6 +5285,10 @@ fastify.get('/api/proxy', async (request, reply) => {
     return reply.status(response.status).send(response.data);
 
   } catch (error) {
+    if (blockedRedirect) {
+      console.warn(`Proxy redirect blocked: ${blockedRedirect} is not whitelisted`);
+      return reply.status(403).send({ error: 'The target redirected to a domain that is not allowed through the proxy.' });
+    }
     console.error('Error in proxy request:', error.message);
     console.error('Full error details:', {
       message: error.message,

server/tests/outboundTls.test.js

@@ -134,6 +134,9 @@ async function usePort() {
 let serverProcess;
 let serverLogs = '';
 let originHttp;
+let innerHttp;
+let innerPort;
+let innerHits = 0;
 let originPort;
 
 const delay = (ms) => new Promise((resolve) => setTimeout(resolve, ms));
@@ -154,7 +157,24 @@ test.before(async () => {
 
     // A plain-HTTP origin standing in for a LAN service, which is how a great
     // many HomeGlow installs actually talk to Immich or Home Assistant.
+    // A second service the proxy must never reach: it is not on the
+    // whitelist, so a redirect to it is refused (mrramam/HomeGlow#56).
+    innerHttp = http.createServer((req, res) => {
+        innerHits += 1;
+        res.end('internal');
+    });
+    await new Promise((resolve) => innerHttp.listen(0, '127.0.0.1', resolve));
+    innerPort = innerHttp.address().port;
+
     originHttp = http.createServer((req, res) => {
+        if (req.url === '/redirect-out') {
+            res.writeHead(302, { Location: `http://localhost:${innerPort}/secret` });
+            return res.end();
+        }
+        if (req.url === '/redirect-in') {
+            res.writeHead(302, { Location: `http://127.0.0.1:${originPort}/hello` });
+            return res.end();
+        }
         res.setHeader('Content-Type', 'application/json');
         res.end(JSON.stringify({ ok: true, path: req.url }));
     });
@@ -201,6 +221,7 @@ test.after(async () => {
         });
     }
     if (originHttp) await new Promise((resolve) => originHttp.close(resolve));
+    if (innerHttp) await new Promise((resolve) => innerHttp.close(resolve));
     if (!keepTestArtifacts) {
         for (const suffix of ['', '-shm', '-wal', '-journal']) {
             const p = `${testDbPath}${suffix}`;
@@ -209,6 +230,24 @@ test.after(async () => {
     }
 });
 
+test('a redirect off the whitelist is refused, and never followed', async () => {
+    const result = await api(`/api/proxy?targetUrl=${encodeURIComponent(`http://127.0.0.1:${originPort}/redirect-out`)}`);
+    assert.equal(result.status, 403);
+    assert.match(result.body.error, /redirected to a domain that is not allowed/);
+    assert.equal(innerHits, 0, 'the off-whitelist host was never asked');
+});
+
+test('a redirect to another whitelisted address is followed', async () => {
+    const result = await api(`/api/proxy?targetUrl=${encodeURIComponent(`http://127.0.0.1:${originPort}/redirect-in`)}`);
+    assert.equal(result.status, 200);
+    assert.equal(result.body.path, '/hello');
+});
+
+test('the proxy fetches only http and https addresses', async () => {
+    const result = await api(`/api/proxy?targetUrl=${encodeURIComponent('ftp://127.0.0.1/file')}`);
+    assert.equal(result.status, 400);
+});
+
 test('proxying a plain-HTTP LAN target still works', async () => {
     const result = await api(`/api/proxy?targetUrl=${encodeURIComponent(`http://127.0.0.1:${originPort}/hello`)}`);
     assert.equal(result.status, 200);

server/tests/pluginPlatform.test.js

@@ -340,6 +340,86 @@ test('increment rejects bad input and non-numeric targets', async () => {
     assert.equal(nonNumeric.status, 409);
 });
 
+// mrramam/HomeGlow#54: a path through __proto__ reached Object.prototype.
+test('increment refuses paths through __proto__, constructor or prototype', async () => {
+    for (const crafted of ['__proto__.pwned', 'constructor.prototype.pwned', 'a.__proto__.pwned', 'prototype']) {
+        const res = await api(`/api/plugin/v1/storage/${PLUGIN_ID}/proto-probe/increment`, {
+            method: 'POST',
+            body: JSON.stringify({ path: crafted, delta: 1 }),
+        });
+        assert.equal(res.status, 400, crafted);
+        assert.match(res.body.error, /__proto__, constructor or prototype/);
+    }
+    // Nothing was stored, and an ordinary increment on the key still works.
+    assert.equal((await api(`/api/plugin/v1/storage/${PLUGIN_ID}/proto-probe`)).status, 404);
+    const ok = await api(`/api/plugin/v1/storage/${PLUGIN_ID}/proto-probe/increment`, {
+        method: 'POST',
+        body: JSON.stringify({ path: 'count', delta: 1 }),
+    });
+    assert.equal(ok.status, 200);
+    assert.deepEqual(ok.body.value, { count: 1 });
+});
+
+// mrramam/HomeGlow#56: the install fetched any address it was handed.
+test('a GitHub install fetches only from the plugin repository', async () => {
+    for (const download_url of [
+        'http://127.0.0.1:1/plugin.html',
+        'https://raw.githubusercontent.com/someone-else/HomeGlowPlugins/main/x.html',
+        'https://raw.githubusercontent.com.evil.example/jherforth/HomeGlowPlugins/main/x.html',
+        'file:///etc/passwd',
+        // Normalized when fetched: these would leave the repository (Mrramam, #319).
+        'https://raw.githubusercontent.com/jherforth/HomeGlowPlugins/%2e%2e/%2e%2e/someone-else/repo/main/x.html',
+        'https://raw.githubusercontent.com/jherforth/HomeGlowPlugins/../../someone-else/repo/main/x.html',
+        'https://raw.githubusercontent.com/jherforth/HomeGlowPlugins/main/%2E%2E%2F%2E%2E%2Fother/x.html',
+        'https://raw.githubusercontent.com/jherforth/HomeGlowPlugins/main/a%5c..%5cx.html',
+        'https://user:[email protected]/jherforth/HomeGlowPlugins/main/x.html',
+        'https://raw.githubusercontent.com:8443/jherforth/HomeGlowPlugins/main/x.html',
+    ]) {
+        const res = await api('/api/widgets/github/install', {
+            method: 'POST',
+            body: JSON.stringify({ download_url, filename: 'x.html' }),
+        });
+        assert.equal(res.status, 400, download_url);
+        assert.match(res.body.error, /jherforth\/HomeGlowPlugins on raw\.githubusercontent\.com/);
+    }
+});
+
+test('a manifest reaction may not name an unsafe path', async () => {
+    const manifest = {
+        ...validManifest,
+        id: 'proto-reaction-plugin',
+        reactions: [{ on: 'clam.deposited', action: 'increment', key: 'bank', path: '__proto__.pwned', delta: 1 }],
+    };
+    const upload = await uploadWidget('proto-reaction.html', widgetHtml(manifest));
+    assert.equal(upload.status, 400);
+    assert.match(upload.body.error, /reactions\[0\]\.path/);
+});
+
+// mrramam/HomeGlow#60: increment added keys past the 500-key cap PUT enforces.
+test('increment counts a new key against the 500-key cap', async () => {
+    const capId = 'storage-cap-plugin';
+    const upload = await uploadWidget('storage-cap.html', widgetHtml({ ...validManifest, id: capId }));
+    assert.equal(upload.status, 200, JSON.stringify(upload.body));
+    const Database = require('better-sqlite3');
+    const db = new Database(testDbPath);
+    const insert = db.prepare('INSERT INTO plugin_storage (plugin_id, key, value_json) VALUES (?, ?, ?)');
+    db.transaction(() => { for (let i = 0; i < 500; i += 1) insert.run(capId, `k${i}`, '{}'); })();
+    db.close();
+
+    const over = await api(`/api/plugin/v1/storage/${capId}/one-too-many/increment`, {
+        method: 'POST',
+        body: JSON.stringify({ path: 'n', delta: 1 }),
+    });
+    assert.equal(over.status, 413);
+    assert.match(over.body.error, /limited to 500 keys/);
+    // A key that already exists can still be incremented.
+    const existing = await api(`/api/plugin/v1/storage/${capId}/k1/increment`, {
+        method: 'POST',
+        body: JSON.stringify({ path: 'n', delta: 1 }),
+    });
+    assert.equal(existing.status, 200);
+});
+
 // --- Declared settings (Phase 2) ---
 
 const SETTINGS_PLUGIN_ID = 'settings-test-plugin';