fix(authz): make Cedar URI entity IDs collision-free

[SashaMIT]

A permission granted to one resource URI can silently apply to a different, colliding URI.

Problem

authorizeResourceRead turned resource URIs into Cedar entity IDs with a lossy sanitizer that rewrote :, /, \, ?, &, =, #, space, and . to _. The mapping is many-to-one, so distinct URIs collide onto one entity ID:

• file:///etc/passwd and file://_etc/passwd both became Resource::"file____etc_passwd"
• mcp://srv/config:admin and mcp://srv/config/admin both became Resource::"mcp___srv_config_admin"

A policy granting Resource::"file____etc_passwd" therefore authorized reads of every URI in the collision class. The confused-deputy condition: an MCP server that serves both a permitted URI and a colliding sensitive URI (or one that lets users create resource URIs) gets the grant applied to a resource the policy author never named.

Severity framing, honestly stated: exploitation needs a policy author who grants by entity ID, plus a colliding pair where one side is permitted and the other is sensitive and server-distinguished. Attribute-based policies (resource.name == ..., resource.uri == ...) match on the exact URI and are unaffected.

Fix

Entities are built programmatically with cedar.NewEntityUID, which accepts any string, so no character rewriting is needed at all. The exact URI is now the entity ID and the sanitizer is removed. Policy authors can name the real URI (for example Resource::"file:///etc/passwd") instead of computing a mangled form.

Note for existing policies: a policy that references a sanitized ID (only possible for URIs containing the rewritten characters) must be updated to name the exact URI. Policies using plain IDs or attribute matching are unchanged.

Tests

Replaced TestSanitizeURIForCedar with TestAuthorizeResourceReadEntityIDsCollisionFree: for both collision pairs above, a grant on the exact URI authorizes that URI and denies the formerly colliding URI. The pkg/authz/... and pkg/vmcp/core suites pass.

Made with Cursor

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.82%. Comparing base (c86cb8a) to head (325d907).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6239      +/-   ##
==========================================
- Coverage   72.83%   72.82%   -0.01%     
==========================================
  Files         743      743              
  Lines       77649    77635      -14     
==========================================
- Hits        56556    56541      -15     
- Misses      17125    17129       +4     
+ Partials     3968     3965       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jhrozek]

The fix looks right to me. I traced both sides: the request UID comes from cedar.NewEntityUID (core.go:513-516) and the entity-map key comes from the same parseCedarEntityID split (entity.go:152,187), so the two stay in sync and arbitrary URI characters are safe. parseCedarEntityID uses SplitN(..., "::", 2), so a URI containing "::" still parses with the full URI as the ID. And since every resource-read path (middleware.go:49-53, response_filter.go:781, vmcp/core/admission.go:199,218) funnels through authorizeResourceRead, this fixes it in one place rather than per caller.

Two comments below, both about documenting the new form rather than the code itself. Non-blocking.

[SashaMIT]

Thanks @jhrozek. Added a docs/authz.md note that read_resource entity IDs are the exact URI (with a Resource::"file:///..." example), plus the Cedar string-literal escape caveat for " / \. Softened the code comment to match. Happy to tweak the wording if you want it shorter.

[jhrozek]

LGTM.

Housekeeping first: my two comments above were against a stale revision — I had an old local commit checked out and didn't re-check the remote head before posting. 325d907 already addressed both, and did it better than I asked. Sorry for the noise; ignore them.

On the fix itself, I traced both sides of the entity ID. The request UID comes from cedar.NewEntityUID (core.go:513-516) and the entity-map key comes from the same parseCedarEntityID split (entity.go:152,187), so the two agree for arbitrary URI characters. parseCedarEntityID uses SplitN(..., "::", 2), so a URI containing "::" still parses with the full URI as the ID. And since every resource-read path — middleware.go:49-53, response_filter.go:781, vmcp/core/admission.go:199,218 — funnels through authorizeResourceRead, this is fixed once rather than per caller.

One thing worth recording: the sanitization was never documented, so writing a policy against a mangled ID required reading the entity ID off a debug log line and copying it back. The likelier experience is pkg/vmcp/core/core_calls_test.go:718, where the policy is written as Resource::"file:///ok" — the intuitive exact-URI form, which matched nothing under the old code, and nobody noticed because the test only asserts the deny half. That permit is live now. So the migration note in the description is appropriately hedged; I doubt there is anyone to migrate.

The docs paragraph is the part I like most — the escaping example resolves to a single backslash in Cedar source, which is right, and it saves the next person the dead end above.

Nit, not blocking: core_test.go:3379 interpolates the URI into policy source with fmt.Sprintf. Fine for both current rows, but a row containing a quote or backslash would fail at NewCedarAuthorizer rather than at the assertion, pointing the reader at the wrong file. A comment naming the constraint would keep the table safe to extend.