feat: add Slack /bacon slash command handler#23
Open
aryanghai12 wants to merge 4 commits intoOWASP-BLT:mainfrom
Open
feat: add Slack /bacon slash command handler#23aryanghai12 wants to merge 4 commits intoOWASP-BLT:mainfrom
aryanghai12 wants to merge 4 commits intoOWASP-BLT:mainfrom
Conversation
- Add src/slack_handler.py with handle_bacon_command() for POST /api/slack/bacon - Validate requests via HMAC-SHA256 Slack signature verification (replay-attack safe) - Parse both plain @username and rich <@uid|display> Slack mention formats - URL-decode form body with urllib.parse.unquote_plus (no CF-only JS dep) - Optional BACON_LEDGER KV persistence when namespace is bound - Route POST /api/slack/bacon in src/index.py - Document SLACK_SIGNING_SECRET / SLACK_BOT_TOKEN in wrangler.toml - Add 22-test suite in tests/test_slack_handler.py (all passing)
aryanghai12
changed the title
e-esakman
suggested changes
Feb 20, 2026
e-esakman
left a comment
e-esakman
left a comment
There was a problem hiding this comment.
have u tested it on ur slack workspace? please show working demo of it.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
Nachiket-Roy
suggested changes
Feb 20, 2026
Nachiket-Roy
left a comment
Nachiket-Roy
left a comment
There was a problem hiding this comment.
Also prefer using D1, and we should add a restriction so that only admins (or an approved role) can use this slash command rather than all workspace users.
…kPassing a plain Python dict as Response headers goes through pyodide as aPyProxy, which the JS Response constructor silently ignores, causing Slackto receive text/plain and render the raw JSON string in the channel.Fix: use to_js(..., dict_converter=Object.fromEntries) to produce a realJS plain object that the Response constructor correctly reads as headers.Also sets ADMIN_USER_IDS in wrangler.toml for admin bootstrap.
Contributor
Author
|
@DonnieBLT i tested it on my workspace and here is the working demo of the same feature. 2026-03-05.14-32-08.mp4 |
|
👋 Hi @aryanghai12! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you!
|
WalkthroughThis PR implements a complete Slack slash-command handler for Changes
Sequence Diagram(s)sequenceDiagram
participant Slack as Slack (Client)
participant Handler as Handler
participant Auth as Authorization
participant Parser as Parser
participant DB as D1 Database
participant Response as Slack Response
Slack->>Handler: POST /api/slack/bacon<br/>(signed request)
Handler->>Handler: Verify Slack signature
Handler->>Parser: Parse form data<br/>(text, user, channel)
Parser->>Handler: Parsed command & args
Handler->>Auth: Check admin status<br/>(bootstrap + DB)
Auth->>Handler: Authorization result
alt Admin Command (approve/remove/list)
Handler->>DB: Execute command<br/>(add/remove/list users)
DB->>Handler: Operation result
else Transfer Command
Handler->>Parser: Extract `@user` & amount
Parser->>Handler: User ID & amount
Handler->>DB: Insert transfer record
DB->>Handler: Confirmation
end
Handler->>Response: Build Slack message<br/>(ephemeral/in-channel)
Response->>Slack: Return formatted response
Slack->>Slack: Display message to user
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 the current code and only fix it if needed.
Inline comments:
In `@src/slack_handler.py`:
- Around line 184-187: The current db.prepare(...) call unconditionally sets
is_admin to 0 which strips D1-backed admin status on re-approve; change the
upsert to preserve an existing is_admin by either (a) querying approved_users
for the user's is_admin and passing that value into the INSERT, or (b) use an
UPSERT that on conflict updates user_name, added_by, added_at but leaves
is_admin unchanged (e.g. ON CONFLICT(user_id) DO UPDATE SET
user_name=excluded.user_name, added_by=excluded.added_by,
added_at=excluded.added_at). Update the db.prepare(...) call that inserts into
approved_users accordingly so re-approving does not overwrite is_admin.
- Around line 299-307: The handler currently skips signature verification when
signing_secret (from SLACK_SIGNING_SECRET) is unset, allowing forged requests;
update the request handling so if signing_secret is None/falsey the endpoint
fails closed instead of bypassing verification — e.g., call _slack_response with
a suitable error message and return make_response(json.dumps(payload), 500 or
401) immediately when signing_secret is missing (before using
_verify_slack_signature), and ensure the same response shape as signature
failures so callers cannot impersonate admins.
In `@tests/test_slack_handler.py`:
- Line 20: The test imports a non-existent symbol _parse_text from
slack_handler; update the import in tests/test_slack_handler.py to import the
actual helper _parse_transfer (or import _parse_transfer as _parse_text if you
prefer to keep the test name) and keep the existing _verify_slack_signature
import so tests can run; update the import line to reference _parse_transfer (or
alias it) to match the function defined in slack_handler.py.
In `@wrangler.toml`:
- Around line 17-24: The documented DB setup only runs migrations/0001_init.sql
but the code expects an approved_users.is_admin column (accessed in
src/slack_handler.py around the approval/read paths), so update the deployment
instructions to also run the second migration that adds the is_admin column
(e.g. migrations/0002_add_is_admin.sql) or explicitly note the ALTER TABLE step;
ensure the README/wrangler.toml comments instruct operators to run wrangler d1
execute ... --file=migrations/0002_add_is_admin.sql (or include the SQL to add
approved_users.is_admin) so the column exists before slack_handler.py performs
reads (line ~89) and inserts (line ~185).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 76f7af9a-b1f4-468c-8452-da935472027a
📒 Files selected for processing (6)
migrations/0001_init.sqlmigrations/0002_add_is_admin.sqlsrc/index.pysrc/slack_handler.pytests/test_slack_handler.pywrangler.toml
Comment on lines
+184
to
+187
| await db.prepare( | ||
| "INSERT OR REPLACE INTO approved_users (user_id, user_name, added_by, added_at, is_admin) " | ||
| "VALUES (?, ?, ?, ?, 0)" | ||
| ).bind(uid, display, sender_id, int(time.time())).run() |
There was a problem hiding this comment.
Preserve existing admin state on re-approve.
Line 186 hard-codes is_admin to 0 on the same path used for existing rows. Re-approving someone who already has a D1-backed admin role silently strips that role.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/slack_handler.py` around lines 184 - 187, The current db.prepare(...)
call unconditionally sets is_admin to 0 which strips D1-backed admin status on
re-approve; change the upsert to preserve an existing is_admin by either (a)
querying approved_users for the user's is_admin and passing that value into the
INSERT, or (b) use an UPSERT that on conflict updates user_name, added_by,
added_at but leaves is_admin unchanged (e.g. ON CONFLICT(user_id) DO UPDATE SET
user_name=excluded.user_name, added_by=excluded.added_by,
added_at=excluded.added_at). Update the db.prepare(...) call that inserts into
approved_users accordingly so re-approving does not overwrite is_admin.
Comment on lines
+299
to
+307
| signing_secret = getattr(env, "SLACK_SIGNING_SECRET", None) | ||
| if signing_secret: | ||
| # JS Headers.get() returns null (Python None) for missing headers, | ||
| # not "", so we must coerce explicitly. | ||
| timestamp = request.headers.get("X-Slack-Request-Timestamp") or "" | ||
| signature = request.headers.get("X-Slack-Signature") or "" | ||
| if not _verify_slack_signature(signing_secret, body_text, timestamp, signature): | ||
| payload = _slack_response(":x: Request signature verification failed.") | ||
| return make_response(json.dumps(payload), 401) |
There was a problem hiding this comment.
Fail closed when the signing secret is missing.
When SLACK_SIGNING_SECRET is unset, this branch skips verification entirely and the rest of the handler trusts the posted user_id. That makes the public endpoint forgeable: a caller can impersonate an admin and invoke approve, remove, or transfer commands.
🔒 Minimal fix
- signing_secret = getattr(env, "SLACK_SIGNING_SECRET", None)
- if signing_secret:
- # JS Headers.get() returns null (Python None) for missing headers,
- # not "", so we must coerce explicitly.
- timestamp = request.headers.get("X-Slack-Request-Timestamp") or ""
- signature = request.headers.get("X-Slack-Signature") or ""
- if not _verify_slack_signature(signing_secret, body_text, timestamp, signature):
- payload = _slack_response(":x: Request signature verification failed.")
- return make_response(json.dumps(payload), 401)
+ signing_secret = (getattr(env, "SLACK_SIGNING_SECRET", None) or "").strip()
+ if not signing_secret:
+ payload = _slack_response(":x: Slack signing secret is not configured.")
+ return make_response(json.dumps(payload), 500)
+
+ # JS Headers.get() returns null (Python None) for missing headers,
+ # not "", so we must coerce explicitly.
+ timestamp = request.headers.get("X-Slack-Request-Timestamp") or ""
+ signature = request.headers.get("X-Slack-Signature") or ""
+ if not _verify_slack_signature(signing_secret, body_text, timestamp, signature):
+ payload = _slack_response(":x: Request signature verification failed.")
+ return make_response(json.dumps(payload), 401)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/slack_handler.py` around lines 299 - 307, The handler currently skips
signature verification when signing_secret (from SLACK_SIGNING_SECRET) is unset,
allowing forged requests; update the request handling so if signing_secret is
None/falsey the endpoint fails closed instead of bypassing verification — e.g.,
call _slack_response with a suitable error message and return
make_response(json.dumps(payload), 500 or 401) immediately when signing_secret
is missing (before using _verify_slack_signature), and ensure the same response
shape as signature failures so callers cannot impersonate admins.
| # Make src/ importable without installing the package | ||
| sys.path.insert(0, str(Path(__file__).parent.parent / "src")) | ||
|
|
||
| from slack_handler import _parse_text, _verify_slack_signature # noqa: E402 |
There was a problem hiding this comment.
Import the parser name that actually exists.
src/slack_handler.py defines _parse_transfer, not _parse_text, so this module will fail before any tests execute. The smallest fix is to alias the real helper here.
🧪 Minimal fix
-from slack_handler import _parse_text, _verify_slack_signature # noqa: E402
+from slack_handler import _parse_transfer as _parse_text, _verify_slack_signature # noqa: E402📝 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
| from slack_handler import _parse_text, _verify_slack_signature # noqa: E402 | |
| from slack_handler import _parse_transfer as _parse_text, _verify_slack_signature # noqa: E402 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_slack_handler.py` at line 20, The test imports a non-existent
symbol _parse_text from slack_handler; update the import in
tests/test_slack_handler.py to import the actual helper _parse_transfer (or
import _parse_transfer as _parse_text if you prefer to keep the test name) and
keep the existing _verify_slack_signature import so tests can run; update the
import line to reference _parse_transfer (or alias it) to match the function
defined in slack_handler.py.
Comment on lines
+17
to
+24
| # --------------------------------------------------------------------------- | ||
| # D1 Database – stores BACON transfers and the approved-user allowlist. | ||
| # Steps: | ||
| # 1. wrangler d1 create blt-bacon-db (run once – note the id) | ||
| # 2. Replace "YOUR_DATABASE_ID" below with the printed database_id. | ||
| # 3. wrangler d1 execute blt-bacon-db --file=migrations/0001_init.sql | ||
| # 4. wrangler deploy | ||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
Run the second migration in the documented setup.
These steps only apply migrations/0001_init.sql, but src/slack_handler.py reads approved_users.is_admin on Line 89 and inserts it on Line 185. A fresh deployment following this block will ship without that column, so DB-backed authorization and /bacon approve break immediately.
📝 Suggested update
# 1. wrangler d1 create blt-bacon-db (run once – note the id)
# 2. Replace "YOUR_DATABASE_ID" below with the printed database_id.
# 3. wrangler d1 execute blt-bacon-db --file=migrations/0001_init.sql
-# 4. wrangler deploy
+# 4. wrangler d1 execute blt-bacon-db --file=migrations/0002_add_is_admin.sql
+# 5. wrangler deploy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wrangler.toml` around lines 17 - 24, The documented DB setup only runs
migrations/0001_init.sql but the code expects an approved_users.is_admin column
(accessed in src/slack_handler.py around the approval/read paths), so update the
deployment instructions to also run the second migration that adds the is_admin
column (e.g. migrations/0002_add_is_admin.sql) or explicitly note the ALTER
TABLE step; ensure the README/wrangler.toml comments instruct operators to run
wrangler d1 execute ... --file=migrations/0002_add_is_admin.sql (or include the
SQL to add approved_users.is_admin) so the column exists before slack_handler.py
performs reads (line ~89) and inserts (line ~185).
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
3 participants
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.
Closes #19
Implements the
/bacon @user <amount>Slack slash command requested in issue #19Changes
src/slack_handler.py(new)Core handler for the slash command:
SLACK_SIGNING_SECRETwith a ±5 minute replay-attack window@username 100and Slack rich-mention<@U12345|username> 100formats (regex-backed, handles decimals)urllib.parse.unquote_plus; no Cloudflare-JS–only deps in the parseable pathBACON_LEDGERKV namespace when bound inwrangler.tomlin_channelmessage so the whole channel sees the transfersrc/index.pyhandle_bacon_commandand routesPOST /api/slack/baconto itwrangler.tomlSLACK_SIGNING_SECRETandSLACK_BOT_TOKENmust be added viawrangler secret puttests/test_slack_handler.py(new)22 unit tests covering:
_parse_text: plain mentions, rich mentions, decimals, dots/hyphens in usernames, zero/large amounts, missing@, non-numeric amounts, empty input_verify_slack_signature: valid signature, wrong secret, tampered body, old/future timestamp, non-numeric timestamp, malformed signatureSlack App Setup
/bacon, set Request URL to: https://blt-rewards..workers.dev/api/slack/baconTest Results
Summary by CodeRabbit
New Features
/baconcommand enabling users to transfer rewards to other team members