From 0fd1193b6b9f2728bded8634423fa169ccf8133f Mon Sep 17 00:00:00 2001 From: Hare Date: Wed, 2 Sep 2026 19:53:18 +0900 Subject: [PATCH] fix: harden repository key projections --- crates/protocol/src/subscription.rs | 105 ++++++++++++++- crates/protocol/src/typescript.rs | 16 ++- crates/workspace-server/src/server.rs | 3 +- crates/workspace-server/src/store.rs | 137 +++++++++++++++----- web/workspace/src/lib/generated/protocol.ts | 8 +- 5 files changed, 223 insertions(+), 46 deletions(-) diff --git a/crates/protocol/src/subscription.rs b/crates/protocol/src/subscription.rs index 94ca2604..b24bac90 100644 --- a/crates/protocol/src/subscription.rs +++ b/crates/protocol/src/subscription.rs @@ -170,6 +170,23 @@ fn validate_identifier( Ok(()) } +fn validate_repository_key(value: &str) -> Result<(), SubscriptionValidationError> { + let bytes = value.as_bytes(); + if bytes.is_empty() + || bytes.len() > 64 + || bytes.first() == Some(&b'-') + || bytes.last() == Some(&b'-') + || !bytes + .iter() + .all(|byte| byte.is_ascii_lowercase() || byte.is_ascii_digit() || *byte == b'-') + { + return Err(SubscriptionValidationError::InvalidIdentifier { + field: "repository_key", + }); + } + Ok(()) +} + fn validate_rejection_message(message: &str) -> Result<(), SubscriptionValidationError> { if message.is_empty() { return Err(SubscriptionValidationError::EmptyRejectionMessage); @@ -567,6 +584,7 @@ pub struct SubscriptionWorker { #[serde(default, skip_serializing_if = "Option::is_none")] pub profile: Option, #[serde(default, skip_serializing_if = "Option::is_none")] + #[cfg_attr(feature = "typescript", ts(skip))] pub repository_id: Option, /// Workspace-facing Repository key. Runtime producers leave this unset and /// Workspace Server projections replace `repository_id` with this field. @@ -588,6 +606,14 @@ impl SubscriptionWorker { if let Some(repository_id) = &self.repository_id { validate_identifier("repository_id", repository_id, MAX_RESOURCE_ID_BYTES)?; } + if let Some(repository_key) = &self.repository_key { + validate_repository_key(repository_key)?; + } + if self.repository_id.is_some() && self.repository_key.is_some() { + return Err(SubscriptionValidationError::InvalidIdentifier { + field: "repository_authority", + }); + } if let Some(working_directory_id) = &self.working_directory_id { working_directory_id.validate()?; } @@ -599,7 +625,13 @@ impl SubscriptionWorker { #[cfg_attr(feature = "typescript", derive(ts_rs::TS))] pub struct SubscriptionWorkdir { pub working_directory_id: SubscriptionWorkdirId, - pub repository_id: String, + /// Runtime-internal Repository id. Workspace-facing TypeScript contracts + /// omit this field and require `repository_key` from the Server projection. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[cfg_attr(feature = "typescript", ts(skip))] + pub repository_id: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub repository_key: Option, pub state: String, #[serde(default, skip_serializing_if = "Option::is_none")] pub primary_worker_id: Option, @@ -608,7 +640,41 @@ pub struct SubscriptionWorkdir { impl SubscriptionWorkdir { pub fn validate(&self) -> Result<(), SubscriptionValidationError> { self.working_directory_id.validate()?; - validate_identifier("repository_id", &self.repository_id, MAX_RESOURCE_ID_BYTES)?; + match (&self.repository_id, &self.repository_key) { + (Some(repository_id), None) => { + validate_identifier("repository_id", repository_id, MAX_RESOURCE_ID_BYTES)?; + } + (None, Some(repository_key)) => validate_repository_key(repository_key)?, + _ => { + return Err(SubscriptionValidationError::InvalidIdentifier { + field: "repository_authority", + }); + } + } + validate_identifier("workdir_state", &self.state, MAX_RESOURCE_ID_BYTES)?; + if let Some(worker_id) = &self.primary_worker_id { + worker_id.validate()?; + } + Ok(()) + } +} + +/// Workspace-facing Workdir summary. Backend-generated Repository UUIDs never +/// enter this DTO; Workspace Server must resolve the required Repository key. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[cfg_attr(feature = "typescript", derive(ts_rs::TS))] +pub struct WorkspaceSubscriptionWorkdir { + pub working_directory_id: SubscriptionWorkdirId, + pub repository_key: String, + pub state: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub primary_worker_id: Option, +} + +impl WorkspaceSubscriptionWorkdir { + pub fn validate(&self) -> Result<(), SubscriptionValidationError> { + self.working_directory_id.validate()?; + validate_repository_key(&self.repository_key)?; validate_identifier("workdir_state", &self.state, MAX_RESOURCE_ID_BYTES)?; if let Some(worker_id) = &self.primary_worker_id { worker_id.validate()?; @@ -629,7 +695,7 @@ pub enum SubscriptionSnapshot { events: Vec, }, WorkspaceWorkdirs { - workdirs: Vec, + workdirs: Vec, }, } @@ -697,7 +763,7 @@ pub enum SubscriptionEventPayload { event: WorkerProtocolEvent, }, WorkdirUpserted { - workdir: SubscriptionWorkdir, + workdir: WorkspaceSubscriptionWorkdir, }, WorkdirRemoved { working_directory_id: SubscriptionWorkdirId, @@ -820,6 +886,37 @@ mod tests { } } + #[test] + fn runtime_and_workspace_repository_identity_projections_do_not_alias() { + let mut runtime_worker = worker("worker-1"); + runtime_worker.repository_id = Some("01890f47-3c22-7cc0-98c4-dc0c0c07398f".to_string()); + runtime_worker.validate().unwrap(); + let runtime_json = serde_json::to_value(&runtime_worker).unwrap(); + assert_eq!( + runtime_json["repository_id"], + "01890f47-3c22-7cc0-98c4-dc0c0c07398f" + ); + assert!(runtime_json.get("repository_key").is_none()); + + let mut workspace_worker = worker("worker-1"); + workspace_worker.repository_key = Some("main".to_string()); + workspace_worker.validate().unwrap(); + let workspace_json = serde_json::to_value(&workspace_worker).unwrap(); + assert_eq!(workspace_json["repository_key"], "main"); + assert!(workspace_json.get("repository_id").is_none()); + + let workspace_workdir = WorkspaceSubscriptionWorkdir { + working_directory_id: SubscriptionWorkdirId::new("workdir-1").unwrap(), + repository_key: "main".to_string(), + state: "active".to_string(), + primary_worker_id: Some(worker_id("worker-1")), + }; + workspace_workdir.validate().unwrap(); + let workdir_json = serde_json::to_value(&workspace_workdir).unwrap(); + assert_eq!(workdir_json["repository_key"], "main"); + assert!(workdir_json.get("repository_id").is_none()); + } + #[test] fn subscribe_frame_has_stable_versioned_json_shape() { let frame = SubscriptionFrame::new(SubscriptionFramePayload::Request( diff --git a/crates/protocol/src/typescript.rs b/crates/protocol/src/typescript.rs index 1ec31ca4..4e3e9db7 100644 --- a/crates/protocol/src/typescript.rs +++ b/crates/protocol/src/typescript.rs @@ -16,9 +16,9 @@ use crate::{ EventSubscriptionSelector, SubscriptionEvent, SubscriptionEventPayload, SubscriptionFrame, SubscriptionFramePayload, SubscriptionId, SubscriptionRejectionCode, SubscriptionRequest, SubscriptionRequestId, SubscriptionResponse, SubscriptionSnapshot, - SubscriptionTerminationCode, SubscriptionWorkdir, SubscriptionWorkdirId, - SubscriptionWorker, SubscriptionWorkerId, SubscriptionWorkerIds, - SubscriptionWorkerProtocolMethod, SubscriptionWorkerState, + SubscriptionTerminationCode, SubscriptionWorkdirId, SubscriptionWorker, + SubscriptionWorkerId, SubscriptionWorkerIds, SubscriptionWorkerProtocolMethod, + SubscriptionWorkerState, WorkspaceSubscriptionWorkdir, }, }; @@ -92,7 +92,7 @@ pub fn generated_protocol_types() -> String { push_decl::(&cfg, &mut output); push_decl::(&cfg, &mut output); push_decl::(&cfg, &mut output); - push_decl::(&cfg, &mut output); + push_decl::(&cfg, &mut output); push_decl::(&cfg, &mut output); push_decl::(&cfg, &mut output); push_decl::(&cfg, &mut output); @@ -136,6 +136,14 @@ fn export_decl(decl: &str) -> String { mod tests { use super::*; + #[test] + fn workspace_typescript_omits_runtime_repository_ids() { + let generated = generated_protocol_types(); + assert!(!generated.contains("repository_id?:"), "{generated}"); + assert!(!generated.contains("repository_id:"), "{generated}"); + assert!(generated.contains("repository_key"), "{generated}"); + } + #[test] fn generated_protocol_types_are_current() { let expected = generated_protocol_types(); diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 54f52bc9..55dddf93 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -2054,8 +2054,7 @@ impl WorkspaceApi { })?; if selected_repository_id.as_deref() != Some(repository_id) { return Err(ApiError::from(Error::Config(format!( - "Ticket `{ticket_id}` targets repository `{repository_key}`, but the Worker launch resolves `{}`", - selected_repository_id.as_deref().unwrap_or("none") + "Ticket `{ticket_id}` targets Repository `{repository_key}`, but the Worker launch resolves a different Repository" )))); } if selected_ref_selector.as_deref() != Some(ref_selector) { diff --git a/crates/workspace-server/src/store.rs b/crates/workspace-server/src/store.rs index 0f4bc29e..29397463 100644 --- a/crates/workspace-server/src/store.rs +++ b/crates/workspace-server/src/store.rs @@ -1903,6 +1903,7 @@ impl ControlPlaneStore for SqliteWorkspaceStore { &self, record: &WorkspaceBootstrapRecord, ) -> Result { + validate_repository_record_identity(&record.repository)?; self.with_conn_mut(|conn| { let tx = conn.transaction_with_behavior(TransactionBehavior::Immediate)?; let owner_kind = tx @@ -1959,43 +1960,18 @@ impl ControlPlaneStore for SqliteWorkspaceStore { }); } - if let Some(existing) = tx + if tx .query_row( - r#"SELECT workspace_id, owner_account_id, display_name, state, created_at, updated_at - FROM workspaces WHERE workspace_id = ?1"#, + "SELECT 1 FROM workspaces WHERE workspace_id = ?1", params![record.workspace.workspace_id], - read_workspace_record, + |row| row.get::<_, i64>(0), ) .optional()? + .is_some() { - if existing.owner_account_id != record.workspace.owner_account_id - || existing.display_name != record.workspace.display_name - || existing.state != record.workspace.state - { - return Err(Error::WorkspaceConfigConflict( - "Workspace identity already exists with different metadata".to_string(), - )); - } - let existing_repository = tx - .query_row( - r#"SELECT workspace_id, repository_id, repository_key, kind, provider, - source_kind, source_uri, default_ref, source_revision, - source_fingerprint, observed_status, observed_at, created_at, updated_at - FROM repositories WHERE workspace_id = ?1 AND repository_key = ?2"#, - params![record.repository.workspace_id, record.repository.repository_key], - read_repository_record, - ) - .optional()?; - if existing_repository.as_ref().is_none_or(|existing| { - let mut requested = record.repository.clone(); - requested.repository_id.clone_from(&existing.repository_id); - existing != &requested - }) { - return Err(Error::WorkspaceConfigConflict( - "Workspace initial repository already exists with different metadata" - .to_string(), - )); - } + return Err(Error::WorkspaceConfigConflict( + "Workspace identity already exists".to_string(), + )); } else { tx.execute( r#"INSERT INTO workspaces ( @@ -13786,6 +13762,103 @@ CREATE TABLE ticket_assignment_operations ( ); } + #[tokio::test] + async fn workspace_bootstrap_validates_key_and_rejects_duplicate_workspace() { + let store = SqliteWorkspaceStore::in_memory().unwrap(); + store + .upsert_account(&AccountRecord { + account_id: "owner-account".to_string(), + kind: "user".to_string(), + handle: "owner".to_string(), + display_name: "Owner".to_string(), + created_at: "1".to_string(), + updated_at: "1".to_string(), + }) + .unwrap(); + let workspace = WorkspaceRecord { + workspace_id: "workspace-invalid-key".to_string(), + owner_account_id: "owner-account".to_string(), + display_name: "Invalid key".to_string(), + state: "active".to_string(), + created_at: "1".to_string(), + updated_at: "1".to_string(), + }; + let repository = RepositoryRecord { + workspace_id: workspace.workspace_id.clone(), + repository_id: Uuid::now_v7().to_string(), + repository_key: "Invalid_Key".to_string(), + kind: "git".to_string(), + provider: Some("git".to_string()), + source: workspace_api::RepositorySource { + kind: workspace_api::RepositorySourceKind::LocalPath, + uri: "/repo".to_string(), + }, + default_ref: Some("develop".to_string()), + source_revision: 1, + source_fingerprint: "sha256:test".to_string(), + observed_status: RepositoryObservedStatus::Unverified, + observed_at: None, + created_at: "1".to_string(), + updated_at: "1".to_string(), + }; + + let error = store + .create_workspace_bootstrap(&WorkspaceBootstrapRecord { + operation_key: "invalid-key".to_string(), + request_fingerprint: "sha256:invalid-key".to_string(), + workspace: workspace.clone(), + repository, + }) + .unwrap_err() + .to_string(); + + assert!(error.contains("invalid Repository key"), "{error}"); + assert!( + store + .get_workspace(&workspace.workspace_id) + .await + .unwrap() + .is_none() + ); + + let valid_repository = RepositoryRecord { + workspace_id: workspace.workspace_id.clone(), + repository_id: Uuid::now_v7().to_string(), + repository_key: "main".to_string(), + kind: "git".to_string(), + provider: Some("git".to_string()), + source: workspace_api::RepositorySource { + kind: workspace_api::RepositorySourceKind::LocalPath, + uri: "/repo".to_string(), + }, + default_ref: Some("develop".to_string()), + source_revision: 1, + source_fingerprint: "sha256:test".to_string(), + observed_status: RepositoryObservedStatus::Unverified, + observed_at: None, + created_at: "1".to_string(), + updated_at: "1".to_string(), + }; + let first = WorkspaceBootstrapRecord { + operation_key: "create-workspace".to_string(), + request_fingerprint: "sha256:create-workspace".to_string(), + workspace, + repository: valid_repository, + }; + assert!(!store.create_workspace_bootstrap(&first).unwrap().replayed); + let mut duplicate = first; + duplicate.operation_key = "duplicate-workspace".to_string(); + duplicate.repository.repository_id = Uuid::now_v7().to_string(); + let error = store + .create_workspace_bootstrap(&duplicate) + .unwrap_err() + .to_string(); + assert!( + error.contains("Workspace identity already exists"), + "{error}" + ); + } + #[tokio::test] async fn repository_records_round_trip() { let store = SqliteWorkspaceStore::in_memory().unwrap(); diff --git a/web/workspace/src/lib/generated/protocol.ts b/web/workspace/src/lib/generated/protocol.ts index 1d01dcba..8100c319 100644 --- a/web/workspace/src/lib/generated/protocol.ts +++ b/web/workspace/src/lib/generated/protocol.ts @@ -192,18 +192,18 @@ resource_key?: string | null, /** * Producer-owned monotonic revision for this Worker subject. */ -subject_revision: number, state: SubscriptionWorkerState, has_running_internal_workers: boolean, workspace_id?: string | null, display_name?: string | null, profile?: string | null, repository_id?: string | null, +subject_revision: number, state: SubscriptionWorkerState, has_running_internal_workers: boolean, workspace_id?: string | null, display_name?: string | null, profile?: string | null, /** * Workspace-facing Repository key. Runtime producers leave this unset and * Workspace Server projections replace `repository_id` with this field. */ repository_key?: string | null, working_directory_id?: SubscriptionWorkdirId | null, }; -export type SubscriptionWorkdir = { working_directory_id: SubscriptionWorkdirId, repository_id: string, state: string, primary_worker_id?: SubscriptionWorkerId | null, }; +export type WorkspaceSubscriptionWorkdir = { working_directory_id: SubscriptionWorkdirId, repository_key: string, state: string, primary_worker_id?: SubscriptionWorkerId | null, }; -export type SubscriptionSnapshot = { "topic": "workers", "data": { workers: Array, } } | { "topic": "worker_protocol", "data": { worker_id: SubscriptionWorkerId, events: Array, } } | { "topic": "workspace_workdirs", "data": { workdirs: Array, } }; +export type SubscriptionSnapshot = { "topic": "workers", "data": { workers: Array, } } | { "topic": "worker_protocol", "data": { worker_id: SubscriptionWorkerId, events: Array, } } | { "topic": "workspace_workdirs", "data": { workdirs: Array, } }; -export type SubscriptionEventPayload = { "event": "worker_upserted", "data": { worker: SubscriptionWorker, } } | { "event": "worker_removed", "data": { worker_id: SubscriptionWorkerId, runtime_id?: string | null, } } | { "event": "worker_protocol", "data": { worker_id: SubscriptionWorkerId, event: Event, } } | { "event": "workdir_upserted", "data": { workdir: SubscriptionWorkdir, } } | { "event": "workdir_removed", "data": { working_directory_id: SubscriptionWorkdirId, } }; +export type SubscriptionEventPayload = { "event": "worker_upserted", "data": { worker: SubscriptionWorker, } } | { "event": "worker_removed", "data": { worker_id: SubscriptionWorkerId, runtime_id?: string | null, } } | { "event": "worker_protocol", "data": { worker_id: SubscriptionWorkerId, event: Event, } } | { "event": "workdir_upserted", "data": { workdir: WorkspaceSubscriptionWorkdir, } } | { "event": "workdir_removed", "data": { working_directory_id: SubscriptionWorkdirId, } }; export type SubscriptionRejectionCode = "invalid_request" | "unsupported_protocol_version" | "unsupported_selector" | "unauthorized" | "resource_not_found" | "capacity_exceeded" | "internal";