diff --git a/CHANGELOG.md b/CHANGELOG.md index 24459d5f69..735acb90b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1397,6 +1397,15 @@ As of 2026-10-21, the milestone includes **829 merged pull requests**. ### Fixed +- **frontend:** SSO sign-ins now return to the page that asked for them, such + as a shared chat link, instead of always opening the default workspace. The + sign-in pages used to send every signed-in request to `/workspace`, which + dropped the `next` path of `/auth/callback?next=...`. A signed-in visit to + `/login?next=...` lost its target the same way. The frontend and Gateway + `next` checks now also reject control characters: browsers delete tab, LF + and CR from a URL, so `?next=%2F%0A%2F%2Fevil.example` could send a + sign-in to another site. ([#7467]) + - **authz:** Authenticated Gateway requests no longer read `config.yaml` and `extensions_config.json` on the event loop. Resolving route permissions checked both files (a `stat` and a full sha256 of each) for every request, @@ -13111,6 +13120,7 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#7410]: https://github.com/bytedance/deer-flow/pull/7410 [#7445]: https://github.com/bytedance/deer-flow/pull/7445 [#7449]: https://github.com/bytedance/deer-flow/pull/7449 +[#7467]: https://github.com/bytedance/deer-flow/pull/7467 [#7507]: https://github.com/bytedance/deer-flow/pull/7507 [#7521]: https://github.com/bytedance/deer-flow/pull/7521 [#7530]: https://github.com/bytedance/deer-flow/pull/7530 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index f667c2b842..c679787dcc 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -1112,6 +1112,13 @@ ### 修复 +- **前端：** SSO 登录后现在会回到发起登录的页面（例如分享的对话链接），不再总是打开 + 默认工作区。此前登录相关页面会把所有已登录的请求都跳转到 `/workspace`，导致 + `/auth/callback?next=...` 中的 `next` 路径丢失；已登录时访问 `/login?next=...` + 也会同样丢失目标页面。前端和 Gateway 对 `next` 的校验现在也会拒绝控制字符：浏览器会 + 删除 URL 中的制表符、LF 和 CR，因此 `?next=%2F%0A%2F%2Fevil.example` 可能把登录后的跳转 + 导向其他网站。([#7467]) + - **鉴权：** 已认证的 Gateway 请求不再在事件循环上读取 `config.yaml` 和 `extensions_config.json`。解析路由权限时，每个请求都会检查这两个文件（各做一次 `stat` 和完整的 sha256），编辑后还会在循环上重新解析 `config.yaml`，期间其他请求和 @@ -10465,6 +10472,7 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#7410]: https://github.com/bytedance/deer-flow/pull/7410 [#7445]: https://github.com/bytedance/deer-flow/pull/7445 [#7449]: https://github.com/bytedance/deer-flow/pull/7449 +[#7467]: https://github.com/bytedance/deer-flow/pull/7467 [#7507]: https://github.com/bytedance/deer-flow/pull/7507 [#7521]: https://github.com/bytedance/deer-flow/pull/7521 [#7530]: https://github.com/bytedance/deer-flow/pull/7530 diff --git a/README.md b/README.md index dc3f074c40..9537f86221 100644 --- a/README.md +++ b/README.md @@ -644,6 +644,8 @@ Passwords, including surrounding spaces, are submitted unchanged. OIDC/SSO authorization endpoints may include provider query parameters such as `?prompt=login`. DeerFlow preserves those parameters when adding the login request, including for configured `authorization_endpoint` overrides. +After an SSO sign-in, DeerFlow returns you to the page that asked you to sign in instead of the default workspace. Opening the login page while already signed in does the same for its `next` target. + Administrators can suspend and restore accounts from **Settings → Users** (admin-only; the section is hidden for ordinary users). The page lists each account's email, role, and status and exposes the same enable/disable operation as `PATCH /api/v1/admin/users/{id}`; the last remaining active admin cannot be disabled, and the Gateway's rejection detail is shown verbatim. A disabled account is rejected by every authentication surface, and its live sessions end on the next request with a login page that states the account has been disabled. Password login for a disabled account deliberately still answers "incorrect email or password" so the state is not disclosed to whoever holds the credentials. 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/routers/auth.py b/backend/app/gateway/routers/auth.py index f8cca960ac..7765ce3df2 100644 --- a/backend/app/gateway/routers/auth.py +++ b/backend/app/gateway/routers/auth.py @@ -1145,8 +1145,10 @@ def validate_next_param(next_param: str | None) -> str | None: """Validate and sanitize the ``next`` redirect parameter. Only allows relative paths starting with ``/``. Rejects protocol-relative - URLs (``//``), absolute URLs, URLs with embedded protocols, and backslashes - that URL parsers may reinterpret as forward slashes. + URLs (``//``), absolute URLs, URLs with embedded protocols, backslashes + that URL parsers may reinterpret as forward slashes, and control + characters: browsers delete tab, LF and CR before parsing a URL, so + ``/\\n//evil.example`` would otherwise reach the frontend as ``//evil.example``. """ if not next_param: return None @@ -1158,4 +1160,6 @@ def validate_next_param(next_param: str | None) -> str | None: return None if ":" in next_param: return None + if any(ord(char) < 0x20 or ord(char) == 0x7F for char in next_param): + return None return next_param diff --git a/backend/tests/test_auth.py b/backend/tests/test_auth.py index e88cb3bf5e..2f693551c7 100644 --- a/backend/tests/test_auth.py +++ b/backend/tests/test_auth.py @@ -2250,3 +2250,13 @@ def test_validate_next_param_rejects_unsafe_paths(): assert validate_next_param("/foo\\bar") is None assert validate_next_param("//evil.example") is None assert validate_next_param("https://evil.example") is None + # Browsers delete tab, LF and CR before parsing, turning these into + # protocol-relative URLs; every other C0 control and DEL is rejected too. + assert validate_next_param("/\t//evil.example") is None + assert validate_next_param("/\n//evil.example") is None + assert validate_next_param("/\r//evil.example") is None + assert validate_next_param("/\n/evil.example") is None + assert validate_next_param("/workspace\x00") is None + assert validate_next_param("/workspace\x7f") is None + # Percent-encoded forms are not decoded by URL parsers and stay local. + assert validate_next_param("/%0A//evil.example") == "/%0A//evil.example" diff --git a/backend/tests/test_oidc_auth.py b/backend/tests/test_oidc_auth.py index 22282bd5d4..ba64029362 100644 --- a/backend/tests/test_oidc_auth.py +++ b/backend/tests/test_oidc_auth.py @@ -88,8 +88,7 @@ async def test_oidc_authorization_url_preserves_endpoint_query(endpoint_query, w assert parsed.query.startswith(endpoint_query[1:] + "&") -@pytest.mark.asyncio -async def test_oidc_login_redirect_preserves_configured_endpoint_query(monkeypatch): +async def _start_oidc_login(monkeypatch, provider, path="/api/v1/auth/oauth/keycloak"): from types import SimpleNamespace import httpx @@ -101,7 +100,6 @@ async def test_oidc_login_redirect_preserves_configured_endpoint_query(monkeypat from app.gateway.routers import auth as auth_router service = OIDCService() - provider = _provider_config(authorization_endpoint="https://issuer.example.com/auth?prompt=login") config = SimpleNamespace(auth=SimpleNamespace(oidc=SimpleNamespace(enabled=True, providers={"keycloak": provider}))) monkeypatch.setattr(app_config_module, "get_app_config", lambda: config) monkeypatch.setattr(auth_router, "_get_oidc_service", lambda: service) @@ -125,10 +123,18 @@ async def test_oidc_login_redirect_preserves_configured_endpoint_query(monkeypat app.include_router(auth_router.router) try: async with httpx.AsyncClient(transport=httpx.ASGITransport(app), base_url="https://app.example.com") as client: - response = await client.get("/api/v1/auth/oauth/keycloak", follow_redirects=False) + return await client.get(path, follow_redirects=False) finally: await service.close() + +@pytest.mark.asyncio +async def test_oidc_login_redirect_preserves_configured_endpoint_query(monkeypatch): + from app.gateway.auth import oidc_state + + provider = _provider_config(authorization_endpoint="https://issuer.example.com/auth?prompt=login") + response = await _start_oidc_login(monkeypatch, provider) + assert response.status_code == 302 query = parse_qs(urlsplit(response.headers["location"]).query) assert query["prompt"] == ["login"] @@ -142,6 +148,29 @@ async def test_oidc_login_redirect_preserves_configured_endpoint_query(monkeypat assert query["code_challenge"] == [oidc_state.compute_code_challenge(state.code_verifier)] +@pytest.mark.asyncio +@pytest.mark.parametrize( + "next_query,expected", + [ + ("%2Fworkspace%2Fchats%2Fabc-123", "/workspace/chats/abc-123"), + # Browsers delete tab, LF and CR before parsing the callback's next, so + # each of these would land on //evil.example. + ("%2F%0A%2F%2Fevil.example", "/workspace"), + ("%2F%09%2F%2Fevil.example", "/workspace"), + ("%2F%0D%2F%2Fevil.example", "/workspace"), + ], +) +async def test_oidc_login_stores_only_a_safe_next_path(monkeypatch, next_query, expected): + from app.gateway.auth import oidc_state + + response = await _start_oidc_login(monkeypatch, _provider_config(), path=f"/api/v1/auth/oauth/keycloak?next={next_query}") + + assert response.status_code == 302 + state = oidc_state._verify_state_signed(response.cookies["df_oidc_state_keycloak"]) + assert state is not None + assert state.next_path == expected + + @pytest.mark.asyncio async def test_oidc_existing_local_account_blocks_sso_login_even_when_unverified(): local_user = User(email="user@example.com", password_hash="hash") diff --git a/frontend/src/app/(auth)/layout.tsx b/frontend/src/app/(auth)/layout.tsx index 84319d8b68..7100606d9a 100644 --- a/frontend/src/app/(auth)/layout.tsx +++ b/frontend/src/app/(auth)/layout.tsx @@ -1,4 +1,3 @@ -import { redirect } from "next/navigation"; import { type ReactNode } from "react"; import { GatewayOfflineFallback } from "@/components/workspace/gateway-offline-fallback"; @@ -22,9 +21,12 @@ export default async function AuthLayout({ switch (result.tag) { case "authenticated": - redirect("/workspace"); + // Signed-in sessions render the page instead of redirecting here. Layouts + // cannot read searchParams, so a redirect from this layout would always + // drop `next`, and the post-SSO /auth/callback?next=… arrives with the + // fresh session cookie. Each page sends a signed-in user on: login and + // the callback to the validated `next`, setup to the workspace. case "needs_setup": - // Allow access to setup page content = ( {children} ); diff --git a/frontend/src/app/(auth)/login/page.tsx b/frontend/src/app/(auth)/login/page.tsx index d6d857ea1e..1d02135f68 100644 --- a/frontend/src/app/(auth)/login/page.tsx +++ b/frontend/src/app/(auth)/login/page.tsx @@ -81,10 +81,11 @@ export default function LoginPage() { setupStatusPhase === "unavailable" || (setupStatusAttempt > 0 && setupStatusPhase === "checking"); - // Redirect if already authenticated (client-side, post-login) + // Redirect if already authenticated. Replace, so Back does not return to a + // sign-in page that immediately redirects again. useEffect(() => { if (isAuthenticated) { - router.push(redirectPath); + router.replace(redirectPath); } }, [isAuthenticated, redirectPath, router]); @@ -231,6 +232,16 @@ export default function LoginPage() { } }; + // A signed-in session is already on its way to `redirectPath` (see the + // effect above), so it never sees the sign-in form. + if (isAuthenticated) { + return ( +
+

