diff --git a/crates/worker/src/lib.rs b/crates/worker/src/lib.rs index 0037fa8cb..53fcb0fe5 100644 --- a/crates/worker/src/lib.rs +++ b/crates/worker/src/lib.rs @@ -71,9 +71,10 @@ pub use session_history::{ }; pub use shared_state::WorkerSharedState; pub use worker::{ - LocalWorkingDirectory, SubjektivSessionAttributionLifecycle, Worker, WorkerError, - WorkerFilesystemAuthority, WorkerRunResult, WorkerWorkspaceContext, WorkspaceBinaryRequest, - WorkspaceBinaryResponse, WorkspaceClient, WorkspaceClientError, WorkspaceId, WorkspaceIdError, + LocalWorkingDirectory, ReviewerChildWorkspaceClient, ReviewerContext, + SubjektivSessionAttributionLifecycle, Worker, WorkerError, WorkerFilesystemAuthority, + WorkerRunResult, WorkerWorkspaceContext, WorkspaceBinaryRequest, WorkspaceBinaryResponse, + WorkspaceClient, WorkspaceClientError, WorkspaceId, WorkspaceIdError, WorkspacePromptCatalogResolution, WorkspaceRequest, WorkspaceRequestMethod, WorkspaceResponse, WorkspaceServerOperation, WorkspaceWorkerDiscoveryRequest, apply_worker_manifest, marker_workspace_client, unavailable_workspace_client, diff --git a/crates/worker/src/worker.rs b/crates/worker/src/worker.rs index ac9d44d6e..2dad45d24 100644 --- a/crates/worker/src/worker.rs +++ b/crates/worker/src/worker.rs @@ -1332,15 +1332,28 @@ impl WorkspaceClient for ReviewerChildWorkspaceClient { mut request: WorkspaceRequest, ) -> Result { let workspace_id = self.workspace_id().unwrap_or_default(); - let ticket_id = &self.context.ticket_id; let ticket_query_path = format!("/api/w/{workspace_id}/tickets/query"); - let ticket_show_path = format!("/api/w/{workspace_id}/tickets/{ticket_id}/show"); + let ticket_show_prefix = format!("/api/w/{workspace_id}/tickets/"); + // ShowTicket uses POST for bounded read parameters. Like Ticket query + // and GET reads, it must allow related Tickets through the bound Host's + // read authority. Only review submission is restricted to the attested MR. + // Match one literal reference segment, not arbitrary POST routes. + let is_ticket_show = request + .path + .strip_prefix(&ticket_show_prefix) + .and_then(|path| path.strip_suffix("/show")) + .is_some_and(|reference| { + !reference.is_empty() + && reference + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'_')) + }); let merge_request_id = &self.context.merge_request_id; let review_path = format!("/api/w/{workspace_id}/merge-requests/{merge_request_id}/reviews"); let read_allowed = request.method == WorkspaceRequestMethod::Get || (request.method == WorkspaceRequestMethod::Post - && (request.path == ticket_query_path || request.path == ticket_show_path)); + && (request.path == ticket_query_path || is_ticket_show)); if request.method == WorkspaceRequestMethod::Post && request.path == review_path { let body = request.body.take().ok_or_else(|| { WorkspaceClientError::Request("review submission requires a JSON body".to_string()) @@ -1868,16 +1881,11 @@ mod reviewer_client_tests { } #[test] - fn reviewer_child_client_rejects_other_ticket_and_mutation_posts() { + fn reviewer_child_client_rejects_mutation_posts() { let inner: Arc = Arc::new(RecordingWorkspaceClient::default()); let client = reviewer_client(inner); for request in [ - WorkspaceRequest::json( - WorkspaceRequestMethod::Post, - "/api/w/ws/tickets/T2/show", - "{}".to_string(), - ), WorkspaceRequest::json( WorkspaceRequestMethod::Post, "/api/w/ws/tickets/T1/comments", @@ -1894,6 +1902,54 @@ mod reviewer_client_tests { } } + #[test] + fn reviewer_ticket_reads_reject_route_injection_and_mutations_without_transport() { + let inner = Arc::new(RecordingWorkspaceClient::default()); + let client = reviewer_client(inner.clone()); + for path in [ + "/api/w/other/tickets/T-1/show", + "/api/w/ws/tickets//show", + "/api/w/ws/tickets/T-1/comments", + "/api/w/ws/tickets/T-1/posts", + "/api/w/ws/tickets/T-1/show/", + "/api/w/ws/tickets/T-1/show?query=1", + "/api/w/ws/tickets/T-1?query=1/show", + "/api/w/ws/tickets/T-1#fragment/show", + "/api/w/ws/tickets/../show", + "/api/w/ws/tickets/T-1/../T-2/show", + "/api/w/ws/tickets/T%2D1/show", + "/api/w/ws/tickets/T-1%2fcomments/show", + "/api/w/ws/merge-requests/MR2/reviews", + ] { + assert!( + client + .execute(WorkspaceRequest::json( + WorkspaceRequestMethod::Post, + path, + "{}", + )) + .is_err(), + "{path}" + ); + } + for method in [ + WorkspaceRequestMethod::Put, + WorkspaceRequestMethod::Patch, + WorkspaceRequestMethod::Delete, + ] { + assert!( + client + .execute(WorkspaceRequest::json( + method, + "/api/w/ws/tickets/T-1/show", + "{}" + )) + .is_err() + ); + } + assert!(inner.requests.lock().unwrap().is_empty()); + } + #[test] fn reviewer_child_client_injects_capability_only_for_attested_review() { let inner = Arc::new(RecordingWorkspaceClient::default()); diff --git a/crates/workspace-server/src/server_ticket_tools_backend_tests.rs b/crates/workspace-server/src/server_ticket_tools_backend_tests.rs index 662bc03b0..7562c53ab 100644 --- a/crates/workspace-server/src/server_ticket_tools_backend_tests.rs +++ b/crates/workspace-server/src/server_ticket_tools_backend_tests.rs @@ -120,6 +120,14 @@ struct TicketSurface { } impl TicketSurface { async fn new(root: &Path, client: Arc, wip: bool) -> Self { + Self::with_access(root, client, wip, TicketFeatureAccess::workflow()).await + } + async fn with_access( + root: &Path, + client: Arc, + wip: bool, + access: TicketFeatureAccess, + ) -> Self { let manifest = worker::WorkerManifest::from_toml("[worker]\nname=\"ticket-tools\"\n[model]\nscheme=\"anthropic\"\nmodel_id=\"unused\"\n[engine]\n[scope]\nallow=[]\n").unwrap(); let engine = agen::Engine::<_, agen::state::Mutable, worker::SessionHistoryMetadata>::new_annotated( @@ -138,22 +146,18 @@ impl TicketSurface { ) .await .unwrap(); - let report = - host.install_features(worker::feature::FeatureRegistryBuilder::new().with_module( - TicketFeature::new(client.clone(), TicketFeatureAccess::workflow()), - )); + let report = host.install_features( + worker::feature::FeatureRegistryBuilder::new() + .with_module(TicketFeature::new(client.clone(), access)), + ); assert!(!report.has_errors()); let handle = host.engine().tool_server_handle(); handle.flush_pending(); - let tools = [ - "TicketAssignmentTrackingQuery", - "TicketAssignmentAnswer", - "TicketImplementationResult", - "TicketImplementationResultQuery", - ] - .into_iter() - .map(|name| (name.into(), handle.get_tool(name).unwrap().1)) - .collect(); + let tools = access + .tool_names() + .into_iter() + .map(|name| (name.into(), handle.get_tool(name).unwrap().1)) + .collect(); if !wip { return Self { tools, wip }; } @@ -162,7 +166,7 @@ impl TicketSurface { worker::feature::builtin::ticket::mount_workspace_http_ticket_wip( &mut mounts, client, - TicketFeatureAccess::workflow(), + access, None, &route, ) @@ -189,8 +193,9 @@ impl TicketSurface { if !self.wip { return execute_tool(self.tools[name].clone(), input, context).await; } - let path = format!("/tickets/{}", input["ticket"].as_str().unwrap()); - input.as_object_mut().unwrap().remove("ticket"); + let reference_field = if name == "ShowTicket" { "id" } else { "ticket" }; + let path = format!("/tickets/{}", input[reference_field].as_str().unwrap()); + input.as_object_mut().unwrap().remove(reference_field); // Observe the real item representation before invoking a mutation. Tracking // reads alone do not establish the Ticket WIP object validator. Explicit // refresh also reconciles stale observations after an unknown result; @@ -201,6 +206,9 @@ impl TicketSurface { ToolExecutionContext::direct(), ) .await?; + if name == "ShowTicket" { + return execute_tool(self.tools["Invoke"].clone(), serde_json::json!({"path":path,"interface":inspected["interfaces"][0]["reference"],"operation":"read","arguments":input}), context).await; + } execute_tool(self.tools["Invoke"].clone(), serde_json::json!({"path":path,"interface":inspected["interfaces"][0]["reference"],"operation":"read","arguments":{}}), ToolExecutionContext::direct()).await?; let inspected = execute_tool( self.tools["Inspect"].clone(), @@ -364,6 +372,210 @@ async fn ticket_public_roundtrip_observed_answer_replays_without_changing_waitin } } +// Reviewer policy, tool projection, reference resolution, authentication and +// persistence remain production code. Only the synchronous HTTP transport is +// replaced by signed in-process router dispatch. Child spawning and MR attestation +// have their own suites; this fixture does not claim to exercise those boundaries. +fn reviewer_client(inner: Arc, bound_ticket: &str) -> Arc { + Arc::new(worker::ReviewerChildWorkspaceClient::new( + inner, + worker::ReviewerContext { + ticket_id: bound_ticket.into(), + merge_request_id: "unsubmitted-review-candidate".into(), + }, + "unused-review-capability".into(), + )) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn reviewer_reads_saved_related_tickets_by_key_and_id_and_pages_their_threads() { + for wip in [false, true] { + let dir = tempfile::tempdir().unwrap(); + let (api, _runtime, identity, source, bound_id) = tracking_fixture(dir.path()).await; + let backend = browser_ticket_backend(&api).unwrap(); + let mut related = ticket::NewTicket::new("Related requirements"); + related.body = ticket::MarkdownText::new("Requirements from a different Ticket"); + let related = backend.create(related).unwrap(); + backend + .add_ticket_relation( + bound_id.clone().into(), + ticket::NewTicketRelation { + kind: ticket::TicketRelationKind::DependsOn, + target: related.id.clone(), + note: None, + author: None, + }, + ) + .unwrap(); + for body in ["Earlier decision", "Latest decision"] { + backend + .add_event( + related.id.clone().into(), + ticket::NewTicketEvent::new(ticket::TicketEventKind::Comment, body), + ) + .unwrap(); + } + let bound = backend.show(bound_id.clone().into()).unwrap(); + let related_before = backend.show(related.id.clone().into()).unwrap(); + let bound_key = bound.meta.resource_key.as_deref().unwrap(); + let related_key = related.resource_key.as_deref().unwrap(); + let transport = ticket_client(&api, identity, &source); + // One client must retain its review binding while following related Tickets. + let client = reviewer_client(transport.clone(), &bound_id); + let surface = + TicketSurface::with_access(dir.path(), client, wip, TicketFeatureAccess::review()) + .await; + for (reference, expected_key, expected_title, expected_body) in [ + ( + bound_key, + bound_key, + "Tracking authority", + bound.document.body.as_str(), + ), + ( + related_key, + related_key, + "Related requirements", + related_before.document.body.as_str(), + ), + ( + related.id.as_str(), + related_key, + "Related requirements", + related_before.document.body.as_str(), + ), + ( + bound_id.as_str(), + bound_key, + "Tracking authority", + bound.document.body.as_str(), + ), + ] { + let shown = surface + .call( + "ShowTicket", + serde_json::json!({"id":reference}), + ToolExecutionContext::direct(), + ) + .await + .unwrap(); + assert_eq!(shown["ticket"], expected_key); + assert_eq!(shown["title"], expected_title); + assert_eq!(shown["body"], expected_body); + if expected_key == bound_key { + assert!( + shown["relations"]["outgoing"] + .as_array() + .unwrap() + .iter() + .any(|relation| relation["ticket"] == related_key + && relation["kind"] == "depends_on") + ); + } + } + let first = surface + .call( + "ShowTicket", + serde_json::json!({"id":related_key,"event_limit":1}), + ToolExecutionContext::direct(), + ) + .await + .unwrap(); + assert_eq!(first["thread"].as_array().unwrap().len(), 1); + assert_eq!(first["thread"][0]["body"], "Latest decision"); + assert_eq!(first["event_page"]["has_more"], true); + let second = surface.call("ShowTicket", serde_json::json!({"id":related_key,"event_limit":1,"event_cursor":first["event_page"]["next_cursor"]}), ToolExecutionContext::direct()).await.unwrap(); + assert_eq!(second["thread"].as_array().unwrap().len(), 1); + assert_eq!(second["thread"][0]["body"], "Earlier decision"); + assert_eq!(backend.show(bound_id.into()).unwrap(), bound); + assert_eq!(backend.show(related.id.into()).unwrap(), related_before); + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn reviewer_read_uses_live_backend_membership_and_cannot_mutate_saved_ticket() { + for wip in [false, true] { + let dir = tempfile::tempdir().unwrap(); + let (api, _runtime, identity, source, ticket) = tracking_fixture(dir.path()).await; + let backend = browser_ticket_backend(&api).unwrap(); + let before = backend.show(ticket.clone().into()).unwrap(); + let transport = ticket_client(&api, identity, &source); + let client = reviewer_client(transport.clone(), &ticket); + let surface = TicketSurface::with_access( + dir.path(), + client.clone(), + wip, + TicketFeatureAccess::review(), + ) + .await; + surface + .call( + "ShowTicket", + serde_json::json!({"id":ticket}), + ToolExecutionContext::direct(), + ) + .await + .unwrap(); + let request_count = transport.requests.lock().unwrap().len(); + for (method, path, body) in [ + ( + WorkspaceRequestMethod::Post, + format!("/api/w/{TEST_WORKSPACE_ID}/tickets/{ticket}/comments"), + serde_json::json!({"body":"unauthorized comment"}), + ), + ( + WorkspaceRequestMethod::Patch, + format!("/api/w/{TEST_WORKSPACE_ID}/tickets/{ticket}"), + serde_json::json!({"title":"unauthorized title"}), + ), + ( + WorkspaceRequestMethod::Post, + format!("/api/w/another-workspace/tickets/{ticket}/show"), + serde_json::json!({}), + ), + ( + WorkspaceRequestMethod::Post, + format!("/api/w/{TEST_WORKSPACE_ID}/merge-requests/other-candidate/reviews"), + serde_json::json!({"verdict":"approved"}), + ), + ] { + let client = client.clone(); + let error = tokio::task::spawn_blocking(move || { + client.execute(WorkspaceRequest::json(method, path, body.to_string())) + }) + .await + .unwrap() + .unwrap_err(); + assert!(error.to_string().contains("read-only"), "{error}"); + } + assert_eq!(transport.requests.lock().unwrap().len(), request_count); + assert_eq!(backend.show(ticket.clone().into()).unwrap(), before); + // Keep a valid Runtime signature but select an unregistered Worker. + // This must fail at the real Backend, not the Reviewer route filter. + *transport.source.lock().unwrap() = + RuntimeWorkerRef::new(source.runtime_id, uuid::Uuid::now_v7().to_string()); + let error = surface + .call( + "ShowTicket", + serde_json::json!({"id":ticket}), + ToolExecutionContext::direct(), + ) + .await + .unwrap_err(); + assert!(error.to_string().contains("401"), "{error}"); + assert!( + transport + .requests + .lock() + .unwrap() + .iter() + .skip(request_count) + .any(|(_, status, _)| *status == 401) + ); + assert_eq!(backend.show(ticket.into()).unwrap(), before); + } +} + fn report_counts(api: &WorkspaceApi) -> (i64, i64) { let conn = rusqlite::Connection::open(&api.config.database_path).unwrap(); (