feat(channels): add Plivo SMS channel adapter#20
Open
sarveshpatil-plivo wants to merge 5 commits into
Open
Conversation
Fill the `sms` channel slot with a Plivo-backed adapter using Plivo's
Messaging API. Outbound sends via POST /v1/Account/{authId}/Message/;
inbound messages arrive on a webhook the host forwards to
handleIncomingWebhook, which verifies the X-Plivo-Signature-V3 signature
and fails closed before emitting.
Register the adapter in adapters/index.ts and document setup in
docs/features/CHANNELS.md. Unit tests cover outbound send, inbound
emit/drop, and a golden-fixture signature check against the Plivo SDK.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: sarveshpatil-plivo <sarvesh.patil@plivo.com>
Register the adapter under a dedicated 'plivo' ChannelPlatform instead of taking over the shared 'sms' slot, and restore the original Twilio entry in the channels doc (Plivo is added as a separate row/section, not a swap). Signed-off-by: sarveshpatil-plivo <sarvesh.patil@plivo.com>
- fail fast on 401/403 account-verification (auth is known-bad), while still tolerating transient/network errors at connect - log when an inbound webhook is dropped because the adapter is not connected Signed-off-by: sarveshpatil-plivo <sarvesh.patil@plivo.com>
sarveshpatil-plivo
requested review from
Victor-Evogor and
jddunn
as code owners
July 14, 2026 08:10
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Reviewer's GuideAdds a new Plivo-backed SMS messaging channel adapter, wires it into the existing channel registry and types, documents configuration and usage, and implements signature-verified inbound webhook handling with accompanying tests for outbound/inbound flows and Plivo V3 signature computation. Sequence diagram for inbound Plivo SMS webhook handlingsequenceDiagram
participant Plivo
participant HostApp
participant PlivoSmsChannelAdapter
participant AgentOSCore
Plivo->>HostApp: POST /plivo/inbound (SMS webhook)
HostApp->>PlivoSmsChannelAdapter: handleIncomingWebhook(body, meta)
PlivoSmsChannelAdapter->>PlivoSmsChannelAdapter: isFromPlivo(body, meta)
PlivoSmsChannelAdapter->>PlivoSmsChannelAdapter: computePlivoV3Signature(method, url, nonce, authToken, params)
alt valid_signature
PlivoSmsChannelAdapter->>AgentOSCore: emit({ type: message, platform: plivo, data: ChannelMessage })
else invalid_or_missing_signature
PlivoSmsChannelAdapter-->>AgentOSCore: [drop webhook]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPlivo SMS support adds a channel adapter for outbound messages and inbound webhooks, including Plivo V3 signature verification, public channel exports, platform typing, tests, and setup documentation. ChangesPlivo SMS channel
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Plivo
participant ExpressRoute
participant PlivoSmsChannelAdapter
participant AgentOS
Plivo->>ExpressRoute: POST signed SMS webhook
ExpressRoute->>PlivoSmsChannelAdapter: handleIncomingWebhook(body, method, URL, headers)
PlivoSmsChannelAdapter->>PlivoSmsChannelAdapter: Validate V3 signature
PlivoSmsChannelAdapter->>AgentOS: Emit normalized message event
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd Plivo SMS channel adapter with signed inbound webhook verification
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/io/channels/adapters/PlivoSmsChannelAdapter.ts" line_range="277-280" />
<code_context>
+ const headers = meta?.headers ?? {};
+ const signature = headerValue(headers, 'x-plivo-signature-v3');
+ const nonce = headerValue(headers, 'x-plivo-signature-v3-nonce');
+ const url = meta?.url ?? this.webhookUrl;
+ const method = (meta?.method ?? 'POST').toUpperCase();
+
+ if (!signature || !nonce || !url || !this.authToken) return false;
+
+ let expected: string;
</code_context>
<issue_to_address>
**suggestion:** Dropping inbound webhooks when `verifySignatureEnabled` is true but no URL is available may lead to silent misconfiguration.
Because `meta.url` may be missing and `this.webhookUrl` may be unset, all inbound messages are marked untrusted (`return false`) and then dropped, with no clear indication of misconfiguration. Consider either enforcing that `webhookUrl` is set in `doConnect` when `verifySignatureEnabled` is true, or logging a clear error when `url` is missing so operators understand why webhooks are being rejected.
Suggested implementation:
```typescript
const url = meta?.url ?? this.webhookUrl;
const method = (meta?.method ?? 'POST').toUpperCase();
if (!signature || !nonce || !this.authToken) return false;
if (!url) {
this.logger?.error(
'Plivo webhook signature verification failed: no request URL available. ' +
'Ensure webhookUrl is configured or meta.url is provided when verifySignatureEnabled is true.',
);
return false;
}
```
1. Ensure the adapter class has a `logger` instance (or similar) available; if not, introduce one consistent with the existing logging conventions in this project and pass it into `PlivoSmsChannelAdapter`.
2. Optionally, enforce that `webhookUrl` is set in your `doConnect` (or equivalent initialization) method when `verifySignatureEnabled` is true by validating configuration there and logging/throwing a configuration error early, so misconfiguration is caught before handling webhooks.
</issue_to_address>
### Comment 2
<location path="docs/features/CHANNELS.md" line_range="297" />
<code_context>
+```bash
+export PLIVO_AUTH_ID=your-auth-id
+export PLIVO_AUTH_TOKEN=your-auth-token
+export PLIVO_PHONE=+14150000000
+```
+
</code_context>
<issue_to_address>
**suggestion:** Consider clarifying the expected format of `PLIVO_PHONE` in the example.
Add a brief note indicating that `PLIVO_PHONE` should be in E.164 format (for example, `+14150000000`), or explicitly state the format Plivo requires to reduce the chance of misconfiguration.
</issue_to_address>
### Comment 3
<location path="src/io/channels/adapters/PlivoSmsChannelAdapter.ts" line_range="376" />
<code_context>
+// Plivo SMS Auth Params
+// ============================================================================
+
+/** Platform-specific parameters for a Plivo SMS connection. */
+export interface PlivoSmsAuthParams extends Record<string, string | undefined> {
+ /** Plivo Auth ID (account identifier). Required. */
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the adapter’s configuration and verification paths by tightening types, specializing POST-only webhook handling, and reducing generic helpers to the concrete Plivo SMS shapes you actually use.
You can trim some of the over‑generalization without losing any current behavior by tightening types and specializing the hot paths.
### 1. Use a boolean flag for signature verification
The current string protocol (`verifySignature !== 'false'`) is surprising. A boolean is simpler and self‑documenting:
```ts
// interface
export interface PlivoSmsAuthParams {
// ...
/** When false, inbound signature verification is disabled. Defaults to true. */
verifySignature?: boolean;
}
// doConnect
this.verifySignatureEnabled = params.verifySignature ?? true;
```
This keeps the default behavior (enabled) but removes the “anything but 'false'” quirk.
### 2. Make `PlivoSmsAuthParams` a closed interface
`extends Record<string, string | undefined>` suggests arbitrary keys, which aren’t used and adds surface area. A closed interface is easier to reason about:
```ts
export interface PlivoSmsAuthParams {
/** Plivo Auth ID (account identifier). Required. */
authId?: string;
/** Plivo Auth Token. If omitted, the `credential` field is used. */
authToken?: string;
/** Plivo sender number / short code / sender id used as `src`. Required. */
phoneNumber?: string;
/** Externally-visible message URL Plivo signs; used to verify inbound webhooks. */
webhookUrl?: string;
/** When false, inbound signature verification is disabled. Defaults to true. */
verifySignature?: boolean;
}
```
If you really need arbitrary extra keys, you can explicitly model them:
```ts
/** Extra provider-specific options. */
extraParams?: Record<string, string | undefined>;
```
### 3. Specialize the inbound verification path to POST callbacks
The adapter only ever verifies POST callbacks. You can simplify `isFromPlivo` by hardcoding POST instead of threading a generic `method`, and by requiring `url` when verification is on:
```ts
handleIncomingWebhook(
body: Record<string, unknown>,
meta?: {
/** The exact externally-visible URL Plivo POSTed to (must byte-match). */
url: string;
headers: Record<string, string | string[] | undefined>;
},
): void {
if (this.status !== 'connected') {
console.warn('[Plivo SMS] Dropping inbound webhook — adapter not connected.');
return;
}
if (this.verifySignatureEnabled && (!meta || !this.isFromPlivo(body, meta))) {
console.warn('[Plivo SMS] Dropping inbound webhook — signature missing or invalid.');
return;
}
// ...
}
private isFromPlivo(
body: Record<string, unknown>,
meta: { url: string; headers: Record<string, string | string[] | undefined> },
): boolean {
const signature = headerValue(meta.headers, 'x-plivo-signature-v3');
const nonce = headerValue(meta.headers, 'x-plivo-signature-v3-nonce');
if (!signature || !nonce || !this.authToken) return false;
let expected: string;
try {
expected = computePlivoV3Signature({
method: 'POST', // only POST used by this adapter
url: meta.url,
nonce,
authToken: this.authToken,
params: body,
});
} catch {
return false;
}
// ...
}
```
This removes the “optional method + fallback URL” branching from the adapter surface, while still allowing `computePlivoV3Signature` to support GET in case you need it elsewhere.
### 4. Narrow `sortedParamsString` to the flat body shape you actually see
Plivo’s inbound SMS payload is a flat form. You can simplify the serializer to a flat map of primitives and drop recursion/array sorting (keeping the function exported and name intact):
```ts
export function computePlivoV3Signature(input: {
method: string;
url: string;
nonce: string;
authToken: string;
params: Record<string, string | number | boolean | undefined>;
}): string {
// ...
if (isPost) {
signed += sortedParamsString(input.params) + '.' + nonce;
}
// ...
}
function sortedParamsString(params: Record<string, string | number | boolean | undefined>): string {
let out = '';
for (const key of Object.keys(params).sort()) {
const value = params[key];
if (value === undefined) continue;
out += key + String(value);
}
return out;
}
```
Call sites that currently pass `Record<string, unknown>` can be constrained at the boundary (e.g., by coercing to `Record<string, string | number | boolean | undefined>`), but the adapter’s concrete usage (flat inbound payload) remains unchanged and the implementation is much easier to follow.
### 5. Avoid generic header scanning in a tight loop
Since `isFromPlivo` only cares about two headers, normalizing header keys once can be clearer than a generic search per lookup:
```ts
private isFromPlivo(
body: Record<string, unknown>,
meta: { url: string; headers: Record<string, string | string[] | undefined> },
): boolean {
const headersLower = new Map<string, string>();
for (const [k, v] of Object.entries(meta.headers)) {
const value = Array.isArray(v) ? v[0] : v;
if (typeof value === 'string') {
headersLower.set(k.toLowerCase(), value);
}
}
const signature = headersLower.get('x-plivo-signature-v3');
const nonce = headersLower.get('x-plivo-signature-v3-nonce');
// ...
}
```
With this in place, you can either inline `headerValue` usage here or drop the helper entirely; either way, you avoid iterating over all headers for each lookup.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Plivo SMS Auth Params | ||
| // ============================================================================ | ||
|
|
||
| /** Platform-specific parameters for a Plivo SMS connection. */ |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the adapter’s configuration and verification paths by tightening types, specializing POST-only webhook handling, and reducing generic helpers to the concrete Plivo SMS shapes you actually use.
You can trim some of the over‑generalization without losing any current behavior by tightening types and specializing the hot paths.
1. Use a boolean flag for signature verification
The current string protocol (verifySignature !== 'false') is surprising. A boolean is simpler and self‑documenting:
// interface
export interface PlivoSmsAuthParams {
// ...
/** When false, inbound signature verification is disabled. Defaults to true. */
verifySignature?: boolean;
}
// doConnect
this.verifySignatureEnabled = params.verifySignature ?? true;This keeps the default behavior (enabled) but removes the “anything but 'false'” quirk.
2. Make PlivoSmsAuthParams a closed interface
extends Record<string, string | undefined> suggests arbitrary keys, which aren’t used and adds surface area. A closed interface is easier to reason about:
export interface PlivoSmsAuthParams {
/** Plivo Auth ID (account identifier). Required. */
authId?: string;
/** Plivo Auth Token. If omitted, the `credential` field is used. */
authToken?: string;
/** Plivo sender number / short code / sender id used as `src`. Required. */
phoneNumber?: string;
/** Externally-visible message URL Plivo signs; used to verify inbound webhooks. */
webhookUrl?: string;
/** When false, inbound signature verification is disabled. Defaults to true. */
verifySignature?: boolean;
}If you really need arbitrary extra keys, you can explicitly model them:
/** Extra provider-specific options. */
extraParams?: Record<string, string | undefined>;3. Specialize the inbound verification path to POST callbacks
The adapter only ever verifies POST callbacks. You can simplify isFromPlivo by hardcoding POST instead of threading a generic method, and by requiring url when verification is on:
handleIncomingWebhook(
body: Record<string, unknown>,
meta?: {
/** The exact externally-visible URL Plivo POSTed to (must byte-match). */
url: string;
headers: Record<string, string | string[] | undefined>;
},
): void {
if (this.status !== 'connected') {
console.warn('[Plivo SMS] Dropping inbound webhook — adapter not connected.');
return;
}
if (this.verifySignatureEnabled && (!meta || !this.isFromPlivo(body, meta))) {
console.warn('[Plivo SMS] Dropping inbound webhook — signature missing or invalid.');
return;
}
// ...
}
private isFromPlivo(
body: Record<string, unknown>,
meta: { url: string; headers: Record<string, string | string[] | undefined> },
): boolean {
const signature = headerValue(meta.headers, 'x-plivo-signature-v3');
const nonce = headerValue(meta.headers, 'x-plivo-signature-v3-nonce');
if (!signature || !nonce || !this.authToken) return false;
let expected: string;
try {
expected = computePlivoV3Signature({
method: 'POST', // only POST used by this adapter
url: meta.url,
nonce,
authToken: this.authToken,
params: body,
});
} catch {
return false;
}
// ...
}This removes the “optional method + fallback URL” branching from the adapter surface, while still allowing computePlivoV3Signature to support GET in case you need it elsewhere.
4. Narrow sortedParamsString to the flat body shape you actually see
Plivo’s inbound SMS payload is a flat form. You can simplify the serializer to a flat map of primitives and drop recursion/array sorting (keeping the function exported and name intact):
export function computePlivoV3Signature(input: {
method: string;
url: string;
nonce: string;
authToken: string;
params: Record<string, string | number | boolean | undefined>;
}): string {
// ...
if (isPost) {
signed += sortedParamsString(input.params) + '.' + nonce;
}
// ...
}
function sortedParamsString(params: Record<string, string | number | boolean | undefined>): string {
let out = '';
for (const key of Object.keys(params).sort()) {
const value = params[key];
if (value === undefined) continue;
out += key + String(value);
}
return out;
}Call sites that currently pass Record<string, unknown> can be constrained at the boundary (e.g., by coercing to Record<string, string | number | boolean | undefined>), but the adapter’s concrete usage (flat inbound payload) remains unchanged and the implementation is much easier to follow.
5. Avoid generic header scanning in a tight loop
Since isFromPlivo only cares about two headers, normalizing header keys once can be clearer than a generic search per lookup:
private isFromPlivo(
body: Record<string, unknown>,
meta: { url: string; headers: Record<string, string | string[] | undefined> },
): boolean {
const headersLower = new Map<string, string>();
for (const [k, v] of Object.entries(meta.headers)) {
const value = Array.isArray(v) ? v[0] : v;
if (typeof value === 'string') {
headersLower.set(k.toLowerCase(), value);
}
}
const signature = headersLower.get('x-plivo-signature-v3');
const nonce = headersLower.get('x-plivo-signature-v3-nonce');
// ...
}With this in place, you can either inline headerValue usage here or drop the helper entirely; either way, you avoid iterating over all headers for each lookup.
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+270
to
+340
| private isFromPlivo( | ||
| body: Record<string, unknown>, | ||
| meta?: { method?: string; url?: string; headers?: Record<string, string | string[] | undefined> }, | ||
| ): boolean { | ||
| const headers = meta?.headers ?? {}; | ||
| const signature = headerValue(headers, 'x-plivo-signature-v3'); | ||
| const nonce = headerValue(headers, 'x-plivo-signature-v3-nonce'); | ||
| const url = meta?.url ?? this.webhookUrl; | ||
| const method = (meta?.method ?? 'POST').toUpperCase(); | ||
|
|
||
| if (!signature || !nonce || !url || !this.authToken) return false; | ||
|
|
||
| let expected: string; | ||
| try { | ||
| expected = computePlivoV3Signature({ method, url, nonce, authToken: this.authToken, params: body }); | ||
| } catch { | ||
| return false; // malformed URL/input is untrusted, not a crash | ||
| } | ||
|
|
||
| const expectedBuf = Buffer.from(expected); | ||
| return signature.split(',').some((candidate) => { | ||
| const candBuf = Buffer.from(candidate.trim()); | ||
| return candBuf.length === expectedBuf.length && timingSafeEqual(candBuf, expectedBuf); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // ============================================================================ | ||
| // Signature helpers (exported for testing against the Plivo SDK golden fixture) | ||
| // ============================================================================ | ||
|
|
||
| /** | ||
| * Compute Plivo's X-Plivo-Signature-V3 for a callback, matching the algorithm | ||
| * in `plivo-python`'s `signature_v3.py`. | ||
| * | ||
| * For a POST callback the signed string is: | ||
| * `{scheme}://{host}{path}?` + (query as sorted `k=v&…` + `.` if present) | ||
| * + sorted, separator-less `key`+`value` body params + `.` + nonce | ||
| * then `base64(HMAC_SHA256(authToken, signedString))`. | ||
| */ | ||
| export function computePlivoV3Signature(input: { | ||
| method: string; | ||
| url: string; | ||
| nonce: string; | ||
| authToken: string; | ||
| params: Record<string, unknown>; | ||
| }): string { | ||
| const { method, url, nonce, authToken, params } = input; | ||
| const parsed = new URL(url); | ||
| const base = `${parsed.protocol}//${parsed.host}${parsed.pathname}`; | ||
| const isPost = method.toUpperCase() === 'POST'; | ||
|
|
||
| let signed = base + '?'; | ||
|
|
||
| // If the URL carried a query string, append it as sorted k=v&k=v. | ||
| if (parsed.search && parsed.search.length > 1) { | ||
| signed += [...parsed.searchParams.entries()] | ||
| .sort((a, b) => (a[0] < b[0] ? -1 : a[0] > b[0] ? 1 : 0)) | ||
| .map(([k, v]) => `${k}=${v}`) | ||
| .join('&'); | ||
| if (isPost) signed += '.'; // separator between query and the POST params | ||
| } | ||
|
|
||
| // POST callbacks append sorted key+value body params, then '.' + nonce. | ||
| // GET callbacks are signed over the query string alone (params ARE the query). | ||
| if (isPost) { | ||
| signed += sortedParamsString(params) + '.' + nonce; | ||
| } | ||
|
|
||
| return createHmac('sha256', authToken).update(signed).digest('base64'); | ||
| } |
There was a problem hiding this comment.
2. Plivo v3 signature inconsistency 🐞 Bug ⛨ Security
PlivoSmsChannelAdapter computes/validates X-Plivo-Signature-V3 using a URL+query+sorted-body-params+nonce signing string, while the existing PlivoVoiceProvider in this repo documents and implements Plivo V3 verification as HMAC(url + nonce) with no body involvement. With two incompatible V3 algorithms in-tree for the same header names, at least one Plivo webhook surface (voice or SMS) is likely being verified with the wrong contract unless Plivo explicitly uses different schemes per product.
Agent Prompt
## Issue description
Two different “Plivo V3” signature algorithms exist in the repo:
- SMS adapter: signs URL (with normalized query) + sorted body params + `.` + nonce.
- Voice provider: signs `{fullRequestURL}{nonce}` only (explicitly states body is not included).
This inconsistency makes signature verification behavior ambiguous and increases the risk of rejecting valid webhooks (or documenting the wrong security behavior).
## Issue Context
- The SMS adapter is new and defaults to `verifySignatureEnabled = true`, so if its algorithm does not match Plivo’s SMS webhook contract, inbound SMS will be dropped.
- The voice provider is already implemented/tested with the URL+nonce scheme.
## Fix Focus Areas
- Decide and document the authoritative signing contract(s) and make implementations consistent (either shared helper or clearly named product-specific helpers):
- src/io/channels/adapters/PlivoSmsChannelAdapter.ts[264-340]
- src/io/channels/telephony/providers/plivo.ts[23-36]
- src/io/channels/telephony/providers/plivo.ts[151-183]
- Add/adjust tests to prevent future drift between Plivo webhook verifiers:
- src/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.ts[18-55]
- src/io/channels/telephony/__tests__/PlivoVoiceProvider.test.ts[38-66]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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 (2)
src/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.ts (1)
95-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for connection-hardening behaviors called out in this PR.
The commit summary highlights two hardening behaviors — failing fast on invalid account credentials (401/403) and logging inbound webhooks dropped while disconnected — but neither has a dedicated test here. Worth adding:
- A
doConnect/initializetest wheremakeFetchreturnsstatus: 401for the account GET, assertinginitializerejects.- A
handleIncomingWebhooktest called beforeconnect()(or aftershutdown()), asserting no event is emitted.Want me to draft these two test cases?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.ts` around lines 95 - 216, The Plivo adapter tests lack coverage for credential failures during initialization and webhook handling while disconnected. Add a connection test using makeFetch to return 401 for the account GET and assert initialize rejects, then add a handleIncomingWebhook test before connect or after shutdown that asserts no message event is emitted and preserves the disconnected-drop behavior.src/io/channels/adapters/PlivoSmsChannelAdapter.ts (1)
118-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile fail-fast detection via
Error.messagestring matching.The 401/403 fail-fast (Line 135) is re-detected in the
catchblock viaerr.message.includes('Authentication failed')(Line 139). Editing either string independently (e.g., i18n, wording tweak) silently turns a hard-auth failure into a tolerated warning, defeating the fail-fast goal. Prefer a typed/sentinel error (custom error class or a property flag) instead of substring matching.♻️ Suggested refactor
+class PlivoAuthError extends Error {} + if (resp.status === 401 || resp.status === 403) { - throw new Error(`[Plivo SMS] Authentication failed (HTTP ${resp.status}) — check authId/authToken.`); + throw new PlivoAuthError(`[Plivo SMS] Authentication failed (HTTP ${resp.status}) — check authId/authToken.`); } console.warn(`[Plivo SMS] Account verification returned HTTP ${resp.status}.`); } catch (err) { - if (err instanceof Error && err.message.includes('Authentication failed')) { + if (err instanceof PlivoAuthError) { throw err; } console.warn(`[Plivo SMS] Account verification failed: ${err}`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/io/channels/adapters/PlivoSmsChannelAdapter.ts` around lines 118 - 143, Replace the authentication-failure message substring check in the verification method with a typed or sentinel error shared by the 401/403 branch and the catch block. Update the branch that throws on unauthorized responses and the catch handling so this error is rethrown reliably regardless of message wording, while other failures remain warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/features/CHANNELS.md`:
- Line 48: Use PLIVO_PHONE_NUMBER consistently in both documented locations:
update the Plivo required-variable table at docs/features/CHANNELS.md lines
48-48, and update the setup example’s export and read references at
docs/features/CHANNELS.md lines 295-309. No other documentation changes are
needed.
In `@src/io/channels/adapters/PlivoSmsChannelAdapter.ts`:
- Around line 119-122: Add request timeouts to both outbound Plivo calls in the
PlivoSmsChannelAdapter: the account-verification fetch and the outbound Message/
POST. Pass an appropriate AbortSignal.timeout value in each fetch request while
preserving the existing headers, authentication, and response handling.
- Around line 270-340: Update isFromPlivo to validate Plivo SMS webhooks using
the X-Plivo-Signature-V2 header and its corresponding V2 verification algorithm,
instead of requiring X-Plivo-Signature-V3 and nonce. Preserve the existing
fail-closed behavior when the signature, URL, or auth token is missing, and use
the appropriate V2 signature helper or implementation.
---
Nitpick comments:
In `@src/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.ts`:
- Around line 95-216: The Plivo adapter tests lack coverage for credential
failures during initialization and webhook handling while disconnected. Add a
connection test using makeFetch to return 401 for the account GET and assert
initialize rejects, then add a handleIncomingWebhook test before connect or
after shutdown that asserts no message event is emitted and preserves the
disconnected-drop behavior.
In `@src/io/channels/adapters/PlivoSmsChannelAdapter.ts`:
- Around line 118-143: Replace the authentication-failure message substring
check in the verification method with a typed or sentinel error shared by the
401/403 branch and the catch block. Update the branch that throws on
unauthorized responses and the catch handling so this error is rethrown reliably
regardless of message wording, while other failures remain warnings.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 62bf8496-20ba-4390-8d90-c73c8c91f4fc
📒 Files selected for processing (5)
docs/features/CHANNELS.mdsrc/io/channels/adapters/PlivoSmsChannelAdapter.tssrc/io/channels/adapters/__tests__/PlivoSmsChannelAdapter.test.tssrc/io/channels/adapters/index.tssrc/io/channels/types.ts
Comment on lines
+119
to
+122
| const resp = await this.fetchImpl( | ||
| `https://api.plivo.com/v1/Account/${this.authId}/`, | ||
| { headers: { Authorization: this.authHeader } }, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No timeout on outbound Plivo fetch calls.
Neither the account-verification GET (Lines 119-122) nor the outbound Message/ POST (Lines 167-182) sets a request timeout (e.g., AbortSignal.timeout(...)). A slow/hung Plivo API response would block the caller (connect or sendMessage) indefinitely.
🕐 Suggested fix
const resp = await this.fetchImpl(
`https://api.plivo.com/v1/Account/${this.authId}/Message/`,
{
method: 'POST',
headers: { ... },
body: JSON.stringify({ ... }),
+ signal: AbortSignal.timeout(10_000),
},
);Also applies to: 167-182
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/io/channels/adapters/PlivoSmsChannelAdapter.ts` around lines 119 - 122,
Add request timeouts to both outbound Plivo calls in the PlivoSmsChannelAdapter:
the account-verification fetch and the outbound Message/ POST. Pass an
appropriate AbortSignal.timeout value in each fetch request while preserving the
existing headers, authentication, and response handling.
Comment on lines
+270
to
+340
| private isFromPlivo( | ||
| body: Record<string, unknown>, | ||
| meta?: { method?: string; url?: string; headers?: Record<string, string | string[] | undefined> }, | ||
| ): boolean { | ||
| const headers = meta?.headers ?? {}; | ||
| const signature = headerValue(headers, 'x-plivo-signature-v3'); | ||
| const nonce = headerValue(headers, 'x-plivo-signature-v3-nonce'); | ||
| const url = meta?.url ?? this.webhookUrl; | ||
| const method = (meta?.method ?? 'POST').toUpperCase(); | ||
|
|
||
| if (!signature || !nonce || !url || !this.authToken) return false; | ||
|
|
||
| let expected: string; | ||
| try { | ||
| expected = computePlivoV3Signature({ method, url, nonce, authToken: this.authToken, params: body }); | ||
| } catch { | ||
| return false; // malformed URL/input is untrusted, not a crash | ||
| } | ||
|
|
||
| const expectedBuf = Buffer.from(expected); | ||
| return signature.split(',').some((candidate) => { | ||
| const candBuf = Buffer.from(candidate.trim()); | ||
| return candBuf.length === expectedBuf.length && timingSafeEqual(candBuf, expectedBuf); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // ============================================================================ | ||
| // Signature helpers (exported for testing against the Plivo SDK golden fixture) | ||
| // ============================================================================ | ||
|
|
||
| /** | ||
| * Compute Plivo's X-Plivo-Signature-V3 for a callback, matching the algorithm | ||
| * in `plivo-python`'s `signature_v3.py`. | ||
| * | ||
| * For a POST callback the signed string is: | ||
| * `{scheme}://{host}{path}?` + (query as sorted `k=v&…` + `.` if present) | ||
| * + sorted, separator-less `key`+`value` body params + `.` + nonce | ||
| * then `base64(HMAC_SHA256(authToken, signedString))`. | ||
| */ | ||
| export function computePlivoV3Signature(input: { | ||
| method: string; | ||
| url: string; | ||
| nonce: string; | ||
| authToken: string; | ||
| params: Record<string, unknown>; | ||
| }): string { | ||
| const { method, url, nonce, authToken, params } = input; | ||
| const parsed = new URL(url); | ||
| const base = `${parsed.protocol}//${parsed.host}${parsed.pathname}`; | ||
| const isPost = method.toUpperCase() === 'POST'; | ||
|
|
||
| let signed = base + '?'; | ||
|
|
||
| // If the URL carried a query string, append it as sorted k=v&k=v. | ||
| if (parsed.search && parsed.search.length > 1) { | ||
| signed += [...parsed.searchParams.entries()] | ||
| .sort((a, b) => (a[0] < b[0] ? -1 : a[0] > b[0] ? 1 : 0)) | ||
| .map(([k, v]) => `${k}=${v}`) | ||
| .join('&'); | ||
| if (isPost) signed += '.'; // separator between query and the POST params | ||
| } | ||
|
|
||
| // POST callbacks append sorted key+value body params, then '.' + nonce. | ||
| // GET callbacks are signed over the query string alone (params ARE the query). | ||
| if (isPost) { | ||
| signed += sortedParamsString(params) + '.' + nonce; | ||
| } | ||
|
|
||
| return createHmac('sha256', authToken).update(signed).digest('base64'); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Plivo currently support X-Plivo-Signature-V3 for SMS/messaging webhooks, or only X-Plivo-Signature-V2?
💡 Result:
Plivo currently uses X-Plivo-Signature-V2 for SMS/messaging webhooks [1][2][3][4]. While X-Plivo-Signature-V3 is supported by Plivo, it is specifically used for voice callbacks, which incorporate a nonce for enhanced security (replay protection) [5][6][2][7]. SMS callbacks and messaging-related webhooks continue to rely on the X-Plivo-Signature-V2 header for signature validation [1][2][3].
Citations:
- 1: https://plivo.com/docs/messaging/concepts/signature-validation
- 2: https://www.sent.dm/es/resources/code-examples/plivo-node-js-nestjs-delivery-status-and-callbacks
- 3: https://www.sent.dm/en/resources/tools/plivo-node-js-express-delivery-status-and-callbacks
- 4: https://support.plivo.com/hc/en-us/articles/38549637789337-How-can-I-validate-that-OTP-requests-and-callbacks-are-authentic
- 5: https://plivo.com/docs/voice/concepts/signature-validation
- 6: https://github.com/plivo/plivo-examples-java/blob/master/voice/validateSignature.java
- 7: https://plivo.com/docs/voice/concepts/callbacks
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/io/channels/adapters/PlivoSmsChannelAdapter.ts"
wc -l "$FILE"
sed -n '80,360p' "$FILE" | cat -nRepository: framerslab/agentos
Length of output: 12059
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/io/channels/adapters/PlivoSmsChannelAdapter.ts"
# Show the configuration fields and the webhook verification method.
sed -n '1,120p' "$FILE" | cat -n
printf '\n---\n'
sed -n '250,345p' "$FILE" | cat -nRepository: framerslab/agentos
Length of output: 9657
Use Plivo SMS signature V2
isFromPlivo()only acceptsX-Plivo-Signature-V3/-Nonce, but Plivo SMS webhooks useX-Plivo-Signature-V2; with signature checking enabled by default, valid inbound SMS will be dropped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/io/channels/adapters/PlivoSmsChannelAdapter.ts` around lines 270 - 340,
Update isFromPlivo to validate Plivo SMS webhooks using the X-Plivo-Signature-V2
header and its corresponding V2 verification algorithm, instead of requiring
X-Plivo-Signature-V3 and nonce. Preserve the existing fail-closed behavior when
the signature, URL, or auth token is missing, and use the appropriate V2
signature helper or implementation.
- Re-export PlivoSmsChannelAdapter and PlivoSmsAuthParams from the channels barrel so the documented import from '@framers/agentos' resolves. - Use PLIVO_PHONE_NUMBER in the docs to match the configured secret name, and note the sender must be E.164. - Log a clear warning when an inbound webhook cannot be verified because no request URL is available, instead of a silent failure that looks like a bad signature.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/io/channels/index.ts`:
- Line 14: Update the Phase 4 comment in the adapter barrel to accurately state
“base class + 12 platform adapters,” unless the intended behavior is to export
14, in which case add the two missing adapter exports. Keep the comment and
exported adapter count consistent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ee1ab8ec-0808-4601-a81b-f46e531a2e3b
📒 Files selected for processing (3)
docs/features/CHANNELS.mdsrc/io/channels/adapters/PlivoSmsChannelAdapter.tssrc/io/channels/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/features/CHANNELS.md
- src/io/channels/adapters/PlivoSmsChannelAdapter.ts
| export type { GroupPolicyInput, GroupPolicyResult, GroupPolicyReason } from './group-policy.js'; | ||
|
|
||
| // Phase 4: Adapter implementations — base class + 13 platform adapters | ||
| // Phase 4: Adapter implementations — base class + 14 platform adapters |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the adapter count in the phase comment.
This barrel currently exports 12 platform adapters, not 14. Update the comment to base class + 12 platform adapters, or add the two missing adapter exports if 14 is the intended total.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/io/channels/index.ts` at line 14, Update the Phase 4 comment in the
adapter barrel to accurately state “base class + 12 platform adapters,” unless
the intended behavior is to export 14, in which case add the two missing adapter
exports. Keep the comment and exported adapter count consistent.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
through another provider.
Plivo, registered as its own
plivochannel platform alongside the existingadapters.
subsystem, filling the SMS gap without touching voice.
so unsigned or invalid requests are dropped.
Checklist
Summary by Sourcery
Add a Plivo-backed SMS messaging channel adapter and wire it into the channels platform registry.
New Features:
plivomessaging platform.Documentation:
Tests:
Summary by CodeRabbit