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.

22 KiB

Raw Blame History

Devil's Advocate Audit Report

Date: 2026-03-26 Scope: Full codebase audit of AI Weekly Synth (Rust/Axum + SolidJS) Approach: Adversarial review -- challenging assumptions, finding hidden risks, questioning decisions

Executive Summary

AI Weekly Synth is a well-structured project with thoughtful security measures. However, it suffers from several uncomfortable truths:

The master encryption key is reconstructed from hex on every single API key operation -- it is never cached. This means the raw key string sits in AppConfig (a Clone-able struct) for the entire process lifetime, defeating the purpose of zeroize.
There is no background cleanup for expired sessions or stale jobs. The sessions table and article_history table will grow unbounded until someone notices.
The generation pipeline has no cancellation or timeout mechanism. A hung LLM call can block a user's generation slot indefinitely since the per-user "one active job" check has no upper time bound beyond the 1-hour TTL.
Rate limiting is entirely in-memory and per-process. A restart resets all rate limit state, and horizontal scaling (multiple instances) would have no shared rate limit enforcement.
The SSRF protection has a documented TOCTOU vulnerability (DNS rebinding) that is acknowledged but not mitigated.

Architectural Concerns

Why Rust for This?

The core business logic -- calling LLM APIs, scraping HTML, shuffling JSON -- is I/O-bound, not CPU-bound. Rust's primary advantage (zero-cost abstractions, memory safety without GC) adds relatively little value here compared to Python/FastAPI or TypeScript/Express, while significantly increasing development velocity cost.

Counter-argument the team would give: "It's a learning project." Fair. But the CLAUDE.md itself says "idiomatic Rust (learning project)" -- which means the architecture was chosen for developer education, not because it's the right tool. This is honest but means the codebase carries cognitive overhead that a maintainer would not expect.

Concrete risk: Contribution and maintenance will be limited to Rust-proficient developers, which is a small pool. If the original author steps away, someone inheriting this codebase will face a steep ramp-up compared to an equivalent Python project.

The Two-Phase Pipeline Is Overengineered

The synthesis pipeline (synthesis.rs, ~1300 lines) implements:

Phase 1: Source scraping with parallel link extraction, history dedup, batch classification
Phase 2: Web search fallback (LLM or Brave) with its own scraping + classification loop

This is the same scrape-classify-summarize logic duplicated twice (lines 429-543 and lines 639-757 are nearly identical patterns). The Brave Search path and the LLM Search path also duplicate filtering logic (homepage filter, cross-phase dedup, history dedup, source diversity -- all repeated in three places).

Concrete risk: A bug fix in one path (e.g., the dedup logic) must be manually replicated in all three paths. The recent commit history confirms this is already happening: fix: filter empty scraped articles + restore URLs after rewrite touched multiple places.

Single-Process In-Memory State

JobStore, RateLimiter, ProviderRateLimiter, and user_rate_limiters are all in-memory DashMap structures. This means:

No horizontal scaling: Two instances would each allow one concurrent job per user.
State loss on restart: All active generation jobs are silently lost.
No persistence of rate limit state: A process restart resets all rate limit windows.

For a "single-tenant self-hosted" app this is acceptable today, but the architecture does not admit a path to multi-instance deployment without significant rework.

Hidden Technical Debt

1. MasterKey Reconstruction on Every Operation

Every time an API key is encrypted or decrypted, the code calls MasterKey::from_hex(&state.config.master_encryption_key). This means:

A new Vec<u8> is allocated, filled with key material, used, then zeroized on drop.
But state.config.master_encryption_key (the hex string) lives in AppConfig, which derives Clone. Every clone of AppState carries a copy of the raw hex key string in memory. The zeroize on MasterKey::drop protects one copy while leaving the canonical source in plain memory.

File: backend/src/config.rs (line 16), backend/src/services/encryption.rs

Risk scenario: A memory dump or core dump of the process would reveal MASTER_ENCRYPTION_KEY in cleartext, because AppConfig::master_encryption_key is a String that is never zeroized. The MasterKey wrapper's zeroize is security theater in this context.

2. No Session Cleanup Scheduler

The sessions::delete_expired function exists (line 99 of db/sessions.rs) but is never called by any background task or scheduler. Expired sessions accumulate forever. The auth middleware checks expiration on each request (and deletes that one session), but sessions that simply expire without being used again are never cleaned.

File: backend/src/db/sessions.rs (line 98-105)

Risk scenario: After a year of operation, the sessions table could have tens of thousands of expired rows. Each auth check does an index lookup, so performance is not the concern -- the concern is data hygiene and compliance (old session metadata like IP addresses and user agents sitting in the database indefinitely).

3. article_history and llm_call_log Unbounded Growth

