You cannot select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
522 lines
14 KiB
Markdown
522 lines
14 KiB
Markdown
# Audit Bug Fixes — Implementation Plan
|
|
|
|
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
|
|
|
**Goal:** Fix 9 bugs (3 P0, 6 P1) identified by the 5-agent code audit.
|
|
|
|
**Architecture:** Independent, targeted fixes across backend and frontend. No architectural changes. Each task is self-contained and can be committed independently.
|
|
|
|
**Tech Stack:** Rust (Axum, tokio), SolidJS, TypeScript
|
|
|
|
**Spec:** `docs/superpowers/specs/2026-03-26-audit-bugfixes-design.md`
|
|
|
|
---
|
|
|
|
### Task 1: Fix UTF-8 panic in error sanitization
|
|
|
|
**Files:**
|
|
- Modify: `backend/src/services/synthesis.rs`
|
|
|
|
- [ ] **Step 1: Find and fix the unsafe slice**
|
|
|
|
In `backend/src/services/synthesis.rs`, find the `sanitize_error_message` function (around line 1305). Replace:
|
|
|
|
```rust
|
|
if msg.len() > 200 {
|
|
format!("{}...", &msg[..200])
|
|
}
|
|
```
|
|
|
|
With:
|
|
|
|
```rust
|
|
if msg.len() > 200 {
|
|
let truncated: String = msg.chars().take(200).collect();
|
|
format!("{}...", truncated)
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Add a test**
|
|
|
|
Add a test in the same file's `#[cfg(test)] mod tests` block:
|
|
|
|
```rust
|
|
#[test]
|
|
fn sanitize_error_message_handles_multibyte_utf8() {
|
|
// This would panic with &msg[..200] if the 200th byte falls mid-character
|
|
let msg = "é".repeat(150); // 300 bytes, 150 chars
|
|
let result = sanitize_error_message(&msg);
|
|
assert!(result.len() <= 600); // 200 chars * 2 bytes each + "..."
|
|
assert!(result.ends_with("..."));
|
|
}
|
|
```
|
|
|
|
Note: `sanitize_error_message` may be private. If so, test via a helper or make `pub(crate)` for testing.
|
|
|
|
- [ ] **Step 3: Run tests**
|
|
|
|
Run: `cd backend && cargo test --lib`
|
|
Expected: All pass
|
|
|
|
- [ ] **Step 4: Commit**
|
|
|
|
```bash
|
|
git add backend/src/services/synthesis.rs
|
|
git commit -m "fix: prevent UTF-8 panic in error message truncation"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 2: Fix job creation race condition + pipeline timeout + panic handling
|
|
|
|
**Files:**
|
|
- Modify: `backend/src/services/synthesis.rs`
|
|
- Modify: `backend/src/handlers/generation.rs`
|
|
|
|
These three bugs (race condition, no timeout, swallowed panics) all touch the job lifecycle in `synthesis.rs` and the spawn in `generation.rs`. They must be applied together to avoid a window where panics permanently lock out users.
|
|
|
|
- [ ] **Step 1: Add `generating_users` DashSet to JobStore in `synthesis.rs`**
|
|
|
|
Add `use dashmap::DashSet;` to imports. Update `JobStore`:
|
|
|
|
```rust
|
|
pub struct JobStore {
|
|
inner: Arc<DashMap<Uuid, JobEntry>>,
|
|
generating_users: Arc<DashSet<Uuid>>,
|
|
}
|
|
```
|
|
|
|
Update `new()`:
|
|
```rust
|
|
pub fn new() -> Self {
|
|
Self {
|
|
inner: Arc::new(DashMap::new()),
|
|
generating_users: Arc::new(DashSet::new()),
|
|
}
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Update `create_job`, `has_active_job`, `cleanup_expired`, add `release_user`**
|
|
|
|
Replace `create_job`:
|
|
```rust
|
|
pub fn create_job(&self, user_id: Uuid) -> Option<(Uuid, Arc<watch::Sender<ProgressEvent>>)> {
|
|
if !self.generating_users.insert(user_id) {
|
|
return None;
|
|
}
|
|
let job_id = Uuid::new_v4();
|
|
let (tx, rx) = watch::channel(ProgressEvent::Progress {
|
|
step: "init".into(), message: "Initialisation...".into(), percent: 0,
|
|
});
|
|
let tx = Arc::new(tx);
|
|
self.inner.insert(job_id, JobEntry {
|
|
tx: Arc::clone(&tx), _rx: rx, user_id, created_at: Instant::now(),
|
|
});
|
|
Some((job_id, tx))
|
|
}
|
|
```
|
|
|
|
Simplify `has_active_job`:
|
|
```rust
|
|
pub fn has_active_job(&self, user_id: Uuid) -> Option<Uuid> {
|
|
if !self.generating_users.contains(&user_id) { return None; }
|
|
for entry in self.inner.iter() {
|
|
if entry.value().user_id == user_id { return Some(*entry.key()); }
|
|
}
|
|
None
|
|
}
|
|
```
|
|
|
|
Add `release_user`:
|
|
```rust
|
|
pub fn release_user(&self, user_id: Uuid) {
|
|
self.generating_users.remove(&user_id);
|
|
}
|
|
```
|
|
|
|
Update `cleanup_expired`:
|
|
```rust
|
|
pub fn cleanup_expired(&self) {
|
|
let now = Instant::now();
|
|
self.inner.retain(|_, entry| {
|
|
let keep = now.duration_since(entry.created_at) < JOB_TTL;
|
|
if !keep { self.generating_users.remove(&entry.user_id); }
|
|
keep
|
|
});
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 3: Update the spawn in `generation.rs` — timeout + panic handling + release**
|
|
|
|
In `backend/src/handlers/generation.rs`, find the spawn block (around line 70-75). Replace it with:
|
|
|
|
```rust
|
|
// Clone tx before moving into spawn so we can use it for panic handling
|
|
let tx_for_panic = Arc::clone(&tx);
|
|
let state_for_panic = state.clone();
|
|
|
|
let join_handle = tokio::spawn(async move {
|
|
let timeout_duration = std::time::Duration::from_secs(900); // 15 minutes
|
|
match tokio::time::timeout(timeout_duration, synthesis::run_generation(job_id, state_clone.clone(), user_id, tx.clone())).await {
|
|
Ok(()) => {}
|
|
Err(_) => {
|
|
tracing::error!(job_id = %job_id, user_id = %user_id, "Generation timed out after 15 minutes");
|
|
let _ = tx.send(synthesis::ProgressEvent::Error {
|
|
message: "La generation a depasse le delai maximum de 15 minutes.".into(),
|
|
});
|
|
}
|
|
}
|
|
state_clone.job_store.release_user(user_id);
|
|
});
|
|
|
|
// Monitor for panics — if the task panics, release_user inside won't run
|
|
tokio::spawn(async move {
|
|
if let Err(e) = join_handle.await {
|
|
tracing::error!(job_id = %job_id, error = %e, "Generation task panicked");
|
|
let _ = tx_for_panic.send(synthesis::ProgressEvent::Error {
|
|
message: "Erreur interne lors de la generation.".into(),
|
|
});
|
|
state_for_panic.job_store.release_user(user_id);
|
|
}
|
|
});
|
|
```
|
|
|
|
Read the file first to get the exact variable names (`state_clone`, `tx`, `job_id`, `user_id`).
|
|
|
|
- [ ] **Step 4: Build and test**
|
|
|
|
Run: `cd backend && cargo build && cargo test --lib`
|
|
Expected: All pass
|
|
|
|
- [ ] **Step 5: Commit**
|
|
|
|
```bash
|
|
git add backend/src/services/synthesis.rs backend/src/handlers/generation.rs
|
|
git commit -m "fix: atomic job creation, 15min timeout, and panic handling"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 3: Fix XSS via innerHTML + setTimeout cleanup in GenerateSynthesis.tsx
|
|
|
|
**Files:**
|
|
- Modify: `frontend/src/pages/GenerateSynthesis.tsx`
|
|
|
|
- [ ] **Step 1: Replace innerHTML with safe text**
|
|
|
|
Find the `innerHTML` usage (around line 249):
|
|
|
|
```tsx
|
|
<p innerHTML={t('generate.description', {
|
|
days: String(settings().max_age_days),
|
|
theme: settings().theme,
|
|
})} />
|
|
```
|
|
|
|
Replace with:
|
|
|
|
```tsx
|
|
<p class="text-gray-600">
|
|
{t('generate.description', {
|
|
days: String(settings().max_age_days),
|
|
theme: settings().theme,
|
|
})}
|
|
</p>
|
|
```
|
|
|
|
- [ ] **Step 2: Add onCleanup for setTimeout**
|
|
|
|
Find the `setTimeout` (around line 187):
|
|
|
|
```tsx
|
|
setTimeout(() => {
|
|
navigate(`/synthesis/${synthId}`);
|
|
}, 2000);
|
|
```
|
|
|
|
Replace with:
|
|
|
|
```tsx
|
|
const timer = setTimeout(() => {
|
|
navigate(`/synthesis/${synthId}`);
|
|
}, 2000);
|
|
onCleanup(() => clearTimeout(timer));
|
|
```
|
|
|
|
Make sure `onCleanup` is imported from `solid-js` at the top of the file.
|
|
|
|
- [ ] **Step 3: TypeScript check**
|
|
|
|
Run: `cd frontend && npx tsc --noEmit`
|
|
Expected: No errors
|
|
|
|
- [ ] **Step 4: Commit**
|
|
|
|
```bash
|
|
git add frontend/src/pages/GenerateSynthesis.tsx
|
|
git commit -m "fix: remove XSS via innerHTML and add setTimeout cleanup"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 4: Add expired session cleanup background task
|
|
|
|
**Files:**
|
|
- Modify: `backend/src/main.rs`
|
|
|
|
- [ ] **Step 1: Add periodic session cleanup**
|
|
|
|
In `backend/src/main.rs`, find the `Commands::Serve` block. After the rate limiter reload and before `let app = router::build_router(...)`, spawn a background task:
|
|
|
|
```rust
|
|
// Periodic session cleanup (every hour)
|
|
{
|
|
let cleanup_pool = state.pool.clone();
|
|
tokio::spawn(async move {
|
|
let mut interval = tokio::time::interval(std::time::Duration::from_secs(3600));
|
|
loop {
|
|
interval.tick().await;
|
|
match crate::db::sessions::delete_expired(&cleanup_pool).await {
|
|
Ok(count) => {
|
|
if count > 0 {
|
|
tracing::info!(deleted = count, "Cleaned up expired sessions");
|
|
}
|
|
}
|
|
Err(e) => {
|
|
tracing::warn!(error = %e, "Failed to clean up expired sessions");
|
|
}
|
|
}
|
|
}
|
|
});
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 2: Build and test**
|
|
|
|
Run: `cd backend && cargo build && cargo test --lib`
|
|
Expected: All pass
|
|
|
|
- [ ] **Step 3: Commit**
|
|
|
|
```bash
|
|
git add backend/src/main.rs
|
|
git commit -m "fix: add periodic expired session cleanup (hourly)"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 5: Fix SSRF bypass via IPv4-mapped IPv6
|
|
|
|
**Files:**
|
|
- Modify: `backend/src/services/scraper.rs`
|
|
|
|
- [ ] **Step 1: Add IPv4-mapped check to `is_private_ip`**
|
|
|
|
In `backend/src/services/scraper.rs`, find the `is_private_ip` function (around line 294). Add a check at the beginning of the `IpAddr::V6` branch:
|
|
|
|
```rust
|
|
IpAddr::V6(v6) => {
|
|
// Check for IPv4-mapped IPv6 addresses (::ffff:x.x.x.x)
|
|
if let Some(mapped_v4) = v6.to_ipv4_mapped() {
|
|
return mapped_v4.is_loopback()
|
|
|| mapped_v4.is_private()
|
|
|| mapped_v4.is_link_local()
|
|
|| mapped_v4.is_unspecified();
|
|
}
|
|
|
|
let segments = v6.segments();
|
|
v6.is_loopback()
|
|
// ... rest of existing checks
|
|
```
|
|
|
|
- [ ] **Step 2: Add tests for IPv4-mapped addresses**
|
|
|
|
Find the SSRF tests section and add:
|
|
|
|
```rust
|
|
#[test]
|
|
fn rejects_ipv4_mapped_ipv6_loopback() {
|
|
let ip: IpAddr = "::ffff:127.0.0.1".parse().unwrap();
|
|
assert!(is_private_ip(ip));
|
|
}
|
|
|
|
#[test]
|
|
fn rejects_ipv4_mapped_ipv6_private_10() {
|
|
let ip: IpAddr = "::ffff:10.0.0.1".parse().unwrap();
|
|
assert!(is_private_ip(ip));
|
|
}
|
|
|
|
#[test]
|
|
fn rejects_ipv4_mapped_ipv6_private_192() {
|
|
let ip: IpAddr = "::ffff:192.168.1.1".parse().unwrap();
|
|
assert!(is_private_ip(ip));
|
|
}
|
|
|
|
#[test]
|
|
fn rejects_ipv4_mapped_ipv6_link_local() {
|
|
let ip: IpAddr = "::ffff:169.254.1.1".parse().unwrap();
|
|
assert!(is_private_ip(ip));
|
|
}
|
|
|
|
#[test]
|
|
fn allows_ipv4_mapped_ipv6_public() {
|
|
let ip: IpAddr = "::ffff:8.8.8.8".parse().unwrap();
|
|
assert!(!is_private_ip(ip));
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 3: Run tests**
|
|
|
|
Run: `cd backend && cargo test --lib scraper`
|
|
Expected: All pass including the 5 new tests
|
|
|
|
- [ ] **Step 4: Commit**
|
|
|
|
```bash
|
|
git add backend/src/services/scraper.rs
|
|
git commit -m "fix: block SSRF via IPv4-mapped IPv6 addresses"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 6: Add SSRF check to source page fetching
|
|
|
|
**Files:**
|
|
- Modify: `backend/src/services/source_scraper.rs`
|
|
- Modify: `backend/src/services/scraper.rs` (make `check_ssrf` public)
|
|
|
|
- [ ] **Step 1: Make `check_ssrf` public**
|
|
|
|
In `backend/src/services/scraper.rs`, find `async fn check_ssrf` (around line 240) and change to:
|
|
|
|
```rust
|
|
pub async fn check_ssrf(url: &url::Url) -> Result<(), AppError> {
|
|
```
|
|
|
|
- [ ] **Step 2: Add SSRF check in `extract_article_links`**
|
|
|
|
In `backend/src/services/source_scraper.rs`, find `extract_article_links` (around line 32). Add the SSRF check before the HTTP fetch:
|
|
|
|
```rust
|
|
pub async fn extract_article_links(
|
|
http_client: &reqwest::Client,
|
|
source_url: &str,
|
|
max_links: usize,
|
|
) -> Result<Vec<String>, AppError> {
|
|
let base_url = Url::parse(source_url)
|
|
.map_err(|e| AppError::BadRequest(format!("Invalid source URL: {}", e)))?;
|
|
|
|
// SSRF check before fetching
|
|
if let Err(e) = crate::services::scraper::check_ssrf(&base_url).await {
|
|
tracing::warn!(url = source_url, error = %e, "Source URL failed SSRF check");
|
|
return Ok(Vec::new());
|
|
}
|
|
|
|
let base_domain = base_url.host_str().unwrap_or("").to_lowercase();
|
|
// ... rest unchanged
|
|
```
|
|
|
|
- [ ] **Step 3: Add SSRF check in `extract_article_links_with_llm`**
|
|
|
|
Same pattern in `extract_article_links_with_llm` (around line 185), add after `base_url` parse:
|
|
|
|
```rust
|
|
// SSRF check before fetching
|
|
if let Err(e) = crate::services::scraper::check_ssrf(&base_url).await {
|
|
tracing::warn!(url = source_url, error = %e, "Source URL failed SSRF check");
|
|
return Ok(Vec::new());
|
|
}
|
|
```
|
|
|
|
- [ ] **Step 4: Build and test**
|
|
|
|
Run: `cd backend && cargo build && cargo test --lib`
|
|
Expected: All pass
|
|
|
|
- [ ] **Step 5: Commit**
|
|
|
|
```bash
|
|
git add backend/src/services/scraper.rs backend/src/services/source_scraper.rs
|
|
git commit -m "fix: add SSRF check before fetching source pages"
|
|
```
|
|
|
|
---
|
|
|
|
### Task 7: Add onCleanup for setTimeout in frontend pages
|
|
|
|
**Files:**
|
|
- Modify: `frontend/src/pages/Home.tsx`
|
|
- Modify: `frontend/src/pages/ArticleHistory.tsx`
|
|
- Modify: `frontend/src/pages/SynthesisDetail.tsx`
|
|
|
|
- [ ] **Step 1: Fix Home.tsx**
|
|
|
|
In `frontend/src/pages/Home.tsx`, add `onCleanup` to the `solid-js` import. Add cleanup for the delete confirmation timers at the component level:
|
|
|
|
```tsx
|
|
onCleanup(() => {
|
|
const timers = deleteTimers();
|
|
Object.values(timers).forEach((timer) => clearTimeout(timer));
|
|
});
|
|
```
|
|
|
|
- [ ] **Step 2: Fix ArticleHistory.tsx**
|
|
|
|
In `frontend/src/pages/ArticleHistory.tsx`, add `onCleanup` and `createSignal` to the `solid-js` import (if not already present). Use a signal to track the timer:
|
|
|
|
```tsx
|
|
const [confirmTimer, setConfirmTimer] = createSignal<ReturnType<typeof setTimeout> | undefined>();
|
|
|
|
// At component top level:
|
|
onCleanup(() => {
|
|
const t = confirmTimer();
|
|
if (t) clearTimeout(t);
|
|
});
|
|
```
|
|
|
|
Then in the `handleClear` function, replace:
|
|
```tsx
|
|
setTimeout(() => setConfirming(false), 3000);
|
|
```
|
|
with:
|
|
```tsx
|
|
setConfirmTimer(setTimeout(() => setConfirming(false), 3000));
|
|
```
|
|
|
|
- [ ] **Step 3: Fix SynthesisDetail.tsx**
|
|
|
|
In `frontend/src/pages/SynthesisDetail.tsx`, add `onCleanup` and `createSignal` to the `solid-js` import. The `setTimeout` is inside the async `sendEmail` handler, so use a signal to track it (same pattern as ArticleHistory):
|
|
|
|
```tsx
|
|
const [emailTimer, setEmailTimer] = createSignal<ReturnType<typeof setTimeout> | undefined>();
|
|
|
|
// At component top level:
|
|
onCleanup(() => {
|
|
const t = emailTimer();
|
|
if (t) clearTimeout(t);
|
|
});
|
|
```
|
|
|
|
Then replace:
|
|
```tsx
|
|
setTimeout(() => setEmailSuccess(false), 5000);
|
|
```
|
|
with:
|
|
```tsx
|
|
setEmailTimer(setTimeout(() => setEmailSuccess(false), 5000));
|
|
```
|
|
|
|
Read the file first to find ALL `setTimeout` calls and apply the same pattern to each.
|
|
|
|
- [ ] **Step 4: TypeScript check**
|
|
|
|
Run: `cd frontend && npx tsc --noEmit`
|
|
Expected: No errors
|
|
|
|
- [ ] **Step 5: Commit**
|
|
|
|
```bash
|
|
git add frontend/src/pages/Home.tsx frontend/src/pages/ArticleHistory.tsx frontend/src/pages/SynthesisDetail.tsx
|
|
git commit -m "fix: add onCleanup for setTimeout in frontend pages"
|
|
```
|