implemented Microsoft Teams delivery channel - #557
Conversation
|
@idrisososanwo Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a Microsoft Teams alert channel that validates webhook URLs, generates Adaptive Card payloads for alert events, supports custom templates, posts through ChangesTeams alert delivery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AlertDispatcher
participant sendTeamsAlert
participant renderAlertTemplate
participant TeamsWebhook
AlertDispatcher->>sendTeamsAlert: webhook URL and AlertEvent
sendTeamsAlert->>renderAlertTemplate: render teams template
sendTeamsAlert->>TeamsWebhook: POST JSON payload
TeamsWebhook-->>sendTeamsAlert: HTTP response
sendTeamsAlert-->>AlertDispatcher: completion or error
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/alerts/teams.ts`:
- Around line 171-195: Update validateWebhookUrl to require the parsed URL to
use the https: scheme and to validate the hostname as a genuine
*.webhook.office.com endpoint, using an exact hostname or strict suffix check
rather than includes. Preserve the existing required, parse-error, and
invalid-URL error behavior.
- Around line 249-262: Update the error-detail extraction in the Teams webhook
response handling to support both a string-valued `body.error` and the existing
nested `body.error.message` object shape, preserving `body.message` precedence
and including whichever diagnostic text is available in the thrown error.
- Around line 26-41: Update buildTitle so resource_alert events receive a
resource-specific title rather than the TTL label, while preserving the existing
TTL title for threshold_crossed events and the current resolved/state_changed
behavior.
In `@tests/alerts/teams.test.ts`:
- Around line 58-77: Extend the “Webhook URL validation” tests with a
`validateWebhookUrl` regression case through `sendTeamsAlert` using the
lookalike host `x.webhook.office.com.evil.com`, and assert it rejects without
calling `mockFetch`. Add an `alert_resolved` fixture to the payload-structure
tests and verify `buildAdaptiveCard` handles it without crashing.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a226aa7b-1b80-4986-9535-25660674d656
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
src/alerts/builtins.tssrc/alerts/teams.tstests/alerts/builtins.test.tstests/alerts/teams.test.ts
📜 Review details
🔇 Additional comments (5)
src/alerts/teams.ts (2)
1-10: LGTM!Also applies to: 197-248, 263-265
64-130: 🩺 Stability & AvailabilityNo change needed for
alert_resolvedcard rendering.alert_resolvedevents areTTLAlertEvent, and bothentryandthresholdfields are present in that shape and populated bybuildAlertEvent, so the generic fact branch is safe.> Likely an incorrect or invalid review comment.tests/alerts/teams.test.ts (1)
1-56: LGTM!Also applies to: 79-191, 193-209
src/alerts/builtins.ts (1)
67-78: LGTM!tests/alerts/builtins.test.ts (1)
10-10: LGTM!Also applies to: 32-34, 47-55, 70-70, 100-104, 121-123
| function buildTitle(event: AlertEvent): string { | ||
| const icon = severityEmoji(event); | ||
| const contractDisplay = event.contractName ?? event.contractId; | ||
|
|
||
| if (event.type === "alert_resolved") { | ||
| return `${icon} Alert Resolved — ${contractDisplay}`; | ||
| } | ||
|
|
||
| if (event.type === "state_changed") { | ||
| const diffLabel = event.diff.diffType.charAt(0).toUpperCase() + event.diff.diffType.slice(1); | ||
| return `${icon} State ${diffLabel} — ${contractDisplay}`; | ||
| } | ||
|
|
||
| const level = event.severity === "critical" ? "CRITICAL" : "Warning"; | ||
| return `${icon} TTL ${level} — ${contractDisplay}`; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
buildTitle mislabels resource_alert events as "TTL".
The default branch (Lines 39-40) is reached by both threshold_crossed and resource_alert events, always emitting TTL ${level}. A CPU/Memory resource alert (confirmed by the resource_alert test fixture in tests/alerts/teams.test.ts Lines 134-148, which has no entry/threshold fields) will render a title like ⚠️ TTL Warning — resource-contract, which is misleading for on-call responders triaging the alert.
🐛 Proposed fix
if (event.type === "state_changed") {
const diffLabel = event.diff.diffType.charAt(0).toUpperCase() + event.diff.diffType.slice(1);
return `${icon} State ${diffLabel} — ${contractDisplay}`;
}
+ if (event.type === "resource_alert") {
+ const resourceLabel = event.resource.type === "cpu" ? "CPU" : "Memory";
+ const level = event.severity === "critical" ? "CRITICAL" : "Warning";
+ return `${icon} ${resourceLabel} ${level} — ${contractDisplay}`;
+ }
+
const level = event.severity === "critical" ? "CRITICAL" : "Warning";
return `${icon} TTL ${level} — ${contractDisplay}`;📝 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.
| function buildTitle(event: AlertEvent): string { | |
| const icon = severityEmoji(event); | |
| const contractDisplay = event.contractName ?? event.contractId; | |
| if (event.type === "alert_resolved") { | |
| return `${icon} Alert Resolved — ${contractDisplay}`; | |
| } | |
| if (event.type === "state_changed") { | |
| const diffLabel = event.diff.diffType.charAt(0).toUpperCase() + event.diff.diffType.slice(1); | |
| return `${icon} State ${diffLabel} — ${contractDisplay}`; | |
| } | |
| const level = event.severity === "critical" ? "CRITICAL" : "Warning"; | |
| return `${icon} TTL ${level} — ${contractDisplay}`; | |
| } | |
| function buildTitle(event: AlertEvent): string { | |
| const icon = severityEmoji(event); | |
| const contractDisplay = event.contractName ?? event.contractId; | |
| if (event.type === "alert_resolved") { | |
| return `${icon} Alert Resolved — ${contractDisplay}`; | |
| } | |
| if (event.type === "state_changed") { | |
| const diffLabel = event.diff.diffType.charAt(0).toUpperCase() + event.diff.diffType.slice(1); | |
| return `${icon} State ${diffLabel} — ${contractDisplay}`; | |
| } | |
| if (event.type === "resource_alert") { | |
| const resourceLabel = event.resource.type === "cpu" ? "CPU" : "Memory"; | |
| const level = event.severity === "critical" ? "CRITICAL" : "Warning"; | |
| return `${icon} ${resourceLabel} ${level} — ${contractDisplay}`; | |
| } | |
| const level = event.severity === "critical" ? "CRITICAL" : "Warning"; | |
| return `${icon} TTL ${level} — ${contractDisplay}`; | |
| } |
🤖 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/alerts/teams.ts` around lines 26 - 41, Update buildTitle so
resource_alert events receive a resource-specific title rather than the TTL
label, while preserving the existing TTL title for threshold_crossed events and
the current resolved/state_changed behavior.
| function validateWebhookUrl(webhookUrl: string): void { | ||
| if (!webhookUrl) { | ||
| throw new Error( | ||
| "Teams webhook URL is required. " + | ||
| "Pass the full URL from your Microsoft Teams channel's Incoming Webhook settings.", | ||
| ); | ||
| } | ||
|
|
||
| let parsed: URL; | ||
| try { | ||
| parsed = new URL(webhookUrl); | ||
| } catch { | ||
| throw new Error( | ||
| `Invalid Teams webhook URL: "${webhookUrl}". ` + | ||
| "Expected a URL like https://<tenant>.webhook.office.com/webhookb2/...", | ||
| ); | ||
| } | ||
|
|
||
| if (!parsed.hostname.includes("webhook.office.com")) { | ||
| throw new Error( | ||
| `Invalid Teams webhook URL: "${webhookUrl}". ` + | ||
| "Expected a URL like https://<tenant>.webhook.office.com/webhookb2/...", | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Hostname validation is bypassable — substring match instead of suffix match; no scheme check.
parsed.hostname.includes("webhook.office.com") accepts any hostname containing that substring anywhere, e.g. x.webhook.office.com.attacker.com, which is not a real Microsoft Teams endpoint. The PR objective requires validating hostnames as genuine *.webhook.office.com endpoints; a substring check doesn't enforce that. There's also no check that the scheme is https:, despite the error message advertising https://....
🛡️ Proposed fix
- if (!parsed.hostname.includes("webhook.office.com")) {
+ const hostname = parsed.hostname.toLowerCase();
+ const isTeamsHost = hostname === "webhook.office.com" || hostname.endsWith(".webhook.office.com");
+ if (parsed.protocol !== "https:" || !isTeamsHost) {
throw new Error(
`Invalid Teams webhook URL: "${webhookUrl}". ` +
"Expected a URL like https://<tenant>.webhook.office.com/webhookb2/...",
);
}📝 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.
| function validateWebhookUrl(webhookUrl: string): void { | |
| if (!webhookUrl) { | |
| throw new Error( | |
| "Teams webhook URL is required. " + | |
| "Pass the full URL from your Microsoft Teams channel's Incoming Webhook settings.", | |
| ); | |
| } | |
| let parsed: URL; | |
| try { | |
| parsed = new URL(webhookUrl); | |
| } catch { | |
| throw new Error( | |
| `Invalid Teams webhook URL: "${webhookUrl}". ` + | |
| "Expected a URL like https://<tenant>.webhook.office.com/webhookb2/...", | |
| ); | |
| } | |
| if (!parsed.hostname.includes("webhook.office.com")) { | |
| throw new Error( | |
| `Invalid Teams webhook URL: "${webhookUrl}". ` + | |
| "Expected a URL like https://<tenant>.webhook.office.com/webhookb2/...", | |
| ); | |
| } | |
| } | |
| function validateWebhookUrl(webhookUrl: string): void { | |
| if (!webhookUrl) { | |
| throw new Error( | |
| "Teams webhook URL is required. " + | |
| "Pass the full URL from your Microsoft Teams channel's Incoming Webhook settings.", | |
| ); | |
| } | |
| let parsed: URL; | |
| try { | |
| parsed = new URL(webhookUrl); | |
| } catch { | |
| throw new Error( | |
| `Invalid Teams webhook URL: "${webhookUrl}". ` + | |
| "Expected a URL like https://<tenant>.webhook.office.com/webhookb2/...", | |
| ); | |
| } | |
| const hostname = parsed.hostname.toLowerCase(); | |
| const isTeamsHost = hostname === "webhook.office.com" || hostname.endsWith(".webhook.office.com"); | |
| if (parsed.protocol !== "https:" || !isTeamsHost) { | |
| throw new Error( | |
| `Invalid Teams webhook URL: "${webhookUrl}". ` + | |
| "Expected a URL like https://<tenant>.webhook.office.com/webhookb2/...", | |
| ); | |
| } | |
| } |
🤖 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/alerts/teams.ts` around lines 171 - 195, Update validateWebhookUrl to
require the parsed URL to use the https: scheme and to validate the hostname as
a genuine *.webhook.office.com endpoint, using an exact hostname or strict
suffix check rather than includes. Preserve the existing required, parse-error,
and invalid-URL error behavior.
| if (!response.ok) { | ||
| let detail = ""; | ||
| try { | ||
| const body = (await response.json()) as { message?: string; error?: { message?: string } }; | ||
| if (body.message) { | ||
| detail = `: ${body.message}`; | ||
| } else if (body.error?.message) { | ||
| detail = `: ${body.error.message}`; | ||
| } | ||
| } catch { | ||
| // body not JSON — ignore | ||
| } | ||
| throw new Error(`Teams webhook request failed: HTTP ${response.status}${detail}`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Error detail extraction misses the common { error: "<string>" } shape.
body.error?.message only handles a nested { error: { message } } object. Many webhook/API error bodies (including the test helper's { error: message } at tests/alerts/teams.test.ts Lines 41-46) use a plain string for error, so detail silently stays empty and useful diagnostic text is dropped from the thrown error.
🔧 Proposed fix
- const body = (await response.json()) as { message?: string; error?: { message?: string } };
+ const body = (await response.json()) as { message?: string; error?: string | { message?: string } };
if (body.message) {
detail = `: ${body.message}`;
+ } else if (typeof body.error === "string") {
+ detail = `: ${body.error}`;
} else if (body.error?.message) {
detail = `: ${body.error.message}`;
}📝 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.
| if (!response.ok) { | |
| let detail = ""; | |
| try { | |
| const body = (await response.json()) as { message?: string; error?: { message?: string } }; | |
| if (body.message) { | |
| detail = `: ${body.message}`; | |
| } else if (body.error?.message) { | |
| detail = `: ${body.error.message}`; | |
| } | |
| } catch { | |
| // body not JSON — ignore | |
| } | |
| throw new Error(`Teams webhook request failed: HTTP ${response.status}${detail}`); | |
| } | |
| if (!response.ok) { | |
| let detail = ""; | |
| try { | |
| const body = (await response.json()) as { message?: string; error?: string | { message?: string } }; | |
| if (body.message) { | |
| detail = `: ${body.message}`; | |
| } else if (typeof body.error === "string") { | |
| detail = `: ${body.error}`; | |
| } else if (body.error?.message) { | |
| detail = `: ${body.error.message}`; | |
| } | |
| } catch { | |
| // body not JSON — ignore | |
| } | |
| throw new Error(`Teams webhook request failed: HTTP ${response.status}${detail}`); | |
| } |
🤖 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/alerts/teams.ts` around lines 249 - 262, Update the error-detail
extraction in the Teams webhook response handling to support both a
string-valued `body.error` and the existing nested `body.error.message` object
shape, preserving `body.message` precedence and including whichever diagnostic
text is available in the thrown error.
| describe("Webhook URL validation", () => { | ||
| it("throws a clear error when URL is empty", async () => { | ||
| await expect(sendTeamsAlert("", makeAlertEvent())).rejects.toThrow(/Teams webhook URL is required/); | ||
| }); | ||
|
|
||
| it("throws when URL is not a valid URL string", async () => { | ||
| await expect(sendTeamsAlert("invalid-url", makeAlertEvent())).rejects.toThrow(/Invalid Teams webhook URL/); | ||
| }); | ||
|
|
||
| it("throws when hostname does not match Teams webhook domain", async () => { | ||
| await expect(sendTeamsAlert("https://discord.com/api/webhooks/123/abc", makeAlertEvent())).rejects.toThrow( | ||
| /Invalid Teams webhook URL/, | ||
| ); | ||
| }); | ||
|
|
||
| it("does not call fetch when URL validation fails", async () => { | ||
| await expect(sendTeamsAlert("https://example.com/hook", makeAlertEvent())).rejects.toThrow(); | ||
| expect(mockFetch).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a regression test for the hostname bypass and alert_resolved coverage.
Once validateWebhookUrl is tightened (see src/alerts/teams.ts Lines 171-195), add a case here asserting a lookalike host like https://x.webhook.office.com.evil.com/webhookb2/... is rejected. Also consider adding an alert_resolved fixture to the payload-structure suite to guard against the potential crash flagged in buildAdaptiveCard.
🤖 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 `@tests/alerts/teams.test.ts` around lines 58 - 77, Extend the “Webhook URL
validation” tests with a `validateWebhookUrl` regression case through
`sendTeamsAlert` using the lookalike host `x.webhook.office.com.evil.com`, and
assert it rejects without calling `mockFetch`. Add an `alert_resolved` fixture
to the payload-structure tests and verify `buildAdaptiveCard` handles it without
crashing.
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
… + completed) Adds src/alerts/teams.ts (Adaptive Card payload, severity coloring per event type) and its tests, per #311. Two things fixed before merging: - CodeRabbit correctly flagged validateWebhookUrl()'s hostname check: `hostname.includes("webhook.office.com")` accepts any hostname that merely contains that substring, e.g. an attacker-controlled "x.webhook.office.com.evil.com" would pass, silently sending real alert content (contract IDs, TTL data) to an attacker-controlled server. Changed to an exact/suffix match on the real hostname, plus an explicit https-only check. Added regression tests for both. - The PR never registered the channel in builtins.ts, so `--type teams` was unreachable from the CLI. Completed the registration (targetOption: "url", matching webhook/discord's pattern) and the matching builtins.test.ts coverage.
|
Merged as cc5f4c0 on
The Adaptive Card payload building and severity-color logic itself is well done. Verified locally: lint, typecheck, full suite (1146/1146), build, and audit all clean. Closing #311 as shipped. |
…rimmed to scope) Adds src/alerts/googlechat.ts and registers it in builtins.ts, per #313. Registration and the sender itself were already correct. Trimmed from the original PR before merging: a bundled Grafana/Prometheus observability stack (devops/grafana/*, devops/prometheus/*, docker-compose. observability.yml, docs/observability.md) - unrelated to this issue, shared branch lineage with several other open PRs (#594, #595, #596) that also carry the identical bundle. Left tests/docker/docker-compose.test.ts untouched by reverting to main's version. Updated tests/alerts/builtins.test.ts for the 10th channel (the PR's branch predated matrix/teams/email, so its own copy of this file didn't know about them). Note for a follow-up: src/alerts/discord.ts has the same hostname- validation weakness fixed in #557 (`hostname.includes("discord")`, even looser than Teams's check) - pre-existing, not part of this PR, flagging separately.
… + completed) Adds src/alerts/teams.ts (Adaptive Card payload, severity coloring per event type) and its tests, per #311. Two things fixed before merging: - CodeRabbit correctly flagged validateWebhookUrl()'s hostname check: `hostname.includes("webhook.office.com")` accepts any hostname that merely contains that substring, e.g. an attacker-controlled "x.webhook.office.com.evil.com" would pass, silently sending real alert content (contract IDs, TTL data) to an attacker-controlled server. Changed to an exact/suffix match on the real hostname, plus an explicit https-only check. Added regression tests for both. - The PR never registered the channel in builtins.ts, so `--type teams` was unreachable from the CLI. Completed the registration (targetOption: "url", matching webhook/discord's pattern) and the matching builtins.test.ts coverage.
…rimmed to scope) Adds src/alerts/googlechat.ts and registers it in builtins.ts, per #313. Registration and the sender itself were already correct. Trimmed from the original PR before merging: a bundled Grafana/Prometheus observability stack (devops/grafana/*, devops/prometheus/*, docker-compose. observability.yml, docs/observability.md) - unrelated to this issue, shared branch lineage with several other open PRs (#594, #595, #596) that also carry the identical bundle. Left tests/docker/docker-compose.test.ts untouched by reverting to main's version. Updated tests/alerts/builtins.test.ts for the 10th channel (the PR's branch predated matrix/teams/email, so its own copy of this file didn't know about them). Note for a follow-up: src/alerts/discord.ts has the same hostname- validation weakness fixed in #557 (`hostname.includes("discord")`, even looser than Teams's check) - pre-existing, not part of this PR, flagging separately.
2 participants
What does this PR do?
Adds a Microsoft Teams alert channel with Adaptive Card support, webhook validation, and customizable alert templates. It also registers the channel as a built-in alert target and includes comprehensive unit tests.
Closes #311
Why?
This enables alert notifications to be delivered directly to Microsoft Teams channels using incoming webhooks, providing rich, structured notifications for operational events while ensuring webhook security and consistent alert formatting.
Does this touch secret-key handling or transaction submission?
Checklist
npm test)npx tsc --noEmit)npm run lint)console.login core logicAdditional Notes
Changes Made
Teamsalert channel that conforms to theAlertChannelinterface.*.webhook.office.comendpoints.attention,warning,good)FactSetrenderAlertTemplate("teams", event).Verification
tests/alerts/teams.test.ts: 13/13 tests passedtests/alerts/builtins.test.ts: 16/16 tests passed