fix: address PR review findings — compress errors, size guard, modal error display
Some checks failed
Test / rust-fmt-check (pull_request) Failing after 1m43s
Test / frontend-typecheck (pull_request) Successful in 2m8s
Test / frontend-tests (pull_request) Successful in 2m7s
Test / rust-clippy (pull_request) Successful in 3m17s
Test / rust-tests (pull_request) Successful in 4m32s
PR Review Automation / review (pull_request) Successful in 5m25s
Some checks failed
Test / rust-fmt-check (pull_request) Failing after 1m43s
Test / frontend-typecheck (pull_request) Successful in 2m8s
Test / frontend-tests (pull_request) Successful in 2m7s
Test / rust-clippy (pull_request) Successful in 3m17s
Test / rust-tests (pull_request) Successful in 4m32s
PR Review Automation / review (pull_request) Successful in 5m25s
- compress_text now returns Result<Vec<u8>, String>; callers propagate the error instead of silently storing empty BLOB on gzip failure - upload_image_attachment_by_content and upload_paste_image now validate decoded byte length against MAX_IMAGE_FILE_BYTES before DB storage, closing the size-bypass gap that existed for base64 content uploads - Image View modal in AttachmentsTab now surfaces the error string when get_image_attachment_data fails, replacing the opaque "could not be loaded" message with actionable diagnostic text Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
1b36ebfb3d
commit
911b6f591b
@ -60,10 +60,12 @@ const SAFE_TEXT_EXTENSIONS: &[&str] = &[
|
||||
|
||||
const SAFE_BINARY_EXTENSIONS: &[&str] = &["pdf", "docx", "doc", "xlsx", "xls"];
|
||||
|
||||
fn compress_text(text: &str) -> Vec<u8> {
|
||||
fn compress_text(text: &str) -> Result<Vec<u8>, String> {
|
||||
let mut encoder = GzEncoder::new(Vec::new(), Compression::default());
|
||||
encoder.write_all(text.as_bytes()).unwrap_or_default();
|
||||
encoder.finish().unwrap_or_default()
|
||||
encoder
|
||||
.write_all(text.as_bytes())
|
||||
.map_err(|e| format!("Compression write error: {e}"))?;
|
||||
encoder.finish().map_err(|e| format!("Compression finish error: {e}"))
|
||||
}
|
||||
|
||||
/// 100 MB cap — prevents decompression-bomb attacks on crafted DB entries.
|
||||
@ -256,7 +258,8 @@ pub async fn upload_log_file(
|
||||
..log_file
|
||||
};
|
||||
|
||||
let compressed = compress_text(&extracted_text);
|
||||
let compressed = compress_text(&extracted_text)
|
||||
.map_err(|e| format!("Failed to compress log content: {e}"))?;
|
||||
|
||||
let db = state.db.lock().map_err(|e| e.to_string())?;
|
||||
db.execute(
|
||||
@ -349,7 +352,8 @@ pub async fn upload_log_file_by_content(
|
||||
..log_file
|
||||
};
|
||||
|
||||
let compressed = compress_text(&content);
|
||||
let compressed = compress_text(&content)
|
||||
.map_err(|e| format!("Failed to compress log content: {e}"))?;
|
||||
|
||||
let db = state.db.lock().map_err(|e| e.to_string())?;
|
||||
db.execute(
|
||||
@ -682,17 +686,27 @@ mod tests {
|
||||
#[test]
|
||||
fn test_compress_decompress_roundtrip() {
|
||||
let original = "Hello, World! This is a log line with some content.";
|
||||
let compressed = compress_text(original);
|
||||
let compressed = compress_text(original).unwrap();
|
||||
assert!(!compressed.is_empty());
|
||||
assert!(compressed.len() < original.len() * 3);
|
||||
let decompressed = decompress_text(&compressed).unwrap();
|
||||
assert_eq!(decompressed, original);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_compress_returns_error_not_empty_on_failure() {
|
||||
// compress_text returns Result — callers must propagate, not silently discard.
|
||||
// For in-memory gzip this essentially never fails, but the API now allows
|
||||
// callers to surface the error rather than storing empty bytes.
|
||||
let result = compress_text("normal log line");
|
||||
assert!(result.is_ok(), "compress_text should succeed for normal input");
|
||||
assert!(!result.unwrap().is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_compress_large_text_is_smaller() {
|
||||
let original = "INFO server started\n".repeat(1000);
|
||||
let compressed = compress_text(&original);
|
||||
let compressed = compress_text(&original).unwrap();
|
||||
assert!(
|
||||
compressed.len() < original.len(),
|
||||
"gzip should compress repetitive text"
|
||||
@ -707,9 +721,6 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_decompress_size_limit_enforced() {
|
||||
// Create a compressible payload that when decompressed exceeds the limit.
|
||||
// We mock the limit by creating a large repetitive string and checking the
|
||||
// round-trip succeeds, then verify the limit constant is set correctly.
|
||||
assert_eq!(
|
||||
MAX_DECOMPRESSED_BYTES,
|
||||
100 * 1024 * 1024,
|
||||
@ -718,7 +729,7 @@ mod tests {
|
||||
|
||||
// A valid small payload must still decompress fine after the guard is in place.
|
||||
let text = "hello world decompression guard test\n".repeat(100);
|
||||
let compressed = compress_text(&text);
|
||||
let compressed = compress_text(&text).unwrap();
|
||||
let result = decompress_text(&compressed);
|
||||
assert!(result.is_ok());
|
||||
assert_eq!(result.unwrap(), text);
|
||||
|
||||
@ -140,6 +140,13 @@ pub async fn upload_image_attachment_by_content(
|
||||
.decode(data_part)
|
||||
.map_err(|_| "Failed to decode base64 image data")?;
|
||||
|
||||
if decoded.len() as u64 > MAX_IMAGE_FILE_BYTES {
|
||||
return Err(format!(
|
||||
"Image content exceeds maximum supported size ({} MB)",
|
||||
MAX_IMAGE_FILE_BYTES / 1024 / 1024
|
||||
));
|
||||
}
|
||||
|
||||
let content_hash = format!("{:x}", sha2::Sha256::digest(&decoded));
|
||||
let file_size = decoded.len() as i64;
|
||||
|
||||
@ -231,6 +238,13 @@ pub async fn upload_paste_image(
|
||||
.decode(data_part)
|
||||
.map_err(|_| "Failed to decode base64 image data")?;
|
||||
|
||||
if decoded.len() as u64 > MAX_IMAGE_FILE_BYTES {
|
||||
return Err(format!(
|
||||
"Pasted image exceeds maximum supported size ({} MB)",
|
||||
MAX_IMAGE_FILE_BYTES / 1024 / 1024
|
||||
));
|
||||
}
|
||||
|
||||
let content_hash = format!("{:x}", sha2::Sha256::digest(&decoded));
|
||||
let file_size = decoded.len() as i64;
|
||||
let file_name = format!("pasted-image-{}.png", uuid::Uuid::now_v7());
|
||||
|
||||
@ -259,6 +259,7 @@ function AttachmentsTab({ navigate }: { navigate: ReturnType<typeof useNavigate>
|
||||
|
||||
const [viewModal, setViewModal] = useState<{ type: "log" | "image"; id: string; title: string } | null>(null);
|
||||
const [modalContent, setModalContent] = useState<string | null>(null);
|
||||
const [modalError, setModalError] = useState<string | null>(null);
|
||||
const [modalLoading, setModalLoading] = useState(false);
|
||||
|
||||
useEffect(() => {
|
||||
@ -290,12 +291,13 @@ function AttachmentsTab({ navigate }: { navigate: ReturnType<typeof useNavigate>
|
||||
const openImageModal = async (id: string, fileName: string) => {
|
||||
setViewModal({ type: "image", id, title: fileName });
|
||||
setModalContent(null);
|
||||
setModalError(null);
|
||||
setModalLoading(true);
|
||||
try {
|
||||
const dataUrl = await getImageAttachmentDataCmd(id);
|
||||
setModalContent(dataUrl);
|
||||
} catch (e) {
|
||||
setModalContent(null);
|
||||
setModalError(String(e));
|
||||
} finally {
|
||||
setModalLoading(false);
|
||||
}
|
||||
@ -304,6 +306,7 @@ function AttachmentsTab({ navigate }: { navigate: ReturnType<typeof useNavigate>
|
||||
const closeModal = () => {
|
||||
setViewModal(null);
|
||||
setModalContent(null);
|
||||
setModalError(null);
|
||||
};
|
||||
|
||||
const formatBytes = (bytes: number) => {
|
||||
@ -510,7 +513,12 @@ function AttachmentsTab({ navigate }: { navigate: ReturnType<typeof useNavigate>
|
||||
/>
|
||||
)}
|
||||
{!modalLoading && viewModal.type === "image" && !modalContent && (
|
||||
<div className="text-center text-muted-foreground py-8">Image could not be loaded.</div>
|
||||
<div className="text-center py-8 space-y-2">
|
||||
<div className="text-muted-foreground">Image could not be loaded.</div>
|
||||
{modalError && (
|
||||
<div className="text-xs text-destructive font-mono">{modalError}</div>
|
||||
)}
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
</div>
|
||||
|
||||
Loading…
Reference in New Issue
Block a user