settings_json: Locate a key by its own range, not by scanning quotes (#62949)

`replace_value_in_json_text` spelled a newly inserted key with
hand-written quotes, so a key carrying a `"` or a `\` produced invalid
JSON. The removal and rename paths then could not find such a key again:
both walked backwards to the nearest raw `"`, which lands inside an
escape, and ate the neighbouring entry while leaving JSON that still
parses.

The parser already hands back the key's byte range, so the three sites
use it, and serde for the spelling, the way key lookup and the
nested-construction path always did.

Worth knowing beyond the escaping: `update_value_in_json_text` pushes
every map key into the key path verbatim, so any settings map whose keys
are not authored by hand reaches this. Most are typed by the user, but
`favorite_config_option_values` is keyed by a `SessionConfigId` that an
ACP agent supplies over the protocol.

Tests cover insertion into a single-line and a multi-line object,
removal and rename, all with a key carrying a quote and a backslash.

Release Notes:

- Fixed a settings key containing a quote or a backslash corrupting
`settings.json`.
This commit is contained in:
Vlad Gaevsky authored and GitHub committed 2026-09-04 15:28:40 +00:00
1 parent 1aab982b0a
commit d081bbb7e1
1 file changed
+50 -17
+50 -17
View File
@@ -99,6 +99,7 @@ pub fn replace_value_in_json_text<T: AsRef<str>>(
let mut depth = 0;
let mut last_value_range = 0..0;
let mut first_key_start = None;
let mut matched_key_start = None;
let mut existing_value_range = 0..text.len();
let mut matches = cursor.matches(&PAIR_QUERY, syntax_tree.root_node(), text.as_bytes());
@@ -135,6 +136,7 @@ pub fn replace_value_in_json_text<T: AsRef<str>>(
.unwrap_or(false);
if found_key {
matched_key_start = Some(key_range.start);
existing_value_range = value_range;
// Reset last value range when increasing in depth
last_value_range = existing_value_range.start..existing_value_range.start;
@@ -166,12 +168,8 @@ pub fn replace_value_in_json_text<T: AsRef<str>>(
let new_val = to_pretty_json(new_value, tab_size, tab_size * depth);
if let Some(replace_key) = replace_key.and_then(|str| serde_json::to_string(str).ok()) {
let new_key = format!("{}: ", replace_key);
if let Some(key_start) = text[..existing_value_range.start].rfind('"') {
if let Some(prev_key_start) = text[..key_start].rfind('"') {
existing_value_range.start = prev_key_start;
} else {
existing_value_range.start = key_start;
}
if let Some(key_start) = matched_key_start {
existing_value_range.start = key_start;
}
(existing_value_range, new_key + &new_val)
} else {
@@ -181,14 +179,8 @@ pub fn replace_value_in_json_text<T: AsRef<str>>(
let mut removal_start = first_key_start.unwrap_or(existing_value_range.start);
let mut removal_end = existing_value_range.end;
// Find the actual key position by looking for the key in the pair
// We need to extend the range to include the key, not just the value
if let Some(key_start) = text[..existing_value_range.start].rfind('"') {
if let Some(prev_key_start) = text[..key_start].rfind('"') {
removal_start = prev_key_start;
} else {
removal_start = key_start;
}
if let Some(key_start) = matched_key_start {
removal_start = key_start;
}
let mut removed_comma = false;
@@ -228,7 +220,7 @@ pub fn replace_value_in_json_text<T: AsRef<str>>(
} else {
if let Some(first_key_start) = first_key_start {
// We have key paths, construct the sub objects
let new_key = key_path[depth].as_ref();
let new_key = serde_json::to_string(key_path[depth].as_ref()).unwrap();
// We don't have the key, construct the nested objects
let new_value = construct_json_value(&key_path[(depth + 1)..], new_value);
@@ -250,11 +242,11 @@ pub fn replace_value_in_json_text<T: AsRef<str>>(
// depth is 0 based, but division needs to be 1 based.
let new_val = to_pretty_json(&new_value, column / (depth + 1), column);
let space = ' ';
let content = format!("\"{new_key}\": {new_val},\n{space:width$}", width = column);
let content = format!("{new_key}: {new_val},\n{space:width$}", width = column);
(first_key_start..first_key_start, content)
} else {
let new_val = serde_json::to_string(&new_value).unwrap();
let mut content = format!(r#""{new_key}": {new_val},"#);
let mut content = format!("{new_key}: {new_val},");
content.push(' ');
(first_key_start..first_key_start, content)
}
@@ -2588,6 +2580,47 @@ mod tests {
)
}
#[test]
fn object_replace_escapes_new_key() {
// An object that already has a key: an empty one takes the nested-construction path.
let single_line = r#"{"theme": "One Dark"}"#;
let multi_line = "{\n \"theme\": \"One Dark\"\n}";
let key = r#"/home/me/say "hi" C:\x"#;
for input in [single_line, multi_line] {
let mut text = input.to_string();
let (range, replacement) =
replace_value_in_json_text(&text, &[key], 4, Some(&json!("One Light")), None);
text.replace_range(range, &replacement);
let parsed: Value = serde_json::from_str(&text)
.expect("a folder name carrying a quote must not break settings.json");
pretty_assertions::assert_eq!(parsed, json!({ "theme": "One Dark", key: "One Light" }));
}
}
#[test]
fn object_remove_and_rename_find_an_escaped_key_by_its_own_range() {
let key = "say \"hi\"";
let input = format!(
"{{{}: \"V\", \"theme\": \"One Dark\"}}",
serde_json::to_string(key).unwrap()
);
let mut removed = input.clone();
let (range, replacement) = replace_value_in_json_text(&removed, &[key], 4, None, None);
removed.replace_range(range, &replacement);
let parsed: Value = serde_json::from_str(&removed).expect("removal must leave valid JSON");
pretty_assertions::assert_eq!(parsed, json!({ "theme": "One Dark" }));
let mut renamed = input;
let (range, replacement) =
replace_value_in_json_text(&renamed, &[key], 4, Some(&json!("V2")), Some("plain"));
renamed.replace_range(range, &replacement);
let parsed: Value = serde_json::from_str(&renamed).expect("rename must leave valid JSON");
pretty_assertions::assert_eq!(parsed, json!({ "plain": "V2", "theme": "One Dark" }));
}
#[test]
fn test_infer_json_indent_size() {
let json_2_spaces = r#"{