From 046d1231495d098143955a977855e218c74dd248 Mon Sep 17 00:00:00 2001 From: Brian Krabach Date: Wed, 1 Apr 2026 17:30:18 -0700 Subject: [PATCH] refactor: remove all cross-origin browser-direct federation code - Delete storeFederationToken function from app.js - Delete window.addEventListener('message',...) handler from app.js - Delete buildAuthTileHTML function from app.js - Delete formatLastSeen function from app.js - Delete buildOfflineTileHTML function from app.js - Delete openLoginPopup function from app.js - Simplify api() to same-origin only (no baseUrl/credentials/X-Muxplex-Token) - Remove federation auth relay -
diff --git a/muxplex/frontend/tests/test_app.mjs b/muxplex/frontend/tests/test_app.mjs index cb7261f..37a48c7 100644 --- a/muxplex/frontend/tests/test_app.mjs +++ b/muxplex/frontend/tests/test_app.mjs @@ -2298,38 +2298,7 @@ test('api with no baseUrl uses relative path', async () => { globalThis.fetch = origFetch; }); -test('api with baseUrl prepends it to path and sets credentials include', async () => { - const calls = []; - const origFetch = globalThis.fetch; - globalThis.fetch = async (url, opts) => { - calls.push({ url, opts }); - return { ok: true }; - }; - await app.api('GET', '/api/sessions', undefined, 'https://remote.example.com'); - - assert.strictEqual(calls.length, 1, 'should call fetch once'); - assert.strictEqual(calls[0].url, 'https://remote.example.com/api/sessions', 'url should prepend baseUrl'); - assert.strictEqual(calls[0].opts.credentials, 'include', 'credentials should be include for cross-origin'); - - globalThis.fetch = origFetch; -}); - -test('api with baseUrl and trailing slash does not double-slash', async () => { - const calls = []; - const origFetch = globalThis.fetch; - globalThis.fetch = async (url, opts) => { - calls.push({ url, opts }); - return { ok: true }; - }; - - await app.api('GET', '/api/sessions', undefined, 'https://remote.example.com/'); - - assert.strictEqual(calls.length, 1, 'should call fetch once'); - assert.strictEqual(calls[0].url, 'https://remote.example.com/api/sessions', 'trailing slash on baseUrl should not create double-slash'); - - globalThis.fetch = origFetch; -}); test('createNewSession polls for session before auto-opening (not immediate setTimeout openSession)', () => { // The old behavior was: setTimeout(() => openSession(...), 500) immediately after POST. @@ -2807,75 +2776,6 @@ test('app.js exports all Phase 2 federation functions', () => { } }); -// --- buildAuthTileHTML --- - -test('buildAuthTileHTML is exported as a function', () => { - assert.strictEqual(typeof app.buildAuthTileHTML, 'function'); -}); - -test('buildAuthTileHTML returns article with source-tile--auth class', () => { - const html = app.buildAuthTileHTML({ name: 'Dev Server', url: 'http://dev:8088' }); - assert.ok(html.startsWith(' { - const html = app.buildAuthTileHTML({ name: 'Dev Server', url: 'http://dev:8088' }); - assert.ok(html.includes('Dev Server'), 'html should include the device name'); -}); - -test('buildAuthTileHTML includes login button with data-url attribute', () => { - const html = app.buildAuthTileHTML({ name: 'Dev Server', url: 'http://dev:8088' }); - assert.ok(html.includes('source-tile__login-btn'), 'html should include source-tile__login-btn class'); - assert.ok(html.includes('data-url="http://dev:8088"'), 'html should include data-url attribute with correct value'); -}); - -test('buildAuthTileHTML escapes HTML in device name', () => { - const html = app.buildAuthTileHTML({ name: '', url: '' }); - assert.ok(!html.includes(''), 'raw script tag should not appear in html'); - assert.ok(html.includes('<script>'), 'escaped script tag should appear in html'); -}); - -// --- buildOfflineTileHTML --- - -test('buildOfflineTileHTML is exported as a function', () => { - assert.strictEqual(typeof app.buildOfflineTileHTML, 'function'); -}); - -test('buildOfflineTileHTML returns article with source-tile--offline class', () => { - const html = app.buildOfflineTileHTML({ name: 'Dev Server', url: 'http://dev:8088', lastSeenAt: null }); - assert.ok(html.startsWith(' { - const html = app.buildOfflineTileHTML({ name: 'Dev Server', url: 'http://dev:8088', lastSeenAt: null }); - assert.ok(html.includes('Dev Server'), 'html should include the device name'); -}); - -test('buildOfflineTileHTML includes Offline badge', () => { - const html = app.buildOfflineTileHTML({ name: 'Dev Server', url: 'http://dev:8088', lastSeenAt: null }); - assert.ok(html.includes('Offline'), 'html should include Offline text'); - assert.ok(html.includes('source-tile__badge'), 'html should include source-tile__badge class'); -}); - -test('buildOfflineTileHTML shows relative last-seen time', () => { - const fiveMinAgo = Date.now() - 5 * 60 * 1000; - const html = app.buildOfflineTileHTML({ name: 'Dev Server', url: 'http://dev:8088', lastSeenAt: fiveMinAgo }); - assert.ok(html.includes('Last seen'), 'html should include Last seen text'); -}); - -test('buildOfflineTileHTML escapes device name', () => { - const html = app.buildOfflineTileHTML({ name: 'bad', url: '', lastSeenAt: null }); - assert.ok(!html.includes('bad'), 'raw bad must not appear in html'); - assert.ok(html.includes('<b>bad</b>'), 'device name should be HTML-escaped'); -}); - -test('buildOfflineTileHTML shows "Never" when lastSeenAt is null', () => { - const html = app.buildOfflineTileHTML({ name: 'Dev Server', url: '', lastSeenAt: null }); - assert.ok(html.includes('Never'), 'html should include Never when lastSeenAt is null'); -}); - // --- buildStatusTileHTML --- test('buildStatusTileHTML is exported as a function', () => { @@ -2901,98 +2801,6 @@ test('buildStatusTileHTML renders statusText in badge span', () => { assert.ok(html.includes('source-tile__badge'), 'html should include source-tile__badge class'); }); -// --- formatLastSeen branch coverage --- - -test('formatLastSeen returns seconds ago for diff < 60', () => { - const thirtySecondsAgo = Date.now() - 30 * 1000; - assert.match(app.formatLastSeen(thirtySecondsAgo), /^\d+s ago$/, 'should return Xs ago for diff < 60s'); -}); - -test('formatLastSeen returns hours ago for diff >= 3600 and < 86400', () => { - const twoHoursAgo = Date.now() - 2 * 3600 * 1000; - assert.match(app.formatLastSeen(twoHoursAgo), /^\d+h ago$/, 'should return Xh ago for diff in range [3600, 86400)'); -}); - -test('formatLastSeen returns days ago for diff >= 86400', () => { - const twoDaysAgo = Date.now() - 2 * 86400 * 1000; - assert.match(app.formatLastSeen(twoDaysAgo), /^\d+d ago$/, 'should return Xd ago for diff >= 86400s'); -}); - -// --- openLoginPopup (task-5-login-popup-flow) --- - -test('openLoginPopup is exported as a function', () => { - assert.strictEqual(typeof app.openLoginPopup, 'function', 'openLoginPopup should be exported as a function'); -}); - -test('openLoginPopup calls window.open with correct URL and dimensions', () => { - const openCalls = []; - const origOpen = globalThis.window.open; - globalThis.window.open = (url, target, features) => { openCalls.push({ url, target, features }); }; - - app.openLoginPopup('http://work:8088'); - - globalThis.window.open = origOpen; - - assert.strictEqual(openCalls.length, 1, 'window.open should be called exactly once'); - assert.strictEqual(openCalls[0].url, 'http://work:8088/login', 'url should be remoteUrl + /login'); - assert.strictEqual(openCalls[0].target, '_blank', 'target should be _blank'); - assert.ok(openCalls[0].features.includes('width=500'), 'features should include width=500'); - assert.ok(openCalls[0].features.includes('height=600'), 'features should include height=600'); -}); - -test('openLoginPopup appends /login to URL without trailing slash', () => { - const openCalls = []; - const origOpen = globalThis.window.open; - globalThis.window.open = (url) => { openCalls.push(url); }; - - app.openLoginPopup('http://work:8088'); - - globalThis.window.open = origOpen; - - assert.strictEqual(openCalls[0], 'http://work:8088/login', 'should append /login when no trailing slash'); -}); - -test('openLoginPopup handles URL with trailing slash', () => { - const openCalls = []; - const origOpen = globalThis.window.open; - globalThis.window.open = (url) => { openCalls.push(url); }; - - app.openLoginPopup('http://work:8088/'); - - globalThis.window.open = origOpen; - - assert.strictEqual(openCalls[0], 'http://work:8088/login', 'should strip trailing slash then append /login'); -}); - -// --- formatLastSeen (auto-recovery detection) --- - -test('formatLastSeen returns seconds for recent timestamps', () => { - const thirtySecondsAgo = Date.now() - 30000; - assert.match(app.formatLastSeen(thirtySecondsAgo), /^\d+s ago$/); -}); - -test('formatLastSeen returns minutes for older timestamps', () => { - const fiveMinutesAgo = Date.now() - 5 * 60 * 1000; - assert.match(app.formatLastSeen(fiveMinutesAgo), /^\d+m ago$/); -}); - -test('formatLastSeen returns hours for much older timestamps', () => { - const threeHoursAgo = Date.now() - 3 * 3600 * 1000; - assert.match(app.formatLastSeen(threeHoursAgo), /^\d+h ago$/); -}); - -test('formatLastSeen returns days for very old timestamps', () => { - const twoDaysAgo = Date.now() - 2 * 86400 * 1000; - assert.match(app.formatLastSeen(twoDaysAgo), /^\d+d ago$/); -}); - -test('formatLastSeen returns Never for null', () => { - assert.strictEqual(app.formatLastSeen(null), 'Never'); -}); - -test('formatLastSeen returns Never for undefined', () => { - assert.strictEqual(app.formatLastSeen(undefined), 'Never'); -}); // --- Issue 1: Loading placeholder tile --- test('createNewSession injects tile--loading placeholder after POST succeeds', () => { @@ -3786,90 +3594,41 @@ test('HTML Sessions panel hidden sessions field appears after bell sound', () => assert.ok(hiddenIdx > bellSoundIdx, 'hidden sessions must appear after bell sound (i.e., near the end)'); }); -// --------------------------------------------------------------------------- -// Federation auth token relay tests (postMessage / X-Muxplex-Token) -// --------------------------------------------------------------------------- +// --- Verification: cross-origin code removed --- -test('api() includes X-Muxplex-Token header when token exists in localStorage for that origin', async () => { - // Store a fake token for https://remote.example.com - const tokens = { 'https://remote.example.com': 'fake-token-abc123' }; - localStorage.setItem('muxplex.federation_tokens', JSON.stringify(tokens)); - - const calls = []; - globalThis.fetch = (url, opts) => { - calls.push({ url, opts }); - return Promise.resolve({ ok: true, status: 200, json: async () => ([]) }); - }; - - await app.api('GET', '/api/sessions', undefined, 'https://remote.example.com'); - - assert.strictEqual(calls.length, 1); - assert.ok(calls[0].opts.headers['X-Muxplex-Token'] === 'fake-token-abc123', - 'X-Muxplex-Token header must be set to the stored token'); - - // Cleanup - localStorage.removeItem('muxplex.federation_tokens'); +test('app.js does not export storeFederationToken', () => { + assert.strictEqual(app.storeFederationToken, undefined, 'storeFederationToken must not be exported'); }); -test('api() does not include X-Muxplex-Token header when no token for that origin', async () => { - // Ensure no tokens stored - localStorage.removeItem('muxplex.federation_tokens'); - - const calls = []; - globalThis.fetch = (url, opts) => { - calls.push({ url, opts }); - return Promise.resolve({ ok: true, status: 200, json: async () => ([]) }); - }; - - await app.api('GET', '/api/sessions', undefined, 'https://remote.example.com'); - - assert.strictEqual(calls.length, 1); - assert.ok(!calls[0].opts.headers['X-Muxplex-Token'], - 'X-Muxplex-Token header must NOT be present when no token stored'); +test('app.js does not export buildAuthTileHTML', () => { + assert.strictEqual(app.buildAuthTileHTML, undefined, 'buildAuthTileHTML must not be exported'); }); -test('api() does not include X-Muxplex-Token header for local (no baseUrl) requests', async () => { - // Store a token for some origin, make sure it does not bleed into local requests - const tokens = { 'https://remote.example.com': 'fake-token-abc123' }; - localStorage.setItem('muxplex.federation_tokens', JSON.stringify(tokens)); +test('app.js does not export buildOfflineTileHTML', () => { + assert.strictEqual(app.buildOfflineTileHTML, undefined, 'buildOfflineTileHTML must not be exported'); +}); +test('app.js does not export openLoginPopup', () => { + assert.strictEqual(app.openLoginPopup, undefined, 'openLoginPopup must not be exported'); +}); + +test('app.js does not export formatLastSeen', () => { + assert.strictEqual(app.formatLastSeen, undefined, 'formatLastSeen must not be exported'); +}); + +test('api() is same-origin only (no baseUrl parameter support)', async () => { const calls = []; globalThis.fetch = (url, opts) => { calls.push({ url, opts }); - return Promise.resolve({ ok: true, status: 200, json: async () => ([]) }); + return Promise.resolve({ ok: true, json: async () => ([]) }); }; await app.api('GET', '/api/sessions'); assert.strictEqual(calls.length, 1); - assert.ok(!calls[0].opts.headers['X-Muxplex-Token'], - 'X-Muxplex-Token header must NOT be present for local requests'); - - // Cleanup - localStorage.removeItem('muxplex.federation_tokens'); -}); - -test('postMessage muxplex-auth-token event stores token in localStorage', () => { - // Simulate receiving a postMessage event - localStorage.removeItem('muxplex.federation_tokens'); - - // Find and invoke the message event listener registered by app.js - // The app registers on window.addEventListener('message', ...) in DOMContentLoaded - // We need to directly call storeFederationToken helper or simulate via the listener. - // Since bindStaticEventListeners or DOMContentLoaded registered a 'message' listener on window, - // we call storeFederationToken directly if exported, or test via the exported API. - const result = app.storeFederationToken('https://remote.host.com', 'relay-token-xyz'); - - const stored = JSON.parse(localStorage.getItem('muxplex.federation_tokens') || '{}'); - assert.strictEqual(stored['https://remote.host.com'], 'relay-token-xyz', - 'storeFederationToken must store token keyed by origin in muxplex.federation_tokens'); -}); - -test('index.html contains popup federation auth relay script', () => { - const source = fs.readFileSync(new URL('../index.html', import.meta.url), 'utf8'); - assert.ok(source.includes('window.opener'), 'index.html must contain window.opener check for federation popup relay'); - assert.ok(source.includes('muxplex-auth-token'), 'index.html must post muxplex-auth-token message type'); - assert.ok(source.includes('/api/auth/token'), 'index.html popup script must fetch /api/auth/token'); + assert.strictEqual(calls[0].url, '/api/sessions', 'should call local path directly'); + assert.ok(!calls[0].opts.credentials, 'no credentials:include for same-origin'); + globalThis.fetch = undefined; }); // ─── Edge-bar design: failing tests added before implementation ─── diff --git a/muxplex/main.py b/muxplex/main.py index 6199738..fcd9c19 100644 --- a/muxplex/main.py +++ b/muxplex/main.py @@ -32,7 +32,6 @@ from fastapi.responses import HTMLResponse from fastapi.staticfiles import StaticFiles from pydantic import BaseModel, field_validator from starlette.responses import JSONResponse, RedirectResponse -from starlette.middleware.cors import CORSMiddleware from muxplex.auth import ( AuthMiddleware, @@ -267,20 +266,6 @@ app.add_middleware( federation_key=_federation_key, ) -# CORS: allow_origins=["*"] with allow_credentials=True is intentional for -# self-hosted federation. Starlette reflects the actual Origin header (rather -# than "*") when credentials are requested, so credentialed cross-origin -# requests work correctly. Do not restrict to a fixed origin list without -# first understanding how remote muxplex peers discover and reach each other. -app.add_middleware( - CORSMiddleware, - allow_origins=["*"], - allow_credentials=True, - allow_methods=["*"], - allow_headers=["*"], -) - - # --------------------------------------------------------------------------- # Request / response models # --------------------------------------------------------------------------- @@ -858,19 +843,6 @@ async def logout() -> RedirectResponse: return response -@app.get("/api/auth/token") -async def get_auth_token(request: Request): - """Return the current session token for federation relay. - - Only accessible when already authenticated (via cookie or localhost bypass). - Used by the login popup to relay the token back to the opener window via postMessage. - """ - cookie = request.cookies.get("muxplex_session") - if cookie and verify_session_cookie(_auth_secret, cookie, _auth_ttl): - return {"token": cookie} - return JSONResponse({"error": "not authenticated"}, status_code=401) - - @app.get("/auth/mode") async def auth_mode_endpoint(): """Return the current auth mode and running username.""" diff --git a/muxplex/tests/test_api.py b/muxplex/tests/test_api.py index d148709..378a80d 100644 --- a/muxplex/tests/test_api.py +++ b/muxplex/tests/test_api.py @@ -1158,60 +1158,6 @@ def test_instance_info_federation_enabled_true_when_key_exists( ) -# --------------------------------------------------------------------------- -# CORS middleware -# --------------------------------------------------------------------------- - - -def test_cors_preflight_returns_200(tmp_path, monkeypatch): - """OPTIONS /api/sessions with CORS preflight headers returns 200 with access-control-allow-origin header. - - NOTE — Spec discrepancy: the acceptance criteria states `access-control-allow-origin: *`, - but the middleware is configured with `allow_credentials=True`. The CORS specification - (RFC 6454 / Fetch standard) forbids the wildcard value when credentials are included; - Starlette therefore reflects the request Origin instead of emitting "*". Keeping - `allow_credentials=True` is intentional for cross-origin federation with session cookies, - so the assertion uses the reflected origin rather than a literal "*". - """ - monkeypatch.setenv("MUXPLEX_PASSWORD", "test-password") - origin = "http://other-muxplex.local:8088" - with TestClient(app) as c: - response = c.options( - "/api/sessions", - headers={ - "Origin": origin, - "Access-Control-Request-Method": "GET", - }, - ) - assert response.status_code == 200 - assert response.headers.get("access-control-allow-origin") == origin - - -def test_cors_allows_any_origin(client): - """GET /api/sessions with Origin header gets access-control-allow-origin header in response. - - NOTE — Spec discrepancy: the acceptance criteria states `access-control-allow-origin: *`, - but this is incompatible with `allow_credentials=True` (CORS spec forbids the two together). - Starlette reflects the request Origin instead. This provides equivalent permissiveness - (any origin is allowed) while remaining spec-compliant. - """ - origin = "http://other-muxplex.local:8088" - response = client.get( - "/api/sessions", - headers={"Origin": origin}, - ) - assert response.headers.get("access-control-allow-origin") == origin - - -def test_cors_allows_credentials(client): - """GET /api/sessions with Origin header includes access-control-allow-credentials: true.""" - response = client.get( - "/api/sessions", - headers={"Origin": "http://other-muxplex.local:8088"}, - ) - assert response.headers.get("access-control-allow-credentials") == "true" - - # --------------------------------------------------------------------------- # POST /api/sessions (create new session) # --------------------------------------------------------------------------- @@ -1494,30 +1440,6 @@ def test_delete_session_default_template_is_tmux_kill(client, monkeypatch, tmp_p ) -# --------------------------------------------------------------------------- -# GET /api/auth/token (cross-origin federation token relay) -# --------------------------------------------------------------------------- - - -def test_get_auth_token_returns_token_when_authenticated(client): - """GET /api/auth/token returns {token: } when request has a valid session cookie.""" - response = client.get("/api/auth/token") - assert response.status_code == 200 - data = response.json() - assert "token" in data - assert isinstance(data["token"], str) - assert len(data["token"]) > 0 - - -def test_get_auth_token_returns_401_when_not_authenticated(monkeypatch): - """GET /api/auth/token returns 401 when request has no valid session cookie.""" - monkeypatch.setenv("MUXPLEX_PASSWORD", "test-password") - with TestClient(app, base_url="http://192.168.1.1") as c: - # No cookie set — endpoint must return 401 with application/json accept - response = c.get("/api/auth/token", headers={"Accept": "application/json"}) - assert response.status_code == 401 - - # --------------------------------------------------------------------------- # Federation Bearer token auth # --------------------------------------------------------------------------- diff --git a/muxplex/tests/test_auth.py b/muxplex/tests/test_auth.py index 9e7fd52..d88da8d 100644 --- a/muxplex/tests/test_auth.py +++ b/muxplex/tests/test_auth.py @@ -462,30 +462,6 @@ def test_middleware_instance_info_path_excluded(): assert response.status_code == 200 -# --------------------------------------------------------------------------- -# X-Muxplex-Token header auth (cross-origin federation) -# --------------------------------------------------------------------------- - - -def test_middleware_valid_token_header_passes(): - """Non-localhost request with a valid X-Muxplex-Token header passes through.""" - app = _make_test_app() - token = create_session_cookie("test-secret", ttl_seconds=3600) - client = TestClient(app, base_url="http://192.168.1.1") - response = client.get("/protected", headers={"X-Muxplex-Token": token}) - assert response.status_code == 200 - assert response.text == "OK" - - -def test_middleware_invalid_token_header_falls_through_to_redirect(): - """Non-localhost request with an invalid X-Muxplex-Token header falls through to redirect.""" - app = _make_test_app() - client = TestClient(app, base_url="http://192.168.1.1", follow_redirects=False) - response = client.get("/protected", headers={"X-Muxplex-Token": "bad.token.value"}) - # Invalid token: should NOT pass auth, falls through to redirect or 401 - assert response.status_code in (307, 401) - - # --------------------------------------------------------------------------- # Bearer token auth (server-to-server federation) # ---------------------------------------------------------------------------