chore: remove headers through guard

[MasterPtato]

No description provided.

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Oct 30, 2025 2:25am

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Oct 30, 2025 2:25am
rivet-inspector	Ignored	Preview		Oct 30, 2025 2:25am
rivet-site	Ignored	Preview		Oct 30, 2025 2:25am

[MasterPtato]

• fix: add version rollback check #3291
• fix: integrate sentry with tracing and axum #3290
• chore: remove headers through guard #3288 👈 (View in Graphite)
• feat: add ws msg ack to runner protocol #3275 : 1 other dependent PR (#3282 )
• fix: improve reconnection with sleeping and tunnel #3261
• main

---

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.

[claude]

PR Review: Remove headers through guard

Summary

This PR removes internal Rivet routing headers (x-rivet-target, x-rivet-actor, x-rivet-token) from proxied requests before forwarding them to upstream services. This is a security best practice that prevents internal routing metadata from leaking to backend services.

✅ Positive Aspects

1. Security improvement: Stripping internal headers before proxying is the correct approach. These headers are used for Guard's internal routing logic and should not be exposed to upstream services.

2. Clean implementation: The header removal is placed at the right location - after creating the request builder but before adding proxy headers from the original request.

3. Consistent constants: Good move consolidating the header name constants in guard-core/src/proxy_service.rs (lines 38-40) rather than having them scattered across files.

4. Documentation: Added helpful doc comment for proxied_request_builder function (line 1067).

🔍 Potential Issues

Critical: WebSocket header leakage

The current implementation only removes these headers for HTTP requests in proxied_request_builder (line 1102-1104), but NOT for WebSocket upgrades.

Looking at handle_websocket_upgrade around line 1313-1323, the WebSocket request uses add_proxy_headers_with_addr which copies all headers except Host (line 2346-2350):

// Copy headers except Host
for (key, value) in original_headers.iter() {
    if key != hyper::header::HOST {
        headers.insert(key.clone(), value.clone());
    }
}

This means x-rivet-target, x-rivet-actor, and x-rivet-token would still be forwarded in WebSocket upgrade requests. This creates an inconsistency and potential security gap.

Recommendation: Apply the same header filtering for WebSocket requests. You have two options:

1. Option A - Remove headers before calling add_proxy_headers_with_addr in the WebSocket handler (around line 1313):

// Remove internal routing headers before adding proxy headers
ws_request.headers_mut().remove(X_RIVET_TARGET);
ws_request.headers_mut().remove(X_RIVET_ACTOR);
ws_request.headers_mut().remove(X_RIVET_TOKEN);

if let Err(err) = add_proxy_headers_with_addr(
    ws_request.headers_mut(),
    &req_headers,
    remote_addr,
) {
    // ...
}

2. Option B - Modify add_proxy_headers_with_addr to skip these headers in the loop (around line 2347):

for (key, value) in original_headers.iter() {
    if key != hyper::header::HOST 
        && key != X_RIVET_TARGET 
        && key != X_RIVET_ACTOR 
        && key != X_RIVET_TOKEN {
        headers.insert(key.clone(), value.clone());
    }
}

Option B is cleaner and ensures consistency across all uses of add_proxy_headers_with_addr.

Minor: Unused constant removal

The removal of X_RIVET_AMESPACE (note the typo - should be NAMESPACE) in guard/src/routing/pegboard_gateway.rs:12 is good cleanup. However, verify this wasn't being used elsewhere - the typo suggests it may have been dead code already.

📋 Testing Recommendations

1. WebSocket header verification: Add a test to verify that internal headers are NOT forwarded in WebSocket upgrade requests
2. HTTP header verification: Add a test to verify that internal headers are NOT forwarded in regular HTTP requests
3. Integration test: Ensure actor routing still works correctly after stripping these headers (they should only be used for routing decisions, not passed through)

🎯 Code Quality

• Style: Follows Rust conventions ✅
• Error handling: N/A for this change ✅
• Performance: No performance impact ✅
• Consistency: Inconsistent between HTTP and WebSocket handling ⚠️

📝 Additional Notes

Looking at the codebase, these headers are used in multiple places for routing:

• guard/src/cache/mod.rs:50 - reads X_RIVET_TARGET
• guard/src/cache/actor.rs:18 - reads X_RIVET_ACTOR
• guard/src/routing/runner.rs:79 - reads X_RIVET_TOKEN

These are all before the proxying happens, which is correct. The headers are used to determine where to route, then stripped before forwarding.

Verdict

Approve with changes required - The HTTP implementation is correct, but the WebSocket implementation needs the same header filtering to close the security gap and maintain consistency.

---

🤖 Generated with Claude Code

[claude]

Updated PR Review: Remove headers through guard

I reviewed this PR previously and the critical WebSocket header leakage issue remains unaddressed. Here's an updated review:

---

🔴 Critical Issue: WebSocket Header Leakage Still Present

The current implementation only removes internal headers for HTTP requests (proxy_service.rs:1102-1104), but NOT for WebSocket upgrades.

Location of the bug: proxy_service.rs:1313-1316

The WebSocket upgrade handler calls add_proxy_headers_with_addr which copies all headers except Host (see line 2346-2350). This means the internal routing headers (x-rivet-target, x-rivet-actor, x-rivet-token) are still being forwarded to upstream WebSocket services.

🔧 Recommended Fix

The cleanest solution is to modify add_proxy_headers_with_addr (line 2340) to skip the internal headers:

fn add_proxy_headers_with_addr(
    headers: &mut hyper::HeaderMap,
    original_headers: &hyper::HeaderMap,
    remote_addr: SocketAddr,
) -> Result<()> {
    // Copy headers except Host and internal Rivet headers
    for (key, value) in original_headers.iter() {
        if key != hyper::header::HOST 
            && key != X_RIVET_TARGET 
            && key != X_RIVET_ACTOR 
            && key != X_RIVET_TOKEN {
            headers.insert(key.clone(), value.clone());
        }
    }
    
    // ... rest of the function
}

This ensures both HTTP and WebSocket requests are handled consistently.

✅ What's Good

1. Security-focused change: Removing internal headers is the right thing to do
2. Constants consolidated: Moving header constants to guard-core is good for consistency
3. Documentation added: The doc comment on proxied_request_builder is helpful
4. Dead code removal: Removing the typo'd X_RIVET_AMESPACE constant is good cleanup

📊 Other Files

universaldb/src/driver/rocksdb/transaction_task.rs:140

• The lifetime annotation change ('_) is correct and improves code clarity
• This appears to be an unrelated fix that could have been a separate commit, but it's fine

🧪 Testing Needed

Before merging, please verify:

1. Internal headers are NOT present in proxied HTTP requests (covered by current fix ✅)
2. Internal headers are NOT present in proxied WebSocket requests (currently broken ❌)
3. Routing still works correctly (headers should only affect routing, not be forwarded)

Verdict

Request changes - The WebSocket header leakage must be fixed before merging. This is a security issue that could expose internal routing metadata to upstream services.

---

🤖 Generated with Claude Code