The article_history table stores a row for every article encountered during generation -- not just used articles, but also filtered, dropped, and deduplicated ones. With 5 categories, 15 links per source, and 10+ sources, a single generation run can produce 100+ rows. The cleanup_old function (called at the start of each generation) only deletes entries where synthesis_id IS NULL, meaning "used" entries are retained forever.

The llm_call_log table stores full prompts and responses (system_prompt, user_prompt, response_body as TEXT). A single generation run with 20 LLM calls storing full prompts and responses could easily be 500KB+ per run.

Files: backend/migrations/20260324000015_add_article_history.sql, backend/migrations/20260324000017_create_llm_call_log.sql

Risk scenario: A user generating weekly syntheses for a year produces ~52 runs x 100 history rows = 5,200+ article_history rows and ~52 x 20 = 1,040 llm_call_log entries (with full prompt text). For the LLM logs, this could be 25MB+ per user per year. There is no size-based cleanup or archival strategy.

4. LLM API Changes Will Break Silently

The LLM provider implementations (gemini.rs, openai.rs, anthropic.rs) parse JSON responses from vendor APIs. When these APIs change their response format (which happens regularly), the failure mode is a parse error that surfaces as a generic "Generation failed" error to the user.

There is no version pinning on the API endpoints (e.g., Gemini uses v1beta, OpenAI uses v1), and no structured error handling that distinguishes "API changed" from "API key invalid" from "model deprecated."

Risk scenario: Google deprecates a Gemini model identifier. The user's settings still reference it. Generation fails with an opaque error. The user has no way to diagnose the problem from the error message.

Security Assessment

Encryption: Sound Implementation, Flawed Key Lifecycle

The AES-256-GCM implementation itself is textbook correct:

Random 12-byte nonces via OS randomness
Proper use of the aes_gcm crate
zeroize on MasterKey drop
No nonce reuse (each encryption generates a fresh nonce)

But the key lifecycle is the vulnerability:

MASTER_ENCRYPTION_KEY is stored as a plain String in AppConfig
AppConfig derives Clone, so the key string is duplicated across every AppState clone
The key is passed through environment variables, which may be visible in /proc/<pid>/environ on Linux
There is no key rotation mechanism. If the key is compromised, every user's API keys must be re-encrypted with a new key -- and there is no tooling for this.

File: backend/src/config.rs (line 16), backend/src/services/encryption.rs

SSRF: Good but Incomplete

The SSRF protection is well-designed:

DNS resolution check before connection
Private IP detection for both IPv4 and IPv6
Redirect policy with per-hop IP validation
Scheme validation (http/https only)

Gaps:

DNS Rebinding (documented): The comment on line 62 of scraper.rs acknowledges TOCTOU between DNS check and actual TCP connection. An attacker controlling DNS could return a public IP for the check, then rebind to 127.0.0.1 for the actual connection.
IPv4-mapped IPv6 not checked: Addresses like ::ffff:127.0.0.1 or ::ffff:10.0.0.1 are not explicitly blocked. Depending on the network stack, these could bypass the IPv4 private checks.
Cloud metadata endpoints: 169.254.169.254 is blocked (link-local), but cloud providers often expose metadata at other addresses (e.g., fd00:ec2::254 on AWS). The IPv6 ULA range (fc00::/7) is blocked, which covers some of these, but not all.

File: backend/src/services/scraper.rs (lines 281-316)

Session Security: Solid Foundation, Missing Features

Strengths:

