fix(security): escape untrusted input at Cedar + ClickHouse (ARN-172, ARN-174)#346
fix(security): escape untrusted input at Cedar + ClickHouse (ARN-172, ARN-174)#346rita-aga wants to merge 1 commit into
Conversation
Two related injection fixes at Temper's untrusted-input boundaries; one ADR (docs/adrs/0158-injection-escaping.md). ARN-172 (Cedar policy injection): generate_cedar_from_matrix built Cedar policy text with format!, splicing agent-influenced agent_id/action/resource_type/ resource_id/role/agent_type/session_id (ultimately from spoofable x-temper-* headers) with no escaping. A `"` or `\` could break the generated permit (failing the whole tenant policy reload) or inject a second, broader permit — defeating the human approval gate. Now build UID positions via Cedar's own EntityUid/EntityTypeName (validating and rejecting non-identifier type names) and render condition string literals via Cedar's EntityId::escaped(). The function is now fallible and fails closed; callers propagate/surface the error (hooks.rs, hooks/generate_cedar.rs, api/decisions.rs -> 400, state/policy_suggestions.rs skips the advisory preview). ARN-174 (ClickHouse SQL injection): interpolate_params doubled single quotes only; ClickHouse also honors C-style backslash escapes, so a value ending in `\` escaped the closing quote and let the next param execute as live SQL. Now escape `\` before doubling `'`. Exploit-first tests are red on the prior code (one parsed to two policies — a live broad-permit injection; others hit a parse error that breaks the reload) and green after, plus fail-closed and exact-value round-trip tests. Closes ARN-172, ARN-174. Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
@greptile review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| policies.into_iter().next().unwrap() | ||
| } | ||
|
|
||
| fn expect_resource_eq(policy: &cedar_policy::Policy, type_name: &str, id: &str) { | ||
| use std::str::FromStr; | ||
| let expected = cedar_policy::EntityUid::from_type_name_and_id( | ||
| cedar_policy::EntityTypeName::from_str(type_name).unwrap(), |
There was a problem hiding this comment.
.unwrap() calls in test helpers — parse_single_policy (line 238) and expect_resource_eq (line 244) — may trip the repo's pre-commit integrity check (no TODO/unwrap). The parse_single_policy case is technically safe because assert_eq!(policies.len(), 1) guarantees the next() succeeds, but the hook does textual matching. Both should use .expect(…) with a message to be consistent with the rest of the file and stay clear of the gate.
| policies.into_iter().next().unwrap() | |
| } | |
| fn expect_resource_eq(policy: &cedar_policy::Policy, type_name: &str, id: &str) { | |
| use std::str::FromStr; | |
| let expected = cedar_policy::EntityUid::from_type_name_and_id( | |
| cedar_policy::EntityTypeName::from_str(type_name).unwrap(), | |
| policies.into_iter().next().expect("Vec had exactly 1 element per assert_eq above") | |
| } | |
| fn expect_resource_eq(policy: &cedar_policy::Policy, type_name: &str, id: &str) { | |
| use std::str::FromStr; | |
| let expected = cedar_policy::EntityUid::from_type_name_and_id( | |
| cedar_policy::EntityTypeName::from_str(type_name) | |
| .expect("type_name in test helper must be a valid Cedar identifier"), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-authz/src/policy_gen_test.rs
Line: 238-244
Comment:
Two bare `.unwrap()` calls in test helpers — `parse_single_policy` (line 238) and `expect_resource_eq` (line 244) — may trip the repo's pre-commit integrity check (`no TODO/unwrap`). The `parse_single_policy` case is technically safe because `assert_eq!(policies.len(), 1)` guarantees the `next()` succeeds, but the hook does textual matching. Both should use `.expect(…)` with a message to be consistent with the rest of the file and stay clear of the gate.
```suggestion
policies.into_iter().next().expect("Vec had exactly 1 element per assert_eq above")
}
fn expect_resource_eq(policy: &cedar_policy::Policy, type_name: &str, id: &str) {
use std::str::FromStr;
let expected = cedar_policy::EntityUid::from_type_name_and_id(
cedar_policy::EntityTypeName::from_str(type_name)
.expect("type_name in test helper must be a valid Cedar identifier"),
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
ARN-165 principle audit — request changes and split the PRThis combines two independent security issues (ARN-172 and ARN-174), contrary to the one-issue/one-PR discipline, and neither root boundary is complete. Cedar blocker
Validation must be mandatory inside the authoritative generator/persist boundary, return a typed error, and have release-equivalent negative tests. Call-site validation is defense in depth, not the invariant. ClickHouse blockerThe replacement is still a custom SQL tokenizer/escaper. That preserves a parallel SQL grammar which can drift on comments, string forms, identifiers, escapes, and future dialect features. The durable boundary is driver-side typed parameters, or a deliberately restricted typed query AST whose renderer is the only SQL construction path. Remove the generic interpolation fallback. Please split the Cedar and ClickHouse changes, add adversarial/fuzz coverage to each, and keep both issues open until their independent contracts are proven. |
1 participant
Closes ARN-172 and ARN-174 under epic ARN-165 — two injection fixes with a shared theme (untrusted input into a query/policy language). Draft for review — do not merge.
ARN-172 — Cedar policy injection (
temper-authz/src/policy_gen.rs)generate_cedar_from_matrixinterpolated agent-influenced ids (agent_id/action/resource/role, ultimately from spoofable headers viaPendingDecision::from_denial) straight into Cedar policy text. A"or\could break the tenant policy reload or craft a permit whose meaning ≠ what the human approved — defeating the approval gate.Fix: stop hand-formatting. Emit UIDs via Cedar's own
EntityId::new(v).escaped()(cedar_uid/cedar_type) and string conditions viacedar_string_literal, so every interpolated value is Cedar-escaped by construction. Callers updated to the new API (decisions.rs, pending_decisions.rs, policy_suggestions.rs, hooks).ARN-174 — ClickHouse SQL injection (
temper-observe/src/clickhouse.rs)interpolate_paramsdoubled single quotes but not backslashes, so a value ending in\left the literal unterminated and the next param executed as live SQL.Fix:
s.replace('\\', "\\\\").replace('\'', "''")— escape backslash first, then quote.Red-green
injected_quote_and_backslash_in_agent_and_action_round_trip,injected_backslash_in_resource_id_does_not_break_reload: a"/\in an id now produces a valid, correctly-scoped policy (pre-fix, one exploit input parsed to two policies — a live broad-permit injection).interpolate_escapes_trailing_backslash_so_next_param_stays_quoted: asserts the injection is neutralized ('x\\' AND b = ' OR 1=1 --').Notes
🤖 Generated with Claude Code
Greptile Summary
Fixes two injection vulnerabilities (ARN-172 and ARN-174) that allowed agent-controlled strings to break out of Cedar policy literals and ClickHouse SQL string parameters respectively. The Cedar fix restructures
generate_cedar_from_matrixaround Cedar's own typed constructors (EntityId,EntityTypeName,EntityUid) instead of raw string formatting, and changes the return type toResult<String, String>so invalid type names fail closed. The ClickHouse fix adds backslash escaping (replace('\\', "\\\\")) before the existing single-quote doubling, fixing the case where a value ending in\\left the literal unterminated.cedar_uid,cedar_type, andcedar_string_literalhelpers delegate all escaping to Cedar itself; all five injection-position sites updated; callers indecisions.rs,pending_decisions.rs,policy_suggestions.rs, and both hook modules updated to handle the new fallible return.interpolate_params; backslash must be escaped first so the\\\\introduced for'escaping is not re-escaped; two new regression tests cover the trailing-backslash breakout and the combined\\'case.policy_gen_test.rsuse Cedar's own parser to verify round-trip confinement; one new ClickHouse test asserts the injection is neutralized end-to-end.Confidence Score: 4/5
Both injection vulnerabilities are correctly neutralised; the Cedar and ClickHouse fixes are well-scoped, well-tested, and safe to merge.
The Cedar fix delegates escaping entirely to Cedar's own typed constructors rather than hand-rolling it, which is the right approach. The ClickHouse fix applies the correct ordering (backslash before quote) and is covered by regression tests that assert the exact rendered output. All callers handle the new fallible return correctly. The two observations are non-blocking: the in_string template scanner gap with backslash-escaped quotes is pre-existing with no active exposure in current templates, and the agent/action round-trip assertion gap in the test is a thoroughness point rather than a missing safety check.
crates/temper-observe/src/clickhouse.rs — the in_string scanner behaviour relative to backslash-escaped quotes in template SQL is worth a follow-up; no active exposure today.
Important Files Changed
generate_cedar_from_matrixchanged toResult<String, String>for fail-closed type-name validationinterpolate_params;in_stringtemplate scanner still does not handle\'in template SQL (pre-existing gap noted in comments)generate_policy_from_matrix; returns 400 BAD_REQUEST on invalid type name, correctly fails closedgenerate_cedar_from_matrixcall sites updated; an invalidresource_typenow skips the suggestion with a warning rather than panicking or producing a broken previewgenerate_policy_from_matrixsignature updated to propagateResult; no logic changesmap_erron the falliblegenerate_cedar_from_matrixcall; error surfaces cleanly via?map_errupdate ashooks.rsfor the alternate hook entry pointSequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Agent as Agent (spoofable headers) participant PD as PendingDecision participant DEC as decisions.rs (approve) participant PG as policy_gen.rs participant CH as ClickHouse Agent->>PD: denial via from_denial(action, resource_id, …) PD->>DEC: "generate_policy_from_matrix(&scope)" DEC->>PG: generate_cedar_from_matrix(agent_id, principal_kind, action, resource_type, resource_id, matrix) note over PG: ARN-172 fix PG->>PG: cedar_type(principal_kind) — validates identifier, fails closed on injection PG->>PG: cedar_uid(Action, action) — Cedar escapes action id PG->>PG: cedar_uid(resource_type, resource_id) — Cedar escapes resource id PG->>PG: cedar_string_literal(role/agentType/sessionId) — Cedar escapes conditions PG-->>DEC: Ok(policy_text) or Err(…) alt type name invalid DEC-->>Agent: 400 BAD_REQUEST (fail closed) else policy generated DEC->>DEC: validate + reload tenant policies end note over CH: ARN-174 fix Agent->>CH: query with SqlParam::String(user_value) CH->>CH: interpolate_params: replace backslash then replace quote CH->>CH: $N placeholder replaced only outside quoted literals CH-->>Agent: query result%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Agent as Agent (spoofable headers) participant PD as PendingDecision participant DEC as decisions.rs (approve) participant PG as policy_gen.rs participant CH as ClickHouse Agent->>PD: denial via from_denial(action, resource_id, …) PD->>DEC: "generate_policy_from_matrix(&scope)" DEC->>PG: generate_cedar_from_matrix(agent_id, principal_kind, action, resource_type, resource_id, matrix) note over PG: ARN-172 fix PG->>PG: cedar_type(principal_kind) — validates identifier, fails closed on injection PG->>PG: cedar_uid(Action, action) — Cedar escapes action id PG->>PG: cedar_uid(resource_type, resource_id) — Cedar escapes resource id PG->>PG: cedar_string_literal(role/agentType/sessionId) — Cedar escapes conditions PG-->>DEC: Ok(policy_text) or Err(…) alt type name invalid DEC-->>Agent: 400 BAD_REQUEST (fail closed) else policy generated DEC->>DEC: validate + reload tenant policies end note over CH: ARN-174 fix Agent->>CH: query with SqlParam::String(user_value) CH->>CH: interpolate_params: replace backslash then replace quote CH->>CH: $N placeholder replaced only outside quoted literals CH-->>Agent: query resultComments Outside Diff (2)
crates/temper-observe/src/clickhouse.rs, line 71-84 (link)in_stringscanner doesn't account for\'in template SQLThe
renderfix correctly doubles backslashes so ClickHouse sees a terminated literal. However, the template scanner'sin_stringstate machine only recognises''as an in-string quote escape (line 74) — it does not handle\'. If a programmer-written template ever contains a\'-escaped quote inside a string literal (valid ClickHouse syntax), the scanner would treat the'following\as a string terminator and togglein_stringtofalsetoo early. Any$Nplaceholder appearing after that point would be substituted outside a string context even though ClickHouse's parser is still inside one. This is a pre-existing gap, but the ADR explicitly calls out that ClickHouse honors C-style backslash escapes, making it worth recording as a follow-up. Templates in the current codebase use''-style quoting so there is no active exposure; the scanner could be hardened by tracking\as an escape prefix whenin_stringis true.Prompt To Fix With AI
crates/temper-authz/src/policy_gen_test.rs, line 643-651 (link)expect_resource_eqinjected_quote_and_backslash_in_agent_and_action_round_tripverifies that the generated text parses as exactly one policy and that the resource constraint round-trips correctly — but it does not assert thatbad_agentorbad_actionalso round-trip to their principal/action constraints. Injection containment is proven byparse_single_policy(exactly one policy, no breakout), but addingpolicy.principal_constraint()andpolicy.action_groups()assertions analogous toexpect_resource_eqwould give the same degree of structural proof for the agent and action positions that the resource tests already provide.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(security): escape untrusted input at..." | Re-trigger Greptile