feat: issue-642#706
Conversation
Autonomously-by: Codex:GPT-5
ceilf6
requested review from
chenhengqi,
fslongjin,
ls-ggg,
tinklone and
up2wing
as code owners
Review: PR #706 — Webhook Delivery for Sandbox Lifecycle EventsOverall, this is a well-structured implementation with thorough tests for happy paths, solid SSRF guards at the URL validation layer, and careful secret redaction. Below are noteworthy issues organized by severity. 🔴 High Severity1. The function reads up to 2048 bytes from the error response but returns 2. DNS rebinding bypasses startup-only IP validation ( Hostnames are resolved to IPs and validated against the private-IP blocklist only at startup, but 3. No replay protection on signed payloads ( The HMAC covers only the request body. 🟠 Medium Severity4. Two-tier silent event drops with no backpressure ( Events are silently dropped at two levels: 5. Background worker panic silently kills all webhook delivery ( The 6. Unnecessary Each subscribed endpoint gets a deep-cloned 7. env var mutation races with parallel tests (
8. Webhook receiver startup message doesn't match behavior ( Says "listening on .../webhook" but 🟡 Low Severity
✅ What's Done Well
|
There was a problem hiding this comment.
Pull request overview
This PR adds webhook delivery for CubeAPI sandbox lifecycle events by introducing an HTTP webhook logging backend, configuration via env/JSON, retry with exponential backoff, optional HMAC-SHA256 signing, and accompanying receiver/examples plus English/Chinese documentation.
Changes:
- Implement
HttpLoggerwebhook backend with async queueing, per-endpoint subscriptions, retries/backoff, and optional signatures. - Add CubeAPI configuration/env examples and docs pages for webhook payloads/headers/verification and a runnable stdlib receiver.
- Wire webhook backend into CubeAPI startup logger fanout when endpoints are configured.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/webhook-receiver/receiver.py | Minimal Python stdlib webhook receiver with optional signature verification and WeCom forwarding |
| examples/webhook-receiver/README.md | Run/config instructions and example payloads for the receiver |
| docs/guide/webhooks.md | English integration guide for webhook configuration, payloads, headers, and signature verification |
| docs/zh/guide/webhooks.md | Chinese integration guide mirroring the English webhook documentation |
| docs/.vitepress/config.mjs | Adds webhook docs links to the Integration/集成 nav |
| deploy/one-click/env.example | Documents webhook-related env vars for one-click deployments |
| CubeAPI/src/main.rs | Enables webhook backend in the logger pipeline when configured |
| CubeAPI/src/logging/http.rs | Implements HTTP webhook backend, endpoint filtering, delivery, retry/backoff, and signing + tests |
| CubeAPI/src/config/mod.rs | Adds webhook endpoint and delivery tuning configuration via env vars |
| CubeAPI/README.md | Documents webhook env vars and links to webhook guide |
| CubeAPI/Cargo.toml | Adds crypto/encoding deps for HMAC-SHA256 signatures |
| CubeAPI/Cargo.lock | Locks new Rust dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let status = response.status(); | ||
| let text = response.text().await.unwrap_or_default(); | ||
| anyhow::bail!("webhook endpoint returned {status}: {text}"); |
| fn default_webhook_queue_capacity() -> usize { | ||
| env_parse("CUBE_API_WEBHOOK_QUEUE_CAPACITY", 1024) | ||
| } |
| tokio::spawn(async move { | ||
| while let Some(msg) = rx.recv().await { | ||
| match msg { | ||
| Msg::Event(event) => { | ||
| let deliveries = endpoints | ||
| .iter() | ||
| .filter(|endpoint| endpoint_subscribes(endpoint, &event.event)) | ||
| .cloned() | ||
| .map(|endpoint| { | ||
| deliver_with_retries( | ||
| client.clone(), | ||
| endpoint, | ||
| event.clone(), | ||
| options.clone(), | ||
| ) | ||
| }); | ||
| futures::future::join_all(deliveries).await; | ||
| } | ||
| Msg::Flush(reply) => { | ||
| let _ = reply.send(()); | ||
| } | ||
| } | ||
| } | ||
| }); |
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/u/20101770?s=60&v=4){kind=small}
| options: HttpWebhookOptions, | ||
| mut rx: mpsc::Receiver<Msg>, | ||
| ) { | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Single worker is a throughput bottleneck. This spawn_worker dequeues events one at a time, and join_all waits for all endpoint deliveries to complete before picking up the next event. A single slow endpoint (or one that hits the 5s timeout) stalls delivery to all other endpoints and accelerates queue filling. Consider per-endpoint channels or spawning independent per-endpoint tasks so slow deliveries don't cascade.
| } | ||
|
|
||
| let status = response.status(); | ||
| let text = response.text().await.unwrap_or_default(); |
There was a problem hiding this comment.
Silent error body read failure. If response.text() fails (connection reset, encoding issue), its error is discarded and the logged error will say something like "webhook endpoint returned 500: " — losing diagnostic value. Log the read error before defaulting to empty string.
| }; | ||
| let events = std::env::var("CUBE_API_WEBHOOK_EVENTS") | ||
| .ok() | ||
| .map(|value| split_csv(&value)) |
There was a problem hiding this comment.
Silent fallback on parse failure. When an env var is present but unparseable (e.g., CUBE_API_WEBHOOK_QUEUE_CAPACITY=not-a-number), this returns the default with no warning. Operators won't know their config was ignored. Add a warn!() when the var exists but parsing fails.
|
|
||
| type HmacSha256 = Hmac<Sha256>; | ||
|
|
||
| /// Configuration for the HTTP webhook backend. |
There was a problem hiding this comment.
Dead code. HttpLoggerConfig and HttpLogger::new() are never used by the new webhook system — the only call site in main.rs uses from_endpoints(). The batch_size and flush_interval_secs fields are never read, but their names imply batching semantics that don't exist (events are forwarded one-at-a-time). Remove ~30 lines and the #[allow(dead_code)] annotation.
|
|
||
|
|
||
| def main() -> None: | ||
| host = os.getenv("WEBHOOK_HOST", "0.0.0.0") |
There was a problem hiding this comment.
Defaulting to 0.0.0.0 exposes the receiver to all network interfaces. This is the example receiver that users may deploy as-is. Combined with the optional nature of HMAC verification, this is a risk. Change the default to 127.0.0.1 and add a Content-Length cap (the current code reads however many bytes the header specifies).
|
@ceilf6/repo-guard CR |
代码评审报告: feat: issue-642风险等级: 中 级联分析
问题发现
行级发现
Karpathy 评审
缺失覆盖
|
Autonomously-by: Codex:GPT-5
| } | ||
| } | ||
| } | ||
| result = deliveries.join_next(), if !deliveries.is_empty() => { |
There was a problem hiding this comment.
JoinSet starvation under sustained load — The biased tokio::select! prioritizes rx.recv() (line 154). When events arrive continuously, the deliveries.join_next() arm at line 184 is never polled because rx.recv() is always immediately ready. Completed delivery tasks accumulate in the JoinSet without being joined, causing unbounded memory growth over time.
Fix: Drain completed tasks non-blockingly after processing each event using JoinSet::try_join_next() before re-entering select!:
while let Ok(Some(result)) = deliveries.try_join_next() {
if let Err(err) = result {
warn!(error = %err, "HttpLogger: webhook delivery task failed");
}
}
| Ok(url.to_string()) | ||
| } | ||
|
|
||
| fn validate_webhook_ip_literal(ip: IpAddr, raw_url: &str) -> anyhow::Result<()> { |
There was a problem hiding this comment.
SSRF bypass via IPv4-mapped IPv6 addresses — validate_webhook_ip_literal delegates to separate branches for IPv4 and IPv6 without checking IPv4-mapped IPv6 addresses (::ffff:x.x.x.x). An attacker can encode a private IP (e.g., 10.0.0.1, 169.254.169.254) as [::ffff:a00:1] or [::ffff:a9fe:a9fe]. The IPv6 validator (is_allowed_webhook_ipv6) sees these as non-loopback, non-local, non-multicast addresses and returns allowed = true, bypassing SSRF protection entirely.
Fix: Check for embedded IPv4 before delegating to the IPv6 validator:
IpAddr::V6(ip) => {
if let Some(ipv4) = ip.to_ipv4_mapped() {
is_allowed_webhook_ipv4(ipv4)
} else {
is_allowed_webhook_ipv6(ip)
}
}
| let endpoints: Vec<HttpWebhookEndpoint> = | ||
| cfg.webhooks.clone().into_iter().map(Into::into).collect(); | ||
| let webhook_logger = HttpLogger::from_endpoints( | ||
| reqwest::Client::new(), |
There was a problem hiding this comment.
Unrestricted HTTP redirect policy — reqwest::Client::new() defaults to following up to 10 cross-origin redirects. A compromised or malicious webhook endpoint could return a 302/307 redirect pointing at an internal service (e.g., cloud metadata API, internal Redis), and the CubeAPI server would follow it without validation.
Fix: Build the client with a restrictive redirect policy:
reqwest::Client::builder()
.redirect(reqwest::redirect::Policy::none())
.build()
.expect("failed to build webhook HTTP client")
| let host = url | ||
| .host_str() | ||
| .ok_or_else(|| anyhow::anyhow!("webhook URL must include a host: {raw_url}"))?; | ||
| if let Ok(ip) = host.parse::<IpAddr>() { |
There was a problem hiding this comment.
DNS hostname SSRF — no IP resolution validation — When the webhook URL uses a DNS name rather than an IP literal, the code skips IP validation entirely (the if let Ok(ip) = host.parse::<IpAddr>() guard at line 295). A DNS name pointing at metadata.google.internal or 10.0.0.1.internal passes validation despite resolving to a private IP.
Fix: Resolve DNS names at config time and validate all resolved addresses against the same IP blocklist. Note this requires making validate_webhook_url async (or using synchronous DNS).
| tracing::warn!(url = %endpoint.url, "ignoring duplicate webhook endpoint URL"); | ||
| return None; | ||
| } | ||
| endpoint.events = endpoint |
There was a problem hiding this comment.
Empty event list silently subscribes to ALL events — When the event filter cleanup removes all entries (e.g., user passes "events": [""]), the resulting empty Vec causes endpoint_subscribes (http.rs:202-208) to return true for every event. This silently converts an invalid config into an all-events subscription. A user with a typo or misplaced comma would unknowingly receive events they didn't intend to subscribe to.
Fix: Log a warning when cleaning removes all explicitly-provided events, so this behavior is at least observable.
| options: HttpWebhookOptions, | ||
| mut rx: mpsc::Receiver<Msg>, | ||
| ) { | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Worker JoinHandle not stored — no graceful shutdown path — The spawn_worker JoinHandle at line 149 is discarded. During shutdown, the axum server stops, flush() drains pending deliveries, and then HttpLogger is dropped — but the worker task is cancelled by the runtime before its drain_deliveries on channel-close can complete. Storing the handle and awaiting it on shutdown would ensure all pending deliveries finish before the process exits.
|
@ceilf6/repo-guard CR again |
代码评审报告: feat: issue-642风险等级: 中 级联分析
问题发现
行级发现
Karpathy 评审
缺失覆盖
|
Autonomously-by: Codex:GPT-5
|
@ceilf6/repo-guard CR again |
代码评审报告: feat: issue-642风险等级: 中 级联分析
问题发现
行级发现
Karpathy 评审
缺失覆盖
|
|
Refs #642 |
|
Hello, thank you for your contribution! Since we have a large number of participants in this event, to speed up the PR review process, could you please attach some screenshots and logs (both are needed) showing the verification process and results for this PR? Thank you for your participation! |
Summary
sandbox.created,sandbox.deleted,sandbox.paused,sandbox.resumed).Validation
cargo fmt --manifest-path CubeAPI/Cargo.toml -- --checkcargo test --manifest-path CubeAPI/Cargo.tomlnpm run docs:buildfromdocs/python3 -m py_compile examples/webhook-receiver/receiver.pygit diff --checkAutonomously-by: Codex:GPT-5