diff --git a/frontend/assets/app.js b/frontend/assets/app.js index 78a765b..aaf04f3 100644 --- a/frontend/assets/app.js +++ b/frontend/assets/app.js @@ -65,14 +65,21 @@ const reducedMotion = typeof matchMedia === "function" && matchMedia("(prefers-reduced-motion: reduce)").matches; const SCROLL = reducedMotion ? "auto" : "smooth"; -/* ---------- document viewer link (phase 10) ---------- - * Every cited document opens in the viewer, in a NEW tab. Both query +/* ---------- document viewer link (phase 10; phase 13 adds `back`) ---------- + * Every cited document opens in the viewer, in a NEW tab. All query * values are percent-encoded: real paths contain slashes and sometimes - * spaces, which would otherwise corrupt the query string. (The renderer + * spaces, which would otherwise corrupt the query string. `back` tells the + * viewer which page to return to when its back button is clicked — the + * chips live in the chat, so chat passes "/" (the viewer validates it: + * only same-origin relative URLs are honored; Sources links omit it and + * get the viewer's /sources.html default). (The renderer * renderMarkdown/escapeHtml now lives in assets/markdown.js — a classic * script loaded by index.html and document.html before these modules.) */ -export function documentUrl(source, path) { - return "/document.html?source=" + encodeURIComponent(source) + "&path=" + encodeURIComponent(path); +export function documentUrl(source, path, back = "/") { + let url = + "/document.html?source=" + encodeURIComponent(source) + "&path=" + encodeURIComponent(path); + if (back) url += "&back=" + encodeURIComponent(back); + return url; } /* ---------- avatar glyphs (phase 08: emoji-free chrome) ---------- @@ -291,7 +298,7 @@ function appendSources(wrap, sources) { const chip = document.createElement("a"); chip.className = "source-chip"; chip.setAttribute("role", "listitem"); - chip.href = documentUrl(s.source, s.path); + chip.href = documentUrl(s.source, s.path, "/"); // back → the chat page chip.target = "_blank"; // open the full document in a new tab chip.rel = "noopener"; chip.textContent = label; diff --git a/frontend/assets/document.js b/frontend/assets/document.js index 2c63f28..c1a5b23 100644 --- a/frontend/assets/document.js +++ b/frontend/assets/document.js @@ -28,14 +28,31 @@ const notFoundEl = document.querySelector("#doc-not-found"); const mainEl = document.querySelector("#main"); const backLink = document.querySelector("#doc-back"); -/* Back: prefer the browser's own history when there is one (the viewer was - * opened from this tab's session); a fresh tab lands on the Sources page. */ -backLink.addEventListener("click", (e) => { - if (window.history.length > 1) { - e.preventDefault(); - window.history.back(); - } -}); +/* Back button (phase 13): the return target comes from the `back` query + * param, not the browser history — both entry points (chat source chips + * and the Sources table) open the viewer in a NEW tab, where there is no + * history to go back to. The param is honored only for same-origin + * relative URLs (starts with "/" but not "//"), so absolute (https://…), + * protocol-relative (//…), and pseudo-protocol (javascript:…) values are + * rejected; anything else falls back to the Sources page. The static + * href="/sources.html" in document.html remains the no-JS fallback, and + * with the href set the anchor's default click behavior IS the + * deterministic navigation (no browser-history heuristics). */ +const backParam = params.get("back") || ""; +const backTarget = + backParam.startsWith("/") && !backParam.startsWith("//") + ? backParam + : "/sources.html"; +backLink.href = backTarget; +const backLabel = backLink.querySelector("span"); +if (backLabel) { + backLabel.textContent = + backTarget === "/" + ? "Chat" + : backTarget === "/sources.html" + ? "Sources" + : "Back"; +} function fmtDate(iso) { try { diff --git a/tests/e2e/test_chat_rag.py b/tests/e2e/test_chat_rag.py index 5ae4bdb..9cace14 100644 --- a/tests/e2e/test_chat_rag.py +++ b/tests/e2e/test_chat_rag.py @@ -98,12 +98,13 @@ def test_on_topic_question_streams_grounded_answer( # Grounded: a kubernetes.md source chip renders under the bubble # (top-N docs can add more chips; the question's doc must be among them). - # Phase 10: chips open the document viewer in a new tab (encoded URL). + # Phase 10: chips open the document viewer in a new tab (encoded URL); + # phase 13 appends back=/ so the viewer's back button returns to chat. chip = page.locator(".msg.brain .source-chip", has_text="kubernetes.md") expect(chip).to_have_count(1) expect(chip.first).to_contain_text("kubernetes.md") expect(chip.first).to_have_attribute( - "href", "/document.html?source=docs&path=homelab%2Fkubernetes.md" + "href", "/document.html?source=docs&path=homelab%2Fkubernetes.md&back=%2F" ) expect(chip.first).to_have_attribute("target", "_blank") expect(chip.first).to_have_attribute("rel", "noopener") diff --git a/tests/e2e/test_document_back_navigation.py b/tests/e2e/test_document_back_navigation.py new file mode 100644 index 0000000..55d64b4 --- /dev/null +++ b/tests/e2e/test_document_back_navigation.py @@ -0,0 +1,206 @@ +"""Phase 13 E2E (Playwright): the viewer's back button returns to the +page the document was opened from. + +Story: ``.agent/user_stories/document-back-navigation.md`` +Run in isolation (DB must be up: ``podman compose up -d db``): + + uv run pytest tests/e2e/test_document_back_navigation.py -v --no-cov + +Both entry points (chat source chips, Sources table links) open the viewer +in a NEW tab, where there is no browser history — so the return target is +carried in the viewer URL: chat chips append ``&back=%2F`` (resolves to +"Chat"), Sources links omit the param (the viewer's default +``/sources.html`` applies → "Sources"). The viewer only honors +same-origin relative ``back`` values; everything else falls back to +``/sources.html``. + +Test → story mapping (Playwright Mapping Rule): +1. ``test_back_from_chat_returns_to_chat`` — question → source chip → + new tab with ``&back=%2F`` → back link href ``/`` labeled "Chat" → + click → the chat page. +2. ``test_back_from_sources_returns_to_sources`` — Sources table link → + new tab without a ``back`` param → back link href ``/sources.html`` + labeled "Sources" → click → the Sources page. +3. ``test_malicious_back_param_is_rejected`` — absolute, + protocol-relative, and ``javascript:`` ``back`` values all fall back + to ``/sources.html`` (labeled "Sources", navigable). +""" +from __future__ import annotations + +import asyncio +import re +from pathlib import Path +from threading import Thread +from typing import Any + +from playwright.sync_api import Page, expect +from sqlalchemy import text + +from app.config import Settings +from app.db import SessionLocal +from app.rag.importer import ImportSummary, import_sources +from app.rag.llm import LLMClient + +REPO = Path(__file__).resolve().parents[2] +FIXTURES = REPO / "tests" / "fixtures" / "docs" +QUESTION = "How is my Kubernetes cluster set up?" +# Seeded fixture doc (source=docs) shared by every test in this file. +DOC_SOURCE = "docs" +DOC_PATH = "homelab%2Fkubernetes.md" +DOC_TITLE = "Kubernetes Homelab Cluster" + + +async def _import_fixtures(mock_port: int) -> ImportSummary: + kwargs: dict[str, Any] = {"_env_file": None, "llm_base_url": f"http://127.0.0.1:{mock_port}/v1"} + settings = Settings(**kwargs) # pyright: ignore[reportCallIssue] + return await import_sources([FIXTURES], LLMClient(settings)) + + +def _run_in_thread(coro: Any) -> Any: + """Run a coroutine on a worker thread. + + Playwright's sync API keeps an asyncio loop running on the test thread, + so ``asyncio.run`` cannot be called directly from a test body. + """ + box: dict[str, Any] = {} + + def runner() -> None: + try: + box["value"] = asyncio.run(coro) + except BaseException as e: # noqa: BLE001 — re-raised on the test thread + box["error"] = e + + t = Thread(target=runner) + t.start() + t.join() + if "error" in box: + raise box["error"] + return box["value"] + + +def _reset_db(mock_port: int, seed: bool) -> ImportSummary | None: + """Truncate the KB (and query log), then optionally re-import fixtures.""" + with SessionLocal() as db: + db.execute(text("TRUNCATE chunks, documents, query_log")) + db.commit() + if not seed: + return None + return _run_in_thread(_import_fixtures(mock_port)) + + +# --------------------------------------------------------------------------- +# 1. Chat source chip → viewer with back=/ → back returns to the chat +# --------------------------------------------------------------------------- + + +def test_back_from_chat_returns_to_chat( + page: Page, app_url: str, mock_llm: int, db_ready: None +) -> None: + _reset_db(mock_llm, seed=True) + page.set_default_timeout(30_000) + page.goto(app_url) + + page.fill("#message-input", QUESTION) + page.click("#send-btn") + + chip = page.locator(".msg.brain .source-chip", has_text="kubernetes.md") + expect(chip).to_have_count(1, timeout=30_000) + # Chat chips carry back=/ (encoded %2F) so the viewer knows where home is. + expect(chip.first).to_have_attribute( + "href", f"/document.html?source={DOC_SOURCE}&path={DOC_PATH}&back=%2F" + ) + + with page.expect_popup() as popup_info: + chip.first.click() + viewer = popup_info.value + expect(viewer).to_have_url( + re.compile( + re.escape(f"{app_url}/document.html?source={DOC_SOURCE}&path={DOC_PATH}&back=%2F") + ) + ) + # The cited document actually rendered (this is the viewer, not an error). + expect(viewer.locator("#doc-title")).to_have_text(DOC_TITLE) + # Back link resolved to the chat page, labeled "Chat". + back = viewer.locator("#doc-back") + expect(back).to_have_attribute("href", "/") + expect(back).to_have_text("Chat") + + # Click: deterministic anchor navigation back to the chat page. + back.click() + expect(viewer).to_have_url(f"{app_url}/") + expect(viewer.locator("#composer")).to_be_visible() + + +# --------------------------------------------------------------------------- +# 2. Sources table link → viewer without back param → back returns to Sources +# --------------------------------------------------------------------------- + + +def test_back_from_sources_returns_to_sources( + page: Page, app_url: str, mock_llm: int, db_ready: None +) -> None: + _reset_db(mock_llm, seed=True) + page.goto(f"{app_url}/sources.html") + + row = page.locator("#docs-tbody tr", has_text="kubernetes.md") + expect(row).to_have_count(1) + link = row.locator("td:nth-child(2) a.doc-link") + expect(link).to_have_count(1) + # Sources links carry NO back param — the viewer's default target + # (/sources.html) applies. + expect(link).to_have_attribute( + "href", f"/document.html?source={DOC_SOURCE}&path={DOC_PATH}" + ) + + with page.expect_popup() as popup_info: + link.click() + viewer = popup_info.value + assert "back=" not in viewer.url, f"unexpected back param: {viewer.url}" + expect(viewer.locator("#doc-title")).to_have_text(DOC_TITLE) + # Back link kept the default target, labeled "Sources". + back = viewer.locator("#doc-back") + expect(back).to_have_attribute("href", "/sources.html") + expect(back).to_have_text("Sources") + + back.click() + expect(viewer).to_have_url(f"{app_url}/sources.html") + expect(viewer.locator("#docs-table")).to_be_visible() + + +# --------------------------------------------------------------------------- +# 3. Hostile back values (absolute, protocol-relative, pseudo-protocol) +# are all rejected in favor of the same-origin default +# --------------------------------------------------------------------------- + + +def test_malicious_back_param_is_rejected( + page: Page, app_url: str, mock_llm: int, db_ready: None +) -> None: + _reset_db(mock_llm, seed=True) + errors: list[str] = [] + dialogs: list[str] = [] + page.on("pageerror", lambda e: errors.append(str(e))) + + def _catch_dialog(d) -> None: # a fired dialog == executed script + dialogs.append(d.message) + d.dismiss() + + page.on("dialog", _catch_dialog) + + viewer_base = f"{app_url}/document.html?source={DOC_SOURCE}&path={DOC_PATH}" + # Anything that is not a same-origin relative URL must be rejected: + # an absolute https URL, a protocol-relative URL, and a javascript: + # pseudo-protocol. + for evil in ("https%3A%2F%2Fevil.com", "%2F%2Fevil.com", "javascript%3Aalert(1)"): + page.goto(f"{viewer_base}&back={evil}") + expect(page.locator("#doc-title")).to_have_text(DOC_TITLE) + back = page.locator("#doc-back") + expect(back).to_have_attribute("href", "/sources.html") + expect(back).to_have_text("Sources") + + # And the fallback is really navigable: clicking lands on Sources. + page.click("#doc-back") + expect(page).to_have_url(f"{app_url}/sources.html") + + assert dialogs == [], f"dialog fired — a back param escaped validation: {dialogs}" + assert errors == [], f"console crashes: {errors}" diff --git a/tests/e2e/test_document_viewer.py b/tests/e2e/test_document_viewer.py index 4769245..da0499e 100644 --- a/tests/e2e/test_document_viewer.py +++ b/tests/e2e/test_document_viewer.py @@ -99,10 +99,11 @@ def test_source_chip_opens_document( chip = page.locator(".msg.brain .source-chip", has_text="kubernetes.md") expect(chip).to_have_count(1, timeout=30_000) - # New-tab contract: same-origin viewer URL, both query values encoded - # (the path's slashes come out as %2F — exactly why encoding matters). + # New-tab contract: same-origin viewer URL, all query values encoded + # (the path's slashes come out as %2F — exactly why encoding matters), + # plus back=/ (phase 13) so the viewer's back button returns to chat. expect(chip.first).to_have_attribute( - "href", "/document.html?source=docs&path=homelab%2Fkubernetes.md" + "href", "/document.html?source=docs&path=homelab%2Fkubernetes.md&back=%2F" ) expect(chip.first).to_have_attribute("target", "_blank") expect(chip.first).to_have_attribute("rel", "noopener") @@ -112,7 +113,9 @@ def test_source_chip_opens_document( viewer = popup_info.value expect(viewer).to_have_url( re.compile( - re.escape(f"{app_url}/document.html?source=docs&path=homelab%2Fkubernetes.md") + re.escape( + f"{app_url}/document.html?source=docs&path=homelab%2Fkubernetes.md&back=%2F" + ) ) ) expect(viewer.locator("#doc-title")).to_have_text("Kubernetes Homelab Cluster") diff --git a/tests/unit/test_document_viewer.py b/tests/unit/test_document_viewer.py index ce35e66..2edce90 100644 --- a/tests/unit/test_document_viewer.py +++ b/tests/unit/test_document_viewer.py @@ -160,18 +160,26 @@ def test_content_requires_both_params() -> None: def test_viewer_url_builder_present_in_chat_and_sources() -> None: """Both entry points (chat chips, Sources rows) build the same - encoded viewer URL and open it in a new tab with rel=noopener.""" + encoded viewer URL and open it in a new tab with rel=noopener. + + Phase 13: the chat builder additionally carries ``back=/`` (encoded + %2F) so the viewer's back button returns to the chat; Sources links + intentionally omit the param (the viewer's /sources.html default).""" for name, js in (("app.js", _read(APP_JS)), ("sources.js", _read(SOURCES_JS))): - assert "function documentUrl(source, path)" in js, name assert '"/document.html?source=" + encodeURIComponent(' in js, name assert '"&path=" + encodeURIComponent(' in js, name app_js = _read(APP_JS) - assert "chip.href = documentUrl(s.source, s.path)" in app_js + # Chat: 3-arg builder with back defaulting to the chat page. + assert 'function documentUrl(source, path, back = "/")' in app_js + assert '"&back=" + encodeURIComponent(back)' in app_js + assert 'chip.href = documentUrl(s.source, s.path, "/")' in app_js assert 'chip.target = "_blank"' in app_js assert 'chip.rel = "noopener"' in app_js sources_js = _read(SOURCES_JS) + # Sources: unchanged 2-arg builder — no back param in the URL. + assert "function documentUrl(source, path)" in sources_js assert 'link.className = "doc-link"' in sources_js assert "link.href = documentUrl(d.source, d.path)" in sources_js assert 'link.target = "_blank"' in sources_js @@ -188,7 +196,9 @@ def _run_node(script: str) -> str: def _extract_function(js: str, name: str) -> str: - match = re.search(rf"(?:export )?function {name}\(source, path\) \{{.*?\n\}}", js, re.S) + match = re.search( + rf"(?:export )?function {name}\(source, path(?:, back = \"/\")?\) \{{.*?\n\}}", js, re.S + ) assert match, f"{name}(source, path) not found" return match.group(0).replace("export ", "", 1) @@ -196,18 +206,21 @@ def _extract_function(js: str, name: str) -> str: @pytest.mark.skipif(not HAVE_NODE, reason="node not available") def test_viewer_url_builder_encodes_spaces_and_slashes() -> None: """Behavioral check of the real builder (app.js) under node: slashes - and spaces in source/path values must come out percent-encoded.""" + and spaces in source/path values must come out percent-encoded, and + the back target is appended + encoded (phase 13).""" fn = _extract_function(_read(APP_JS), "documentUrl") out = _run_node( f"{fn}\n" "console.log(documentUrl('Homelab', 'kubernetes.md'));\n" "console.log(documentUrl('Homelab', 'notes/my file.yaml'));\n" - "console.log(documentUrl('H omelab', 'a/b.md'));" + "console.log(documentUrl('H omelab', 'a/b.md'));\n" + "console.log(documentUrl('Homelab', 'kubernetes.md', '/sources.html'));" ) assert out.splitlines() == [ - "/document.html?source=Homelab&path=kubernetes.md", - "/document.html?source=Homelab&path=notes%2Fmy%20file.yaml", - "/document.html?source=H%20omelab&path=a%2Fb.md", + "/document.html?source=Homelab&path=kubernetes.md&back=%2F", + "/document.html?source=Homelab&path=notes%2Fmy%20file.yaml&back=%2F", + "/document.html?source=H%20omelab&path=a%2Fb.md&back=%2F", + "/document.html?source=Homelab&path=kubernetes.md&back=%2Fsources.html", ] @@ -259,10 +272,19 @@ def test_markdown_renderer_stays_xss_safe_and_unchanged() -> None: def test_viewer_js_rendering_contracts() -> None: """document.js: raw formats go in via textContent (never parsed as HTML), markdown via the shared renderer, 404 → designed not-found - state, back link prefers browser history when there is one.""" + state, and (phase 13) the back link resolves the ``back`` param — + same-origin relative URLs only, /sources.html default, no browser + history heuristics (both entry points are fresh tabs).""" js = _read(DOCUMENT_JS) assert "pre.textContent = doc.content" in js # raw formats: text node assert "renderMarkdown(doc.content)" in js # md/markdown: shared renderer assert "showNotFound" in js - assert "history.length > 1" in js + # Phase 13: deterministic back-target resolution, no history heuristics. + assert "history.length" not in js + assert "window.history.back" not in js + assert 'backParam.startsWith("/")' in js # same-origin relative only… + assert 'backParam.startsWith("//")' in js # …and not protocol-relative + assert 'backLink.href = backTarget' in js # deterministic anchor navigation + assert '"/sources.html"' in js # default target + no-JS fallback value + assert '"Chat"' in js and '"Sources"' in js # labels for the two entry points assert "encodeURIComponent" in js # content fetch uses the same encoding