Skip to content

Security fix: isolate health checks from admin API rate-limit bucket #126

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

Closed
cursor wants to merge 2 commits into from
Closed

Security fix: isolate health checks from admin API rate-limit bucket #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79280ec
fix(http): isolate health checks from admin API rate-limit bucket
cursoragent Aug 6, 2026
23023dc
Update Cargo.lock
github-actions[bot] Aug 6, 2026
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
4 changes: 2 additions & 2 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

57 changes: 57 additions & 0 deletions src/http.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2730,6 +2730,46 @@ mod tests {
..Default::default()
});
let app = router(state);
let (header, value) = bearer("token");

for _ in 0..3 {
let resp = app
.clone()
.oneshot(
Request::builder()
.uri("/api/v1/streams")
.header(header, &value)
.body(Body::empty())
.unwrap(),
)
.await
.unwrap();
assert_eq!(resp.status(), StatusCode::OK);
}

let resp = app
.oneshot(
Request::builder()
.uri("/api/v1/streams")
.header(header, value)
.body(Body::empty())
.unwrap(),
)
.await
.unwrap();
assert_eq!(resp.status(), StatusCode::TOO_MANY_REQUESTS);
}

#[tokio::test]
async fn health_rate_limit_does_not_exhaust_authenticated_api_budget() {
let state = test_state_with_config(ServerConfig {
api_token: "token".to_string(),
http_rate_limit_api: 3,
http_rate_limit_default: 3,
..Default::default()
});
let app = router(state);
let (header, value) = bearer("token");

for _ in 0..3 {
let resp = app
Expand All @@ -2746,6 +2786,7 @@ mod tests {
}

let resp = app
.clone()
.oneshot(
Request::builder()
.uri("/api/v1/health")
Expand All @@ -2755,6 +2796,22 @@ mod tests {
.await
.unwrap();
assert_eq!(resp.status(), StatusCode::TOO_MANY_REQUESTS);

let resp = app
.oneshot(
Request::builder()
.uri("/api/v1/streams")
.header(header, value)
.body(Body::empty())
.unwrap(),
)
.await
.unwrap();
assert_eq!(
resp.status(),
StatusCode::OK,
"authenticated admin API must not share the public health rate-limit bucket"
);
}

#[tokio::test]
Expand Down
54 changes: 47 additions & 7 deletions src/rate_limit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,19 @@ impl RateLimiter {
true
}

fn limit_for_path(&self, path: &str) -> usize {
if path.starts_with("/api/") {
self.config.api_max
/// Classify a request path into a rate-limit bucket and its per-window cap.
/// `/api/v1/health` is public and probed by orchestrators; it must not share
/// the authenticated `/api/*` bucket or unauthenticated health traffic can
/// exhaust the admin API budget for the same client IP.
fn classify_path(&self, path: &str) -> (usize, &'static str) {
if path == "/api/v1/health" {
(self.config.default_max, "health")
Comment on lines +134 to +135

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Align the health cap with the documented rate-limit setting

When operators tune the published rate-limit settings, this silently makes /api/v1/health use HTTP_RATE_LIMIT_DEFAULT, although .env.example documents HTTP_RATE_LIMIT_API as covering /api/* and the default setting as covering all other routes. For example, a deployment with API=120 and DEFAULT=1 now returns 429 on its second health probe even though its documented /api/* allowance is 120. Either expose/document a health-specific setting or update the configuration contract so deployments do not unexpectedly break health monitoring.

Useful? React with 👍 / 👎.

} else if path.starts_with("/api/") {
(self.config.api_max, "api")
Comment on lines +136 to +137

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Separate unauthenticated requests from the admin API bucket

When an attacker shares the resolved client IP with an administrator, they can still exhaust this bucket by repeatedly requesting a protected route such as GET /api/v1/streams without a bearer token. The rate-limit middleware runs before handle_streams_list performs authentication, so these 401 responses consume the same api budget and the next legitimate authenticated request receives 429. Thus, moving only /api/v1/health leaves the claimed admin API denial-of-service fix trivially bypassable; the protected budget needs to distinguish authenticated requests rather than only the health path.

Useful? React with 👍 / 👎.

} else if path.starts_with("/stats") {
self.config.stats_max
(self.config.stats_max, "stats")
Comment on lines 138 to +139

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve independent budgets for the two stats endpoints

When the same client polls both /stats and /stats-nginx, this classification now gives both requests the identical {peer}:stats key. Before this commit the key was derived from the first path segment, so each endpoint independently allowed stats_max requests; a workload that legitimately made up to that many requests to each endpoint can now receive 429 after only stats_max combined requests. Preserve the previous per-endpoint keys for non-health routes unless this unrelated compatibility change is intentional.

Useful? React with 👍 / 👎.

} else {
self.config.default_max
(self.config.default_max, "default")
}
}
}
Expand Down Expand Up @@ -193,8 +199,8 @@ pub async fn middleware(
) -> Response {
let path = request.uri().path();
let peer = client_ip(&request, limiter.trusted_proxies.as_slice());
let key = format!("{}:{}", peer, path.split('/').nth(1).unwrap_or(""));
let max = limiter.limit_for_path(path);
let (max, bucket) = limiter.classify_path(path);
let key = format!("{peer}:{bucket}");
if !limiter.check(&key, max) {
let method = request.method().as_str();
crate::log_warn!("HTTP: {method} {path} from {peer} → 429 rate limit exceeded");
Expand Down Expand Up @@ -295,6 +301,40 @@ mod tests {
);
}

#[test]
fn health_uses_separate_bucket_from_authenticated_api() {
let limiter = RateLimiter::new(
HttpRateLimitConfig {
api_max: 3,
default_max: 60,
..HttpRateLimitConfig::default()
},
Vec::new(),
);

let (health_max, health_bucket) = limiter.classify_path("/api/v1/health");
let (api_max, api_bucket) = limiter.classify_path("/api/v1/streams");
assert_eq!(health_bucket, "health");
assert_eq!(api_bucket, "api");
assert_eq!(health_max, 60);
assert_eq!(api_max, 3);

for i in 0..3 {
assert!(
limiter.check("127.0.0.1:api", api_max),
"api request {i} should succeed"
);
}
assert!(
!limiter.check("127.0.0.1:api", api_max),
"api bucket should be exhausted"
);
assert!(
limiter.check("127.0.0.1:health", health_max),
"health bucket must remain independent of the api bucket"
);
}

#[test]
fn stats_limit_uses_configured_bucket() {
let limiter = RateLimiter::new(
Expand Down