docs+mcp-gateway: MCP plan and Phase 1 NATS gateway - #178
Closed
yordis wants to merge 8 commits into
Closed
Conversation
Capture the design space before implementation so the irreversible decisions (host ABI, DSL, tenancy, correlation) are debated on paper. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…oint The gateway plan had no runnable edge path against mcp-nats subjects; deploying Phase 1 first proves queue-group forwarding and JetStream audit before betting SpiceDB and transport adapters on speculative wiring. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Operators need an explicit broker-backed check before layering SpiceDB, but CI clusters rarely expose NATS uniformly; deferring ignored forward tests hides whole-path drift without silently failing default automation. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryMedium Risk Overview Phase 1 enforcement is introduced: a hardcoded CEL gate decides when to invoke SpiceDB authorization (currently Reviewed by Cursor Bugbot for commit b23337d. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
WalkthroughIntroduces a complete NATS-based MCP gateway service with request interception, CEL-based authorization policy, JetStream audit publishing, decision tracing, and end-to-end request forwarding from gateway ingress topic to backend server topic. ChangesMCP Gateway Core Implementation
Sequence Diagram(s)sequenceDiagram
participant Client
participant GatewayIngress as Gateway<br/>(gateway.request)
participant AuthPolicy as Auth & Policy
participant Backend as Backend<br/>(server.*)
participant Audit as Audit Stream
Client->>GatewayIngress: MCP JSON-RPC on gateway topic
GatewayIngress->>AuthPolicy: Extract method, check if requires SpiceDB
alt Authorization Required
AuthPolicy-->>GatewayIngress: Policy decision (allow/deny)
alt Denied
GatewayIngress->>Audit: Publish "deny" event
GatewayIngress-->>Client: JSON-RPC -32003 error
else Allowed
GatewayIngress->>Backend: Forward to rewritten backend topic
alt Has Reply Subject
Backend-->>GatewayIngress: Response
GatewayIngress->>Audit: Publish "allow" event
GatewayIngress-->>Client: Forward response
else Fire-and-Forget
GatewayIngress->>Audit: Publish "allow" event
end
end
else No Auth Required
GatewayIngress->>Backend: Forward directly
alt Has Reply Subject
Backend-->>GatewayIngress: Response
GatewayIngress->>Audit: Publish "allow" event
GatewayIngress-->>Client: Forward response
else Fire-and-Forget
GatewayIngress->>Audit: Publish "allow" event
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Code Coverage SummaryDetailsDiff against mainResults for commit: b23337d Minimum allowed coverage is ♻️ This comment has been updated with latest results |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-mcp-gateway/Cargo.toml (1)
26-26: ⚡ Quick winRemove redundant
bytesfrom dev-dependencies.
bytesis already declared in[dependencies]at line 12, making the dev-dependency declaration redundant.♻️ Proposed fix
[dev-dependencies] -bytes = { workspace = true } uuid = { workspace = true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-mcp-gateway/Cargo.toml` at line 26, The dev-dependency declaration for the crate named "bytes" is redundant because "bytes" is already declared in the [dependencies] section; remove the "bytes = { workspace = true }" entry from the [dev-dependencies] section so the project uses the single workspace dependency declaration instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rsworkspace/crates/trogon-mcp-gateway/src/authz.rs`:
- Around line 7-13: AuthzContext currently exposes primitive &str fields
(tenant, server_id, jsonrpc_method, tool_name); replace these with domain value
objects (e.g., TenantId/Option<TenantId>, ServerId, JsonRpcMethod, ToolName)
that validate at construction so invalid values are unrepresentable; implement
small constructors or TryFrom<&str>/new functions on ServerId, JsonRpcMethod,
ToolName, TenantId to enforce validation and convert existing call sites to
construct/propagate those types into the AuthzContext struct and any consumers
of AuthzContext (update references to AuthzContext::server_id, ::jsonrpc_method,
::tool_name, ::tenant accordingly).
- Around line 15-24: Replace the string-wrapper error type AuthzError(pub
String) with a typed enum (e.g., enum AuthzError { Unauthorized, InvalidToken,
IoError(std::io::Error), External(Box<dyn std::error::Error + Send + Sync>), ...
}) so callers can pattern-match and preserve context; update the impl
fmt::Display for AuthzError to match on the enum and render human-friendly
messages for each variant, implement std::error::Error for the enum (propagate
source errors via source() or #[from] conversions), and add From conversions
where appropriate so functions that previously returned AuthzError(String) now
return specific variants (refer to AuthzError, its Display impl, and the
std::error::Error impl to locate and change the code).
In `@rsworkspace/crates/trogon-mcp-gateway/src/gateway.rs`:
- Around line 23-31: GatewayError is currently a string wrapper (pub struct
GatewayError(pub String)) which erases source context; replace it with a typed
enum (e.g., enum GatewayError { PolicyInit{ source: Box<dyn std::error::Error +
Send + Sync> }, Subscription{ source: ... }, SubjectParse{ source: ... }, Authz{
source: ... }, Publish{ source: ... }, Io{ source: ... }, Other(String) }) or a
struct with explicit source fields, implement std::error::Error by returning
source via source(), and implement Display to include a clear message while
preserving the underlying error; update all places that construct GatewayError
(the constructors/uses referenced in gateway.rs lines around GatewayError, and
the other noted sites at 51-56, 112-113, 124, 133, 145, 351-359) to wrap the
original error into the appropriate enum variant (using Box::new(...) or the
concrete error type) rather than to_string()/format!(), so callers can match on
variants and retain original error context for observability and typed handling.
- Around line 114-117: The code currently drops malformed messages when
jsonrpc_method(&msg.payload) returns None (the jsonrpc_method check around
msg.payload and msg.subject) and simply returns Ok(()); instead, construct and
send an immediate JSON-RPC error response indicating "Missing method" (or
JSON-RPC Invalid Request) back to the caller using the same reply/publish path
the gateway uses for request/reply messages, then return after sending; replace
the warn+return path with logic that builds the standard JSON-RPC error object
(including the original id if present from msg.payload), sends it to the
requester's reply address (or via the same publishing function used elsewhere in
this module), and only then return Ok(()) so callers waiting for a reply receive
the protocol error.
In `@rsworkspace/crates/trogon-mcp-gateway/src/policy.rs`:
- Around line 7-16: PolicyError currently stores a raw String; change it to a
typed error (enum or struct) that preserves source errors from the
cel-interpreter instead of stringifying them—for example replace PolicyError(pub
String) with an enum PolicyError { Interpreter { source: Box<dyn
std::error::Error + Send + Sync> }, Other { msg: String } } (or a struct with a
source field) and implement Display and std::error::Error::source to return the
boxed source; then update any construction sites that created
PolicyError(String::from(...)) to wrap the original cel-interpreter error (e.g.,
PolicyError::Interpreter { source: Box::new(err) }) so the original error
context is preserved.
- Around line 24-28: The code in phase1_hardcoded currently discards the typed
error from Program::compile(SPICEDB_GATE_EXPR) by converting it to a String;
update PolicyError to include a variant (e.g.,
PolicyError::Compile(cel_interpreter::Error) or a boxed source:
PolicyError::Compile(Box<dyn std::error::Error + Send + Sync>)) and then change
the map_err to preserve the original error (e.g., .map_err(|e|
PolicyError::Compile(e)) or .map_err(PolicyError::from)), ensuring the concrete
error type from Program::compile is wrapped as the source instead of calling
to_string().
- Around line 30-43: The function requires_spicedb_for_method is converting
cel_interpreter errors to plain strings (via .to_string()) which loses typed
information; update PolicyError to hold the original error (e.g., add variants
or a boxed source like Box<dyn Error + Send + Sync> and implement From for
cel_interpreter error types), then replace the map_err calls around
cel_interpreter::to_value and self.program.execute to wrap/convert the original
error into PolicyError (e.g., map_err(Into::into) or map_err(|e|
PolicyError::ValueError(e))) so callers can inspect the underlying
cel_interpreter error instead of only a string; keep references to to_value(...)
failure and self.program.execute(...) failure when creating the new PolicyError
variants.
In `@rsworkspace/crates/trogon-mcp-gateway/src/trace.rs`:
- Line 24: The current use of self.inner.lock().expect("trace mutex poisoned")
will hard-panic on a poisoned mutex; replace both calls (the one creating let
mut guard = self.inner.lock()... at lines shown and the other at line 32) with
poison-aware handling that recovers the inner data instead of panicking: call
self.inner.lock() and on Err(poison) use poison.into_inner() (or
match/unwrap_or_else to obtain the inner guard) so the code continues serving
requests; preserve the existing variable name (guard) and semantics after you
recover the inner data.
In `@rsworkspace/crates/trogon-mcp-gateway/tests/e2e_nats_forward.rs`:
- Around line 50-53: The backend responder is currently swallowing publish and
flush errors by calling .await.ok() which hides failures; change the responder
to fail fast by asserting or unwrapping the results of publish_with_headers (the
call that sends Bytes::from(backend_reply) with async_nats::HeaderMap::new() and
subject reply) and backend_nats.flush().await so any send/flush error surfaces
in the test (e.g., replace the .ok() calls with expect/unwrap/assert on the
Result returned by publish_with_headers and flush).
---
Nitpick comments:
In `@rsworkspace/crates/trogon-mcp-gateway/Cargo.toml`:
- Line 26: The dev-dependency declaration for the crate named "bytes" is
redundant because "bytes" is already declared in the [dependencies] section;
remove the "bytes = { workspace = true }" entry from the [dev-dependencies]
section so the project uses the single workspace dependency declaration instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d51ec70-4432-41e5-b7d5-7e8949a1d32a
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
MCP_GATEWAY_PLAN.mdrsworkspace/crates/trogon-mcp-gateway/Cargo.tomlrsworkspace/crates/trogon-mcp-gateway/README.mdrsworkspace/crates/trogon-mcp-gateway/src/audit.rsrsworkspace/crates/trogon-mcp-gateway/src/authz.rsrsworkspace/crates/trogon-mcp-gateway/src/config.rsrsworkspace/crates/trogon-mcp-gateway/src/gateway.rsrsworkspace/crates/trogon-mcp-gateway/src/lib.rsrsworkspace/crates/trogon-mcp-gateway/src/main.rsrsworkspace/crates/trogon-mcp-gateway/src/policy.rsrsworkspace/crates/trogon-mcp-gateway/src/subject.rsrsworkspace/crates/trogon-mcp-gateway/src/trace.rsrsworkspace/crates/trogon-mcp-gateway/tests/e2e_nats_forward.rsrsworkspace/crates/trogon-telemetry/src/service_name.rs
Comment on lines
+7
to
+13
| #[derive(Clone, Debug)] | ||
| pub struct AuthzContext<'a> { | ||
| pub tenant: Option<&'a str>, | ||
| pub server_id: &'a str, | ||
| pub jsonrpc_method: &'a str, | ||
| pub tool_name: Option<&'a str>, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Prefer domain-specific value objects over primitive strings.
AuthzContext uses &str for tenant, server_id, jsonrpc_method, and tool_name. These represent domain concepts that should be value objects with validation guarantees (e.g., ServerId, JsonRpcMethod, ToolName).
Introducing domain types ensures correctness at construction and prevents invalid values from propagating through the authorization pipeline. As per coding guidelines: "Prefer domain-specific value objects over primitives (e.g., AcpPrefix instead of String). Each type's factory must guarantee correctness at construction—invalid instances should be unrepresentable."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rsworkspace/crates/trogon-mcp-gateway/src/authz.rs` around lines 7 - 13,
AuthzContext currently exposes primitive &str fields (tenant, server_id,
jsonrpc_method, tool_name); replace these with domain value objects (e.g.,
TenantId/Option<TenantId>, ServerId, JsonRpcMethod, ToolName) that validate at
construction so invalid values are unrepresentable; implement small constructors
or TryFrom<&str>/new functions on ServerId, JsonRpcMethod, ToolName, TenantId to
enforce validation and convert existing call sites to construct/propagate those
types into the AuthzContext struct and any consumers of AuthzContext (update
references to AuthzContext::server_id, ::jsonrpc_method, ::tool_name, ::tenant
accordingly).
Comment on lines
+15
to
+24
| #[derive(Debug)] | ||
| pub struct AuthzError(pub String); | ||
|
|
||
| impl fmt::Display for AuthzError { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.write_str(&self.0) | ||
| } | ||
| } | ||
|
|
||
| impl std::error::Error for AuthzError {} |
There was a problem hiding this comment.
Replace String-based error with typed variants.
AuthzError(pub String) violates coding guidelines: "Errors must be typed—use structs or enums, never String or format!()." String-based errors cannot be pattern-matched by callers and lose the ability to preserve underlying error context.
🔧 Recommended refactor to enum-based error
-#[derive(Debug)]
-pub struct AuthzError(pub String);
-
-impl fmt::Display for AuthzError {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
- f.write_str(&self.0)
- }
-}
-
-impl std::error::Error for AuthzError {}
+#[derive(Debug)]
+pub enum AuthzError {
+ CheckFailed { reason: String },
+ Unreachable { source: Box<dyn std::error::Error + Send + Sync> },
+}
+
+impl fmt::Display for AuthzError {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ match self {
+ Self::CheckFailed { reason } => write!(f, "authorization check failed: {}", reason),
+ Self::Unreachable { source } => write!(f, "authorization service unreachable: {}", source),
+ }
+ }
+}
+
+impl std::error::Error for AuthzError {
+ fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+ match self {
+ Self::Unreachable { source } => Some(&**source),
+ _ => None,
+ }
+ }
+}As per coding guidelines: "Errors must be typed—use structs or enums, never String or format!()."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[derive(Debug)] | |
| pub struct AuthzError(pub String); | |
| impl fmt::Display for AuthzError { | |
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | |
| f.write_str(&self.0) | |
| } | |
| } | |
| impl std::error::Error for AuthzError {} | |
| #[derive(Debug)] | |
| pub enum AuthzError { | |
| CheckFailed { reason: String }, | |
| Unreachable { source: Box<dyn std::error::Error + Send + Sync> }, | |
| } | |
| impl fmt::Display for AuthzError { | |
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | |
| match self { | |
| Self::CheckFailed { reason } => write!(f, "authorization check failed: {}", reason), | |
| Self::Unreachable { source } => write!(f, "authorization service unreachable: {}", source), | |
| } | |
| } | |
| } | |
| impl std::error::Error for AuthzError { | |
| fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { | |
| match self { | |
| Self::Unreachable { source } => Some(&**source), | |
| _ => None, | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rsworkspace/crates/trogon-mcp-gateway/src/authz.rs` around lines 15 - 24,
Replace the string-wrapper error type AuthzError(pub String) with a typed enum
(e.g., enum AuthzError { Unauthorized, InvalidToken, IoError(std::io::Error),
External(Box<dyn std::error::Error + Send + Sync>), ... }) so callers can
pattern-match and preserve context; update the impl fmt::Display for AuthzError
to match on the enum and render human-friendly messages for each variant,
implement std::error::Error for the enum (propagate source errors via source()
or #[from] conversions), and add From conversions where appropriate so functions
that previously returned AuthzError(String) now return specific variants (refer
to AuthzError, its Display impl, and the std::error::Error impl to locate and
change the code).
Comment on lines
+23
to
+31
| pub struct GatewayError(pub String); | ||
|
|
||
| impl std::fmt::Display for GatewayError { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.write_str(&self.0) | ||
| } | ||
| } | ||
|
|
||
| impl std::error::Error for GatewayError {} |
There was a problem hiding this comment.
Replace stringly-typed gateway errors with structured variants and preserved sources.
GatewayError(pub String) plus to_string()/&'static str conversions erases source context across the pipeline (policy init, subscription, subject parsing, authz, publish/request). This makes typed handling and observability brittle.
💡 Proposed refactor sketch
-#[derive(Debug)]
-pub struct GatewayError(pub String);
+#[derive(Debug, thiserror::Error)]
+pub enum GatewayError {
+ #[error("policy initialization failed")]
+ PolicyInit {
+ #[source]
+ source: crate::policy::PolicyError,
+ },
+ #[error("subscription failed")]
+ Subscribe {
+ #[source]
+ source: async_nats::SubscribeError,
+ },
+ #[error("subject rewrite failed: {source}")]
+ SubjectRewrite {
+ #[from]
+ source: crate::subject::GatewaySubjectRewriteError,
+ },
+ #[error("invalid ingress subject: {source}")]
+ InvalidIngressSubject {
+ #[source]
+ source: ParseServerIdError,
+ },
+ // ...other typed variants for request/publish/authz...
+}As per coding guidelines, "Errors must be typed—use structs or enums, never String or format!()" and "Never discard error context by converting a typed error into a string; wrap the source error as a field or variant instead."
Also applies to: 51-56, 112-113, 124-124, 133-133, 145-145, 351-359
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rsworkspace/crates/trogon-mcp-gateway/src/gateway.rs` around lines 23 - 31,
GatewayError is currently a string wrapper (pub struct GatewayError(pub String))
which erases source context; replace it with a typed enum (e.g., enum
GatewayError { PolicyInit{ source: Box<dyn std::error::Error + Send + Sync> },
Subscription{ source: ... }, SubjectParse{ source: ... }, Authz{ source: ... },
Publish{ source: ... }, Io{ source: ... }, Other(String) }) or a struct with
explicit source fields, implement std::error::Error by returning source via
source(), and implement Display to include a clear message while preserving the
underlying error; update all places that construct GatewayError (the
constructors/uses referenced in gateway.rs lines around GatewayError, and the
other noted sites at 51-56, 112-113, 124, 133, 145, 351-359) to wrap the
original error into the appropriate enum variant (using Box::new(...) or the
concrete error type) rather than to_string()/format!(), so callers can match on
variants and retain original error context for observability and typed handling.
Comment on lines
+114
to
+117
| let Some(jsonrpc_method) = jsonrpc_method(&msg.payload) else { | ||
| warn!(subject = %msg.subject, "ingress message has no JSON-RPC method"); | ||
| return Ok(()); | ||
| }; |
There was a problem hiding this comment.
Return a JSON-RPC error when method is missing on request/reply ingress.
At Line 114-117, malformed request payloads are dropped after a warning. If the caller used request/reply, it waits until timeout with no protocol error response. Emit an immediate JSON-RPC error response instead.
💡 Proposed fix
let Some(jsonrpc_method) = jsonrpc_method(&msg.payload) else {
warn!(subject = %msg.subject, "ingress message has no JSON-RPC method");
+ if msg.reply.is_some() {
+ respond_with_jsonrpc_error(
+ client,
+ &msg,
+ jsonrpc_request_id(&msg.payload),
+ -32600,
+ "invalid JSON-RPC request: missing method".to_string(),
+ )
+ .await?;
+ }
return Ok(());
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rsworkspace/crates/trogon-mcp-gateway/src/gateway.rs` around lines 114 - 117,
The code currently drops malformed messages when jsonrpc_method(&msg.payload)
returns None (the jsonrpc_method check around msg.payload and msg.subject) and
simply returns Ok(()); instead, construct and send an immediate JSON-RPC error
response indicating "Missing method" (or JSON-RPC Invalid Request) back to the
caller using the same reply/publish path the gateway uses for request/reply
messages, then return after sending; replace the warn+return path with logic
that builds the standard JSON-RPC error object (including the original id if
present from msg.payload), sends it to the requester's reply address (or via the
same publishing function used elsewhere in this module), and only then return
Ok(()) so callers waiting for a reply receive the protocol error.
Comment on lines
+7
to
+16
| #[derive(Debug)] | ||
| pub struct PolicyError(pub String); | ||
|
|
||
| impl std::fmt::Display for PolicyError { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.write_str(&self.0) | ||
| } | ||
| } | ||
|
|
||
| impl std::error::Error for PolicyError {} |
There was a problem hiding this comment.
Replace String-based error with typed variants that preserve source errors.
PolicyError(pub String) violates coding guidelines. The error should preserve underlying error context from cel-interpreter instead of converting to String.
🔧 Recommended refactor to preserve error context
-#[derive(Debug)]
-pub struct PolicyError(pub String);
-
-impl std::fmt::Display for PolicyError {
- fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
- f.write_str(&self.0)
- }
-}
-
-impl std::error::Error for PolicyError {}
+#[derive(Debug)]
+pub enum PolicyError {
+ CompileFailed { source: cel_interpreter::ParseError },
+ ExecutionFailed { source: cel_interpreter::ExecutionError },
+ SerializationFailed { source: serde_json::Error },
+ UnexpectedType { expected: &'static str, actual: String },
+}
+
+impl std::fmt::Display for PolicyError {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ Self::CompileFailed { .. } => write!(f, "policy compilation failed"),
+ Self::ExecutionFailed { .. } => write!(f, "policy execution failed"),
+ Self::SerializationFailed { .. } => write!(f, "CEL value serialization failed"),
+ Self::UnexpectedType { expected, actual } => {
+ write!(f, "expected {} but got {}", expected, actual)
+ }
+ }
+ }
+}
+
+impl std::error::Error for PolicyError {
+ fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+ match self {
+ Self::CompileFailed { source } => Some(source),
+ Self::ExecutionFailed { source } => Some(source),
+ Self::SerializationFailed { source } => Some(source),
+ _ => None,
+ }
+ }
+}As per coding guidelines: "Errors must be typed—use structs or enums, never String or format!()" and "Never discard error context by converting a typed error into a string; wrap the source error as a field or variant instead."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[derive(Debug)] | |
| pub struct PolicyError(pub String); | |
| impl std::fmt::Display for PolicyError { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| f.write_str(&self.0) | |
| } | |
| } | |
| impl std::error::Error for PolicyError {} | |
| #[derive(Debug)] | |
| pub enum PolicyError { | |
| CompileFailed { source: cel_interpreter::ParseError }, | |
| ExecutionFailed { source: cel_interpreter::ExecutionError }, | |
| SerializationFailed { source: serde_json::Error }, | |
| UnexpectedType { expected: &'static str, actual: String }, | |
| } | |
| impl std::fmt::Display for PolicyError { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| match self { | |
| Self::CompileFailed { .. } => write!(f, "policy compilation failed"), | |
| Self::ExecutionFailed { .. } => write!(f, "policy execution failed"), | |
| Self::SerializationFailed { .. } => write!(f, "CEL value serialization failed"), | |
| Self::UnexpectedType { expected, actual } => { | |
| write!(f, "expected {} but got {}", expected, actual) | |
| } | |
| } | |
| } | |
| } | |
| impl std::error::Error for PolicyError { | |
| fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { | |
| match self { | |
| Self::CompileFailed { source } => Some(source), | |
| Self::ExecutionFailed { source } => Some(source), | |
| Self::SerializationFailed { source } => Some(source), | |
| _ => None, | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rsworkspace/crates/trogon-mcp-gateway/src/policy.rs` around lines 7 - 16,
PolicyError currently stores a raw String; change it to a typed error (enum or
struct) that preserves source errors from the cel-interpreter instead of
stringifying them—for example replace PolicyError(pub String) with an enum
PolicyError { Interpreter { source: Box<dyn std::error::Error + Send + Sync> },
Other { msg: String } } (or a struct with a source field) and implement Display
and std::error::Error::source to return the boxed source; then update any
construction sites that created PolicyError(String::from(...)) to wrap the
original cel-interpreter error (e.g., PolicyError::Interpreter { source:
Box::new(err) }) so the original error context is preserved.
Comment on lines
+24
to
+28
| pub fn phase1_hardcoded() -> Result<Self, PolicyError> { | ||
| Program::compile(SPICEDB_GATE_EXPR) | ||
| .map(|program| Self { program }) | ||
| .map_err(|e| PolicyError(e.to_string())) | ||
| } |
There was a problem hiding this comment.
Preserve typed error instead of converting to String.
Line 27 discards cel_interpreter's typed error by calling .to_string(). This loses the error source chain and prevents programmatic error handling.
🔧 Proposed fix (requires PolicyError refactor above)
pub fn phase1_hardcoded() -> Result<Self, PolicyError> {
Program::compile(SPICEDB_GATE_EXPR)
.map(|program| Self { program })
- .map_err(|e| PolicyError(e.to_string()))
+ .map_err(|source| PolicyError::CompileFailed { source })
}As per coding guidelines: "Never discard error context by converting a typed error into a string; wrap the source error as a field or variant instead."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rsworkspace/crates/trogon-mcp-gateway/src/policy.rs` around lines 24 - 28,
The code in phase1_hardcoded currently discards the typed error from
Program::compile(SPICEDB_GATE_EXPR) by converting it to a String; update
PolicyError to include a variant (e.g.,
PolicyError::Compile(cel_interpreter::Error) or a boxed source:
PolicyError::Compile(Box<dyn std::error::Error + Send + Sync>)) and then change
the map_err to preserve the original error (e.g., .map_err(|e|
PolicyError::Compile(e)) or .map_err(PolicyError::from)), ensuring the concrete
error type from Program::compile is wrapped as the source instead of calling
to_string().
Comment on lines
+30
to
+43
| pub fn requires_spicedb_for_method(&self, jsonrpc_method: &str) -> Result<bool, PolicyError> { | ||
| let mut ctx = Context::default(); | ||
| let mcp = serde_json::json!({ "method": jsonrpc_method }); | ||
| let value = cel_interpreter::to_value(&mcp).map_err(|e| PolicyError(e.to_string()))?; | ||
| ctx.add_variable_from_value("mcp", value); | ||
| match self.program.execute(&ctx) { | ||
| Ok(Value::Bool(b)) => Ok(b), | ||
| Ok(other) => Err(PolicyError(format!( | ||
| "policy expression must yield bool, got {:?}", | ||
| other.type_of() | ||
| ))), | ||
| Err(e) => Err(PolicyError(e.to_string())), | ||
| } | ||
| } |
There was a problem hiding this comment.
Preserve typed errors instead of converting to String.
Lines 33 and 41 discard typed errors from cel_interpreter by calling .to_string(). This loses error context and prevents callers from inspecting the underlying cause.
🔧 Proposed fix (requires PolicyError refactor above)
pub fn requires_spicedb_for_method(&self, jsonrpc_method: &str) -> Result<bool, PolicyError> {
let mut ctx = Context::default();
let mcp = serde_json::json!({ "method": jsonrpc_method });
- let value = cel_interpreter::to_value(&mcp).map_err(|e| PolicyError(e.to_string()))?;
+ let value = cel_interpreter::to_value(&mcp)
+ .map_err(|source| PolicyError::SerializationFailed { source })?;
ctx.add_variable_from_value("mcp", value);
match self.program.execute(&ctx) {
Ok(Value::Bool(b)) => Ok(b),
- Ok(other) => Err(PolicyError(format!(
- "policy expression must yield bool, got {:?}",
- other.type_of()
- ))),
- Err(e) => Err(PolicyError(e.to_string())),
+ Ok(other) => Err(PolicyError::UnexpectedType {
+ expected: "bool",
+ actual: format!("{:?}", other.type_of()),
+ }),
+ Err(source) => Err(PolicyError::ExecutionFailed { source }),
}
}As per coding guidelines: "Never discard error context by converting a typed error into a string; wrap the source error as a field or variant instead."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rsworkspace/crates/trogon-mcp-gateway/src/policy.rs` around lines 30 - 43,
The function requires_spicedb_for_method is converting cel_interpreter errors to
plain strings (via .to_string()) which loses typed information; update
PolicyError to hold the original error (e.g., add variants or a boxed source
like Box<dyn Error + Send + Sync> and implement From for cel_interpreter error
types), then replace the map_err calls around cel_interpreter::to_value and
self.program.execute to wrap/convert the original error into PolicyError (e.g.,
map_err(Into::into) or map_err(|e| PolicyError::ValueError(e))) so callers can
inspect the underlying cel_interpreter error instead of only a string; keep
references to to_value(...) failure and self.program.execute(...) failure when
creating the new PolicyError variants.
|
|
||
| impl TraceStore { | ||
| pub fn insert(&self, request_id: impl Into<String>, trace: DecisionTrace) { | ||
| let mut guard = self.inner.lock().expect("trace mutex poisoned"); |
There was a problem hiding this comment.
Avoid panicking on trace lock poisoning in the request path.
At Line 24 and Line 32, expect(...) turns a prior panic while holding the mutex into repeated hard panics on future requests. Recover from poison and keep serving.
💡 Proposed fix
- let mut guard = self.inner.lock().expect("trace mutex poisoned");
+ let mut guard = match self.inner.lock() {
+ Ok(g) => g,
+ Err(poisoned) => poisoned.into_inner(),
+ };
@@
- self.inner.lock().expect("trace mutex poisoned").get(request_id).cloned()
+ let guard = match self.inner.lock() {
+ Ok(g) => g,
+ Err(poisoned) => poisoned.into_inner(),
+ };
+ guard.get(request_id).cloned()Also applies to: 32-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rsworkspace/crates/trogon-mcp-gateway/src/trace.rs` at line 24, The current
use of self.inner.lock().expect("trace mutex poisoned") will hard-panic on a
poisoned mutex; replace both calls (the one creating let mut guard =
self.inner.lock()... at lines shown and the other at line 32) with poison-aware
handling that recovers the inner data instead of panicking: call
self.inner.lock() and on Err(poison) use poison.into_inner() (or
match/unwrap_or_else to obtain the inner guard) so the code continues serving
requests; preserve the existing variable name (guard) and semantics after you
recover the inner data.
Comment on lines
+50
to
+53
| .publish_with_headers(reply.to_string(), async_nats::HeaderMap::new(), Bytes::from(backend_reply)) | ||
| .await | ||
| .ok(); | ||
| backend_nats.flush().await.ok(); |
There was a problem hiding this comment.
Fail fast in the backend responder instead of swallowing send failures.
Line 50-53 currently hides publish/flush failures, which turns actionable failures into opaque request timeouts. Prefer explicit assertions in this test path.
💡 Proposed fix
backend_nats
.publish_with_headers(reply.to_string(), async_nats::HeaderMap::new(), Bytes::from(backend_reply))
.await
- .ok();
- backend_nats.flush().await.ok();
+ .expect("backend publish reply");
+ backend_nats.flush().await.expect("backend flush reply");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .publish_with_headers(reply.to_string(), async_nats::HeaderMap::new(), Bytes::from(backend_reply)) | |
| .await | |
| .ok(); | |
| backend_nats.flush().await.ok(); | |
| .publish_with_headers(reply.to_string(), async_nats::HeaderMap::new(), Bytes::from(backend_reply)) | |
| .await | |
| .expect("backend publish reply"); | |
| backend_nats.flush().await.expect("backend flush reply"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rsworkspace/crates/trogon-mcp-gateway/tests/e2e_nats_forward.rs` around lines
50 - 53, The backend responder is currently swallowing publish and flush errors
by calling .await.ok() which hides failures; change the responder to fail fast
by asserting or unwrapping the results of publish_with_headers (the call that
sends Bytes::from(backend_reply) with async_nats::HeaderMap::new() and subject
reply) and backend_nats.flush().await so any send/flush error surfaces in the
test (e.g., replace the .ok() calls with expect/unwrap/assert on the Result
returned by publish_with_headers and flush).
Operators need reads and invokes checked by the same PDP without custom clients. Request-reply must not wedge when SpiceDB faults; aligning errors with Trogon RPC codes preserves client semantics. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
| } | ||
| } | ||
| out | ||
| } |
There was a problem hiding this comment.
SpiceDB object ID normalization has collision vulnerability
Medium Severity
normalize_spicedb_object_token uses _ as the escape prefix for non-allowed characters (e.g., : → _3a), but _ itself is in the allowed-passthrough set. This makes the encoding non-injective — inputs "a_3a" and "a:" both normalize to "a_3a". Similarly, whitespace maps to _, colliding with literal underscores. In the authorization path, this could cause a SpiceDB permission check for one tool or resource to incorrectly match another.
Reviewed by Cursor Bugbot for commit 783b1ac. Configure here.
Forgeable tenancy headers alone are inadequate when SpiceDB gates tools/call and resources/read; callers need phased enforceable identity aligned with a broker chokepoint rather than layering SPIRE or STS inside this binary. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Phase 1 already shipped chokepoint PDP and audit; the plan should reflect ingress JWT so operators and reviewers do not assume header-only tenancy. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…y egress Server lanes must not derive principal facts from forgeable tenancy alone after ingress hardening, and authorization decisions deserve monotonic datastore bounds whenever SpiceDB returns snapshot tokens alongside results. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b23337d. Configure here.
| } | ||
| } | ||
| out | ||
| } |
There was a problem hiding this comment.
Client-injected gateway identity headers not stripped on egress
High Severity
When JWT mode is validate or require, egress_header_map only strips the trogon-mcp-tenant header but does not strip gateway-controlled identity headers (trogon-mcp-verified-sub, trogon-mcp-verified-tenant, trogon-mcp-identity-source, trogon-mcp-jwt-issuer). When identity resolves as non-JWT (e.g., no Bearer token in validate mode), append_verified_gateway_identity_headers early-returns without overwriting those headers. A malicious client can inject forged trogon-mcp-verified-sub values that reach backends as trusted identity.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b23337d. Configure here.
| jsonwebtoken = { version = "=10.4.0", features = ["rust_crypto"] } | ||
| mcp-nats = { workspace = true } | ||
| pem = "=3.0.6" | ||
| reqwest = { version = "=0.12.22", default-features = false, features = ["json", "rustls-tls"] } |
There was a problem hiding this comment.
Exact reqwest pin downgrades dependency for entire workspace
Medium Severity
The exact version pin reqwest = "=0.12.22" forces the entire workspace to downgrade from 0.12.28 to 0.12.22 (visible in the Cargo.lock diff). This affects all other workspace members that depend on reqwest, potentially losing bug fixes and security patches from the six intermediate versions.
Reviewed by Cursor Bugbot for commit b23337d. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary