mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Fix MIME types for path-backed feedback attachments (#30796)
## Why Path-backed feedback attachments were always labeled `text/plain`, even when the attached file was a gzip archive. Sentry consumers could therefore UTF-8-decode a valid Codex Desktop log bundle and corrupt the transferred bytes before anyone inspected it. Desktop already creates a valid archive and sends its path through `feedback/upload`; the bad metadata was assigned later by app-server's feedback upload path. Slack investigation: https://openai.slack.com/archives/C09NZ54M4KY/p1782867266569699 ## What changed Path-backed feedback attachments now derive their MIME type from the final uploaded filename. Gzip files use `application/gzip`, known text formats remain text, and unrecognized files use the safe `application/octet-stream` fallback. Attachment filenames and bytes are unchanged. ## How it works - **Classify at the upload boundary:** The feedback crate selects MIME metadata after resolving the final filename, including filename overrides. - **Preserve text rollouts:** Codex `.jsonl` rollouts remain `text/plain`, while other known formats use the repository's existing `mime_guess` mapping. - **Protect unknown binaries:** Unrecognized extensions fall back to `application/octet-stream` instead of being treated as UTF-8 text. - **Keep the wire stable:** `feedback/upload` still accepts the same path list, so Desktop, generated protocol surfaces, and remote-host minimums do not change. ## Verification Added focused coverage for gzip MIME, unknown binary fallback, `.jsonl` text handling, and exact filename/byte preservation. Ran the complete `codex-feedback` test suite (9 tests), crate-scoped Clippy, Rust formatting, Bazel lock refresh, and diff checks successfully.
This commit is contained in:
Generated
+1
@@ -3077,6 +3077,7 @@ dependencies = [
|
||||
"anyhow",
|
||||
"codex-login",
|
||||
"codex-protocol",
|
||||
"mime_guess",
|
||||
"pretty_assertions",
|
||||
"sentry",
|
||||
"tracing",
|
||||
|
||||
@@ -11,6 +11,7 @@ workspace = true
|
||||
anyhow = { workspace = true }
|
||||
codex-login = { workspace = true }
|
||||
codex-protocol = { workspace = true }
|
||||
mime_guess = { workspace = true }
|
||||
sentry = { version = "0.46" }
|
||||
tracing = { workspace = true }
|
||||
tracing-subscriber = { workspace = true }
|
||||
|
||||
@@ -4,6 +4,7 @@ use std::collections::btree_map::Entry;
|
||||
use std::fs;
|
||||
use std::io::Write;
|
||||
use std::io::{self};
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
use std::sync::Mutex;
|
||||
@@ -593,10 +594,22 @@ impl FeedbackSnapshot {
|
||||
.map(|s| s.to_string_lossy().to_string())
|
||||
.unwrap_or_else(|| "extra-log.log".to_string())
|
||||
});
|
||||
let content_type = match Path::new(&filename)
|
||||
.extension()
|
||||
.and_then(|extension| extension.to_str())
|
||||
{
|
||||
Some(extension) if extension.eq_ignore_ascii_case("jsonl") => {
|
||||
"text/plain".to_string()
|
||||
}
|
||||
_ => mime_guess::from_path(&filename)
|
||||
.first_or_octet_stream()
|
||||
.essence_str()
|
||||
.to_string(),
|
||||
};
|
||||
attachments.push(Attachment {
|
||||
buffer: data,
|
||||
filename,
|
||||
content_type: Some("text/plain".to_string()),
|
||||
content_type: Some(content_type),
|
||||
ty: None,
|
||||
});
|
||||
}
|
||||
@@ -774,6 +787,10 @@ mod tests {
|
||||
b"Connectivity diagnostics\n\n- Proxy environment variables are set and may affect connectivity.\n - HTTPS_PROXY = https://example.com:443".to_vec()
|
||||
);
|
||||
assert_eq!(attachments_with_diagnostics[3].buffer, b"rollout".to_vec());
|
||||
assert_eq!(
|
||||
attachments_with_diagnostics[3].content_type.as_deref(),
|
||||
Some("text/plain")
|
||||
);
|
||||
assert_eq!(
|
||||
OsStr::new(attachments_with_diagnostics[3].filename.as_str()),
|
||||
OsStr::new(extra_filename.as_str())
|
||||
@@ -794,6 +811,62 @@ mod tests {
|
||||
fs::remove_file(extra_path).expect("extra attachment should be removed");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn path_backed_attachments_use_binary_content_types() {
|
||||
let suffix = ThreadId::new();
|
||||
let gzip_filename = format!("codex-desktop-app-logs-{suffix}.tar.gz");
|
||||
let unknown_filename = format!("codex-feedback-extra-{suffix}.binunknown");
|
||||
let gzip_path = std::env::temp_dir().join(&gzip_filename);
|
||||
let unknown_path = std::env::temp_dir().join(&unknown_filename);
|
||||
let gzip_bytes = b"\x1f\x8b\x08\x00\xff";
|
||||
let unknown_bytes = b"\x00\x9f\x92\x96";
|
||||
fs::write(&gzip_path, gzip_bytes).expect("gzip attachment should be written");
|
||||
fs::write(&unknown_path, unknown_bytes).expect("unknown attachment should be written");
|
||||
|
||||
let attachments = CodexFeedback::new()
|
||||
.snapshot(/*session_id*/ None)
|
||||
.feedback_attachments(
|
||||
/*include_logs*/ false,
|
||||
&[],
|
||||
&[
|
||||
FeedbackAttachmentPath {
|
||||
path: gzip_path.clone(),
|
||||
attachment_filename_override: None,
|
||||
},
|
||||
FeedbackAttachmentPath {
|
||||
path: unknown_path.clone(),
|
||||
attachment_filename_override: None,
|
||||
},
|
||||
],
|
||||
/*logs_override*/ None,
|
||||
);
|
||||
|
||||
fs::remove_file(gzip_path).expect("gzip attachment should be removed");
|
||||
fs::remove_file(unknown_path).expect("unknown attachment should be removed");
|
||||
assert_eq!(
|
||||
attachments
|
||||
.iter()
|
||||
.map(|attachment| (
|
||||
attachment.filename.as_str(),
|
||||
attachment.content_type.as_deref(),
|
||||
attachment.buffer.as_slice(),
|
||||
))
|
||||
.collect::<Vec<_>>(),
|
||||
vec![
|
||||
(
|
||||
gzip_filename.as_str(),
|
||||
Some("application/gzip"),
|
||||
gzip_bytes.as_slice(),
|
||||
),
|
||||
(
|
||||
unknown_filename.as_str(),
|
||||
Some("application/octet-stream"),
|
||||
unknown_bytes.as_slice(),
|
||||
),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn upload_tags_include_client_tags_and_preserve_reserved_fields() {
|
||||
let mut tags = BTreeMap::new();
|
||||
|
||||
Reference in New Issue
Block a user