From 311f4a2c0c905126e14fad243d2d7836373454e8 Mon Sep 17 00:00:00 2001
From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com>
Date: Sat, 1 Aug 2026 09:46:54 +0000
Subject: [PATCH] Fix OAuth authorize 500: encode login `next` URL as JS, not
 HTML
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The sign-in page threads the post-login destination back through inline JS
(`const NEXT = "..."`), but built that literal with `html.escape()`. Inside a
`<script>` element the browser does not decode HTML entities, so every `&` in
the destination URL became a literal `&amp;`. For a multi-parameter `next`
(the OAuth `/authorize` URL), the redirect after login parsed
`?response_type=code&client_id=X&redirect_uri=Y` as params `response_type`,
`amp;client_id`, `amp;redirect_uri` — dropping the real names. `client_id` thus
arrived empty at `/authorize`, and the file-backed client store resolved the
empty key to its root directory (IsADirectoryError) → HTTP 500. A plain-path
`next` (no `&`) hid the bug; the MCP OAuth flow is the first multi-param `next`.

- pages.py: new `_js_string()` encodes `next_url` as a JS string literal
  (JSON + &/</> so it's inert to the HTML tokenizer yet decodes
  back at runtime). Applied to both the standard and shared login pages.
- oauth_server.py: a blank/missing client_id now short-circuits to the clean
  400 "unknown client" page instead of hitting the store and 500ing.
- tests: new test_pages.py (JS-encoding + no-breakout + no HTML-escape) and a
  blank-client_id 400 regression in test_oauth_server.py. Suite 195 -> 200.

Claude-Session: https://claude.ai/code/session_01Gw5RPgrQhC88Hc3DyACYWF
---
 enlace_auth/auth/oauth_server.py |  6 +++-
 enlace_auth/auth/pages.py        | 32 +++++++++++++++---
 tests/test_oauth_server.py       | 23 +++++++++++++
 tests/test_pages.py              | 57 ++++++++++++++++++++++++++++++++
 4 files changed, 113 insertions(+), 5 deletions(-)
 create mode 100644 tests/test_pages.py

