From 003e6cde9657a0debf51406ecf6edb4f9b92d70f Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 27 Oct 2026 15:35:46 +0100 Subject: [PATCH 1/8] refactor: extract the bookmark card into a component (#277) --- .../components/dashboard/BookmarkCard.svelte | 89 +++++++++++++++++++ .../dashboard/DashboardBookmarks.svelte | 80 ++--------------- src/lib/dashboard/types.ts | 7 +- 3 files changed, 102 insertions(+), 74 deletions(-) create mode 100644 src/lib/components/dashboard/BookmarkCard.svelte diff --git a/src/lib/components/dashboard/BookmarkCard.svelte b/src/lib/components/dashboard/BookmarkCard.svelte new file mode 100644 index 00000000..5303f639 --- /dev/null +++ b/src/lib/components/dashboard/BookmarkCard.svelte @@ -0,0 +1,89 @@ + + +
+ + +
+
+ + {bookmark.name} + +
+
+ + {product.name} + + {#if bookmark.environment} + + {bookmark.environment} + + {/if} +
+
+ + {bookmarkHostname(bookmark.url)} + +
+
+ + +
diff --git a/src/lib/components/dashboard/DashboardBookmarks.svelte b/src/lib/components/dashboard/DashboardBookmarks.svelte index eb7288ec..b9f1e202 100644 --- a/src/lib/components/dashboard/DashboardBookmarks.svelte +++ b/src/lib/components/dashboard/DashboardBookmarks.svelte @@ -2,15 +2,11 @@ import { SvelteSet } from 'svelte/reactivity'; import * as m from '$lib/paraglide/messages.js'; import IconAdd from 'virtual:icons/material-symbols/add'; - import IconEdit from 'virtual:icons/material-symbols/edit'; - import IconStar from 'virtual:icons/material-symbols/star'; - import IconStarOutline from 'virtual:icons/material-symbols/star-outline'; import AddBookmarkModal from '$lib/components/dashboard/AddBookmarkModal.svelte'; + import BookmarkCard from '$lib/components/dashboard/BookmarkCard.svelte'; import { updateBookmark } from '$lib/dashboard/bookmarks-api.js'; import { addToast } from '$lib/stores/toast.svelte.js'; - import { getProduct } from '$lib/dashboard/products'; - import ProductIcon from '$lib/components/dashboard/ProductIcon.svelte'; - import { bookmarkHostname, type Bookmark } from '$lib/dashboard/types'; + import type { Bookmark } from '$lib/dashboard/types'; let { bookmarks @@ -83,72 +79,12 @@ {:else if bookmarks.length > 0}
{#each bookmarks as bookmark (bookmark.id)} - {@const product = getProduct(bookmark.productId)} -
- - -
-
- - {bookmark.name} - -
-
- - {product.name} - - {#if bookmark.environment} - - {bookmark.environment} - - {/if} -
-
- - {bookmarkHostname(bookmark.url)} - -
-
- - -
+ togglePin(bookmark)} + onEdit={() => openEditBookmark(bookmark)} + /> {/each}
{:else} diff --git a/src/lib/dashboard/types.ts b/src/lib/dashboard/types.ts index 60c9f85f..3941ead6 100644 --- a/src/lib/dashboard/types.ts +++ b/src/lib/dashboard/types.ts @@ -18,8 +18,11 @@ export const bookmarkFormSchema = z.object({ pinned: z.boolean() }); -/** A stored bookmark as returned by the API; `createdAt` is an ISO timestamp. */ -export type Bookmark = z.infer & { id: string; createdAt: string }; +/** The fields every bookmark is displayed with; the id keys it in lists. */ +export type BookmarkLink = z.infer & { id: string }; + +/** A user's stored bookmark as returned by the API; `createdAt` is an ISO timestamp. */ +export type Bookmark = BookmarkLink & { createdAt: string }; /** * Body of a bookmark create request. Stored bookmarks keep any product id, so a From a5a55a87508bb40a9670282e89c927d4a70c094b Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 27 Oct 2026 15:40:03 +0100 Subject: [PATCH 2/8] feat: show global bookmarks from STACKABLE_COCKPIT_GLOBAL_BOOKMARKS (#277) Administrators can configure read-only bookmarks for every user as a JSON array, also via the Helm chart (features.bookmarks.global). They are shown before the user's own bookmarks on the dashboard and in the sidebar, also without a signed-in user. Invalid configuration stops the server at startup. --- .env.example | 3 + .env.test | 2 + README.md | 6 ++ deploy/helm/cockpit/README.md | 1 + deploy/helm/cockpit/templates/deployment.yaml | 6 ++ deploy/helm/cockpit/values.yaml | 10 +++ e2e/bookmarks.spec.ts | 1 - e2e/global-bookmarks.spec.ts | 64 +++++++++++++++ src/hooks.server.ts | 2 + .../components/dashboard/BookmarkCard.svelte | 57 +++++++------- .../dashboard/DashboardBookmarks.svelte | 32 +++++--- .../DashboardBookmarks.svelte.spec.ts | 57 +++++++++++++- .../components/layout/sidebar/Sidebar.svelte | 9 ++- src/lib/dashboard/types.ts | 3 + .../server/dashboard/global-bookmarks.test.ts | 78 +++++++++++++++++++ src/lib/server/dashboard/global-bookmarks.ts | 57 ++++++++++++++ src/routes/(app)/+layout.server.ts | 8 +- src/routes/(app)/+layout.svelte | 2 +- src/routes/(app)/+page.svelte | 12 ++- src/routes/(app)/layout.server.test.ts | 34 +++++++- src/routes/(app)/page.svelte.spec.ts | 37 +++++++-- 21 files changed, 423 insertions(+), 58 deletions(-) create mode 100644 e2e/global-bookmarks.spec.ts create mode 100644 src/lib/server/dashboard/global-bookmarks.test.ts create mode 100644 src/lib/server/dashboard/global-bookmarks.ts diff --git a/.env.example b/.env.example index 2bd6f7f4..5520e989 100644 --- a/.env.example +++ b/.env.example @@ -56,6 +56,9 @@ DATABASE_PASSWORD=change-me # STACKABLE_COCKPIT_COMPLETION_ENABLED=false # Disable SQL editor code completion (default: true) # STACKABLE_COCKPIT_STORAGE_BROWSER_ENABLED=true # Enable S3/HDFS file browser (default: false) # PUBLIC_STACKABLE_COCKPIT_STORAGE_TRANSFER_ENABLED=true # Enables cut, copy, paste, rename and drag-and-drop move in the storage browser; also enforced by the transfer API (default: false) +# Bookmarks shown to every user (JSON array; productId, name and http(s) url required, environment and pinned optional). +# Invalid JSON or entries stop the server at startup. +# STACKABLE_COCKPIT_GLOBAL_BOOKMARKS='[{"productId":"superset","name":"Dashboards","url":"https://superset.example.com"}]' # Storage browser limits and settings. Invalid values stop the server at startup. # STACKABLE_COCKPIT_TEXT_PREVIEW_BYTES=262144 # Max bytes fetched for text, TSV and JSON previews; editable files are fetched completely (default: 262144) diff --git a/.env.test b/.env.test index 23c4b8ae..3c74a4f6 100644 --- a/.env.test +++ b/.env.test @@ -17,6 +17,8 @@ PUBLIC_STACKABLE_COCKPIT_DEFAULT_PAGE_SIZE=25 PUBLIC_STACKABLE_COCKPIT_MAX_RECENT_FILES=15 PUBLIC_STACKABLE_COCKPIT_UPLOAD_CONCURRENCY=3 PUBLIC_STACKABLE_COCKPIT_STORAGE_TRANSFER_ENABLED=true +# Shown on every page, so names must not clash with other tests' locators. +STACKABLE_COCKPIT_GLOBAL_BOOKMARKS='[{"productId":"opensearch","name":"Cluster Logs","environment":"prod","url":"https://logs.example.com","pinned":true},{"productId":"custom","name":"Team Wiki","url":"https://wiki.example.com"}]' # One object at a time, so the cancel E2E test can stop a move mid-way. STACKABLE_COCKPIT_STORAGE_TRANSFER_CONCURRENCY=1 GARAGE_ADMIN_URL=http://localhost:30902 diff --git a/README.md b/README.md index 1e2b6c16..d4f7dda8 100644 --- a/README.md +++ b/README.md @@ -124,6 +124,12 @@ Invalid values (e.g. `abc` for an integer) stop the server at startup with an er | ------------------------------------------------ | --------- | ------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------- | | `STACKABLE_COCKPIT_STORAGE_TRANSFER_CONCURRENCY` | `integer` | `8` | Objects copied at once within one copy, move or rename request. Copies run server-side in the storage backend (S3 CopyObject); no data streams through the app. | `STACKABLE_COCKPIT_STORAGE_TRANSFER_CONCURRENCY=4` | +### Dashboard Bookmarks + +| Variable | Type | Default | Description | Example | +| ------------------------------------ | ------------ | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------- | +| `STACKABLE_COCKPIT_GLOBAL_BOOKMARKS` | `JSON` array | none | Bookmarks shown to every user, before their own, on the dashboard and in the sidebar. Users cannot change them. Each entry needs `productId` (e.g. `trino`, `superset`, or `custom`; unknown ids show as a custom link), `name` (up to 40 characters) and an HTTP(S) `url`; `environment` and `pinned` are optional. Invalid JSON or entries stop the server at startup. | `STACKABLE_COCKPIT_GLOBAL_BOOKMARKS='[{"productId":"superset","name":"Dashboards","url":"https://superset.example.com"}]'` | + ### Request Body Size | Variable | Type | Default | Description | Example | diff --git a/deploy/helm/cockpit/README.md b/deploy/helm/cockpit/README.md index 245972ac..c0a5f106 100644 --- a/deploy/helm/cockpit/README.md +++ b/deploy/helm/cockpit/README.md @@ -122,6 +122,7 @@ All feature parameters are optional. Omit them to use the application defaults. | `features.trino.completion.enabled` | Enable SQL code completion. | `true` (application default) | | `features.storage.enabled` | Show the S3/HDFS file browser and enable routes under `/storage`. | `false` (application default) | | `features.storage.transfer` | Show Cut, Copy, Paste and Rename and allow drag-and-drop move. The transfer API rejects copies, moves and renames while this is disabled. | `false` (application default) | +| `features.bookmarks.global` | Bookmarks shown to every user, before their own; users cannot change them. A list of entries with `productId` (e.g. `trino`, `superset` or `custom`; unknown ids show as a custom link), `name` (up to 40 characters) and an HTTP(S) `url`, plus optional `environment` and `pinned`. Invalid entries stop the application at startup. | none | ### Storage Browser Parameters diff --git a/deploy/helm/cockpit/templates/deployment.yaml b/deploy/helm/cockpit/templates/deployment.yaml index 60ed19bf..8878cfa7 100644 --- a/deploy/helm/cockpit/templates/deployment.yaml +++ b/deploy/helm/cockpit/templates/deployment.yaml @@ -140,6 +140,12 @@ spec: - name: STACKABLE_COCKPIT_DOWNLOAD_HISTORY_RETENTION_DAYS value: {{ include "cockpit.envValue" .Values.storage.downloadHistoryRetentionDays }} {{- end }} + {{- with (index $features "bookmarks") }} + {{- with .global }} + - name: STACKABLE_COCKPIT_GLOBAL_BOOKMARKS + value: {{ toJson . | quote }} + {{- end }} + {{- end }} {{- if .Values.trino.queryTtl }} - name: STACKABLE_COCKPIT_QUERY_TTL value: {{ .Values.trino.queryTtl | int64 |quote }} diff --git a/deploy/helm/cockpit/values.yaml b/deploy/helm/cockpit/values.yaml index 0e810288..28b23d54 100644 --- a/deploy/helm/cockpit/values.yaml +++ b/deploy/helm/cockpit/values.yaml @@ -114,6 +114,16 @@ terminationGracePeriodSeconds: 30 # # Show Cut, Copy, Paste and Rename and allow drag-and-drop move. The transfer API rejects # # copies, moves and renames while disabled. Default: false (application default). # transfer: false +# bookmarks: +# # Bookmarks shown to every user, before their own. Users cannot change them. +# # productId (e.g. trino, superset or custom; unknown ids show as a custom link), +# # name (up to 40 characters) and an http(s) url are required; environment and +# # pinned are optional. Invalid entries stop the application at startup. Default: none. +# global: +# - productId: superset +# name: Dashboards +# url: https://superset.example.com +# environment: prod logging: level: "" diff --git a/e2e/bookmarks.spec.ts b/e2e/bookmarks.spec.ts index 61260a00..a0ea1727 100644 --- a/e2e/bookmarks.spec.ts +++ b/e2e/bookmarks.spec.ts @@ -147,6 +147,5 @@ test.describe('Dashboard bookmarks', () => { await expect(confirmDialog).toBeHidden(); await expect(editDialog).toBeHidden(); await expect(page.getByLabel('Bookmarks').getByText('Dashboards')).not.toBeVisible(); - await expect(page.getByText(/no bookmarks yet/i)).toBeVisible(); }); }); diff --git a/e2e/global-bookmarks.spec.ts b/e2e/global-bookmarks.spec.ts new file mode 100644 index 00000000..1776c441 --- /dev/null +++ b/e2e/global-bookmarks.spec.ts @@ -0,0 +1,64 @@ +import type { Page } from '@playwright/test'; +import { test, expect } from './support/fixtures.js'; +import { waitForHydration } from './support/helpers'; +import { resetBookmarks } from './support/bookmarks.js'; + +// The global bookmarks come from STACKABLE_COCKPIT_GLOBAL_BOOKMARKS in .env.test. +test.describe('Global bookmarks', () => { + test.use({ locale: 'en-US' }); + + const section = (page: Page) => page.getByLabel('Bookmarks'); + const sidebarNav = (page: Page) => page.getByRole('navigation', { name: 'Main navigation' }); + + test.beforeEach(async ({ page }) => { + await resetBookmarks(page, [ + { productId: 'superset', name: 'Dashboards', url: 'https://superset.example.com' } + ]); + }); + + test.afterEach(async ({ page }) => { + await resetBookmarks(page); + }); + + test('shows global bookmarks before the user’s own, read-only', async ({ page }) => { + await page.goto('/'); + await waitForHydration(page); + + // Only the title links: the image links are hidden from assistive technology. + await expect(section(page).getByRole('link')).toHaveText([ + 'Cluster Logs', + 'Team Wiki', + 'Dashboards' + ]); + await expect(section(page).getByText('3 links')).toBeVisible(); + await expect(section(page).getByText('prod', { exact: true })).toBeVisible(); + + for (const name of ['Cluster Logs', 'Team Wiki']) { + await expect(section(page).getByRole('button', { name })).toHaveCount(0); + } + await expect( + section(page).getByRole('button', { name: 'Edit bookmark Dashboards' }) + ).toBeVisible(); + }); + + test('lists global bookmarks in the sidebar by their configured pin', async ({ + page, + viewport + }) => { + test.skip((viewport?.width ?? 1280) < 1024, 'Sidebar is off-canvas on small screens'); + await page.addInitScript(() => localStorage.removeItem('sidebar_tools_open')); + await page.goto('/'); + await waitForHydration(page); + + await expect(sidebarNav(page).getByRole('link', { name: 'Cluster Logs' })).toHaveAttribute( + 'href', + 'https://logs.example.com' + ); + await expect(sidebarNav(page).getByRole('link', { name: 'Team Wiki' })).toBeVisible(); + + // Collapsing Tools hides the unpinned global bookmark but keeps the pinned one. + await page.getByRole('button', { name: 'Tools', exact: true }).click(); + await expect(sidebarNav(page).getByRole('link', { name: 'Team Wiki' })).not.toBeVisible(); + await expect(sidebarNav(page).getByRole('link', { name: 'Cluster Logs' })).toBeVisible(); + }); +}); diff --git a/src/hooks.server.ts b/src/hooks.server.ts index 6df0fe73..1d826708 100644 --- a/src/hooks.server.ts +++ b/src/hooks.server.ts @@ -16,6 +16,7 @@ import '$lib/server/storage/config.js'; import { storageEncryptionKey } from '$lib/server/storage/encryption-key.js'; import { notConnected } from '$lib/server/storage/not-connected.js'; import { runMigrations } from '$lib/server/migrate.js'; +import { globalBookmarks } from '$lib/server/dashboard/global-bookmarks.js'; // Allow self-signed TLS certificates in development (e.g. local Trino with self-signed certs). if (dev) { @@ -125,6 +126,7 @@ export const handle = sequence( * detected immediately rather than on the first request that uses the key. */ export const init: ServerInit = async () => { + globalBookmarks(); // throws immediately if the env var is invalid if (!(await runMigrations())) { throw new Error('Database migrations failed'); } diff --git a/src/lib/components/dashboard/BookmarkCard.svelte b/src/lib/components/dashboard/BookmarkCard.svelte index 5303f639..7f4bc90c 100644 --- a/src/lib/components/dashboard/BookmarkCard.svelte +++ b/src/lib/components/dashboard/BookmarkCard.svelte @@ -15,8 +15,9 @@ }: { bookmark: BookmarkLink; pinPending?: boolean; - onTogglePin: () => void; - onEdit: () => void; + /** Without handlers the card is read-only (global bookmarks). */ + onTogglePin?: () => void; + onEdit?: () => void; } = $props(); const product = $derived(getProduct(bookmark.productId)); @@ -62,28 +63,32 @@ - - + {#if onTogglePin} + + {/if} + {#if onEdit} + + {/if} diff --git a/src/lib/components/dashboard/DashboardBookmarks.svelte b/src/lib/components/dashboard/DashboardBookmarks.svelte index b9f1e202..340dc1e5 100644 --- a/src/lib/components/dashboard/DashboardBookmarks.svelte +++ b/src/lib/components/dashboard/DashboardBookmarks.svelte @@ -6,19 +6,27 @@ import BookmarkCard from '$lib/components/dashboard/BookmarkCard.svelte'; import { updateBookmark } from '$lib/dashboard/bookmarks-api.js'; import { addToast } from '$lib/stores/toast.svelte.js'; - import type { Bookmark } from '$lib/dashboard/types'; + import type { Bookmark, GlobalBookmark } from '$lib/dashboard/types'; let { - bookmarks + bookmarks, + globalBookmarks = [], + loadFailed = false }: { - /** Null when loading them failed: the section says so instead of listing them. */ + /** The user's bookmarks; null when unavailable, which leaves only the global ones. */ bookmarks: Bookmark[] | null; + /** Read-only bookmarks for everyone, shown before the user's. */ + globalBookmarks?: GlobalBookmark[]; + /** Loading the user's bookmarks failed: the section says so. */ + loadFailed?: boolean; } = $props(); let addModalOpen = $state(false); let editingBookmark: Bookmark | null = $state(null); const pinning = new SvelteSet(); + const count = $derived(globalBookmarks.length + (bookmarks?.length ?? 0)); + function openEditBookmark(bookmark: Bookmark) { editingBookmark = bookmark; addModalOpen = true; @@ -56,9 +64,9 @@ > {m.bookmark_section_title()} - {#if bookmarks?.length} + {#if count > 0} - {m.bookmark_count_links({ count: bookmarks.length })} + {m.bookmark_count_links({ count })} {/if} @@ -74,11 +82,15 @@ {/if} - {#if !bookmarks} -
{m.bookmark_load_error()}
- {:else if bookmarks.length > 0} + {#if loadFailed} +
{m.bookmark_load_error()}
+ {/if} + {#if count > 0}
- {#each bookmarks as bookmark (bookmark.id)} + {#each globalBookmarks as bookmark (bookmark.id)} + + {/each} + {#each bookmarks ?? [] as bookmark (bookmark.id)} {/each}
- {:else} + {:else if !loadFailed}

{m.dashboard_bookmarks_empty()}

diff --git a/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts b/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts index fb4cf75e..da29fddf 100644 --- a/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts +++ b/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts @@ -1,7 +1,7 @@ import { page } from 'vitest/browser'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { render } from 'vitest-browser-svelte'; -import type { Bookmark } from '$lib/dashboard/types'; +import type { Bookmark, GlobalBookmark } from '$lib/dashboard/types'; import DashboardBookmarks from './DashboardBookmarks.svelte'; const { updateBookmark, addToast } = vi.hoisted(() => ({ @@ -27,6 +27,15 @@ const bookmark: Bookmark = { createdAt: '2026-01-01T00:00:00.000Z' }; +const globalBookmark: GlobalBookmark = { + id: 'global-0', + productId: 'custom', + name: 'Team Wiki', + environment: '', + url: 'https://wiki.example.com', + pinned: true +}; + describe('DashboardBookmarks', () => { beforeEach(() => vi.clearAllMocks()); @@ -41,7 +50,7 @@ describe('DashboardBookmarks', () => { }); it('shows a message instead of the list, without adding, when loading failed', async () => { - render(DashboardBookmarks, { bookmarks: null }); + render(DashboardBookmarks, { bookmarks: null, loadFailed: true }); await expect .element(page.getByRole('heading', { level: 3, name: 'Bookmarks' })) @@ -55,6 +64,19 @@ describe('DashboardBookmarks', () => { await expect.element(page.getByText(/no bookmarks yet/i)).not.toBeInTheDocument(); }); + it('still shows the global bookmarks when loading the user’s failed', async () => { + render(DashboardBookmarks, { + bookmarks: null, + globalBookmarks: [globalBookmark], + loadFailed: true + }); + + await expect + .element(page.getByText('Bookmarks could not be loaded. Please reload the page later.')) + .toBeVisible(); + await expect.element(page.getByRole('link', { name: 'Team Wiki' })).toBeVisible(); + }); + it('shows an empty state when there are no bookmarks', async () => { render(DashboardBookmarks, { bookmarks: [] }); @@ -108,6 +130,37 @@ describe('DashboardBookmarks', () => { } }); + it('shows global bookmarks first, read-only, and counts them', async () => { + render(DashboardBookmarks, { bookmarks: [bookmark], globalBookmarks: [globalBookmark] }); + + const titles = page + .getByRole('link') + .elements() + .map((link) => link.textContent?.trim()); + expect(titles).toEqual(['Team Wiki', 'Shared Dashboard']); + await expect.element(page.getByText('2 links')).toBeVisible(); + await expect + .element(page.getByRole('button', { name: 'Edit bookmark Team Wiki' })) + .not.toBeInTheDocument(); + await expect + .element(page.getByRole('button', { name: /pin bookmark Team Wiki/i })) + .not.toBeInTheDocument(); + await expect + .element(page.getByRole('button', { name: 'Edit bookmark Shared Dashboard' })) + .toBeVisible(); + }); + + it('shows only the global bookmarks, without adding, when the user’s are unavailable', async () => { + render(DashboardBookmarks, { bookmarks: null, globalBookmarks: [globalBookmark] }); + + await expect.element(page.getByRole('link', { name: 'Team Wiki' })).toBeVisible(); + await expect + .element(page.getByRole('button', { name: /add bookmark/i })) + .not.toBeInTheDocument(); + await expect.element(page.getByText(/bookmarks are added by hand/i)).not.toBeInTheDocument(); + await expect.element(page.getByText(/no bookmarks yet/i)).not.toBeInTheDocument(); + }); + it('opens the add dialog', async () => { render(DashboardBookmarks, { bookmarks: [] }); diff --git a/src/lib/components/layout/sidebar/Sidebar.svelte b/src/lib/components/layout/sidebar/Sidebar.svelte index 25a68e79..ead78e8d 100644 --- a/src/lib/components/layout/sidebar/Sidebar.svelte +++ b/src/lib/components/layout/sidebar/Sidebar.svelte @@ -12,7 +12,7 @@ import { getPlatformSection, getToolsSection } from './nav-items.js'; import { getProduct } from '$lib/dashboard/products'; import ProductIcon from '$lib/components/dashboard/ProductIcon.svelte'; - import type { Bookmark } from '$lib/dashboard/types'; + import type { BookmarkLink } from '$lib/dashboard/types'; import stackableLogo from '$lib/logos/stackable.svg?enhanced'; import TooltipTrigger from '$lib/components/TooltipTrigger.svelte'; @@ -25,7 +25,8 @@ collapsed?: boolean; mobileOpen?: boolean; storageBrowserEnabled?: boolean; - bookmarks?: Bookmark[]; + /** Global bookmarks first, then the user's; both are plain links here. */ + bookmarks?: BookmarkLink[]; } = $props(); const platformSection = $derived(getPlatformSection()); @@ -154,12 +155,12 @@ {/snippet} -{#snippet bookmarkIcon(bookmark: Bookmark)} +{#snippet bookmarkIcon(bookmark: BookmarkLink)} {@const product = getProduct(bookmark.productId)} {/snippet} -{#snippet bookmarkItem(bookmark: Bookmark)} +{#snippet bookmarkItem(bookmark: BookmarkLink)}
  • & { id: string }; +/** A bookmark configured for everyone by the administrator; read-only. */ +export type GlobalBookmark = BookmarkLink; + /** A user's stored bookmark as returned by the API; `createdAt` is an ISO timestamp. */ export type Bookmark = BookmarkLink & { createdAt: string }; diff --git a/src/lib/server/dashboard/global-bookmarks.test.ts b/src/lib/server/dashboard/global-bookmarks.test.ts new file mode 100644 index 00000000..e9826eff --- /dev/null +++ b/src/lib/server/dashboard/global-bookmarks.test.ts @@ -0,0 +1,78 @@ +import { describe, it, expect, vi } from 'vitest'; + +vi.mock('$lib/server/logging', () => import('$lib/test-utils/mock-logger.js')); + +import { parseGlobalBookmarks } from './global-bookmarks.js'; + +const json = (value: unknown) => JSON.stringify(value); + +describe('parseGlobalBookmarks', () => { + it('has no global bookmarks when the variable is unset or blank', () => { + expect(parseGlobalBookmarks(undefined)).toEqual([]); + expect(parseGlobalBookmarks(' ')).toEqual([]); + }); + + it('parses bookmarks, numbers their ids and defaults the optional fields', () => { + const bookmarks = parseGlobalBookmarks( + json([ + { + productId: 'superset', + name: 'Dashboards', + environment: 'prod', + url: 'https://superset.example.com', + pinned: true + }, + { productId: 'custom', name: 'Team Wiki', url: 'https://wiki.example.com' } + ]) + ); + + expect(bookmarks).toEqual([ + { + id: 'global-0', + productId: 'superset', + name: 'Dashboards', + environment: 'prod', + url: 'https://superset.example.com', + pinned: true + }, + { + id: 'global-1', + productId: 'custom', + name: 'Team Wiki', + environment: '', + url: 'https://wiki.example.com', + pinned: false + } + ]); + }); + + it('rejects invalid JSON', () => { + expect(() => parseGlobalBookmarks('[{')).toThrow(/not valid JSON/); + }); + + it('rejects anything but an array', () => { + expect(() => parseGlobalBookmarks(json({ name: 'Wiki' }))).toThrow(/is invalid/); + }); + + it.each([ + ['a non-HTTP URL', { url: 'javascript:alert(1)' }, 'url'], + ['a missing name', { name: undefined }, 'name'], + ['a name above the length limit', { name: 'x'.repeat(41) }, 'name'], + ['a misspelt key', { pined: true }, 'pined'] + ])('rejects %s and names the entry and field', (_, override, field) => { + const entry = { productId: 'custom', name: 'Wiki', url: 'https://wiki.example.com' }; + + const parse = () => parseGlobalBookmarks(json([entry, { ...entry, ...override }])); + + expect(parse).toThrow('[1]'); + expect(parse).toThrow(field); + }); + + it('does not repeat the configured values in the error', () => { + const url = 'ftp://user:secret@files.example.com'; + + expect(() => parseGlobalBookmarks(json([{ productId: 'custom', name: 'Files', url }]))).toThrow( + expect.objectContaining({ message: expect.not.stringContaining('secret') }) + ); + }); +}); diff --git a/src/lib/server/dashboard/global-bookmarks.ts b/src/lib/server/dashboard/global-bookmarks.ts new file mode 100644 index 00000000..9a4f612f --- /dev/null +++ b/src/lib/server/dashboard/global-bookmarks.ts @@ -0,0 +1,57 @@ +import { z } from 'zod'; +import { env } from '$env/dynamic/private'; +import { logger } from '$lib/server/logging'; +import { bookmarkFormSchema, type GlobalBookmark } from '$lib/dashboard/types'; + +const log = logger.child({ module: 'global-bookmarks' }); + +// Strict, so a misspelt key such as "pined" fails instead of being ignored. An +// unknown productId is accepted and shown as a custom link, as for user bookmarks. +const globalBookmarksSchema = z.array( + z.strictObject({ + ...bookmarkFormSchema.shape, + environment: bookmarkFormSchema.shape.environment.default(''), + pinned: bookmarkFormSchema.shape.pinned.default(false) + }) +); + +/** + * Parse `STACKABLE_COCKPIT_GLOBAL_BOOKMARKS`: a JSON array of bookmarks shown + * to every user. Throws on invalid JSON or entries; the error names the failing + * entries and fields but not their values, as URLs can contain credentials. + */ +export function parseGlobalBookmarks(value: string | undefined): GlobalBookmark[] { + if (!value?.trim()) return []; + let json: unknown; + try { + json = JSON.parse(value); + } catch { + throw new Error('STACKABLE_COCKPIT_GLOBAL_BOOKMARKS is not valid JSON'); + } + const parsed = globalBookmarksSchema.safeParse(json); + if (!parsed.success) { + throw new Error( + `STACKABLE_COCKPIT_GLOBAL_BOOKMARKS is invalid:\n${z.prettifyError(parsed.error)}` + ); + } + return parsed.data.map((bookmark, index) => ({ id: `global-${index}`, ...bookmark })); +} + +let cached: GlobalBookmark[] | undefined; + +/** + * The configured global bookmarks. Parsed on first call and cached; called at + * server start so an invalid configuration stops the server. A function rather + * than a module-level constant so tests can import this file without the env var. + */ +export function globalBookmarks(): GlobalBookmark[] { + if (cached) return cached; + try { + cached = parseGlobalBookmarks(env.STACKABLE_COCKPIT_GLOBAL_BOOKMARKS); + } catch (err) { + log.error({ err }, 'Invalid global bookmarks configuration'); + throw err; + } + log.info({ count: cached.length }, 'Loaded global bookmarks'); + return cached; +} diff --git a/src/routes/(app)/+layout.server.ts b/src/routes/(app)/+layout.server.ts index 2c13f544..dd3329e7 100644 --- a/src/routes/(app)/+layout.server.ts +++ b/src/routes/(app)/+layout.server.ts @@ -1,12 +1,18 @@ import type { LayoutServerLoad } from './$types'; import { storageBrowserEnabled } from '$lib/server/feature-flags.js'; import { listBookmarks } from '$lib/server/dashboard/bookmarks-db.js'; +import { globalBookmarks } from '$lib/server/dashboard/global-bookmarks.js'; import { dashboardBookmarksTotal } from '$lib/server/metrics.js'; import { BOOKMARKS_DEPENDENCY, type Bookmark } from '$lib/dashboard/types'; export const load: LayoutServerLoad = async ({ locals, depends }) => { depends(BOOKMARKS_DEPENDENCY); - return { user: locals.user, storageBrowserEnabled, ...(await loadBookmarks(locals)) }; + return { + user: locals.user, + storageBrowserEnabled, + globalBookmarks: globalBookmarks(), + ...(await loadBookmarks(locals)) + }; }; /** diff --git a/src/routes/(app)/+layout.svelte b/src/routes/(app)/+layout.svelte index 5aa94105..ff81f039 100644 --- a/src/routes/(app)/+layout.svelte +++ b/src/routes/(app)/+layout.svelte @@ -78,7 +78,7 @@ bind:collapsed={sidebarCollapsed} bind:mobileOpen storageBrowserEnabled={data.storageBrowserEnabled} - bookmarks={data.bookmarks ?? []} + bookmarks={[...data.globalBookmarks, ...(data.bookmarks ?? [])]} />
    diff --git a/src/routes/(app)/+page.svelte b/src/routes/(app)/+page.svelte index 99b18442..d459a9ba 100644 --- a/src/routes/(app)/+page.svelte +++ b/src/routes/(app)/+page.svelte @@ -8,9 +8,13 @@

    {m.dashboard_welcome()}

    - - {#if data.bookmarks || data.bookmarksFailed} - + + {#if data.bookmarks || data.bookmarksFailed || data.globalBookmarks.length > 0} + {/if}
    diff --git a/src/routes/(app)/layout.server.test.ts b/src/routes/(app)/layout.server.test.ts index 885c95ee..3591be61 100644 --- a/src/routes/(app)/layout.server.test.ts +++ b/src/routes/(app)/layout.server.test.ts @@ -4,9 +4,23 @@ const { listBookmarks } = vi.hoisted(() => ({ listBookmarks: vi.fn() })); vi.mock('$lib/server/dashboard/bookmarks-db.js', () => ({ listBookmarks })); vi.mock('$lib/server/feature-flags.js', () => ({ storageBrowserEnabled: false })); +vi.mock('$lib/server/dashboard/global-bookmarks.js', () => ({ + globalBookmarks: () => [GLOBAL_BOOKMARK] +})); import { load } from './+layout.server.js'; +const { GLOBAL_BOOKMARK } = vi.hoisted(() => ({ + GLOBAL_BOOKMARK: { + id: 'global-0', + productId: 'custom', + name: 'Team Wiki', + environment: '', + url: 'https://wiki.example.com', + pinned: true + } +})); + const BOOKMARK = { id: '00000000-0000-0000-0000-000000000001', productId: 'trino', @@ -33,15 +47,23 @@ describe('(app) layout load', () => { const data = await load(event); - expect(data).toMatchObject({ bookmarks: [BOOKMARK], bookmarksFailed: false }); + expect(data).toMatchObject({ + bookmarks: [BOOKMARK], + bookmarksFailed: false, + globalBookmarks: [GLOBAL_BOOKMARK] + }); expect(listBookmarks).toHaveBeenCalledWith('user-1'); expect(event.depends).toHaveBeenCalledWith('app:bookmarks'); }); - it('has no bookmarks without a signed-in user', async () => { + it('has only the global bookmarks without a signed-in user', async () => { const data = await load(mockEvent(undefined)); - expect(data).toMatchObject({ bookmarks: null, bookmarksFailed: false }); + expect(data).toMatchObject({ + bookmarks: null, + bookmarksFailed: false, + globalBookmarks: [GLOBAL_BOOKMARK] + }); expect(listBookmarks).not.toHaveBeenCalled(); }); @@ -51,7 +73,11 @@ describe('(app) layout load', () => { const data = await load(event); - expect(data).toMatchObject({ bookmarks: null, bookmarksFailed: true }); + expect(data).toMatchObject({ + bookmarks: null, + bookmarksFailed: true, + globalBookmarks: [GLOBAL_BOOKMARK] + }); expect(event.locals.logger.error).toHaveBeenCalled(); }); }); diff --git a/src/routes/(app)/page.svelte.spec.ts b/src/routes/(app)/page.svelte.spec.ts index 2f1ce1a8..e3c27a0a 100644 --- a/src/routes/(app)/page.svelte.spec.ts +++ b/src/routes/(app)/page.svelte.spec.ts @@ -1,12 +1,30 @@ import { page } from 'vitest/browser'; import { describe, expect, it } from 'vitest'; import { render } from 'vitest-browser-svelte'; -import type { Bookmark } from '$lib/dashboard/types'; +import type { Bookmark, GlobalBookmark } from '$lib/dashboard/types'; import Page from './+page.svelte'; -const renderPage = (bookmarks: Bookmark[] | null = [], bookmarksFailed = false) => +const globalBookmark: GlobalBookmark = { + id: 'global-0', + productId: 'custom', + name: 'Team Wiki', + environment: '', + url: 'https://wiki.example.com', + pinned: false +}; + +const renderPage = ( + bookmarks: Bookmark[] | null = [], + { globalBookmarks = [] as GlobalBookmark[], bookmarksFailed = false } = {} +) => render(Page, { - data: { user: undefined, storageBrowserEnabled: false, bookmarks, bookmarksFailed } + data: { + user: undefined, + storageBrowserEnabled: false, + bookmarks, + bookmarksFailed, + globalBookmarks + } } as unknown as Parameters>[1]); describe('/(app)/+page.svelte', () => { @@ -23,7 +41,7 @@ describe('/(app)/+page.svelte', () => { }); it('says so when loading the bookmarks failed', async () => { - renderPage(null, true); + renderPage(null, { bookmarksFailed: true }); await expect .element(page.getByText('Bookmarks could not be loaded. Please reload the page later.')) @@ -33,7 +51,16 @@ describe('/(app)/+page.svelte', () => { .not.toBeInTheDocument(); }); - it('hides everything bookmark-related without a signed-in user', async () => { + it('shows the global bookmarks when the user’s are unavailable', async () => { + renderPage(null, { globalBookmarks: [globalBookmark] }); + + await expect.element(page.getByRole('link', { name: 'Team Wiki' })).toBeVisible(); + await expect + .element(page.getByRole('button', { name: /add bookmark/i })) + .not.toBeInTheDocument(); + }); + + it('hides everything bookmark-related without a user or global bookmarks', async () => { renderPage(null); await expect.element(page.getByRole('heading', { level: 2 })).toBeInTheDocument(); From 379432fe1ff1a2a4ee1f80703ee77613e68ebf40 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 27 Oct 2026 16:47:46 +0100 Subject: [PATCH 3/8] fix: reject a JSON string for global bookmarks in the Helm chart (#277) features.bookmarks.global must be a YAML list. A JSON string, e.g. copied from the README, was encoded again and stopped the application at startup; Helm now fails at render time with a clear message instead. --- deploy/helm/cockpit/README.md | 2 +- deploy/helm/cockpit/templates/deployment.yaml | 3 +++ deploy/helm/cockpit/values.yaml | 3 ++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/deploy/helm/cockpit/README.md b/deploy/helm/cockpit/README.md index c0a5f106..2524cc90 100644 --- a/deploy/helm/cockpit/README.md +++ b/deploy/helm/cockpit/README.md @@ -122,7 +122,7 @@ All feature parameters are optional. Omit them to use the application defaults. | `features.trino.completion.enabled` | Enable SQL code completion. | `true` (application default) | | `features.storage.enabled` | Show the S3/HDFS file browser and enable routes under `/storage`. | `false` (application default) | | `features.storage.transfer` | Show Cut, Copy, Paste and Rename and allow drag-and-drop move. The transfer API rejects copies, moves and renames while this is disabled. | `false` (application default) | -| `features.bookmarks.global` | Bookmarks shown to every user, before their own; users cannot change them. A list of entries with `productId` (e.g. `trino`, `superset` or `custom`; unknown ids show as a custom link), `name` (up to 40 characters) and an HTTP(S) `url`, plus optional `environment` and `pinned`. Invalid entries stop the application at startup. | none | +| `features.bookmarks.global` | Bookmarks shown to every user, before their own; users cannot change them. A list of entries with `productId` (e.g. `trino`, `superset` or `custom`; unknown ids show as a custom link), `name` (up to 40 characters) and an HTTP(S) `url`, plus optional `environment` and `pinned`. Must be a YAML list, not a JSON string. Invalid entries stop the application at startup. | none | ### Storage Browser Parameters diff --git a/deploy/helm/cockpit/templates/deployment.yaml b/deploy/helm/cockpit/templates/deployment.yaml index 8878cfa7..9eed44bf 100644 --- a/deploy/helm/cockpit/templates/deployment.yaml +++ b/deploy/helm/cockpit/templates/deployment.yaml @@ -142,6 +142,9 @@ spec: {{- end }} {{- with (index $features "bookmarks") }} {{- with .global }} + {{- if not (kindIs "slice" .) }} + {{- fail "features.bookmarks.global must be a YAML list of bookmarks; a JSON string is not accepted" }} + {{- end }} - name: STACKABLE_COCKPIT_GLOBAL_BOOKMARKS value: {{ toJson . | quote }} {{- end }} diff --git a/deploy/helm/cockpit/values.yaml b/deploy/helm/cockpit/values.yaml index 28b23d54..116ee8f9 100644 --- a/deploy/helm/cockpit/values.yaml +++ b/deploy/helm/cockpit/values.yaml @@ -118,7 +118,8 @@ terminationGracePeriodSeconds: 30 # # Bookmarks shown to every user, before their own. Users cannot change them. # # productId (e.g. trino, superset or custom; unknown ids show as a custom link), # # name (up to 40 characters) and an http(s) url are required; environment and -# # pinned are optional. Invalid entries stop the application at startup. Default: none. +# # pinned are optional. Must be a YAML list, not a JSON string. Invalid entries stop +# # the application at startup. Default: none. # global: # - productId: superset # name: Dashboards From f673d8b08bafbfa25729e778cd1df94427ba9adc Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 27 Oct 2026 16:47:53 +0100 Subject: [PATCH 4/8] refactor: define the bookmark display order once (#277) The sidebar and the dashboard both list global bookmarks first via bookmarksInDisplayOrder. The GlobalBookmark alias is gone in favour of BookmarkLink. --- .../dashboard/DashboardBookmarks.svelte | 30 +++++++++++-------- .../DashboardBookmarks.svelte.spec.ts | 4 +-- src/lib/dashboard/types.test.ts | 26 +++++++++++++++- src/lib/dashboard/types.ts | 14 +++++++-- src/lib/server/dashboard/global-bookmarks.ts | 8 ++--- src/routes/(app)/+layout.svelte | 3 +- src/routes/(app)/page.svelte.spec.ts | 6 ++-- 7 files changed, 64 insertions(+), 27 deletions(-) diff --git a/src/lib/components/dashboard/DashboardBookmarks.svelte b/src/lib/components/dashboard/DashboardBookmarks.svelte index 340dc1e5..e0d880d6 100644 --- a/src/lib/components/dashboard/DashboardBookmarks.svelte +++ b/src/lib/components/dashboard/DashboardBookmarks.svelte @@ -6,7 +6,7 @@ import BookmarkCard from '$lib/components/dashboard/BookmarkCard.svelte'; import { updateBookmark } from '$lib/dashboard/bookmarks-api.js'; import { addToast } from '$lib/stores/toast.svelte.js'; - import type { Bookmark, GlobalBookmark } from '$lib/dashboard/types'; + import { bookmarksInDisplayOrder, type Bookmark, type BookmarkLink } from '$lib/dashboard/types'; let { bookmarks, @@ -16,7 +16,7 @@ /** The user's bookmarks; null when unavailable, which leaves only the global ones. */ bookmarks: Bookmark[] | null; /** Read-only bookmarks for everyone, shown before the user's. */ - globalBookmarks?: GlobalBookmark[]; + globalBookmarks?: BookmarkLink[]; /** Loading the user's bookmarks failed: the section says so. */ loadFailed?: boolean; } = $props(); @@ -25,7 +25,8 @@ let editingBookmark: Bookmark | null = $state(null); const pinning = new SvelteSet(); - const count = $derived(globalBookmarks.length + (bookmarks?.length ?? 0)); + const allBookmarks = $derived(bookmarksInDisplayOrder(globalBookmarks, bookmarks)); + const count = $derived(allBookmarks.length); function openEditBookmark(bookmark: Bookmark) { editingBookmark = bookmark; @@ -87,16 +88,19 @@ {/if} {#if count > 0}
    - {#each globalBookmarks as bookmark (bookmark.id)} - - {/each} - {#each bookmarks ?? [] as bookmark (bookmark.id)} - togglePin(bookmark)} - onEdit={() => openEditBookmark(bookmark)} - /> + {#each allBookmarks as bookmark (bookmark.id)} + + {@const own = bookmarks?.find((b) => b.id === bookmark.id)} + {#if own} + togglePin(own)} + onEdit={() => openEditBookmark(own)} + /> + {:else} + + {/if} {/each}
    {:else if !loadFailed} diff --git a/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts b/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts index da29fddf..bc9cba8b 100644 --- a/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts +++ b/src/lib/components/dashboard/DashboardBookmarks.svelte.spec.ts @@ -1,7 +1,7 @@ import { page } from 'vitest/browser'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { render } from 'vitest-browser-svelte'; -import type { Bookmark, GlobalBookmark } from '$lib/dashboard/types'; +import type { Bookmark, BookmarkLink } from '$lib/dashboard/types'; import DashboardBookmarks from './DashboardBookmarks.svelte'; const { updateBookmark, addToast } = vi.hoisted(() => ({ @@ -27,7 +27,7 @@ const bookmark: Bookmark = { createdAt: '2026-01-01T00:00:00.000Z' }; -const globalBookmark: GlobalBookmark = { +const globalBookmark: BookmarkLink = { id: 'global-0', productId: 'custom', name: 'Team Wiki', diff --git a/src/lib/dashboard/types.test.ts b/src/lib/dashboard/types.test.ts index 645e1c30..471d9091 100644 --- a/src/lib/dashboard/types.test.ts +++ b/src/lib/dashboard/types.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { bookmarkFormSchema, bookmarkHostname } from './types'; +import { bookmarkFormSchema, bookmarkHostname, bookmarksInDisplayOrder } from './types'; const urlSchema = bookmarkFormSchema.shape.url; @@ -37,3 +37,27 @@ describe('bookmarkHostname', () => { expect(bookmarkHostname('superset.example')).toBe('superset.example'); }); }); + +describe('bookmarksInDisplayOrder', () => { + const link = (id: string) => ({ + id, + productId: 'custom', + name: id, + environment: '', + url: 'https://example.com', + pinned: false + }); + + it('lists global bookmarks before the user’s own', () => { + const own = { ...link('own'), createdAt: '2026-10-27T10:00:00.000Z' }; + + expect(bookmarksInDisplayOrder([link('global')], [own]).map((b) => b.id)).toEqual([ + 'global', + 'own' + ]); + }); + + it('lists only the global bookmarks when the user’s are unavailable', () => { + expect(bookmarksInDisplayOrder([link('global')], null).map((b) => b.id)).toEqual(['global']); + }); +}); diff --git a/src/lib/dashboard/types.ts b/src/lib/dashboard/types.ts index bc5a70f5..62171b16 100644 --- a/src/lib/dashboard/types.ts +++ b/src/lib/dashboard/types.ts @@ -21,9 +21,6 @@ export const bookmarkFormSchema = z.object({ /** The fields every bookmark is displayed with; the id keys it in lists. */ export type BookmarkLink = z.infer & { id: string }; -/** A bookmark configured for everyone by the administrator; read-only. */ -export type GlobalBookmark = BookmarkLink; - /** A user's stored bookmark as returned by the API; `createdAt` is an ISO timestamp. */ export type Bookmark = BookmarkLink & { createdAt: string }; @@ -42,6 +39,17 @@ export const bookmarkUpdateSchema = bookmarkCreateSchema export type BookmarkUpdate = z.infer; +/** + * All bookmarks in the order the dashboard and the sidebar list them: the + * global ones first, then the user's own. + */ +export function bookmarksInDisplayOrder( + globalBookmarks: BookmarkLink[], + bookmarks: Bookmark[] | null +): BookmarkLink[] { + return [...globalBookmarks, ...(bookmarks ?? [])]; +} + /** Hostname of a bookmark URL, or the input unchanged if it does not parse. */ export function bookmarkHostname(url: string): string { try { diff --git a/src/lib/server/dashboard/global-bookmarks.ts b/src/lib/server/dashboard/global-bookmarks.ts index 9a4f612f..1e3809dd 100644 --- a/src/lib/server/dashboard/global-bookmarks.ts +++ b/src/lib/server/dashboard/global-bookmarks.ts @@ -1,7 +1,7 @@ import { z } from 'zod'; import { env } from '$env/dynamic/private'; import { logger } from '$lib/server/logging'; -import { bookmarkFormSchema, type GlobalBookmark } from '$lib/dashboard/types'; +import { bookmarkFormSchema, type BookmarkLink } from '$lib/dashboard/types'; const log = logger.child({ module: 'global-bookmarks' }); @@ -20,7 +20,7 @@ const globalBookmarksSchema = z.array( * to every user. Throws on invalid JSON or entries; the error names the failing * entries and fields but not their values, as URLs can contain credentials. */ -export function parseGlobalBookmarks(value: string | undefined): GlobalBookmark[] { +export function parseGlobalBookmarks(value: string | undefined): BookmarkLink[] { if (!value?.trim()) return []; let json: unknown; try { @@ -37,14 +37,14 @@ export function parseGlobalBookmarks(value: string | undefined): GlobalBookmark[ return parsed.data.map((bookmark, index) => ({ id: `global-${index}`, ...bookmark })); } -let cached: GlobalBookmark[] | undefined; +let cached: BookmarkLink[] | undefined; /** * The configured global bookmarks. Parsed on first call and cached; called at * server start so an invalid configuration stops the server. A function rather * than a module-level constant so tests can import this file without the env var. */ -export function globalBookmarks(): GlobalBookmark[] { +export function globalBookmarks(): BookmarkLink[] { if (cached) return cached; try { cached = parseGlobalBookmarks(env.STACKABLE_COCKPIT_GLOBAL_BOOKMARKS); diff --git a/src/routes/(app)/+layout.svelte b/src/routes/(app)/+layout.svelte index ff81f039..7e13224b 100644 --- a/src/routes/(app)/+layout.svelte +++ b/src/routes/(app)/+layout.svelte @@ -6,6 +6,7 @@ import Header from '$lib/components/layout/header/Header.svelte'; import ToastHost from '$lib/components/ToastHost.svelte'; import SessionExpiredModal from '$lib/components/layout/SessionExpiredModal.svelte'; + import { bookmarksInDisplayOrder } from '$lib/dashboard/types'; let { children, data } = $props(); @@ -78,7 +79,7 @@ bind:collapsed={sidebarCollapsed} bind:mobileOpen storageBrowserEnabled={data.storageBrowserEnabled} - bookmarks={[...data.globalBookmarks, ...(data.bookmarks ?? [])]} + bookmarks={bookmarksInDisplayOrder(data.globalBookmarks, data.bookmarks)} />
    diff --git a/src/routes/(app)/page.svelte.spec.ts b/src/routes/(app)/page.svelte.spec.ts index e3c27a0a..a18519e7 100644 --- a/src/routes/(app)/page.svelte.spec.ts +++ b/src/routes/(app)/page.svelte.spec.ts @@ -1,10 +1,10 @@ import { page } from 'vitest/browser'; import { describe, expect, it } from 'vitest'; import { render } from 'vitest-browser-svelte'; -import type { Bookmark, GlobalBookmark } from '$lib/dashboard/types'; +import type { Bookmark, BookmarkLink } from '$lib/dashboard/types'; import Page from './+page.svelte'; -const globalBookmark: GlobalBookmark = { +const globalBookmark: BookmarkLink = { id: 'global-0', productId: 'custom', name: 'Team Wiki', @@ -15,7 +15,7 @@ const globalBookmark: GlobalBookmark = { const renderPage = ( bookmarks: Bookmark[] | null = [], - { globalBookmarks = [] as GlobalBookmark[], bookmarksFailed = false } = {} + { globalBookmarks = [] as BookmarkLink[], bookmarksFailed = false } = {} ) => render(Page, { data: { From c92752ccdac666dd5d96e77293c5b394e5faa63c Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Tue, 27 Oct 2026 16:47:53 +0100 Subject: [PATCH 5/8] test: trim the global bookmarks E2E test to the wiring (#277) --- e2e/global-bookmarks.spec.ts | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/e2e/global-bookmarks.spec.ts b/e2e/global-bookmarks.spec.ts index 1776c441..18265d25 100644 --- a/e2e/global-bookmarks.spec.ts +++ b/e2e/global-bookmarks.spec.ts @@ -20,7 +20,9 @@ test.describe('Global bookmarks', () => { await resetBookmarks(page); }); - test('shows global bookmarks before the user’s own, read-only', async ({ page }) => { + // Card rendering and the read-only cards are covered by the DashboardBookmarks + // component tests; this checks the wiring from the env var to the page. + test('shows the configured bookmarks before the user’s own', async ({ page }) => { await page.goto('/'); await waitForHydration(page); @@ -30,15 +32,6 @@ test.describe('Global bookmarks', () => { 'Team Wiki', 'Dashboards' ]); - await expect(section(page).getByText('3 links')).toBeVisible(); - await expect(section(page).getByText('prod', { exact: true })).toBeVisible(); - - for (const name of ['Cluster Logs', 'Team Wiki']) { - await expect(section(page).getByRole('button', { name })).toHaveCount(0); - } - await expect( - section(page).getByRole('button', { name: 'Edit bookmark Dashboards' }) - ).toBeVisible(); }); test('lists global bookmarks in the sidebar by their configured pin', async ({ From 565a5aba5277cef59bcf3f8dc3472085536ee427 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Thu, 29 Oct 2026 14:59:32 +0100 Subject: [PATCH 6/8] refactor: move global bookmarks to the dashboard config (#277) Global bookmarks are configuration, not a feature toggle. They are parsed with parseEnv like the storage config, via a new envJson helper, and the module is imported for its side effect at startup. In the Helm chart, features.bookmarks.global moves to bookmarks.global, as features only holds toggles. --- deploy/helm/cockpit/README.md | 7 ++- deploy/helm/cockpit/templates/deployment.yaml | 6 +- deploy/helm/cockpit/values.yaml | 24 ++++---- src/hooks.server.ts | 7 +-- src/lib/env-schema.test.ts | 30 +++++++++- src/lib/env-schema.ts | 23 ++++++++ ...lobal-bookmarks.test.ts => config.test.ts} | 17 ++++-- src/lib/server/dashboard/config.ts | 42 ++++++++++++++ src/lib/server/dashboard/global-bookmarks.ts | 57 ------------------- src/routes/(app)/+layout.server.ts | 4 +- src/routes/(app)/layout.server.test.ts | 4 +- 11 files changed, 133 insertions(+), 88 deletions(-) rename src/lib/server/dashboard/{global-bookmarks.test.ts => config.test.ts} (81%) create mode 100644 src/lib/server/dashboard/config.ts delete mode 100644 src/lib/server/dashboard/global-bookmarks.ts diff --git a/deploy/helm/cockpit/README.md b/deploy/helm/cockpit/README.md index 2524cc90..b5cfab6b 100644 --- a/deploy/helm/cockpit/README.md +++ b/deploy/helm/cockpit/README.md @@ -122,7 +122,6 @@ All feature parameters are optional. Omit them to use the application defaults. | `features.trino.completion.enabled` | Enable SQL code completion. | `true` (application default) | | `features.storage.enabled` | Show the S3/HDFS file browser and enable routes under `/storage`. | `false` (application default) | | `features.storage.transfer` | Show Cut, Copy, Paste and Rename and allow drag-and-drop move. The transfer API rejects copies, moves and renames while this is disabled. | `false` (application default) | -| `features.bookmarks.global` | Bookmarks shown to every user, before their own; users cannot change them. A list of entries with `productId` (e.g. `trino`, `superset` or `custom`; unknown ids show as a custom link), `name` (up to 40 characters) and an HTTP(S) `url`, plus optional `environment` and `pinned`. Must be a YAML list, not a JSON string. Invalid entries stop the application at startup. | none | ### Storage Browser Parameters @@ -146,6 +145,12 @@ Limits and tuning for the file browser. Empty values use the application default | `storage.parquetPreviewDisallowedCompressionTypes` | Comma-separated Parquet compression codecs that are not previewed. A `-no_offset` suffix (e.g. `gzip-no_offset`) only blocks files without an offset index. | `""` (application default `gzip-no_offset`) | | `storage.downloadHistoryRetentionDays` | Days a user's download history is kept. Integer from 1 to 3650. | `""` (application default `30`) | +### Bookmark Parameters + +| Parameter | Description | Default | +| --- | --- | --- | +| `bookmarks.global` | Bookmarks shown to every user, before their own; users cannot change them. A list of entries with `productId` (e.g. `trino`, `superset` or `custom`; unknown ids show as a custom link), `name` (up to 40 characters) and an HTTP(S) `url`, plus optional `environment` and `pinned`. Must be a YAML list, not a JSON string. Invalid entries stop the application at startup. | `[]` | + ### Authentication Parameters OIDC is the only supported auth mechanism. Disable for local testing. diff --git a/deploy/helm/cockpit/templates/deployment.yaml b/deploy/helm/cockpit/templates/deployment.yaml index 9eed44bf..7cad21ca 100644 --- a/deploy/helm/cockpit/templates/deployment.yaml +++ b/deploy/helm/cockpit/templates/deployment.yaml @@ -140,15 +140,13 @@ spec: - name: STACKABLE_COCKPIT_DOWNLOAD_HISTORY_RETENTION_DAYS value: {{ include "cockpit.envValue" .Values.storage.downloadHistoryRetentionDays }} {{- end }} - {{- with (index $features "bookmarks") }} - {{- with .global }} + {{- with .Values.bookmarks.global }} {{- if not (kindIs "slice" .) }} - {{- fail "features.bookmarks.global must be a YAML list of bookmarks; a JSON string is not accepted" }} + {{- fail "bookmarks.global must be a YAML list of bookmarks; a JSON string is not accepted" }} {{- end }} - name: STACKABLE_COCKPIT_GLOBAL_BOOKMARKS value: {{ toJson . | quote }} {{- end }} - {{- end }} {{- if .Values.trino.queryTtl }} - name: STACKABLE_COCKPIT_QUERY_TTL value: {{ .Values.trino.queryTtl | int64 |quote }} diff --git a/deploy/helm/cockpit/values.yaml b/deploy/helm/cockpit/values.yaml index 116ee8f9..e1fcc9dd 100644 --- a/deploy/helm/cockpit/values.yaml +++ b/deploy/helm/cockpit/values.yaml @@ -114,17 +114,6 @@ terminationGracePeriodSeconds: 30 # # Show Cut, Copy, Paste and Rename and allow drag-and-drop move. The transfer API rejects # # copies, moves and renames while disabled. Default: false (application default). # transfer: false -# bookmarks: -# # Bookmarks shown to every user, before their own. Users cannot change them. -# # productId (e.g. trino, superset or custom; unknown ids show as a custom link), -# # name (up to 40 characters) and an http(s) url are required; environment and -# # pinned are optional. Must be a YAML list, not a JSON string. Invalid entries stop -# # the application at startup. Default: none. -# global: -# - productId: superset -# name: Dashboards -# url: https://superset.example.com -# environment: prod logging: level: "" @@ -200,6 +189,19 @@ database: # The Secret Operator mounts ca.crt into /stackable/database-tls. secretClass: "" +# Dashboard bookmarks. +bookmarks: + # Bookmarks shown to every user, before their own. Users cannot change them. + # productId (e.g. trino, superset or custom; unknown ids show as a custom link), + # name (up to 40 characters) and an http(s) url are required; environment and + # pinned are optional. Must be a YAML list, not a JSON string. Invalid entries stop + # the application at startup. + global: [] + # - productId: superset + # name: Dashboards + # url: https://superset.example.com + # environment: prod + # Storage browser settings that are not feature flags. Empty or omitted values use the application # defaults; invalid values stop the application at startup. storage: diff --git a/src/hooks.server.ts b/src/hooks.server.ts index 1d826708..90eacd04 100644 --- a/src/hooks.server.ts +++ b/src/hooks.server.ts @@ -10,13 +10,13 @@ import { requestLogger, logger } from '$lib/server/logging'; import { getConnectionFromHeader } from '$lib/server/storage/connection.js'; import { STORAGE_CONNECTION_ID_HEADER } from '$lib/storage/connection-id-header.js'; import { storageBrowserEnabled } from '$lib/server/feature-flags.js'; -// Imported for its side effect: parses the storage and PUBLIC_ client config at -// startup, so invalid values stop the server instead of failing the first request. +// Imported for their side effect: parse the storage, dashboard and PUBLIC_ client +// config at startup, so invalid values stop the server instead of failing the first request. import '$lib/server/storage/config.js'; +import '$lib/server/dashboard/config.js'; import { storageEncryptionKey } from '$lib/server/storage/encryption-key.js'; import { notConnected } from '$lib/server/storage/not-connected.js'; import { runMigrations } from '$lib/server/migrate.js'; -import { globalBookmarks } from '$lib/server/dashboard/global-bookmarks.js'; // Allow self-signed TLS certificates in development (e.g. local Trino with self-signed certs). if (dev) { @@ -126,7 +126,6 @@ export const handle = sequence( * detected immediately rather than on the first request that uses the key. */ export const init: ServerInit = async () => { - globalBookmarks(); // throws immediately if the env var is invalid if (!(await runMigrations())) { throw new Error('Database migrations failed'); } diff --git a/src/lib/env-schema.test.ts b/src/lib/env-schema.test.ts index df99abe0..f083fb41 100644 --- a/src/lib/env-schema.test.ts +++ b/src/lib/env-schema.test.ts @@ -1,6 +1,13 @@ import { describe, it, expect } from 'vitest'; import { z } from 'zod'; -import { envBoolean, envInteger, envList, envOptionalInteger, parseEnv } from './env-schema.js'; +import { + envBoolean, + envInteger, + envJson, + envList, + envOptionalInteger, + parseEnv +} from './env-schema.js'; describe('envBoolean', () => { const schema = envBoolean(false); @@ -52,6 +59,27 @@ describe('envList', () => { }); }); +describe('envJson', () => { + const schema = envJson(z.array(z.number()), []); + + it('parses and validates the JSON', () => { + expect(schema.parse('[1, 2]')).toEqual([1, 2]); + expect(schema.safeParse('["a"]').success).toBe(false); + }); + + it('uses the default when unset or blank', () => { + expect(schema.parse(undefined)).toEqual([]); + expect(schema.parse(' ')).toEqual([]); + }); + + it('rejects invalid JSON without repeating the value', () => { + const result = schema.safeParse('[secret'); + expect(result.error?.issues).toEqual([ + expect.objectContaining({ message: 'must be valid JSON' }) + ]); + }); +}); + describe('parseEnv', () => { const schema = z.object({ A: envInteger(1), B: envBoolean(true) }); diff --git a/src/lib/env-schema.ts b/src/lib/env-schema.ts index 4a40f54d..623c4dc9 100644 --- a/src/lib/env-schema.ts +++ b/src/lib/env-schema.ts @@ -56,6 +56,29 @@ export function envList(defaultValue: string[], separator: RegExp | string = ',' ); } +/** JSON validated with `schema`, or `defaultValue` when unset. */ +export function envJson( + schema: T, + defaultValue: Exclude, undefined> +) { + return z.preprocess( + blankAsUnset, + z + .string() + .transform((value, ctx): unknown => { + try { + return JSON.parse(value); + } catch { + // No parser message: it can quote the value, which may hold credentials. + ctx.addIssue({ code: 'custom', message: 'must be valid JSON' }); + return z.NEVER; + } + }) + .pipe(schema) + .default(defaultValue) + ); +} + /** * Parses `env` with `schema`. Throws one error that names every invalid * variable, so a misconfigured deployment fails at startup with all problems diff --git a/src/lib/server/dashboard/global-bookmarks.test.ts b/src/lib/server/dashboard/config.test.ts similarity index 81% rename from src/lib/server/dashboard/global-bookmarks.test.ts rename to src/lib/server/dashboard/config.test.ts index e9826eff..22ef2617 100644 --- a/src/lib/server/dashboard/global-bookmarks.test.ts +++ b/src/lib/server/dashboard/config.test.ts @@ -2,11 +2,14 @@ import { describe, it, expect, vi } from 'vitest'; vi.mock('$lib/server/logging', () => import('$lib/test-utils/mock-logger.js')); -import { parseGlobalBookmarks } from './global-bookmarks.js'; +import { parseDashboardConfig } from './config.js'; + +const parseGlobalBookmarks = (value: string | undefined) => + parseDashboardConfig({ STACKABLE_COCKPIT_GLOBAL_BOOKMARKS: value }).globalBookmarks; const json = (value: unknown) => JSON.stringify(value); -describe('parseGlobalBookmarks', () => { +describe('parseDashboardConfig', () => { it('has no global bookmarks when the variable is unset or blank', () => { expect(parseGlobalBookmarks(undefined)).toEqual([]); expect(parseGlobalBookmarks(' ')).toEqual([]); @@ -47,11 +50,15 @@ describe('parseGlobalBookmarks', () => { }); it('rejects invalid JSON', () => { - expect(() => parseGlobalBookmarks('[{')).toThrow(/not valid JSON/); + expect(() => parseGlobalBookmarks('[{')).toThrow( + 'STACKABLE_COCKPIT_GLOBAL_BOOKMARKS: must be valid JSON' + ); }); it('rejects anything but an array', () => { - expect(() => parseGlobalBookmarks(json({ name: 'Wiki' }))).toThrow(/is invalid/); + expect(() => parseGlobalBookmarks(json({ name: 'Wiki' }))).toThrow( + /STACKABLE_COCKPIT_GLOBAL_BOOKMARKS: .*array/ + ); }); it.each([ @@ -64,7 +71,7 @@ describe('parseGlobalBookmarks', () => { const parse = () => parseGlobalBookmarks(json([entry, { ...entry, ...override }])); - expect(parse).toThrow('[1]'); + expect(parse).toThrow('STACKABLE_COCKPIT_GLOBAL_BOOKMARKS.1'); expect(parse).toThrow(field); }); diff --git a/src/lib/server/dashboard/config.ts b/src/lib/server/dashboard/config.ts new file mode 100644 index 00000000..0e9dbfde --- /dev/null +++ b/src/lib/server/dashboard/config.ts @@ -0,0 +1,42 @@ +// Dashboard settings: configuration, not feature toggles (those are in +// `$lib/server/feature-flags`). Parsed once at module init. + +import { z } from 'zod'; +import { env } from '$env/dynamic/private'; +import { envJson, parseEnv } from '$lib/env-schema.js'; +import { logger } from '$lib/server/logging'; +import { bookmarkFormSchema, type BookmarkLink } from '$lib/dashboard/types'; + +const log = logger.child({ module: 'dashboard-config' }); + +// Strict, so a misspelt key such as "pined" fails instead of being ignored. An +// unknown productId is accepted and shown as a custom link, as for user bookmarks. +const globalBookmarkSchema = z.strictObject({ + ...bookmarkFormSchema.shape, + environment: bookmarkFormSchema.shape.environment.default(''), + pinned: bookmarkFormSchema.shape.pinned.default(false) +}); + +const schema = z.object({ + STACKABLE_COCKPIT_GLOBAL_BOOKMARKS: envJson(z.array(globalBookmarkSchema), []) +}); + +/** + * Throws on invalid configuration. The error names the failing entries and + * fields but not their values, as URLs can contain credentials. + */ +export function parseDashboardConfig(env: Record) { + const config = parseEnv(schema, env); + const globalBookmarks: BookmarkLink[] = config.STACKABLE_COCKPIT_GLOBAL_BOOKMARKS.map( + (bookmark, index) => ({ id: `global-${index}`, ...bookmark }) + ); + return { globalBookmarks }; +} + +const config = parseDashboardConfig(env); +log.info({ count: config.globalBookmarks.length }, 'Loaded global bookmarks'); + +/** Bookmarks shown to every user, before their own, on the dashboard and in the + * sidebar. Users cannot change them. + * Controlled by `STACKABLE_COCKPIT_GLOBAL_BOOKMARKS` (JSON array). Default: none. */ +export const globalBookmarks = config.globalBookmarks; diff --git a/src/lib/server/dashboard/global-bookmarks.ts b/src/lib/server/dashboard/global-bookmarks.ts deleted file mode 100644 index 1e3809dd..00000000 --- a/src/lib/server/dashboard/global-bookmarks.ts +++ /dev/null @@ -1,57 +0,0 @@ -import { z } from 'zod'; -import { env } from '$env/dynamic/private'; -import { logger } from '$lib/server/logging'; -import { bookmarkFormSchema, type BookmarkLink } from '$lib/dashboard/types'; - -const log = logger.child({ module: 'global-bookmarks' }); - -// Strict, so a misspelt key such as "pined" fails instead of being ignored. An -// unknown productId is accepted and shown as a custom link, as for user bookmarks. -const globalBookmarksSchema = z.array( - z.strictObject({ - ...bookmarkFormSchema.shape, - environment: bookmarkFormSchema.shape.environment.default(''), - pinned: bookmarkFormSchema.shape.pinned.default(false) - }) -); - -/** - * Parse `STACKABLE_COCKPIT_GLOBAL_BOOKMARKS`: a JSON array of bookmarks shown - * to every user. Throws on invalid JSON or entries; the error names the failing - * entries and fields but not their values, as URLs can contain credentials. - */ -export function parseGlobalBookmarks(value: string | undefined): BookmarkLink[] { - if (!value?.trim()) return []; - let json: unknown; - try { - json = JSON.parse(value); - } catch { - throw new Error('STACKABLE_COCKPIT_GLOBAL_BOOKMARKS is not valid JSON'); - } - const parsed = globalBookmarksSchema.safeParse(json); - if (!parsed.success) { - throw new Error( - `STACKABLE_COCKPIT_GLOBAL_BOOKMARKS is invalid:\n${z.prettifyError(parsed.error)}` - ); - } - return parsed.data.map((bookmark, index) => ({ id: `global-${index}`, ...bookmark })); -} - -let cached: BookmarkLink[] | undefined; - -/** - * The configured global bookmarks. Parsed on first call and cached; called at - * server start so an invalid configuration stops the server. A function rather - * than a module-level constant so tests can import this file without the env var. - */ -export function globalBookmarks(): BookmarkLink[] { - if (cached) return cached; - try { - cached = parseGlobalBookmarks(env.STACKABLE_COCKPIT_GLOBAL_BOOKMARKS); - } catch (err) { - log.error({ err }, 'Invalid global bookmarks configuration'); - throw err; - } - log.info({ count: cached.length }, 'Loaded global bookmarks'); - return cached; -} diff --git a/src/routes/(app)/+layout.server.ts b/src/routes/(app)/+layout.server.ts index dd3329e7..3e47a053 100644 --- a/src/routes/(app)/+layout.server.ts +++ b/src/routes/(app)/+layout.server.ts @@ -1,7 +1,7 @@ import type { LayoutServerLoad } from './$types'; import { storageBrowserEnabled } from '$lib/server/feature-flags.js'; import { listBookmarks } from '$lib/server/dashboard/bookmarks-db.js'; -import { globalBookmarks } from '$lib/server/dashboard/global-bookmarks.js'; +import { globalBookmarks } from '$lib/server/dashboard/config.js'; import { dashboardBookmarksTotal } from '$lib/server/metrics.js'; import { BOOKMARKS_DEPENDENCY, type Bookmark } from '$lib/dashboard/types'; @@ -10,7 +10,7 @@ export const load: LayoutServerLoad = async ({ locals, depends }) => { return { user: locals.user, storageBrowserEnabled, - globalBookmarks: globalBookmarks(), + globalBookmarks, ...(await loadBookmarks(locals)) }; }; diff --git a/src/routes/(app)/layout.server.test.ts b/src/routes/(app)/layout.server.test.ts index 3591be61..b6dfe0e9 100644 --- a/src/routes/(app)/layout.server.test.ts +++ b/src/routes/(app)/layout.server.test.ts @@ -4,9 +4,7 @@ const { listBookmarks } = vi.hoisted(() => ({ listBookmarks: vi.fn() })); vi.mock('$lib/server/dashboard/bookmarks-db.js', () => ({ listBookmarks })); vi.mock('$lib/server/feature-flags.js', () => ({ storageBrowserEnabled: false })); -vi.mock('$lib/server/dashboard/global-bookmarks.js', () => ({ - globalBookmarks: () => [GLOBAL_BOOKMARK] -})); +vi.mock('$lib/server/dashboard/config.js', () => ({ globalBookmarks: [GLOBAL_BOOKMARK] })); import { load } from './+layout.server.js'; From 63ff78982fc64ef323a2c271748cfecbb668f98b Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Thu, 29 Oct 2026 15:38:59 +0100 Subject: [PATCH 7/8] test: check which sidebar list each bookmark lands in (#277) The pinned global bookmark from .env.test always shows Favourites, so the test passed even if the user's own pins stopped reaching the sidebar. --- e2e/sidebar-bookmarks.spec.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/e2e/sidebar-bookmarks.spec.ts b/e2e/sidebar-bookmarks.spec.ts index e361554e..873ab168 100644 --- a/e2e/sidebar-bookmarks.spec.ts +++ b/e2e/sidebar-bookmarks.spec.ts @@ -37,11 +37,18 @@ test.describe('Sidebar bookmarks', () => { await page.goto('/'); await waitForHydration(page); + // The pinned global bookmark from .env.test always shows Favourites, so check + // which list each of the user's bookmarks lands in. + const listWith = (name: string) => + sidebarNav(page) + .getByRole('list') + .filter({ has: page.getByRole('link', { name }) }); await expect(page.getByText('Favourites')).toBeVisible(); - await expect(sidebarNav(page).getByRole('link', { name: 'Pipelines' })).toBeVisible(); + await expect(listWith('Pipelines')).toBeVisible(); + await expect(listWith('Pipelines').getByRole('link', { name: 'Dashboards' })).toHaveCount(0); await expect(page.getByRole('button', { name: 'Tools', exact: true })).toBeVisible(); - await expect(sidebarNav(page).getByRole('link', { name: 'Dashboards' })).toBeVisible(); - await expect(sidebarNav(page).getByRole('link', { name: 'Trino' })).toBeVisible(); + await expect(listWith('Trino').getByRole('link', { name: 'Dashboards' })).toBeVisible(); + await expect(listWith('Trino').getByRole('link', { name: 'Pipelines' })).toHaveCount(0); }); test('opens each bookmark in a new tab', async ({ page }) => { From 74f3217f5a416e32979d7e5df74d9922f13999a8 Mon Sep 17 00:00:00 2001 From: Johannes Fleck Date: Mon, 2 Nov 2026 17:33:56 +0100 Subject: [PATCH 8/8] fix: reject credentials in bookmark URLs (#277) Bookmark URLs are sent to the browser, and global ones to every user, so a user name or password in the URL would be visible in the page source. They are now rejected for global bookmarks at startup and for user bookmarks in the API and the bookmark dialog. Tokens in the query string cannot be detected, so the docs now say global bookmark URLs must not contain secrets. --- .env.example | 3 ++- CHANGELOG.md | 2 +- README.md | 6 +++--- deploy/helm/cockpit/README.md | 2 +- deploy/helm/cockpit/values.yaml | 3 ++- messages/de.json | 2 +- messages/en.json | 2 +- .../dashboard/AddBookmarkModal.svelte.spec.ts | 16 ++++++++++++---- src/lib/dashboard/types.test.ts | 5 ++++- src/lib/dashboard/types.ts | 12 +++++++++++- src/lib/server/dashboard/config.test.ts | 3 ++- 11 files changed, 40 insertions(+), 16 deletions(-) diff --git a/.env.example b/.env.example index 2759e8b8..4cbd5f80 100644 --- a/.env.example +++ b/.env.example @@ -59,7 +59,8 @@ DATABASE_PASSWORD=change-me # STACKABLE_COCKPIT_STORAGE_BROWSER_ENABLED=true # Enable S3/HDFS file browser (default: false) # PUBLIC_STACKABLE_COCKPIT_STORAGE_TRANSFER_ENABLED=true # Enables cut, copy, paste, rename and drag-and-drop move in the storage browser; also enforced by the transfer API (default: false) # Bookmarks shown to every user (JSON array; productId, name and http(s) url required, environment and pinned optional). -# Invalid JSON or entries stop the server at startup. +# Invalid JSON or entries stop the server at startup. Every user can see the URLs in the page source, +# so they must not contain a user name, password, token or other secret. # STACKABLE_COCKPIT_GLOBAL_BOOKMARKS='[{"productId":"superset","name":"Dashboards","url":"https://superset.example.com"}]' # Storage browser limits and settings. Invalid values stop the server at startup. diff --git a/CHANGELOG.md b/CHANGELOG.md index 9742cb7a..cc022280 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,5 +8,5 @@ All notable changes to this project will be documented in this file. - Trino SQL query editor. - S3 file browser. -- Bookmarks. +- Bookmarks, including global bookmarks set by the administrator. - OIDC login with user impersonation in Trino. diff --git a/README.md b/README.md index d4f7dda8..f13ad6ba 100644 --- a/README.md +++ b/README.md @@ -126,9 +126,9 @@ Invalid values (e.g. `abc` for an integer) stop the server at startup with an er ### Dashboard Bookmarks -| Variable | Type | Default | Description | Example | -| ------------------------------------ | ------------ | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------- | -| `STACKABLE_COCKPIT_GLOBAL_BOOKMARKS` | `JSON` array | none | Bookmarks shown to every user, before their own, on the dashboard and in the sidebar. Users cannot change them. Each entry needs `productId` (e.g. `trino`, `superset`, or `custom`; unknown ids show as a custom link), `name` (up to 40 characters) and an HTTP(S) `url`; `environment` and `pinned` are optional. Invalid JSON or entries stop the server at startup. | `STACKABLE_COCKPIT_GLOBAL_BOOKMARKS='[{"productId":"superset","name":"Dashboards","url":"https://superset.example.com"}]'` | +| Variable | Type | Default | Description | Example | +| ------------------------------------ | ------------ | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------- | +| `STACKABLE_COCKPIT_GLOBAL_BOOKMARKS` | `JSON` array | none | Bookmarks shown to every user, before their own, on the dashboard and in the sidebar. Users cannot change them. Each entry needs `productId` (e.g. `trino`, `superset`, or `custom`; unknown ids show as a custom link), `name` (up to 40 characters) and an HTTP(S) `url`; `environment` and `pinned` are optional. URLs must not contain a user name or password, and are visible to every user in the page source, so do not put tokens or other secrets in them. Invalid JSON or entries stop the server at startup. | `STACKABLE_COCKPIT_GLOBAL_BOOKMARKS='[{"productId":"superset","name":"Dashboards","url":"https://superset.example.com"}]'` | ### Request Body Size diff --git a/deploy/helm/cockpit/README.md b/deploy/helm/cockpit/README.md index b5cfab6b..1dbba728 100644 --- a/deploy/helm/cockpit/README.md +++ b/deploy/helm/cockpit/README.md @@ -149,7 +149,7 @@ Limits and tuning for the file browser. Empty values use the application default | Parameter | Description | Default | | --- | --- | --- | -| `bookmarks.global` | Bookmarks shown to every user, before their own; users cannot change them. A list of entries with `productId` (e.g. `trino`, `superset` or `custom`; unknown ids show as a custom link), `name` (up to 40 characters) and an HTTP(S) `url`, plus optional `environment` and `pinned`. Must be a YAML list, not a JSON string. Invalid entries stop the application at startup. | `[]` | +| `bookmarks.global` | Bookmarks shown to every user, before their own; users cannot change them. A list of entries with `productId` (e.g. `trino`, `superset` or `custom`; unknown ids show as a custom link), `name` (up to 40 characters) and an HTTP(S) `url`, plus optional `environment` and `pinned`. Must be a YAML list, not a JSON string. URLs must not contain a user name or password, and are visible to every user in the page source, so do not put tokens or other secrets in them. Invalid entries stop the application at startup. | `[]` | ### Authentication Parameters diff --git a/deploy/helm/cockpit/values.yaml b/deploy/helm/cockpit/values.yaml index e1fcc9dd..872b8ba9 100644 --- a/deploy/helm/cockpit/values.yaml +++ b/deploy/helm/cockpit/values.yaml @@ -195,7 +195,8 @@ bookmarks: # productId (e.g. trino, superset or custom; unknown ids show as a custom link), # name (up to 40 characters) and an http(s) url are required; environment and # pinned are optional. Must be a YAML list, not a JSON string. Invalid entries stop - # the application at startup. + # the application at startup. Every user can see the URLs in the page source, so they + # must not contain a user name, password, token or other secret. global: [] # - productId: superset # name: Dashboards diff --git a/messages/de.json b/messages/de.json index bd7844fb..de8b46a5 100644 --- a/messages/de.json +++ b/messages/de.json @@ -509,7 +509,7 @@ "bookmark_env_placeholder": "z.B. dev, prod", "bookmark_url_label": "URL", "bookmark_url_placeholder": "https://superset.example.com", - "bookmark_url_error": "Geben Sie eine gültige HTTP- oder HTTPS-URL ein.", + "bookmark_url_error": "Geben Sie eine gültige HTTP- oder HTTPS-URL ohne Benutzername und Passwort ein.", "bookmark_pin_label": "Lesezeichen anheften", "bookmark_preview_label": "Vorschau", "bookmark_section_title": "Lesezeichen", diff --git a/messages/en.json b/messages/en.json index 4a25cd94..977ea52f 100644 --- a/messages/en.json +++ b/messages/en.json @@ -509,7 +509,7 @@ "bookmark_env_placeholder": "e.g. dev, prod", "bookmark_url_label": "URL", "bookmark_url_placeholder": "https://superset.example.com", - "bookmark_url_error": "Enter a valid HTTP or HTTPS URL.", + "bookmark_url_error": "Enter a valid HTTP or HTTPS URL without a user name or password.", "bookmark_pin_label": "Pin bookmark", "bookmark_preview_label": "Preview", "bookmark_section_title": "Bookmarks", diff --git a/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts b/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts index 6f091e0b..fe9ba659 100644 --- a/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts +++ b/src/lib/components/dashboard/AddBookmarkModal.svelte.spec.ts @@ -90,7 +90,9 @@ describe('AddBookmarkModal', () => { await page.getByLabelText('URL').fill('not a URL'); await userEvent.keyboard('{Enter}'); - await expect.element(page.getByText('Enter a valid HTTP or HTTPS URL.')).toBeVisible(); + await expect + .element(page.getByText('Enter a valid HTTP or HTTPS URL without a user name or password.')) + .toBeVisible(); expect(addBookmark).not.toHaveBeenCalled(); }); @@ -101,7 +103,9 @@ describe('AddBookmarkModal', () => { await page.getByLabelText('URL').fill('not a URL'); await page.getByRole('button', { name: 'Add bookmark' }).click(); - await expect.element(page.getByText('Enter a valid HTTP or HTTPS URL.')).toBeVisible(); + await expect + .element(page.getByText('Enter a valid HTTP or HTTPS URL without a user name or password.')) + .toBeVisible(); await expect.element(page.getByLabelText('URL')).toHaveAttribute('aria-invalid', 'true'); expect(addBookmark).not.toHaveBeenCalled(); }); @@ -113,7 +117,9 @@ describe('AddBookmarkModal', () => { await page.getByLabelText('URL').fill('javascript:alert(1)'); await page.getByRole('button', { name: 'Add bookmark' }).click(); - await expect.element(page.getByText('Enter a valid HTTP or HTTPS URL.')).toBeVisible(); + await expect + .element(page.getByText('Enter a valid HTTP or HTTPS URL without a user name or password.')) + .toBeVisible(); await expect.element(page.getByLabelText('URL')).toHaveAttribute('aria-invalid', 'true'); expect(addBookmark).not.toHaveBeenCalled(); }); @@ -160,7 +166,9 @@ describe('AddBookmarkModal', () => { await page.getByLabelText('URL').fill('superset.corp'); await page.getByLabelText('Name').click(); - await expect.element(page.getByText('Enter a valid HTTP or HTTPS URL.')).toBeVisible(); + await expect + .element(page.getByText('Enter a valid HTTP or HTTPS URL without a user name or password.')) + .toBeVisible(); }); it('prefills the default name of the preselected product', async () => { diff --git a/src/lib/dashboard/types.test.ts b/src/lib/dashboard/types.test.ts index 471d9091..5e2fa735 100644 --- a/src/lib/dashboard/types.test.ts +++ b/src/lib/dashboard/types.test.ts @@ -20,7 +20,10 @@ describe('bookmark URL validation', () => { 'data:text/html,', 'ftp://files.example.com', 'superset.corp', - 'http://' + 'http://', + 'https://admin:secret@superset.example.com', + 'https://token@superset.example.com', + 'https://:secret@superset.example.com' ])('rejects %s', (url) => { expect(urlSchema.safeParse(url).success).toBe(false); }); diff --git a/src/lib/dashboard/types.ts b/src/lib/dashboard/types.ts index 62171b16..9e583a54 100644 --- a/src/lib/dashboard/types.ts +++ b/src/lib/dashboard/types.ts @@ -14,7 +14,17 @@ export const bookmarkFormSchema = z.object({ productId: z.string().min(1).max(64), name: z.string().trim().min(1).max(BOOKMARK_MAX_LENGTH.name), environment: z.string().trim().max(BOOKMARK_MAX_LENGTH.environment), - url: z.url({ protocol: /^https?$/ }).max(BOOKMARK_MAX_LENGTH.url), + // No user name or password: bookmark URLs are sent to the browser, and global + // ones to every user. + url: z + .url({ protocol: /^https?$/ }) + .max(BOOKMARK_MAX_LENGTH.url) + // Zod runs this even when the URL check above failed, which reports that case. + .refine((url) => { + if (!URL.canParse(url)) return true; + const { username, password } = new URL(url); + return !username && !password; + }, 'URL must not contain a user name or password'), pinned: z.boolean() }); diff --git a/src/lib/server/dashboard/config.test.ts b/src/lib/server/dashboard/config.test.ts index 22ef2617..700a5c58 100644 --- a/src/lib/server/dashboard/config.test.ts +++ b/src/lib/server/dashboard/config.test.ts @@ -63,6 +63,7 @@ describe('parseDashboardConfig', () => { it.each([ ['a non-HTTP URL', { url: 'javascript:alert(1)' }, 'url'], + ['credentials in the URL', { url: 'https://user:secret@wiki.example.com' }, 'url'], ['a missing name', { name: undefined }, 'name'], ['a name above the length limit', { name: 'x'.repeat(41) }, 'name'], ['a misspelt key', { pined: true }, 'pined'] @@ -76,7 +77,7 @@ describe('parseDashboardConfig', () => { }); it('does not repeat the configured values in the error', () => { - const url = 'ftp://user:secret@files.example.com'; + const url = 'https://user:secret@files.example.com'; expect(() => parseGlobalBookmarks(json([{ productId: 'custom', name: 'Files', url }]))).toThrow( expect.objectContaining({ message: expect.not.stringContaining('secret') })