diff --git a/docs/guides/plugin-development.md b/docs/guides/plugin-development.md index 29560dda..29a836e7 100644 --- a/docs/guides/plugin-development.md +++ b/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": "" }` (a declared **household number** setting — resolved live at fire time), or `{ "payload": "" }` (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=`. 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 diff --git a/server/index.js b/server/index.js index 3112db2e..9bccb7ac 100644 --- a/server/index.js +++ b/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, diff --git a/server/tests/outboundTls.test.js b/server/tests/outboundTls.test.js index dff79ece..5e1e3826 100644 --- a/server/tests/outboundTls.test.js +++ b/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); diff --git a/server/tests/pluginPlatform.test.js b/server/tests/pluginPlatform.test.js index 5f513dbb..90709b66 100644 --- a/server/tests/pluginPlatform.test.js +++ b/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:pass@raw.githubusercontent.com/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';