feat: post audit requests directly to Slack via webhook helper#1765
Conversation
Replaces the manual /tmp copy-paste fallback in /request-audit with a direct post via incoming webhooks read from .env. Reuses the existing SlackNotifier (script/utils/slack-notifier.ts) behind a small generic CLI so any future skill can post to a Slack channel the same way. Convention: webhook_<channel-name>=<incoming-webhook-url> in .env. Helper exits 0 on success, 2 if the webhook env var is unset (skill falls back to the existing /tmp manual file for that channel only), and 1 on Slack/network error. Also fixes the channel names in the skill table: #lifi-external-sujith -> #dev-sc-audit #lifi-external-burrasec -> #dev-sc-audit-burrasec Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| const channel = get('--channel').replace(/^#/, '') | ||
| if (!/^[a-z0-9._-]+$/i.test(channel)) | ||
| throw new Error(`invalid channel name: ${channel}`) | ||
| return { channel, messageFile: get('--message-file') } |
There was a problem hiding this comment.
parseArgs() returns a user-controlled messageFile path with no validation; validate or restrict allowed paths (e.g., disallow absolute/parent paths or enforce a safe directory) before using it.
Details
✨ AI Reasoning
The script parses a --message-file argument and returns it from parseArgs(), exposing an unvalidated file path for later access. parseArgs() returns messageFile without any validation or normalization, enabling arbitrary file access when later used. This newly added code directly passes a user-controlled filename through to the file read logic without checks.
🔧 How do I fix it?
Implement server-side access control check(s) to verify that the currently authenticated user is authorized to access the specific object or resource ID being requested.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
Skill was carrying ~30% redundancy that cost tokens on every invocation without changing behavior. Each loaded line is read on every /request-audit run, so duplicate or stale guidance is pure overhead. Removed: - **Quality Checklist (~21 lines)** — every bullet restated a rule already given in Steps 1–6. The model just read those rules ~200 lines earlier; repeating them as a checklist doesn't improve compliance, it doubles the token cost of the same guidance. - **Historical note about legacy MCP path (~3 lines)** — told the model to ignore an error string (`mcp_externally_shared_channel_restricted`) it would never see, since the MCP path was removed in PR #1765. Pure noise with no actionable content. - **Standalone "Webhook posting note" callout between Step 3a and 3b (~3 lines)** — duplicated guidance already encoded in Step 6 step 1 ("drop the 🧵 suffix … incoming webhooks can't thread"). Removing the standalone callout improves locality: the rule now lives only where it is applied. - **"File output exception" callout in Step 3 (~5 lines)** — Step 6b already states the same rule ("plain text — Slack does not render backtick inline code from pasted content"). Replaced with a one-clause parenthetical pointing to Step 6b instead of a separate quote block. - **Two redundant rows in Error Handling table (~2 lines)** — the "Helper exits 0/1/2" rows duplicated the exit-code table already shown in Step 6. Replaced with a one-line pointer; kept the rows that are unique to Error Handling (PR not found, no commits, scope undetermined, invalid 1–4 input). - **Examples block in usage callout (~4 lines)** — line above already shows the canonical usage form; the three example invocations added no information. Compressed: - **"Slack ID note" warning** — 4 sentences of prose → 2. Kept the actionable claim (re-verify Burrasec ID before relying on it) and the reason (webhooks paste IDs verbatim, wrong ID renders as @unknown). Dropped meta-commentary about how the IDs were inherited. - **"Context (reason for audit)" preamble** — 3 paragraphs of prose + 5 numbered points → 1 sentence + 4-bullet list. Same rules, no prose loss; cut ~8 lines. What was deliberately NOT removed: - The worked PR #1715 example stays inline. It is the only place the skill calibrates prose density ("5–7 sentences" is abstract; the example shows what that actually looks like). Splitting it into a sibling file was tested and reverted — the model reads it anyway, so splitting just adds a Read round-trip without saving real tokens. - Step-by-step structure, exact templates with placeholders, exit-code → action table in Step 6, blocking-interaction guards ("Stop here. Wait for the user's reply"), and the `--urgent` flag rules. Net: 480 → 440 lines (-8%) with no operational rule lost. Skill ran PR #1715 successfully end-to-end in the same session that produced this trim, so the removed content was demonstrably not load-bearing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Summary
/tmpcopy-paste fallback in/request-auditwith a direct post via Slack incoming webhooks read from.env.script/utils/send-slack-webhook-message.tsthat wraps the existingSlackNotifier— any future skill posting via webhook reuses it.#dev-sc-auditand#dev-sc-audit-burrasec, not#lifi-external-*).Why
The two audit channels are Slack Connect channels — Slack's API blocks bot tokens from posting there (the original
slack_send_messageMCP path always failed withmcp_externally_shared_channel_restricted). Channel-bound incoming webhooks are not subject to that restriction, and we already havescript/utils/slack-notifier.tsfor posting to webhook URLs. Reusing it keeps the change tiny.How developers use it
Add to your
.env(URLs in 1Password — vaultEngineering, itemslack-webhooks):Run
/request-audit <PR>. At Step 4, confirm. The skill posts directly and prints success per channel. If a webhook isn't set, that channel falls back to the existing/tmp/audit-request-{pr}.mdflow (manual copy-paste).The convention is reusable: any channel
#Xis posted to viawebhook_Xin.env, and any caller canbun run script/utils/send-slack-webhook-message.ts --channel X --message-file ....Threading note (out of scope)
Plain incoming webhooks return only the literal
ok— nots— so we cannot post a parent and then reply in-thread. This PR posts a single combined message per channel (parent + blank line + reply, with the:thread:ornament dropped). Threading can come back later as a phase-2 PR if we install a Slack app withchat:writein the LI.FI Protocol workspace and switch tochat.postMessage.Files changed
script/utils/send-slack-webhook-message.ts— new, ~45 LOC..agents/commands/request-audit.md— Step 6 rewritten to call the helper, Step 6b demoted to per-channel error path, Setup section added, channel names corrected, Quality Checklist + Error Handling table updated. (.claude/skills/...and.cursor/commands/...are symlinks to this file.).env.example— two new lines documenting the webhook env vars.script/utils/slack-notifier.ts— unchanged, reused.Test plan
2, warn lists missing var, nothing postedevil channel!) → exit1, no postwebhook_audit-relay-test→ exit0, message visible#dev-sc-audit(coordinate with Sujith first, delete after) → exit0, message visible#dev-sc-audit-burrasec(coordinate with Josip first, delete after) → exit0, message visible/request-auditagainst a recent PR, both channels: skill prints success per channel, no/tmpfile written/tmp/audit-request-{pr}.mdand skill names the missing env varhttps://hooks.slack.com/services/INVALID/URL/x): helper retries 3× viaSlackNotifier.sendNotificationWithRetry, exits1, skill surfaces error, no fallback file🤖 Generated with Claude Code