From 5a157197b6ea166d5c5c04cc1d2816bcf9cc05f9 Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Thu, 30 Jul 2026 01:42:40 -0700 Subject: [PATCH] fix: PUT-1398 (#3478) --- .../controllers/apps/AppController.test.ts | 11 +- src/backend/drivers/apps/AppDriver.js | 38 ++- src/backend/drivers/apps/AppDriver.test.ts | 142 +++++++++ src/backend/util/appIcon.ts | 292 ++++++++++++++---- src/dev-center/js/apps.js | 12 +- 5 files changed, 414 insertions(+), 81 deletions(-) diff --git a/src/backend/controllers/apps/AppController.test.ts b/src/backend/controllers/apps/AppController.test.ts index 44e872b1c..33c849873 100644 --- a/src/backend/controllers/apps/AppController.test.ts +++ b/src/backend/controllers/apps/AppController.test.ts @@ -92,6 +92,11 @@ const uniqueName = (prefix: string) => const uniqueIndexUrl = () => `https://example-${Math.random().toString(36).slice(2, 10)}.test/`; +// 1x1 transparent PNG. The `icon` write path validates the decoded payload, +// not just the declared MIME, so icon fixtures must be real images. +const MINIMAL_PNG_BASE64 = + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII='; + const createApp = async ( actor: Actor, overrides: Record = {}, @@ -718,7 +723,9 @@ describe('AppController GET /app-icon/:app_uid', () => { it('decodes a data URL icon and serves the declared MIME', async () => { const owner = await makeUser(); - const png = Buffer.from('mock-png-bytes'); + // Real PNG bytes: the write path sniffs the decoded payload and + // rejects anything that isn't the image type it claims to be. + const png = Buffer.from(MINIMAL_PNG_BASE64, 'base64'); const dataUrl = `data:image/png;base64,${png.toString('base64')}`; const app = await createApp(owner.actor, { icon: dataUrl }); @@ -740,7 +747,7 @@ describe('AppController GET /app-icon/:app_uid', () => { // than the redirect path that serves the same resource. it('caches an inline data-URL icon as long as the redirect path', async () => { const owner = await makeUser(); - const dataUrl = `data:image/png;base64,${Buffer.from('x').toString('base64')}`; + const dataUrl = `data:image/png;base64,${MINIMAL_PNG_BASE64}`; const app = await createApp(owner.actor, { icon: dataUrl }); const { res, captured } = makeRes(); diff --git a/src/backend/drivers/apps/AppDriver.js b/src/backend/drivers/apps/AppDriver.js index cedb2af0a..74e9f7158 100644 --- a/src/backend/drivers/apps/AppDriver.js +++ b/src/backend/drivers/apps/AppDriver.js @@ -25,10 +25,10 @@ import { DEFAULT_TEMP_SUBSCRIPTION, } from '../../services/metering/consts.js'; import { - ICON_DATA_URL_MIME_ALLOWLIST, isAppIconEndpointUrl, isRawBase64ImageString, normalizeRawBase64ImageString, + validateIconDataUrl, } from '../../util/appIcon.js'; import { buildHostedBackingDenial, @@ -582,8 +582,10 @@ export class AppDriver extends PuterDriver { // Accepted shapes (mirrors v1's `image-base64` proptype so // puter-js callers keep working): // 1. Empty string — unset - // 2. Raw base64 (no prefix) — normalized to a PNG data URL - // 3. `data:image/;…` with an allow-listed MIME + // 2. Raw base64 (no prefix) of a real image — normalized to a + // data URL carrying the sniffed MIME + // 3. `data:image/;base64,` with an allow-listed + // MIME that matches the decoded payload // 4. `/app-icon/` endpoint URL (relative, or absolute // on a host we control) // Anything else (including arbitrary http(s) URLs) is rejected: @@ -594,24 +596,20 @@ export class AppDriver extends PuterDriver { // Raw base64 → wrap as data URL (v1 parity) if (isRawBase64ImageString(iconStr)) { iconStr = normalizeRawBase64ImageString(iconStr); - } else if (iconStr.startsWith('data:')) { - const semi = iconStr.indexOf(';'); - const comma = iconStr.indexOf(','); - const mimeEnd = - semi !== -1 && (comma === -1 || semi < comma) - ? semi - : comma; - const mime = - mimeEnd !== -1 - ? iconStr.slice(5, mimeEnd).toLowerCase() - : ''; - if (!ICON_DATA_URL_MIME_ALLOWLIST.includes(mime)) { - throw new HttpError( - 400, - '`icon` data URL must use an image MIME type', - { legacyCode: 'bad_request' }, - ); + } + if (iconStr.startsWith('data:')) { + // Validates the whole URL, payload included — a MIME + // prefix check alone let arbitrary text (quotes, tags) + // through, which a Dev Center template then interpolated + // into markup. + const verdict = validateIconDataUrl(iconStr); + if (!verdict.ok) { + throw new HttpError(400, `\`icon\` ${verdict.reason}`, { + legacyCode: 'bad_request', + }); } + // Store the canonical form, not the caller's spelling. + iconStr = verdict.normalized; } else if (!isAppIconEndpointUrl(iconStr, this.config)) { throw new HttpError( 400, diff --git a/src/backend/drivers/apps/AppDriver.test.ts b/src/backend/drivers/apps/AppDriver.test.ts index f331247eb..c0cb619ff 100644 --- a/src/backend/drivers/apps/AppDriver.test.ts +++ b/src/backend/drivers/apps/AppDriver.test.ts @@ -607,6 +607,148 @@ describe('AppDriver.create additional branches', () => { expect(created.icon).toBe(png); }); + // The write path once validated only the MIME prefix, so an + // allow-listed prefix plus arbitrary text was stored verbatim and later + // interpolated into a Dev Center template — stored XSS in a godmode app + // that carries the user's session token. Reachable with an app-under-user + // token, the lowest-privilege credential we issue. + describe('icon data URL payload validation', () => { + const ATTACK_PAYLOAD = + 'data:image/png;base64,iVBORw0KGgo=" a5x="1">'; + + const createWithIcon = async (icon: string, label: string) => { + const { actor } = await makeUser(); + return withActor(actor, () => + driver.create({ + object: { + name: uniqueName(label), + title: 't', + index_url: uniqueIndexUrl(), + icon, + }, + }), + ); + }; + + it('rejects the reported breakout payload', async () => { + await expect( + createWithIcon(ATTACK_PAYLOAD, 'xss-icon'), + ).rejects.toMatchObject({ statusCode: 400 }); + }); + + it('rejects the breakout payload on update, not just create', async () => { + const { actor } = await makeUser(); + const created = await withActor(actor, () => + driver.create({ + object: { + name: uniqueName('xss-upd'), + title: 't', + index_url: uniqueIndexUrl(), + }, + }), + ); + await expect( + withActor(actor, () => + driver.update({ + uid: created.uid, + object: { icon: ATTACK_PAYLOAD }, + }), + ), + ).rejects.toMatchObject({ statusCode: 400 }); + }); + + it('rejects an allow-listed MIME whose payload is not an image', async () => { + // Valid base64, decodes cleanly — just isn't a PNG. + const notAnImage = `data:image/png;base64,${Buffer.from( + 'not an image at all', + ).toString('base64')}`; + await expect( + createWithIcon(notAnImage, 'notimg-icon'), + ).rejects.toMatchObject({ statusCode: 400 }); + }); + + it('rejects a payload whose bytes contradict the declared MIME', async () => { + const pngBytes = + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII='; + await expect( + createWithIcon( + `data:image/gif;base64,${pngBytes}`, + 'mismatch-icon', + ), + ).rejects.toMatchObject({ statusCode: 400 }); + }); + + it('rejects a percent-encoded (non-base64) data URL', async () => { + // The only shape that can carry literal `<` and `"`. + await expect( + createWithIcon( + 'data:image/svg+xml,%3Csvg%20xmlns%3D%22http%3A//www.w3.org/2000/svg%22%3E%3C/svg%3E', + 'pct-icon', + ), + ).rejects.toMatchObject({ statusCode: 400 }); + }); + + it('rejects base64 with smuggled non-base64 characters', async () => { + // `Buffer.from(…,'base64')` silently drops these; the + // round-trip check is what catches them. + await expect( + createWithIcon( + 'data:image/png;base64,iVBORw0KGgo=', + 'smuggle-icon', + ), + ).rejects.toMatchObject({ statusCode: 400 }); + }); + + it('accepts a base64 SVG icon and stores it canonically', async () => { + const svg = Buffer.from( + '', + ).toString('base64'); + const created = await createWithIcon( + `data:image/svg+xml;base64,${svg}`, + 'svg-icon', + ); + expect(created.icon).toBe(`data:image/svg+xml;base64,${svg}`); + }); + + it('accepts image/jpg as an alias of image/jpeg', async () => { + // Minimal JPEG SOI + APP0 header — enough to sniff. + const jpeg = Buffer.concat([ + Buffer.from([0xff, 0xd8, 0xff, 0xe0]), + Buffer.from('0000JFIF'), + ]).toString('base64'); + const created = await createWithIcon( + `data:image/jpg;base64,${jpeg}`, + 'jpg-icon', + ); + expect(String(created.icon).startsWith('data:image/jpg;base64,')).toBe( + true, + ); + }); + + it('strips line wrapping from an otherwise valid payload', async () => { + const png = + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII='; + const wrapped = `${png.slice(0, 40)}\n${png.slice(40)}`; + const created = await createWithIcon( + `data:image/png;base64,${wrapped}`, + 'wrapped-icon', + ); + expect(created.icon).toBe(`data:image/png;base64,${png}`); + }); + + it('rejects raw base64 that does not decode to an image', async () => { + // v1 wrapped any base64 as image/png regardless of content. + await expect( + createWithIcon( + Buffer.from('definitely not an image payload').toString( + 'base64', + ), + 'rawtext-icon', + ), + ).rejects.toMatchObject({ statusCode: 400 }); + }); + }); + it('normalizes a raw-base64 icon into a data: URL', async () => { const { actor } = await makeUser(); // Raw base64 of a 1x1 PNG (no data: prefix) diff --git a/src/backend/util/appIcon.ts b/src/backend/util/appIcon.ts index 34ef9b21f..c8637e43d 100644 --- a/src/backend/util/appIcon.ts +++ b/src/backend/util/appIcon.ts @@ -3,18 +3,19 @@ * * This file is part of Puter. * - * Puter is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published - * by the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. + * Puter is free software: you can redistribute it and/or modify it under the + * terms of the GNU Affero General Public License as published by the Free + * Software Foundation, either version 3 of the License, or (at your option) any + * later version. * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more + * details. * * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . + * along with this program. If not, see + * [https://www.gnu.org/licenses/](https://www.gnu.org/licenses/). */ // Always routes through the backend `/app-icon//` endpoint rather @@ -56,6 +57,179 @@ interface TrustedIconHostConfig { } const RAW_BASE64_REGEX = /^[A-Za-z0-9+/]+={0,2}$/; +const BASE64_CHARS_REGEX = /^[A-Za-z0-9+/]*={0,2}$/; +// `data:/[;param[=value]]…,`. The parameter list is +// matched as a group of its own so it can be checked exhaustively — the +// previous prefix-scan only looked at the bytes before the first `;` or `,` +// and never inspected the payload at all. +const DATA_URL_REGEX = + /^data:([a-z0-9][a-z0-9!#$&^_.+-]*\/[a-z0-9][a-z0-9!#$&^_.+-]*)((?:;[a-z0-9!#$&^_.+-]+(?:=[^;,]*)?)*),([\s\S]*)$/i; +// Cap on how far into a payload we look for the `.png (original) @@ -64,51 +238,62 @@ const APP_ICON_ENDPOINT_PATH_REGEX = /^\/app-icon\/[^/?#]+(?:\/\d+)?\/?$/; const APP_ICON_SUBDOMAIN_PATH_REGEX = /^\/app-[A-Za-z0-9_-]+(?:-\d+)?\.png$/; /** - * v1-compatible raw-base64 detector. Legacy puter-js callers pass the - * base64 payload without a `data:` prefix; v1 accepted it and normalized - * to `data:image/png;base64,` before storage. We mirror that here - * so clients that worked on v1 keep working. - * - * Rejects anything shorter than 16 chars, not aligned to base64 length, - * or that doesn't round-trip through Buffer — catches random strings - * that happen to match the charset. + * Decode a bare base64 string and sniff it, or null if it isn't base64 at all + * or doesn't decode to a recognized image. */ -export function isRawBase64ImageString(value: unknown): value is string { - if (typeof value !== 'string') return false; +function sniffRawBase64Image( + value: unknown, +): { bytes: Buffer; mime: string; compact: string } | null { + if (typeof value !== 'string') return null; const trimmed = value.trim(); - if (trimmed.length < 16) return false; - if (!RAW_BASE64_REGEX.test(trimmed)) return false; - if (trimmed.length % 4 !== 0) return false; - try { - const decoded = Buffer.from(trimmed, 'base64'); - if (decoded.length === 0) return false; - const stripped = trimmed.replace(/=+$/, ''); - const reencoded = decoded.toString('base64').replace(/=+$/, ''); - return stripped === reencoded; - } catch { - return false; - } + if (trimmed.length < 16) return null; + if (!RAW_BASE64_REGEX.test(trimmed)) return null; + const bytes = decodeStrictBase64(trimmed); + if (!bytes) return null; + const mime = sniffImageMime(bytes); + if (!mime) return null; + return { bytes, mime, compact: trimmed }; } -/** Wrap raw base64 in a `data:image/png;base64,…` URL; pass other values through. */ +/** + * V1-compatible raw-base64 detector. Legacy puter-js callers pass the base64 + * payload without a `data:` prefix; v1 accepted it and normalized to + * `data:image/png;base64,` before storage. We mirror that here so clients + * that worked on v1 keep working. + * + * Rejects anything shorter than 16 chars, not aligned to base64 length, or that + * doesn't round-trip through Buffer — catches random strings that happen to + * match the charset. Also rejects base64 that decodes to something other than a + * recognized image: v1 wrapped any base64 as `image/png` regardless of content, + * which let non-image bytes into the icon column under an image MIME type. + */ +export function isRawBase64ImageString(value: unknown): value is string { + return sniffRawBase64Image(value) !== null; +} + +/** + * Wrap raw base64 in a `data:;base64,…` URL; pass other values + * through unchanged (the caller then rejects them — a bare string that isn't a + * valid image is not one of the accepted `icon` shapes). + */ export function normalizeRawBase64ImageString(value: string): string { - const trimmed = value.trim(); - if (!isRawBase64ImageString(trimmed)) return value; - return `data:image/png;base64,${trimmed}`; + const sniffed = sniffRawBase64Image(value); + if (!sniffed) return value; + return `data:${sniffed.mime};base64,${sniffed.compact}`; } /** * Whether `value` is a reference we own — accepts two shapes: - * - `/app-icon/(/)?` : the AppController endpoint - * - `/app-(-)?.png` : the file written by - * AppIconService onto the - * `puter-app-icons` subdomain * - * Relative paths must use the endpoint shape (the subdomain-file shape - * is only meaningful when paired with a trusted host). Absolute URLs - * accept either shape but only on a trusted host — without the host - * check an authenticated user could set `icon` to an attacker URL and - * turn `/app-icon/:uid` into a Puter-branded open redirector. + * - `/app-icon/(/)?` : the AppController endpoint + * - `/app-(-)?.png` : the file written by AppIconService onto the + * `puter-app-icons` subdomain + * + * Relative paths must use the endpoint shape (the subdomain-file shape is only + * meaningful when paired with a trusted host). Absolute URLs accept either + * shape but only on a trusted host — without the host check an authenticated + * user could set `icon` to an attacker URL and turn `/app-icon/:uid` into a + * Puter-branded open redirector. */ export function isAppIconEndpointUrl( value: string, @@ -137,17 +322,18 @@ export function isAppIconEndpointUrl( /** * Whether `url` points at a host we control for app-icon hosting. * - * Used to gate both the legacy redirect fallback in `/app-icon/:uid` and - * the write-path validator in AppDriver — without this check an - * authenticated user can set `icon` to an arbitrary attacker URL and - * turn the unauthenticated `/app-icon/:uid` route into a Puter-branded - * open redirector (cached publicly for 15 minutes). + * Used to gate both the legacy redirect fallback in `/app-icon/:uid` and the + * write-path validator in AppDriver — without this check an authenticated user + * can set `icon` to an arbitrary attacker URL and turn the unauthenticated + * `/app-icon/:uid` route into a Puter-branded open redirector (cached publicly + * for 15 minutes). * * Accepts: - * - `puter-app-icons.` (and …_alt) - * - The configured `api_base_url` host (AppIconService rewrites icon - * columns to `${api_base_url}/app-icon/`, so it must be trusted - * or round-tripped writes would fail validation). + * + * - `puter-app-icons.` (and …_alt) + * - The configured `api_base_url` host (AppIconService rewrites icon columns to + * `${api_base_url}/app-icon/`, so it must be trusted or round-tripped + * writes would fail validation). */ export function isTrustedIconHost( url: string, diff --git a/src/dev-center/js/apps.js b/src/dev-center/js/apps.js index 219cc793b..2be245f5b 100644 --- a/src/dev-center/js/apps.js +++ b/src/dev-center/js/apps.js @@ -1378,7 +1378,7 @@ $(document).on('click', '.delete-app-settings', async function (e) { const app_data = await puter.apps.get(app_name, { icon_size: 16 }); if ( app_data.metadata?.locked ) { - puter.ui.alert(`${app_data.title} is locked and cannot be deleted.`, [ + puter.ui.alert(`${html_encode(app_data.title)} is locked and cannot be deleted.`, [ { label: 'Ok', }, @@ -1568,7 +1568,7 @@ function generate_app_card (app) { background-position: center; background-repeat: no-repeat; background-size: 92%; - background-image: url(${app.icon === null ? './img/app.svg' : app.icon}); + background-image: url(${html_encode(app.icon === null ? './img/app.svg' : app.icon)}); width: 60px; height: 60px; margin-right: 10px; @@ -2457,7 +2457,7 @@ $(document).on('click', '.delete-apps-btn', async function (e) { if ( app_data.metadata?.locked ) { if ( apps.length === 1 ) { - puter.ui.alert(`${app_data.title} is locked and cannot be deleted.`, [ + puter.ui.alert(`${html_encode(app_data.title)} is locked and cannot be deleted.`, [ { label: 'Ok', }, @@ -2468,7 +2468,7 @@ $(document).on('click', '.delete-apps-btn', async function (e) { break; } - let resp = await puter.ui.alert(`${app_data.title} is locked and cannot be deleted.`, [ + let resp = await puter.ui.alert(`${html_encode(app_data.title)} is locked and cannot be deleted.`, [ { label: 'Skip and Continue', value: 'Continue', @@ -3142,7 +3142,7 @@ function app_context_menu (app_name, app_title, app_uid) { overwrite: false, appUID: app_uid, }).then(async (uploaded) => { - puter.ui.alert(`${app_title} shortcut has been added to your desktop.`, [ + puter.ui.alert(`${html_encode(app_title)} shortcut has been added to your desktop.`, [ { label: 'Ok', type: 'primary', @@ -3182,7 +3182,7 @@ async function attempt_delete_app (app_name, app_title, app_uid) { const app_data = await puter.apps.get(app_name, { icon_size: 16 }); if ( app_data.metadata?.locked ) { - puter.ui.alert(`${app_data.title} is locked and cannot be deleted.`, [ + puter.ui.alert(`${html_encode(app_data.title)} is locked and cannot be deleted.`, [ { label: 'Ok', },