chore(guard): handle cors on the gateway #3341
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
PR Review: Handle CORS on the GatewaySummaryThis PR consolidates CORS handling at the gateway level in Rust, removing the previous TypeScript-based CORS middleware configuration. The changes simplify CORS management by implementing a consistent policy across both the Rust gateway and TypeScript routers. Code Quality & Best Practices ✅Rust Implementation (pegboard-gateway/src/lib.rs)
TypeScript Implementation (common/cors.ts)
Code Style Alignment:
Potential Issues 🔍1. Overly Permissive CORS Policy (SECURITY CONCERN) The current implementation accepts any origin and echoes it back: // engine/packages/pegboard-gateway/src/lib.rs:81-86
let origin = req
.headers()
.get("origin")
.and_then(|v| v.to_str().ok())
.unwrap_or("*")
.to_string();Issue: This allows requests from any origin, which could enable:
Comment in code states:
Recommendation:
2. Missing Wildcard Safety Check // line 113
.unwrap_or("*")When Recommendation: Either:
3. Inconsistent Header Handling The gateway allows all headers via wildcard: // line 123
.header("access-control-allow-headers", requested_headers)However, Recommendation:
4. WebSocket CORS Coverage The comment states:
However, WebSocket connections don't use the standard CORS preflight mechanism. WebSockets use the Recommendation:
Performance Considerations ⚡Positive:
Minor Optimization:
Security Concerns 🔒Critical:
Medium: Positive Security Aspects:
Test Coverage 📋Concerns:
Recommendations:
Configuration Removal Impact 🔧The PR removes // DELETED: Lines that defined defaultInspectorOrigins and defaultCors
const defaultInspectorOrigins = [
"http://localhost:43708",
// ... studio.rivet.gg, inspect.rivet.dev, etc.
];Impact:
Recommendation:
Overall AssessmentStrengths:
Concerns:
Recommendations SummaryBefore Merging:
Post-Merge (Technical Debt):
Great work on consolidating CORS handling! The implementation is clean, but please address the security concerns before merging. |
NathanFlurry
force-pushed
the
11-04-chore_guard_handle_cors_on_the_gateway
branch
from
8bc1d65 to
70ec891
Compare
NathanFlurry
force-pushed
the
11-04-chore_guard_add_support_for_routing_runner_ws_to__runners_connect_
branch
from
50142b7 to
23c26cd
Compare
PR Review: chore(guard): handle cors on the gatewaySummaryThis PR consolidates CORS handling at the gateway level, removing scattered CORS configurations from various TypeScript routers and implementing a unified approach in both Rust (pegboard-gateway) and TypeScript (new cors middleware). This is a good architectural improvement that simplifies CORS management. Code Quality & Best Practices ✅Strengths:
Code style notes:
Security Concerns
|
Code Review: chore(guard): handle cors on the gatewaySummaryThis PR consolidates CORS handling at the gateway level instead of in individual routers. This is a sensible architectural change that centralizes CORS logic and solves the issue of not being able to send OPTIONS requests to actors without authentication tokens. ✅ Strengths
🔍 Issues & Concerns1. Security: Overly Permissive CORS Policy (HIGH Priority)The current implementation allows requests from any origin ( // lib.rs:85-86
let origin = req
.headers()
.get("origin")
.and_then(|v| v.to_str().ok())
.unwrap_or("*") // ⚠️ Allows any origin!
.to_string();Problem: The comment on line 103 states "lets the actor handle CORS manually in Recommendation:
2. Header Validation Logic Issue (MEDIUM Priority)In let requested_headers = req
.headers()
.get("access-control-request-headers")
.and_then(|v| v.to_str().ok())
.unwrap_or("*");Problem: Using Recommendation: Provide a specific list of common headers instead of 3. Inconsistent Behavior (LOW Priority)The TypeScript implementation removes 4. Missing Error Context (LOW Priority)In return Ok(response.body(ResponseBody::Full(Full::new(Bytes::new())))?);Consider using 5. Test Coverage (MEDIUM Priority)No tests were added for the new CORS functionality. Consider adding tests for:
📝 Code Style Notes
🎯 RecommendationsBefore Merging:
Follow-up: 🔒 Security AssessmentThe current implementation has moderate security concerns due to the permissive origin policy. While the Verdict: Approve with the understanding that origin validation must be implemented either at the gateway or thoroughly documented as an actor responsibility. ✨ Overall AssessmentThis is a solid architectural improvement that centralizes CORS handling. The code quality is good, but the security model needs clarification and potentially tightening before production use. Recommendation: Request changes for the permissive origin policy before merging. 🤖 Generated with Claude Code |
NathanFlurry
force-pushed
the
11-04-chore_guard_add_support_for_routing_runner_ws_to__runners_connect_
branch
from
23c26cd to
e86c567
Compare
NathanFlurry
force-pushed
the
11-04-chore_guard_handle_cors_on_the_gateway
branch
from
70ec891 to
e7f7c04
Compare
PR Review: CORS Handling at Gateway LevelSummaryThis PR centralizes CORS handling at the gateway level (both Rust and TypeScript), removing the need for individual CORS configurations in the actor, manager, and inspector routers. This is a smart architectural decision that simplifies the codebase. ✅ Strengths
|
Code ReviewSummaryThis PR consolidates CORS handling at the gateway level, moving it from individual middleware configurations to a unified implementation in both the Rust pegboard-gateway and TypeScript RivetKit router. This is a sensible architectural change that simplifies CORS management and ensures consistent behavior. Strengths
Critical Issues1. Security: Overly Permissive CORS Policy (CRITICAL)The current implementation allows requests from ANY origin by echoing back whatever origin is sent. This effectively disables CORS protection. While the comment mentions letting the actor handle CORS manually in onBeforeConnect, this creates a security gap:
Recommendation: Implement origin validation at the gateway level with a configurable allowlist. 2. Security: Wildcard Header ExposureBoth implementations use access-control-expose-headers: * which exposes ALL response headers to JavaScript, including potentially sensitive headers. Consider explicitly listing headers that need to be exposed. 3. Breaking Change Without Migration PathThe PR removes the cors configuration option from both InspectorConfig and RunnerConfig without deprecation warnings or migration guidance. Users who customized CORS settings will have their configurations silently ignored. 4. Testing: No Test CoverageThe PR adds significant security-sensitive code but includes no tests. Consider adding unit tests for CORS header generation, integration tests for preflight requests, and edge case tests. Other Issues
PerformanceThe implementation is efficient with early returns for OPTIONS requests and minimal allocations. Recommendations Priority
ConclusionThis PR makes a good architectural improvement by centralizing CORS handling. However, the overly permissive CORS policy is a critical security concern that should be addressed before merging. Review based on CLAUDE.md conventions. Generated with Claude Code. |
graphite-app
bot
changed the base branch from
11-04-chore_guard_add_support_for_routing_runner_ws_to__runners_connect_
to
graphite-base/3341
graphite-app
bot
force-pushed
the
11-04-chore_guard_handle_cors_on_the_gateway
branch
from
e7f7c04 to
63d1984
Compare
graphite-app
bot
force-pushed
the
graphite-base/3341
branch
from
e86c567 to
f9dd2a8
Compare
graphite-app
bot
force-pushed
the
11-04-chore_guard_handle_cors_on_the_gateway
branch
from
63d1984 to
c9797c4
Compare
PR Review: chore(guard): handle cors on the gatewaySummaryThis PR centralizes CORS handling at the gateway level, removing distributed CORS configuration from TypeScript routers. The implementation is solid with good security considerations, but there are a few items worth discussing. Code Quality & Best Practices ✅Rust Implementation (pegboard-gateway/src/lib.rs)
TypeScript Implementation (common/cors.ts)
Configuration Cleanup
Security Concerns
|
| Feature | Rust | TypeScript |
|---|---|---|
| Echo Origin | ✅ | ✅ |
| Credentials Support | ✅ | ✅ |
| Allowed Methods | ✅ Same list | ✅ Same list |
| Max Age | ✅ 86400 | ✅ 86400 |
| Vary Header | ✅ | ✅ |
| Expose Headers | ✅ * |
✅ * |
Minor difference: TypeScript removes content-length/content-type headers from preflight (cors.ts:39-40), while Rust doesn't explicitly do this. This is fine since Rust returns an empty body for OPTIONS.
Test Coverage 🧪
Missing: No tests included for CORS behavior
Recommendations:
- Add tests for OPTIONS preflight requests (both Rust and TypeScript)
- Test that
Vary: Originheader is present when Origin is provided - Test that CORS headers are added to actual responses
- Test behavior when no Origin header is present
- Test WebSocket CORS handling (mentioned in comments that CORS applies to WebSockets too)
Potential Bugs 🐛
1. Header Conflict Risk
// lib.rs:213-215
for (key, value) in response_start.headers {
response_builder = response_builder.header(key, value);
}
// lib.rs:218-221
response_builder = response_builder
.header("access-control-allow-origin", &origin)
.header("access-control-allow-credentials", "true")
.header("access-control-expose-headers", "*");If an actor sets its own CORS headers in response_start.headers, they will be overwritten by the gateway. This might be intentional, but worth documenting.
2. TypeScript async/await Pattern
// cors.ts:14
return async (c, next) => {This is correct for Hono middleware, but ensure that all routers properly await CORS middleware. From the diffs, it looks like it's used correctly with .use().
Breaking Changes ⚠️
Configuration Changes:
- Removed
corsfield fromInspectorConfig - Removed
corsfield fromRunnerConfig
Impact: Any users relying on custom CORS configuration will need to migrate to actor-level origin validation in onBeforeConnect. This should be documented in release notes.
Summary
Strengths:
- ✅ Clean architectural improvement (centralized CORS handling)
- ✅ Consistent implementation across Rust and TypeScript
- ✅ Good security practices (Vary header, proper preflight handling)
- ✅ Performance improvement (OPTIONS not forwarded to actors)
Concerns:
⚠️ Critical: Very permissive CORS policy - ensure actors properly validate origins⚠️ Missing test coverage for CORS behavior⚠️ Breaking change requiring migration documentation- ℹ️ Actor CORS headers may be silently overwritten
Recommendation: Approve with the caveat that:
- Origin validation in
onBeforeConnectis verified and documented - Tests are added for CORS behavior
- Release notes explain the breaking change and migration path
Overall Assessment: This is a solid refactoring that improves the architecture. The main security concern is the permissive CORS policy, which appears intentional but needs clear documentation and verification that actors handle origin validation properly.
Code Review: CORS Handling Migration to GatewaySummaryThis PR centralizes CORS handling at the gateway level, removing per-router CORS configuration and implementing a consistent CORS policy across both Rust (pegboard-gateway) and TypeScript (RivetKit) components. The change simplifies the architecture and ensures uniform CORS behavior. ✅ Strengths1. Architectural Improvement
2. Code Quality
3. Security Considerations
🔒 Security Concerns1. CRITICAL: Overly Permissive CORS Policy The current implementation accepts requests from any origin ( Rust (lib.rs:79-86) let origin = req
.headers()
.get("origin")
.and_then(|v| v.to_str().ok())
.unwrap_or("*") // ⚠️ Accepts all origins by default
.to_string();TypeScript (cors.ts:16) const origin = c.req.header("origin") || "*"; // ⚠️ Same issueImpact:
Recommendation: let origin = req
.headers()
.get("origin")
.and_then(|v| v.to_str().ok())
.filter(|o| is_allowed_origin(o)) // Validate against allowlist
.unwrap_or_else(|| {
tracing::warn\!("request missing or invalid origin header");
"null" // Reject unknown origins
})
.to_string();The comment mentions "lets the actor handle CORS manually in 2. Header Exposure Risk .header("access-control-expose-headers", "*")This exposes all response headers to the client. While convenient, this could leak sensitive headers like:
Recommendation: Specify only the headers that need to be exposed, or document why 3. Allowed Methods Too Broad? Consider if all these methods are needed. DELETE in particular should be carefully evaluated for CORS exposure. 🐛 Potential Bugs1. Header Conflict Risk lib.rs:213-215 // Add headers from actor
for (key, value) in response_start.headers {
response_builder = response_builder.header(key, value);
}
// Add CORS headers to actual request
response_builder = response_builder
.header("access-control-allow-origin", &origin)
// ...If the actor response already includes CORS headers, they will be overwritten by the gateway. This could cause issues if:
Recommendation:
2. TypeScript: Response Header Mutation After cors.ts:45-55 await next();
// Add CORS headers to actual request
c.header("access-control-allow-origin", origin);If the response is already sent by the handler, these headers may not be applied. Verify that Hono allows header modification after ⚡ Performance Considerations1. String Allocation Overhead (Minor) lib.rs:79-86 let origin = req
.headers()
.get("origin")
.and_then(|v| v.to_str().ok())
.unwrap_or("*")
.to_string(); // String allocationThe origin is extracted before the OPTIONS check, meaning it allocates a String even for non-OPTIONS requests. Consider: let origin_header = req.headers().get("origin");
if req.method() == hyper::Method::OPTIONS {
let origin = origin_header
.and_then(|v| v.to_str().ok())
.unwrap_or("*");
// ... handle OPTIONS
}2. Double Header Iteration The code iterates headers twice: once at line 90-94 for extraction, then again for CORS at line 79-86. Minor, but could be optimized. 🧪 Test CoverageMISSING: No tests found for the new CORS functionality. Recommended Tests: Rust Tests: #[cfg(test)]
mod tests {
#[tokio::test]
async fn test_cors_preflight_with_origin() {
// Test OPTIONS request with Origin header
}
#[tokio::test]
async fn test_cors_preflight_without_origin() {
// Test OPTIONS request defaults to "*"
}
#[tokio::test]
async fn test_cors_actual_request() {
// Test CORS headers on actual request
}
#[tokio::test]
async fn test_vary_header_not_added_for_wildcard() {
// Test Vary: Origin only added when origin \!= "*"
}
}TypeScript Tests: 📋 Code Style & Best Practices✅ Good:
Minor Issues:
🔄 Breaking ChangesConfiguration Removal:
Migration Impact:
Recommendation:
📝 Documentation Needs
🎯 Action ItemsHigh Priority:
Medium Priority: Low Priority: ✨ Overall AssessmentCode Quality: ⭐⭐⭐⭐ (Good) The implementation is clean and consistent, but the overly permissive CORS policy is a significant security concern. The PR should not be merged until origin validation is added. Recommendation: Request changes to implement proper origin validation before merging. |
PR Review: Handle CORS on the GatewaySummaryThis PR moves CORS handling from the application layer (TypeScript middleware) to the gateway layer (Rust), consolidating CORS logic and allowing preflight OPTIONS requests to be handled without requiring authentication tokens. This is a well-architected change that simplifies the system. ✅ Strengths
🔍 Issues & Concerns1. Security: Overly Permissive CORS Policy
|
Merge activity
|
graphite-app
bot
deleted the
11-04-chore_guard_handle_cors_on_the_gateway
branch
No description provided.