104 lines
6.1 KiB
Markdown
104 lines
6.1 KiB
Markdown
# Batch 4 Security + Stability Verification Findings
|
||
|
||
**Date:** 2026-05-24
|
||
**File:** `crates/cgcx-server/src/main.rs`
|
||
**Lines analyzed:** 1–1346
|
||
|
||
---
|
||
|
||
## 1. `get_metadata` and `serve_file` — Forward-submitted content edge cases
|
||
|
||
**Status:** No forward-specific breakage, but a functional gap exists.
|
||
|
||
Forward submissions are stored in the same `contents` / `content_files` tables as regular uploads. The web server has no forward-specific endpoints or logic (the forward system is handled entirely by the Telegram bot layer). Therefore:
|
||
|
||
- `get_metadata` (line ~565) and `serve_file` (line ~640) treat forward-submitted content identically to direct uploads.
|
||
- Status checks (`Deleted` / `Blacklisted`) and `max_views` pre-checks work the same for all content.
|
||
|
||
**Finding:** `get_metadata` does **not** increment views. This is correct for metadata alone, but it does mean a client can call `get_metadata` unlimited times on auto-destroy content without consuming the view budget. Combined with the `serve_raw_file` bypass (see §4), a user can enumerate metadata and download raw files without ever triggering auto-destroy.
|
||
|
||
**File/line:** `main.rs:565–625` (`get_metadata`), `main.rs:640–920` (`serve_file`)
|
||
|
||
---
|
||
|
||
## 2. `serve_file` view-increment logic — Concurrent requests for multi-file content
|
||
|
||
**Status:** Two issues found.
|
||
|
||
### 2a. Per-file view consumption breaks multi-file auto-destroy content
|
||
`serve_file` increments `view_count` once **per file request** (`main.rs:825`). If a content item contains 3 files, viewing all 3 files consumes 3 views. For content with `max_views = 1`, only the first file request succeeds before the content is marked for deletion. This means multi-file auto-destroy content is effectively unusable — users cannot view all files before the content self-destructs.
|
||
|
||
**File/line:** `main.rs:825` (`repo.increment_views(&content_id)`)
|
||
|
||
### 2b. TOCTOU race condition allows overserving past `max_views`
|
||
The flow in `serve_file` is:
|
||
1. Read `content.view_count` from `repo.get()` (stale snapshot).
|
||
2. Check `content.view_count >= max` → early `GONE` if true.
|
||
3. Later, if not HEAD/range/conditional, call `repo.increment_views()` atomically.
|
||
4. Check `new_views >= max` → trigger cleanup, **but still stream the file**.
|
||
|
||
Because there is no lock between the pre-check and the increment, two (or more) concurrent requests can both pass step 2, both increment, and both be served even if the total exceeds `max_views`. The DB increment is atomic, but the **serve decision** is not gated on the increment result.
|
||
|
||
Example with `max_views = 1`:
|
||
- Request A: pre-check passes (view_count = 0)
|
||
- Request B: pre-check passes (view_count = 0)
|
||
- Request A: `increment_views` → 1, triggers cleanup, streams file
|
||
- Request B: `increment_views` → 2, triggers cleanup, streams file
|
||
|
||
Both requests are served; the content is overserved.
|
||
|
||
**File/line:** `main.rs:778–790` (pre-check), `main.rs:825–850` (increment + post-check)
|
||
|
||
---
|
||
|
||
## 3. Password + auto-destroy interaction
|
||
|
||
**Status:** Partially fixed, but one bypass remains.
|
||
|
||
### What is correct:
|
||
- **HEAD requests:** `is_head = method == Method::HEAD` skips the increment block (`main.rs:824`).
|
||
- **Conditional requests:** `is_conditional = headers.contains_key(IF_NONE_MATCH)` skips the increment block (`main.rs:823`).
|
||
- **Unauthorized requests:** Password check happens **before** any view increment. An unauthorized request returns `401 Unauthorized` without consuming a view (`main.rs:799–808`).
|
||
|
||
### What is broken:
|
||
- **`serve_raw_file` does not increment views at all** (see §4). An attacker without a password can still be blocked by the auth check, but an **authorized** user can view password-protected auto-destroy content unlimited times via `/raw` without consuming views. The password gate works, but the view counter does not.
|
||
|
||
**File/line:** `main.rs:799–808` (auth before increment), `main.rs:820–826` (HEAD/conditional skip), `main.rs:923–1013` (`serve_raw_file` missing increment)
|
||
|
||
---
|
||
|
||
## 4. `serve_raw_file` — Missing view increment
|
||
|
||
**Status:** Confirmed bypass. **Not intentional.**
|
||
|
||
`serve_raw_file` (`main.rs:923`):
|
||
- Checks status and `max_views` (`main.rs:935–941`).
|
||
- Checks password (`main.rs:943–954`).
|
||
- Streams the fully decrypted file (`main.rs:995–1013`).
|
||
- **Never calls `repo.increment_views()`**.
|
||
|
||
This means:
|
||
1. **Auto-destroy bypass:** Any client (authorized or not, depending on password) can download the raw file unlimited times without the view counter ever increasing. Content with `max_views = 1` will never auto-destroy if accessed through `/raw`.
|
||
2. **Inconsistency:** The regular `serve_file` endpoint consumes views, but `/raw` does not. This creates an obvious bypass path.
|
||
|
||
**File/line:** `main.rs:923–1013` (`serve_raw_file`)
|
||
|
||
---
|
||
|
||
## Recommended Fixes
|
||
|
||
| # | Issue | Recommendation |
|
||
|---|-------|----------------|
|
||
| 1 | `serve_raw_file` view bypass | Add the same view-increment logic to `serve_raw_file` that exists in `serve_file`, including the HEAD / conditional / range skips. |
|
||
| 2 | Multi-file over-increment | Consider incrementing views in `get_metadata` (or adding a separate "content view" session token) so that one metadata fetch + N file fetches counts as 1 view, not N+1. Alternatively, document that each file request = 1 view. |
|
||
| 3 | TOCTOU race on `max_views` | Move the serve/deny decision into the DB transaction. Use `UPDATE … RETURNING` to atomically increment and read the new view count, and if the new count exceeds `max`, abort the transaction and return `GONE` **before** streaming. Alternatively, accept the soft limit behavior but add a `CHECK` constraint or second gate after increment that returns `GONE` if `new_views > max`. |
|
||
|
||
---
|
||
|
||
## Summary
|
||
|
||
- **§1:** Forward content is handled safely; no forward-specific edge cases in the web handlers.
|
||
- **§2:** `serve_file` has a race condition that allows concurrent requests to overserve past `max_views`. Multi-file content also consumes multiple views (one per file).
|
||
- **§3:** HEAD, conditional, and unauthorized requests correctly avoid consuming views.
|
||
- **§4:** **`serve_raw_file` is a confirmed bypass** — it never increments views, allowing unlimited access to auto-destroy content.
|