Task #517: App image upload, external links, built-in route lock
Admin Add/Edit App now supports: - Custom image upload (or fall back to Lucide icon) via the existing ServiceImageUploader; rendered on the home launcher when set. - Open mode picker: internal (default), external_tab (window.open), external_iframe (renders inside /embedded/:id). External URL input shown conditionally and required when an external mode is chosen. - Internal route field is hidden entirely when an external mode is selected, and locked (readOnly + lock hint) when editing a built-in app. Slug input is also locked (readOnly) for built-in apps so the built-in identity cannot drift via the form. Backend: - apps schema gains image_url, external_url, open_mode (default 'internal'); drizzle-kit push applied. - New lib/db/src/built-in-apps.ts exports BUILTIN_APP_SLUGS + isBuiltinAppSlug. Exposed via subpath export `@workspace/db/built-in-apps`; the file has zero imports so the browser bundle uses it without pulling in `pg`. tx-os now imports it directly — duplicate FE constant removed. - Built-in slug list: services, notifications, admin, notes, my-orders, orders-incoming, executive-meetings (everything with a hardcoded <Route> in artifacts/tx-os/src/App.tsx). calendar / documents are seeded but admin-defined and remain editable. - PATCH /apps/:id rejects route changes whose previous slug is built-in with 400 + code='builtin_route_locked'. Same-route no-op is allowed; non-route updates on built-ins still work. - PATCH /apps/:id ALSO rejects slug changes when the previous slug is built-in (code='builtin_slug_locked'). UpdateAppBody zod schema intentionally omits slug, so we inspect req.body.slug raw before zod stripping. Closes the 2-step bypass: rename slug (allowed) → change route (now previous.slug looks non-built-in, allowed). - POST /apps and PATCH /apps/:id reject externalUrl values that are not http:// or https:// (code='invalid_external_url'). Prevents shipping javascript:/data:/file: payloads tenant-wide via launcher. Other: - New SPA route /embedded/:id and embedded-app page (iframe host with back + open-in-new-tab + error/not-embeddable states). - OpenAPI App / CreateAppBody / UpdateAppBody extended; codegen ran. - en/ar locales: admin.appImage, appExternalUrl, appOpenMode.*, builtinPathLocked, embeddedFrame.*. - scripts/src/seed.ts: drift guard throws if a seeded built-in slug uses a route that does not match the hardcoded SPA route. Tests: - New API test apps-builtin-route-lock.test.mjs (6/6 pass): reject built-in route change, allow non-route built-in updates, allow non-builtin route changes, reject built-in slug change (anti-bypass), reject non-http(s) externalUrl scheme + accept https, allow built-in same-route no-op. - New Playwright E2E admin-app-image-external-embedded.spec.mjs (passes): launcher renders custom image_url, external_tab opens external URL via window.open, external_iframe navigates to /embedded/:id and renders <iframe src=externalUrl>. Out of scope (pre-existing, not introduced here): - Failing tests in executive-meetings-* and tsc errors in api-server/src/routes/executive-meetings.ts.
This commit is contained in:
@@ -237,6 +237,20 @@ router.post("/apps", requireAdmin, async (req, res): Promise<void> => {
|
||||
// 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<void> => {
|
||||
.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<string, unknown> | 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
|
||||
|
||||
@@ -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,<script>1</script>", "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.
|
||||
|
||||
@@ -1323,12 +1323,18 @@ export default function AdminPage() {
|
||||
<Input
|
||||
value={editingApp.form.slug}
|
||||
onChange={(e) => 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"
|
||||
/>
|
||||
<p className="text-[11px] text-slate-400 leading-snug">
|
||||
{lang === "ar" ? "معرّف فريد للتطبيق (أحرف صغيرة بدون مسافات)" : "Unique app identifier (lowercase, no spaces)"}
|
||||
{isBuiltin
|
||||
? t("admin.builtinPathLocked")
|
||||
: (lang === "ar" ? "معرّف فريد للتطبيق (أحرف صغيرة بدون مسافات)" : "Unique app identifier (lowercase, no spaces)")}
|
||||
</p>
|
||||
</div>
|
||||
{!isExternal && (
|
||||
|
||||
Reference in New Issue
Block a user