fix: authorize same-origin conversation reads without Origin
This commit is contained in:
@@ -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<ChatMessagePage> {
|
||||
let (account, _) = account_context(&api, &context, &workspace).await?;
|
||||
let (account, _) = read_context(&api, &context, &workspace).await?;
|
||||
api.store
|
||||
.db_executor()
|
||||
.run(move || -> ApiResult<_> {
|
||||
|
||||
@@ -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<SubjectBodyResponse> {
|
||||
account_context(&api, &context, &workspace).await?;
|
||||
read_context(&api, &context, &workspace).await?;
|
||||
api.host_executors
|
||||
.host()?
|
||||
.run(move || {
|
||||
|
||||
@@ -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<Value>, 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;
|
||||
|
||||
@@ -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())
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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({
|
||||
|
||||
Reference in New Issue
Block a user