diff --git a/CHANGELOG.md b/CHANGELOG.md index 2dfa8ac..485ff5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,6 +60,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Redact Cloudflare API tokens in `DocumentConversionSettings` Debug output, + including pretty formatting, cloned settings and settings retained after build, + independently of enabled document backends. + - Bound DOCX table dimensions and padded cell work before local conversion, including the default markkit backend. The deprecated Office renderer admits Markdown bytes before escaping or padding into one capped buffer. diff --git a/crates/fetchkit/src/documents/settings.rs b/crates/fetchkit/src/documents/settings.rs index 974f212..6f96ca6 100644 --- a/crates/fetchkit/src/documents/settings.rs +++ b/crates/fetchkit/src/documents/settings.rs @@ -1,6 +1,10 @@ +// Decisions: Preserve string-based settings across all feature sets; redact only +// the credential in Debug without changing public storage or build/clone semantics. //! String-based document conversion settings shared by the CLI, MCP server and //! Python bindings. Libraries can use [`DocumentConversion`] directly instead. +use std::fmt; + use super::{DocumentConversion, DocumentFormat}; /// Built-in converter names accepted by [`DocumentConversionSettings`]. @@ -12,7 +16,7 @@ pub const BUILTIN_CONVERTERS: &[&str] = &["pdf-inspector", "markkit", "office", /// Plain settings resolved into a [`DocumentConversion`]. /// /// Credentials are passed explicitly; nothing is read from the environment. -#[derive(Debug, Default, Clone)] +#[derive(Default, Clone)] pub struct DocumentConversionSettings { /// Accepted formats (e.g. `["pdf", "docx"]`). `None` keeps every supported format. pub formats: Option>, @@ -26,11 +30,30 @@ pub struct DocumentConversionSettings { /// Cloudflare account id, required when `converters` names `cloudflare`. pub cloudflare_account_id: Option, /// Cloudflare API token with Workers AI permission. + /// Redacted in settings `Debug`; direct field access still exposes the value. pub cloudflare_api_token: Option, /// Honour proxy environment variables for remote converter uploads. pub respect_proxy_env: bool, } +impl fmt::Debug for DocumentConversionSettings { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + // THREAT[TM-LEAK-007]: Never format the original credential or a clone of it. + f.debug_struct("DocumentConversionSettings") + .field("formats", &self.formats) + .field("converters", &self.converters) + .field("allow_remote", &self.allow_remote) + .field("max_document_bytes", &self.max_document_bytes) + .field("cloudflare_account_id", &self.cloudflare_account_id) + .field( + "cloudflare_api_token", + &self.cloudflare_api_token.as_ref().map(|_| ""), + ) + .field("respect_proxy_env", &self.respect_proxy_env) + .finish() + } +} + impl DocumentConversionSettings { /// Whether every field is at its default, i.e. nothing to configure. pub fn is_default(&self) -> bool { diff --git a/crates/fetchkit/tests/document_settings.rs b/crates/fetchkit/tests/document_settings.rs new file mode 100644 index 0000000..4900a5d --- /dev/null +++ b/crates/fetchkit/tests/document_settings.rs @@ -0,0 +1,72 @@ +use fetchkit::DocumentConversionSettings; + +fn assert_redacted(settings: &DocumentConversionSettings) { + for output in [format!("{settings:?}"), format!("{settings:#?}")] { + assert!(output.contains("")); + assert!(!output.contains("dummy-credential")); + for visible in [ + "formats", + "pdf", + "docx", + "converters", + "cloudflare", + "allow_remote: true", + "max_document_bytes", + "42", + "cloudflare_account_id", + "test-account", + "cloudflare_api_token", + "respect_proxy_env: true", + ] { + assert!( + output.contains(visible), + "missing nonsecret setting: {visible}" + ); + } + } +} + +#[test] +fn settings_debug_redacts_before_and_after_build_and_clone() { + let settings = DocumentConversionSettings { + formats: Some(vec!["pdf".into(), "docx".into()]), + converters: Some(vec!["cloudflare".into()]), + allow_remote: true, + max_document_bytes: Some(42), + cloudflare_account_id: Some("test-account".into()), + cloudflare_api_token: Some("dummy-credential".into()), + respect_proxy_env: true, + }; + assert_redacted(&settings); + let result = settings.build(); + #[cfg(feature = "docs-cloudflare")] + assert!(result.is_ok()); + #[cfg(not(feature = "docs-cloudflare"))] + assert_eq!( + result.unwrap_err(), + "converter `cloudflare` requires the docs-cloudflare feature" + ); + assert_redacted(&settings); + let cloned = settings.clone(); + assert_eq!( + cloned.cloudflare_api_token.as_deref(), + Some("dummy-credential") + ); + assert_redacted(&cloned); +} + +#[test] +fn settings_debug_distinguishes_absent_and_empty_credentials() { + let mut settings = DocumentConversionSettings::default(); + for output in [format!("{settings:?}"), format!("{settings:#?}")] { + assert!(output.contains("cloudflare_api_token: None")); + assert!(!output.contains("")); + assert!(output.contains("allow_remote: false")); + assert!(output.contains("respect_proxy_env: false")); + } + settings.cloudflare_api_token = Some(String::new()); + for output in [format!("{settings:?}"), format!("{settings:#?}")] { + assert!(output.contains("")); + assert!(!output.contains("cloudflare_api_token: None")); + } +} diff --git a/docs/hardening.md b/docs/hardening.md index eeea905..cbaac6d 100644 --- a/docs/hardening.md +++ b/docs/hardening.md @@ -129,3 +129,16 @@ another service: from `max_body_size`. Lower it, or restrict `formats`, for tighter budgets. - Local parsing of hostile documents is CPU-bound; isolate the process and apply outer CPU and memory limits for high-volume untrusted input (TM-DOS-007/008). + +`DocumentConversionSettings` redacts `cloudflare_api_token` in compact and pretty +`Debug` output, including cloned settings and settings retained after `build()`. +This works even without the `docs-cloudflare` feature. A present token is shown as +``; an absent token remains `None`. Other settings stay visible. +Converter credentials also use the redacting `SecretString` wrapper. + +Redaction applies to these representations. The public settings token field still +contains the original string, and building does not clear it. Avoid logging that +field directly or placing credentials in other settings. Fetchkit does not sanitize +arbitrary application logs or converter response text. Its CLI/MCP and Python +configuration paths do not log settings; embedding applications control their own +diagnostic output and access. diff --git a/knowledge/foundations/tool-contract.md b/knowledge/foundations/tool-contract.md index f2377cb..94b9af2 100644 --- a/knowledge/foundations/tool-contract.md +++ b/knowledge/foundations/tool-contract.md @@ -360,6 +360,11 @@ By default, Fetchkit blocks connections to private/reserved IP ranges: (Cloudflare credentials from `CLOUDFLARE_ACCOUNT_ID` / `CLOUDFLARE_API_TOKEN`), Python `document_formats`, `converters`, `allow_remote_conversion`, `max_document_bytes`, `cloudflare_account_id`, `cloudflare_api_token`. +- `DocumentConversionSettings::Debug` redacts a present Cloudflare token in compact + and pretty output, including clones and settings retained after `build()`, with + every backend feature configuration. Other settings remain visible; absent tokens + are `None`. The public token field remains `Option`: explicit field + access/formatting is not redacted, and building does not clear it. - For binary without a matching processor: - Return metadata (`content_type`, `size`, `filename`, `last_modified`) - Include `error: "Binary content is not supported. Only textual content (HTML, text, JSON, etc.) can be fetched."`, diff --git a/knowledge/log.md b/knowledge/log.md index 3245335..3042f65 100644 --- a/knowledge/log.md +++ b/knowledge/log.md @@ -1,5 +1,9 @@ # Fetchkit Knowledge Update Log +## 2026-10-13 + +* **Credential redaction**: `DocumentConversionSettings` now redacts present Cloudflare tokens in compact/pretty `Debug`, including clones and after build, independently of backend features. Public string storage remains unchanged; explicit field logging and downstream diagnostic access remain caller responsibilities (TM-LEAK-007). + ## 2026-10-12 * **DOCX table budgets**: Both local backends now admit table dimensions and padded cell work before conversion; the deprecated renderer additionally admits exact Markdown bytes before allocation. Tiny offline fixtures cover limits, escaping and normal output under TM-DOS-008. diff --git a/knowledge/security/threat-model.md b/knowledge/security/threat-model.md index b9a7443..889b3c1 100644 --- a/knowledge/security/threat-model.md +++ b/knowledge/security/threat-model.md @@ -377,7 +377,7 @@ escaping and unchanged normal tables; no resource-exhaustion payload is needed. | TM-LEAK-004 | User-Agent reveals software version | Info | Default UA `Everruns Fetchkit/1.0` reveals stack; configurable | **BY DESIGN** | | TM-LEAK-005 | Timing side-channels (connect time reveals network proximity) | Low | 1-second timeout masks some timing; not fully mitigated | **ACCEPTED** | | TM-LEAK-006 | Fetched documents disclosed to a remote converter | Medium | Remote converters off by default, explicit `allow_remote(true)`, forced off by `hardened()`, backend named in `extraction_method` | MITIGATED | -| TM-LEAK-007 | Remote converter credentials exposed | Medium | Tokens held in `SecretString`, redacted from `Debug`, never echoed in errors or responses | MITIGATED | +| TM-LEAK-007 | Remote converter credentials exposed | Medium | Converter secrets use `SecretString`; settings `Debug` redacts the token; authentication errors omit credentials | MITIGATED | ### Mitigation Details @@ -391,9 +391,21 @@ operator configuration, never from fetched content, and the response's `ConverterClient`, so a host can route them through its own egress boundary. **TM-LEAK-007 — Converter credential exposure (MITIGATED):** -API tokens and configured header values are stored as `SecretString`, which redacts -itself from `Debug`; authentication failures are reported by status, without the -token or request headers. +Converter API tokens and configured header values are stored as `SecretString`, +which redacts itself from `Debug`; authentication failures are reported by status, +without the token or request headers. `DocumentConversionSettings` retains its public +`Option` token field for configuration compatibility, but its custom `Debug` +formats present tokens (including empty strings) as `` and absent tokens +as `None`. This applies to compact/pretty formatting, clones and the original +settings after `build()`, independently of backend features. Noncredential fields +remain visible. Tiny public-API formatting contracts cover these cases. + +This is representation redaction, not a global log sanitizer or memory erasure: +callers can still read/format the token field directly, put secrets in other fields, +or expose service-controlled response text. `build(&self)` does not clear settings. +The in-tree CLI/MCP and Python configuration paths do not log settings; exposure +through configuration diagnostics requires an embedding application's logging sink +and diagnostic-reader access. No default production diagnostic leak is established. **TM-LEAK-001 — Error message detail (MITIGATED):** Fetchkit's error types (`FetchError`) use generic messages that don't include