From ce50a28dfd03a65664b18209eaf940c9bfb8ea8b Mon Sep 17 00:00:00 2001 From: YxinMiracle <56109594+YxinMiracle@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:43:49 +0800 Subject: [PATCH] fix(auth): enforce write permission for Live Browser WebSockets (#5621) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(auth): enforce write permission for Live Browser WebSockets Resolve route permissions before accepting browser streams and require threads:write, matching the existing HTTP navigation endpoint. Preserve shared authorization failure semantics and reject unexpected setup errors before acquiring a browser session. Add authorization, frame delivery, input dispatch, cancellation, and ownership regressions. Document the admission-only permission check. * fix(auth): improve browser authorization diagnostics --------- Co-authored-by: YxinMiracle <“939157765@qq.com”> --- README.md | 2 + backend/app/gateway/AGENTS.md | 2 +- backend/app/gateway/routers/browser.py | 17 +- .../tests/test_browser_readonly_security.py | 225 ++++++++++++++++++ backend/tests/test_browser_router.py | 14 +- 5 files changed, 256 insertions(+), 4 deletions(-) create mode 100644 backend/tests/test_browser_readonly_security.py diff --git a/README.md b/README.md index 0e900de61..10b81d55e 100644 --- a/README.md +++ b/README.md @@ -384,6 +384,8 @@ Existing valid JSONL records remain readable without rewriting the files. The unified nginx endpoint is same-origin by default and does not emit browser CORS headers. If you run a split-origin or port-forwarded browser client, set `GATEWAY_CORS_ORIGINS` to comma-separated exact origins such as `http://localhost:3000`; the Gateway then applies the CORS allowlist and matching CSRF origin checks. +When fine-grained authorization is enabled, Live Browser connections require `threads:write` as well as ownership of the thread, even when only viewing frames: the same connection can control the browser. Permission checks run when connecting. Restart Gateway after upgrading to disconnect sessions admitted by older code. + Browser login uses `HttpOnly` session cookies. The login page offers a "keep me signed in" option that extends the browser session when the request is HTTPS (including trusted `X-Forwarded-Proto: https`) or localhost HTTP. The localhost exception uses the direct request `Host` and ignores forwarded host headers. Public HTTP deployments, including many temporary sandbox URLs, fall back to session cookies by default. DeerFlow never stores the password in browser storage; the UI may remember only the email address. DeerFlow still uses `Forwarded` / `X-Forwarded-*` headers to recover the browser-facing scheme and origin behind a proxy. The bundled nginx sets `X-Forwarded-Proto`, but preserves an upstream HTTPS value and does not overwrite every forwarded header. Configure the outer trusted proxy to replace or strip client-supplied forwarding headers before traffic reaches DeerFlow. diff --git a/backend/app/gateway/AGENTS.md b/backend/app/gateway/AGENTS.md index b5ff56f12..60ba34e07 100644 --- a/backend/app/gateway/AGENTS.md +++ b/backend/app/gateway/AGENTS.md @@ -211,6 +211,6 @@ failure policy rather than exposing the unfiltered user-scoped catalog. ### Route and skill-listing authorization -Gateway route authorization uses `authz.py::resolve_route_permissions()` as the single provider integration point for both `AuthMiddleware` and decorator-only authentication. When enabled, it evaluates the six registered `threads:*` / `runs:*` permissions as `resource="route"` requests whose targets are the full `resource:action` strings. Decisions use the async provider API and are cached for the request in `AuthContext`; decorators do not call the provider again. Provider resolution or decision errors follow `authorization.fail_closed`, scoped per permission for decision errors. When authorization is disabled, the legacy complete permission set is returned without resolving a provider. Existing `owner_check` enforcement and `require_admin_user()` management gates remain independent and unchanged. Tests: `tests/test_authorization_route_permissions.py`, `tests/test_auth.py`, and `tests/test_auth_middleware.py`. +`authz.py::resolve_route_permissions()` is shared by HTTP middleware, decorator-only auth, and Live Browser WebSocket admission. It evaluates registered permissions asynchronously as `resource="route"`, targeting full `resource:action` strings. HTTP caches decisions in `AuthContext`; provider errors follow `authorization.fail_closed` (decision errors are per-permission). Disabled authorization returns all permissions without a provider. Owner and admin checks remain independent. Live requires `threads:write` with `is_internal=False` after login/Origin checks but before acceptance, owner lookup, or session acquisition; denial closes 4403, unexpected setup errors close 4501, cancellation propagates. Checks are admission-only; restart Gateway to close old connections. Tests: `test_authorization_route_permissions.py`, `test_browser_readonly_security.py`, `test_auth.py`, `test_auth_middleware.py` in `tests/`. Skill listing authorization mirrors the models pattern: `routers/skills.py` routes resolve `(provider, principal)` through `authz.py::resolve_skill_authorization()` — a thin sibling of `resolve_model_authorization`, both delegating to the shared `_resolve_route_scoped_authorization` core — and the shared `_filter_visible_skills` helper filters the user-scoped catalog (public + caller's custom skills) through `filter_resources(principal, "skill", ...)` by name. Three user-facing surfaces apply it: `GET /api/skills` (the frontend skill list / slash-command autocomplete), `GET /api/skills/custom`, and `GET /api/skills/{name}` — the detail endpoint returns the standard 404 for an invisible skill so it cannot become an existence oracle the filtered list closed (`get_model`'s 403 is an execution decision via `authorize("model", "use")`, which has no skills equivalent in this layer). Anonymous callers are unfiltered (mirroring `list_models`), and provider resolution/decision errors follow `authorization.fail_closed` (fail-closed → empty listing / 404, fail-open → full listing). Skill management endpoints (`install`/`upload`/`reload`/custom-skill CRUD) stay `require_admin_user`-gated, and runtime activation authorization is a separate layer. The built-in RBAC provider maps this to the per-role `skills` policy key. Tests: `tests/test_skills_listing_authorization.py`. diff --git a/backend/app/gateway/routers/browser.py b/backend/app/gateway/routers/browser.py index 50817ea4e..bcb63ecd3 100644 --- a/backend/app/gateway/routers/browser.py +++ b/backend/app/gateway/routers/browser.py @@ -7,7 +7,7 @@ import logging from fastapi import APIRouter, HTTPException, Request, WebSocket, WebSocketDisconnect from pydantic import BaseModel, Field -from app.gateway.authz import require_permission +from app.gateway.authz import Permissions, require_permission, resolve_route_permissions from app.gateway.browser_capability import browser_capability from deerflow.config.paths import get_paths from deerflow.runtime.user_context import get_effective_user_id, reset_current_user, set_current_user @@ -222,6 +222,21 @@ async def browser_stream(websocket: WebSocket, thread_id: ThreadId) -> None: await websocket.close(code=4403) return + # HTTP auth middleware does not run for WebSockets. Live is bidirectional, + # so even a viewer must have the same write permission as REST navigation. + # _authenticate_ws accepts session cookies or the auth-disabled user, not + # internal-auth tokens. Both sources are non-internal, including the + # synthetic admin used when authentication is disabled. + try: + permissions = await resolve_route_permissions(user, is_internal=False) + except Exception: + logger.warning("Failed to resolve browser stream permissions", exc_info=True) + await websocket.close(code=4501) + return + if Permissions.THREADS_WRITE not in permissions: + await websocket.close(code=4403) + return + thread_store = getattr(websocket.app.state, "thread_store", None) if thread_store is None: # Fail closed: the live stream drives a real browser (cookies, diff --git a/backend/tests/test_browser_readonly_security.py b/backend/tests/test_browser_readonly_security.py new file mode 100644 index 000000000..ba8b625d9 --- /dev/null +++ b/backend/tests/test_browser_readonly_security.py @@ -0,0 +1,225 @@ +"""Offline A1 regression: fake identity/store/browser, real route authorization. + +No real account, database, browser, network listener, or model is used. +""" + +import asyncio +import base64 +import json +import threading +from contextlib import contextmanager +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient +from starlette.websockets import WebSocketDisconnect + +from app.gateway.auth_disabled import get_auth_disabled_user +from app.gateway.auth_middleware import AuthMiddleware +from app.gateway.routers import browser +from deerflow.authz.provider import AuthzDecision +from deerflow.authz.rbac import RbacAuthorizationProvider +from deerflow.config.authorization_config import AuthorizationConfig + + +@pytest.mark.parametrize("role", ["admin", "user"]) +@pytest.mark.parametrize("writable", [False, True]) +@pytest.mark.parametrize("binary", [False, True]) +def test_browser_stream_enforces_write_permission(monkeypatch, role, writable, binary): + permissions = ["threads:read", "runs:read", "projects:read"] + if writable: + permissions.append("threads:write") + policy = { + "routes": {"allow": permissions}, + "tools": {"allow": False}, + "sandbox": {"allow": False}, + "mcp_servers": {"allow": False}, + "models": {"allow": "*"}, + "skills": {"allow": "*"}, + } + provider = RbacAuthorizationProvider(roles={"admin": policy, "user": policy}) + config = AuthorizationConfig(enabled=True, fail_closed=True, default_role="user") + monkeypatch.setattr("app.gateway.authz._get_route_authorization_config", lambda: config) + monkeypatch.setattr("app.gateway.authz._get_cached_route_provider", lambda _: provider) + user = SimpleNamespace(id="readonly-demo-owner", system_role=role, oauth_provider=None, oauth_id=None) + monkeypatch.setattr("app.gateway.deps.get_current_user_from_request", AsyncMock(return_value=user)) + monkeypatch.setattr(browser, "_authenticate_ws", AsyncMock(return_value=user)) + monkeypatch.setattr(browser, "_browser_tools_enabled", lambda: True) + monkeypatch.setattr("deerflow.config.get_app_config", lambda: SimpleNamespace(get_tool_config=lambda _: None)) + + received = threading.Event() + frame_sent = threading.Event() + send_browser_frame = browser._send_browser_frame + + async def send_frame(websocket, data, *, binary): + await send_browser_frame(websocket, data, binary=binary) + frame_sent.set() + + monkeypatch.setattr(browser, "_send_browser_frame", send_frame) + events = [] + acquisitions = [] + frame = b"\xff\xd8\xffsynthetic-frame\xff\xd9" + + async def start_screencast(on_frame): + on_frame(frame) + + session = SimpleNamespace( + start_screencast=AsyncMock(side_effect=start_screencast), + stop_screencast=AsyncMock(), + current_url=AsyncMock(return_value="about:blank"), + tabs=AsyncMock(return_value=[]), + ) + + async def dispatch_input(event): + events.append(event) + received.set() + + session.dispatch_input = dispatch_input + + @contextmanager + def acquire_session(thread_id, **kwargs): + acquisitions.append(thread_id) + yield session + + monkeypatch.setattr( + "deerflow.community.browser_automation.get_browser_session_manager", + lambda: SimpleNamespace(acquire_session=acquire_session), + ) + app = FastAPI() + app.include_router(browser.router) + app.add_middleware(AuthMiddleware) + app.state.thread_store = SimpleNamespace(get=AsyncMock(return_value={"user_id": user.id})) + click = {"type": "click", "nx": 0.25, "ny": 0.5} + close_code = None + accepted = False + with TestClient(app) as client: + client.cookies.set("access_token", "synthetic-cookie-not-a-real-token") + if not writable: + response = client.post("/api/threads/test-thread/browser/navigate", json={"url": "https://example.invalid"}) + assert response.status_code == 403, response.text + assert acquisitions == [] + print(f"{role}: HTTP navigation=403") + try: + path = "/api/threads/test-thread/browser/stream" + ("?frame_format=binary" if binary else "") + with client.websocket_connect(path, headers={"origin": "http://testserver"}) as ws: + accepted = True + ws.send_json(click) + assert received.wait(3), "Accepted connection, but no click reached the fake browser within 3 seconds" + assert frame_sent.wait(3), "No frame was sent within 3 seconds" + while True: + message = ws.receive() + if "bytes" in message: + assert binary and message["bytes"] == frame + break + payload = json.loads(message["text"]) + if payload.get("type") == "frame": + assert not binary and payload["data"] == base64.b64encode(frame).decode("ascii") + break + except WebSocketDisconnect as exc: + close_code = exc.code + + print(f"{role}: writable={writable}, WS accepted={accepted}, close={close_code}, fake browser events={events}") + if writable: + assert accepted and events == [click] + session.stop_screencast.assert_awaited_once() + else: + assert (accepted, close_code, acquisitions, events) == (False, 4403, [], []), "Read-only identity reached browser control" + session.start_screencast.assert_not_awaited() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("failure", ["none", "resolve", "missing", "decision", "invalid_decision", "unknown_role"]) +@pytest.mark.parametrize("fail_closed", [True, False]) +async def test_browser_stream_uses_shared_authorization_failure_policy(monkeypatch, failure, fail_closed): + config = AuthorizationConfig(enabled=True, fail_closed=fail_closed) + monkeypatch.setattr("app.gateway.authz._get_route_authorization_config", lambda: config) + decision = AsyncMock(return_value=AuthzDecision(allow=True, reasons=[])) + if failure == "decision": + decision.side_effect = RuntimeError("synthetic policy outage") + elif failure == "invalid_decision": + decision.return_value = None + provider = SimpleNamespace(aauthorize=decision) + if failure == "unknown_role": + provider = RbacAuthorizationProvider(roles={"user": {"routes": {"allow": "*"}}}) + factory = MagicMock(return_value=None if failure == "missing" else provider) + if failure == "resolve": + factory.side_effect = ValueError("synthetic invalid provider") + monkeypatch.setattr("app.gateway.authz._get_cached_route_provider", factory) + user = get_auth_disabled_user() # Synthetic admin must NOT bypass configured authorization. + monkeypatch.setattr(browser, "_authenticate_ws", AsyncMock(return_value=user)) + websocket = MagicMock() + websocket.headers = {} + websocket.close = AsyncMock() + websocket.app.state.thread_store.get = AsyncMock(return_value={"user_id": user.id}) + monkeypatch.setattr(browser, "_browser_tools_enabled", lambda: True) + negotiate = AsyncMock(return_value=None) # Stop before browser acquisition for allowed cases. + monkeypatch.setattr(browser, "_negotiate_browser_frame_format", negotiate) + + await browser.browser_stream(websocket, "test-thread") + + if failure != "none" and fail_closed: + websocket.close.assert_awaited_once_with(code=4403) + websocket.app.state.thread_store.get.assert_not_awaited() + negotiate.assert_not_awaited() + else: + negotiate.assert_awaited_once() + websocket.close.assert_not_awaited() + if failure == "none": + assert all(not call.args[0].principal.is_internal for call in decision.await_args_list) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("error", [ValueError("synthetic config error"), asyncio.CancelledError()]) +async def test_browser_stream_authorization_setup_error_or_cancellation_never_accepts(monkeypatch, caplog, error): + monkeypatch.setattr(browser, "_authenticate_ws", AsyncMock(return_value=get_auth_disabled_user())) + monkeypatch.setattr("app.gateway.authz._get_route_authorization_config", MagicMock(side_effect=error)) + websocket = MagicMock() + websocket.headers = {} + websocket.close = AsyncMock() + negotiate = AsyncMock() + monkeypatch.setattr(browser, "_negotiate_browser_frame_format", negotiate) + + if isinstance(error, asyncio.CancelledError): + with pytest.raises(asyncio.CancelledError): + await browser.browser_stream(websocket, "test-thread") + websocket.close.assert_not_awaited() + else: + await browser.browser_stream(websocket, "test-thread") + websocket.close.assert_awaited_once_with(code=4501) + records = [record for record in caplog.records if record.name == browser.logger.name and record.getMessage() == "Failed to resolve browser stream permissions"] + if isinstance(error, asyncio.CancelledError): + assert records == [] + else: + assert len(records) == 1 + assert records[0].exc_info is not None + assert records[0].exc_info[1] is error + assert records[0].exc_info[2] is not None + negotiate.assert_not_awaited() + websocket.app.state.thread_store.get.assert_not_called() + + +@pytest.mark.asyncio +async def test_browser_stream_rechecks_policy_on_new_connection(monkeypatch): + config = AuthorizationConfig(enabled=False) + monkeypatch.setattr("app.gateway.authz._get_route_authorization_config", lambda: config) + provider = RbacAuthorizationProvider(roles={"admin": {"routes": {"allow": []}}}) + factory = MagicMock(return_value=provider) + monkeypatch.setattr("app.gateway.authz._get_cached_route_provider", factory) + monkeypatch.setattr(browser, "_authenticate_ws", AsyncMock(return_value=get_auth_disabled_user())) + monkeypatch.setattr(browser, "_browser_tools_enabled", lambda: True) + websocket = MagicMock() + websocket.headers = {} + websocket.close = AsyncMock() + websocket.app.state.thread_store.get = AsyncMock(return_value={"user_id": get_auth_disabled_user().id}) + negotiate = AsyncMock(return_value=None) + monkeypatch.setattr(browser, "_negotiate_browser_frame_format", negotiate) + + await browser.browser_stream(websocket, "test-thread") + factory.assert_not_called() # Disabled mode keeps legacy behavior. + negotiate.assert_awaited_once() + config.enabled = True + await browser.browser_stream(websocket, "test-thread") + websocket.close.assert_awaited_once_with(code=4403) + negotiate.assert_awaited_once() # Second connection never gets this far. diff --git a/backend/tests/test_browser_router.py b/backend/tests/test_browser_router.py index 3cbe9a151..2f57a436c 100644 --- a/backend/tests/test_browser_router.py +++ b/backend/tests/test_browser_router.py @@ -1,3 +1,4 @@ +import ipaddress import json import logging from types import SimpleNamespace @@ -17,6 +18,13 @@ from app.gateway.routers.browser import ( _should_apply_browser_seed, _ws_origin_allowed, ) +from deerflow.config.authorization_config import AuthorizationConfig + + +@pytest.fixture(autouse=True) +def _isolate_route_authorization(monkeypatch): + # Existing routing/ownership tests must not inherit local operator policies. + monkeypatch.setattr("app.gateway.authz._get_route_authorization_config", lambda: AuthorizationConfig(enabled=False)) class _FakeWebSocket: @@ -64,10 +72,11 @@ def test_browser_stream_closes_4404_when_thread_store_missing(): _expect_ws_close(app, 4404) -def test_browser_stream_rejects_legacy_null_owner_thread(): +@pytest.mark.parametrize("record", [None, {"user_id": None}, {"user_id": "another-user"}]) +def test_browser_stream_rejects_missing_or_unowned_thread(record): store = MagicMock() store.check_access = AsyncMock(return_value=True) - store.get = AsyncMock(return_value={"thread_id": "thread-1", "user_id": None}) + store.get = AsyncMock(return_value=record) app = _browser_ws_app(store) with ( patch.object(browser_router, "_authenticate_ws", AsyncMock(return_value=_user())), @@ -332,4 +341,5 @@ def test_validate_browser_url_rejects_private_and_non_http(monkeypatch): assert validate_browser_url("file:///etc/passwd") is not None assert validate_browser_url("ftp://example.com") is not None # A normal public URL passes (returns None = allowed). + monkeypatch.setattr(browser_tools, "_resolve_host_addresses", lambda _host: [ipaddress.ip_address("93.184.215.14")]) assert validate_browser_url("https://github.com/bytedance/deer-flow") is None