diff --git a/CHANGELOG.md b/CHANGELOG.md index 4503a602..9e333d82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,26 +25,59 @@ deploying. ### Read before upgrading +- **Back up `system_settings` and `models` first, and plan for no way + back to 2.2.** The upgrade converts the guard settings and drops a + column (next item). Version 2.2 cannot run on the converted database: + its settings are gone, so it would start on its seeded defaults, and + it can no longer read the models table. Take a copy before deploying: + + ```sh + pg_dump --data-only --table=system_settings --table=models "$DATABASE_URL" > thinkwatch-2.2-guards.sql + ``` + +- **Stop every replica of 2.2 before the first 3.0 one starts.** A 2.2 + replica still running when 3.0 converts finds its settings gone (it + then filters and redacts nothing) and cannot rebuild its router; one + that restarts writes its seeded defaults back. 3.0 does not convert + those a second time — it removes them at its next start and logs a + warning — but the 2.2 replica runs on them until then. With the Helm + chart (release `thinkwatch`, namespace `thinkwatch` here): + + ```sh + kubectl -n thinkwatch scale deployment/thinkwatch-server --replicas=0 + kubectl -n thinkwatch wait --for=delete pod \ + -l app.kubernetes.io/name=think-watch,app.kubernetes.io/component=server --timeout=5m + helm upgrade thinkwatch deploy/helm/think-watch -n thinkwatch # with your usual values + kubectl -n thinkwatch scale deployment/thinkwatch-server --replicas= + ``` + + The last command matters when `autoscaling.enabled` is on: the chart + then leaves the replica count alone, and it stays at 0. With Docker + Compose, `docker compose stop server` before pulling and starting the + new image. - **The old guard settings are converted at the first start, and behave as before.** `security.content_filter_patterns`, `security.hidden_text`, `security.pii_redactor_patterns` and `security.tool_inspection` become `security.content`, `security.redact` and `security.inspect_tools`, and are deleted, in - one transaction during the boot migration; a second start finds - nothing to convert. A content rule identical to a built-in rule - becomes that rule, switched on, any other a custom rule; a list with - rules in it runs in enforce mode with every built-in rule it did not - name switched off. The four seeded PII patterns become the built-in - rules for the same data (`cn-resident-id`, `bank-card`, `email`, - `cn-mobile-phone`), any other pattern a custom rule whose label is - its old placeholder prefix. A model's `output_guardrails` length cap - becomes `max_output_tokens` (below), and the column is dropped. **Stop - every replica of the previous version before the first new one - starts**: a replica still running 2.2 finds its settings gone (it - then filters and redacts nothing) and can no longer rebuild its - router once the column is dropped. To see what was converted, read - the three keys from Settings or `system_settings` afterwards; the - start-up log lists them too. + one transaction during the boot migration. The conversion is recorded + in `security.legacy_converted` (when, which version, what it + converted); with that record present, old keys or the old column + that show up again are removed, never converted, so the policies in + force are not overwritten. A content rule identical to a built-in + rule becomes that rule, switched on, any other a custom rule; a list + with rules in it runs in enforce mode with every built-in rule it did + not name switched off. The four seeded PII patterns become the + built-in rules for the same data (`cn-resident-id`, `bank-card`, + `email`, `cn-mobile-phone`), any other pattern a custom rule whose + label is its old placeholder prefix. Whatever the old runtime was + skipping (a rule that did not compile, a built-in rule id it did not + know, an `output_guardrails` value it could not read) is left out, + each with a warning in the start-up log. A model's + `output_guardrails` length cap becomes `max_output_tokens` (below), + and the column is dropped. To see what was converted, read the three + keys from Settings or `system_settings` afterwards; the start-up log + lists them too. - **Placeholders are written `<>`, not `{{EMAIL_1}}`.** The label of a custom rule is upper case letters, digits and underscores (an old prefix is converted: `REDACTED-SSN` → @@ -67,6 +100,14 @@ deploying. console's security page. With an empty PII list, redaction converts to observe mode: it records what it finds and changes nothing. +- **The built-in rules that replace the seeded PII patterns are + stricter.** An identity number has to have a real province code, a + real date of birth and a matching check digit; a card number a known + network's prefix and length and a valid Luhn digit, and published + test card numbers do not count. Numbers the old regexes took for + them — any 18 digits, any 16 — are no longer replaced. A deployment + that relied on the looser match can add its old regex back as a + custom rule. - **Tool calls are judged as the client receives them, and two built-in rules are new.** Inspection now reads a tool call converted to the caller's format and with redacted values restored — what the client @@ -81,15 +122,41 @@ deploying. characters are content filter rules: `unicode-tags` and `bidi-controls`, plus `zero-width` and `private-use`, which ship off. `security.hidden_text: block` converts to those two rules refusing, - `warn` and `log` to recording, `off` to switching them off. + `warn` and `log` to recording, `off` to switching them off. Recording + writes an audit event: `hidden_text: log` used to reach only the + application log, and now writes `gateway.content_flagged`. +- **A deployment with no content rules starts recording.** An empty + content filter list converts to observe mode with the built-in rules + that ship on, so requests matching them (`ignore previous + instructions` and the like) write `gateway.content_flagged` events + where 2.2 wrote nothing. Nothing on the wire changes. - **The output length guardrail is replaced by a model's maximum output - tokens.** A cap of N bytes on the answer converts to `ceil(N / 4)` - output tokens. The answer is no longer measured or cut: a request - asking for more tokens than the cap is lowered to it, and one asking - for none gets it, in whichever field its API uses; the upstream stops - there. The model API's `output_guardrails` field is gone; - `max_output_tokens` (1 to 2147483647, `null` for no limit) replaces - it. + tokens — check each model's after upgrading.** A cap of N bytes on + the answer converts to `ceil(N / 4)` output tokens, stored as + converted. The answer is no longer measured or cut: a request asking + for more tokens than the cap is lowered to it, in whichever field its + API uses (a Chat request that sets both `max_tokens` and + `max_completion_tokens` has both lowered), and the upstream stops + there; one asking for less keeps its own. A request that sets no limit + is held to the cap only when the cap is within what the gateway knows + the model's family to take (32,000 tokens for Claude models, 8,192 for + others); a Chat request is then given `max_completion_tokens` on + OpenAI's own endpoint, whose reasoning models refuse `max_tokens`, and + `max_tokens` elsewhere. Above that figure, the request goes out + without a limit rather than with one the model could refuse, and the + model's own default applies: a 100,000-byte cap on a non-Claude model + converts to 25,000 tokens, so its requests that set no limit are not + capped at all. To hold every request to a cap, have the clients send + a limit, or set the cap to that family figure or below. Reasoning + (thinking) tokens count towards the cap on the APIs that bill them as + output, so a cap that fit an answer can cut short a model that thinks + first. An Anthropic request with extended thinking has its thinking + budget lowered below the cap too, or thinking turned off when the cap + is 1,024 tokens or less, the smallest budget Anthropic takes. The + model API's `output_guardrails` field is gone: a request that still + sets one (anything but `null` or `[]`) is refused with `400`, so a + script cannot believe answers are still capped. `max_output_tokens` + (1 to 2147483647, `null` for no limit) replaces it. - **A new installation observes by default.** Every guard starts in observe mode, with only the built-in rules that rarely misfire switched on (personal data such as e-mail addresses and phone @@ -97,26 +164,52 @@ deploying. guard is switched to its third mode. - **A content filter refusal is `403`**, with the error type of the caller's API (`permission_error` for OpenAI-style APIs). Keyword and - regex rules used to refuse with `400`. -- **Guard policies are changed with their own permissions.** Writing - `security.redact` through `PATCH /api/admin/settings` takes - `pii_redactor:write`, `security.content` and `security.inspect_tools` - take `content_filter:write`; `settings:write` no longer covers them. - The seeded `admin` and `super_admin` roles hold both. + regex rules used to refuse with `400`. In `gateway_logs`, its + `error_type` is `PolicyBlocked`, where it was `TransformError`. +- **A rule that deletes text changes what is stored.** The request a + content rule stripped goes upstream, is redacted and is captured as + the stripped one: the audit log's request body is what was sent, not + what the caller typed. +- **Changing a guard policy takes `settings:write` and the guard's own + permission**: `pii_redactor:write` for `security.redact`, + `content_filter:write` for `security.content` and + `security.inspect_tools`, through `PATCH /api/admin/settings`. 2.2 + checked `settings:write` on the server and the guard permission in + the console; both are checked on the server now. Trying a sample + takes `pii_redactor:read` or `content_filter:read`. Reading the + policies — `GET /api/admin/security`, which the console's security + page loads — takes `settings:read`. The seeded `admin` and + `super_admin` roles hold all of them. - **Console API changes.** `GET /api/admin/security` lists each guard's mode and every rule, and `POST /api/admin/security/{guard}/test` tries a sample; they replace `/api/admin/settings/content-filter/test`, `/content-filter/presets`, `/pii-redactor/test`, `/tool-inspection/rules` and `/tool-inspection/test`, which are gone. -- **Audit events.** Every guard hit writes one event: - `gateway.content_flagged`, `gateway.content_stripped` and +- **Audit events.** Every rule that matches writes one event per + request: `gateway.content_flagged`, `gateway.content_stripped` and `gateway.content_blocked`; `gateway.redaction_flagged` and `gateway.redaction_replaced`; `gateway.tool_call_flagged` and `gateway.tool_call_blocked` as before. `gateway.hidden_text_flagged` and `gateway.hidden_text_blocked` are gone; hidden characters are - content events. With `audit.body_redact_pii` on, captured bodies are - redacted with the outbound redaction rules, built-in ones included, - whatever the redaction mode. + content events. **No event carries the request's text**: a content + event names the rule, the outcome, how many matches and whether they + were in a tool result; a redaction event names the rule and counts + the values and their occurrences, and only a built-in rule's lists a + few in masked form (`sk-an…7f9c`) — a custom rule's values are not + written at all. A request writes at most 20 events per guard (the + rules that changed it, or matched most, first), each with the number + of rules that matched (`rules_in_request`). With + `audit.body_redact_pii` on, captured bodies are redacted with the + outbound redaction rules, built-in ones included, whatever the + redaction mode. +- **Metrics renamed.** `gateway_hidden_text_total` is gone: hidden + characters count in `gateway_content_matched_total{outcome,custom}` + with every content rule. `pii_pattern_invalid_total{pattern}` is now + `guard_policy_invalid_total{guard}` (a stored policy with a rule that + does not compile; the rule is left out), next to + `guard_policy_unreadable_total{guard}` (a stored policy that cannot be + read; the guard runs on its factory policy). Redaction counts values + in `gateway_redaction_found_total{kind,outcome}`. ### Added @@ -134,17 +227,17 @@ deploying. ### Changed -- **Core crates at ThinkWatch-Core v0.58.0.** `tw-dialect`, `tw-guard`, +- **Core crates at ThinkWatch-Core v0.59.0.** `tw-dialect`, `tw-guard`, `tw-breaker` and `tw-bedrock` move from v0.55.0; the shared guard model described above comes with them. ### Fixed - **A credential in a matched tool call no longer reaches the audit - log.** The excerpt of a tool call that inspection cut or recorded — - and of a content filter hit — is masked with the redaction rules - before it is written; a key the model echoed, or one restored from a - placeholder, used to be stored as it was. + log.** The excerpt of a tool call that inspection cut or recorded is + masked with the redaction rules before it is written; 2.2 stored the + matched arguments as they were, a key the model wrote in them + included. - **A request's audit events and its log row carry the same id** when the caller sends no `x-trace-id`. The log row of a request that went through used to carry a second, unrelated id. diff --git a/Cargo.lock b/Cargo.lock index 797b894a..69413714 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4671,8 +4671,8 @@ dependencies = [ [[package]] name = "tw-bedrock" -version = "0.58.0" -source = "git+https://github.com/ThinkWatchProject/ThinkWatch-Core.git?tag=v0.58.0#c2a7bc637421e7be13bae185b2db609703b74473" +version = "0.59.0" +source = "git+https://github.com/ThinkWatchProject/ThinkWatch-Core.git?tag=v0.59.0#ef10a8b0bbc1325edf43553a87169029672808a4" dependencies = [ "aws-credential-types", "aws-sigv4", @@ -4686,16 +4686,16 @@ dependencies = [ [[package]] name = "tw-breaker" -version = "0.58.0" -source = "git+https://github.com/ThinkWatchProject/ThinkWatch-Core.git?tag=v0.58.0#c2a7bc637421e7be13bae185b2db609703b74473" +version = "0.59.0" +source = "git+https://github.com/ThinkWatchProject/ThinkWatch-Core.git?tag=v0.59.0#ef10a8b0bbc1325edf43553a87169029672808a4" dependencies = [ "serde", ] [[package]] name = "tw-dialect" -version = "0.58.0" -source = "git+https://github.com/ThinkWatchProject/ThinkWatch-Core.git?tag=v0.58.0#c2a7bc637421e7be13bae185b2db609703b74473" +version = "0.59.0" +source = "git+https://github.com/ThinkWatchProject/ThinkWatch-Core.git?tag=v0.59.0#ef10a8b0bbc1325edf43553a87169029672808a4" dependencies = [ "serde", "serde_json", @@ -4703,8 +4703,8 @@ dependencies = [ [[package]] name = "tw-guard" -version = "0.58.0" -source = "git+https://github.com/ThinkWatchProject/ThinkWatch-Core.git?tag=v0.58.0#c2a7bc637421e7be13bae185b2db609703b74473" +version = "0.59.0" +source = "git+https://github.com/ThinkWatchProject/ThinkWatch-Core.git?tag=v0.59.0#ef10a8b0bbc1325edf43553a87169029672808a4" dependencies = [ "base64 0.22.1", "bytes", diff --git a/Cargo.toml b/Cargo.toml index 8a1cdafd..9a2b4f4a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -70,10 +70,10 @@ opt-level = 3 # never re-exported through a local shim. And the reverse: something only # this side uses (the at-rest crypto, IMDSv2 credentials, the gateway error) # lives here, not in core. -tw-bedrock = { git = "https://github.com/ThinkWatchProject/ThinkWatch-Core.git", tag = "v0.58.0" } -tw-breaker = { git = "https://github.com/ThinkWatchProject/ThinkWatch-Core.git", tag = "v0.58.0" } -tw-dialect = { git = "https://github.com/ThinkWatchProject/ThinkWatch-Core.git", tag = "v0.58.0" } -tw-guard = { git = "https://github.com/ThinkWatchProject/ThinkWatch-Core.git", tag = "v0.58.0" } +tw-bedrock = { git = "https://github.com/ThinkWatchProject/ThinkWatch-Core.git", tag = "v0.59.0" } +tw-breaker = { git = "https://github.com/ThinkWatchProject/ThinkWatch-Core.git", tag = "v0.59.0" } +tw-dialect = { git = "https://github.com/ThinkWatchProject/ThinkWatch-Core.git", tag = "v0.59.0" } +tw-guard = { git = "https://github.com/ThinkWatchProject/ThinkWatch-Core.git", tag = "v0.59.0" } # Web framework axum = { version = "0.8", features = ["macros", "ws"] } diff --git a/crates/common/src/guard_policy/legacy.rs b/crates/common/src/guard_policy/legacy.rs index 5bcb2fc8..7fe61d1f 100644 --- a/crates/common/src/guard_policy/legacy.rs +++ b/crates/common/src/guard_policy/legacy.rs @@ -81,8 +81,19 @@ fn description(guard: Guard) -> &'static str { } } -/// Convert whatever the old settings left behind. A no-op on a database -/// that has none of them. +/// The key that records that this database runs the unified guard +/// settings: written once, by the first start of a version that has them — +/// with what it converted, if there was anything to convert. +pub const MARKER: &str = "security.legacy_converted"; + +/// Convert whatever the old settings left behind, once. +/// +/// **The first start converts; no later start does.** Once [`MARKER`] is +/// written, old keys or the old column showing up again were written by a +/// version from before the unification started against this database — +/// a rollback, or an old replica restarting, whose seeds write their +/// defaults back. Converting them would overwrite the policies in force +/// with those defaults, so they are only removed, with a warning. pub async fn upgrade(pool: &PgPool) -> anyhow::Result<()> { let mut tx = pool.begin().await?; sqlx::query("SELECT pg_advisory_xact_lock($1)") @@ -90,6 +101,11 @@ pub async fn upgrade(pool: &PgPool) -> anyhow::Result<()> { .execute(&mut *tx) .await?; + let marker: Option = + sqlx::query_scalar("SELECT value FROM system_settings WHERE key = $1") + .bind(MARKER) + .fetch_optional(&mut *tx) + .await?; let old_keys: Vec = OLD_KEYS.iter().map(|k| k.to_string()).collect(); let old: Vec<(String, Value)> = sqlx::query_as("SELECT key, value FROM system_settings WHERE key = ANY($1) FOR UPDATE") @@ -103,7 +119,30 @@ pub async fn upgrade(pool: &PgPool) -> anyhow::Result<()> { ) .fetch_one(&mut *tx) .await?; - if old.is_empty() && !column { + let removed: Vec<&str> = old.iter().map(|(k, _)| k.as_str()).collect(); + + if let Some(marker) = marker { + if old.is_empty() && !column { + return Ok(()); + } + sqlx::query("DELETE FROM system_settings WHERE key = ANY($1)") + .bind(&old_keys) + .execute(&mut *tx) + .await?; + if column { + sqlx::query("ALTER TABLE models DROP COLUMN output_guardrails") + .execute(&mut *tx) + .await?; + } + tx.commit().await?; + tracing::warn!( + keys = ?removed, + column = column.then_some("models.output_guardrails"), + converted = %marker, + "Guard settings of a version before 3.0 reappeared after they were converted \ + (was an older version started against this database?): removed, not converted; \ + the policies in force are unchanged" + ); return Ok(()); } @@ -138,12 +177,12 @@ pub async fn upgrade(pool: &PgPool) -> anyhow::Result<()> { let mut capped = 0usize; if column { - let rows: Vec<(uuid::Uuid, Value)> = - sqlx::query_as("SELECT id, output_guardrails FROM models") + let rows: Vec<(uuid::Uuid, String, Value)> = + sqlx::query_as("SELECT id, model_id, output_guardrails FROM models") .fetch_all(&mut *tx) .await?; - for (id, guardrails) in rows { - if let Some(n) = max_output_tokens(&guardrails) { + for (id, model_id, guardrails) in rows { + if let Some(n) = model_cap(&model_id, &guardrails) { sqlx::query("UPDATE models SET max_output_tokens = $2 WHERE id = $1") .bind(id) .bind(n) @@ -156,14 +195,36 @@ pub async fn upgrade(pool: &PgPool) -> anyhow::Result<()> { .execute(&mut *tx) .await?; } + + let record = serde_json::json!({ + "at": chrono::Utc::now().to_rfc3339(), + "version": env!("CARGO_PKG_VERSION"), + "converted": removed, + "models_capped": capped, + }); + sqlx::query( + "INSERT INTO system_settings (key, value, category, description) + VALUES ($1, $2, 'security', $3) + ON CONFLICT (key) DO NOTHING", + ) + .bind(MARKER) + .bind(&record) + .bind( + "When this database moved to the unified guard settings, and what it converted \ + (written once at start-up; not editable)", + ) + .execute(&mut *tx) + .await?; tx.commit().await?; - tracing::info!( - converted = ?converted.iter().map(|(g, v)| format!("{}={v}", super::key(*g))).collect::>(), - removed = ?old.iter().map(|(k, _)| k.as_str()).collect::>(), - models_capped = capped, - "Converted the previous guard settings" - ); + if !removed.is_empty() || column { + tracing::info!( + converted = ?converted.iter().map(|(g, v)| format!("{}={v}", super::key(*g))).collect::>(), + removed = ?removed, + models_capped = capped, + "Converted the previous guard settings" + ); + } Ok(()) } @@ -270,9 +331,14 @@ pub fn content(patterns: Option<&Value>, hidden: Option<&Value>) -> ContentPolic usable }) .collect(); - let hidden: HiddenText = hidden - .and_then(|v| serde_json::from_value(v.clone()).ok()) - .unwrap_or_default(); + let hidden: HiddenText = match hidden.map(|v| serde_json::from_value(v.clone())) { + Some(Ok(h)) => h, + Some(Err(e)) => { + tracing::warn!(error = %e, "{HIDDEN_TEXT} was unreadable, so it ran as `warn` — converted as `warn`"); + HiddenText::default() + } + None => HiddenText::default(), + }; // Built-in rules the list named, with the action they had there. The // same rule twice reported its most severe action. @@ -483,11 +549,20 @@ pub fn tools(stored: Option<&Value>) -> ToolPolicy { } None => OldToolInspection::default(), }; + // A built-in id the engine does not know: the old runtime ignored it, + // and the policy would refuse it. let known = |id: &String| { - tw_guard::tools::rules::builtin() + let ok = tw_guard::tools::rules::builtin() .dangerous .iter() - .any(|s| &s.id == id) + .any(|s| &s.id == id); + if !ok { + tracing::warn!( + rule = %id, + "{TOOL_INSPECTION} named a built-in rule that does not exist — not converted" + ); + } + ok }; let mut names = Names::default(); ToolPolicy { @@ -503,8 +578,16 @@ pub fn tools(stored: Option<&Value>) -> ToolPolicy { .custom .into_iter() .filter(|c| { - tw_guard::tools::rules::single(&c.name, &c.pattern, false).is_ok() - && !c.name.trim().is_empty() + let ok = tw_guard::tools::rules::single(&c.name, &c.pattern, false).is_ok() + && !c.name.trim().is_empty(); + if !ok { + tracing::warn!( + rule = %c.name, + "Tool-call rule the gateway was skipping (no name, or a pattern that does \ + not compile) — not converted" + ); + } + ok }) .map(|c| CustomToolRule { name: names.unique(&c.name), @@ -538,6 +621,27 @@ pub fn max_output_tokens(guardrails: &Value) -> Option { .map(|n| i32::try_from(n.div_ceil(4)).unwrap_or(i32::MAX)) } +/// The converted cap of one model ([`max_output_tokens`]), stored as it +/// converts. It is not lowered to what the gateway knows the model's +/// family to take: a lower cap would also lower the limit of every request +/// that sets one above it, cutting answers 2.2 let through. A request that +/// sets no limit is held to a cap above that figure only by the model's +/// own default (see the gateway's `Outbound::cap_output`). +/// +/// Logs a value it could not read. +fn model_cap(model_id: &str, guardrails: &Value) -> Option { + let n = max_output_tokens(guardrails); + if n.is_none() && !guardrails.as_array().is_some_and(Vec::is_empty) { + tracing::warn!( + model = model_id, + output_guardrails = %guardrails, + "This model's output_guardrails could not be read, so the gateway ran it without \ + a cap — converted as no cap" + ); + } + n +} + // ---------------------------------------------------------------- shared /// A list stored under `key`, every element read strictly. One element @@ -567,6 +671,9 @@ impl Names { candidate = format!("{name} ({n})"); n += 1; } + if candidate != name { + tracing::warn!(rule = %name, renamed = %candidate, "Two old rules shared a name — the second is renamed"); + } candidate } } @@ -961,6 +1068,18 @@ mod tests { assert_eq!(tools(Some(&json!({"mode": "loud"}))), ToolPolicy::default()); } + #[test] + fn a_converted_cap_is_stored_as_it_converts() { + let bytes = |n: usize| json!([{"type": "max_length", "max_chars": n}]); + // Above the 8,192 the gateway fills in for a non-Claude model, and + // above Claude's 32,000: not lowered to either. + assert_eq!(model_cap("gpt-4o", &bytes(100_000)), Some(25_000)); + assert_eq!(model_cap("claude-opus", &bytes(200_000)), Some(50_000)); + assert_eq!(model_cap("gpt-4o", &bytes(4096)), Some(1024)); + assert_eq!(model_cap("m", &json!({"oops": 1})), None); + assert_eq!(model_cap("m", &json!([])), None); + } + #[test] fn a_length_cap_in_bytes_becomes_a_quarter_of_it_in_tokens() { let cap = |v: Value| max_output_tokens(&v); diff --git a/crates/common/src/guard_policy/mod.rs b/crates/common/src/guard_policy/mod.rs index 54d14c1d..408fb89d 100644 --- a/crates/common/src/guard_policy/mod.rs +++ b/crates/common/src/guard_policy/mod.rs @@ -42,20 +42,103 @@ pub fn guard_of(key: &str) -> Option { Guard::ALL.iter().copied().find(|g| self::key(*g) == key) } +/// How many custom rules a policy may hold, and how long one's pattern +/// may be, in characters. Every rule runs on every request: a policy past +/// these is a mistake, or an attempt to slow the gateway down, rather than +/// a configuration. The limits this gateway had before the guards were +/// unified. +pub struct Limits { + pub custom_rules: usize, + pub pattern_chars: usize, +} + +/// The limits of one guard's policy. +pub fn limits(guard: Guard) -> Limits { + match guard { + Guard::Redact => Limits { + custom_rules: 100, + pattern_chars: 1000, + }, + Guard::InspectTools => Limits { + custom_rules: 100, + pattern_chars: 1000, + }, + Guard::Content => Limits { + custom_rules: 500, + pattern_chars: 500, + }, + } +} + /// Check a value for one guard's key before it is saved: its shape (a -/// misspelt field is an error, not a silent factory value) and its rules -/// (unknown built-in ids, patterns that do not compile, malformed code -/// points or placeholder names, custom rules without a name or sharing -/// one). The error is the sentence the admin sees. +/// misspelt field is an error, not a silent factory value), its size +/// ([`limits`]; a custom tool-call rule may not take a built-in rule's +/// name either), and its rules (unknown built-in ids, patterns that do not +/// compile, malformed code points or placeholder names, custom rules +/// without a name or sharing one). The error is the sentence the admin +/// sees. pub fn validate(guard: Guard, value: &Value) -> Result<(), String> { let checked = match guard { - Guard::Redact => parse::(guard, value)?.check(), - Guard::InspectTools => parse::(guard, value)?.check(), - Guard::Content => parse::(guard, value)?.check(), + Guard::Redact => { + let p = parse::(guard, value)?; + within_limits(guard, p.custom.iter().map(|c| (&c.name, &c.pattern)))?; + p.check() + } + Guard::InspectTools => { + let p = parse::(guard, value)?; + within_limits(guard, p.custom.iter().map(|c| (&c.name, &c.pattern)))?; + let builtin = &tw_guard::tools::rules::builtin().dangerous; + if let Some(c) = p + .custom + .iter() + .find(|c| builtin.iter().any(|b| b.id == c.name)) + { + return Err(format!( + "{}: custom rule `{}` has the name of a built-in rule; give it another name", + key(guard), + c.name + )); + } + p.check() + } + Guard::Content => { + let p = parse::(guard, value)?; + within_limits(guard, p.custom.iter().map(|c| (&c.name, &c.pattern)))?; + p.check() + } }; checked.map_err(|e| e.to_string()) } +/// Custom rules no more and no longer than [`limits`] allows. +fn within_limits<'a>( + guard: Guard, + custom: impl ExactSizeIterator, +) -> Result<(), String> { + let Limits { + custom_rules, + pattern_chars, + } = limits(guard); + if custom.len() > custom_rules { + return Err(format!( + "{}: {} custom rules; at most {custom_rules} are allowed", + key(guard), + custom.len() + )); + } + for (name, pattern) in custom { + let n = pattern.chars().count(); + if n > pattern_chars { + return Err(format!( + "{}: the pattern of custom rule `{name}` is {n} characters long; at most \ + {pattern_chars} are allowed", + key(guard) + )); + } + } + Ok(()) +} + fn parse(guard: Guard, value: &Value) -> Result { // serde would read a struct out of a JSON array too (`[]` as every // field left out); a policy is an object. @@ -209,6 +292,59 @@ mod tests { assert!(e.contains("placeholder name"), "{e}"); } + #[test] + fn a_policy_past_the_limits_is_refused_with_the_reason() { + let rules = |n: usize, pattern: &str| -> Vec { + (0..n) + .map(|i| json!({"name": format!("r{i}"), "pattern": pattern})) + .collect() + }; + // At the limit: fine. + for (g, n) in [ + (Guard::Content, 500), + (Guard::Redact, 100), + (Guard::InspectTools, 100), + ] { + assert_eq!( + validate(g, &json!({"custom": rules(n, "x")})), + Ok(()), + "{g}" + ); + let e = validate(g, &json!({"custom": rules(n + 1, "x")})).unwrap_err(); + assert!(e.contains(&format!("at most {n}")), "{g}: {e}"); + } + for (g, n) in [ + (Guard::Content, 500), + (Guard::Redact, 1000), + (Guard::InspectTools, 1000), + ] { + let long = "a".repeat(n); + assert_eq!( + validate(g, &json!({"custom": rules(1, &long)})), + Ok(()), + "{g}" + ); + let longer = "a".repeat(n + 1); + let e = validate(g, &json!({"custom": rules(1, &longer)})).unwrap_err(); + assert!( + e.contains(&format!("{} characters long", n + 1)), + "{g}: {e}" + ); + } + // Characters, not bytes. + let cjk = "字".repeat(500); + assert_eq!( + validate(Guard::Content, &json!({"custom": rules(1, &cjk)})), + Ok(()) + ); + let e = validate( + Guard::InspectTools, + &json!({"custom": [{"name": "curl-pipe-sh", "pattern": "curl"}]}), + ) + .unwrap_err(); + assert!(e.contains("name of a built-in rule"), "{e}"); + } + #[test] fn a_custom_rule_that_does_not_compile_is_dropped_and_the_rest_run() { let content: ContentPolicy = serde_json::from_value(json!({"custom": [ diff --git a/crates/gateway/src/content_filter.rs b/crates/gateway/src/content_filter.rs index 550ad721..1fc06386 100644 --- a/crates/gateway/src/content_filter.rs +++ b/crates/gateway/src/content_filter.rs @@ -85,18 +85,45 @@ pub fn refusal(hit: &ScreenHit, mask: &Redaction) -> GatewayError { GatewayError::PolicyBlocked(message) } -/// Record every hit of a screening: one audit event each +/// At most this many content filter events per request: the refusing hit +/// first, then what was stripped, then what was only recorded. +pub const RULE_EVENTS_MAX: usize = 20; + +/// The hits of a screening in the order their events are written, the +/// ones that changed the request first. +pub fn by_weight(screening: &Screening) -> Vec<&ScreenHit> { + let weight = |o: Outcome| match o { + Outcome::Blocked => 0, + Outcome::Stripped => 1, + Outcome::Recorded => 2, + }; + let mut hits: Vec<&ScreenHit> = screening.hits.iter().collect(); + hits.sort_by_key(|h| weight(h.outcome)); + hits +} + +/// Record the hits of a screening: an audit event per rule /// (`gateway.content_flagged`, `gateway.content_stripped` or -/// `gateway.content_blocked`), a counter, and a log line that carries no -/// text of the caller's. The event's excerpt and revealed text are masked. -pub fn record(audit: &AuditLogger, caller: &Caller, screening: &Screening, mask: &Redaction) { - for s in &screening.hits { +/// `gateway.content_blocked`), at most [`RULE_EVENTS_MAX`] of them, a +/// counter, and a log line. +/// +/// **No text of the request is written**, not even a masked excerpt: the +/// matched text is part of the request body, and the body is for those who +/// may read bodies (`logs:read_bodies`), not for everyone who reads the +/// audit log. An event says which rule, what became of the request, how +/// many matches and whether they were in a tool result. +pub fn record(audit: &AuditLogger, caller: &Caller, screening: &Screening) { + let hits = by_weight(screening); + if hits.len() > RULE_EVENTS_MAX { + tracing::warn!( + trace_id = %caller.trace_id, + rules = hits.len(), + written = RULE_EVENTS_MAX, + "more content rules matched one request than are written to the audit log" + ); + } + for (i, s) in hits.iter().enumerate() { let h = &s.hit; - let action = match s.outcome { - Outcome::Recorded => "gateway.content_flagged", - Outcome::Stripped => "gateway.content_stripped", - Outcome::Blocked => "gateway.content_blocked", - }; tracing::info!( trace_id = %caller.trace_id, rule = %h.rule, @@ -111,7 +138,15 @@ pub fn record(audit: &AuditLogger, caller: &Caller, screening: &Screening, mask: "custom" => if h.custom { "true" } else { "false" }, ) .increment(1); - let mut detail = serde_json::json!({ + if i >= RULE_EVENTS_MAX { + continue; + } + let action = match s.outcome { + Outcome::Recorded => "gateway.content_flagged", + Outcome::Stripped => "gateway.content_stripped", + Outcome::Blocked => "gateway.content_blocked", + }; + audit.log(caller.audit(action).detail(serde_json::json!({ "trace_id": caller.trace_id, "model": caller.model, "rule": h.rule, @@ -121,12 +156,8 @@ pub fn record(audit: &AuditLogger, caller: &Caller, screening: &Screening, mask: "outcome": s.outcome.slug(), "in_tool_result": h.in_tool_result, "count": h.count, - "excerpt": mask.mask(&h.snippet), - }); - if !h.revealed.is_empty() { - detail["revealed"] = serde_json::Value::String(mask.mask(&h.revealed)); - } - audit.log(caller.audit(action).detail(detail)); + "rules_in_request": hits.len(), + }))); } } @@ -273,6 +304,41 @@ mod tests { assert_eq!(body["messages"][1]["content"], "jailbreak"); } + #[test] + fn the_events_put_what_changed_the_request_first() { + let f = filter( + Mode::Enforce, + vec![ + rule( + "Rec", + "alpha", + ContentMatch::Contains, + ContentAction::Record, + ), + rule( + "Strip", + "beta", + ContentMatch::Contains, + ContentAction::Strip, + ), + rule( + "Block", + "gamma", + ContentMatch::Contains, + ContentAction::Block, + ), + ], + ); + let order = |text: &str| -> Vec { + by_weight(&f.screen(Dialect::Chat, &chat(text))) + .iter() + .map(|h| h.hit.rule.clone()) + .collect() + }; + assert_eq!(order("alpha beta gamma"), ["Block", "Rec", "Strip"]); + assert_eq!(order("alpha beta"), ["Strip", "Rec"]); + } + #[test] fn a_refusal_quoting_a_credential_masks_it() { let key = "sk-ant-api03-AAAAAAAAAAAAAAAAAAAAAAAAAAAA"; diff --git a/crates/gateway/src/guards.rs b/crates/gateway/src/guards.rs index 3018d7ca..dbe1f41a 100644 --- a/crates/gateway/src/guards.rs +++ b/crates/gateway/src/guards.rs @@ -15,10 +15,15 @@ //! | outbound redaction | `gateway.redaction_flagged` (observe), `gateway.redaction_replaced` (enforce) | //! | tool-call inspection | `gateway.tool_call_flagged` (recorded), `gateway.tool_call_blocked` (cut) | //! -//! **What an event quotes is masked first**, with the outbound redaction -//! rules ([`crate::redaction::Redaction::mask`]): an excerpt of a tool call -//! or of the caller's text can carry a credential, and the audit log is -//! forwarded and read far more widely than the request it came from. +//! **The audit log gets no text of the request.** It is forwarded and read +//! far more widely than the request it came from (`logs:read_all`, against +//! `logs:read_bodies` for bodies). A content event names the rule and +//! counts the matches; a redaction event names the rule and counts the +//! values, and only a built-in rule's carries a few, masked +//! (`sk-an…7f9c`); a tool-call event quotes the arguments that matched — +//! the upstream's text, not the caller's — masked with the redaction rules +//! ([`crate::redaction::Redaction::mask`]). A request writes at most a +//! score of events per guard, one per rule. use std::sync::Arc; diff --git a/crates/gateway/src/proxy/generate.rs b/crates/gateway/src/proxy/generate.rs index 42214535..e391ece7 100644 --- a/crates/gateway/src/proxy/generate.rs +++ b/crates/gateway/src/proxy/generate.rs @@ -215,9 +215,9 @@ pub(crate) struct Outbound { /// The path the caller called. A Gemini request's model and action /// are in it. pub path: String, - /// What the caller sent — after the content filter, with the model's - /// output cap applied and, in enforce mode, redacted values swapped for - /// placeholders — otherwise exactly as sent. + /// What the caller sent — after the content filter and, in enforce + /// mode, with redacted values swapped for placeholders — otherwise + /// exactly as sent. pub body: Value, pub stream: bool, /// The caller's headers that belong to its format — `anthropic-beta` @@ -234,6 +234,9 @@ pub(crate) struct Outbound { pub redaction: Redaction, /// The placeholders the caller's request was numbered with. pub ledger: Ledger, + /// The model's output cap (`models.max_output_tokens`), applied to each + /// hop as it is addressed (see [`Outbound::cap_output`]). + pub max_output_tokens: Option, } /// The request as it goes out to one upstream, and what it takes to read @@ -269,6 +272,34 @@ impl Outbound { && self.body.pointer("/stream_options/include_usage") != Some(&Value::Bool(true)) } + /// The model's output cap, on what one hop sends: `body`, in `dialect`, + /// to `model`. + /// + /// A limit the request carries (the caller's, or the one a conversion + /// to Anthropic writes) is lowered to the cap. One it does not carry is + /// filled in only when the cap is no more than what the gateway knows + /// the model's family to take (`fallback_max_output_tokens`: 32,000 for + /// Claude, 8,192 otherwise). Above that, a filled-in limit could be one + /// the model refuses outright, and the model's own limit applies + /// instead. + /// + /// `official` is whether the hop goes to the vendor's own endpoint, the + /// same flag the conversion gets: a Chat request with no limit is given + /// `max_completion_tokens` there (OpenAI's reasoning models refuse + /// `max_tokens`) and `max_tokens` elsewhere, where compatible servers + /// mostly read only that. + fn cap_output(&self, dialect: Dialect, body: &mut Value, model: &str, official: bool) { + let Some(cap) = self.max_output_tokens.map(u64::from) else { + return; + }; + if tw_dialect::params::max_output_tokens(dialect, body).is_none() + && cap > tw_dialect::official::fallback_max_output_tokens(model) + { + return; + } + tw_dialect::params::cap_max_output_tokens(dialect, body, cap, official); + } + /// Address the request to `protocol`, naming `model` upstream. pub(crate) fn address( &self, @@ -325,6 +356,7 @@ impl Outbound { opts["include_usage"] = Value::Bool(true); } } + self.cap_output(client, &mut body, model, official); let collect = decode(&body)?.encode(&target(client)).session; let mut bytes = serde_json::to_vec(&body).unwrap_or_default(); // Reasoning signatures a conversion wrote earlier in this @@ -358,9 +390,16 @@ impl Outbound { "Fields the upstream's format cannot carry were left out" ); } + let mut body = prepared.body; + if self.max_output_tokens.is_some() + && let Ok(mut v) = serde_json::from_slice::(&body) + { + self.cap_output(protocol.dialect(), &mut v, model, official); + body = serde_json::to_vec(&v).unwrap_or(body); + } // The conversion moved the placeholders along with the text; one it // assembled from two pieces is numbered here. - let (body, ledger) = self.redaction.replace(prepared.body, &self.ledger); + let (body, ledger) = self.redaction.replace(body, &self.ledger); Ok(Wire { body, path: prepared.path, @@ -599,10 +638,10 @@ async fn run( let caller = Caller::of(&identity, &metadata.request_id, &mapped_model); // 3. Content filter, on the caller's text where its own format puts - // it. Every hit is an audit event; a refusal is the caller's 403, - // quoting their words (masked). + // it. Every hit is an audit event (without the text); a refusal is + // the caller's 403, quoting their words (masked). let screening = guards.content.screen(surface.dialect, &body); - crate::content_filter::record(&state.audit, &caller, &screening, &guards.redaction); + crate::content_filter::record(&state.audit, &caller, &screening); if let Some(hit) = screening.refusal() { return Err(ctx .emit(crate::content_filter::refusal(hit, &guards.redaction)) @@ -639,7 +678,7 @@ async fn run( let (findings, ledger) = guards.redaction.look(&body); crate::redaction::record(&state.audit, &caller, guards.redaction.mode, &findings); let request_for_audit = body.to_vec(); - let mut outbound_body = if ledger.is_empty() { + let outbound_body = if ledger.is_empty() { raw } else { let (replaced, _) = guards.redaction.replace(body.to_vec(), &ledger); @@ -650,21 +689,15 @@ async fn run( })? }; - // 6. The model's output cap: a caller asking for more is lowered to - // it, one asking for nothing gets it. The upstream stops there by - // itself; the answer is not measured. - if let Some(cap) = state + // 6. The model's output cap. Applied to each hop as it is addressed + // (`Outbound::cap_output`): whether to fill one in depends on the + // upstream model, and the field on the upstream's format. The + // upstream stops there by itself; the answer is not measured. + let max_output_tokens = state .router .load() .config_for(&mapped_model) - .max_output_tokens - { - tw_dialect::params::cap_max_output_tokens( - surface.dialect, - &mut outbound_body, - u64::from(cap), - ); - } + .max_output_tokens; let call_ctx = CallCtx::new( Some(trace_id.clone()), @@ -691,7 +724,14 @@ async fn run( // deterministic prompt amortises one upstream call across an // unbounded quota window. let cache_fingerprint = if surface.caches { - ResponseCache::fingerprint(&outbound_body) + ResponseCache::fingerprint(&outbound_body).map(|mut fp| { + // The cap is applied after this, per hop: an answer made under + // one cap must not be served under another. + if let Some(cap) = max_output_tokens { + fp.extend_from_slice(format!("\nmax_output_tokens={cap}").as_bytes()); + } + fp + }) } else { None }; @@ -793,6 +833,7 @@ async fn run( input_estimate, redaction: guards.redaction.clone(), ledger: ledger.clone(), + max_output_tokens, }; let snapshot = |route: &RouteEntry, sel_record| crate::lifecycle::ChatPostInvokeDeps { state: state.clone(), @@ -1020,4 +1061,62 @@ mod tests { assert_eq!(gemini_target("/v1beta/models/g:countTokens"), None); assert_eq!(gemini_target("/v1beta/models/:generateContent"), None); } + + /// The body a Chat caller's request goes out with to a Chat upstream, + /// under a model cap of `cap`. + fn sent_to_chat(ask: Value, cap: u32, official: bool) -> Value { + let redaction = Redaction::new(&tw_guard::policy::RedactPolicy { + mode: tw_guard::policy::Mode::Off, + ..Default::default() + }); + let (_, ledger) = redaction.look(b"{}"); + let outbound = Outbound { + surface: CHAT, + path: "/v1/chat/completions".into(), + body: ask, + stream: false, + dialect_headers: Vec::new(), + input_estimate: 0, + redaction, + ledger, + max_output_tokens: Some(cap), + }; + let wire = outbound + .address(UpstreamProtocol::OpenAiChat, "gpt-5", official) + .unwrap_or_else(|e| panic!("{e:?}")); + serde_json::from_slice(&wire.body).unwrap() + } + + #[test] + fn a_chat_request_without_a_limit_gets_the_field_its_endpoint_reads() { + let ask = serde_json::json!({ + "model": "gpt-5", + "messages": [{"role": "user", "content": "ping"}] + }); + // OpenAI's own endpoint: its reasoning models refuse `max_tokens`. + let sent = sent_to_chat(ask.clone(), 4096, true); + assert_eq!(sent["max_completion_tokens"], 4096, "{sent}"); + assert!(sent.get("max_tokens").is_none(), "{sent}"); + // Anywhere else: compatible servers mostly read only `max_tokens`. + let sent = sent_to_chat(ask, 4096, false); + assert_eq!(sent["max_tokens"], 4096, "{sent}"); + assert!(sent.get("max_completion_tokens").is_none(), "{sent}"); + } + + #[test] + fn both_chat_limits_are_held_to_the_cap() { + // An upstream that reads only `max_tokens` would otherwise go + // uncapped; the smaller one the caller wrote is kept. + let ask = serde_json::json!({ + "model": "gpt-5", + "max_tokens": 99_999, + "max_completion_tokens": 100, + "messages": [{"role": "user", "content": "ping"}] + }); + for official in [true, false] { + let sent = sent_to_chat(ask.clone(), 4096, official); + assert_eq!(sent["max_tokens"], 4096, "{sent}"); + assert_eq!(sent["max_completion_tokens"], 100, "{sent}"); + } + } } diff --git a/crates/gateway/src/redaction.rs b/crates/gateway/src/redaction.rs index 7c708c9e..50a1b7a3 100644 --- a/crates/gateway/src/redaction.rs +++ b/crates/gateway/src/redaction.rs @@ -27,7 +27,7 @@ use std::sync::Arc; use think_watch_common::audit::AuditLogger; use tw_guard::policy::{Mode, RedactPolicy}; use tw_guard::redact::replace::Ledger; -use tw_guard::redact::rules::{Finding, RuleSet}; +use tw_guard::redact::rules::{Finding, Rule, RuleSet}; use crate::guards::Caller; @@ -118,43 +118,112 @@ pub fn restore_body(ledger: &Ledger, body: &[u8]) -> Vec { } } -/// Record what was found in a request: one audit event per value +/// At most this many redaction events per request: one per rule, the rules +/// that found the most first. A request carrying thousands of values writes +/// a handful of rows, not thousands. +pub const RULE_EVENTS_MAX: usize = 20; + +/// A built-in rule's event lists at most this many of the values it found, +/// masked. +pub const MASKED_MAX: usize = 5; + +/// What one rule found in one request. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RuleHits { + pub rule: Rule, + /// How many different values. + pub values: usize, + /// How many times, all values together. + pub count: u64, + /// The first few values in their masked form (`sk-an…7f9c`, `…1234`) — + /// built-in rules only. **A custom rule's values are not written at + /// all**: masking keeps the first five and last four characters, and of + /// a phone number or an IP address that is nearly all of it. + pub masked: Vec, +} + +/// The findings of one request, one entry per rule, the rule with the most +/// occurrences first (ties in the order found). +pub fn per_rule(findings: &[Finding]) -> Vec { + let mut out: Vec = Vec::new(); + for f in findings { + let at = match out.iter().position(|r| r.rule == f.rule) { + Some(at) => at, + None => { + out.push(RuleHits { + rule: f.rule.clone(), + values: 0, + count: 0, + masked: Vec::new(), + }); + out.len() - 1 + } + }; + let r = &mut out[at]; + r.values += 1; + r.count += f.count; + if !f.rule.custom() && r.masked.len() < MASKED_MAX { + r.masked.push(f.masked.clone()); + } + } + out.sort_by_key(|r| std::cmp::Reverse(r.count)); + out +} + +/// Record what was found in a request: an audit event per rule /// (`gateway.redaction_replaced` in enforce mode, `gateway.redaction_flagged` -/// in observe mode) and a counter. The value itself is never written — -/// only its masked form. +/// in observe mode), at most [`RULE_EVENTS_MAX`] of them, and a counter. +/// A value is never written as it is: a built-in rule's event carries a +/// few in masked form, a custom rule's only how many there were. pub fn record(audit: &AuditLogger, caller: &Caller, mode: Mode, findings: &[Finding]) { if findings.is_empty() { return; } - let replaced = mode.acts(); - let (action, outcome) = if replaced { + let (action, outcome) = if mode.acts() { ("gateway.redaction_replaced", "replaced") } else { ("gateway.redaction_flagged", "recorded") }; + let rules = per_rule(findings); tracing::info!( trace_id = %caller.trace_id, - found = findings.len(), + rules = rules.len(), + values = findings.len(), outcome, "outbound redaction found values in the request" ); - for f in findings { + if rules.len() > RULE_EVENTS_MAX { + tracing::warn!( + trace_id = %caller.trace_id, + rules = rules.len(), + written = RULE_EVENTS_MAX, + "more redaction rules matched one request than are written to the audit log" + ); + } + for r in &rules { metrics::counter!( "gateway_redaction_found_total", - "kind" => f.rule.kind().slug(), + "kind" => r.rule.kind().slug(), "outcome" => outcome, ) - .increment(1); - audit.log(caller.audit(action).detail(serde_json::json!({ + .increment(r.values as u64); + } + for r in rules.iter().take(RULE_EVENTS_MAX) { + let mut detail = serde_json::json!({ "trace_id": caller.trace_id, "model": caller.model, - "rule": f.rule.id(), - "custom": f.rule.custom(), - "kind": f.rule.kind().slug(), - "masked": f.masked, - "count": f.count, + "rule": r.rule.id(), + "custom": r.rule.custom(), + "kind": r.rule.kind().slug(), + "values": r.values, + "count": r.count, "outcome": outcome, - }))); + "rules_in_request": rules.len(), + }); + if !r.rule.custom() { + detail["masked"] = serde_json::json!(r.masked); + } + audit.log(caller.audit(action).detail(detail)); } } @@ -245,6 +314,62 @@ mod tests { ); } + #[test] + fn findings_are_one_entry_per_rule_and_a_custom_rule_keeps_no_values() { + let r = Redaction::new(&RedactPolicy { + mode: Mode::Enforce, + custom: vec![tw_guard::policy::CustomRedactRule { + name: "ssn".into(), + pattern: r"\d{3}-\d{2}-\d{4}".into(), + label: None, + disabled: false, + }], + ..Default::default() + }); + let other = "sk-ant-api03-BBBBBBBBBBBBBBBBBBBBBBBBBBBB"; + let body = serde_json::json!({"messages": [{"role": "user", "content": format!( + "{KEY} {other} {KEY} 123-45-6789 987-65-4321 123-45-6789 111-22-3333" + )}]}) + .to_string(); + let (found, _) = r.look(body.as_bytes()); + let rules = per_rule(&found); + assert_eq!(rules.len(), 2, "{rules:?}"); + // The rule with the most occurrences first. + assert_eq!(rules[0].rule.id(), "ssn"); + assert_eq!((rules[0].values, rules[0].count), (3, 4)); + assert!( + rules[0].masked.is_empty(), + "a custom rule's values are not kept" + ); + assert_eq!(rules[1].rule.id(), "anthropic-api-key"); + assert_eq!((rules[1].values, rules[1].count), (2, 3)); + assert_eq!(rules[1].masked.len(), 2); + assert!( + rules[1] + .masked + .iter() + .all(|m| m.starts_with("sk-an") && !m.contains(KEY) && !m.contains(other)) + ); + } + + #[test] + fn a_built_in_rule_lists_only_a_few_masked_values() { + let r = Redaction::new(&RedactPolicy { + mode: Mode::Observe, + ..Default::default() + }); + let keys: Vec = (0..12) + .map(|i| format!("sk-ant-api03-{}", format!("{i:02}").repeat(14))) + .collect(); + let body = serde_json::json!({"messages": [{"role": "user", "content": keys.join(" ")}]}) + .to_string(); + let (found, _) = r.look(body.as_bytes()); + let rules = per_rule(&found); + assert_eq!(rules.len(), 1); + assert_eq!(rules[0].values, 12); + assert_eq!(rules[0].masked.len(), MASKED_MAX); + } + #[test] fn a_captured_body_is_redacted_for_storage() { let r = redaction(Mode::Observe); diff --git a/crates/server/src/handlers/admin/security.rs b/crates/server/src/handlers/admin/security.rs index 8c693287..d45c0495 100644 --- a/crates/server/src/handlers/admin/security.rs +++ b/crates/server/src/handlers/admin/security.rs @@ -7,9 +7,11 @@ //! policy from it and writes it back through `PATCH /api/admin/settings` //! (one `security.*` key per guard, see `think_watch_common::guard_policy`). //! -//! Permissions are the ones these features always had: `pii_redactor:*` -//! for outbound redaction, `content_filter:*` for the content filter and -//! tool-call inspection. Reading the policies is reading settings. +//! Permissions are the ones these features always had: trying a sample +//! takes `pii_redactor:read` for outbound redaction and +//! `content_filter:read` for the content filter and tool-call inspection; +//! changing a policy takes `settings:write` and the guard's `:write`. +//! Reading the policies is reading settings. use axum::Json; use axum::extract::{Path, State}; @@ -22,7 +24,8 @@ use think_watch_common::errors::AppError; use crate::app::AppState; use crate::middleware::auth_guard::AuthUser; -/// The permission that changes a guard's policy. +/// The permission that changes a guard's policy, on top of `settings:write` +/// (see `settings::update_settings`). pub(crate) fn write_permission(guard: Guard) -> &'static str { match guard { Guard::Redact => "pii_redactor:write", diff --git a/crates/server/src/handlers/admin/settings.rs b/crates/server/src/handlers/admin/settings.rs index affcb72c..d73fa480 100644 --- a/crates/server/src/handlers/admin/settings.rs +++ b/crates/server/src/handlers/admin/settings.rs @@ -245,11 +245,11 @@ pub struct UpdateSettingsRequest { /// PATCH /api/admin/settings — update one or more settings. /// -/// Changing a setting takes `settings:write`, except a request guard's -/// policy (`security.redact`, `security.inspect_tools`, -/// `security.content`), which takes the permission that guard always had: -/// `pii_redactor:write` for redaction, `content_filter:write` for the other -/// two. +/// Changing a setting takes `settings:write`. A request guard's policy +/// (`security.redact`, `security.inspect_tools`, `security.content`) takes +/// the guard's own permission as well — `pii_redactor:write` for redaction, +/// `content_filter:write` for the other two — the two checks it always +/// had: the server's, and the console's. #[utoipa::path( patch, path = "/api/admin/settings", @@ -267,17 +267,14 @@ pub async fn update_settings( State(state): State, Json(req): Json, ) -> Result, AppError> { - let mut permissions: Vec<&'static str> = req - .settings - .keys() - .map(|key| match guard_policy::guard_of(key) { - Some(guard) => super::security::write_permission(guard), - None => "settings:write", - }) + let mut permissions: Vec<&'static str> = std::iter::once("settings:write") + .chain( + req.settings + .keys() + .filter_map(|key| guard_policy::guard_of(key)) + .map(super::security::write_permission), + ) .collect(); - if permissions.is_empty() { - permissions.push("settings:write"); - } permissions.sort_unstable(); permissions.dedup(); for permission in permissions { diff --git a/crates/server/src/handlers/models.rs b/crates/server/src/handlers/models.rs index 8ee354a1..dc1da4a6 100644 --- a/crates/server/src/handlers/models.rs +++ b/crates/server/src/handlers/models.rs @@ -124,6 +124,10 @@ pub struct CreateModelRequest { /// for nothing gets it. NULL/missing = no limit. #[serde(default)] pub max_output_tokens: Option, + /// Removed: see [`refuse_output_guardrails`]. + #[serde(default)] + #[schema(value_type = Option>, deprecated)] + pub output_guardrails: Option, } #[utoipa::path( @@ -169,6 +173,7 @@ pub async fn create_model( req.affinity_mode.as_deref(), req.affinity_ttl_secs, )?; + refuse_output_guardrails(req.output_guardrails.as_ref())?; let max_output_tokens = max_output_tokens(req.max_output_tokens)?; let model = repo::insert( @@ -249,6 +254,26 @@ pub struct UpdateModelRequest { #[serde(default, deserialize_with = "deserialize_some")] #[schema(value_type = Option)] pub max_output_tokens: Option>, + /// Removed: see [`refuse_output_guardrails`]. + #[serde(default)] + #[schema(value_type = Option>, deprecated)] + pub output_guardrails: Option, +} + +/// `output_guardrails`, the length cap measured on the answer, is gone +/// (`max_output_tokens` caps the request instead). A client still sending +/// one is refused rather than ignored: ignored, it would believe answers +/// are still capped. An empty list or `null` asks for nothing and passes. +pub(crate) fn refuse_output_guardrails(value: Option<&Value>) -> Result<(), AppError> { + match value { + None | Some(Value::Null) => Ok(()), + Some(Value::Array(rules)) if rules.is_empty() => Ok(()), + Some(_) => Err(AppError::BadRequest( + "output_guardrails was removed: answers are no longer measured. Set \ + max_output_tokens instead (a cap of N bytes is about N / 4 tokens)." + .into(), + )), + } } /// A model's output-token cap as stored: 1 to 2147483647 (the column is a @@ -372,6 +397,7 @@ pub async fn update_model( None => existing.tags.clone(), Some(inner) => inner.clone(), }; + refuse_output_guardrails(req.output_guardrails.as_ref())?; let new_max_output_tokens: Option = match req.max_output_tokens { None => existing.max_output_tokens, Some(inner) => max_output_tokens(inner)?, @@ -1598,6 +1624,18 @@ mod tests { } } + #[test] + fn a_length_cap_is_refused_not_ignored() { + assert!(refuse_output_guardrails(None).is_ok()); + assert!(refuse_output_guardrails(Some(&Value::Null)).is_ok()); + assert!(refuse_output_guardrails(Some(&serde_json::json!([]))).is_ok()); + let e = refuse_output_guardrails(Some( + &serde_json::json!([{"type": "max_length", "max_chars": 4096}]), + )) + .unwrap_err(); + assert!(e.to_string().contains("max_output_tokens"), "{e}"); + } + #[test] fn a_patch_tells_absent_from_null() { let absent: UpdateModelRequest = serde_json::from_value(serde_json::json!({})).unwrap(); diff --git a/crates/test-support/tests/content_filter_pii.rs b/crates/test-support/tests/content_filter_pii.rs index 564ee158..71e083bb 100644 --- a/crates/test-support/tests/content_filter_pii.rs +++ b/crates/test-support/tests/content_filter_pii.rs @@ -13,8 +13,10 @@ //! request. In enforce mode the upstream sees `<>` placeholders //! and the caller gets the values back in the answer. //! -//! Both policies are thinkwatch-core's shape (`tw_guard::policy`); every -//! hit is an audit event, its excerpt masked. +//! Both policies are thinkwatch-core's shape (`tw_guard::policy`). Every +//! rule that matches writes an audit event — without the request's text: a +//! content event names the rule and counts the matches, a redaction event +//! counts the values and, for a built-in rule only, lists a few masked. use serde_json::Value; use think_watch_test_support::prelude::*; @@ -313,18 +315,18 @@ async fn observe_is_the_default_records_the_hit_and_changes_nothing() { assert_eq!(e["rule"], "ignore-previous-instructions", "{e}"); assert_eq!(e["action"], "block", "what enforce mode would do"); assert_eq!(e["outcome"], "recorded"); + assert_eq!(e["count"], 1); + // The request's text is not the audit log's. assert!( - e["excerpt"] - .as_str() - .unwrap() - .contains("ignore previous instructions"), + e.get("excerpt").is_none() && e.get("revealed").is_none(), "{e}" ); + assert!(!e.to_string().contains("write a poem"), "{e}"); } #[ignore = "integration test — run via `make test-it`"] #[tokio::test] -async fn a_refusal_quoting_a_credential_masks_it_in_the_answer_and_the_audit_log() { +async fn a_refusal_quoting_a_credential_masks_it_and_the_audit_log_quotes_nothing() { let app = TestApp::spawn_with_clickhouse().await; app.set_setting( "security.content", @@ -344,10 +346,13 @@ async fn a_refusal_quoting_a_credential_masks_it_in_the_answer_and_the_audit_log .await; assert_eq!(status, 403, "{text}"); assert!(!text.contains(KEY), "{text}"); + // The caller is told what matched, in their own words, masked. + assert!(text.contains("here is my key sk-an…"), "{text}"); let events = audited(&app, &user_id, "gateway.content_blocked").await; - let excerpt = events[0]["excerpt"].as_str().unwrap(); - assert!(!excerpt.contains(KEY), "{excerpt}"); - assert!(excerpt.contains("sk-an…"), "{excerpt}"); + let e = &events[0]; + assert_eq!(e["rule"], "Keys", "{e}"); + assert!(e.get("excerpt").is_none(), "{e}"); + assert!(!e.to_string().contains("sk-an"), "{e}"); } // ---------------------------------------------------------------- redaction @@ -418,10 +423,89 @@ async fn redaction_observes_by_default_and_records_the_masked_value() { let e = &events[0]; assert_eq!(e["rule"], "anthropic-api-key", "{e}"); assert_eq!(e["outcome"], "recorded"); - let masked = e["masked"].as_str().unwrap(); + assert_eq!( + (e["values"].as_u64(), e["count"].as_u64()), + (Some(1), Some(1)) + ); + // A built-in rule's event keeps the masked form core gives it. + let masked = e["masked"][0].as_str().unwrap(); assert!(!masked.contains(KEY) && masked.starts_with("sk-an"), "{e}"); } +#[ignore = "integration test — run via `make test-it`"] +#[tokio::test] +async fn a_custom_redaction_rule_records_one_event_with_counts_and_no_values() { + let app = TestApp::spawn_with_clickhouse().await; + app.set_setting( + "security.redact", + json!({"custom": [{"name": "ssn", "pattern": "\\d{3}-\\d{2}-\\d{4}"}]}), + ) + .await; + let upstream = echo_upstream().await; + let (key, user_id) = seed_route(&app, &upstream.uri(), "redact-custom").await; + let (status, text) = post_as( + &app, + &key, + "/v1/chat/completions", + &json!({"model": "redact-custom", "messages": [ + {"role": "user", "content": "123-45-6789, 987-65-4321, 123-45-6789 and 555-12-3456"} + ]}), + ) + .await; + assert_eq!(status, 200, "{text}"); + + let events = audited(&app, &user_id, "gateway.redaction_flagged").await; + assert_eq!(events.len(), 1, "one event per rule: {events:?}"); + let e = &events[0]; + assert_eq!(e["rule"], "ssn", "{e}"); + assert_eq!(e["custom"], true); + assert_eq!( + (e["values"].as_u64(), e["count"].as_u64()), + (Some(3), Some(4)) + ); + // Masking keeps most of a short number; a custom rule's values are + // not written at all. + assert!(e.get("masked").is_none(), "{e}"); + for digits in ["6789", "4321", "3456"] { + assert!(!e.to_string().contains(digits), "{e}"); + } +} + +#[ignore = "integration test — run via `make test-it`"] +#[tokio::test] +async fn one_request_writes_at_most_twenty_redaction_events() { + let app = TestApp::spawn_with_clickhouse().await; + let custom: Vec = (0..25) + .map(|i| json!({"name": format!("r{i:02}"), "pattern": format!("tok{i:02}x")})) + .collect(); + app.set_setting("security.redact", json!({"custom": custom})) + .await; + let upstream = echo_upstream().await; + let (key, user_id) = seed_route(&app, &upstream.uri(), "redact-many").await; + let said: Vec = (0..25).map(|i| format!("tok{i:02}x")).collect(); + let (status, text) = post_as( + &app, + &key, + "/v1/chat/completions", + &json!({"model": "redact-many", "messages": [ + {"role": "user", "content": said.join(" ")} + ]}), + ) + .await; + assert_eq!(status, 200, "{text}"); + + let first = audited(&app, &user_id, "gateway.redaction_flagged").await; + // The audit pipeline flushes in batches: give the rest time to land. + tokio::time::sleep(std::time::Duration::from_secs(3)).await; + let events = audited(&app, &user_id, "gateway.redaction_flagged").await; + assert!(events.len() >= first.len()); + assert_eq!(events.len(), 20, "{} events", events.len()); + assert!( + events.iter().all(|e| e["rules_in_request"] == 25), + "{events:?}" + ); +} + // ---------------------------------------------------------------- console #[ignore = "integration test — run via `make test-it`"] @@ -627,22 +711,19 @@ async fn saving_a_policy_the_gateway_cannot_run_is_refused() { assert_eq!(r.status.as_u16(), 400, "{}", r.text()); } -#[ignore = "integration test — run via `make test-it`"] -#[tokio::test] -async fn each_guard_is_changed_and_tried_with_the_permission_it_always_had() { - let app = TestApp::spawn().await; +/// A console session for a user holding one custom role that allows +/// exactly `actions`. +async fn session_with(app: &TestApp, actions: &[&str]) -> TestClient { // Short: the seeded user's address is built from it. let role = format!("gd{}", &Uuid::new_v4().simple().to_string()[..8]); sqlx::query( "INSERT INTO rbac_roles (name, description, is_system, policy_document) - VALUES ($1, 'content filter only', FALSE, $2)", + VALUES ($1, 'guard permissions test', FALSE, $2)", ) .bind(&role) - .bind( - json!({"Version": "2024-01-01", "Statement": [{"Sid": "Guards", "Effect": "Allow", - "Action": ["content_filter:read", "content_filter:write", "settings:read"], - "Resource": "*"}]}), - ) + .bind(json!({"Version": "2024-01-01", "Statement": [ + {"Sid": "Test", "Effect": "Allow", "Action": actions, "Resource": "*"} + ]})) .execute(&app.db) .await .unwrap(); @@ -657,30 +738,139 @@ async fn each_guard_is_changed_and_tried_with_the_permission_it_always_had() { .await .unwrap() .assert_ok(); + con +} - con.get("/api/admin/security").await.unwrap().assert_ok(); - for key in ["security.content", "security.inspect_tools"] { - con.patch( - "/api/admin/settings", - json!({"settings": {key: {"mode": "enforce"}}}), - ) +async fn patch_status(con: &TestClient, key: &str, value: Value) -> u16 { + con.patch("/api/admin/settings", json!({"settings": {key: value}})) + .await + .unwrap() + .status + .as_u16() +} + +#[ignore = "integration test — run via `make test-it`"] +#[tokio::test] +async fn each_guard_is_changed_and_tried_with_the_permission_it_always_had() { + let app = TestApp::spawn().await; + + // The guard's own permission is not enough: a policy is a setting. + let guard_only = session_with( + &app, + &[ + "content_filter:read", + "content_filter:write", + "settings:read", + ], + ) + .await; + guard_only + .get("/api/admin/security") .await .unwrap() .assert_ok(); + for key in ["security.content", "security.inspect_tools"] { + assert_eq!( + patch_status(&guard_only, key, json!({"mode": "off"})).await, + 403, + "{key}" + ); } - con.patch( - "/api/admin/settings", - json!({"settings": {"security.redact": {"mode": "enforce"}}}), - ) - .await - .unwrap() - .assert_status(403); - con.post("/api/admin/security/content/test", json!({"sample": "x"})) + // Trying a sample takes only the guard's read permission. + guard_only + .post("/api/admin/security/content/test", json!({"sample": "x"})) .await .unwrap() .assert_ok(); - con.post("/api/admin/security/redact/test", json!({"sample": "x"})) + guard_only + .post("/api/admin/security/redact/test", json!({"sample": "x"})) .await .unwrap() .assert_status(403); + + // Nor is `settings:write` alone: a role made to change settings does + // not get to switch the guards off on upgrade. + let settings_only = session_with(&app, &["settings:read", "settings:write"]).await; + for key in [ + "security.content", + "security.inspect_tools", + "security.redact", + ] { + assert_eq!( + patch_status(&settings_only, key, json!({"mode": "off"})).await, + 403, + "{key}" + ); + } + assert_eq!( + patch_status(&settings_only, "setup.site_name", json!("Renamed")).await, + 200 + ); + + // Both: the content filter and tool-call inspection, not redaction. + let both = session_with( + &app, + &["content_filter:write", "settings:read", "settings:write"], + ) + .await; + for key in ["security.content", "security.inspect_tools"] { + assert_eq!( + patch_status(&both, key, json!({"mode": "enforce"})).await, + 200, + "{key}" + ); + } + assert_eq!( + patch_status(&both, "security.redact", json!({"mode": "enforce"})).await, + 403 + ); + // A request mixing a guard with another setting needs all of it. + let r = both + .patch( + "/api/admin/settings", + json!({"settings": {"security.redact": {}, "setup.site_name": "x"}}), + ) + .await + .unwrap(); + assert_eq!(r.status.as_u16(), 403, "{}", r.text()); +} + +#[ignore = "integration test — run via `make test-it`"] +#[tokio::test] +async fn a_policy_past_the_limits_is_refused() { + let app = TestApp::spawn().await; + let con = admin_session(&app).await; + let custom: Vec = (0..101) + .map(|i| json!({"name": format!("r{i}"), "pattern": format!("p{i}")})) + .collect(); + let r = con + .patch( + "/api/admin/settings", + json!({"settings": {"security.redact": {"custom": custom}}}), + ) + .await + .unwrap(); + assert_eq!(r.status.as_u16(), 400, "{}", r.text()); + assert!(r.text().contains("at most 100"), "{}", r.text()); + let long = "a".repeat(501); + let r = con + .patch( + "/api/admin/settings", + json!({"settings": {"security.content": {"custom": [{"name": "long", "pattern": long}]}}}), + ) + .await + .unwrap(); + assert_eq!(r.status.as_u16(), 400, "{}", r.text()); + assert!(r.text().contains("501 characters"), "{}", r.text()); + let r = con + .patch( + "/api/admin/settings", + json!({"settings": {"security.inspect_tools": {"custom": [ + {"name": "rm-rf-root", "pattern": "rm"} + ]}}}), + ) + .await + .unwrap(); + assert_eq!(r.status.as_u16(), 400, "{}", r.text()); + assert!(r.text().contains("built-in rule"), "{}", r.text()); } diff --git a/crates/test-support/tests/guard_settings_upgrade.rs b/crates/test-support/tests/guard_settings_upgrade.rs index 8c2211c4..796da651 100644 --- a/crates/test-support/tests/guard_settings_upgrade.rs +++ b/crates/test-support/tests/guard_settings_upgrade.rs @@ -4,17 +4,27 @@ //! //! A database is put back the way an upgraded deployment finds it — the //! old keys as the old seeds wrote them, the old `models.output_guardrails` -//! column with a cap in it — and migrated again. The new keys hold the -//! converted policies, the old keys and column are gone, a second run -//! changes nothing, and the gateway behaves as the old settings did. +//! column with a cap in it, and no record of a conversion — and migrated +//! again. The new keys hold the converted policies, the old keys and +//! column are gone, a second run changes nothing, and the gateway behaves +//! as the old settings did. Old settings written back after that (an older +//! version started against the database) are removed, not converted. use serde_json::Value; use think_watch_test_support::prelude::*; -/// What the previous version's `db/seeds.sql` wrote, and a tool-call -/// inspection an operator had changed. -async fn put_back_the_old_settings(app: &TestApp) { - let old = [ +const OLD_KEYS: [&str; 4] = [ + "security.content_filter_patterns", + "security.pii_redactor_patterns", + "security.hidden_text", + "security.tool_inspection", +]; + +/// The content filter and PII lists as the previous version's +/// `db/seeds.sql` wrote them, with `hidden_text` and `tool_inspection` as +/// given. +fn old_settings(hidden_text: Value, tool_inspection: Value) -> Vec<(&'static str, Value)> { + vec![ ( "security.content_filter_patterns", json!([ @@ -41,15 +51,18 @@ async fn put_back_the_old_settings(app: &TestApp) { {"name": "ipv4", "regex": "\\b\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}\\b", "placeholder_prefix": "IP"} ]), ), - ("security.hidden_text", json!("block")), - ( - "security.tool_inspection", - json!({"mode": "enforce", "disabled": ["chmod-777"], "actions": {"rm-rf-root": "cut"}, "custom": []}), - ), - ]; - for (key, value) in old { + ("security.hidden_text", hidden_text), + ("security.tool_inspection", tool_inspection), + ] +} + +async fn write_old_keys(app: &TestApp, settings: Vec<(&'static str, Value)>) { + for (key, value) in settings { fixtures::set_setting(&app.db, key, value).await.unwrap(); } +} + +async fn add_old_column(app: &TestApp) { sqlx::query( "ALTER TABLE models ADD COLUMN output_guardrails JSONB NOT NULL DEFAULT '[]'::jsonb", ) @@ -58,6 +71,24 @@ async fn put_back_the_old_settings(app: &TestApp) { .unwrap(); } +/// The database as an upgraded 2.2 deployment hands it over: the settings +/// it kept, the column it had, and no record of a conversion. +async fn put_back_a_2_2_database(app: &TestApp) { + sqlx::query("DELETE FROM system_settings WHERE key = 'security.legacy_converted'") + .execute(&app.db) + .await + .unwrap(); + write_old_keys( + app, + old_settings( + json!("block"), + json!({"mode": "enforce", "disabled": ["chmod-777"], "actions": {"rm-rf-root": "cut"}, "custom": []}), + ), + ) + .await; + add_old_column(app).await; +} + async fn setting(app: &TestApp, key: &str) -> Option { sqlx::query_scalar("SELECT value FROM system_settings WHERE key = $1") .bind(key) @@ -97,13 +128,17 @@ async fn a_fresh_install_ships_the_factory_policies() { assert_eq!(setting(&app, key).await, None, "{key}"); } assert!(!has_old_column(&app).await); + // Recorded as running the unified settings from the start: nothing + // to convert, now or later. + let marker = setting(&app, "security.legacy_converted").await.unwrap(); + assert_eq!(marker["converted"], json!([]), "{marker}"); } #[ignore = "integration test — run via `make test-it`"] #[tokio::test] async fn the_old_settings_are_converted_once_and_behave_as_before() { let app = TestApp::spawn().await; - put_back_the_old_settings(&app).await; + put_back_a_2_2_database(&app).await; let upstream = MockProvider::openai_chat_ok("upgraded-model").await; let user = fixtures::create_random_user(&app.db).await.unwrap(); let provider = @@ -118,6 +153,16 @@ async fn the_old_settings_are_converted_once_and_behave_as_before() { .execute(&app.db) .await .unwrap(); + // A loose cap, more than the 8,192 tokens the gateway fills in for a + // non-Claude model that is asked for no limit: stored as it converts. + fixtures::create_model_and_route(&app.db, provider.id, "upgraded-big") + .await + .unwrap(); + sqlx::query("UPDATE models SET output_guardrails = $1 WHERE model_id = 'upgraded-big'") + .bind(json!([{"type": "max_length", "max_chars": 100000}])) + .execute(&app.db) + .await + .unwrap(); think_watch_common::db::run_migrations(&app.db) .await @@ -140,6 +185,23 @@ async fn the_old_settings_are_converted_once_and_behave_as_before() { .await .unwrap(); assert_eq!(cap, Some(250), "1000 bytes is 250 tokens"); + let big: Option = + sqlx::query_scalar("SELECT max_output_tokens FROM models WHERE model_id = 'upgraded-big'") + .fetch_one(&app.db) + .await + .unwrap(); + assert_eq!( + big, + Some(25_000), + "100,000 bytes is 25,000 tokens, not lowered" + ); + let marker = setting(&app, "security.legacy_converted").await.unwrap(); + assert_eq!(marker["models_capped"], 2, "{marker}"); + assert_eq!( + marker["converted"].as_array().map(Vec::len), + Some(4), + "{marker}" + ); // ...and the new ones hold what does the same. let content = setting(&app, "security.content").await.unwrap(); @@ -216,3 +278,96 @@ async fn the_old_settings_are_converted_once_and_behave_as_before() { assert_eq!(sent["messages"][0]["content"], "write to <>"); assert_eq!(sent["max_tokens"], 250, "{sent}"); } + +#[ignore = "integration test — run via `make test-it`"] +#[tokio::test] +async fn old_settings_written_back_after_the_conversion_are_removed_not_converted() { + let app = TestApp::spawn().await; + put_back_a_2_2_database(&app).await; + let provider = fixtures::create_provider( + &app.db, + &unique_name("back"), + "openai", + "http://127.0.0.1:9", + None, + ) + .await + .unwrap(); + fixtures::create_model_and_route(&app.db, provider.id, "written-back") + .await + .unwrap(); + sqlx::query("UPDATE models SET output_guardrails = $1 WHERE model_id = 'written-back'") + .bind(json!([{"type": "max_length", "max_chars": 4000}])) + .execute(&app.db) + .await + .unwrap(); + think_watch_common::db::run_migrations(&app.db) + .await + .unwrap(); + + // The operator changes a policy after the upgrade, and a model's cap. + fixtures::set_setting( + &app.db, + "security.content", + json!({"mode": "observe", "enable": ["zero-width"]}), + ) + .await + .unwrap(); + sqlx::query("UPDATE models SET max_output_tokens = 777 WHERE model_id = 'written-back'") + .execute(&app.db) + .await + .unwrap(); + let before: Vec> = futures::future::join_all( + [ + "security.content", + "security.redact", + "security.inspect_tools", + "security.legacy_converted", + ] + .map(|k| setting(&app, k)), + ) + .await; + + // A 2.2 process starts against the database: its seeds write their + // defaults back. Someone puts the old column back too. + write_old_keys( + &app, + old_settings( + json!("warn"), + json!({"mode": "observe", "disabled": [], "actions": {}, "custom": []}), + ), + ) + .await; + add_old_column(&app).await; + sqlx::query("UPDATE models SET output_guardrails = $1 WHERE model_id = 'written-back'") + .bind(json!([{"type": "max_length", "max_chars": 40}])) + .execute(&app.db) + .await + .unwrap(); + + think_watch_common::db::run_migrations(&app.db) + .await + .unwrap(); + + for key in OLD_KEYS { + assert_eq!(setting(&app, key).await, None, "{key} was not removed"); + } + assert!(!has_old_column(&app).await, "the column was not removed"); + let after: Vec> = futures::future::join_all( + [ + "security.content", + "security.redact", + "security.inspect_tools", + "security.legacy_converted", + ] + .map(|k| setting(&app, k)), + ) + .await; + assert_eq!(after, before, "the policies in force changed"); + let cap: Option = + sqlx::query_scalar("SELECT max_output_tokens FROM models WHERE model_id = 'written-back'") + .fetch_one(&app.db) + .await + .unwrap(); + assert_eq!(cap, Some(777), "the cap was converted again"); +} diff --git a/crates/test-support/tests/hidden_text.rs b/crates/test-support/tests/hidden_text.rs index 733c2e6d..f9f156ed 100644 --- a/crates/test-support/tests/hidden_text.rs +++ b/crates/test-support/tests/hidden_text.rs @@ -159,7 +159,7 @@ async fn enforce_strips_them_out_of_the_box() { #[ignore = "integration test — run via `make test-it`"] #[tokio::test] -async fn observe_is_the_default_and_records_what_the_characters_spell() { +async fn observe_is_the_default_and_records_the_hit_without_the_text() { let app = TestApp::spawn_with_clickhouse().await; let upstream = MockProvider::openai_chat_ok("hidden-model").await; let (key, user_id) = seed(&app, &upstream.uri()).await; @@ -190,10 +190,12 @@ async fn observe_is_the_default_and_records_what_the_characters_spell() { assert_eq!(v["outcome"], "recorded", "{v}"); assert_eq!(v["in_tool_result"], true, "{v}"); assert_eq!(v["count"], 6, "{v}"); - // What the tag characters spell, so an operator can judge it. - assert_eq!(v["revealed"], "ignore", "{v}"); - // The excerpt shows them, rather than hiding them again. - assert!(v["excerpt"].as_str().unwrap().contains("U+E0069"), "{v}"); + // What they spell is part of the request, and the request's + // text is not the audit log's. + assert!( + v.get("revealed").is_none() && v.get("excerpt").is_none(), + "{v}" + ); return; } tokio::time::sleep(std::time::Duration::from_millis(50)).await; diff --git a/crates/test-support/tests/max_output_tokens.rs b/crates/test-support/tests/max_output_tokens.rs index 4cf971c3..00cb9a5b 100644 --- a/crates/test-support/tests/max_output_tokens.rs +++ b/crates/test-support/tests/max_output_tokens.rs @@ -2,9 +2,12 @@ //! //! The cap is applied to the request, not measured on the answer: a caller //! asking for more output tokens than the model allows is lowered to the -//! cap, one asking for none gets it, one asking for less keeps its own. The -//! upstream stops there by itself. Each format names the field its own way -//! (`max_tokens`, `max_completion_tokens`, `max_output_tokens`, +//! cap, one asking for less keeps its own, and one asking for none gets the +//! cap — unless the cap is more than the model's family is known to take +//! (8,192 tokens, or 32,000 for Claude), where a filled-in limit could be +//! refused and the model's own limit applies instead. The upstream stops +//! there by itself. Each format names the field its own way (`max_tokens`, +//! `max_completion_tokens`, `max_output_tokens`, //! `generationConfig.maxOutputTokens`), and a request forwarded as sent is //! capped as surely as a converted one. @@ -331,4 +334,87 @@ async fn the_model_api_sets_and_clears_the_cap() { .unwrap(); assert_eq!(r.status.as_u16(), 400, "{bad}: {}", r.text()); } + + // The length cap that this replaced is refused, not silently ignored. + let r = con + .patch( + &format!("/api/admin/models/{id}"), + json!({"output_guardrails": [{"type": "max_length", "max_chars": 4000}]}), + ) + .await + .unwrap(); + assert_eq!(r.status.as_u16(), 400, "{}", r.text()); + assert!(r.text().contains("max_output_tokens"), "{}", r.text()); + let r = con + .post( + "/api/admin/models", + json!({"model_id": unique_name("cap-old"), "display_name": "Old", + "output_guardrails": [{"type": "max_length", "max_chars": 4000}]}), + ) + .await + .unwrap(); + assert_eq!(r.status.as_u16(), 400, "{}", r.text()); + // Asking for none passes. + con.patch( + &format!("/api/admin/models/{id}"), + json!({"output_guardrails": []}), + ) + .await + .unwrap() + .assert_ok(); +} + +#[ignore = "integration test — run via `make test-it`"] +#[tokio::test] +async fn a_cap_above_what_the_family_takes_is_not_filled_in() { + // 25,000 tokens: what a 100,000-byte cap converts to, on a model whose + // family the gateway only knows to take 8,192. + let app = TestApp::spawn().await; + let upstream = chat_upstream().await; + let key = seed(&app, &upstream.uri(), "openai", "cap-loose", Some(25_000)).await; + for (ask, expect) in [ + (None, None), + (Some(30_000), Some(25_000)), + (Some(100), Some(100)), + ] { + let mut body = json!({"model": "cap-loose", "temperature": 0.5, + "messages": [{"role": "user", "content": "ping"}]}); + if let Some(n) = ask { + body["max_tokens"] = json!(n); + } + let before = upstream.received_requests().await.len(); + let (status, text) = post(&app, &key, "/v1/chat/completions", &body).await; + assert_eq!(status, 200, "{ask:?}: {text}"); + let sent: Value = upstream.received_requests().await[before] + .body_json() + .unwrap(); + assert_eq!(chat_max(&sent), expect, "ask={ask:?}: {sent}"); + } +} + +#[ignore = "integration test — run via `make test-it`"] +#[tokio::test] +async fn a_conversion_that_must_write_a_limit_writes_the_lower_one() { + // A Chat caller asking for no limit, routed to Anthropic, which requires + // one: the conversion writes the family's figure, and the cap lowers it. + let app = TestApp::spawn().await; + let upstream = MockProvider::anthropic_messages_ok("cap-claude-conv").await; + let key = seed( + &app, + &upstream.uri(), + "anthropic", + "cap-claude-conv", + Some(CAP), + ) + .await; + let (status, text) = post( + &app, + &key, + "/v1/chat/completions", + &json!({"model": "cap-claude-conv", "messages": [{"role": "user", "content": "ping"}]}), + ) + .await; + assert_eq!(status, 200, "{text}"); + let sent: Value = upstream.received_requests().await[0].body_json().unwrap(); + assert_eq!(sent["max_tokens"], CAP, "{sent}"); } diff --git a/web/src/i18n/en.json b/web/src/i18n/en.json index a8c98743..090bd855 100644 --- a/web/src/i18n/en.json +++ b/web/src/i18n/en.json @@ -791,7 +791,7 @@ "unlimited": "Unlimited", "routingOverrideTitle": "Routing overrides", "routingOverrideHint": "Override the gateway-wide defaults for this model. Leave blank to inherit from Settings → Gateway.", - "maxOutputTokensHint": "The most output tokens a request to this model can ask for: a larger max_tokens is lowered to it, and a request without one gets it. Leave blank for no limit.", + "maxOutputTokensHint": "The most output tokens a request to this model can ask for. A request that sets a limit gets the smaller of the two; one that sets none gets this value if it is within the model family's default (32,000 for Claude, 8,192 for others), and the model's own default otherwise. Leave blank for no limit.", "strategy": { "weighted": "Weighted random", "latency": "Latency-first", diff --git a/web/src/i18n/zh.json b/web/src/i18n/zh.json index 759ebe32..985d25da 100644 --- a/web/src/i18n/zh.json +++ b/web/src/i18n/zh.json @@ -791,7 +791,7 @@ "unlimited": "不限制", "routingOverrideTitle": "路由覆盖", "routingOverrideHint": "针对此模型覆盖网关全局默认。留空则继承「设置 → 网关」中的配置。", - "maxOutputTokensHint": "发往此模型的请求最多输出这么多 token:请求中的 max_tokens 更大时改为此值,未写时按此值发送。留空则不限制。", + "maxOutputTokensHint": "发往此模型的请求最多输出这么多 token。请求写了上限时取两者中较小的;未写时,此值不超过该模型系列的默认上限(Claude 32,000,其他 8,192)则按此值发送,否则按模型默认。留空则不限制。", "strategy": { "weighted": "加权随机", "latency": "延迟优先", diff --git a/web/src/routes/gateway/models/types.ts b/web/src/routes/gateway/models/types.ts index 9f54d7f6..af32b694 100644 --- a/web/src/routes/gateway/models/types.ts +++ b/web/src/routes/gateway/models/types.ts @@ -33,8 +33,9 @@ export interface ModelRow { affinity_mode?: AffinityMode | null; affinity_ttl_secs?: number | null; /// Most output tokens a request to this model may ask for: a larger - /// `max_tokens` is lowered to it, and a request without one gets it. - /// null ⇒ no limit. + /// limit in the request is lowered to it, and a request without one + /// gets it when it is within the model family's default (32,000 for + /// Claude, 8,192 for others). null ⇒ no limit. max_output_tokens?: number | null; } diff --git a/web/src/routes/gateway/security/index.tsx b/web/src/routes/gateway/security/index.tsx index b73dea11..993aadee 100644 --- a/web/src/routes/gateway/security/index.tsx +++ b/web/src/routes/gateway/security/index.tsx @@ -50,6 +50,9 @@ const READ_PERMISSION: Record = { content: 'content_filter:read', }; +/** A policy is saved through the settings endpoint: that takes `settings:write` as well as the guard's own. */ +const mayWrite = (g: Guard) => hasPermission('settings:write') && hasPermission(WRITE_PERMISSION[g]); + type DialogState = | null | { kind: 'custom'; guard: Guard; editing: SecurityRuleView | null; seed?: RuleSeed } @@ -282,7 +285,7 @@ export function GatewaySecurityPage() { @@ -299,7 +302,7 @@ export function GatewaySecurityPage() { taken={(detail?.[dialog.guard].rules ?? []) .filter((r) => r.custom && r.id !== dialog.editing?.id) .map((r) => r.id)} - canWrite={hasPermission(WRITE_PERMISSION[dialog.guard])} + canWrite={mayWrite(dialog.guard)} onClose={() => setDialog(null)} onSave={(input) => saveCustom(dialog.guard, dialog.editing, input)} /> @@ -308,7 +311,7 @@ export function GatewaySecurityPage() { setDialog(null)} onCopy={ hasAction(dialog.guard) && patternOf(dialog.rule)