{t.login.pleaseWait}

+
+ ); + } + const actualTheme = theme === "system" ? resolvedTheme : theme; return ( diff --git a/frontend/src/core/auth/next-path.ts b/frontend/src/core/auth/next-path.ts index 41a96b2635..31bb88a86d 100644 --- a/frontend/src/core/auth/next-path.ts +++ b/frontend/src/core/auth/next-path.ts @@ -1,5 +1,18 @@ export const DEFAULT_AUTH_NEXT_PATH = "/workspace"; +// The URL parser deletes tab, LF and CR before parsing, so `/\n//evil.example` +// passes the `//` check below and then navigates to `//evil.example`. +// Reject every C0 control and DEL rather than only the three it strips. +function hasControlCharacter(value: string): boolean { + for (let index = 0; index < value.length; index++) { + const code = value.charCodeAt(index); + if (code < 0x20 || code === 0x7f) { + return true; + } + } + return false; +} + export function validateAuthNextPath( nextPath: string | null | undefined, ): string | null { @@ -15,6 +28,9 @@ export function validateAuthNextPath( if (nextPath.includes("\\") || nextPath.includes(":")) { return null; } + if (hasControlCharacter(nextPath)) { + return null; + } return nextPath; } diff --git a/frontend/tests/unit/app/auth-layout-next.dom.test.tsx b/frontend/tests/unit/app/auth-layout-next.dom.test.tsx new file mode 100644 index 0000000000..4236e29e2b --- /dev/null +++ b/frontend/tests/unit/app/auth-layout-next.dom.test.tsx @@ -0,0 +1,167 @@ +import { afterEach, beforeEach, describe, expect, it, rs } from "@rstest/core"; +import { cleanup, render, screen, waitFor } from "@testing-library/react"; +import type { ReactNode } from "react"; + +const replace = rs.fn(); +const push = rs.fn(); +// Next's app router object is stable across renders; a fresh object per call +// would re-run every effect that depends on it. +const router = { push, replace }; +const redirect = rs.fn((url: string) => { + throw new Error(`NEXT_REDIRECT ${url}`); +}); +let searchParams = new URLSearchParams(); + +rs.mock("next/navigation", () => ({ + useRouter: () => router, + usePathname: () => window.location.pathname, + useSearchParams: () => searchParams, + redirect: (url: string) => redirect(url), +})); +rs.mock("@/env", () => ({ + env: { NEXT_PUBLIC_STATIC_WEBSITE_ONLY: "false" }, +})); +rs.mock("@/core/auth/server", () => ({ getServerSideUser: rs.fn() })); +rs.mock("@/core/i18n/server", () => ({ + detectLocaleServer: async () => "en-US", +})); +rs.mock("@/core/i18n/context", () => ({ + I18nProvider: ({ children }: { children: ReactNode }) => children, +})); +rs.mock("@/core/i18n/hooks", () => ({ useI18n: () => ({ t: enUS }) })); +rs.mock("next-themes", () => ({ + useTheme: () => ({ theme: "light", resolvedTheme: "light" }), +})); +rs.mock("@/components/ui/flickering-grid", () => ({ + FlickeringGrid: () => null, +})); +rs.mock("@/core/auth/setup", () => ({ + ...(rs.requireActual("@/core/auth/setup") as object), + fetchSetupStatus: rs.fn(async () => ({ + needs_setup: false, + registration_enabled: true, + })), +})); + +import AuthCallbackPage from "@/app/(auth)/auth/callback/page"; +import AuthLayout from "@/app/(auth)/layout"; +import LoginPage from "@/app/(auth)/login/page"; +import SetupPage from "@/app/(auth)/setup/page"; +import { getServerSideUser } from "@/core/auth/server"; +import type * as Setup from "@/core/auth/setup"; +import type { AuthResult, User } from "@/core/auth/types"; +import { enUS } from "@/core/i18n/locales/en-US"; + +const user: User = { + id: "u-1", + email: "sso@example.com", + system_role: "user", + needs_setup: false, + oauth_provider: "keycloak", +}; +const DEEP_LINK = "/workspace/chats/abc-123?tab=files"; + +const jsonResponse = (body: unknown, status = 200) => + new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, + }); + +async function renderAuthRoute(result: AuthResult, page: ReactNode) { + rs.mocked(getServerSideUser).mockResolvedValue(result); + render(await AuthLayout({ children: page })); +} + +beforeEach(() => { + // The auth pages fetch plain path strings. + globalThis.fetch = rs.fn(async (input: string) => + input.endsWith("/api/v1/auth/me") + ? jsonResponse(user) + : jsonResponse({ providers: [] }), + ) as unknown as typeof globalThis.fetch; +}); +afterEach(() => { + cleanup(); + rs.clearAllMocks(); + searchParams = new URLSearchParams(); +}); + +describe("auth routes with a signed-in session", () => { + it("sends the SSO callback to its next path", async () => { + // The Gateway sets the session cookie on the same redirect that lands + // here, so the layout always sees this request as authenticated. + searchParams = new URLSearchParams({ next: DEEP_LINK }); + + await renderAuthRoute({ tag: "authenticated", user }, ); + + expect(redirect).not.toHaveBeenCalled(); + await waitFor(() => expect(replace).toHaveBeenCalledWith(DEEP_LINK)); + expect(replace).toHaveBeenCalledTimes(1); + }); + + // `/\n//…` and `/\t//…` become `//evil.example/phish` once the URL parser + // deletes the control character. + const UNSAFE_NEXT = [ + "//evil.example/phish", + "/\n//evil.example/phish", + "/\t//evil.example/phish", + ]; + + it.each(UNSAFE_NEXT)( + "sends the SSO callback to the workspace for unsafe next %j", + async (next) => { + searchParams = new URLSearchParams({ next }); + + await renderAuthRoute( + { tag: "authenticated", user }, + , + ); + + await waitFor(() => expect(replace).toHaveBeenCalledWith("/workspace")); + expect(replace).toHaveBeenCalledTimes(1); + }, + ); + + it.each(UNSAFE_NEXT)( + "sends a signed-in login visit to the workspace for unsafe next %j", + async (next) => { + searchParams = new URLSearchParams({ next }); + + await renderAuthRoute({ tag: "authenticated", user }, ); + + await waitFor(() => expect(replace).toHaveBeenCalledWith("/workspace")); + expect(replace).toHaveBeenCalledTimes(1); + }, + ); + + it("sends a signed-in login visit to its next path without the form", async () => { + searchParams = new URLSearchParams({ next: DEEP_LINK }); + + await renderAuthRoute({ tag: "authenticated", user }, ); + + expect(redirect).not.toHaveBeenCalled(); + await waitFor(() => expect(replace).toHaveBeenCalledWith(DEEP_LINK)); + expect(push).not.toHaveBeenCalled(); + expect(screen.queryByLabelText(enUS.login.email)).toBeNull(); + expect(screen.getByText(enUS.login.pleaseWait)).toBeTruthy(); + }); + + it("sends a signed-in setup visit to the workspace", async () => { + await renderAuthRoute({ tag: "authenticated", user }, ); + + expect(redirect).not.toHaveBeenCalled(); + await waitFor(() => expect(replace).toHaveBeenCalledWith("/workspace")); + }); +}); + +describe("auth routes without a session", () => { + it("renders the sign-in form and stays on the page", async () => { + searchParams = new URLSearchParams({ next: DEEP_LINK }); + + await renderAuthRoute({ tag: "unauthenticated" }, ); + + expect(screen.getByLabelText(enUS.login.email)).toBeTruthy(); + expect(replace).not.toHaveBeenCalled(); + expect(push).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/tests/unit/core/auth/next-path.test.ts b/frontend/tests/unit/core/auth/next-path.test.ts index 1d9a06e295..d87ef9a47d 100644 --- a/frontend/tests/unit/core/auth/next-path.test.ts +++ b/frontend/tests/unit/core/auth/next-path.test.ts @@ -23,8 +23,24 @@ describe("auth next path validation", () => { expect(validateAuthNextPath("/foo\\bar")).toBeNull(); }); + test("rejects control characters the URL parser would delete", () => { + // Tab, LF and CR are deleted before parsing, which turns each of these + // into a protocol-relative URL for evil.example. + expect(validateAuthNextPath("/\t//evil.example")).toBeNull(); + expect(validateAuthNextPath("/\n//evil.example")).toBeNull(); + expect(validateAuthNextPath("/\r//evil.example")).toBeNull(); + expect(validateAuthNextPath("/\n/evil.example")).toBeNull(); + expect(validateAuthNextPath("/workspace\u0000")).toBeNull(); + expect(validateAuthNextPath("/workspace\u007f")).toBeNull(); + // Percent-encoded forms are not decoded by the parser and stay local. + expect(validateAuthNextPath("/%0A//evil.example")).toBe( + "/%0A//evil.example", + ); + }); + test("falls back for unsafe paths", () => { expect(resolveAuthNextPath("/\\evil.example")).toBe("/workspace"); + expect(resolveAuthNextPath("/\n//evil.example")).toBe("/workspace"); expect(resolveAuthNextPath("/safe", "/fallback")).toBe("/safe"); }); });