Skip to content

Conversation

@MasterPtato
Copy link
Contributor

@MasterPtato MasterPtato commented Jan 29, 2026

Fixes RVT-5332

@linear
Copy link

linear bot commented Jan 29, 2026

Copy link
Contributor Author

MasterPtato commented Jan 29, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 29, 2026

More templates

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@4058

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@4058

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@4058

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@4058

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@4058

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@4058

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@4058

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@4058

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@4058

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@4058

commit: 4e72006

@claude
Copy link

claude bot commented Jan 29, 2026

PR Review: fix(gateway): configure cors for error responses

Summary

This PR fixes CORS header configuration by moving CORS header insertion from the pegboard gateway layer to the guard-core proxy service layer. This ensures CORS headers are applied to all responses, including error responses that might be generated before reaching the actor.

Code Quality ✅

Strengths:

  • Clean separation of concerns - CORS configuration is now properly centralized in the request context
  • Good refactoring that follows the existing patterns in the codebase
  • Proper use of structured data with CorsConfig struct
  • Improved iterator-based header collection in pegboard-gateway/src/lib.rs:108-117 is more idiomatic than the previous mutable approach

Potential Issues ⚠️

1. Error Handling - Header Value Conversion (Medium Priority)

guard-core/src/proxy_service.rs:569-581

The code uses HeaderValue::from_str(&cors.allow_origin)? and HeaderValue::from_str(&cors.expose_headers)? which can fail with the ? operator. However, error responses won't have CORS headers if this fails, which defeats the purpose of this PR.

Recommendation:

// Current code can fail silently
headers.insert(
    "access-control-allow-origin",
    HeaderValue::from_str(&cors.allow_origin)?,
);

// Better approach - handle invalid header values gracefully
match HeaderValue::from_str(&cors.allow_origin) {
    Ok(value) => { headers.insert("access-control-allow-origin", value); },
    Err(e) => {
        tracing::warn\!(
            origin = %cors.allow_origin,
            error = ?e,
            "failed to create header value for cors allow-origin"
        );
        // Fallback to "*" or continue without the header
        headers.insert("access-control-allow-origin", HeaderValue::from_static("*"));
    }
}

Alternatively, since you control the origin value from the request headers, you could validate it earlier when setting the CORS config.

2. Hardcoded Header Names (Low Priority)

guard-core/src/proxy_service.rs:567-581

Header names are hardcoded as string literals instead of using constants.

Recommendation:

use hyper::header::{ACCESS_CONTROL_ALLOW_ORIGIN, ACCESS_CONTROL_ALLOW_CREDENTIALS, ACCESS_CONTROL_EXPOSE_HEADERS, VARY};

headers.insert(ACCESS_CONTROL_ALLOW_ORIGIN, HeaderValue::from_str(&cors.allow_origin)?);

However, some of these constants might not be in the standard hyper::header module. If that's the case, define them as constants at the module level for consistency.

3. Unnecessary Unused Import Removal (Nitpick)

guard/src/routing/mod.rs:4

The removal of use anyhow::Result; appears to be an unrelated cleanup. While this is fine, it's worth noting this is technically unrelated to the CORS fix.

Performance Considerations ✅

  • The change adds minimal overhead - just a conditional check and a few header insertions
  • Iterator-based header collection in pegboard-gateway is more efficient than the previous approach
  • No blocking operations or expensive allocations introduced

Security Considerations ⚠️

1. Origin Validation (Important)

pegboard-gateway/src/lib.rs:100-105

The code extracts the origin from request headers and echoes it back without validation:

let origin = req
    .headers()
    .get("origin")
    .and_then(|v| v.to_str().ok())
    .unwrap_or("*")
    .to_string();

While the comment at line 122-124 mentions "This implementation allows requests from anywhere and lets the actor handle CORS manually in onBeforeConnect", there's a subtle security concern:

  • Cache Poisoning Risk: If there are any caching layers between the client and the gateway, echoing arbitrary origins without validation could lead to cache poisoning attacks
  • The Vary: Origin header (line 148, 585) mitigates this, which is good
  • However, malicious origins could still be echoed back

Current mitigation: The code correctly adds Vary: Origin when origin is not "*", which is the proper defense.

Additional recommendation: Consider documenting this behavior more explicitly or adding origin validation if there's a known set of allowed origins.

2. CORS Credentials with Wildcard (Secure ✅)

The code properly handles the constraint that Access-Control-Allow-Credentials: true cannot be used with Access-Control-Allow-Origin: *. It always echoes the actual origin when credentials are enabled, which is correct.

Test Coverage ⚠️

Missing tests:

  • No new tests added for the CORS header behavior
  • No tests verifying CORS headers appear on error responses
  • No tests for the header value conversion error paths

Recommendations:

  1. Add a test in guard-core/tests/ that verifies CORS headers appear on both successful and error responses
  2. Test the edge case where the origin header contains invalid characters
  3. Test that the Vary: Origin header is properly set

Example test structure:

#[tokio::test]
async fn test_cors_headers_on_error_response() {
    // Setup test server that returns an error
    // Send request with Origin header
    // Assert response has proper CORS headers even on error
}

Architecture ✅

The architectural change is sound:

  • Moving CORS handling to the proxy service layer ensures consistent behavior
  • The RequestContext::cors field is the right place to store this configuration
  • The flow is clear: gateway sets CORS config → proxy service applies headers

Documentation 📝

Missing:

  • No code comments explaining why CORS is set at two different layers (OPTIONS in gateway, actual headers in proxy_service)
  • The relationship between the OPTIONS preflight handling and the actual request CORS headers could be clearer

Recommendation:
Add a comment in proxy_service.rs explaining:

// Apply CORS headers configured by the gateway layer.
// This ensures CORS headers are present on all responses, including errors
// that occur before reaching the actor.
if let Some(cors) = &req_ctx.cors {
    // ...
}

Conventional Commit Format ✅

The commit message follows the conventional commit format correctly: fix(gateway): configure cors for error responses

Summary of Recommendations

Must Fix:

  1. Handle HeaderValue::from_str() errors gracefully to ensure CORS headers are always applied (defeats the purpose if it fails)

Should Consider:
2. Add test coverage for CORS headers on error responses
3. Add inline comments explaining the two-layer CORS handling approach

Nice to Have:
4. Use header constants instead of string literals
5. Document origin echoing security considerations

Overall Assessment

This is a well-structured fix that addresses a real issue with CORS headers not appearing on error responses. The main concern is the error handling in header value conversion, which should be addressed to ensure the fix works reliably. Otherwise, the code quality is good and follows the repository's patterns.

Approval Status: ⚠️ Approve with minor changes recommended

The error handling issue should be fixed, but it won't break existing functionality - it just means CORS headers might still be missing in rare cases with malformed origin headers.

@MasterPtato MasterPtato force-pushed the 01-28-fix_gateway_configure_cors_for_error_responses branch from 03e46b7 to c9720e1 Compare January 29, 2026 00:46
@MasterPtato MasterPtato force-pushed the 01-28-fix_gateway_configure_cors_for_error_responses branch from c9720e1 to 4e72006 Compare January 29, 2026 01:27
@MasterPtato MasterPtato force-pushed the 01-28-chore_guard_clean_up_guard branch from d078517 to 30b52ba Compare January 29, 2026 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants