Skip to content

feat(interventions): add cedar vended intervention handler#2365

Open
lizradway wants to merge 14 commits into
strands-agents:mainfrom
lizradway:cedar
Open

feat(interventions): add cedar vended intervention handler#2365
lizradway wants to merge 14 commits into
strands-agents:mainfrom
lizradway:cedar

Conversation

@lizradway
Copy link
Copy Markdown
Member

Description

Adds a CedarAuthorization intervention handler that evaluates Cedar authorization policies at the tool-call boundary, providing identity-aware access control for agent tool invocations.

The handler integrates with @cedar-policy/cedar-wasm (optional peer dependency) to evaluate Cedar policies before each tool call, returning deny() or proceed() based on the policy decision.

Follows the cedar-for-agents schema generator conventions:

  • One Cedar action per tool (e.g. Action::"search")
  • Resource is unconstrained by default (use resourceResolver for domain-specific entities)
  • Context is nested: { input: <tool args>, session: { hour_utc, call_count, environment } }

Cedar-to-tool mapping

Cedar concept Maps to Example
Principal User identity from invocationState (via principalResolver) User::"alice@acme.com"
Action Tool name Action::"search"
Resource Unconstrained default, or domain object via resourceResolver Resource::"agent" or Record::"42"
Context.input Tool's input arguments { query: "report" }
Context.session Automatic invocation metadata { hour_utc: 14, call_count: 3 }

Key features

  • Fail-closed design — missing principal identity denies all tool calls
  • File-based or inline policies — pass Cedar policy text directly or a path to a .cedar file
  • Pluggable resource resolution — map tools to domain-specific Cedar resources via record or function
  • Context enrichment — automatic hour_utc, call_count, environment in session context, plus custom enricher callback
  • Session-aware rate limiting — built-in per-session call counters with LRU eviction
  • Configurable error handlingonError: 'throw' | 'deny' | 'proceed'

Usage

import { Agent } from '@strands-agents/sdk'
import { CedarAuthorization } from '@strands-agents/sdk/vended-interventions/cedar'

const cedar = new CedarAuthorization({
  policies: './policies/agent.cedar',
  entities: './policies/entities.json',
  principalResolver: (state) => {
    if (!state.user_id) return undefined
    return { type: 'User', id: String(state.user_id) }
  },
})

const agent = new Agent({
  tools: [searchTool, deleteTool],
  interventions: [cedar],
})

await agent.invoke('Delete record 42', {
  invocationState: { user_id: 'alice@acme.com' },
})

Related Issues

Documentation PR

N/A

Type of Change

New feature

Testing

How have you tested the change?

  • I ran npm run check for TypeScript (type-check, tests, lint)
  • 20 unit tests using real Cedar policy evaluation (no mocks) covering: permit/deny, default-deny, role-based access, rate limiting, environment restrictions, fail-closed on missing principal, malformed policy handling, resource resolution (default, record-based, function), context enrichment, file-based policy/entity loading, onError modes (throw/deny/proceed), session reset
  • 3 integration tests with real Bedrock model + real cedar-wasm: policy permit, policy forbid, fail-closed
  • Interactive example (examples/cedar/) tested end-to-end with 5 scenarios

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Comment thread strands-ts/src/vended-interventions/cedar/cedar.ts
Comment thread strands-ts/src/vended-interventions/cedar/cedar.ts
Comment thread strands-ts/src/vended-interventions/cedar/cedar.ts
Comment thread strands-ts/src/vended-interventions/cedar/cedar.ts
Comment thread strands-ts/src/vended-interventions/cedar/cedar.ts
Comment thread strands-ts/src/vended-interventions/cedar/__tests__/cedar.test.ts Outdated
@github-actions
Copy link
Copy Markdown
Contributor

Assessment: Comment

Well-structured Cedar authorization handler that follows existing vended-intervention patterns. The fail-closed security design, real Cedar evaluation in tests, and comprehensive coverage of authorization scenarios are strong.

Review Themes
  • Environment compatibility: The module uses Node.js-specific APIs (node:fs, @cedar-policy/cedar-wasm/nodejs) at the top level, and the test file naming (cedar.test.ts vs cedar.test.node.ts) doesn't reflect this — it will be picked up by the browser test project where it cannot run.
  • Type safety: The cast from Record<string, unknown> (InvocationState) to Record<string, JSONValue> silently narrows types in a way that could surprise consumers.
  • API ambiguity: The policies string parameter overloads path detection and inline text in a way that has edge cases (non-.cedar file paths, inline text ending with .cedar).
  • Session management: The "LRU eviction" described in the PR is actually FIFO, and _maxSessions isn't configurable.

Good first vended intervention — the core authorization logic is clean and the test suite covers the important authorization scenarios thoroughly.

@lizradway lizradway marked this pull request as ready for review May 28, 2026 21:08
Comment thread strands-ts/src/vended-interventions/cedar/cedar.ts Outdated
Comment thread strands-ts/src/vended-interventions/cedar/cedar.ts
@github-actions
Copy link
Copy Markdown
Contributor

Assessment: Comment

The Cedar authorization handler is well-implemented with a fail-closed security design and thorough test coverage using real Cedar policy evaluation.

Review Themes
  • Environment compatibility: The module is Node.js-only due to @cedar-policy/cedar-wasm/nodejs and node:fs imports, but the test file naming (cedar.test.ts) would include it in the browser test project. Should be cedar.test.node.ts.
  • API clarity: The policies parameter overloads file-path detection with inline text via endsWith('.cedar') heuristic, which has edge cases. Consider separate policies/policiesFile options.
  • Session management: The eviction strategy is FIFO (not LRU as described), and _maxSessions is not configurable.
  • API review: Per team/API_BAR_RAISING.md, this introduces a new public class (CedarAuthorization) that customers will directly use. Consider adding the needs-api-review label to ensure the public API surface gets appropriate review.

Solid implementation overall — the core authorization logic, TSDoc, and test scenarios are comprehensive.