Session tokens are 32 bytes of OS randomness (256 bits of entropy)
Only SHA-256 hashes stored in DB (database leak doesn't compromise sessions)
HttpOnly, SameSite=Lax cookies
CSRF via X-Requested-With header check

Gaps:

No session sliding expiration: The last_active_at column exists but is never updated. Sessions expire at a fixed time regardless of activity. This is a design choice, but it means active users get logged out after 30 days.
No concurrent session limits: A user can have unlimited active sessions. A stolen session token remains valid for 30 days with no mechanism to detect anomalous session usage.
No session revocation UI: Users cannot see or revoke their active sessions.
Magic link tokens in GET parameters: The GET /api/v1/auth/verify?token=... endpoint puts the magic link token in the URL query string. This token ends up in browser history, server access logs, and potentially Referer headers. The POST endpoint exists as an alternative, but the email link uses GET.

File: backend/src/middleware/auth.rs, backend/src/handlers/auth.rs (line 244)

Error Message Sanitization: Allowlist Would Be Safer

The sanitize_error_message function (line 1291 of synthesis.rs) uses a blocklist approach -- checking for known sensitive patterns. This is inherently fragile:

What if an error contains connection refused to internal-host.corp.net:5432? It would not match any blocklist pattern and would be passed through.
The truncation at 200 chars still allows significant information leakage.

A safer approach would be an allowlist of known-safe error patterns, defaulting to a generic message for anything unrecognized.

Operational Risks

What Will Wake You at 3am

1. Database Fills Up (No Monitoring or Alerts)

The Docker Compose configuration does not set any PostgreSQL max_connections, shared_buffers, or storage limits. The postgres_data volume grows without bound. The shm_size: 128mb is set, but there are no alerts on disk usage.

Risk scenario: The llm_call_log table (storing full LLM prompts and responses as TEXT) grows past available disk. PostgreSQL starts rejecting writes. All generation jobs fail. The error appears as a generic "Internal error" to users.

File: docker-compose.yml

2. Hung Generation Jobs Block Users

If an LLM API call hangs (the reqwest timeout is 120 seconds, but the overall pipeline has no timeout), the user's generation slot is occupied. The has_active_job check prevents starting a new one. The only escape is the 1-hour job TTL, but during that hour the user is stuck.

There is no admin endpoint to cancel a stuck job, and no watchdog to kill jobs that exceed a reasonable duration.

File: backend/src/services/synthesis.rs (line 88, JOB_TTL)

3. Rate Limit Bypass on Restart

All rate limiting state is in-memory. A server restart (or deployment) resets all rate limit counters. An attacker who discovers this can time their abuse to coincide with deployments.

The ProviderRateLimiter::reload_from_db also resets all timestamps (line 182-191), meaning an admin updating rate limits for any provider resets the sliding window for that provider.

File: backend/src/services/rate_limiter.rs (lines 182-191)

4. No Database Backups Configured

The Docker Compose configuration provides no backup mechanism for the PostgreSQL volume. There is no pg_dump cron job, no WAL archiving, no replication.

Risk scenario: The Docker volume is lost due to host failure. All user data, syntheses, encrypted API keys, and settings are permanently gone.

UX Failure Modes

1. Generation Takes 10+ Minutes

With 10 sources, 15 links each, batch size 5, and LLM classification per article, the pipeline can make 30+ LLM calls (with rate limiting waits between batches). At 5-10 seconds per LLM call plus rate limit waits, a generation can easily take 5-15 minutes.

The SSE progress stream provides percentage updates, but the percentages are approximate (hardcoded ranges: 25-65% for Phase 1 processing, 70-90% for Phase 2). A user seeing "Articles 3-5/87..." at 27% for several minutes may assume it is stuck.

There is no estimated time remaining, no cancel button functionality, and if the browser tab is closed and reopened, the SSE reconnection has only 3 retries with exponential backoff (1s, 2s, 4s) before giving up permanently.

2. All Sources Down

If all user sources return errors during Phase 1, the pipeline silently moves to Phase 2 (web search fallback). If the user has no web search configured (no Brave key, LLM search disabled), the pipeline reaches the "no valid articles found" error at line 848. The error message ("Aucun article valide trouve. Verifiez vos sources et categories.") does not distinguish between "your sources are all down" and "we found articles but they were all filtered out."

3. Error Messages Are in French Only

All user-facing error messages in the backend are hardcoded in French. The CLAUDE.md mentions "i18n-ready" with French translations in the frontend, but the backend error messages bypass i18n entirely. A non-French-speaking user would see opaque French error messages.

Examples from synthesis.rs: "Limite de requetes atteinte", "Aucun article valide trouve", "Erreur d'authentification avec le fournisseur IA."

Edge Cases & Race Conditions

1. Race Condition in Job Creation

The create_job method (line 107 of synthesis.rs) iterates all entries in the DashMap to check if the user already has an active job, then inserts a new entry. This is not atomic. Two concurrent requests could both pass the check and create two jobs for the same user. The trigger_generate handler has a separate has_active_job check before create_job, making it doubly racy -- two requests could both pass has_active_job, then both call create_job.

Risk scenario: User double-clicks the "Generate" button rapidly. Two generation jobs start simultaneously, consuming double the LLM API quota and potentially producing duplicate syntheses.

File: backend/src/handlers/generation.rs (lines 43-62)

2. Settings Race with Generation

A user can update their settings while a generation is in progress. The generation pipeline reads settings once at the start (line 251). If the user changes their provider or model mid-generation, the change does not affect the running job (which is correct), but the user might be confused about which settings were used.

More problematically, if the user deletes their API key while a generation is in progress, the generation will fail at the next LLM call with a decryption or not-found error.

3. URL Normalization Edge Cases

The normalize_article_url function (line 1069) strips tracking parameters and trailing slashes but does not handle:

URL encoding differences: https://example.com/caf%C3%A9 vs https://example.com/cafe would be treated as different articles.
www vs non-www: https://www.example.com/article vs https://example.com/article are treated as different URLs.
Port normalization: https://example.com:443/article vs https://example.com/article.
Case sensitivity in paths: While the final .to_lowercase() handles this, some servers treat paths as case-sensitive.

File: backend/src/services/synthesis.rs (lines 1069-1107)

4. `sanitize_error_message` Truncation is Byte-Unsafe

Line 1313: &msg[..200] slices at byte position 200. If the message contains multi-byte UTF-8 characters (common in French: accented characters), this will panic at runtime if byte 200 falls in the middle of a multi-byte character.

File: backend/src/services/synthesis.rs (line 1313)

Risk scenario: An LLM returns an error message containing accented characters. The error sanitization panics, crashing the generation task. The job is left in a "progress" state forever (until TTL cleanup).

The Uncomfortable Questions

1. What Happens When a User's API Key Is Compromised?

If a user's LLM API key is compromised (e.g., database breach, even though keys are encrypted), there is no mechanism to:

Notify the user
Force re-encryption with a new master key
Audit which keys were decrypted and when
Rotate the master encryption key without downtime

2. Is the Per-Article LLM Classification Sustainable?

Each article in the pipeline requires an individual LLM call for classification and summarization. With 20 categories x 4 items = 80 target articles, and typical scrape success rates around 50%, the pipeline may attempt 160+ LLM calls per generation. At $0.01-0.10 per call, a single generation could cost $1.60-$16.00. Is the user aware of this cost?

3. Why Is `SESSION_SECRET` Required but Never Used?

The SESSION_SECRET environment variable is validated at startup (must be 64+ characters) but is never referenced in the actual session creation or verification code. Sessions use cryptographically random tokens with SHA-256 hashing -- the session secret is not used as an HMAC key or for any other purpose. It appears to be a vestigial requirement.

File: backend/src/config.rs (line 15), search for session_secret usage outside config shows no references in auth or session code.

4. What If the Article History Uniqueness Assumption Is Wrong?

The migration 20260324000016_enrich_article_history.sql drops the unique index on (user_id, url_hash) and replaces it with a non-unique index. This means the same URL can appear multiple times in the history table (e.g., once as "filtered_history", once as "used"). The check_urls_exist function returns a HashSet of matching hashes, so duplicates don't cause functional issues, but the table grows faster than expected and the ON CONFLICT DO NOTHING in insert_urls (which still exists) will never trigger since there is no unique constraint.

5. Is the "Single-Tenant" Assumption Actually Enforced?

The CLAUDE.md says "Single-tenant self-hosted (one instance per deployment)" but nothing in the code enforces this. Multiple users can register, each with their own settings and API keys. The rate limiting is global per-provider (not per-user by default). If two users generate simultaneously using the same LLM provider, they share the same rate limit bucket -- one user's generation can starve the other.

Prioritized Risk Mitigation

P0 -- Fix Before Next Release

UTF-8 truncation panic in sanitize_error_message (line 1313 of synthesis.rs): Replace &msg[..200] with a char-boundary-safe truncation. This is a runtime crash waiting to happen.
Race condition in job creation: Make has_active_job + create_job atomic by using a single DashMap operation or a mutex. Otherwise double-generation is possible on double-click.

P1 -- Fix Within 30 Days

Add expired session cleanup: Either a background tokio task that runs sessions::delete_expired periodically (e.g., every hour), or a database-level scheduled function. The function exists but is never called.
Add generation pipeline timeout: Wrap the run_generation_inner call with tokio::time::timeout(Duration::from_secs(600), ...) to ensure no job runs indefinitely.
Add database backup strategy: At minimum, add a pg_dump cron job or document a backup procedure. The current setup has zero data redundancy.

P2 -- Fix Within 90 Days

Stop storing master_encryption_key as a Clone-able String: Either store it in an Arc<MasterKey> in AppState (created once at startup) instead of reconstructing it per-operation, or at minimum stop deriving Clone on AppConfig and use Arc<AppConfig> instead.
Add database growth monitoring: Track table sizes for article_history, llm_call_log, and syntheses. Add cleanup policies or admin-facing growth dashboards.
Implement admin job management: Add an admin endpoint to list active jobs and cancel stuck ones.
Address IPv4-mapped IPv6 in SSRF checks: Add explicit checks for ::ffff: prefixed private IPv4 addresses.

P3 -- Address Eventually

Deduplicate pipeline logic: Extract the scrape-classify-summarize-filter pattern into a shared function called by both Phase 1, Phase 2 Brave, and Phase 2 LLM paths.
Add master key rotation tooling: A CLI command that decrypts all keys with the old master key and re-encrypts with a new one.
Evaluate whether SESSION_SECRET is actually needed: If it serves no purpose, remove it to reduce configuration burden. If it was intended for HMAC-signed sessions, either implement that or document why the current approach is preferred.
Add user-facing cost estimation: Before starting generation, estimate the number of LLM calls and display an approximate cost based on the selected provider's pricing.

22 KiB Raw Blame History