fix(channels): address CodeRabbit findings on PRs #55 + #58#63
Merged
Conversation
📝 WalkthroughWalkthroughWorkspace adds three new channel worker crates, sets workspace Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant LinkedIn as LinkedIn Webhook
participant IIIWorker as Worker<br/>(Main Handler)
participant StateStore as State Store<br/>(Dedup Cache)
participant Agent as Agent Service
Client->>LinkedIn: POST /webhook<br/>(X-LI-Signature header)
LinkedIn->>IIIWorker: Validate HMAC-SHA256
IIIWorker->>IIIWorker: Constant-time signature check
alt Signature Invalid
IIIWorker->>Client: 401 Unauthorized
else Signature Valid
IIIWorker->>IIIWorker: Process all elements
loop For each element
IIIWorker->>StateStore: Get notificationId (24h TTL)
alt Already seen
IIIWorker->>IIIWorker: Skip element
else New notification
StateStore->>StateStore: Store notificationId
IIIWorker->>Agent: Send to agent
Agent-->>IIIWorker: Return response
end
end
IIIWorker->>Client: 200 OK<br/>(even if elements failed)
end
sequenceDiagram
actor Client
participant Reddit as Reddit API
participant IIIWorker as Worker<br/>(send_message)
participant Vault as Vault
participant TokenCache as Token Cache
Client->>IIIWorker: POST request
IIIWorker->>Reddit: POST /api/comment<br/>(current bearer token)
Reddit-->>IIIWorker: 401 Unauthorized
alt Token Expired
IIIWorker->>Vault: Refresh access token
Vault-->>IIIWorker: New token
IIIWorker->>TokenCache: Store refreshed token
IIIWorker->>Reddit: POST /api/comment<br/>(new bearer token)
Reddit-->>IIIWorker: 200 OK / Success
else Other failure
IIIWorker-->>Client: Error response
end
sequenceDiagram
actor Client
participant Teams as Teams Bot
participant IIIWorker as Worker<br/>(handle_webhook)
participant AllowList as Service URL<br/>AllowList
Client->>Teams: POST activity
Teams->>IIIWorker: Webhook payload
IIIWorker->>IIIWorker: Parse serviceUrl
IIIWorker->>AllowList: Validate host (HTTPS + allowed suffixes)
alt Untrusted serviceUrl
IIIWorker->>Client: 401 JSON error
IIIWorker->>IIIWorker: Log warning
else Allowed
IIIWorker->>IIIWorker: Process activity & send reply
IIIWorker->>Client: 200 OK
end
sequenceDiagram
actor Client
participant Webex as Webex Webhook
participant IIIWorker as Worker<br/>(Main Handler)
participant BotCache as Bot PersonId<br/>Cache (Arc)
participant Webex_API as Webex /v1/people/me
Client->>Webex: Activity event
Webex->>IIIWorker: Webhook payload<br/>(data.personId)
IIIWorker->>BotCache: Check cached bot personId
alt Cache empty
IIIWorker->>Webex_API: GET /v1/people/me
Webex_API-->>IIIWorker: Own personId
IIIWorker->>BotCache: Store in Arc<RwLock<Option>>
else Cache hit
BotCache-->>IIIWorker: Return cached personId
end
IIIWorker->>IIIWorker: Compare payload.personId vs. bot.personId
alt Self-generated
IIIWorker->>Client: Skip reply (prevent loop)
else External
IIIWorker->>IIIWorker: Process & send reply
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 0/3 reviews remaining, refill in 50 minutes. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+180
to
184
| return Ok(json!({ | ||
| "status_code": 200, | ||
| "body": { "ok": false, "error": format!("{e}") } | ||
| })); | ||
| } |
There was a problem hiding this comment.
🔴 Security audit skipped when Signal send_message fails due to early return
The newly added return Ok(...) on send failure at line 180 causes the function to exit before the security::audit fire-and-forget spawn at lines 186–207. Previously (workers/channel-signal/src/main.rs before this PR), the send error was only logged and execution continued to the audit. Now, any incoming Signal message where the outbound reply fails to send will have no audit trail entry, violating the project's security audit pattern used by all other channel adapters (e.g., WhatsApp at workers/channel-whatsapp/src/main.rs:301-317, email at workers/channel-email/src/main.rs:146-163).
Prompt for agents
In workers/channel-signal/src/main.rs, the handle_webhook function has an early return on send_message failure (lines 180-184) that skips the security::audit spawn at lines 186-207. The audit block should always execute regardless of whether the reply was sent successfully. Move the audit spawn before the send_message call, or remove the early return and let execution continue to the audit spawn (matching the old behavior where only tracing::error was called). The same pattern is used in other channel workers like WhatsApp and email where the audit always fires.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+248
to
+252
| return Ok(json!({ | ||
| "status_code": 401, | ||
| "body": { "error": "Untrusted serviceUrl" } | ||
| })); | ||
| } |
There was a problem hiding this comment.
🔴 Security audit skipped when Teams serviceUrl is untrusted due to early return
The new is_allowed_service_url check at line 243 returns 401 at lines 248–251, exiting handle_webhook before the security::audit spawn at lines 276–297. This means incoming messages from untrusted service URLs — which are arguably the most security-relevant events — will have no audit trail. The agent::chat trigger at line 221 still fires (consuming resources), but the audit never records it. Before this PR, the audit always executed (workers/channel-teams/src/main.rs old code at the end of handle_webhook).
Prompt for agents
In workers/channel-teams/src/main.rs handle_webhook, the is_allowed_service_url check at line 243 returns early (lines 248-252) before the security::audit spawn at lines 276-297. The audit should fire for all inbound messages, especially those from untrusted sources. Either move the serviceUrl validation before agent::chat (so no resources are consumed for untrusted URLs, and then still audit), or move the audit spawn before the serviceUrl check. The simplest fix is to restructure so the audit fires before any early returns related to reply sending.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
92
to
+98
| if !resp.status().is_success() { | ||
| return Ok(None); | ||
| let status = resp.status(); | ||
| let body = resp.text().await.unwrap_or_default(); | ||
| return Err(IIIError::Handler(format!( | ||
| "Webex fetch failed ({status}): {}", | ||
| body.chars().take(300).collect::<String>() | ||
| ))); |
There was a problem hiding this comment.
🔴 Webex fetch_message now returns Err instead of Ok(None) on non-success HTTP, breaking webhook processing
The fetch_message function previously returned Ok(None) when the Webex API responded with a non-success status (e.g., 404 for deleted messages, 429 for rate limits, 503 for outages). The caller at workers/channel-webex/src/main.rs:214 uses ? to propagate errors, so Err(...) now causes the entire webhook handler to fail and return an error. This change from graceful degradation to hard failure means transient Webex API errors will cause webhook delivery to fail, potentially triggering retry loops from Webex and preventing the 200 OK acknowledgment.
Old vs new behavior
Old (line 83 before PR): return Ok(None); → caller returns 200 OK gracefully.
New (lines 93-98): return Err(IIIError::Handler(...)); → propagated via ? at line 214, webhook returns error.
Suggested change
| if !resp.status().is_success() { | |
| return Ok(None); | |
| let status = resp.status(); | |
| let body = resp.text().await.unwrap_or_default(); | |
| return Err(IIIError::Handler(format!( | |
| "Webex fetch failed ({status}): {}", | |
| body.chars().take(300).collect::<String>() | |
| ))); | |
| if !resp.status().is_success() { | |
| let status = resp.status(); | |
| tracing::warn!(status = %status, message_id, "Webex message fetch non-success"); | |
| return Ok(None); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
workers/channel-signal/src/main.rs (1)
62-69: Inconsistentsplit_messagenewline handling compared to Webex.The PR summary mentions fixing leading
\nafter newline split in teams/webex/whatsapp, but Signal'ssplit_messagestill usesremaining[split_at..]without skipping the newline character. Webex was updated to skip it (lines 72-76 inchannel-webex/src/main.rs).If this is intentional (e.g., Signal messages should preserve leading newlines), please add a comment explaining the difference. Otherwise, consider applying the same fix:
♻️ Suggested fix to match Webex behavior
chunks.push(remaining[..split_at].to_string()); - remaining = remaining[split_at..].to_string(); + remaining = if split_at < cutoff && remaining.as_bytes().get(split_at) == Some(&b'\n') { + remaining[split_at + 1..].to_string() + } else { + remaining[split_at..].to_string() + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-signal/src/main.rs` around lines 62 - 69, The Signal split_message implementation currently sets remaining = remaining[split_at..] which preserves a leading '\n' after splitting; to match Webex behavior, when split_at comes from window.rfind('\n') skip the newline by using remaining = remaining[split_at + 1..] (ensure you only +1 when the split_at index points at the newline and bounds-check for safety), update the code paths using split_at/window/remaining/chunks in split_message accordingly, or if preserving the newline is intentional add a clear comment in split_message explaining why Signal differs from Webex/Teams/WhatsApp.workers/channel-twitch/src/main.rs (1)
75-85: Consider using thesubtlecrate for constant-time comparison.The manual XOR-folding implementation is functionally correct, but the
subtlecrate (already in the workspace per PR summary) provides a well-auditedConstantTimeEqtrait. This avoids subtle compiler optimization risks that could leak timing information.♻️ Suggested refactor using subtle
+use subtle::ConstantTimeEq; -fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { - if a.len() != b.len() { - return false; - } - let mut diff: u8 = 0; - for (x, y) in a.iter().zip(b.iter()) { - diff |= x ^ y; - } - diff == 0 -} +fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { + a.ct_eq(b).into() +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-twitch/src/main.rs` around lines 75 - 85, Replace the ad-hoc constant_time_eq function with the well-audited implementation from the subtle crate: import subtle::ConstantTimeEq (or the appropriate trait/type such as subtle::Choice) and use its ct_eq/constant-time equality method wherever constant_time_eq is defined/used; remove the manual XOR-folding loop in the constant_time_eq function and delegate to the subtle trait implementation so comparisons remain constant-time and avoid compiler optimization risks.workers/channel-whatsapp/src/main.rs (1)
12-21: Consider using thesubtlecrate for constant-time comparison.Same recommendation as for Twitch: the
subtlecrate'sConstantTimeEqprovides compiler-barrier-protected constant-time comparison.♻️ Suggested refactor using subtle
+use subtle::ConstantTimeEq; -fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { - if a.len() != b.len() { - return false; - } - let mut diff: u8 = 0; - for (x, y) in a.iter().zip(b.iter()) { - diff |= x ^ y; - } - diff == 0 -} +fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { + a.ct_eq(b).into() +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-whatsapp/src/main.rs` around lines 12 - 21, The custom constant-time byte comparison in function constant_time_eq should be replaced with the well-tested implementation from the subtle crate; add subtle as a dependency and use the ConstantTimeEq trait (e.g. import subtle::ConstantTimeEq and call a.ct_eq(b).unwrap_u8() == 1 or the provided boolean helper) in place of the manual loop to ensure compiler-barrier-protected constant-time semantics and reduce risk of timing leaks.workers/channel-linkedin/src/main.rs (2)
165-199: Best-effort deduplication has an inherent race condition.The
state::getfollowed bystate::setpattern has a TOCTOU (time-of-check-to-time-of-use) window where concurrent webhook deliveries of the same notification could both pass the duplicate check. This is acceptable for webhook deduplication where the downstreamagent::chatshould be idempotent, but worth noting.If stronger guarantees are needed, consider using an atomic "set if not exists" operation if the state service supports it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-linkedin/src/main.rs` around lines 165 - 199, The current record_notification_id function uses a non-atomic state::get followed by state::set (TOCTOU) which can allow duplicate wins; change it to use an atomic "set if not exists" operation on the state service (e.g. a single trigger call like function_id "state::set_if_not_exists" or an equivalent atomic flag/option supported by the state API) instead of get+set, pass the same scope "channel_dedupe", key format "linkedin:{notification_id}" and TTL NOTIFICATION_DEDUPE_TTL_SECS, and return true only when the atomic set succeeds (false when it already exists); update error handling around the single TriggerRequest and remove the separate state::get usage in record_notification_id.
426-461: Consider adding unit tests for signature verification.The existing tests cover message splitting and extraction, but the new security-critical functions (
hmac_sha256_hex,verify_linkedin_signature,constant_time_eq) lack test coverage. While the PR description notes tests are deferred, these cryptographic functions are good candidates for unit testing since they're pure functions with no external dependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-linkedin/src/main.rs` around lines 426 - 461, Add unit tests covering the pure crypto helpers: hmac_sha256_hex, verify_linkedin_signature, and constant_time_eq inside the existing tests module; create deterministic test vectors (e.g., known key/message pairs including empty and unicode inputs) and assert hmac_sha256_hex returns the expected hex string, then build a valid LinkedIn-style signature string (e.g., "sha256=" + hmac from hmac_sha256_hex) and assert verify_linkedin_signature returns true for that signature and false for a tampered signature, and add assertions that constant_time_eq returns true for identical inputs and false for different or different-length inputs; place tests alongside the existing mod tests and reference the functions by name (hmac_sha256_hex, verify_linkedin_signature, constant_time_eq) so they are exercised deterministically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workers/channel-linkedin/src/main.rs`:
- Around line 335-365: The code currently skips LinkedIn signature verification
when raw_body is None (raw_body variable), allowing unsigned POSTs to be
processed; update the handler to require rawBody for POST requests by returning
a 400/401 error if raw_body is missing (before you call get_secret or
verify_linkedin_signature) and only proceed to set body = input.get("body")...
after signature verification succeeds; locate the
raw_body/signature/verify_linkedin_signature logic and the body assignment in
main.rs and add a guard that rejects the request when raw_body is None for POSTs
(use the same error JSON shape used elsewhere) so verify_linkedin_signature and
get_secret are always enforced for POST webhooks.
- Around line 17-26: Replace the hand-rolled constant_time_eq function with the
vetted subtle crate: add subtle = { workspace = true } to the [dependencies]
section of this crate's Cargo.toml, import subtle::ConstantTimeEq in main.rs,
then replace calls to constant_time_eq with the ConstantTimeEq::ct_eq method (or
use b.ct_eq(a).into() as needed) and remove the constant_time_eq function
definition; ensure all call sites referencing constant_time_eq are updated to
the new API.
In `@workers/channel-twitch/src/main.rs`:
- Around line 180-197: The current logic skips signature verification when
TWITCH_EVENTSUB_SECRET is set but raw_body is absent; update the block that
calls get_secret and verify_eventsub_signature so that if secret is non-empty
and raw_body.is_none() you return a 400 JSON response indicating the missing raw
body required for signature verification (similar to channel-slack) before
checking headers; keep the existing checks for message_id/timestamp/signature
and the verify_eventsub_signature call unchanged, using the same variables
(secret, raw_body, message_id, timestamp, signature, verify_eventsub_signature)
to locate where to add the early return.
---
Nitpick comments:
In `@workers/channel-linkedin/src/main.rs`:
- Around line 165-199: The current record_notification_id function uses a
non-atomic state::get followed by state::set (TOCTOU) which can allow duplicate
wins; change it to use an atomic "set if not exists" operation on the state
service (e.g. a single trigger call like function_id "state::set_if_not_exists"
or an equivalent atomic flag/option supported by the state API) instead of
get+set, pass the same scope "channel_dedupe", key format
"linkedin:{notification_id}" and TTL NOTIFICATION_DEDUPE_TTL_SECS, and return
true only when the atomic set succeeds (false when it already exists); update
error handling around the single TriggerRequest and remove the separate
state::get usage in record_notification_id.
- Around line 426-461: Add unit tests covering the pure crypto helpers:
hmac_sha256_hex, verify_linkedin_signature, and constant_time_eq inside the
existing tests module; create deterministic test vectors (e.g., known
key/message pairs including empty and unicode inputs) and assert hmac_sha256_hex
returns the expected hex string, then build a valid LinkedIn-style signature
string (e.g., "sha256=" + hmac from hmac_sha256_hex) and assert
verify_linkedin_signature returns true for that signature and false for a
tampered signature, and add assertions that constant_time_eq returns true for
identical inputs and false for different or different-length inputs; place tests
alongside the existing mod tests and reference the functions by name
(hmac_sha256_hex, verify_linkedin_signature, constant_time_eq) so they are
exercised deterministically.
In `@workers/channel-signal/src/main.rs`:
- Around line 62-69: The Signal split_message implementation currently sets
remaining = remaining[split_at..] which preserves a leading '\n' after
splitting; to match Webex behavior, when split_at comes from window.rfind('\n')
skip the newline by using remaining = remaining[split_at + 1..] (ensure you only
+1 when the split_at index points at the newline and bounds-check for safety),
update the code paths using split_at/window/remaining/chunks in split_message
accordingly, or if preserving the newline is intentional add a clear comment in
split_message explaining why Signal differs from Webex/Teams/WhatsApp.
In `@workers/channel-twitch/src/main.rs`:
- Around line 75-85: Replace the ad-hoc constant_time_eq function with the
well-audited implementation from the subtle crate: import subtle::ConstantTimeEq
(or the appropriate trait/type such as subtle::Choice) and use its
ct_eq/constant-time equality method wherever constant_time_eq is defined/used;
remove the manual XOR-folding loop in the constant_time_eq function and delegate
to the subtle trait implementation so comparisons remain constant-time and avoid
compiler optimization risks.
In `@workers/channel-whatsapp/src/main.rs`:
- Around line 12-21: The custom constant-time byte comparison in function
constant_time_eq should be replaced with the well-tested implementation from the
subtle crate; add subtle as a dependency and use the ConstantTimeEq trait (e.g.
import subtle::ConstantTimeEq and call a.ct_eq(b).unwrap_u8() == 1 or the
provided boolean helper) in place of the manual loop to ensure
compiler-barrier-protected constant-time semantics and reduce risk of timing
leaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 092ff363-1912-485c-a048-f0b9abb67950
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlworkers/channel-email/Cargo.tomlworkers/channel-email/src/main.rsworkers/channel-linkedin/Cargo.tomlworkers/channel-linkedin/src/main.rsworkers/channel-reddit/Cargo.tomlworkers/channel-reddit/src/main.rsworkers/channel-signal/Cargo.tomlworkers/channel-signal/src/main.rsworkers/channel-teams/Cargo.tomlworkers/channel-teams/src/main.rsworkers/channel-twitch/Cargo.tomlworkers/channel-twitch/src/main.rsworkers/channel-webex/Cargo.tomlworkers/channel-webex/src/main.rsworkers/channel-whatsapp/Cargo.tomlworkers/channel-whatsapp/src/main.rs
Comment on lines
+17
to
+26
| fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { | ||
| if a.len() != b.len() { | ||
| return false; | ||
| } | ||
| let mut diff: u8 = 0; | ||
| for (x, y) in a.iter().zip(b.iter()) { | ||
| diff |= x ^ y; | ||
| } | ||
| diff == 0 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify subtle is available in workspace dependencies
rg -n 'subtle' Cargo.toml workers/channel-linkedin/Cargo.tomlRepository: iii-experimental/agentos
Length of output: 95
🏁 Script executed:
# Check if the file exists and examine lines 17-26
if [ -f "workers/channel-linkedin/src/main.rs" ]; then
echo "=== File found. Content of lines 17-26: ==="
sed -n '17,26p' workers/channel-linkedin/src/main.rs
echo ""
echo "=== Context around line 45: ==="
sed -n '40,50p' workers/channel-linkedin/src/main.rs
else
echo "File not found at workers/channel-linkedin/src/main.rs"
fiRepository: iii-experimental/agentos
Length of output: 732
🏁 Script executed:
# Check the workers/channel-linkedin/Cargo.toml for subtle dependency
echo "=== workers/channel-linkedin/Cargo.toml content: ==="
cat workers/channel-linkedin/Cargo.tomlRepository: iii-experimental/agentos
Length of output: 691
🏁 Script executed:
# List files in workers/channel-linkedin to understand structure
echo "=== Directory structure of workers/channel-linkedin: ==="
find workers/channel-linkedin -type f | head -20Repository: iii-experimental/agentos
Length of output: 239
Consider using subtle::ConstantTimeEq for constant-time comparison, but note that subtle must be added to this crate's dependencies first.
The root workspace has subtle = "2", but workers/channel-linkedin/Cargo.toml does not declare it as a dependency. To use subtle::ConstantTimeEq, you'll need to add subtle = { workspace = true } to the [dependencies] section of workers/channel-linkedin/Cargo.toml. Using a standard, battle-tested library like subtle is preferable to hand-rolled implementations for cryptographic operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-linkedin/src/main.rs` around lines 17 - 26, Replace the
hand-rolled constant_time_eq function with the vetted subtle crate: add subtle =
{ workspace = true } to the [dependencies] section of this crate's Cargo.toml,
import subtle::ConstantTimeEq in main.rs, then replace calls to constant_time_eq
with the ConstantTimeEq::ct_eq method (or use b.ct_eq(a).into() as needed) and
remove the constant_time_eq function definition; ensure all call sites
referencing constant_time_eq are updated to the new API.
Comment on lines
+335
to
+365
| // Verify X-LI-Signature on POST. | ||
| let raw_body = input.get("rawBody").and_then(|v| v.as_str()); | ||
| let signature = input | ||
| .get("headers") | ||
| .and_then(|h| h.get("x-li-signature")) | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or(""); | ||
| if let Some(raw) = raw_body { | ||
| let secret = get_secret(iii, "LINKEDIN_CLIENT_SECRET").await; | ||
| if secret.is_empty() { | ||
| return Ok(json!({ | ||
| "status_code": 500, | ||
| "body": { "error": "LINKEDIN_CLIENT_SECRET not configured" } | ||
| })); | ||
| } | ||
| if signature.is_empty() { | ||
| return Ok(json!({ | ||
| "status_code": 401, | ||
| "body": { "error": "Missing X-LI-Signature header" } | ||
| })); | ||
| } | ||
| if let Err(e) = verify_linkedin_signature(&secret, raw, signature) { | ||
| tracing::warn!(error = %e, "linkedin signature rejected"); | ||
| return Ok(json!({ | ||
| "status_code": 401, | ||
| "body": { "error": "Invalid LinkedIn signature" } | ||
| })); | ||
| } | ||
| } | ||
|
|
||
| let body = input.get("body").cloned().unwrap_or(input); |
There was a problem hiding this comment.
Signature verification is skipped when rawBody is absent.
If rawBody is None (line 342), the code proceeds to process the webhook body without signature verification. This could allow unsigned requests to be processed if the HTTP trigger framework doesn't consistently provide rawBody.
Consider requiring rawBody for POST requests and rejecting the webhook if it's missing:
🔒 Proposed fix to enforce signature verification
// Verify X-LI-Signature on POST.
- let raw_body = input.get("rawBody").and_then(|v| v.as_str());
+ let Some(raw_body) = input.get("rawBody").and_then(|v| v.as_str()) else {
+ tracing::warn!("linkedin: POST missing rawBody for signature verification");
+ return Ok(json!({
+ "status_code": 400,
+ "body": { "error": "Missing rawBody for signature verification" }
+ }));
+ };
let signature = input
.get("headers")
.and_then(|h| h.get("x-li-signature"))
.and_then(|v| v.as_str())
.unwrap_or("");
- if let Some(raw) = raw_body {
- let secret = get_secret(iii, "LINKEDIN_CLIENT_SECRET").await;
- if secret.is_empty() {
- return Ok(json!({
- "status_code": 500,
- "body": { "error": "LINKEDIN_CLIENT_SECRET not configured" }
- }));
- }
- if signature.is_empty() {
- return Ok(json!({
- "status_code": 401,
- "body": { "error": "Missing X-LI-Signature header" }
- }));
- }
- if let Err(e) = verify_linkedin_signature(&secret, raw, signature) {
- tracing::warn!(error = %e, "linkedin signature rejected");
- return Ok(json!({
- "status_code": 401,
- "body": { "error": "Invalid LinkedIn signature" }
- }));
- }
+ let secret = get_secret(iii, "LINKEDIN_CLIENT_SECRET").await;
+ if secret.is_empty() {
+ return Ok(json!({
+ "status_code": 500,
+ "body": { "error": "LINKEDIN_CLIENT_SECRET not configured" }
+ }));
+ }
+ if signature.is_empty() {
+ return Ok(json!({
+ "status_code": 401,
+ "body": { "error": "Missing X-LI-Signature header" }
+ }));
+ }
+ if let Err(e) = verify_linkedin_signature(&secret, raw_body, signature) {
+ tracing::warn!(error = %e, "linkedin signature rejected");
+ return Ok(json!({
+ "status_code": 401,
+ "body": { "error": "Invalid LinkedIn signature" }
+ }));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-linkedin/src/main.rs` around lines 335 - 365, The code
currently skips LinkedIn signature verification when raw_body is None (raw_body
variable), allowing unsigned POSTs to be processed; update the handler to
require rawBody for POST requests by returning a 400/401 error if raw_body is
missing (before you call get_secret or verify_linkedin_signature) and only
proceed to set body = input.get("body")... after signature verification
succeeds; locate the raw_body/signature/verify_linkedin_signature logic and the
body assignment in main.rs and add a guard that rejects the request when
raw_body is None for POSTs (use the same error JSON shape used elsewhere) so
verify_linkedin_signature and get_secret are always enforced for POST webhooks.
Comment on lines
+180
to
+197
| let secret = get_secret(iii, "TWITCH_EVENTSUB_SECRET").await; | ||
| if !secret.is_empty() | ||
| && let Some(raw) = raw_body.as_deref() | ||
| { | ||
| if message_id.is_empty() || timestamp.is_empty() || signature.is_empty() { | ||
| return Ok(json!({ | ||
| "status_code": 401, | ||
| "body": { "error": "Missing Twitch EventSub headers" } | ||
| })); | ||
| } | ||
| if let Err(e) = verify_eventsub_signature(&secret, message_id, timestamp, raw, signature) { | ||
| tracing::warn!(error = %e, "twitch eventsub signature rejected"); | ||
| return Ok(json!({ | ||
| "status_code": 401, | ||
| "body": { "error": "Invalid Twitch EventSub signature" } | ||
| })); | ||
| } | ||
| } |
There was a problem hiding this comment.
Signature verification is skipped when rawBody is absent.
When TWITCH_EVENTSUB_SECRET is configured but rawBody is missing from the request, the signature check is bypassed silently. Per the pattern in channel-slack (which returns 400 for missing rawBody), consider returning an error when signature verification is expected but cannot be performed.
🛡️ Suggested fix to handle missing rawBody
let secret = get_secret(iii, "TWITCH_EVENTSUB_SECRET").await;
- if !secret.is_empty()
- && let Some(raw) = raw_body.as_deref()
- {
+ if !secret.is_empty() {
+ let Some(raw) = raw_body.as_deref() else {
+ return Ok(json!({
+ "status_code": 400,
+ "body": { "error": "Missing rawBody for signature verification" }
+ }));
+ };
if message_id.is_empty() || timestamp.is_empty() || signature.is_empty() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let secret = get_secret(iii, "TWITCH_EVENTSUB_SECRET").await; | |
| if !secret.is_empty() | |
| && let Some(raw) = raw_body.as_deref() | |
| { | |
| if message_id.is_empty() || timestamp.is_empty() || signature.is_empty() { | |
| return Ok(json!({ | |
| "status_code": 401, | |
| "body": { "error": "Missing Twitch EventSub headers" } | |
| })); | |
| } | |
| if let Err(e) = verify_eventsub_signature(&secret, message_id, timestamp, raw, signature) { | |
| tracing::warn!(error = %e, "twitch eventsub signature rejected"); | |
| return Ok(json!({ | |
| "status_code": 401, | |
| "body": { "error": "Invalid Twitch EventSub signature" } | |
| })); | |
| } | |
| } | |
| let secret = get_secret(iii, "TWITCH_EVENTSUB_SECRET").await; | |
| if !secret.is_empty() { | |
| let Some(raw) = raw_body.as_deref() else { | |
| return Ok(json!({ | |
| "status_code": 400, | |
| "body": { "error": "Missing rawBody for signature verification" } | |
| })); | |
| }; | |
| if message_id.is_empty() || timestamp.is_empty() || signature.is_empty() { | |
| return Ok(json!({ | |
| "status_code": 401, | |
| "body": { "error": "Missing Twitch EventSub headers" } | |
| })); | |
| } | |
| if let Err(e) = verify_eventsub_signature(&secret, message_id, timestamp, raw, signature) { | |
| tracing::warn!(error = %e, "twitch eventsub signature rejected"); | |
| return Ok(json!({ | |
| "status_code": 401, | |
| "body": { "error": "Invalid Twitch EventSub signature" } | |
| })); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-twitch/src/main.rs` around lines 180 - 197, The current logic
skips signature verification when TWITCH_EVENTSUB_SECRET is set but raw_body is
absent; update the block that calls get_secret and verify_eventsub_signature so
that if secret is non-empty and raw_body.is_none() you return a 400 JSON
response indicating the missing raw body required for signature verification
(similar to channel-slack) before checking headers; keep the existing checks for
message_id/timestamp/signature and the verify_eventsub_signature call unchanged,
using the same variables (secret, raw_body, message_id, timestamp, signature,
verify_eventsub_signature) to locate where to add the early return.
Webhook security - LinkedIn: HMAC-SHA256 challengeCode GET handshake, X-LI-Signature verification on POST, notificationId dedup via state::*, process all elements (not just first), 10s HTTP timeout - Twitch: EventSub HMAC-SHA256 (Message-Id + Timestamp + raw body) signature verification, sender_id now uses TWITCH_BOT_USER_ID instead of broadcaster_id - WhatsApp: X-Hub-Signature-256 verification (HMAC-SHA256 of raw body via WHATSAPP_APP_SECRET) plus GET hub.challenge endpoint with constant-time verify_token compare - Teams: validate serviceUrl against Bot Framework host suffixes (with TEAMS_ALLOWED_SERVICE_URLS escape hatch) before sending authenticated outbound replies; reqwest connect/total timeouts (5s/15s) - Webex: cache bot personId from /people/me and drop self-posted webhook events to break reply loops; propagate non-2xx fetch errors instead of silently acking Reliability - Reddit: invalidate token cache on 401 and retry once with refresh - Signal: empty agent reply now returns 200 (with `ok:false`) so the bridge does not retry; send_message failures are surfaced to the caller rather than logged-and-acked - WhatsApp: agent::chat failures no longer fail the webhook response; caller-supplied phone numbers redacted (sha256:8) in error logs - Email: SMTP_* secrets resolved through vault::get with env fallback, recipient address redacted in send-failure logs, blank/whitespace agent replies no longer trigger empty outgoing email Hygiene - Newline-aware split_message no longer leaves a leading '\n' on the next chunk in teams/webex/whatsapp - Workspace MSRV pinned to 1.88 (let-chains stabilization) and the reddit/linkedin/twitch crates wired into workspace members
rohitg00
force-pushed
the
fix/cr-wave2-channels
branch
from
5b77d06 to
338cc82
Compare
Comment on lines
+17
to
+26
| fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { | ||
| if a.len() != b.len() { | ||
| return false; | ||
| } | ||
| let mut diff: u8 = 0; | ||
| for (x, y) in a.iter().zip(b.iter()) { | ||
| diff |= x ^ y; | ||
| } | ||
| diff == 0 | ||
| } |
There was a problem hiding this comment.
🟡 Hand-rolled constant_time_eq with timing-leaking length check instead of subtle::ConstantTimeEq already in the workspace
Three channel workers (linkedin, twitch, whatsapp) each implement a custom constant_time_eq with an early return on a.len() != b.len() that leaks length information through timing. The subtle crate is already a workspace dependency (Cargo.toml:71) and is used by the existing security-map worker at workers/security-map/src/main.rs:8 (use subtle::ConstantTimeEq) and workers/security-map/src/main.rs:35 (token.as_bytes().ct_eq(expected.as_bytes())). This violates the established security pattern and the CLAUDE.md rule "Security: timing-safe HMAC via safeEqual()".
Prompt for agents
In workers/channel-linkedin/src/main.rs (lines 17-26), workers/channel-twitch/src/main.rs (lines 76-85), and workers/channel-whatsapp/src/main.rs (lines 12-21), replace the hand-rolled constant_time_eq function with subtle::ConstantTimeEq from the subtle crate, which is already a workspace dependency. Add subtle.workspace = true to each worker's Cargo.toml dependencies. Then replace the constant_time_eq calls with the .ct_eq() method, e.g.: expected.as_bytes().ct_eq(signature.as_bytes()).into(). This matches the existing pattern in workers/security-map/src/main.rs:35 and ensures true constant-time comparison without the early-return length leak.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
workers/channel-linkedin/src/main.rs (1)
336-365:⚠️ Potential issue | 🔴 CriticalRequire
rawBodyfor POST before processing webhook payload.At Line 342, signature verification is skipped when
rawBodyis absent, and execution continues to Line 365. That still permits unsigned webhook processing.🔒 Suggested fix
- let raw_body = input.get("rawBody").and_then(|v| v.as_str()); + let Some(raw_body) = input.get("rawBody").and_then(|v| v.as_str()) else { + tracing::warn!("linkedin: POST missing rawBody for signature verification"); + return Ok(json!({ + "status_code": 400, + "body": { "error": "Missing rawBody for signature verification" } + })); + }; let signature = input .get("headers") .and_then(|h| h.get("x-li-signature")) .and_then(|v| v.as_str()) .unwrap_or(""); - if let Some(raw) = raw_body { - let secret = get_secret(iii, "LINKEDIN_CLIENT_SECRET").await; - if secret.is_empty() { - return Ok(json!({ - "status_code": 500, - "body": { "error": "LINKEDIN_CLIENT_SECRET not configured" } - })); - } - if signature.is_empty() { - return Ok(json!({ - "status_code": 401, - "body": { "error": "Missing X-LI-Signature header" } - })); - } - if let Err(e) = verify_linkedin_signature(&secret, raw, signature) { - tracing::warn!(error = %e, "linkedin signature rejected"); - return Ok(json!({ - "status_code": 401, - "body": { "error": "Invalid LinkedIn signature" } - })); - } + let secret = get_secret(iii, "LINKEDIN_CLIENT_SECRET").await; + if secret.is_empty() { + return Ok(json!({ + "status_code": 500, + "body": { "error": "LINKEDIN_CLIENT_SECRET not configured" } + })); + } + if signature.is_empty() { + return Ok(json!({ + "status_code": 401, + "body": { "error": "Missing X-LI-Signature header" } + })); + } + if let Err(e) = verify_linkedin_signature(&secret, raw_body, signature) { + tracing::warn!(error = %e, "linkedin signature rejected"); + return Ok(json!({ + "status_code": 401, + "body": { "error": "Invalid LinkedIn signature" } + })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-linkedin/src/main.rs` around lines 336 - 365, The code currently skips signature verification when raw_body is None, allowing unsigned POST webhooks to proceed; update the request handling so that for POST requests you require rawBody before processing: check the request method (e.g., inspect input.get("method") or headers to detect POST) and if it's POST and raw_body is None return an error (401 or 400) instead of continuing; ensure get_secret and verify_linkedin_signature are only called when raw_body is Some, and keep the final body assignment (body = input.get("body").cloned().unwrap_or(input)) after this guarded validation so unsigned POSTs never bypass signature verification.
🧹 Nitpick comments (2)
workers/channel-linkedin/src/main.rs (1)
167-199: Dedup is committed before successful processing, which can drop retried events.Line 232 marks a notification as seen before
agent::chat(Line 239). Ifagent::chatfails (Line 251), retries will be treated as duplicates and skipped. This creates permanent message loss on transient failures.Also applies to: 228-235, 239-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-linkedin/src/main.rs` around lines 167 - 199, The dedupe key is being written (state::set) inside record_notification_id before processing completes, causing retries to be skipped if agent::chat fails; change the flow so that record_notification_id only checks state::get and does NOT set the "seen" flag, and instead perform the state::set (using the same scope/key and NOTIFICATION_DEDUPE_TTL_SECS) only after agent::chat returns successfully (or after any downstream processing succeeds); alternatively implement a two‑phase marker (e.g., "processing" -> "done") where you set "processing" before work and atomically flip to "done" on success and clear on failure, but simplest fix is to move the state::set call out of record_notification_id and invoke it after agent::chat succeeds.workers/channel-email/src/main.rs (1)
60-68: Parallelize secret fetches to cut webhook latencyThese lookups are independent but currently awaited serially. Running them concurrently will reduce reply-send latency under normal vault conditions.
⚡ Suggested refactor
- let host = { - let v = get_secret(iii, "SMTP_HOST").await; - if v.is_empty() { "localhost".to_string() } else { v } - }; - let port_raw = get_secret(iii, "SMTP_PORT").await; - let port: u16 = port_raw.parse().unwrap_or(587); - let secure = get_secret(iii, "SMTP_SECURE").await == "true"; - let user = get_secret(iii, "SMTP_USER").await; - let pass = get_secret(iii, "SMTP_PASS").await; + let (host_raw, port_raw, secure_raw, user, pass) = tokio::join!( + get_secret(iii, "SMTP_HOST"), + get_secret(iii, "SMTP_PORT"), + get_secret(iii, "SMTP_SECURE"), + get_secret(iii, "SMTP_USER"), + get_secret(iii, "SMTP_PASS"), + ); + let host = if host_raw.is_empty() { "localhost".to_string() } else { host_raw }; + let port: u16 = port_raw.parse().unwrap_or(587); + let secure = secure_raw.eq_ignore_ascii_case("true");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/channel-email/src/main.rs` around lines 60 - 68, The secret fetches for SMTP_HOST, SMTP_PORT, SMTP_SECURE, SMTP_USER and SMTP_PASS are being awaited serially which increases latency; refactor the block that builds host, port_raw, secure, user, pass to run get_secret("SMTP_HOST"), get_secret("SMTP_PORT"), get_secret("SMTP_SECURE"), get_secret("SMTP_USER"), and get_secret("SMTP_PASS") concurrently (e.g., using tokio::join! or futures::join!) and then map the results: apply the host default if empty, parse port_raw into u16 with fallback 587, compare secure == "true", and assign user/pass from the joined results; keep existing variable names (host, port_raw/port, secure, user, pass) so downstream code remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workers/channel-email/src/main.rs`:
- Around line 135-142: The current tracing::error call logs the full error (%e)
which can contain user identifiers and bypass redaction; change the logging to
avoid emitting raw error text—either pass a redacted string (e.g.
redact(&e.to_string()) or redact_error(&e)) or log a non-sensitive static
message plus an error kind/variant (e.g. e.kind() or a mapped enum) from the
send_mail result; update the error site where send_mail(..).await is matched
(the tracing::error block in the reply-send branch) to use the
redacted/sanitized value or structured fields that do not include raw addresses
or SMTP responses. Ensure you reference send_mail, the tracing::error call, and
the redact helper (or implement a redact_error helper) so the logged output
never directly formats the error Display.
In `@workers/channel-teams/src/main.rs`:
- Around line 237-253: Move the serviceUrl trust check to run before any call to
agent::chat so untrusted activities never reach the agent; specifically, after
you obtain service_url (and the allowed list via get_secret for
"TEAMS_ALLOWED_SERVICE_URLS") call is_allowed_service_url(&service_url,
&allowed_extra) and return the 401 JSON response immediately if it fails,
ensuring the existing agent::chat invocation (the agent::chat call site) is only
reached when the check passes; update the control flow around get_secret,
is_allowed_service_url, and the agent::chat call to reflect this early gate.
In `@workers/channel-whatsapp/src/main.rs`:
- Around line 195-223: The current flow skips signature verification when
raw_body is None, allowing unauthenticated POSTs; change the logic so that
absence of raw_body triggers an immediate error response (fail-closed) before
proceeding: check raw_body early and if it's None return a 400/401 JSON error
(similar format to the existing responses), then proceed to call get_secret(...)
and verify_meta_signature(...) with raw and signature as before; reference
symbols: raw_body variable, signature extraction, get_secret(iii,
"WHATSAPP_APP_SECRET"), and verify_meta_signature(...) to locate and update the
conditional.
---
Duplicate comments:
In `@workers/channel-linkedin/src/main.rs`:
- Around line 336-365: The code currently skips signature verification when
raw_body is None, allowing unsigned POST webhooks to proceed; update the request
handling so that for POST requests you require rawBody before processing: check
the request method (e.g., inspect input.get("method") or headers to detect POST)
and if it's POST and raw_body is None return an error (401 or 400) instead of
continuing; ensure get_secret and verify_linkedin_signature are only called when
raw_body is Some, and keep the final body assignment (body =
input.get("body").cloned().unwrap_or(input)) after this guarded validation so
unsigned POSTs never bypass signature verification.
---
Nitpick comments:
In `@workers/channel-email/src/main.rs`:
- Around line 60-68: The secret fetches for SMTP_HOST, SMTP_PORT, SMTP_SECURE,
SMTP_USER and SMTP_PASS are being awaited serially which increases latency;
refactor the block that builds host, port_raw, secure, user, pass to run
get_secret("SMTP_HOST"), get_secret("SMTP_PORT"), get_secret("SMTP_SECURE"),
get_secret("SMTP_USER"), and get_secret("SMTP_PASS") concurrently (e.g., using
tokio::join! or futures::join!) and then map the results: apply the host default
if empty, parse port_raw into u16 with fallback 587, compare secure == "true",
and assign user/pass from the joined results; keep existing variable names
(host, port_raw/port, secure, user, pass) so downstream code remains unchanged.
In `@workers/channel-linkedin/src/main.rs`:
- Around line 167-199: The dedupe key is being written (state::set) inside
record_notification_id before processing completes, causing retries to be
skipped if agent::chat fails; change the flow so that record_notification_id
only checks state::get and does NOT set the "seen" flag, and instead perform the
state::set (using the same scope/key and NOTIFICATION_DEDUPE_TTL_SECS) only
after agent::chat returns successfully (or after any downstream processing
succeeds); alternatively implement a two‑phase marker (e.g., "processing" ->
"done") where you set "processing" before work and atomically flip to "done" on
success and clear on failure, but simplest fix is to move the state::set call
out of record_notification_id and invoke it after agent::chat succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 81257f30-0441-4939-a487-3bb05d5af6bb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlworkers/channel-email/Cargo.tomlworkers/channel-email/src/main.rsworkers/channel-linkedin/Cargo.tomlworkers/channel-linkedin/src/main.rsworkers/channel-reddit/Cargo.tomlworkers/channel-reddit/src/main.rsworkers/channel-signal/Cargo.tomlworkers/channel-signal/src/main.rsworkers/channel-teams/Cargo.tomlworkers/channel-teams/src/main.rsworkers/channel-twitch/Cargo.tomlworkers/channel-twitch/src/main.rsworkers/channel-webex/Cargo.tomlworkers/channel-webex/src/main.rsworkers/channel-whatsapp/Cargo.tomlworkers/channel-whatsapp/src/main.rs
✅ Files skipped from review due to trivial changes (6)
- workers/channel-signal/Cargo.toml
- workers/channel-webex/Cargo.toml
- workers/channel-reddit/Cargo.toml
- workers/channel-teams/Cargo.toml
- workers/channel-linkedin/Cargo.toml
- workers/channel-email/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- workers/channel-twitch/Cargo.toml
- workers/channel-whatsapp/Cargo.toml
- Cargo.toml
- workers/channel-signal/src/main.rs
- workers/channel-twitch/src/main.rs
Comment on lines
+135
to
+142
| if !reply.trim().is_empty() | ||
| && let Err(e) = send_mail(iii, from, &reply_subject, reply).await | ||
| { | ||
| tracing::error!( | ||
| to_hash = %redact(from), | ||
| error = %e, | ||
| "failed to send email reply" | ||
| ); |
There was a problem hiding this comment.
Redaction is bypassed by logging full error text
Line 140 logs %e directly. Error strings in this path can include user identifiers (addresses) from parsing/SMTP responses, which defeats the redaction goal and creates a privacy/compliance risk.
🔐 Safer logging change
if !reply.trim().is_empty()
&& let Err(e) = send_mail(iii, from, &reply_subject, reply).await
{
tracing::error!(
to_hash = %redact(from),
- error = %e,
+ error_kind = "send_mail_failed",
"failed to send email reply"
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-email/src/main.rs` around lines 135 - 142, The current
tracing::error call logs the full error (%e) which can contain user identifiers
and bypass redaction; change the logging to avoid emitting raw error text—either
pass a redacted string (e.g. redact(&e.to_string()) or redact_error(&e)) or log
a non-sensitive static message plus an error kind/variant (e.g. e.kind() or a
mapped enum) from the send_mail result; update the error site where
send_mail(..).await is matched (the tracing::error block in the reply-send
branch) to use the redacted/sanitized value or structured fields that do not
include raw addresses or SMTP responses. Ensure you reference send_mail, the
tracing::error call, and the redact helper (or implement a redact_error helper)
so the logged output never directly formats the error Display.
Comment on lines
+237
to
253
| let allowed_extra: Vec<String> = get_secret(iii, "TEAMS_ALLOWED_SERVICE_URLS") | ||
| .await | ||
| .split(',') | ||
| .map(|s| s.trim().to_string()) | ||
| .filter(|s| !s.is_empty()) | ||
| .collect(); | ||
| if !is_allowed_service_url(&service_url, &allowed_extra) { | ||
| tracing::warn!( | ||
| service_url = %service_url, | ||
| "rejecting Teams activity with untrusted serviceUrl" | ||
| ); | ||
| return Ok(json!({ | ||
| "status_code": 401, | ||
| "body": { "error": "Untrusted serviceUrl" } | ||
| })); | ||
| } | ||
| let app_id = get_secret(iii, "TEAMS_APP_ID").await; |
There was a problem hiding this comment.
Validate serviceUrl before calling agent::chat.
Right now, trust checking happens only after Lines 221-233 have already invoked agent::chat. Untrusted activities can still consume agent capacity and influence downstream behavior before being rejected.
🛡️ Proposed fix (move gate earlier)
+ let allowed_extra: Vec<String> = get_secret(iii, "TEAMS_ALLOWED_SERVICE_URLS")
+ .await
+ .split(',')
+ .map(|s| s.trim().to_string())
+ .filter(|s| !s.is_empty())
+ .collect();
+ if !is_allowed_service_url(&service_url, &allowed_extra) {
+ tracing::warn!(
+ service_url = %service_url,
+ "rejecting Teams activity with untrusted serviceUrl"
+ );
+ return Ok(json!({
+ "status_code": 401,
+ "body": { "error": "Untrusted serviceUrl" }
+ }));
+ }
+
let agent_id = resolve_agent(iii, "teams", &conversation_id).await;
@@
- if !reply.is_empty() {
- let allowed_extra: Vec<String> = get_secret(iii, "TEAMS_ALLOWED_SERVICE_URLS")
- .await
- .split(',')
- .map(|s| s.trim().to_string())
- .filter(|s| !s.is_empty())
- .collect();
- if !is_allowed_service_url(&service_url, &allowed_extra) {
- tracing::warn!(
- service_url = %service_url,
- "rejecting Teams activity with untrusted serviceUrl"
- );
- return Ok(json!({
- "status_code": 401,
- "body": { "error": "Untrusted serviceUrl" }
- }));
- }
+ if !reply.is_empty() {
let app_id = get_secret(iii, "TEAMS_APP_ID").await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-teams/src/main.rs` around lines 237 - 253, Move the
serviceUrl trust check to run before any call to agent::chat so untrusted
activities never reach the agent; specifically, after you obtain service_url
(and the allowed list via get_secret for "TEAMS_ALLOWED_SERVICE_URLS") call
is_allowed_service_url(&service_url, &allowed_extra) and return the 401 JSON
response immediately if it fails, ensuring the existing agent::chat invocation
(the agent::chat call site) is only reached when the check passes; update the
control flow around get_secret, is_allowed_service_url, and the agent::chat call
to reflect this early gate.
Comment on lines
+195
to
+223
| let raw_body = req.get("rawBody").and_then(|v| v.as_str()); | ||
| let signature = req | ||
| .get("headers") | ||
| .and_then(|h| h.get("x-hub-signature-256")) | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or(""); | ||
| if let Some(raw) = raw_body { | ||
| let secret = get_secret(iii, "WHATSAPP_APP_SECRET").await; | ||
| if secret.is_empty() { | ||
| return Ok(json!({ | ||
| "status_code": 500, | ||
| "body": { "error": "WHATSAPP_APP_SECRET not configured" } | ||
| })); | ||
| } | ||
| if signature.is_empty() { | ||
| return Ok(json!({ | ||
| "status_code": 401, | ||
| "body": { "error": "Missing X-Hub-Signature-256 header" } | ||
| })); | ||
| } | ||
| if let Err(e) = verify_meta_signature(&secret, raw, signature) { | ||
| tracing::warn!(error = %e, "whatsapp signature rejected"); | ||
| return Ok(json!({ | ||
| "status_code": 401, | ||
| "body": { "error": "Invalid X-Hub-Signature-256" } | ||
| })); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Fail closed when rawBody is missing for POST signature validation.
Line 201 gates validation behind if let Some(raw) = raw_body; when rawBody is absent, the webhook continues unauthenticated. This creates a bypass path.
🔒 Proposed fix
- let raw_body = req.get("rawBody").and_then(|v| v.as_str());
+ let raw_body = req.get("rawBody").and_then(|v| v.as_str());
@@
- if let Some(raw) = raw_body {
- let secret = get_secret(iii, "WHATSAPP_APP_SECRET").await;
- if secret.is_empty() {
- return Ok(json!({
- "status_code": 500,
- "body": { "error": "WHATSAPP_APP_SECRET not configured" }
- }));
- }
- if signature.is_empty() {
- return Ok(json!({
- "status_code": 401,
- "body": { "error": "Missing X-Hub-Signature-256 header" }
- }));
- }
- if let Err(e) = verify_meta_signature(&secret, raw, signature) {
- tracing::warn!(error = %e, "whatsapp signature rejected");
- return Ok(json!({
- "status_code": 401,
- "body": { "error": "Invalid X-Hub-Signature-256" }
- }));
- }
- }
+ let Some(raw) = raw_body else {
+ return Ok(json!({
+ "status_code": 401,
+ "body": { "error": "Missing rawBody for signature verification" }
+ }));
+ };
+ let secret = get_secret(iii, "WHATSAPP_APP_SECRET").await;
+ if secret.is_empty() {
+ return Ok(json!({
+ "status_code": 500,
+ "body": { "error": "WHATSAPP_APP_SECRET not configured" }
+ }));
+ }
+ if signature.is_empty() {
+ return Ok(json!({
+ "status_code": 401,
+ "body": { "error": "Missing X-Hub-Signature-256 header" }
+ }));
+ }
+ if let Err(e) = verify_meta_signature(&secret, raw, signature) {
+ tracing::warn!(error = %e, "whatsapp signature rejected");
+ return Ok(json!({
+ "status_code": 401,
+ "body": { "error": "Invalid X-Hub-Signature-256" }
+ }));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let raw_body = req.get("rawBody").and_then(|v| v.as_str()); | |
| let signature = req | |
| .get("headers") | |
| .and_then(|h| h.get("x-hub-signature-256")) | |
| .and_then(|v| v.as_str()) | |
| .unwrap_or(""); | |
| if let Some(raw) = raw_body { | |
| let secret = get_secret(iii, "WHATSAPP_APP_SECRET").await; | |
| if secret.is_empty() { | |
| return Ok(json!({ | |
| "status_code": 500, | |
| "body": { "error": "WHATSAPP_APP_SECRET not configured" } | |
| })); | |
| } | |
| if signature.is_empty() { | |
| return Ok(json!({ | |
| "status_code": 401, | |
| "body": { "error": "Missing X-Hub-Signature-256 header" } | |
| })); | |
| } | |
| if let Err(e) = verify_meta_signature(&secret, raw, signature) { | |
| tracing::warn!(error = %e, "whatsapp signature rejected"); | |
| return Ok(json!({ | |
| "status_code": 401, | |
| "body": { "error": "Invalid X-Hub-Signature-256" } | |
| })); | |
| } | |
| } | |
| let raw_body = req.get("rawBody").and_then(|v| v.as_str()); | |
| let signature = req | |
| .get("headers") | |
| .and_then(|h| h.get("x-hub-signature-256")) | |
| .and_then(|v| v.as_str()) | |
| .unwrap_or(""); | |
| let Some(raw) = raw_body else { | |
| return Ok(json!({ | |
| "status_code": 401, | |
| "body": { "error": "Missing rawBody for signature verification" } | |
| })); | |
| }; | |
| let secret = get_secret(iii, "WHATSAPP_APP_SECRET").await; | |
| if secret.is_empty() { | |
| return Ok(json!({ | |
| "status_code": 500, | |
| "body": { "error": "WHATSAPP_APP_SECRET not configured" } | |
| })); | |
| } | |
| if signature.is_empty() { | |
| return Ok(json!({ | |
| "status_code": 401, | |
| "body": { "error": "Missing X-Hub-Signature-256 header" } | |
| })); | |
| } | |
| if let Err(e) = verify_meta_signature(&secret, raw, signature) { | |
| tracing::warn!(error = %e, "whatsapp signature rejected"); | |
| return Ok(json!({ | |
| "status_code": 401, | |
| "body": { "error": "Invalid X-Hub-Signature-256" } | |
| })); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/channel-whatsapp/src/main.rs` around lines 195 - 223, The current
flow skips signature verification when raw_body is None, allowing
unauthenticated POSTs; change the logic so that absence of raw_body triggers an
immediate error response (fail-closed) before proceeding: check raw_body early
and if it's None return a 400/401 JSON error (similar format to the existing
responses), then proceed to call get_secret(...) and verify_meta_signature(...)
with raw and signature as before; reference symbols: raw_body variable,
signature extraction, get_secret(iii, "WHATSAPP_APP_SECRET"), and
verify_meta_signature(...) to locate and update the conditional.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses CodeRabbit findings on the recently merged channel-adapter PRs (#55 and #58). Stays inside the eight channel workers (reddit, linkedin, twitch, email, whatsapp, teams, webex, signal) plus the workspace
Cargo.toml. Also wires the reddit/linkedin/twitch crates into the workspace so they actually build undercargo check --workspace.Findings by severity → worker → file:line → status
Critical
workers/channel-linkedin/src/main.rs:215: webhook validation contract — fixedchallengeCodehandshake responds with{ challengeCode, challengeResponse }wherechallengeResponseis HMAC-SHA256(challengeCode, LINKEDIN_CLIENT_SECRET).X-LI-Signatureheader (hmacsha256={hex}) against the raw body using constant-time comparison.state::get/state::seton thechannel_dedupescope keyed bynotificationIdwith a 24h TTL.workers/channel-twitch/src/main.rs:125: EventSub signature verification — fixed. HMAC-SHA256 overMessage-Id + Message-Timestamp + raw body, expectedsha256={hex}, constant-time compare; missing/invalid headers return 401.workers/channel-whatsapp/src/main.rs:170:X-Hub-Signature-256validation — fixed. HMAC-SHA256 of raw body viaWHATSAPP_APP_SECRET, constant-time compare; rejects with 401 when missing/invalid.workers/channel-whatsapp/src/main.rs:23: MSRV declared — fixed. Workspacerust-version = "1.88"set and inherited by all eight channel crates.workers/channel-teams/src/main.rs:172: untrustedserviceUrltoken leakage — fixed.serviceUrlis now validated against Bot Framework host suffixes (*.botframework.com,*.botframework.azure.us,*.botframework.cn) plus an optionalTEAMS_ALLOWED_SERVICE_URLSallowlist before sending authenticated replies. Untrusted hosts return 401.workers/channel-webex/src/main.rs:183: bot self-loop guard — fixed. Bot's ownpersonIdcached lazily from/v1/people/me(process-localRwLock<Option<String>>); messages wheredata.personId == bot_idshort-circuit beforeagent::chat.Major
workers/channel-reddit/src/main.rs:132: token invalidation on 401 — fixed. OnUNAUTHORIZEDwe refresh, update the cache, and retry once.workers/channel-twitch/src/main.rs:96:sender_idshould be the bot — fixed. Now readsTWITCH_BOT_USER_IDand sends that in the Helix Send Chat Message payload.workers/channel-linkedin/src/main.rs:135: process all elements — fixed. Iteratesbody.elements[]and runsprocess_elementon each.workers/channel-linkedin/src/main.rs:224: HTTP timeout — fixed.reqwest::Client::builder().timeout(10s).build().workers/channel-email/src/main.rs:37: vault fallback for SMTP — fixed.build_transportnow readsSMTP_HOST/PORT/SECURE/USER/PASSviaget_secret(vault → env).workers/channel-email/src/main.rs:107: skip blank reply — fixed.reply.trim().is_empty()guard before SMTP send.workers/channel-email/src/main.rs:106: PII in logs — fixed. Sender address replaced withsha256:prefix-8 hash in failure logs.workers/channel-signal/src/main.rs:204: silent send failure — fixed. Send errors now return200 { ok:false, error }so the caller sees the failure instead of a bareok:true.workers/channel-teams/src/main.rs:248: HTTP timeouts — fixed. Shared client usesconnect_timeout(5s)+timeout(15s).workers/channel-webex/src/main.rs:83: propagate non-2xx fetch — fixed.fetch_messagenow returnsErrwith status + body excerpt instead ofOk(None).workers/channel-whatsapp/src/main.rs:164: don't fail webhook onagent::chat— fixed. Errors are logged and treated as empty reply; webhook still returns 200.workers/channel-whatsapp/src/main.rs:171: PII in logs — fixed.fromredacted viasha256:prefix-8 hash in send-failure logs.workers/channel-whatsapp/src/main.rs:220: GET verification endpoint — fixed. New GET trigger handleshub.mode=subscribeand echoeshub.challengeafter constant-timehub.verify_tokencheck againstWHATSAPP_VERIFY_TOKEN.Minor
workers/channel-signal/src/main.rs:172: 500 on empty reply triggers retries — fixed. Now200 { ok:false, error: "Empty agent response" }.workers/channel-teams/src/main.rs:66: leading\nafter newline split — fixed.workers/channel-webex/src/main.rs:69: leading\nafter newline split — fixed.workers/channel-whatsapp/src/main.rs:68: leading\nafter newline split — fixed.Out of scope (deferred)
The following CR comments were on
src/__tests__/*.test.ts(TypeScript test suites undersrc/), which falls outside the "8 channel workers + shared types" scope of this PR and is left for a follow-up:src/__tests__/channels-mastodon.test.ts:1(🟡 ts-nocheck) — deferredsrc/__tests__/channels-mastodon.test.ts:35(🟠 shared harness) — deferredsrc/__tests__/channels-reddit.test.ts:63(🟠 duplicates Mastodon suite) — deferredsrc/__tests__/channels-matrix.test.ts:153(🟡 conditional assertion) — deferredsrc/__tests__/channels-matrix.test.ts:181(🟡 conditional assertion) — deferredsrc/__tests__/channels-teams.test.ts:61(🟠 shared harness) — deferredCounts
Test plan
cargo check --workspacecargo test --workspace --releasecargo clippy -p agentos-channel-{reddit,linkedin,twitch,email,whatsapp,teams,webex,signal} --all-targets -- -D warningsserviceUrlallowlist (4 cases)Summary by CodeRabbit
New Features
Bug Fixes
Security