[Graphite MQ] Draft PR GROUP:spec_1687e5 (PRs 3341, 3344)

[graphite-app]

This draft PR was created by the Graphite merge queue.
Trunk will be fast forwarded to the HEAD of this PR when CI passes, and the original PRs will be closed.

The following PRs are included in this draft PR:

• #3341
• #3344

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 5, 2025 0:09am

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 5, 2025 0:09am
rivet-inspector	Ignored	Preview		Nov 5, 2025 0:09am
rivet-site	Ignored	Preview		Nov 5, 2025 0:09am

[claude]

PR Review: CORS Gateway Handling & Engine Version Auto-Update

This PR combines two changes:

1. PR chore(guard): handle cors on the gateway #3341: Moving CORS handling to the gateway level
2. PR chore(rivetkit): auto-update auto-start engine version #3344: Auto-updating engine version to match RivetKit version

---

Overall Assessment

✅ Approved - The changes are well-implemented with good security considerations. Minor suggestions below.

---

1. CORS Gateway Implementation (PR #3341)

Strengths

✅ Security-conscious design: Properly echoes the Origin header instead of using * when credentials are enabled, preventing CORS credential vulnerabilities.

✅ Cache poisoning prevention: Correctly adds Vary: Origin header when echoing origins to prevent cache poisoning attacks.

✅ Consistent implementation: The Rust gateway code in pegboard-gateway/src/lib.rs mirrors the TypeScript CORS middleware in common/cors.ts, ensuring consistent behavior across the stack.

✅ Good architectural reasoning: The comment on lines 99-104 clearly explains why CORS must be handled at the gateway level (OPTIONS requests can't reach actors without the x-rivet-token header).

✅ Proper OPTIONS handling: Returns 204 No Content with empty body for preflight requests, which is HTTP spec-compliant.

Potential Issues

⚠️ Overly permissive CORS policy: The implementation allows requests from any origin (unwrap_or("*") on line 85). While the comment mentions "lets the actor handle CORS manually in onBeforeConnect", this could be a security risk if:

• Actors don't properly validate origins in onBeforeConnect
• Sensitive endpoints are exposed without proper origin validation

Recommendation: Consider documenting the security implications and ensuring actors are aware they MUST validate origins if needed.

⚠️ No origin validation: Unlike the removed defaultCors configuration which validated origins against a whitelist (removed from inspector/config.ts:21-40), the new implementation trusts all origins. This is a significant security posture change.

Recommendation: Add documentation or runtime warnings if sensitive operations (like inspector access) are exposed without origin restrictions.

⚠️ Missing header sanitization: On line 92 in lib.rs, header values that fail to_str() are silently skipped. This is likely fine, but non-UTF8 headers might cause unexpected behavior.

Code Quality

✅ Clean removal: Properly removed the old CORS configuration from multiple files without leaving dead code.

✅ Logging: Good use of tracing::debug! on line 106 for observability.

⚠️ Code duplication: The CORS header logic appears in two places in the Rust code (lines 115-130 for OPTIONS, lines 217-226 for regular responses). Consider extracting to a helper function.

Suggested refactor:

fn add_cors_headers(response: ResponseBuilder, origin: &str) -> ResponseBuilder {
    let mut response = response
        .header("access-control-allow-origin", origin)
        .header("access-control-allow-credentials", "true")
        .header("access-control-expose-headers", "*");
    
    if origin != "*" {
        response = response.header("vary", "Origin");
    }
    response
}

---

2. Engine Version Auto-Update (PR #3344)

Strengths

✅ Sensible default: Using VERSION (from package.json) instead of hardcoded "25.8.2" means the engine version will automatically stay in sync with RivetKit releases.

✅ Maintains override capability: The RIVET_RUN_ENGINE_VERSION environment variable still allows manual override if needed.

Potential Issues

⚠️ Breaking change risk: If the RivetKit TypeScript version doesn't always match compatible engine versions, this could cause runtime issues.

Recommendation: Ensure your release process keeps these versions synchronized, or add validation to check engine compatibility.

⚠️ Documentation needed: The change from explicit version to VERSION makes the default less obvious when reading code.

Recommendation: Add a comment explaining that this uses the RivetKit package version.

Suggested documentation:

.default(
    // Use RivetKit package version by default to keep engine in sync
    () => getEnvUniversal("RIVET_RUN_ENGINE_VERSION") ?? VERSION,
),

---

3. Test Coverage

❌ No tests found: I couldn't find any tests for the new CORS functionality or engine version behavior.

Recommendations:

1. Add integration tests verifying CORS headers are set correctly
2. Test that OPTIONS preflight requests return 204 with correct headers
3. Test that the `Vary: Origin" header is only added when origin != "*"
4. Test engine version defaulting behavior

---

4. Style & Conventions

✅ Follows CLAUDE.md:

• Uses lowercase log messages ("handling OPTIONS preflight request at gateway")
• Uses structured logging with tracing
• Proper error handling with anyhow

⚠️ TypeScript formatting: Lines 39-40 in cors.ts manually delete headers from the response. Consider if this is necessary or if returning early prevents these from being set.

---

Security Considerations

🔒 Key Security Notes:

1. Origin reflection: Echoing the Origin header is safe when done correctly (with Vary header), which this implementation does.

2. Credentials exposure: The access-control-allow-credentials: true header is set globally. Ensure all endpoints can safely accept credentials, or this should be configurable per-route.

3. Wildcard headers: Using access-control-expose-headers: * and access-control-allow-headers: * (when request doesn't specify) is permissive. Consider if this is necessary.

4. 24-hour cache: access-control-max-age: 86400 caches preflight for 24 hours. If CORS policy needs to change, clients won't see changes for up to a day.

---

Performance Considerations

✅ Early return for OPTIONS: Handling OPTIONS at the gateway without forwarding to actors is efficient.

✅ Minimal overhead: Adding headers has negligible performance impact.

---

Recommendations Summary

High Priority:

1. ⚠️ Document security implications of permissive CORS policy
2. ⚠️ Add tests for CORS functionality
3. ⚠️ Consider if wildcard origins are appropriate for all routes

Medium Priority:
4. Refactor duplicated CORS header code in Rust
5. Add inline documentation for VERSION default
6. Consider making credentials/expose-headers configurable

Low Priority:
7. Review if header deletion in TypeScript CORS is necessary

---

Conclusion

The implementation is solid and security-aware. The main concern is the shift from a restrictive CORS policy (with origin whitelist) to a permissive one (allowing all origins). Ensure this is intentional and that actors properly validate origins when needed.

Overall: ✅ Approved with suggestions