fix: make request-local rate limiter more robust#671
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThese changes implement a trace ID header propagation mechanism across the HTTP stack. A new header constant Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant Fetch as Fetch Handler<br/>(vendor/deno_fetch)
participant Runtime as Runtime HTTP<br/>(ext/runtime/js)
participant RequestCtx as Request Context<br/>(ext/runtime/js)
participant RateLimit as Rate Limiter
Client->>Fetch: Initiate HTTP Request
Fetch->>Fetch: Check if tracing active<br/>(isTraced)
alt Tracing Active
Fetch->>Fetch: Inject FETCH_TRACE_ID_HEADER<br/>into request
end
Fetch->>RateLimit: Check rate limit<br/>(effectivelyTraced flag)
RateLimit->>RateLimit: Use trace ID for<br/>rate-limit key
RateLimit-->>Fetch: Rate limit decision
Fetch->>Runtime: Forward request with headers
Runtime->>RequestCtx: Extract traceId
RequestCtx->>RequestCtx: Check FETCH_TRACE_ID_HEADER
alt Header Present
RequestCtx->>RequestCtx: Use header value
else Header Absent
RequestCtx->>RequestCtx: Parse traceparent header
end
Runtime->>Runtime: Enter request context<br/>with extracted traceId
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/runtime/js/http.js`:
- Around line 232-248: Do not trust the incoming FETCH_TRACE_ID_HEADER from
external requests: remove the unconditional read of
requestEvent.request.headers.get(FETCH_TRACE_ID_HEADER) in the trace-id
resolution logic and instead either (a) strip that header at the HTTP handler
boundary so it never reaches this code, or (b) only accept it after a proven
internal validation (e.g., an internal hop marker or request origin check)
before passing it into enterRequestContext; keep the existing traceparent
parsing fallback (the traceparent split/parts logic) as the safe default and
ensure only validated/internal trace IDs are handed to enterRequestContext.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7ca57b99-ff5b-49e4-82a5-95eef3422ae3
📒 Files selected for processing (4)
ext/node/polyfills/http.tsext/runtime/js/http.jsext/runtime/js/request_context.jsvendor/deno_fetch/26_fetch.js
| // x-er-fetch-trace-id takes priority: injected by the runtime's fetch / | ||
| // node:http into every outbound request, so it is tamper-proof once set. | ||
| // On the very first hop it won't be present, so we fall back to parsing | ||
| // the W3C traceparent header. | ||
| let traceId = | ||
| requestEvent.request.headers.get(FETCH_TRACE_ID_HEADER) ?? null; | ||
| if (traceId === null) { | ||
| const traceParent = requestEvent.request.headers.get("traceparent"); | ||
| if (traceParent) { | ||
| // traceparent format: 00-{trace-id}-{parent-id}-{flags} | ||
| const parts = traceParent.split("-"); | ||
| if (parts.length >= 4 && parts[1].length === 32) { | ||
| traceId = parts[1]; | ||
| } | ||
| } | ||
| } | ||
| const prevCtx = enterRequestContext(traceId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for x-er-fetch-trace-id header usages and FETCH_TRACE_ID_HEADER
rg -n "x-er-fetch-trace-id" -i --type-list | head -20
rg -n "x-er-fetch-trace-id" -iRepository: supabase/edge-runtime
Length of output: 775
🏁 Script executed:
#!/bin/bash
# Search for FETCH_TRACE_ID_HEADER definition and usage
rg -n "FETCH_TRACE_ID_HEADER" -A 3 -B 3Repository: supabase/edge-runtime
Length of output: 3196
🏁 Script executed:
#!/bin/bash
# Check for any header stripping or validation logic in request handling
rg -n "headers.delete|headers.remove|strip" --type js --type ts -A 2 -B 2 | head -50Repository: supabase/edge-runtime
Length of output: 3171
🏁 Script executed:
#!/bin/bash
# Look for rate limiting code and how traceId is used
rg -n "is_traced|bucket|rate.limit" --type rs --type js -A 3 -B 1 | head -80Repository: supabase/edge-runtime
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Find where http.js is imported/used
rg -n "from.*http.js|import.*http.js" --type js --type tsRepository: supabase/edge-runtime
Length of output: 481
🏁 Script executed:
#!/bin/bash
# Look for context about requestEvent - where does it come from?
rg -n "requestEvent" ext/runtime/js/http.js -B 5 -A 5 | head -40Repository: supabase/edge-runtime
Length of output: 1678
🏁 Script executed:
#!/bin/bash
# Check if there's any middleware or edge handling that processes requests before http.js
rg -n "middleware|edge|gateway" --type js --type ts -i | head -30Repository: supabase/edge-runtime
Length of output: 3116
🏁 Script executed:
#!/bin/bash
# Look for the HTTP server setup/listener to understand request origin
rg -n "listen|serve|Deno.serve|createServer" --type js --type ts -A 3 | head -50Repository: supabase/edge-runtime
Length of output: 2997
🏁 Script executed:
#!/bin/bash
# Check the handleHttp function more completely to see how it processes connections
rg -n "handleHttp" ext/runtime/js/http.js -B 10 -A 30Repository: supabase/edge-runtime
Length of output: 2100
🏁 Script executed:
#!/bin/bash
# Look for where the HTTP server is actually started to understand request flow
rg -n "function serve|export.*serve" ext/runtime/js/http.js -A 5 -B 2Repository: supabase/edge-runtime
Length of output: 620
🏁 Script executed:
#!/antml/bash
# Check if there's any request validation or header filtering in the codebase
rg -n "request\.headers|headers\.delete|delete.*header|untrusted" --type js --type ts | grep -i "x-er\|header\|delete\|strip\|untrusted" | head -40Repository: supabase/edge-runtime
Length of output: 1160
🏁 Script executed:
#!/bin/bash
# Look at the full respond function to understand context better
sed -n '227,300p' ext/runtime/js/http.jsRepository: supabase/edge-runtime
Length of output: 2430
Security risk: Incoming x-er-fetch-trace-id header is trusted without validation.
An external client can set this header on their request, allowing them to control the trace ID used for their request context. This header is intended for propagation between internal hops, but the code does not validate that it originated from an internal request—it accepts it directly from any incoming HTTP request.
The header should be stripped from incoming requests at the HTTP handler level, or validated to ensure it originates from an internal hop only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ext/runtime/js/http.js` around lines 232 - 248, Do not trust the incoming
FETCH_TRACE_ID_HEADER from external requests: remove the unconditional read of
requestEvent.request.headers.get(FETCH_TRACE_ID_HEADER) in the trace-id
resolution logic and instead either (a) strip that header at the HTTP handler
boundary so it never reaches this code, or (b) only accept it after a proven
internal validation (e.g., an internal hop marker or request origin check)
before passing it into enterRequestContext; keep the existing traceparent
parsing fallback (the traceparent split/parts logic) as the safe default and
ensure only validated/internal trace IDs are handed to enterRequestContext.
There was a problem hiding this comment.
That's a good point. We should sign it using something like HMAC to ensure integrity.
1 participant
What kind of change does this PR introduce?
Enhancement
Description
...WIP