fix(monday): verify webhook signatures and fail closed on missing secret - #610
Conversation
verifyMondayWebhookSignature returned { valid: true } for an empty
secret, matching the fail-open family already fixed in the Spotify, Zoom
and Slack verifiers (corsairdev#519, corsairdev#520, corsairdev#514).
Refs corsairdev#581
The verification block was commented out in all three event handlers, so itemCreated, statusChanged and columnValueChanged accepted any request that matched the event shape -- including forged ones -- and itemCreated went on to write to the database. Enable the existing check in each handler, returning 401 before any DB write. The challenge handler is intentionally left unverified: it is the subscription handshake Monday sends before a secret is exchanged, and it performs no writes. Refs corsairdev#581
Adds packages/monday/webhooks/webhooks.test.ts: the verifier's fail-closed paths, and per-handler assertions that a forged signature returns 401 and that itemCreated performs no upsert on rejection. Also maps corsair/core in the package's jest config, as cloudinary already does, so the handler modules resolve under ts-jest -- without it the suite cannot import a handler at all. Refs corsairdev#581
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMonday webhook handlers now verify Authorization JWTs before processing events. Invalid requests return HTTP 401. The verifier validates HS256 signatures, expiration, Bearer prefixes, and configured secrets. Tests cover verification and persistence behavior. ChangesMonday webhook security
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Monday
participant WebhookHandler
participant verifyMondayWebhookSignature
participant Persistence
Monday->>WebhookHandler: Submit webhook request
WebhookHandler->>verifyMondayWebhookSignature: Validate Authorization JWT
verifyMondayWebhookSignature-->>WebhookHandler: Return validation result
alt Signature is valid
WebhookHandler->>Persistence: Persist event
WebhookHandler-->>Monday: Return success
else Signature is invalid
WebhookHandler-->>Monday: Return HTTP 401
end
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR replaces the incorrect body-HMAC comparison with HS256 JWT verification and enables fail-closed authentication in the three Monday event handlers.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported raw-HMAC treatment of Monday JWTs has been replaced with signature verification over the JWT signing input. Important Files Changed
Sequence DiagramsequenceDiagram
participant Monday
participant Handler as Monday webhook handler
participant Verifier as JWT verifier
participant DB
Monday->>Handler: Event with Authorization JWT
Handler->>Verifier: Verify JWT using ctx.key
alt Missing secret or invalid JWT
Verifier-->>Handler: Invalid
Handler-->>Monday: 401
else Valid JWT
Verifier-->>Handler: Valid
Handler->>DB: Process or persist event
Handler-->>Monday: Success
end
Reviews (2): Last reviewed commit: "Merge branch 'main' into fix/monday-webh..." | Re-trigger Greptile |
|
@greptileai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@packages/monday/webhooks/types.ts`:
- Around line 221-223: Update the token extraction logic around authHeader in
types.ts and the corresponding tenant-matching logic in tenant-matcher.ts to
recognize the Bearer scheme case-insensitively, while preserving the existing
raw-token behavior for headers without that scheme.
🪄 Autofix
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: 23da8e1b-65eb-4733-a281-447b1f2be6fa
📒 Files selected for processing (2)
packages/monday/webhooks/types.tspackages/monday/webhooks/webhooks.test.ts
| const token = authHeader.startsWith('Bearer ') | ||
| ? authHeader.slice('Bearer '.length) | ||
| : authHeader; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target context ---'
cat -n packages/monday/webhooks/types.ts | sed -n '190,245p'
printf '%s\n' '--- related symbols and tests ---'
rg -n --glob '!node_modules' 'authHeader|Bearer |bearer |JWT|jwt|webhook' packages/monday | head -250Repository: corsairdev/corsair
Length of output: 10023
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- JWT verifier and surrounding types ---'
cat -n packages/monday/webhooks/types.ts | sed -n '115,230p'
printf '%s\n' '--- webhook authentication tests ---'
cat -n packages/monday/webhooks/webhooks.test.ts | sed -n '1,170p'
printf '%s\n' '--- tenant matcher implementation ---'
cat -n packages/monday/webhooks/tenant-matcher.ts | sed -n '1,90p'
printf '%s\n' '--- all callers of the two authentication helpers ---'
rg -n --glob '!node_modules' 'verifyMondayWebhookSignature|matchMondayTenantWebhook' packages/mondayRepository: corsairdev/corsair
Length of output: 13743
🌐 Web query:
RFC 9110 Authorization authentication scheme case insensitive Bearer
💡 Result:
Under RFC 9110, the authentication scheme identifier used in the HTTP Authorization header is case-insensitive [1][2]. Section 11.1 of RFC 9110 explicitly states that the authentication scheme is identified by a case-insensitive token [1][3]. Consequently, the "Bearer" authentication scheme is case-insensitive [2][4]. Implementations should treat "Bearer", "bearer", "BEARER", and any other casing variations as equivalent [5][4]. While "Bearer" is often used in documentation and examples, it is a matter of convention rather than a strict requirement for the protocol [2]. Many software projects have had to address interoperability issues where parsers incorrectly treated these schemes as case-sensitive; consistent with RFC 9110, these implementations are updated to perform case-insensitive comparisons to ensure compliance [6][5][4].
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc9110
- 2: case sensitivity of Bearer http authentication scheme oauth-wg/oauth-v2-1#166
- 3: https://datatracker.ietf.org/doc/html/rfc7235
- 4: Fix parsing of Authorization Bearer header owncast/owncast#3376
- 5: rails/rails@fdb35f0
- 6: Authorization Header's auth-scheme should be case-insensitive keycloak/keycloak#48387
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import re
jwt = "header.payload.signature"
inputs = [
f"Bearer {jwt}",
f"bearer {jwt}",
f"BEARER {jwt}",
f"Bearer {jwt}",
jwt,
]
def current(value):
return value[len("Bearer "):] if value.startswith("Bearer ") else value
def proposed(value):
return re.sub(r"^Bearer +", "", value, count=1, flags=re.I)
for value in inputs:
print(f"{value!r}")
print(f" current: {current(value)!r}")
print(f" proposed: {proposed(value)!r}")
PYRepository: corsairdev/corsair
Length of output: 725
Parse the Bearer scheme case-insensitively.
HTTP authentication schemes are case-insensitive. A bearer <JWT> header currently fails JWT validation and tenant matching. Apply the same case-insensitive extraction in packages/monday/webhooks/tenant-matcher.ts.
🤖 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 `@packages/monday/webhooks/types.ts` around lines 221 - 223, Update the token
extraction logic around authHeader in types.ts and the corresponding
tenant-matching logic in tenant-matcher.ts to recognize the Bearer scheme
case-insensitively, while preserving the existing raw-token behavior for headers
without that scheme.
ambikeesshh
left a comment
ambikeesshh
left a comment
There was a problem hiding this comment.
verifier was HMAC against body, monday board webhooks are HS256 JWT
fixed that + bearer casing + fail-closed
lgtm now. you can merge it @devjain32
3 participants
Description
Fixes #581.
Two related problems, as described in the issue:
1. The handlers never verified anything. In
item-created.ts,status-changed.tsandcolumn-value-changed.tsthe verification block was present but commented out, and execution fell straight through to the event logic — so any request matching the event shape was accepted, anditemCreatedwent on to write to the database.The block is now enabled in each handler, returning 401 before any DB write.
2.
verifyMondayWebhookSignaturefailed open on an empty secret.if (!secret) return { valid: true }→ now returnsMissing webhook secret, matching the string and shape already used by the Spotify verifier after the same family was fixed there (#519, #520, #514).challenge.tsis deliberately left aloneIt has its own matcher (
createMondayChallengeMatch) and is the subscription handshake Monday sends when the webhook is first registered — before any secret is exchanged — so verifying it would make webhook setup impossible. It performs no DB write and only echoes the challenge token back. Flagging it explicitly since "all handlers" could be read to include it.Tests
Adds
packages/monday/webhooks/webhooks.test.ts(9 tests, all passing):Verifier — missing secret →
Missing webhook secret; missingAuthorizationheader; missing raw body; correctly-signed request → valid.Handlers — for each of the three: a signature computed with a different key returns
success: false/statusCode: 401. ForitemCreatedspecifically:ctx.db.items.upsertByEntityIdis never calledsuccess: trueand the item is upserted with the expected payloadThat covers all four boxes in the issue, including "no DB write" on the reject path.
One extra line: the jest config
packages/monday/jest.config.cjsmapscorsair/httpbut notcorsair/core. The handlers importlogEventFromContextfromcorsair/core, so without that mapping the suite cannot import a handler at all — it dies withCannot find module 'corsair/core'before running a single assertion. I added the one mapping, copying it verbatim frompackages/cloudinary/jest.config.cjs, which is the plugin that already does handler-level webhook tests. Still insidepackages/monday/**, so R1 scope holds.Checklist
pnpm lintand all checks pass —biome checkclean on changed files; the repo's lint-staged hook ran clean on all three commitspnpm typecheckand there are no TypeScript errors —tsc --build packages/mondayexits 0pnpm buildand all packages build successfullypnpm testand all tests pass — green in CI; see note for local runsOn the test box. My suite passes 9/9.
packages/monday/api.test.tsfails 11/11 on a clean checkout ofmainas well (it builds an auth header from an unset API key), so this branch changes nothing there:Screenshots / Demos (if applicable)
No UI. Local JWT verification only. Evidence is the unit tests + issue:
#581
Additional Notes
packages/monday/**only (R1).Summary by CodeRabbit
Security
Bug Fixes
Tests