Harden snapshots and header templates

Author: eldryoth

src/headers.rs

@@ -300,7 +300,7 @@ fn render_header_template(value: &str, context: &RequestHeaderTemplateContext) -
         };
         let variable = &after_open[..close];
         if let Some(value) = context.variable(variable) {
-            rendered.push_str(value);
+            push_header_template_variable(&mut rendered, value);
         }
         rest = &after_open[close + 1..];
     }
@@ -309,6 +309,10 @@ fn render_header_template(value: &str, context: &RequestHeaderTemplateContext) -
     rendered
 }
 
+fn push_header_template_variable(rendered: &mut String, value: &str) {
+    rendered.extend(value.chars().filter(|character| !character.is_control()));
+}
+
 fn apply_response_mutations(
     response: &mut pingora::http::ResponseHeader,
     unset: &[String],
@@ -853,6 +857,36 @@ mod tests {
         }
     }
 
+    #[cfg(not(feature = "privacy-mode"))]
+    #[test]
+    fn dynamic_header_rendering_strips_control_characters_from_variables() {
+        let context = RequestHeaderTemplateContext {
+            headers: ::http::HeaderMap::new(),
+            host: Some("example.test".to_owned()),
+            remote_addr: Some("203.0.113.10\r\nx-injected: true".to_owned()),
+            scheme: "https",
+            uri: "/chat/\r\nbad".to_owned(),
+            path: "/chat/\u{7f}bad".to_owned(),
+            query: "room=main\tadmin=false".to_owned(),
+            request_id: Some("req-123\nbad".to_owned()),
+        };
+
+        let rendered = render_header_template(
+            "{host} {remote_addr} {scheme} {uri} {path} {query} {request_id}",
+            &context,
+        );
+
+        assert_eq!(
+            rendered,
+            "example.test 203.0.113.10x-injected: true https /chat/bad /chat/bad room=mainadmin=false req-123bad"
+        );
+        assert!(
+            rendered
+                .bytes()
+                .all(|byte| !matches!(byte, 0x00..=0x1f | 0x7f))
+        );
+    }
+
     #[cfg(not(feature = "privacy-mode"))]
     #[test]
     fn applies_user_friendly_request_header_operations() {

src/snapshot.rs

@@ -75,6 +75,7 @@ impl SnapshotStore {
         message: Option<&str>,
     ) -> Result<ConfigSnapshot, SnapshotError> {
         self.validate_root()?;
+        self.ensure_store_capacity()?;
         config.validate().map_err(|error| {
             SnapshotError::Config(crate::config::ConfigLoadError::Validate(error))
         })?;
@@ -260,6 +261,39 @@ impl SnapshotStore {
         Ok(())
     }
 
+    fn ensure_store_capacity(&self) -> Result<(), SnapshotError> {
+        if !self.safe_existing_root()? || !self.safe_existing_configs_dir()? {
+            return Ok(());
+        }
+
+        let mut snapshots = 0_usize;
+        for entry in fs::read_dir(self.configs_dir()).map_err(SnapshotError::Io)? {
+            let entry = entry.map_err(SnapshotError::Io)?;
+            let path = entry.path();
+            if path.extension().and_then(|extension| extension.to_str()) != Some("toml") {
+                continue;
+            }
+            let Some(stem) = path.file_stem().and_then(|stem| stem.to_str()) else {
+                continue;
+            };
+            if stem.ends_with(".meta") {
+                continue;
+            }
+            snapshots = snapshots.saturating_add(1);
+            if snapshots >= MAX_SNAPSHOT_STORE_ENTRIES {
+                return Err(SnapshotError::Io(io::Error::new(
+                    io::ErrorKind::StorageFull,
+                    format!(
+                        "snapshot store {} reached the {} snapshot limit",
+                        self.root.display(),
+                        MAX_SNAPSHOT_STORE_ENTRIES
+                    ),
+                )));
+            }
+        }
+        Ok(())
+    }
+
     fn validate_root(&self) -> Result<(), SnapshotError> {
         if self.root.as_os_str().is_empty()
             || self
@@ -755,6 +789,44 @@ mod tests {
         );
     }
 
+    #[test]
+    fn snapshot_config_rejects_full_snapshot_store_before_writing() {
+        let dir = TestDir::new("snapshot-capacity-before-write");
+        let store = SnapshotStore::new(dir.path());
+        let configs = dir.child("configs");
+        std::fs::create_dir(&configs).unwrap();
+        for index in 0..super::MAX_SNAPSHOT_STORE_ENTRIES {
+            std::fs::write(safe_child_path(&configs, &format!("s{index:04}.toml")), b"").unwrap();
+        }
+
+        let error = store
+            .snapshot_config(&Config::default(), Some("should not write"))
+            .unwrap_err();
+
+        assert!(
+            matches!(error, SnapshotError::Io(error) if error.kind() == std::io::ErrorKind::StorageFull)
+        );
+        let created_configs = std::fs::read_dir(&configs)
+            .unwrap()
+            .filter(|entry| {
+                entry
+                    .as_ref()
+                    .ok()
+                    .and_then(|entry| {
+                        entry
+                            .path()
+                            .extension()
+                            .and_then(|value| value.to_str())
+                            .map(str::to_owned)
+                    })
+                    .as_deref()
+                    == Some("toml")
+            })
+            .count();
+        assert_eq!(created_configs, super::MAX_SNAPSHOT_STORE_ENTRIES);
+        assert!(store.current_id().unwrap().is_none());
+    }
+
     #[test]
     fn current_snapshot_loads_current_pointer() {
         let dir = TestDir::new("snapshot-current-load");