fix(asana): reject webhook secret overwrite on configured accounts - #615
Conversation
|
@geekyvaishnavi is attempting to deploy a commit to the corsair Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe Asana webhook challenge now uses timing-safe secret comparison, protects stored secrets from overwrites, persists new secrets before success, and returns explicit error statuses. Tests cover registration, retries, conflicts, missing headers, persistence failures, and response behavior. ChangesAsana webhook security
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Asana
participant ChallengeHandler
participant SecretStorage
Asana->>ChallengeHandler: Send X-Hook-Secret
ChallengeHandler->>SecretStorage: Load stored secret
SecretStorage-->>ChallengeHandler: Return stored secret
ChallengeHandler->>ChallengeHandler: Compare secrets
ChallengeHandler->>SecretStorage: Persist first-time secret
ChallengeHandler-->>Asana: Return status and secret
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR protects configured Asana accounts from unauthenticated webhook-secret replacement and handles persistence failures before acknowledging a challenge.
Confidence Score: 4/5The PR should not merge until the outstanding concurrent first-registration race is resolved or explicitly accepted by maintainers. The overwrite guard closes the ordinary replacement path, but its separate read and write still allow overlapping public challenge requests to observe an empty value and race to install different account-level signing keys. Files Needing Attention: packages/asana/webhooks/challenge.ts Important Files Changed
Reviews (2): Last reviewed commit: "fix(asana): drop delete-path secret clea..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Plugin PR scorecard —
|
| Check | Status | Notes |
|---|---|---|
| R1 — Scope: plugin files only | ✅ | |
| R2 — Tests with assertions | ✅ | |
| R3 — Description complete | ✅ | |
| R3 — Linked issue / claim | ✅ | |
| R4 — Demo video / recording | ✅ |
Rules: PLUGIN_PR_RULES.md · re-runs on every push
|
Hey @geekyvaishnavi, thanks for the contribution! 🏴☠️ Before a maintainer reviews, please fix the items below — the review re-runs automatically on your next push. Must fix
Knowledge Base Used: The provider-plugin package pattern
How this was verified: The public handler performs separate asynchronous get and set operations, while the key manager provides no atomic compare-and-set across competing requests. If anything remains after your next push, a bot commit will clean it up; a maintainer always does the final review and merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/asana/endpoints/webhooks-management.ts`:
- Line 38: Update the post-delete cleanup flow around set_webhook_signature in
the webhook deletion handler so a storage failure after successful upstream
deletion is durably retryable or recoverable through a cleanup-only path.
Preserve the successful upstream deletion state, ensure a later challenge with a
new secret can recover without a stale secret causing 401, and add coverage for
storage failure after deletion.
In `@packages/asana/webhooks/challenge.ts`:
- Around line 40-63: Make the first-time registration flow in the webhook
challenge handler atomic by replacing the separate
get_webhook_signature/set_webhook_signature sequence with the storage layer’s
set-if-absent or compare-and-set operation. Treat registration as successful
only when no value existed or the existing value equals hookSecret; reject
conflicting concurrent values with the existing unauthorized response. Add a
test that interleaves two concurrent requests and verifies only the matching
secret is accepted and persisted.
🪄 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: e1dc98d5-597c-4b6b-96dd-6036bf354933
📒 Files selected for processing (3)
packages/asana/endpoints/webhooks-management.tspackages/asana/webhooks/challenge.test.tspackages/asana/webhooks/challenge.ts
| const storedSecret = await ctx.keys.get_webhook_signature(); | ||
|
|
||
| if (storedSecret) { | ||
| // A different value belongs to no handshake Corsair | ||
| // started, so refuse it and do not echo the sender's value back. | ||
| if (!secretsMatch(storedSecret, hookSecret)) { | ||
| return { | ||
| success: false, | ||
| statusCode: 401, | ||
| error: 'Webhook signing secret is already configured', | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| success: true, | ||
| responseHeaders: { | ||
| 'X-Hook-Secret': storedSecret, | ||
| }, | ||
| data: { hookSecret: storedSecret }, | ||
| }; | ||
| } | ||
|
|
||
| try { | ||
| await ctx.keys.set_webhook_signature(hookSecret); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether the key abstraction provides an atomic conditional-write API.
rg -n -C 5 '\b(get_webhook_signature|set_webhook_signature)\s*\(' packages
# Inspect key-store implementations for transactions, conditional writes, or locking.
rg -n -i -C 4 'webhook_signature|compare.?and.?set|set.?if.?absent|transaction|atomic|mutex' packagesRepository: corsairdev/corsair
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the concrete key-manager interface and implementation without scanning unrelated plugin code.
rg -n -S -g '*.ts' \
'set_webhook_signature|get_webhook_signature|createAccountKeyManager|AccountKeyManager' \
packages/corsair packages/cli packages/asana \
| head -200Repository: corsairdev/corsair
Length of output: 6729
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -S -g '*.ts' \
'set_webhook_signature|get_webhook_signature|createAccountKeyManager|AccountKeyManager' \
packages/corsair packages/cli packages/asana \
| head -200Repository: corsairdev/corsair
Length of output: 6729
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/corsair/core/auth/key-manager.ts
sed -n '1,40p;200,235p;260,330p;430,570p' packages/corsair/core/auth/key-manager.ts
printf '\n--- existing concurrent-write test ---\n'
sed -n '1,180p' packages/corsair/tests/key-manager-concurrent-writes.test.ts
printf '\n--- challenge implementation and tests ---\n'
sed -n '1,140p' packages/asana/webhooks/challenge.ts
sed -n '1,160p' packages/asana/webhooks/challenge.test.tsRepository: corsairdev/corsair
Length of output: 17507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Determine whether each request can receive a separate account key manager and
# whether the write chain is scoped only to one manager instance.
sed -n '450,510p' packages/corsair/core/client/index.ts
sed -n '285,315p;500,565p' packages/corsair/core/auth/key-manager.ts
rg -n -S -g '*.ts' 'challenge\.handler|webhooks.*challenge|handler:\s*challenge|keys:\s*accountKeyManager' packages/corsair packages/asanaRepository: corsairdev/corsair
Length of output: 5539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '450,510p' packages/corsair/core/client/index.ts
sed -n '285,315p;500,565p' packages/corsair/core/auth/key-manager.ts
rg -n -S -g '*.ts' \
'challenge\.handler|webhooks.*challenge|handler:\s*challenge|keys:\s*accountKeyManager' \
packages/corsair packages/asanaRepository: corsairdev/corsair
Length of output: 5539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check account caching and update behavior before finalizing the concurrency finding.
sed -n '330,455p' packages/corsair/core/auth/key-manager.ts
sed -n '1,120p' packages/corsair/core/auth/key-manager.tsRepository: corsairdev/corsair
Length of output: 6675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '330,455p' packages/corsair/core/auth/key-manager.ts
sed -n '1,120p' packages/corsair/core/auth/key-manager.tsRepository: corsairdev/corsair
Length of output: 6675
Other (CWE-362): Concurrent Execution using Shared Resource with Improper Synchronization ('Race Condition')
Reachability: External
Reachability path
● Entry
packages/asana/webhooks/challenge.test.ts
│
▼
● Sink
packages/asana/webhooks/challenge.ts
Make first-time secret registration atomic.
The per-manager write chain does not make the read-and-write operation conditional. Concurrent handlers can both observe no stored secret, then overwrite each other with different values. Use a storage-level set-if-absent or compare-and-set operation. Accept an existing value only when it matches hookSecret. Add an interleaved concurrent-request test.
🤖 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/asana/webhooks/challenge.ts` around lines 40 - 63, Make the
first-time registration flow in the webhook challenge handler atomic by
replacing the separate get_webhook_signature/set_webhook_signature sequence with
the storage layer’s set-if-absent or compare-and-set operation. Treat
registration as successful only when no value existed or the existing value
equals hookSecret; reject conflicting concurrent values with the existing
unauthorized response. Add a test that interleaves two concurrent requests and
verifies only the matching secret is accepted and persisted.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@geekyvaishnavi Please have a look into greptile reviews |
@Dhirenderchoudhary |
|
Remaining findings are being fixed by a bot commit — it will be re-reviewed automatically. |
4 participants
Description
Fixes #597.
The Asana webhook challenge handler persisted any
X-Hook-Secretheader it received, with no check for an already-configured secret. The webhook URL is public by necessity, so anyone able to reach it could POST their own secret, have it stored as the signing key viactx.keys.set_webhook_signature, and then forge signed events thattaskEventaccepts. It also silently broke the integration, since genuine Asana events continued to be signed with the original secret and started failing verification.webhooks/challenge.ts— the handler now reads the stored secret before writing:The comparison uses
crypto.timingSafeEqualrather than===, matching the existing precedent incore/auth/state.ts. Rejections are logged, since a blocked handshake is otherwise invisible to operators.Two smaller defects on the same path are fixed here as well:
set_webhook_signaturewas a floating promise, so a storage failure was swallowed and the handler still returnedsuccess: truewhile echoing a secret that was never saved; and the missing-header branch carried nostatusCode, defaulting to 500 for what is a client error.webhooks/challenge.test.ts— new, 8 tests covering match behavior, first-time registration, the overwrite attempt the issue asks for, an overwrite attempt at equal secret length (so the comparison is not merely a length check), handshake retry, missing header, and persistence failure. They fail against the previous handler and pass against this one.Known limitation (deliberate). Once a secret is configured, this handler never replaces it — including for a legitimate re-registration. A safe rotation path needs per-webhook secret storage: Corsair keeps one account-level
webhook_signature, while Asana issues one secret per webhook, so an account with multiple webhooks cannot be represented correctly today. That is a core storage change, out of scope for a plugin PR, and I've opened #NNN to track it. Blocking rotation is the safer failure mode in the meantime — the alternative is the unauthenticated overwrite this PR exists to close.An earlier revision of this PR cleared the stored secret in
webhookManagement.deleteto keep re-registration working. Greptile correctly flagged that it would wipe a secret shared by an account's other webhooks, so that change has been dropped andendpoints/webhooks-management.tsis now identical tomain.I deliberately did not tighten
matchto handshake-shaped requests only. It resembles a fix but is not one — an attacker can trivially send an empty body. The storage check is the actual defense.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
On the test checkbox.
pnpm testat the repo root cannot complete on a local machine: 51 plugins ship credential-gated live-API tests (api.test.ts), and turbo halts at the first one it reaches (@corsair-dev/airtable, which needsAIRTABLE_API_KEY). Scoped results for the package this PR touches:pnpm --filter @corsair-dev/asana test -- challenge.test.ts→ 8/8 passpackages/asana/api.test.ts→ 39 failures, pre-existing onmain, needsASANA_ACCESS_TOKENpnpm --filter @corsair-dev/asana typecheck→ cleanpnpm lint→ cleanpnpm run validate:plugins→ all plugins pass