privacy: raise the classify/redact body cap to 1 MB - #1013
Conversation
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 3 selected item(s). |
Review · Status🟩 CompletedIronLoop completed the review and posted it to GitHub. ResultRun detailsAutomatic trigger · attempt 1 of 3 · completed in 6m 4s |
Review —
|
| # | allocation | site |
|---|---|---|
| 1 | body: Bytes |
handler arg |
| 2 | parsed.input (serde_json::Value) — not dropped, lives to end of fn |
completions.rs:6523 |
| 3 | texts: Vec<String> (s.clone() / s.to_string()) |
completions.rs:6541-6548 |
| 4 | haystack = texts.join(...) |
auto_redact/mod.rs:177 |
| 5 | upstream_bytes (re-serialized, plus JSON escaping) |
completions.rs:6666 |
| 6 | redacted: Vec<String> + final response JSON |
completions.rs:6766 |
That's ~6 concurrent 1 MB allocations per in-flight redact, so ~400 MB/org at 64 concurrent, not 128 MB — and this is a shared multi-tenant TEE, so it's N orgs × that. Still bounded, but worth restating in the PR body with the right number since the 128 MB figure is what the merge decision rests on.
ℹ️ 3. The unauthenticated-surface argument is right in conclusion but wrong in mechanism (pre-existing — not a blocker for this diff)
Auth rejects before the size limit is evaluated. […] Only authenticated, rate-limited, credit-checked callers reach the size path.
Axum applies layers so the last .layer() is outermost. On text_inference_routes that's from_fn(middleware::body_hash_middleware) (crates/api/src/lib.rs:1629) — it runs before auth, and body.collect() (middleware/body_hash.rs:37) has no limit. DefaultBodyLimit only inserts an extension; enforcement happens at Bytes extraction inside the handler, after the body is already fully buffered.
So the 401 you observed came after the 300 KB was buffered. Your conclusion still holds — this PR does not widen anything, because the per-route cap never gated pre-auth buffering in the first place. But the reason is "already unbounded, unchanged here," not "auth gates it." (The repo half-knows this: auto_redact_adversarial.rs:474 notes "middleware ordering can produce 400 instead," and the comment at lib.rs:1633-1634 claiming body_hash sits after auth on the file routes looks incorrect for the same reason.) Worth a separate issue.
Nit — the just-under tests
assert_ne!(status, 413) passes on a 500/502 from the mock too. It does prove the cap moved (1 MB would have been a hard 413 at 256 KB), so it's not wrong — but asserting the expected success status would make it a stronger guard. Envelope math checks out: 1,047,552 + ~45 bytes leaves ~979 bytes of headroom under 1,048,576.
Verdict: #1 is the one I'd want addressed before merge, or at least consciously accepted — spawn_blocking alone is a two-line change that removes the runtime-starvation half of the risk. #2 is a correction to the PR body. #3 is pre-existing and belongs in its own issue.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review · Summary
Found one request-size enforcement issue in the expanded privacy endpoints.
Findings: 🔴 High 1
Code-specific findings are attached to the diff.
Validation
- ✅ Privacy request-path inspection — Reviewed the updated route configuration, body-hash middleware, both handlers, and the changed boundary tests.
Review details
- Run:
2b6eeb49-1100-41d8-baa1-ee01f1803418 - Attempts: 1
| const PRIVACY_CLASSIFY_MAX_BODY_SIZE: usize = 256 * 1024; // 256 KB | ||
| // The privacy model's context window is 128k tokens. A 1 MB cap covers it for | ||
| // typical text while keeping this route well below the 25 MB router default. | ||
| const PRIVACY_CLASSIFY_MAX_BODY_SIZE: usize = 1024 * 1024; // 1 MB |
There was a problem hiding this comment.
🔴 High · Enforce the body limit before hashing
DefaultBodyLimit constrains the later Bytes extractor, not direct body reads. Both privacy routes pass through body_hash_middleware, which calls body.collect() before that extractor runs, so an oversized (including chunked) request is fully buffered and hashed before it becomes a 413. The new 1 MiB cap therefore does not bound memory consumed on this path. Apply a streaming request-body limit or bounded collection around the hashing step while preserving the intended authentication order.
There was a problem hiding this comment.
Fixed in e4b8efc. You were right, and this undercut the premise of the PR — I had claimed the cap bounded memory when it bounded nothing on this path.
The fix needed two parts, because with_limited_body() alone would not have worked:
let (parts, body) = request.with_limited_body().into_parts();
// LengthLimitError -> 413; everything else stays 400That reads the limit from the request extension — but the extension did not exist yet when this middleware ran. DefaultBodyLimit was applied per-route (innermost) while body_hash_middleware is the outermost layer, so the limit was inserted after the hashing. The privacy routes therefore moved into their own router with the 1 MiB limit as the outermost layer, and the 25 MiB text-route limit moved outermost too.
Middleware execution order (body_hash -> auth -> rate limit -> usage) is unchanged and no route's limit is lowered — I kept the auth reordering out of scope deliberately, since that is a larger change affecting every route.
Chunked bodies with no Content-Length are covered: test_body_hash_rejects_chunked_body_over_configured_limit streams past a 4-byte limit and asserts 413 with the handler never invoked, and test_body_hash_allows_body_at_configured_limit pins the exact-limit boundary.
Regression-checked the routes that share this middleware: audio 48, ohttp 6, image_edits 3, privacy 31, adversarial 8 — all passing.
The 256 KB per-route cap was sized from the same wrong belief as the models table's `context_length: 512` — the constant's own comment said "model context is small (e.g. 512 tokens)". openai/privacy-filter advertises a 128,000-token context window. Measured against the live endpoint, 256 KB admits roughly: ~36,000 tokens sparse English (7.26 bytes/token, measured) ~65,000 tokens typical English (~4 bytes/token) ~87,000 tokens dense/code-like text (~3 bytes/token) So the cap, not the model, was bounding input. 128,000 tokens is ~512 KB of typical English and ~930 KB of sparse text, so 1 MB covers the real context window with headroom for JSON escaping. Risk is bounded. Auth rejects before the size limit is evaluated — verified live: 300 KB with an invalid key returns 401, the same body with a valid key returns 413 — so only authenticated, rate-limited, credit-checked callers reach the size path. These routes also sit on a router whose default limit is already 25 MB for audio transcription; at 1 MB, privacy/classify stays 25x tighter than its siblings. The two oversized-payload tests built a 300 KB body, which is now UNDER the cap and would have asserted nothing. They are raised past 1 MB, and a just-under-the-limit case is added to each so the cap is shown to have moved rather than merely been renamed. Refs #987
…time Three findings from review of the cap raise. 1. The cap did not bound anything on the path that matters. `DefaultBodyLimit` only inserts an extension consulted by the `Bytes` extractor inside the handler. `body_hash_middleware` is the outermost layer on these routers and called `body.collect()` with no limit, so an oversized request -- including a chunked one with no Content-Length -- was fully buffered and hashed before anything turned it into a 413. `body_hash_middleware` now uses `RequestExt::with_limited_body()` so it honours whatever limit the route configured, and maps `LengthLimitError` to 413 (everything else stays 400, as before). That only works if the limit extension exists before the middleware runs, and it did not: `DefaultBodyLimit` was applied per-route (innermost) while body_hash is outermost. The privacy routes therefore move to their own router with the 1 MiB limit as the OUTERMOST layer, and the 25 MiB text-route limit moves outermost too. Middleware execution order (body_hash -> auth -> rate limit -> usage) is unchanged; no route's limit is lowered. The comment on the file-upload routes claiming body_hash sits after auth was wrong for the same layer-ordering reason, and is corrected. Actually reordering auth ahead of body_hash is a larger change and is left alone. 2. `/privacy/redact` ran CPU-bound superlinear work on the async runtime. `apply_detected_spans` is roughly O(U*L + U^2) in unique PII spans and input length -- `would_collide` scans the whole joined input per candidate, and the minted-dummy check was a linear scan of `entries`. Span count is provider-controlled and both terms scale with the cap, so raising it 4x is ~16x worst-case CPU. With no `.await` in that loop a tokio worker was blocked throughout, stalling unrelated in-flight streams on the same thread. It now runs under `spawn_blocking`, and `RedactionMap` keeps a `HashSet` of minted dummies so the collision check is O(1) and short-circuits the haystack scan. `entries` keeps its descending-length ordering, which `unredact` depends on. 3. The just-under-limit tests asserted only `!= 413`, which also passes on a 500. They now assert the expected success status. cargo test -p api: privacy 31, adversarial 8, auto_redact 21, audio 48, ohttp 6 cargo test -p api body_hash: 4 (2 new: chunked-over-limit -> 413, at-limit -> ok) cargo test -p services auto_redact: 49 cargo clippy --workspace: 0 warnings
7a1cca9 to
e4b8efc
Compare
|
All three addressed in e4b8efc, and both corrections to the PR body were already applied — thank you, items 2 and 3 were straightforwardly wrong on my part. 1. I left the haystack-prefix optimisation alone as you suggested — bigger change, separate PR. 2. The memory figure. Corrected in the PR body: ~6 concurrent 1 MB allocations per in-flight redact, ~400 MB/org at the 64-concurrent cap, not the ~128 MB I wrote, and N orgs on a shared TEE. Your table of the six live allocations was more careful than my original count. 3. The unauthenticated-surface mechanism. Also corrected in the PR body, and this one is worth stating plainly: I had read the layer stack, suspected exactly what you describe, then let an empirical result (300 KB + bad key -> 401, + good key -> 413) talk me out of it. The experiment was real; my inference from it was not — the 401 came after the body was buffered. Conclusion unchanged (this PR widens nothing, because that path was already unbounded), but for the right reason now. The ironloop thread covers the fix; I will file the auth-ordering issue separately as you suggested. Nit. Just-under-limit tests now assert the expected success status rather than |
1 participant
Stacked on #1012 — base is
fix/privacy-classify-status-codes, so merge that first. Both branches touch the same test files; stacking avoids a conflict.Part of #987.
Why
The 256 KB per-route cap was sized from the same wrong belief as the
modelstable'scontext_length: 512— the constant's own comment read "Privacy classify input is text only, model context is small (e.g. 512 tokens)."openai/privacy-filteradvertises a 128,000-token context window.Measured against the live endpoint, 256 KB admits roughly:
So the cap, not the model, was bounding input. 128,000 tokens is ~512 KB of typical English and ~930 KB of sparse text, so 1 MB covers the real context window with headroom for JSON escaping.
Risk
Auth rejects before the size limit is evaluated.This was wrong in mechanism, though the conclusion stands. Axum applies layers so the last.layer()is outermost; ontext_inference_routesthat isbody_hash_middleware(lib.rs:1629), which runs before auth and callsbody.collect()with no limit.DefaultBodyLimitonly inserts an extension consulted atBytesextraction inside the handler — after buffering. So the 401 I observed for a 300 KB request with an invalid key came after those 300 KB were buffered.The conclusion that this PR does not widen the unauthenticated surface still holds — but because that path was already unbounded and is unchanged here, not because auth gates it.
Memory is higher than first stated.
body_hash_middleware'sbody_bytes.clone()is a refcountedBytesclone and nearly free, but/privacy/redactmaterializes the input several more times, concurrently: theBytesarg,parsed.input,texts: Vec<String>,haystack = texts.join(..), re-serializedupstream_bytes, and theredactedvec plus response JSON. That is roughly 6 concurrent 1 MB allocations per in-flight redact, so ~400 MB per org at the 64-concurrent cap — not the ~128 MB originally stated. Bounded, but this is a shared multi-tenant TEE, so it is N orgs × that.What does hold unchanged:
AUDIO_TRANSCRIPTION_MAX_BODY_SIZE = 25 MB; the per-route layer overrides it downward. At 1 MB, privacy/classify remains 25x tighter than its siblings (/audio/transcriptions,/embeddings,/rerank,/score).DEFAULT_COMPLETION_TIMEOUT_SECS = 600; measured latency was 1.1–5.2s for inputs up to 34k tokens.Addressed from review
body_hash_middlewarehonours the route's configuredDefaultBodyLimitrather than collecting unbounded, so the 1 MB cap actually bounds memory on this path./privacy/redactspan application moved off the async runtime.apply_detected_spansis O(U·L + U²) in unique PII values and input length, with no.awaitin the loop — a ~16x worst-case CPU increase with this cap change, blocking a tokio worker throughout. Now runs underspawn_blocking.!= 413.Tests
The two oversized-payload tests built a 300 KB body — now under the cap, so they would have asserted nothing. Raised past 1 MB, and a just-under-the-limit case added to each so the cap is shown to have moved rather than merely been renamed.
Deploy order
Do not deploy this ahead of nearai/cvm-compose-files#223. Until that lands, the backend still chunks at
PRIVACY_MAX_LENGTH=4096, so a 1 MB input becomes ~32 sequential chunks down the path that previously hard-exited on CUDA OOM. After #223 it is a single ~15 GB pass on a dedicated 141 GB H200.