fix(zohomail): validate handshake signature before persisting secret - #616
fix(zohomail): validate handshake signature before persisting secret#616Rudra2637 wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Rudra2637
changed the title
📝 WalkthroughWalkthroughThe Zoho Mail handshake now loads the stored webhook secret before validation. It rejects unsigned or invalid secret replacements, persists secrets only after successful validation, and adds integration tests for failed setup and secret rotation. ChangesZoho webhook secret validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Zoho as Zoho webhook request
participant Handler as Zoho webhook handler
participant Store as Webhook secret store
participant Verifier as Signature verifier
Zoho->>Handler: Send x-hook-secret and optional signature
Handler->>Store: Load existing secret
Handler->>Verifier: Verify signature against stored or incoming secret
Handler->>Store: Persist secret after validation
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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/zohomail/webhooks/challenge.ts`:
- Around line 30-32: Update the webhook-secret initialization flow around
ctx.keys.get_webhook_signature so first-time setup uses an atomic
create-if-absent or compare-and-set write, preventing concurrent unsigned
requests from overwriting an established secret. Preserve normal reuse of an
existing secret and add coverage for concurrent initialization attempts.
🪄 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: 5502f93d-822e-47f4-b2c4-d776abd7584f
📒 Files selected for processing (2)
packages/zohomail/webhooks.integration.test.tspackages/zohomail/webhooks/challenge.ts
| let existingSecret: string | undefined; | ||
| try { | ||
| await ctx.keys.set_webhook_signature(hookSecret); | ||
| existingSecret = (await ctx.keys.get_webhook_signature()) ?? undefined; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the key-store contract and look for conditional-write support.
ast-grep outline packages/zohomail/webhooks/challenge.ts --items all
rg -n -C 12 '\b(get_webhook_signature|set_webhook_signature)\b' packages
rg -n -C 8 '\b(compareAndSet|compare_and_set|createIfAbsent|create_if_absent|transaction|serializ|mutex|lock|version)\b' packagesRepository: corsairdev/corsair
Length of output: 50375
Authorization Bypass (CWE-367): Time-of-check Time-of-use (TOCTOU) Race Condition
Reachability: External · Exploitability: Moderate
Make first-time webhook-secret setup atomic.
When two requests read an empty secret before either write, an unsigned request can overwrite the legitimate secret. Use a create-if-absent or compare-and-set write and add concurrent setup coverage.
🤖 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/zohomail/webhooks/challenge.ts` around lines 30 - 32, Update the
webhook-secret initialization flow around ctx.keys.get_webhook_signature so
first-time setup uses an atomic create-if-absent or compare-and-set write,
preventing concurrent unsigned requests from overwriting an established secret.
Preserve normal reuse of an existing secret and add coverage for concurrent
initialization attempts.
Greptile SummaryThe PR moves Zoho Mail handshake signature validation before secret persistence and adds coverage for rejected overwrites and authenticated rotation. However, the read-check-write sequence remains non-atomic, allowing concurrent first-time handshakes to bypass the new ownership requirement.
Confidence Score: 3/5The PR should not merge until concurrent first-time handshakes cannot overwrite an established secret without proving ownership. The new validation blocks sequential unauthorized overwrites, but two overlapping requests can both observe an absent secret and independently persist, allowing an unsigned later write to replace the legitimate value. Files Needing Attention: packages/zohomail/webhooks/challenge.ts
|
| Filename | Overview |
|---|---|
| packages/zohomail/webhooks/challenge.ts | Reorders validation before persistence and authenticates rotations, but uses a non-atomic read-check-write sequence that permits concurrent unsigned overwrite. |
| packages/zohomail/webhooks.integration.test.ts | Adds useful sequential overwrite and rotation coverage, but does not exercise concurrent first-time handshakes. |
Sequence Diagram
sequenceDiagram
participant L as Legitimate handshake
participant A as Unsigned attacker handshake
participant H as Zoho handler
participant K as Key storage
L->>H: New secret and valid signature
A->>H: Attacker secret, no signature
H->>K: Read existing secret
K-->>H: None
H->>K: Read existing secret
K-->>H: None
Note over H: Both requests authorize as first-time setup
L->>K: Persist legitimate secret
A->>K: Persist attacker secret
Note over K: Attacker secret overwrites legitimate secret
Reviews (1): Last reviewed commit: "fix(zohomail): validate handshake signat..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let existingSecret: string | undefined; | ||
| try { | ||
| await ctx.keys.set_webhook_signature(hookSecret); | ||
| existingSecret = (await ctx.keys.get_webhook_signature()) ?? undefined; |
There was a problem hiding this comment.
If two first-time handshakes for the same tenant overlap, both can observe that no secret exists and proceed without ownership verification, allowing a later unsigned write to replace the legitimate secret and break or take control of subsequent webhook authentication.
How this was verified: The handler performs independent read and write operations, while the key manager serializes only writes and the webhook path provides no per-tenant lock around the ownership decision.
Plugin PR scorecard —
|
| Check | Status | Notes |
|---|---|---|
| R1 — Scope: plugin files only | ✅ | |
| R2 — Tests with assertions | ✅ | |
| R3 — Description complete | ✅ | |
| R3 — Linked issue / claim | No "Fixes #…" or claim link — add one if this PR has a claim or issue | |
| R4 — Demo video / recording | ✅ |
Rules: PLUGIN_PR_RULES.md · re-runs on every push
|
Hey @Rudra2637, thanks for the contribution! 🏴☠️ Before a maintainer reviews, please fix the items below — the review re-runs automatically on your next push. Must fix
How this was verified: The handler performs independent read and write operations, while the key manager serializes only writes and the webhook path provides no per-tenant lock around the ownership decision. If anything remains after your next push, a bot commit will clean it up; a maintainer always does the final review and merge. |
1 participant
Description
Fixes the ZohoMail webhook handshake handler logic to prevent unauthorized or bare webhook secret overwrite attempts.
Specifically:
ctx.keys.set_webhook_signature(...).x-hook-secretif present, or persist directly if no signature is provided.Fixes #583
Checklist
Before submitting your PR, please verify the following:
pnpm lintand all checks passpnpm typecheckand there are no TypeScript errorspnpm buildand all packages build successfullypnpm testand all tests passScreenshots / Demos (if applicable)
Additional Notes
No breaking changes or new package dependencies introduced. All modifications are localized to
@corsair-dev/zohomail.Summary by CodeRabbit