diff --git a/.env.development b/.env.development new file mode 100644 index 00000000..e69de29b diff --git a/AGENTS.md b/AGENTS.md index a747c0e9..a5898272 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -34,9 +34,14 @@ This is a **single SvelteKit application** (not a monorepo). ├── e2e/ # Playwright E2E tests ├── static/ # Static assets ├── docker/Dockerfile # Production container image -└── CLAUDE.md # AI assistant instructions +├── CLAUDE.md # AI assistant instructions +└── TECH_DEBT.md # Known tech debt and deferred security concerns ``` +## Tech Debt + +When introducing shortcuts, known issues, or deferred security work, add an entry to `TECH_DEBT.md`. Keep entries concise: what the issue is, why it is acceptable now, and what the correct long-term fix is. + ## Development Guidelines ### Browser Compatibility diff --git a/TECH_DEBT.md b/TECH_DEBT.md new file mode 100644 index 00000000..88998f8b --- /dev/null +++ b/TECH_DEBT.md @@ -0,0 +1,79 @@ +# Tech Debt + +Tracked issues that are acceptable at the current early stage but must be addressed before production. + +--- + +## Security + +### No authentication or authorisation on the Trino API route + +**File:** `src/routes/api/trino/query/+server.ts`, `src/hooks.server.ts` + +The `/api/trino/query` endpoint is completely unauthenticated. Any request — from any origin — can execute arbitrary SQL against any Trino instance. OIDC authentication is planned (env vars are wired up, `hooks.server.ts` has the right structure) but not yet implemented. Until auth middleware is in place there is also no per-user rate limiting or query quota. + +--- + +### Trino credentials stored in localStorage + +**File:** `src/routes/(app)/trino/+page.svelte:41–44` + +Username and password are persisted in plaintext localStorage. This is convenient for development (survives page reloads) but violates credential storage best practices — localStorage is accessible to any script on the page and visible in DevTools. Long-term the connection config should be stored server-side (tied to the authenticated session), with credentials never leaving the server after initial setup. + +--- + +### Credentials sent in every request body + +**File:** `src/routes/(app)/trino/+page.svelte:82–84` + +Because there is no server-side session yet, connection credentials (including password) are included in the JSON body of every `/api/trino/query` POST. Once server-side sessions exist the client should send only a session token, not raw credentials. + +--- + +### Raw upstream error messages returned to the client + +**File:** `src/routes/api/trino/query/+server.ts:165, 181, 209` + +Trino error messages and Node.js exception messages are returned to the browser without any sanitisation. Trino errors may expose schema details, table names, or internal query plans. These should be classified (query error vs. infrastructure error) and sanitised before being surfaced to users. + +--- + +## API & Validation + +### API route request body not validated with Zod + +**File:** `src/routes/api/trino/query/+server.ts:83–103` + +`parseConnection` uses manual `typeof` checks instead of a Zod schema. The AGENTS.md guidelines require Zod for all validation. Additionally, `request.json()` is called without a try/catch — a malformed JSON body will throw an unhandled error rather than returning a 400. + +--- + +### In-memory query cache has no total size bound + +**File:** `src/routes/api/trino/query/+server.ts:40–47` + +Each cached query can hold up to `MAX_CACHED_ROWS` (100 000) rows. `evictStale()` is only called when a new query arrives, not on a timer, so a long idle period followed by many concurrent queries could accumulate significant memory before eviction runs. Needs a bounded cache (e.g. LRU with a memory cap) and a periodic eviction timer. + +--- + +### Displayed results not cleared on connection change + +**File:** `src/routes/(app)/trino/+page.svelte:40–46` + +The `$effect` that persists connection settings only resets `queryId`, not `rows`, `columns`, or `error`. After switching to a different Trino instance the previous result set remains visible until a new query is run, which is confusing. + +--- + +## Infrastructure + +### No Content Security Policy headers + +No CSP headers are set anywhere. This leaves the app exposed to XSS in ways that a strict CSP would mitigate. Should be added in a SvelteKit hook once the app stabilises. + +--- + +### `allowedHosts: true` in Vite config + +**File:** `vite.config.ts` + +The dev server accepts requests from any host. This enables DNS rebinding attacks against local development environments. Should be restricted to `localhost` / `127.0.0.1` unless remote dev access is explicitly needed. diff --git a/e2e/helpers.ts b/e2e/helpers.ts new file mode 100644 index 00000000..42d1957d --- /dev/null +++ b/e2e/helpers.ts @@ -0,0 +1,11 @@ +import type { Page } from '@playwright/test'; + +/** + * Wait for SvelteKit client-side hydration to complete. + * + * The root layout adds a `hydrated` class to `
` inside `onMount`, + * which fires after hydration finishes and the app is fully interactive. + */ +export async function waitForHydration(page: Page) { + await page.locator('body.hydrated').waitFor(); +} diff --git a/e2e/i18n.spec.ts b/e2e/i18n.spec.ts index e8044d1b..e9a9a99c 100644 --- a/e2e/i18n.spec.ts +++ b/e2e/i18n.spec.ts @@ -1,4 +1,5 @@ import { test, expect } from '@playwright/test'; +import { waitForHydration } from './helpers'; test.describe('Internationalisation', () => { test.use({ locale: 'en-US' }); @@ -31,9 +32,7 @@ test.describe('Internationalisation', () => { await expect(page.locator('html')).toHaveAttribute('lang', 'en'); // Wait for client hydration; locale switch relies on an attached click handler. - await expect - .poll(() => page.evaluate(() => localStorage.getItem('theme'))) - .toMatch(/^(light|dark)$/); + await waitForHydration(page); await page.getByRole('button', { name: 'Language' }).click(); const englishOption = page.locator('#lang-switcher button[lang="en"]'); diff --git a/e2e/smoke.spec.ts b/e2e/smoke.spec.ts index bcc29f04..77e97478 100644 --- a/e2e/smoke.spec.ts +++ b/e2e/smoke.spec.ts @@ -1,4 +1,5 @@ import { test, expect } from '@playwright/test'; +import { waitForHydration } from './helpers'; test.describe('Smoke tests', () => { test.use({ locale: 'en-US' }); @@ -20,9 +21,10 @@ test.describe('Smoke tests', () => { // Dashboard content is rendered await expect(page.getByText('Welcome back')).toBeVisible(); - // Trino nav item is present but disabled + // Trino nav item is present and navigable const trinoLink = page.getByRole('link', { name: 'Trino' }); - await expect(trinoLink).toHaveAttribute('aria-disabled', 'true'); + await expect(trinoLink).toBeVisible(); + await expect(trinoLink).not.toHaveAttribute('aria-disabled', 'true'); }); test('theme toggle switches between light and dark', async ({ page }) => { @@ -34,9 +36,7 @@ test.describe('Smoke tests', () => { }); // Wait for client hydration/theme initialisation before interacting. - await expect - .poll(() => page.evaluate(() => localStorage.getItem('theme'))) - .toMatch(/^(light|dark)$/); + await waitForHydration(page); await expect(html).toHaveAttribute('data-theme', /^(light|dark)$/); await expect(toggle).toBeVisible(); diff --git a/e2e/trino.spec.ts b/e2e/trino.spec.ts new file mode 100644 index 00000000..089b96d2 --- /dev/null +++ b/e2e/trino.spec.ts @@ -0,0 +1,257 @@ +import { test, expect } from '@playwright/test'; +import * as http from 'node:http'; +import type { AddressInfo } from 'node:net'; +import { waitForHydration } from './helpers'; + +const COLUMNS = [ + { name: 'id', type: 'integer' }, + { name: 'name', type: 'varchar' } +]; + +// Starts a lightweight HTTP server that acts as a mock Trino endpoint. +// The server action fetches `{trino_url}/v1/statement` from the SvelteKit server +// (Node.js), so we need a real TCP server reachable by the server process. +// page.route() only intercepts browser-side requests and cannot mock server-side +// Node.js fetch calls. +async function startMockTrinoServer( + handler: (req: http.IncomingMessage, res: http.ServerResponse) => void +): Promise<{ url: string; stop: () => Promise{m.trino_query_error()}
+{queryError}
+| {col.name} | + {/each} +
|---|
| + {#if cell === null} + null + {:else} + {String(cell)} + {/if} + | + {/each} +
{m.trino_results_empty()}
+ {/if} +