From f7ea2362dd88a9576163e195826e055a173db265 Mon Sep 17 00:00:00 2001 From: Hare Date: Sat, 31 Oct 2026 19:33:58 +0900 Subject: [PATCH] fix: authorize same-origin conversation reads without Origin --- crates/workspace-server/src/server/chat.rs | 14 ++- .../src/server/subject_body.rs | 14 ++- .../src/server/tests/subject_body_tests.rs | 107 ++++++++++++++++++ .../src/server/worker_operations.rs | 18 ++- .../subject_conversation_connected.ts | 7 +- .../memory/SubjectConversation.svelte | 2 + .../conversation-route-fence.browser.spec.ts | 7 +- 7 files changed, 161 insertions(+), 8 deletions(-) diff --git a/crates/workspace-server/src/server/chat.rs b/crates/workspace-server/src/server/chat.rs index be82ccd07..e411fdf30 100644 --- a/crates/workspace-server/src/server/chat.rs +++ b/crates/workspace-server/src/server/chat.rs @@ -28,6 +28,16 @@ async fn account_context( api: &WorkspaceApi, context: &ServerRequestContext, workspace: &str, +) -> ApiResult<(String, WorkerOperationContext)> { + let (account, authority) = read_context(api, context, workspace).await?; + authority.authorize_workspace(api).await?; + Ok((account, authority)) +} + +async fn read_context( + api: &WorkspaceApi, + context: &ServerRequestContext, + workspace: &str, ) -> ApiResult<(String, WorkerOperationContext)> { if context.runtime_source.is_some() || context.worker_source.is_some() { return Err(Error::WorkspacePermissionDenied( @@ -42,7 +52,7 @@ async fn account_context( let authority = WorkerOperationContext::from_request_async(api, context) .await .map_err(|error| Error::WorkspacePermissionDenied(error.message))?; - authority.authorize_workspace(api).await?; + authority.authorize_workspace_read(api).await?; Ok((actor.account_id.clone(), authority)) } @@ -70,7 +80,7 @@ pub(super) async fn messages( conversation: String, query: ChatMessagePageQuery, ) -> ApiResult { - let (account, _) = account_context(&api, &context, &workspace).await?; + let (account, _) = read_context(&api, &context, &workspace).await?; api.store .db_executor() .run(move || -> ApiResult<_> { diff --git a/crates/workspace-server/src/server/subject_body.rs b/crates/workspace-server/src/server/subject_body.rs index b1868505c..fb66fceee 100644 --- a/crates/workspace-server/src/server/subject_body.rs +++ b/crates/workspace-server/src/server/subject_body.rs @@ -141,6 +141,16 @@ async fn account_context( api: &WorkspaceApi, context: &server_api::ServerRequestContext, workspace: &str, +) -> ApiResult<(String, WorkerOperationContext)> { + let (account, authority) = read_context(api, context, workspace).await?; + authority.authorize_workspace(api).await?; + Ok((account, authority)) +} + +async fn read_context( + api: &WorkspaceApi, + context: &server_api::ServerRequestContext, + workspace: &str, ) -> ApiResult<(String, WorkerOperationContext)> { validate_workspace_scope(api, workspace)?; let actor = subject_authorization::administration_actor(context)?; @@ -148,7 +158,7 @@ async fn account_context( let authority = WorkerOperationContext::from_request_async(api, context) .await .map_err(|error| Error::WorkspacePermissionDenied(error.message))?; - authority.authorize_workspace(api).await?; + authority.authorize_workspace_read(api).await?; Ok((actor.account_id.clone(), authority)) } @@ -158,7 +168,7 @@ pub(super) async fn get( workspace: String, subject: String, ) -> ApiResult { - account_context(&api, &context, &workspace).await?; + read_context(&api, &context, &workspace).await?; api.host_executors .host()? .run(move || { diff --git a/crates/workspace-server/src/server/tests/subject_body_tests.rs b/crates/workspace-server/src/server/tests/subject_body_tests.rs index af423f87f..3f5fa2880 100644 --- a/crates/workspace-server/src/server/tests/subject_body_tests.rs +++ b/crates/workspace-server/src/server/tests/subject_body_tests.rs @@ -86,6 +86,113 @@ impl BodyFixture { } } +#[tokio::test] +async fn browser_subject_and_conversation_reads_allow_missing_origin_but_mutations_do_not() { + let f = BodyFixture::new().await; + let mut headers = HeaderMap::new(); + headers.insert( + axum::http::header::AUTHORIZATION, + format!("Bearer {}", f.owner).parse().unwrap(), + ); + let actor = resolve_actor(&ServerAuthApi::from(&f.api), &headers) + .await + .unwrap() + .unwrap(); + let token = "subject-read-browser-session"; + f.api + .store + .create_browser_session(&BrowserSessionRecord { + token_hash: crate::auth::token_hash(token), + session_id: "subject-read-session".into(), + user_id: actor.user_id, + created_at: now_registry_timestamp(), + expires_at: "2099-01-01T00:00:00Z".into(), + revoked_at: None, + }) + .unwrap(); + let AuthConfig::Passkey { + cookie_name, + origin, + .. + } = &f.api.config.auth; + let browser_request = |method: &str, path: &str, body: Option, origin: Option<&str>| { + let mut request = Request::builder() + .method(method) + .uri(path) + .header(axum::http::header::COOKIE, format!("{cookie_name}={token}")) + .header(CONTENT_TYPE, "application/json"); + if let Some(origin) = origin { + request = request.header(ORIGIN, origin); + } + request + .body(Body::from(body.map(|v| v.to_string()).unwrap_or_default())) + .unwrap() + }; + let conversations = format!("/api/w/{TEST_WORKSPACE_ID}/chat/conversations"); + let create = json!({"operation_id":"browser-read-conversation", "subject_id":f.subject}); + let (status, conversation) = response( + &f.api, + browser_request("POST", &conversations, Some(create.clone()), Some(origin)), + ) + .await; + assert_eq!(status, StatusCode::OK, "{conversation}"); + let messages = format!( + "{conversations}/{}/messages", + conversation["id"].as_str().unwrap() + ); + for path in [f.path(), messages.clone()] { + for origin in [None, Some(origin.as_str())] { + let (status, body) = + response(&f.api, browser_request("GET", &path, None, origin)).await; + assert_eq!(status, StatusCode::OK, "{path}: {body}"); + } + let (status, body) = response( + &f.api, + browser_request("GET", &path, None, Some("https://foreign.example")), + ) + .await; + assert_eq!(status, StatusCode::FORBIDDEN, "{body}"); + } + let expected = + json!({"runtime_id":"runtime-test", "worker_id":"not-started", "generation":"1"}); + for (path, body) in [ + (conversations, create), + ( + messages.clone(), + json!({"operation_id":"must-not-send", "text":"must not be saved"}), + ), + ( + format!("{}/start", f.path()), + json!({"request_id":"must-not-start", "expected_current":null, "expected_observation_token":null}), + ), + ( + format!("{}/stop", f.path()), + json!({"request_id":"must-not-stop", "expected_current":expected, "expected_observation_token":"not-observed"}), + ), + ] { + for origin in [None, Some("https://foreign.example")] { + let (status, error) = response( + &f.api, + browser_request("POST", &path, Some(body.clone()), origin), + ) + .await; + assert_eq!(status, StatusCode::FORBIDDEN, "{path}: {error}"); + } + } + let (status, page) = response(&f.api, browser_request("GET", &messages, None, None)).await; + assert_eq!(status, StatusCode::OK, "{page}"); + assert_eq!(page["messages"], json!([])); + assert!(f.read().await["current"].is_null()); + f.api + .store + .revoke_browser_session(&crate::auth::token_hash(token), &now_registry_timestamp()) + .unwrap(); + for path in [f.path(), messages] { + let (status, body) = response(&f.api, browser_request("GET", &path, None, None)).await; + assert_eq!(status, StatusCode::UNAUTHORIZED, "{body}"); + } +} + #[tokio::test] async fn body_read_retains_an_occupied_owner_when_runtime_observation_is_unavailable() { let f = BodyFixture::new().await; diff --git a/crates/workspace-server/src/server/worker_operations.rs b/crates/workspace-server/src/server/worker_operations.rs index 6157522ee..1671d9726 100644 --- a/crates/workspace-server/src/server/worker_operations.rs +++ b/crates/workspace-server/src/server/worker_operations.rs @@ -121,6 +121,20 @@ impl WorkerOperationContext { } pub(super) async fn authorize_workspace(&self, api: &WorkspaceApi) -> ApiResult<()> { + self.authorize_workspace_request(api, true).await + } + + /// Read-only snapshots revalidate the caller without requiring the Origin header + /// that browsers omit on same-origin GETs. An explicit foreign Origin still fails. + pub(super) async fn authorize_workspace_read(&self, api: &WorkspaceApi) -> ApiResult<()> { + self.authorize_workspace_request(api, false).await + } + + async fn authorize_workspace_request( + &self, + api: &WorkspaceApi, + require_origin: bool, + ) -> ApiResult<()> { if let Self::Browser { headers, source } = self { let actor = resolve_actor(&ServerAuthApi::from(api), headers) .await? @@ -129,7 +143,9 @@ impl WorkerOperationContext { "Worker connection credentials were revoked".into(), ) })?; - if matches!(actor.auth_method, ActorAuthMethod::BrowserSession) { + if matches!(actor.auth_method, ActorAuthMethod::BrowserSession) + && (require_origin || headers.contains_key(ORIGIN)) + { let AuthConfig::Passkey { origin, .. } = &api.config.auth; if headers.get(ORIGIN).and_then(|value| value.to_str().ok()) != Some(origin.as_str()) diff --git a/tools/web-ux/browser-tests/subject_conversation_connected.ts b/tools/web-ux/browser-tests/subject_conversation_connected.ts index ecc6db9b0..7dc3a9f00 100644 --- a/tools/web-ux/browser-tests/subject_conversation_connected.ts +++ b/tools/web-ux/browser-tests/subject_conversation_connected.ts @@ -74,7 +74,6 @@ try { const context = await browser.newContext({ viewport: { width: 1440, height: 1000 }, colorScheme: "light", - extraHTTPHeaders: { Origin: origin.origin }, }); const sessionCookie = (value: string) => ({ name: "yoi_workspace_session", @@ -212,7 +211,10 @@ try { const retry = posts[2]; const conflict = await context.request.post( `${origin.origin}${base}/${encodeURIComponent(input.conversation_id)}/messages`, - { data: { ...retry, text: "changed payload under original operation" } }, + { + headers: { Origin: origin.origin }, + data: { ...retry, text: "changed payload under original operation" }, + }, ); assertEquals(conflict.status(), 409); await page.goto(`${origin.origin}${legacy}/conversation${suffix}`); @@ -365,6 +367,7 @@ try { .json(); assert(ownerIdentity.actor.account_id !== foreignIdentity.actor.account_id); const denied = await context.request.post(deniedPath, { + headers: { Origin: origin.origin }, data: { operation_id: "foreign-cannot-post", text: "must not append" }, }); assertEquals(denied.status(), 403); diff --git a/web/workspace/src/lib/workspace/memory/SubjectConversation.svelte b/web/workspace/src/lib/workspace/memory/SubjectConversation.svelte index 192aa598d..2d3d09c45 100644 --- a/web/workspace/src/lib/workspace/memory/SubjectConversation.svelte +++ b/web/workspace/src/lib/workspace/memory/SubjectConversation.svelte @@ -74,6 +74,8 @@ window.addEventListener('online', reconnect); document.addEventListener('visibilitychange', reconnect); const unsubscribe = browserSession.subscribe(() => { + // A withdrawn request keeps its actual failure; an aborted fetch is not an Account change. + if (active !== context || context.controller.signal.aborted) return; if (!current(context)) { withdrawAccess(context); sendError = null; diff --git a/web/workspace/src/lib/workspace/memory/conversation-route-fence.browser.spec.ts b/web/workspace/src/lib/workspace/memory/conversation-route-fence.browser.spec.ts index 05e2b11d5..fe725ea4f 100644 --- a/web/workspace/src/lib/workspace/memory/conversation-route-fence.browser.spec.ts +++ b/web/workspace/src/lib/workspace/memory/conversation-route-fence.browser.spec.ts @@ -62,6 +62,9 @@ for (const status of [401, 403, 404]) { vi.stubGlobal( "fetch", vi.fn(async (input: string, init?: RequestInit) => { + if (String(input) === "/api/auth/whoami" && browserSession.snapshot.kind === "ready") { + return Response.json(browserSession.snapshot.value); + } if (String(input).endsWith("/conversations")) return Response.json(conversation("s")); if (init?.method === "POST") { posted = true; @@ -84,7 +87,9 @@ for (const status of [401, 403, 404]) { const attempt = readConversationAttempt(sessionStorage, props)!; denied = true; await fireEvent.click(screen.getByRole("button", { name: "Refresh history" })); - await screen.findByRole("alert"); + const deniedMessage = (await screen.findByRole("alert")).textContent; + await browserSession.refresh(); + expect(screen.getByRole("alert").textContent).toBe(deniedMessage); expect(screen.queryByText("retained private text")).toBeNull(); expect(screen.queryByRole("textbox")).toBeNull(); release(Response.json({