diff --git a/enlace_auth/auth/oauth_server.py b/enlace_auth/auth/oauth_server.py
index 41039ac..c1192aa 100644
--- a/enlace_auth/auth/oauth_server.py
+++ b/enlace_auth/auth/oauth_server.py
@@ -280,7 +280,11 @@ def _validate_authorize(
         """Parse + validate /authorize params. Returns (ok, error_page)."""
         client_id = params.get("client_id", "")
         redirect_uri = params.get("redirect_uri", "")
-        client = client_store.get(client_id)
+        # Guard the empty key explicitly: a file-backed client_store resolves an
+        # empty client_id to its root directory and raises IsADirectoryError
+        # (a 500) instead of returning None — so a missing/blank client_id must
+        # short-circuit to the clean "unknown client" page below.
+        client = client_store.get(client_id) if client_id else None
         if not client or redirect_uri not in client.get("redirect_uris", []):
             # Cannot safely redirect to an unverified URI — show an error page.
             return None, HTMLResponse(
diff --git a/enlace_auth/auth/pages.py b/enlace_auth/auth/pages.py
index b8ae754..ab8d171 100644
--- a/enlace_auth/auth/pages.py
+++ b/enlace_auth/auth/pages.py
@@ -14,9 +14,33 @@
 
 from __future__ import annotations
 
+import json
 from html import escape
 from typing import Optional
 
+
+def _js_string(value: str) -> str:
+    """Encode a Python string as a JS string literal safe to embed inline in a
+    ``<script>`` element.
+
+    ``html.escape`` is WRONG here: inside a ``<script>`` element the browser does
+    not decode HTML entities, so escaping ``&`` to ``&amp;`` corrupts the string
+    at runtime (e.g. a ``next`` URL's ``?a=1&b=2`` query becomes ``?a=1&amp;b=2``,
+    dropping every parameter after the first). JSON is the correct encoding for a
+    JS string literal; the extra ``\\uXXXX`` replacements keep ``&``/``<``/``>``
+    inert in the HTML tokenizer (no ``</script>`` breakout) while the JS engine
+    decodes them back to the original characters at runtime. Returns the literal
+    *including* its surrounding quotes.
+    """
+    return (
+        json.dumps(value)
+        .replace("&", "\\u0026")
+        .replace("<", "\\u003c")
+        .replace(">", "\\u003e")
+        .replace("\u2028", "\\u2028")
+        .replace("\u2029", "\\u2029")
+    )
+
 # --------------------------------------------------------------------------
 # Shared chrome
 # --------------------------------------------------------------------------
@@ -191,7 +215,7 @@ def render_login_page(
         error: optional error banner (e.g. a stale-session note).
         show_register_hint: whether to show the "ask an admin" footnote.
     """
-    next_js = escape(next_url, quote=True)
+    next_js = _js_string(next_url)
     hint = (
         '<p class="muted">No account? Accounts are created by the platform admin.</p>'
         if show_register_hint
@@ -218,7 +242,7 @@ def render_login_page(
 <script>
 {_CSRF_JS}
 {_PW_TOGGLE_JS}
-const NEXT = "{next_js}";
+const NEXT = {next_js};
 const f = document.getElementById('f');
 const msg = document.getElementById('msg');
 const submit = document.getElementById('submit');
@@ -269,7 +293,7 @@ def render_shared_login_page(
         error: optional error banner.
     """
     app_js = escape(app, quote=True)
-    next_js = escape(next_url, quote=True)
+    next_js = _js_string(next_url)
     app_disp = escape(app)
     body = f"""<div class="card">
 <h1>Enter password</h1>
@@ -288,7 +312,7 @@ def render_shared_login_page(
 <script>
 {_CSRF_JS}
 const APP = "{app_js}";
-const NEXT = "{next_js}";
+const NEXT = {next_js};
 const f = document.getElementById('f');
 const msg = document.getElementById('msg');
 const submit = document.getElementById('submit');
diff --git a/tests/test_oauth_server.py b/tests/test_oauth_server.py
index f562917..4ca2e96 100644
--- a/tests/test_oauth_server.py
+++ b/tests/test_oauth_server.py
@@ -129,6 +129,29 @@ def test_authorize_rejects_unknown_client(tmp_path):
     assert r.status_code == 400
 
 
+def test_authorize_blank_client_id_is_clean_400_not_500(tmp_path):
+    # Regression: a file-backed client_store resolves an empty client_id to its
+    # root directory and raised IsADirectoryError (a 500). A blank/missing
+    # client_id must short-circuit to the same clean 400 as an unknown client.
+    client, cookie = _build(tmp_path)
+    for cid in ("", None):
+        params = {
+            "response_type": "code",
+            "redirect_uri": REDIRECT,
+            "code_challenge": "x",
+            "code_challenge_method": "S256",
+        }
+        if cid is not None:
+            params["client_id"] = cid
+        r = client.get(
+            "/auth/oauth/authorize",
+            params=params,
+            cookies={COOKIE: cookie},
+            follow_redirects=False,
+        )
+        assert r.status_code == 400, f"client_id={cid!r} -> {r.status_code}"
+
+
 def test_full_authorization_code_flow_with_consent(tmp_path):
     client, cookie = _build(tmp_path)
     cid = _register(client)
diff --git a/tests/test_pages.py b/tests/test_pages.py
new file mode 100644
index 0000000..d1c38b9
--- /dev/null
+++ b/tests/test_pages.py
@@ -0,0 +1,57 @@
+"""Regression tests for the browser-facing auth pages (:mod:`enlace_auth.auth.pages`).
+
+Focus: the ``next`` URL is threaded back through the sign-in page as an inline-JS
+constant. It must be embedded as a **JS string literal** (JSON-encoded), never
+HTML-escaped — inside a ``<script>`` element the browser does not decode HTML
+entities, so escaping ``&`` to ``&amp;`` corrupts a multi-parameter ``next`` URL
+(dropping every query parameter after the first). That corruption broke the OAuth
+authorization-code flow, whose ``next`` is the multi-param ``/authorize`` URL.
+"""
+
+from urllib.parse import parse_qs
+
+from enlace_auth.auth.pages import (
+    _js_string,
+    render_login_page,
+    render_shared_login_page,
+)
+
+# A representative OAuth authorize URL: several params, an embedded https redirect.
+AUTHZ = (
+    "/auth/oauth/authorize?response_type=code&client_id=QsnRJ56Z"
+    "&redirect_uri=https://claude.ai/api/mcp/auth_callback"
+    "&code_challenge=abc&code_challenge_method=S256&state=xyz"
+)
+
+
+def _js_runtime_value(literal: str) -> str:
+    """Decode a JS string literal (with ``\\uXXXX`` escapes) to its runtime value."""
+    assert literal[0] == '"' and literal[-1] == '"'
+    return literal[1:-1].encode("utf-8").decode("unicode_escape")
+
+
+def test_js_string_preserves_query_params():
+    runtime = _js_runtime_value(_js_string(AUTHZ))
+    assert runtime == AUTHZ
+    assert parse_qs(runtime.split("?", 1)[1])["client_id"] == ["QsnRJ56Z"]
+
+
+def test_js_string_has_no_script_breakout():
+    lit = _js_string("/x?a=</script><b>&c=1")
+    assert "</script>" not in lit and "<" not in lit and ">" not in lit and "&" not in lit
+    # ...but the runtime value is exactly the original, untouched.
+    assert _js_runtime_value(lit) == "/x?a=</script><b>&c=1"
+
+
+def test_login_page_does_not_html_escape_next_into_js():
+    page = render_login_page(next_url=AUTHZ)
+    # The bug's fingerprint: an HTML entity where a raw ampersand must be.
+    assert "&amp;client_id" not in page
+    # The fix's fingerprint: query separators carried as JS unicode escapes.
+    assert "\\u0026client_id" in page
+
+
+def test_shared_login_page_does_not_html_escape_next_into_js():
+    page = render_shared_login_page(app="demo", next_url=AUTHZ)
+    assert "&amp;client_id" not in page
+    assert "\\u0026client_id" in page