Skip to content

fix: make request-local rate limiter more robust #671

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nyannyacha wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: make request-local rate limiter more robust #671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions ext/node/polyfills/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,14 +496,18 @@ class ClientRequest extends OutgoingMessage {

const traceId = internals.getRequestTraceId?.();
const isTraced = traceId !== null && traceId !== undefined;
if (isTraced) {
(headers as [string, string][]).push([internals.FETCH_TRACE_ID_HEADER, traceId]);
}
const rlKey = isTraced ? traceId : "";
const effectivelyTraced = isTraced;
const allowed = op_check_outbound_rate_limit(
parsedUrl.href,
rlKey,
isTraced,
effectivelyTraced,
);
if (!allowed) {
const msg = isTraced
const msg = effectivelyTraced
? `Rate limit exceeded for trace ${rlKey}`
: `Rate limit exceeded for function`;
throw new Deno.errors.RateLimitError(msg);
Expand Down
26 changes: 18 additions & 8 deletions ext/runtime/js/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import "ext:deno_http/01_http.js";

import { core, internals, primordials } from "ext:core/mod.js";
import { enterRequestContext } from "ext:runtime/request_context.js";
import {
enterRequestContext,
FETCH_TRACE_ID_HEADER,
} from "ext:runtime/request_context.js";
import { RequestPrototype } from "ext:deno_fetch/23_request.js";
import {
fromInnerResponse,
Expand Down Expand Up @@ -226,13 +229,20 @@ async function respond(requestEvent, httpConn, options, snapshot) {
/** @type {Response} */
let response;

const traceParent = requestEvent.request.headers.get("traceparent");
let traceId = null;
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];
// 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);
Comment on lines +232 to 248
Copy link

@coderabbitai coderabbitai bot Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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" -i

Repository: 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 3

Repository: 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 -50

Repository: 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 -80

Repository: 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 ts

Repository: 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 -40

Repository: 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 -30

Repository: 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 -50

Repository: 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 30

Repository: 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 2

Repository: 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 -40

Repository: 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.js

Repository: 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.

Copy link
Contributor Author

@nyannyacha nyannyacha Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. We should sign it using something like HMAC to ensure integrity.

Expand Down
8 changes: 7 additions & 1 deletion ext/runtime/js/request_context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ const { AsyncVariable } = core;

const requestTraceIdVar = new AsyncVariable();

// Header injected by the runtime's fetch / node:http into every outbound
// request to propagate the request-local trace ID for rate limiting.
// Defined once here and shared via internals to avoid scattered string literals.
const FETCH_TRACE_ID_HEADER = "x-er-fetch-trace-id";

function enterRequestContext(traceId) {
return requestTraceIdVar.enter(traceId);
}
Expand All @@ -14,5 +19,6 @@ function getRequestTraceId() {

internals.enterRequestContext = enterRequestContext;
internals.getRequestTraceId = getRequestTraceId;
internals.FETCH_TRACE_ID_HEADER = FETCH_TRACE_ID_HEADER;

export { enterRequestContext, getRequestTraceId };
export { enterRequestContext, FETCH_TRACE_ID_HEADER, getRequestTraceId };
10 changes: 8 additions & 2 deletions vendor/deno_fetch/26_fetch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,16 +396,22 @@ function fetch(input, init = { __proto__: null }) {
}

// Rate limit check.
// Inject x-er-fetch-trace-id so the next hop can read the rate-limit
// key independently of the OTel traceparent (which user code can alter).
const traceId = internals.getRequestTraceId?.();
const isTraced = traceId !== null && traceId !== undefined;
if (isTraced) {
requestObject.headers.set(internals.FETCH_TRACE_ID_HEADER, traceId);
}
const rlKey = isTraced ? traceId : "";
const effectivelyTraced = isTraced;
const allowed = op_check_outbound_rate_limit(
requestObject.url,
rlKey,
isTraced,
effectivelyTraced,
);
if (!allowed) {
const msg = isTraced
const msg = effectivelyTraced
? `Rate limit exceeded for trace ${rlKey}`
: `Rate limit exceeded for function`;
reject(new Deno.errors.RateLimitError(msg));
Expand Down
Loading