mirror of
https://github.com/storytold/craft-libs.git
synced 2026-11-11 04:52:13 +00:00
security: validate settings sections against the defaults alone
deserialize_sections tried each top-level key against a document holding every key accepted before it, cloning and deserializing the growing document per key. A config.toml with many unknown keys took quadratic time (20k keys: >2 minutes, now <1 s). Each key is now checked against the defaults plus that key, and accepted keys are merged once. CodeRabbit CWE-407, crates/settings/src/document.rs:47
This commit is contained in:
2 files changed
+25
-4
No files matched your search
@@ -36,18 +36,22 @@ pub fn new_document(version: u32) -> DocumentMut {
|
||||
/// is kept. This is what keeps a renamed enum variant from resetting the whole file.
|
||||
pub fn deserialize_sections<T: Settings>(doc: &DocumentMut) -> (T, Vec<SectionError>) {
|
||||
let defaults = T::default();
|
||||
let Ok(mut merged) = toml_edit::ser::to_document(&defaults) else {
|
||||
let Ok(base) = toml_edit::ser::to_document(&defaults) else {
|
||||
return (defaults, Vec::new());
|
||||
};
|
||||
let mut merged = base.clone();
|
||||
let mut errors = Vec::new();
|
||||
for (key, item) in doc.iter() {
|
||||
if key == SCHEMA_VERSION_KEY {
|
||||
continue;
|
||||
}
|
||||
let mut candidate = merged.clone();
|
||||
// Each section is tried against the defaults alone, never against the sections accepted
|
||||
// before it, so a file of many keys costs the defaults plus that key per key instead of
|
||||
// the whole document so far.
|
||||
let mut candidate = base.clone();
|
||||
candidate[key] = item.clone();
|
||||
match toml_edit::de::from_document::<T>(candidate.clone()) {
|
||||
Ok(_) => merged = candidate,
|
||||
match toml_edit::de::from_document::<T>(candidate) {
|
||||
Ok(_) => merged[key] = item.clone(),
|
||||
Err(e) => errors.push(SectionError { section: key.to_owned(), message: e.to_string() }),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -47,6 +47,23 @@ fn bad_value_only_resets_its_section() {
|
||||
assert!(config_path(tmp.path()).exists(), "the file is not moved aside");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn many_unknown_keys_load_in_linear_time() {
|
||||
// Each key was once tried against every key accepted before it.
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
let mut text = String::from("schema_version = 2\n");
|
||||
for i in 0..20_000 {
|
||||
text.push_str(&format!("unknown_{i} = \"{}\"\n", "x".repeat(64)));
|
||||
}
|
||||
text.push_str("[general]\nlanguage = \"de\"\n");
|
||||
write_config(tmp.path(), &text);
|
||||
let started = std::time::Instant::now();
|
||||
let (store, report) = open_in(tmp.path());
|
||||
assert!(report.section_errors.is_empty(), "{report:?}");
|
||||
assert_eq!(store.get().general.language, "de");
|
||||
assert!(started.elapsed() < std::time::Duration::from_secs(20), "{:?}", started.elapsed());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn newer_schema_opens_read_only_and_never_overwrites() {
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
|
||||
Reference in new issue
Block a user