harden(deploy): add CSP + security headers, drop external Google Fonts - #1
Merged
Merged
Conversation
Two defense-in-depth fixes from the security review: 1. Self-contained fonts. Remove the Google Fonts <link>s from build/template.html; the --ui/--mono CSS stacks already fall back to system fonts. The served page now makes zero third-party requests (privacy + air-gap + truly self-contained). 2. Security headers. deploy/http_handlers.xml now sends a strict CSP (default-src 'none'; connect-src 'self' <issuer-origins>; frame-ancestors 'none'; base-uri 'none'; img-src data:; script/style 'unsafe-inline' since the bundle is inlined), plus nosniff and Referrer-Policy: no-referrer. connect-src is the real win — it bounds where the sessionStorage tokens can be sent if an XSS ever lands. install.sh resolves the issuer's OIDC discovery and rewrites connect-src to the real issuer + token-endpoint origins (fail-soft to the Google default if discovery is unreachable), writing the rendered file to dist/http_handlers.xml. New --dry-run flag renders config.json + http_handlers.xml and prints them with no ClickHouse contact. README + DEPLOYMENT docs updated. No src/ changes; 319 tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QennTvGKAtJZrv9EpQagef
…pkce transients Security-review follow-ups (#4/#6/#7): - sqlString (core/format.js): escape `\` -> `\\` before doubling `'`. CH honors backslash escapes in string literals, so a value ending in `\` escaped the closing quote and could break out (second-order via loadColumns' system.tables names). Now closed. - bootstrap (main.js): handle an IdP error redirect (?error=access_denied&…) instead of dropping silently to the login screen — surfaces error_description||error, and the URL cleanup now strips the error params too. - setTokens (ui/app.js): clear the one-shot oauth_verifier + oauth_state from sessionStorage once tokens are held (refresh path is a harmless no-op). Tests added in the same change; 323 pass, per-file coverage gate holds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QennTvGKAtJZrv9EpQagef
The editor overlays a <textarea> on a highlight <pre>. With a fractional line box (line-height 1.7 * 13px = 22.1px) the textarea's internal text layout and the block <pre> rounded lines differently, so the native selection drifted upward from the painted glyphs, growing with line number (visible by ~line 37). Use an integer px line-height (22px) on .sql-editor so both lay out lines identically before `zoom: 1.2` scales them together, and match .sql-gutter > div height so the line numbers stay row-aligned. Verified in-browser: selection on a deep line now brackets the glyphs exactly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QennTvGKAtJZrv9EpQagef
In TSV/JSON output mode the result is a plain <div>, so ⌘A fell through to the browser's whole-page select-all. Make the raw (.raw-text-view) and JSON (.json-view) panes focusable (tabindex=0) and, when ⌘/Ctrl+A fires with focus inside one, select that node's contents via Selection.selectAllChildren so it can be copied. Focus elsewhere (e.g. the editor textarea) still falls through to the native select-all, so ⌘A in the SQL area keeps selecting the whole query. Verified in-browser: ⌘A in a focused TSV pane selects exactly the TSV text; ⌘A in the editor is not intercepted. 325 tests pass; shortcuts/results at 100%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QennTvGKAtJZrv9EpQagef
Each data-column header gets a right-edge resize handle. Dragging it freezes every column at its current width, switches the table to table-layout:fixed, and tracks the cursor to set that column's width (content ellipsizes when narrowed). Widths are stored on the per-query result object so they survive the frequent re-renders (sort, streaming chunks, view switches) and reset on a new query. Details: - zoom-safe: the client-px delta is divided by a per-element scale (getBoundingClientRect().width / offsetWidth) so the edge tracks the cursor under the global `zoom: 1.2` (verified in-browser: 70 CSS px → 84 device px). - the handle swallows its own click so resizing/clicking it never triggers the column sort; the row-number column is not resizable. Verified in-browser: dragging narrows a column with ellipsis clipping while others keep their widths. 329 tests pass; results.js at 100%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QennTvGKAtJZrv9EpQagef
History had no in-app way to clear it (only the saved panel had per-item delete). Add a "Clear history" button in the History panel header (wires the existing clearHistory) and a per-row delete × (new deleteHistory state op, mirroring deleteSaved). Both persist via localStorage like the rest of the panel. The row-level delete stops propagation so it doesn't also load the query. Verified in-browser: Clear history empties asb:history + the list; per-row delete removes one entry. 332 tests pass; state.js + saved-history.js at 100%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QennTvGKAtJZrv9EpQagef
BorisTyshkevich
added a commit
that referenced
this pull request
Jun 23, 2026
…t, qualified fallback, doc-cache, single tokenize (#26, #27) Five findings from a follow-up manual review; verified live on otel. Correctness: - Hover docs now resolve the hovered word from the string/comment-masked text (host.maskedValue()), so hovering a function/keyword inside a string or comment no longer pops a phantom doc card — consistent with signature help (#1). - Signature help strips the optional-param brackets ClickHouse uses (`name(a, b[, c])`) before splitting on commas, so args render cleanly and the active-arg highlight aligns (was showing `offset[` / `length]` for substring) (#2). - completionContext only flags `qualified` when a real identifier precedes the dot; a bare dot (`.col`, `count().c`) now falls back to normal completion instead of an empty dropdown (#4). - loadEntityDoc returns null on a query FAILURE vs '' for genuinely-no-doc, and entityDoc caches the latter but drops the former — a transient error no longer permanently suppresses a function's hover/footer doc for the session (#8). Performance: - The keystroke path tokenizes the buffer ONCE and shares the token list between the syntax highlighter and the literal mask, instead of two full passes per keystroke. maskFromTokens()/renderTokensInto() consume a token list; the editor memoizes it by (text, refData) so it re-tokenizes when server keyword/func sets arrive after connect (#5). Tests cover each: hover-in-literal, bracketed-param signature split, bare-dot fallback, failed-doc retry, and re-highlight-on-refData-change (the shared-token cache invalidation). All gated layers stay 100/100/100/100 (ui glue within its functions>=95 / branches>=90 floor). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGBS74oUsXarGkCRQKEFLu
BorisTyshkevich
added a commit
that referenced
this pull request
Jun 25, 2026
- run(): a typed EXPLAIN now honors exactly what was typed — a plain EXPLAIN always opens the verbatim Explain view instead of inheriting a stale rich view from a previous run/tab. Drop the unused app.state.explainView. (review #1) - dot.parseDot: blank quoted label strings before edge scanning and only keep edges between declared processors, so a `->` inside a label (e.g. a lambda `x -> x + 1`) no longer fabricates phantom nodes/edges. (review #2) - panzoom.zoomBox: guard a degenerate zero-size viewBox (defensive). - overlay: Esc stops propagation so it doesn't also reach the app key handler. - remove now-dead isExplain from core/format.js (superseded by parseExplain). - document the ontime DOT fixture's capture provenance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGBS74oUsXarGkCRQKEFLu
BorisTyshkevich
added a commit
that referenced
this pull request
Jun 25, 2026
…line/Estimate) + Explain button (#37) * feat(explain): EXPLAIN result views (Explain/Indexes/Projections/Pipeline/Estimate) + Explain button Replace the one-dimensional raw EXPLAIN dump with five views in the data pane: - Explain (default) runs the user's EXPLAIN verbatim as clean TabSeparatedRaw, so arbitrary/complex parameters are honored. - Indexes / Projections derive `indexes=1` / `projections=1` from the inner query. - Pipeline derives `EXPLAIN PIPELINE graph=1` and renders the Graphviz DOT as a zero-dep SVG layered graph (pure parse+layout in src/core/dot.js). - Estimate derives `EXPLAIN ESTIMATE` and renders structured rows as a table. On Run we auto-select a rich view only when the typed statement is exactly that canonical form; anything else stays on the verbatim Explain tab. Switching a view re-runs the derived query and never edits the editor SQL. New "Explain" toolbar button (between Format and Save) explains any query without editing it. No new runtime dependency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGBS74oUsXarGkCRQKEFLu * docs(explain): README EXPLAIN-views section; neutral empty-Estimate message Reword the empty-Estimate placeholder (a trivial count() is answered from metadata, so ESTIMATE returns no part rows even on a MergeTree table). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGBS74oUsXarGkCRQKEFLu * feat(explain): vertical pipeline graph, drop EXPLAIN header stats, e2e graph test - Pipeline graph now lays out top→bottom: sequential stages stack vertically, parallel processors in a stage sit side-by-side horizontally. Edges flow bottom→top-centre; self-loops are filtered. - EXPLAIN views drop the ms/rows/bytes header stats (not meaningful for a plan) to give the five tabs room. - Add a Playwright e2e test that renders a real `EXPLAIN PIPELINE graph = 1` capture from the antalya ontime dataset (fact/dim join + aggregated subquery join, 37 processors / 40 edges) and asserts the vertical-with-parallel-lanes SVG. Fixture: tests/e2e/fixtures/ontime-pipeline-graph.dot. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGBS74oUsXarGkCRQKEFLu * feat(explain): fullscreen pan/zoom overlay for the pipeline graph Add an Expand button (results header, Pipeline view) that opens the graph in a fullscreen overlay with wheel-zoom around the cursor, drag-pan, +/- and Fit-to-screen controls, and Esc/✕/backdrop close. Makes complex pipelines (which the current layout draws wide) navigable; better automatic layout is deferred to a follow-up PR. - src/core/panzoom.js: pure viewBox algebra (fit/zoom/pan), 100% covered. - src/ui/explain-graph.js: extract buildPipelineSvg; openPipelineFullscreen. - src/ui/results.js: Expand button; icons.js: expand + minus glyphs. - e2e: pipeline.html exposes __openFullscreen; spec drives real wheel/drag/Esc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGBS74oUsXarGkCRQKEFLu * fix(explain): address code-review findings - run(): a typed EXPLAIN now honors exactly what was typed — a plain EXPLAIN always opens the verbatim Explain view instead of inheriting a stale rich view from a previous run/tab. Drop the unused app.state.explainView. (review #1) - dot.parseDot: blank quoted label strings before edge scanning and only keep edges between declared processors, so a `->` inside a label (e.g. a lambda `x -> x + 1`) no longer fabricates phantom nodes/edges. (review #2) - panzoom.zoomBox: guard a degenerate zero-size viewBox (defensive). - overlay: Esc stops propagation so it doesn't also reach the app key handler. - remove now-dead isExplain from core/format.js (superseded by parseExplain). - document the ontime DOT fixture's capture provenance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QGBS74oUsXarGkCRQKEFLu --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
7 tasks
This was referenced Jul 22, 2026
BorisTyshkevich
added a commit
that referenced
this pull request
Jul 26, 2026
Review findings on the branch. **`create-first` could still destroy Dashboard #1.** The `dashboardCount === 0` gate was read when the MENU OPENED, but the commit lands after a file chooser, a multi-dashboard picker, or a conflict dialog — and a second tab (or the placeholder's own Create button, which stays live behind a non-modal chooser) can seat a Dashboard in that window. `create-first` commits through the compatibility slot, and `withCompatibilityDashboard` unconditionally replaces entry 0, so the import would have silently deleted a Dashboard it never named, with no confirm and no diagnostic. `planImportDashboard` now re-checks at DEQUEUE time against the same baseline every other target is validated against, behind `requireEmptyCollection`, and fails closed with `dashboard-import-collection-not-empty`. Same shape as the existing stale-explicit-target path. Sabotage-checked. **Disabled rows were not announced.** `menu.ts` set the native `disabled` attribute, which drops the button from the accessibility tree — so a screen reader never reached "Export Dashboard…, Open a dashboard". Under #452 the disabled state IS how the menu explains itself, and the issue requires rows to be "announced but cannot activate". Now `aria-disabled` only: the row stays in the roving-focus order and is announced, and activation is removed by having no handler. Reverses the #426 choice for the shared primitive, so the tree menu's spec moves with it. Also from review: - `triggerImportDashboard` deleted — no production caller remained after the menu took over, its docstring claimed a teardown that `disposeFileMenuOverlays` never performed, and its body-mounted input was the one surviving cancelled-chooser leak. It was also a second implementation of one operation. - `.file-menu` widened to 288px leaked to the Dashboard tree menu and style picker; the width moved to a scoped `.app-file-menu`, which is now the File menu's own test selector. - Issue tests 12 and 13 were uncovered: added focus-restoration assertions on both surfaces (Escape and toggle-close) and a leak sweep that cancels every picker across a surface switch. - Pickers carry `data-picker="<row id>"` so specs stop depending on append order. - Dropped an unused `replaceDashboard` import and two stale comments. Part of #452 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pNgXnY3YpXzkUidL81vqu
lesandie
pushed a commit
to lesandie/altinity-sql-browser
that referenced
this pull request
Aug 2, 2026
The File button changed meaning with the work surface. The Dashboard had its own menu (import/export only, dropping every workspace and Library operation), Dashboard View dropped a row again, the two implementations grouped and ordered their rows differently, and only one of the two triggers toggled shut when clicked again. Now every surface — Query, Dashboard Edit, Dashboard View, the empty-Dashboard placeholder, the Dashboard workspace-not-found fallback — renders the same control. Context changes enabled state only; a row never disappears or moves. - `src/core/file-menu-model.ts` (new, pure, 100%) owns the eight rows, their order, the availability rules and the footer. Primitives in, descriptors out. It declares its own `'view' | 'edit'` literal rather than importing `DashboardSurfaceMode`, which would drag `src/workspace/` into `src/core/`. - `src/ui/file-menu.ts` is the one control: trigger (toggle, focus-restore), rows painted from the model, picker lifecycle, keyboard owner, dispatch. - `buildDashboardFileMenu`, `AppHeaderOptions.fileButton` and `ActionsRegistry.exportDashboard`/`.importDashboard` are gone; surfaces pass context only. Dashboard commands now take an explicit target instead of re-reading `selectedDashboardId(app.mainSurface)`, which returned `null` in Query mode and silently made both act on the collection's *first* Dashboard. Import onto the empty placeholder creates the first Dashboard, and is offered only when the collection is genuinely empty — that placeholder is also reached when a selection stops resolving against a non-empty collection, where `create-first` would have overwritten Dashboard Altinity#1. Export keeps Altinity#341's degraded fallback but gates it on id equality, so it still never becomes a silent no-op and still never retargets. A target removed mid-dialog fails closed with no commit. Presentation: disabled rows follow the settled `.is-disabled` treatment with `aria-disabled`, are skipped by keyboard navigation, and state their reason on their own line — a real-browser check showed reasons clipping mid-word beside the label, so `.fm-reason` wraps and `.file-menu` widened 252px → 288px to fit "Download Library as Markdown" plus its extension. README and the File-menu subsection of Altinity#408 are reconciled. Filed Altinity#454 (import bounces to Query, pre-existing) and Altinity#455 (check-boundaries does not enforce the `src/core` rule its comments cite). Part of Altinity#452 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pNgXnY3YpXzkUidL81vqu
BorisTyshkevich
added a commit
that referenced
this pull request
Aug 5, 2026
… retire dashboardTreeRevision app.currentWorkspace/app.mainSurface become signal-backed accessor pairs (peeking getter, notifying setter) so a mutation is its own notification — the #426/#427 bug class (a write site forgetting to bump the invalidation counter) becomes structurally impossible. app.committedWorkspace (ReadonlySignal<StoredWorkspaceV5|null>) and app.treeNavigation (a computed structural key over kind/dashboardId/currentMember) are the two tracked reads app-shell.ts's tab-count/tree/Library effects subscribe through, replacing state.dashboardTreeRevision. app.currentWorkspace's setter is asymmetric (no null): a transitional null publication is a named departure operation owned by a new closure-private surface-retirement coordinator in app.ts (retireToWorkspaceLoading/ -Missing/-Failure/-Login), which batches the publication atomically with disposing any live shell so a surface never repaints against transitional state — five independent review passes each found a different call site where this raced disposal, hence one coordinator with exclusive mutation authority rather than five patches. SurfaceStatePort/DashboardApp/TabsApp/ DashboardTreeApp are narrowed to readonly on currentWorkspace (the fourth, DashboardTreeApp, was documented read-only but not type-readonly before this change). app.reloadDashboardRoute() (a post-commit fold-and-reassign that would double-publish once the aggregate is signal-backed) is deleted; afterLibraryChange's Dashboard branch calls the render-only app.renderCurrentSurface() instead. Tests: tests/unit/surface-lifecycle-arch.test.ts (a static-source scan backing the coordinator's compile/scan-layered "no lifecycle bypass" claim) and tests/unit/surface-accessor-contracts.test.ts (@ts-expect-error fixtures for the asymmetric setter and the four narrowed ports) are new, plus a new app.test.ts describe block covering the issue's Tests #1/#3/#5 and the plan's invariant map (delivery-only no-ops, adversarial-id collision-freedom, failure-path status/null ordering, the four-arm live-shell no-repaint sweep, one-commit-exactly-once settlement, and the mixed-snapshot batch-ordering regression). fake-app.ts's makeApp() installs real per-call signals on the returned object (object spread evaluates an accessor pair into a plain value, so the fake needs the same defineProperty treatment createApp() gets natively) for reactivity parity in fixtures. No persisted/schema change, no user-visible behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defense-in-depth fixes from the security review. No
src/logic changes — the 100% coverage gate is untouched (319 tests pass).1. Truly self-contained — drop external Google Fonts
build/template.htmlno longer pulls Inter / JetBrains Mono fromfonts.googleapis.com/gstatic.com. The--ui/--monostacks instyles.cssalready fall back to system fonts, so the UI is unchanged in spirit and the served page now makes zero third-party requests (privacy, air-gap, and the "self-contained single file" claim all become literally true). The favicon stays an inlinedata:URI.2. Strict CSP + hardening headers
The
/sqlresponse (deploy/http_handlers.xml) now sends:Content-Security-Policy:default-src 'none'; script-src 'unsafe-inline'; style-src 'unsafe-inline'; img-src data:; font-src 'self'; connect-src 'self' <issuer-origins>; base-uri 'none'; frame-ancestors 'none''unsafe-inline'is required because the JS+CSS are inlined into the single HTML file; the real protection isconnect-src, which bounds where thesessionStorageid_token/refresh_token can be sent if an XSS ever lands.X-Content-Type-Options: nosniffandReferrer-Policy: no-referrer(on both the SPA and config responses).connect-src is templated, not guessed
http_handlers.xmlis normally a static drop-in, but the OIDC origins are deployment-specific. Soinstall.shnow:connect-srcinto a rendereddist/http_handlers.xml;--dry-runflag that rendersconfig.json+http_handlers.xmland prints them with no ClickHouse contact.Non-Google manual installs: edit one
connect-srcline (documented in README +deploy/http_handlers.xmlcomment).Verification
npm test→ 319 pass;npm run build→grep googleapis|gstatic dist/sql.html= 0 matches.xmllintclean on committed + rendered XML.sedrewrite verified against Google- and Auth0-shaped discovery docs (Google → 2 origins, Auth0 → 1;'self'and trailing;preserved).🤖 Generated with Claude Code
https://claude.ai/code/session_01QennTvGKAtJZrv9EpQagef