diff --git a/packages/core/src/behaviors/mod.rs b/packages/core/src/behaviors/mod.rs index 22e93c1bb..853e764b3 100644 --- a/packages/core/src/behaviors/mod.rs +++ b/packages/core/src/behaviors/mod.rs @@ -1275,7 +1275,7 @@ pub struct SchemaNodeBehavior; /// here would reject names the rest of the system handles correctly: CEL strips /// the prefix, the graph resolver matches with and without it, and identifier /// validation in the query path permits it. -fn validate_schema_field_name(name: &str) -> Result<(), NodeValidationError> { +pub(crate) fn validate_schema_field_name(name: &str) -> Result<(), NodeValidationError> { let invalid = |reason: &str| { Err(NodeValidationError::InvalidProperties(format!( "Invalid field name '{}': {}", diff --git a/packages/core/src/schema/mod.rs b/packages/core/src/schema/mod.rs index 68e73fb89..10b8927f1 100644 --- a/packages/core/src/schema/mod.rs +++ b/packages/core/src/schema/mod.rs @@ -799,8 +799,10 @@ pub async fn handle_update_schema( // --- Phase 0: Verify schema exists, validate renames, run playbook impact check --- // Schema existence is verified upfront so rename/playbook validation errors are reported - // before any mutations execute. - node_service + // before any mutations execute. The fetched schema is also the pre-mutation snapshot the + // protection-level and grammar checks below validate `remove_fields`/`rename_fields` + // against, before Phase 1 or Phase 2 touch anything. + let schema_before = node_service .get_schema_node(¶ms.schema_id) .await .map_err(|e| MarkdownError::internal_error(format!("Failed to get schema: {}", e)))? @@ -845,6 +847,67 @@ pub async fn handle_update_schema( } } + // --- Phase 0: Reject mutations that touch a Core/System-protected field --- + // + // `SchemaField` carries a protection level, and `SchemaNode::can_delete_field` + // / `can_modify_field` exist to enforce it — a Core/System field (e.g. + // `task.status`) must never be deletable or renameable by a caller. Neither + // was ever consulted on this path: `remove_fields` below deletes by name + // alone, and `rename_schema_field` rekeys by name alone, so either could + // permanently strip a protected field from a core schema with no recovery + // path through the API. + // + // Checked here, against `schema_before` (the schema as it stood before this + // call touched anything), and before Phase 1 executes a single rename: a + // rename migrates every existing node's property data and rewrites the + // schema as it goes, so validating the whole batch upfront — rather than + // per-entry as Phase 1 runs — keeps a batch with one bad entry from + // partially applying before the rejection is returned. + if let Some(ref remove_names) = params.remove_fields { + for name in remove_names { + if let Some(field) = schema_before.get_field(name) { + if !schema_before.can_delete_field(name) { + return Err(MarkdownError::invalid_params(format!( + "Field '{}' on schema '{}' is {}-protected and cannot be removed — \ + only User-protected fields may be removed via update_schema. Core \ + and System fields are immutable through this API.", + name, params.schema_id, field.protection + ))); + } + } + } + } + + if let Some(ref renames) = params.rename_fields { + for rename in renames { + if let Some(field) = schema_before.get_field(&rename.from) { + if !schema_before.can_modify_field(&rename.from) { + return Err(MarkdownError::invalid_params(format!( + "Field '{}' on schema '{}' is {}-protected and cannot be renamed or \ + relabeled — only User-protected fields may be modified via \ + update_schema. Core and System fields are immutable through this API.", + rename.from, params.schema_id, field.protection + ))); + } + } + + // The destination name's grammar must be rejected here too, before + // Phase 1 runs — not left to Phase 2's re-validation. An identity + // rename is persisted by `rename_schema_field` immediately, + // migrating every node's property data as it executes; Phase 2's + // check runs only after that migration has already committed, so a + // malformed name (spaces, punctuation, more than one namespace + // prefix) would already be durably written to the schema and every + // node instance by the time it was caught. + crate::behaviors::validate_schema_field_name(&rename.to).map_err(|e| { + MarkdownError::invalid_params(format!( + "rename_fields: invalid destination name '{}': {}", + rename.to, e + )) + })?; + } + } + // Check if any active playbooks would be affected by this schema change. // Done before any mutations so a blocked rename doesn't partially execute. let affected = diff --git a/packages/core/src/schema/schema_test.rs b/packages/core/src/schema/schema_test.rs index efad83deb..e8a7844b0 100644 --- a/packages/core/src/schema/schema_test.rs +++ b/packages/core/src/schema/schema_test.rs @@ -588,6 +588,374 @@ async fn test_rename_field_migrates_node_data() { ); } +// ============================================================================ +// Protection-level enforcement: Core/System fields must survive remove_fields +// and rename_fields, and a malformed rename destination must never persist +// ============================================================================ + +#[tokio::test] +async fn test_update_schema_remove_fields_rejects_core_protected_field() { + let (svc, _tmp) = create_test_service().await; + + let result = handle_update_schema( + &svc, + json!({ + "schema_id": "task", + "remove_fields": ["status"] + }), + ) + .await; + + let err = result + .expect_err("removing a Core-protected field must be rejected") + .to_string(); + assert!( + err.contains("status") && err.contains("core"), + "error should name the field and its protection level: {err}" + ); + + let schema = svc + .get_schema_node("task") + .await + .expect("get_schema_node should succeed") + .expect("core task schema should exist"); + assert!( + schema.fields.iter().any(|f| f.name == "status"), + "status must survive a rejected removal: {:?}", + schema.fields.iter().map(|f| &f.name).collect::>() + ); +} + +#[tokio::test] +async fn test_update_schema_remove_fields_rejects_system_protected_field() { + let (svc, _tmp) = create_test_service().await; + + let result = handle_update_schema( + &svc, + json!({ + "schema_id": "person", + "remove_fields": ["_possible_duplicate"] + }), + ) + .await; + + let err = result + .expect_err("removing a System-protected field must be rejected") + .to_string(); + assert!( + err.contains("_possible_duplicate") && err.contains("system"), + "error should name the field and its protection level: {err}" + ); + + let schema = svc + .get_schema_node("person") + .await + .expect("get_schema_node should succeed") + .expect("core person schema should exist"); + assert!( + schema + .fields + .iter() + .any(|f| f.name == "_possible_duplicate"), + "System-protected field must survive a rejected removal" + ); +} + +/// The positive-path counterpart to the two tests above: the guard is +/// field-level, not schema-level — a User-protected field must remain +/// removable even on a schema NodeSpace ships. +#[tokio::test] +async fn test_update_schema_remove_fields_allows_user_protected_field_on_core_schema() { + let (svc, _tmp) = create_test_service().await; + + let result = handle_update_schema( + &svc, + json!({ + "schema_id": "task", + "remove_fields": ["priority"] + }), + ) + .await; + + assert!( + result.is_ok(), + "removing a User-protected field, even on a core schema, must still succeed: {:?}", + result + ); + + let schema = svc + .get_schema_node("task") + .await + .expect("get_schema_node should succeed") + .expect("core task schema should exist"); + assert!( + !schema.fields.iter().any(|f| f.name == "priority"), + "priority should have been removed" + ); +} + +/// A batch mixing an allowed removal with a forbidden one must reject the +/// whole call rather than applying the allowed half before hitting the +/// protected one — the same all-or-nothing guarantee `update_schema` already +/// gives the namespace-prefix and playbook-impact checks. +#[tokio::test] +async fn test_update_schema_remove_fields_rejects_whole_batch_when_one_field_is_protected() { + let (svc, _tmp) = create_test_service().await; + + let result = handle_update_schema( + &svc, + json!({ + "schema_id": "task", + "remove_fields": ["priority", "status"] + }), + ) + .await; + + assert!( + result.is_err(), + "a batch containing a protected field must be rejected entirely" + ); + + let schema = svc + .get_schema_node("task") + .await + .expect("get_schema_node should succeed") + .expect("core task schema should exist"); + let field_names: Vec<&str> = schema.fields.iter().map(|f| f.name.as_str()).collect(); + assert!( + field_names.contains(&"priority"), + "priority must survive — the batch must not partially apply: {field_names:?}" + ); + assert!( + field_names.contains(&"status"), + "status must survive: {field_names:?}" + ); +} + +#[tokio::test] +async fn test_update_schema_rename_fields_rejects_core_protected_field_and_preserves_node_data() { + use crate::services::CreateNodeParams; + + let (svc, _tmp) = create_test_service().await; + + // A real task instance with a real status value — a bypassed check would + // migrate this exact value to the rejected destination key. + let node_params = CreateNodeParams { + id: None, + node_type: "task".to_string(), + content: "test task".to_string(), + parent_id: None, + position: crate::services::InsertPositionOwned::End, + properties: serde_json::json!({ + "task": { "status": "open" } + }), + }; + let node_id = svc + .create_node_with_parent(node_params) + .await + .expect("create_node_with_parent failed"); + + let result = handle_update_schema( + &svc, + json!({ + "schema_id": "task", + "rename_fields": [{ "from": "status", "to": "custom:status" }] + }), + ) + .await; + + let err = result + .expect_err("renaming a Core-protected field must be rejected") + .to_string(); + assert!( + err.contains("status") && err.contains("core"), + "error should name the field and its protection level: {err}" + ); + + // Schema definition unchanged. + let schema = svc + .get_schema_node("task") + .await + .expect("get_schema_node should succeed") + .expect("core task schema should exist"); + assert!( + schema.fields.iter().any(|f| f.name == "status"), + "status must survive a rejected rename" + ); + assert!( + !schema.fields.iter().any(|f| f.name == "custom:status"), + "the rejected destination name must not appear in the schema" + ); + + // Node data untouched — the migration must never have run. + let node = svc + .get_node(&node_id) + .await + .expect("get_node failed") + .expect("node not found"); + let task_props = node + .properties + .get("task") + .expect("task-namespaced properties should exist"); + assert_eq!( + task_props.get("status").and_then(|v| v.as_str()), + Some("open"), + "status value must be untouched by a rejected rename" + ); + assert!( + task_props.get("custom:status").is_none(), + "no data should have been migrated to the rejected destination key" + ); +} + +/// A display-only relabel (`from == to` with `friendlyName` set) is still a +/// modification of a protected field's definition and must be rejected the +/// same way an identity rename is. +#[tokio::test] +async fn test_update_schema_rename_fields_rejects_friendly_name_only_relabel_of_core_field() { + let (svc, _tmp) = create_test_service().await; + + let result = handle_update_schema( + &svc, + json!({ + "schema_id": "task", + "rename_fields": [{ "from": "status", "to": "status", "friendlyName": "State" }] + }), + ) + .await; + + let err = result + .expect_err("relabeling a Core-protected field must be rejected") + .to_string(); + assert!( + err.contains("status") && err.contains("core"), + "error should name the field and its protection level: {err}" + ); + + let schema = svc + .get_schema_node("task") + .await + .expect("get_schema_node should succeed") + .expect("core task schema should exist"); + let status_field = schema + .fields + .iter() + .find(|f| f.name == "status") + .expect("status field should still exist"); + assert_eq!( + status_field.friendly_name, "Status", + "friendly_name must be unchanged by a rejected relabel" + ); +} + +/// A malformed destination name must be rejected before the rename runs, not +/// after — `rename_schema_field` persists the schema rewrite and migrates +/// every node's property data immediately, so validating only the final +/// field list (as Phase 2 already did) catches the problem after the damage +/// is already committed. +#[tokio::test] +async fn test_update_schema_rename_fields_validates_destination_grammar_before_persisting() { + use crate::services::CreateNodeParams; + + let (svc, _tmp) = create_test_service().await; + + handle_create_schema( + &svc, + json!({ + "name": "Venue", + "fields": [ + { "name": "capacity", "type": "number", "protection": "user", "indexed": false } + ] + }), + ) + .await + .expect("create_schema should succeed"); + + // A real venue instance, so a bypassed check would be visible in node + // data too, not just the schema definition. + let node_params = CreateNodeParams { + id: None, + node_type: "venue".to_string(), + content: "test venue".to_string(), + parent_id: None, + position: crate::services::InsertPositionOwned::End, + properties: serde_json::json!({ "venue": { "capacity": 100 } }), + }; + let node_id = svc + .create_node_with_parent(node_params) + .await + .expect("create_node_with_parent failed"); + + // A space is not valid field-name grammar — this is the exact kind of + // malformed destination a caller can send. + let result = handle_update_schema( + &svc, + json!({ + "schema_id": "venue", + "rename_fields": [{ "from": "capacity", "to": "max capacity" }] + }), + ) + .await; + + let err = result + .expect_err("a malformed destination name must be rejected") + .to_string(); + assert!( + err.contains("max capacity"), + "error should name the bad destination: {err}" + ); + + // The schema must not have been rewritten with the bad name. + let schema = svc + .get_schema_node("venue") + .await + .expect("get_schema_node should succeed") + .expect("venue schema should exist"); + let field_names: Vec<&str> = schema.fields.iter().map(|f| f.name.as_str()).collect(); + assert!( + field_names.contains(&"capacity"), + "capacity must survive a rejected rename: {field_names:?}" + ); + assert!( + !field_names.iter().any(|n| n.contains("max capacity")), + "the malformed destination name must never reach the schema: {field_names:?}" + ); + + // Node data must not have been migrated to the bad key either. + let node = svc + .get_node(&node_id) + .await + .expect("get_node failed") + .expect("node not found"); + let props = node + .properties + .get("venue") + .expect("venue-namespaced properties should exist"); + assert_eq!( + props.get("capacity").and_then(|v| v.as_i64()), + Some(100), + "capacity value must be untouched by a rejected rename" + ); + assert!( + props.get("max capacity").is_none(), + "no data should have been migrated to the rejected destination key" + ); + + // A subsequent, valid update on the same schema must still succeed — + // proving the schema was never left wedged on the rejected name. + handle_update_schema( + &svc, + json!({ + "schema_id": "venue", + "add_fields": [ + { "name": "address", "type": "string", "protection": "user", "indexed": false } + ] + }), + ) + .await + .expect("a schema untouched by the rejected rename should accept a further update"); +} + #[tokio::test] async fn test_rename_field_friendly_name_only_updates_label_without_migrating_data() { use crate::services::CreateNodeParams; diff --git a/packages/core/src/services/node_service/mod.rs b/packages/core/src/services/node_service/mod.rs index 13975afb5..a30afc13a 100644 --- a/packages/core/src/services/node_service/mod.rs +++ b/packages/core/src/services/node_service/mod.rs @@ -2089,6 +2089,84 @@ mod tests { (service, temp_dir) } + // ======================================================================== + // Protection-level guard on the two schema-field mutation primitives. + // + // `handle_update_schema` (packages/core/src/schema/mod.rs) is the only + // production caller of either method below, and it rejects a protected + // field before reaching them — but the guard belongs here too, at the + // primitive itself, so a future second caller can't reintroduce the + // bypass by skipping that upstream check. + // ======================================================================== + + #[tokio::test] + async fn rename_schema_field_rejects_core_protected_field() { + let (svc, _tmp) = create_test_service().await; + + let result = svc + .rename_schema_field("task", "status", "custom:status") + .await; + + let err = result + .expect_err("renaming a Core-protected field must be rejected") + .to_string(); + assert!( + err.contains("status") && err.contains("core"), + "error should name the field and its protection level: {err}" + ); + + let schema = svc + .get_schema_node("task") + .await + .unwrap() + .expect("core task schema should exist"); + assert!( + schema.fields.iter().any(|f| f.name == "status"), + "status must survive a rejected rename" + ); + } + + #[tokio::test] + async fn rename_schema_field_allows_user_protected_field_on_core_schema() { + let (svc, _tmp) = create_test_service().await; + + // "priority" is User-protected on the core "task" schema — the guard + // is field-level, not schema-level, so this must still succeed. + let result = svc.rename_schema_field("task", "priority", "urgency").await; + assert!( + result.is_ok(), + "renaming a User-protected field must still succeed: {result:?}" + ); + + let schema = svc.get_schema_node("task").await.unwrap().unwrap(); + assert!(schema.fields.iter().any(|f| f.name == "urgency")); + assert!(!schema.fields.iter().any(|f| f.name == "priority")); + } + + #[tokio::test] + async fn update_schema_field_friendly_name_rejects_core_protected_field() { + let (svc, _tmp) = create_test_service().await; + + let result = svc + .update_schema_field_friendly_name("task", "status", "State") + .await; + + let err = result + .expect_err("relabeling a Core-protected field must be rejected") + .to_string(); + assert!( + err.contains("status") && err.contains("core"), + "error should name the field and its protection level: {err}" + ); + + let schema = svc.get_schema_node("task").await.unwrap().unwrap(); + let status_field = schema.fields.iter().find(|f| f.name == "status").unwrap(); + assert_eq!( + status_field.friendly_name, "Status", + "friendly_name must be unchanged by a rejected relabel" + ); + } + /// ADR-059 §6/§7: embedding aggregation never spans an access boundary — the /// boundary is made UNREACHABLE BY CONSTRUCTION (the root-only `member_of` /// constraint of §2 + the embeddable-type list), NOT filtered at aggregation diff --git a/packages/core/src/services/node_service/schema.rs b/packages/core/src/services/node_service/schema.rs index 529db9f02..f383fd8d8 100644 --- a/packages/core/src/services/node_service/schema.rs +++ b/packages/core/src/services/node_service/schema.rs @@ -561,6 +561,25 @@ impl NodeService { ))); } + // Reject renaming a Core/System-protected field. `can_modify_field` + // exists exactly for this — only a `User`-protected field's storage + // key may change. Checked before the migration below runs: renaming + // rekeys every existing node's property data and rewrites the schema + // as it goes, so a check that ran afterwards would find the damage + // already durably applied. + if !schema.can_modify_field(from) { + let protection = schema + .get_field(from) + .map(|f| f.protection.clone()) + .unwrap_or_default(); + return Err(NodeServiceError::invalid_update(format!( + "Field '{}' in schema '{}' is {}-protected and cannot be renamed — only \ + User-protected fields may be renamed. Core and System fields are immutable \ + through update_schema.", + from, type_id, protection + ))); + } + // Validate destination field does not already exist if schema.fields.iter().any(|f| f.name == to) { return Err(NodeServiceError::invalid_update(format!( @@ -653,6 +672,23 @@ impl NodeService { ))); } + // Same protection-level guard as `rename_schema_field`'s identity + // rename: a Core/System field is immutable through `update_schema`, + // and a relabel is a modification of the field definition just as + // much as a storage-key rename is. + if !schema.can_modify_field(field_name) { + let protection = schema + .get_field(field_name) + .map(|f| f.protection.clone()) + .unwrap_or_default(); + return Err(NodeServiceError::invalid_update(format!( + "Field '{}' in schema '{}' is {}-protected and cannot be relabeled — only \ + User-protected fields may be modified. Core and System fields are immutable \ + through update_schema.", + field_name, type_id, protection + ))); + } + // `SchemaField::friendly_name` is documented as always populated in // storage, with every reader assuming so unconditionally — a blank // value must never reach it here any more than it can through diff --git a/packages/nodespace-types/src/schema.rs b/packages/nodespace-types/src/schema.rs index d422dfb16..e0e77b61e 100644 --- a/packages/nodespace-types/src/schema.rs +++ b/packages/nodespace-types/src/schema.rs @@ -101,6 +101,20 @@ pub enum SchemaProtectionLevel { System, } +impl std::fmt::Display for SchemaProtectionLevel { + /// Mirrors the wire form (`#[serde(rename_all = "lowercase")]`) so error + /// messages naming a protection level match what the same value + /// serializes to over the API. + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let s = match self { + SchemaProtectionLevel::Core => "core", + SchemaProtectionLevel::User => "user", + SchemaProtectionLevel::System => "system", + }; + write!(f, "{s}") + } +} + #[derive(Debug, Clone, Default, Serialize, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct SchemaField {