const invocationState = event.invocationState as Record<string, CedarValueJson>
const principal = this._principalResolver(invocationState)
if (!principal) {
return deny('No principal identity found in invocation state')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do I have to pass it in invocation state, can't i pass it in original config? Is this something we expect to change with each invocation?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly should the principal resolver be required?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do I have to pass it in invocation state, can't i pass it in original config? Is this something we expect to change with each invocation?

this is expected to change for any multi-tenant deployed solution. but i think it makes sense to be able to pass this into the config for the cli case, can add a fall through to config if user is not in invocation state.

similarly should the principal resolver be required?

added static principal identity override option to config, so the resolver is no longer required.

Comment thread strands-ts/src/vended-interventions/cedar/cedar.ts
@agent-of-mkmeral
Copy link
Copy Markdown
Contributor

🔴 Two security findings in the rate-limit / environment handling

Great PR overall — the core Cedar evaluation is solid (default-deny, forbid-wins, fail-closed on missing principal, atomic reload(), schema validation all hold under adversarial testing). But two of the handler's own helper paths (not the Cedar engine) let a caller defeat policies that look airtight. Both are reproducible against real @cedar-policy/cedar-wasm evaluation, and both are fixable in cedar.ts with no engine changes.

The root cause of #1 is also a correctness/accuracy problem with call_count worth calling out on its own (bottom section).

🔴 #1 — Rate-limit bypass via session-map eviction (HIGH)

_incrementCallCount evicts the oldest session when the map hits _maxSessions = 1000:

private _incrementCallCount(sessionId: string, toolName: string): number {
  let session = this._callCounts.get(sessionId)
  if (!session) {
    if (this._callCounts.size >= this._maxSessions) {
      const oldest = this._callCounts.keys().next().value!   // evict oldest-inserted
      this._callCounts.delete(oldest)
    }
    session = new Map()
    this._callCounts.set(sessionId, session)
  }
  const next = (session.get(toolName) ?? 0) + 1
  session.set(toolName, next)
  return next
}

The sessionId comes straight from caller-influenced invocationState:

const sessionId = (invocationState.session_id as string | undefined) ?? '_default'

Attack: against a policy like permit(... action == Action::"send_email") when { context.session.call_count < 3 }, a caller who can vary session_id hits the rate limit, then spawns 1000 throwaway sessions to evict their own counter from the map — resetting call_count to 1.

Observed (real cedar-wasm eval):

attacker call 1: proceed
attacker call 2: proceed
attacker call 3: deny   (rate limit hit)
... create 1000 distinct throwaway sessions (evicts attacker's entry) ...
attacker call AFTER flood: proceed   <-- counter reset, limit bypassed

Control (no flood) stays denied on the 4th call, confirming eviction is the cause.

Fix options: evict on a key that isn't attacker-controllable, or use a real LRU keyed on identity, or a persistent/shared store — or, if call_count is only meant as a soft guardrail, document it as best-effort and not a security boundary (see the call_count section below). Don't silently drop a counter that a forbid/permit condition depends on.

🔴 #7forbid … when environment == "production" silently fails open when env is omitted (MED-HIGH)
environment: (invocationState.environment as string | undefined) ?? 'unknown',

A natural, idiomatic deny-in-prod policy:

permit(principal, action == Action::"deploy", resource);
forbid(principal, action == Action::"deploy", resource)
  when { context.session.environment == "production" };

never fires when the caller simply omits environment — it defaults to "unknown", not "production", so the forbid doesn't match and the action is allowed.

Observed:

environment = "production"  -> deny     (correct)
environment = <omitted>     -> proceed  (production guard bypassed)

The allow-list form does fail closed on omission (verified), which is the safer pattern:

permit(principal, action == Action::"deploy", resource)
  when { context.session.environment == "production" };   // omitted env -> no permit -> deny

Fix options: drop the ?? 'unknown' default and omit the field instead (so has checks / schema force explicitness), and/or document loudly that prod-gating must be written as an allow-list (permit … when env == "production"), never a deny-list (forbid … when env == "production"). As written, the deny-list form is a deny→allow footgun triggered by not setting a field.

⚠️ call_count accuracy — it's best-effort, not a reliable counter

Independent of the bypass above, call_count is inaccurate in several ways that are worth documenting (or fixing) so policy authors don't over-trust it:

private readonly _callCounts = new Map<string, Map<string, number>>()  // in-memory only
  1. Not persisted — in-memory on the handler instance. A process restart or a new CedarAuthorization instance resets all counts to 0.
  2. Per-instance, not global — behind N load-balanced workers each replica has its own map, so the effective limit is up to the policy value.
  3. Resettable via eviction — the FIFO drop above (finding ci: update sphinx requirement from <6.0.0,>=5.0.0 to >=5.0.0,<9.0.0 #1).
  4. Counts denied calls too_incrementCallCount runs before the authorization decision, so blocked attempts still consume budget. Defensible as anti-probing, but it means the number is “attempts,” not “successful calls.”
  5. Keyed on session_id, not identity — a fresh session_id is a fresh counter, so it doesn't bound a user across sessions.

Also: the PR description refers to the eviction as “LRU”, but the implementation is FIFO (_callCounts.keys().next().value drops the first-inserted key, not the least-recently-used). Worth reconciling the wording either way.

Suggestion: if call_count is intended as a hard limit, make it durable + identity-keyed (e.g. a shared store) and don't evict security-relevant counters. If it's a soft guardrail, a one-line doc note on context.session.call_count (“best-effort, single-process, not a security boundary; enforce hard rate limits upstream”) would set the right expectation.


None of these touch the Cedar engine — they're all in the handler's helper logic around it (_incrementCallCount, the environment default). Happy to open a PR with regression tests (a session-eviction test and an omitted-environment test) if that's useful. Nice work on the rest — the abstraction fit (3rd InterventionHandler impl) and the real-evaluation test suite are great. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-hil Human in the loop and suspend/resume area-interventions Related to interventions enhancement New feature or request needs-api-review Makes changes to the public API surface size/xl typescript Pull requests that update typescript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants