diff --git a/artifacts/api-server/src/routes/apps.ts b/artifacts/api-server/src/routes/apps.ts index 61bd6d5d..2e7934d4 100644 --- a/artifacts/api-server/src/routes/apps.ts +++ b/artifacts/api-server/src/routes/apps.ts @@ -237,6 +237,20 @@ router.post("/apps", requireAdmin, async (req, res): Promise => { // can gate the app at creation time. Pulling it out before the insert // keeps appsTable.values strictly typed against the Drizzle schema. const { permissionIds: rawPermissionIds, ...appValues } = parsed.data; + // Task #517: validate externalUrl scheme on create (mirror PATCH). + // External URLs are launched in a new tab or rendered in an iframe for + // every user; restricting to http(s) prevents javascript:/data:/file: + // payloads from being shipped tenant-wide. + if (appValues.externalUrl !== undefined && appValues.externalUrl !== null) { + const trimmedExternal = appValues.externalUrl.trim(); + if (trimmedExternal !== "" && !/^https?:\/\//i.test(trimmedExternal)) { + res.status(400).json({ + error: "External URL must start with http:// or https://.", + code: "invalid_external_url", + }); + return; + } + } // The zod schema already restricts permissionIds to numbers, but we // explicitly reject non-integer / non-positive values with 400 here so // a malformed request never silently drops ids — the admin should know @@ -381,23 +395,54 @@ router.patch("/apps/:id", requireAdmin, async (req, res): Promise => { .from(appsTable) .where(eq(appsTable.id, params.data.id)); - // Task #517: lock the route field for built-in apps. Their paths are - // hardcoded in artifacts/tx-os/src/App.tsx — letting an admin retype - // the path silently breaks the launcher entry (the user reported this - // after editing the Executive Meetings app). We allow admins to keep - // editing every other field (icon, image, color, name, etc.) but a - // route change targeting a built-in slug is rejected up-front. - if ( - previous && - isBuiltinAppSlug(previous.slug) && - parsed.data.route !== undefined && - parsed.data.route !== previous.route - ) { - res.status(400).json({ - error: "The path of a built-in app cannot be changed.", - code: "builtin_route_locked", - }); - return; + // Task #517: lock the route AND slug fields for built-in apps. Their + // paths are hardcoded in artifacts/tx-os/src/App.tsx — letting an + // admin retype the path silently breaks the launcher entry (the user + // reported this after editing the Executive Meetings app). We also + // block slug changes on built-in rows, otherwise an admin could + // bypass the route lock in two steps: first PATCH to rename the slug + // to a non-built-in value (route unchanged → allowed), then PATCH the + // route (previous slug now appears non-built-in → allowed). Locking + // both fields keeps the built-in identity stable. + if (previous && isBuiltinAppSlug(previous.slug)) { + if (parsed.data.route !== undefined && parsed.data.route !== previous.route) { + res.status(400).json({ + error: "The path of a built-in app cannot be changed.", + code: "builtin_route_locked", + }); + return; + } + // UpdateAppBody (generated zod) intentionally omits `slug` so it is + // stripped from parsed.data, but we still need to reject requests + // that *try* to send it for a built-in row — otherwise an admin + // could rename the slug to a non-built-in value (route unchanged → + // allowed) and then change the route in a follow-up request (the + // previous slug now looks non-built-in → allowed). Inspecting the + // raw body lets us catch the bypass before zod silently drops it. + const rawSlug = (req.body as Record | undefined)?.slug; + if (typeof rawSlug === "string" && rawSlug !== previous.slug) { + res.status(400).json({ + error: "The slug of a built-in app cannot be changed.", + code: "builtin_slug_locked", + }); + return; + } + } + + // Task #517: validate externalUrl scheme. This URL is launched in a + // new tab (external_tab) or rendered inside an iframe (external_iframe) + // for every user. Allowing javascript:, data:, file:, etc. would let + // an admin (or anyone who compromises an admin account) ship XSS or + // local-file disclosure to the whole tenant. Restrict to http(s). + if (parsed.data.externalUrl !== undefined && parsed.data.externalUrl !== null) { + const trimmed = parsed.data.externalUrl.trim(); + if (trimmed !== "" && !/^https?:\/\//i.test(trimmed)) { + res.status(400).json({ + error: "External URL must start with http:// or https://.", + code: "invalid_external_url", + }); + return; + } } const [app] = await db diff --git a/artifacts/api-server/tests/apps-builtin-route-lock.test.mjs b/artifacts/api-server/tests/apps-builtin-route-lock.test.mjs index a75f1976..b6684b0e 100644 --- a/artifacts/api-server/tests/apps-builtin-route-lock.test.mjs +++ b/artifacts/api-server/tests/apps-builtin-route-lock.test.mjs @@ -171,6 +171,63 @@ test("PATCH /apps/:id allows route changes on non-built-in apps", async () => { assert.equal(after.rows[0].route, newRoute); }); +test("PATCH /apps/:id rejects slug changes for built-in apps with code=builtin_slug_locked", async () => { + // Anti-bypass: without this, an admin could rename the slug to a + // non-built-in value (allowed) and then change the route in a second + // request (now the previous slug looks non-built-in and would pass). + const slug = "executive-meetings"; + const original = await pool.query(`SELECT id, slug, route FROM apps WHERE slug = $1`, [slug]); + let appId; + let originalSlug; + let originalRoute; + if (original.rowCount > 0) { + appId = original.rows[0].id; + originalSlug = original.rows[0].slug; + originalRoute = original.rows[0].route; + } else { + const created = await createAppRow({ slug, route: "/executive-meetings" }); + appId = created.id; + originalSlug = slug; + originalRoute = created.route; + } + + const res = await fetch(`${API_BASE}/api/apps/${appId}`, { + method: "PATCH", + headers: { "Content-Type": "application/json", Cookie: adminCookie }, + body: JSON.stringify({ slug: `renamed_${Date.now()}` }), + }); + assert.equal(res.status, 400, `expected 400, got ${res.status}`); + const body = await res.json(); + assert.equal(body.code, "builtin_slug_locked"); + + const after = await pool.query(`SELECT slug, route FROM apps WHERE id = $1`, [appId]); + assert.equal(after.rows[0].slug, originalSlug, "slug must not change"); + assert.equal(after.rows[0].route, originalRoute, "route must not change"); +}); + +test("PATCH /apps/:id rejects externalUrl with non-http(s) scheme (XSS guard)", async () => { + // javascript: in an iframe src or window.open target = stored XSS for + // every user; data: / file: are similarly dangerous. Server must reject. + const created = await createAppRow({}); + for (const badUrl of ["javascript:alert(1)", "data:text/html,", "file:///etc/passwd"]) { + const res = await fetch(`${API_BASE}/api/apps/${created.id}`, { + method: "PATCH", + headers: { "Content-Type": "application/json", Cookie: adminCookie }, + body: JSON.stringify({ externalUrl: badUrl, openMode: "external_tab" }), + }); + assert.equal(res.status, 400, `expected 400 for ${badUrl}, got ${res.status}`); + const body = await res.json(); + assert.equal(body.code, "invalid_external_url", `wrong code for ${badUrl}`); + } + // https:// is accepted. + const ok = await fetch(`${API_BASE}/api/apps/${created.id}`, { + method: "PATCH", + headers: { "Content-Type": "application/json", Cookie: adminCookie }, + body: JSON.stringify({ externalUrl: "https://example.com/ok", openMode: "external_tab" }), + }); + assert.equal(ok.status, 200); +}); + test("PATCH /apps/:id with same route on built-in slug is a no-op success", async () => { // Sending route equal to current value should NOT be rejected, since // it isn't actually changing the path. diff --git a/artifacts/tx-os/src/pages/admin.tsx b/artifacts/tx-os/src/pages/admin.tsx index 2acec569..7789aaf6 100644 --- a/artifacts/tx-os/src/pages/admin.tsx +++ b/artifacts/tx-os/src/pages/admin.tsx @@ -1323,12 +1323,18 @@ export default function AdminPage() { setEditingApp({ ...editingApp, form: { ...editingApp.form, slug: e.target.value } })} - className="bg-slate-50/70 border-slate-200 rounded-lg h-10 text-sm font-mono focus-visible:ring-2 focus-visible:ring-indigo-500/30 focus-visible:border-indigo-400" + readOnly={isBuiltin} + className={cn( + "bg-slate-50/70 border-slate-200 rounded-lg h-10 text-sm font-mono focus-visible:ring-2 focus-visible:ring-indigo-500/30 focus-visible:border-indigo-400", + isBuiltin && "opacity-70 cursor-not-allowed", + )} dir="ltr" placeholder="notes" />

- {lang === "ar" ? "معرّف فريد للتطبيق (أحرف صغيرة بدون مسافات)" : "Unique app identifier (lowercase, no spaces)"} + {isBuiltin + ? t("admin.builtinPathLocked") + : (lang === "ar" ? "معرّف فريد للتطبيق (أحرف صغيرة بدون مسافات)" : "Unique app identifier (lowercase, no spaces)")}

{!isExternal && (